[PATCH v2 0/3] Introduce pull.autostash

[PATCH v2 0/3] Introduce pull.autostash

[Ramkumar Ramachandra]

I've been using this patch for some time now, and have to say that
'git pull' is much more useable now.  The feature is turned off by
default so that it doesn't break any existing tests.

[1/3] and [2/3] are simple "while we're there" patches, that are
unchanged from the last round.

[3/3] is actually fine in this round.  I'm embarassed that I sent out
the previous round in such a hurry: there were a lot of things wrong
with it, as Junio's review pointed out.

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

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

-- 
1.8.2.1.392.g6943ea6

[PATCH 1/3] pull: 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 638aabb..5fe69fa 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -285,11 +285,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 $verbosity"
+	eval="git rebase $diffstat $strategy_args $merge_args $verbosity"
 	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
 	;;
 *)
-	eval="git-merge $diffstat $no_commit $verify_signatures $edit $squash $no_ff $ff_only"
+	eval="git merge $diffstat $no_commit $verify_signatures $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.1.392.g6943ea6

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

[Philip Oakley]

From: "Ramkumar Ramachandra" <artagnon@gmail.com>
Sent: Saturday, April 13, 2013 10:15 PM
> 14e5d40c (pull: Fix parsing of -X<option>, 2010-01-17) added the lines
> containing "git-push" and "git-merge", even though the prevelant style

s /git-push/git rebase/    The fix is to a git-rebase not git-push 
invocation.

> 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 638aabb..5fe69fa 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -285,11 +285,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 $verbosity"
> + eval="git rebase $diffstat $strategy_args $merge_args $verbosity"
>  eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
>  ;;
> *)
> - eval="git-merge $diffstat $no_commit $verify_signatures $edit 
> $squash $no_ff $ff_only"
> + eval="git merge $diffstat $no_commit $verify_signatures $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.1.392.g6943ea6
>
> --
> 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
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2013.0.3272 / Virus Database: 3162/6242 - Release Date: 
> 04/13/13
> 

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

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

[Ramkumar Ramachandra]

Currently, executing a 'git pull' on a dirty worktree gives the
following annoying message:

    # User doesn't notice dirty worktree
    $ git pull

    ... # fetch operation

    error: Your local changes to the following files would be overwritten by merge:
        quux
    Please, commit your changes or stash them before you can merge.
    Aborting

At which point the user will stash her changes, re-execute git pull,
and pop the stash.  This process can easily be automated out for a
smooth:

    $ git pull

    ... # fetch operation

    Saved working directory and index state WIP on master: 8ea73ed baz
    HEAD is now at 8ea73ed baz

    ... # The merge/rebase process

    Dropped refs/stash@{0} (83f47fbfb07a741817633f9191dc4a1530f79249)

If there is a conflict during the merge/rebase process, the user has
to pop the stash by hand after committing the conflict resolution:

    $ git pull

    ... # fetch operation

    Saved working directory and index state WIP on master: 8ea73ed baz
    HEAD is now at 8ea73ed baz

    ... # The merge/rebase process

    Automatic merge failed; fix conflicts and then commit the result.
    Please run 'git stash pop' after commiting the conflict resolution.

Introduce a new configuration variable pull.autostash that does
exactly this.

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3d750e0..6c5cd8e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1804,6 +1804,11 @@ pull.rebase::
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).
 
+pull.autostash::
+	When true, automatically stash all changes before attempting
+	to merge/rebase, and pop the stash after a successful
+	merge/rebase.
+
 pull.octopus::
 	The default merge strategy to use when pulling multiple branches
 	at once.
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 24ab07a..1c5384b 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 5fe69fa..4edc06a 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 || echo false)
 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
 		;;
@@ -202,7 +209,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= &&
@@ -219,6 +227,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
@@ -294,4 +308,19 @@ true)
 	eval="$eval \"\$merge_name\" HEAD $merge_head"
 	;;
 esac
-eval "exec $eval"
+
+if test "$autostash" = true && stash_required
+then
+	git stash || die "$(gettext "Cannot autostash")" &&
+	require_clean_work_tree "pull" "Please commit or stash them." &&
+	if eval "$eval"
+	then
+		git stash pop || die "$(gettext "Cannot pop stash")"
+	else
+		exit_code=$?
+		echo "Please run 'git stash pop' after commiting the conflict resolution."
+		exit $exit_code
+	fi
+else
+	eval "exec $eval"
+fi
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 3bdfe82..4545671 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -117,4 +117,67 @@ 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.1.392.g6943ea6

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Currently, executing a 'git pull' on a dirty worktree gives the
> following annoying message:

s/annoying//;

