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

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

[Johannes Schindelin]

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

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

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

[Junio C Hamano]

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?

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

[Johannes Schindelin]

Hi,

On Thu, 5 Feb 2009, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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.

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

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

[PATCH 0/2] submodule path handling fixes

[Johannes Schindelin]

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

Re: [PATCH 0/2] submodule path handling fixes

[Junio C Hamano]

Both patches look trivially correct.

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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