Skip to contentSkip to conversation

Possible bug

PATCH In review Reuben Thomas 6 participants 37 messages Jun 15, 2016

lore
Share & tools
lore

Reuben Thomas

author
#0 lore raw

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?

Signature · 1 line
http://rrt.sc3d.org
#1 lore raw

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

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 <[email protected]>
Signed-off-by Michael J Gruber <[email protected]>

---
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(-)
builtin/commit.c +1 -1
index e1af9b1..431590c 100644
@@ -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];
Signature · 1 line
1.7.6.336.gdf067
#2 lore raw

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

Quoted 3 lines · Michael J Gruber

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?

#3 lore raw

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

Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:

Quoted 7 lines
Quoted 3 lines · Michael J Gruber

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?

Quoted 2 lines

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.

Quoted 2 lines

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

#4 lore raw

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

Quoted 5 lines · Michael J Gruber

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.

#5 lore raw

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

Junio C Hamano venit, vidit, dixit 27.07.2011 17:41:

Quoted 8 lines
Quoted 5 lines · Michael J Gruber

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.

Quoted 8 lines

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

#6 lore raw

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

In reply to Michael J Gruber
Quoted 3 lines · Michael J Gruber

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).

author
#7 lore raw

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

In reply to Michael J Gruber
Quoted 11 lines · Michael J Gruber

Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:

Quoted 4 lines

Michael J Gruber <[email protected]> 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'.

Signature · 1 line
http://rrt.sc3d.org
#8 lore raw

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

Reuben Thomas venit, vidit, dixit 27.07.2011 11:45:

Quoted 27 lines · Michael J Gruber
Quoted 11 lines

Junio C Hamano venit, vidit, dixit 25.07.2011 21:02:

Quoted 4 lines

Michael J Gruber <[email protected]> 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
Quoted 2 lines

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

#9 lore raw

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

On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber
<[email protected]> wrote:
[snip]

Quoted 2 lines

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

#10 lore raw

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

Quoted 20 lines · John Szakmeister

On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber
<[email protected]> wrote:
[snip]

Quoted 2 lines

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] .

Quoted 8 lines
 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}).

#11 lore raw

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

In reply to John Szakmeister

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 <[email protected]>

---
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(-)
builtin/ls-files.c +1 -1
index 1570123..e0b1401 100644
@@ -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;
Signature · 1 line
1.7.6.336.gdf067
#12 lore raw

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

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 <[email protected]>

---

Quoted 3 lines · Michael J Gruber

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(-)
builtin/checkout.c +1 -1
index d647a31..a3380d9 100644
@@ -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 */
builtin/commit.c +1 -1
index e1af9b1..a16d00b 100644
@@ -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)
builtin/ls-files.c +8 -3
index 1570123..72b986f 100644
@@ -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");
cache.h +1 -1
index 9e12d55..86518fb 100644
@@ -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);
quote.c +6 -2
index 63d3b01..532fd3b 100644
@@ -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;
Signature · 1 line
1.7.3.1.105.g84915
#13 lore raw

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

Quoted 10 lines · 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.
Quoted 8 lines

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?

#14 lore raw

PATCH 1/1 test ls-files with relative paths

Signed-off-by Clemens Buchacher <[email protected]>

---

Quoted 2 lines · Junio C Hamano

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
t/t3005-ls-files-relative.sh new file +74 -0
new file mode 100755
index 0000000..e9a2f75
@@ -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
Signature · 1 line
1.7.3.1.105.g84915
#15 lore raw

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

Quoted 4 lines · Clemens Buchacher
Signed-off-by Clemens Buchacher <[email protected]>

---

Quoted 1 line · Junio C Hamano
Quoted 8 lines
Quoted 1 line

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.

t/t3005-ls-files-relative.sh line 24

Quoted 11 lines
t/t3005-ls-files-relative.sh new file +11 -0
+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.

Quoted 5 lines
t/t3005-ls-files-relative.sh new file +5 -0
+		cd sub &&
+		git add ../x*
+	)
+'
+

t/t3005-ls-files-relative.sh line 39

Quoted 10 lines
t/t3005-ls-files-relative.sh new file +10 -0
+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.

t/t3005-ls-files-relative.sh line 49

Quoted 6 lines
t/t3005-ls-files-relative.sh new file +6 -0
+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.

Review

Quoted 2 lines
(quoted diff) +2 -0
+		set ../x* &&
+		IFS="$new_line" && echo "$*" >>expect && unset IFS &&

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

#16 lore raw

PATCH v2 1/1 ls-files: fix pathspec display on error

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 <[email protected]>

---

