threads / patch / 17600

patchsubmodule: handle trailing slash, warn about non-submodules

Subject: [PATCH] submodule: handle trailing slash, warn about non-submodules

## tl;dr

8 messages between Feb 6, 2009 and Feb 7, 2009. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Johannes Schindelin· Feb 6, 2009, 04:00 UTC · lore
Earlier, when you called
	git submodule path/to/submodule/

(which happens easily if you are a heavy user of tab-completion), Git would silently ignore the given path, as "git ls-files path/to/submodule/" does not return anything due to the trailing slash.

Git would also silently ignore paths that do not point to submodules at all, without warning the user about the likely mistake.

Now, "git submodule" strips trailing slashes, and it warns about parameters that are no submodules.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-submodule.sh |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)
Show changes to git-submodule.sh +13 −1
diff --git a/git-submodule.sh b/git-submodule.sh
index 2f47e06..b878909 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -59,7 +59,19 @@ resolve_relative_url ()
 #
 module_list()
 {
-	git ls-files --stage -- "$@" | grep '^160000 '
+	while test $# -gt 0
+	do
+		line=$(git ls-files --stage ${1%/} | grep '^160000 ')
+		case "$line" in
+		'')
+			echo "Warning: ignoring non-submodule '$1'" >&2
+			;;
+		*)
+			echo "$line"
+			;;
+		esac
+		shift
+	done
 }
 
 #

-- 
1.6.1.2.630.g01a7e
Junio C Hamano· Feb 6, 2009, 05:29 UTC · re: Johannes Schindelin · lore

Re: [PATCH] submodule: handle trailing slash, warn about non-submodules

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 7 quoted lines
> Earlier, when you called
>
> 	git submodule path/to/submodule/
>
> (which happens easily if you are a heavy user of tab-completion), Git
> would silently ignore the given path, as "git ls-files path/to/submodule/"
> does not return anything due to the trailing slash.
Does --error-unmatch help in such a case to avoid shell loops?
Johannes Schindelin· Feb 6, 2009, 11:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule: handle trailing slash, warn about non-submodules

Hi,
On Thu, 5 Feb 2009, Junio C Hamano wrote:
Show 11 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Earlier, when you called
> >
> > 	git submodule path/to/submodule/
> >
> > (which happens easily if you are a heavy user of tab-completion), Git
> > would silently ignore the given path, as "git ls-files path/to/submodule/"
> > does not return anything due to the trailing slash.
> 
> Does --error-unmatch help in such a case to avoid shell loops?
No, as the second issue solved by the shell loop still holds true:
	$ git ls-files --error-unmatch submodule/
	error: pathspec 'submodule/' did not match any file(s) known to git.
	Did you forget to 'git add'?
That would be really awkward, don't you think?

BTW I have been asked about this behavior _easily_ a 50 times. I just finally broke down and went for the easiest solution.

A proper solution would most probably involve a rewrite as a builtin.

Ciao, Dscho

Junio C Hamano· Feb 7, 2009, 08:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH] submodule: handle trailing slash, warn about non-submodules

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 7 quoted lines
> Earlier, when you called
>
> 	git submodule path/to/submodule/
>
> (which happens easily if you are a heavy user of tab-completion), Git
> would silently ignore the given path, as "git ls-files path/to/submodule/"
> does not return anything due to the trailing slash.

I see a sign of concentrating too much on the topic you were interested in to blind yourself here in this patch.

Show 22 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2f47e06..b878909 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -59,7 +59,19 @@ resolve_relative_url ()
>  #
>  module_list()
>  {
> -	git ls-files --stage -- "$@" | grep '^160000 '
> +	while test $# -gt 0
> +	do
> +		line=$(git ls-files --stage "${1%/}" | grep '^160000 ')
> +		case "$line" in
> +		'')
> +			echo "Warning: ignoring non-submodule '$1'" >&2
> +			;;
> +		*)
> +			echo "$line"
> +			;;
> +		esac
> +		shift
> +	done

Almost everybody seems to call module_list with the arguments it received from the caller, but that "$@" could be none. Worse, cmd_foreach always calls module_list with no argument. Earlier you got all submodules, now you get none.

So you would need:
        case $# in
        0)
                git ls-files --stage -- | grep '^160000 '
                ;;
        *)
                your while loop
                ;;
        esac
or something, I think.
Johannes Schindelin· Feb 7, 2009, 13:42 UTC · re: Junio C Hamano · lore

