git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Wink Saville <wink@saville.com>
Cc: git@vger.kernel.org, sunshine@sunshineco.com, Johannes.Schindelin@gmx.de
Subject: Re: [RFC PATCH v3.1 2/9 2/2] rebase-interactive: Do not automatically run code
Date: Thu, 22 Mar 2018 13:46:36 -0700	[thread overview]
Message-ID: <xmqqefkbltxv.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <7ce3cfef9ff3ab97ac8292fae94a0024a1d85505.1521748846.git.wink@saville.com> (Wink Saville's message of "Thu, 22 Mar 2018 13:03:24 -0700")

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?

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

  reply	other threads:[~2018-03-22 20:46 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-22 16:57 [RFC PATCH v3 0/9] rebase-interactive: Wink Saville
2018-03-22 16:57 ` [RFC PATCH v3 1/9] Simplify pick_on_preserving_merges Wink Saville
2018-03-22 16:57 ` [RFC PATCH v3 2/9] Call git_rebase__interactive from run_specific_rebase Wink Saville
2018-03-22 18:27   ` Junio C Hamano
2018-03-22 19:28     ` Wink Saville
2018-03-22 20:03       ` [RFC PATCH v3.1 2/9 2/2] rebase-interactive: Do not automatically run code Wink Saville
2018-03-22 20:46         ` Junio C Hamano [this message]
2018-03-22 22:45           ` Wink Saville
2018-03-23  2:06             ` Junio C Hamano
2018-03-22 16:57 ` [RFC PATCH v3 3/9] Indent function git_rebase__interactive Wink Saville
2018-03-23 17:43   ` Johannes Schindelin
2018-03-22 16:57 ` [RFC PATCH v3 4/9] Extract functions out of git_rebase__interactive Wink Saville
2018-03-22 16:57 ` [RFC PATCH v3 5/9] Use new functions in git_rebase__interactive Wink Saville
2018-03-23 17:42   ` Johannes Schindelin
2018-03-23 18:24     ` Junio C Hamano
2018-03-23 20:09       ` Wink Saville
2018-03-22 16:57 ` [RFC PATCH v3 6/9] Add and use git_rebase__interactive__preserve_merges Wink Saville
2018-03-22 16:57 ` [RFC PATCH v3 7/9] Remove unused code paths from git_rebase__interactive Wink Saville
2018-03-22 16:57 ` [RFC PATCH v3 8/9] Remove unused code paths from git_rebase__interactive__preserve_merges Wink Saville
2018-03-22 16:57 ` [RFC PATCH v3 9/9] Remove merges_option and a blank line Wink Saville

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqefkbltxv.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=sunshine@sunshineco.com \
    --cc=wink@saville.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).