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 v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges
Date: Wed, 21 Mar 2018 12:42:32 -0700 [thread overview]
Message-ID: <xmqqy3ilnrkn.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <c169a69be3c61fd1e90eaf79febccef2afdfbd3b.1521653702.git.wink@saville.com> (Wink Saville's message of "Wed, 21 Mar 2018 10:44:38 -0700")
Wink Saville <wink@saville.com> writes:
> Refactor git_rebase__interactive__preserve_merges out of
> git_rebase__interactive resulting in fewer conditionals making
> both routines are simpler.
>
> Changed run_specific_rebase in git-rebase to dispatch to the appropriate
> function after sourcing git-rebase--interactive.
> ---
I think this will become (more) reviewable if it is split into two
patches (at least). A preliminary patch that does the style changes
and line wrapping (left below) as step #1, and all the rest on top
as step #2.
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 331c8dfea..65ff75654 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1,5 +1,7 @@
> -# This shell script fragment is sourced by git-rebase to implement
> -# its interactive mode. "git rebase --interactive" makes it easy
> +#!/bin/sh
Addition of #! implies that this might be invoked as the top-level
script; is that the case now? I did not get such an impression from
the proposed log message and it is still always dot-sourced (in
which case #! gives a wrong signal to the readers).
> +# This shell script fragment is sourced by git-rebase--interactive
> +# and git-rebase--interactive--preserve-merges in support of the
> +# interactive mode. "git rebase --interactive" makes it easy
> # to fix up commits in the middle of a series and rearrange commits.
> #
> # Copyright (c) 2006 Johannes E. Schindelin
> @@ -7,6 +9,7 @@
> # The original idea comes from Eric W. Biederman, in
> # https://public-inbox.org/git/m1odwkyuf5.fsf_-_@ebiederm.dsl.xmission.com/
> #
> +
> # The file containing rebase commands, comments, and empty lines.
Why?
> @@ -274,7 +290,8 @@ pick_one () {
>
> case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
> case "$force_rebase" in '') ;; ?*) ff= ;; esac
> - output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid commit name: \$sha1")"
> + output git rev-parse --verify $sha1 ||
> + die "$(eval_gettext "Invalid commit name: \$sha1")"
Just linewrapping.
> @@ -287,8 +304,8 @@ pick_one () {
> ${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
> "$strategy_args" $empty_args $ff "$@"
>
> - # If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
> - # previous task so this commit is not lost.
> + # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
> + # Reschedule previous task so this commit is not lost.
Ditto.
> @@ -307,17 +324,15 @@ pick_one_preserving_merges () {
> esac
> sha1=$(git rev-parse $sha1)
>
> - if test -f "$state_dir"/current-commit
> + if test -f "$state_dir"/current-commit && test "$fast_forward" = t
> then
> - if test "$fast_forward" = t
> - then
> - while read current_commit
> - do
> - git rev-parse HEAD > "$rewritten"/$current_commit
> - done <"$state_dir"/current-commit
> - rm "$state_dir"/current-commit ||
> - die "$(gettext "Cannot write current commit's replacement sha1")"
> - fi
> + while read current_commit
> + do
> + git rev-parse HEAD > "$rewritten"/$current_commit
> + done <"$state_dir"/current-commit
> + rm "$state_dir"/current-commit ||
> + die "$(gettext \
> + "Cannot write current commit's replacement sha1")"
> fi
Good code simplification that turns
if A
if B
do this thing
fi
fi
into
if A & B
do this thing
fi
> @@ -337,10 +352,10 @@ pick_one_preserving_merges () {
> if test -f "$rewritten"/$p
> then
> new_p=$(cat "$rewritten"/$p)
> -
> - # If the todo reordered commits, and our parent is marked for
> - # rewriting, but hasn't been gotten to yet, assume the user meant to
> - # drop it on top of the current HEAD
> + # If the todo reordered commits, and our parent is
> + # marked for rewriting, but hasn't been gotten to yet,
> + # assume the user meant to drop it on top of the
> + # current HEAD
Just line wrapping.
> @@ -379,7 +394,7 @@ pick_one_preserving_merges () {
> then
> # detach HEAD to current parent
> output git checkout $first_parent 2> /dev/null ||
> - die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
> + die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
> fi
Ditto.
> @@ -553,7 +568,7 @@ do_next () {
> pick|p)
> comment_for_reflog pick
>
> - mark_action_done
> + mark_action_done $sha1 "$rest"
This demands more attention from the readers than all the changes we
have seen so far which were just line wrapping and whitespace
changes. That is why it is better to leave these changes to a
separate patch after preliminary clean-up. It allows reviewers'
eyes coast over the clean-up step, and then lets them focus on the
more "meaningful" changes like this one.
> @@ -637,7 +652,7 @@ you are able to reword the commit.")"
> read -r command rest < "$todo"
> mark_action_done
> eval_gettextln "Executing: \$rest"
> - "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
> + "${SHELL:-/bin/sh}" -c "$rest" # Actual execution
Why? This change is not justified in the proposed log message.
next prev parent reply other threads:[~2018-03-21 19:42 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-20 20:45 [RFC PATCH 0/3] rebase-interactive Wink Saville
2018-03-20 20:45 ` [RFC PATCH 1/3] rebase-interactive: create git-rebase--interactive--lib.sh Wink Saville
2018-03-20 20:45 ` [RFC PATCH 2/3] rebase-interactive: create git-rebase--interactive--preserve-merges Wink Saville
2018-03-20 20:45 ` [RFC PATCH 3/3] rebase-interactive: refactor git-rebase--interactive to use library Wink Saville
2018-03-20 21:11 ` [RFC PATCH 0/3] rebase-interactive Eric Sunshine
2018-03-20 21:23 ` Wink Saville
2018-03-20 21:47 ` Eric Sunshine
2018-03-21 3:31 ` Wink Saville
2018-03-21 8:54 ` Eric Sunshine
2018-03-21 17:44 ` [RFC PATCH v2 0/1] rebase-interactive: Add git_rebase__interactive__preserve_merges Wink Saville
2018-03-21 17:44 ` [RFC PATCH v2 1/1] " Wink Saville
2018-03-21 19:42 ` Junio C Hamano [this message]
2018-03-21 21:49 ` Wink Saville
2018-03-21 22:43 ` Junio C Hamano
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=xmqqy3ilnrkn.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).