Git development
 help / color / mirror / Atom feed
* Re: [PATCH] sh-setup: work around "unset IFS" bug in some shells
From: Andreas Schwab @ 2012-12-07 22:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Marc Branchaud, Git Mailing List
In-Reply-To: <7va9tpfq46.fsf_-_@alter.siamese.dyndns.org>

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

^ permalink raw reply

* Re: [PATCH 0/6] Improve remote helper documentation
From: Max Horn @ 2012-12-07 22:38 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Sverre Rabbelier, Florian Achleitner, Felipe Contreras
In-Reply-To: <7vehj1fs3c.fsf@alter.siamese.dyndns.org>


On 07.12.2012, at 22:52, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>> On 07.12.2012, at 20:09, Junio C Hamano wrote:
>> 
>>> Except for a minor nit in 6/6; I think "defined options" should be
>>> "defined attributes".
>>> 
>>>   --- a/Documentation/git-remote-helpers.txt
>>>   +++ b/Documentation/git-remote-helpers.txt
>>>   @@ -227,6 +227,8 @@ Support for this command is mandatory.
>>>           the name; unrecognized attributes are ignored. The list ends
>>>           with a blank line.
>>>    +
>>>   +See REF LIST ATTRIBUTES for a list of currently defined options.
>>>   ++
>> 
>> Indeed, my mistake. Dumb question since I am still not completely
>> familiar with the procedures here: Would you just fix this
>> yourself as you apply this, or is it better (or even required)
>> that I reroll for this? (Of course if I ned to re-roll for other
>> reasons, I'd fix this one as well).
> 
> I didn't see any other issues myself, and I didn't see anybody
> raising issues on the series, either, so I could fix it up by
> amending what is already queued, as long as you are OK with it.

Sure, I'd appreciate if you could make that fix.

Thanks,
Max

^ permalink raw reply

* Re: [PATCH] sh-setup: work around "unset IFS" bug in some shells
From: Marc Branchaud @ 2012-12-07 22:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7va9tpfq46.fsf_-_@alter.siamese.dyndns.org>

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
> 

^ permalink raw reply

* [PATCH] sh-setup: work around "unset IFS" bug in some shells
From: Junio C Hamano @ 2012-12-07 22:34 UTC (permalink / raw)
  To: Marc Branchaud; +Cc: Git Mailing List
In-Reply-To: <7vobi5fu3c.fsf@alter.siamese.dyndns.org>

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

^ permalink raw reply related

* [PATCH] sh-setup: Explicitly set IFS to its default, instead of unsetting it.
From: marcnarc @ 2012-12-07 22:15 UTC (permalink / raw)
  To: git; +Cc: Marc Branchaud
In-Reply-To: <50C24ED7.90000@xiplink.com>

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

^ permalink raw reply related

* Re: [PATCH 0/6] Improve remote helper documentation
From: Junio C Hamano @ 2012-12-07 21:52 UTC (permalink / raw)
  To: Max Horn; +Cc: git, Sverre Rabbelier, Florian Achleitner, Felipe Contreras
In-Reply-To: <A6FF9929-74F8-4F2F-BA0B-1B1086D4E7F5@quendi.de>

Max Horn <max@quendi.de> writes:

> On 07.12.2012, at 20:09, Junio C Hamano wrote:
>
>> Except for a minor nit in 6/6; I think "defined options" should be
>> "defined attributes".
>> 
>>    --- a/Documentation/git-remote-helpers.txt
>>    +++ b/Documentation/git-remote-helpers.txt
>>    @@ -227,6 +227,8 @@ Support for this command is mandatory.
>>            the name; unrecognized attributes are ignored. The list ends
>>            with a blank line.
>>     +
>>    +See REF LIST ATTRIBUTES for a list of currently defined options.
>>    ++
>
> Indeed, my mistake. Dumb question since I am still not completely
> familiar with the procedures here: Would you just fix this
> yourself as you apply this, or is it better (or even required)
> that I reroll for this? (Of course if I ned to re-roll for other
> reasons, I'd fix this one as well).

I didn't see any other issues myself, and I didn't see anybody
raising issues on the series, either, so I could fix it up by
amending what is already queued, as long as you are OK with it.

Thanks.

^ permalink raw reply

* Re: [PATCH 0/6] Improve remote helper documentation
From: Max Horn @ 2012-12-07 21:41 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Sverre Rabbelier, Florian Achleitner, Felipe Contreras
In-Reply-To: <7v1uf1he6m.fsf@alter.siamese.dyndns.org>


On 07.12.2012, at 20:09, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>> Various remote helper capabilities and commands were not
>> documented, in particular 'export', or documented in a misleading
>> way (e.g. 'for-push' was listed as a ref attribute understood by
>> git, which is not the case). This patch series changes that, and
>> also address some other things in the remote helper documentation
>> that I found jarring when reading through it.
>> 
>> Note that the description of export and (im|ex)port-marks probably can be
>> improved, and I hope that somebody who knows more about them
>> than me and/or is better at writing documentation will do just that.
>> But I felt it was better to provide something than to do nothing
>> and only complain, as I did previously on this subject ;-).
> 
> A second ping to people who have touched transport-helper.c,
> remote-testsvn.c, git-remote-testgit, and contrib/remote-helpers/ in
> the past 18 months for comments.  I've re-read the documentation
> updates myself and didn't find anything majorly objectionable.
> 
> Except for a minor nit in 6/6; I think "defined options" should be
> "defined attributes".
> 
>    --- a/Documentation/git-remote-helpers.txt
>    +++ b/Documentation/git-remote-helpers.txt
>    @@ -227,6 +227,8 @@ Support for this command is mandatory.
>            the name; unrecognized attributes are ignored. The list ends
>            with a blank line.
>     +
>    +See REF LIST ATTRIBUTES for a list of currently defined options.
>    ++

Indeed, my mistake. Dumb question since I am still not completely familiar with the procedures here: Would you just fix this yourself as you apply this, or is it better (or even required) that I reroll for this? (Of course if I ned to re-roll for other reasons, I'd fix this one as well).


Thanks,
Max

^ permalink raw reply

* Re: [PATCH] git(1): remove a defunct link to "list of authors"
From: Shawn Pearce @ 2012-12-07 21:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk3sthhfy.fsf@alter.siamese.dyndns.org>

On Fri, Dec 7, 2012 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The linked page has not been showing the promised "more complete
>> list" for more than 6 months by now, and nobody has resurrected
>> the list there nor elsewhere since then.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  * If somebody has a working replacement URL, we could use that
>>    instead, of course.  Takers?
>
> A possible alternative could be https://www.ohloh.net/p/git/contributors/summary

Eh, I think just removing the link is sufficient.

^ permalink raw reply

* Re: Weird problem with git-submodule.sh
From: Junio C Hamano @ 2012-12-07 21:08 UTC (permalink / raw)
  To: Marc Branchaud; +Cc: Git Mailing List
In-Reply-To: <50C25539.9010206@xiplink.com>

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.

^ permalink raw reply

* Re: Weird problem with git-submodule.sh
From: Marc Branchaud @ 2012-12-07 21:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vsj7hfw6q.fsf@alter.siamese.dyndns.org>

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.

^ permalink raw reply

* Re: Weird problem with git-submodule.sh
From: Marc Branchaud @ 2012-12-07 20:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vsj7hfw6q.fsf@alter.siamese.dyndns.org>

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.

^ permalink raw reply

* Re: [PATCH 0/6] Improve remote helper documentation
From: Sverre Rabbelier @ 2012-12-07 20:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Max Horn, git, Florian Achleitner, Felipe Contreras
In-Reply-To: <7v1uf1he6m.fsf@alter.siamese.dyndns.org>

On Fri, Dec 7, 2012 at 11:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
> A second ping to people who have touched transport-helper.c,
> remote-testsvn.c, git-remote-testgit, and contrib/remote-helpers/ in
> the past 18 months for comments.  I've re-read the documentation
> updates myself and didn't find anything majorly objectionable.

FWIW:

Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* Re: Weird problem with git-submodule.sh
From: Junio C Hamano @ 2012-12-07 20:23 UTC (permalink / raw)
  To: Marc Branchaud; +Cc: Git Mailing List
In-Reply-To: <50C24ED7.90000@xiplink.com>

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.

^ permalink raw reply

* Re: Weird problem with git-submodule.sh
From: Marc Branchaud @ 2012-12-07 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vwqwtfzis.fsf@alter.siamese.dyndns.org>

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.

^ permalink raw reply

* Re: Weird problem with git-submodule.sh
From: Junio C Hamano @ 2012-12-07 19:11 UTC (permalink / raw)
  To: Marc Branchaud; +Cc: Git Mailing List
In-Reply-To: <50C22F72.6010701@xiplink.com>

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

^ permalink raw reply

* Re: [PATCH 0/6] Improve remote helper documentation
From: Junio C Hamano @ 2012-12-07 19:09 UTC (permalink / raw)
  To: Max Horn; +Cc: git, Sverre Rabbelier, Florian Achleitner, Felipe Contreras
In-Reply-To: <1354038279-76475-1-git-send-email-max@quendi.de>

Max Horn <max@quendi.de> writes:

> Various remote helper capabilities and commands were not
> documented, in particular 'export', or documented in a misleading
> way (e.g. 'for-push' was listed as a ref attribute understood by
> git, which is not the case). This patch series changes that, and
> also address some other things in the remote helper documentation
> that I found jarring when reading through it.
>
> Note that the description of export and (im|ex)port-marks probably can be
> improved, and I hope that somebody who knows more about them
> than me and/or is better at writing documentation will do just that.
> But I felt it was better to provide something than to do nothing
> and only complain, as I did previously on this subject ;-).

A second ping to people who have touched transport-helper.c,
remote-testsvn.c, git-remote-testgit, and contrib/remote-helpers/ in
the past 18 months for comments.  I've re-read the documentation
updates myself and didn't find anything majorly objectionable.

Except for a minor nit in 6/6; I think "defined options" should be
"defined attributes".

    --- a/Documentation/git-remote-helpers.txt
    +++ b/Documentation/git-remote-helpers.txt
    @@ -227,6 +227,8 @@ Support for this command is mandatory.
            the name; unrecognized attributes are ignored. The list ends
            with a blank line.
     +
    +See REF LIST ATTRIBUTES for a list of currently defined options.
    ++

^ permalink raw reply

* Re: Weird problem with git-submodule.sh
From: Marc Branchaud @ 2012-12-07 18:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vvccdhhod.fsf@alter.siamese.dyndns.org>

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.

^ permalink raw reply

* Re: [PATCH] git(1): remove a defunct link to "list of authors"
From: Junio C Hamano @ 2012-12-07 17:59 UTC (permalink / raw)
  To: git
In-Reply-To: <7vobi5hhn9.fsf@alter.siamese.dyndns.org>

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

> The linked page has not been showing the promised "more complete
> list" for more than 6 months by now, and nobody has resurrected
> the list there nor elsewhere since then.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * If somebody has a working replacement URL, we could use that
>    instead, of course.  Takers?

A possible alternative could be https://www.ohloh.net/p/git/contributors/summary

^ permalink raw reply

* [PATCH] git(1): remove a defunct link to "list of authors"
From: Junio C Hamano @ 2012-12-07 17:54 UTC (permalink / raw)
  To: git

The linked page has not been showing the promised "more complete
list" for more than 6 months by now, and nobody has resurrected
the list there nor elsewhere since then.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * If somebody has a working replacement URL, we could use that
   instead, of course.  Takers?

 Documentation/git.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git c/Documentation/git.txt w/Documentation/git.txt
index cbe0883..5133952 100644
--- c/Documentation/git.txt
+++ w/Documentation/git.txt
@@ -868,8 +868,7 @@ Authors
 -------
 Git was started by Linus Torvalds, and is currently maintained by Junio
 C Hamano. Numerous contributions have come from the git mailing list
-<git@vger.kernel.org>. For a more complete list of contributors, see
-http://git-scm.com/about. If you have a clone of git.git itself, the
+<git@vger.kernel.org>.  If you have a clone of git.git itself, the
 output of linkgit:git-shortlog[1] and linkgit:git-blame[1] can show you
 the authors for specific parts of the project.
 

^ permalink raw reply related

* Re: Weird problem with git-submodule.sh
From: Junio C Hamano @ 2012-12-07 17:54 UTC (permalink / raw)
  To: Marc Branchaud; +Cc: Git Mailing List
In-Reply-To: <50C22B15.1030607@xiplink.com>

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?

^ permalink raw reply

* Weird problem with git-submodule.sh
From: Marc Branchaud @ 2012-12-07 17:44 UTC (permalink / raw)
  To: Git Mailing List

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.

^ permalink raw reply

* Re: [PATCH] completion: add option --recurse-submodules to "git push"
From: Junio C Hamano @ 2012-12-07 17:21 UTC (permalink / raw)
  To: Steffen Jaeckel; +Cc: git, Heiko Voigt, Felipe Contreras
In-Reply-To: <1354883304-6860-1-git-send-email-steffen.jaeckel@stzedn.de>

Steffen Jaeckel <steffen.jaeckel@stzedn.de> writes:

> Signed-off-by: Steffen Jaeckel <steffen.jaeckel@stzedn.de>
> ---
>  contrib/completion/git-completion.bash | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 0b77eb1..5b4d2e1 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1434,6 +1434,10 @@ _git_pull ()
>  	__git_complete_remote_or_refspec
>  }
>  
> +__git_push_recurse_submodules_options="
> +	check on-demand
> +"

