Jeff King <peff@xxxxxxxx> writes:
> 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.
> Reported-by: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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" ;-).
> - 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.
> - 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 ;-).
> 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
