[PATCH 0/3] Introduce pull.autostash

[PATCH 0/3] Introduce pull.autostash

[Ramkumar Ramachandra]

This series adds a pull.autostash, making 'git pull' a tad bit more
usable.  It is part of my bigger plan to make 'git pull' just DTRT
(via configuration variables) in 90% of the cases.

[1/3] and [2/3] are tiny "while we're here" patches.
[3/3] is the main patch with tests.

Thanks for reading.

Ramkumar Ramachandra (3):
  git-pull.sh: prefer invoking "git <command>" over "git-<command>"
  t5521 (pull-options): use test_commit() where appropriate
  git-pull.sh: introduce --[no-]autostash and pull.autostash

 Documentation/config.txt   |  5 ++++
 Documentation/git-pull.txt |  7 ++++++
 git-pull.sh                | 30 +++++++++++++++++++---
 t/t5521-pull-options.sh    | 63 ++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 99 insertions(+), 6 deletions(-)

-- 
1.8.2.141.gad203c2.dirty

[PATCH 1/3] git-pull.sh: prefer invoking "git <command>" over "git-<command>"

[Ramkumar Ramachandra]

14e5d40c (pull: Fix parsing of -X<option>, 2010-01-17) added the lines
containing "git-push" and "git-merge", even though the prevelant style
at the time was to use the unhyphenated "git <command>" form.  Fix
this.

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

diff --git a/git-pull.sh b/git-pull.sh
index 266e682..37e1cd4 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -279,11 +279,11 @@ fi
 merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 case "$rebase" in
 true)
-	eval="git-rebase $diffstat $strategy_args $merge_args"
+	eval="git rebase $diffstat $strategy_args $merge_args"
 	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
 	;;
 *)
-	eval="git-merge $diffstat $no_commit $edit $squash $no_ff $ff_only"
+	eval="git merge $diffstat $no_commit $edit $squash $no_ff $ff_only"
 	eval="$eval  $log_arg $strategy_args $merge_args $verbosity $progress"
 	eval="$eval \"\$merge_name\" HEAD $merge_head"
 	;;
-- 
1.8.2.141.gad203c2.dirty

[PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate

[Ramkumar Ramachandra]

test_commit() is a well-defined function in test-lib-functions.sh that
allows you to create commits with a terse syntax.  Prefer using it
over creating commits by hand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5521-pull-options.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 1b06691..4a804f0 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -7,8 +7,8 @@ test_description='pull options'
 test_expect_success 'setup' '
 	mkdir parent &&
 	(cd parent && git init &&
-	 echo one >file && git add file &&
-	 git commit -m one)
+	 test_commit "one" file "one"
+	)
 '
 
 test_expect_success 'git pull -q' '
-- 
1.8.2.141.gad203c2.dirty

Re: [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> test_commit() is a well-defined function in test-lib-functions.sh that
> allows you to create commits with a terse syntax.  Prefer using it
> over creating commits by hand.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5521-pull-options.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 1b06691..4a804f0 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -7,8 +7,8 @@ test_description='pull options'
>  test_expect_success 'setup' '
>  	mkdir parent &&
>  	(cd parent && git init &&
> -	 echo one >file && git add file &&
> -	 git commit -m one)
> +	 test_commit "one" file "one"
> +	)
>  '

In this test script perhaps it is OK, but I'd prefer people being
careful *not* to use test_commit in tests that involve refs (i.e.
pushing, pulling, ls-remote, for-each-ref, describe...) and paths
(i.e.  ls-files, diff, ...).

There is one good point in the helper: it creates a commit with a
predictable timestamp.

But it does a lot other *bad* things than that single good thing.
It adds a new path, and adds a new tag; neither of which is not
desirable in many circumstances.

A better future direction would be to first make these "frill"
features into options to test_commit helper, fix the users that
depend on these additional tags and stuff to explicitly ask for
them, and then start advocating it for wider use, I think.

Re: [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> test_commit() is a well-defined function in test-lib-functions.sh that
>> allows you to create commits with a terse syntax.  Prefer using it
>> over creating commits by hand.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>>  t/t5521-pull-options.sh | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
>> index 1b06691..4a804f0 100755
>> --- a/t/t5521-pull-options.sh
>> +++ b/t/t5521-pull-options.sh
>> @@ -7,8 +7,8 @@ test_description='pull options'
>>  test_expect_success 'setup' '
>>       mkdir parent &&
>>       (cd parent && git init &&
>> -      echo one >file && git add file &&
>> -      git commit -m one)
>> +      test_commit "one" file "one"
>> +     )
>>  '
>
> In this test script perhaps it is OK, but I'd prefer people being
> careful *not* to use test_commit in tests that involve refs (i.e.
> pushing, pulling, ls-remote, for-each-ref, describe...) and paths
> (i.e.  ls-files, diff, ...).

Okay.

> There is one good point in the helper: it creates a commit with a
> predictable timestamp.

Yes, test_tick.  I've noticed that several tests call test_tick by
hand before invoking "git commit".

> But it does a lot other *bad* things than that single good thing.
> It adds a new path, and adds a new tag; neither of which is not
> desirable in many circumstances.
>
> A better future direction would be to first make these "frill"
> features into options to test_commit helper, fix the users that
> depend on these additional tags and stuff to explicitly ask for
> them, and then start advocating it for wider use, I think.

Agreed.  In fact, the commit message is constrained, because of this;
you can't create a commit with a commit message involving spaces,
because that would result in an invalid tag name.

[PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash

[Ramkumar Ramachandra]

This new configuration variable executes 'git stash' before attempting
to merge/rebase, and 'git stash pop' after a successful merge/rebase.
It makes it convenient for people to pull with dirty worktrees.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt   |  5 ++++
 Documentation/git-pull.txt |  7 ++++++
 git-pull.sh                | 26 ++++++++++++++++++--
 t/t5521-pull-options.sh    | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c1f435f..0becafe 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1786,6 +1786,11 @@ pull.rebase::
 	of merging the default branch from the default remote when "git
 	pull" is run. See "branch.<name>.rebase" for setting this on a
 	per-branch basis.
+
+pull.autostash::
+	When true, automatically stash all changes before attempting to
+	merge/rebase, and pop the stash after a successful
+	merge/rebase.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index c975743..bb57c86 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -94,6 +94,13 @@ must be given before the options meant for 'git fetch'.
 	has to be called afterwards to bring the work tree up to date with the
 	merge result.
 
+--[no-]autostash::
+	When turned on, automatically stash all changes before
+	attempting to merge/rebase, and pop the stash after a
+	successful merge/rebase.  Useful for people who want to pull
+	with a dirty worktree.  This option can also be set via the
+	`pull.autostash` configuration variable.
+
 Options related to merging
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/git-pull.sh b/git-pull.sh
index 37e1cd4..ad8e494 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -48,6 +48,7 @@ if test -z "$rebase"
 then
 	rebase=$(git config --bool pull.rebase)
 fi
+autostash=$(git config --bool pull.autostash)
 dry_run=
 while :
 do
@@ -116,6 +117,12 @@ do
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
 		rebase=false
 		;;
+	--autostash)
+		autostash=true
+		;;
+	--no-autostash)
+		autostash=false
+		;;
 	--recurse-submodules)
 		recurse_submodules=--recurse-submodules
 		;;
@@ -196,7 +203,8 @@ test true = "$rebase" && {
 		then
 			die "$(gettext "updating an unborn branch with changes added to the index")"
 		fi
-	else
+	elif test "$autostash" = false
+	then
 		require_clean_work_tree "pull with rebase" "Please commit or stash them."
 	fi
 	oldremoteref= &&
@@ -213,6 +221,12 @@ test true = "$rebase" && {
 		fi
 	done
 }
+
+stash_required () {
+	! (git diff-files --quiet --ignore-submodules &&
+	git diff-index --quiet --cached HEAD --ignore-submodules)
+}
+
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok "$@" || exit 1
 test -z "$dry_run" || exit 0
@@ -288,4 +302,12 @@ true)
 	eval="$eval \"\$merge_name\" HEAD $merge_head"
 	;;
 esac
