{"thread":{"id":"45375","subject":"[PATCH] add--interactive: do not expand pathspecs with ls-files","startedAt":"2017-03-14T16:30:33Z","lastAt":"2017-03-14T22:09:45Z","messageCount":6,"participants":["Jeff King","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314034","messageId":"20170314163024.et2cp5e3imrrrf7k@sigill.intra.peff.net","threadId":"45375","inReplyTo":null,"subject":"[PATCH] add--interactive: do not expand pathspecs with ls-files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-14T16:30:24Z","receivedAt":"2017-03-14T16:30:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we want to get the list of modified files, we first\nexpand any user-provided pathspecs with \"ls-files\", and then\nfeed the resulting list of paths as arguments to\n\"diff-index\" and \"diff-files\". If your pathspec expands into\na large number of paths, you may run into one of two\nproblems:\n\n  1. The OS may complain about the size of the argument\n     list, and refuse to run. For example:\n\n       $ (ulimit -s 128 && git add -p drivers)\n       Can't exec \"git\": Argument list too long at .../git-add--interactive line 177.\n       Died at .../git-add--interactive line 177.\n\n     That's on the linux.git repository, which has about 20K\n     files in the \"drivers\" directory (none of them modified\n     in this case). The \"ulimit -s\" trick is necessary to\n     show the problem on Linux even for such a gigantic set\n     of paths. Other operating systems have much smaller\n     limits (e.g., a real-world case was seen with only 5K\n     files on OS X).\n\n  2. Even when it does work, it's really slow. The pathspec\n     code is not optimized for huge numbers of paths. Here's\n     the same case without the ulimit:\n\n       $ time git add -p drivers\n       No changes.\n\n       real\t0m16.559s\n       user\t0m53.140s\n       sys\t0m0.220s\n\nWe can improve this by skipping \"ls-files\" completely, and\njust feeding the original pathspecs to the diff commands.\nThis solution was discussed in 2010:\n\n  http://public-inbox.org/git/20100105041438.GB12574@coredump.intra.peff.net/\n\nbut at the time the diff code's pathspecs were more\nprimitive than those used by ls-files (e.g., they did not\nsupport globs). Making the change would have caused a\nuser-visible regression, so we didn't.\n\nSince then, the pathspec code has been unified, and the diff\ncommands natively understand pathspecs like '*.c'.\n\nThis patch implements that solution. That skips the\nargument-list limits, and the result runs much faster:\n\n  $ time git add -p drivers\n  No changes.\n\n  real\t0m0.149s\n  user\t0m0.116s\n  sys\t0m0.080s\n\nThere are two new tests. The first just exercises the\nglobbing behavior to confirm that we are not causing a\nregression there. The second checks the actual argument\nbehavior using GIT_TRACE. We _could_ do it with the \"ulimit\n-s\" trick, as above. But that would mean the test could only\nrun where \"ulimit -s\" works. And tests of that sort are\nexpensive, because we have to come up with enough files to\nactually bust the limit (we can't just shrink the \"128\" down\ninfinitely, since it is also the in-program stack size).\n\nFinally, two caveats and possibilities for future work:\n\n  a. This fixes one argument-list expansion, but there may\n     be others. In fact, it's very likely that if you run\n     \"git add -i\" and select a large number of modified\n     files that the script would try to feed them all to a\n     single git command.\n\n     In practice this is probably fine. The real issue here\n     is that the argument list was growing with the _total_\n     number of files, not the number of modified or selected\n     files.\n\n  b. If the repository contains filenames with literal wildcard\n     characters (e.g., \"foo*\"), the original code expanded\n     them via \"ls-files\" and then fed those wildcard names\n     to \"diff-index\", which would have treated them as\n     wildcards. This was a bug, which is now fixed (though\n     unless you really go through some contortions with\n     \":(literal)\", it's likely that your original pathspec\n     would match whatever the accidentally-expanded wildcard\n     would anyway).\n\n     So this takes us one step closer to working correctly\n     with files whose names contain wildcard characters, but\n     it's likely that others remain (e.g., if \"git add -i\"\n     feeds the selected paths to \"git add\").\n\nReported-by: Wincent Colaiuta <win@wincent.com>\nReported-by: Mislav Marohnić <mislav.marohnic@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI credited Wincent for reporting it in 2010. Hopefully his email still\nworks. :)\n\n git-add--interactive.perl  | 13 ++-----------\n t/t3701-add-interactive.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+), 11 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex f5c816e27..77b4ed53a 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -275,20 +275,11 @@ sub list_modified {\n \tmy ($only) = @_;\n \tmy (%data, @return);\n \tmy ($add, $del, $adddel, $file);\n-\tmy @tracked = ();\n-\n-\tif (@ARGV) {\n-\t\t@tracked = map {\n-\t\t\tchomp $_;\n-\t\t\tunquote_path($_);\n-\t\t} run_cmd_pipe(qw(git ls-files --), @ARGV);\n-\t\treturn if (!@tracked);\n-\t}\n \n \tmy $reference = get_diff_reference($patch_mode_revision);\n \tfor (run_cmd_pipe(qw(git diff-index --cached\n \t\t\t     --numstat --summary), $reference,\n-\t\t\t     '--', @tracked)) {\n+\t\t\t     '--', @ARGV)) {\n \t\tif (($add, $del, $file) =\n \t\t    /^([-\\d]+)\t([-\\d]+)\t(.*)/) {\n \t\t\tmy ($change, $bin);\n@@ -313,7 +304,7 @@ sub list_modified {\n \t\t}\n \t}\n \n-\tfor (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), @tracked)) {\n+\tfor (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), @ARGV)) {\n \t\tif (($add, $del, $file) =\n \t\t    /^([-\\d]+)\t([-\\d]+)\t(.*)/) {\n \t\t\t$file = unquote_path($file);\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex aaa258daa..f9528fa00 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -412,4 +412,47 @@ test_expect_success 'patch-mode via -i prompts for files' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add -p handles globs' '\n+\tgit reset --hard &&\n+\n+\tmkdir -p subdir &&\n+\techo base >one.c &&\n+\techo base >subdir/two.c &&\n+\tgit add \"*.c\" &&\n+\tgit commit -m base &&\n+\n+\techo change >one.c &&\n+\techo change >subdir/two.c &&\n+\tgit add -p \"*.c\" <<-\\EOF &&\n+\ty\n+\ty\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\tone.c\n+\tsubdir/two.c\n+\tEOF\n+\tgit diff --cached --name-only >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add -p does not expand argument lists' '\n+\tgit reset --hard &&\n+\n+\techo content >not-changed &&\n+\tgit add not-changed &&\n+\tgit commit -m \"add not-changed file\" &&\n+\n+\techo change >file &&\n+\tGIT_TRACE=$(pwd)/trace.out git add -p . <<-\\EOF &&\n+\ty\n+\tEOF\n+\n+\t# we know that \"file\" must be mentioned since we actually\n+\t# update it, but we want to be sure that our \".\" pathspec\n+\t# was not expanded into the argument list of any command.\n+\t# So look only for \"not-changed\".\n+\t! grep not-changed trace.out\n+'\n+\n test_done\n-- \n2.12.0.550.g163ff0384.dirty\n"},{"id":"314035","messageId":"20170314163156.qhfvdphqohbexm4h@sigill.intra.peff.net","threadId":"45375","inReplyTo":"20170314163024.et2cp5e3imrrrf7k@sigill.intra.peff.net","subject":"Re: [PATCH] add--interactive: do not expand pathspecs with ls-files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-14T16:31:56Z","receivedAt":"2017-03-14T16:32:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 14, 2017 at 12:30:24PM -0400, Jeff King wrote:\n\n>   2. Even when it does work, it's really slow. The pathspec\n>      code is not optimized for huge numbers of paths. Here's\n>      the same case without the ulimit:\n> \n>        $ time git add -p drivers\n>        No changes.\n> \n>        real\t0m16.559s\n>        user\t0m53.140s\n>        sys\t0m0.220s\n\nBy the way, I wondered how we managed to consume more CPU than\nwall-clock time. I think the answer is that diff-files does the pathspec\nmatching during the threaded preload stage.\n\n-Peff\n"},{"id":"314037","messageId":"20170314172504.GA102212@google.com","threadId":"45375","inReplyTo":"20170314163024.et2cp5e3imrrrf7k@sigill.intra.peff.net","subject":"Re: [PATCH] add--interactive: do not expand pathspecs with ls-files","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-14T17:25:04Z","receivedAt":"2017-03-14T17:25:13Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/14, Jeff King wrote:\n>   b. If the repository contains filenames with literal wildcard\n>      characters (e.g., \"foo*\"), the original code expanded\n>      them via \"ls-files\" and then fed those wildcard names\n>      to \"diff-index\", which would have treated them as\n>      wildcards. This was a bug, which is now fixed (though\n>      unless you really go through some contortions with\n>      \":(literal)\", it's likely that your original pathspec\n>      would match whatever the accidentally-expanded wildcard\n>      would anyway).\n> \n>      So this takes us one step closer to working correctly\n>      with files whose names contain wildcard characters, but\n>      it's likely that others remain (e.g., if \"git add -i\"\n>      feeds the selected paths to \"git add\").\n\nIt definitely makes things more difficult when people use wildcard\ncharacters in filenames.  Is there any reason people use wildcards as\nliteral characters in filenames other than them not knowing any better?\n\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index f5c816e27..77b4ed53a 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -275,20 +275,11 @@ sub list_modified {\n>  \tmy ($only) = @_;\n>  \tmy (%data, @return);\n>  \tmy ($add, $del, $adddel, $file);\n> -\tmy @tracked = ();\n> -\n> -\tif (@ARGV) {\n> -\t\t@tracked = map {\n> -\t\t\tchomp $_;\n> -\t\t\tunquote_path($_);\n> -\t\t} run_cmd_pipe(qw(git ls-files --), @ARGV);\n> -\t\treturn if (!@tracked);\n> -\t}\n>  \n>  \tmy $reference = get_diff_reference($patch_mode_revision);\n>  \tfor (run_cmd_pipe(qw(git diff-index --cached\n>  \t\t\t     --numstat --summary), $reference,\n> -\t\t\t     '--', @tracked)) {\n> +\t\t\t     '--', @ARGV)) {\n>  \t\tif (($add, $del, $file) =\n>  \t\t    /^([-\\d]+)\t([-\\d]+)\t(.*)/) {\n>  \t\t\tmy ($change, $bin);\n> @@ -313,7 +304,7 @@ sub list_modified {\n>  \t\t}\n>  \t}\n>  \n> -\tfor (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), @tracked)) {\n> +\tfor (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), @ARGV)) {\n\nSuch a small and easy change for that big of a perf win.\n\n>  \t\tif (($add, $del, $file) =\n>  \t\t    /^([-\\d]+)\t([-\\d]+)\t(.*)/) {\n>  \t\t\t$file = unquote_path($file);\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index aaa258daa..f9528fa00 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -412,4 +412,47 @@ test_expect_success 'patch-mode via -i prompts for files' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'add -p handles globs' '\n> +\tgit reset --hard &&\n> +\n> +\tmkdir -p subdir &&\n> +\techo base >one.c &&\n> +\techo base >subdir/two.c &&\n> +\tgit add \"*.c\" &&\n> +\tgit commit -m base &&\n> +\n> +\techo change >one.c &&\n> +\techo change >subdir/two.c &&\n> +\tgit add -p \"*.c\" <<-\\EOF &&\n> +\ty\n> +\ty\n> +\tEOF\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\tone.c\n> +\tsubdir/two.c\n> +\tEOF\n> +\tgit diff --cached --name-only >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'add -p does not expand argument lists' '\n> +\tgit reset --hard &&\n> +\n> +\techo content >not-changed &&\n> +\tgit add not-changed &&\n> +\tgit commit -m \"add not-changed file\" &&\n> +\n> +\techo change >file &&\n> +\tGIT_TRACE=$(pwd)/trace.out git add -p . <<-\\EOF &&\n> +\ty\n> +\tEOF\n> +\n> +\t# we know that \"file\" must be mentioned since we actually\n> +\t# update it, but we want to be sure that our \".\" pathspec\n> +\t# was not expanded into the argument list of any command.\n> +\t# So look only for \"not-changed\".\n> +\t! grep not-changed trace.out\n> +'\n> +\n>  test_done\n\nTests look sane to me.\n\n-- \nBrandon Williams\n"},{"id":"314038","messageId":"20170314173150.2triogunl4af4d6z@sigill.intra.peff.net","threadId":"45375","inReplyTo":"20170314172504.GA102212@google.com","subject":"Re: [PATCH] add--interactive: do not expand pathspecs with ls-files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-14T17:31:50Z","receivedAt":"2017-03-14T17:32:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 14, 2017 at 10:25:04AM -0700, Brandon Williams wrote:\n\n> On 03/14, Jeff King wrote:\n> >   b. If the repository contains filenames with literal wildcard\n> >      characters (e.g., \"foo*\"), the original code expanded\n> >      them via \"ls-files\" and then fed those wildcard names\n> >      to \"diff-index\", which would have treated them as\n> >      wildcards. This was a bug, which is now fixed (though\n> >      unless you really go through some contortions with\n> >      \":(literal)\", it's likely that your original pathspec\n> >      would match whatever the accidentally-expanded wildcard\n> >      would anyway).\n> > \n> >      So this takes us one step closer to working correctly\n> >      with files whose names contain wildcard characters, but\n> >      it's likely that others remain (e.g., if \"git add -i\"\n> >      feeds the selected paths to \"git add\").\n> \n> It definitely makes things more difficult when people use wildcard\n> characters in filenames.  Is there any reason people use wildcards as\n> literal characters in filenames other than them not knowing any better?\n\nThe philosophers of the ages have struggled with the question of why\nusers do anything.\n\nI suspect that wildcards in filenames are pretty uncommon, just judging\nfrom the lack of complaints. But they do come up. The most recent one\nwas:\n\n  http://public-inbox.org/git/c9876671-6252-5dfa-18df-a6719dc6834c@gmail.com/\n\nIt's unclear if that was stimulated by a real-world case, or just\nmucking around.\n\n-Peff\n"},{"id":"314087","messageId":"xmqqd1djh5xl.fsf@gitster.mtv.corp.google.com","threadId":"45375","inReplyTo":"20170314163024.et2cp5e3imrrrf7k@sigill.intra.peff.net","subject":"Re: [PATCH] add--interactive: do not expand pathspecs with ls-files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-14T22:03:34Z","receivedAt":"2017-03-14T22:03:42Z","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> We can improve this by skipping \"ls-files\" completely, and\n> just feeding the original pathspecs to the diff commands.\n> This solution was discussed in 2010:\n>\n>   http://public-inbox.org/git/20100105041438.GB12574@coredump.intra.peff.net/\n>\n> but at the time the diff code's pathspecs were more\n> primitive than those used by ls-files (e.g., they did not\n> support globs). Making the change would have caused a\n> user-visible regression, so we didn't.\n\nHeh.  The change and the reasoning are both obviously correct, but\nhow did you find this?  Do you have a pile of \"old patches that\nshould be resurrected when time is right\" and this was picked out of\nit, or did you see somebody else hit the same thing recently and\nthen went back to the archive?\n\n>   b. If the repository contains filenames with literal wildcard\n>      characters (e.g., \"foo*\"), the original code expanded\n>      them via \"ls-files\" and then fed those wildcard names\n>      to \"diff-index\", which would have treated them as\n>      wildcards. This was a bug, which is now fixed (though\n>      unless you really go through some contortions with\n>      \":(literal)\", it's likely that your original pathspec\n>      would match whatever the accidentally-expanded wildcard\n>      would anyway).\n>\n>      So this takes us one step closer to working correctly\n>      with files whose names contain wildcard characters, but\n>      it's likely that others remain (e.g., if \"git add -i\"\n>      feeds the selected paths to \"git add\").\n\nWe didn't run with --literal-pathspecs which was a bug, but I\nsuspect that it didn't exist back then ;-).\n"},{"id":"314089","messageId":"20170314220937.xg6isonertnykx4y@sigill.intra.peff.net","threadId":"45375","inReplyTo":"xmqqd1djh5xl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add--interactive: do not expand pathspecs with ls-files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-14T22:09:37Z","receivedAt":"2017-03-14T22:09:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 14, 2017 at 03:03:34PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We can improve this by skipping \"ls-files\" completely, and\n> > just feeding the original pathspecs to the diff commands.\n> > This solution was discussed in 2010:\n> >\n> >   http://public-inbox.org/git/20100105041438.GB12574@coredump.intra.peff.net/\n> >\n> > but at the time the diff code's pathspecs were more\n> > primitive than those used by ls-files (e.g., they did not\n> > support globs). Making the change would have caused a\n> > user-visible regression, so we didn't.\n> \n> Heh.  The change and the reasoning are both obviously correct, but\n> how did you find this?  Do you have a pile of \"old patches that\n> should be resurrected when time is right\" and this was picked out of\n> it, or did you see somebody else hit the same thing recently and\n> then went back to the archive?\n\nThe latter.  Mislav reported it to me off-list earlier today, and I\ngenerated the patch. But after scratching my head at the familiarity and\nespecially wondering if there was some reason to use \"ls-files\" here, I\ndug up the linked thread. The fact that the patches are identical is\njust coincidence (though it's such a simple change that it seems highly\nlikely).\n\n> >      So this takes us one step closer to working correctly\n> >      with files whose names contain wildcard characters, but\n> >      it's likely that others remain (e.g., if \"git add -i\"\n> >      feeds the selected paths to \"git add\").\n> \n> We didn't run with --literal-pathspecs which was a bug, but I\n> suspect that it didn't exist back then ;-).\n\nYep. I think judiciously inserting \"--literal-pathspecs\" is probably the\ncorrect fix. In an earlier thread (that I linked elsewhere from the\ndiscussion here) I suggested just setting $GIT_LITERAL_PATHSPECS to 1.\nBut that is probably a bad idea. As this patch shows, we do still\nsometimes feed the original non-expanded pathspec to some commands.\n\nI left that as potential low-hanging fruit for somebody who cares more\n(the trickiest part is probably not the fix, but coming up with a test\ncase).\n\n-Peff\n"}]}