git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Jeff King <peff@peff.net>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset
Date: Mon, 20 Mar 2017 15:08:43 -0400
Message-ID: <20170320190843.aw3fsenjvcek7a44@sigill.intra.peff.net> (raw)
In-Reply-To: <20170319202351.8825-4-t.gummerer@gmail.com>

On Sun, Mar 19, 2017 at 08:23:51PM +0000, Thomas Gummerer wrote:

> For "git stash -p --no-keep-index", the pathspec argument is currently
> not passed to "git reset".  This means that changed that are staged but
> that are excluded from the pathspec still get unstaged by git stash -p.

Yeah, I noticed this while playing around with patch 2. This seems
like an improvement to me. Unlike the other patches (which are just
tweaking quietness), I think this one really needs a test.

Also, s/changed/changes/ above.

> ---
> So this make things a bit inconsistent in for example the following
> situation:
> 
>     $ git status -s
>      M git-stash.sh
>     M  read-cache.c
> 
> where using git stash -p --no-keep-index, when only changes in
> git-stash.sh are added to the stash would reset read-cache.c, but with
> git stash -p --no-keep-index -- git-stash.sh, read-cache.c would not
> be reset.

I think it's OK. You can't select (or not select) changes from the index
anyway. TBH, I kind of wonder if "git stash -p --no-keep-index" makes
any sense at all.

> However it does bring things more in line with git stash --
> <pathspec>, so I think this is an improvement.

I did notice one other case while looking at this that I think is much
more serious. The "read-tree" call in the non-patch-mode case doesn't
use a pathspec either. So if we have our same setup where "one" and
"two" have unstaged changes and we do:

  git stash push -k one

Then we stash "one", but the change in "two" is wiped out completely!

I don't think read-tree takes pathspecs, so it would have to drop the
"-u" and replace it with checkout-index. I'm confused about why we would
need it in the first place, though. We should have already dealt with
the working tree files in the earlier block, so there should be nothing
to checkout.

The "-u" goes all the way back to 7bedebcaa (stash: introduce 'stash
save --keep-index' option, 2008-06-27). I wonder if it has always been
unnecessary, but we never noticed because it's just a noop.

-Peff

  reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-17 14:50 [BUG] "git stash -- path" reports wrong unstaged changes Jeff King
2017-03-18 18:36 ` Thomas Gummerer
2017-03-19 20:23   ` Thomas Gummerer
2017-03-19 20:23     ` [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec> Thomas Gummerer
2017-03-20 17:51       ` Junio C Hamano
2017-03-20 18:48         ` Jeff King
2017-03-20 19:42           ` Junio C Hamano
2017-03-21 20:48             ` Thomas Gummerer
2017-03-20 18:42       ` Jeff King
2017-03-19 20:23     ` [PATCH/RFC 2/3] stash: make push -p -q --no-keep-index quiet Thomas Gummerer
2017-03-20 18:55       ` Jeff King
2017-03-19 20:23     ` [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset Thomas Gummerer
2017-03-20 19:08       ` Jeff King [this message]
2017-03-21 21:14         ` Thomas Gummerer
2017-03-21 22:12           ` Jeff King
2017-03-21 22:12     ` [PATCH v2 0/3] Re: [BUG] "git stash -- path" reports wrong unstaged changes Thomas Gummerer
2017-03-21 22:12       ` [PATCH v2 1/3] stash: don't show internal implementation details Thomas Gummerer
2017-03-21 22:14         ` Jeff King
2017-03-22 21:33           ` Thomas Gummerer
2017-03-22 21:41             ` Jeff King
2017-03-21 22:12       ` [PATCH v2 2/3] stash: pass the pathspec argument to git reset Thomas Gummerer
2017-03-21 22:19         ` Jeff King
2017-03-21 22:12       ` [PATCH v2 3/3] stash: keep untracked files intact in stash -k Thomas Gummerer
2017-03-21 22:38         ` Jeff King
2017-03-22 21:46           ` Thomas Gummerer
2017-03-20 18:41   ` [BUG] "git stash -- path" reports wrong unstaged changes Jeff King

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170320190843.aw3fsenjvcek7a44@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=t.gummerer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox