threads / discuss / 32286

Weird problem with git-submodule.sh

Subject: Weird problem with git-submodule.sh

## tl;dr

19 messages between Dec 7, 2012 and Dec 12, 2012.

replies: 18people: 5as markdown or json

Marc Branchaud· Dec 7, 2012, 17:44 UTC · lore
Hi all,
This is with git 1.8.0.1 on all the machines involved.
One of our build machines is having trouble with "git submodule":
	$ git submodule init external/openssl
	No submodule mapping found in .gitmodules for path ''

(.gitmodules and other aspects of the repo are fine -- the submodules work perfectly on other machines.)

The problem seems to be in cmd_init() with the construct
	module_list "$@" |
	while read mode sha1 stage sm_path
	do
		...
Explicitly setting IFS before the call to module_list makes it work:
	IFS=" "
	module_list "$@" |
	while read mode sha1 stage sm_path
	do
		...

If IFS is unset, the "while read" loop ends up with everything in the $mode variable, and the other 3 variables are empty.

If I isolate module_list() and a simple "while read" loop into a standalone script, like this:

	module_list()
	{
		...
	}
	module_list "$@" |
	while read mode sha1 stage sm_path
	do
		echo - $mode - $sha1 - $stage - $sm_path -
	done
It works -- each individual variable is set properly.
It seems that the problem only occurs inside git-submodule.sh.
Any ideas?
		M.
Junio C Hamano· Dec 7, 2012, 17:54 UTC · re: Marc Branchaud · lore

Re: Weird problem with git-submodule.sh

Marc Branchaud <marcnarc@xiplink.com> writes:
Show 5 quoted lines
> This is with git 1.8.0.1 on all the machines involved.
>
> One of our build machines is having trouble with "git submodule":
> ...
> Any ideas?

How and why is the IFS set differently only on one of your build machines?

Marc Branchaud· Dec 7, 2012, 18:03 UTC · re: Junio C Hamano · lore

Re: Weird problem with git-submodule.sh

On 12-12-07 12:54 PM, Junio C Hamano wrote:
Show 10 quoted lines
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>> This is with git 1.8.0.1 on all the machines involved.
>>
>> One of our build machines is having trouble with "git submodule":
>> ...
>> Any ideas?
> 
> How and why is the IFS set differently only on one of your build
> machines?
It's not.  On all machines:
	$ set | grep IFS
	IFS=$' \t\n'

As I said, if I isolate the module_list() function into another script it works fine, with the exact same environment that breaks in git-submodule.sh.

Also, note that at the top of git-submodule there's
	. git-sh-setup
which does
	unset IFS
		M.
Junio C Hamano· Dec 7, 2012, 19:11 UTC · re: Marc Branchaud · lore

Re: Weird problem with git-submodule.sh

Marc Branchaud <marcnarc@xiplink.com> writes:
Show 23 quoted lines
> On 12-12-07 12:54 PM, Junio C Hamano wrote:
>> Marc Branchaud <marcnarc@xiplink.com> writes:
>> 
>>> This is with git 1.8.0.1 on all the machines involved.
>>>
>>> One of our build machines is having trouble with "git submodule":
>>> ...
>>> Any ideas?
>> 
>> How and why is the IFS set differently only on one of your build
>> machines?
>
> It's not.  On all machines:
> 	$ set | grep IFS
> 	IFS=$' \t\n'
>
> As I said, if I isolate the module_list() function into another script it
> works fine, with the exact same environment that breaks in git-submodule.sh.
>
> Also, note that at the top of git-submodule there's
> 	. git-sh-setup
> which does
> 	unset IFS

Yeah, now it makes sense why you wrote "Weird" on the subject line. What difference, if any, does the problematic box have compared to your other healthy boxes? It uses a different /bin/sh?

Just taking a shot in the dark...
Marc Branchaud· Dec 7, 2012, 20:17 UTC · re: Junio C Hamano · lore

Re: Weird problem with git-submodule.sh

