[PATCH v2 0/8] rebase.autostash completed

[PATCH v2 0/8] rebase.autostash completed

[Ramkumar Ramachandra]

Hi,

The feature is finished with documentation and tests in this
iteration.  I've written an extensive t3420 which proves that the
feature works flawlessly.  Further, I've made every attempt to
actually explain what I'm doing: I've taken care to inspect all the
return values.

Overall, I'm elated with the design and interface.  I think it is most
intuitive, while not trading off power/ flexibility.

One subtle detail that you might disagree with: I report success if
the rebase succeeds but the stash application fails.  Are we okay with
this?

Also, does t3420 exercise all the cases sufficiently?  Have I missed
anything?

Enjoy reading and reviewing this.

Ramkumar Ramachandra (8):
  am: suppress error output from a conditional
  rebase -i: don't error out if $state_dir already exists
  am: tighten a conditional that checks for $dotest
  rebase: prepare to do generic housekeeping
  am: return control to caller, for housekeeping
  rebase -i: return control to caller, for housekeeping
  rebase --merge: return control to caller, for housekeeping
  rebase: implement --[no-]autostash and rebase.autostash

 Documentation/config.txt     |   8 +++
 Documentation/git-rebase.txt |  10 +++
 git-am.sh                    |  15 +++--
 git-rebase--am.sh            |   8 +--
 git-rebase--interactive.sh   |  11 ++--
 git-rebase--merge.sh         |   5 +-
 git-rebase.sh                |  46 +++++++++++++-
 t/t3420-rebase-autostash.sh  | 148 +++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 233 insertions(+), 18 deletions(-)
 create mode 100755 t/t3420-rebase-autostash.sh

-- 
1.8.3.rc1.52.gc14258d

[PATCH 1/8] am: suppress error output from a conditional

[Ramkumar Ramachandra]

In preparation for a later patch that creates $dotest/autostash in
git-rebase.sh before anything else happens, don't assume that the
presence of a $dotest directory implies the existence of the $next and
$last files.  The check for the files is in a conditional anyway, but
`cat` is executed on potentially non-existent files.  Suppress this
error output.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-am.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c092855..88aa438 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -446,8 +446,8 @@ done
 # If the dotest directory exists, but we have finished applying all the
 # patches in them, clear it out.
 if test -d "$dotest" &&
-   last=$(cat "$dotest/last") &&
-   next=$(cat "$dotest/next") &&
+   last=$(cat "$dotest/last" 2>/dev/null) &&
+   next=$(cat "$dotest/next" 2>/dev/null) &&
    test $# != 0 &&
    test "$next" -gt "$last"
 then
-- 
1.8.3.rc1.52.gc14258d

[PATCH 2/8] rebase -i: don't error out if $state_dir already exists

[Ramkumar Ramachandra]

In preparation for a later patch that will create $state_dir/autostash
in git-rebase.sh before anything else can happen, change a `mkdir
$state_dir` call to `mkdir -p $state_dir`.  The change is safe,
because this is not a test to detect an in-progress rebase (that is
already done much earlier in git-rebase.sh).

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5822b2c..3411139 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -842,7 +842,7 @@ then
 fi
 
 orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
-mkdir "$state_dir" || die "Could not create temporary $state_dir"
+mkdir -p "$state_dir" || die "Could not create temporary $state_dir"
 
 : > "$state_dir"/interactive || die "Could not mark as interactive"
 write_basic_state
-- 
1.8.3.rc1.52.gc14258d

[PATCH 3/8] am: tighten a conditional that checks for $dotest

[Ramkumar Ramachandra]

We currently assume that, if a $dotest directory exists, an am had
been called earlier.  This assumption might get our conditional to
match a stray $dotest directory created somewhere else, and result in
failures down the line. So, tighten the conditional by additionally
looking for the file $dotest/last.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-am.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index 88aa438..f4ef8fc 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -454,7 +454,7 @@ then
    rm -fr "$dotest"
 fi
 
-if test -d "$dotest"
+if test -d "$dotest" && test -f "$dotest/last"
 then
 	case "$#,$skip$resolved$abort" in
 	0,*t*)
-- 
1.8.3.rc1.52.gc14258d

[PATCH 4/8] rebase: prepare to do generic housekeeping

[Ramkumar Ramachandra]

