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

Ramkumar Ramachandra [off-list ref] 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.

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.