In general, avoid forcing your value judgement to readers when you
do not have to.  Instead, you can just highlight the source of your
annoyance and let them naturally feel the annoyance themselves,
perhaps like this:

	When the user runs "git pull" with local changes in the
	working tree that interfere with the merge (or any change if
	the user is using --rebase), the user is told that the merge
	or rebase cannot be done _after_ the command spends time to
	download necessary objects.

	The user can then decide not to perform the pull and finish
	what he was in the middle of doing (e.g. by "git checkout -b
	topic" and then finishing the work), before coming back to
	the branch and running "git pull" again, or "git stash" the
	work in progress, run "git pull" and then "git stash pop".

	Introduce a new flag "git pull --autostash" to perform the
	latter automatically. Also add pull.autostash configuration
	to do so without giving the command line flag, whose effect
	can be overriden by the --no-autostash option.

While I understand where this wish comes from, I am not sure if this
is generally a good idea, or if we are solving a wrong problem.

>     Saved working directory and index state WIP on master: 8ea73ed baz
>     HEAD is now at 8ea73ed baz
>
>     ... # The merge/rebase process

I think there is one step missing after "The merge/rebase process"
above, which is

      ... # The 'stash pop' process

>     Dropped refs/stash@{0} (83f47fbfb07a741817633f9191dc4a1530f79249)

If the pull-merge were something that would induce the "annoyance"
that triggered this topic, by definition, the local change overlaps
with the merge, and this internal "stash pop" will touch the paths
the merge touched and it is likely not result in "Dropped" but leave
further conflicts to be resolved.

In the end, simple cases (the canonical example being Linus keeping
a local change to "Name = Unicycling Gorilla" in his Makefile while
running dozens of "git pull" every day) would not be helped with
this feature very much because there is rarely overlap, while in a
complex case the user really needs help to save away his WIP, the
user is forced to resolve the conflict immediately upon the failed
"stash pop" at the end of "git pull".  If the conflict turns out to
be something the user would not want to resolve at that moment, the
user needs to know the way out, something like:

	git reset --hard		;# go back to the result of pull
        git checkout -b wip ORIG_HEAD	;# start from before the pull
        git stash pop			;# recover his wip
	... perhaps work a bit more ...
        git commit -a -m wip		;# save it away for safety
	git checkout -			;# result of pull

so that he can come back tomorrow, check out "wip" branch, and
decide what to do.


The "--autosquash" option (or not adding either the configuration or
the option) would encourage the user to think about the nature of
WIP he has in the working tree before running "git pull".  Is it a
too complex beast that may interfere with what others are doing, or
is it a trivial change that he can stash away and pop it back?  If
the former, he will be far better off doing "checkout -b", keep
working until the local change gets into somewhat a better shape and
"commit", before pulling into the original branch.  If the latter,
he is better off doing either

 - "git pull", after finding it conflicts, run "git stash", "git
   merge FETCH_HEAD" and "git stash pop"; or

 - "git pull --autostash".

And not having "--autostash" would mean he would do the former, and
will do "stash" only when it matters.  "--autostash" is a good to
neutral addition in that sense.  But I suspect that pull.autostash
configuration is not a good addition because it encourages a bad,
pain-inducing workflow.  In simple cases it may not hurt, but when
local changes are complex, it would actively hurt than not having it,
and the configuration robs the incentive to choose.

The equation is somewhat different for "pull-rebase", as "rebase"
insists you to start from a clean working tree, so "download and
then stop" annoyance feels bigger.  I have a suspicion that
loosening that may be a more productive fix to the real problem.

> +stash_required () {
> +	! (git diff-files --quiet --ignore-submodules &&
> +	git diff-index --quiet --cached HEAD --ignore-submodules)
> +}

require_clean_work_tree refreshes the index before running
diff-files to make this check safe, but you do not do that here.  Do
you know the index has been refreshed when this is called?

Even though this is mere two-line logic, the duplication of the
logic look a bit wasteful.  Is it too hard to teach "dry-run, quiet"
mode to require_clean_work_tree helper?  Then the auto-stash
codepath can simply be:

	(require_clean_work_tree --quiet) || git stash

no?

> +if test "$autostash" = true && stash_required
> +then
> +	git stash || die "$(gettext "Cannot autostash")" &&
> +	require_clean_work_tree "pull" "Please commit or stash them." &&
> +	if eval "$eval"
> +	then
> +		git stash pop || die "$(gettext "Cannot pop stash")"

This "cannot pop stash" is really where the user gets in the deep
yoghurt and needs help.

> +	else
> +		exit_code=$?
> +		echo "Please run 'git stash pop' after commiting the conflict resolution."

Given that many people do not read messages carefully, it would be a
better idea to list them in the order the user carries them out,
perhaps

	Please resolve conflicts and commit, and then 'git stash pop'.

or something.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> In the end, simple cases (the canonical example being Linus keeping
> a local change to "Name = Unicycling Gorilla" in his Makefile while
> running dozens of "git pull" every day) would not be helped with
> this feature very much because there is rarely overlap, while in a
> complex case the user really needs help to save away his WIP, the
> user is forced to resolve the conflict immediately upon the failed
> "stash pop" at the end of "git pull".  If the conflict turns out to
> be something the user would not want to resolve at that moment, the
> user needs to know the way out, something like:
>
>         git reset --hard                ;# go back to the result of pull
>         git checkout -b wip ORIG_HEAD   ;# start from before the pull
>         git stash pop                   ;# recover his wip
>         ... perhaps work a bit more ...
>         git commit -a -m wip            ;# save it away for safety
>         git checkout -                  ;# result of pull
>
> so that he can come back tomorrow, check out "wip" branch, and
> decide what to do.

What does this have to do with pull at all?  git-pull.sh is a simple
shell script that runs fetch followed by a merge or rebase.  You're
handling a merge now, and there's proper support for it in the tool;
git merge --abort will work with one caveat:  "If there were
uncommitted worktree changes present when the merge started, git merge
--abort will in some cases be unable to reconstruct these changes. It
is therefore recommended to always commit or stash your changes before
running git merge.", to directly quote the manpage.  Really, it has
the same problem as rebase, if you want --abort to work.

"Fixing" git merge --abort and git rebase are topics for another day.
What I'm doing it making it easier to work with the merge/rebase
following the fetch by automatically stashing worktree + index
changes.

> The "--autosquash" option (or not adding either the configuration or
> the option) would encourage the user to think about the nature of
> WIP he has in the working tree before running "git pull".

I'm lost.  What does git rebase --autosquash (or rebase.autosquash)
have anything to do with git pull --autostash?

> Is it a
> too complex beast that may interfere with what others are doing, or
> is it a trivial change that he can stash away and pop it back?  If
> the former, he will be far better off doing "checkout -b", keep
> working until the local change gets into somewhat a better shape and
> "commit", before pulling into the original branch.

A pull-merge person having complex worktree changes should not execute
git pull blindly in the first place.  That's what git fetch is for:
inspect the incoming changes, and decide how to integrate it into the
local branch.  git pull is a tool that serves an entirely different
purpose, in my opinion.

> But I suspect that pull.autostash
> configuration is not a good addition because it encourages a bad,
> pain-inducing workflow.  In simple cases it may not hurt, but when
> local changes are complex, it would actively hurt than not having it,
> and the configuration robs the incentive to choose.

But I'm a pull-rebase guy, and I always want pull.autostash.  Will you
deny me the configuration variable, simply because it is potentially
pain-inducing to pull-merge people who set it blindly and run git pull
blindly?

I'm not arguing linking pull.autostash to pull.rebase either: my
change is admittedly useful to *some* pull-merge people.  And I'm not
arguing for making it true by default.

> The equation is somewhat different for "pull-rebase", as "rebase"
> insists you to start from a clean working tree, so "download and
> then stop" annoyance feels bigger.  I have a suspicion that
> loosening that may be a more productive fix to the real problem.

Rebase performs relatively simple operation on the worktree, and I
would like to keep it that way.  I have no desire to "fix" rebase.

>         (require_clean_work_tree --quiet) || git stash

Good point.  Will fix in the next iteration.

> This "cannot pop stash" is really where the user gets in the deep
> yoghurt and needs help.

Yes, and your point being?  That life would've been easier if the user
had committed all those changes in the first place?  And that the
penalty for not having done that is now a git checkout wip ORIG_HEAD;
git stash pop; git checkout master; git rebase wip?

>         Please resolve conflicts and commit, and then 'git stash pop'.

Cool, will fix.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> In the end, simple cases (the canonical example being Linus keeping
>> a local change to "Name = Unicycling Gorilla" in his Makefile while
>> running dozens of "git pull" every day) would not be helped with
>> this feature very much because there is rarely overlap, while in a
>> complex case the user really needs help to save away his WIP, the
>> user is forced to resolve the conflict immediately upon the failed
>> "stash pop" at the end of "git pull".  If the conflict turns out to
>> be something the user would not want to resolve at that moment, the
>> user needs to know the way out, something like:
>>
>>         git reset --hard                ;# go back to the result of pull
>>         git checkout -b wip ORIG_HEAD   ;# start from before the pull
>>         git stash pop                   ;# recover his wip
>>         ... perhaps work a bit more ...
>>         git commit -a -m wip            ;# save it away for safety
>>         git checkout -                  ;# result of pull
>>
>> so that he can come back tomorrow, check out "wip" branch, and
>> decide what to do.
>
> What does this have to do with pull at all?

Exactly.

By tempting the user to use autostash first, you are forcing the
user to say "reset --hard" (the first one), "ORIG_HEAD" (to start
from the pre-pull state), "stash pop" (to recover the autostashed
WIP), to a user who got conflict during "stash pop" after the pull
integrated the committed work with the remote side.

If the user did this instead:

        ... Let's save my large WIP away in a more permanent place first
	git checkout -b wip
        ... perhaps work a bit more ...
        git commit -a -m wip
        git checkout -
	... and then ...
	git pull

the user wouldn't have had to do those extra steps.

Another thing to consider is that even with the current system, many
users do not have a clear idea on what the state of the working tree
is when a "pull" fails (there are largely two classes). Either the
merge failed without damaging the WIP before the pull at all, or
there wasn't any interaction between the WIP and the change pull
wanted to bring in and only the paths with conflicts need to be
resolved and added (i.e. "commit -a" is a no-no). This "auto-pop"
adds a third failure mode to "pull". It is a non-issue for Git-heads
like us (we do read the messages from the programs), but not all
users are like us.

>> The "--autosquash" option (or not adding either the configuration or
>> the option) would encourage the user to think about the nature of
>> WIP he has in the working tree before running "git pull".
>
> I'm lost.  What does git rebase --autosquash (or rebase.autosquash)
> have anything to do with git pull --autostash?

Sorry, the typo meant --autostash.

>> Is it a
>> too complex beast that may interfere with what others are doing, or
>> is it a trivial change that he can stash away and pop it back?  If
>> the former, he will be far better off doing "checkout -b", keep
>> working until the local change gets into somewhat a better shape and
>> "commit", before pulling into the original branch.
>
> A pull-merge person having complex worktree changes should not execute
> git pull blindly in the first place.   That's what git fetch is for:

Huh?  You are not making sense.

I would understand it if it were "That's what git commit is for",
though.

> But I'm a pull-rebase guy, and I always want pull.autostash.

That is why I said the equation is different for pull-rebase, where
"rebase" insists that you start from a squeaky clean working tree,
so you can check the condition early before "git pull" even starts.
While --autostash will not improve the life for pull-merge people
(and pull.autostash will actively hurt them), pull.autostash does
help pull-rebase people work around the limitation of "rebase".

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> By tempting the user to use autostash first, you are forcing the
> user to say "reset --hard" (the first one), "ORIG_HEAD" (to start
> from the pre-pull state), "stash pop" (to recover the autostashed
> WIP), to a user who got conflict during "stash pop" after the pull
> integrated the committed work with the remote side.
>
> If the user did this instead:
>
>         ... Let's save my large WIP away in a more permanent place first
>         git checkout -b wip
>         ... perhaps work a bit more ...
>         git commit -a -m wip
>         git checkout -
>         ... and then ...
>         git pull
>
> the user wouldn't have had to do those extra steps.

Hm, yes.  For a pull-merge guy who opts for pull.autostash, the
penalty for forgetting to commit a big WIP in advance is too high.  It
probably makes sense to restrict the autostash feature to kick in only
in the rebase codepath: it might be a good idea to implement
rebase.autostash for reduced case of non-interactive rebases instead.
I'm only slightly iffy about --onto, but it's not a big concern.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +stash_required () {
> +	! (git diff-files --quiet --ignore-submodules &&
> +	git diff-index --quiet --cached HEAD --ignore-submodules)
> +}

Isn't this too pessimistic? If the local changes do not overlap (in
terms of files) with the pulled changes, then autosquash is not needed.

