Possible bug

Possible bug

[Reuben Thomas]

Observe the following exchange with git 1.7.4.1, which I found rather
perplexing:

$ cat ~/reportbug-git
$ git rm -f ../INSTALL
rm 'INSTALL'
$ git ci -m "INSTALL is now provided by gnulib." ../INSTALL
error: pathspec 'ALL' did not match any file(s) known to git.
$ cd ..
$ git ci -m "INSTALL is now provided by gnulib." INSTALL
[master 0895314] INSTALL is now provided by gnulib.
 1 files changed, 0 insertions(+), 1 deletions(-)
 delete mode 120000 INSTALL

Is this a bug, or merely some magic I don't know about?

-- 
http://rrt.sc3d.org

[RFC/PATCH] commit: allow partial commits with relative paths

[Michael J Gruber]

In order to do partial commits, git-commit overlays a tree on the
cache and checks pathspecs against the result. Currently, the overlaying
is done using "prefix" which prevents relative pathspecs with ".." and
absolute pathspec from matching when they refer to files not under
"prefix" and absent from the index, but still in the tree (i.e. files
staged for removal).

Overlay the full tree instead.

Reported-by: Reuben Thomas <rrt@sc3d.org>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
RFC because lack of test, and also because I'm not sure we want this, and
what to do about git add which has the same problem, but would need a
different fix.
---
 builtin/commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..431590c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -257,7 +257,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 	m = xcalloc(1, i);
 
 	if (with_tree)
-		overlay_tree_on_cache(with_tree, prefix);
+		overlay_tree_on_cache(with_tree, NULL);
 
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
-- 
1.7.6.336.gdf067

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> RFC because lack of test, and also because I'm not sure we want this, and
> what to do about git add which has the same problem, but would need a
> different fix.

The reason you doubt we would want *this* is...?  Also what is the "same
problem"?

Perhaps it would become clearer if you supported *this* with a sample
workflow?

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> RFC because lack of test, and also because I'm not sure we want this, and
>> what to do about git add which has the same problem, but would need a
>> different fix.
>
> The reason you doubt we would want *this* is...?  Also what is the "same
> problem"?
>
> Perhaps it would become clearer if you supported *this* with a sample
> workflow?

Having said that, I think your change to avoid restricting the overlay to
the current subdirectory makes sense, as long as we allow paths outside
the current subdirectory to be given to the command. I suspect that this
call was relied on an earlier behaviour to error out when paths outside
the current directory were given to the command.

Although I do not know offhand if there are other places in the codepath
with the same issue (i.e. for whatever reason they do not expect to be fed
a relative path, and were relying on the early abort), I think what
your tries to do is taking us in the right direction.

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> RFC because lack of test, and also because I'm not sure we want this, and
>> what to do about git add which has the same problem, but would need a
>> different fix.
> 
> The reason you doubt we would want *this* is...?

I'm not sure the patch has side effects; I'm not sure we want to change
existing behaviour. I.e., is this behaviour intentional or a bug?

> Also what is the "same
> problem"?

The one reported by the OP for commit:

git rm ../a
git commit -m "blurb" ../a
error: pathspec '../a' did not match any file(s) known to git.

has the obvious analogue for add (add is going on behind the scenes of
the above commit, although we don't call the add codepath):

git rm ../a
git add ../a
fatal: pathspec 'a' did not match any files

Restaging a staged change should be a noop, shouldn't it?

The difference is that "git add a" does not work from the root directory
either after the removal of a has been staged. That's why we can leave
it as is. "commit", otoh, clearly behaves differently (depending on
subdir or root dir).

BTW: Note how different our messages are.

> Perhaps it would become clearer if you supported *this* with a sample
> workflow?

Well, the workflow is that described by the OP. It could go into the
commit message of an actual non-RFC patch.

Michael

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Reuben Thomas]

On 27 July 2011 09:22, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>> Also what is the "same
>> problem"?
>
> The one reported by the OP for commit:
>
> git rm ../a
> git commit -m "blurb" ../a
> error: pathspec '../a' did not match any file(s) known to git.

Actually, this is not what I reported. This explains why I couldn't
quite understand the direction this thread took. Look again:

$ cat ~/reportbug-git
$ git rm -f ../INSTALL
rm 'INSTALL'
$ git ci -m "INSTALL is now provided by gnulib." ../INSTALL
error: pathspec 'ALL' did not match any file(s) known to git.
$ cd ..
$ git ci -m "INSTALL is now provided by gnulib." INSTALL
[master 0895314] INSTALL is now provided by gnulib.
 1 files changed, 0 insertions(+), 1 deletions(-)
 delete mode 120000 INSTALL

The thing I didn't understand is that I tried to check in ../INSTALL,
but git complained about pathspec 'ALL'.

-- 
http://rrt.sc3d.org

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Michael J Gruber]

Reuben Thomas venit, vidit, dixit 27.07.2011 11:45:
> On 27 July 2011 09:22, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>> Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:
>>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>>
>>> Also what is the "same
>>> problem"?
>>
>> The one reported by the OP for commit:
>>
>> git rm ../a
>> git commit -m "blurb" ../a
>> error: pathspec '../a' did not match any file(s) known to git.
> 
> Actually, this is not what I reported. This explains why I couldn't
> quite understand the direction this thread took. Look again:
> 
> $ cat ~/reportbug-git
> $ git rm -f ../INSTALL
> rm 'INSTALL'
> $ git ci -m "INSTALL is now provided by gnulib." ../INSTALL
> error: pathspec 'ALL' did not match any file(s) known to git.
> $ cd ..
> $ git ci -m "INSTALL is now provided by gnulib." INSTALL
> [master 0895314] INSTALL is now provided by gnulib.
>  1 files changed, 0 insertions(+), 1 deletions(-)
>  delete mode 120000 INSTALL
> 
> The thing I didn't understand is that I tried to check in ../INSTALL,
> but git complained about pathspec 'ALL'.

Well, you didn't say so, did you?

Also, we don't know what subdir you were doing this in, but I bet it has
a 7-character name...

I can't reproduce that name truncation with a current git, btw. (So I
won't bother bisecting where we removed a spurious offset by "prefix".)

In any case, I still think git should allow partial commits with staged
deletions from within a subdir, which is what my patch is about.

Michael

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Reuben Thomas]

On 27 July 2011 10:53, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> Reuben Thomas venit, vidit, dixit 27.07.2011 11:45:
>>
>> $ cat ~/reportbug-git
>> $ git rm -f ../INSTALL
>> rm 'INSTALL'
>> $ git ci -m "INSTALL is now provided by gnulib." ../INSTALL
>> error: pathspec 'ALL' did not match any file(s) known to git.
>> $ cd ..
>> $ git ci -m "INSTALL is now provided by gnulib." INSTALL
>> [master 0895314] INSTALL is now provided by gnulib.
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>  delete mode 120000 INSTALL
>>
>> The thing I didn't understand is that I tried to check in ../INSTALL,
>> but git complained about pathspec 'ALL'.
>
> Well, you didn't say so, did you?
>
> Also, we don't know what subdir you were doing this in, but I bet it has
> a 7-character name...

Its name was "src".

> I can't reproduce that name truncation with a current git, btw.

Thanks for testing that.

> In any case, I still think git should allow partial commits with staged
> deletions from within a subdir, which is what my patch is about.

Absolutely, this looks useful.

-- 
http://rrt.sc3d.org

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[John Szakmeister]

On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
[snip]
> I can't reproduce that name truncation with a current git, btw. (So I
> won't bother bisecting where we removed a spurious offset by "prefix".)

I can:

:: git --version
git version 1.7.6.347.g4db0d
:: git init reproduce-bug
Initialized empty Git repository in /Users/jszakmeister/tmp/reproduce-bug/.git/
:: cd reproduce-bug
:: echo foo > foo.txt
:: mkdir bar
:: echo bar bar/bar.txt
bar bar/bar.txt
:: git add .
:: git commit -m '.'
[master (root-commit) a5f76f1] .
 2 files changed, 2 insertions(+), 0 deletions(-)
 create mode 100644 bar/bar.txt
 create mode 100644 foo.txt
:: cd bar
:: git rm ../foo.txt
rm 'foo.txt'
:: git commit ../foo.txt
error: pathspec 'txt' did not match any file(s) known to git.

-John

[RFC/PATCH] ls-files: fix pathspec display on error

[Michael J Gruber]

Trying to commit or ls a file "../$a" from within a subdirectory "aha"
gives the following output, when "$a" runs through the values "a", "aa"
etc.:

error: pathspec '../a' did not match any file(s) known to git.
error: pathspec '../aa' did not match any file(s) known to git.
error: pathspec '../aaa' did not match any file(s) known to git.
error: pathspec '' did not match any file(s) known to git.
error: pathspec 'a' did not match any file(s) known to git.
error: pathspec 'aa' did not match any file(s) known to git.
error: pathspec 'aaa' did not match any file(s) known to git.
error: pathspec 'aaaa' did not match any file(s) known to git.
error: pathspec 'aaaaa' did not match any file(s) known to git.
error: pathspec 'aaaaaa' did not match any file(s) known to git.

This comes from the fact that report_path_error() tries to chop off a
prefix from the pathspec which may have been resolved and consumed already.

Fix this by displaying the full pathspec.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
So, I couldn't reproduce because it depends on the length of the dirname.
This may be fallout from efad1a5 (ls-files: allow relative pathspec, 2010-06-03)
but I haven't checked and won't be able to for the next 3 weeks, sorry.
I just wanted to send out this possible fix before I leave.
---
 builtin/ls-files.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1570123..e0b1401 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -418,7 +418,7 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
 			continue;
 
 		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec[num] + prefix_len);
+		      pathspec[num]);
 		errors++;
 	}
 	return errors;
-- 
1.7.6.336.gdf067

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> git rm ../a
> git add ../a
> fatal: pathspec 'a' did not match any files

If you didn't have that ../a from the beginning, wouldn't you get the same
error message? Or instead of "git add ../a", you did "(cd .. && git add a)"
what message do (and more importantly "should") you get?

I think it would be an improvement if we said "../a" in the message, but
otherwise I agree with you that there is no bug here.

A side note. If this didn't work:

    $ git rm --cached ../a
    $ git add ../a

I would suspect that would be a bug (this however seems to work).

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> The one reported by the OP for commit:
>
> git rm ../a
> git commit -m "blurb" ../a
> error: pathspec '../a' did not match any file(s) known to git.

That does look like a bug given that (starting from a state where that
"../a" is a tracked file that appear in the HEAD commit):

	git rm ../a
        (cd .. && git commit -m "removed a" a)

does work. For "git add ../a" after removing the path from both index and
the working tree, see my other message.

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 27.07.2011 17:41:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> The one reported by the OP for commit:
>>
>> git rm ../a
>> git commit -m "blurb" ../a
>> error: pathspec '../a' did not match any file(s) known to git.
> 
> That does look like a bug given that (starting from a state where that
> "../a" is a tracked file that appear in the HEAD commit):
> 
> 	git rm ../a
>         (cd .. && git commit -m "removed a" a)
> 
> does work. For "git add ../a" after removing the path from both index and
> the working tree, see my other message.

Full agreement on both.
Now I really gotta run, sorry. But "I'll be back" :)

Michael

Re: [RFC/PATCH] commit: allow partial commits with relative paths

[Erik Faye-Lund]

On Wed, Jul 27, 2011 at 12:19 PM, John Szakmeister <john@szakmeister.net> wrote:
> On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
> [snip]
>> I can't reproduce that name truncation with a current git, btw. (So I
>> won't bother bisecting where we removed a spurious offset by "prefix".)
>
> I can:
>
> :: git --version
> git version 1.7.6.347.g4db0d
> :: git init reproduce-bug
> Initialized empty Git repository in /Users/jszakmeister/tmp/reproduce-bug/.git/
> :: cd reproduce-bug
> :: echo foo > foo.txt
> :: mkdir bar
> :: echo bar bar/bar.txt
> bar bar/bar.txt
> :: git add .
> :: git commit -m '.'
> [master (root-commit) a5f76f1] .
>  2 files changed, 2 insertions(+), 0 deletions(-)
>  create mode 100644 bar/bar.txt
>  create mode 100644 foo.txt
> :: cd bar
> :: git rm ../foo.txt
> rm 'foo.txt'
> :: git commit ../foo.txt
> error: pathspec 'txt' did not match any file(s) known to git.

What seems to happen here, is report_path_error gets called with a
pathspec that doesn't contain the prefix (which is "bar/"), yet it
tries to skip prefix_len characters ahead in it when reporting.

Why the pathspec isn't prefixed by "bar/" is because of the
normalize_path_copy-call in prefix_path, when called through
get_pathspec("bar/", {"../foo.txt", NULL}).

Re: [RFC/PATCH] ls-files: fix pathspec display on error

[Clemens Buchacher]

The following sequence of commands reveals an issue with error
reporting of relative paths:

 $ mkdir sub
 $ cd sub
 $ git ls-files --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.
 $ git commit --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.

This bug is visible only if the normalized path (i.e., the relative
path from the repository root) is longer than the prefix.
Otherwise, the code skips over the normalized path and reads from
an unused memory location which still contains a leftover of the
original command line argument.

So instead, use the existing facilities to deal with relative paths
correctly.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Wed, Jul 27, 2011 at 01:56:14PM +0200, Michael J Gruber wrote:
>
> So, I couldn't reproduce because it depends on the length of the dirname.
> This may be fallout from efad1a5 (ls-files: allow relative pathspec, 2010-06-03)

Since that was me I had a look. I don't think this is a regression,
however.  We have had this bug probably forever. Fortunately, it's
easy to fix with what we already have.

Clemens

 builtin/checkout.c |    2 +-
 builtin/commit.c   |    2 +-
 builtin/ls-files.c |   11 ++++++++---
 cache.h            |    2 +-
 quote.c            |    8 ++++++--
 5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d647a31..a3380d9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);
 	}
 
-	if (report_path_error(ps_matched, pathspec, 0))
+	if (report_path_error(ps_matched, pathspec, NULL, -1))
 		return 1;
 
 	/* "checkout -m path" to recreate conflicted state */
diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..a16d00b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -272,7 +272,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	return report_path_error(m, pattern, prefix ? strlen(prefix) : 0);
+	return report_path_error(m, pattern, prefix, -1);
 }
 
 static void add_remove_files(struct string_list *list)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1570123..72b986f 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -388,11 +388,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
+int report_path_error(const char *ps_matched, const char **pathspec,
+		const char *prefix, int prefix_len)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
 	 */
+	struct strbuf sb = STRBUF_INIT;
+	const char *name;
 	int num, errors = 0;
 	for (num = 0; pathspec[num]; num++) {
 		int other, found_dup;
@@ -417,10 +420,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
 		if (found_dup)
 			continue;
 
+		name = quote_path_relative(pathspec[num], -1, &sb, prefix);
 		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec[num] + prefix_len);
+		      name);
 		errors++;
 	}
+	strbuf_release(&sb);
 	return errors;
 }
 
@@ -611,7 +616,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix_len);
+		bad = report_path_error(ps_matched, pathspec, prefix, prefix_len);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/cache.h b/cache.h
index 9e12d55..86518fb 100644
--- a/cache.h
+++ b/cache.h
@@ -1188,7 +1188,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 #define ws_tab_width(rule)     ((rule) & WS_TAB_WIDTH_MASK)
 
 /* ls-files */
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset);
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/quote.c b/quote.c
index 63d3b01..532fd3b 100644
--- a/quote.c
+++ b/quote.c
@@ -325,8 +325,12 @@ static const char *path_relative(const char *in, int len,
 
 	if (len < 0)
 		len = strlen(in);
-	if (prefix && prefix_len < 0)
-		prefix_len = strlen(prefix);
+	if (prefix_len < 0) {
+		if (prefix)
+			prefix_len = strlen(prefix);
+		else
+			prefix_len = 0;
+	}
 
 	off = 0;
 	i = 0;
-- 
1.7.3.1.105.g84915

[PATCH] commit: allow partial commits with relative paths

[Clemens Buchacher]

In order to do partial commits, git-commit overlays a tree on the
cache and checks pathspecs against the result. Currently, the overlaying
is done using "prefix" which prevents relative pathspecs with ".." and
absolute pathspec from matching when they refer to files not under
"prefix" and absent from the index, but still in the tree (i.e. files
staged for removal).

Instead, determine the maximal common prefix for all specified
paths using the pathspec_prefix() routine from ls-files.c.  Any use
of global variables is removed from pathspec_prefix() so that it
can be called from commit.c.

Reported-by: Reuben Thomas <rrt@sc3d.org>
Analyzed-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>

On Mon, Jul 25, 2011 at 09:42:10AM +0200, Michael J Gruber wrote:
> 
> Overlay the full tree instead.

That is one option. On the other hand, ls-files already provides
the function we need to do this properly.

With your permission I am stealing your commit message.

Clemens
---
 builtin/commit.c   |    6 ++++--
 builtin/ls-files.c |   38 ++------------------------------------
 cache.h            |    1 +
 setup.c            |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a16d00b..c2db12a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -256,8 +256,10 @@ static int list_paths(struct string_list *list, const char *with_tree,
 		;
 	m = xcalloc(1, i);
 
-	if (with_tree)
-		overlay_tree_on_cache(with_tree, prefix);
+	if (with_tree) {
+		const char *max_prefix = pathspec_prefix(prefix, pattern);
+		overlay_tree_on_cache(with_tree, max_prefix);
+	}
 
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 72b986f..fef5642 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -276,41 +276,6 @@ static void prune_cache(const char *prefix)
 	active_nr = last;
 }
 
-static const char *pathspec_prefix(const char *prefix)
-{
-	const char **p, *n, *prev;
-	unsigned long max;
-
-	if (!pathspec) {
-		max_prefix_len = prefix ? strlen(prefix) : 0;
-		return prefix;
-	}
-
-	prev = NULL;
-	max = PATH_MAX;
-	for (p = pathspec; (n = *p) != NULL; p++) {
-		int i, len = 0;
-		for (i = 0; i < max; i++) {
-			char c = n[i];
-			if (prev && prev[i] != c)
-				break;
-			if (!c || c == '*' || c == '?')
-				break;
-			if (c == '/')
-				len = i+1;
-		}
-		prev = n;
-		if (len < max) {
-			max = len;
-			if (!max)
-				break;
-		}
-	}
-
-	max_prefix_len = max;
-	return max ? xmemdupz(prev, max) : NULL;
-}
-
 static void strip_trailing_slash_from_submodules(void)
 {
 	const char **p;
@@ -581,7 +546,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		strip_trailing_slash_from_submodules();
 
 	/* Find common prefix for all pathspec's */
-	max_prefix = pathspec_prefix(prefix);
+	max_prefix = pathspec_prefix(prefix, pathspec);
+	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec && error_unmatch) {
diff --git a/cache.h b/cache.h
index 86518fb..dd3edaa 100644
--- a/cache.h
+++ b/cache.h
@@ -441,6 +441,7 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
+extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
diff --git a/setup.c b/setup.c
index 5ea5502..2c51a9a 100644
--- a/setup.c
+++ b/setup.c
@@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
+const char *pathspec_prefix(const char *prefix, const char **pathspec)
+{
+	const char **p, *n, *prev;
+	unsigned long max;
+
+	if (!pathspec)
+		return prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;
+
+	prev = NULL;
+	max = PATH_MAX;
+	for (p = pathspec; (n = *p) != NULL; p++) {
+		int i, len = 0;
+		for (i = 0; i < max; i++) {
+			char c = n[i];
+			if (prev && prev[i] != c)
+				break;
+			if (!c || c == '*' || c == '?')
+				break;
+			if (c == '/')
+				len = i+1;
+		}
+		prev = n;
+		if (len < max) {
+			max = len;
+			if (!max)
+				break;
+		}
+	}
+
+	return max ? xmemdupz(prev, max) : NULL;
+}
+
 /*
  * Test if it looks like we're at a git directory.
  * We want to see:
-- 
1.7.3.1.105.g84915

Re: [PATCH] commit: allow partial commits with relative paths

[Michael J Gruber]

Clemens Buchacher venit, vidit, dixit 29.07.2011 15:35:
> In order to do partial commits, git-commit overlays a tree on the
> cache and checks pathspecs against the result. Currently, the overlaying
> is done using "prefix" which prevents relative pathspecs with ".." and
> absolute pathspec from matching when they refer to files not under
> "prefix" and absent from the index, but still in the tree (i.e. files
> staged for removal).
> 
> Instead, determine the maximal common prefix for all specified
> paths using the pathspec_prefix() routine from ls-files.c.  Any use
> of global variables is removed from pathspec_prefix() so that it
> can be called from commit.c.
> 
> Reported-by: Reuben Thomas <rrt@sc3d.org>
> Analyzed-by: Michael J Gruber <git@drmicha.warpmail.net>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> 
> On Mon, Jul 25, 2011 at 09:42:10AM +0200, Michael J Gruber wrote:
>>
>> Overlay the full tree instead.
> 
> That is one option. On the other hand, ls-files already provides
> the function we need to do this properly.
> 

How is this huge patch more "proper" then what I proposed?

> With your permission I am stealing your commit message.

I don't care about the message but don't see the point of this patch.
Using the same message certainly won't explain the difference...

> 
> Clemens
> ---
>  builtin/commit.c   |    6 ++++--
>  builtin/ls-files.c |   38 ++------------------------------------
>  cache.h            |    1 +
>  setup.c            |   32 ++++++++++++++++++++++++++++++++
>  4 files changed, 39 insertions(+), 38 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a16d00b..c2db12a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -256,8 +256,10 @@ static int list_paths(struct string_list *list, const char *with_tree,
>  		;
>  	m = xcalloc(1, i);
>  
> -	if (with_tree)
> -		overlay_tree_on_cache(with_tree, prefix);
> +	if (with_tree) {
> +		const char *max_prefix = pathspec_prefix(prefix, pattern);
> +		overlay_tree_on_cache(with_tree, max_prefix);
> +	}
>  
>  	for (i = 0; i < active_nr; i++) {
>  		struct cache_entry *ce = active_cache[i];
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 72b986f..fef5642 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -276,41 +276,6 @@ static void prune_cache(const char *prefix)
>  	active_nr = last;
>  }
>  
> -static const char *pathspec_prefix(const char *prefix)
> -{
> -	const char **p, *n, *prev;
> -	unsigned long max;
> -
> -	if (!pathspec) {
> -		max_prefix_len = prefix ? strlen(prefix) : 0;
> -		return prefix;
> -	}
> -
> -	prev = NULL;
> -	max = PATH_MAX;
> -	for (p = pathspec; (n = *p) != NULL; p++) {
> -		int i, len = 0;
> -		for (i = 0; i < max; i++) {
> -			char c = n[i];
> -			if (prev && prev[i] != c)
> -				break;
> -			if (!c || c == '*' || c == '?')
> -				break;
> -			if (c == '/')
> -				len = i+1;
> -		}
> -		prev = n;
> -		if (len < max) {
> -			max = len;
> -			if (!max)
> -				break;
> -		}
> -	}
> -
> -	max_prefix_len = max;
> -	return max ? xmemdupz(prev, max) : NULL;
> -}
> -
>  static void strip_trailing_slash_from_submodules(void)
>  {
>  	const char **p;
> @@ -581,7 +546,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>  		strip_trailing_slash_from_submodules();
>  
>  	/* Find common prefix for all pathspec's */
> -	max_prefix = pathspec_prefix(prefix);
> +	max_prefix = pathspec_prefix(prefix, pathspec);
> +	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
>  
>  	/* Treat unmatching pathspec elements as errors */
>  	if (pathspec && error_unmatch) {
> diff --git a/cache.h b/cache.h
> index 86518fb..dd3edaa 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -441,6 +441,7 @@ extern void set_git_work_tree(const char *tree);
>  #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>  
>  extern const char **get_pathspec(const char *prefix, const char **pathspec);
> +extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
>  extern void setup_work_tree(void);
>  extern const char *setup_git_directory_gently(int *);
>  extern const char *setup_git_directory(void);
> diff --git a/setup.c b/setup.c
> index 5ea5502..2c51a9a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
>  	return pathspec;
>  }
>  
> +const char *pathspec_prefix(const char *prefix, const char **pathspec)
> +{
> +	const char **p, *n, *prev;
> +	unsigned long max;
> +
> +	if (!pathspec)
> +		return prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;
> +
> +	prev = NULL;
> +	max = PATH_MAX;
> +	for (p = pathspec; (n = *p) != NULL; p++) {
> +		int i, len = 0;
> +		for (i = 0; i < max; i++) {
> +			char c = n[i];
> +			if (prev && prev[i] != c)
> +				break;
> +			if (!c || c == '*' || c == '?')
> +				break;
> +			if (c == '/')
> +				len = i+1;
> +		}
> +		prev = n;
> +		if (len < max) {
> +			max = len;
> +			if (!max)
> +				break;
> +		}
> +	}
> +
> +	return max ? xmemdupz(prev, max) : NULL;
> +}
> +
>  /*
>   * Test if it looks like we're at a git directory.
>   * We want to see:

Re: [PATCH] commit: allow partial commits with relative paths

[Clemens Buchacher]

On Sat, Jul 30, 2011 at 06:45:40PM +0200, Michael J Gruber wrote:
> 
> > With your permission I am stealing your commit message.
> 
> I don't care about the message but don't see the point of this patch.
> Using the same message certainly won't explain the difference...

Well, to be fair I did add to your message, explaining what I did.

The point of providing a prefix at all is performance optimization.
If you say there is no common prefix for the files of interest,
then you cannot leave any files out and you have to read the entire
tree into the index.

But even if we cannot use the working directory as a prefix, we can
still figure out if there is a common prefix for all given paths,
and use that instead. I merely copied that idea from ls-tree.

And considering that most of my patch the almost verbatim move of a
function from one file to another, I think my change is not that
big:

> >  4 files changed, 39 insertions(+), 38 deletions(-)

Clemens

Re: [PATCH] commit: allow partial commits with relative paths

[Michael J Gruber]

Clemens Buchacher venit, vidit, dixit 30.07.2011 19:00:
> On Sat, Jul 30, 2011 at 06:45:40PM +0200, Michael J Gruber wrote:
>>
>>> With your permission I am stealing your commit message.
>>
>> I don't care about the message but don't see the point of this patch.
>> Using the same message certainly won't explain the difference...
> 
> Well, to be fair I did add to your message, explaining what I did.
> 
> The point of providing a prefix at all is performance optimization.
> If you say there is no common prefix for the files of interest,
> then you cannot leave any files out and you have to read the entire
> tree into the index.
> 
> But even if we cannot use the working directory as a prefix, we can
> still figure out if there is a common prefix for all given paths,
> and use that instead. I merely copied that idea from ls-tree.
> 

That's the kind of explanation that I meant. Thanks.

Michael

> And considering that most of my patch the almost verbatim move of a
> function from one file to another, I think my change is not that
> big:
> 
>>>  4 files changed, 39 insertions(+), 38 deletions(-)
> 
> Clemens

[PATCH v2] commit: allow partial commits with relative paths

[Clemens Buchacher]

In order to do partial commits, git-commit overlays a tree on the
cache and checks pathspecs against the result. Currently, the
overlaying is done using "prefix" which prevents relative pathspecs
with ".." and absolute pathspec from matching when they refer to
files not under "prefix" and absent from the index, but still in
the tree (i.e.  files staged for removal).

The point of providing a prefix at all is performance optimization.
If we say there is no common prefix for the files of interest, then
we have to read the entire tree into the index.

But even if we cannot use the working directory as a prefix, we can
still figure out if there is a common prefix for all given paths,
and use that instead. The pathspec_prefix() routine from ls-files.c
does exactly that.

Any use of global variables is removed from pathspec_prefix() so
that it can be called from commit.c.

Reported-by: Reuben Thomas <rrt@sc3d.org>
Analyzed-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sat, Jul 30, 2011 at 07:04:28PM +0200, Michael J Gruber wrote:
> 
> That's the kind of explanation that I meant. Thanks.

I do tend to be too brief. Sorry about that.

Only the commit message changed in this v2.

 builtin/commit.c   |    6 ++++--
 builtin/ls-files.c |   38 ++------------------------------------
 cache.h            |    1 +
 setup.c            |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a16d00b..c2db12a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -256,8 +256,10 @@ static int list_paths(struct string_list *list, const char *with_tree,
 		;
 	m = xcalloc(1, i);
 
-	if (with_tree)
-		overlay_tree_on_cache(with_tree, prefix);
+	if (with_tree) {
+		const char *max_prefix = pathspec_prefix(prefix, pattern);
+		overlay_tree_on_cache(with_tree, max_prefix);
+	}
 
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 72b986f..fef5642 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -276,41 +276,6 @@ static void prune_cache(const char *prefix)
 	active_nr = last;
 }
 
-static const char *pathspec_prefix(const char *prefix)
-{
-	const char **p, *n, *prev;
-	unsigned long max;
-
-	if (!pathspec) {
-		max_prefix_len = prefix ? strlen(prefix) : 0;
-		return prefix;
-	}
-
-	prev = NULL;
-	max = PATH_MAX;
-	for (p = pathspec; (n = *p) != NULL; p++) {
-		int i, len = 0;
-		for (i = 0; i < max; i++) {
-			char c = n[i];
-			if (prev && prev[i] != c)
-				break;
-			if (!c || c == '*' || c == '?')
-				break;
-			if (c == '/')
-				len = i+1;
-		}
-		prev = n;
-		if (len < max) {
-			max = len;
-			if (!max)
-				break;
-		}
-	}
-
-	max_prefix_len = max;
-	return max ? xmemdupz(prev, max) : NULL;
-}
-
 static void strip_trailing_slash_from_submodules(void)
 {
 	const char **p;
@@ -581,7 +546,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		strip_trailing_slash_from_submodules();
 
 	/* Find common prefix for all pathspec's */
-	max_prefix = pathspec_prefix(prefix);
+	max_prefix = pathspec_prefix(prefix, pathspec);
+	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec && error_unmatch) {
diff --git a/cache.h b/cache.h
index 86518fb..dd3edaa 100644
--- a/cache.h
+++ b/cache.h
@@ -441,6 +441,7 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
+extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
diff --git a/setup.c b/setup.c
index 5ea5502..2c51a9a 100644
--- a/setup.c
+++ b/setup.c
@@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
+const char *pathspec_prefix(const char *prefix, const char **pathspec)
+{
+	const char **p, *n, *prev;
+	unsigned long max;
+
+	if (!pathspec)
+		return prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;
+
+	prev = NULL;
+	max = PATH_MAX;
+	for (p = pathspec; (n = *p) != NULL; p++) {
+		int i, len = 0;
+		for (i = 0; i < max; i++) {
+			char c = n[i];
+			if (prev && prev[i] != c)
+				break;
+			if (!c || c == '*' || c == '?')
+				break;
+			if (c == '/')
+				len = i+1;
+		}
+		prev = n;
+		if (len < max) {
+			max = len;
+			if (!max)
+				break;
+		}
+	}
+
+	return max ? xmemdupz(prev, max) : NULL;
+}
+
 /*
  * Test if it looks like we're at a git directory.
  * We want to see:
-- 
1.7.3.1.105.g84915

Re: [RFC/PATCH] ls-files: fix pathspec display on error

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> The following sequence of commands reveals an issue with error
> reporting of relative paths:
>
>  $ mkdir sub
>  $ cd sub
>  $ git ls-files --error-unmatch ../bbbbb
>  error: pathspec 'b' did not match any file(s) known to git.
>  $ git commit --error-unmatch ../bbbbb
>  error: pathspec 'b' did not match any file(s) known to git.
>
> This bug is visible only if the normalized path (i.e., the relative
> path from the repository root) is longer than the prefix.
> Otherwise, the code skips over the normalized path and reads from
> an unused memory location which still contains a leftover of the
> original command line argument.
>
> So instead, use the existing facilities to deal with relative paths
> correctly.

Sounds sane; tests?

Re: [PATCH v2] commit: allow partial commits with relative paths

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> ...
> But even if we cannot use the working directory as a prefix, we can
> still figure out if there is a common prefix for all given paths,
> and use that instead. The pathspec_prefix() routine from ls-files.c
> does exactly that.
> ...
> Only the commit message changed in this v2.

Looks sane; tests?

Thanks.

[PATCH] test ls-files with relative paths

[Clemens Buchacher]

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Sun, Jul 31, 2011 at 05:01:22PM -0700, Junio C Hamano wrote:
> 
> Sounds sane; tests?

Certainly. I'm not testing partial commit since it uses the same
codepath as ls-files --error-unmatch anyways.

 t/t3005-ls-files-relative.sh |   74 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100755 t/t3005-ls-files-relative.sh

diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
new file mode 100755
index 0000000..e9a2f75
--- /dev/null
+++ b/t/t3005-ls-files-relative.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='ls-files tests with relative paths
+
+This test runs git ls-files with various relative path arguments.
+'
+
+. ./test-lib.sh
+
+new_line='
+'
+sq=\'
+
+test_expect_success 'prepare' '
+	: >never-mind-me &&
+	git add never-mind-me &&
+	mkdir top &&
+	(
+		cd top &&
+		mkdir sub &&
+		x="x xa xbc xdef xghij xklmno" &&
+		y=$(echo "$x" | tr x y) &&
+		touch $x &&
+		touch $y &&
+		cd sub &&
+		git add ../x*
+	)
+'
+
+test_expect_success 'ls-files with mixed levels' '
+	(
+		cd top/sub &&
+		f=$(cat <<-EOF
+		../../never-mind-me
+		../x
+		EOF
+		) &&
+		echo "$f" >expect &&
+		git ls-files $f >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'ls-files -c' '
+	(
+		cd top/sub &&
+		for f in ../y*
+		do
+			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+		done >expect &&
+		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+		set ../x* &&
+		IFS="$new_line" && echo "$*" >>expect && unset IFS &&
+		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'ls-files -o' '
+	(
+		cd top/sub &&
+		for f in ../x*
+		do
+			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+		done >expect &&
+		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+		set ../y* &&
+		IFS="$new_line" && echo "$*" >>expect && unset IFS &&
+		(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_cmp expect actual
+	)
+'
+
+test_done
-- 
1.7.3.1.105.g84915

Re: [PATCH] test ls-files with relative paths

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
> On Sun, Jul 31, 2011 at 05:01:22PM -0700, Junio C Hamano wrote:
>> 
>> Sounds sane; tests?
>
> Certainly. I'm not testing partial commit since it uses the same
> codepath as ls-files --error-unmatch anyways.
>
>  t/t3005-ls-files-relative.sh |   74 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 74 insertions(+), 0 deletions(-)
>  create mode 100755 t/t3005-ls-files-relative.sh

Thanks.  This should be a part of the primary patch, not a standalone patch.

> +test_expect_success 'prepare' '
> +	: >never-mind-me &&
> +	git add never-mind-me &&
> +	mkdir top &&
> +	(
> +		cd top &&
> +		mkdir sub &&
> +		x="x xa xbc xdef xghij xklmno" &&
> +		y=$(echo "$x" | tr x y) &&
> +		touch $x &&
> +		touch $y &&

These are not meant to be quoted; good.

> +		cd sub &&
> +		git add ../x*
> +	)
> +'
> +
> +test_expect_success 'ls-files with mixed levels' '
> +	(
> +		cd top/sub &&
> +		f=$(cat <<-EOF
> +		../../never-mind-me
> +		../x
> +		EOF
> +		) &&
> +		echo "$f" >expect &&
> +		git ls-files $f >actual &&

This is a funny way to do this; why not cat the here-document into expect
and feed $(cat expect) to ls-files?  I suspect that it would become easier
to read and certainly less confusing without need on the readers' side to
scratch their heads wondering "does this $f need to be quoted?"  and such.

> +test_expect_success 'ls-files -c' '
> +	(
> +		cd top/sub &&
> +		for f in ../y*
> +		do
> +			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."

Why not $sq$f$sq?  The above looks harder to read with the braces.

> +		set ../x* &&
> +		IFS="$new_line" && echo "$*" >>expect && unset IFS &&

That's a funny way to say "ls ../x*" isn't it?

[PATCH v2] ls-files: fix pathspec display on error

[Clemens Buchacher]

The following sequence of commands reveals an issue with error
reporting of relative paths:

 $ mkdir sub
 $ cd sub
 $ git ls-files --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.
 $ git commit --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.

This bug is visible only if the normalized path (i.e., the relative
path from the repository root) is longer than the prefix.
Otherwise, the code skips over the normalized path and reads from
an unused memory location which still contains a leftover of the
original command line argument.

So instead, use the existing facilities to deal with relative paths
correctly.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Mon, Aug 01, 2011 at 01:14:14PM -0700, Junio C Hamano wrote:
> 
> Thanks.  This should be a part of the primary patch, not a
> standalone patch.

Guilty on all counts! No changes except to address your comments.
Thanks for reviewing.

Clemens

 builtin/checkout.c           |    2 +-
 builtin/commit.c             |    2 +-
 builtin/ls-files.c           |   11 +++++--
 cache.h                      |    2 +-
 quote.c                      |    8 +++-
 t/t3005-ls-files-relative.sh |   70 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 87 insertions(+), 8 deletions(-)
 create mode 100755 t/t3005-ls-files-relative.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d647a31..a3380d9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);
 	}
 
-	if (report_path_error(ps_matched, pathspec, 0))
+	if (report_path_error(ps_matched, pathspec, NULL, -1))
 		return 1;
 
 	/* "checkout -m path" to recreate conflicted state */
diff --git a/builtin/commit.c b/builtin/commit.c
index cb73857..c2db12a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -274,7 +274,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	return report_path_error(m, pattern, prefix ? strlen(prefix) : 0);
+	return report_path_error(m, pattern, prefix, -1);
 }
 
 static void add_remove_files(struct string_list *list)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 0e98bff..fef5642 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -353,11 +353,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
+int report_path_error(const char *ps_matched, const char **pathspec,
+		const char *prefix, int prefix_len)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
 	 */
+	struct strbuf sb = STRBUF_INIT;
+	const char *name;
 	int num, errors = 0;
 	for (num = 0; pathspec[num]; num++) {
 		int other, found_dup;
@@ -382,10 +385,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
 		if (found_dup)
 			continue;
 
+		name = quote_path_relative(pathspec[num], -1, &sb, prefix);
 		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec[num] + prefix_len);
+		      name);
 		errors++;
 	}
