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

[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.
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                | 38 +++++++++++++++++++++++++++++++++++---
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c67038b..03ad701 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 8412d81..c8fddfe 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,24 @@ move_to_original_branch () {
 	esac
 }
 
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		git stash apply $stash_sha1 2>&1 >/dev/null ||
+		die "
+$(eval_gettext 'Applying autostash resulted in conflicts.
+Either fix the conflicts now, or run
+	git reset --hard
+and apply the stash on your desired branch:
+	git stash apply $stash_sha1
+at any time.')" &&
+		echo "Applied autostash"
+	fi
+	git gc --auto &&
+	rm -rf "$state_dir"
+}
+
 run_specific_rebase () {
 	if [ "$interactive_rebase" = implied ]; then
 		GIT_EDITOR=:
@@ -153,8 +173,7 @@ run_specific_rebase () {
 	ret=$?
 	if test $ret = 0
 	then
-		git gc --auto &&
-		rm -rf "$state_dir"
+		apply_autostash
 	fi
 	exit $ret
 }
@@ -248,6 +267,9 @@ do
 	--stat)
 		diffstat=t
 		;;
+	--autostash)
+		autostash=true
+		;;
 	-v)
 		verbose=t
 		diffstat=t
@@ -348,7 +370,7 @@ abort)
 		;;
 	esac
 	output git reset --hard $orig_head
-	rm -r "$state_dir"
+	apply_autostash
 	exit
 	;;
 edit-todo)
@@ -487,6 +509,16 @@ case "$#" in
 	;;
 esac
 
