[PATCH v3 0/7] rebase.autostash completed

[PATCH v3 0/7] rebase.autostash completed

[Ramkumar Ramachandra]

Minor changes since v2 in response to reviews by Junio and Eric
Sunshine.

Should be ready to merge soon.

Ramkumar Ramachandra (7):
  am: tighten a conditional that checks for $dotest
  rebase -i: don't error out if $state_dir already exists
  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                    |  13 +++-
 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(+), 16 deletions(-)
 create mode 100755 t/t3420-rebase-autostash.sh

-- 
1.8.3.rc1.51.gd7a04de

[PATCH 1/7] am: tighten a conditional that checks for $dotest

[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
$dotest/next and $dotest/last files.  Look for them explicitly.

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

diff --git a/git-am.sh b/git-am.sh
index c092855..ccb854a 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -446,6 +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" &&
+   test -f "$dotest/last" &&
+   test -f "$dotest/next" &&
    last=$(cat "$dotest/last") &&
    next=$(cat "$dotest/next") &&
    test $# != 0 &&
@@ -454,7 +456,7 @@ then
    rm -fr "$dotest"
 fi
 
-if test -d "$dotest"
+if test -d "$dotest" && test -f "$dotest/last" && test -f "$dotest/next"
 then
 	case "$#,$skip$resolved$abort" in
 	0,*t*)
-- 
1.8.3.rc1.51.gd7a04de

[PATCH 2/7] 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.51.gd7a04de

[PATCH 3/7] 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.  The code that this patch adds is currently unreachable, and
will only start to be used when git-rebase--$backend.sh scripts are
taught to return control in later patches.

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..f8b533d 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 -eq 0
+	then
+		git gc --auto &&
+		rm -rf "$state_dir"
+	fi
+	exit $ret
 }
 
 run_pre_rebase_hook () {
-- 
1.8.3.rc1.51.gd7a04de

[PATCH 4/7] 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 ccb854a..1cf3d1d 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -906,5 +906,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.51.gd7a04de

[PATCH 5/7] 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.51.gd7a04de

[PATCH 6/7] 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.51.gd7a04de

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

[Ramkumar Ramachandra]

This new feature allows a rebase to be executed on a dirty worktree or
index.  It works by creating a temporary "dangling merge commit" out
of the worktree and index changes (via 'git stash create'), and
automatically applying it after a successful rebase or abort.

rebase stores the SHA-1 hex of the temporary merge commit, along with
the rest of the rebase state, in either
.git/{rebase-merge,rebase-apply}/autostash depending on the kind of
rebase.  Since $state_dir is automatically removed at the end of a
successful rebase or abort, so is the autostash.

The advantage of this approach is that we do not affect the normal
stash's reflogs, making the autostash invisible to the end-user.  This
means that you can use 'git stash' during a rebase as usual.

When the autostash application results in a conflict, we push
$state_dir/autostash onto the normal stash and remove $state_dir
ending the rebase.  The user can inspect the stash, and pop or drop at
any time.

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 f8b533d..709ef6b 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 the stash.
+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 -eq 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..479cbb2
--- /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.51.gd7a04de

Re: [PATCH 7/7] 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')"

Writing it like this:

			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')"

with a blank line before the next "echo", it would be more readable.

As I said in a separate message, having a code that knows where
"stash" is and how it is organized outside the implementation of
"git stash" is less than ideal.  It probably makes more sense to
let programs like this to say:

			git stash store -m "autostash" $stash_sha1 || exit


> +			echo "
> +$(gettext 'Applying autostash resulted in conflicts.
> +Your changes are safe in the stash.
> +You can apply or drop it at any time.')"

This feels funny.  Why not

			gettext "$msg"

without an extra command substitution with an echo?

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Writing it like this:
> [...]
> with a blank line before the next "echo", it would be more readable.

> This feels funny.  Why not
> [...]
> without an extra command substitution with an echo?

I'll re-roll if there are more comments.  Otherwise, can you fix these
up locally?  Thanks.

>                         git stash store -m "autostash" $stash_sha1 || exit

Definitely nicer.  I'll write follow-ups for pull and stash.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Writing it like this:
>> [...]
>> with a blank line before the next "echo", it would be more readable.
>
>> This feels funny.  Why not
>> [...]
>> without an extra command substitution with an echo?
>
> I'll re-roll if there are more comments.  Otherwise, can you fix these
> up locally?  Thanks.

No, thanks.  I won't be even taking the patch right now, so you have
plenty of time ;-).

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> No, thanks.  I won't be even taking the patch right now, so you have
> plenty of time ;-).

