Weird problem with git-submodule.sh

Weird problem with git-submodule.sh

[Marc Branchaud]

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.

Re: Weird problem with git-submodule.sh

[Junio C Hamano]

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?

Re: Weird problem with git-submodule.sh

[Marc Branchaud]

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

		M.

Re: Weird problem with git-submodule.sh

[Junio C Hamano]

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

Re: Weird problem with git-submodule.sh

[Marc Branchaud]

On 12-12-07 02:11 PM, Junio C Hamano wrote:
> 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.

Re: Weird problem with git-submodule.sh

[Junio C Hamano]

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.

Re: Weird problem with git-submodule.sh

[Marc Branchaud]

On 12-12-07 03:23 PM, Junio C Hamano wrote:
> 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.

Re: Weird problem with git-submodule.sh

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[Marc Branchaud]

I like your patch's subject line better than mine.

		M.


On 12-12-07 05:34 PM, Junio C Hamano wrote:
> 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
> 

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

[Andreas Schwab]

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

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

[Junio C Hamano]

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

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

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

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

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

Re: Weird problem with git-submodule.sh

[Stefano Lattarini]

Hi Junio, Marc.

On 12/07/2012 10:08 PM, Junio C Hamano wrote:
> 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

Re: Weird problem with git-submodule.sh

[Phil Hord]

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

Re: Weird problem with git-submodule.sh

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

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

Re: Weird problem with git-submodule.sh

[Phil Hord]

On Wed, Dec 12, 2012 at 2:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

Re: Weird problem with git-submodule.sh

[Marc Branchaud]

On 12-12-07 03:23 PM, Junio C Hamano wrote:
> 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.

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

[marcnarc]

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