{"thread":{"id":"42605","subject":"compactionHeuristic=true is not used by interactive staging","startedAt":"2016-06-16T06:55:52Z","lastAt":"2016-06-16T23:40:50Z","messageCount":7,"participants":["Alex Prengère","Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"289215","messageId":"CAEtHF9Na7yppEzuutp5=98KjLucgq=7Xv9GPidhaYEuC0oeqQQ@mail.gmail.com","threadId":"42605","inReplyTo":null,"subject":"compactionHeuristic=true is not used by interactive staging","fromName":"Alex Prengère","fromEmail":"alexprengere@gmail.com","sentAt":"2016-06-14T14:22:54Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":false,"sender":{"key":"alexprengere@gmail.com","avatar":null},"body":"Hello,\nI just did a fresh clone of git from Github and installed the version\n2.9.0 on Fedora 22.\n\nI tried the new compactionHeuristic = true, which is awesome.\nThe only thing that struck me was that this option was not used when\ndoing an interactive staging, meaning `git diff` and `git add -p` will\nformat patches differently. Perhaps this is intended and there is a\nway to force interactive staging to use specific diff options, but I\ndid not find it in the doc.\n\nThanks,\nAlex\n"},{"id":"289256","messageId":"20160614214211.GB21560@sigill.intra.peff.net","threadId":"42605","inReplyTo":"CAEtHF9Na7yppEzuutp5=98KjLucgq=7Xv9GPidhaYEuC0oeqQQ@mail.gmail.com","subject":"Re: compactionHeuristic=true is not used by interactive staging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-14T21:42:11Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 14, 2016 at 04:22:54PM +0200, Alex Prengère wrote:\n\n> Hello,\n> I just did a fresh clone of git from Github and installed the version\n> 2.9.0 on Fedora 22.\n> \n> I tried the new compactionHeuristic = true, which is awesome.\n> The only thing that struck me was that this option was not used when\n> doing an interactive staging, meaning `git diff` and `git add -p` will\n> format patches differently. Perhaps this is intended and there is a\n> way to force interactive staging to use specific diff options, but I\n> did not find it in the doc.\n\nThat's because it's handled in the \"UI config\", and plumbing commands\nare not affected (and \"add -p\" is built on plumbing commands). The same\nis true of diff.algorithm, for instance.\n\nTo make this work, add--interactive would have to manually enable\nparticular options that it thinks it can handle (and in fact this is\ndone with diff.algorithm already). So we'd need a patch similar to\n2cc0f53 (add--interactive: respect diff.algorithm, 2013-06-12).\n\nNobody noticed so far because originally the compaction heuristic was on\nby default, and so just worked everywhere. But we backed off on that at\nthe last minute after finding a few cases where the diff looks worse.\n\n-Peff\n"},{"id":"289258","messageId":"xmqq37of8mx4.fsf@gitster.mtv.corp.google.com","threadId":"42605","inReplyTo":"20160614214211.GB21560@sigill.intra.peff.net","subject":"Re: compactionHeuristic=true is not used by interactive staging","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-14T21:45:27Z","receivedAt":"2016-06-16T06:55:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Nobody noticed so far because originally the compaction heuristic was on\n> by default, and so just worked everywhere. But we backed off on that at\n> the last minute after finding a few cases where the diff looks worse.\n\nYup, and that is why this is called \"experimental\" in the release\nnotes ;-)\n"},{"id":"289277","messageId":"CAEtHF9Ne7a25cOF_6L=ZXRnmR4NbnUBDFR+6V-PryjtK8yNO8g@mail.gmail.com","threadId":"42605","inReplyTo":"xmqq37of8mx4.fsf@gitster.mtv.corp.google.com","subject":"Re: compactionHeuristic=true is not used by interactive staging","fromName":"Alex Prengère","fromEmail":"alexprengere@gmail.com","sentAt":"2016-06-15T06:24:47Z","receivedAt":"2016-06-16T06:55:53Z","isPatch":false,"sender":{"key":"alexprengere@gmail.com","avatar":null},"body":"I see, it makes sense ;-) Indeed it would seem logical to have all\ncommands showing diffs (diff, add -p, log -p, reset -p, etc..) respect\nthe diff options.\n\nThanks for your quick answer!\n\n2016-06-14 23:45 GMT+02:00 Junio C Hamano :\n> Jeff King writes:\n>\n>> Nobody noticed so far because originally the compaction heuristic was on\n>> by default, and so just worked everywhere. But we backed off on that at\n>> the last minute after finding a few cases where the diff looks worse.\n>\n> Yup, and that is why this is called \"experimental\" in the release\n> notes ;-)\n"},{"id":"289359","messageId":"20160616122729.GC15988@sigill.intra.peff.net","threadId":"42605","inReplyTo":"CAEtHF9Ne7a25cOF_6L=ZXRnmR4NbnUBDFR+6V-PryjtK8yNO8g@mail.gmail.com","subject":"[PATCH] add--interactive: respect diff.compactionHeuristic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T12:27:29Z","receivedAt":"2016-06-16T12:27:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 15, 2016 at 08:24:47AM +0200, Alex Prengère wrote:\n\n> I see, it makes sense ;-) Indeed it would seem logical to have all\n> commands showing diffs (diff, add -p, log -p, reset -p, etc..) respect\n> the diff options.\n\nHere's a patch to do so, similar to what we do for diff.algorithm.\n\n-- >8 --\nSubject: add--interactive: respect diff.compactionHeuristic\n\nWe use plumbing to generate the diff, so it doesn't\nautomatically pick up UI config like compactionHeuristic.\nLet's forward it on, since interactive adding is porcelain.\n\nNote that we only need to handle the \"true\" case. There's no\npoint in passing --no-compaction-heuristic when the variable\nis false, since nothing else could have turned it on.\n\nSigned-off-by: Jeff King \n---\n git-add--interactive.perl | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 822f857..642cce1 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -45,6 +45,7 @@ my ($diff_new_color) =\n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n my $diff_algorithm = $repo->config('diff.algorithm');\n+my $diff_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');\n my $diff_filter = $repo->config('interactive.difffilter');\n \n my $use_readkey = 0;\n@@ -749,6 +750,9 @@ sub parse_diff {\n \tif (defined $diff_algorithm) {\n \t\tsplice @diff_cmd, 1, 0, \"--diff-algorithm=${diff_algorithm}\";\n \t}\n+\tif ($diff_compaction_heuristic) {\n+\t\tsplice @diff_cmd, 1, 0, \"--compaction-heuristic\";\n+\t}\n \tif (defined $patch_mode_revision) {\n \t\tpush @diff_cmd, get_diff_reference($patch_mode_revision);\n \t}\n-- \n2.9.0.160.g4984cba\n\n"},{"id":"289369","messageId":"CAGZ79kaPVFT=yeSAkmf55+qXcCgMkwpgkC8jXhuZvqj5mWtgLw@mail.gmail.com","threadId":"42605","inReplyTo":"20160616122729.GC15988@sigill.intra.peff.net","subject":"Re: [PATCH] add--interactive: respect diff.compactionHeuristic","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-16T16:50:45Z","receivedAt":"2016-06-16T16:51:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 16, 2016 at 5:27 AM, Jeff King wrote:\n> On Wed, Jun 15, 2016 at 08:24:47AM +0200, Alex Prengère wrote:\n>\n>> I see, it makes sense ;-) Indeed it would seem logical to have all\n>> commands showing diffs (diff, add -p, log -p, reset -p, etc..) respect\n>> the diff options.\n>\n> Here's a patch to do so, similar to what we do for diff.algorithm.\n>\n> -- >8 --\n> Subject: add--interactive: respect diff.compactionHeuristic\n>\n> We use plumbing to generate the diff, so it doesn't\n> automatically pick up UI config like compactionHeuristic.\n> Let's forward it on, since interactive adding is porcelain.\n>\n> Note that we only need to handle the \"true\" case. There's no\n> point in passing --no-compaction-heuristic when the variable\n> is false, since nothing else could have turned it on.\n\nbecause we don't want to implement --[no-]compaction-heuristic\nas a command line switch to git-add?\nFine with me.\n\nStepping back and looking how the compaction heuristic turned out,\nI think this is what we did not want to see, i.e. the need to bring it in\nevery command, but rather enable and release it. But we backed off\nof the default-on, and now people may ask for the --no-compaction-heuristic\nin interactive add eventually, when they run into a corner case.\n\nFor now:\nReviewed-by: Stefan Beller \n\nThanks,\nStefan\n\n\n\n>\n> Signed-off-by: Jeff King \n> ---\n> git-add--interactive.perl | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 822f857..642cce1 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -45,6 +45,7 @@ my ($diff_new_color) =\n> my $normal_color = $repo->get_color(\"\", \"reset\");\n>\n> my $diff_algorithm = $repo->config('diff.algorithm');\n> +my $diff_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');\n> my $diff_filter = $repo->config('interactive.difffilter');\n>\n> my $use_readkey = 0;\n> @@ -749,6 +750,9 @@ sub parse_diff {\n> if (defined $diff_algorithm) {\n> splice @diff_cmd, 1, 0, \"--diff-algorithm=${diff_algorithm}\";\n> }\n> + if ($diff_compaction_heuristic) {\n> + splice @diff_cmd, 1, 0, \"--compaction-heuristic\";\n> + }\n> if (defined $patch_mode_revision) {\n> push @diff_cmd, get_diff_reference($patch_mode_revision);\n> }\n> --\n> 2.9.0.160.g4984cba\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"289415","messageId":"20160616234043.GC15013@sigill.intra.peff.net","threadId":"42605","inReplyTo":"CAGZ79kaPVFT=yeSAkmf55+qXcCgMkwpgkC8jXhuZvqj5mWtgLw@mail.gmail.com","subject":"Re: [PATCH] add--interactive: respect diff.compactionHeuristic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T23:40:43Z","receivedAt":"2016-06-16T23:40:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2016 at 09:50:45AM -0700, Stefan Beller wrote:\n\n> > -- >8 --\n> > Subject: add--interactive: respect diff.compactionHeuristic\n> >\n> > We use plumbing to generate the diff, so it doesn't\n> > automatically pick up UI config like compactionHeuristic.\n> > Let's forward it on, since interactive adding is porcelain.\n> >\n> > Note that we only need to handle the \"true\" case. There's no\n> > point in passing --no-compaction-heuristic when the variable\n> > is false, since nothing else could have turned it on.\n> \n> because we don't want to implement --[no-]compaction-heuristic\n> as a command line switch to git-add?\n> Fine with me.\n\nWe could, but I don't think it is worth the effort (and anyway, it would\noverride the config :) ).\n\n> Stepping back and looking how the compaction heuristic turned out,\n> I think this is what we did not want to see, i.e. the need to bring it in\n> every command, but rather enable and release it. But we backed off\n> of the default-on, and now people may ask for the --no-compaction-heuristic\n> in interactive add eventually, when they run into a corner case.\n\nYeah, I'm not excited to be plumbing it through, especially if we just\nend up flipping it on by default. But perhaps people would still want to\nbe able to do the opposite (turning it off for a specific case via the\nconfig). I dunno.\n\n> For now:\n> Reviewed-by: Stefan Beller \n\nThanks.\n\n-Peff\n"}]}