Re: [PATCH v5 2/3] stash push: avoid printing errors

[Thomas Gummerer <t.gummerer@gmail.com>]

On 03/20, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > ...
> > Fix this by avoiding the 'git clean' if a pathspec is given, and use the
> > pipeline that's used for pathspec mode to get rid of the untracked files
> > as well.
> 
> That made me wonder if we can get rid of 'git clean' altogether by
> pretending that we saw a pathspec '.' that match everything when no
> pathspec is given---that way we only have to worry about a single
> codepath.
> 
> But I guess doing it this way can minimize potential damage.  Those
> who do not use pathspec when running "git stash" won't be affected
> even if this change had some bugs ;-)

Heh yeah, we found enough bugs in this code so far, so it's probably
best to leave the part that's working alone at least for now.

> > diff --git a/git-stash.sh b/git-stash.sh
> > index 4c92ec931f..5e06f96da5 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -308,14 +308,16 @@ push_stash () {
> >  	if test -z "$patch_mode"
> >  	then
> >  		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
> > -		if test -n "$untracked"
> > +		if test -n "$untracked" && test $# = 0
> >  		then
> >  			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"

Argh I just noticed we could drop the "$@" here, as this is no longer
the pathspec case.  It doesn't hurt anything, except it may be a bit
confusing when reading the code.

Although if we end up implementing 'git checkout --index <pathspec>',
we'd have to add it back, but we do have a test covering this case, so
there's no worries about forgetting to add it back.

> >  		fi
> >  
> >  		if test $# != 0
> >  		then
> > -			git add -u -- "$@"
> > +			test -z "$untracked" && UPDATE_OPTION="-u" || UPDATE_OPTION=
> > +			test "$untracked" = "all" && FORCE_OPTION="--force" || FORCE_OPTION=
> > +			git add $UPDATE_OPTION $FORCE_OPTION -- "$@"
> >  			git diff-index -p --cached --binary HEAD -- "$@" |
> >  			git apply --index -R
> >  		else
> 
> Thanks, I'll take the change as-is.
> 
> I however wonder if we restructure the code to
> 
> 	if test $# = 0
> 	then
> 		# no pathspec
> 		if test -n "$untracked"
> 		then
> 			git clean --force --quiet -d $CLEAN_OPTION -- "$@"
> 		fi
> 		git reset --hard -q
> 	else
> 		test -z "$untracked" && UPDATE=-u || UPDATE=
> 		test "$untracked" = all && FORCE=--force || FORCE=
> 		git add $UPDATE $FORCE-- "$@"
> 		git diff-index -p --cached --binary HEAD -- "$@" |
> 		git apply --index -R
> 	fi
> 
> it becomes easier to understand what is going on.

I like that code structure more than what I have now.  I see you
already merged what I had to next, and I like keeping the change small
now that we're in the rc period (assuming you want to get this into
2.17?)  Maybe we can restructure the code as a separate cleanup once
2.17 is out, so this has more time to cook in master and hopefully
we'd notice regressions before the next release?

> That way, once we have a plumbing command to help the else clause of
> the above, i.e. "git checkout --index <tree-ish> -- <pathspec>"
> [*1*], then we can lose the if/then/else and rewrite the whole "we
> have created stash, so it's time to get rid of local modifications
> to the paths that match the pathspec" code to:
> 
> 	if test "$untracked"
> 	then
> 		git clean --force --quiet -d $CLEAN_OPTION -- "$@"
> 	fi
> 	git checkout --index HEAD -- "$@"

Yeah, this would be nice to have.  I wanted to have a look at what it
would take to implement 'git checkout --{cached,index}', but I'm not
familiar with the checkout code at all, so it will probably be a while
until I can get around to do it. 

> [Footnote]
> cf. https://public-inbox.org/git/xmqq4loqplou.fsf@gitster.mtv.corp.google.com/
> 
> What we want in the case in the code in question when there is
> pathspec is "match the index entries and the working tree files to
> those that appear in a given tree for paths that match the given
> pathspec".  This is close to "git checkout <tree-ish> -- <pathspec>"
> but not quite.  Current "git checkout <tree-ish> -- <pathspec>" is
> an overlay operation in that paths that match <pathspec> but do not
> exist in <tree-ish> are *NOT* removed from the working tree.  We
> obviously cannot change the behaviour of the command.
> 
> But we can add an option to ask for the new behaviour.  In general,
> for an operation that affects the index and the working tree, we can
> have "--cached" mode that affects only the index without touching
> the working tree, and "--index" mode that affects both.
> 
> "git reset <tree-ish> -- <pathspec>", which is a UI mistake, is
> better spelled "git checkout --cached <tree-ish> -- <pathspec>".  We
> take paths that match <pathspec> from <tree-ish> and stuff into the
> index, and remove entries from the index for paths that do not exist
> in <tree-ish>.  And if we extend that to "--index" mode, that is
> exactly what we want to happen.