On 12-12-07 02:11 PM, Junio C Hamano wrote:
Show 31 quoted lines
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>> On 12-12-07 12:54 PM, Junio C Hamano wrote:
>>> Marc Branchaud <marcnarc@xiplink.com> writes:
>>>
>>>> This is with git 1.8.0.1 on all the machines involved.
>>>>
>>>> One of our build machines is having trouble with "git submodule":
>>>> ...
>>>> Any ideas?
>>>
>>> How and why is the IFS set differently only on one of your build
>>> machines?
>>
>> It's not.  On all machines:
>> 	$ set | grep IFS
>> 	IFS=$' \t\n'
>>
>> As I said, if I isolate the module_list() function into another script it
>> works fine, with the exact same environment that breaks in git-submodule.sh.
>>
>> Also, note that at the top of git-submodule there's
>> 	. git-sh-setup
>> which does
>> 	unset IFS
> 
> Yeah, now it makes sense why you wrote "Weird" on the subject line.
> What difference, if any, does the problematic box have compared to
> your other healthy boxes?  It uses a different /bin/sh?
> 
> Just taking a shot in the dark...

Bisected this down to exactly that "unset IFS" line in git-sh-setup.sh, from your commit 785063e02bb249 (whaddya trying to do to me Junio?? :) ):