+	strbuf_release(&sb);
 	return errors;
 }
 
@@ -577,7 +582,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix_len);
+		bad = report_path_error(ps_matched, pathspec, prefix, prefix_len);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/cache.h b/cache.h
index 1b5d861..dd3edaa 100644
--- a/cache.h
+++ b/cache.h
@@ -1189,7 +1189,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 #define ws_tab_width(rule)     ((rule) & WS_TAB_WIDTH_MASK)
 
 /* ls-files */
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset);
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/quote.c b/quote.c
index 63d3b01..532fd3b 100644
--- a/quote.c
+++ b/quote.c
@@ -325,8 +325,12 @@ static const char *path_relative(const char *in, int len,
 
 	if (len < 0)
 		len = strlen(in);
-	if (prefix && prefix_len < 0)
-		prefix_len = strlen(prefix);
+	if (prefix_len < 0) {
+		if (prefix)
+			prefix_len = strlen(prefix);
+		else
+			prefix_len = 0;
+	}
 
 	off = 0;
 	i = 0;
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
new file mode 100755
index 0000000..3c3ff5e
--- /dev/null
+++ b/t/t3005-ls-files-relative.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='ls-files tests with relative paths
+
+This test runs git ls-files with various relative path arguments.
+'
+
+. ./test-lib.sh
+
+new_line='
+'
+sq=\'
+
+test_expect_success 'prepare' '
+	: >never-mind-me &&
+	git add never-mind-me &&
+	mkdir top &&
+	(
+		cd top &&
+		mkdir sub &&
+		x="x xa xbc xdef xghij xklmno" &&
+		y=$(echo "$x" | tr x y) &&
+		touch $x &&
+		touch $y &&
+		cd sub &&
+		git add ../x*
+	)
+'
+
+test_expect_success 'ls-files with mixed levels' '
+	(
+		cd top/sub &&
+		cat >expect <<-EOF &&
+		../../never-mind-me
+		../x
+		EOF
+		git ls-files $(cat expect) >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'ls-files -c' '
+	(
+		cd top/sub &&
+		for f in ../y*
+		do
+			echo "error: pathspec $sq$f$sq did not match any file(s) known to git."
+		done >expect &&
+		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+		ls ../x* >>expect &&
+		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'ls-files -o' '
+	(
+		cd top/sub &&
+		for f in ../x*
+		do
+			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+		done >expect &&
+		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+		ls ../y* >>expect &&
+		(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_cmp expect actual
+	)
+'
+
+test_done
-- 
1.7.3.1.105.g84915

Re: [PATCH v2] ls-files: fix pathspec display on error

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> ... No changes except to address your comments.
> Thanks for reviewing.

Thank *you* for re-rolling.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 0e98bff..fef5642 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -353,11 +353,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
>  	}
>  }
>  
> -int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
> +int report_path_error(const char *ps_matched, const char **pathspec,
> +		const char *prefix, int prefix_len)
>  {
>  	/*
>  	 * Make sure all pathspec matched; otherwise it is an error.
>  	 */
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *name;
>  	int num, errors = 0;
>  	for (num = 0; pathspec[num]; num++) {
>  		int other, found_dup;

> @@ -382,10 +385,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
>  		if (found_dup)
>  			continue;
>  
> +		name = quote_path_relative(pathspec[num], -1, &sb, prefix);
>  		error("pathspec '%s' did not match any file(s) known to git.",
> -		      pathspec[num] + prefix_len);
> +		      name);

Is prefix_len still being used in this function?

> +		ls ../x* >>expect &&
> +		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&

The "|| true" construct says "ls-files _may_ exit with non-zero, but we do
not care". Shouldn't we be actively expecting it to exit with non-zero,
using test-must-fail here?

I am not sure if passing NULL in checkout.c is the right thing to do. I
know that is not the problem this patch introduces, but it leads to this
inconsistent behaviour:

	$ cd Documentation
        $ git checkout nosuch.txt
        error: pathspec 'Documentation/nosuch.txt' did not match...
        $ git commit nosuch.txt
        error: pathspec 'nosuch.txt' did not match...

I suspect that cmd_checkout() should pass prefix down to checkout_paths()
so that the latter can properly strip it from the pathspec.

Perhaps this squashed in on top of your patch...

 builtin/checkout.c           |    6 +++---
 builtin/commit.c             |    2 +-
 builtin/ls-files.c           |    5 ++---
 cache.h                      |    2 +-
 t/t3005-ls-files-relative.sh |    6 +++---
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index efe5e8e..a5717f1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -201,7 +201,7 @@ static int checkout_merged(int pos, struct checkout *state)
 }
 
 static int checkout_paths(struct tree *source_tree, const char **pathspec,
-			  struct checkout_opts *opts)
+			  const char *prefix, struct checkout_opts *opts)
 {
 	int pos;
 	struct checkout state;
@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);
 	}
 
