threads / discuss / 55999

git add --interactive patch improvement for split hunks

Subject: git add --interactive patch improvement for split hunks

## tl;dr

9 messages between Jun 24, 2021 and Jun 30, 2021.

replies: 8people: 4as markdown or json

Ulrich Windl· Jun 24, 2021, 10:35 UTC · lore
Hi!

I noticed that git add -interactive's patch displays the function context for the diffs, but that function context is lost when the hunks are split. It would help the user (especially for hunks covering multiple functioins) if function context were still provided for split hunks.

Maybe just consider this (buggy) example (with screen shot in case the lines get severely mangled): (15/20) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]? n

@@ -1097,13 +1743,13 @@ static inline    void    bitmap_copy_bits(
     }
     else
     {
-        const unsigned        s_i    = FASTBIT(s_pos);
-        const unsigned        d_i    = FASTBIT(d_pos);
+        const unsigned        s_i    = FASTBIT(s_pos + count);
+        const unsigned        d_i    = FASTBIT(d_pos + count);
         const fastword_t    *s_fw_p;
         fastword_t        *d_fw_p;
 
-        for ( s_fw_p = src->fast_words + FASTWORD(s_pos),
-              d_fw_p = dst->fast_words + FASTWORD(d_pos);
+        for ( s_fw_p = src->fast_words + FASTWORD(s_pos + count),
+              d_fw_p = dst->fast_words + FASTWORD(d_pos + count);
               count >= FASTWORD_BITS;
               --s_fw_p, --d_fw_p, count -= FASTWORD_BITS )
         {
(16/20) Stage this hunk [y,n,q,a,d,K,j,J,g,/,s,e,?]? s
Split into 2 hunks.
@@ -1097,8 +1743,8 @@
     }
     else
     {
-        const unsigned        s_i    = FASTBIT(s_pos);
-        const unsigned        d_i    = FASTBIT(d_pos);
+        const unsigned        s_i    = FASTBIT(s_pos + count);
+        const unsigned        d_i    = FASTBIT(d_pos + count);
         const fastword_t    *s_fw_p;
         fastword_t        *d_fw_p;
 
(16/21) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]? n
@@ -1102,8 +1748,8 @@
         const fastword_t    *s_fw_p;
         fastword_t        *d_fw_p;
 
-        for ( s_fw_p = src->fast_words + FASTWORD(s_pos),
-              d_fw_p = dst->fast_words + FASTWORD(d_pos);
+        for ( s_fw_p = src->fast_words + FASTWORD(s_pos + count),
+              d_fw_p = dst->fast_words + FASTWORD(d_pos + count);
               count >= FASTWORD_BITS;
               --s_fw_p, --d_fw_p, count -= FASTWORD_BITS )
         {
(17/21) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]?
Jeff King· Jun 24, 2021, 15:41 UTC · re: Ulrich Windl · lore

Re: git add --interactive patch improvement for split hunks

On Thu, Jun 24, 2021 at 12:35:16PM +0200, Ulrich Windl wrote:
Show 6 quoted lines
> I noticed that git add -interactive's patch displays the function
> context for the diffs, but that function context is lost when the
> hunks are split.
>
> It would help the user (especially for hunks covering multiple
> functioins) if function context were still provided for split hunks.

This was discussed a while ago (and there is even a patch) in this thread:

  https://lore.kernel.org/git/20201117020522.GD19433@coredump.intra.peff.net/

The short of it is that the upcoming builtin-in-C version of the code will preserve the function header when splitting. The patch in that message adds it to the existing perl version, but I didn't really bother moving it forward, since that code is all supposed to eventually go away[0].

One thing you may not like, though: both the builtin version and that patch only put the funcname context in the _first_ hunk of the split. Doing it for subsequent hunks is much trickier, since there can be a funcname in the split context itself. E.g.:

  @@ ... @@ void foo()
           int x;
  -        int y = 1;
  +        int y = 2;
   
  -        x = 3;
  +        x = 4;
   }
could split into two hunks, both annotated with "void foo()". But:
  @@ ... @@ void foo()
           int x;
  -        x = 3;
  +        x = 4;
   }
   void bar()
   {
  -        int y = 1;
  +        int y = 2;
   }

would be wrong to say "void foo()" for the second hunk. We'd have to re-scan the interior context lines for a funcname to find it. That's all-but-impossible in the perl version, but might be do-able in the C version (since it has easy access to the funcname-matching patterns and machinery).

-Peff
[0] I'm not sure what the timetable is for switching to the C version of
    add--interactive. If it's going to be a while, I don't mind moving
    forward the other patch I showed. But maybe the time is here to
    think about switching the default of add.interactive.useBuiltin, and
    ironing out any final bugs?
Ulrich Windl· Jun 28, 2021, 10:10 UTC · re: Jeff King · lore

Antw: [EXT] Re: git add --interactive patch improvement for split hunks

>>> Jeff King <peff@peff.net> schrieb am 24.06.2021 um 17:41 in Nachricht
<YNSnlhbE30xDfVMY@coredump.intra.peff.net>:
[...]
Show 32 quoted lines
> One thing you may not like, though: both the builtin version and that
> patch only put the funcname context in the _first_ hunk of the split.
> Doing it for subsequent hunks is much trickier, since there can be a
> funcname in the split context itself. E.g.:
> 
>   @@ ... @@ void foo()
>            int x;
>   -        int y = 1;
>   +        int y = 2;
>    
>   -        x = 3;
>   +        x = 4;
>    }
> 
> could split into two hunks, both annotated with "void foo()". But:
> 
>   @@ ... @@ void foo()
>            int x;
>   -        x = 3;
>   +        x = 4;
>    }
>    void bar()
>    {
>   -        int y = 1;
>   +        int y = 2;
>    }
> 
> would be wrong to say "void foo()" for the second hunk. We'd have to
> re-scan the interior context lines for a funcname to find it. That's
> all-but-impossible in the perl version, but might be do-able in the C
> version (since it has easy access to the funcname-matching patterns and
> machinery).