There should be a test for the case of non-overlapping changes.

> +if test "$autostash" = true && stash_required
> +then
> +	git stash || die "$(gettext "Cannot autostash")" &&
> +	require_clean_work_tree "pull" "Please commit or stash them." &&
> +	if eval "$eval"
> +	then
> +		git stash pop || die "$(gettext "Cannot pop stash")"
> +	else
> +		exit_code=$?
> +		echo "Please run 'git stash pop' after commiting the conflict resolution."
> +		exit $exit_code
> +	fi
> +else
> +	eval "exec $eval"
> +fi

Shouldn't this belong to "git merge" instead (i.e. implement "git merge
--autosquash" and call it from "git pull")? Users doing "git fetch &&
git merge" by hand should be able to use --autosquash, I think.

Something should be done for "git rebase" and "git pull --rebase" too.
In this case, the UI can be much smoother, since the user would have to
run "git rebase --continue" anyway, so you can do the auto-stash-pop for
him by adding something like "exec git stash pop" at the end of the todo-list.

Ideally, "git rebase --abort" should auto-stash-pop too, although this
is much less trivial to implement.

Maybe a good first step is to implement --autosquash only for non-rebase
pull, and later to implement "git rebase --autosquash" and call it from
"git pull".

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> +stash_required () {
>> +	! (git diff-files --quiet --ignore-submodules &&
>> +	git diff-index --quiet --cached HEAD --ignore-submodules)
>> +}
>
> Isn't this too pessimistic? If the local changes do not overlap (in
> terms of files) with the pulled changes, then autosquash is not needed.

Yes, that is why I said for pull-merge, --authsquash is neutral-to-better
and pull.autosquash is harmful.

But for pull-rebase folks, I can understand why this "working tree
must be squeakily clean" logic is appropriate, if we were to do
this. The root cause is because rebase insists to be run on such a
working tree.

And the worst part is that in order to check if local changes
overlap, you need to fetch first. But Ram's annoyance is about the
user being told the merge/rebase cannot proceed _after_ fetch is
done.

> Shouldn't this belong to "git merge" instead (i.e. implement "git merge
> --autosquash" and call it from "git pull")? Users doing "git fetch &&
> git merge" by hand should be able to use --autosquash, I think.

See my other message. I do not think autosquash would help "merge"
folks very much, and will actively hurt when it matters.

> Something should be done for "git rebase" and "git pull --rebase" too.

That I would agree. I am not sure autosquash is the best approach,
but we should be able to help them more.

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

[Matthieu Moy]

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

> Yes, that is why I said for pull-merge, --authsquash is neutral-to-better
> and pull.autosquash is harmful.

To speak from my experience: I find myself typing "git stash && git pull
&& git stash pop" relatively often. Typical use-case: I start working on
something, a colleague works on the same thing and I need to see what he
did to continue. Probably not something so frequent for large projects,
but very frequent for small projects (e.g. writing a paper together with
a single .tex file and the deadline approaching). In this case, "git
pull --rebase" makes sense for advanced enough users, but newbies who
have been told "rebase is too dangerous for you, don't use it", it would
be cool to have --autostash too.

I tend to agree that pull.autostash is harmful. At least in its current
form, it is really too easy to overlook the "Please run 'git stash pop'
after commiting the conflict resolution." message:

<do some important changes>
$ git pull
<fix conflicts>
$ git status
<tells me to commit>
$ git commit
<WTF, where are my important changes?!?>

