git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Introduce pull.autostash
@ 2013-03-22 12:29 Ramkumar Ramachandra
  2013-03-22 12:29 ` [PATCH 1/3] git-pull.sh: prefer invoking "git <command>" over "git-<command>" Ramkumar Ramachandra
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-22 12:29 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] git-pull.sh: prefer invoking "git <command>" over "git-<command>"
  2013-03-22 12:29 [PATCH 0/3] Introduce pull.autostash Ramkumar Ramachandra
@ 2013-03-22 12:29 ` Ramkumar Ramachandra
  2013-03-22 12:29 ` [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate Ramkumar Ramachandra
  2013-03-22 12:29 ` [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash Ramkumar Ramachandra
  2 siblings, 0 replies; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-22 12:29 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate
  2013-03-22 12:29 [PATCH 0/3] Introduce pull.autostash Ramkumar Ramachandra
  2013-03-22 12:29 ` [PATCH 1/3] git-pull.sh: prefer invoking "git <command>" over "git-<command>" Ramkumar Ramachandra
@ 2013-03-22 12:29 ` Ramkumar Ramachandra
  2013-03-22 16:51   ` Junio C Hamano
  2013-03-22 12:29 ` [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash Ramkumar Ramachandra
  2 siblings, 1 reply; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-22 12:29 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash
  2013-03-22 12:29 [PATCH 0/3] Introduce pull.autostash Ramkumar Ramachandra
  2013-03-22 12:29 ` [PATCH 1/3] git-pull.sh: prefer invoking "git <command>" over "git-<command>" Ramkumar Ramachandra
  2013-03-22 12:29 ` [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate Ramkumar Ramachandra
@ 2013-03-22 12:29 ` Ramkumar Ramachandra
  2013-03-22 17:02   ` Junio C Hamano
  2 siblings, 1 reply; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-22 12:29 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate
  2013-03-22 12:29 ` [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate Ramkumar Ramachandra
@ 2013-03-22 16:51   ` Junio C Hamano
  2013-03-23 12:38     ` Ramkumar Ramachandra
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2013-03-22 16:51 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash
  2013-03-22 12:29 ` [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash Ramkumar Ramachandra
@ 2013-03-22 17:02   ` Junio C Hamano
  2013-03-23 12:48     ` Ramkumar Ramachandra
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2013-03-22 17:02 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List

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?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate
  2013-03-22 16:51   ` Junio C Hamano
@ 2013-03-23 12:38     ` Ramkumar Ramachandra
  0 siblings, 0 replies; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-23 12:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash
  2013-03-22 17:02   ` Junio C Hamano
@ 2013-03-23 12:48     ` Ramkumar Ramachandra
  2013-03-24  7:28       ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-23 12:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash
  2013-03-23 12:48     ` Ramkumar Ramachandra
@ 2013-03-24  7:28       ` Junio C Hamano
  2013-03-24 17:56         ` Ramkumar Ramachandra
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2013-03-24  7:28 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List

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".

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash
  2013-03-24  7:28       ` Junio C Hamano
@ 2013-03-24 17:56         ` Ramkumar Ramachandra
  2013-03-24 21:12           ` Ramkumar Ramachandra
  0 siblings, 1 reply; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-24 17:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

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?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash
  2013-03-24 17:56         ` Ramkumar Ramachandra
@ 2013-03-24 21:12           ` Ramkumar Ramachandra
  0 siblings, 0 replies; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-03-24 21:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate
  2013-04-13 21:15 [PATCH v2 0/3] Introduce pull.autostash Ramkumar Ramachandra
@ 2013-04-13 21:15 ` Ramkumar Ramachandra
  0 siblings, 0 replies; 12+ messages in thread
From: Ramkumar Ramachandra @ 2013-04-13 21:15 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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 aa31abe..3bdfe82 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.1.392.g6943ea6

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-04-13 21:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-22 12:29 [PATCH 0/3] Introduce pull.autostash Ramkumar Ramachandra
2013-03-22 12:29 ` [PATCH 1/3] git-pull.sh: prefer invoking "git <command>" over "git-<command>" Ramkumar Ramachandra
2013-03-22 12:29 ` [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate Ramkumar Ramachandra
2013-03-22 16:51   ` Junio C Hamano
2013-03-23 12:38     ` Ramkumar Ramachandra
2013-03-22 12:29 ` [PATCH 3/3] git-pull.sh: introduce --[no-]autostash and pull.autostash Ramkumar Ramachandra
2013-03-22 17:02   ` Junio C Hamano
2013-03-23 12:48     ` Ramkumar Ramachandra
2013-03-24  7:28       ` Junio C Hamano
2013-03-24 17:56         ` Ramkumar Ramachandra
2013-03-24 21:12           ` Ramkumar Ramachandra
2013-04-13 21:15 [PATCH v2 0/3] Introduce pull.autostash Ramkumar Ramachandra
2013-04-13 21:15 ` [PATCH 2/3] t5521 (pull-options): use test_commit() where appropriate Ramkumar Ramachandra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).