Re: [PATCH] Remove duplicate pathspecs from ls-files command line

Alex Riesen [off-list ref] writes:

The first entry wins, all the subsequent entries will be discarded.

Signed-off-by: Alex Riesen <redacted>
---

martin f krafft, Wed, Aug 29, 2007 10:11:22 +0200:

quoted

when using git-add from a script, the following fails:

  $ git commit -m. foo foo
  error: pathspec 'foo' did not match any file(s) known to git.
  Did you forget to 'git add'?

I am bringing this up in the context of
http://bugs.debian.org/439992, where debcommit.pl would duplicate
a file argument under certain conditions. It's since been fixed, but
I wonder whether git-commit could be made more robust in the
presence of duplicate arguments? Or is this behaviour by choice?

Don't think so. Looks like accident. The patch below fixes it,
by introducing a costly argument duplication check. Shouldn't
be a problem for a normal use (git-ls-files expects globs, not
pathnames).

Thanks both for your attention to the detail.  It was to catch

	git commit Makefiel

and did not mean to warn about listing the same thing twice (it
is still a mistaken usage in the sense that it is unnecessary to
list things twice, not in the sense that it instructs the
command to commit the same file twice).

The patch is not wrong per-se from correctness standpoint, but I
must say that it is a horrible thing to do from both performance
and principle point of view.

That loop is plain old O(n^2) that penalizes everybody.

Please do not penalize sane callers when you try to improve
support of mistaken usage.  Move expensive error recovery in the
error path when possible, and have _only_ mistaken users pay the
price.

Like this perhaps.

---
 builtin-ls-files.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index d36181a..cce17b5 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c

@@ -511,8 +511,28 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		 */
 		int num, errors = 0;
 		for (num = 0; pathspec[num]; num++) {
+			int other, found_dup;
+
 			if (ps_matched[num])
 				continue;
+			/*
+			 * The caller might have fed identical pathspec
+			 * twice.  Do not barf on such a mistake.
+			 */
+			for (found_dup = other = 0;
+			     !found_dup && pathspec[other];
+			     other++) {
+				if (other == num || !ps_matched[other])
+					continue;
+				if (!strcmp(pathspec[other], pathspec[num]))
+					/*
+					 * Ok, we have a match already.
+					 */
+					found_dup = 1;
+			}
+			if (found_dup)
+				continue;
+
 			error("pathspec '%s' did not match any file(s) known to git.",
 			      pathspec[num] + prefix_offset);
 			errors++;

also sprach Junio C Hamano [off-list ref] [2007.08.29.2244 +0200]:

Like this perhaps.

This also works as expected. Thanks!

-- 
martin;              (greetings from the heart of the sun.)
  \____ echo mailto: !#^."<*>"|tr "<*> mailto:" net@madduck
 
"cs class at 8:30am. ugly. if you can wake up early enough to get
 good grades here, you need to develop hacker habits..."
                                     -- jeff bailey on #debian-devel
 
spamtraps: madduck.bogus@madduck.net

Junio C Hamano, Wed, Aug 29, 2007 22:44:22 +0200:

That loop is plain old O(n^2) that penalizes everybody.

Maybe something in pathspec matching code could be reused to notice
the duplications? It has to go through all of them anyway...

Please do not penalize sane callers when you try to improve
support of mistaken usage.  Move expensive error recovery in the
error path when possible, and have _only_ mistaken users pay the
price.

Like this perhaps.

I just would write it shorter (except for that ugly label before
closing brace).

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index d36181a..258868e 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c

@@ -511,11 +511,28 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		 */
 		int num, errors = 0;
 		for (num = 0; pathspec[num]; num++) {
+			int other;
+
 			if (ps_matched[num])
 				continue;
+			/*
+			 * The caller might have fed identical pathspec
+			 * twice.  Do not barf on such a mistake.
+			 */
+			for (other = 0; pathspec[other]; other++) {
+				if (other == num || !ps_matched[other])
+					continue;
+				if (!strcmp(pathspec[other], pathspec[num]))
+					/*
+					 * Ok, we have a match already.
+					 */
+					goto found_dup;
+			}
+
 			error("pathspec '%s' did not match any file(s) known to git.",
 			      pathspec[num] + prefix_offset);
 			errors++;
+		found_dup:;
 		}
 
 		if (errors)

-- 
1.5.3.rc7.26.g5f7e4