rebase wouldn't have this issue if "stash pop" is part of the sequence
to apply.

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

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> I tend to agree that pull.autostash is harmful. At least in its current
> form, it is really too easy to overlook the "Please run 'git stash pop'
> after commiting the conflict resolution." message:
>
> <do some important changes>
> $ git pull
> <fix conflicts>
> $ git status
> <tells me to commit>
> $ git commit
> <WTF, where are my important changes?!?>

First, this is not why Junio said pull.autostash is harmful: he had a
completely different objection.

Second, this makes no sense.  The user consciously made the decision
to set pull.autostash to true: it isn't turned on by default, and
there's no magic that's turning it on without the user knowing.  This
case is roughly equivalent to the user executing 'git stash' and
suddenly waking up one day and complaining "WTF, where are my
important changes?!?".  Sure, it might be a good idea to teach git
status to show the stash state, but that's an orthogonal topic.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> First, this is not why Junio said pull.autostash is harmful: he had a
> completely different objection.

Yes. This is another objection.

> Second, this makes no sense.  The user consciously made the decision
> to set pull.autostash to true

... possibly a long time ago.

> This case is roughly equivalent to the user executing 'git stash' and
> suddenly waking up one day and complaining "WTF, where are my
> important changes?!?".

I disagree. A configuration option is something you set once, and then
forget about. A command, or a command-line option, is something you
explicitely add when you need it.

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

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> I disagree. A configuration option is something you set once, and then
> forget about. A command, or a command-line option, is something you
> explicitely add when you need it.

You're making it out to be a much bigger difference than it actually
is.  Users will simply alias pull to 'pull --autostash' (a lot of them
already alias it to pull --ff-only, and I'm going to fix this soon).
The decision making process for creating a configuration variable
shouldn't be "this is potentially dangerous, and therefore therefore
it shouldn't be a configuration variable", but rather "this is a
rarely used option that users only need <50% of the time, and
therefore it shouldn't be a configuration variable".  In my case,
pull.autostash is my 90~95% usecase, and I'm not unique in this
aspect.  Therefore, it should be a configuration variable that can be
consciously turned off with a --no-autostash.

If your criticism were that git status doesn't show stash state, I
agree with you.  However, I don't agree with basing it on user
forgetfulness in having set pull.autostash a long time ago + lack of
observation skills to notice the message printed by git pull.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Matthieu Moy wrote:
>> I disagree. A configuration option is something you set once, and then
>> forget about. A command, or a command-line option, is something you
>> explicitely add when you need it.
>
> You're making it out to be a much bigger difference than it actually
> is.  Users will simply alias pull to 'pull --autostash' (a lot of them
> already alias it to pull --ff-only, and I'm going to fix this soon).

No, they don't.  Git forbids redefining commands with aliases. They may
have an alias like "git pullauto" or so, but not "git pull".

> If your criticism were that git status doesn't show stash state, I
> agree with you.

There's not much we can do about it now, as Git cannot guess whether a
stash is to be re-applied later or just kept "in case". My main use of
"git stash" is "I want a reset --hard, but stash is safer", I wouldn't
want "status" to remind me when I have a stash because it is almost
always the case.

Showing the "autostash" status in "git status" would make sense OTOH,
but I agree that it's another topic.

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

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> No, they don't.  Git forbids redefining commands with aliases. They may
> have an alias like "git pullauto" or so, but not "git pull".

Ofcourse, but you get the point.  I use p for push, and pu for pull myself.

> There's not much we can do about it now, as Git cannot guess whether a
> stash is to be re-applied later or just kept "in case". My main use of
> "git stash" is "I want a reset --hard, but stash is safer", I wouldn't
> want "status" to remind me when I have a stash because it is almost
> always the case.
>
> Showing the "autostash" status in "git status" would make sense OTOH,
> but I agree that it's another topic.

If the HEAD of the stash contains a stash beginning with the message
"pull.autostash: ", show it in the status.  End of story.

Anyway, no point arguing about this since we've decided not to pursue
pull.autostash anyway.

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

[Jonathan Nieder]

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

>> Yes, that is why I said for pull-merge, --authsquash is neutral-to-better
>> and pull.autosquash is harmful.
>
> To speak from my experience: I find myself typing "git stash && git pull
> && git stash pop" relatively often.

To that end, maybe "git pull --stash" would be an easier to type option
name than "git pull --autostash".

That would also avoid the question of "What is automatically going to
happen here?  Is it going to stash only sometimes?  Is it going to
automatically stash what I pull?".

I'm not a big stash user (except "git stash --keep-index" to prepare
to test changes in the index), so I can't say much more about the
details.

Thanks,
Jonathan

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> Isn't this too pessimistic? If the local changes do not overlap (in
> terms of files) with the pulled changes, then autosquash is not needed.
>
> There should be a test for the case of non-overlapping changes.

In the pull-rebase case, no; it is not too pessimistic.

In the pull-merge case, maybe: if your worktree isn't clean, you lose
a lot of goodies like merge --abort anyway, and I hate that.
Secondly, it's impossible to determine whether the changes overlap or
not before actually running merge_trees().  If there were an easy way
to do it, I might have considered it.

Overall, I don't see how an extra stash/ stash pop where not
absolutely necessary hurts.

> Shouldn't this belong to "git merge" instead (i.e. implement "git merge
> --autosquash" and call it from "git pull")? Users doing "git fetch &&
> git merge" by hand should be able to use --autosquash, I think.

--autosquash reminds me of rebase.autosquash, and that is completely
unrelated to the issue at hand.  Did you mean git merge --squash (to
update the worktree/index but not create the actual commit?).  Sure,
it's probably useful to have a merge.squash configuration variable,
but I don't see what it has to do with the pull.autostash I'm
proposing.

> Something should be done for "git rebase" and "git pull --rebase" too.
> In this case, the UI can be much smoother, since the user would have to
> run "git rebase --continue" anyway, so you can do the auto-stash-pop for
> him by adding something like "exec git stash pop" at the end of the todo-list.

No.  I'm against executing a special codepath for a pull-rebase that
has no equivalent in the pull-merge world.  Or did you mean: have one
configuration variable to git merge --squash and do this for git
rebase, as if they're equivalent from the pull perspective?  No, they
aren't.

> Ideally, "git rebase --abort" should auto-stash-pop too, although this
> is much less trivial to implement.

As I already pointed out in my message to Junio, "fixing" rebase is
not the topic of discussion at all.

> Maybe a good first step is to implement --autosquash only for non-rebase
> pull, and later to implement "git rebase --autosquash" and call it from
> "git pull".

"Implement" git rebase --autosquash?  If I just set rebase.autosquash
to true, the rebase will automatically autosquash whether called from
git pull or otherwise.  Sorry, I just don't understand what you're
saying.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> In the pull-merge case, maybe: if your worktree isn't clean, you lose
> a lot of goodies like merge --abort anyway, and I hate that.

AFAICT, "git merge --abort" is an alias for "git reset --merge" which
was precisely designed to reset only modifications comming from a merge,
and not the local changes that were present before the merge was
started. The man pages are relatively obscure on the subject, but I'd
call that a documentation bug.