There were some additional comments from Matthieu, so I will re-roll
(just this part).  I've even posted a stash series based on this one.
Do you have any additional comments?

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

[Matthieu Moy]

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"

Any reason why this is not using gettext and the other messages do.

> +		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 the stash.
> +You can apply or drop it at any time.')"

Good idea to put the autostash in an actual stash. Perhaps the message
can be made more explicit, e.g. 

  You can run "git stash apply" or "git stash drop" at any time.

(actually, "git stash pop" may be a better suggestion to "git stash
apply" here).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> Any reason why this is not using gettext and the other messages do.
>   You can run "git stash apply" or "git stash drop" at any time.

Fixed.  Thanks.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Matthieu Moy wrote:
>> Any reason why this is not using gettext and the other messages do.
>>   You can run "git stash apply" or "git stash drop" at any time.
>
> Fixed.  Thanks.

After thinking a bit, I have another nit about the message: it's not
clear whether it's saying "your changes are in the work tree, but with
conflicts so you may want to retry later with "git stash pop"", or "I
could not apply your changes at all, do it yourself with "git stash
pop"".

I suppose it's the former, but it could be more explicit.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3 0/7] rebase.autostash completed

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Minor changes since v2 in response to reviews by Junio and Eric
> Sunshine.
>
> Should be ready to merge soon.

That, and the "completed" on the Subject line are not for you to
decide, I would have to say ;-)

Overall the patches look cleanly done, but I did not carefully look
into the implementation.  Especially I did not check if there are
still undesirable data loss behaviour in corner cases that people
were worried about in the discussion.  It would have been nice to CC
people who voiced concerns during the previous reviews.

A few comments on possible follow-up work, not meant as a suggestion
to include in this series:

 * The series shows that it is necessary to first prepare a stash
   entry with "stash create", then later decide to queue it to the
   stash (or decide not to do so) in some applications.

   In the longer term, it is unmaintainable to make such users (like
   this new code) of the stash mechanism manually do so, and we may
   want to add a "git stash __store" subcommand, not meant for the
   interactive use of end users.  The implementation of the stash
   can later be changed without affecting such users by doing so.

 * The primary reason why you wanted this autostash was because
   "rebase" is implemented in a lazy and stupid way, compared to
   "merge" that detects possible conflicts with local changes and
   refrains from touching the working tree at all, and any local
   change that do not interfere with the merge are permitted.

   Perhaps "rebase" can be taught to be more careful when checking
   if local changes may overlap with the changes being replayed.
   When replaying a range A..B on top of the onto commit O, perhaps
   "rebase" can at least do these:

    - If the index is dirty with respect to HEAD, stop just like
      "merge" does.

    - Take a union of paths different between O and the working
      tree, and untracked new paths in the working tree.  These are
      the possible "local changes".  If there is no way the replay
      of A..B touches any of these paths, we do not even have to
      worry about stashing.

    - Take output from "diff-tree -m --name-only" for each commit in
      the replayed range (note that I am not adding -M to take
      maximal coverage).  Check if any of the paths overlap with the
      "local changes", and if so, stop.

   to be par with "merge". 

When the latter is done, there is _no_ justification for autostash
to be an option specific fo "rebase.  At that point, "rebase" and
"merge" would refuse to start in the same situation, i.e. the local
changes you have are known to interfere with the integration with
the branch, diminishing the need to "autostash" greatly.

When autostash is still needed to proceed after such change to
"rebase", I suspect that the user may be better off saving such
work-in-progress in a temporary commit (possibly after making it
into a better shape before even starting to "rebase" or "merge")
than using a transient mechanism like stash to save such a work,
because it is likely that such local changes have a real conflict
with the work being integrated.

Those who still want to use stash would benefit from having an
autostash, but at that point, there is no reason to single out
"rebase" for the autostash feature.  Those who want to stash
immediately before a "merge" that is known to conflict can use the
same autostash and may want to do so for exactly the same reason
they may want to use it for "rebase".

Re: [PATCH v3 0/7] rebase.autostash completed

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Especially I did not check if there are
> still undesirable data loss behaviour in corner cases that people
> were worried about in the discussion.

Check the tests.  What am I missing?

>    In the longer term, it is unmaintainable to make such users (like
>    this new code) of the stash mechanism manually do so, and we may
>    want to add a "git stash __store" subcommand, not meant for the
>    interactive use of end users.  The implementation of the stash
>    can later be changed without affecting such users by doing so.

Yes, a "store" that stores a commit created with "create" would be
nice.  Why the horrible double-underscore though?  "git stash create"
is not meant for interactive use of end users either.

>    Perhaps "rebase" can be taught to be more careful when checking
>    if local changes may overlap with the changes being replayed.

Frankly, I don't know if it's worth the effort.  It might be a nice
theoretical exercise, but what tangible benefit do I get as the end
user (now that I have rebase.autostash)?  In fact, I'll probably be
slowing down the interactive rebase noticeably by executing a
diff-tree at every step.  And for what?

> Those who still want to use stash would benefit from having an
> autostash, but at that point, there is no reason to single out
> "rebase" for the autostash feature.  Those who want to stash
> immediately before a "merge" that is known to conflict can use the
> same autostash and may want to do so for exactly the same reason
> they may want to use it for "rebase".

Each command that wants to have autostash will have to implement it
independently.  You argued that an implicit merge.autostash may be
harmful earlier: maybe an explicit --autostash; but at this point, the
returns are diminishing: what is the great advantage of --autostash
over a quick manual g ss, g sp (git stash save, git stash pop)?  I
don't know what problem you're trying to solve anymore.

Re: [PATCH v3 0/7] rebase.autostash completed

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Especially I did not check if there are
>> still undesirable data loss behaviour in corner cases that people
>> were worried about in the discussion.
>
> Check the tests.  What am I missing?

I didn't do a thorough check, but my earlier comments are taken into
account, I didn't see anything wrong and the tests in 7/7 are good.

>>    Perhaps "rebase" can be taught to be more careful when checking
>>    if local changes may overlap with the changes being replayed.
>
> Frankly, I don't know if it's worth the effort.  It might be a nice
> theoretical exercise, but what tangible benefit do I get as the end
> user (now that I have rebase.autostash)?  In fact, I'll probably be
> slowing down the interactive rebase noticeably by executing a
> diff-tree at every step.  And for what?

One benefit would be to avoid triggering rebuild (and editor reload) by
keeping the timestamps intact. But I agree it's probably not worth the
effort (and definitely isn't in the scope of this series).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3 0/7] rebase.autostash completed

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> One benefit would be to avoid triggering rebuild (and editor reload) by
> keeping the timestamps intact. But I agree it's probably not worth the
> effort (and definitely isn't in the scope of this series).

It isn't in the scope, of course.  If rebase were done right, the
series will become much less necessary in the first place ;-).

Re: [PATCH v3 0/7] rebase.autostash completed

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Especially I did not check if there are
>> still undesirable data loss behaviour in corner cases that people
>> were worried about in the discussion.
>
> Check the tests.  What am I missing?
>
>>    In the longer term, it is unmaintainable to make such users (like
>>    this new code) of the stash mechanism manually do so, and we may
>>    want to add a "git stash __store" subcommand, not meant for the
>>    interactive use of end users.  The implementation of the stash
>>    can later be changed without affecting such users by doing so.
>
> Yes, a "store" that stores a commit created with "create" would be
> nice.  Why the horrible double-underscore though?  "git stash create"
> is not meant for interactive use of end users either.

"create" is not advertised very widely, but "store" is too close to
what is already familiar to the people "save" and we really do not
want to confuse them.  "store -m message commit" sounds as if you
are creating a stash to apply to the given $commit.

Re: [PATCH v3 0/7] rebase.autostash completed

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> "create" is not advertised very widely, but "store" is too close to
> what is already familiar to the people "save" and we really do not
> want to confuse them.  "store -m message commit" sounds as if you
> are creating a stash to apply to the given $commit.

In the store series I posted a few minutes ago, I use the format
"store <commit> <message>".  I've not advertised it in the usage (like
create), and documented it just like create.  Maybe we should add the
line "Not for end user interactive use" to both descriptions?  We can
get that double-underscore if you really want, but it'd stick out like
a sore thumb since we can't change create.

Re: [PATCH v3 0/7] rebase.autostash completed

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Maybe we should add the
> line "Not for end user interactive use" to both descriptions?  We can
> get that double-underscore if you really want, but it'd stick out like
> a sore thumb since we can't change create.

Yeah, good point about the __name.

It was a mistake that the description of 'create' does not have
"This is probably not what you want to use; see 'save'" there.
Let's do that, and do the same for 'store' (that is, not advertise
in 'git stash -h' output, describe in the manual for scripters, and
mark it not for the end user in the description).