[PATCH 0/2] submodule path handling fixes

My earlier attempt at this was misguided by my laziness, and I was willing to sacrifice correctness for the ability to move on to other problems.

So now, I taught plumbing to handle the trailing slash problem, which is not only much cleaner, but helps other users of ls-files, too.

Note that at this moment, it is beyond my reach to fix the issue that "git submodule no-such-submodule" will _only_ warn and not _fail_.

That would take a builtinification, as ls-files is used in a pipe in git-submodule.sh, and AFAIR there is no really portable way to catch errors in a pipe, short of using temporary files.

Johannes Schindelin (2):
  Let ls-files strip trailing slashes in submodules' paths
  submodule: warn about non-submodules
 builtin-ls-files.c         |   21 ++++++++++++++++++++-
 git-submodule.sh           |    2 +-
 t/t7400-submodule-basic.sh |   13 +++++++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)
Johannes Schindelin· Feb 7, 2009, 13:43 UTC · re: Junio C Hamano · lore

[PATCH 1/2] Let ls-files strip trailing slashes in submodules' paths

Tab completion makes it easy to add a trailing slash to a submodule path. As it is completely clear what the user actually wanted to say, be nice and strip that slash at the end.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-ls-files.c         |   21 ++++++++++++++++++++-
 t/t7400-submodule-basic.sh |    6 ++++++
 2 files changed, 26 insertions(+), 1 deletions(-)
Show changes to 2 files +26 −1

builtin-ls-files.c, t/t7400-submodule-basic.sh

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 3434031..9dec282 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -262,6 +262,21 @@ static const char *verify_pathspec(const char *prefix)
 	return max ? xmemdupz(prev, max) : NULL;
 }
 
+static void strip_trailing_slash_from_submodules(void)
+{
+	const char **p;
+
+	for (p = pathspec; *p != NULL; p++) {
+		int len = strlen(*p), pos;
+
+		if (len < 1 || (*p)[len - 1] != '/')
+			continue;
+		pos = cache_name_pos(*p, len - 1);
+		if (pos >= 0 && S_ISGITLINK(active_cache[pos]->ce_mode))
+			*p = xstrndup(*p, len - 1);
+	}
+}
+
 /*
  * Read the tree specified with --with-tree option
  * (typically, HEAD) into stage #1 and then
@@ -510,6 +525,11 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 
 	pathspec = get_pathspec(prefix, argv + i);
 
+	/* be nice with submodule patsh ending in a slash */
+	read_cache();
+	if (pathspec)
+		strip_trailing_slash_from_submodules();
+
 	/* Verify that the pathspec matches the prefix */
 	if (pathspec)
 		prefix = verify_pathspec(prefix);
@@ -533,7 +553,6 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	      show_killed | show_modified))
 		show_cached = 1;
 
-	read_cache();
 	if (prefix)
 		prune_cache(prefix);
 	if (with_tree) {
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2ec7ac6..a74f24c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -234,4 +234,10 @@ test_expect_success 'gracefully add submodule with a trailing slash' '
 
 '
 
+test_expect_success 'ls-files gracefully handles trailing slash' '
+
+	test "init" = "$(git ls-files init/)"
+
+'
+
 test_done

-- 
1.6.1.2.630.g01a7e
Johannes Schindelin· Feb 7, 2009, 13:43 UTC · re: Junio C Hamano · lore

[PATCH 2/2] submodule: warn about non-submodules

Earlier, when you called
        git submodule some/bogus/path

Git would silently ignore the path, without warning the user about the likely mistake. Now it does.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-submodule.sh           |    2 +-
 t/t7400-submodule-basic.sh |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletions(-)
Show changes to 2 files +8 −1

git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index 2f47e06..6cc2d33 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -59,7 +59,7 @@ resolve_relative_url ()
 #
 module_list()
 {
-	git ls-files --stage -- "$@" | grep '^160000 '
+	git ls-files --error-unmatch --stage -- "$@" | grep '^160000 '
 }
 
 #
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a74f24c..b8cb2df 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -240,4 +240,11 @@ test_expect_success 'ls-files gracefully handles trailing slash' '
 
 '
 
+test_expect_success 'submodule <invalid-path> warns' '
+
+	git submodule no-such-submodule 2> output.err &&
+	grep "^error: .*no-such-submodule" output.err
+
+'
+
 test_done

-- 
1.6.1.2.630.g01a7e

← back to recent threads