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

From: Kyle J. McKay <mackyle@gmail.com>
Date: Apr 11, 2014, 14:29 UTC
Message-ID: <D2BF8546-87B4-431A-B86D-6EFAF205279C@gmail.com>
In-Reply-To: <vpqsipkmeum.fsf@anie.imag.fr>

On Apr 11, 2014, at 01:48, Matthieu Moy wrote:

Show 28 quoted lines

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> If script2.sh is changed to this:
>>
>> # script2.sh fixed
>> main() {
>>        if [ 5 -gt 3 ]; then
>>                return 5
>>        fi
>>        case bad in *)
>>                echo always shows
>>        esac
>>        echo should not get here
>>        ! :
>> }
>> main
>
> Wouldn't it be better to just stop using . within function?
>
> The .-ed script could define the complete function, and then the
> function would be used from the toplevel script.
>
> If I understand correctly, your version uses nested functions with  
> file
> inclusion between both levels of nesting. That might work for the  
> shells
> you tested, but if the goal is to avoid using tricky features that may
> trigger bugs on some shells, that seems backward.

There are already nested functions with file inclusion between both levels of nesting in git-rebase--interactive.sh and git-rebase-- merge.sh now, so it's not introducing anything new.

> IOW, why not move the whole run_specific_rebase_internal function to
> git-rebase--$type?

So what change are you proposing exactly?

The current code in maint does this:

git-rebase.sh: top-level
   git-rebase.sh: run_specific_rebase()
     git-rebase.sh: run_specific_rebase_internal() -- contains "dot"
       git-rebase--$type.sh: top-level -- has "return"

To make the kind of change I think you're proposing would be somewhat more invasive than the proposed patch. Each of the git-rebase--$type scripts would have to be modified not to do anything other than define functions when included which would require some code movement to avoid having two "main" functions -- either that or there need to be multiple "dot" includes in git-rebase.sh so the code not in a function only executes when the selected case that uses it is active -- or the entire contents of each git-rebase--$type script gets indented and becomes a function definition, but that would introduce functions defining functions which seems like it would add use of a new tricky feature not previously used.

I'm not saying it's a bad idea, it's just somewhat more invasive than simply inserting 3 lines.

--Kyle