{"thread":{"id":"34090","subject":"Different diff strategies in add --interactive","startedAt":"2013-06-10T14:28:25Z","lastAt":"2013-06-23T20:37:00Z","messageCount":10,"participants":["John Keeping","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"220290","messageId":"20130610142825.GI22905@serenity.lan","threadId":"34090","inReplyTo":null,"subject":"Different diff strategies in add --interactive","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-10T14:28:25Z","receivedAt":"2013-06-10T14:28:25Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"I've just been trying to use \"add -p\" to stage some changes which happen\nto be textually entangled with other changes that I do not want to\nstage.\n\nIt turns out that \"git diff --patience\" does a really good job\nsplitting this into exactly the hunks I want, but \"add --interactive\"\ndoesn't let me change the diff algorithm it uses.  I tried setting\n\"diff.algorithm\" to \"patience\", but of course add--interactive uses\nplumbing diff commands that ignore configuration settings.\n\nAs a one off, I locally modified add--interactive to unconditionally use\npatience diff and it has worked perfectly in this case, but I don't want\nto have to apply a patch if I ever want this behaviour in the future.\n\nI think the first thing to do is read the \"diff.algorithm\" setting in\ngit-add--interactive and pass its value to the underlying diff-index and\ndiff-files commands, but should we also have a command line parameter to\ngit-add to specify the diff algorithm in interactive mode?  And if so,\ncan we simply add \"--diff-algorithm\" to git-add, or is that too\nconfusing?\n"},{"id":"220363","messageId":"7v38sphiiw.fsf@alter.siamese.dyndns.org","threadId":"34090","inReplyTo":"20130610142825.GI22905@serenity.lan","subject":"Re: Different diff strategies in add --interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-10T19:28:55Z","receivedAt":"2013-06-10T19:28:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> I think the first thing to do is read the \"diff.algorithm\" setting in\n> git-add--interactive and pass its value to the underlying diff-index and\n> diff-files commands, but should we also have a command line parameter to\n> git-add to specify the diff algorithm in interactive mode?  And if so,\n> can we simply add \"--diff-algorithm\" to git-add, or is that too\n> confusing?\n\nMaking \"git add--interactive\" read from diff.algorithm is probably a\ngood idea, because the command itself definitely is a Porcelain.  We\nwould probably need a way to defeat the configured default for\ncompleteness, either:\n\n    git add -p --diff-algorithm=default\n    git -c diff.algorithm=default add -p\n\nbut I suspect that a new option to \"git add\" that only takes effect\ntogether with \"-p\" is probably an overkill, only in order to support\nthe former and not having to say the latter, but I can be persuaded\neither way.\n\nAs long as \"git add --diff-algorithm=foo\" without \"-i\" or \"-p\"\noption lets the user know it has no effect (error out, give warning\nand continue, etc. whose details I do not deeply care), that is.\n"},{"id":"220382","messageId":"20130610211140.GD13333@sigill.intra.peff.net","threadId":"34090","inReplyTo":"7v38sphiiw.fsf@alter.siamese.dyndns.org","subject":"Re: Different diff strategies in add --interactive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-10T21:11:41Z","receivedAt":"2013-06-10T21:11:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 10, 2013 at 12:28:55PM -0700, Junio C Hamano wrote:\n\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > I think the first thing to do is read the \"diff.algorithm\" setting in\n> > git-add--interactive and pass its value to the underlying diff-index and\n> > diff-files commands, but should we also have a command line parameter to\n> > git-add to specify the diff algorithm in interactive mode?  And if so,\n> > can we simply add \"--diff-algorithm\" to git-add, or is that too\n> > confusing?\n> \n> Making \"git add--interactive\" read from diff.algorithm is probably a\n> good idea, because the command itself definitely is a Porcelain.  We\n> would probably need a way to defeat the configured default for\n> completeness, either:\n> \n>     git add -p --diff-algorithm=default\n>     git -c diff.algorithm=default add -p\n> \n> but I suspect that a new option to \"git add\" that only takes effect\n> together with \"-p\" is probably an overkill, only in order to support\n> the former and not having to say the latter, but I can be persuaded\n> either way.\n\nWorse than that, you would need to add such an option to \"checkout -p\",\n\"reset -p\", \"stash -p\", etc. I think the latter form you suggest is\nprobably acceptable in this case.\n\nOverall, I think respecting diff.algorithm in add--interactive is a very\nsane thing to do. I would even be tempted to say we should allow a few\nother select diff options (e.g., fewer or more context lines). If you\nallowed diff options like this:\n\n  git add --patch=\"--patience -U5\"\n\nthat is very flexible, but I would not want to think about what the code\ndoes when you pass --patch=\"--raw\" or equal nonsense.\n\nBut I cannot off the top of my head think of other options besides -U\nthat would be helpful. I have never particularly wanted it for \"add -p\",\neither, though I sometimes generate patches to the list with a greater\nnumber of context lines when I think it makes the changes to a short\nfunction more readable.\n\n-Peff\n"},{"id":"220389","messageId":"20130610214638.GK22905@serenity.lan","threadId":"34090","inReplyTo":"20130610211140.GD13333@sigill.intra.peff.net","subject":"Re: Different diff strategies in add --interactive","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-10T21:46:38Z","receivedAt":"2013-06-10T21:46:38Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Jun 10, 2013 at 05:11:41PM -0400, Jeff King wrote:\n> On Mon, Jun 10, 2013 at 12:28:55PM -0700, Junio C Hamano wrote:\n> > John Keeping <john@keeping.me.uk> writes:\n> > \n> > > I think the first thing to do is read the \"diff.algorithm\" setting in\n> > > git-add--interactive and pass its value to the underlying diff-index and\n> > > diff-files commands, but should we also have a command line parameter to\n> > > git-add to specify the diff algorithm in interactive mode?  And if so,\n> > > can we simply add \"--diff-algorithm\" to git-add, or is that too\n> > > confusing?\n> > \n> > Making \"git add--interactive\" read from diff.algorithm is probably a\n> > good idea, because the command itself definitely is a Porcelain.  We\n> > would probably need a way to defeat the configured default for\n> > completeness, either:\n> > \n> >     git add -p --diff-algorithm=default\n> >     git -c diff.algorithm=default add -p\n> > \n> > but I suspect that a new option to \"git add\" that only takes effect\n> > together with \"-p\" is probably an overkill, only in order to support\n> > the former and not having to say the latter, but I can be persuaded\n> > either way.\n> \n> Worse than that, you would need to add such an option to \"checkout -p\",\n> \"reset -p\", \"stash -p\", etc. I think the latter form you suggest is\n> probably acceptable in this case.\n\nThat's what I'm planning to do at the moment, if anyone wants to extend\nit further in the future then that can be built on top.\n\n> Overall, I think respecting diff.algorithm in add--interactive is a very\n> sane thing to do. I would even be tempted to say we should allow a few\n> other select diff options (e.g., fewer or more context lines). If you\n> allowed diff options like this:\n> \n>   git add --patch=\"--patience -U5\"\n> \n> that is very flexible, but I would not want to think about what the code\n> does when you pass --patch=\"--raw\" or equal nonsense.\n\nAn alternative would be to permit them to be set from within the\ninteractive UI.  I'd find it quite useful to experiment with various\ndiff options when I encounter a hunk that isn't as easy to pick as I'd\nlike.  I expect it would be very hard to do that on a per-hunk basis,\nalthough per-file doesn't seem like it would be too hard.\n\nI don't intend to investigate that though - respecting diff.algorithm is\ngood enough for my usage.\n\n> But I cannot off the top of my head think of other options besides -U\n> that would be helpful. I have never particularly wanted it for \"add -p\",\n> either, though I sometimes generate patches to the list with a greater\n> number of context lines when I think it makes the changes to a short\n> function more readable.\n\n--function-context might also be useful, but that's in the same category\nas -U.\n\nThe patch I'm using is below.  I'm not sure how we can test this though;\nit seems fragile to invent a patch that appears different with different\ndiff algorithms.  Any suggestions?\n\n-- >8 --\n git-add--interactive.perl | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex d2c4ce6..0b0fac2 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -44,6 +44,8 @@ my ($diff_new_color) =\n \n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n+my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');\n+\n my $use_readkey = 0;\n my $use_termcap = 0;\n my %term_escapes;\n@@ -731,6 +733,9 @@ sub run_git_apply {\n sub parse_diff {\n \tmy ($path) = @_;\n \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n+\tif ($diff_algorithm ne \"default\") {\n+\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n+\t}\n \tif (defined $patch_mode_revision) {\n \t\tpush @diff_cmd, $patch_mode_revision;\n \t}\n-- \n1.8.3.779.g691e267\n"},{"id":"220393","messageId":"20130610215656.GA28345@sigill.intra.peff.net","threadId":"34090","inReplyTo":"20130610214638.GK22905@serenity.lan","subject":"Re: Different diff strategies in add --interactive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-10T21:56:56Z","receivedAt":"2013-06-10T21:56:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 10, 2013 at 10:46:38PM +0100, John Keeping wrote:\n\n> > Overall, I think respecting diff.algorithm in add--interactive is a very\n> > sane thing to do. I would even be tempted to say we should allow a few\n> > other select diff options (e.g., fewer or more context lines). If you\n> > allowed diff options like this:\n> > \n> >   git add --patch=\"--patience -U5\"\n> > \n> > that is very flexible, but I would not want to think about what the code\n> > does when you pass --patch=\"--raw\" or equal nonsense.\n> \n> An alternative would be to permit them to be set from within the\n> interactive UI.  I'd find it quite useful to experiment with various\n> diff options when I encounter a hunk that isn't as easy to pick as I'd\n> like.  I expect it would be very hard to do that on a per-hunk basis,\n> although per-file doesn't seem like it would be too hard.\n\nThat's an interesting idea, for a subset of options (e.g., \"increase\ncontext for this hunk\"). I suspect implementing it would be painful,\nthough, as you would have to re-run diff, and you have no guarantee of\ngetting the same set of hunks (e.g., the hunk might end up coalesced\nwith another).\n\n> I don't intend to investigate that though - respecting diff.algorithm is\n> good enough for my usage.\n\nI don't blame you. :)\n\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index d2c4ce6..0b0fac2 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -44,6 +44,8 @@ my ($diff_new_color) =\n>  \n>  my $normal_color = $repo->get_color(\"\", \"reset\");\n>  \n> +my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');\n> +\n>  my $use_readkey = 0;\n>  my $use_termcap = 0;\n>  my %term_escapes;\n> @@ -731,6 +733,9 @@ sub run_git_apply {\n>  sub parse_diff {\n>  \tmy ($path) = @_;\n>  \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n> +\tif ($diff_algorithm ne \"default\") {\n> +\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n> +\t}\n>  \tif (defined $patch_mode_revision) {\n>  \t\tpush @diff_cmd, $patch_mode_revision;\n\nYeah, that looks like the sane way to do it to me. As a perl style\nthing, I think the usual way of spelling 'default' is 'undef'. I.e.:\n\n  my $diff_algorithm = $repo->config('diff.algorithm');\n  ...\n  if (defined $diff_algorithm) {\n          push @diff_cmd, \"--diff-algorithm=$diff_algorithm\";\n  }\n\n-Peff\n"},{"id":"220657","messageId":"20130612184410.GB23890@serenity.lan","threadId":"34090","inReplyTo":"20130610215656.GA28345@sigill.intra.peff.net","subject":"[PATCH] add--interactive: respect diff.algorithm","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-12T18:44:10Z","receivedAt":"2013-06-12T18:44:10Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When staging hunks interactively it is sometimes useful to use an\nalternative diff algorithm which splits the changes into hunks in a more\nlogical manner.  This is not possible because the plumbing commands\ncalled by add--interactive ignore the \"diff.algorithm\" configuration\noption (as they should).\n\nSince add--interactive is a porcelain command it should respect this\nconfiguration variable.  To do this, make it read diff.algorithm and\npass its value to the underlying diff-index and diff-files invocations.\n\nAt this point, do not add options to \"git add\", \"git reset\" or \"git\ncheckout\" (all of which can call git-add--interactive).  If a user want\nto override the value on the command line they can use:\n\n\tgit -c diff.algorithm=$ALGO ...\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\nOn Mon, Jun 10, 2013 at 05:56:56PM -0400, Jeff King wrote:\n> On Mon, Jun 10, 2013 at 10:46:38PM +0100, John Keeping wrote:\n> \n> > > Overall, I think respecting diff.algorithm in add--interactive is a very\n> > > sane thing to do. I would even be tempted to say we should allow a few\n> > > other select diff options (e.g., fewer or more context lines). If you\n> > > allowed diff options like this:\n> > > \n> > >   git add --patch=\"--patience -U5\"\n> > > \n> > > that is very flexible, but I would not want to think about what the code\n> > > does when you pass --patch=\"--raw\" or equal nonsense.\n> > \n> > An alternative would be to permit them to be set from within the\n> > interactive UI.  I'd find it quite useful to experiment with various\n> > diff options when I encounter a hunk that isn't as easy to pick as I'd\n> > like.  I expect it would be very hard to do that on a per-hunk basis,\n> > although per-file doesn't seem like it would be too hard.\n> \n> That's an interesting idea, for a subset of options (e.g., \"increase\n> context for this hunk\"). I suspect implementing it would be painful,\n> though, as you would have to re-run diff, and you have no guarantee of\n> getting the same set of hunks (e.g., the hunk might end up coalesced\n> with another).\n\nI think you'd need to re-run the diff over the whole file and then skip\nhunks until you reach one that overlaps with the original hunk.  But I\nsuspect it would end up being quite a lot more complicated than that.\n\n> > diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> > index d2c4ce6..0b0fac2 100755\n> > --- a/git-add--interactive.perl\n> > +++ b/git-add--interactive.perl\n> > @@ -44,6 +44,8 @@ my ($diff_new_color) =\n> >  \n> >  my $normal_color = $repo->get_color(\"\", \"reset\");\n> >  \n> > +my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');\n> > +\n> >  my $use_readkey = 0;\n> >  my $use_termcap = 0;\n> >  my %term_escapes;\n> > @@ -731,6 +733,9 @@ sub run_git_apply {\n> >  sub parse_diff {\n> >  \tmy ($path) = @_;\n> >  \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n> > +\tif ($diff_algorithm ne \"default\") {\n> > +\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n> > +\t}\n> >  \tif (defined $patch_mode_revision) {\n> >  \t\tpush @diff_cmd, $patch_mode_revision;\n> \n> Yeah, that looks like the sane way to do it to me. As a perl style\n> thing, I think the usual way of spelling 'default' is 'undef'. I.e.:\n> \n>   my $diff_algorithm = $repo->config('diff.algorithm');\n>   ...\n>   if (defined $diff_algorithm) {\n>           push @diff_cmd, \"--diff-algorithm=$diff_algorithm\";\n>   }\n\nOK.  The \"default\" is actually \"the value that is equivalent to 'myers'\nfor diff.algorithm\" and I was originally going to add --diff-algorithm\nto the command line unconditionally.\n\nBut I think it's better to add it only when it has been specified so\nI've changed it as you suggest.\n\n\n git-add--interactive.perl | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex d2c4ce6..5310959 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -44,6 +44,8 @@ my ($diff_new_color) =\n \n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n+my $diff_algorithm = $repo->config('diff.algorithm');\n+\n my $use_readkey = 0;\n my $use_termcap = 0;\n my %term_escapes;\n@@ -731,6 +733,9 @@ sub run_git_apply {\n sub parse_diff {\n \tmy ($path) = @_;\n \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n+\tif (defined $diff_algorithm) {\n+\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n+\t}\n \tif (defined $patch_mode_revision) {\n \t\tpush @diff_cmd, $patch_mode_revision;\n \t}\n-- \n1.8.3.779.g691e267\n"},{"id":"220660","messageId":"20130612191817.GA4898@sigill.intra.peff.net","threadId":"34090","inReplyTo":"20130612184410.GB23890@serenity.lan","subject":"Re: [PATCH] add--interactive: respect diff.algorithm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-12T19:18:17Z","receivedAt":"2013-06-12T19:18:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 12, 2013 at 07:44:10PM +0100, John Keeping wrote:\n\n> When staging hunks interactively it is sometimes useful to use an\n> alternative diff algorithm which splits the changes into hunks in a more\n> logical manner.  This is not possible because the plumbing commands\n> called by add--interactive ignore the \"diff.algorithm\" configuration\n> option (as they should).\n> \n> Since add--interactive is a porcelain command it should respect this\n> configuration variable.  To do this, make it read diff.algorithm and\n> pass its value to the underlying diff-index and diff-files invocations.\n> \n> At this point, do not add options to \"git add\", \"git reset\" or \"git\n> checkout\" (all of which can call git-add--interactive).  If a user want\n\ns/want/wants/\n\n> >   if (defined $diff_algorithm) {\n> >           push @diff_cmd, \"--diff-algorithm=$diff_algorithm\";\n> >   }\n> \n> OK.  The \"default\" is actually \"the value that is equivalent to 'myers'\n> for diff.algorithm\" and I was originally going to add --diff-algorithm\n> to the command line unconditionally.\n\nYeah, that might have made sense, too, but the in-between (we know that\n\"default\" is a special token and don't pass it) does not to me.\n\nPatch looks obviously correct to me. Thanks.\n\n-Peff\n"},{"id":"221738","messageId":"7v8v20fxee.fsf@alter.siamese.dyndns.org","threadId":"34090","inReplyTo":"20130612184410.GB23890@serenity.lan","subject":"Re: [PATCH] add--interactive: respect diff.algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-23T19:19:05Z","receivedAt":"2013-06-23T19:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n>> > +my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');\n>> > +\n>> >  my $use_readkey = 0;\n>> >  my $use_termcap = 0;\n>> >  my %term_escapes;\n>> > @@ -731,6 +733,9 @@ sub run_git_apply {\n>> >  sub parse_diff {\n>> >  \tmy ($path) = @_;\n>> >  \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n>> > +\tif ($diff_algorithm ne \"default\") {\n>> > +\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n>> > +\t}\n\nThis is not exactly sanitary for \"stash -p\", whose DIFF element is\ndefined like so:\n\n\t'stash' => {\n\t\tDIFF => 'diff-index -p HEAD',\n\nand you will end up appending an option after a non-option argument,\n\nIt may happen to be accepted by the command line parser which is\noverly lax, but we would want to tighten it in the longer term.\n\nAs a band-aid, we could do something like the attached patch, but\nfor the longer term, we might need to rethink the way the tweaking\nof the command line is done by $patch_mode_revision.\n\n-- >8 --\nSubject: add -i: add extra options at the right place in \"diff\" command line\n\nAppending \"--diff-algorithm=histogram\" at the end of canned command\nline for various modes of \"diff\" is correct for most of them but not\nfor \"stash\" that has a non-option already wired in, like so:\n\n\t'stash' => {\n\t\tDIFF => 'diff-index -p HEAD',\n\nAppending an extra option after non-option may happen to work due to\noverly lax command line parser, but that is not something we should\nrely on.  Instead, splice in the extra argument immediately after a\n'-p' option, which is an option to ask for textual diff output that\nhas to be in all variants.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-add--interactive.perl | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 5310959..b50551a 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -730,11 +730,23 @@ sub run_git_apply {\n \treturn close $fh;\n }\n \n+# The command array must have a single \"-p\" to ask for output in the\n+# patch form.  Splice additional options immediately after it; we\n+# should not be randomly appending them, as some of the canned command.\n+# has non-option argument like HEAD already on it.\n+\n+sub splice_diff_options {\n+\tmy $diff_cmd = shift;\n+\t@$diff_cmd = map {\n+\t\t($_ eq '-p') ? ($_, @_) : $_;\t\t\n+\t} @$diff_cmd;\n+}\n+\n sub parse_diff {\n \tmy ($path) = @_;\n \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n \tif (defined $diff_algorithm) {\n-\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n+\t\tsplice_diff_options(\\@diff_cmd, \"--diff-algorithm=${diff_algorithm}\");\n \t}\n \tif (defined $patch_mode_revision) {\n \t\tpush @diff_cmd, $patch_mode_revision;\n"},{"id":"221739","messageId":"20130623195017.GI4676@serenity.lan","threadId":"34090","inReplyTo":"7v8v20fxee.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] add--interactive: respect diff.algorithm","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T19:50:18Z","receivedAt":"2013-06-23T19:50:18Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Jun 23, 2013 at 12:19:05PM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> >> > +my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');\n> >> > +\n> >> >  my $use_readkey = 0;\n> >> >  my $use_termcap = 0;\n> >> >  my %term_escapes;\n> >> > @@ -731,6 +733,9 @@ sub run_git_apply {\n> >> >  sub parse_diff {\n> >> >  \tmy ($path) = @_;\n> >> >  \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n> >> > +\tif ($diff_algorithm ne \"default\") {\n> >> > +\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n> >> > +\t}\n> \n> This is not exactly sanitary for \"stash -p\", whose DIFF element is\n> defined like so:\n> \n> \t'stash' => {\n> \t\tDIFF => 'diff-index -p HEAD',\n> \n> and you will end up appending an option after a non-option argument,\n> \n> It may happen to be accepted by the command line parser which is\n> overly lax, but we would want to tighten it in the longer term.\n> \n> As a band-aid, we could do something like the attached patch, but\n> for the longer term, we might need to rethink the way the tweaking\n> of the command line is done by $patch_mode_revision.\n\nI originally used splice here then for some reason decided that just\nappending the option was okay (I think I thought we were already\nappending an option with $patch_mode_revision).\n\nThe patch below involves deeper Perl magic than I fully grok, but\nwouldn't it be simpler to simply use the fact that the string is\n\"command --options...\" and use:\n\n    splice @diff_cmd 1, 0, \"--diff-algorithm=${diff_algorithm}\";\n\n> -- >8 --\n> Subject: add -i: add extra options at the right place in \"diff\" command line\n> \n> Appending \"--diff-algorithm=histogram\" at the end of canned command\n> line for various modes of \"diff\" is correct for most of them but not\n> for \"stash\" that has a non-option already wired in, like so:\n> \n> \t'stash' => {\n> \t\tDIFF => 'diff-index -p HEAD',\n> \n> Appending an extra option after non-option may happen to work due to\n> overly lax command line parser, but that is not something we should\n> rely on.  Instead, splice in the extra argument immediately after a\n> '-p' option, which is an option to ask for textual diff output that\n> has to be in all variants.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  git-add--interactive.perl | 14 +++++++++++++-\n>  1 file changed, 13 insertions(+), 1 deletion(-)\n> \n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 5310959..b50551a 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -730,11 +730,23 @@ sub run_git_apply {\n>  \treturn close $fh;\n>  }\n>  \n> +# The command array must have a single \"-p\" to ask for output in the\n> +# patch form.  Splice additional options immediately after it; we\n> +# should not be randomly appending them, as some of the canned command.\n> +# has non-option argument like HEAD already on it.\n> +\n> +sub splice_diff_options {\n> +\tmy $diff_cmd = shift;\n> +\t@$diff_cmd = map {\n> +\t\t($_ eq '-p') ? ($_, @_) : $_;\t\t\n> +\t} @$diff_cmd;\n> +}\n> +\n>  sub parse_diff {\n>  \tmy ($path) = @_;\n>  \tmy @diff_cmd = split(\" \", $patch_mode_flavour{DIFF});\n>  \tif (defined $diff_algorithm) {\n> -\t\tpush @diff_cmd, \"--diff-algorithm=${diff_algorithm}\";\n> +\t\tsplice_diff_options(\\@diff_cmd, \"--diff-algorithm=${diff_algorithm}\");\n>  \t}\n>  \tif (defined $patch_mode_revision) {\n>  \t\tpush @diff_cmd, $patch_mode_revision;\n"},{"id":"221740","messageId":"7v4ncoftsj.fsf@alter.siamese.dyndns.org","threadId":"34090","inReplyTo":"20130623195017.GI4676@serenity.lan","subject":"Re: [PATCH] add--interactive: respect diff.algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-23T20:37:00Z","receivedAt":"2013-06-23T20:37:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> The patch below involves deeper Perl magic than I fully grok, but\n> wouldn't it be simpler to simply use the fact that the string is\n> \"command --options...\" and use:\n>\n>     splice @diff_cmd 1, 0, \"--diff-algorithm=${diff_algorithm}\";\n\nThat inserts the extra options as the first thing on the command\nline, which is also fine.\n"}]}