{"thread":{"id":"27900","subject":"Possible bug","startedAt":"2011-07-23T20:25:59Z","lastAt":"2011-09-08T16:51:53Z","messageCount":37,"participants":["Reuben Thomas","Michael J Gruber","Junio C Hamano","John Szakmeister","Erik Faye-Lund","Clemens Buchacher"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"171901","messageId":"CAOnWdohKfwEOMx=wr_PKiW+ucYBK2ZWykm_7dqr7hy4xGRM02A@mail.gmail.com","threadId":"27900","inReplyTo":null,"subject":"Possible bug","fromName":"Reuben Thomas","fromEmail":"rrt@sc3d.org","sentAt":"2011-07-23T20:25:59Z","receivedAt":"2011-07-23T20:25:59Z","isPatch":false,"sender":{"key":"rrt@sc3d.org","avatar":"https://gravatar.com/avatar/f1d2fd8cb53a9a1876636e095ed6c2e4f8ed357692bc1250cb91fdda23a2ac0a?d=mp&s=160"},"body":"Observe the following exchange with git 1.7.4.1, which I found rather\nperplexing:\n\n$ cat ~/reportbug-git\n$ git rm -f ../INSTALL\nrm 'INSTALL'\n$ git ci -m \"INSTALL is now provided by gnulib.\" ../INSTALL\nerror: pathspec 'ALL' did not match any file(s) known to git.\n$ cd ..\n$ git ci -m \"INSTALL is now provided by gnulib.\" INSTALL\n[master 0895314] INSTALL is now provided by gnulib.\n 1 files changed, 0 insertions(+), 1 deletions(-)\n delete mode 120000 INSTALL\n\nIs this a bug, or merely some magic I don't know about?\n\n-- \nhttp://rrt.sc3d.org\n"},{"id":"171977","messageId":"19b908142567fcfafe4da3d0fd60b134d30c613a.1311579663.git.git@drmicha.warpmail.net","threadId":"27900","inReplyTo":"CAOnWdohKfwEOMx=wr_PKiW+ucYBK2ZWykm_7dqr7hy4xGRM02A@mail.gmail.com","subject":"[RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-07-25T07:42:10Z","receivedAt":"2011-07-25T07:42:10Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In order to do partial commits, git-commit overlays a tree on the\ncache and checks pathspecs against the result. Currently, the overlaying\nis done using \"prefix\" which prevents relative pathspecs with \"..\" and\nabsolute pathspec from matching when they refer to files not under\n\"prefix\" and absent from the index, but still in the tree (i.e. files\nstaged for removal).\n\nOverlay the full tree instead.\n\nReported-by: Reuben Thomas \nSigned-off-by: Michael J Gruber \n---\nRFC because lack of test, and also because I'm not sure we want this, and\nwhat to do about git add which has the same problem, but would need a\ndifferent fix.\n---\n builtin/commit.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e1af9b1..431590c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -257,7 +257,7 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \tm = xcalloc(1, i);\n \n \tif (with_tree)\n-\t\toverlay_tree_on_cache(with_tree, prefix);\n+\t\toverlay_tree_on_cache(with_tree, NULL);\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n-- \n1.7.6.336.gdf067\n"},{"id":"172002","messageId":"7v8vrmrxok.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"19b908142567fcfafe4da3d0fd60b134d30c613a.1311579663.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-25T19:02:19Z","receivedAt":"2011-07-25T19:02:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> RFC because lack of test, and also because I'm not sure we want this, and\n> what to do about git add which has the same problem, but would need a\n> different fix.\n\nThe reason you doubt we would want *this* is...? Also what is the \"same\nproblem\"?\n\nPerhaps it would become clearer if you supported *this* with a sample\nworkflow?\n"},{"id":"172003","messageId":"7v4o2arwaa.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"7v8vrmrxok.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-25T19:32:29Z","receivedAt":"2011-07-25T19:32:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael J Gruber writes:\n>\n>> RFC because lack of test, and also because I'm not sure we want this, and\n>> what to do about git add which has the same problem, but would need a\n>> different fix.\n>\n> The reason you doubt we would want *this* is...? Also what is the \"same\n> problem\"?\n>\n> Perhaps it would become clearer if you supported *this* with a sample\n> workflow?\n\nHaving said that, I think your change to avoid restricting the overlay to\nthe current subdirectory makes sense, as long as we allow paths outside\nthe current subdirectory to be given to the command. I suspect that this\ncall was relied on an earlier behaviour to error out when paths outside\nthe current directory were given to the command.\n\nAlthough I do not know offhand if there are other places in the codepath\nwith the same issue (i.e. for whatever reason they do not expect to be fed\na relative path, and were relying on the early abort), I think what\nyour tries to do is taking us in the right direction.\n"},{"id":"172139","messageId":"4E2FCAC4.7020408@drmicha.warpmail.net","threadId":"27900","inReplyTo":"7v8vrmrxok.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-07-27T08:22:28Z","receivedAt":"2011-07-27T08:22:28Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:\n> Michael J Gruber writes:\n> \n>> RFC because lack of test, and also because I'm not sure we want this, and\n>> what to do about git add which has the same problem, but would need a\n>> different fix.\n> \n> The reason you doubt we would want *this* is...?\n\nI'm not sure the patch has side effects; I'm not sure we want to change\nexisting behaviour. I.e., is this behaviour intentional or a bug?\n\n> Also what is the \"same\n> problem\"?\n\nThe one reported by the OP for commit:\n\ngit rm ../a\ngit commit -m \"blurb\" ../a\nerror: pathspec '../a' did not match any file(s) known to git.\n\nhas the obvious analogue for add (add is going on behind the scenes of\nthe above commit, although we don't call the add codepath):\n\ngit rm ../a\ngit add ../a\nfatal: pathspec 'a' did not match any files\n\nRestaging a staged change should be a noop, shouldn't it?\n\nThe difference is that \"git add a\" does not work from the root directory\neither after the removal of a has been staged. That's why we can leave\nit as is. \"commit\", otoh, clearly behaves differently (depending on\nsubdir or root dir).\n\nBTW: Note how different our messages are.\n\n> Perhaps it would become clearer if you supported *this* with a sample\n> workflow?\n\nWell, the workflow is that described by the OP. It could go into the\ncommit message of an actual non-RFC patch.\n\nMichael\n"},{"id":"172147","messageId":"CAOnWdojgxZNs4qyEG55URScqOGb6k5e6Mh_B3By6Lj6SpN9L9g@mail.gmail.com","threadId":"27900","inReplyTo":"4E2FCAC4.7020408@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Reuben Thomas","fromEmail":"rrt@sc3d.org","sentAt":"2011-07-27T09:45:28Z","receivedAt":"2011-07-27T09:45:28Z","isPatch":true,"sender":{"key":"rrt@sc3d.org","avatar":"https://gravatar.com/avatar/f1d2fd8cb53a9a1876636e095ed6c2e4f8ed357692bc1250cb91fdda23a2ac0a?d=mp&s=160"},"body":"On 27 July 2011 09:22, Michael J Gruber wrote:\n> Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:\n>> Michael J Gruber writes:\n>>\n>> Also what is the \"same\n>> problem\"?\n>\n> The one reported by the OP for commit:\n>\n> git rm ../a\n> git commit -m \"blurb\" ../a\n> error: pathspec '../a' did not match any file(s) known to git.\n\nActually, this is not what I reported. This explains why I couldn't\nquite understand the direction this thread took. Look again:\n\n$ cat ~/reportbug-git\n$ git rm -f ../INSTALL\nrm 'INSTALL'\n$ git ci -m \"INSTALL is now provided by gnulib.\" ../INSTALL\nerror: pathspec 'ALL' did not match any file(s) known to git.\n$ cd ..\n$ git ci -m \"INSTALL is now provided by gnulib.\" INSTALL\n[master 0895314] INSTALL is now provided by gnulib.\n 1 files changed, 0 insertions(+), 1 deletions(-)\n delete mode 120000 INSTALL\n\nThe thing I didn't understand is that I tried to check in ../INSTALL,\nbut git complained about pathspec 'ALL'.\n\n-- \nhttp://rrt.sc3d.org\n"},{"id":"172150","messageId":"4E2FE024.2030204@drmicha.warpmail.net","threadId":"27900","inReplyTo":"CAOnWdojgxZNs4qyEG55URScqOGb6k5e6Mh_B3By6Lj6SpN9L9g@mail.gmail.com","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-07-27T09:53:40Z","receivedAt":"2011-07-27T09:53:40Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Reuben Thomas venit, vidit, dixit 27.07.2011 11:45:\n> On 27 July 2011 09:22, Michael J Gruber wrote:\n>> Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:\n>>> Michael J Gruber writes:\n>>>\n>>> Also what is the \"same\n>>> problem\"?\n>>\n>> The one reported by the OP for commit:\n>>\n>> git rm ../a\n>> git commit -m \"blurb\" ../a\n>> error: pathspec '../a' did not match any file(s) known to git.\n> \n> Actually, this is not what I reported. This explains why I couldn't\n> quite understand the direction this thread took. Look again:\n> \n> $ cat ~/reportbug-git\n> $ git rm -f ../INSTALL\n> rm 'INSTALL'\n> $ git ci -m \"INSTALL is now provided by gnulib.\" ../INSTALL\n> error: pathspec 'ALL' did not match any file(s) known to git.\n> $ cd ..\n> $ git ci -m \"INSTALL is now provided by gnulib.\" INSTALL\n> [master 0895314] INSTALL is now provided by gnulib.\n> 1 files changed, 0 insertions(+), 1 deletions(-)\n> delete mode 120000 INSTALL\n> \n> The thing I didn't understand is that I tried to check in ../INSTALL,\n> but git complained about pathspec 'ALL'.\n\nWell, you didn't say so, did you?\n\nAlso, we don't know what subdir you were doing this in, but I bet it has\na 7-character name...\n\nI can't reproduce that name truncation with a current git, btw. (So I\nwon't bother bisecting where we removed a spurious offset by \"prefix\".)\n\nIn any case, I still think git should allow partial commits with staged\ndeletions from within a subdir, which is what my patch is about.\n\nMichael\n"},{"id":"172152","messageId":"CAOnWdog-FqOJJ7nFFW-Sj9U5obVWgVdww5yFUy5uWJW8fw4svA@mail.gmail.com","threadId":"27900","inReplyTo":"4E2FE024.2030204@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Reuben Thomas","fromEmail":"rrt@sc3d.org","sentAt":"2011-07-27T10:00:51Z","receivedAt":"2011-07-27T10:00:51Z","isPatch":true,"sender":{"key":"rrt@sc3d.org","avatar":"https://gravatar.com/avatar/f1d2fd8cb53a9a1876636e095ed6c2e4f8ed357692bc1250cb91fdda23a2ac0a?d=mp&s=160"},"body":"On 27 July 2011 10:53, Michael J Gruber wrote:\n> Reuben Thomas venit, vidit, dixit 27.07.2011 11:45:\n>>\n>> $ cat ~/reportbug-git\n>> $ git rm -f ../INSTALL\n>> rm 'INSTALL'\n>> $ git ci -m \"INSTALL is now provided by gnulib.\" ../INSTALL\n>> error: pathspec 'ALL' did not match any file(s) known to git.\n>> $ cd ..\n>> $ git ci -m \"INSTALL is now provided by gnulib.\" INSTALL\n>> [master 0895314] INSTALL is now provided by gnulib.\n>>  1 files changed, 0 insertions(+), 1 deletions(-)\n>>  delete mode 120000 INSTALL\n>>\n>> The thing I didn't understand is that I tried to check in ../INSTALL,\n>> but git complained about pathspec 'ALL'.\n>\n> Well, you didn't say so, did you?\n>\n> Also, we don't know what subdir you were doing this in, but I bet it has\n> a 7-character name...\n\nIts name was \"src\".\n\n> I can't reproduce that name truncation with a current git, btw.\n\nThanks for testing that.\n\n> In any case, I still think git should allow partial commits with staged\n> deletions from within a subdir, which is what my patch is about.\n\nAbsolutely, this looks useful.\n\n-- \nhttp://rrt.sc3d.org\n"},{"id":"172154","messageId":"CAEBDL5XJbYT9yczZiVPpeA3mpROuKQdw+-AOJjUmRp4W6M5uzg@mail.gmail.com","threadId":"27900","inReplyTo":"4E2FE024.2030204@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2011-07-27T10:19:45Z","receivedAt":"2011-07-27T10:19:45Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber\n wrote:\n[snip]\n> I can't reproduce that name truncation with a current git, btw. (So I\n> won't bother bisecting where we removed a spurious offset by \"prefix\".)\n\nI can:\n\n:: git --version\ngit version 1.7.6.347.g4db0d\n:: git init reproduce-bug\nInitialized empty Git repository in /Users/jszakmeister/tmp/reproduce-bug/.git/\n:: cd reproduce-bug\n:: echo foo > foo.txt\n:: mkdir bar\n:: echo bar bar/bar.txt\nbar bar/bar.txt\n:: git add .\n:: git commit -m '.'\n[master (root-commit) a5f76f1] .\n 2 files changed, 2 insertions(+), 0 deletions(-)\n create mode 100644 bar/bar.txt\n create mode 100644 foo.txt\n:: cd bar\n:: git rm ../foo.txt\nrm 'foo.txt'\n:: git commit ../foo.txt\nerror: pathspec 'txt' did not match any file(s) known to git.\n\n-John\n"},{"id":"172156","messageId":"b535c21e1becdf8aeeb1d3f8ddaa7f5415830844.1311767679.git.git@drmicha.warpmail.net","threadId":"27900","inReplyTo":"CAEBDL5XJbYT9yczZiVPpeA3mpROuKQdw+-AOJjUmRp4W6M5uzg@mail.gmail.com","subject":"[RFC/PATCH] ls-files: fix pathspec display on error","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-07-27T11:56:14Z","receivedAt":"2011-07-27T11:56:14Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Trying to commit or ls a file \"../$a\" from within a subdirectory \"aha\"\ngives the following output, when \"$a\" runs through the values \"a\", \"aa\"\netc.:\n\nerror: pathspec '../a' did not match any file(s) known to git.\nerror: pathspec '../aa' did not match any file(s) known to git.\nerror: pathspec '../aaa' did not match any file(s) known to git.\nerror: pathspec '' did not match any file(s) known to git.\nerror: pathspec 'a' did not match any file(s) known to git.\nerror: pathspec 'aa' did not match any file(s) known to git.\nerror: pathspec 'aaa' did not match any file(s) known to git.\nerror: pathspec 'aaaa' did not match any file(s) known to git.\nerror: pathspec 'aaaaa' did not match any file(s) known to git.\nerror: pathspec 'aaaaaa' did not match any file(s) known to git.\n\nThis comes from the fact that report_path_error() tries to chop off a\nprefix from the pathspec which may have been resolved and consumed already.\n\nFix this by displaying the full pathspec.\n\nSigned-off-by: Michael J Gruber \n---\nSo, I couldn't reproduce because it depends on the length of the dirname.\nThis may be fallout from efad1a5 (ls-files: allow relative pathspec, 2010-06-03)\nbut I haven't checked and won't be able to for the next 3 weeks, sorry.\nI just wanted to send out this possible fix before I leave.\n---\n builtin/ls-files.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 1570123..e0b1401 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -418,7 +418,7 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_\n \t\t\tcontinue;\n \n \t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n-\t\t pathspec[num] + prefix_len);\n+\t\t pathspec[num]);\n \t\terrors++;\n \t}\n \treturn errors;\n-- \n1.7.6.336.gdf067\n"},{"id":"172165","messageId":"7vfwlrpwea.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"4E2FCAC4.7020408@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-27T15:37:33Z","receivedAt":"2011-07-27T15:37:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> git rm ../a\n> git add ../a\n> fatal: pathspec 'a' did not match any files\n\nIf you didn't have that ../a from the beginning, wouldn't you get the same\nerror message? Or instead of \"git add ../a\", you did \"(cd .. && git add a)\"\nwhat message do (and more importantly \"should\") you get?\n\nI think it would be an improvement if we said \"../a\" in the message, but\notherwise I agree with you that there is no bug here.\n\nA side note. If this didn't work:\n\n $ git rm --cached ../a\n $ git add ../a\n\nI would suspect that would be a bug (this however seems to work).\n"},{"id":"172166","messageId":"7vbowfpw8c.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"4E2FCAC4.7020408@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-27T15:41:07Z","receivedAt":"2011-07-27T15:41:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> The one reported by the OP for commit:\n>\n> git rm ../a\n> git commit -m \"blurb\" ../a\n> error: pathspec '../a' did not match any file(s) known to git.\n\nThat does look like a bug given that (starting from a state where that\n\"../a\" is a tracked file that appear in the HEAD commit):\n\n\tgit rm ../a\n (cd .. && git commit -m \"removed a\" a)\n\ndoes work. For \"git add ../a\" after removing the path from both index and\nthe working tree, see my other message.\n"},{"id":"172170","messageId":"4E3033DC.6020103@drmicha.warpmail.net","threadId":"27900","inReplyTo":"7vbowfpw8c.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-07-27T15:50:52Z","receivedAt":"2011-07-27T15:50:52Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 27.07.2011 17:41:\n> Michael J Gruber writes:\n> \n>> The one reported by the OP for commit:\n>>\n>> git rm ../a\n>> git commit -m \"blurb\" ../a\n>> error: pathspec '../a' did not match any file(s) known to git.\n> \n> That does look like a bug given that (starting from a state where that\n> \"../a\" is a tracked file that appear in the HEAD commit):\n> \n> \tgit rm ../a\n> (cd .. && git commit -m \"removed a\" a)\n> \n> does work. For \"git add ../a\" after removing the path from both index and\n> the working tree, see my other message.\n\nFull agreement on both.\nNow I really gotta run, sorry. But \"I'll be back\" :)\n\nMichael\n"},{"id":"172238","messageId":"CABPQNSb_k8Ye1zaD6rJcfXPT1_pOU9NBG_Z7pFDgNqtUyL4GUQ@mail.gmail.com","threadId":"27900","inReplyTo":"CAEBDL5XJbYT9yczZiVPpeA3mpROuKQdw+-AOJjUmRp4W6M5uzg@mail.gmail.com","subject":"Re: [RFC/PATCH] commit: allow partial commits with relative paths","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-28T07:38:05Z","receivedAt":"2011-07-28T07:38:05Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Jul 27, 2011 at 12:19 PM, John Szakmeister wrote:\n> On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber\n> wrote:\n> [snip]\n>> I can't reproduce that name truncation with a current git, btw. (So I\n>> won't bother bisecting where we removed a spurious offset by \"prefix\".)\n>\n> I can:\n>\n> :: git --version\n> git version 1.7.6.347.g4db0d\n> :: git init reproduce-bug\n> Initialized empty Git repository in /Users/jszakmeister/tmp/reproduce-bug/.git/\n> :: cd reproduce-bug\n> :: echo foo > foo.txt\n> :: mkdir bar\n> :: echo bar bar/bar.txt\n> bar bar/bar.txt\n> :: git add .\n> :: git commit -m '.'\n> [master (root-commit) a5f76f1] .\n>  2 files changed, 2 insertions(+), 0 deletions(-)\n>  create mode 100644 bar/bar.txt\n>  create mode 100644 foo.txt\n> :: cd bar\n> :: git rm ../foo.txt\n> rm 'foo.txt'\n> :: git commit ../foo.txt\n> error: pathspec 'txt' did not match any file(s) known to git.\n\nWhat seems to happen here, is report_path_error gets called with a\npathspec that doesn't contain the prefix (which is \"bar/\"), yet it\ntries to skip prefix_len characters ahead in it when reporting.\n\nWhy the pathspec isn't prefixed by \"bar/\" is because of the\nnormalize_path_copy-call in prefix_path, when called through\nget_pathspec(\"bar/\", {\"../foo.txt\", NULL}).\n"},{"id":"172340","messageId":"20110729130330.GA31941@toss.lan","threadId":"27900","inReplyTo":"b535c21e1becdf8aeeb1d3f8ddaa7f5415830844.1311767679.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] ls-files: fix pathspec display on error","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-07-29T13:03:30Z","receivedAt":"2011-07-29T13:03:30Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The following sequence of commands reveals an issue with error\nreporting of relative paths:\n\n $ mkdir sub\n $ cd sub\n $ git ls-files --error-unmatch ../bbbbb\n error: pathspec 'b' did not match any file(s) known to git.\n $ git commit --error-unmatch ../bbbbb\n error: pathspec 'b' did not match any file(s) known to git.\n\nThis bug is visible only if the normalized path (i.e., the relative\npath from the repository root) is longer than the prefix.\nOtherwise, the code skips over the normalized path and reads from\nan unused memory location which still contains a leftover of the\noriginal command line argument.\n\nSo instead, use the existing facilities to deal with relative paths\ncorrectly.\n\nSigned-off-by: Clemens Buchacher \n---\nOn Wed, Jul 27, 2011 at 01:56:14PM +0200, Michael J Gruber wrote:\n>\n> So, I couldn't reproduce because it depends on the length of the dirname.\n> This may be fallout from efad1a5 (ls-files: allow relative pathspec, 2010-06-03)\n\nSince that was me I had a look. I don't think this is a regression,\nhowever. We have had this bug probably forever. Fortunately, it's\neasy to fix with what we already have.\n\nClemens\n\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/ls-files.c | 11 ++++++++---\n cache.h | 2 +-\n quote.c | 8 ++++++--\n 5 files changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d647a31..a3380d9 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \t\tmatch_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);\n \t}\n \n-\tif (report_path_error(ps_matched, pathspec, 0))\n+\tif (report_path_error(ps_matched, pathspec, NULL, -1))\n \t\treturn 1;\n \n \t/* \"checkout -m path\" to recreate conflicted state */\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e1af9b1..a16d00b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -272,7 +272,7 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \t\t\titem->util = item; /* better a valid pointer than a fake one */\n \t}\n \n-\treturn report_path_error(m, pattern, prefix ? strlen(prefix) : 0);\n+\treturn report_path_error(m, pattern, prefix, -1);\n }\n \n static void add_remove_files(struct string_list *list)\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 1570123..72b986f 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -388,11 +388,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n \t}\n }\n \n-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)\n+int report_path_error(const char *ps_matched, const char **pathspec,\n+\t\tconst char *prefix, int prefix_len)\n {\n \t/*\n \t * Make sure all pathspec matched; otherwise it is an error.\n \t */\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *name;\n \tint num, errors = 0;\n \tfor (num = 0; pathspec[num]; num++) {\n \t\tint other, found_dup;\n@@ -417,10 +420,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_\n \t\tif (found_dup)\n \t\t\tcontinue;\n \n+\t\tname = quote_path_relative(pathspec[num], -1, &sb, prefix);\n \t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n-\t\t pathspec[num] + prefix_len);\n+\t\t name);\n \t\terrors++;\n \t}\n+\tstrbuf_release(&sb);\n \treturn errors;\n }\n \n@@ -611,7 +616,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \n \tif (ps_matched) {\n \t\tint bad;\n-\t\tbad = report_path_error(ps_matched, pathspec, prefix_len);\n+\t\tbad = report_path_error(ps_matched, pathspec, prefix, prefix_len);\n \t\tif (bad)\n \t\t\tfprintf(stderr, \"Did you forget to 'git add'?\\n\");\n \ndiff --git a/cache.h b/cache.h\nindex 9e12d55..86518fb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1188,7 +1188,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\n #define ws_tab_width(rule) ((rule) & WS_TAB_WIDTH_MASK)\n \n /* ls-files */\n-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset);\n+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);\n void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n \n char *alias_lookup(const char *alias);\ndiff --git a/quote.c b/quote.c\nindex 63d3b01..532fd3b 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -325,8 +325,12 @@ static const char *path_relative(const char *in, int len,\n \n \tif (len < 0)\n \t\tlen = strlen(in);\n-\tif (prefix && prefix_len < 0)\n-\t\tprefix_len = strlen(prefix);\n+\tif (prefix_len < 0) {\n+\t\tif (prefix)\n+\t\t\tprefix_len = strlen(prefix);\n+\t\telse\n+\t\t\tprefix_len = 0;\n+\t}\n \n \toff = 0;\n \ti = 0;\n-- \n1.7.3.1.105.g84915\n"},{"id":"172344","messageId":"20110729133551.GA8707@toss.lan","threadId":"27900","inReplyTo":"19b908142567fcfafe4da3d0fd60b134d30c613a.1311579663.git.git@drmicha.warpmail.net","subject":"[PATCH] commit: allow partial commits with relative paths","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-07-29T13:35:51Z","receivedAt":"2011-07-29T13:35:51Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In order to do partial commits, git-commit overlays a tree on the\ncache and checks pathspecs against the result. Currently, the overlaying\nis done using \"prefix\" which prevents relative pathspecs with \"..\" and\nabsolute pathspec from matching when they refer to files not under\n\"prefix\" and absent from the index, but still in the tree (i.e. files\nstaged for removal).\n\nInstead, determine the maximal common prefix for all specified\npaths using the pathspec_prefix() routine from ls-files.c. Any use\nof global variables is removed from pathspec_prefix() so that it\ncan be called from commit.c.\n\nReported-by: Reuben Thomas \nAnalyzed-by: Michael J Gruber \nSigned-off-by: Clemens Buchacher \n\nOn Mon, Jul 25, 2011 at 09:42:10AM +0200, Michael J Gruber wrote:\n> \n> Overlay the full tree instead.\n\nThat is one option. On the other hand, ls-files already provides\nthe function we need to do this properly.\n\nWith your permission I am stealing your commit message.\n\nClemens\n---\n builtin/commit.c | 6 ++++--\n builtin/ls-files.c | 38 ++------------------------------------\n cache.h | 1 +\n setup.c | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 39 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a16d00b..c2db12a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -256,8 +256,10 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \t\t;\n \tm = xcalloc(1, i);\n \n-\tif (with_tree)\n-\t\toverlay_tree_on_cache(with_tree, prefix);\n+\tif (with_tree) {\n+\t\tconst char *max_prefix = pathspec_prefix(prefix, pattern);\n+\t\toverlay_tree_on_cache(with_tree, max_prefix);\n+\t}\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 72b986f..fef5642 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -276,41 +276,6 @@ static void prune_cache(const char *prefix)\n \tactive_nr = last;\n }\n \n-static const char *pathspec_prefix(const char *prefix)\n-{\n-\tconst char **p, *n, *prev;\n-\tunsigned long max;\n-\n-\tif (!pathspec) {\n-\t\tmax_prefix_len = prefix ? strlen(prefix) : 0;\n-\t\treturn prefix;\n-\t}\n-\n-\tprev = NULL;\n-\tmax = PATH_MAX;\n-\tfor (p = pathspec; (n = *p) != NULL; p++) {\n-\t\tint i, len = 0;\n-\t\tfor (i = 0; i < max; i++) {\n-\t\t\tchar c = n[i];\n-\t\t\tif (prev && prev[i] != c)\n-\t\t\t\tbreak;\n-\t\t\tif (!c || c == '*' || c == '?')\n-\t\t\t\tbreak;\n-\t\t\tif (c == '/')\n-\t\t\t\tlen = i+1;\n-\t\t}\n-\t\tprev = n;\n-\t\tif (len < max) {\n-\t\t\tmax = len;\n-\t\t\tif (!max)\n-\t\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\tmax_prefix_len = max;\n-\treturn max ? xmemdupz(prev, max) : NULL;\n-}\n-\n static void strip_trailing_slash_from_submodules(void)\n {\n \tconst char **p;\n@@ -581,7 +546,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tstrip_trailing_slash_from_submodules();\n \n \t/* Find common prefix for all pathspec's */\n-\tmax_prefix = pathspec_prefix(prefix);\n+\tmax_prefix = pathspec_prefix(prefix, pathspec);\n+\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n \n \t/* Treat unmatching pathspec elements as errors */\n \tif (pathspec && error_unmatch) {\ndiff --git a/cache.h b/cache.h\nindex 86518fb..dd3edaa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -441,6 +441,7 @@ extern void set_git_work_tree(const char *tree);\n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n extern const char **get_pathspec(const char *prefix, const char **pathspec);\n+extern const char *pathspec_prefix(const char *prefix, const char **pathspec);\n extern void setup_work_tree(void);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\ndiff --git a/setup.c b/setup.c\nindex 5ea5502..2c51a9a 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \treturn pathspec;\n }\n \n+const char *pathspec_prefix(const char *prefix, const char **pathspec)\n+{\n+\tconst char **p, *n, *prev;\n+\tunsigned long max;\n+\n+\tif (!pathspec)\n+\t\treturn prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;\n+\n+\tprev = NULL;\n+\tmax = PATH_MAX;\n+\tfor (p = pathspec; (n = *p) != NULL; p++) {\n+\t\tint i, len = 0;\n+\t\tfor (i = 0; i < max; i++) {\n+\t\t\tchar c = n[i];\n+\t\t\tif (prev && prev[i] != c)\n+\t\t\t\tbreak;\n+\t\t\tif (!c || c == '*' || c == '?')\n+\t\t\t\tbreak;\n+\t\t\tif (c == '/')\n+\t\t\t\tlen = i+1;\n+\t\t}\n+\t\tprev = n;\n+\t\tif (len < max) {\n+\t\t\tmax = len;\n+\t\t\tif (!max)\n+\t\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\treturn max ? xmemdupz(prev, max) : NULL;\n+}\n+\n /*\n * Test if it looks like we're at a git directory.\n * We want to see:\n-- \n1.7.3.1.105.g84915\n"},{"id":"172405","messageId":"4E343534.8000604@drmicha.warpmail.net","threadId":"27900","inReplyTo":"20110729133551.GA8707@toss.lan","subject":"Re: [PATCH] commit: allow partial commits with relative paths","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-07-30T16:45:40Z","receivedAt":"2011-07-30T16:45:40Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Clemens Buchacher venit, vidit, dixit 29.07.2011 15:35:\n> In order to do partial commits, git-commit overlays a tree on the\n> cache and checks pathspecs against the result. Currently, the overlaying\n> is done using \"prefix\" which prevents relative pathspecs with \"..\" and\n> absolute pathspec from matching when they refer to files not under\n> \"prefix\" and absent from the index, but still in the tree (i.e. files\n> staged for removal).\n> \n> Instead, determine the maximal common prefix for all specified\n> paths using the pathspec_prefix() routine from ls-files.c. Any use\n> of global variables is removed from pathspec_prefix() so that it\n> can be called from commit.c.\n> \n> Reported-by: Reuben Thomas \n> Analyzed-by: Michael J Gruber \n> Signed-off-by: Clemens Buchacher \n> \n> On Mon, Jul 25, 2011 at 09:42:10AM +0200, Michael J Gruber wrote:\n>>\n>> Overlay the full tree instead.\n> \n> That is one option. On the other hand, ls-files already provides\n> the function we need to do this properly.\n> \n\nHow is this huge patch more \"proper\" then what I proposed?\n\n> With your permission I am stealing your commit message.\n\nI don't care about the message but don't see the point of this patch.\nUsing the same message certainly won't explain the difference...\n\n> \n> Clemens\n> ---\n> builtin/commit.c | 6 ++++--\n> builtin/ls-files.c | 38 ++------------------------------------\n> cache.h | 1 +\n> setup.c | 32 ++++++++++++++++++++++++++++++++\n> 4 files changed, 39 insertions(+), 38 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index a16d00b..c2db12a 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -256,8 +256,10 @@ static int list_paths(struct string_list *list, const char *with_tree,\n> \t\t;\n> \tm = xcalloc(1, i);\n> \n> -\tif (with_tree)\n> -\t\toverlay_tree_on_cache(with_tree, prefix);\n> +\tif (with_tree) {\n> +\t\tconst char *max_prefix = pathspec_prefix(prefix, pattern);\n> +\t\toverlay_tree_on_cache(with_tree, max_prefix);\n> +\t}\n> \n> \tfor (i = 0; i < active_nr; i++) {\n> \t\tstruct cache_entry *ce = active_cache[i];\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 72b986f..fef5642 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -276,41 +276,6 @@ static void prune_cache(const char *prefix)\n> \tactive_nr = last;\n> }\n> \n> -static const char *pathspec_prefix(const char *prefix)\n> -{\n> -\tconst char **p, *n, *prev;\n> -\tunsigned long max;\n> -\n> -\tif (!pathspec) {\n> -\t\tmax_prefix_len = prefix ? strlen(prefix) : 0;\n> -\t\treturn prefix;\n> -\t}\n> -\n> -\tprev = NULL;\n> -\tmax = PATH_MAX;\n> -\tfor (p = pathspec; (n = *p) != NULL; p++) {\n> -\t\tint i, len = 0;\n> -\t\tfor (i = 0; i < max; i++) {\n> -\t\t\tchar c = n[i];\n> -\t\t\tif (prev && prev[i] != c)\n> -\t\t\t\tbreak;\n> -\t\t\tif (!c || c == '*' || c == '?')\n> -\t\t\t\tbreak;\n> -\t\t\tif (c == '/')\n> -\t\t\t\tlen = i+1;\n> -\t\t}\n> -\t\tprev = n;\n> -\t\tif (len < max) {\n> -\t\t\tmax = len;\n> -\t\t\tif (!max)\n> -\t\t\t\tbreak;\n> -\t\t}\n> -\t}\n> -\n> -\tmax_prefix_len = max;\n> -\treturn max ? xmemdupz(prev, max) : NULL;\n> -}\n> -\n> static void strip_trailing_slash_from_submodules(void)\n> {\n> \tconst char **p;\n> @@ -581,7 +546,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n> \t\tstrip_trailing_slash_from_submodules();\n> \n> \t/* Find common prefix for all pathspec's */\n> -\tmax_prefix = pathspec_prefix(prefix);\n> +\tmax_prefix = pathspec_prefix(prefix, pathspec);\n> +\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> \n> \t/* Treat unmatching pathspec elements as errors */\n> \tif (pathspec && error_unmatch) {\n> diff --git a/cache.h b/cache.h\n> index 86518fb..dd3edaa 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -441,6 +441,7 @@ extern void set_git_work_tree(const char *tree);\n> #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n> \n> extern const char **get_pathspec(const char *prefix, const char **pathspec);\n> +extern const char *pathspec_prefix(const char *prefix, const char **pathspec);\n> extern void setup_work_tree(void);\n> extern const char *setup_git_directory_gently(int *);\n> extern const char *setup_git_directory(void);\n> diff --git a/setup.c b/setup.c\n> index 5ea5502..2c51a9a 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n> \treturn pathspec;\n> }\n> \n> +const char *pathspec_prefix(const char *prefix, const char **pathspec)\n> +{\n> +\tconst char **p, *n, *prev;\n> +\tunsigned long max;\n> +\n> +\tif (!pathspec)\n> +\t\treturn prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;\n> +\n> +\tprev = NULL;\n> +\tmax = PATH_MAX;\n> +\tfor (p = pathspec; (n = *p) != NULL; p++) {\n> +\t\tint i, len = 0;\n> +\t\tfor (i = 0; i < max; i++) {\n> +\t\t\tchar c = n[i];\n> +\t\t\tif (prev && prev[i] != c)\n> +\t\t\t\tbreak;\n> +\t\t\tif (!c || c == '*' || c == '?')\n> +\t\t\t\tbreak;\n> +\t\t\tif (c == '/')\n> +\t\t\t\tlen = i+1;\n> +\t\t}\n> +\t\tprev = n;\n> +\t\tif (len < max) {\n> +\t\t\tmax = len;\n> +\t\t\tif (!max)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn max ? xmemdupz(prev, max) : NULL;\n> +}\n> +\n> /*\n> * Test if it looks like we're at a git directory.\n> * We want to see:\n"},{"id":"172407","messageId":"20110730170045.GA9900@toss.lan","threadId":"27900","inReplyTo":"4E343534.8000604@drmicha.warpmail.net","subject":"Re: [PATCH] commit: allow partial commits with relative paths","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-07-30T17:00:45Z","receivedAt":"2011-07-30T17:00:45Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sat, Jul 30, 2011 at 06:45:40PM +0200, Michael J Gruber wrote:\n> \n> > With your permission I am stealing your commit message.\n> \n> I don't care about the message but don't see the point of this patch.\n> Using the same message certainly won't explain the difference...\n\nWell, to be fair I did add to your message, explaining what I did.\n\nThe point of providing a prefix at all is performance optimization.\nIf you say there is no common prefix for the files of interest,\nthen you cannot leave any files out and you have to read the entire\ntree into the index.\n\nBut even if we cannot use the working directory as a prefix, we can\nstill figure out if there is a common prefix for all given paths,\nand use that instead. I merely copied that idea from ls-tree.\n\nAnd considering that most of my patch the almost verbatim move of a\nfunction from one file to another, I think my change is not that\nbig:\n\n> > 4 files changed, 39 insertions(+), 38 deletions(-)\n\nClemens\n"},{"id":"172408","messageId":"4E34399C.2080109@drmicha.warpmail.net","threadId":"27900","inReplyTo":"20110730170045.GA9900@toss.lan","subject":"Re: [PATCH] commit: allow partial commits with relative paths","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-07-30T17:04:28Z","receivedAt":"2011-07-30T17:04:28Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Clemens Buchacher venit, vidit, dixit 30.07.2011 19:00:\n> On Sat, Jul 30, 2011 at 06:45:40PM +0200, Michael J Gruber wrote:\n>>\n>>> With your permission I am stealing your commit message.\n>>\n>> I don't care about the message but don't see the point of this patch.\n>> Using the same message certainly won't explain the difference...\n> \n> Well, to be fair I did add to your message, explaining what I did.\n> \n> The point of providing a prefix at all is performance optimization.\n> If you say there is no common prefix for the files of interest,\n> then you cannot leave any files out and you have to read the entire\n> tree into the index.\n> \n> But even if we cannot use the working directory as a prefix, we can\n> still figure out if there is a common prefix for all given paths,\n> and use that instead. I merely copied that idea from ls-tree.\n> \n\nThat's the kind of explanation that I meant. Thanks.\n\nMichael\n\n> And considering that most of my patch the almost verbatim move of a\n> function from one file to another, I think my change is not that\n> big:\n> \n>>> 4 files changed, 39 insertions(+), 38 deletions(-)\n> \n> Clemens\n"},{"id":"172409","messageId":"20110730171347.GA10432@toss.lan","threadId":"27900","inReplyTo":"4E34399C.2080109@drmicha.warpmail.net","subject":"[PATCH v2] commit: allow partial commits with relative paths","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-07-30T17:13:47Z","receivedAt":"2011-07-30T17:13:47Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In order to do partial commits, git-commit overlays a tree on the\ncache and checks pathspecs against the result. Currently, the\noverlaying is done using \"prefix\" which prevents relative pathspecs\nwith \"..\" and absolute pathspec from matching when they refer to\nfiles not under \"prefix\" and absent from the index, but still in\nthe tree (i.e. files staged for removal).\n\nThe point of providing a prefix at all is performance optimization.\nIf we say there is no common prefix for the files of interest, then\nwe have to read the entire tree into the index.\n\nBut even if we cannot use the working directory as a prefix, we can\nstill figure out if there is a common prefix for all given paths,\nand use that instead. The pathspec_prefix() routine from ls-files.c\ndoes exactly that.\n\nAny use of global variables is removed from pathspec_prefix() so\nthat it can be called from commit.c.\n\nReported-by: Reuben Thomas \nAnalyzed-by: Michael J Gruber \nSigned-off-by: Clemens Buchacher \n---\n\nOn Sat, Jul 30, 2011 at 07:04:28PM +0200, Michael J Gruber wrote:\n> \n> That's the kind of explanation that I meant. Thanks.\n\nI do tend to be too brief. Sorry about that.\n\nOnly the commit message changed in this v2.\n\n builtin/commit.c | 6 ++++--\n builtin/ls-files.c | 38 ++------------------------------------\n cache.h | 1 +\n setup.c | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 39 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a16d00b..c2db12a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -256,8 +256,10 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \t\t;\n \tm = xcalloc(1, i);\n \n-\tif (with_tree)\n-\t\toverlay_tree_on_cache(with_tree, prefix);\n+\tif (with_tree) {\n+\t\tconst char *max_prefix = pathspec_prefix(prefix, pattern);\n+\t\toverlay_tree_on_cache(with_tree, max_prefix);\n+\t}\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 72b986f..fef5642 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -276,41 +276,6 @@ static void prune_cache(const char *prefix)\n \tactive_nr = last;\n }\n \n-static const char *pathspec_prefix(const char *prefix)\n-{\n-\tconst char **p, *n, *prev;\n-\tunsigned long max;\n-\n-\tif (!pathspec) {\n-\t\tmax_prefix_len = prefix ? strlen(prefix) : 0;\n-\t\treturn prefix;\n-\t}\n-\n-\tprev = NULL;\n-\tmax = PATH_MAX;\n-\tfor (p = pathspec; (n = *p) != NULL; p++) {\n-\t\tint i, len = 0;\n-\t\tfor (i = 0; i < max; i++) {\n-\t\t\tchar c = n[i];\n-\t\t\tif (prev && prev[i] != c)\n-\t\t\t\tbreak;\n-\t\t\tif (!c || c == '*' || c == '?')\n-\t\t\t\tbreak;\n-\t\t\tif (c == '/')\n-\t\t\t\tlen = i+1;\n-\t\t}\n-\t\tprev = n;\n-\t\tif (len < max) {\n-\t\t\tmax = len;\n-\t\t\tif (!max)\n-\t\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\tmax_prefix_len = max;\n-\treturn max ? xmemdupz(prev, max) : NULL;\n-}\n-\n static void strip_trailing_slash_from_submodules(void)\n {\n \tconst char **p;\n@@ -581,7 +546,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tstrip_trailing_slash_from_submodules();\n \n \t/* Find common prefix for all pathspec's */\n-\tmax_prefix = pathspec_prefix(prefix);\n+\tmax_prefix = pathspec_prefix(prefix, pathspec);\n+\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n \n \t/* Treat unmatching pathspec elements as errors */\n \tif (pathspec && error_unmatch) {\ndiff --git a/cache.h b/cache.h\nindex 86518fb..dd3edaa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -441,6 +441,7 @@ extern void set_git_work_tree(const char *tree);\n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n extern const char **get_pathspec(const char *prefix, const char **pathspec);\n+extern const char *pathspec_prefix(const char *prefix, const char **pathspec);\n extern void setup_work_tree(void);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\ndiff --git a/setup.c b/setup.c\nindex 5ea5502..2c51a9a 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \treturn pathspec;\n }\n \n+const char *pathspec_prefix(const char *prefix, const char **pathspec)\n+{\n+\tconst char **p, *n, *prev;\n+\tunsigned long max;\n+\n+\tif (!pathspec)\n+\t\treturn prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;\n+\n+\tprev = NULL;\n+\tmax = PATH_MAX;\n+\tfor (p = pathspec; (n = *p) != NULL; p++) {\n+\t\tint i, len = 0;\n+\t\tfor (i = 0; i < max; i++) {\n+\t\t\tchar c = n[i];\n+\t\t\tif (prev && prev[i] != c)\n+\t\t\t\tbreak;\n+\t\t\tif (!c || c == '*' || c == '?')\n+\t\t\t\tbreak;\n+\t\t\tif (c == '/')\n+\t\t\t\tlen = i+1;\n+\t\t}\n+\t\tprev = n;\n+\t\tif (len < max) {\n+\t\t\tmax = len;\n+\t\t\tif (!max)\n+\t\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\treturn max ? xmemdupz(prev, max) : NULL;\n+}\n+\n /*\n * Test if it looks like we're at a git directory.\n * We want to see:\n-- \n1.7.3.1.105.g84915\n"},{"id":"172476","messageId":"7vipqiknjh.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"20110729130330.GA31941@toss.lan","subject":"Re: [RFC/PATCH] ls-files: fix pathspec display on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-01T00:01:22Z","receivedAt":"2011-08-01T00:01:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> The following sequence of commands reveals an issue with error\n> reporting of relative paths:\n>\n> $ mkdir sub\n> $ cd sub\n> $ git ls-files --error-unmatch ../bbbbb\n> error: pathspec 'b' did not match any file(s) known to git.\n> $ git commit --error-unmatch ../bbbbb\n> error: pathspec 'b' did not match any file(s) known to git.\n>\n> This bug is visible only if the normalized path (i.e., the relative\n> path from the repository root) is longer than the prefix.\n> Otherwise, the code skips over the normalized path and reads from\n> an unused memory location which still contains a leftover of the\n> original command line argument.\n>\n> So instead, use the existing facilities to deal with relative paths\n> correctly.\n\nSounds sane; tests?\n"},{"id":"172477","messageId":"7vei16kncq.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"20110730171347.GA10432@toss.lan","subject":"Re: [PATCH v2] commit: allow partial commits with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-01T00:05:25Z","receivedAt":"2011-08-01T00:05:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> ...\n> But even if we cannot use the working directory as a prefix, we can\n> still figure out if there is a common prefix for all given paths,\n> and use that instead. The pathspec_prefix() routine from ls-files.c\n> does exactly that.\n> ...\n> Only the commit message changed in this v2.\n\nLooks sane; tests?\n\nThanks.\n"},{"id":"172558","messageId":"20110801180320.GA17556@toss","threadId":"27900","inReplyTo":"7vipqiknjh.fsf@alter.siamese.dyndns.org","subject":"[PATCH] test ls-files with relative paths","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-08-01T18:03:20Z","receivedAt":"2011-08-01T18:03:20Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"\nSigned-off-by: Clemens Buchacher \n---\nOn Sun, Jul 31, 2011 at 05:01:22PM -0700, Junio C Hamano wrote:\n> \n> Sounds sane; tests?\n\nCertainly. I'm not testing partial commit since it uses the same\ncodepath as ls-files --error-unmatch anyways.\n\n t/t3005-ls-files-relative.sh | 74 ++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 74 insertions(+), 0 deletions(-)\n create mode 100755 t/t3005-ls-files-relative.sh\n\ndiff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh\nnew file mode 100755\nindex 0000000..e9a2f75\n--- /dev/null\n+++ b/t/t3005-ls-files-relative.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='ls-files tests with relative paths\n+\n+This test runs git ls-files with various relative path arguments.\n+'\n+\n+. ./test-lib.sh\n+\n+new_line='\n+'\n+sq=\\'\n+\n+test_expect_success 'prepare' '\n+\t: >never-mind-me &&\n+\tgit add never-mind-me &&\n+\tmkdir top &&\n+\t(\n+\t\tcd top &&\n+\t\tmkdir sub &&\n+\t\tx=\"x xa xbc xdef xghij xklmno\" &&\n+\t\ty=$(echo \"$x\" | tr x y) &&\n+\t\ttouch $x &&\n+\t\ttouch $y &&\n+\t\tcd sub &&\n+\t\tgit add ../x*\n+\t)\n+'\n+\n+test_expect_success 'ls-files with mixed levels' '\n+\t(\n+\t\tcd top/sub &&\n+\t\tf=$(cat <<-EOF\n+\t\t../../never-mind-me\n+\t\t../x\n+\t\tEOF\n+\t\t) &&\n+\t\techo \"$f\" >expect &&\n+\t\tgit ls-files $f >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'ls-files -c' '\n+\t(\n+\t\tcd top/sub &&\n+\t\tfor f in ../y*\n+\t\tdo\n+\t\t\techo \"error: pathspec ${sq}${f}${sq} did not match any file(s) known to git.\"\n+\t\tdone >expect &&\n+\t\techo \"Did you forget to ${sq}git add${sq}?\" >>expect &&\n+\t\tset ../x* &&\n+\t\tIFS=\"$new_line\" && echo \"$*\" >>expect && unset IFS &&\n+\t\t(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'ls-files -o' '\n+\t(\n+\t\tcd top/sub &&\n+\t\tfor f in ../x*\n+\t\tdo\n+\t\t\techo \"error: pathspec ${sq}${f}${sq} did not match any file(s) known to git.\"\n+\t\tdone >expect &&\n+\t\techo \"Did you forget to ${sq}git add${sq}?\" >>expect &&\n+\t\tset ../y* &&\n+\t\tIFS=\"$new_line\" && echo \"$*\" >>expect && unset IFS &&\n+\t\t(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_done\n-- \n1.7.3.1.105.g84915\n"},{"id":"172594","messageId":"7vhb60j3e1.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"20110801180320.GA17556@toss","subject":"Re: [PATCH] test ls-files with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-01T20:14:14Z","receivedAt":"2011-08-01T20:14:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> Signed-off-by: Clemens Buchacher \n> ---\n> On Sun, Jul 31, 2011 at 05:01:22PM -0700, Junio C Hamano wrote:\n>> \n>> Sounds sane; tests?\n>\n> Certainly. I'm not testing partial commit since it uses the same\n> codepath as ls-files --error-unmatch anyways.\n>\n> t/t3005-ls-files-relative.sh | 74 ++++++++++++++++++++++++++++++++++++++++++\n> 1 files changed, 74 insertions(+), 0 deletions(-)\n> create mode 100755 t/t3005-ls-files-relative.sh\n\nThanks. This should be a part of the primary patch, not a standalone patch.\n\n> +test_expect_success 'prepare' '\n> +\t: >never-mind-me &&\n> +\tgit add never-mind-me &&\n> +\tmkdir top &&\n> +\t(\n> +\t\tcd top &&\n> +\t\tmkdir sub &&\n> +\t\tx=\"x xa xbc xdef xghij xklmno\" &&\n> +\t\ty=$(echo \"$x\" | tr x y) &&\n> +\t\ttouch $x &&\n> +\t\ttouch $y &&\n\nThese are not meant to be quoted; good.\n\n> +\t\tcd sub &&\n> +\t\tgit add ../x*\n> +\t)\n> +'\n> +\n> +test_expect_success 'ls-files with mixed levels' '\n> +\t(\n> +\t\tcd top/sub &&\n> +\t\tf=$(cat <<-EOF\n> +\t\t../../never-mind-me\n> +\t\t../x\n> +\t\tEOF\n> +\t\t) &&\n> +\t\techo \"$f\" >expect &&\n> +\t\tgit ls-files $f >actual &&\n\nThis is a funny way to do this; why not cat the here-document into expect\nand feed $(cat expect) to ls-files? I suspect that it would become easier\nto read and certainly less confusing without need on the readers' side to\nscratch their heads wondering \"does this $f need to be quoted?\" and such.\n\n> +test_expect_success 'ls-files -c' '\n> +\t(\n> +\t\tcd top/sub &&\n> +\t\tfor f in ../y*\n> +\t\tdo\n> +\t\t\techo \"error: pathspec ${sq}${f}${sq} did not match any file(s) known to git.\"\n\nWhy not $sq$f$sq? The above looks harder to read with the braces.\n\n> +\t\tset ../x* &&\n> +\t\tIFS=\"$new_line\" && echo \"$*\" >>expect && unset IFS &&\n\nThat's a funny way to say \"ls ../x*\" isn't it?\n"},{"id":"172604","messageId":"20110801211958.GA23238@toss","threadId":"27900","inReplyTo":"7vhb60j3e1.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] ls-files: fix pathspec display on error","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-08-01T21:19:58Z","receivedAt":"2011-08-01T21:19:58Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The following sequence of commands reveals an issue with error\nreporting of relative paths:\n\n $ mkdir sub\n $ cd sub\n $ git ls-files --error-unmatch ../bbbbb\n error: pathspec 'b' did not match any file(s) known to git.\n $ git commit --error-unmatch ../bbbbb\n error: pathspec 'b' did not match any file(s) known to git.\n\nThis bug is visible only if the normalized path (i.e., the relative\npath from the repository root) is longer than the prefix.\nOtherwise, the code skips over the normalized path and reads from\nan unused memory location which still contains a leftover of the\noriginal command line argument.\n\nSo instead, use the existing facilities to deal with relative paths\ncorrectly.\n\nSigned-off-by: Clemens Buchacher \n---\nOn Mon, Aug 01, 2011 at 01:14:14PM -0700, Junio C Hamano wrote:\n> \n> Thanks. This should be a part of the primary patch, not a\n> standalone patch.\n\nGuilty on all counts! No changes except to address your comments.\nThanks for reviewing.\n\nClemens\n\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/ls-files.c | 11 +++++--\n cache.h | 2 +-\n quote.c | 8 +++-\n t/t3005-ls-files-relative.sh | 70 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 87 insertions(+), 8 deletions(-)\n create mode 100755 t/t3005-ls-files-relative.sh\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d647a31..a3380d9 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \t\tmatch_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);\n \t}\n \n-\tif (report_path_error(ps_matched, pathspec, 0))\n+\tif (report_path_error(ps_matched, pathspec, NULL, -1))\n \t\treturn 1;\n \n \t/* \"checkout -m path\" to recreate conflicted state */\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex cb73857..c2db12a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -274,7 +274,7 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \t\t\titem->util = item; /* better a valid pointer than a fake one */\n \t}\n \n-\treturn report_path_error(m, pattern, prefix ? strlen(prefix) : 0);\n+\treturn report_path_error(m, pattern, prefix, -1);\n }\n \n static void add_remove_files(struct string_list *list)\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 0e98bff..fef5642 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -353,11 +353,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n \t}\n }\n \n-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)\n+int report_path_error(const char *ps_matched, const char **pathspec,\n+\t\tconst char *prefix, int prefix_len)\n {\n \t/*\n \t * Make sure all pathspec matched; otherwise it is an error.\n \t */\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *name;\n \tint num, errors = 0;\n \tfor (num = 0; pathspec[num]; num++) {\n \t\tint other, found_dup;\n@@ -382,10 +385,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_\n \t\tif (found_dup)\n \t\t\tcontinue;\n \n+\t\tname = quote_path_relative(pathspec[num], -1, &sb, prefix);\n \t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n-\t\t pathspec[num] + prefix_len);\n+\t\t name);\n \t\terrors++;\n \t}\n+\tstrbuf_release(&sb);\n \treturn errors;\n }\n \n@@ -577,7 +582,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \n \tif (ps_matched) {\n \t\tint bad;\n-\t\tbad = report_path_error(ps_matched, pathspec, prefix_len);\n+\t\tbad = report_path_error(ps_matched, pathspec, prefix, prefix_len);\n \t\tif (bad)\n \t\t\tfprintf(stderr, \"Did you forget to 'git add'?\\n\");\n \ndiff --git a/cache.h b/cache.h\nindex 1b5d861..dd3edaa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1189,7 +1189,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\n #define ws_tab_width(rule) ((rule) & WS_TAB_WIDTH_MASK)\n \n /* ls-files */\n-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset);\n+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);\n void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n \n char *alias_lookup(const char *alias);\ndiff --git a/quote.c b/quote.c\nindex 63d3b01..532fd3b 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -325,8 +325,12 @@ static const char *path_relative(const char *in, int len,\n \n \tif (len < 0)\n \t\tlen = strlen(in);\n-\tif (prefix && prefix_len < 0)\n-\t\tprefix_len = strlen(prefix);\n+\tif (prefix_len < 0) {\n+\t\tif (prefix)\n+\t\t\tprefix_len = strlen(prefix);\n+\t\telse\n+\t\t\tprefix_len = 0;\n+\t}\n \n \toff = 0;\n \ti = 0;\ndiff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh\nnew file mode 100755\nindex 0000000..3c3ff5e\n--- /dev/null\n+++ b/t/t3005-ls-files-relative.sh\n@@ -0,0 +1,70 @@\n+#!/bin/sh\n+\n+test_description='ls-files tests with relative paths\n+\n+This test runs git ls-files with various relative path arguments.\n+'\n+\n+. ./test-lib.sh\n+\n+new_line='\n+'\n+sq=\\'\n+\n+test_expect_success 'prepare' '\n+\t: >never-mind-me &&\n+\tgit add never-mind-me &&\n+\tmkdir top &&\n+\t(\n+\t\tcd top &&\n+\t\tmkdir sub &&\n+\t\tx=\"x xa xbc xdef xghij xklmno\" &&\n+\t\ty=$(echo \"$x\" | tr x y) &&\n+\t\ttouch $x &&\n+\t\ttouch $y &&\n+\t\tcd sub &&\n+\t\tgit add ../x*\n+\t)\n+'\n+\n+test_expect_success 'ls-files with mixed levels' '\n+\t(\n+\t\tcd top/sub &&\n+\t\tcat >expect <<-EOF &&\n+\t\t../../never-mind-me\n+\t\t../x\n+\t\tEOF\n+\t\tgit ls-files $(cat expect) >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'ls-files -c' '\n+\t(\n+\t\tcd top/sub &&\n+\t\tfor f in ../y*\n+\t\tdo\n+\t\t\techo \"error: pathspec $sq$f$sq did not match any file(s) known to git.\"\n+\t\tdone >expect &&\n+\t\techo \"Did you forget to ${sq}git add${sq}?\" >>expect &&\n+\t\tls ../x* >>expect &&\n+\t\t(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'ls-files -o' '\n+\t(\n+\t\tcd top/sub &&\n+\t\tfor f in ../x*\n+\t\tdo\n+\t\t\techo \"error: pathspec ${sq}${f}${sq} did not match any file(s) known to git.\"\n+\t\tdone >expect &&\n+\t\techo \"Did you forget to ${sq}git add${sq}?\" >>expect &&\n+\t\tls ../y* >>expect &&\n+\t\t(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_done\n-- \n1.7.3.1.105.g84915\n"},{"id":"172611","messageId":"7vvcughii5.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"20110801211958.GA23238@toss","subject":"Re: [PATCH v2] ls-files: fix pathspec display on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-01T22:30:42Z","receivedAt":"2011-08-01T22:30:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> ... No changes except to address your comments.\n> Thanks for reviewing.\n\nThank *you* for re-rolling.\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 0e98bff..fef5642 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -353,11 +353,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n> \t}\n> }\n> \n> -int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)\n> +int report_path_error(const char *ps_matched, const char **pathspec,\n> +\t\tconst char *prefix, int prefix_len)\n> {\n> \t/*\n> \t * Make sure all pathspec matched; otherwise it is an error.\n> \t */\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tconst char *name;\n> \tint num, errors = 0;\n> \tfor (num = 0; pathspec[num]; num++) {\n> \t\tint other, found_dup;\n\n> @@ -382,10 +385,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_\n> \t\tif (found_dup)\n> \t\t\tcontinue;\n> \n> +\t\tname = quote_path_relative(pathspec[num], -1, &sb, prefix);\n> \t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n> -\t\t pathspec[num] + prefix_len);\n> +\t\t name);\n\nIs prefix_len still being used in this function?\n\n> +\t\tls ../x* >>expect &&\n> +\t\t(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&\n\nThe \"|| true\" construct says \"ls-files _may_ exit with non-zero, but we do\nnot care\". Shouldn't we be actively expecting it to exit with non-zero,\nusing test-must-fail here?\n\nI am not sure if passing NULL in checkout.c is the right thing to do. I\nknow that is not the problem this patch introduces, but it leads to this\ninconsistent behaviour:\n\n\t$ cd Documentation\n $ git checkout nosuch.txt\n error: pathspec 'Documentation/nosuch.txt' did not match...\n $ git commit nosuch.txt\n error: pathspec 'nosuch.txt' did not match...\n\nI suspect that cmd_checkout() should pass prefix down to checkout_paths()\nso that the latter can properly strip it from the pathspec.\n\nPerhaps this squashed in on top of your patch...\n\n builtin/checkout.c | 6 +++---\n builtin/commit.c | 2 +-\n builtin/ls-files.c | 5 ++---\n cache.h | 2 +-\n t/t3005-ls-files-relative.sh | 6 +++---\n 5 files changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex efe5e8e..a5717f1 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -201,7 +201,7 @@ static int checkout_merged(int pos, struct checkout *state)\n }\n \n static int checkout_paths(struct tree *source_tree, const char **pathspec,\n-\t\t\t struct checkout_opts *opts)\n+\t\t\t const char *prefix, struct checkout_opts *opts)\n {\n \tint pos;\n \tstruct checkout state;\n@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \t\tmatch_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);\n \t}\n \n-\tif (report_path_error(ps_matched, pathspec, NULL, -1))\n+\tif (report_path_error(ps_matched, pathspec, prefix))\n \t\treturn 1;\n \n \t/* \"checkout -m path\" to recreate conflicted state */\n@@ -1060,7 +1060,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tif (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)\n \t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\nchecking out of the index.\"));\n \n-\t\treturn checkout_paths(source_tree, pathspec, &opts);\n+\t\treturn checkout_paths(source_tree, pathspec, prefix, &opts);\n \t}\n \n \tif (patch_mode)\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a16d00b..9679a99 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -272,7 +272,7 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \t\t\titem->util = item; /* better a valid pointer than a fake one */\n \t}\n \n-\treturn report_path_error(m, pattern, prefix, -1);\n+\treturn report_path_error(m, pattern, prefix);\n }\n \n static void add_remove_files(struct string_list *list)\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 72b986f..468bb13 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -388,8 +388,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n \t}\n }\n \n-int report_path_error(const char *ps_matched, const char **pathspec,\n-\t\tconst char *prefix, int prefix_len)\n+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix)\n {\n \t/*\n \t * Make sure all pathspec matched; otherwise it is an error.\n@@ -616,7 +615,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \n \tif (ps_matched) {\n \t\tint bad;\n-\t\tbad = report_path_error(ps_matched, pathspec, prefix, prefix_len);\n+\t\tbad = report_path_error(ps_matched, pathspec, prefix);\n \t\tif (bad)\n \t\t\tfprintf(stderr, \"Did you forget to 'git add'?\\n\");\n \ndiff --git a/cache.h b/cache.h\nindex 80c60b4..d55a6bb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1175,7 +1175,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\n #define ws_tab_width(rule) ((rule) & WS_TAB_WIDTH_MASK)\n \n /* ls-files */\n-int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);\n+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix);\n void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n \n char *alias_lookup(const char *alias);\ndiff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh\nindex 3c3ff5e..a2b63e2 100755\n--- a/t/t3005-ls-files-relative.sh\n+++ b/t/t3005-ls-files-relative.sh\n@@ -48,7 +48,7 @@ test_expect_success 'ls-files -c' '\n \t\tdone >expect &&\n \t\techo \"Did you forget to ${sq}git add${sq}?\" >>expect &&\n \t\tls ../x* >>expect &&\n-\t\t(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&\n+\t\ttest_must_fail git ls-files -c --error-unmatch ../[xy]* >actual 2>&1 &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -58,11 +58,11 @@ test_expect_success 'ls-files -o' '\n \t\tcd top/sub &&\n \t\tfor f in ../x*\n \t\tdo\n-\t\t\techo \"error: pathspec ${sq}${f}${sq} did not match any file(s) known to git.\"\n+\t\t\techo \"error: pathspec $sq$f$sq did not match any file(s) known to git.\"\n \t\tdone >expect &&\n \t\techo \"Did you forget to ${sq}git add${sq}?\" >>expect &&\n \t\tls ../y* >>expect &&\n-\t\t(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&\n+\t\ttest_must_fail git ls-files -o --error-unmatch ../[xy]* >actual 2>&1 &&\n \t\ttest_cmp expect actual\n \t)\n '\n"},{"id":"172700","messageId":"7vbow7ebzw.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"20110730171347.GA10432@toss.lan","subject":"Re: [PATCH v2] commit: allow partial commits with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T21:31:47Z","receivedAt":"2011-08-02T21:31:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> diff --git a/setup.c b/setup.c\n> index 5ea5502..2c51a9a 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n> \treturn pathspec;\n> }\n> \n> +const char *pathspec_prefix(const char *prefix, const char **pathspec)\n> +{\n\nAs a public function, this sorely needs a comment that describes what it\ndoes. More importantly, when I tried to come up with an example\ndescription, it became very clear that this neither prefixes anything to\npathspec, nor prefixes pathspec to anything else.\n\nAs an internal helper in ls-files implementation it was perfectly\nfine that the function was slightly misnamed, but if you are making it\ninto a public API, we should get its name right.\n\nPerhaps \"common_prefix()\"?\n\nDon't you also want to consolidate dir.c:common_prefix() with this?\n\nWhat happens when pathspec has the recently introduced magic elements,\ne.g. ':/' that widens the match to the whole tree?\n"},{"id":"172796","messageId":"20110803192828.GA4228@toss","threadId":"27900","inReplyTo":"7vbow7ebzw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] commit: allow partial commits with relative paths","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-08-03T19:28:28Z","receivedAt":"2011-08-03T19:28:28Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi Junio,\n\nOn Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:\n> \n> Perhaps \"common_prefix()\"?\n\nYes, I was thinking the same thing actually.\n\n> Don't you also want to consolidate dir.c:common_prefix() with this?\n\nI wasn't aware of it. I'm really swamped right now, but I'll take a\nlook at it soon.\n\nClemens\n"},{"id":"172822","messageId":"7vwreu9mjj.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"20110803192828.GA4228@toss","subject":"Re: [PATCH v2] commit: allow partial commits with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-03T22:07:28Z","receivedAt":"2011-08-03T22:07:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:\n>> \n>> Perhaps \"common_prefix()\"?\n>\n> Yes, I was thinking the same thing actually.\n>\n>> Don't you also want to consolidate dir.c:common_prefix() with this?\n>\n> I wasn't aware of it. I'm really swamped right now, but I'll take a\n> look at it soon.\n\nIt is not too big a deal, though, so I'll merge this one to next and we\ncan fix it up in-tree later.\n\nThanks.\n"},{"id":"174804","messageId":"1315132921-26949-1-git-send-email-drizzd@aon.at","threadId":"27900","inReplyTo":"7vbow7ebzw.fsf@alter.siamese.dyndns.org","subject":"renaming pathspec_prefix (was: Re: [PATCH v2] commit: allow partial commits with relative paths)","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-09-04T10:41:58Z","receivedAt":"2011-09-04T10:41:58Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> \n> > diff --git a/setup.c b/setup.c\n> > index 5ea5502..2c51a9a 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n> > \treturn pathspec;\n> > }\n> > \n> > +const char *pathspec_prefix(const char *prefix, const char **pathspec)\n> > +{\n> \n> As a public function, this sorely needs a comment that describes what it\n> does. More importantly, when I tried to come up with an example\n> description, it became very clear that this neither prefixes anything to\n> pathspec, nor prefixes pathspec to anything else.\n> \n> As an internal helper in ls-files implementation it was perfectly\n> fine that the function was slightly misnamed, but if you are making it\n> into a public API, we should get its name right.\n> \n> Perhaps \"common_prefix()\"?\n> \n> Don't you also want to consolidate dir.c:common_prefix() with this?\n\nYes. This should do it:\n\n[PATCH 1/3] remove prefix argument from pathspec_prefix\n[PATCH 2/3] consolidate pathspec_prefix and common_prefix\n[PATCH 3/3] rename pathspec_prefix -> common_prefix and move to\n\n> What happens when pathspec has the recently introduced magic elements,\n> e.g. ':/' that widens the match to the whole tree?\n\nIf I understand correctly that is resolved in get_pathspec. And\npathspec_prefix (now common_prefix) is called later.\n\nClemens\n"},{"id":"174803","messageId":"1315132921-26949-2-git-send-email-drizzd@aon.at","threadId":"27900","inReplyTo":"1315132921-26949-1-git-send-email-drizzd@aon.at","subject":"[PATCH 1/3] remove prefix argument from pathspec_prefix","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-09-04T10:41:59Z","receivedAt":"2011-09-04T10:41:59Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Passing a prefix to a function that is supposed to find the prefix\nis strange. And it's really only used if the pathspec is NULL. Make\nthe callers handle this case instead.\n\nSigned-off-by: Clemens Buchacher \n---\n builtin/commit.c | 5 +++--\n builtin/ls-files.c | 2 +-\n cache.h | 2 +-\n setup.c | 4 ++--\n 4 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex cbc9613..64fe501 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -255,8 +255,9 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \tm = xcalloc(1, i);\n \n \tif (with_tree) {\n-\t\tconst char *max_prefix = pathspec_prefix(prefix, pattern);\n-\t\toverlay_tree_on_cache(with_tree, max_prefix);\n+\t\tchar *max_prefix = pathspec_prefix(pattern);\n+\t\toverlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);\n+\t\tfree(max_prefix);\n \t}\n \n \tfor (i = 0; i < active_nr; i++) {\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex e8a800d..a54c2a2 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tstrip_trailing_slash_from_submodules();\n \n \t/* Find common prefix for all pathspec's */\n-\tmax_prefix = pathspec_prefix(prefix, pathspec);\n+\tmax_prefix = pathspec_prefix(pathspec);\n \tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n \n \t/* Treat unmatching pathspec elements as errors */\ndiff --git a/cache.h b/cache.h\nindex 607c2ea..0ccc84d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -444,7 +444,7 @@ extern void set_git_work_tree(const char *tree);\n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n extern const char **get_pathspec(const char *prefix, const char **pathspec);\n-extern const char *pathspec_prefix(const char *prefix, const char **pathspec);\n+extern char *pathspec_prefix(const char **pathspec);\n extern void setup_work_tree(void);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\ndiff --git a/setup.c b/setup.c\nindex 27c1d47..0906790 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -236,13 +236,13 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \treturn pathspec;\n }\n \n-const char *pathspec_prefix(const char *prefix, const char **pathspec)\n+char *pathspec_prefix(const char **pathspec)\n {\n \tconst char **p, *n, *prev;\n \tunsigned long max;\n \n \tif (!pathspec)\n-\t\treturn prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;\n+\t\treturn NULL;\n \n \tprev = NULL;\n \tmax = PATH_MAX;\n-- \n1.7.6.1\n"},{"id":"174805","messageId":"1315132921-26949-3-git-send-email-drizzd@aon.at","threadId":"27900","inReplyTo":"1315132921-26949-1-git-send-email-drizzd@aon.at","subject":"[PATCH 2/3] consolidate pathspec_prefix and common_prefix","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-09-04T10:42:00Z","receivedAt":"2011-09-04T10:42:00Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The implementation from pathspec_prefix (slightly modified)\nreplaces the current common_prefix, because it also respects glob\ncharacters.\n\nSigned-off-by: Clemens Buchacher \n---\n\nI wonder if PATH_MAX is needed and respected everywhere. Do we have\nany previous experience with very long paths?\n\nClemens\n\n dir.c | 52 ++++++++++++++++++++++++++--------------------------\n dir.h | 1 +\n setup.c | 29 ++---------------------------\n 3 files changed, 29 insertions(+), 53 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 08281d2..7099fb3 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -34,37 +34,37 @@ int fnmatch_icase(const char *pattern, const char *string, int flags)\n \treturn fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));\n }\n \n-static int common_prefix(const char **pathspec)\n+unsigned long common_prefix_len(const char **pathspec)\n {\n-\tconst char *path, *slash, *next;\n-\tint prefix;\n+\tconst char *n, *first;\n+\tunsigned long max;\n \n \tif (!pathspec)\n \t\treturn 0;\n \n-\tpath = *pathspec;\n-\tslash = strrchr(path, '/');\n-\tif (!slash)\n-\t\treturn 0;\n-\n-\t/*\n-\t * The first 'prefix' characters of 'path' are common leading\n-\t * path components among the pathspecs we have seen so far,\n-\t * including the trailing slash.\n-\t */\n-\tprefix = slash - path + 1;\n-\twhile ((next = *++pathspec) != NULL) {\n-\t\tint len, last_matching_slash = -1;\n-\t\tfor (len = 0; len < prefix && next[len] == path[len]; len++)\n-\t\t\tif (next[len] == '/')\n-\t\t\t\tlast_matching_slash = len;\n-\t\tif (len == prefix)\n-\t\t\tcontinue;\n-\t\tif (last_matching_slash < 0)\n-\t\t\treturn 0;\n-\t\tprefix = last_matching_slash + 1;\n+\tfirst = *pathspec;\n+\tmax = PATH_MAX;\n+\twhile ((n = *pathspec++)) {\n+\t\tint i, len = 0;\n+\t\tfor (i = 0; i < max; i++) {\n+\t\t\tchar c = n[i];\n+\t\t\tif (!c || c != first[i] || is_glob_special(c))\n+\t\t\t\tbreak;\n+\t\t\tif (c == '/')\n+\t\t\t\tlen = i+1;\n+\t\t}\n+\t\tif (len < max) {\n+\t\t\tmax = len;\n+\t\t\tif (!max)\n+\t\t\t\tbreak;\n+\t\t}\n \t}\n-\treturn prefix;\n+\n+\t/* Nothing in the first PATH_MAX characters? */\n+\tif (max > 0 && first[max-1] != '/')\n+\t\tmax = 0;\n+\n+\treturn max;\n }\n \n int fill_directory(struct dir_struct *dir, const char **pathspec)\n@@ -76,7 +76,7 @@ int fill_directory(struct dir_struct *dir, const char **pathspec)\n \t * Calculate common prefix for the pathspec, and\n \t * use that to optimize the directory walk\n \t */\n-\tlen = common_prefix(pathspec);\n+\tlen = common_prefix_len(pathspec);\n \tpath = \"\";\n \n \tif (len)\ndiff --git a/dir.h b/dir.h\nindex 433b5b4..0e55b71 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -64,6 +64,7 @@ struct dir_struct {\n #define MATCHED_RECURSIVELY 1\n #define MATCHED_FNMATCH 2\n #define MATCHED_EXACTLY 3\n+extern unsigned long common_prefix_len(const char **pathspec);\n extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);\n extern int match_pathspec_depth(const struct pathspec *pathspec,\n \t\t\t\tconst char *name, int namelen,\ndiff --git a/setup.c b/setup.c\nindex 0906790..0c60dbd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -238,34 +238,9 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \n char *pathspec_prefix(const char **pathspec)\n {\n-\tconst char **p, *n, *prev;\n-\tunsigned long max;\n+\tunsigned long len = common_prefix_len(pathspec);\n \n-\tif (!pathspec)\n-\t\treturn NULL;\n-\n-\tprev = NULL;\n-\tmax = PATH_MAX;\n-\tfor (p = pathspec; (n = *p) != NULL; p++) {\n-\t\tint i, len = 0;\n-\t\tfor (i = 0; i < max; i++) {\n-\t\t\tchar c = n[i];\n-\t\t\tif (prev && prev[i] != c)\n-\t\t\t\tbreak;\n-\t\t\tif (!c || c == '*' || c == '?')\n-\t\t\t\tbreak;\n-\t\t\tif (c == '/')\n-\t\t\t\tlen = i+1;\n-\t\t}\n-\t\tprev = n;\n-\t\tif (len < max) {\n-\t\t\tmax = len;\n-\t\t\tif (!max)\n-\t\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\treturn max ? xmemdupz(prev, max) : NULL;\n+\treturn len ? xmemdupz(prefix, len) : NULL;\n }\n \n /*\n-- \n1.7.6.1\n"},{"id":"174806","messageId":"1315132921-26949-4-git-send-email-drizzd@aon.at","threadId":"27900","inReplyTo":"1315132921-26949-1-git-send-email-drizzd@aon.at","subject":"[PATCH 3/3] rename pathspec_prefix -> common_prefix and move to dir.[ch]","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-09-04T10:42:01Z","receivedAt":"2011-09-04T10:42:01Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"\nSigned-off-by: Clemens Buchacher \n---\n builtin/commit.c | 2 +-\n builtin/ls-files.c | 2 +-\n cache.h | 1 -\n dir.c | 11 +++++++++++\n dir.h | 1 +\n setup.c | 7 -------\n 6 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 64fe501..b9ab5ef 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -255,7 +255,7 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \tm = xcalloc(1, i);\n \n \tif (with_tree) {\n-\t\tchar *max_prefix = pathspec_prefix(pattern);\n+\t\tchar *max_prefix = common_prefix(pattern);\n \t\toverlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);\n \t\tfree(max_prefix);\n \t}\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex a54c2a2..7cff175 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tstrip_trailing_slash_from_submodules();\n \n \t/* Find common prefix for all pathspec's */\n-\tmax_prefix = pathspec_prefix(pathspec);\n+\tmax_prefix = common_prefix(pathspec);\n \tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n \n \t/* Treat unmatching pathspec elements as errors */\ndiff --git a/cache.h b/cache.h\nindex 0ccc84d..586b987 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -444,7 +444,6 @@ extern void set_git_work_tree(const char *tree);\n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n extern const char **get_pathspec(const char *prefix, const char **pathspec);\n-extern char *pathspec_prefix(const char **pathspec);\n extern void setup_work_tree(void);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\ndiff --git a/dir.c b/dir.c\nindex 7099fb3..f606da9 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -67,6 +67,17 @@ unsigned long common_prefix_len(const char **pathspec)\n \treturn max;\n }\n \n+/*\n+ * Returns a copy of the longest leading path common among all\n+ * pathspecs.\n+ */\n+char *common_prefix(const char **pathspec)\n+{\n+\tunsigned long len = common_prefix_len(pathspec);\n+\n+\treturn len ? xmemdupz(*pathspec, len) : NULL;\n+}\n+\n int fill_directory(struct dir_struct *dir, const char **pathspec)\n {\n \tconst char *path;\ndiff --git a/dir.h b/dir.h\nindex 0e55b71..9a33d23 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -65,6 +65,7 @@ struct dir_struct {\n #define MATCHED_FNMATCH 2\n #define MATCHED_EXACTLY 3\n extern unsigned long common_prefix_len(const char **pathspec);\n+extern char *common_prefix(const char **pathspec);\n extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);\n extern int match_pathspec_depth(const struct pathspec *pathspec,\n \t\t\t\tconst char *name, int namelen,\ndiff --git a/setup.c b/setup.c\nindex 0c60dbd..52bbb70 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -236,13 +236,6 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \treturn pathspec;\n }\n \n-char *pathspec_prefix(const char **pathspec)\n-{\n-\tunsigned long len = common_prefix_len(pathspec);\n-\n-\treturn len ? xmemdupz(prefix, len) : NULL;\n-}\n-\n /*\n * Test if it looks like we're at a git directory.\n * We want to see:\n-- \n1.7.6.1\n"},{"id":"174946","messageId":"7vmxeh8pf4.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"1315132921-26949-2-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/3] remove prefix argument from pathspec_prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T19:02:39Z","receivedAt":"2011-09-06T19:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> Passing a prefix to a function that is supposed to find the prefix\n> is strange. And it's really only used if the pathspec is NULL. Make\n> the callers handle this case instead.\n>\n> Signed-off-by: Clemens Buchacher \n\nWhile I find the above rationale a reasonable justification for the\nremoval of a parameter from the function, it does not seem to justify why\nthe type of the returned value from the function needed to be changed.\n\nIs it because we no longer ever return \"prefix\" we pass in which is a\npointer to a constant memory region to begin with?\n\nWe also didn't free() in the earlier code (because we do not know if it\ncan be freed) and leaking xmemdupz() if the function didn't return the\n\"prefix\", but now you plugged the small leak. Isn't it something you\nshould advertise?\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index cbc9613..64fe501 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -255,8 +255,9 @@ static int list_paths(struct string_list *list, const char *with_tree,\n> \tm = xcalloc(1, i);\n> \n> \tif (with_tree) {\n> -\t\tconst char *max_prefix = pathspec_prefix(prefix, pattern);\n> -\t\toverlay_tree_on_cache(with_tree, max_prefix);\n> +\t\tchar *max_prefix = pathspec_prefix(pattern);\n> +\t\toverlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);\n> +\t\tfree(max_prefix);\n> \t}\n> \n> \tfor (i = 0; i < active_nr; i++) {\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index e8a800d..a54c2a2 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n> \t\tstrip_trailing_slash_from_submodules();\n> \n> \t/* Find common prefix for all pathspec's */\n> -\tmax_prefix = pathspec_prefix(prefix, pathspec);\n> +\tmax_prefix = pathspec_prefix(pathspec);\n> \tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> \n> \t/* Treat unmatching pathspec elements as errors */\n> diff --git a/cache.h b/cache.h\n> index 607c2ea..0ccc84d 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -444,7 +444,7 @@ extern void set_git_work_tree(const char *tree);\n> #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n> \n> extern const char **get_pathspec(const char *prefix, const char **pathspec);\n> -extern const char *pathspec_prefix(const char *prefix, const char **pathspec);\n> +extern char *pathspec_prefix(const char **pathspec);\n> extern void setup_work_tree(void);\n> extern const char *setup_git_directory_gently(int *);\n> extern const char *setup_git_directory(void);\n> diff --git a/setup.c b/setup.c\n> index 27c1d47..0906790 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -236,13 +236,13 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n> \treturn pathspec;\n> }\n> \n> -const char *pathspec_prefix(const char *prefix, const char **pathspec)\n> +char *pathspec_prefix(const char **pathspec)\n> {\n> \tconst char **p, *n, *prev;\n> \tunsigned long max;\n> \n> \tif (!pathspec)\n> -\t\treturn prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;\n> +\t\treturn NULL;\n> \n> \tprev = NULL;\n> \tmax = PATH_MAX;\n"},{"id":"174951","messageId":"7vaaah8muf.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"7vmxeh8pf4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] remove prefix argument from pathspec_prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T19:58:16Z","receivedAt":"2011-09-06T19:58:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Is it because we no longer ever return \"prefix\" we pass in which is a\n> pointer to a constant memory region to begin with?\n>\n> We also didn't free() in the earlier code (because we do not know if it\n> can be freed) and leaking xmemdupz() if the function didn't return the\n> \"prefix\", but now you plugged the small leak. Isn't it something you\n> should advertise?\n\nNah, the leak is not necessarily plugged in all callers anyway, so scratch\nthat part. I've rewritten it like this:\n\ncommit 5879f5684cfe8a38326b4ffd078f96e35c68e640\nAuthor: Clemens Buchacher \nDate: Sun Sep 4 12:41:59 2011 +0200\n\n remove prefix argument from pathspec_prefix\n \n Passing a prefix to a function that is supposed to find the prefix is\n strange. And it's really only used if the pathspec is NULL. Make the\n callers handle this case instead.\n \n As we are always returning a fresh copy of a string (or NULL), change the\n type of the returned value to non-const \"char *\".\n \n Signed-off-by: Clemens Buchacher \n Signed-off-by: Junio C Hamano \n"},{"id":"175071","messageId":"20110908071237.GA4408@ecki","threadId":"27900","inReplyTo":"7vaaah8muf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] remove prefix argument from pathspec_prefix","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-09-08T07:12:37Z","receivedAt":"2011-09-08T07:12:37Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Tue, Sep 06, 2011 at 12:58:16PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Is it because we no longer ever return \"prefix\" we pass in which is a\n> > pointer to a constant memory region to begin with?\n> >\n> > We also didn't free() in the earlier code (because we do not know if it\n> > can be freed) and leaking xmemdupz() if the function didn't return the\n> > \"prefix\", but now you plugged the small leak. Isn't it something you\n> > should advertise?\n> \n> Nah, the leak is not necessarily plugged in all callers anyway, so scratch\n> that part. I've rewritten it like this:\n\nOk.\n\nThe only other caller, though, is cmd_ls_files(). And it would be\ntrivial to plug that leak as well.\n\nBut is it considered a leak, if the program is going to terminate\nright after the function returns? I recall that being used as an\nargument against free'ing such memory on this list.\n\nOn the other hand, that practice makes it harder to analyze leaks\nusing memory leak detectors like for example valgrind.\n\nClemens\n"},{"id":"175119","messageId":"7vmxefx9hy.fsf@alter.siamese.dyndns.org","threadId":"27900","inReplyTo":"20110908071237.GA4408@ecki","subject":"Re: [PATCH 1/3] remove prefix argument from pathspec_prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-08T16:51:53Z","receivedAt":"2011-09-08T16:51:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n>> > We also didn't free() in the earlier code (because we do not know if it\n>> > can be freed) and leaking xmemdupz() if the function didn't return the\n>> > \"prefix\", but now you plugged the small leak. Isn't it something you\n>> > should advertise?\n>> \n>> Nah, the leak is not necessarily plugged in all callers anyway, so scratch\n>> that part. I've rewritten it like this:\n>\n> Ok.\n>\n> The only other caller, though, is cmd_ls_files(). And it would be\n> trivial to plug that leak as well.\n>\n> But is it considered a leak, if the program is going to terminate\n> right after the function returns?\n\nit not a big deal to leak immediately before exit, and a patch whose sole\npurpose is to plug them is of little value.\n\nBut if you are already in the vicinity, updating a function that happens\nto have such a leak, the cost to decide not plugging the leak would be\nabout the same as plugging it, so it would be worth doing in such a case.\n"}]}