-	if (report_path_error(ps_matched, pathspec, NULL, -1))
+	if (report_path_error(ps_matched, pathspec, prefix))
 		return 1;
 
 	/* "checkout -m path" to recreate conflicted state */
@@ -1060,7 +1060,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)
 			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\nchecking out of the index."));
 
-		return checkout_paths(source_tree, pathspec, &opts);
+		return checkout_paths(source_tree, pathspec, prefix, &opts);
 	}
 
 	if (patch_mode)
diff --git a/builtin/commit.c b/builtin/commit.c
index a16d00b..9679a99 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -272,7 +272,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	return report_path_error(m, pattern, prefix, -1);
+	return report_path_error(m, pattern, prefix);
 }
 
 static void add_remove_files(struct string_list *list)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 72b986f..468bb13 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -388,8 +388,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec,
-		const char *prefix, int prefix_len)
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
@@ -616,7 +615,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix, prefix_len);
+		bad = report_path_error(ps_matched, pathspec, prefix);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/cache.h b/cache.h
index 80c60b4..d55a6bb 100644
--- a/cache.h
+++ b/cache.h
@@ -1175,7 +1175,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 #define ws_tab_width(rule)     ((rule) & WS_TAB_WIDTH_MASK)
 
 /* ls-files */
-int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
index 3c3ff5e..a2b63e2 100755
--- a/t/t3005-ls-files-relative.sh
+++ b/t/t3005-ls-files-relative.sh
@@ -48,7 +48,7 @@ test_expect_success 'ls-files -c' '
 		done >expect &&
 		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
 		ls ../x* >>expect &&