+if test "$autostash" = true && ! require_clean_work_tree --quiet
+then
+	stash_sha1=$(git stash create) || die "$(gettext "Cannot autostash")" &&
+	mkdir -p "$state_dir" &&
+	echo $stash_sha1 >"$state_dir/autostash" &&
+	stash_abbrev=$(git rev-parse --short $stash_sha1) &&
+	echo "$(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,
-- 
1.8.2.1.578.ga933817

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +apply_autostash () {
> +	if test -f "$state_dir/autostash"
> +	then
> +		stash_sha1=$(cat "$state_dir/autostash")
> +		git stash apply $stash_sha1 2>&1 >/dev/null ||
> +		die "
> +$(eval_gettext 'Applying autostash resulted in conflicts.
> +Either fix the conflicts now, or run
> +	git reset --hard
> +and apply the stash on your desired branch:
> +	git stash apply $stash_sha1
> +at any time.')" &&
> +		echo "Applied autostash"

That && looks funny.  What does it even mean for die to succeed and
give control back to you for a chance to say "Applied"?

	stash apply || die
	echo applied

would be far more logical.

> +	fi
> +	git gc --auto &&
> +	rm -rf "$state_dir"
> +}

This is somewhat worrysome.  After getting the "oops, we cannot
apply" message, the "$stash_sha1" in the message on the terminal is
the ONLY indication of where the (presumably precious) local change
the user used to have can be recovered from.

You do not "rm -fr $state_dir" in such a case, so perhaps telling
the user the location of that "autostash" file may help her
somewhat.

For that matter, wouldn't it be a lot simpler to put the autostash
ref somewhere in refs/ hierarchy, instead of storing an object name
of the stash that keeps (presumably precious) local change of the
user in a plain-text file that is not at all known by "gc"?

> +
>  run_specific_rebase () {
>  	if [ "$interactive_rebase" = implied ]; then
>  		GIT_EDITOR=:
> @@ -153,8 +173,7 @@ run_specific_rebase () {
>  	ret=$?
>  	if test $ret = 0
>  	then
> -		git gc --auto &&
> -		rm -rf "$state_dir"
> +		apply_autostash
>  	fi
>  	exit $ret
>  }
> @@ -248,6 +267,9 @@ do
>  	--stat)
>  		diffstat=t
>  		;;
> +	--autostash)
> +		autostash=true
> +		;;
>  	-v)
>  		verbose=t
>  		diffstat=t
> @@ -348,7 +370,7 @@ abort)
>  		;;
>  	esac
>  	output git reset --hard $orig_head
> -	rm -r "$state_dir"
> +	apply_autostash
>  	exit
>  	;;
>  edit-todo)
> @@ -487,6 +509,16 @@ case "$#" in
>  	;;
>  esac
>  
> +if test "$autostash" = true && ! require_clean_work_tree --quiet
> +then
> +	stash_sha1=$(git stash create) || die "$(gettext "Cannot autostash")" &&
> +	mkdir -p "$state_dir" &&
> +	echo $stash_sha1 >"$state_dir/autostash" &&
> +	stash_abbrev=$(git rev-parse --short $stash_sha1) &&
> +	echo "$(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,

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> +apply_autostash () {
>> +	if test -f "$state_dir/autostash"
>> +	then
>> +		stash_sha1=$(cat "$state_dir/autostash")
>> +		git stash apply $stash_sha1 2>&1 >/dev/null ||
>> +		die "
>> +$(eval_gettext 'Applying autostash resulted in conflicts.
>> +Either fix the conflicts now, or run
>> +	git reset --hard
>> +and apply the stash on your desired branch:
>> +	git stash apply $stash_sha1
>> +at any time.')" &&
>> +		echo "Applied autostash"
>
> That && looks funny.  What does it even mean for die to succeed and
> give control back to you for a chance to say "Applied"?
>
> 	stash apply || die
> 	echo applied
>
> would be far more logical.
>
>> +	fi
>> +	git gc --auto &&
>> +	rm -rf "$state_dir"
>> +}
>
> This is somewhat worrisome.

One more thing on this function.  [4/7] (and [5/7]) justified nicely
why it is a good idea to have a central place to do the clean-up
tasks.  apply_autostash is a poor name for that "clean-up" function.
The central clean-up may happen to involve applying a stash in this
version, but applying stash will not stay the entirety of the
clean-up work forever.  The entirety of the clean-up work used to be
just 'git gc --auto && rm -fr "$state_dir"' for eternity, and this
series is adding something else. It is not hard to imagine somebody
else would want to add other kinds of clean-up tasks in here.

Perhaps "finish_rebase" or something?

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Perhaps "finish_rebase" or something?

Sure.  That makes sense.

I was too busy struggling with shell code to notice these things ;)

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

[Phil Hord]

On Tue, Apr 23, 2013 at 10:02 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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                | 38 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c67038b..03ad701 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 8412d81..c8fddfe 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,24 @@ move_to_original_branch () {
>         esac
>  }
>
> +apply_autostash () {
> +       if test -f "$state_dir/autostash"
> +       then
> +               stash_sha1=$(cat "$state_dir/autostash")
> +               git stash apply $stash_sha1 2>&1 >/dev/null ||
> +               die "
> +$(eval_gettext 'Applying autostash resulted in conflicts.
> +Either fix the conflicts now, or run
> +       git reset --hard
> +and apply the stash on your desired branch:
> +       git stash apply $stash_sha1
> +at any time.')" &&
> +               echo "Applied autostash"
> +       fi
> +       git gc --auto &&
> +       rm -rf "$state_dir"
> +}

Because I am in a git-rebase which has apparently failed, I would
expect 'git rebase --abort' would save me here.  But it does not and
you have given me some unique instructions to try to recover.  I
suppose rebase--abort cannot be made to recover in this case because
this is a rebase-wrapper and all of my rebase-state is already
discarded.  But I would much prefer to have the normal "undo"-ability
of git-rebase here, once I realize I have made a mistake or
encountered conflicts I am not prepared to handle right now.


> +
>  run_specific_rebase () {
>         if [ "$interactive_rebase" = implied ]; then
>                 GIT_EDITOR=:
> @@ -153,8 +173,7 @@ run_specific_rebase () {
>         ret=$?
>         if test $ret = 0
>         then
> -               git gc --auto &&
> -               rm -rf "$state_dir"
> +               apply_autostash
>         fi
>         exit $ret
>  }
> @@ -248,6 +267,9 @@ do
>         --stat)
>                 diffstat=t
>                 ;;
> +       --autostash)
> +               autostash=true
> +               ;;
>         -v)
>                 verbose=t
>                 diffstat=t
> @@ -348,7 +370,7 @@ abort)
>                 ;;
>         esac
>         output git reset --hard $orig_head
> -       rm -r "$state_dir"
> +       apply_autostash
>         exit
>         ;;
>  edit-todo)
> @@ -487,6 +509,16 @@ case "$#" in
>         ;;
>  esac
>
> +if test "$autostash" = true && ! require_clean_work_tree --quiet
> +then
> +       stash_sha1=$(git stash create) || die "$(gettext "Cannot autostash")" &&
> +       mkdir -p "$state_dir" &&
> +       echo $stash_sha1 >"$state_dir/autostash" &&
> +       stash_abbrev=$(git rev-parse --short $stash_sha1) &&
> +       echo "$(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,
> --
> 1.8.2.1.578.ga933817
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ramkumar Ramachandra]

