git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jan 22, 2022, 09:05 UTC
Message-ID
<nycvar.QRO.7.76.6.2201220938360.2121@tvgsbejvaqbjf.bet>
In-Reply-To
<47e7b23a-56a3-b533-63ed-a854503b59ed@gmail.com>
Hi Phillip,

first of all: thank you for these patches. I read over them and they have my ACK.

On Wed, 19 Jan 2022, Phillip Wood wrote:
Show 28 quoted lines
> On 12/01/2022 18:51, Junio C Hamano wrote:
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> > > I'm not sure I want to go with your extra changes. I've left some
> > > comments on them below
> > >
> > > > @@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const
> > > > struct pathspec *ps)
> > > >    else if (starts_with(p, "@@ ") ||
> > > >      (hunk == &file_diff->head &&
> > > >   		  (skip_prefix(p, "deleted file", &deleted)))) {
> > > > -		if (marker == '-' || marker == '+')
> > > > -			/*
> > > > -			 * Should not happen; previous hunk did not end
> > > > -			 * in a context line? Handle it anyway.
> > > > -			 */
> > > > +			hunk->splittable_into++;
> > > > +		/*
> > > > +		 * Should not increment "splittable_into";
> > > > +		 * previous hunk did not end in a context
> > > > +		 * line? Handle it anyway.
> > > > +		 */
> > > > +		complete_file(marker, &hunk->splittable_into);
> > > >      ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
> > > >         file_diff->hunk_alloc);
> > >
> > > I deliberately left this alone as I think we should probably make
> > > this BUG() out instead of silently accepting an invalid diff.

FWIW this was overzealous defensive programming on my part. More on that below.

> > As we are reading our own output, I agree that such a data error is
> > a BUG().

Indeed. I was less worried about the output format changing, and more concerned with bugs in my parser ;-)

Although, having said that, I had meant to verify that `git add -p` cannot be asked to produce and consume diffs with `-U0` when I wrote that comment. Now I did that, and I am now confident that there is no way to ask `git add -p` to generate and use context line-free diffs: we neither add `-U<n>` in https://github.com/git/git/blob/v2.34.1/add-patch.c#L398-L417 nor do we call the user-facing `git diff` command that would interpret `diff.context`, but instead we use `git diff-index` and `git diff-files` (which ignore that config setting).

Show 6 quoted lines
> > In any case, a helper to see if the file ended without post-context
> > is one thing, and a helper that specify what happens after we are
> > done with a single file, before we move on top the next file or
> > after processing the last file, is another thing.  The latter may be
> > able to make use of the former, but the latter may want to do more
> > than that in the future.

If you are concerned about the name of the function: maybe a better name would be `maybe_increment_splittable_hunk_count(marker)`.

Show 18 quoted lines
> >
> > As complete_file() is about finalizing the processing we have done
> > to the current file, it should be used for that purpose, and nothing
> > else, I think the hunk I see at
> > https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a
> > (reproduced below) is simply a nonsense.
> >
> > Stepping back a bit, though, is this helper really finalizing the
> > current file, or is it finalizing the current hunk?  If it were the
> > latter, then its use in the hunk I called "nonsense" above actually
> > makes perfect sense.
>
> Even if the helper is finalizing the current hunk then I think that "nonsense"
> hunk would still wrong as it would be calling finalize_hunk() on _every_
> context line in the hunk rather than just being called once to finalize the
> hunk. We could call the function something like update_splittable() but then
> we'd need to explain why we were calling that function at the start of a diff
> and at the end of the loop.

Right. The point of this check is to see whether we missed counting a splittable hunk. Then it makes more sense to call it at the beginning of a file, at the end of a file _and_ at a context line.

Having said all that, I am really fine with what landed in `next`.

Thank you, Dscho

Previous: Junio C HamanoNext: Phillip Wood
Message 19 of 21 in “builtin add -p: fix hunk splitting”
  1. 0/2 builtin add -p: fix hunk splittingPhillip Wood via GitGitGadget, Dec 20, 2021
  2. 1/2 t3701: clean up hunk splitting testsPhillip Wood via GitGitGadget, Dec 20, 2021
  3. Junio C HamanoDec 20, 2021
  4. 2/2 builtin add -p: fix hunk splittingPhillip Wood via GitGitGadget, Dec 20, 2021
  5. Ævar Arnfjörð BjarmasonDec 20, 2021
  6. Phillip WoodJan 11, 2022
  7. Ævar Arnfjörð BjarmasonJan 11, 2022
  8. Junio C HamanoDec 20, 2021
  9. 0/2 builtin add -p: fix hunk splittingPhillip Wood via GitGitGadget, Jan 11, 2022
  10. 1/2 t3701: clean up hunk splitting testsPhillip Wood via GitGitGadget, Jan 11, 2022
  11. 2/2 builtin add -p: fix hunk splittingPhillip Wood via GitGitGadget, Jan 11, 2022
  12. Ævar Arnfjörð BjarmasonJan 11, 2022
  13. Phillip WoodJan 11, 2022
  14. Junio C HamanoJan 12, 2022
  15. Phillip WoodJan 19, 2022
  16. Junio C HamanoJan 20, 2022
  17. Ævar Arnfjörð BjarmasonJan 20, 2022
  18. Junio C HamanoJan 20, 2022
  19. Johannes SchindelinJan 22, 2022
  20. Phillip WoodJan 24, 2022
  21. Junio C HamanoJan 12, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.