Author: Junio C Hamano <gitster@pobox.com>
Date:   Wed Aug 8 12:08:17 2012 -0700
    sh-setup: protect from exported IFS
    Many scripted Porcelains rely on being able to split words at the
    default $IFS characters, i.e. SP, HT and LF.  If the user exports a
    non-default IFS to the environment, what they read from plumbing
    commands such as ls-files that use HT to delimit fields may not be
    split in the way we expect.
    Protect ourselves by resetting it, just like we do so against CDPATH
    exported to the environment.
    Noticed by Andrew Dranse <adranse@oanda.com>.
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Perhaps IFS should be set to " \t\n" (which I believe is sh's default) instead of just unsetting it altogether? (Note that in my testing I had to set IFS to a literal <space><tab><newline> string.)

		M.
Junio C Hamano· Dec 7, 2012, 20:23 UTC · re: Marc Branchaud · lore

Re: Weird problem with git-submodule.sh

Marc Branchaud <marcnarc@xiplink.com> writes:
Show 17 quoted lines
>     sh-setup: protect from exported IFS
>
>     Many scripted Porcelains rely on being able to split words at the
>     default $IFS characters, i.e. SP, HT and LF.  If the user exports a
>     non-default IFS to the environment, what they read from plumbing
>     commands such as ls-files that use HT to delimit fields may not be
>     split in the way we expect.
>
>     Protect ourselves by resetting it, just like we do so against CDPATH
>     exported to the environment.
>
>     Noticed by Andrew Dranse <adranse@oanda.com>.
>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Perhaps IFS should be set to " \t\n" (which I believe is sh's default)
> instead of just unsetting it altogether?
POSIX.1 says this:
    IFS - A string treated as a list of characters that is used for
    field splitting and to split lines into fields with the read
    command.  If IFS is not set, it shall behave as normal for an
    unset variable, except that field splitting by the shell and
    line splitting by the read command shall be performed as if the
    value of IFS is <space> <tab> <newline>; see Field Splitting.
    Implementations may ignore the value of IFS in the environment, or
    the absence of IFS from the environment, at the time the shell is
    invoked, in which case the shell shall set IFS to <space> <tab>
    <newline> when it is invoked.

So setting it to SP HT LF should not make a difference, or your shell is buggy.

This is exactly why I asked you about the /bin/sh on your problematic box.

Marc Branchaud· Dec 7, 2012, 20:44 UTC · re: Junio C Hamano · lore

Re: Weird problem with git-submodule.sh

On 12-12-07 03:23 PM, Junio C Hamano wrote:
Show 39 quoted lines
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>>     sh-setup: protect from exported IFS
>>
>>     Many scripted Porcelains rely on being able to split words at the
>>     default $IFS characters, i.e. SP, HT and LF.  If the user exports a
>>     non-default IFS to the environment, what they read from plumbing
>>     commands such as ls-files that use HT to delimit fields may not be
>>     split in the way we expect.
>>
>>     Protect ourselves by resetting it, just like we do so against CDPATH
>>     exported to the environment.
>>
>>     Noticed by Andrew Dranse <adranse@oanda.com>.
>>
>>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> Perhaps IFS should be set to " \t\n" (which I believe is sh's default)
>> instead of just unsetting it altogether?
> 
> POSIX.1 says this:
> 
>     IFS - A string treated as a list of characters that is used for
>     field splitting and to split lines into fields with the read
>     command.  If IFS is not set, it shall behave as normal for an
>     unset variable, except that field splitting by the shell and
>     line splitting by the read command shall be performed as if the
>     value of IFS is <space> <tab> <newline>; see Field Splitting.
> 
>     Implementations may ignore the value of IFS in the environment, or
>     the absence of IFS from the environment, at the time the shell is
>     invoked, in which case the shell shall set IFS to <space> <tab>
>     <newline> when it is invoked.
> 
> So setting it to SP HT LF should not make a difference, or your
> shell is buggy.
> 
> This is exactly why I asked you about the /bin/sh on your
> problematic box.
Fair 'nuf.

It's FreeBSD 7.2, which I know is an obsolete version but I'm not able to upgrade the machine. I believe FreeBSD's sh is, or is derived from, dash.

Anyway, given that there is at least one buggy version of sh, wouldn't it be better for git to explicitly set IFS?

		M.
Junio C Hamano· Dec 7, 2012, 21:08 UTC · re: Marc Branchaud · lore

Re: Weird problem with git-submodule.sh

Marc Branchaud <marcnarc@xiplink.com> writes:
> It's FreeBSD 7.2, which I know is an obsolete version but I'm not able to
> upgrade the machine.  I believe FreeBSD's sh is, or is derived from, dash.

Finally. Yes, as you suspected, I am perfectly fine to explicitly set IFS to the default values.

I wanted to have specific names to write in the commit log message, in-code comments and possibly release notes. That way, people can decide if the issue affects them and they should upgrade once the fix is made.

Junio C Hamano· Dec 7, 2012, 22:34 UTC · re: Junio C Hamano · lore

[PATCH] sh-setup: work around "unset IFS" bug in some shells

With an unset IFS, field splitting is supposed to act as if IFS is set to the usual SP HT LF, but Marc Branchaud reports that the shell on FreeBSD 7.2 gets this wrong.

It is easy to set it to the default value manually, so let's do so.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-sh-setup.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index ee0e0bc..107c144 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -12,8 +12,11 @@
 # But we protect ourselves from such a user mistake nevertheless.
 unset CDPATH
 
-# Similarly for IFS
-unset IFS
+# Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
+# do not equate an unset IFS with IFS with the default, so here is
+# an explicit SP HT LF.
+IFS=' 	
+'
 
 git_broken_path_fix () {
 	case ":$PATH:" in

-- 
1.8.1.rc0.125.g5274144
Marc Branchaud· Dec 7, 2012, 22:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] sh-setup: work around "unset IFS" bug in some shells

I like your patch's subject line better than mine.
		M.
On 12-12-07 05:34 PM, Junio C Hamano wrote:
Show 30 quoted lines
> With an unset IFS, field splitting is supposed to act as if IFS is
> set to the usual SP HT LF, but Marc Branchaud reports that the shell
> on FreeBSD 7.2 gets this wrong.
> 
> It is easy to set it to the default value manually, so let's do so.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  git-sh-setup.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index ee0e0bc..107c144 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -12,8 +12,11 @@
>  # But we protect ourselves from such a user mistake nevertheless.
>  unset CDPATH
>  
> -# Similarly for IFS
> -unset IFS
> +# Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
> +# do not equate an unset IFS with IFS with the default, so here is
> +# an explicit SP HT LF.
> +IFS=' 	
> +'
>  
>  git_broken_path_fix () {
>  	case ":$PATH:" in
>
Andreas Schwab· Dec 7, 2012, 22:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] sh-setup: work around "unset IFS" bug in some shells

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> +# Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
> +# do not equate an unset IFS with IFS with the default, so here is
> +# an explicit SP HT LF.
> +IFS=' 	
> +'

Trailing whitespace can easily get lost, so it's probably better to stick a '' in here. The sequence <space><tab> is also easily being mangled to <tab>.

IFS=' '' '' '

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Junio C Hamano· Dec 7, 2012, 22:58 UTC · re: Andreas Schwab · lore

Re: [PATCH] sh-setup: work around "unset IFS" bug in some shells

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 9 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> +# Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
>> +# do not equate an unset IFS with IFS with the default, so here is
>> +# an explicit SP HT LF.
>> +IFS=' 	
>> +'
>
> Trailing whitespace can easily get lost

The comment above the assingment spell the character names out for that exact reason. Honestly, I think

> IFS=' ''	''
> '

is a lot harder to read. If we were writing in bash, we would probably use $' \t\n' but because we aren't, the best you can do with your approach is what you wrote, but it would make the reader wonder what these empty '' are there for and how many spaces are in between the ''<string here>'' pair.

In other words, I agree with your concerns, but I do not think your rewrite helps very much.

Andreas Schwab· Dec 8, 2012, 09:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] sh-setup: work around "unset IFS" bug in some shells

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> +# Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
>>> +# do not equate an unset IFS with IFS with the default, so here is
>>> +# an explicit SP HT LF.
>>> +IFS=' 	
>>> +'
>>
>> Trailing whitespace can easily get lost
>
> The comment above the assingment spell the character names out for
> that exact reason.  Honestly, I think

That won't help if you don't look for it. The loss can easily happen while you are in a different part of the file.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Stefano Lattarini· Dec 9, 2012, 21:05 UTC · re: Junio C Hamano · lore

Re: Weird problem with git-submodule.sh

Hi Junio, Marc.
On 12/07/2012 10:08 PM, Junio C Hamano wrote:
Show 13 quoted lines
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>> It's FreeBSD 7.2, which I know is an obsolete version but I'm not able to
>> upgrade the machine.  I believe FreeBSD's sh is, or is derived from, dash.
> 
> Finally.  Yes, as you suspected, I am perfectly fine to explicitly
> set IFS to the default values.
> 
> I wanted to have specific names to write in the commit log message,
> in-code comments and possibly release notes.  That way, people can
> decide if the issue affects them and they should upgrade once the
> fix is made.
>

The Autoconf manual suggests against unsetting IFS instead of resetting it to the default sequence for yet another reason: if IFS is unset, code that tries to save and restore its value will incorrectly reset it to an empty value, thus disabling field splitting:

    unset IFS
    # default separators used for field splitting
    # ...
    saved_IFS=$IFS
    IFS=:
    # code using the new IFS
    IFS=$saved_IFS
    # no field splitting performed from now on!

Not sure how this is relevant for the Git codebase, but maybe it is something worth reporting in the commit message of a proposed patch.

Regards,
  Stefano
Phil Hord· Dec 12, 2012, 19:10 UTC · re: Stefano Lattarini · lore

Re: Weird problem with git-submodule.sh

Marc Branchaud <marcnarc@xiplink.com> writes:
> It's FreeBSD 7.2, which I know is an obsolete version but I'm not able to
> upgrade the machine.  I believe FreeBSD's sh is, or is derived from, dash.

Dash has been the default '/bin/sh' for Ubuntu for quite a long time now[1] in spite of repeated reports of compatibility problems[2].

Phil

[1] https://wiki.ubuntu.com/DashAsBinSh [2] https://bugs.launchpad.net/ubuntu/+source/dash/+bug/141481

Junio C Hamano· Dec 12, 2012, 19:44 UTC · re: Phil Hord · lore

Re: Weird problem with git-submodule.sh

Phil Hord <phil.hord@gmail.com> writes:
Show 6 quoted lines
> Marc Branchaud <marcnarc@xiplink.com> writes:
>> It's FreeBSD 7.2, which I know is an obsolete version but I'm not able to
>> upgrade the machine.  I believe FreeBSD's sh is, or is derived from, dash.
>
> Dash has been the default '/bin/sh' for Ubuntu for quite a long time
> now[1] in spite of repeated reports of compatibility problems[2].

Wasn't the ancestry more like BSD ash (buggy) came before dash and Marc is running a BSD ash decendant that shared common ancestor with, not a decendant of, dash?

In any case, I do not think that is relevant; does does not seem to have this IFS bug.

> [2] https://bugs.launchpad.net/ubuntu/+source/dash/+bug/141481

None of the ones listed seems to me a bug. Rather, I see it as a sign that the reporter does not know POSIX shell well and only learned his/her shell through bash.

Phil Hord· Dec 12, 2012, 23:12 UTC · re: Junio C Hamano · lore

Re: Weird problem with git-submodule.sh

On Wed, Dec 12, 2012 at 2:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> Phil Hord <phil.hord@gmail.com> writes:
>> [2] https://bugs.launchpad.net/ubuntu/+source/dash/+bug/141481
>
> None of the ones listed seems to me a bug.  Rather, I see it as a
> sign that the reporter does not know POSIX shell well and only
> learned his/her shell through bash.

You're probably right. I run into enough problems with 'dash' as /bin/sh that I always have to disable it early after a new install. In particular, an third-party embedded linux kernel build script fails in cryptic ways with dash. But it is probably the third-party's poor understanding of POSIX shell which is to blame.

I think git's 'make test' previously would also fail under dash, but it seems to be happy with it atm.

Phil
Marc Branchaud· Dec 7, 2012, 21:01 UTC · re: Junio C Hamano · lore

Re: Weird problem with git-submodule.sh

On 12-12-07 03:23 PM, Junio C Hamano wrote:
Show 33 quoted lines
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>>     sh-setup: protect from exported IFS
>>
>>     Many scripted Porcelains rely on being able to split words at the
>>     default $IFS characters, i.e. SP, HT and LF.  If the user exports a
>>     non-default IFS to the environment, what they read from plumbing
>>     commands such as ls-files that use HT to delimit fields may not be
>>     split in the way we expect.
>>
>>     Protect ourselves by resetting it, just like we do so against CDPATH
>>     exported to the environment.
>>
>>     Noticed by Andrew Dranse <adranse@oanda.com>.
>>
>>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> Perhaps IFS should be set to " \t\n" (which I believe is sh's default)
>> instead of just unsetting it altogether?
> 
> POSIX.1 says this:
> 
>     IFS - A string treated as a list of characters that is used for
>     field splitting and to split lines into fields with the read
>     command.  If IFS is not set, it shall behave as normal for an
>     unset variable, except that field splitting by the shell and
>     line splitting by the read command shall be performed as if the
>     value of IFS is <space> <tab> <newline>; see Field Splitting.
> 
>     Implementations may ignore the value of IFS in the environment, or
>     the absence of IFS from the environment, at the time the shell is
>     invoked, in which case the shell shall set IFS to <space> <tab>
>     <newline> when it is invoked.

Not to defend anyone, but I can understand how an implementer might think they're complying with the above while still deciding that an explicit "unset IFS" means IFS=''.

		M.
marcnarc@xiplink.com· Dec 7, 2012, 22:15 UTC · re: Marc Branchaud · lore

[PATCH] sh-setup: Explicitly set IFS to its default, instead of unsetting it.

From: Marc Branchaud <marcnarc@xiplink.com>

Some sh implementations interpret "unset IFS" to mean IFS=''. This was seen in FreeBSD 7.2's sh.

We need to make sure IFS has its default value: <space><tab><newline>.
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 git-sh-setup.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index ee0e0bc..56e6498 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -12,8 +12,11 @@
 # But we protect ourselves from such a user mistake nevertheless.
 unset CDPATH
 
-# Similarly for IFS
-unset IFS
+# Similarly for IFS, except that some sh implementations interpret "unset IFS"
+# as IFS='', so we need to set IFS explicitly to its POSIX default using
+# literal <space><tab><newline> characters.
+IFS=' 	
+'
 
 git_broken_path_fix () {
 	case ":$PATH:" in

-- 
1.8.0.1

← back to recent threads