-eval "exec $eval"
+
+if test "$autostash" = true && stash_required
+then
+	git stash
+	eval "$eval"
+	test $? = 0 && git stash pop
+else
+	eval "exec $eval"
+fi
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 4a804f0..cecacbc 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -90,4 +90,63 @@ test_expect_success 'git pull --all' '
 	)
 '
 
+test_expect_success 'pull --autostash with clean worktree' '
+	mkdir clonedautostash &&
+	(cd clonedautostash &&
+	git init &&
+	git pull --autostash ../parent &&
+	test_must_fail test_path_is_file .git/refs/stash
+	test_commit one
+	) &&
+	rm -rf clonedautostash
+'
+
+test_expect_success 'pull.autostash with clean worktree' '
+	mkdir clonedautostash &&
+	(cd clonedautostash &&
+	git init &&
+	test_config pull.autostash true &&
+	git pull ../parent &&
+	test_must_fail test_path_is_file .git/refs/stash
+	test_commit one
+	) &&
+	rm -rf clonedautostash
+'
+
+test_expect_success 'pull.autostash without conflict' '
+	mkdir clonedautostash &&
+	(cd clonedautostash &&
+	git init &&
+	test_commit "root_commit" &&
+	cat >quux <<-\EOF &&
+	this is a non-conflicting file
+	EOF
+	git add quux &&
+	test_config pull.autostash true &&
+	git pull ../parent &&
+	test_must_fail test_path_is_file .git/refs/stash &&
+	test_path_is_file quux &&
+	test_commit one
+	) &&
+	rm -rf clonedautostash
+'
+
+test_expect_success 'pull.autostash with conflict' '
+	mkdir clonedautostash &&
+	(cd clonedautostash &&
+	git init &&
+	test_commit "will_conflict" file "this is a conflicting file" &&
+	cat >quux <<-\EOF &&
+	this is a non-conflicting file
+	EOF
+	git add quux &&
+	test_config pull.autostash true &&
+	test_must_fail git pull ../parent &&
+	test_must_fail test_commit one &&
+	test_path_is_file .git/refs/stash &&
+	test_must_fail test_path_is_file quux
+	) &&
+	rm -rf clonedautostash
+'
+
 test_done
