From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Dmitry Potapov <dpotapov@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH] rebase -i: commit when continuing after "edit"
Date: Mon, 24 Sep 2007 22:37:08 -0700 [thread overview]
Message-ID: <7vlkav71bv.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <Pine.LNX.4.64.0709240121080.28395@racer.site> (Johannes Schindelin's message of "Mon, 24 Sep 2007 01:29:30 +0100 (BST)")
> When doing an "edit" on a commit, editing and git-adding some files,
> "git rebase -i" complained about a missing "author-script". The idea was
> that the user would call "git commit --amend" herself.
>
> But we can be nice and do that for the user.
>
> To do this, rebase -i stores the author script and message whenever
> writing out a patch, and it remembers to do an "amend" by creating the
> file "amend" in "$DOTEST".
>
> Noticed by Dmitry Potapov.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2fa53fd..5982967 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1,484 +1,488 @@
> #!/bin/sh
> ...
> output () {
> case "$VERBOSE" in
> '')
> "$@" > "$DOTEST"/output 2>&1
> status=$?
> test $status != 0 &&
> cat "$DOTEST"/output
> return $status
> ;;
One more level of indent, please.
> *)
> "$@"
> esac
I find it is usually less error prone to help the longer term
maintainability if you do not omit double-semicolon before esac.
> }
>
> require_clean_work_tree () {
> # test if working tree is dirty
> git rev-parse --verify HEAD > /dev/null &&
> git update-index --refresh &&
> git diff-files --quiet &&
> git diff-index --cached --quiet HEAD ||
> die "Working tree is dirty"
> }
> ...
> mark_action_done () {
> sed -e 1q < "$TODO" >> "$DONE"
> sed -e 1d < "$TODO" >> "$TODO".new
> mv -f "$TODO".new "$TODO"
> count=$(($(wc -l < "$DONE")))
> total=$(($count+$(wc -l < "$TODO")))
> printf "Rebasing (%d/%d)\r" $count $total
> test -z "$VERBOSE" || echo
> }
>
> make_patch () {
> parent_sha1=$(git rev-parse --verify "$1"^ 2> /dev/null)
> git diff "$parent_sha1".."$1" > "$DOTEST"/patch
What's the point of using --verify when you do not error out
upon error? I find this quite inconsistent; your
require_clean_work_tree above is so nicely written.
Is there anything (other than user's common sense, which we
cannot always count on these days) that prevents the caller to
feed a root commit to this function, I wonder?
> -}
> -
> -die_with_patch () {
> test -f "$DOTEST"/message ||
> git cat-file commit $sha1 | sed "1,/^$/d" > "$DOTEST"/message
> test -f "$DOTEST"/author-script ||
> get_author_ident_from_commit $sha1 > "$DOTEST"/author-script
> +}
Are these "$sha1" still valid, or do you need "$1" given to the
make_patch function?
> pick_one () {
> no_ff=
> case "$1" in -n) sha1=$2; no_ff=t ;; *) sha1=$1 ;; esac
> output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
> test -d "$REWRITTEN" &&
> pick_one_preserving_merges "$@" && return
> parent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)
> current_sha1=$(git rev-parse --verify HEAD)
Again --verify without verifying.
> if test $no_ff$current_sha1 = $parent_sha1; then
> output git reset --hard $sha1
> test "a$1" = a-n && output git reset --soft $current_sha1
> sha1=$(git rev-parse --short $sha1)
> output warn Fast forward to $sha1
> else
> output git cherry-pick $STRATEGY "$@"
> fi
> }
>
> pick_one_preserving_merges () {
> case "$1" in -n) sha1=$2 ;; *) sha1=$1 ;; esac
> sha1=$(git rev-parse $sha1)
>
> if test -f "$DOTEST"/current-commit
> then
> current_commit=$(cat "$DOTEST"/current-commit) &&
> git rev-parse HEAD > "$REWRITTEN"/$current_commit &&
> rm "$DOTEST"/current-commit ||
> die "Cannot write current commit's replacement sha1"
> fi
>
> # rewrite parents; if none were rewritten, we can fast-forward.
> fast_forward=t
> preserve=t
> new_parents=
> for p in $(git rev-list --parents -1 $sha1 | cut -d\ -f2-)
Just a style nit. A string literal for a SP is easier to read
if written as a SP inside sq pair (i.e. ' ') not backslash
followed by a SP (i.e. \ ).
> do
> if test -f "$REWRITTEN"/$p
> then
> preserve=f
> new_p=$(cat "$REWRITTEN"/$p)
> test $p != $new_p && fast_forward=f
> case "$new_parents" in
> *$new_p*)
> ;; # do nothing; that parent is already there
> *)
> new_parents="$new_parents $new_p"
> esac
> fi
> done
> case $fast_forward in
> t)
> output warn "Fast forward to $sha1"
> test $preserve=f && echo $sha1 > "$REWRITTEN"/$sha1
Testing if concatenation of $preserve and "=f" is not an empty
string, which would almost always yield true?
> ;;
> f)
> test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
>
> first_parent=$(expr "$new_parents" : " \([^ ]*\)")
Style; typically regexp form of expr and sed expression are
easier to read with quoted with sq, not dq.
> # detach HEAD to current parent
> output git checkout $first_parent 2> /dev/null ||
> die "Cannot move HEAD to $first_parent"
>
> echo $sha1 > "$DOTEST"/current-commit
> case "$new_parents" in
> \ *\ *)
Likewise here: ' '*' '*
> # redo merge
> author_script=$(get_author_ident_from_commit $sha1)
> eval "$author_script"
> msg="$(git cat-file commit $sha1 | \
> sed -e '1,/^$/d' -e "s/[\"\\]/\\\\&/g")"
What's this backquoting about? Your "output" does not eval (and
it shouldn't), so that's not it. Working around incompatible
echo that does auto magic used to write MERGE_MSG? Can we lose
the backquoting by using printf "%s\n" there?
> ...
> nth_string () {
> case "$1" in
> *1[0-9]|*[04-9]) echo "$1"th;;
> *1) echo "$1"st;;
> *2) echo "$1"nd;;
> *3) echo "$1"rd;;
> esac
> }
Cute.
> ...
> do_next () {
> test -f "$DOTEST"/message && rm "$DOTEST"/message
> test -f "$DOTEST"/author-script && rm "$DOTEST"/author-script
> + test -f "$DOTEST"/amend && rm "$DOTEST"/amend
As you do not check the error from "rm", how are these different
from rm -f "$DOTEST/frotz"?
> read command sha1 rest < "$TODO"
> case "$command" in
> \#|'')
> mark_action_done
> ;;
Perhaps '#'*?
> ...
> edit)
> comment_for_reflog edit
>
> mark_action_done
> pick_one $sha1 ||
> die_with_patch $sha1 "Could not apply $sha1... $rest"
> make_patch $sha1
> + : > "$DOTEST"/amend
Good catch, but ':' is redundant ;-)
> warn
> warn "You can amend the commit now, with"
> warn
> warn " git commit --amend"
> warn
> exit 0
> ;;
> squash)
> comment_for_reflog squash
>
> test -z "$(grep -ve '^$' -e '^#' < $DONE)" &&
> die "Cannot 'squash' without a previous commit"
Why "test -z"? Wouldn't this be equivalent?
grep -v -q -e '^$' -e '^#' "$DONE" || die ...
>
> mark_action_done
> make_squash_message $sha1 > "$MSG"
> case "$(peek_next_command)" in
> squash)
> EDIT_COMMIT=
> USE_OUTPUT=output
> cp "$MSG" "$SQUASH_MSG"
> ;;
One more level of indent, please.
> *)
> EDIT_COMMIT=-e
> USE_OUTPUT=
> test -f "$SQUASH_MSG" && rm "$SQUASH_MSG"
Again, "test -f && rm"?
> ...
> pick_one -n $sha1 || failed=t
> author_script=$(get_author_ident_from_commit $sha1)
> echo "$author_script" > "$DOTEST"/author-script
> case $failed in
> f)
> # This is like --amend, but with a different message
> eval "$author_script"
> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
> $USE_OUTPUT git commit -F "$MSG" $EDIT_COMMIT
> ;;
The "export" here makes me somewhat nervous -- no chance these
leak into the next round?
> ...
> HEAD=$(git rev-parse --verify HEAD) || die "No HEAD?"
> UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base"
>
> test -z "$ONTO" && ONTO=$UPSTREAM
>
> : > "$DOTEST"/interactive || die "Could not mark as interactive"
> git symbolic-ref HEAD > "$DOTEST"/head-name ||
> die "Could not get HEAD"
It was somewhat annoying that you cannot "rebase -i" the
detached HEAD state.
> ...
> cat > "$TODO" << EOF
> # Rebasing $SHORTUPSTREAM..$SHORTHEAD onto $SHORTONTO
> #
> # Commands:
> # pick = use commit
> # edit = use commit, but stop for amending
> # squash = use commit, but meld into previous commit
> #
> # If you remove a line here THAT COMMIT WILL BE LOST.
> #
> EOF
> git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
> --abbrev=7 --reverse --left-right --cherry-pick \
> $UPSTREAM...$HEAD | \
> sed -n "s/^>/pick /p" >> "$TODO"
>
> test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
> die_abort "Nothing to do"
Same comment here as before, and there is another one a few
lines below. Perhaps a trivial shell function is in order?
next prev parent reply other threads:[~2007-09-25 5:37 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-09-23 22:45 git-rebase--interactive needs a better message Dmitry Potapov
2007-09-23 23:56 ` Johannes Schindelin
2007-09-24 0:29 ` [PATCH] rebase -i: commit when continuing after "edit" Johannes Schindelin
2007-09-24 9:11 ` Dmitry Potapov
2007-09-25 5:37 ` Junio C Hamano [this message]
2007-09-25 12:32 ` Johannes Schindelin
2007-09-25 13:26 ` Johannes Sixt
2007-09-25 13:50 ` Johannes Schindelin
2007-09-25 14:17 ` Johannes Sixt
2007-09-25 14:31 ` Johannes Schindelin
2007-09-25 15:01 ` Johannes Sixt
2007-09-25 15:16 ` Johannes Schindelin
2007-09-25 14:46 ` David Kastrup
2007-09-25 15:54 ` Johannes Sixt
2007-09-25 16:04 ` David Kastrup
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=7vlkav71bv.fsf@gitster.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=dpotapov@gmail.com \
--cc=git@vger.kernel.org \
/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).