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

From: Kyle J. McKay <mackyle@gmail.com>
Date: Apr 16, 2014, 04:32 UTC
Message-ID: <FAD02895-24B2-46C3-ABEF-E9CE17926FF9@gmail.com>
In-Reply-To: <xmqqob03le3v.fsf@gitster.dls.corp.google.com>

On Apr 14, 2014, at 15:51, Junio C Hamano wrote:

Show 9 quoted lines

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

Show 16 quoted lines

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

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.

Otherwise the patch in pu looks fine (all t34*.sh tests pass for me on FreeBSD with pu at 8d8dc6db).

Thank you for adding the comments.

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

--Kyle