-- 
1.8.2.141.gad203c2.dirty

Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> @@ -1786,6 +1786,11 @@ pull.rebase::
>  	of merging the default branch from the default remote when "git
>  	pull" is run. See "branch.<name>.rebase" for setting this on a
>  	per-branch basis.
> +
> +pull.autostash::
> +	When true, automatically stash all changes before attempting to
> +	merge/rebase, and pop the stash after a successful
> +	merge/rebase.
>  +
>  *NOTE*: this is a possibly dangerous operation; do *not* use
>  it unless you understand the implications (see linkgit:git-rebase[1]

Is autosquash a possibly dangerous operation?

> @@ -196,7 +203,8 @@ test true = "$rebase" && {
>  		then
>  			die "$(gettext "updating an unborn branch with changes added to the index")"
>  		fi
> -	else
> +	elif test "$autostash" = false
> +	then
>  		require_clean_work_tree "pull with rebase" "Please commit or stash them."
>  	fi

A safety net, after you run "git stash", to validate that the
added "git stash" indeed made the working tree clean, is necessary
below, but there does not seem to be any.

>  	oldremoteref= &&
> @@ -213,6 +221,12 @@ test true = "$rebase" && {
>  		fi
>  	done
>  }
> +
> +stash_required () {
> +	! (git diff-files --quiet --ignore-submodules &&
> +	git diff-index --quiet --cached HEAD --ignore-submodules)
> +}
> +
>  orig_head=$(git rev-parse -q --verify HEAD)
>  git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok "$@" || exit 1
>  test -z "$dry_run" || exit 0
> @@ -288,4 +302,12 @@ true)
>  	eval="$eval \"\$merge_name\" HEAD $merge_head"
>  	;;
>  esac
> -eval "exec $eval"
> +
> +if test "$autostash" = true && stash_required
> +then
> +	git stash
> +	eval "$eval"
> +	test $? = 0 && git stash pop
> +else
> +	eval "exec $eval"
> +fi

Delaying to run "git stash" as much as possible is fine, but with
the above, can the user tell if something was stashed and she has
to "stash pop" once she is done helping the command to resolve the
conflicts, or she shouldn't run "stash pop" after she is done
(because if nothing is stashed at this point, that "pop" will pop an
unrelated stash)?

What protects the "git stash" from failing and how is its error
handled?

Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> @@ -1786,6 +1786,11 @@ pull.rebase::
>>       of merging the default branch from the default remote when "git
>>       pull" is run. See "branch.<name>.rebase" for setting this on a
>>       per-branch basis.
>> +
>> +pull.autostash::
>> +     When true, automatically stash all changes before attempting to
>> +     merge/rebase, and pop the stash after a successful
>> +     merge/rebase.
>>  +
>>  *NOTE*: this is a possibly dangerous operation; do *not* use
>>  it unless you understand the implications (see linkgit:git-rebase[1]
>
> Is autosquash a possibly dangerous operation?

Oops.  I must admit I was in a bit of a hurry with the documentation.
I was eager to send out the series seeing that the tests were passing.

>> @@ -196,7 +203,8 @@ test true = "$rebase" && {
>>               then
>>                       die "$(gettext "updating an unborn branch with changes added to the index")"
>>               fi
>> -     else
>> +     elif test "$autostash" = false
>> +     then
>>               require_clean_work_tree "pull with rebase" "Please commit or stash them."
>>       fi
>
> A safety net, after you run "git stash", to validate that the
> added "git stash" indeed made the working tree clean, is necessary
> below, but there does not seem to be any.

Um, isn't that part of the "git stash" testsuite?

>>       oldremoteref= &&
>> @@ -213,6 +221,12 @@ test true = "$rebase" && {
>>               fi
>>       done
>>  }
>> +
>> +stash_required () {
>> +     ! (git diff-files --quiet --ignore-submodules &&
>> +     git diff-index --quiet --cached HEAD --ignore-submodules)
>> +}
>> +
>>  orig_head=$(git rev-parse -q --verify HEAD)
>>  git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok "$@" || exit 1
>>  test -z "$dry_run" || exit 0
>> @@ -288,4 +302,12 @@ true)
>>       eval="$eval \"\$merge_name\" HEAD $merge_head"
>>       ;;
>>  esac
>> -eval "exec $eval"
>> +
>> +if test "$autostash" = true && stash_required
>> +then
>> +     git stash
>> +     eval "$eval"
>> +     test $? = 0 && git stash pop
>> +else
>> +     eval "exec $eval"
>> +fi
>
> Delaying to run "git stash" as much as possible is fine, but with
> the above, can the user tell if something was stashed and she has
> to "stash pop" once she is done helping the command to resolve the
> conflicts, or she shouldn't run "stash pop" after she is done
> (because if nothing is stashed at this point, that "pop" will pop an
> unrelated stash)?

I could easily tell, from the "git pull" output: if conflict, then
stash hasn't been applied.  Otherwise, yes.  Should we print a message
guarded by an advice variable?

> What protects the "git stash" from failing and how is its error
> handled?

Oh, this is my stupidity.  I should've just &&-chained the git stash,
eval $eval, and git stash pop.

Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>>> +     elif test "$autostash" = false
>>> +     then
>>>               require_clean_work_tree "pull with rebase" "Please commit or stash them."
>>>       fi
>>
>> A safety net, after you run "git stash", to validate that the
>> added "git stash" indeed made the working tree clean, is necessary
>> below, but there does not seem to be any.
>
> Um, isn't that part of the "git stash" testsuite?

You should always "trust but verify" what other commands do at key
points of the operation; and I think this "require-clean-work-tree"
is a key precondition for this mode of operation to work correctly.

Especially because you do not even bother to check the result of
"git stash" before continuing ;-).

>>> +if test "$autostash" = true && stash_required
>>> +then
>>> +     git stash
>>> +     eval "$eval"
>>> +     test $? = 0 && git stash pop
>>> +else
>>> +     eval "exec $eval"
>>> +fi
>>
>> Delaying to run "git stash" as much as possible is fine, but with
>> the above, can the user tell if something was stashed and she has
>> to "stash pop" once she is done helping the command to resolve the
>> conflicts, or she shouldn't run "stash pop" after she is done
>> (because if nothing is stashed at this point, that "pop" will pop an
>> unrelated stash)?
>
> I could easily tell, from the "git pull" output: if conflict, then
> stash hasn't been applied.

The question was about "git stash save".  Depending on the cleanness
of the tree when "git pull" was started, "save" may or may not have
been done.  After resolving a conflicted "git pull" and committing
the result, the user does not have much clue if she needs to pop
anything, does she?  Instead of the usual "resolve the conflicts and
commit the result", she may need to see "resolve the conflicts,
commit the result, *AND* UNSTASH".

Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>>> +     elif test "$autostash" = false
>>>> +     then
>>>>               require_clean_work_tree "pull with rebase" "Please commit or stash them."
>>>>       fi
>>>
>>> A safety net, after you run "git stash", to validate that the
>>> added "git stash" indeed made the working tree clean, is necessary
>>> below, but there does not seem to be any.
>>
>> Um, isn't that part of the "git stash" testsuite?
>
> You should always "trust but verify" what other commands do at key
> points of the operation; and I think this "require-clean-work-tree"
> is a key precondition for this mode of operation to work correctly.
>
> Especially because you do not even bother to check the result of
> "git stash" before continuing ;-).