On successful completion of a rebase in git-rebase--$backend.sh, the
$backend script cleans up on its own and exits.  The cleanup routine
is however, independent of the $backend, and each $backend script
unnecessarily duplicates this work:

    rm -rf "$state_dir"
    git gc --auto

Prepare git-rebase.sh for later patches that return control from each
$backend script back to us, for performing this generic cleanup
routine.

Another advantage is that git-rebase.sh can implement a generic
finish_rebase() to possibly do additional tasks in addition to the
cleanup.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index 2c692c3..84dc7b0 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -150,6 +150,13 @@ run_specific_rebase () {
 		autosquash=
 	fi
 	. git-rebase--$type
+	ret=$?
+	if test $ret = 0
+	then
+		git gc --auto &&
+		rm -rf "$state_dir"
+	fi
+	exit $ret
 }
 
 run_pre_rebase_hook () {
-- 
1.8.3.rc1.52.gc14258d

[PATCH 5/8] am: return control to caller, for housekeeping

[Ramkumar Ramachandra]

We only need to do these two tasks

    git gc --auto
    rm -fr "$dotest"

ourselves if the script was invoked as a standalone program; when
invoked with --rebasing (from git-rebase--am.sh), cascade control back
to the ultimate caller git-rebase.sh to do this for us.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-am.sh         | 9 +++++++--
 git-rebase--am.sh | 8 ++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index f4ef8fc..47c1021 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -904,5 +904,10 @@ if test -s "$dotest"/rewritten; then
     fi
 fi
 
-rm -fr "$dotest"
-git gc --auto
+# If am was called with --rebasing (from git-rebase--am), it's up to
+# the caller to take care of housekeeping.
+if ! test -f "$dotest/rebasing"
+then
+	rm -fr "$dotest"
+	git gc --auto
+fi
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index f84854f..34e3102 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -7,12 +7,12 @@ case "$action" in
 continue)
 	git am --resolved --resolvemsg="$resolvemsg" &&
 	move_to_original_branch
-	exit
+	return
 	;;
 skip)
 	git am --skip --resolvemsg="$resolvemsg" &&
 	move_to_original_branch
-	exit
+	return
 	;;
 esac
 
@@ -56,7 +56,7 @@ else
 
 		As a result, git cannot rebase them.
 		EOF
-		exit $?
+		return $?
 	fi
 
 	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
@@ -68,7 +68,7 @@ fi
 if test 0 != $ret
 then
 	test -d "$state_dir" && write_basic_state
-	exit $ret
+	return $ret
 fi
 
 move_to_original_branch
-- 
1.8.3.rc1.52.gc14258d

[PATCH 6/8] rebase -i: return control to caller, for housekeeping

[Ramkumar Ramachandra]

Return control to the caller git-rebase.sh to get these two tasks

    rm -fr "$dotest"
    git gc --auto

done by it.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase--interactive.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3411139..f953d8d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -628,17 +628,16 @@ do_next () {
 		"$GIT_DIR"/hooks/post-rewrite rebase < "$rewritten_list"
 		true # we don't care if this hook failed
 	fi &&
-	rm -rf "$state_dir" &&
-	git gc --auto &&
 	warn "Successfully rebased and updated $head_name."
 
-	exit
+	return 1 # not failure; just to break the do_rest loop
 }
 
+# can only return 0, when the infinite loop breaks
 do_rest () {
 	while :
 	do
-		do_next
+		do_next || break
 	done
 }
 
@@ -805,11 +804,13 @@ first and then run 'git rebase --continue' again."
 
 	require_clean_work_tree "rebase"
 	do_rest
+	return 0
 	;;
 skip)
 	git rerere clear
 
 	do_rest
+	return 0
 	;;
 edit-todo)
 	git stripspace --strip-comments <"$todo" >"$todo".new
-- 
1.8.3.rc1.52.gc14258d

[PATCH 7/8] rebase --merge: return control to caller, for housekeeping

[Ramkumar Ramachandra]

Return control to the caller git-rebase.sh to get these two tasks

    rm -fr "$dotest"
    git gc --auto

done by it.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase--merge.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index b10f2cf..16d1817 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -96,7 +96,6 @@ finish_rb_merge () {
 			"$GIT_DIR"/hooks/post-rewrite rebase <"$state_dir"/rewritten
 		fi
 	fi
-	rm -r "$state_dir"
 	say All done.
 }
 
