Skip to contentSkip to conversation

reset: handle submodule with trailing slash

PATCH In review John Keeping 5 participants 27 messages Jun 15, 2016

lore
Share & tools
lore

John Keeping

author
#0 lore raw

cover 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(-)
Signature · 1 line
1.8.2
author
#1 lore raw

PATCH 1/2 reset: handle submodule with trailing slash

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 <[email protected]>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
builtin/reset.c +5 -0
index 5e4c551..9efac0f 100644
@@ -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);
 }
t/t7400-submodule-basic.sh +4 -2
index 4192fe0..c268d3c 100755
@@ -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
 
 '
Signature · 1 line
1.8.2
#2 lore raw

Am 10.09.2013 21:13, schrieb John Keeping:

Quoted 2 lines

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.

Quoted 52 lines

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 <[email protected]>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
builtin/reset.c +5 -0
index 5e4c551..9efac0f 100644
@@ -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);
 }
t/t7400-submodule-basic.sh +4 -2
index 4192fe0..c268d3c 100755
@@ -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
 
 '
author
#3 lore raw
Quoted 6 lines · Jens Lehmann

Am 10.09.2013 21:13, schrieb John Keeping:

Quoted 2 lines

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 52 lines
Quoted 52 lines

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 <[email protected]>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
builtin/reset.c +5 -0
index 5e4c551..9efac0f 100644
@@ -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);
 }
t/t7400-submodule-basic.sh +4 -2
index 4192fe0..c268d3c 100755
@@ -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
 
 '
#4 lore raw
In reply to John Keeping

Am 10.09.2013 21:13, schrieb John Keeping:

Quoted 16 lines

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 <[email protected]>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
builtin/reset.c +0 -0
index 5e4c551..9efac0f 100644

builtin/reset.c static void parse_args(struct pathspec *pathspec, · line 225

Quoted 6 lines
builtin/reset.c +3 -0
@@ -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.

Quoted 32 lines
builtin/reset.c +2 -0
+
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
 		       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
 		       prefix, argv);
 }
t/t7400-submodule-basic.sh +4 -2
index 4192fe0..c268d3c 100755
@@ -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
 
 '
author
#5 lore raw
Quoted 12 lines · Johannes Sixt

Am 10.09.2013 21:13, schrieb John Keeping:

Quoted 11 lines

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 <[email protected]>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
Quoted 14 lines

builtin/reset.c static void parse_args(struct pathspec *pathspec, · line 225

Quoted 11 lines
builtin/reset.c +3 -0
index 5e4c551..9efac0f 100644
@@ -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 --

builtin/reset.c +1 -2
index 9efac0f..800117f 100644
@@ -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]));
#6 lore raw
Quoted 46 lines · John Keeping
Quoted 12 lines · Johannes Sixt

Am 10.09.2013 21:13, schrieb John Keeping:

Quoted 11 lines

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 <[email protected]>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
Quoted 14 lines

builtin/reset.c static void parse_args(struct pathspec *pathspec, · line 225

Quoted 11 lines
builtin/reset.c +3 -0
index 5e4c551..9efac0f 100644
@@ -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 --

builtin/reset.c +0 -1
index 9efac0f..800117f 100644
@@ -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);

