threads / patch / 10015

patch, 5 partsTwo real fixes, two minor fixes, and a style fix

Subject: [PATCH 0/5] Two real fixes, two minor fixes, and a style fix

## tl;dr

9 messages between Sep 25, 2007 and Sep 26, 2007. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Johannes Schindelin· Sep 25, 2007, 15:42 UTC · lore

Hi,

this patch series arose out of the comments to the simple fix I sent out earlier, which has now become the first patch of the series.

Ciao, Dscho

Johannes Schindelin· Sep 25, 2007, 15:42 UTC · re: Johannes Schindelin · lore

[PATCH 1/5] rebase -i: commit when continuing after "edit"

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.

Noticed by Dmitry Potapov.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh    |   17 +++++++++++------
 t/t3404-rebase-interactive.sh |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 7 deletions(-)

Show changes to 2 files +24 −7

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d2983d1..58f6f28 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,15 +77,16 @@ mark_action_done () {
 }
 
 make_patch () {
-	parent_sha1=$(git rev-parse --verify "$1"^ 2> /dev/null)
+	parent_sha1=$(git rev-parse --verify "$1"^) ||
+		die "Cannot get patch for $1^"
 	git diff "$parent_sha1".."$1" > "$DOTEST"/patch
+	test -f "$DOTEST"/message ||
+		git cat-file commit "$1" | sed "1,/^$/d" > "$DOTEST"/message
+	test -f "$DOTEST"/author-script ||
+		get_author_ident_from_commit "$1" > "$DOTEST"/author-script
 }
 
 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
 	make_patch "$1"
 	die "$2"
 }
@@ -214,6 +215,7 @@ peek_next_command () {
 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
 	read command sha1 rest < "$TODO"
 	case "$command" in
 	\#|'')
@@ -233,6 +235,7 @@ do_next () {
 		pick_one $sha1 ||
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
 		make_patch $sha1
+		: > "$DOTEST"/amend
 		warn
 		warn "You can amend the commit now, with"
 		warn
@@ -332,7 +335,9 @@ do
 		git update-index --refresh &&
 		git diff-files --quiet &&
 		! git diff-index --cached --quiet HEAD &&
-		. "$DOTEST"/author-script &&
+		. "$DOTEST"/author-script && {
+			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
+		} &&
 		export GIT_AUTHOR_NAME GIT_AUTHOR_NAME GIT_AUTHOR_DATE &&
 		git commit -F "$DOTEST"/message -e
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 718c9c1..1af73a4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -80,7 +80,7 @@ cat "$1".tmp
 action=pick
 for line in $FAKE_LINES; do
 	case $line in
-	squash)
+	squash|edit)
 		action="$line";;
 	*)
 		echo sed -n "${line}s/^pick/$action/p"
@@ -297,4 +297,16 @@ test_expect_success 'ignore patch if in upstream' '
 	test $HEAD = $(git rev-parse HEAD^)
 '
 
+test_expect_success '--continue tries to commit, even for "edit"' '
+	parent=$(git rev-parse HEAD^) &&
+	test_tick &&
+	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
+	echo edited > file7 &&
+	git add file7 &&
+	FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue &&
+	test edited = $(git show HEAD:file7) &&
+	git show HEAD | grep chouette &&
+	test $parent = $(git rev-parse HEAD^)
+'
+
 test_done

-- 
1.5.3.2.1057.gf4dc1

Johannes Schindelin· Sep 25, 2007, 15:42 UTC · re: Johannes Schindelin · lore

[PATCH 2/5] rebase -i: style fixes and minor cleanups

This patch indents ";;" consistently with the rest of git's shell scripts, and makes sure that ";;" are before each "esac".

It introduces a helper function "has_action", to make it easier to read the intentions of the code.

Errors from "git rev-parse --verify" are no longer ignored.

Spaces are quoted using single quotes instead of a backslash, for readability.

A "test $preserve=f" (missing spaces) was fixed; hashes are no longer written to "$DOTEST"/rewritten/ unnecessarily.

We used to quote the message for a squash, only to have "echo" unquote it. Now we use "printf" and do not need to quote to start with.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh |   48 +++++++++++++++++++++++++++----------------
 1 files changed, 30 insertions(+), 18 deletions(-)

Show changes to git-rebase--interactive.sh +30 −18

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 58f6f28..c850411 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -41,9 +41,10 @@ output () {
 		test $status != 0 &&
 			cat "$DOTEST"/output
 		return $status
-	;;
+		;;
 	*)
 		"$@"
+		;;
 	esac
 }
 
@@ -63,6 +64,7 @@ comment_for_reflog () {
 	''|rebase*)
 		GIT_REFLOG_ACTION="rebase -i ($1)"
 		export GIT_REFLOG_ACTION
+		;;
 	esac
 }
 
@@ -96,13 +98,18 @@ die_abort () {
 	die "$1"
 }
 
+has_action () {
+	grep -vqe '^$' -e '^#' "$1"
+}
+
 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)
+	parent_sha1=$(git rev-parse --verify $sha1^) ||
+		die "Could not get the parent of $sha1"
 	current_sha1=$(git rev-parse --verify HEAD)
 	if test $no_ff$current_sha1 = $parent_sha1; then
 		output git reset --hard $sha1
@@ -130,7 +137,7 @@ pick_one_preserving_merges () {
 	fast_forward=t
 	preserve=t
 	new_parents=
-	for p in $(git rev-list --parents -1 $sha1 | cut -d\  -f2-)
+	for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)
 	do
 		if test -f "$REWRITTEN"/$p
 		then
@@ -142,41 +149,43 @@ pick_one_preserving_merges () {
 				;; # 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
+		test $preserve = f || echo $sha1 > "$REWRITTEN"/$sha1
 		;;
 	f)
 		test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
 
-		first_parent=$(expr "$new_parents" : " \([^ ]*\)")
+		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
 		# 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
-		\ *\ *)
+		' '*' '*)
 			# 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")"
+			msg="$(git cat-file commit $sha1 | sed -e '1,/^$/d')"
 			# NEEDSWORK: give rerere a chance
 			if ! output git merge $STRATEGY -m "$msg" $new_parents
 			then
-				echo "$msg" > "$GIT_DIR"/MERGE_MSG
+				printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
 				die Error redoing merge $sha1
 			fi
 			;;
 		*)
 			output git cherry-pick $STRATEGY "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
+			;;
 		esac
+		;;
 	esac
 }
 
@@ -213,12 +222,11 @@ peek_next_command () {
 }
 
 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
+	rm -f "$DOTEST"/message "$DOTEST"/author-script \
+		"$DOTEST"/amend || exit
 	read command sha1 rest < "$TODO"
 	case "$command" in
-	\#|'')
+	'#'*|'')
 		mark_action_done
 		;;
 	pick)
@@ -246,7 +254,7 @@ do_next () {
 	squash)
 		comment_for_reflog squash
 
-		test -z "$(grep -ve '^$' -e '^#' < $DONE)" &&
+		has_action "$DONE" ||
 			die "Cannot 'squash' without a previous commit"
 
 		mark_action_done
@@ -256,11 +264,12 @@ do_next () {
 			EDIT_COMMIT=
 			USE_OUTPUT=output
 			cp "$MSG" "$SQUASH_MSG"
-		;;
+			;;
 		*)
 			EDIT_COMMIT=-e
 			USE_OUTPUT=
-			test -f "$SQUASH_MSG" && rm "$SQUASH_MSG"
+			rm -f "$SQUASH_MSG" || exit
+			;;
 		esac
 
 		failed=f
@@ -280,11 +289,13 @@ do_next () {
 			warn
 			warn "Could not apply $sha1... $rest"
 			die_with_patch $sha1 ""
+			;;
 		esac
 		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		die_with_patch $sha1 "Please fix this in the file $TODO."
+		;;
 	esac
 	test -s "$TODO" && return
 
@@ -475,17 +486,18 @@ EOF
 			$UPSTREAM...$HEAD | \
 			sed -n "s/^>/pick /p" >> "$TODO"
 
-		test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
+		has_action "$TODO" ||
 			die_abort "Nothing to do"
 
 		cp "$TODO" "$TODO".backup
 		git_editor "$TODO" ||
 			die "Could not execute editor"
 
-		test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
+		has_action "$TODO" ||
 			die_abort "Nothing to do"
 
 		output git checkout $ONTO && do_rest
+		;;
 	esac
 	shift
 done

-- 
1.5.3.2.1057.gf4dc1

Johannes Schindelin· Sep 25, 2007, 15:43 UTC · re: Johannes Schindelin · lore

[PATCH 3/5] rebase -i: Fix numbers in progress report

Instead of counting all lines in done and todo, we now count the actions before outputting "$Rebasing ($count/$total)".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Show changes to git-rebase--interactive.sh +2 −2

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c850411..4f46a15 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,8 +72,8 @@ 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")))
+	count=$(($(grep -ve '^$' -e '^#' < "$DONE" | wc -l)))
+	total=$(($count+$(grep -ve '^$' -e '^#' < "$TODO" | wc -l)))
 	printf "Rebasing (%d/%d)\r" $count $total
 	test -z "$VERBOSE" || echo
 }

-- 
1.5.3.2.1057.gf4dc1

Johannes Schindelin· Sep 25, 2007, 15:43 UTC · re: Johannes Schindelin · lore

[PATCH 4/5] rebase -i: work on a detached HEAD

Earlier, rebase -i refused to rebase a detached HEAD. Now it no longer does.

Incidentally, this fixes "git gc --auto" shadowing the true exit status.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh    |   23 +++++++++++++++--------
 t/t3404-rebase-interactive.sh |    8 ++++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

Show changes to 2 files +23 −8

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4f46a15..445a299 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -312,17 +312,20 @@ do_next () {
 	else
 		NEWHEAD=$(git rev-parse HEAD)
 	fi &&
-	message="$GIT_REFLOG_ACTION: $HEADNAME onto $SHORTONTO)" &&
-	git update-ref -m "$message" $HEADNAME $NEWHEAD $OLDHEAD &&
-	git symbolic-ref HEAD $HEADNAME && {
+	case $HEADNAME in
+	refs/*)
+		message="$GIT_REFLOG_ACTION: $HEADNAME onto $SHORTONTO)" &&
+		git update-ref -m "$message" $HEADNAME $NEWHEAD $OLDHEAD &&
+		git symbolic-ref HEAD $HEADNAME
+		;;
+	esac && {
 		test ! -f "$DOTEST"/verbose ||
 			git diff --stat $(cat "$DOTEST"/head)..HEAD
 	} &&
 	rm -rf "$DOTEST" &&
+	git gc --auto &&
 	warn "Successfully rebased and updated $HEADNAME."
 
-	git gc --auto
-
 	exit
 }
 
@@ -362,7 +365,11 @@ do
 
 		HEADNAME=$(cat "$DOTEST"/head-name)
 		HEAD=$(cat "$DOTEST"/head)
-		git symbolic-ref HEAD $HEADNAME &&
+		case $HEADNAME in
+		refs/*)
+			git symbolic-ref HEAD $HEADNAME
+			;;
+		esac &&
 		output git reset --hard $HEAD &&
 		rm -rf "$DOTEST"
 		exit
@@ -439,8 +446,8 @@ do
 		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"
+		git symbolic-ref HEAD > "$DOTEST"/head-name 2> /dev/null ||
+			echo "detached HEAD" > "$DOTEST"/head-name
 
 		echo $HEAD > "$DOTEST"/head
 		echo $UPSTREAM > "$DOTEST"/upstream
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 1af73a4..f2214dd 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -309,4 +309,12 @@ test_expect_success '--continue tries to commit, even for "edit"' '
 	test $parent = $(git rev-parse HEAD^)
 '
 
+test_expect_success 'rebase a detached HEAD' '
+	grandparent=$(git rev-parse HEAD~2) &&
+	git checkout $(git rev-parse HEAD) &&
+	test_tick &&
+	FAKE_LINES="2 1" git rebase -i HEAD~2 &&
+	test $grandparent = $(git rev-parse HEAD~2)
+'
+
 test_done

-- 
1.5.3.2.1057.gf4dc1

Johannes Schindelin· Sep 25, 2007, 15:43 UTC · re: Johannes Schindelin · lore

[PATCH 5/5] rebase -i: avoid exporting GIT_AUTHOR_* variables

It is somewhat unsafe to export the GIT_AUTHOR_* variables, since a later call to git-commit or git-merge could pick them up inadvertently.

So avoid the export, using a recipe provided by Johannes Sixt.

Incidentally, this fixes authorship of merges with "rebase --preserve -i".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

Show changes to git-rebase--interactive.sh +8 −2

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 445a299..e3e89dd 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -174,7 +174,11 @@ pick_one_preserving_merges () {
 			eval "$author_script"
 			msg="$(git cat-file commit $sha1 | sed -e '1,/^$/d')"
 			# NEEDSWORK: give rerere a chance
-			if ! output git merge $STRATEGY -m "$msg" $new_parents
+			if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
+				GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
+				GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
+				output git merge $STRATEGY -m "$msg" \
+					$new_parents
 			then
 				printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
 				die Error redoing merge $sha1
@@ -281,7 +285,9 @@ do_next () {
 		f)
 			# This is like --amend, but with a different message
 			eval "$author_script"
-			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
+			GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
+			GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
+			GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
 			$USE_OUTPUT git commit -F "$MSG" $EDIT_COMMIT
 			;;
 		t)

-- 
1.5.3.2.1057.gf4dc1

Johannes Sixt· Sep 26, 2007, 10:33 UTC · re: Johannes Schindelin · lore

Re: [PATCH 5/5] rebase -i: avoid exporting GIT_AUTHOR_* variables

Johannes Schindelin schrieb:

Show 28 quoted lines

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 445a299..e3e89dd 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -174,7 +174,11 @@ pick_one_preserving_merges () {
>  			eval "$author_script"
>  			msg="$(git cat-file commit $sha1 | sed -e '1,/^$/d')"
>  			# NEEDSWORK: give rerere a chance
> -			if ! output git merge $STRATEGY -m "$msg" $new_parents
> +			if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
> +				GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
> +				GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
> +				output git merge $STRATEGY -m "$msg" \
> +					$new_parents
>  			then
>  				printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
>  				die Error redoing merge $sha1
> @@ -281,7 +285,9 @@ do_next () {
>  		f)
>  			# This is like --amend, but with a different message
>  			eval "$author_script"
> -			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
> +			GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
> +			GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
> +			GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
>  			$USE_OUTPUT git commit -F "$MSG" $EDIT_COMMIT
>  			;;
>  		t)

According to Herbert Xu's recent post (http://article.gmane.org/gmane.comp.version-control.git/59219) this won't export the variables in all shells since 'output' is a shell function. :-(

-- Hannes

Johannes Schindelin· Sep 26, 2007, 10:51 UTC · re: Johannes Sixt · lore

Re: [PATCH 5/5] rebase -i: avoid exporting GIT_AUTHOR_* variables

Hi,

On Wed, 26 Sep 2007, Johannes Sixt wrote:

Show 35 quoted lines

> Johannes Schindelin schrieb:
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 445a299..e3e89dd 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -174,7 +174,11 @@ pick_one_preserving_merges () {
> >  			eval "$author_script"
> >  			msg="$(git cat-file commit $sha1 | sed -e '1,/^$/d')"
> >  			# NEEDSWORK: give rerere a chance
> > -			if ! output git merge $STRATEGY -m "$msg"
> > $new_parents
> > +			if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
> > +				GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
> > +				GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
> > +				output git merge $STRATEGY -m "$msg" \
> > +					$new_parents
> >  			then
> >  				printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
> >  				die Error redoing merge $sha1
> > @@ -281,7 +285,9 @@ do_next () {
> >  		f)
> >  			# This is like --amend, but with a different message
> >  			eval "$author_script"
> > -			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
> > GIT_AUTHOR_DATE
> > +			GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
> > +			GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
> > +			GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
> >  			$USE_OUTPUT git commit -F "$MSG" $EDIT_COMMIT
> >  			;;
> >  		t)
> 
> According to Herbert Xu's recent post
> (http://article.gmane.org/gmane.comp.version-control.git/59219) this won't
> export the variables in all shells since 'output' is a shell function. :-(

Argh.

You know, I am _so_ close to just scrap it and rewrite it in C.

Ciao, Dscho

Johannes Sixt· Sep 26, 2007, 11:18 UTC · lore

Re: [PATCH 5/5] rebase -i: avoid exporting GIT_AUTHOR_* variables

David Kastrup schrieb:

> If $USE_OUTPUT can be either shell function or executable or shell
> function plus args, however...

UTSL: $USE_OUTPUT is either 'output' (ie. a shell function) or empty.

-- Hannes