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

Michael J Gruber [off-list ref] 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?

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

Michael J Gruber [off-list ref] writes:

quoted

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

On 27 July 2011 09:22, Michael J Gruber [off-list ref] wrote:

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

quoted

Michael J Gruber [off-list ref] 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

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

On 27 July 2011 09:22, Michael J Gruber [off-list ref] wrote:

quoted

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

quoted

Michael J Gruber [off-list ref] 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

On 27 July 2011 10:53, Michael J Gruber [off-list ref] wrote:

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

quoted

$ 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

On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber
[off-list ref] 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

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

On Wed, Jul 27, 2011 at 12:19 PM, John Szakmeister [off-list ref] wrote:

On Wed, Jul 27, 2011 at 5:53 AM, Michael J Gruber
[off-list ref] wrote:
[snip]

quoted

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

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