builtin/reset.c static void set_reflog_message(struct strbuf *sb, const char *action, · line 173

Quoted 7 lines
builtin/reset.c +1 -1
@@ -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.

Signature · 1 line
Duy
author
#7 lore raw
Quoted 62 lines · Duy Nguyen
Quoted 46 lines · John Keeping
Quoted 12 lines · Johannes Sixt

Am 10.09.2013 21:13, schrieb John Keeping:

Quoted 11 lines

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 <[email protected]>
---
 builtin/reset.c            | 5 +++++
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
Quoted 14 lines

builtin/reset.c static void parse_args(struct pathspec *pathspec, · line 225

Quoted 11 lines
builtin/reset.c +3 -0
index 5e4c551..9efac0f 100644
@@ -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 --

builtin/reset.c +0 -1
index 9efac0f..800117f 100644
@@ -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);

builtin/reset.c static void set_reflog_message(struct strbuf *sb, const char *action, · line 173

Quoted 7 lines
builtin/reset.c +1 -1
@@ -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.

Quoted 3 lines

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.

#8 lore raw
Quoted 33 lines · John Keeping
Quoted 29 lines
Quoted 14 lines

-- >8 --

builtin/reset.c +0 -1
index 9efac0f..800117f 100644
@@ -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);

builtin/reset.c static void set_reflog_message(struct strbuf *sb, const char *action, · line 173

Quoted 7 lines
builtin/reset.c +1 -1
@@ -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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Signature · 1 line
Duy
#9 lore raw
In reply to Duy Nguyen
Quoted 3 lines · Duy Nguyen

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.

Do I smell a breakage here? Isn't "reset --soft HEAD" (or some known good commit) a way to recover from a corrupt index?

If that is the case, I do not think it is OK at all. What do we suggest as an alternative? "rm .git/index && read-tree"?

Quoted 8 lines

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.

author
#10 lore raw
Quoted 11 lines · Junio C Hamano
Quoted 3 lines · Duy Nguyen

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.

Do I smell a breakage here? Isn't "reset --soft HEAD" (or some known good commit) a way to recover from a corrupt index?

If that is the case, I do not think it is OK at all. What do we suggest as an alternative? "rm .git/index && read-tree"?

Duy's suggestion below is necessary to avoid this then I think - we'll die if the user has a corrupt index and gives a path with a trailing slash, but without that path we won't try to load the index.

Quoted 8 lines
Quoted 8 lines

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.

#11 lore raw
Quoted 10 lines · John Keeping
Quoted 9 lines · Junio C Hamano
Quoted 3 lines · Duy Nguyen

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.

Do I smell a breakage here? Isn't "reset --soft HEAD" (or some known good commit) a way to recover from a corrupt index?

Quoted 6 lines
Quoted 2 lines

If that is the case, I do not think it is OK at all. What do we suggest as an alternative? "rm .git/index && read-tree"?

Duy's suggestion below is necessary to avoid this then I think - we'll die if the user has a corrupt index and gives a path with a trailing slash, but without that path we won't try to load the index.

Sorry, but I don't quite follow. Isn't "git reset --soft <path>" a nonsense, with or without a trailing slash at the end of <path>?

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

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.

author
#12 lore raw
Quoted 15 lines · Junio C Hamano
Quoted 13 lines · John Keeping
Quoted 11 lines · Junio C Hamano
Quoted 3 lines · Duy Nguyen

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.

Do I smell a breakage here? Isn't "reset --soft HEAD" (or some known good commit) a way to recover from a corrupt index?

If that is the case, I do not think it is OK at all. What do we suggest as an alternative? "rm .git/index && read-tree"?

Quoted 6 lines
Quoted 3 lines

Duy's suggestion below is necessary to avoid this then I think - we'll die if the user has a corrupt index and gives a path with a trailing slash, but without that path we won't try to load the index.

Sorry, but I don't quite follow. Isn't "git reset --soft <path>" a nonsense, with or without a trailing slash at the end of <path>?

Yes, but my point was that providing the user doesn't give a path with a trailing slash we will get past the option parser if we take the approach below.

However, I think we do do a read_cache when using "reset --soft" since we go through builtin/reset.c::die_if_unmerged_cache() which dies if read_cache fails. So I don't think we are losing anything by moving this check earlier.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

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.

#13 lore raw
Quoted 4 lines · John Keeping

However, I think we do do a read_cache when using "reset --soft" since we go through builtin/reset.c::die_if_unmerged_cache() which dies if read_cache fails. So I don't think we are losing anything by moving this check earlier.

Thanks.

author
#14 lore raw

PATCH 2/2 rm: re-use parse_pathspec's trailing-slash removal

In reply to John Keeping

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 <[email protected]>
---
 builtin/rm.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
builtin/rm.c +4 -16
index 9b59ab3..3a0e0ea 100644
@@ -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);
Signature · 1 line
1.8.2
#15 lore raw

Re: [PATCH 2/2] rm: re-use parse_pathspec's trailing-slash removal

Quoted 26 lines · John Keeping

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 <[email protected]>
---
 builtin/rm.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
builtin/rm.c +0 -9
index 9b59ab3..3a0e0ea 100644
@@ -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 {

builtin/rm.c int cmd_rm(int argc, const char **argv, const char *prefix) · line 303

Quoted 10 lines
builtin/rm.c +3 -7
-                               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.

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.

Quoted 4 lines
builtin/rm.c +1 -0
+                      prefix, argv);
        refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
 
        seen = xcalloc(pathspec.nr, 1);
Signature · 1 line
Duy
author
#16 lore raw

Re: [PATCH 2/2] rm: re-use parse_pathspec's trailing-slash removal

Quoted 33 lines · Duy Nguyen
Quoted 32 lines · John Keeping

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 <[email protected]>
---
 builtin/rm.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
builtin/rm.c +0 -15
index 9b59ab3..3a0e0ea 100644
@@ -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);
-               }
-       }
-
Quoted 8 lines