Phil Hord wrote:
> Because I am in a git-rebase which has apparently failed, I would
> expect 'git rebase --abort' would save me here.  But it does not and
> you have given me some unique instructions to try to recover.  I
> suppose rebase--abort cannot be made to recover in this case because
> this is a rebase-wrapper and all of my rebase-state is already
> discarded.  But I would much prefer to have the normal "undo"-ability
> of git-rebase here, once I realize I have made a mistake or
> encountered conflicts I am not prepared to handle right now.

You're asking for a hammer solution, when I'm advocating a solution
that offers more flexibility and control.  Commits and worktree
changes are fundamentally two different things, and I treat them
differently.

rebase.autostash is simply a shortcut for:

    $ git stash && git rebase ... && git stash pop

Except that your stash is not blocked during the rebase process: we
use a special stash.  If the last 'git stash pop' fails, do you do
this?

    $ git reset --hard HEAD@{1}
    $ git stash pop
    # snip, snip ...
    # redo the entire rebase

I _never_ find myself doing this; in your hammer solution, you're
advocating that we always do this.

The stash is a powerful tool when used properly: a stash isn't
attached to any branch, and therefore perfectly designed to keep small
temporary worktree changes for a short period of time.
rebase.autostash is _not_ a way to take away power from the user, or
save the user from learning how to use stash.

That said, the current implementation is very rough and I will improve
it in the next iterations.  If the apply fails, I will push the
changes onto stash@{0}, and let the user do a 'git stash pop' instead
of having to remember (or copy out) a SHA-1 displayed in the terminal
output.  Essentially, this leaves the user in the exact same state as
if she had done a 'git stash && git rebase ... && git stash pop'.

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

[Matthieu Moy]

Phil Hord <phil.hord@gmail.com> writes:

> Because I am in a git-rebase which has apparently failed, I would
> expect 'git rebase --abort' would save me here.  

More generally, if I "git rebase --abort" in the middle of a rebase (not
necessarily at the end), I'd expect the stash to be restored. Right now,
if I read correctly, "git rebase --abort" would discard the stashed
changes without giving me the sha1 I'd need to use to recover it :-(.

Recovering the stash should be doable with stg like

--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -341,6 +341,11 @@ abort)
                ;;
        esac
        output git reset --hard $orig_head
+       if test -f "$state_dir/autostash"
+       then
+           stash_sha1=$(cat "$state_dir/autostash")
+           git stash apply $stash_sha1
+       fi
        rm -r "$state_dir"
        exit
        ;;

the "stash apply" should succeed without conflict by construction
because the stash is applied on the commit it was created.

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

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

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

[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

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