threads / patch / 33581

patch, 7 partsIntroduce rebase.autostash

Subject: [PATCH 0/7] Introduce rebase.autostash

## tl;dr

28 messages between Apr 23, 2013 and Apr 25, 2013. Diffs are folded; open one to read it.

replies: 27people: 5as markdown or json

Ramkumar Ramachandra· Apr 23, 2013, 14:01 UTC · lore
Yes!

After struggling with shell for a couple of days, I finally managed to achieve what I set out to do: implement a rebase.autostash without leaking the autostash detail into specific rebases. And I'm absolutely elated with my approach. Patches [1/7] to [6/7] make no reference to autostash, and pretend to be nice by-the-way changes unrelated to any new feature. [7/7] suddenly introduces a little bit of code in git-rebase.sh, and everything just starts working magically.

Okay, now for the bad news. I've mostly just worked backward from tests, so I can't justify all my changs fully. Also, there are no new tests, because I don't know what to test: we pretty much have to run the entire rebase testsuite with rebase.autostash turned on, right?

If you can't wait for a few weeks (reviews, iterations) and want to use this immediately, just pull in the rebase.autostash branch from gh:artagnon/git and enjoy.

Cheers.
Ramkumar Ramachandra (7):
  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
  am: don't do housekeeping when rebasing
  rebase -i: return control to the caller, for housekeeping
  sh-setup: introduce require_clean_work_tree --quiet
  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   | 14 ++++++--------
 git-rebase.sh                | 41 ++++++++++++++++++++++++++++++++++++++++-
 git-sh-setup.sh              | 10 +++++++---
 7 files changed, 85 insertions(+), 21 deletions(-)
-- 
1.8.2.1.578.ga933817
Ramkumar Ramachandra· Apr 23, 2013, 14:01 UTC · re: Ramkumar Ramachandra · lore

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

When testing if the $dotest directory exists, and if $next is greater than $last, the script currently executes `cat` on files that might not exist. So, suppress the error output from `cat`.

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.2.1.578.ga933817
Martin von Zweigbergk· Apr 23, 2013, 14:38 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

> When testing if the $dotest directory exists, and if $next is greater
> than $last
When can that happen? If one edits the todo?
Junio C Hamano· Apr 23, 2013, 16:29 UTC · re: Martin von Zweigbergk · lore

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

Martin von Zweigbergk <martinvonz@gmail.com> writes:
Show 6 quoted lines
> On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> When testing if the $dotest directory exists, and if $next is greater
>> than $last
>
> When can that happen? If one edits the todo?

More importantly, that condition is an unexpected error, which the user may need to supply to help diagnosing the issue better, isn't it?

Ramkumar Ramachandra· Apr 23, 2013, 16:31 UTC · re: Martin von Zweigbergk · lore

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

Martin von Zweigbergk wrote:
Show 6 quoted lines
> On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> When testing if the $dotest directory exists, and if $next is greater
>> than $last
>
> When can that happen? If one edits the todo?
When git-rebase.sh creates a $dotest directory with just an autostash file :)

Like I said in my cover letter, I really wrote [7/7] first, and kept modifying things until all the tests passed. So, I can't really justify these changes independent of [7/7].

Junio C Hamano· Apr 23, 2013, 17:29 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 13 quoted lines
> Martin von Zweigbergk wrote:
>> On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra
>> <artagnon@gmail.com> wrote:
>>> When testing if the $dotest directory exists, and if $next is greater
>>> than $last
>>
>> When can that happen? If one edits the todo?
>
> When git-rebase.sh creates a $dotest directory with just an autostash file :)
>
> Like I said in my cover letter, I really wrote [7/7] first, and kept
> modifying things until all the tests passed.  So, I can't really
> justify these changes independent of [7/7].
That explains a lot of mystery ;-)

Perhaps all of these "oops, 7/7 breaks this and that so let's work them around" can be avoided if the series did not store the object name of the stash that records the local changes as a plain text file inside a $dotest/ directory, and instead used a dedicated ref somewhere under refs/ hierarchy, no?

