From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Philippe Blain <levraiphilippeblain@gmail•com>,
git@vger•kernel.org,
Johannes Schindelin <johannes.schindelin@gmx•de>
Subject: Re: [PATCH] add-patch: handle "* Unmerged path" lines
Date: Thu, 09 Mar 2023 10:05:58 -0800 [thread overview]
Message-ID: <xmqqcz5hby0p.fsf@gitster.g> (raw)
In-Reply-To: <ZAmfqC9WMl3XeyEr@coredump.intra.peff.net> (Jeff King's message of "Thu, 9 Mar 2023 03:58:16 -0500")
Jeff King <peff@peff•net> 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@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" ;-).
> - 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
next prev parent reply other threads:[~2023-03-09 18:06 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-07 19:47 [ANNOUNCE] Git v2.40.0-rc2 Junio C Hamano
2023-03-08 20:51 ` 'BUG' in builtin add -p (was :Re: [ANNOUNCE] Git v2.40.0-rc2) Philippe Blain
2023-03-09 8:58 ` [PATCH] add-patch: handle "* Unmerged path" lines Jeff King
2023-03-09 18:05 ` Junio C Hamano [this message]
2023-03-10 9:29 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqcz5hby0p.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=levraiphilippeblain@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox