[PATCH 0/2] reset: handle submodule with trailing slash

The first patch is the important one here, the second one I noticed
while checking if any other commands fail to handle submodule paths with
a trailing slash and is just a simplification.

John Keeping (2):
  reset: handle submodule with trailing slash
  rm: re-use parse_pathspec's trailing-slash removal

 builtin/reset.c            |  5 +++++
 builtin/rm.c               | 20 ++++----------------
 t/t7400-submodule-basic.sh |  6 ++++--
 3 files changed, 13 insertions(+), 18 deletions(-)

-- 
1.8.2

Instead of re-implementing the "remove trailing slashes" loop in
builtin/rm.c just pass PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to
parse_pathspec.

Signed-off-by: John Keeping <redacted>
---
 builtin/rm.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 9b59ab3..3a0e0ea 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c

@@ -298,22 +298,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
-	/*
-	 * Drop trailing directory separators from directories so we'll find
-	 * submodules in the index.
-	 */
-	for (i = 0; i < argc; i++) {
-		size_t pathlen = strlen(argv[i]);
-		if (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&
-		    is_directory(argv[i])) {
-			do {
-				pathlen--;
-			} while (pathlen && is_dir_sep(argv[i][pathlen - 1]));
-			argv[i] = xmemdupz(argv[i], pathlen);
-		}
-	}
-
-	parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);
+	parse_pathspec(&pathspec, 0,
+		       PATHSPEC_PREFER_CWD |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       prefix, argv);
 	refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
 
 	seen = xcalloc(pathspec.nr, 1);

-- 
1.8.2

When using tab-completion, a directory path will often end with a
trailing slash which currently confuses "git rm" when dealing with
submodules.  Now that we have parse_pathspec we can easily handle this
by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.

Signed-off-by: John Keeping <redacted>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 5e4c551..9efac0f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,
 		}
 	}
 	*rev_ret = rev;
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
 		       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
 		       prefix, argv);
 }

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 4192fe0..c268d3c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '
 
 '
 
-test_expect_success 'gracefully add submodule with a trailing slash' '
+test_expect_success 'gracefully add/reset submodule with a trailing slash' '
 
 	git reset --hard &&
 	git commit -m "commit subproject" init &&

@@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '
 	git add init/ &&
 	test_must_fail git diff --exit-code --cached init &&
 	test $commit = $(git ls-files --stage |
-		sed -n "s/^160000 \([^ ]*\).*/\1/p")
+		sed -n "s/^160000 \([^ ]*\).*/\1/p") &&
+	git reset init/ &&
+	git diff --exit-code --cached init
 
 '
 

-- 
1.8.2

Am 10.09.2013 21:13, schrieb John Keeping:

When using tab-completion, a directory path will often end with a
trailing slash which currently confuses "git rm" when dealing with

I think you meant to say "git reset" in the line above. Apart from
that I'm all for it.

On Tue, Sep 10, 2013 at 09:37:45PM +0200, Jens Lehmann wrote:

Am 10.09.2013 21:13, schrieb John Keeping:

quoted

When using tab-completion, a directory path will often end with a
trailing slash which currently confuses "git rm" when dealing with

I think you meant to say "git reset" in the line above. Apart from
that I'm all for it.

Yeah, you're right - I obviously got confused between the two patches :-(.
I'll wait for more feedback before submitting a re-roll.

quoted

submodules.  Now that we have parse_pathspec we can easily handle this
by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.

Signed-off-by: John Keeping <redacted>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 5e4c551..9efac0f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,
 		}
 	}
 	*rev_ret = rev;
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
 		       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
 		       prefix, argv);
 }

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 4192fe0..c268d3c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '
 
 '
 
-test_expect_success 'gracefully add submodule with a trailing slash' '
+test_expect_success 'gracefully add/reset submodule with a trailing slash' '
 
 	git reset --hard &&
 	git commit -m "commit subproject" init &&

@@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '
 	git add init/ &&
 	test_must_fail git diff --exit-code --cached init &&
 	test $commit = $(git ls-files --stage |
-		sed -n "s/^160000 \([^ ]*\).*/\1/p")
+		sed -n "s/^160000 \([^ ]*\).*/\1/p") &&
+	git reset init/ &&
+	git diff --exit-code --cached init
 
 '
 

Am 10.09.2013 21:13, schrieb John Keeping:

When the index is now read here, I would have expected hunk in this
patch that removes a read_cache() invocation.

On Wed, Sep 11, 2013 at 2:13 AM, John Keeping [off-list ref] wrote:

I notice that _CHEAP implementation and the removed code are not
exactly the same. But I think they have the same purpose so it's
probably ok even there are some subtle behavioral changes.

You may want to improve _CHEAP to remove consecutive trailing slashes
(i.e. foo//// -> foo) too. And maybe is is_dir_sep() instead of
explicit == '/' comparison in there.

+                      prefix, argv);
        refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);

        seen = xcalloc(pathspec.nr, 1);

-- 
Duy

On Wed, Sep 11, 2013 at 08:05:44AM +0200, Johannes Sixt wrote:

Am 10.09.2013 21:13, schrieb John Keeping:

quoted

When using tab-completion, a directory path will often end with a
trailing slash which currently confuses "git rm" when dealing with
submodules.  Now that we have parse_pathspec we can easily handle this
by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.

Signed-off-by: John Keeping <redacted>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 5e4c551..9efac0f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,
 		}
 	}
 	*rev_ret = rev;
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));

When the index is now read here, I would have expected hunk in this
patch that removes a read_cache() invocation.

I think that needs to look like this on top - there's also a
read_cache_unmerged() around line 68 but I don't think we can remove
that one.

-- >8 --

diff --git a/builtin/reset.c b/builtin/reset.c
index 9efac0f..800117f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
 
-	read_cache();
 	if (do_diff_cache(tree_sha1, &opt))
 		return 1;
 	diffcore_std(&opt);

@@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,
 
 static void die_if_unmerged_cache(int reset_type)
 {
-	if (is_merge() || read_cache() < 0 || unmerged_cache())
+	if (is_merge() || unmerged_cache())
 		die(_("Cannot do a %s reset in the middle of a merge."),
 		    _(reset_type_names[reset_type]));

On Wed, Sep 11, 2013 at 02:48:51PM +0700, Duy Nguyen wrote:

On Wed, Sep 11, 2013 at 2:13 AM, John Keeping [off-list ref] wrote:

quoted

Instead of re-implementing the "remove trailing slashes" loop in
builtin/rm.c just pass PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to
parse_pathspec.

Signed-off-by: John Keeping <redacted>
---
 builtin/rm.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 9b59ab3..3a0e0ea 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c

@@ -298,22 +298,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
        if (read_cache() < 0)
                die(_("index file corrupt"));

-       /*
-        * Drop trailing directory separators from directories so we'll find
-        * submodules in the index.
-        */
-       for (i = 0; i < argc; i++) {
-               size_t pathlen = strlen(argv[i]);
-               if (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&
-                   is_directory(argv[i])) {
-                       do {
-                               pathlen--;
-                       } while (pathlen && is_dir_sep(argv[i][pathlen - 1]));
-                       argv[i] = xmemdupz(argv[i], pathlen);
-               }
-       }
-
-       parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);
+       parse_pathspec(&pathspec, 0,
+                      PATHSPEC_PREFER_CWD |
+                      PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,

I notice that _CHEAP implementation and the removed code are not
exactly the same. But I think they have the same purpose so it's
probably ok even there are some subtle behavioral changes.

Providing that there's only one trailing slash, the user-visible effect
should be the same since the only case affected by that is submodules.
In fact _CHEAP does better in the case where the submodule does not
exist in the working tree.

You may want to improve _CHEAP to remove consecutive trailing slashes
(i.e. foo//// -> foo) too. And maybe is is_dir_sep() instead of
explicit == '/' comparison in there.

Sounds good, I'll try to look at that tonight.

quoted

+                      prefix, argv);
        refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);

        seen = xcalloc(pathspec.nr, 1);

On Wed, Sep 11, 2013 at 3:20 PM, John Keeping [off-list ref] wrote:

reset --soft does not go through these code paths (i.e. it does not
need index at all). If we fail to load index index in "reset --soft" I
think it's ok to die(). Corrupt index is fatal anyway. But "reset
--soft" now has to pay the cost to load index, which could be slow
when the index is big. Assuming nobody does "reset --soft" that often
I think this is OK.

Alternatively we could load index lazily in _CHEAP code only when we
see trailing slashes, then replace these read_cache() with
read_cache_unless_its_already_loaded_earlier() or something.
-- 
Duy

On Wed, Sep 11, 2013 at 05:54:48PM +0700, Duy Nguyen wrote:

On Wed, Sep 11, 2013 at 3:20 PM, John Keeping [off-list ref] wrote:

quoted

On Wed, Sep 11, 2013 at 08:05:44AM +0200, Johannes Sixt wrote:

quoted

Am 10.09.2013 21:13, schrieb John Keeping:

quoted

When using tab-completion, a directory path will often end with a
trailing slash which currently confuses "git rm" when dealing with
submodules.  Now that we have parse_pathspec we can easily handle this
by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.

Signed-off-by: John Keeping <redacted>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 5e4c551..9efac0f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,
            }
    }
    *rev_ret = rev;
+
+   if (read_cache() < 0)
+           die(_("index file corrupt"));

When the index is now read here, I would have expected hunk in this
patch that removes a read_cache() invocation.

I think that needs to look like this on top - there's also a
read_cache_unmerged() around line 68 but I don't think we can remove
that one.

-- >8 --

diff --git a/builtin/reset.c b/builtin/reset.c
index 9efac0f..800117f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,
        opt.output_format = DIFF_FORMAT_CALLBACK;
        opt.format_callback = update_index_from_diff;

-       read_cache();
        if (do_diff_cache(tree_sha1, &opt))
                return 1;
        diffcore_std(&opt);

