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

[Junio C Hamano <gitster@pobox.com>]

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

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

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