On 03/20, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxx/
>
> 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.
