git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] rebase -i: commit when continuing after "edit"

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 25, 2007, 05:37 UTC
Message-ID
<7vlkav71bv.fsf@gitster.siamese.dyndns.org>
In-Reply-To
<Pine.LNX.4.64.0709240121080.28395@racer.site>
Show 31 quoted lines
> 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.

Show 24 quoted lines
>  }
>  
>  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?

Show 8 quoted lines
> -}
> -
> -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?

Show 8 quoted lines
>  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.
Show 27 quoted lines
>  	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. \ ).

Show 18 quoted lines
>  	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?

Show 5 quoted lines
>  		;;
>  	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.

Show 7 quoted lines
>  		# 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: ' '*' '*
Show 5 quoted lines
>  			# 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?

Show 9 quoted lines
> ...
>  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.
Show 5 quoted lines
> ...
>  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"?

Show 5 quoted lines
>  	read command sha1 rest < "$TODO"
>  	case "$command" in
>  	\#|'')
>  		mark_action_done
>  		;;
Perhaps '#'*?
Show 9 quoted lines
> ...
>  	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 ;-)
Show 12 quoted lines
>  		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 ...
Show 9 quoted lines
>  
>  		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"?
Show 11 quoted lines
> ...
>  		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?

Show 9 quoted lines
> ...
>  		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.

Show 19 quoted lines
> ...
>  		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?

Previous: Dmitry PotapovNext: Johannes Schindelin
Message 5 of 15 in “git-rebase--interactive needs a better message”
  1. Dmitry PotapovSep 23, 2007
  2. Johannes SchindelinSep 23, 2007
  3. rebase -i: commit when continuing after "edit"Johannes Schindelin, Sep 24, 2007
  4. Dmitry PotapovSep 24, 2007
  5. Junio C HamanoSep 25, 2007
  6. Johannes SchindelinSep 25, 2007
  7. Johannes SixtSep 25, 2007
  8. Johannes SchindelinSep 25, 2007
  9. Johannes SixtSep 25, 2007
  10. Johannes SchindelinSep 25, 2007
  11. Johannes SixtSep 25, 2007
  12. Johannes SchindelinSep 25, 2007
  13. David KastrupSep 25, 2007
  14. Johannes SixtSep 25, 2007
  15. David KastrupSep 25, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.