builtin/rm.c int cmd_rm(int argc, const char **argv, const char *prefix) · line 303

Quoted 4 lines
builtin/rm.c +3 -1
-       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.

Quoted 3 lines

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 4 lines
Quoted 4 lines
builtin/rm.c +1 -0
+                      prefix, argv);
        refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
 
        seen = xcalloc(pathspec.nr, 1);
author
#17 lore raw

v2 cover submodule trailing slash improvements

In reply to John Keeping

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(-)
Signature · 1 line
1.8.4.277.gfbd6843.dirty
author
#18 lore raw

PATCH v2 1/4 pathspec: use is_dir_sep() to check for trailing slashes

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

Signed-off-by John Keeping <[email protected]>
---
 pathspec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
pathspec.c +3 -2
index ad1a9f5..7c6963b 100644
@@ -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) {
Signature · 1 line
1.8.4.277.gfbd6843.dirty
#19 lore raw

Re: [PATCH v2 1/4] pathspec: use is_dir_sep() to check for trailing slashes

Am 12.09.2013 21:24, schrieb John Keeping:

Quoted 22 lines

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

Signed-off-by John Keeping <[email protected]>
---
 pathspec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
pathspec.c +1 -1
index ad1a9f5..7c6963b 100644
@@ -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--;

pathspec.c static unsigned prefix_pathspec(struct pathspec_item *item, · line 274

Quoted 9 lines
pathspec.c +2 -1
@@ -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) {

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

author
#20 lore raw

PATCH v2 2/4 pathspec: strip multiple trailing slashes from submodules

In reply to John Keeping

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 <[email protected]>
---
 pathspec.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)
pathspec.c +17 -10
index 7c6963b..11b031a 100644
@@ -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);
 		}
Signature · 1 line
1.8.4.277.gfbd6843.dirty
#21 lore raw

Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules

Quoted 4 lines · John Keeping

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.

Where does prefix_pathspec()'s input, which could have an unwanted trailing slash, come from?

If it is read from some of our internal data structure and known to have at most one, then this change makes me feel very uneasy to cope with potentially sloppy end-user input and data generated by ourselves with the same logic. It will allow our internal to be sloppy without forcing us notice and fix that sloppiness.

If it is coming from an end-user input, then I would not object to the change, though.

Thanks.

Quoted 51 lines
Signed-off-by John Keeping <[email protected]>
---
 pathspec.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)
pathspec.c +17 -10
index 7c6963b..11b031a 100644
@@ -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);
 		}
author
#22 lore raw

Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules

Quoted 10 lines · Junio C Hamano
Quoted 4 lines · John Keeping

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.

Where does prefix_pathspec()'s input, which could have an unwanted trailing slash, come from?

Quoted 8 lines

If it is read from some of our internal data structure and known to have at most one, then this change makes me feel very uneasy to cope with potentially sloppy end-user input and data generated by ourselves with the same logic. It will allow our internal to be sloppy without forcing us notice and fix that sloppiness.

If it is coming from an end-user input, then I would not object to the change, though.

I added this in response to Duy's comment on v1 [1].

[1] http://article.gmane.org/gmane.comp.version-control.git/234548

Looking more closely, this does come from user input (via the argv passed into parse_pathspec) but does (some of the time) go through prefix_path_gently which calls normalize_path_copy_len.

