Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2014, 16:47 UTC
Message-ID: <xmqqsipdi5lw.fsf@gitster.dls.corp.google.com>
In-Reply-To: <FAD02895-24B2-46C3-ABEF-E9CE17926FF9@gmail.com>

"Kyle J. McKay" <mackyle@gmail.com> writes:

Show 39 quoted lines

> On Apr 14, 2014, at 15:51, Junio C Hamano wrote:
>> I think we would want to see the actual change formatted this way
>> (without needing to pass "-w" to "git show"), as it will make it
>> clear that this artificial extra level of "define the whole thing
>> inside a function and then make a single call to it" is a workaround
>> of specific shell's glitch that we would not have to have in an
>> ideal world ;-)
>>
>> Besides that would make it less likely to cause conflicts with the
>> real changes in flight.
>
> Sounds good to me.
>
>> Please double check what I queued on 'pu' when I push out today's
>> integration result.
>
>> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
>> index a4f683a5..2ab514ea 100644
>> --- a/git-rebase--am.sh
>> +++ b/git-rebase--am.sh
>> @@ -4,6 +4,13 @@
>>  # Copyright (c) 2010 Junio C Hamano.
>>  #
>>
>> +# The whole contents of the file is run by dot-sourcing this file
>> from
>> +# inside a shell function, and "return"s we see below are expected to
>> +# return from that function that dot-sources us.  However, FreeBSD
>> +# /bin/sh misbehaves on such a construct, so we will work it around
>> by
>> +# enclosing the whole thing inside an extra layer of a function.
>> +git_rebase__am () {
>
> I think the wording may be just a bit off:
>
>> and "return"s we see below are expected to return from that function
>> that dot-sources us.
>
> I thought that was one of the buggy behaviors we are working around?

Yeah, it is written as if every reader has the knowledge that the extra

	extra__func () {
        ...
        }
        extra__func

around did not originally exist. The description does not read well with the work-around already there.

Show 8 quoted lines

> Maybe I'm just reading it wrong...
>
> Does this wording seem any clearer?
>
> 	and "return"s we see below are expected not to return
> 	from the function that dot-sources us, but rather to
> 	the next command after the one in the function that
> 	dot-sources us.

Not really. The comment was not about explaining "return"s. When a reader reads the text with the work-around, it is clear that these "return"s are inside this extra function and there is no possible interpretation other than that they are to return from the extra function.

The comment was meant to explain why a seemingly strange "define a function and then immediately call it just once" pattern is used, and "Earlier, these returns were not inside any function when this file is viewed standalone. Because this file is to be dot-sourced within a function, they were to return from that dot-sourcing function --- but some shells do not behave that way, hence this strange construct." was meant to be that explanation, but apparently it failed to convey what I meant to say.

> If I'm the only one getting a wrong meaning from the comments, then no
> reason to change them.

I agree that the description does not read well with the work-around already there. I am not sure what would be a better way to phrase it, though.