Ramkumar Ramachandra· Apr 23, 2013, 17:31 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 5 quoted lines
> Perhaps all of these "oops, 7/7 breaks this and that so let's work
> them around" can be avoided if the series did not store the object
> name of the stash that records the local changes as a plain text
> file inside a $dotest/ directory, and instead used a dedicated ref
> somewhere under refs/ hierarchy, no?

Exactly. Which is why I was asking for named stashes on the other thread. Much cleaner than keeping track of it yourself.

Ramkumar Ramachandra· Apr 23, 2013, 14:01 UTC · re: Ramkumar Ramachandra · lore

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

Practically speaking, the only reason why a `mkdir $state_dir` would fail is because $state_dir already exists. There is no problem in this case, and we can proceed as usual. So, change the `mkdir` call to `mkdir -p`, and strip the `|| die`.

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 048a140..cc3a9a7 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,7 +837,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"
 
 : > "$state_dir"/interactive || die "Could not mark as interactive"
 write_basic_state
-- 
1.8.2.1.578.ga933817
Martin von Zweigbergk· Apr 23, 2013, 14:45 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

> Practically speaking, the only reason why a `mkdir $state_dir` would
> fail is because $state_dir already exists.
Would we ever get to this point in the code if it already exists?

Also, I had the feeling that the check it might fail if the user does not have permissions to create the directory. Is there always something else that will fail first?

Junio C Hamano· Apr 23, 2013, 16:35 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Practically speaking, the only reason why a `mkdir $state_dir` would
> fail is because $state_dir already exists.  There is no problem in
> this case, and we can proceed as usual.

That was started as a cheap way to detect and avoid running the same rebase -i while another one is already in progress, way back when we did not have a separate "git-rebase--*backend*" scripts, I think.

If we have a separate safety measure to prevent it, lifting it may be safe, but in a sane case, $state_dir should _not_ exist when we start the command, and mkdir should _not_ fail to create it.

So I do not see why this change could be an improvement. I would understand it if some future changes may have already created the state dir from a separate codepath (which may be an indication of a poor design of integrating that separate codepath, but that is a different matter) and mkdir needs to be turned into "mkdir -p" here, but I do not think removal of "|| die" is warranted even in such a case.

Show 21 quoted lines
>  So, change the `mkdir` call
> to `mkdir -p`, and strip the `|| die`.
>
> 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 048a140..cc3a9a7 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -837,7 +837,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"
>  
>  : > "$state_dir"/interactive || die "Could not mark as interactive"
>  write_basic_state
Ramkumar Ramachandra· Apr 23, 2013, 16:38 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 7 quoted lines
> So I do not see why this change could be an improvement.  I would
> understand it if some future changes may have already created the
> state dir from a separate codepath (which may be an indication of a
> poor design of integrating that separate codepath, but that is a
> different matter) and mkdir needs to be turned into "mkdir -p" here,
> but I do not think removal of "|| die" is warranted even in such a
> case.

Yeah, it's [7/7] creating a $state_dir/autostash before anything else can happen. Okay, if I put the "|| die" back in, does it require any more justification?

Ramkumar Ramachandra· Apr 23, 2013, 14:02 UTC · re: Ramkumar Ramachandra · lore

[PATCH 3/7] 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.2.1.578.ga933817
Junio C Hamano· Apr 23, 2013, 16:45 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 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,...

Hmm. that explanation sounds like that is sweeping a real issue under the rug. Shouldn't your patch fixing that creator of the stray one? That stray one that errorneously creates $dotest, even when it is not applying patch messages, may create the directory with 'last' but without 'next' or the messages, so checking only 'last' does not sound like solving any problem.

If this were done as a part of an addition that has to create $dotest even when it does not (yet) deposit patches in there, the way that particular addition creates and uses $dotest may justify why testing only 'last' is sufficient. But as a standalone change, neither the patch text or the above explanation makes much sense to me.

Show 10 quoted lines
> 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*)
Ramkumar Ramachandra· Apr 23, 2013, 14:02 UTC · re: Ramkumar Ramachandra · lore

[PATCH 4/7] am: don't do housekeeping when rebasing

Perform these two tasks:
    git gc --auto
    rm -fr "$dotest"

only when not called with --rebasing (from git-rebase--am.sh). Otherwise, return control to the caller so that it can do the needful. The advantage of doing this is that the caller can implement a generic cleanup routine (that possibly does other things) independent of specific rebases.

Patch the ultimate caller, git-rebase.sh, to perform these two steps for the moment. Later patches will add functionality.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-am.sh         | 9 +++++++--
 git-rebase--am.sh | 8 ++++----
 git-rebase.sh     | 7 +++++++
 3 files changed, 18 insertions(+), 6 deletions(-)
Show changes to 3 files +18 −6

git-am.sh, git-rebase--am.sh, git-rebase.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..8230094 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
diff --git a/git-rebase.sh b/git-rebase.sh
index b2f1c76..8412d81 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.2.1.578.ga933817
Junio C Hamano· Apr 23, 2013, 16:51 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 4/7] am: don't do housekeeping when rebasing

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 10 quoted lines
> Perform these two tasks:
>
>     git gc --auto
>     rm -fr "$dotest"
>
> only when not called with --rebasing (from git-rebase--am.sh).
> Otherwise, return control to the caller so that it can do the needful.
> The advantage of doing this is that the caller can implement a generic
> cleanup routine (that possibly does other things) independent of
> specific rebases.

The above justification and the patch text make a lot of sense, but they make me wonder why there is no corresponding removal from the other git-rebase--*backend*.

Unconditional changes from "exit" to "return $?" looked somewhat suspicious, but this is a sourced script so it should be safe.

Show 77 quoted lines
>  git-am.sh         | 9 +++++++--
>  git-rebase--am.sh | 8 ++++----
>  git-rebase.sh     | 7 +++++++
>  3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index f4ef8fc..47c1021 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -904,5 +904,10 @@ if test -s "$dotest"/rewritten; then
>      fi
>  fi
>  
> -rm -fr "$dotest"
> -git gc --auto
> +# If am was called with --rebasing (from git-rebase--am), it's up to
> +# the caller to take care of housekeeping.
> +if ! test -f "$dotest/rebasing"
> +then
> +	rm -fr "$dotest"
> +	git gc --auto
> +fi
> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index f84854f..8230094 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
> diff --git a/git-rebase.sh b/git-rebase.sh
> index b2f1c76..8412d81 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 () {
Ramkumar Ramachandra· Apr 23, 2013, 14:02 UTC · re: Ramkumar Ramachandra · lore

[PATCH 5/7] rebase -i: return control to the caller, for housekeeping

On a successful interactive rebase, git-rebase--interactive.sh currently cleans up and exits on its own. Instead of doing these two things ourselves:

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

let us return control to the caller (git-rebase.sh), to do the needful. The advantage of doing this is that the caller can implement a generic cleanup routine (and possibly other things) independent of specific rebases.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase--interactive.sh | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
Show changes to git-rebase--interactive.sh +5 −7
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cc3a9a7..9514e31 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -597,7 +597,7 @@ do_next () {
 		fi
 		;;
 	esac
-	test -s "$todo" && return
+	test -s "$todo" && return 1
 
 	comment_for_reflog finish &&
 	newhead=$(git rev-parse HEAD) &&
@@ -623,17 +623,15 @@ 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 0
 }
 
 do_rest () {
 	while :
 	do
-		do_next
+		do_next && break
 	done
 }
 
@@ -799,12 +797,12 @@ first and then run 'git rebase --continue' again."
 	record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
 
 	require_clean_work_tree "rebase"
-	do_rest
+	do_rest && return 0
 	;;
 skip)
 	git rerere clear
 
-	do_rest
+	do_rest && return 0
 	;;
 edit-todo)
 	git stripspace --strip-comments <"$todo" >"$todo".new
-- 
1.8.2.1.578.ga933817
Martin von Zweigbergk· Apr 23, 2013, 16:51 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping

On Tue, Apr 23, 2013 at 7:02 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 10 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cc3a9a7..9514e31 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -597,7 +597,7 @@ do_next () {
>                 fi
>                 ;;
>         esac
> -       test -s "$todo" && return
> +       test -s "$todo" && return 1

Unlike the other cases below, this seems to be replacing success by failure. What does it mean in practice that $todo is empty?

Show 12 quoted lines
>         comment_for_reflog finish &&
>         newhead=$(git rev-parse HEAD) &&
> @@ -623,17 +623,15 @@ 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 0

So after this patch, the "warning" will coming before gc is run. It's a change, but it seems fine. gc usually only prints a few line, right?

Show 9 quoted lines
>  }
>
>  do_rest () {
>         while :
>         do
> -               do_next
> +               do_next && break
>         done
>  }

Normally one would break if unsuccessful. What would fail if this was replaced by "do_next || break" and the above ".. && return 1" was ".. && return". I assume that was your first attempt, but why did it not work?

Ramkumar Ramachandra· Apr 25, 2013, 14:30 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping

Martin von Zweigbergk wrote:
> Normally one would break if unsuccessful. What would fail if this was
> replaced by "do_next || break" and the above ".. && return 1" was "..
> && return". I assume that was your first attempt, but why did it not
> work?

Thanks. This was a major thinko on my part, but I don't remember exactly why I got confused. I'll explain this patch properly in the next iteration.

Junio C Hamano· Apr 23, 2013, 16:57 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 14 quoted lines
> On a successful interactive rebase, git-rebase--interactive.sh
> currently cleans up and exits on its own.  Instead of doing these
> two things ourselves:
>
>     rm -fr "$dotest"
>     git gc --auto
>
> let us return control to the caller (git-rebase.sh), to do the
> needful.  The advantage of doing this is that the caller can implement
> a generic cleanup routine (and possibly other things) independent of
> specific rebases.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

And this answers the question in my review for [4/7]. It would make sense to have these two patch subseries asn three patches (prepare git-rebase.sh, and then convert each backends separately), or a single patch; two patches like this does not make much sense to me.

Show 35 quoted lines
>  git-rebase--interactive.sh | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cc3a9a7..9514e31 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -597,7 +597,7 @@ do_next () {
>  		fi
>  		;;
>  	esac
> -	test -s "$todo" && return
> +	test -s "$todo" && return 1
>  
>  	comment_for_reflog finish &&
>  	newhead=$(git rev-parse HEAD) &&
> @@ -623,17 +623,15 @@ 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 0
>  }
>  
>  do_rest () {
>  	while :
>  	do
> -		do_next
> +		do_next && break
>  	done
>  }

This is somewhat suspicious. We used to die when do_next failed, or let do_next exit with success.

But now you let do_rest return (what does it return???)...
Show 7 quoted lines
>  
> @@ -799,12 +797,12 @@ first and then run 'git rebase --continue' again."
>  	record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
>  
>  	require_clean_work_tree "rebase"
> -	do_rest
> +	do_rest && return 0

... and its caller reports success here only when it succeeds. What happens do_rest returns a failure?

Show 9 quoted lines
>  	;;
>  skip)
>  	git rerere clear
>  
> -	do_rest
> +	do_rest && return 0
>  	;;
>  edit-todo)
>  	git stripspace --strip-comments <"$todo" >"$todo".new
Ramkumar Ramachandra· Apr 23, 2013, 14:02 UTC · re: Ramkumar Ramachandra · lore

[PATCH 6/7] sh-setup: introduce require_clean_work_tree --quiet