@@ -110,7 +109,7 @@ continue)
 		continue_merge
 	done
 	finish_rb_merge
-	exit
+	return
 	;;
 skip)
 	read_state
@@ -122,7 +121,7 @@ skip)
 		continue_merge
 	done
 	finish_rb_merge
-	exit
+	return
 	;;
 esac
 
-- 
1.8.3.rc1.52.gc14258d

[PATCH 8/8] rebase: implement --[no-]autostash and rebase.autostash

[Ramkumar Ramachandra]

This new feature allows a rebase to be executed on a dirty worktree.
It works by creating a temporary stash and storing it in
$state_dir/autostash before the operation, and applying it after a
successful operation.  It will be removed along with the $state_dir if
the operation is aborted.

The feature creates a special stash that does not affect the normal
stash's reflogs, and will therefore be invisible to the end user.
This special stash is essentially a dangling merge commit which has
reasonable lifetime specified by gc.pruneexpire (default 2 weeks).

Most significantly, this feature means that a caller like pull (with
pull.rebase set to true) can easily be patched to remove the
require_clean_work_tree restriction.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt     |   8 +++
 Documentation/git-rebase.txt |  10 +++
 git-rebase.sh                |  43 ++++++++++++-
 t/t3420-rebase-autostash.sh  | 148 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 206 insertions(+), 3 deletions(-)
 create mode 100755 t/t3420-rebase-autostash.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..7fd4035 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1867,6 +1867,14 @@ rebase.stat::
 rebase.autosquash::
 	If set to true enable '--autosquash' option by default.
 
+rebase.autostash::
+	When set to true, automatically create a temporary stash
+	before the operation begins, and apply it after the operation
+	ends.  This means that you can run rebase on a dirty worktree.
+	However, use with care: the final stash application after a
+	successful rebase might result in non-trivial conflicts.
+	Defaults to false.
+
 receive.autogc::
 	By default, git-receive-pack will run "git-gc --auto" after
 	receiving data from git-push and updating refs.  You can stop
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index aca8405..c84854a 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -208,6 +208,9 @@ rebase.stat::
 rebase.autosquash::
 	If set to true enable '--autosquash' option by default.
 
+rebase.autostash::
+	If set to true enable '--autostash' option by default.
+
 OPTIONS
 -------
 --onto <newbase>::
@@ -394,6 +397,13 @@ If the '--autosquash' option is enabled by default using the
 configuration variable `rebase.autosquash`, this option can be
 used to override and disable this setting.
 
+--[no-]autostash::
+	Automatically create a temporary stash before the operation
+	begins, and apply it after the operation ends.  This means
+	that you can run rebase on a dirty worktree.  However, use
+	with care: the final stash application after a successful
+	rebase might result in non-trivial conflicts.
+
 --no-ff::
 	With --interactive, cherry-pick all rebased commits instead of
 	fast-forwarding over the unchanged ones.  This ensures that the
diff --git a/git-rebase.sh b/git-rebase.sh
index 84dc7b0..b58c6d9 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -13,6 +13,7 @@ git-rebase --continue | --abort | --skip | --edit-todo
  Available options are
 v,verbose!         display a diffstat of what changed upstream
 q,quiet!           be quiet. implies --no-stat
+autostash!         automatically stash/stash pop before and after
 onto=!             rebase onto given branch instead of upstream
 p,preserve-merges! try to recreate merges instead of ignoring them
 s,strategy=!       use the given merge strategy
@@ -64,6 +65,7 @@ apply_dir="$GIT_DIR"/rebase-apply
 verbose=
 diffstat=
 test "$(git config --bool rebase.stat)" = true && diffstat=t
+autostash="$(git config --bool rebase.autostash || echo false)"
 git_am_opt=
 rebase_root=
 force_rebase=
@@ -143,6 +145,28 @@ move_to_original_branch () {
 	esac
 }
 
+finish_rebase () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 2>&1 >/dev/null
+		then
+			echo "Applied autostash"
+		else
+			ref_stash=refs/stash &&
+			: >>"$GIT_DIR/logs/$ref_stash" &&
+			git update-ref -m "autostash" $ref_stash $stash_sha1 \
+				|| die "$(eval_gettext 'Cannot store $stash_sha1')" &&
+			echo "
+$(gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in stash@{0}.
+You can apply or drop it at any time.')"
+		fi
+	fi
+	git gc --auto &&
+	rm -rf "$state_dir"
+}
+
 run_specific_rebase () {
 	if [ "$interactive_rebase" = implied ]; then
 		GIT_EDITOR=:
@@ -153,8 +177,7 @@ run_specific_rebase () {
 	ret=$?
 	if test $ret = 0
 	then
-		git gc --auto &&
-		rm -rf "$state_dir"
+		finish_rebase
 	fi
 	exit $ret
 }
@@ -248,6 +271,9 @@ do
 	--stat)
 		diffstat=t
 		;;
