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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2018, 20:46 UTC
Message-ID: <xmqqefkbltxv.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <7ce3cfef9ff3ab97ac8292fae94a0024a1d85505.1521748846.git.wink@saville.com>

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 ;-)

Show 19 quoted lines

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

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

>  git_rebase__am () {
> -
> +echo "git_rebase_am:+" 1>&5

debuggin'? I see similar stuff left in other parts (snipped) of this patch.