-		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_must_fail git ls-files -c --error-unmatch ../[xy]* >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '
@@ -58,11 +58,11 @@ test_expect_success 'ls-files -o' '
 		cd top/sub &&
 		for f in ../x*
 		do
-			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+			echo "error: pathspec $sq$f$sq did not match any file(s) known to git."
 		done >expect &&
 		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
 		ls ../y* >>expect &&
-		(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_must_fail git ls-files -o --error-unmatch ../[xy]* >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '

Re: [PATCH v2] commit: allow partial commits with relative paths

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/setup.c b/setup.c
> index 5ea5502..2c51a9a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
>  	return pathspec;
>  }
>  
> +const char *pathspec_prefix(const char *prefix, const char **pathspec)
> +{

As a public function, this sorely needs a comment that describes what it
does. More importantly, when I tried to come up with an example
description, it became very clear that this neither prefixes anything to
pathspec, nor prefixes pathspec to anything else.

As an internal helper in ls-files implementation it was perfectly
fine that the function was slightly misnamed, but if you are making it
into a public API, we should get its name right.

Perhaps "common_prefix()"?

Don't you also want to consolidate dir.c:common_prefix() with this?

What happens when pathspec has the recently introduced magic elements,
e.g. ':/' that widens the match to the whole tree?

Re: [PATCH v2] commit: allow partial commits with relative paths

[Clemens Buchacher]

Hi Junio,

On Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:
> 
> Perhaps "common_prefix()"?

Yes, I was thinking the same thing actually.

> Don't you also want to consolidate dir.c:common_prefix() with this?

I wasn't aware of it. I'm really swamped right now, but I'll take a
look at it soon.

Clemens

Re: [PATCH v2] commit: allow partial commits with relative paths

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:
>> 
>> Perhaps "common_prefix()"?
>
> Yes, I was thinking the same thing actually.
>
>> Don't you also want to consolidate dir.c:common_prefix() with this?
>
> I wasn't aware of it. I'm really swamped right now, but I'll take a
> look at it soon.

It is not too big a deal, though, so I'll merge this one to next and we
can fix it up in-tree later.

Thanks.

renaming pathspec_prefix (was: Re: [PATCH v2] commit: allow partial commits with relative paths)

[Clemens Buchacher]

On Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > diff --git a/setup.c b/setup.c
> > index 5ea5502..2c51a9a 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
> >  	return pathspec;
> >  }
> >  
> > +const char *pathspec_prefix(const char *prefix, const char **pathspec)
> > +{
> 
> As a public function, this sorely needs a comment that describes what it
> does. More importantly, when I tried to come up with an example
> description, it became very clear that this neither prefixes anything to
> pathspec, nor prefixes pathspec to anything else.
> 
> As an internal helper in ls-files implementation it was perfectly
> fine that the function was slightly misnamed, but if you are making it
> into a public API, we should get its name right.
> 
> Perhaps "common_prefix()"?
> 
> Don't you also want to consolidate dir.c:common_prefix() with this?

Yes. This should do it:

[PATCH 1/3] remove prefix argument from pathspec_prefix
[PATCH 2/3] consolidate pathspec_prefix and common_prefix
[PATCH 3/3] rename pathspec_prefix -> common_prefix and move to

> What happens when pathspec has the recently introduced magic elements,
> e.g. ':/' that widens the match to the whole tree?

If I understand correctly that is resolved in get_pathspec. And
pathspec_prefix (now common_prefix) is called later.

Clemens

[PATCH 1/3] remove prefix argument from pathspec_prefix

[Clemens Buchacher]

Passing a prefix to a function that is supposed to find the prefix
is strange. And it's really only used if the pathspec is NULL. Make
the callers handle this case instead.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/commit.c   |    5 +++--
 builtin/ls-files.c |    2 +-
 cache.h            |    2 +-
 setup.c            |    4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index cbc9613..64fe501 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -255,8 +255,9 @@ static int list_paths(struct string_list *list, const char *with_tree,
 	m = xcalloc(1, i);
 
 	if (with_tree) {
-		const char *max_prefix = pathspec_prefix(prefix, pattern);
-		overlay_tree_on_cache(with_tree, max_prefix);
+		char *max_prefix = pathspec_prefix(pattern);
+		overlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);
+		free(max_prefix);
 	}
 
 	for (i = 0; i < active_nr; i++) {
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e8a800d..a54c2a2 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		strip_trailing_slash_from_submodules();
 
 	/* Find common prefix for all pathspec's */
-	max_prefix = pathspec_prefix(prefix, pathspec);
+	max_prefix = pathspec_prefix(pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
diff --git a/cache.h b/cache.h
index 607c2ea..0ccc84d 100644
--- a/cache.h
+++ b/cache.h
@@ -444,7 +444,7 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
-extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
+extern char *pathspec_prefix(const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
diff --git a/setup.c b/setup.c
index 27c1d47..0906790 100644
--- a/setup.c
+++ b/setup.c
@@ -236,13 +236,13 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
-const char *pathspec_prefix(const char *prefix, const char **pathspec)
+char *pathspec_prefix(const char **pathspec)
 {
 	const char **p, *n, *prev;
 	unsigned long max;
 
 	if (!pathspec)
-		return prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;
+		return NULL;
 
 	prev = NULL;
 	max = PATH_MAX;
-- 
1.7.6.1

[PATCH 2/3] consolidate pathspec_prefix and common_prefix

[Clemens Buchacher]

The implementation from pathspec_prefix (slightly modified)
replaces the current common_prefix, because it also respects glob
characters.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I wonder if PATH_MAX is needed and respected everywhere. Do we have
any previous experience with very long paths?

Clemens

 dir.c   |   52 ++++++++++++++++++++++++++--------------------------
 dir.h   |    1 +
 setup.c |   29 ++---------------------------
 3 files changed, 29 insertions(+), 53 deletions(-)

diff --git a/dir.c b/dir.c
index 08281d2..7099fb3 100644
--- a/dir.c
+++ b/dir.c
@@ -34,37 +34,37 @@ int fnmatch_icase(const char *pattern, const char *string, int flags)
 	return fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));
 }
 
-static int common_prefix(const char **pathspec)
+unsigned long common_prefix_len(const char **pathspec)
 {
-	const char *path, *slash, *next;
-	int prefix;
+	const char *n, *first;
+	unsigned long max;
 
 	if (!pathspec)
 		return 0;
 
-	path = *pathspec;
-	slash = strrchr(path, '/');
-	if (!slash)
-		return 0;
-
-	/*
-	 * The first 'prefix' characters of 'path' are common leading
-	 * path components among the pathspecs we have seen so far,
-	 * including the trailing slash.
-	 */
-	prefix = slash - path + 1;
-	while ((next = *++pathspec) != NULL) {
-		int len, last_matching_slash = -1;
-		for (len = 0; len < prefix && next[len] == path[len]; len++)
-			if (next[len] == '/')
-				last_matching_slash = len;
-		if (len == prefix)
-			continue;
-		if (last_matching_slash < 0)
-			return 0;
-		prefix = last_matching_slash + 1;
+	first = *pathspec;
+	max = PATH_MAX;
+	while ((n = *pathspec++)) {
+		int i, len = 0;
+		for (i = 0; i < max; i++) {
+			char c = n[i];
+			if (!c || c != first[i] || is_glob_special(c))
+				break;
+			if (c == '/')
+				len = i+1;
+		}
+		if (len < max) {
+			max = len;
+			if (!max)
+				break;
+		}
 	}
-	return prefix;
+
+	/* Nothing in the first PATH_MAX characters? */
+	if (max > 0 && first[max-1] != '/')
+		max = 0;
+
+	return max;
 }
 
 int fill_directory(struct dir_struct *dir, const char **pathspec)
@@ -76,7 +76,7 @@ int fill_directory(struct dir_struct *dir, const char **pathspec)
 	 * Calculate common prefix for the pathspec, and
 	 * use that to optimize the directory walk
 	 */
-	len = common_prefix(pathspec);
+	len = common_prefix_len(pathspec);
 	path = "";
 
 	if (len)
diff --git a/dir.h b/dir.h
index 433b5b4..0e55b71 100644
--- a/dir.h
+++ b/dir.h
@@ -64,6 +64,7 @@ struct dir_struct {
 #define MATCHED_RECURSIVELY 1
 #define MATCHED_FNMATCH 2
 #define MATCHED_EXACTLY 3
+extern unsigned long common_prefix_len(const char **pathspec);
 extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);
 extern int match_pathspec_depth(const struct pathspec *pathspec,
 				const char *name, int namelen,
diff --git a/setup.c b/setup.c
index 0906790..0c60dbd 100644
--- a/setup.c
+++ b/setup.c
@@ -238,34 +238,9 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 
 char *pathspec_prefix(const char **pathspec)
 {
-	const char **p, *n, *prev;
-	unsigned long max;
+	unsigned long len = common_prefix_len(pathspec);
 
-	if (!pathspec)
-		return NULL;
-
-	prev = NULL;
-	max = PATH_MAX;
-	for (p = pathspec; (n = *p) != NULL; p++) {
-		int i, len = 0;
-		for (i = 0; i < max; i++) {
-			char c = n[i];
-			if (prev && prev[i] != c)
-				break;
-			if (!c || c == '*' || c == '?')
-				break;
-			if (c == '/')
-				len = i+1;
-		}
-		prev = n;
-		if (len < max) {
-			max = len;
-			if (!max)
-				break;
-		}
-	}
-
-	return max ? xmemdupz(prev, max) : NULL;
+	return len ? xmemdupz(prefix, len) : NULL;
 }
 
 /*
-- 
1.7.6.1

[PATCH 3/3] rename pathspec_prefix -> common_prefix and move to dir.[ch]

[Clemens Buchacher]

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/commit.c   |    2 +-
 builtin/ls-files.c |    2 +-
 cache.h            |    1 -
 dir.c              |   11 +++++++++++
 dir.h              |    1 +
 setup.c            |    7 -------
 6 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 64fe501..b9ab5ef 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -255,7 +255,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 	m = xcalloc(1, i);
 
 	if (with_tree) {
-		char *max_prefix = pathspec_prefix(pattern);
+		char *max_prefix = common_prefix(pattern);
 		overlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);
 		free(max_prefix);
 	}
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a54c2a2..7cff175 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		strip_trailing_slash_from_submodules();
 
 	/* Find common prefix for all pathspec's */
-	max_prefix = pathspec_prefix(pathspec);
+	max_prefix = common_prefix(pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
diff --git a/cache.h b/cache.h
index 0ccc84d..586b987 100644
--- a/cache.h
+++ b/cache.h
@@ -444,7 +444,6 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
-extern char *pathspec_prefix(const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
diff --git a/dir.c b/dir.c
index 7099fb3..f606da9 100644
--- a/dir.c
+++ b/dir.c
@@ -67,6 +67,17 @@ unsigned long common_prefix_len(const char **pathspec)
 	return max;
 }
 
+/*
+ * Returns a copy of the longest leading path common among all
+ * pathspecs.
+ */
+char *common_prefix(const char **pathspec)
+{
+	unsigned long len = common_prefix_len(pathspec);
+
+	return len ? xmemdupz(*pathspec, len) : NULL;
+}
+
 int fill_directory(struct dir_struct *dir, const char **pathspec)
 {
 	const char *path;
diff --git a/dir.h b/dir.h
index 0e55b71..9a33d23 100644
--- a/dir.h
+++ b/dir.h
@@ -65,6 +65,7 @@ struct dir_struct {
 #define MATCHED_FNMATCH 2
 #define MATCHED_EXACTLY 3
 extern unsigned long common_prefix_len(const char **pathspec);
+extern char *common_prefix(const char **pathspec);
 extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);
 extern int match_pathspec_depth(const struct pathspec *pathspec,
 				const char *name, int namelen,
diff --git a/setup.c b/setup.c
index 0c60dbd..52bbb70 100644
--- a/setup.c
+++ b/setup.c
@@ -236,13 +236,6 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
-char *pathspec_prefix(const char **pathspec)
-{
-	unsigned long len = common_prefix_len(pathspec);
-
-	return len ? xmemdupz(prefix, len) : NULL;
-}
-
 /*
  * Test if it looks like we're at a git directory.
  * We want to see:
-- 
1.7.6.1

Re: [PATCH 1/3] remove prefix argument from pathspec_prefix

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Passing a prefix to a function that is supposed to find the prefix
> is strange. And it's really only used if the pathspec is NULL. Make
> the callers handle this case instead.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>

While I find the above rationale a reasonable justification for the
removal of a parameter from the function, it does not seem to justify why
the type of the returned value from the function needed to be changed.

Is it because we no longer ever return "prefix" we pass in which is a
pointer to a constant memory region to begin with?

We also didn't free() in the earlier code (because we do not know if it
can be freed) and leaking xmemdupz() if the function didn't return the
"prefix", but now you plugged the small leak. Isn't it something you
should advertise?

> diff --git a/builtin/commit.c b/builtin/commit.c
> index cbc9613..64fe501 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -255,8 +255,9 @@ static int list_paths(struct string_list *list, const char *with_tree,
>  	m = xcalloc(1, i);
>  
>  	if (with_tree) {
> -		const char *max_prefix = pathspec_prefix(prefix, pattern);
> -		overlay_tree_on_cache(with_tree, max_prefix);
> +		char *max_prefix = pathspec_prefix(pattern);
> +		overlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);
> +		free(max_prefix);
>  	}
>  
>  	for (i = 0; i < active_nr; i++) {
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index e8a800d..a54c2a2 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>  		strip_trailing_slash_from_submodules();
>  
>  	/* Find common prefix for all pathspec's */
> -	max_prefix = pathspec_prefix(prefix, pathspec);
> +	max_prefix = pathspec_prefix(pathspec);
>  	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
>  
>  	/* Treat unmatching pathspec elements as errors */
> diff --git a/cache.h b/cache.h
> index 607c2ea..0ccc84d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -444,7 +444,7 @@ extern void set_git_work_tree(const char *tree);
>  #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>  
>  extern const char **get_pathspec(const char *prefix, const char **pathspec);
> -extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
> +extern char *pathspec_prefix(const char **pathspec);
>  extern void setup_work_tree(void);
>  extern const char *setup_git_directory_gently(int *);
>  extern const char *setup_git_directory(void);
> diff --git a/setup.c b/setup.c
> index 27c1d47..0906790 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -236,13 +236,13 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
>  	return pathspec;
>  }
>  
> -const char *pathspec_prefix(const char *prefix, const char **pathspec)
> +char *pathspec_prefix(const char **pathspec)
>  {
>  	const char **p, *n, *prev;
>  	unsigned long max;
>  
>  	if (!pathspec)
> -		return prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;
> +		return NULL;
>  
>  	prev = NULL;
>  	max = PATH_MAX;

Re: [PATCH 1/3] remove prefix argument from pathspec_prefix

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Is it because we no longer ever return "prefix" we pass in which is a
> pointer to a constant memory region to begin with?
>
> We also didn't free() in the earlier code (because we do not know if it
> can be freed) and leaking xmemdupz() if the function didn't return the
> "prefix", but now you plugged the small leak. Isn't it something you
> should advertise?

Nah, the leak is not necessarily plugged in all callers anyway, so scratch
that part. I've rewritten it like this:

commit 5879f5684cfe8a38326b4ffd078f96e35c68e640
Author: Clemens Buchacher <drizzd@aon.at>
Date:   Sun Sep 4 12:41:59 2011 +0200

    remove prefix argument from pathspec_prefix
    
    Passing a prefix to a function that is supposed to find the prefix is
    strange. And it's really only used if the pathspec is NULL. Make the
    callers handle this case instead.
    
    As we are always returning a fresh copy of a string (or NULL), change the
    type of the returned value to non-const "char *".
    
    Signed-off-by: Clemens Buchacher <drizzd@aon.at>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH 1/3] remove prefix argument from pathspec_prefix

[Clemens Buchacher]

On Tue, Sep 06, 2011 at 12:58:16PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Is it because we no longer ever return "prefix" we pass in which is a
> > pointer to a constant memory region to begin with?
> >
> > We also didn't free() in the earlier code (because we do not know if it
> > can be freed) and leaking xmemdupz() if the function didn't return the
> > "prefix", but now you plugged the small leak. Isn't it something you
> > should advertise?
> 
> Nah, the leak is not necessarily plugged in all callers anyway, so scratch
> that part. I've rewritten it like this:

Ok.

The only other caller, though, is cmd_ls_files(). And it would be
trivial to plug that leak as well.

But is it considered a leak, if the program is going to terminate
right after the function returns? I recall that being used as an
argument against free'ing such memory on this list.

On the other hand, that practice makes it harder to analyze leaks
using memory leak detectors like for example valgrind.

Clemens

Re: [PATCH 1/3] remove prefix argument from pathspec_prefix

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

>> > We also didn't free() in the earlier code (because we do not know if it
>> > can be freed) and leaking xmemdupz() if the function didn't return the
>> > "prefix", but now you plugged the small leak. Isn't it something you
>> > should advertise?
>> 
>> Nah, the leak is not necessarily plugged in all callers anyway, so scratch
>> that part. I've rewritten it like this:
>
> Ok.
>
> The only other caller, though, is cmd_ls_files(). And it would be
> trivial to plug that leak as well.
>
> But is it considered a leak, if the program is going to terminate
> right after the function returns?

it not a big deal to leak immediately before exit, and a patch whose sole
purpose is to plug them is of little value.

But if you are already in the vicinity, updating a function that happens
to have such a leak, the cost to decide not plugging the leak would be
about the same as plugging it, so it would be worth doing in such a case.