It's not immediately clear to me when prefix_pathspec goes through this particular code path, but I think we may be able to drop this (and the previous patch) without affecting the user.

Quoted 51 lines
Quoted 51 lines
Signed-off-by John Keeping <[email protected]>
---
 pathspec.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)
pathspec.c +17 -10
index 7c6963b..11b031a 100644
@@ -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);
 		}
#23 lore raw

Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules

Quoted 17 lines · John Keeping
Quoted 16 lines · Junio C Hamano
Quoted 4 lines · John Keeping

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.

Where does prefix_pathspec()'s input, which could have an unwanted trailing slash, come from?

If it is read from some of our internal data structure and known to have at most one, then this change makes me feel very uneasy to cope with potentially sloppy end-user input and data generated by ourselves with the same logic. It will allow our internal to be sloppy without forcing us notice and fix that sloppiness.

Quoted 6 lines
Quoted 2 lines

If it is coming from an end-user input, then I would not object to the change, though.

I added this in response to Duy's comment on v1 [1].

[1] http://article.gmane.org/gmane.comp.version-control.git/234548

Looks like I add more noise to this thread than anything valuable. Yes, once argv goes through parse_pathspec it's normalized so we do not need to strip consecutive slashes any more. I'm not entirely sure if it also converts Windows '\\' to '/'..

Quoted 7 lines

Looking more closely, this does come from user input (via the argv passed into parse_pathspec) but does (some of the time) go through prefix_path_gently which calls normalize_path_copy_len.

It's not immediately clear to me when prefix_pathspec goes through this particular code path, but I think we may be able to drop this (and the previous patch) without affecting the user.

Signature · 1 line
Duy
author
#24 lore raw

Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules

Quoted 21 lines · Duy Nguyen
Quoted 20 lines · John Keeping
Quoted 18 lines · Junio C Hamano
Quoted 4 lines · John Keeping

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.

Where does prefix_pathspec()'s input, which could have an unwanted trailing slash, come from?

If it is read from some of our internal data structure and known to have at most one, then this change makes me feel very uneasy to cope with potentially sloppy end-user input and data generated by ourselves with the same logic. It will allow our internal to be sloppy without forcing us notice and fix that sloppiness.

If it is coming from an end-user input, then I would not object to the change, though.

Quoted 8 lines
Quoted 3 lines

I added this in response to Duy's comment on v1 [1].

[1] http://article.gmane.org/gmane.comp.version-control.git/234548

Looks like I add more noise to this thread than anything valuable. Yes, once argv goes through parse_pathspec it's normalized so we do not need to strip consecutive slashes any more. I'm not entirely sure if it also converts Windows '\\' to '/'..

If it goes through normalize_path_copy_len then it does.

Junio, can you please drop the first two patches in this series? I can resend the final two unmodified if necessary, but I suspect it's easier for you to just drop the commits.

Quoted 7 lines
Quoted 7 lines

Looking more closely, this does come from user input (via the argv passed into parse_pathspec) but does (some of the time) go through prefix_path_gently which calls normalize_path_copy_len.

It's not immediately clear to me when prefix_pathspec goes through this particular code path, but I think we may be able to drop this (and the previous patch) without affecting the user.

author
#25 lore raw

PATCH v2 3/4 rm: re-use parse_pathspec's trailing-slash removal

In reply to John Keeping

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 <[email protected]>
---
 builtin/rm.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
builtin/rm.c +4 -16
index 9b59ab3..3a0e0ea 100644
@@ -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);
Signature · 1 line
1.8.4.277.gfbd6843.dirty
author
#26 lore raw

PATCH v2 4/4 reset: handle submodule with trailing slash

In reply to John Keeping

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 <[email protected]>
---
 builtin/reset.c            | 8 ++++++--
 t/t7400-submodule-basic.sh | 6 ++++--
 2 files changed, 10 insertions(+), 4 deletions(-)
builtin/reset.c +6 -2
index 5e4c551..800117f 100644
@@ -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);
 }
t/t7400-submodule-basic.sh +4 -2
index 4192fe0..c268d3c 100755
@@ -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
 
 '
Signature · 1 line
1.8.4.277.gfbd6843.dirty