git add --interactive patch improvement for split hunks

git add --interactive patch improvement for split hunks

[Ulrich Windl]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

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,?]?




[-- Attachment #2: Screenshot from 2021-06-24 12-29-38.png --]
[-- Type: image/png, Size: 125599 bytes --]

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

[Jeff King]

On Thu, Jun 24, 2021 at 12:35:16PM +0200, Ulrich Windl wrote:

> 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?

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

[Ulrich Windl]

>>> 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.

> 
> -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?

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

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 28 2021, Ulrich Windl wrote:

>>>> 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.

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

[Jeff King]

On Mon, Jun 28, 2021 at 01:20:46PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > 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. :(

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

[Junio C Hamano]

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?

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

[Jeff King]

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.

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Jeff King wrote:

> 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.

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

[Jeff King]

On Wed, Jun 30, 2021 at 10:27:16AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > 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. :)

> 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