> Overall, I don't see how an extra stash/ stash pop where not
> absolutely necessary hurts.

It does. stashing means the user will have to "stash pop" later. One
extra step, one extra opportunity to forget something important.

A minor annoyance is that it will touch files that have no reason to be
touched, hence may trigger extra rebuilds with "make", disturbing text
editors that have the file open, etc.

The fact that "git pull" just works on dirty trees with non-overlapping
changes is an important feature of Git.

>> Shouldn't this belong to "git merge" instead (i.e. implement "git merge
>> --autosquash" and call it from "git pull")? Users doing "git fetch &&
>> git merge" by hand should be able to use --autosquash, I think.
>
> --autosquash reminds me of rebase.autosquash, and that is completely
> unrelated to the issue at hand.

A typo seems to have propagated in this thread. I meant auto*stash*, not
auto*squash*, sorry. I guess it's the same typo in Junio's message.

>> Something should be done for "git rebase" and "git pull --rebase" too.
>> In this case, the UI can be much smoother, since the user would have to
>> run "git rebase --continue" anyway, so you can do the auto-stash-pop for
>> him by adding something like "exec git stash pop" at the end of the todo-list.
>
> No.  I'm against executing a special codepath for a pull-rebase that
> has no equivalent in the pull-merge world.

I think you're taking it the wrong way. You seem to insist that
autostash should be a pull feature. I think it should be a feature of
merge and rebase, and the convenience script "git pull" should expose
them to the user.

I see no reason to prevent users running fetch and then merge or rebase
to use the autostash feature.

>> Ideally, "git rebase --abort" should auto-stash-pop too, although this
>> is much less trivial to implement.
>
> As I already pointed out in my message to Junio, "fixing" rebase is
> not the topic of discussion at all.

It's not about fixing the existing rebase. It's about implementing
autostash the right way.

As a user, when I run "git rebase --continue" and it tells me it's done,
I expect the work to actually be done. This is the case today. This
won't be the case after autostash is introduced if the user has to
remember to run "stash pop" afterwards.

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

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> AFAICT, "git merge --abort" is an alias for "git reset --merge"

Yes, that is correct.

> which
> was precisely designed to reset only modifications comming from a merge,
> and not the local changes that were present before the merge was
> started. The man pages are relatively obscure on the subject, but I'd
> call that a documentation bug.

I see.  Either way, we need a clean worktree for it to work, no?

> It does. stashing means the user will have to "stash pop" later. One
> extra step, one extra opportunity to forget something important.

That's only if there are conflicts.  If there are conflicts, you'll
have to stash anyway if:
- You're doing a pull-merge and want merge --abort to work.
- You're doing a pull-rebase.

In the case when there are no conflicts, what is the problem?

> A minor annoyance is that it will touch files that have no reason to be
> touched, hence may trigger extra rebuilds with "make", disturbing text
> editors that have the file open, etc.

Okay, I need to ask you something at this point: do you ever run merge
on a dirty worktree unless you're absolutely sure that your local
changes won't conflict with the changes introduced by the merge?  I
_never_ run merge on a dirty worktree myself.

> The fact that "git pull" just works on dirty trees with non-overlapping
> changes is an important feature of Git.

That's only a pull-merge.  Unfortunately, making git-pull.sh uniform
means that we have to fall back to the least-common-denominator of
functionality (which is currently pull-rebase).

> I think you're taking it the wrong way. You seem to insist that
> autostash should be a pull feature. I think it should be a feature of
> merge and rebase, and the convenience script "git pull" should expose
> them to the user.
>
> I see no reason to prevent users running fetch and then merge or rebase
> to use the autostash feature.

Okay, so you're proposing a uniform --autostash feature for both merge
and rebase.  Sorry, I didn't get it last time due to the typos in your
email; yeah, this is worth investigating.

> As a user, when I run "git rebase --continue" and it tells me it's done,
> I expect the work to actually be done. This is the case today. This
> won't be the case after autostash is introduced if the user has to
> remember to run "stash pop" afterwards.

And how will you implement that for merge, since there is no merge
--continue to execute stash pop from?  Do you propose to make commit
do the stash pop'ing?

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

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Okay, so you're proposing a uniform --autostash feature for both merge
> and rebase.  Sorry, I didn't get it last time due to the typos in your
> email; yeah, this is worth investigating.

So, I thought about this and have concluded that pull is the Right
place for autostash because of the following reasons:

0. autostash is purely a convenience function that's useful in simple
reduced cases like a dumb 'git pull'; when the script we're
implementing it for doesn't have a ton of command-line options.

1. git stash; git pull; git stash pop; is a very common idiom.  git
stash; git rebase master; git stash pop; is a less common idiom, but I
agree that it is one rebase case where autostash could be useful.
Having git rebase -i show "exec git stash pop" at the end of a
user-editable instruction sheet is less than ideal.  Having git rebase
--onto master HEAD~3 do an autostash _might_ be useful, but I'm not
sure about it.  git stash; git merge feature-branch; git stash pop; is
not a common idiom at all.

2. git-stash.sh is a helper script, in the same spirit as git-pull.sh
and git-rebase.sh.  It is natural and easy to implement autostash for
pull and rebase, but not for a C builtin like merge.  In fact, it's
impossible to implement it for merge unless we make git commit execute
git stash pop (yuck!).

If you want, you can implement a rebase.autostash, but that is not my
itch.  Considering the gymnastics the implementation would have to do,
I'd be against a merge.autostash.  So, again: what is your gripe
against my pull.autostash implementation, apart from the fact that git
status doesn't show stash information? (I _might_ decide to fix this)

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Matthieu Moy wrote:
>> AFAICT, "git merge --abort" is an alias for "git reset --merge"
>
> Yes, that is correct.
>
>> which
>> was precisely designed to reset only modifications comming from a merge,
>> and not the local changes that were present before the merge was
>> started. The man pages are relatively obscure on the subject, but I'd
>> call that a documentation bug.
>
> I see.  Either way, we need a clean worktree for it to work, no?

No, you don't. Just try if you're not convinced:

$ git checkout -b branch
Switched to a new branch 'branch'
$ date > test.txt && git commit -m 'on branch' test.txt
[branch 2482623] on branch
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git checkout -
Switched to branch 'master'
$ date > test.txt && git commit -m 'on master' test.txt
[master c322d35] on master
 1 file changed, 1 insertion(+), 1 deletion(-)
$ date > other.txt 
$ git status
# On branch master
# Changes not staged for commit:
#
#       modified:   other.txt
#
no changes added to commit (use "git add" and/or "git commit -a")
$ git merge branch
Auto-merging test.txt
CONFLICT (content): Merge conflict in test.txt
Automatic merge failed; fix conflicts and then commit the result.
$ git status
# On branch master
# You have unmerged paths.
#
# Unmerged paths:
#
#       both modified:      test.txt
#
# Changes not staged for commit:
#
#       modified:   other.txt
#
no changes added to commit (use "git add" and/or "git commit -a")
$ git merge --abort
$ git status
# On branch master
# Changes not staged for commit:
#
#       modified:   other.txt
#
no changes added to commit (use "git add" and/or "git commit -a")
$ 

