{"thread":{"id":"14471","subject":"[PATCH 1/2] git-gui: Fix \"Stage/Unstage Line\" with one line of context.","startedAt":"2008-07-15T21:11:00Z","lastAt":"2008-07-18T06:21:25Z","messageCount":7,"participants":["Johannes Sixt","Junio C Hamano","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"83431","messageId":"1216156261-9687-1-git-send-email-johannes.sixt@telecom.at","threadId":"14471","inReplyTo":null,"subject":"[PATCH 1/2] git-gui: Fix \"Stage/Unstage Line\" with one line of context.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-07-15T21:11:00Z","receivedAt":"2008-07-15T21:11:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"To \"Stage/Unstage Line\" we construct a patch that contains exactly one\nchange (either addition or removal); the hunk header was forged by counting\nthe old side and adjusting the count by +/-1 for the new side. But when we\ncounted the context we never counted the changed line itself. If the hunk\nhad only one removal line and one line of context, like this:\n\n    @@ -1,3 +1,2 @@\n     context 1\n    -removal\n     context 2\n\nWe had constructed this patch:\n\n    @@ -1,2 +1,1 @@\n     context 1\n    -removal\n     context 2\n\nwhich does not apply because git apply deduces that it must apply at the\nend of the file. (\"context 2\" is considered garbage and ignored.) The fix\nis that removal lines must be counted towards the context of the old side.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n lib/diff.tcl |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/lib/diff.tcl b/lib/diff.tcl\nindex 96ba949..ee7f391 100644\n--- a/lib/diff.tcl\n+++ b/lib/diff.tcl\n@@ -423,6 +423,9 @@ proc apply_line {x y} {\n \t\t\t# the line to stage/unstage\n \t\t\tset ln [$ui_diff get $i_l $next_l]\n \t\t\tset patch \"$patch$ln\"\n+\t\t\tif {$c1 eq {-}} {\n+\t\t\t\tset n [expr $n+1]\n+\t\t\t}\n \t\t} elseif {$c1 ne {-} && $c1 ne {+}} {\n \t\t\t# context line\n \t\t\tset ln [$ui_diff get $i_l $next_l]\n-- \n1.5.6.3.323.g1e58\n"},{"id":"83432","messageId":"1216156261-9687-2-git-send-email-johannes.sixt@telecom.at","threadId":"14471","inReplyTo":"1216156261-9687-1-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH 2/2] git-gui: Allow \"Stage Line\" to stage adjacent changes independently","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-07-15T21:11:01Z","receivedAt":"2008-07-15T21:11:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Consider this hunk:\n\n  @@ -10,4 +10,4 @@\n   context before\n  -old 1\n  -old 2\n  +new 1\n  +new 2\n   context after\n\n[Nomenclature: to \"stage change 2\" means to stage lines \"-old 1\" and\n\"+new 1\", in any order; likewise for \"unstage\" and \"change 2\".]\n\nPreviously, we had this situation:\n\n  stage change 1 was not possible\n  stage change 2 was possible\n  unstage change 1 was possible\n  unstage change 2 was not possible\n\nWith this change we have this situation:\n\n  stage change 1 is possible\n  stage change 2 is not possible\n  unstage change 1 is possible\n  unstage change 2 is not possible\n\nWhat's the deal?\n\nPreviously, it was impossible to stage change 1 without also staging\nchange 2; not even by staging the complete hunk and unstaging the unwanted\npart.\n\nWith this change we can achieve all goals:\n\n  hunk is     want staged   want unstaged   do this\n  ----------------------------------------------------------------------\n  unstaged    change 1      change 2        stage change 1\n  unstaged    change 2      change 1        stage hunk, unstage change 1\n  staged      change 1      change 2        unstage hunk, stage change 1\n  staged      change 2      change 1        unstage change 1\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n lib/diff.tcl |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 58 insertions(+), 3 deletions(-)\n\ndiff --git a/lib/diff.tcl b/lib/diff.tcl\nindex ee7f391..77990c5 100644\n--- a/lib/diff.tcl\n+++ b/lib/diff.tcl\n@@ -411,6 +411,53 @@ proc apply_line {x y} {\n \tset hh [lindex [split $hh ,] 0]\n \tset hln [lindex [split $hh -] 1]\n \n+\t# There is a special situation to take care of. Consider this hunk:\n+\t#\n+\t#    @@ -10,4 +10,4 @@\n+\t#     context before\n+\t#    -old 1\n+\t#    -old 2\n+\t#    +new 1\n+\t#    +new 2\n+\t#     context after\n+\t#\n+\t# We used to keep the context lines in the order they appear in the\n+\t# hunk. But then it is not possible to correctly stage only\n+\t# \"-old 1\" and \"+new 1\" - it would result in this staged text:\n+\t#\n+\t#    context before\n+\t#    old 2\n+\t#    new 1\n+\t#    context after\n+\t#\n+\t# (By symmetry it is not possible to *un*stage \"old 2\" and \"new 2\".)\n+\t#\n+\t# We resolve the problem by introducing an asymmetry, namely, when\n+\t# a \"+\" line is *staged*, it is moved in front of the context lines\n+\t# that are generated from the \"-\" lines that are immediately before\n+\t# the \"+\" block. That is, we construct this patch:\n+\t#\n+\t#    @@ -10,4 +10,5 @@\n+\t#     context before\n+\t#    +new 1\n+\t#     old 1\n+\t#     old 2\n+\t#     context after\n+\t#\n+\t# But we do *not* treat \"-\" lines that are *un*staged in a special\n+\t# way.\n+\t#\n+\t# With this asymmetry it is possible to stage the change\n+\t# \"old 1\" -> \"new 1\" directly, and to stage the change\n+\t# \"old 2\" -> \"new 2\" by first staging the entire hunk and\n+\t# then unstaging the change \"old 1\" -> \"new 1\".\n+\n+\t# This is non-empty if and only if we are _staging_ changes;\n+\t# then it accumulates the consecutive \"-\" lines (after converting\n+\t# them to context lines) in order to be moved after the \"+\" change\n+\t# line.\n+\tset pre_context {}\n+\n \tset n 0\n \tset i_l [$ui_diff index \"$i_l + 1 lines\"]\n \tset patch {}\n@@ -422,19 +469,27 @@ proc apply_line {x y} {\n \t\t    [$ui_diff compare $the_l < $next_l]} {\n \t\t\t# the line to stage/unstage\n \t\t\tset ln [$ui_diff get $i_l $next_l]\n-\t\t\tset patch \"$patch$ln\"\n \t\t\tif {$c1 eq {-}} {\n \t\t\t\tset n [expr $n+1]\n+\t\t\t\tset patch \"$patch$pre_context$ln\"\n+\t\t\t} else {\n+\t\t\t\tset patch \"$patch$ln$pre_context\"\n \t\t\t}\n+\t\t\tset pre_context {}\n \t\t} elseif {$c1 ne {-} && $c1 ne {+}} {\n \t\t\t# context line\n \t\t\tset ln [$ui_diff get $i_l $next_l]\n-\t\t\tset patch \"$patch$ln\"\n+\t\t\tset patch \"$patch$pre_context$ln\"\n \t\t\tset n [expr $n+1]\n+\t\t\tset pre_context {}\n \t\t} elseif {$c1 eq $to_context} {\n \t\t\t# turn change line into context line\n \t\t\tset ln [$ui_diff get \"$i_l + 1 chars\" $next_l]\n-\t\t\tset patch \"$patch $ln\"\n+\t\t\tif {$c1 eq {-}} {\n+\t\t\t\tset pre_context \"$pre_context $ln\"\n+\t\t\t} else {\n+\t\t\t\tset patch \"$patch $ln\"\n+\t\t\t}\n \t\t\tset n [expr $n+1]\n \t\t}\n \t\tset i_l $next_l\n-- \n1.5.6.3.323.g1e58\n"},{"id":"83437","messageId":"7vy742zul7.fsf@gitster.siamese.dyndns.org","threadId":"14471","inReplyTo":"1216156261-9687-2-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 2/2] git-gui: Allow \"Stage Line\" to stage adjacent changes independently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-15T21:49:24Z","receivedAt":"2008-07-15T21:49:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> Consider this hunk:\n>\n>   @@ -10,4 +10,4 @@\n>    context before\n>   -old 1\n>   -old 2\n>   +new 1\n>   +new 2\n>    context after\n>\n> [Nomenclature: to \"stage change 2\" means to stage lines \"-old 1\" and\n> \"+new 1\", in any order; likewise for \"unstage\" and \"change 2\".]\n\nYou lost me.\n\nDo you mean to say that you always interpret the above hunk as:\n\n   @@ -10,4 +10,4 @@\n    context before\n   -old 1\n   +new 1\n   -old 2\n   +new 2\n    context after\n\nand call \"replace 'old 1' with 'new 1'\" as \"change 1\", \"replace 'old\n2' with 'new 2'\" as \"change 2\"?\n\nIf it is what you are doing, it does not make much sense to me.  \"new 1\"\nmay correspond to \"old 1\" and \"old 2\" while \"new 2\" may be an independent\naddition.  E.g.\n\n   @@ -10,4 +10,4 @@\n    context before\n   -#define add(x,y) \\\n   - (x) + (y)\n   +#define add(x,y) ((x)+(y))\n   +#define sub(x,y) ((x)-(y))\n    context after\n\nI might want to pick bugfix of add() definition without using the new\ndefinition of sub().\n\nPlease call\n\n\t\"-old 1\" - change #1\n        \"-old 2\" - change #2\n        \"+new 1\" - change #3\n        \"+new 2\" - change #4\n\nand try explaining what you are doing again, pretty please?\n"},{"id":"83451","messageId":"20080716003503.GA24339@spearce.org","threadId":"14471","inReplyTo":"7vy742zul7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git-gui: Allow \"Stage Line\" to stage adjacent changes independently","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-07-16T00:35:03Z","receivedAt":"2008-07-16T00:35:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Sixt <johannes.sixt@telecom.at> writes:\n> \n> > Consider this hunk:\n> >\n> >   @@ -10,4 +10,4 @@\n> >    context before\n> >   -old 1\n> >   -old 2\n> >   +new 1\n> >   +new 2\n> >    context after\n> >\n> > [Nomenclature: to \"stage change 2\" means to stage lines \"-old 1\" and\n> > \"+new 1\", in any order; likewise for \"unstage\" and \"change 2\".]\n> \n> You lost me.\n...\n> and try explaining what you are doing again, pretty please?\n\nMe too.  I really appreciate the effort spent to improve this\nfeature, but I really didn't follow the commit message at all.  :-|\n\n-- \nShawn.\n"},{"id":"83484","messageId":"487DA003.3090905@viscovery.net","threadId":"14471","inReplyTo":"7vy742zul7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git-gui: Allow \"Stage Line\" to stage adjacent changes independently","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-07-16T07:15:15Z","receivedAt":"2008-07-16T07:15:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt <johannes.sixt@telecom.at> writes:\n> \n>> Consider this hunk:\n>>\n>>   @@ -10,4 +10,4 @@\n>>    context before\n>>   -old 1\n>>   -old 2\n>>   +new 1\n>>   +new 2\n>>    context after\n>>\n>> [Nomenclature: to \"stage change 2\" means to stage lines \"-old 1\" and\n>> \"+new 1\", in any order; likewise for \"unstage\" and \"change 2\".]\n> \n> You lost me.\n> \n> Do you mean to say that you always interpret the above hunk as:\n> \n>    @@ -10,4 +10,4 @@\n>     context before\n>    -old 1\n>    +new 1\n>    -old 2\n>    +new 2\n>     context after\n> \n> and call \"replace 'old 1' with 'new 1'\" as \"change 1\", \"replace 'old\n> 2' with 'new 2'\" as \"change 2\"?\n\nNo, it is not that I *always* interpret it this way. There is a problem to\nfix only if I *want* to interpret it this way. Probably that's what I have\nto make clear?\n\n> If it is what you are doing, it does not make much sense to me.  \"new 1\"\n> may correspond to \"old 1\" and \"old 2\" while \"new 2\" may be an independent\n> addition.  E.g.\n> \n>    @@ -10,4 +10,4 @@\n>     context before\n>    -#define add(x,y) \\\n>    - (x) + (y)\n>    +#define add(x,y) ((x)+(y))\n>    +#define sub(x,y) ((x)-(y))\n>     context after\n> \n> I might want to pick bugfix of add() definition without using the new\n> definition of sub().\n\nIn order to that, there is nothing to fix; you can do that today without\nthis patch.\n\n> Please call\n> \n> \t\"-old 1\" - change #1\n>         \"-old 2\" - change #2\n>         \"+new 1\" - change #3\n>         \"+new 2\" - change #4\n> \n> and try explaining what you are doing again, pretty please?\n\nNo, this sounds like 4 independent changes, and that is not what this fix\nis about.\n\nI'll try to come up with a better wording.\n\n-- Hannes\n"},{"id":"83687","messageId":"1216300911-5170-1-git-send-email-johannes.sixt@telecom.at","threadId":"14471","inReplyTo":"20080716003503.GA24339@spearce.org","subject":"[PATCH 2/2 v2] git-gui: \"Stage Line\": Treat independent changes in adjacent lines better","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-07-17T13:21:51Z","receivedAt":"2008-07-17T13:21:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Assume that we want to commit these states:\n\n  Old state == HEAD    Intermediate state   New state\n  --------------------------------------------------------\n  context before       context before       context before\n  old 1                new 1                new 1\n  old 2                old 2                new 2\n  context after        context after        context after\n\nthat is, want to commit two changes in this order:\n\n  1. transform \"old 1\" into \"new 1\"\n  2. transform \"old 2\" into \"new 2\"\n\n[This discussion and this patch is about this very case and one other case\nas outlined below; any other intermediate states that one could imagine are\nnot affected by this patch.]\n\nNow assume further, that we have not staged and commited anything, but we\nhave already changed the working file to the new state. Then we will see\nthis hunk in the \"Unstaged Changes\":\n\n  @@ -1,4 +1,4 @@\n   context before\n  -old 1\n  -old 2\n  +new 1\n  +new 2\n   context after\n\nThe obvious way to stage the intermediate state is to apply \"Stage This\nLine\" to \"-old 1\" and \"+new 1\". Unfortunately, this resulted in this\nintermediate state:\n\n  context before\n  old 2\n  new 1\n  context after\n\nwhich is not what we wanted. In fact, it was impossible to stage the\nintermediate state using \"Stage Line\". The crux was that if a \"+\" line was\nstaged, then the \"-\" lines were converted to context lines and arranged\n*before* the \"+\" line in the forged hunk that we fed to 'git apply'.\n\nWith this patch we now treat \"+\" lines that are staged differently. In\nparticular, the \"-\" lines before the \"+\" block are moved *after* the\nstaged \"+\" line. Now it is possible to get the correct intermediate state\nby staging \"-old 1\" and \"+new 1\". Problem solved.\n\nBut there is a catch.\n\nNoticing that we didn't get the right intermediate state by staging\n\"-old 1\" and \"+new 1\", we could have had the idea to stage the complete\nhunk and to *unstage* \"-old 2\" and \"+new 2\". But... the result is the same.\nThe reason is that there is the exact symmetric problem with unstaging the\nlast \"-\" and \"+\" line that are in adjacent blocks of \"-\" and \"+\" lines.\n\nThis patch does *not* change the way in which \"-\" lines are *unstaged*.\n\nWhy? Because if we did (i.e. move \"+\" lines before the \"-\" line after\nconverting them to context lines), then it would be impossible to stage\nthis intermediate state:\n\n  context before\n  old 1\n  new 2\n  context after\n\nthat is, it would be impossible to stage the two independet changes in the\nopposite order.\n\nLet's look at this case a bit further: The obvious way to get this\nintermediate state would be to apply \"Stage This Line\" to \"-old 2\" and\n\"+new 2\". Before this patch, this worked as expected. With this patch, it\ndoes not work as expected, but it can still be achieved by first staging\nthe entire hunk, then *unstaging* \"-old 1\" and \"+new 1\".\n\nIn summary, this patch makes a common case possible, at the expense that\na less common case is made more complicated for the user.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n\n  The patch is unmodified, but I hope the commit message is clearer.\n\n  -- Hannes\n\n lib/diff.tcl |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 58 insertions(+), 3 deletions(-)\n\ndiff --git a/lib/diff.tcl b/lib/diff.tcl\nindex ee7f391..77990c5 100644\n--- a/lib/diff.tcl\n+++ b/lib/diff.tcl\n@@ -411,6 +411,53 @@ proc apply_line {x y} {\n \tset hh [lindex [split $hh ,] 0]\n \tset hln [lindex [split $hh -] 1]\n \n+\t# There is a special situation to take care of. Consider this hunk:\n+\t#\n+\t#    @@ -10,4 +10,4 @@\n+\t#     context before\n+\t#    -old 1\n+\t#    -old 2\n+\t#    +new 1\n+\t#    +new 2\n+\t#     context after\n+\t#\n+\t# We used to keep the context lines in the order they appear in the\n+\t# hunk. But then it is not possible to correctly stage only\n+\t# \"-old 1\" and \"+new 1\" - it would result in this staged text:\n+\t#\n+\t#    context before\n+\t#    old 2\n+\t#    new 1\n+\t#    context after\n+\t#\n+\t# (By symmetry it is not possible to *un*stage \"old 2\" and \"new 2\".)\n+\t#\n+\t# We resolve the problem by introducing an asymmetry, namely, when\n+\t# a \"+\" line is *staged*, it is moved in front of the context lines\n+\t# that are generated from the \"-\" lines that are immediately before\n+\t# the \"+\" block. That is, we construct this patch:\n+\t#\n+\t#    @@ -10,4 +10,5 @@\n+\t#     context before\n+\t#    +new 1\n+\t#     old 1\n+\t#     old 2\n+\t#     context after\n+\t#\n+\t# But we do *not* treat \"-\" lines that are *un*staged in a special\n+\t# way.\n+\t#\n+\t# With this asymmetry it is possible to stage the change\n+\t# \"old 1\" -> \"new 1\" directly, and to stage the change\n+\t# \"old 2\" -> \"new 2\" by first staging the entire hunk and\n+\t# then unstaging the change \"old 1\" -> \"new 1\".\n+\n+\t# This is non-empty if and only if we are _staging_ changes;\n+\t# then it accumulates the consecutive \"-\" lines (after converting\n+\t# them to context lines) in order to be moved after the \"+\" change\n+\t# line.\n+\tset pre_context {}\n+\n \tset n 0\n \tset i_l [$ui_diff index \"$i_l + 1 lines\"]\n \tset patch {}\n@@ -422,19 +469,27 @@ proc apply_line {x y} {\n \t\t    [$ui_diff compare $the_l < $next_l]} {\n \t\t\t# the line to stage/unstage\n \t\t\tset ln [$ui_diff get $i_l $next_l]\n-\t\t\tset patch \"$patch$ln\"\n \t\t\tif {$c1 eq {-}} {\n \t\t\t\tset n [expr $n+1]\n+\t\t\t\tset patch \"$patch$pre_context$ln\"\n+\t\t\t} else {\n+\t\t\t\tset patch \"$patch$ln$pre_context\"\n \t\t\t}\n+\t\t\tset pre_context {}\n \t\t} elseif {$c1 ne {-} && $c1 ne {+}} {\n \t\t\t# context line\n \t\t\tset ln [$ui_diff get $i_l $next_l]\n-\t\t\tset patch \"$patch$ln\"\n+\t\t\tset patch \"$patch$pre_context$ln\"\n \t\t\tset n [expr $n+1]\n+\t\t\tset pre_context {}\n \t\t} elseif {$c1 eq $to_context} {\n \t\t\t# turn change line into context line\n \t\t\tset ln [$ui_diff get \"$i_l + 1 chars\" $next_l]\n-\t\t\tset patch \"$patch $ln\"\n+\t\t\tif {$c1 eq {-}} {\n+\t\t\t\tset pre_context \"$pre_context $ln\"\n+\t\t\t} else {\n+\t\t\t\tset patch \"$patch $ln\"\n+\t\t\t}\n \t\t\tset n [expr $n+1]\n \t\t}\n \t\tset i_l $next_l\n-- \n1.5.6.1.275.g0a3e0f\n"},{"id":"83832","messageId":"7vod4vd862.fsf@gitster.siamese.dyndns.org","threadId":"14471","inReplyTo":"1216300911-5170-1-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH 2/2 v2] git-gui: \"Stage Line\": Treat independent changes in adjacent lines better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-18T06:21:25Z","receivedAt":"2008-07-18T06:21:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> Assume that we want to commit these states:\n>\n>   Old state == HEAD    Intermediate state   New state\n>   --------------------------------------------------------\n>   context before       context before       context before\n>   old 1                new 1                new 1\n>   old 2                old 2                new 2\n>   context after        context after        context after\n\nMuch easier to understand.  Thanks.\n"}]}