git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Thomas Gummerer <t.gummerer@gmail.com>
Date
Mar 21, 2018, 21:36 UTC
Message-ID
<20180321213626.GG2224@hank>
In-Reply-To
<xmqqzi32sn5u.fsf@gitster-ct.c.googlers.com>
On 03/20, Junio C Hamano wrote:
Show 15 quoted lines
> 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.

Show 12 quoted lines
> > 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.

Show 33 quoted lines
> >  		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?

Show 11 quoted lines
> 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.

Show 23 quoted lines
> [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.
Previous: Junio C HamanoNext: Thomas Gummerer
Message 26 of 33 in “git stash push -u always warns "pathspec '...' did not match any files"”
  1. Marc StrapetzMar 3, 2018
  2. Thomas GummererMar 3, 2018
  3. Marc StrapetzMar 4, 2018
  4. Junio C HamanoMar 9, 2018
  5. Marc StrapetzMar 10, 2018
  6. Thomas GummererMar 10, 2018
  7. 1/2 stash push: avoid printing errorsThomas Gummerer, Mar 14, 2018
  8. 2/2 stash push -u: don't create empty stashThomas Gummerer, Mar 14, 2018
  9. Junio C HamanoMar 15, 2018
  10. Thomas GummererMar 16, 2018
  11. Marc StrapetzMar 15, 2018
  12. Thomas GummererMar 16, 2018
  13. 0/2 stash push -u -- <pathspec> fixesThomas Gummerer, Mar 16, 2018
  14. 2/2 stash push -u: don't create empty stashThomas Gummerer, Mar 16, 2018
  15. 1/2 stash push: avoid printing errorsThomas Gummerer, Mar 16, 2018
  16. Junio C HamanoMar 16, 2018
  17. 0/3 stash push -u -- <pathspec> fixesJunio C Hamano, Mar 16, 2018
  18. 2/3 stash push: avoid printing errorsJunio C Hamano, Mar 16, 2018
  19. 1/3 stash: fix nonsense pipelineJunio C Hamano, Mar 16, 2018
  20. 3/3 stash push -u: don't create empty stashJunio C Hamano, Mar 16, 2018
  21. Thomas GummererMar 17, 2018
  22. 0/3 stash push -u -- <pathspec> fixesThomas Gummerer, Mar 19, 2018
  23. 1/3 stash: fix nonsense pipelineThomas Gummerer, Mar 19, 2018
  24. 2/3 stash push: avoid printing errorsThomas Gummerer, Mar 19, 2018
  25. Junio C HamanoMar 20, 2018
  26. Thomas GummererMar 21, 2018
  27. stash: drop superfluos pathspec parameter (was: Re: [PATCH v5 2/3] stash push: avoid printing errors)Thomas Gummerer, Mar 21, 2018
  28. Junio C HamanoMar 21, 2018
  29. Junio C HamanoMar 21, 2018
  30. 3/3 stash push -u: don't create empty stashThomas Gummerer, Mar 19, 2018
  31. Marc StrapetzMar 20, 2018
  32. Marc StrapetzMar 19, 2018
  33. Thomas GummererMar 19, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.