There may be corner-cases where it doesn't work, but I never encountered
such case.

>> It does. stashing means the user will have to "stash pop" later. One
>> extra step, one extra opportunity to forget something important.
>
> That's only if there are conflicts.  If there are conflicts, you'll
> have to stash anyway if:
> - You're doing a pull-merge and want merge --abort to work.

Again, no.

>> A minor annoyance is that it will touch files that have no reason to be
>> touched, hence may trigger extra rebuilds with "make", disturbing text
>> editors that have the file open, etc.
>
> Okay, I need to ask you something at this point: do you ever run merge
> on a dirty worktree unless you're absolutely sure that your local
> changes won't conflict with the changes introduced by the merge? 

Most of the time, I just run "git pull" or "git merge". I know it's
conservative enough, to it will stop if there's anything dangerous.

> That's only a pull-merge.  Unfortunately, making git-pull.sh uniform
> means that we have to fall back to the least-common-denominator of
> functionality (which is currently pull-rebase).

You may want to, but you don't have to. pull-merge and pull-rebase
already have different behavior in case of non-overlapping changes:

$ git pull --rebase . branch
Cannot pull with rebase: You have unstaged changes.
Please commit or stash them.
$ git pull --no-rebase . branch
From .
 * branch            branch     -> FETCH_HEAD
[...]

I don't see any reason to restrict to the common denominator in the same
situation for another feature.

I can accept the "it's too hard to implement" argument, but not "it
doesn't bring anything".

>> As a user, when I run "git rebase --continue" and it tells me it's done,
>> I expect the work to actually be done. This is the case today. This
>> won't be the case after autostash is introduced if the user has to
>> remember to run "stash pop" afterwards.
>
> And how will you implement that for merge, since there is no merge
> --continue to execute stash pop from?  Do you propose to make commit
> do the stash pop'ing?

No, I'm not proposing to do anything for merge. There's no reason to try
being uniform in conflict resolution for pull-merge and pull-rebase as
it is already different now. We already have "git rebase --continue", we
don't have "git merge --continue". So what? The fact that merge doesn't
have the equivalent doesn't mean we should not do something for "rebase
--continue".

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Matthieu Moy wrote:
>>> AFAICT, "git merge --abort" is an alias for "git reset --merge"
>>
>> Yes, that is correct.
>>
>>> which
>>> was precisely designed to reset only modifications comming from a merge,
>>> and not the local changes that were present before the merge was
>>> started. The man pages are relatively obscure on the subject, but I'd
>>> call that a documentation bug.
>>
>> I see.  Either way, we need a clean worktree for it to work, no?
>
> No, you don't. Just try if you're not convinced:

Heh, I still remember breaking "git merge" and got yelled at loudly.

cf. http://thread.gmane.org/gmane.comp.version-control.git/9073

>>> It does. stashing means the user will have to "stash pop" later. One
>>> extra step, one extra opportunity to forget something important.
>>
>> That's only if there are conflicts.  If there are conflicts, you'll
>> have to stash anyway if:
>> - You're doing a pull-merge and want merge --abort to work.
>
> Again, no.
>
>>> A minor annoyance is that it will touch files that have no reason to be
>>> touched, hence may trigger extra rebuilds with "make", disturbing text
>>> editors that have the file open, etc.
>>
>> Okay, I need to ask you something at this point: do you ever run merge
>> on a dirty worktree unless you're absolutely sure that your local
>> changes won't conflict with the changes introduced by the merge? 
>
> Most of the time, I just run "git pull" or "git merge". I know it's
> conservative enough, to it will stop if there's anything dangerous.

Exactly.

> No, I'm not proposing to do anything for merge. There's no reason to try
> being uniform in conflict resolution for pull-merge and pull-rebase as
> it is already different now. We already have "git rebase --continue", we
> don't have "git merge --continue". So what? The fact that merge doesn't
> have the equivalent doesn't mean we should not do something for "rebase
> --continue".

Well said.

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> No, you don't. Just try if you're not convinced:

Oh, I trusted the documentation on this one and never tried with a
dirty worktree myself.  Please fix the documentation, if you know how
exactly to correct it.

> No, I'm not proposing to do anything for merge. There's no reason to try
> being uniform in conflict resolution for pull-merge and pull-rebase as
> it is already different now. We already have "git rebase --continue", we
> don't have "git merge --continue". So what? The fact that merge doesn't
> have the equivalent doesn't mean we should not do something for "rebase
> --continue".

Well, you can't blame me for the misunderstanding then.

In a previous email, you wrote:
> Shouldn't this belong to "git merge" instead (i.e. implement "git merge
> --autosquash" and call it from "git pull")? Users doing "git fetch &&
> git merge" by hand should be able to use --autosquash, I think.

Junio's criticism of pull.autostash hurting pull-merge people is
cogent; my current plan is to ditch pull.autostash altogether, and
implement rebase.autostash for the reduced case of a non-interactive
rebase.

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

[John Keeping]

On Mon, Apr 15, 2013 at 10:15:54PM +0530, Ramkumar Ramachandra wrote:
> Junio's criticism of pull.autostash hurting pull-merge people is
> cogent; my current plan is to ditch pull.autostash altogether, and
> implement rebase.autostash for the reduced case of a non-interactive
> rebase.

Why restrict it to non-interactive?  I'd find it useful when doing
interactive rebases as well - consider the case when you simply want to
re-order some commits.

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

[Ramkumar Ramachandra]

John Keeping wrote:
> Why restrict it to non-interactive?  I'd find it useful when doing
> interactive rebases as well - consider the case when you simply want to
> re-order some commits.

Actually, I made a mistake: it should be doable for any specific
rebase (includes rebase--interactive, rebase--am, and rebase--merge)
just as easily, without leaking the autostash detail into them.  The
last statement in git-rebase.sh is run_specific_rebase, which just
needs to be wrapped in a git stash/ git stash pop.

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Matthieu Moy wrote:

>> No, you don't. Just try if you're not convinced:
>
> Oh, I trusted the documentation on this one and never tried with a
> dirty worktree myself.  Please fix the documentation, if you know how
> exactly to correct it.

The manual says:

	"git pull" and "git merge" will stop without doing anything when
	local uncommitted changes overlap with files that git pull/git
	merge may need to update.

That accurately describes the behavior.

I wouldn't be surprised if there's still a documentation bug, though:
a lack of clarity, a missing hint somewhere else, something else
misleading.  That seems especially likely when you say "I trusted the
documentation on this one".  Care to point to the appropriate section,
so it can be fixed?

Thanks,
Jonathan

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

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> I wouldn't be surprised if there's still a documentation bug, though:
> a lack of clarity, a missing hint somewhere else, something else
> misleading.  That seems especially likely when you say "I trusted the
> documentation on this one".  Care to point to the appropriate section,
> so it can be fixed?