There always was a related bug (IMHO) that showed the context of the previous function even though the actual change was within a new function (that starts within the context lines). So if that bug were fixed, my guess is that the other would be as well. However I don't know how easy or hard the fix will be. Maybe the "definition" of function context is just different; I don't really know.

Show 8 quoted lines
> 
> -Peff
> 
> [0] I'm not sure what the timetable is for switching to the C version of
>     add--interactive. If it's going to be a while, I don't mind moving
>     forward the other patch I showed. But maybe the time is here to
>     think about switching the default of add.interactive.useBuiltin, and
>     ironing out any final bugs?
Ævar Arnfjörð Bjarmason· Jun 28, 2021, 11:20 UTC · re: Ulrich Windl · lore

Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks

On Mon, Jun 28 2021, Ulrich Windl wrote:
Show 43 quoted lines
>>>> Jeff King <peff@peff.net> schrieb am 24.06.2021 um 17:41 in Nachricht
> <YNSnlhbE30xDfVMY@coredump.intra.peff.net>:
>
> [...]
>> One thing you may not like, though: both the builtin version and that
>> patch only put the funcname context in the _first_ hunk of the split.
>> Doing it for subsequent hunks is much trickier, since there can be a
>> funcname in the split context itself. E.g.:
>> 
>>   @@ ... @@ void foo()
>>            int x;
>>   -        int y = 1;
>>   +        int y = 2;
>>    
>>   -        x = 3;
>>   +        x = 4;
>>    }
>> 
>> could split into two hunks, both annotated with "void foo()". But:
>> 
>>   @@ ... @@ void foo()
>>            int x;
>>   -        x = 3;
>>   +        x = 4;
>>    }
>>    void bar()
>>    {
>>   -        int y = 1;
>>   +        int y = 2;
>>    }
>> 
>> would be wrong to say "void foo()" for the second hunk. We'd have to
>> re-scan the interior context lines for a funcname to find it. That's
>> all-but-impossible in the perl version, but might be do-able in the C
>> version (since it has easy access to the funcname-matching patterns and
>> machinery).
>
> There always was a related bug (IMHO) that showed the context of the
> previous function even though the actual change was within a new
> function (that starts within the context lines). So if that bug were
> fixed, my guess is that the other would be as well.
> However I don't know how easy or hard the fix will be.
> Maybe the "definition" of function context is just different; I don't really know.

Does that bug perhaps have anything to do with: https://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/

I have some planned fixes to that behavior, but it's currently blocked on a combination of myself having a lot of outstanding patches, and that linked patch needing another series (as of yet unsubmitted/un-re-rolled) to get us proper testing in this area of git. I.e. our testing of what function context we should find is really lacking.

Jeff King· Jun 30, 2021, 02:16 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks

On Mon, Jun 28, 2021 at 01:20:46PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 9 quoted lines
> > There always was a related bug (IMHO) that showed the context of the
> > previous function even though the actual change was within a new
> > function (that starts within the context lines). So if that bug were
> > fixed, my guess is that the other would be as well.
> > However I don't know how easy or hard the fix will be.
> > Maybe the "definition" of function context is just different; I don't really know.
> 
> Does that bug perhaps have anything to do with:
> https://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/

I think it's similar. The issue is that we search backwards for a funcname match from the top of the hunk, _not_ from the first changed line. IIRC, that has been discussed before and considered "not a bug", but I could be mis-remembering (and it's a tricky thing to search in the archive for[0]).

The problem with split hunks is different, though; we do not search for a funcname line at all on the second half of the hunk.

