Re: [PATCH] add-patch: handle "* Unmerged path" lines

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 9, 2023, 18:05 UTC
Message-ID: <xmqqcz5hby0p.fsf@gitster.g>
In-Reply-To: <ZAmfqC9WMl3XeyEr@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> So let's handle these lines as a noop. There's not really anything
> useful to do with a conflicted merge in this case, and that's what we do
> for other cases like "add -p". There we get a "diff --cc" line, which we
> accept as starting a new file, but we refuse to use any of its hunks
> (their headers start with "@@@" and not "@@ ", so we silently ignore
> them).
>
> It seems like simply recognizing the line and continuing in our parsing
> loop would work. But we actually need to run the rest of the loop body
> to handle matching up our colored/filtered output. But that code assumes
> that we have some active file_diff we're working on. So instead, we'll
> just insert a dummy entry into our array. This ends up the same as if we
> saw a "diff --cc" line (a file with no hunks).

OK.

Show 16 quoted lines

> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This patch just fixes the immediate bug. There's some possible future
> work:
>
>   - we could print a warning that the path is ignored. We don't do that
>     for "diff --cc" entries, either, though often those involve an index
>     refresh that will print "my-conflict: needs merge" or similar.
>
>     Doing so would require parsing the path name from the line. We don't
>     seem to quote it in any way, though. So a name like "foo\nbar" would
>     probably produce confusing output (though this patch would do the
>     right thing; we'd have a dummy entry for "foo", and then just
>     tack the useless "bar" line onto it). We should decide what the diff
>     side should produce before we start trying to parse it here.

We should write a name like "foo\ndiff --git a/foo b/foo" off as "if it hurts, don't do it" ;-).

Show 6 quoted lines

>   - arguably we could shrink the list to only non-conflicted entries
>     beforehand. That's what the "patch" menu item does if you run a full
>     add--interactive. But it would be slower (you have to run an extra
>     diff now). On the other hand, that is what the perl version did (and
>     it consistently printed "ignoring unmerged: foo", and then said "No
>     changes".

We already lost scripted version so it is not a solace that it worked correctly X-<. I do not know what to think that it took this long for people to hit this issue after 1fc18798 (Merge branch 'js/use-builtin-add-i', 2022-05-30). The work to reimplement "add -i" in C started at f83dff60 (Start to implement a built-in version of `git add --interactive`, 2019-11-13) and looking at the output of

    $ git log --format='%cI %h %s' --merges --grep="add-[ip]"

it seems that we have caught and fixed more bugs before we made it the default than after, and all the more recent fixes are on the smaller side, so I think we are in a good shape.

Show 5 quoted lines

>   - it's a little weird that the interactive-patch parser will complain
>     if the first line of the diff is garbage, but not if it sees garbage
>     later on. If we were more strict, that would have triggered the BUG()
>     rather than tacking the unknown line to the hunk (and we _should_ be
>     able to recognize arbitrary hunk lines by their "[-+ ]" prefixes).

There is recount_edited_hunk() but I am not sure if it can be relied upon (I've seen emacs's diff edit mode miscounting lines).

Another weird thing is that we do not complain if a patch does not have any hunk, but I guess we are lucky---that is what this fix takes advantage of ;-).

Show 51 quoted lines

>     But there may be corner cases I'm not thinking of, so I left it for
>     now.
>
>  add-patch.c                |  3 ++-
>  t/t3701-add-interactive.sh | 21 +++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index a86a92e1646..d7fc4f4cd21 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -483,7 +483,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  		if (!eol)
>  			eol = pend;
>  
> -		if (starts_with(p, "diff ")) {
> +		if (starts_with(p, "diff ") ||
> +		    starts_with(p, "* Unmerged path ")) {
>  			complete_file(marker, hunk);
>  			ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,
>  				   file_diff_alloc);
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 3a99837d9b1..e80e2b377c1 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1075,4 +1075,25 @@ test_expect_success 'show help from add--helper' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'reset -p with unmerged files' '
> +	test_when_finished "git checkout --force main" &&
> +	test_commit one conflict &&
> +	git checkout -B side HEAD^ &&
> +	test_commit two conflict &&
> +	test_must_fail git merge one &&
> +
> +	# this is a noop with only an unmerged entry
> +	git reset -p &&
> +
> +	# add files that sort before and after unmerged entry
> +	echo a >a &&
> +	echo z >z &&
> +	git add a z &&
> +
> +	# confirm that we can reset those files
> +	printf "%s\n" y y | git reset -p &&
> +	git diff-index --cached --diff-filter=u HEAD >staged &&
> +	test_must_be_empty staged
> +'
> +
>  test_done