As I pointed out in a previous email, I'm referring to the --abort
section of git-merge(1):

"If there were uncommitted worktree changes present when the merge
started, git merge --abort will in some cases be unable to reconstruct
these changes. It is therefore recommended to always commit or stash
your changes before running git merge."

Matthieu (and probably others) run git merge with a dirty worktree
most of the time, while I never do (because I read this section).

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> "If there were uncommitted worktree changes present when the merge
> started, git merge --abort will in some cases be unable to reconstruct
> these changes. It is therefore recommended to always commit or stash
> your changes before running git merge."
>
> Matthieu (and probably others) run git merge with a dirty worktree
> most of the time, while I never do (because I read this section).

The above says "in some cases".  It's presumably referring to the
case "One exception is when the changed index entries are in the state
that would result from the merge already" described in the
"PRE-MERGE CHECKS" section.

Improved wording welcome.

Ciao,
Jonathan

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> I think you're taking it the wrong way. You seem to insist that
> autostash should be a pull feature. I think it should be a feature of
> merge and rebase, and the convenience script "git pull" should expose
> them to the user.
>
> I see no reason to prevent users running fetch and then merge or rebase
> to use the autostash feature.

I agree that is a good way to look at the problem and would lead to
a much better design of the division of labor among these three
moving parts.

> It's not about fixing the existing rebase. It's about implementing
> autostash the right way.
>
> As a user, when I run "git rebase --continue" and it tells me it's done,
> I expect the work to actually be done. This is the case today. This
> won't be the case after autostash is introduced if the user has to
> remember to run "stash pop" afterwards.

Your "rebase can do the autostash right way" idea in the other
message is a good one, I think.  The rebase proper will sequencially
either apply patches (if the user is using the "format-patch | am"
variant) or cherry-pick commits ("rebase -m").  Conceptually we can
view the WIP in the working tree as just another commit at the tip
of the original history to be rebased (modulo that it should not be
left as a commit in the resulting history, and we may need to
differentiate what was in the index and what was only in the working
tree).  Ignoring the "conflicts at stash pop time" issue for now, a
rough outline may look like:

	git-rebase --autostash

		git commit --allow-empty -m 'index part'
	        git commit --allow-empty -a -m 'working tree part'
                git rebase --onto $UPSTREAM
                git reset HEAD^
                git reset --soft HEAD^

The first "reset" is to match the index to what was "stashed" with
the first "commit" ('index part') while keeping the working tree
changes the original WIP had ('working tree part'), and the last
"reset --soft" is to move the HEAD back to the tip of the originally
committed history.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>         git-rebase --autostash
>
>                 git commit --allow-empty -m 'index part'
>                 git commit --allow-empty -a -m 'working tree part'
>                 git rebase --onto $UPSTREAM
>                 git reset HEAD^
>                 git reset --soft HEAD^

Why are you rolling your own stash?  What do you have against git stash?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>         git-rebase --autostash
>>
>>                 git commit --allow-empty -m 'index part'
>>                 git commit --allow-empty -a -m 'working tree part'
>>                 git rebase --onto $UPSTREAM
>>                 git reset HEAD^
>>                 git reset --soft HEAD^
>
> Why are you rolling your own stash?  What do you have against git stash?

Nothing.  It just felt that it does not _have to be_ implemented
with stash, and explaining things in terms of commit and reset would
be simpler.

If "rebase -m" were to be taught to do this, the natural way to do
so is to 

  (1) Prepare the todo the usual way
  (2) Do those two commits for index and working tree
  (3) Append two insns (exec reset HEAD^ and exec reset --soft
      HEAD^) at the end of the rebase todo file.

Then the usual "rebase --continue" would just work.  Instead you
could use "git stash" at step #2 and add "exec git stash pop" at
step #3.

"rebase--am" could also be told to generate (on the preparation
side) and notice (on the application side) a pair of patch files at
the end that represent the index state and the working tree state
and apply them without making the WIP part into a commit.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If "rebase -m" were to be taught to do this, the natural way to do
> so is to
>
>   (1) Prepare the todo the usual way
>   (2) Do those two commits for index and working tree
>   (3) Append two insns (exec reset HEAD^ and exec reset --soft
>       HEAD^) at the end of the rebase todo file.

Er, no.  I don't want to touch the instruction sheet.  It becomes
especially problematic in -i, when the instruction sheet is
user-editable.

> "rebase--am" could also be told to generate (on the preparation
> side) and notice (on the application side) a pair of patch files at
> the end that represent the index state and the working tree state
> and apply them without making the WIP part into a commit.

Ugh, no.  I don't want to leak the implementation detail of autostash
into specific rebases.  Why can't I wrap the last statment in
git-rebase.sh in git stash/ git stash pop like I did with git-pull.sh?

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

[John Keeping]

On Mon, Apr 15, 2013 at 11:38:20PM +0530, Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:
> > If "rebase -m" were to be taught to do this, the natural way to do
> > so is to
> >
> >   (1) Prepare the todo the usual way
> >   (2) Do those two commits for index and working tree
> >   (3) Append two insns (exec reset HEAD^ and exec reset --soft
> >       HEAD^) at the end of the rebase todo file.
> 
> Er, no.  I don't want to touch the instruction sheet.  It becomes
> especially problematic in -i, when the instruction sheet is
> user-editable.
> 
> > "rebase--am" could also be told to generate (on the preparation
> > side) and notice (on the application side) a pair of patch files at
> > the end that represent the index state and the working tree state
> > and apply them without making the WIP part into a commit.
> 
> Ugh, no.  I don't want to leak the implementation detail of autostash
> into specific rebases.  Why can't I wrap the last statment in
> git-rebase.sh in git stash/ git stash pop like I did with git-pull.sh?

How does that work with the following:

- run_specific_rebase fails, so the user needs to fix it up and then run
  "git rebase --continue".  We don't want to pop the stash in this case.

- the user runs "git rebase --continue" with staged changes, knowing the
  git-rebase will commit those.  We don't want to create a stash in this
  case since it will remove the changes the user wants to commit.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> If "rebase -m" were to be taught to do this, the natural way to do
>> so is to
>>
>>   (1) Prepare the todo the usual way
>>   (2) Do those two commits for index and working tree
>>   (3) Append two insns (exec reset HEAD^ and exec reset --soft
>>       HEAD^) at the end of the rebase todo file.
>
> Er, no.  I don't want to touch the instruction sheet.  It becomes
> especially problematic in -i, when the instruction sheet is
> user-editable.

I do not find this problematic. It shows the user what's going on. It
may be a good idea to append the last instructions after launching the
editor if we want to partially hide it (but it's still going to be
visible with rebase --edit-todo)

>> "rebase--am" could also be told to generate (on the preparation
>> side) and notice (on the application side) a pair of patch files at
>> the end that represent the index state and the working tree state
>> and apply them without making the WIP part into a commit.
>
> Ugh, no.  I don't want to leak the implementation detail of autostash
> into specific rebases.  Why can't I wrap the last statment in
> git-rebase.sh in git stash/ git stash pop like I did with git-pull.sh?