@@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,

 static void die_if_unmerged_cache(int reset_type)
 {
-       if (is_merge() || read_cache() < 0 || unmerged_cache())
+       if (is_merge() || unmerged_cache())
                die(_("Cannot do a %s reset in the middle of a merge."),
                    _(reset_type_names[reset_type]));

reset --soft does not go through these code paths (i.e. it does not
need index at all). If we fail to load index index in "reset --soft" I
think it's ok to die(). Corrupt index is fatal anyway. But "reset
--soft" now has to pay the cost to load index, which could be slow
when the index is big. Assuming nobody does "reset --soft" that often
I think this is OK.

Alternatively we could load index lazily in _CHEAP code only when we
see trailing slashes, then replace these read_cache() with
read_cache_unless_its_already_loaded_earlier() or something.

read_cache() already has an early return if the index is already loaded
so I don't think we need to worry about a special function for that.

I'm not sure it's worth optimizing this case too heavily, but it might
be a nice change to make parse_pathspec() not rely on the index being
loaded before it is called with certain flags.

On Wed, Sep 11, 2013 at 6:08 PM, John Keeping [off-list ref] wrote:

quoted

quoted

-- >8 --

diff --git a/builtin/reset.c b/builtin/reset.c
index 9efac0f..800117f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,
        opt.output_format = DIFF_FORMAT_CALLBACK;
        opt.format_callback = update_index_from_diff;

-       read_cache();
        if (do_diff_cache(tree_sha1, &opt))
                return 1;
        diffcore_std(&opt);

@@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,

 static void die_if_unmerged_cache(int reset_type)
 {
-       if (is_merge() || read_cache() < 0 || unmerged_cache())
+       if (is_merge() || unmerged_cache())
                die(_("Cannot do a %s reset in the middle of a merge."),
                    _(reset_type_names[reset_type]));

reset --soft does not go through these code paths (i.e. it does not
need index at all). If we fail to load index index in "reset --soft" I
think it's ok to die(). Corrupt index is fatal anyway. But "reset
--soft" now has to pay the cost to load index, which could be slow
when the index is big. Assuming nobody does "reset --soft" that often
I think this is OK.

Alternatively we could load index lazily in _CHEAP code only when we
see trailing slashes, then replace these read_cache() with
read_cache_unless_its_already_loaded_earlier() or something.

read_cache() already has an early return if the index is already loaded
so I don't think we need to worry about a special function for that.

I'm not sure it's worth optimizing this case too heavily, but it might
be a nice change to make parse_pathspec() not rely on the index being
loaded before it is called with certain flags.

Yeah I ddin't check. I agree putting read_cache() in _CHEAP code
sounds nice. We won't need to worry about forgotten read_cache()
elsewhere.
-- 
Duy

Changes since v1:

* Improvements to existing pathspec code to use is_dir_sep instead of
  comparing against '/' and handle multiple trailing slashes
* Remove calls to read_cache() made redundant by a new call in
  builtin/reset.c::parse_args()

John Keeping (4):
  pathspec: use is_dir_sep() to check for trailing slashes
  pathspec: strip multiple trailing slashes from submodules
  rm: re-use parse_pathspec's trailing-slash removal
  reset: handle submodule with trailing slash

 builtin/reset.c            |  8 ++++++--
 builtin/rm.c               | 20 ++++----------------
 pathspec.c                 | 30 +++++++++++++++++++-----------
 t/t7400-submodule-basic.sh |  6 ++++--
 4 files changed, 33 insertions(+), 31 deletions(-)

-- 
1.8.4.277.gfbd6843.dirty

This allows us to correctly removing trailing backslashes on Windows
when checking for submodules.

Signed-off-by: John Keeping <redacted>
---
 pathspec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index ad1a9f5..7c6963b 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -252,7 +252,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 	item->prefix = prefixlen;
 
 	if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
-	    (item->len >= 1 && item->match[item->len - 1] == '/') &&
+	    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&
 	    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
 	    S_ISGITLINK(active_cache[i]->ce_mode)) {
 		item->len--;

@@ -267,7 +267,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 			if (!S_ISGITLINK(ce->ce_mode))
 				continue;
 
-			if (item->len <= ce_len || match[ce_len] != '/' ||
+			if (item->len <= ce_len ||
+			    !is_dir_sep(match[ce_len]) ||
 			    memcmp(ce->name, match, ce_len))
 				continue;
 			if (item->len == ce_len + 1) {

-- 
1.8.4.277.gfbd6843.dirty

This allows us to replace the submodule path trailing slash removal in
builtin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to
parse_pathspec() without changing the behaviour with respect to multiple
trailing slashes.

Signed-off-by: John Keeping <redacted>
---
 pathspec.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 7c6963b..11b031a 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -251,12 +251,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 	item->len = strlen(item->match);
 	item->prefix = prefixlen;
 
-	if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
-	    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&
-	    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
-	    S_ISGITLINK(active_cache[i]->ce_mode)) {
-		item->len--;
-		match[item->len] = '\0';
+	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) {
+		size_t pathlen = item->len;
+		while (pathlen > 0 && is_dir_sep(item->match[pathlen - 1]))
+			pathlen--;
+
+		if ((i = cache_name_pos(item->match, pathlen)) >= 0 &&
+		    S_ISGITLINK(active_cache[i]->ce_mode)) {
+			item->len = pathlen;
+			match[item->len] = '\0';
+		}
 	}
 
 	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)

@@ -271,11 +275,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 			    !is_dir_sep(match[ce_len]) ||
 			    memcmp(ce->name, match, ce_len))
 				continue;
-			if (item->len == ce_len + 1) {
-				/* strip trailing slash */
+
+			while (item->len > 0 && is_dir_sep(match[item->len - 1]))
 				item->len--;
-				match[item->len] = '\0';
-			} else
+
+			/* strip trailing slash */
+			match[item->len] = '\0';
+
+			if (item->len != ce_len)
 				die (_("Pathspec '%s' is in submodule '%.*s'"),
 				     elt, ce_len, ce->name);
 		}

-- 
1.8.4.277.gfbd6843.dirty

Instead of re-implementing the "remove trailing slashes" loop in
builtin/rm.c just pass PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to
parse_pathspec.

Signed-off-by: John Keeping <redacted>
---
 builtin/rm.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 9b59ab3..3a0e0ea 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c

@@ -298,22 +298,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
-	/*
-	 * Drop trailing directory separators from directories so we'll find
-	 * submodules in the index.
-	 */
-	for (i = 0; i < argc; i++) {
-		size_t pathlen = strlen(argv[i]);
-		if (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&
-		    is_directory(argv[i])) {
-			do {
-				pathlen--;
-			} while (pathlen && is_dir_sep(argv[i][pathlen - 1]));
-			argv[i] = xmemdupz(argv[i], pathlen);
-		}
-	}
-
-	parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);
+	parse_pathspec(&pathspec, 0,
+		       PATHSPEC_PREFER_CWD |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       prefix, argv);
 	refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
 
 	seen = xcalloc(pathspec.nr, 1);

-- 
1.8.4.277.gfbd6843.dirty

When using tab-completion, a directory path will often end with a
trailing slash which currently confuses "git reset" when dealing with
submodules.  Now that we have parse_pathspec we can easily handle this
by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.

To do this, we need to move the read_cache() call before the
parse_pathspec() call.  All of the existing paths through cmd_reset()
that do not die early already call read_cache() at some point, so there
is no performance impact to doing this in the common case.

Signed-off-by: John Keeping <redacted>
---
 builtin/reset.c            | 8 ++++++--
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 5e4c551..800117f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
 
-	read_cache();
 	if (do_diff_cache(tree_sha1, &opt))
 		return 1;
 	diffcore_std(&opt);

@@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,
 
 static void die_if_unmerged_cache(int reset_type)
 {
-	if (is_merge() || read_cache() < 0 || unmerged_cache())
+	if (is_merge() || unmerged_cache())
 		die(_("Cannot do a %s reset in the middle of a merge."),
 		    _(reset_type_names[reset_type]));
 

@@ -220,8 +219,13 @@ static void parse_args(struct pathspec *pathspec,
 		}
 	}
 	*rev_ret = rev;
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
 		       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
 		       prefix, argv);
 }

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 4192fe0..c268d3c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '
 
 '
 
-test_expect_success 'gracefully add submodule with a trailing slash' '
+test_expect_success 'gracefully add/reset submodule with a trailing slash' '
 
 	git reset --hard &&
 	git commit -m "commit subproject" init &&

@@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '
 	git add init/ &&
 	test_must_fail git diff --exit-code --cached init &&
 	test $commit = $(git ls-files --stage |
-		sed -n "s/^160000 \([^ ]*\).*/\1/p")
+		sed -n "s/^160000 \([^ ]*\).*/\1/p") &&
+	git reset init/ &&
+	git diff --exit-code --cached init
 
 '
 

-- 
1.8.4.277.gfbd6843.dirty

Am 12.09.2013 21:24, schrieb John Keeping:

A design decisions to keep in mind:

Paths in the index *ALWAYS* use the slash, even on Windows. On Windows,
pathspec that are user input must undergo backslash-to-slash
transformation at a very early stage so that later processing that
compares the user input to index contents need not do it on the fly. The
backslash-to-slash transformation used to happen in get_pathspec() via
prefix_path() and normalize_path_copy().

If, at this point, the contents of 'match' is still being parsed for
pathspec magic, then it is likely correct to use is_dir_sep().

On the other hand, if at this point the contents of 'match' are used to
execute pathspec magic, then it is not correct to use is_dir_sep(); the
conversion of backslash to slash should have happened earlier, and no
backslashes should be present anymore.

(Yes, this means that on Windows we cannot escape glob characters
because, e.g., 'a\*.c' was turned into 'a/*.c'.)

-- Hannes