-Peff
[0] I did come up with:
      https://lore.kernel.org/git/1399824596-4670-1-git-send-email-avarab@gmail.com/
    which has discussion between you and me on this very same splitting
    topic back in 2014! Double-curious, your patch there implements the
    same "keep the hunk header on split" we've been discussing here, and
    we were all positive on it. Yet it doesn't seem to have ever gotten
    applied.
    It looks like Junio carried it in "What's Cooking" for almost a
    year, marked as "waiting for re-roll" to handle the squash, but then
    eventually discarded it as stale. :(
Junio C Hamano· Jun 30, 2021, 06:09 UTC · re: Jeff King · lore

Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks

Jeff King <peff@peff.net> writes:
>     It looks like Junio carried it in "What's Cooking" for almost a
>     year, marked as "waiting for re-roll" to handle the squash, but then
>     eventually discarded it as stale. :(
Heh, thanks for digging.

Is the moral of the story that we should merge down unfinished topics more aggressively (hoping that the untied loose ends would be tied after they hit released version), we should prod owners of stalled topics with sharper stick more often, or something else?

Jeff King· Jun 30, 2021, 07:31 UTC · re: Junio C Hamano · lore

Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks

On Tue, Jun 29, 2021 at 11:09:19PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >     It looks like Junio carried it in "What's Cooking" for almost a
> >     year, marked as "waiting for re-roll" to handle the squash, but then
> >     eventually discarded it as stale. :(
> 
> Heh, thanks for digging.
> 
> Is the moral of the story that we should merge down unfinished
> topics more aggressively (hoping that the untied loose ends would be
> tied after they hit released version), we should prod owners of
> stalled topics with sharper stick more often, or something else?

I'm not sure. I think the topic would have graduated if either you had just applied the squash and merged it down, or if the original author had checked back in over the intervening year to say "hey, what happened to my patch" (either by reading "what's cooking" or manually).

I suspect drive-by contributors might not realize they need to do the latter in some cases, but I wouldn't have counted 2014-era Ævar in that boat. So I dunno.

-Peff
Ævar Arnfjörð Bjarmason· Jun 30, 2021, 08:27 UTC · re: Jeff King · lore

Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks

On Wed, Jun 30 2021, Jeff King wrote:
Show 23 quoted lines
> On Tue, Jun 29, 2021 at 11:09:19PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >     It looks like Junio carried it in "What's Cooking" for almost a
>> >     year, marked as "waiting for re-roll" to handle the squash, but then
>> >     eventually discarded it as stale. :(
>> 
>> Heh, thanks for digging.
>> 
>> Is the moral of the story that we should merge down unfinished
>> topics more aggressively (hoping that the untied loose ends would be
>> tied after they hit released version), we should prod owners of
>> stalled topics with sharper stick more often, or something else?
>
> I'm not sure. I think the topic would have graduated if either you had
> just applied the squash and merged it down, or if the original author
> had checked back in over the intervening year to say "hey, what happened
> to my patch" (either by reading "what's cooking" or manually).
>
> I suspect drive-by contributors might not realize they need to do the
> latter in some cases, but I wouldn't have counted 2014-era Ævar in that
> boat. So I dunno.

Or maybe the moral of the story that it's a net addition of complexity to git-add--interactive.perl. If I didn't care enough to remember or notice the issue again maybe it wasn't all that important to begin with.

Likewise when it got ejected nobody else seemed to notice/care enough to say "hey I liked that feature" & to pick it up.

I'd entirely forgotten I wrote that. Now that I'm reminded of it I don't care enough myself to rebase it, test it again, and especially not to figure out if/how it's going to interact with the new C implementation / add and adjust a test for the two.

But maybe someone else will, it would be neat if someone has more of an itch from the lack of that feature & wants to pick it up.

Jeff King· Jun 30, 2021, 17:06 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks

On Wed, Jun 30, 2021 at 10:27:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> > I'm not sure. I think the topic would have graduated if either you had
> > just applied the squash and merged it down, or if the original author
> > had checked back in over the intervening year to say "hey, what happened
> > to my patch" (either by reading "what's cooking" or manually).
> >
> > I suspect drive-by contributors might not realize they need to do the
> > latter in some cases, but I wouldn't have counted 2014-era Ævar in that
> > boat. So I dunno.
> 
> Or maybe the moral of the story that it's a net addition of complexity
> to git-add--interactive.perl. If I didn't care enough to remember or
> notice the issue again maybe it wasn't all that important to begin with.
> 
> Likewise when it got ejected nobody else seemed to notice/care enough to
> say "hey I liked that feature" & to pick it up.
Yeah, that's probably a fair interpretation, too. :)
Show 7 quoted lines
> I'd entirely forgotten I wrote that. Now that I'm reminded of it I don't
> care enough myself to rebase it, test it again, and especially not to
> figure out if/how it's going to interact with the new C implementation /
> add and adjust a test for the two.
> 
> But maybe someone else will, it would be neat if someone has more of an
> itch from the lack of that feature & wants to pick it up.

I can probably save you a little time/mental energy here: the C version already does what your patch was trying to do. Once we switch to it as the default, your patch would be obsolete anyway. :)

-Peff

← back to recent threads