Because "git rebase" needs multiple runs in case of conflicts. You have
to store the information somewhere in the filesystem, not in a variable.
What you need to store is whether you need to unstash, and where you are
in the process (in Junio's version, you may be doing the actual rebase,
or fixing conflicts in index state application, or fixing conflicts in
tree state application, or done). Storing what you have to do and where
you are in the process really sounds like a job for the instruction
sheet, no?

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

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> Because "git rebase" needs multiple runs in case of conflicts. You have
> to store the information somewhere in the filesystem, not in a variable.
> What you need to store is whether you need to unstash, and where you are
> in the process (in Junio's version, you may be doing the actual rebase,
> or fixing conflicts in index state application, or fixing conflicts in
> tree state application, or done). Storing what you have to do and where
> you are in the process really sounds like a job for the instruction
> sheet, no?

No.  Ultimately, the entry point of all these invocations is
git-rebase.sh.  The plan is to refactor calls from git-rebase.sh to
git-rebase--*.sh scripts so that those scripts return control to
git-rebase.sh, which will be the final exit point.  The logic is very
simple: On the very first invocation of rebase (ie. no existing rebase
in progress), stash.  If the return statement from the specific rebase
script is 1 (which means that there are conflicts to be resolved),
exit as usual.  If it is 0 (which means that the rebase completely
successfully), pop the stash before exiting as usual.

What's so complicated about that?  I'm against leaking the autostash
implementation detail into specific rebases, because I value a clean
and pleasant implementation over everything else.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> If it is 0 (which means that the rebase completely successfully), pop
> the stash before exiting as usual.

And you're going to pop the stash even if no stash were triggered when
starting the rebase.

Really, think about it again: you're not going to guess whether you have
to unstash without storing this information somewhere. You may argue
about storing it outside the todolist, you can't unstash
unconditionally.

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

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

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> And you're going to pop the stash even if no stash were triggered when
> starting the rebase.
>
> Really, think about it again: you're not going to guess whether you have
> to unstash without storing this information somewhere. You may argue
> about storing it outside the todolist, you can't unstash
> unconditionally.

Yes, touching a simple "autostash" file at stash-time, and removing it
at pop-time will do.  I don't see why it should be part of a
(potentially user-editable) todo-list, which serves an entirely
different purpose.

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

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Yes, touching a simple "autostash" file at stash-time, and removing it
> at pop-time will do.  I don't see why it should be part of a
> (potentially user-editable) todo-list, which serves an entirely
> different purpose.

You'll have to apply the index state and then the tree state. How do you
know whether the next call to "git rebase" should apply one or the other?

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

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

[Matthieu Moy]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Yes, touching a simple "autostash" file at stash-time, and removing it
>> at pop-time will do.  I don't see why it should be part of a
>> (potentially user-editable) todo-list, which serves an entirely
>> different purpose.
>
> You'll have to apply the index state and then the tree state. How do you
> know whether the next call to "git rebase" should apply one or the other?

Plus: what happens if the user ran "git stash" during rebase? In Junio's
version, the right commits are applied. Running blindly "stash pop" may
pop the wrong stash.

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> If it is 0 (which means that the rebase completely successfully), pop
>> the stash before exiting as usual.
>
> And you're going to pop the stash even if no stash were triggered when
> starting the rebase.
>
> Really, think about it again: you're not going to guess whether you have
> to unstash without storing this information somewhere. You may argue
> about storing it outside the todolist, you can't unstash
> unconditionally.

Yes, it can be part of todo, but it does not have to be.

Regardless of where the information is stored, implementing the last
step as "stash pop" will add a small inconsistency the end user
needs to be aware of.

Imagine "git rebase" stops, asks you to help with conflicts, and you
look at it, play with the working tree, and made a mess.  If this
was the last "stash pop" invocation, the way to go back and start
again is "reset --hard && stash pop".  For all the other steps, that
is not how the user goes back to the originally conflicted state in
order to retry from scratch.

Makes me wonder if the world were a better place if the rebase-todo
list were implemented as a dedicated stash and "rebase --continue"
were a mere "stash pop" followed by a commit if pop goes smoothly.

I am not suggesting to actually do so, but it is an intriguing
thought from the perspective of end user experience, isn't it?

In any case, I am not saying that it is a _bad_ thing to implement
the last "restore the WIP" stage as "stash pop". I am just pointing
out that the messaging needs to be done carefully to make sure the
user understands which one is which, as the way to deal with the
situation where it stops and asks the user for help would be
different from normal step in the rebase sequence that replays a
commit.

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

[Phil Hord]

On Tue, Apr 16, 2013 at 5:20 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Matthieu Moy wrote:
> No.  Ultimately, the entry point of all these invocations is
> git-rebase.sh.  The plan is to refactor calls from git-rebase.sh to
> git-rebase--*.sh scripts so that those scripts return control to
> git-rebase.sh, which will be the final exit point.  The logic is very
> simple: On the very first invocation of rebase (ie. no existing rebase
> in progress), stash.  If the return statement from the specific rebase
> script is 1 (which means that there are conflicts to be resolved),
> exit as usual.  If it is 0 (which means that the rebase completely
> successfully), pop the stash before exiting as usual.
>
> What's so complicated about that?  I'm against leaking the autostash
> implementation detail into specific rebases, because I value a clean
> and pleasant implementation over everything else.

It can be more complex than you realize.

   $ git pull --rebase --stash

    It seems that there is already a .git/rebase-apply directory, and
    I wonder if you are in the middle of another rebase.  If that is the
    case, please try
            git rebase (--continue | --abort | --skip)
    If that is not the case, please
            rm -fr .git/rebase-apply
    and run me again.  I am stopping in case you still have something
    valuable there.

If I follow the latter advice about 'rm -rf', who will remember that
'rebase' had something stashed, and what will they do with it when
they do?

What if it is weeks or months later?  I would be surprised to see
long-forgotten wip show up in my workspace all of a sudden.

Phil

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

[Ramkumar Ramachandra]

Phil Hord wrote:
> If I follow the latter advice about 'rm -rf', who will remember that
> 'rebase' had something stashed, and what will they do with it when
> they do?
>
> What if it is weeks or months later?  I would be surprised to see
> long-forgotten wip show up in my workspace all of a sudden.

Ultimately, I think an ideal implementation requires this entire
autostash implementation to reside completely within the $state_dir.
The issue of a long-forgotten WIP is then the same issue as a
long-forgotten rebase, and a rm -rf $state_dir will get rid of the WIP
as well.

The other reason is that it shouldn't interact with the rest of git.
This means no touching any refs or reflogs, and this can be
problematic if we decide to use the standard git stash.  I'm currently
working towards seeing if it's possible to get stash to create "named
stashes" that we can predictably retrieve later, to avoid rolling our
own homegrown stash.

Yes, the problem is much more complex than I initially thought.  It
was much simpler to implement it for git-pull.sh.