Skip to content

Implement the git add --patch backend in C #173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 19 commits into from

Conversation

dscho
Copy link
Member

@dscho dscho commented Mar 28, 2019

Out of all the patch series on the journey to provide git add --interactive and git add --patch in built-in versions, this is the big one, as can be expected from the fact that the git add --patch functionality makes up over half of the 1,800+ lines of git-add--interactive.perl.

The two patches that stick out are of course the ones to implement hunk splitting and hunk editing: these operations are fundamentally more complicated, and less obvious, than the entire rest of the operations.

@dscho dscho force-pushed the add-p-in-c branch 5 times, most recently from a3aa9dd to 158d5ad Compare March 29, 2019 20:01
@dscho
Copy link
Member Author

dscho commented Mar 29, 2019

/AzurePipelines help

@azure-pipelines
Copy link

Supported commands
     help:
          Get descriptions, examples and documentation about supported commands
          Example: help "command_name"
     run:
          Run all pipelines or a specific pipeline for this repository using a comment. Use
          this command by itself to trigger all related pipelines, or specify a pipeline
          to run.
          Example: "run" or "run pipeline_name"

See additional documentation.

@dscho dscho force-pushed the add-p-in-c branch 2 times, most recently from f009c8b to a42c37b Compare April 3, 2019 20:28
@dscho dscho changed the base branch from master to js/builtin-add-i August 26, 2019 08:45
@dscho dscho force-pushed the add-p-in-c branch 2 times, most recently from 83dd10a to 822c5eb Compare August 26, 2019 13:09
@dscho dscho force-pushed the js/builtin-add-i branch from f3665cf to cf308ea Compare August 30, 2019 23:13
@dscho dscho force-pushed the js/builtin-add-i branch 2 times, most recently from b8f458c to 2b5d5c1 Compare November 14, 2019 03:27
dscho added a commit to dscho/git that referenced this pull request Nov 14, 2019
Phillip Wood pointed out, long ago, that `temp` -> `merged` would make
it easier to read this code. Somehow, this change only made it into
gitgitgadget#173 but not into Git for
Windows' `master`.

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit to dscho/git that referenced this pull request Nov 14, 2019
Phillip Wood pointed out, long ago, that the code is underdocumented and
could use a helper for calling the editor. Somehow, this change only
made it into gitgitgadget#173 but not into
Git for Windows' `master`.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the add-p-in-c branch 2 times, most recently from dc2703d to e232f4a Compare November 25, 2019 15:22
If this developer's workflow is any indication, then this is *the* most
useful feature of Git's interactive `add `command.

Note: once again, this is not a verbatim conversion from the Perl code
to C: the `hunk_splittable()` function, for example, essentially did all
the work of splitting the hunk, just to find out whether more than one
hunk would have been the result (and then tossed that result into the
trash). In C we instead count the number of resulting hunks (without
actually doing the work of splitting, but just counting the transitions
from non-context lines to context lines), and store that information
with the hunk, and we do that *while* parsing the diff in the first
place.

Another deviation: the built-in `git add -p` was designed with a single
strbuf holding the diff (and another one holding the colored diff, if
that one was asked for) in mind, and hunks essentially store just the
start and end offsets pointing into that strbuf. As a consequence, when
we split hunks, we now use a special mode where the hunk header is
generated dynamically, and only the rest of the hunk is stored using
such start/end offsets. This way, we also avoid the frequent
formatting/re-parsing of the hunk header of the Perl version.

Signed-off-by: Johannes Schindelin <[email protected]>
This is considered "the right thing to do", according to 933e44d
("add -p": work-around an old laziness that does not coalesce hunks,
2011-04-06).

Note: we cannot simply modify the hunks while merging them; Once we
implement hunk editing, we will call `reassemble_patch()` whenever a
hunk is edited, therefore we must not modify the hunks (because the user
might e.g. hit `K` and change their mind whether to stage the previous
hunk).

