{"thread":{"id":"9684","subject":"double occurrence of filenames on command lines","startedAt":"2007-08-29T08:11:22Z","lastAt":"2007-08-30T05:52:21Z","messageCount":9,"participants":["martin f krafft","Alex Riesen","Junio C Hamano","David Kastrup"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"51781","messageId":"20070829081122.GA604@piper.oerlikon.madduck.net","threadId":"9684","inReplyTo":null,"subject":"double occurrence of filenames on command lines","fromName":"martin f krafft","fromEmail":"madduck@madduck.net","sentAt":"2007-08-29T08:11:22Z","receivedAt":"2007-08-29T08:11:22Z","isPatch":false,"sender":{"key":"madduck@madduck.net","avatar":null},"body":"Dear list,\n\nWhile I can say something like\n\n git add foo bar foo\n (note the doubled foo)\n\nwhen using git-add from a script, the following fails:\n\n $ git commit -m. foo foo\n error: pathspec 'foo' did not match any file(s) known to git.\n Did you forget to 'git add'?\n\nI am bringing this up in the context of\nhttp://bugs.debian.org/439992, where debcommit.pl would duplicate\na file argument under certain conditions. It's since been fixed, but\nI wonder whether git-commit could be made more robust in the\npresence of duplicate arguments? Or is this behaviour by choice?\n\nPS: please keep 439992-quiet@bugs.debian.org on Cc.\n\n-- \nmartin; (greetings from the heart of the sun.)\n \\____ echo mailto: !#^.\"<*>\"|tr \"<*> mailto:\" net@madduck\n \nan egg has the shortest sex-life of all: if gets laid once; it gets\neaten once. it also has to come in a box with 11 others, and the\nonly person who will sit on its face is its mother.\n \nspamtraps: madduck.bogus@madduck.net\n"},{"id":"51844","messageId":"20070829194410.GA11824@steel.home","threadId":"9684","inReplyTo":"20070829081122.GA604@piper.oerlikon.madduck.net","subject":"[PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-29T19:44:10Z","receivedAt":"2007-08-29T19:44:10Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The first entry wins, all the subsequent entries will be discarded.\n\nSigned-off-by: Alex Riesen \n---\n\nmartin f krafft, Wed, Aug 29, 2007 10:11:22 +0200:\n> when using git-add from a script, the following fails:\n> \n> $ git commit -m. foo foo\n> error: pathspec 'foo' did not match any file(s) known to git.\n> Did you forget to 'git add'?\n> \n> I am bringing this up in the context of\n> http://bugs.debian.org/439992, where debcommit.pl would duplicate\n> a file argument under certain conditions. It's since been fixed, but\n> I wonder whether git-commit could be made more robust in the\n> presence of duplicate arguments? Or is this behaviour by choice?\n> \n\nDon't think so. Looks like accident. The patch below fixes it,\nby introducing a costly argument duplication check. Shouldn't\nbe a problem for a normal use (git-ls-files expects globs, not\npathnames).\n\n setup.c | 21 +++++++++++++++++----\n 1 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 06004f1..b13b628 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -111,10 +111,19 @@ void verify_non_filename(const char *prefix, const char *arg)\n \t\tdie(\"'%s': %s\", arg, strerror(errno));\n }\n \n+static const char **has_pathspec(const char **start, const char **end, const char *spec)\n+{\n+\tconst char **p;\n+\tfor (p = start; p != end; ++p)\n+\t\tif (!strcmp(*p, spec))\n+\t\t\treturn p;\n+\treturn NULL;\n+}\n+\n const char **get_pathspec(const char *prefix, const char **pathspec)\n {\n \tconst char *entry = *pathspec;\n-\tconst char **p;\n+\tconst char **in, **out;\n \tint prefixlen;\n \n \tif (!prefix && !entry)\n@@ -128,11 +137,15 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \t}\n \n \t/* Otherwise we have to re-write the entries.. */\n-\tp = pathspec;\n+\tin = out = pathspec;\n \tprefixlen = prefix ? strlen(prefix) : 0;\n \tdo {\n-\t\t*p = prefix_path(prefix, prefixlen, entry);\n-\t} while ((entry = *++p) != NULL);\n+\t\tconst char *spec = prefix_path(prefix, prefixlen, entry);\n+\t\tif (!has_pathspec(pathspec, out, spec))\n+\t\t\t*out++ = spec;\n+\t} while ((entry = *++in) != NULL);\n+\tif (in != out)\n+\t\t*out = NULL;\n \treturn (const char **) pathspec;\n }\n \n-- \n1.5.3.rc7.24.g0e57\n"},{"id":"51851","messageId":"7v4piioyu1.fsf@gitster.siamese.dyndns.org","threadId":"9684","inReplyTo":"20070829194410.GA11824@steel.home","subject":"Re: [PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-29T20:44:22Z","receivedAt":"2007-08-29T20:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> The first entry wins, all the subsequent entries will be discarded.\n>\n> Signed-off-by: Alex Riesen \n> ---\n>\n> martin f krafft, Wed, Aug 29, 2007 10:11:22 +0200:\n>> when using git-add from a script, the following fails:\n>> \n>> $ git commit -m. foo foo\n>> error: pathspec 'foo' did not match any file(s) known to git.\n>> Did you forget to 'git add'?\n>> \n>> I am bringing this up in the context of\n>> http://bugs.debian.org/439992, where debcommit.pl would duplicate\n>> a file argument under certain conditions. It's since been fixed, but\n>> I wonder whether git-commit could be made more robust in the\n>> presence of duplicate arguments? Or is this behaviour by choice?\n>\n> Don't think so. Looks like accident. The patch below fixes it,\n> by introducing a costly argument duplication check. Shouldn't\n> be a problem for a normal use (git-ls-files expects globs, not\n> pathnames).\n\nThanks both for your attention to the detail. It was to catch\n\n\tgit commit Makefiel\n\nand did not mean to warn about listing the same thing twice (it\nis still a mistaken usage in the sense that it is unnecessary to\nlist things twice, not in the sense that it instructs the\ncommand to commit the same file twice).\n\nThe patch is not wrong per-se from correctness standpoint, but I\nmust say that it is a horrible thing to do from both performance\nand principle point of view.\n\nThat loop is plain old O(n^2) that penalizes everybody.\n\nPlease do not penalize sane callers when you try to improve\nsupport of mistaken usage. Move expensive error recovery in the\nerror path when possible, and have _only_ mistaken users pay the\nprice.\n\nLike this perhaps.\n\n---\n builtin-ls-files.c | 20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex d36181a..cce17b5 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -511,8 +511,28 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tint num, errors = 0;\n \t\tfor (num = 0; pathspec[num]; num++) {\n+\t\t\tint other, found_dup;\n+\n \t\t\tif (ps_matched[num])\n \t\t\t\tcontinue;\n+\t\t\t/*\n+\t\t\t * The caller might have fed identical pathspec\n+\t\t\t * twice. Do not barf on such a mistake.\n+\t\t\t */\n+\t\t\tfor (found_dup = other = 0;\n+\t\t\t !found_dup && pathspec[other];\n+\t\t\t other++) {\n+\t\t\t\tif (other == num || !ps_matched[other])\n+\t\t\t\t\tcontinue;\n+\t\t\t\tif (!strcmp(pathspec[other], pathspec[num]))\n+\t\t\t\t\t/*\n+\t\t\t\t\t * Ok, we have a match already.\n+\t\t\t\t\t */\n+\t\t\t\t\tfound_dup = 1;\n+\t\t\t}\n+\t\t\tif (found_dup)\n+\t\t\t\tcontinue;\n+\n \t\t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n \t\t\t pathspec[num] + prefix_offset);\n \t\t\terrors++;\n"},{"id":"51852","messageId":"20070829205735.GA1537@piper.oerlikon.madduck.net","threadId":"9684","inReplyTo":"20070829194410.GA11824@steel.home","subject":"Re: [PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"martin f krafft","fromEmail":"madduck@debian.org","sentAt":"2007-08-29T20:57:35Z","receivedAt":"2007-08-29T20:57:35Z","isPatch":true,"sender":{"key":"madduck@debian.org","avatar":null},"body":"also sprach Alex Riesen [2007.08.29.2144 +0200]:\n> Signed-off-by: Alex Riesen \n\nSigned-off-by: martin f krafft \n\nI confirm that Alex' patch does what it should:\n\n$ git init\nInitialized empty Git repository in .git/\n$ date > a; git add a; git commit -m.\nCreated initial commit afb6bca: .\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 a\n$ date >> a\n$ git commit -m. a a\nCreated commit f41d10a: .\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\n-- \n .''`. martin f. krafft \n: :' : proud Debian developer, author, administrator, and user\n`. `'` http://people.debian.org/~madduck - http://debiansystem.info\n `- Debian - when you have better things to do than fixing systems\n \n\"although occasionally there is something to be said for solitude.\"\n -- special agent dale cooper\n"},{"id":"51854","messageId":"20070829210403.GA12808@piper.oerlikon.madduck.net","threadId":"9684","inReplyTo":"7v4piioyu1.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"martin f krafft","fromEmail":"madduck@madduck.net","sentAt":"2007-08-29T21:04:03Z","receivedAt":"2007-08-29T21:04:03Z","isPatch":true,"sender":{"key":"madduck@madduck.net","avatar":null},"body":"also sprach Junio C Hamano [2007.08.29.2244 +0200]:\n> Like this perhaps.\n\nThis also works as expected. Thanks!\n\n-- \nmartin; (greetings from the heart of the sun.)\n \\____ echo mailto: !#^.\"<*>\"|tr \"<*> mailto:\" net@madduck\n \n\"cs class at 8:30am. ugly. if you can wake up early enough to get\n good grades here, you need to develop hacker habits...\"\n -- jeff bailey on #debian-devel\n \nspamtraps: madduck.bogus@madduck.net\n"},{"id":"51856","messageId":"20070829211519.GE11824@steel.home","threadId":"9684","inReplyTo":"7v4piioyu1.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-29T21:15:19Z","receivedAt":"2007-08-29T21:15:19Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Wed, Aug 29, 2007 22:44:22 +0200:\n> \n> That loop is plain old O(n^2) that penalizes everybody.\n> \n\nMaybe something in pathspec matching code could be reused to notice\nthe duplications? It has to go through all of them anyway...\n\n> Please do not penalize sane callers when you try to improve\n> support of mistaken usage. Move expensive error recovery in the\n> error path when possible, and have _only_ mistaken users pay the\n> price.\n> \n> Like this perhaps.\n> \n\nI just would write it shorter (except for that ugly label before\nclosing brace).\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex d36181a..258868e 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -511,11 +511,28 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tint num, errors = 0;\n \t\tfor (num = 0; pathspec[num]; num++) {\n+\t\t\tint other;\n+\n \t\t\tif (ps_matched[num])\n \t\t\t\tcontinue;\n+\t\t\t/*\n+\t\t\t * The caller might have fed identical pathspec\n+\t\t\t * twice. Do not barf on such a mistake.\n+\t\t\t */\n+\t\t\tfor (other = 0; pathspec[other]; other++) {\n+\t\t\t\tif (other == num || !ps_matched[other])\n+\t\t\t\t\tcontinue;\n+\t\t\t\tif (!strcmp(pathspec[other], pathspec[num]))\n+\t\t\t\t\t/*\n+\t\t\t\t\t * Ok, we have a match already.\n+\t\t\t\t\t */\n+\t\t\t\t\tgoto found_dup;\n+\t\t\t}\n+\n \t\t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n \t\t\t pathspec[num] + prefix_offset);\n \t\t\terrors++;\n+\t\tfound_dup:;\n \t\t}\n \n \t\tif (errors)\n-- \n1.5.3.rc7.26.g5f7e4\n"},{"id":"51858","messageId":"85tzqic9bf.fsf@lola.goethe.zz","threadId":"9684","inReplyTo":"20070829211519.GE11824@steel.home","subject":"Re: [PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-29T21:36:20Z","receivedAt":"2007-08-29T21:36:20Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Alex Riesen writes:\n\n> Junio C Hamano, Wed, Aug 29, 2007 22:44:22 +0200:\n>> \n>> That loop is plain old O(n^2) that penalizes everybody.\n>> \n>\n> Maybe something in pathspec matching code could be reused to notice\n> the duplications? It has to go through all of them anyway...\n\nDoes anything speak against sorting the pathspecs? That is an O(n log\nn) operation, and dup detection is trivial afterwards.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"51864","messageId":"7vr6llak4h.fsf@gitster.siamese.dyndns.org","threadId":"9684","inReplyTo":"85tzqic9bf.fsf@lola.goethe.zz","subject":"Re: [PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-30T01:25:50Z","receivedAt":"2007-08-30T01:25:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup writes:\n\n> Does anything speak against sorting the pathspecs? That is an O(n log\n> n) operation,\n\nNot sorting is O(0) operation without losing cycles for the\nnormal case. I think you can sort first in that error handling\npath to avoid O(n^2) but sorting upfront to remove duplicates\nfor every case is unnecessary bloat that penalizes sane callers.\n"},{"id":"51872","messageId":"85myw9d0x6.fsf@lola.goethe.zz","threadId":"9684","inReplyTo":"7vr6llak4h.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Remove duplicate pathspecs from ls-files command line","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-30T05:52:21Z","receivedAt":"2007-08-30T05:52:21Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> David Kastrup writes:\n>\n>> Does anything speak against sorting the pathspecs? That is an O(n log\n>> n) operation,\n>\n> Not sorting is O(0) operation without losing cycles for the\n> normal case. I think you can sort first in that error handling\n> path to avoid O(n^2) but sorting upfront to remove duplicates\n> for every case is unnecessary bloat that penalizes sane callers.\n\nNot if you need to sort the file list anyway in order to merge it with\nthe index. Of course, sorting patterns will not in all cases\nestablish the order of their expansions.\n\nIn any case, uniqueness should be establishable when matching to the\nindex, regardless of whether this matching is done by sort&merge\n(probably the most efficient variant) or by binary search as it is\ndone now if I understand correctly. I am just not happy with the\nensuing binary _insertion_ as this is an O(nm) operation when\ninserting n elements into an m element data structure. A list merge\nis O(n+m), in contrast, and we can access the index sequentially.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"}]}