+	--autostash)
+		autostash=true
+		;;
 	-v)
 		verbose=t
 		diffstat=t
@@ -348,7 +374,7 @@ abort)
 		;;
 	esac
 	output git reset --hard $orig_head
-	rm -r "$state_dir"
+	finish_rebase
 	exit
 	;;
 edit-todo)
@@ -487,6 +513,17 @@ case "$#" in
 	;;
 esac
 
+if test "$autostash" = true && ! (require_clean_work_tree) 2>/dev/null
+then
+	stash_sha1=$(git stash create "autostash") \
+		|| die "$(gettext 'Cannot autostash')" &&
+	mkdir -p "$state_dir" &&
+	echo $stash_sha1 >"$state_dir/autostash" &&
+	stash_abbrev=$(git rev-parse --short $stash_sha1) &&
+	echo "$(eval_gettext 'Created autostash: $stash_abbrev')" &&
+	git reset --hard
+fi
+
 require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
 
 # Now we are rebasing commits $upstream..$orig_head (or with --root,
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
new file mode 100755
index 0000000..8386998
--- /dev/null
+++ b/t/t3420-rebase-autostash.sh
@@ -0,0 +1,148 @@
+#!/bin/sh
+#
+# Copyright (c) 2013 Ramkumar Ramachandra
+#
+
+test_description='git rebase --autostash tests'
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo hello-world >file0 &&
+	git add . &&
+	test_tick &&
+	git commit -m "initial commit" &&
+	git checkout -b feature-branch &&
+	echo another-hello >file1 &&
+	echo goodbye >file2 &&
+	git add . &&
+	test_tick &&
+	git commit -m "second commit" &&
+	echo final-goodbye >file3 &&
+	git add . &&
+	test_tick &&
+	git commit -m "third commit"
+	git checkout -b unrelated-onto-branch master &&
+	echo unrelated >file4 &&
+	git add . &&
+	test_tick &&
+	git commit -m "unrelated commit"
+	git checkout -b related-onto-branch master &&
+	echo conflicting-change >file2 &&
+	git add . &&
+	test_tick &&
+	git commit -m "related commit"
+'
+
+testrebase() {
+	type=$1
+	dotest=$2
+
+	test_expect_success "rebase$type: dirty worktree, non-conflicting rebase" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch &&
+		echo dirty >>file3 &&
+		git rebase$type unrelated-onto-branch &&
+		grep unrelated file4 &&
+		grep dirty file3 &&
+		git checkout feature-branch
+	'
+
+	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch
+		echo dirty >>file3 &&
+		git add file3 &&
+		git rebase$type unrelated-onto-branch &&
+		grep unrelated file4 &&
+		grep dirty file3 &&
+		git checkout feature-branch
+	'
+
+	test_expect_success "rebase$type: conflicting rebase" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch &&
+		echo dirty >>file3 &&
+		test_must_fail git rebase$type related-onto-branch &&
+		test_path_is_file $dotest/autostash &&
+		! grep dirty file3 &&
+		rm -rf $dotest &&
+		git reset --hard &&
+		git checkout feature-branch
+	'
+
+	test_expect_success "rebase$type: --continue" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch &&
+		echo dirty >>file3 &&
+		test_must_fail git rebase$type related-onto-branch &&
+		test_path_is_file $dotest/autostash &&
+		! grep dirty file3 &&
+		echo "conflicting-plus-goodbye" >file2 &&
+		git add file2 &&
+		git rebase --continue &&
+		test_path_is_missing $dotest/autostash &&
+		grep dirty file3 &&
+		git checkout feature-branch
+	'
+
+	test_expect_success "rebase$type: --skip" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch &&
+		echo dirty >>file3 &&
+		test_must_fail git rebase$type related-onto-branch &&
+		test_path_is_file $dotest/autostash &&
+		! grep dirty file3 &&
+		git rebase --skip &&
+		test_path_is_missing $dotest/autostash &&
+		grep dirty file3 &&
+		git checkout feature-branch
+	'
+
+	test_expect_success "rebase$type: --abort" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch &&
+		echo dirty >>file3 &&
+		test_must_fail git rebase$type related-onto-branch &&
+		test_path_is_file $dotest/autostash &&
+		! grep dirty file3 &&
+		git rebase --abort &&
+		test_path_is_missing $dotest/autostash &&
+		grep dirty file3 &&
+		git checkout feature-branch
+	'
+
+	test_expect_success "rebase$type: non-conflicting rebase, conflicting stash" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch &&
+		echo dirty >file4 &&
+		git add file4 &&
+		git rebase$type unrelated-onto-branch &&
+		test_path_is_missing $dotest &&
+		git reset --hard &&
+		grep unrelated file4 &&
+		! grep dirty file4 &&
+		git checkout feature-branch &&
+		git stash pop &&
+		grep dirty file4
+	'
+}
+
+testrebase "" .git/rebase-apply
+testrebase " --merge" .git/rebase-merge
+testrebase " --interactive" .git/rebase-merge
+
+test_done
-- 
1.8.3.rc1.52.gc14258d

Re: [PATCH 4/8] rebase: prepare to do generic housekeeping

[Eric Sunshine]

On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> On successful completion of a rebase in git-rebase--$backend.sh, the
> $backend script cleans up on its own and exits.  The cleanup routine
> is however, independent of the $backend, and each $backend script
> unnecessarily duplicates this work:
>
>     rm -rf "$state_dir"
>     git gc --auto
>
> Prepare git-rebase.sh for later patches that return control from each
> $backend script back to us, for performing this generic cleanup
> routine.
>
> Another advantage is that git-rebase.sh can implement a generic
> finish_rebase() to possibly do additional tasks in addition to the
> cleanup.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-rebase.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2c692c3..84dc7b0 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -150,6 +150,13 @@ run_specific_rebase () {
>                 autosquash=
>         fi
>         . git-rebase--$type
> +       ret=$?
> +       if test $ret = 0

For numeric comparison, use '-eq' rather than '=', which is for strings.

> +       then
> +               git gc --auto &&
> +               rm -rf "$state_dir"
> +       fi
> +       exit $ret
>  }
>
>  run_pre_rebase_hook () {

Re: [PATCH 8/8] rebase: implement --[no-]autostash and rebase.autostash

[Eric Sunshine]

On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> new file mode 100755
> index 0000000..8386998
> --- /dev/null
> +++ b/t/t3420-rebase-autostash.sh
> @@ -0,0 +1,148 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2013 Ramkumar Ramachandra
> +#
> +
> +test_description='git rebase --autostash tests'
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +       echo hello-world >file0 &&
> +       git add . &&
> +       test_tick &&
> +       git commit -m "initial commit" &&
> +       git checkout -b feature-branch &&
> +       echo another-hello >file1 &&
> +       echo goodbye >file2 &&
> +       git add . &&
> +       test_tick &&
> +       git commit -m "second commit" &&
> +       echo final-goodbye >file3 &&
> +       git add . &&
> +       test_tick &&
> +       git commit -m "third commit"

Broken &&-chain.

> +       git checkout -b unrelated-onto-branch master &&
> +       echo unrelated >file4 &&
> +       git add . &&
> +       test_tick &&
> +       git commit -m "unrelated commit"

Broken &&-chain.

> +       git checkout -b related-onto-branch master &&
> +       echo conflicting-change >file2 &&
> +       git add . &&
> +       test_tick &&
> +       git commit -m "related commit"
> +'
> +
> +testrebase() {
> +       type=$1
> +       dotest=$2
> +
> +       test_expect_success "rebase$type: dirty worktree, non-conflicting rebase" '
> +               test_config rebase.autostash true &&
> +               git reset --hard &&
> +               git checkout -b rebased-feature-branch feature-branch &&
> +               test_when_finished git branch -D rebased-feature-branch &&
> +               echo dirty >>file3 &&
> +               git rebase$type unrelated-onto-branch &&
> +               grep unrelated file4 &&
> +               grep dirty file3 &&
> +               git checkout feature-branch
> +       '
> +
> +       test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
> +               test_config rebase.autostash true &&
> +               git reset --hard &&
> +               git checkout -b rebased-feature-branch feature-branch &&
> +               test_when_finished git branch -D rebased-feature-branch

Broken &&-chain.

> +               echo dirty >>file3 &&
> +               git add file3 &&
> +               git rebase$type unrelated-onto-branch &&
> +               grep unrelated file4 &&
> +               grep dirty file3 &&
> +               git checkout feature-branch
> +       '
> +
> +       test_expect_success "rebase$type: conflicting rebase" '
> +               test_config rebase.autostash true &&
> +               git reset --hard &&
> +               git checkout -b rebased-feature-branch feature-branch &&
> +               test_when_finished git branch -D rebased-feature-branch &&
> +               echo dirty >>file3 &&
> +               test_must_fail git rebase$type related-onto-branch &&
> +               test_path_is_file $dotest/autostash &&
> +               ! grep dirty file3 &&
> +               rm -rf $dotest &&
> +               git reset --hard &&
> +               git checkout feature-branch
> +       '
> +
> +       test_expect_success "rebase$type: --continue" '
> +               test_config rebase.autostash true &&
> +               git reset --hard &&
> +               git checkout -b rebased-feature-branch feature-branch &&
> +               test_when_finished git branch -D rebased-feature-branch &&
> +               echo dirty >>file3 &&
> +               test_must_fail git rebase$type related-onto-branch &&
> +               test_path_is_file $dotest/autostash &&
> +               ! grep dirty file3 &&
> +               echo "conflicting-plus-goodbye" >file2 &&
> +               git add file2 &&
> +               git rebase --continue &&
> +               test_path_is_missing $dotest/autostash &&
> +               grep dirty file3 &&
> +               git checkout feature-branch
> +       '
> +
> +       test_expect_success "rebase$type: --skip" '
> +               test_config rebase.autostash true &&
> +               git reset --hard &&
> +               git checkout -b rebased-feature-branch feature-branch &&
> +               test_when_finished git branch -D rebased-feature-branch &&
> +               echo dirty >>file3 &&
> +               test_must_fail git rebase$type related-onto-branch &&
> +               test_path_is_file $dotest/autostash &&
> +               ! grep dirty file3 &&
> +               git rebase --skip &&
> +               test_path_is_missing $dotest/autostash &&
> +               grep dirty file3 &&
> +               git checkout feature-branch
> +       '
> +
> +       test_expect_success "rebase$type: --abort" '
> +               test_config rebase.autostash true &&
> +               git reset --hard &&
> +               git checkout -b rebased-feature-branch feature-branch &&
> +               test_when_finished git branch -D rebased-feature-branch &&
> +               echo dirty >>file3 &&
> +               test_must_fail git rebase$type related-onto-branch &&
> +               test_path_is_file $dotest/autostash &&
> +               ! grep dirty file3 &&
> +               git rebase --abort &&
> +               test_path_is_missing $dotest/autostash &&
> +               grep dirty file3 &&
> +               git checkout feature-branch
> +       '
> +
> +       test_expect_success "rebase$type: non-conflicting rebase, conflicting stash" '
> +               test_config rebase.autostash true &&
> +               git reset --hard &&
> +               git checkout -b rebased-feature-branch feature-branch &&
> +               test_when_finished git branch -D rebased-feature-branch &&
> +               echo dirty >file4 &&
> +               git add file4 &&
> +               git rebase$type unrelated-onto-branch &&
> +               test_path_is_missing $dotest &&
> +               git reset --hard &&
> +               grep unrelated file4 &&
> +               ! grep dirty file4 &&
> +               git checkout feature-branch &&
> +               git stash pop &&
> +               grep dirty file4
> +       '
> +}
> +
> +testrebase "" .git/rebase-apply
> +testrebase " --merge" .git/rebase-merge
> +testrebase " --interactive" .git/rebase-merge
> +
> +test_done

Re: [PATCH 1/8] am: suppress error output from a conditional

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> In preparation for a later patch that creates $dotest/autostash in
> git-rebase.sh before anything else happens, don't assume that the
> presence of a $dotest directory implies the existence of the $next and
> $last files.  The check for the files is in a conditional anyway, but
> `cat` is executed on potentially non-existent files.  Suppress this
> error output.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-am.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index c092855..88aa438 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -446,8 +446,8 @@ done
>  # If the dotest directory exists, but we have finished applying all the
>  # patches in them, clear it out.
>  if test -d "$dotest" &&
> -   last=$(cat "$dotest/last") &&
> -   next=$(cat "$dotest/next") &&
> +   last=$(cat "$dotest/last" 2>/dev/null) &&
> +   next=$(cat "$dotest/next" 2>/dev/null) &&
>     test $# != 0 &&
>     test "$next" -gt "$last"
>  then

If you are introducing "dotest exists but next/last may not be
present" as a valid new state, it probably should check the presence
and/or absence of them explicitly, but more importantly, it is a
good idea to move "test $# != 0" higher. Earlier it did not matter
because when $dotest existed, next/last were supposed to be there
and absence of them was an error codepath. Now, missing these files
is not an error but is a perfectly normal state, so checking what
can be checked more cheaply makes sense as a general code hygiene.

As you may already know, I am not taking a patch that is not meant
for 'master' to fix regressions in 1.8.3 at this point in the cycle
after -rc2; please hold onto this and other patches as they won't
stay in my mailbox.

Re: [PATCH 4/8] rebase: prepare to do generic housekeeping

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> On successful completion of a rebase in git-rebase--$backend.sh, the
>> $backend script cleans up on its own and exits.  The cleanup routine
>> is however, independent of the $backend, and each $backend script
>> unnecessarily duplicates this work:
>>
>>     rm -rf "$state_dir"
>>     git gc --auto
>>
>> Prepare git-rebase.sh for later patches that return control from each
>> $backend script back to us, for performing this generic cleanup
>> routine.
>>
>> Another advantage is that git-rebase.sh can implement a generic
>> finish_rebase() to possibly do additional tasks in addition to the
>> cleanup.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>>  git-rebase.sh | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index 2c692c3..84dc7b0 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -150,6 +150,13 @@ run_specific_rebase () {
>>                 autosquash=
>>         fi
>>         . git-rebase--$type
>> +       ret=$?
>> +       if test $ret = 0
>
> For numeric comparison, use '-eq' rather than '=', which is for strings.

Do not listen to this.

We know the condition we want is to have $?  with a value that
stringifies to "0" and the above reads much easier.

>
>> +       then
>> +               git gc --auto &&
>> +               rm -rf "$state_dir"
>> +       fi
>> +       exit $ret
>>  }
>>
>>  run_pre_rebase_hook () {

Re: [PATCH 8/8] rebase: implement --[no-]autostash and rebase.autostash

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +finish_rebase () {
> +	if test -f "$state_dir/autostash"
> +	then
> +		stash_sha1=$(cat "$state_dir/autostash")
> +		if git stash apply $stash_sha1 2>&1 >/dev/null
> +		then
> +			echo "Applied autostash"
> +		else
> +			ref_stash=refs/stash &&
> +			: >>"$GIT_DIR/logs/$ref_stash" &&
> +			git update-ref -m "autostash" $ref_stash $stash_sha1 \
> +				|| die "$(eval_gettext 'Cannot store $stash_sha1')" &&
> +			echo "
> +$(gettext 'Applying autostash resulted in conflicts.
> +...

This is confusing (or confused):

	these && might && fail
            || die &&
        success

It is much easier to read if you just drop && after die and write it
like so:
	
		ref_stash=refs/stash &&
		: >>"$GIT_DIR/logs/$ref_stash" &&
		git update-ref -m "autostash" $ref_stash $stash_sha1 ||
		die "$(eval_gettext 'Cannot store $stash_sha1')"

		echo "... success message comes here..."

Re: [PATCH 4/8] rebase: prepare to do generic housekeeping

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> On successful completion of a rebase in git-rebase--$backend.sh, the
> $backend script cleans up on its own and exits.  The cleanup routine
> is however, independent of the $backend, and each $backend script
> unnecessarily duplicates this work:
>
>     rm -rf "$state_dir"
>     git gc --auto
>
> Prepare git-rebase.sh for later patches that return control from each
> $backend script back to us, for performing this generic cleanup
> routine.
>
> Another advantage is that git-rebase.sh can implement a generic
> finish_rebase() to possibly do additional tasks in addition to the
> cleanup.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-rebase.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2c692c3..84dc7b0 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -150,6 +150,13 @@ run_specific_rebase () {
>  		autosquash=
>  	fi
>  	. git-rebase--$type
> +	ret=$?
> +	if test $ret = 0
> +	then
> +		git gc --auto &&
> +		rm -rf "$state_dir"
> +	fi
> +	exit $ret
>  }

Doesn't this exit look suspicious?  The existing callsites of this
shell function has a lot of code after them (e.g. when "continue"
$action is given, run_specific_rebase is run) but there is no exit
after the call returns, so they may already be expecting the
function not to return, exiting by itself.  But then the last step
of this function in the original code, ". git-rebase--$type", would
be the one that is causing us to exit, no?

So it is either (1) the added code is unreachable and unexercised at
this point in the series, or (2) my analysis above is incorrect and
". git-rebase--$type" does return to let the caller proceed, but
this patch changes the behaviour and breaks the caller.  I think it
is the former but then the organization of the series does not make
sense.

Perhaps this should come a bit later in the series?

At least the log message should mention that this is adding an
unreachable cruft at this step, if the order is to be kept.

Re: [PATCH 4/8] rebase: prepare to do generic housekeeping

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> So it is either (1) the added code is unreachable and unexercised at
> this point in the series, or

Yeah, it's (1).

> Perhaps this should come a bit later in the series?

When exactly?  I picked up on your suggestion to separate out the
preparation-for-$backend-to-return step.  The next three patches
convert each of the $backend scripts to return.

> At least the log message should mention that this is adding an
> unreachable cruft at this step, if the order is to be kept.

Okay, I can update the commit message.

Re: [PATCH 1/8] am: suppress error output from a conditional

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If you are introducing "dotest exists but next/last may not be
> present" as a valid new state, it probably should check the presence
> and/or absence of them explicitly,

Um, a test -f preceding the cat?  Why, when cat implicitly checks
existence anyway?

> but more importantly, it is a
> good idea to move "test $# != 0" higher.

This?

diff --git a/git-am.sh b/git-am.sh
index 47c1021..1e10e31 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -446,9 +446,9 @@ done
 # If the dotest directory exists, but we have finished applying all the
 # patches in them, clear it out.
 if test -d "$dotest" &&
+   test $# != 0 &&
    last=$(cat "$dotest/last" 2>/dev/null) &&
    next=$(cat "$dotest/next" 2>/dev/null) &&
-   test $# != 0 &&
    test "$next" -gt "$last"
 then
    rm -fr "$dotest"

> Earlier it did not matter
> because when $dotest existed, next/last were supposed to be there
> and absence of them was an error codepath. Now, missing these files
> is not an error but is a perfectly normal state,

It's not a "normal state" for the purposes of git-am.sh.  There is no
codepath in the program that depends only on $dotest, but not
next/last.  I would frame this as: "checking for $dotest is not a
sufficient prerequisite anymore; we have to additionally look for
next/last".  To git-am.sh, an empty $dotest or just a
$dotest/autostash is equivalent to no $dotest at all.

> so checking what
> can be checked more cheaply makes sense as a general code hygiene.

Yeah, I agree.  See "am: tighten a conditional that checks for
$dotest", where I get away with just checking $dotest/last (and assume
that $dotest/next is present by extension).  How do I apply your
suggestion to this particular patch?

Re: [PATCH 1/8] am: suppress error output from a conditional

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> If you are introducing "dotest exists but next/last may not be
>> present" as a valid new state, it probably should check the presence
>> and/or absence of them explicitly,
>
> Um, a test -f preceding the cat?  Why, when cat implicitly checks
> existence anyway?

With that logic, 'test -d "$dotest"' on the first line would also be
redundant, as we read from "$dotest/last" and that will catch an
error.

It is more about what is conceptually right.  You need to think what
the condition _means_.

We protect the "remove $dotest directory" with the precondition you
are changing, but what is that precondition really trying to say?
If $dotest does not exist, obviously we do not need to remove it,
but what is the essence of that sereis of tests?

It is what the comment says:

    "We have finished applying all the patches in them"

Earlier, presense of $dotest _must_ have meant that last and next
should exist (otherwise you have a corrupt state and we did want to
see error messages), and the check original did was perfectly fine.

Now, a mere presense of $dotest does _not_ mean "we have finished
applying all the patches", because sometimes you create it without
having anything to put in last or next yet.  That is why it starts
to make sense to do

        if test -d "$dotest" &&
           test -f "$dotest/last" &&
           test -f "$dotest/next" &&
           last=$(cat "$dotest/last") &&
           ...