[PATCH 0/2] module_list enhancements

[PATCH 0/2] module_list enhancements

[Fredrik Gustafsson]

Cleanup and enhanced module_list (see patches for details). All new functionality is
in the first patch, the second one deals only with cleanup. I would prefer if both
got applied.

Fredrik Gustafsson (2):
  [submodule] handle multibyte characters in name
  [submodule] Replace perl-code with sh

 git-submodule.sh           | 55 +++++++++++++++++++++-------------------------
 t/t7400-submodule-basic.sh |  5 +++++
 2 files changed, 30 insertions(+), 30 deletions(-)

-- 
1.8.3.253.g20b40b5.dirty

[PATCH 1/2] [submodule] handle multibyte characters in name

[Fredrik Gustafsson]

Bugg reported here:
http://thread.gmane.org/gmane.comp.version-control.git/218922/focus=226791

Note that newline (\n) is still not supported and will not be until the
sh-script is replaced by something in an other language. This however
let us to use mostly all other strange characters.

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
---
 git-submodule.sh           | 3 ++-
 t/t7400-submodule-basic.sh | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..31524d3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -113,9 +113,10 @@ resolve_relative_url ()
 module_list()
 {
 	(
-		git ls-files --error-unmatch --stage -- "$@" ||
+		git ls-files --error-unmatch --stage -z -- "$@" ||
 		echo "unmatched pathspec exists"
 	) |
+	sed -e 's/\x00/\n/g' |
 	perl -e '
 	my %unmerged = ();
 	my ($null_sha1) = ("0" x 40);
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ff26535..47ab7e7 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -868,4 +868,9 @@ test_expect_success 'submodule deinit fails when submodule has a .git directory
 	test -n "$(git config --get-regexp "submodule\.example\.")"
 '
 
+test_expect_success 'submodule with strange name works "å äö"' '
+	git init "å äö" &&
+	git submodule add "./å äö" &&
+	test -n "$(cat .gitmodules | grep "å äö")"
+'
 test_done
-- 
1.8.3.253.g20b40b5.dirty

Re: [PATCH 1/2] submodule: handle multibyte characters in name

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> Bugg reported here:
> http://thread.gmane.org/gmane.comp.version-control.git/218922/focus=226791

The URL is nice supplemental info as footnote, but please write log
message in a way that a reader can understand without going there.
In this case, it wouldn't be so hard, I think, perhaps like:

	Many "git submodule" operations do not work on a submodule
	at a path whose name is not in ASCII.

	This is because "git ls-files" is used to find which paths
	are bound to submodules to the current working tree, and the
	output is C-quoted by default for non ASCII pathnames.

	Read from "git ls-files -z" instead, which is easier than
	unwrapping C-quote ourselves.

or something.

>  module_list()
>  {
>  	(
> -		git ls-files --error-unmatch --stage -- "$@" ||
> +		git ls-files --error-unmatch --stage -z -- "$@" ||
>  		echo "unmatched pathspec exists"
>  	) |
> +	sed -e 's/\x00/\n/g' |

It is strange to preprosess input to be read by a Perl script with
sed ;-)

How about doing it this way instead?  Does the result pass your
test?

 git-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..19faf58 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -113,7 +113,7 @@ resolve_relative_url ()
 module_list()
 {
 	(
-		git ls-files --error-unmatch --stage -- "$@" ||
+		git ls-files --error-unmatch --stage -z -- "$@" ||
 		echo "unmatched pathspec exists"
 	) |
 	perl -e '
@@ -121,6 +121,7 @@ module_list()
 	my ($null_sha1) = ("0" x 40);
 	my @out = ();
 	my $unmatched = 0;
+	$/ = "\0";
 	while (<STDIN>) {
 		if (/^unmatched pathspec/) {
 			$unmatched = 1;

Re: [PATCH 1/2] submodule: handle multibyte characters in name

[Jens Lehmann]

Am 12.06.2013 23:06, schrieb Junio C Hamano:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
> 
>> Bugg reported here:
>> http://thread.gmane.org/gmane.comp.version-control.git/218922/focus=226791
> 
> The URL is nice supplemental info as footnote, but please write log
> message in a way that a reader can understand without going there.
> In this case, it wouldn't be so hard, I think, perhaps like:
> 
> 	Many "git submodule" operations do not work on a submodule
> 	at a path whose name is not in ASCII.
> 
> 	This is because "git ls-files" is used to find which paths
> 	are bound to submodules to the current working tree, and the
> 	output is C-quoted by default for non ASCII pathnames.
> 
> 	Read from "git ls-files -z" instead, which is easier than
> 	unwrapping C-quote ourselves.
> 
> or something.
> 
>>  module_list()
>>  {
>>  	(
>> -		git ls-files --error-unmatch --stage -- "$@" ||
>> +		git ls-files --error-unmatch --stage -z -- "$@" ||
>>  		echo "unmatched pathspec exists"
>>  	) |
>> +	sed -e 's/\x00/\n/g' |
> 
> It is strange to preprosess input to be read by a Perl script with
> sed ;-)
> 
> How about doing it this way instead?  Does the result pass your
> test?

Hmm, I just came around to test that patch, and for me the new
test even succeeds without the changes to module_list(). So I'm
not convinced yet what we are fixing here ;-)

The original poster reported that the submodule just added locally
is not showing up in a subsequent `git submodule`. And it doesn't
for me either, no matter if the path contains umlauts or not. Will
take a deeper look when I find some more time to do that, maybe
recent changes to "git add" play a role here too.

>  git-submodule.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 79bfaac..19faf58 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -113,7 +113,7 @@ resolve_relative_url ()
>  module_list()
>  {
>  	(
> -		git ls-files --error-unmatch --stage -- "$@" ||
> +		git ls-files --error-unmatch --stage -z -- "$@" ||
>  		echo "unmatched pathspec exists"
>  	) |
>  	perl -e '
> @@ -121,6 +121,7 @@ module_list()
>  	my ($null_sha1) = ("0" x 40);
>  	my @out = ();
>  	my $unmatched = 0;
> +	$/ = "\0";
>  	while (<STDIN>) {
>  		if (/^unmatched pathspec/) {
>  			$unmatched = 1;
> 

Re: [PATCH 1/2] submodule: handle multibyte characters in name

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Hmm, I just came around to test that patch, and for me the new
> test even succeeds without the changes to module_list(). So I'm
> not convinced yet what we are fixing here ;-)

My guess is that you have core.quotepaths set to false.

> The original poster reported that the submodule just added locally
> is not showing up in a subsequent `git submodule`. And it doesn't
> for me either, no matter if the path contains umlauts or not. Will
> take a deeper look when I find some more time to do that, maybe
> recent changes to "git add" play a role here too.

Thanks.

Re: [PATCH 1/2] submodule: handle multibyte characters in name

[Fredrik Gustafsson]

On Wed, Jun 12, 2013 at 03:57:52PM -0700, Junio C Hamano wrote:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
> > Hmm, I just came around to test that patch, and for me the new
> > test even succeeds without the changes to module_list(). So I'm
> > not convinced yet what we are fixing here ;-)
> 
> My guess is that you have core.quotepaths set to false.

Actually the bug-reporter claimed to see the submodule in .gitmodules
but not when listed with git submodule.

The test is missing one line in the end to check for this. Sorry about
that.

I've a new iteration ready for this but want to test it a bit more
locally so I don't miss anything else first.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH 1/2] [submodule] handle multibyte characters in name

[Phil Hord]

On Tue, Jun 11, 2013 at 7:04 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> Bugg reported here:
> http://thread.gmane.org/gmane.comp.version-control.git/218922/focus=226791
>
> Note that newline (\n) is still not supported and will not be until the
> sh-script is replaced by something in an other language. This however
> let us to use mostly all other strange characters.

Please explain the commit better so the reader can understand what the
commit does and why.  I can see what's going on by reading the commit
and the original thread, but I should not have to.

Thanks,
Phil

[PATCH 2/2] [submodule] Replace perl-code with sh

[Fredrik Gustafsson]

This is a work built on
http://thread.gmane.org/gmane.comp.version-control.git/198873/focus=198930

Basically git-submodule.sh needs to use something else than sh to handle
newline in filenames (and therefore needs to use a language that accepts
\0 in strings).

However, since we're not there yet. I've thrown out the only
perl-dependency for git-submodule.sh. It decreases the number of
lines of code and uses the same solution as the rest of the script
already do.

This would lead to less forks and faster code. A simple testrun of
t7400-submodule-basic.sh before this patch resulted in:
real    0m8.359s
user    0m8.921s
sys     0m3.888s

real    0m9.062s
user    0m9.025s
sys     0m3.784s

real    0m8.490s
user    0m9.065s
sys     0m3.740s

After this patch was applied:
real    0m7.417s
user    0m8.717s
sys     0m3.804s

real    0m7.873s
user    0m8.821s
sys     0m3.692s

real    0m8.950s
user    0m8.765s
sys     0m3.760s

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
---
 git-submodule.sh | 52 +++++++++++++++++++++++-----------------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 31524d3..1652781 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -112,39 +112,33 @@ resolve_relative_url ()
 #
 module_list()
 {
+	null_sha1=0000000000000000000000000000000000000000
+	unmerged=
 	(
 		git ls-files --error-unmatch --stage -z -- "$@" ||
-		echo "unmatched pathspec exists"
+		echo "#unmatched"
 	) |
 	sed -e 's/\x00/\n/g' |
-	perl -e '
-	my %unmerged = ();
-	my ($null_sha1) = ("0" x 40);
-	my @out = ();
-	my $unmatched = 0;
-	while (<STDIN>) {
-		if (/^unmatched pathspec/) {
-			$unmatched = 1;
-			next;
-		}
-		chomp;
-		my ($mode, $sha1, $stage, $path) =
-			/^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/;
-		next unless $mode eq "160000";
-		if ($stage ne "0") {
-			if (!$unmerged{$path}++) {
-				push @out, "$mode $null_sha1 U\t$path\n";
-			}
-			next;
-		}
-		push @out, "$_\n";
-	}
-	if ($unmatched) {
-		print "#unmatched\n";
-	} else {
-		print for (@out);
-	}
-	'
+	while read mode sha1 stage path
+	do
+		if test $mode = "#unmatched"
+		then
+			echo "#unmatched"
+		fi
+		if test $mode = "160000"
+		then
+			if test $stage != "0"
+			then
+				if test "$unmerged" != "$path"
+				then
+					echo "$mode $null_sha1 U $path"
+				fi
+				unmerged="$path"
+			else
+				echo "$mode $sha1 $stage $path"
+			fi
+		fi
+	done
 }
 
 die_if_unmatched ()
-- 
1.8.3.253.g20b40b5.dirty

Re: [PATCH 2/2] [submodule] Replace perl-code with sh

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> This is a work built on
> http://thread.gmane.org/gmane.comp.version-control.git/198873/focus=198930
>
> Basically git-submodule.sh needs to use something else than sh to handle
> newline in filenames (and therefore needs to use a language that accepts
> \0 in strings).

Have you considered "git -c core.quotepath=false ls-files"?  You
won't be able to handle paths with LF or dq '"' in them, but that
feels much less worse than doing

	sed -e 's/\x00/\n/'