Quoted 3 lines · Junio C Hamano

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
builtin/checkout.c +1 -1
index d647a31..a3380d9 100644
@@ -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 */
builtin/commit.c +1 -1
index cb73857..c2db12a 100644
@@ -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)
builtin/ls-files.c +8 -3
index 0e98bff..fef5642 100644
@@ -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");
cache.h +1 -1
index 1b5d861..dd3edaa 100644
@@ -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);
quote.c +6 -2
index 63d3b01..532fd3b 100644
@@ -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;
t/t3005-ls-files-relative.sh new file +70 -0

70 lines not on this page · raw message

Signature · 1 line
1.7.3.1.105.g84915
#17 lore raw

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

Quoted 2 lines · Clemens Buchacher

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

Thank *you* for re-rolling.

Quoted 22 lines
builtin/ls-files.c +4 -1
index 0e98bff..fef5642 100644
@@ -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;

builtin/ls-files.c int report_path_error(const char *ps_matched, const char **pathspec, int prefix_ · line 390

Quoted 7 lines
builtin/ls-files.c +2 -1
@@ -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?

builtin/ls-files.c int report_path_error(const char *ps_matched, const char **pathspec, int prefix_ · line 392

Quoted 2 lines
builtin/ls-files.c +2 -0
+		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(-)
builtin/checkout.c +3 -3
index efe5e8e..a5717f1 100644
@@ -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)
builtin/commit.c +1 -1
index a16d00b..9679a99 100644
@@ -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)
builtin/ls-files.c +2 -3
index 72b986f..468bb13 100644
@@ -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");
cache.h +1 -1
index 80c60b4..d55a6bb 100644
@@ -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);
t/t3005-ls-files-relative.sh +3 -3
index 3c3ff5e..a2b63e2 100755
@@ -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
 	)
 '
author
#18 lore raw

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

In reply to Michael J Gruber
Quoted 13 lines · Michael J Gruber

Reuben Thomas venit, vidit, dixit 27.07.2011 11:45:

Quoted 12 lines
$ 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
Quoted 7 lines
Quoted 2 lines

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".

Quoted 1 line

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

Thanks for testing that.

Quoted 2 lines

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.

Signature · 1 line
http://rrt.sc3d.org
#19 lore raw

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

In reply to Junio C Hamano
Quoted 11 lines · Junio C Hamano
Quoted 3 lines · Michael J Gruber

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.

#20 lore raw

PATCH 1/1 commit: allow partial commits with relative paths

In reply to 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).

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 <[email protected]>
Analyzed-by Michael J Gruber <[email protected]>
Signed-off-by Clemens Buchacher <[email protected]>
Quoted 2 lines · Michael J Gruber

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(-)
builtin/commit.c +4 -2
index a16d00b..c2db12a 100644
@@ -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];
builtin/ls-files.c +2 -36
index 72b986f..fef5642 100644
@@ -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) {
cache.h +1 -0
index 86518fb..dd3edaa 100644
@@ -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);
setup.c +32 -0
index 5ea5502..2c51a9a 100644
@@ -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:
Signature · 1 line
1.7.3.1.105.g84915
#21 lore raw

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

Clemens Buchacher venit, vidit, dixit 29.07.2011 15:35:

Quoted 16 lines

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 <[email protected]>
Analyzed-by Michael J Gruber <[email protected]>
Signed-off-by Clemens Buchacher <[email protected]>
Quoted 7 lines
Quoted 2 lines · Michael J Gruber

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?

Quoted 1 line

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...

Quoted 137 lines

Clemens

---
 builtin/commit.c   |    6 ++++--
 builtin/ls-files.c |   38 ++------------------------------------
 cache.h            |    1 +
 setup.c            |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 39 insertions(+), 38 deletions(-)
builtin/commit.c +4 -2
index a16d00b..c2db12a 100644
@@ -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];
builtin/ls-files.c +2 -36
index 72b986f..fef5642 100644
@@ -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) {
cache.h +1 -0
index 86518fb..dd3edaa 100644
@@ -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);
setup.c +32 -0
index 5ea5502..2c51a9a 100644
@@ -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:
#22 lore raw

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

Quoted 5 lines · Michael J Gruber
Quoted 2 lines

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:

Quoted 1 line
Quoted 1 line
 4 files changed, 39 insertions(+), 38 deletions(-)

Clemens

#23 lore raw

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

Clemens Buchacher venit, vidit, dixit 30.07.2011 19:00:

Quoted 9 lines · Michael J Gruber
Quoted 5 lines
Quoted 2 lines

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.

Quoted 9 lines

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

Quoted 7 lines

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:

Quoted 1 line
Quoted 1 line
 4 files changed, 39 insertions(+), 38 deletions(-)

Clemens

#24 lore raw

