threads / patch / 33771

v2, 8 partsrebase.autostash completed

Subject: [PATCH v2 0/8] rebase.autostash completed

## tl;dr

18 messages between May 10, 2013 and May 12, 2013. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · lore
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
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to git-am.sh +2 −2
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
Junio C Hamano· May 10, 2013, 14:46 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 27 quoted lines
> 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.

Ramkumar Ramachandra· May 11, 2013, 23:38 UTC · re: Junio C Hamano · lore

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

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?
Show changes to git-am.sh +1 −1
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?
Junio C Hamano· May 12, 2013, 03:09 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 7 quoted lines
> 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") &&
           ...
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to git-rebase--interactive.sh +1 −1
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
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to git-am.sh +1 −1
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
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(+)
Show changes to git-rebase.sh +7 −0
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
Eric Sunshine· May 10, 2013, 14:33 UTC · re: Ramkumar Ramachandra · lore

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

On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 31 quoted lines
> 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.
Show 8 quoted lines
> +       then
> +               git gc --auto &&
> +               rm -rf "$state_dir"
> +       fi
> +       exit $ret
>  }
>
>  run_pre_rebase_hook () {
Junio C Hamano· May 10, 2013, 15:18 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 35 quoted lines
> 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.

Show 9 quoted lines
>
>> +       then
>> +               git gc --auto &&
>> +               rm -rf "$state_dir"
>> +       fi
>> +       exit $ret
>>  }
>>
>>  run_pre_rebase_hook () {
Junio C Hamano· May 10, 2013, 15:32 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 37 quoted lines
> 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.

Ramkumar Ramachandra· May 10, 2013, 16:39 UTC · re: Junio C Hamano · lore

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

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.
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +11 −6

git-am.sh, git-rebase--am.sh

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
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to git-rebase--interactive.sh +5 −4
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
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to git-rebase--merge.sh +2 −3
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
Ramkumar Ramachandra· May 10, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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
Show changes to 4 files +206 −3

Documentation/config.txt, Documentation/git-rebase.txt, git-rebase.sh, 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
Eric Sunshine· May 10, 2013, 14:41 UTC · re: Ramkumar Ramachandra · lore

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

On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 29 quoted lines
> 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.
Show 5 quoted lines
> +       git checkout -b unrelated-onto-branch master &&
> +       echo unrelated >file4 &&
> +       git add . &&
> +       test_tick &&
> +       git commit -m "unrelated commit"
Broken &&-chain.
Show 28 quoted lines
> +       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.
Show 92 quoted lines
> +               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
Junio C Hamano· May 10, 2013, 15:29 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 15 quoted lines
> +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..."

← back to recent threads