Most of the existing completion functions do not seem to define
separate variables like this; instead, they literally embed their
choices at the point of use.

Is it expected that the same set of choices will appear in the
completion of many other subcommand options?  [jc: Cc'ed Heiko so
that we can sanity check the answer to this question].  If so, the
variable may be justified; otherwise, not.

>  _git_push ()
>  {
>  	case "$prev" in
> @@ -1446,10 +1450,15 @@ _git_push ()
>  		__gitcomp_nl "$(__git_remotes)" "" "${cur##--repo=}"
>  		return
>  		;;
> +	--recurse-submodules=*)
> +		__gitcomp "$__git_push_recurse_submodules_options" "" "${cur##--recurse-submodules=}"
> +		return
> +		;;

Owners of the completion script, does this look reasonable?
[jc: Cc'ed Felipe for this]

This is a tangent, but why is it a double-hash "##" not a
single-hash "#", other than "because all others use ##"?

>  	--*)
>  		__gitcomp "
>  			--all --mirror --tags --dry-run --force --verbose
>  			--receive-pack= --repo= --set-upstream
> +			--recurse-submodules=
>  		"
>  		return
>  		;;

^ permalink raw reply

* RE: any way to re-release git's block sha1 implementation under a different license?
From: Pyeron, Jason J CTR (US) @ 2012-12-07 14:39 UTC (permalink / raw)
  To: git@vger.kernel.org
In-Reply-To: <CAFBatg3AcjOjQqKJrRe_fkva8OD=F=5aS7kczF3e1ePw_KcJng@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

> -----Original Message-----
> From: Liu Liu
> Sent: Friday, December 07, 2012 2:52 AM
> 
> Hi,
> 
> I am reaching out because in my personal project (
> https://github.com/liuliu/ccv
> ), I used the block sha1 implementation (
> https://github.com/git/git/blob/master/block-sha1/sha1.c) in git. It is
> a
> fast, generalized and standalone implementation, however, since it is a
> part of git, it is under GPL license. Is there any possibilities for
> the
> original author/authors to release this particular piece of code under
> BSD
> / MIT or Apache license? Thanks!


Worse comes to worse, use the Mozilla code it was based on. Assumption is that the original code was under the http://en.wikipedia.org/wiki/Mozilla_Public_License

-Jason



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5615 bytes --]

^ permalink raw reply

* [PATCH 2/2] fetch-pack: avoid repeatedly re-scanning pack directory
From: Jeff King @ 2012-12-07 14:04 UTC (permalink / raw)
  To: git
In-Reply-To: <20121207135351.GA10538@sigill.intra.peff.net>

When we look up a sha1 object for reading, we first check
packfiles, and then loose objects. If we still haven't found
it, we re-scan the list of packfiles in `objects/pack`. This
final step ensures that we can co-exist with a simultaneous
repack process which creates a new pack and then prunes the
old object.

This extra re-scan usually does not have a performance
impact for two reasons:

  1. If an object is missing, then typically the re-scan
     will find a new pack, then no more misses will occur.
     Or if it truly is missing, then our next step is
     usually to die().

  2. Re-scanning is cheap enough that we do not even notice.

However, these do not always hold. The assumption in (1) is
that the caller is expecting to find the object. This is
usually the case, but the call to `parse_object` in
`everything_local` does not follow this pattern. It is
looking to see whether we have objects that the remote side
is advertising, not something we expect to have. Therefore
if we are fetching from a remote which has many refs
pointing to objects we do not have, we may end up
re-scanning the pack directory many times.

Even with this extra re-scanning, the impact is often not
noticeable due to (2); we just readdir() the packs directory
and skip any packs that are already loaded. However, if
there are a large number of packs, then even enumerating the
directory directory can be expensive (especially if we do it
repeatedly). Having this many packs is a good sign the user
should run `git gc`, but it would still be nice to avoid
having to scan the directory at all.

This patch checks has_sha1_file (which does not have the
re-scan and re-check behavior) in the critical loop, and
avoids calling parse_object at all if we do not have the
object.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm lukewarm on this patch.  The re-scan _shouldn't_ be that expensive,
so maybe patch 1 is enough to be a reasonable fix. The fact that we
re-scan repeatedly seems ugly and hacky to me, but it really is just
opendir/readdir/closedir in the case that nothing has changed (and if
something has changed, then it's a good thing to be checking). And with
my patch, fetch-pack would not notice new packs from a simultaneous
repack process (although it's OK, as the result is not incorrect, but
merely that we may ask for the object from the server).

Another option would be to make the reprepare_packed_git re-scan less
expensive by checking the mtime of the directory before scanning it.

 fetch-pack.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index 099ff4d..b4383c6 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -594,6 +594,9 @@ static int everything_local(struct fetch_pack_args *args,
 	for (ref = *refs; ref; ref = ref->next) {
 		struct object *o;
 
+		if (!has_sha1_file(ref->old_sha1))
+			continue;
+
 		o = parse_object(ref->old_sha1);
 		if (!o)
 			continue;
-- 
1.8.1.rc0.4.g5948dfd.dirty

^ permalink raw reply related

* [PATCH 1/2] fetch: run gc --auto after fetching
From: Jeff King @ 2012-12-07 13:58 UTC (permalink / raw)
  To: git
In-Reply-To: <20121207135351.GA10538@sigill.intra.peff.net>

We generally try to run "gc --auto" after any commands that
might introduce a large number of new objects. An obvious
place to do so is after running "fetch", which may introduce
new loose objects or packs (depending on the size of the
fetch).

While an active developer repository will probably
eventually trigger a "gc --auto" on another action (e.g.,
git-rebase), there are two good reasons why it is nicer to
do it at fetch time:

  1. Read-only repositories which track an upstream (e.g., a
     continuous integration server which fetches and builds,
     but never makes new commits) will accrue loose objects
     and small packs, but never coalesce them into a more
     efficient larger pack.

  2. Fetching is often already perceived to be slow to the
     user, since they have to wait on the network. It's much
     more pleasant to include a potentially slow auto-gc as
     part of the already-long network fetch than in the
     middle of productive work with git-rebase or similar.

Signed-off-by: Jeff King <peff@peff.net>
---
The original "gc --auto" patch in 2007 suggested adding this to fetch,
but we never did (there was some brief mention that maybe tweaking
fetch.unpacklimit would be relevant, but I think you would eventually
want to gc, whether you are accruing packs or loose objects).

I didn't bother protecting this with an option (as we do for
receive.gc). I don't see much point, as anybody who doesn't want gc on
their client repos will already have set gc.auto to prevent it from
triggering via other commands).

 builtin/fetch.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4b5a898..1ddbf0d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -959,6 +959,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	struct string_list list = STRING_LIST_INIT_NODUP;
 	struct remote *remote;
 	int result = 0;
+	static const char *argv_gc_auto[] = {
+		"gc", "--auto", NULL,
+	};
 
 	packet_trace_identity("fetch");
 
@@ -1026,5 +1029,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	list.strdup_strings = 1;
 	string_list_clear(&list, 0);
 
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+
 	return result;
 }
-- 
1.8.1.rc0.4.g5948dfd.dirty

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox