git-rebase--interactive needs a better message

git-rebase--interactive needs a better message

[Dmitry Potapov]

I have tried to use git-rebase --interactive today, and run into a strange
error message saying:

/usr/bin/git-rebase--interactive: line 333: $GIT_DIR/.dotest-merge/author-script: No such file or directory

I had to scratch my head for a while before I realized that I forgot to
say git-commit. So, it was mine mistake, but I think that it should be
possible to have a better error message suggesting the user what to do.
I have looked at the git-rebase--interactive script, but I am not sure
that I understand it good enough to propose a patch.

To reproduce the problem, run the following script in an empty directory:
===
#!/bin/sh
set -e

git-init
echo 'version 1' > foo
git-add foo
git-commit -m 'commit 1' foo
echo 'versionn 2' >> foo
git-commit -m 'commit 2' foo
echo 'version 3' >> foo
git-commit -m 'commit 3' foo

GIT_EDITOR='sed -i -e "/commit 2/{s/^pick/edit/}"' git-rebase -i HEAD~2
sed -i -e "s/versionn/version/" foo
git-update-index foo
# Missing git-commit --amend
git-rebase --continue
===

Error message:
==
/usr/bin/git-rebase--interactive: line 333: /tmp/zzz/.git/.dotest-merge/author-script: No such file or directory
===

I use git version 1.5.3.1

Dmitry Potapov

Re: git-rebase--interactive needs a better message

[Johannes Schindelin]

Ho.

On Mon, 24 Sep 2007, Dmitry Potapov wrote:

> I have tried to use git-rebase --interactive today, and run into a strange
> error message saying:
> 
> /usr/bin/git-rebase--interactive: line 333: $GIT_DIR/.dotest-merge/author-script: No such file or directory
> 
> I had to scratch my head for a while before I realized that I forgot to
> say git-commit. So, it was mine mistake, but I think that it should be
> possible to have a better error message suggesting the user what to do.

Actually, it should just work, I'd say.  IOW git-rebase--interactive 
should store an author script also for "edit"s.

Ciao,
Dscho

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

[Johannes Schindelin]

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>
---

	On Mon, 24 Sep 2007, Dmitry Potapov wrote:

	> I have tried to use git-rebase --interactive today, and run into 
	> a strange error message saying:
	> 
	> /usr/bin/git-rebase--interactive: \
	>	line 333: $GIT_DIR/.dotest-merge/author-script: \
	>		No such file or directory

	Could you please apply this patch and try if the issue is gone?

	The patch looks a bit strange, because some code moved from 
	die_with_patch() to make_patch(), and the diff makes it look like 
	the end of the function moved instead.

	Funnily enough we discussed human readable diffs briefly on #git
	today, but I think even the best diff algorithms could not catch 
	that.

 git-rebase--interactive.sh    |   12 ++++++++----
 t/t3404-rebase-interactive.sh |   14 +++++++++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8258b7a..e4cf282 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -79,13 +79,13 @@ mark_action_done () {
 make_patch () {
 	parent_sha1=$(git rev-parse --verify "$1"^ 2> /dev/null)
 	git diff "$parent_sha1".."$1" > "$DOTEST"/patch
-}
-
-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
+}
+
+die_with_patch () {
 	make_patch "$1"
 	die "$2"
 }
@@ -214,6 +214,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 +234,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 +334,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.1039.g855b8

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

[Dmitry Potapov]

On Mon, Sep 24, 2007 at 01:29:30AM +0100, Johannes Schindelin wrote:
> 	On Mon, 24 Sep 2007, Dmitry Potapov wrote:
> 
> 	> I have tried to use git-rebase --interactive today, and run into 
> 	> a strange error message saying:
> 	> 
> 	> /usr/bin/git-rebase--interactive: \
> 	>	line 333: $GIT_DIR/.dotest-merge/author-script: \
> 	>		No such file or directory
> 
> 	Could you please apply this patch and try if the issue is gone?

I have tested it only on a couple cases, but everything works fine now.

Thank you,
Dmitry

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

[Junio C Hamano]

> 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?

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

[Johannes Schindelin]

Hi,

I'll send out a fixed patch series later, where the "commit when 
continuing" is th first one, and the style nits are addressed in the 
second one.  A third one will have a minor fixup, a 4th will address the 
detached HEAD issue.

But I need some clarification of the quoting, as detailed below.

On Mon, 24 Sep 2007, Junio C Hamano wrote:

> >  	case "$VERBOSE" in
> >  	'')
> >  		"$@" > "$DOTEST"/output 2>&1
> >  		status=$?
> >  		test $status != 0 &&
> >  			cat "$DOTEST"/output
> >  		return $status
> >  	;;
> 
> One more level of indent, please.

Sorry, of course.

> >  	*)
> >  		"$@"
> >  	esac
> 
> I find it is usually less error prone to help the longer term 
> maintainability if you do not omit double-semicolon before esac.

There were quite a few instances; I fixed them all.

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

Thanks (for the latter).  I changed that (the former).

> 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?

There is a corner case, where it is possible:

A - B - C

D - E - F

$ git checkout F
$ git rebase -i C

I am not quite certain how to fix it... And besides, this fix has no place 
in a style patch.

> > -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?

They were not even valid before.  "$1" it is.

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

Okay, fixed.

> >  	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. \ ).

Right.

> >  	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?

Ouch.  This is wrong.  And fixing that exposed another error: it should be 
an "||" instead of a "&&".

Since it did not trigger erroneous behaviour, just unnecessary writing, I 
put it into the style fixes category.

> >  		first_parent=$(expr "$new_parents" : " \([^ ]*\)")
> 
> Style; typically regexp form of expr and sed expression are easier to 
> read with quoted with sq, not dq.

But in this case, the expression does not change, right?  Fixed.

> >  			# 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?

I think so.  I never was good with quoting, but I guess that the more 
important part is the '-m "$msg"'.  This part could use a sanity check of 
somebody who gets quoting right, i.e. you.

> >  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"?

The difference: the user will not see many irritating error messages.

I changed this code to use a newly written function "remove_if_exists", 
which die()s if the file exists and could not be removed.

So this is not technically a style fix, but minor enough that I'll put it 
into that patch, too.

> >  	\#|'')
> >  		mark_action_done
> >  		;;
> 
> Perhaps '#'*?

Yeah.  It did not matter much, as not many users wrote "#something" 
anyway.

> > ...
> >  	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 ;-)

?

This idiom ": > file" is what I used ever since you said that "touch" is 
not so good (not builtin, may behave differently, etc)

Besides, it is not a catch... rebase -i needs to know if it 
continues after "edit", so it can amend the current commit (instead of 
making a new commit, as in the other cases) before continuing.

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

Yep.  I introduced a new function, "has_action".

> >  		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?

I am somewhat wary: I get quoting wrong all the time.  Would

	$USE_OUTPUT $author_script git commit -F "$MSG" $EDIT_COMMIT

work?  I have the slight suspicion that it would not, since

	eval "$author_script"

needs extra quoting in $author_script, no?

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

Will fix in a separate patch.

Ciao,
Dscho

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

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Mon, 24 Sep 2007, Junio C Hamano wrote:
>>>  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"?
> 
> The difference: the user will not see many irritating error messages.
> 
> I changed this code to use a newly written function "remove_if_exists", 
> which die()s if the file exists and could not be removed.

Why? rm -f does nothing if the file does not exist, and fails if it cannot 
remove an existing file. It all boils down to:

	rm -f "$DOTEST"/message "$DOTEST"/author-script \
		"$DOTEST"/amend || exit

>>>  			# 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?
> 
> I am somewhat wary: I get quoting wrong all the time.  Would
> 
> 	$USE_OUTPUT $author_script git commit -F "$MSG" $EDIT_COMMIT
> 
> work?  I have the slight suspicion that it would not, since
> 
> 	eval "$author_script"
> 
> needs extra quoting in $author_script, no?

How about:

	eval "$author_script"
	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

and if you dislike that, put the two questionable lines in parenthesis.

-- Hannes

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

[Johannes Schindelin]

Hi,

On Tue, 25 Sep 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > On Mon, 24 Sep 2007, Junio C Hamano wrote:
> > > >  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"?
> > 
> > The difference: the user will not see many irritating error messages.
> > 
> > I changed this code to use a newly written function "remove_if_exists",
> > which die()s if the file exists and could not be removed.
> 
> Why? rm -f does nothing if the file does not exist, and fails if it cannot
> remove an existing file. It all boils down to:
> 
> 	rm -f "$DOTEST"/message "$DOTEST"/author-script \
> 		"$DOTEST"/amend || exit

You're completely right.  I somehow assumed that it would print an 
annoying message, but I was wrong.

BTW I am continually amazed at the ease of rebase -i to fix issues like 
these in a patch series.  Thanks Eric!

> > > >  			# 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?
> > 
> > I am somewhat wary: I get quoting wrong all the time.  Would
> > 
> > 	$USE_OUTPUT $author_script git commit -F "$MSG" $EDIT_COMMIT
> > 
> > work?  I have the slight suspicion that it would not, since
> > 
> > 	eval "$author_script"
> > 
> > needs extra quoting in $author_script, no?
> 
> How about:
> 
> 	eval "$author_script"
> 	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
> 
> and if you dislike that, put the two questionable lines in parenthesis.

That looks ugly.  I'd rather have something like

	eval "$USE_OUTPUT $author_script git commit -F \"$MSG\" $EDIT_COMMIT"

but I'm not quite certain if that is enough, what with the funny 
characters people put into path names these days ($MSG points to 
"$DOTEST"/message).

BTW I just realised that the _same_ issue should have occurred in the 
"squash" case, but there I _forgot_ to export the environment variables.

Ciao,
Dscho

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

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Tue, 25 Sep 2007, Johannes Sixt wrote:
>> How about:
>>
>> 	eval "$author_script"
>> 	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
>>
>> and if you dislike that, put the two questionable lines in parenthesis.
> 
> That looks ugly.  I'd rather have something like
> 
> 	eval "$USE_OUTPUT $author_script git commit -F \"$MSG\" $EDIT_COMMIT"
> 
> but I'm not quite certain if that is enough, what with the funny 
> characters people put into path names these days ($MSG points to 
> "$DOTEST"/message).

I, too, find it ugly, but I think it's the most readable way to do it. Your 
version is certainly underquoted.

I poked around a bit, but one major obstacle is that the assignments in 
$author_script are on separate lines, which you would have to splice into a 
single line before you can insert them in the eval.

-- Hannes

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

[Johannes Schindelin]

Hi,

On Tue, 25 Sep 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> 
> > On Tue, 25 Sep 2007, Johannes Sixt wrote:
> > > How about:
> > > 
> > > 	eval "$author_script"
> > > 	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
> > > 
> > > and if you dislike that, put the two questionable lines in parenthesis.
> > 
> > That looks ugly.  I'd rather have something like
> > 
> > 	eval "$USE_OUTPUT $author_script git commit -F \"$MSG\" $EDIT_COMMIT"
> > 
> > but I'm not quite certain if that is enough, what with the funny 
> > characters people put into path names these days ($MSG points to 
> > "$DOTEST"/message).
> 
> I, too, find it ugly, but I think it's the most readable way to do it. 
> Your version is certainly underquoted.
> 
> I poked around a bit, but one major obstacle is that the assignments in 
> $author_script are on separate lines, which you would have to splice 
> into a single line before you can insert them in the eval.

But is your version not underquoted, too?  For example, if the author name 
is, say 'Johannes "Dscho" Schindelin', would your version still get the \" 
in the name?

Ciao,
Dscho

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

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Tue, 25 Sep 2007, Johannes Sixt wrote:
> 
>> Johannes Schindelin schrieb:
>>
>>> On Tue, 25 Sep 2007, Johannes Sixt wrote:
>>>> How about:
>>>>
>>>> 	eval "$author_script"
>>>> 	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
>>>>
>>>> and if you dislike that, put the two questionable lines in parenthesis.
>>> That looks ugly.  I'd rather have something like
>>>
>>> 	eval "$USE_OUTPUT $author_script git commit -F \"$MSG\" $EDIT_COMMIT"
>>>
>>> but I'm not quite certain if that is enough, what with the funny 
>>> characters people put into path names these days ($MSG points to 
>>> "$DOTEST"/message).
>> I, too, find it ugly, but I think it's the most readable way to do it. 
>> Your version is certainly underquoted.
>>
>> I poked around a bit, but one major obstacle is that the assignments in 
>> $author_script are on separate lines, which you would have to splice 
>> into a single line before you can insert them in the eval.
> 
> But is your version not underquoted, too?  For example, if the author name 
> is, say 'Johannes "Dscho" Schindelin', would your version still get the \" 
> in the name?

No, it's not underquoted; yes, it would still get the \" in the name. The 
shell parses the assignments

	GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME"

only once; it does not parse it again after the dq'd string was expanded.

-- Hannes

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

[Johannes Schindelin]

Hi,

On Tue, 25 Sep 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> 
> > On Tue, 25 Sep 2007, Johannes Sixt wrote:
> > 
> > > Johannes Schindelin schrieb:
> > > 
> > > > On Tue, 25 Sep 2007, Johannes Sixt wrote:
> > > > > How about:
> > > > > 
> > > > > 	eval "$author_script"
> > > > > 	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
> > > > > 
> > > > > and if you dislike that, put the two questionable lines in 
> > > > > parenthesis.
> > > >
> > > > That looks ugly.  I'd rather have something like
> > > > 
> > > > 	eval "$USE_OUTPUT $author_script git commit -F \"$MSG\" $EDIT_COMMIT"
> > > > 
> > > > but I'm not quite certain if that is enough, what with the funny 
> > > > characters people put into path names these days ($MSG points to 
> > > > "$DOTEST"/message).
> > >
> > > I, too, find it ugly, but I think it's the most readable way to do 
> > > it. Your version is certainly underquoted.
> > > 
> > > I poked around a bit, but one major obstacle is that the assignments 
> > > in $author_script are on separate lines, which you would have to 
> > > splice into a single line before you can insert them in the eval.
> > 
> > But is your version not underquoted, too?  For example, if the author 
> > name is, say 'Johannes "Dscho" Schindelin', would your version still 
> > get the \" in the name?
> 
> No, it's not underquoted; yes, it would still get the \" in the name. 
> The shell parses the assignments
> 
> 	GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME"
> 
> only once; it does not parse it again after the dq'd string was expanded.

Ah, okay.  I'll go with your version then.

Ciao,
Dscho

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

[David Kastrup]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Johannes Schindelin schrieb:
>> Hi,
>>
>> On Tue, 25 Sep 2007, Johannes Sixt wrote:
>>> How about:
>>>
>>> 	eval "$author_script"
>>> 	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
>>>
>>> and if you dislike that, put the two questionable lines in parenthesis.
>>
>> That looks ugly.  I'd rather have something like
>>
>> 	eval "$USE_OUTPUT $author_script git commit -F \"$MSG\" $EDIT_COMMIT"
>>
>> but I'm not quite certain if that is enough, what with the funny
>> characters people put into path names these days ($MSG points to
>> "$DOTEST"/message).
>
> I, too, find it ugly, but I think it's the most readable way to do
> it. Your version is certainly underquoted.

Proper quoting in my book would be

eval "$USE_OUTPUT $author_script git commit -F" '"$MSG"' "$EDIT_COMMIT"

(I am not sure I am correct about $EDIT_COMMIT, not having looked at
its definition).

Important is the double quotation of $MSG to keep it as a single argument.

> I poked around a bit, but one major obstacle is that the assignments
> in $author_script are on separate lines, which you would have to
> splice into a single line before you can insert them in the eval.

Hm?  Why?  Newlines separate assignments just as reliable as spaces
do.  They are primarily special to the tty as line separators, not the
shell as such.

-- 
David Kastrup

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

[Johannes Sixt]

David Kastrup schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> I poked around a bit, but one major obstacle is that the assignments
>> in $author_script are on separate lines, which you would have to
>> splice into a single line before you can insert them in the eval.
> 
> Hm?  Why?  Newlines separate assignments just as reliable as spaces
> do.  They are primarily special to the tty as line separators, not the
> shell as such.

The task here is to have the assignments on the same line as the command at 
the end so that they are locally exported. Here we are inside an 'eval', and 
the new-lines *do* what their name suggest: make new lines.

-- Hannes

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

[David Kastrup]

Johannes Sixt <j.sixt@viscovery.net> writes:

> David Kastrup schrieb:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>> I poked around a bit, but one major obstacle is that the assignments
>>> in $author_script are on separate lines, which you would have to
>>> splice into a single line before you can insert them in the eval.
>>
>> Hm?  Why?  Newlines separate assignments just as reliable as spaces
>> do.  They are primarily special to the tty as line separators, not the
>> shell as such.
>
> The task here is to have the assignments on the same line as the
> command at the end so that they are locally exported. Here we are
> inside an 'eval', and the new-lines *do* what their name suggest: make
> new lines.

The documentation to eval clearly states:

`eval'
          eval [ARGUMENTS]
     The arguments are concatenated together into a single command,
     which is then read and executed, and its exit status returned as
     the exit status of `eval'.  If there are no arguments or only
     empty arguments, the return status is zero.

So we are talking about a single command here.  However, we indeed get

$ eval "echo x
> y
> z"
x
bash: y: command not found
bash: z: command not found
$

Um, so I have been talking nonsense.  Does "the docs made me do it"
count as excuse?

-- 
David Kastrup