{"thread":{"id":"36632","subject":"[PATCH] git-add--interactive: Preserve diff heading when splitting hunks","startedAt":"2014-05-11T16:09:56Z","lastAt":"2014-05-12T21:12:35Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"241261","messageId":"1399824596-4670-1-git-send-email-avarab@gmail.com","threadId":"36632","inReplyTo":null,"subject":"[PATCH] git-add--interactive: Preserve diff heading when splitting hunks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2014-05-11T16:09:56Z","receivedAt":"2014-05-11T16:09:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the display of hunks in hunk splitting mode to preserve the diff\nheading, which hasn't been done ever since the hunk splitting was\ninitially added in v1.4.4.2-270-g835b2ae.\n\nSplitting the first hunk of this patch will now result in:\n\n    Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n    Split into 2 hunks.\n    @@ -792,7 +792,7 @@ sub hunk_splittable {\n    [...]\n\nInstead of:\n\n    Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n    Split into 2 hunks.\n    @@ -792,7 +792,7 @@\n    [...]\n\nThis makes it easier to use the tool when you're splitting some giant\nhunk and can't remember in which function you are anymore.\n\nThe diff is somewhat larger than I initially expected because in order\nto display the headings in the same color scheme as the output from\ngit-diff(1) itself I had to split up the code that would previously\ncolor diff output that previously consisted entirely of the fraginfo,\nbut now consists of the fraginfo and the diff heading (the latter of\nwhich isn't colored).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-add--interactive.perl | 40 ++++++++++++++++++++++++----------------\n 1 file changed, 24 insertions(+), 16 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 1fadd69..ed1e564 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -792,11 +792,11 @@ sub hunk_splittable {\n \n sub parse_hunk_header {\n \tmy ($line) = @_;\n-\tmy ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =\n-\t    $line =~ /^@@ -(\\d+)(?:,(\\d+))? \\+(\\d+)(?:,(\\d+))? @@/;\n+\tmy ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $heading) =\n+\t    $line =~ /^@@ -(\\d+)(?:,(\\d+))? \\+(\\d+)(?:,(\\d+))? @@(.*)/;\n \t$o_cnt = 1 unless defined $o_cnt;\n \t$n_cnt = 1 unless defined $n_cnt;\n-\treturn ($o_ofs, $o_cnt, $n_ofs, $n_cnt);\n+\treturn ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $heading);\n }\n \n sub split_hunk {\n@@ -808,8 +808,7 @@ sub split_hunk {\n \t# If there are context lines in the middle of a hunk,\n \t# it can be split, but we would need to take care of\n \t# overlaps later.\n-\n-\tmy ($o_ofs, undef, $n_ofs) = parse_hunk_header($text->[0]);\n+\tmy ($o_ofs, undef, $n_ofs, undef, $heading) = parse_hunk_header($text->[0]);\n \tmy $hunk_start = 1;\n \n       OUTER:\n@@ -886,17 +885,26 @@ sub split_hunk {\n \t\tmy $o_cnt = $hunk->{OCNT};\n \t\tmy $n_cnt = $hunk->{NCNT};\n \n-\t\tmy $head = (\"@@ -$o_ofs\" .\n-\t\t\t    (($o_cnt != 1) ? \",$o_cnt\" : '') .\n-\t\t\t    \" +$n_ofs\" .\n-\t\t\t    (($n_cnt != 1) ? \",$n_cnt\" : '') .\n-\t\t\t    \" @@\\n\");\n-\t\tmy $display_head = $head;\n-\t\tunshift @{$hunk->{TEXT}}, $head;\n-\t\tif ($diff_use_color) {\n-\t\t\t$display_head = colored($fraginfo_color, $head);\n-\t\t}\n-\t\tunshift @{$hunk->{DISPLAY}}, $display_head;\n+\t\tmy $fraginfo = join(\n+\t\t\t\"\",\n+\t\t\t\"@@ -$o_ofs\",\n+\t\t\t(($o_cnt != 1) ? \",$o_cnt\" : ''),\n+\t\t\t\" +$n_ofs\",\n+\t\t\t(($n_cnt != 1) ? \",$n_cnt\" : ''),\n+\t\t\t\" @@\"\n+\t\t);\n+\t\tunshift @{$hunk->{TEXT}}, join(\n+\t\t\t\"\",\n+\t\t\t$fraginfo,\n+\t\t\t$heading,\n+\t\t\t\"\\n\"\n+\t\t);\n+\t\tunshift @{$hunk->{DISPLAY}}, join(\n+\t\t\t\"\",\n+\t\t\t$diff_use_color ? colored($fraginfo_color, $fraginfo) : $fraginfo,\n+\t\t\t$heading,\n+\t\t\t\"\\n\"\n+\t\t);\n \t}\n \treturn @split;\n }\n-- \n2.0.0.rc0\n"},{"id":"241264","messageId":"xmqqfvkgurrm.fsf@gitster.dls.corp.google.com","threadId":"36632","inReplyTo":"1399824596-4670-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-11T17:52:45Z","receivedAt":"2014-05-11T17:52:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change the display of hunks in hunk splitting mode to preserve the diff\n> heading, which hasn't been done ever since the hunk splitting was\n> initially added in v1.4.4.2-270-g835b2ae.\n>\n> Splitting the first hunk of this patch will now result in:\n>\n>     Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n>     Split into 2 hunks.\n>     @@ -792,7 +792,7 @@ sub hunk_splittable {\n>     [...]\n>\n> Instead of:\n>\n>     Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n>     Split into 2 hunks.\n>     @@ -792,7 +792,7 @@\n>     [...]\n>\n> This makes it easier to use the tool when you're splitting some giant\n> hunk and can't remember in which function you are anymore.\n\nMakes sense to me.\n\n>\n> The diff is somewhat larger than I initially expected because in order\n> to display the headings in the same color scheme as the output from\n> git-diff(1) itself I had to split up the code that would previously\n> color diff output that previously consisted entirely of the fraginfo,\n> but now consists of the fraginfo and the diff heading (the latter of\n> which isn't colored).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  git-add--interactive.perl | 40 ++++++++++++++++++++++++----------------\n>  1 file changed, 24 insertions(+), 16 deletions(-)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 1fadd69..ed1e564 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -792,11 +792,11 @@ sub hunk_splittable {\n>  \n>  sub parse_hunk_header {\n>  \tmy ($line) = @_;\n> -\tmy ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =\n> -\t    $line =~ /^@@ -(\\d+)(?:,(\\d+))? \\+(\\d+)(?:,(\\d+))? @@/;\n> +\tmy ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $heading) =\n> +\t    $line =~ /^@@ -(\\d+)(?:,(\\d+))? \\+(\\d+)(?:,(\\d+))? @@(.*)/;\n>  \t$o_cnt = 1 unless defined $o_cnt;\n>  \t$n_cnt = 1 unless defined $n_cnt;\n> -\treturn ($o_ofs, $o_cnt, $n_ofs, $n_cnt);\n> +\treturn ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $heading);\n>  }\n>  \n>  sub split_hunk {\n> @@ -808,8 +808,7 @@ sub split_hunk {\n>  \t# If there are context lines in the middle of a hunk,\n>  \t# it can be split, but we would need to take care of\n>  \t# overlaps later.\n> -\n> -\tmy ($o_ofs, undef, $n_ofs) = parse_hunk_header($text->[0]);\n> +\tmy ($o_ofs, undef, $n_ofs, undef, $heading) = parse_hunk_header($text->[0]);\n>  \tmy $hunk_start = 1;\n>  \n>        OUTER:\n> @@ -886,17 +885,26 @@ sub split_hunk {\n>  \t\tmy $o_cnt = $hunk->{OCNT};\n>  \t\tmy $n_cnt = $hunk->{NCNT};\n>  \n> -\t\tmy $head = (\"@@ -$o_ofs\" .\n> -\t\t\t    (($o_cnt != 1) ? \",$o_cnt\" : '') .\n> -\t\t\t    \" +$n_ofs\" .\n> -\t\t\t    (($n_cnt != 1) ? \",$n_cnt\" : '') .\n> -\t\t\t    \" @@\\n\");\n> -\t\tmy $display_head = $head;\n> -\t\tunshift @{$hunk->{TEXT}}, $head;\n> -\t\tif ($diff_use_color) {\n> -\t\t\t$display_head = colored($fraginfo_color, $head);\n> -\t\t}\n> -\t\tunshift @{$hunk->{DISPLAY}}, $display_head;\n> +\t\tmy $fraginfo = join(\n> +\t\t\t\"\",\n> +\t\t\t\"@@ -$o_ofs\",\n> +\t\t\t(($o_cnt != 1) ? \",$o_cnt\" : ''),\n> +\t\t\t\" +$n_ofs\",\n> +\t\t\t(($n_cnt != 1) ? \",$n_cnt\" : ''),\n> +\t\t\t\" @@\"\n> +\t\t);\n> +\t\tunshift @{$hunk->{TEXT}}, join(\n> +\t\t\t\"\",\n> +\t\t\t$fraginfo,\n> +\t\t\t$heading,\n> +\t\t\t\"\\n\"\n> +\t\t);\n> +\t\tunshift @{$hunk->{DISPLAY}}, join(\n> +\t\t\t\"\",\n> +\t\t\t$diff_use_color ? colored($fraginfo_color, $fraginfo) : $fraginfo,\n> +\t\t\t$heading,\n> +\t\t\t\"\\n\"\n> +\t\t);\n>  \t}\n>  \treturn @split;\n>  }\n"},{"id":"241302","messageId":"20140512183955.GB31164@sigill.intra.peff.net","threadId":"36632","inReplyTo":"1399824596-4670-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-12T18:39:55Z","receivedAt":"2014-05-12T18:39:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 11, 2014 at 04:09:56PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the display of hunks in hunk splitting mode to preserve the diff\n> heading, which hasn't been done ever since the hunk splitting was\n> initially added in v1.4.4.2-270-g835b2ae.\n> \n> Splitting the first hunk of this patch will now result in:\n> \n>     Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n>     Split into 2 hunks.\n>     @@ -792,7 +792,7 @@ sub hunk_splittable {\n>     [...]\n> \n> Instead of:\n> \n>     Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n>     Split into 2 hunks.\n>     @@ -792,7 +792,7 @@\n>     [...]\n> \n> This makes it easier to use the tool when you're splitting some giant\n> hunk and can't remember in which function you are anymore.\n\nThis makes a lot of sense to me. I did notice two interesting quirks,\none of which might be worth addressing.\n\nOne, there is a slightly funny artifact in that the hunk header comes\nfrom the top of the context line, and that top is a different position\nfor each of the split hunks. So in a file like:\n\n  header_A\n      content\n  header_B\n      one\n      two\n      three\n      four\n\nyou might have a diff like:\n\n  @@ ... @@ header_A\n   header_B\n       one\n       two\n  +    new line 1\n       three\n  +    new line 2\n       four\n\nThe hunk header for \"new line 1\" is \"A\", because \"B\" itself is part of\nthe context. But the hunk header for \"new line 2\", if it were an\nindependent hunk, would be \"B\". We print \"A\" because we copy it from the\noriginal hunk.\n\nIt probably won't matter much in practice (and I can even see an\nargument that \"A\" is the \"right\" answer). And figuring out \"B\" here\nwould be prohibitively difficult, I would think, as it would require\napplying the funcname rules internal to git-diff to a hunk that git-diff\nitself never actually sees.\n\nSince the output from your patch is strictly better than what we saw\nbefore, I think there is no reason we cannot leave such an improvement\nto later (or never).\n\n> The diff is somewhat larger than I initially expected because in order\n> to display the headings in the same color scheme as the output from\n> git-diff(1) itself I had to split up the code that would previously\n> color diff output that previously consisted entirely of the fraginfo,\n> but now consists of the fraginfo and the diff heading (the latter of\n> which isn't colored).\n\nThe func heading is not colored by default, but you can configure it to\nbe so with color.diff.func. I double-checked the behavior with your\npatch: you end up with the uncolored header in the split hunks, because\nit is parsed from the uncolored line. Which is not bad, but I think we\ncan trivially do better, just by adding back in the color as we do with\nthe fraginfo.\n\nLike:\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex ed1e564..ac5763d 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -29,6 +29,10 @@ my ($fraginfo_color) =\n \t$diff_use_color ? (\n \t\t$repo->get_color('color.diff.frag', 'cyan'),\n \t) : ();\n+my ($funcname_color) =\n+\t$diff_use_color ? (\n+\t\t$repo->get_color('color.diff.func', ''),\n+\t) : ();\n my ($diff_plain_color) =\n \t$diff_use_color ? (\n \t\t$repo->get_color('color.diff.plain', ''),\n@@ -902,7 +906,7 @@ sub split_hunk {\n \t\tunshift @{$hunk->{DISPLAY}}, join(\n \t\t\t\"\",\n \t\t\t$diff_use_color ? colored($fraginfo_color, $fraginfo) : $fraginfo,\n-\t\t\t$heading,\n+\t\t\t$diff_use_color ? colored($funcname_color, $heading) : $heading,\n \t\t\t\"\\n\"\n \t\t);\n \t}\n\nI didn't prepare a commit message because I think it should probably\njust be squashed in.\n\n-Peff\n"},{"id":"241310","messageId":"CACBZZX4W=1jPGcuBYstQkBDaT_wu38Fhxu642qwaFtCCqBdnyQ@mail.gmail.com","threadId":"36632","inReplyTo":"20140512183955.GB31164@sigill.intra.peff.net","subject":"Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2014-05-12T19:42:56Z","receivedAt":"2014-05-12T19:42:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, May 12, 2014 at 8:39 PM, Jeff King <peff@peff.net> wrote:\n> On Sun, May 11, 2014 at 04:09:56PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Change the display of hunks in hunk splitting mode to preserve the diff\n>> heading, which hasn't been done ever since the hunk splitting was\n>> initially added in v1.4.4.2-270-g835b2ae.\n>>\n>> Splitting the first hunk of this patch will now result in:\n>>\n>>     Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n>>     Split into 2 hunks.\n>>     @@ -792,7 +792,7 @@ sub hunk_splittable {\n>>     [...]\n>>\n>> Instead of:\n>>\n>>     Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s\n>>     Split into 2 hunks.\n>>     @@ -792,7 +792,7 @@\n>>     [...]\n>>\n>> This makes it easier to use the tool when you're splitting some giant\n>> hunk and can't remember in which function you are anymore.\n>\n> This makes a lot of sense to me. I did notice two interesting quirks,\n> one of which might be worth addressing.\n>\n> One, there is a slightly funny artifact in that the hunk header comes\n> from the top of the context line, and that top is a different position\n> for each of the split hunks. So in a file like:\n>\n>   header_A\n>       content\n>   header_B\n>       one\n>       two\n>       three\n>       four\n>\n> you might have a diff like:\n>\n>   @@ ... @@ header_A\n>    header_B\n>        one\n>        two\n>   +    new line 1\n>        three\n>   +    new line 2\n>        four\n>\n> The hunk header for \"new line 1\" is \"A\", because \"B\" itself is part of\n> the context. But the hunk header for \"new line 2\", if it were an\n> independent hunk, would be \"B\". We print \"A\" because we copy it from the\n> original hunk.\n>\n> It probably won't matter much in practice (and I can even see an\n> argument that \"A\" is the \"right\" answer). And figuring out \"B\" here\n> would be prohibitively difficult, I would think, as it would require\n> applying the funcname rules internal to git-diff to a hunk that git-diff\n> itself never actually sees.\n>\n> Since the output from your patch is strictly better than what we saw\n> before, I think there is no reason we cannot leave such an improvement\n> to later (or never).\n\nGood suggestion, but tricky as you point out. Another thing I've\nwanted many times is to make it smart enough that when you edit code\nlike:\n\n  A()\n  B();\n\nAnd change it to:\n\n  X();\n\n  Y();\n\nThe change from A->X and B->Y may be completely unrelated and just\nmade in code where the author didn't add whitespace between unrelated\nstatements.\n\nBut because you change all the lines the tool can't split them up, it\ncould try harder and split hunks like that if you add a whitespace\nboundary, or just go all the way down to adding/removing individual\nlines, so you wouldn't have to fall down to \"edit\" mode and do so\nmanually.\n\n\n>> The diff is somewhat larger than I initially expected because in order\n>> to display the headings in the same color scheme as the output from\n>> git-diff(1) itself I had to split up the code that would previously\n>> color diff output that previously consisted entirely of the fraginfo,\n>> but now consists of the fraginfo and the diff heading (the latter of\n>> which isn't colored).\n>\n> The func heading is not colored by default, but you can configure it to\n> be so with color.diff.func. I double-checked the behavior with your\n> patch: you end up with the uncolored header in the split hunks, because\n> it is parsed from the uncolored line. Which is not bad, but I think we\n> can trivially do better, just by adding back in the color as we do with\n> the fraginfo.\n>\n> Like:\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index ed1e564..ac5763d 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -29,6 +29,10 @@ my ($fraginfo_color) =\n>         $diff_use_color ? (\n>                 $repo->get_color('color.diff.frag', 'cyan'),\n>         ) : ();\n> +my ($funcname_color) =\n> +       $diff_use_color ? (\n> +               $repo->get_color('color.diff.func', ''),\n> +       ) : ();\n>  my ($diff_plain_color) =\n>         $diff_use_color ? (\n>                 $repo->get_color('color.diff.plain', ''),\n> @@ -902,7 +906,7 @@ sub split_hunk {\n>                 unshift @{$hunk->{DISPLAY}}, join(\n>                         \"\",\n>                         $diff_use_color ? colored($fraginfo_color, $fraginfo) : $fraginfo,\n> -                       $heading,\n> +                       $diff_use_color ? colored($funcname_color, $heading) : $heading,\n>                         \"\\n\"\n>                 );\n>         }\n>\n> I didn't prepare a commit message because I think it should probably\n> just be squashed in.\n\nWell spotted, indeed, that should be squashed in.\n\nOn a related note I thought by doing color.ui=auto I was turning on\nall the colors, it would be nice if there was a built-in colorscheme\nthat added more coloring to items like these across our tools, it's\nuseful to have the hunk headers colored differently so they stand out\nmore.\n"},{"id":"241313","messageId":"20140512200952.GA2329@sigill.intra.peff.net","threadId":"36632","inReplyTo":"CACBZZX4W=1jPGcuBYstQkBDaT_wu38Fhxu642qwaFtCCqBdnyQ@mail.gmail.com","subject":"Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-12T20:09:52Z","receivedAt":"2014-05-12T20:09:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 12, 2014 at 09:42:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Good suggestion, but tricky as you point out. Another thing I've\n> wanted many times is to make it smart enough that when you edit code\n> like:\n> \n>   A()\n>   B();\n> \n> And change it to:\n> \n>   X();\n> \n>   Y();\n> \n> The change from A->X and B->Y may be completely unrelated and just\n> made in code where the author didn't add whitespace between unrelated\n> statements.\n> \n> But because you change all the lines the tool can't split them up, it\n> could try harder and split hunks like that if you add a whitespace\n> boundary, or just go all the way down to adding/removing individual\n> lines, so you wouldn't have to fall down to \"edit\" mode and do so\n> manually.\n\nYeah, I frequently run into this, too, and have just gotten good at\nediting the patch. :)\n\nI think splitting line-by-line, as you suggest, would be more general.\nHowever, working within a single cluster of lines is difficult (both\nline-by-line and in your whitespace example), because you have to\ncorrelate removed and added lines. E.g., the diff for your change above\nmight look like:\n\n  @@ ... @@\n   context\n   -A();\n   -B();\n   +X();\n   +\n   +Y();\n   more context\n\nWe would want to end up with three hunks:\n\n  -A();\n  +X();\n\n  +\n\n  -B();\n  +Y();\n\nbut how do we know which preimage lines correspond with which postimage\nlines? You can probably come up with heuristics for this case (e.g.,\nintroduced whitespace gets its own hunk, and then count postimage lines\nfrom each end), but there are many harder cases.\n\n> > I didn't prepare a commit message because I think it should probably\n> > just be squashed in.\n> \n> Well spotted, indeed, that should be squashed in.\n\nIf you do (or Junio does), I think the commit message needs a slight\nupdate.\n\n> On a related note I thought by doing color.ui=auto I was turning on\n> all the colors, it would be nice if there was a built-in colorscheme\n> that added more coloring to items like these across our tools, it's\n> useful to have the hunk headers colored differently so they stand out\n> more.\n\nI don't set color.diff.func myself, but having just looked at it when\nplaying with your patch, I think it looks nice (I did \"yellow\").\n\nI also set color.diff.meta to \"magenta\", which I think makes the hunk\nheader stand out a bit more.\n\nSo yeah, I'd be fine with proposing changes to the default color scheme,\nthough I do not envy you the inevitable bikeshed war. :)\n\n-Peff\n"},{"id":"241319","messageId":"xmqqvbtapuyk.fsf@gitster.dls.corp.google.com","threadId":"36632","inReplyTo":"20140512183955.GB31164@sigill.intra.peff.net","subject":"Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-12T21:07:15Z","receivedAt":"2014-05-12T21:07:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> One, there is a slightly funny artifact in that the hunk header comes\n> from the top of the context line, and that top is a different position\n> for each of the split hunks. So in a file like:\n>\n>   header_A\n>       content\n>   header_B\n>       one\n>       two\n>       three\n>       four\n>\n> you might have a diff like:\n>\n>   @@ ... @@ header_A\n>    header_B\n>        one\n>        two\n>   +    new line 1\n>        three\n>   +    new line 2\n>        four\n>\n> The hunk header for \"new line 1\" is \"A\", because \"B\" itself is part of\n> the context. But the hunk header for \"new line 2\", if it were an\n> independent hunk, would be \"B\". We print \"A\" because we copy it from the\n> original hunk.\n>\n> It probably won't matter much in practice (and I can even see an\n> argument that \"A\" is the \"right\" answer).\n\nI tend to agree with both points.\n\n> And figuring out \"B\" here\n> would be prohibitively difficult, I would think, as it would require\n> applying the funcname rules internal to git-diff to a hunk that git-diff\n> itself never actually sees.\n\nYou can actually apply a split hunk being proposed to a temporary\nfile and then ask \"git diff\" about it, so I do not think difficult\nis too much of an issue, but I doubt we would want to see header_B,\nexactly because when the user says \"Split this hunk\", s/he is very\nwell aware that the second one is artificial and was split from the\noriginal hunk whose header said header_A.\n\n> Since the output from your patch is strictly better than what we saw\n> before, I think there is no reason we cannot leave such an improvement\n> to later (or never).\n\nYes.\n"},{"id":"241320","messageId":"20140512211235.GA4705@sigill.intra.peff.net","threadId":"36632","inReplyTo":"xmqqvbtapuyk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-12T21:12:35Z","receivedAt":"2014-05-12T21:12:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 12, 2014 at 02:07:15PM -0700, Junio C Hamano wrote:\n\n> > And figuring out \"B\" here\n> > would be prohibitively difficult, I would think, as it would require\n> > applying the funcname rules internal to git-diff to a hunk that git-diff\n> > itself never actually sees.\n> \n> You can actually apply a split hunk being proposed to a temporary\n> file and then ask \"git diff\" about it, so I do not think difficult\n> is too much of an issue,\n\nTrue, I didn't think of that.\n\n> but I doubt we would want to see header_B,\n> exactly because when the user says \"Split this hunk\", s/he is very\n> well aware that the second one is artificial and was split from the\n> original hunk whose header said header_A.\n\nRight, that's along the lines of the \"you could make the argument\" I was\nthinking of. Since you are thinking it, too, I'm definitely in favor of\nstopping at Ævar's patch and seeing if anybody even notices or\ncomplains.\n\nThanks.\n\n-Peff\n"}]}