PATCH v2 1/1 commit: allow partial commits with relative paths

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 <[email protected]>
Analyzed-by Michael J Gruber <[email protected]>
Signed-off-by Clemens Buchacher <[email protected]>

---

Quoted 2 lines · Michael J Gruber

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(-)
builtin/commit.c +4 -2
index a16d00b..c2db12a 100644
@@ -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];
builtin/ls-files.c +2 -36
index 72b986f..fef5642 100644
@@ -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) {
cache.h +1 -0
index 86518fb..dd3edaa 100644
@@ -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);
setup.c +32 -0
index 5ea5502..2c51a9a 100644
@@ -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:
Signature · 1 line
1.7.3.1.105.g84915
#25 lore raw

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

Quoted 7 lines · Clemens Buchacher

...
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.

#26 lore raw

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

In reply to Clemens Buchacher

setup.c const char **get_pathspec(const char *prefix, const char **pathspec) · line 268

Quoted 10 lines · Clemens Buchacher
setup.c +2 -0
index 5ea5502..2c51a9a 100644
@@ -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?

#27 lore raw

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

Hi Junio,

Quoted 2 lines · Junio C Hamano

Perhaps "common_prefix()"?

Yes, I was thinking the same thing actually.

Quoted 1 line

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

#28 lore raw

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

Quoted 10 lines · Clemens Buchacher
Quoted 2 lines · Junio C Hamano

Perhaps "common_prefix()"?

Yes, I was thinking the same thing actually.

Quoted 1 line

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.

#29 lore raw

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

In reply to Junio C Hamano
Quoted 18 lines · Junio C Hamano

setup.c const char **get_pathspec(const char *prefix, const char **pathspec) · line 268

Quoted 10 lines · Clemens Buchacher
setup.c +2 -0
index 5ea5502..2c51a9a 100644
@@ -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.

Quoted 7 lines

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

Quoted 2 lines

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

#30 lore raw

PATCH 1/3 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.

Signed-off-by Clemens Buchacher <[email protected]>
---
 builtin/commit.c   |    5 +++--
 builtin/ls-files.c |    2 +-
 cache.h            |    2 +-
 setup.c            |    4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)
builtin/commit.c +3 -2
index cbc9613..64fe501 100644
@@ -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++) {
builtin/ls-files.c +1 -1
index e8a800d..a54c2a2 100644
@@ -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 */
cache.h +1 -1
index 607c2ea..0ccc84d 100644
@@ -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);
setup.c +2 -2
index 27c1d47..0906790 100644
@@ -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;
Signature · 1 line
1.7.6.1
#31 lore raw

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

Quoted 5 lines · 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 <[email protected]>

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?

Quoted 62 lines
builtin/commit.c +3 -2
index cbc9613..64fe501 100644
@@ -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++) {
builtin/ls-files.c +1 -1
index e8a800d..a54c2a2 100644
@@ -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 */
cache.h +1 -1
index 607c2ea..0ccc84d 100644
@@ -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);
setup.c +2 -2
index 27c1d47..0906790 100644
@@ -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;
#32 lore raw

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

Quoted 7 lines · Junio C Hamano

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 <[email protected]>
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 <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>
#33 lore raw

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

Quoted 5 lines · Junio C Hamano
Quoted 3 lines · Junio C Hamano

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

Quoted 7 lines
Quoted 4 lines

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

#34 lore raw

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

Quoted 8 lines · Clemens Buchacher
Quoted 7 lines
Quoted 4 lines

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:

Quoted 7 lines

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.

#35 lore raw

PATCH 2/3 consolidate pathspec_prefix and common_prefix

In reply to 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 <[email protected]>

---

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(-)
dir.c +26 -26
index 08281d2..7099fb3 100644
@@ -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)
dir.h +1 -0
index 433b5b4..0e55b71 100644
@@ -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,
setup.c +2 -27
index 0906790..0c60dbd 100644
@@ -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;
 }
 
 /*
Signature · 1 line
1.7.6.1
#36 lore raw

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

In reply to Clemens Buchacher
Signed-off-by Clemens Buchacher <[email protected]>
---
 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(-)
builtin/commit.c +1 -1
index 64fe501..b9ab5ef 100644
@@ -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);
 	}
builtin/ls-files.c +1 -1
index a54c2a2..7cff175 100644
@@ -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 */
cache.h +0 -1
index 0ccc84d..586b987 100644
@@ -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);
dir.c +11 -0
index 7099fb3..f606da9 100644
@@ -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;
dir.h +1 -0
index 0e55b71..9a33d23 100644
@@ -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,
setup.c +0 -7
index 0c60dbd..52bbb70 100644
@@ -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:
Signature · 1 line
1.7.6.1