Some callers might want to know whether or not the worktree is clean, and require_clean_work_tree() has the logic for this. The current implementation of the function prints a message and exits if the worktree wasn't clean. Introduce a --quiet switch to get it to report the status of the worktree back to the caller.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-sh-setup.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
Show changes to git-sh-setup.sh +7 −3
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 2f78359..5fa22a8 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -172,6 +172,7 @@ require_clean_work_tree () {
 
 	if ! git diff-files --quiet --ignore-submodules
 	then
+		test "$1" = "--quiet" ||
 		echo >&2 "Cannot $1: You have unstaged changes."
 		err=1
 	fi
@@ -180,9 +181,11 @@ require_clean_work_tree () {
 	then
 		if [ $err = 0 ]
 		then
-		    echo >&2 "Cannot $1: Your index contains uncommitted changes."
+			test "$1" = "--quiet" ||
+			echo >&2 "Cannot $1: Your index contains uncommitted changes."
 		else
-		    echo >&2 "Additionally, your index contains uncommitted changes."
+			test "$1" = "--quiet" ||
+			echo >&2 "Additionally, your index contains uncommitted changes."
 		fi
 		err=1
 	fi
@@ -190,8 +193,9 @@ require_clean_work_tree () {
 	if [ $err = 1 ]
 	then
 		test -n "$2" && echo >&2 "$2"
-		exit 1
+		test "$1" = "--quiet" || exit 1
 	fi
+	return $err
 }
 
 # Generate a sed script to parse identities from a commit.
-- 
1.8.2.1.578.ga933817
Junio C Hamano· Apr 23, 2013, 17:00 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 6/7] sh-setup: introduce require_clean_work_tree --quiet

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 5 quoted lines
> Some callers might want to know whether or not the worktree is clean,
> and require_clean_work_tree() has the logic for this.  The current
> implementation of the function prints a message and exits if the
> worktree wasn't clean.  Introduce a --quiet switch to get it to report
> the status of the worktree back to the caller.

This makes 75% sense, but I find it an extremely bad taste to tie exit vs return with --quiet. They are orthogonal and unrelated concepts.

Doing this
	if (require_clean_work_tree) 2>/dev/null
        then
		: happy, the working tree is clean
	else
		... let's stash ...
	fi

without changing anything else may be better than adding such a conflating --quiet option.

Show 42 quoted lines
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-sh-setup.sh | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 2f78359..5fa22a8 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -172,6 +172,7 @@ require_clean_work_tree () {
>  
>  	if ! git diff-files --quiet --ignore-submodules
>  	then
> +		test "$1" = "--quiet" ||
>  		echo >&2 "Cannot $1: You have unstaged changes."
>  		err=1
>  	fi
> @@ -180,9 +181,11 @@ require_clean_work_tree () {
>  	then
>  		if [ $err = 0 ]
>  		then
> -		    echo >&2 "Cannot $1: Your index contains uncommitted changes."
> +			test "$1" = "--quiet" ||
> +			echo >&2 "Cannot $1: Your index contains uncommitted changes."
>  		else
> -		    echo >&2 "Additionally, your index contains uncommitted changes."
> +			test "$1" = "--quiet" ||
> +			echo >&2 "Additionally, your index contains uncommitted changes."
>  		fi
>  		err=1
>  	fi
> @@ -190,8 +193,9 @@ require_clean_work_tree () {
>  	if [ $err = 1 ]
>  	then
>  		test -n "$2" && echo >&2 "$2"
> -		exit 1
> +		test "$1" = "--quiet" || exit 1
>  	fi
> +	return $err
>  }
>  
>  # Generate a sed script to parse identities from a commit.
Ramkumar Ramachandra· Apr 23, 2013, 14:02 UTC · re: Ramkumar Ramachandra · lore

[PATCH 7/7] 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                | 38 +++++++++++++++++++++++++++++++++++---
 3 files changed, 53 insertions(+), 3 deletions(-)
Show changes to 3 files +53 −3

Documentation/config.txt, Documentation/git-rebase.txt, git-rebase.sh

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
Junio C Hamano· Apr 23, 2013, 17:07 UTC · re: Ramkumar Ramachandra · lore

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

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

Show 50 quoted lines
> +
>  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,
Junio C Hamano· Apr 23, 2013, 17:34 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 30 quoted lines
> 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?
Phil Hord· Apr 23, 2013, 22:45 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, Apr 23, 2013 at 10:02 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 110 quoted lines
> 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.

Show 57 quoted lines
> +
>  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
Ramkumar Ramachandra· Apr 24, 2013, 08:27 UTC · re: Phil Hord · lore

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

Phil Hord wrote:
Show 8 quoted lines
> 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'.

Matthieu Moy· Apr 24, 2013, 17:52 UTC · re: Phil Hord · lore

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

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
Show changes to git-rebase.sh +5 −0
--- 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/

← back to recent threads