Signed-off-by: Johannes Schindelin <[email protected]>
This helper supports the scenario where Git has a populated `strbuf` and
wants to let the user edit it interactively.

In `git add -p`, we will use this to allow interactive hunk editing: the
diff hunks are already in memory, but we need to write them out to a
file so that an editor can be launched, then read everything back once
the user is done editing.

Signed-off-by: Johannes Schindelin <[email protected]>
Just like `git add --edit` allows the user to edit the diff before it is
being applied to the index, this feature allows the user to edit the
diff *hunk*.

Naturally, it gets a bit more complicated here because the result has
to play well with the remaining hunks of the overall diff. Therefore,
we have to do a loop in which we let the user edit the hunk, then test
whether the result would work, and if not, drop the edits and let the
user decide whether to try editing the hunk again.

Note: in contrast to the Perl version, we use the same diff
"coalescing" (i.e. merging overlapping hunks into a single one) also for
the check after editing, and we introduce a new flag for that purpose
that asks the `reassemble_patch()` function to pretend that all hunks
were selected for use.

This allows us to continue to run `git apply` *without* the
`--allow-overlap` option (unlike the Perl version), and it also fixes
two known breakages in `t3701-add-interactive.sh` (which we cannot mark
as resolved so far because the Perl script version is still the default
and continues to have those breakages).

Signed-off-by: Johannes Schindelin <[email protected]>
With this patch, it is now possible to see a summary of the available
hunks and to navigate between them (by number).

A test is added to verify that this behavior matches the one of the Perl
version of `git add -p`.

Signed-off-by: Johannes Schindelin <[email protected]>
This patch implements the hunk searching feature in the C version of
`git add -p`.

A test is added to verify that this behavior matches the one of the Perl
version of `git add -p`.

Note that this involves a change of behavior: the Perl version uses (of
course) the Perl flavor of regular expressions, while this patch uses
the regcomp()/regexec(), i.e. POSIX extended regular expressions. In
practice, this behavior change is unlikely to matter.

Signed-off-by: Johannes Schindelin <[email protected]>
This command is actually very similar to the 'd' ("do not stage this
hunk or any of the later hunks in the file") command: it just does
something on top, namely leave the loop and return a value indicating
that we're quittin'.

Signed-off-by: Johannes Schindelin <[email protected]>
When displaying the only hunk in a file's diff, the prompt already
excludes the commands to navigate to the previous/next hunk.

Let's also let the `?` command show only the help lines corresponding to
the commands that are displayed in the prompt.

Signed-off-by: Johannes Schindelin <[email protected]>
This patch will make `git add -p` show "No changes." or "Only binary
files changed." in that case.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member Author

dscho commented Dec 13, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 13, 2019

Submitted as [email protected]

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 13, 2019

This patch series was integrated into pu via git@c66c859.

@gitgitgadget gitgitgadget bot added the pu label Dec 13, 2019
@dscho

This comment has been minimized.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 16, 2019

This branch is now known as js/add-p-in-c.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 16, 2019

This patch series was integrated into pu via git@a88962f.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 17, 2019

This patch series was integrated into pu via git@3f68c51.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 18, 2019

This patch series was integrated into pu via git@537873b.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 19, 2019

This patch series was integrated into pu via git@993e3e8.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 19, 2019

This patch series was integrated into next via git@c0deed9.

@gitgitgadget gitgitgadget bot added the next label Dec 19, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Dec 20, 2019

This patch series was integrated into pu via git@c4e929b.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 25, 2019

This patch series was integrated into pu via git@45b96a6.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 25, 2019

This patch series was integrated into next via git@45b96a6.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 25, 2019

This patch series was integrated into master via git@45b96a6.

@gitgitgadget gitgitgadget bot added the master label Dec 25, 2019
@gitgitgadget gitgitgadget bot closed this Dec 25, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Dec 25, 2019

Closed via 45b96a6.

@dscho dscho deleted the add-p-in-c branch December 26, 2019 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant