Re: [RFC PATCH v3.1 2/9 2/2] rebase-interactive: Do not automatically run code

[Wink Saville <wink@saville.com>]

On Thu, Mar 22, 2018 at 1:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes:
>
>> At Junio's suggestion have git-rebase--am and git-rebase--merge work the
>> same way as git-rebase--interactive. This makes the code more consistent.
>
> I mumbled about making git_rebase__$type functions for all $type in
> my previous response, but that was done without even looking at
> git-rebase--$type.sh scriptlets.  It seems that they all shared the
> same structure (i.e. define git_rebase__$type function and then at
> the end clla it) and were consistent already.  It was the v3 that
> changed the calling convention only for interactive, which made it
> inconsistent.  If you are making git-rebase.sh call the helper shell
> function for all backend $type, you are keeping the existing
> consistency.
>
> This is no longer about "interactive" alone, though, and need to be
> retitled ;-)
>
>> Signed-off-by: Wink Saville <wink@saville.com>
>> ---
>>  git-rebase--am.sh          | 17 ++++++-----------
>>  git-rebase--interactive.sh |  8 +++++++-
>>  git-rebase--merge.sh       | 17 ++++++-----------
>>  git-rebase.sh              | 13 ++++---------
>>  4 files changed, 23 insertions(+), 32 deletions(-)
>>
>> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
>> index be3f06892..47dc69ed9 100644
>> --- a/git-rebase--am.sh
>> +++ b/git-rebase--am.sh
>> @@ -4,17 +4,14 @@
>>  # Copyright (c) 2010 Junio C Hamano.
>>  #
>>
>> +# The whole contents of this file is loaded by dot-sourcing it from
>> +# inside another shell function, hence no shebang on the first line
>> +# and then the caller invokes git_rebase__am.
>
> Is this comment necessary?

Removed

>
>> +# Previously this file was sourced and it called itself to get this
>> +# was to get around a bug in older (9.x) versions of FreeBSD.
>
> ECANTPARSE.  But this probably is no longer needed here, even though
> it may make sense to explain why this comment is no longer relevant
> in the log message.  E.g.
>
>         The backend scriptlets for "git rebase" are structured in a
>         bit unusual way for historical reasons.  Originally, it was
>         designed in such a way that dot-sourcing them from "git
>         rebase" would be sufficient to invoke the specific backend.
>         When it was discovered that some shell implementations
>         (e.g. FreeBSD 9.x) misbehaved by exiting when "return" is
>         executed at the top level of a dot-sourced script (the
>         original was expecting that the control returns to the next
>         command in "git rebase" after dot-sourcing the scriptlet),
>         the whole body of git-rebase--$backend.sh was made into a
>         shell function git_rebase__$backend and then the scriptlet
>         was made to call this function at the end as a workaround.
>
>         Move the call to "git rebase" side, instead of at the end of
>         each scriptlet.  This would give us a more normal
>         arrangement where a function library lives in a scriptlet
>         that is dot-sourced, and then these helper functions are
>         called by the script that dot-sourced the scriptlet.
>
>         While at it, remove the large comment that explains why this
>         rather unusual structure was used from these scriptlets.
>
> or something like that in the log message, and then we can get rid
> of these in-code comments, I would think.

Updated commit message

>>  git_rebase__am () {
>> -
>> +echo "git_rebase_am:+" 1>&5
>
> debuggin'?  I see similar stuff left in other parts (snipped) of
> this patch.

Removed debugging :(

Currently I'm not rebasing the other commits (3..9)
to reduce the amount of work I have to do in each
review cycle, is that OK?

Also, will you merge commits 1 and 2 before the other
commits or is the procedure to merge the complete set
at once?