If you think it's enough to replicate the codepath that precedes the
actual saving in 'git stash' (which is essentially
require-clean-work-tree), I'm in agreement with you.  I thought you
were implying a wider safety net, that wouldn't even assume the basic
sanity of stash.

>>>> +if test "$autostash" = true && stash_required
>>>> +then
>>>> +     git stash
>>>> +     eval "$eval"
>>>> +     test $? = 0 && git stash pop
>>>> +else
>>>> +     eval "exec $eval"
>>>> +fi
>>>
>>> Delaying to run "git stash" as much as possible is fine, but with
>>> the above, can the user tell if something was stashed and she has
>>> to "stash pop" once she is done helping the command to resolve the
>>> conflicts, or she shouldn't run "stash pop" after she is done
>>> (because if nothing is stashed at this point, that "pop" will pop an
>>> unrelated stash)?
>>
>> I could easily tell, from the "git pull" output: if conflict, then
>> stash hasn't been applied.
>
> The question was about "git stash save".  Depending on the cleanness
> of the tree when "git pull" was started, "save" may or may not have
> been done.  After resolving a conflicted "git pull" and committing
> the result, the user does not have much clue if she needs to pop
> anything, does she?  Instead of the usual "resolve the conflicts and
> commit the result", she may need to see "resolve the conflicts,
> commit the result, *AND* UNSTASH".

Yes, good point.  Will it suffice to print a message?

Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>>>>> +     elif test "$autostash" = false
>>>>> +     then
>>>>>               require_clean_work_tree "pull with rebase" "Please commit or stash them."
>>>>>       fi
>>>>
>>>> A safety net, after you run "git stash", to validate that the
>>>> added "git stash" indeed made the working tree clean, is necessary
>>>> below, but there does not seem to be any.
>>>
>>> Um, isn't that part of the "git stash" testsuite?
>>
>> You should always "trust but verify" what other commands do at key
>> points of the operation; and I think this "require-clean-work-tree"
>> is a key precondition for this mode of operation to work correctly.
>>
>> Especially because you do not even bother to check the result of
>> "git stash" before continuing ;-).
>
> If you think it's enough to replicate the codepath that precedes the
> actual saving in 'git stash' (which is essentially
> require-clean-work-tree), I'm in agreement with you.  I thought you
> were implying a wider safety net, that wouldn't even assume the basic
> sanity of stash.

Er, s/codepath that precedes the actual saving in 'git stash'/codepath
that precedes the actual pulling or merging in 'git pull'/

I'm feeling a little muddled up today; weekends are usually bad.