threads / patch / 34113

patch, 2 partsmodule_list enhancements

Subject: [PATCH 0/2] module_list enhancements

## tl;dr

9 messages between Jun 11, 2013 and Jun 13, 2013. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Fredrik Gustafsson· Jun 11, 2013, 23:04 UTC · lore

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
Fredrik Gustafsson· Jun 11, 2013, 23:04 UTC · re: Fredrik Gustafsson · lore

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

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(-)
Show changes to 2 files +7 −1

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

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
Junio C Hamano· Jun 12, 2013, 21:06 UTC · re: Fredrik Gustafsson · lore

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

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.
Show 8 quoted lines
>  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(-)
Show changes to git-submodule.sh +2 −1
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;
Jens Lehmann· Jun 12, 2013, 21:38 UTC · re: Junio C Hamano · lore

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

Am 12.06.2013 23:06, schrieb Junio C Hamano:
Show 35 quoted lines
> 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.

Show 25 quoted lines
>  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;
>
Junio C Hamano· Jun 12, 2013, 22:57 UTC · re: Jens Lehmann · lore

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

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.
Show 5 quoted lines
> 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.
Fredrik Gustafsson· Jun 12, 2013, 23:10 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 12, 2013 at 03:57:52PM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> 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
Phil Hord· Jun 13, 2013, 01:41 UTC · re: Fredrik Gustafsson · lore

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

On Tue, Jun 11, 2013 at 7:04 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
Show 6 quoted lines
> 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

Fredrik Gustafsson· Jun 11, 2013, 23:04 UTC · re: Fredrik Gustafsson · lore

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

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(-)
Show changes to git-submodule.sh +23 −29
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
Junio C Hamano· Jun 12, 2013, 21:08 UTC · re: Fredrik Gustafsson · lore

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

Fredrik Gustafsson <iveqy@iveqy.com> writes:
Show 6 quoted lines
> 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/'

← back to recent threads