From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.2 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,RCVD_IN_SORBS_SPAM, RP_MATCHES_RCVD shortcircuit=no autolearn=no autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 331202095B for ; Sat, 18 Mar 2017 18:43:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751785AbdCRSn1 (ORCPT ); Sat, 18 Mar 2017 14:43:27 -0400 Received: from mail-wm0-f41.google.com ([74.125.82.41]:36048 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751780AbdCRSn0 (ORCPT ); Sat, 18 Mar 2017 14:43:26 -0400 Received: by mail-wm0-f41.google.com with SMTP id n11so36846465wma.1 for ; Sat, 18 Mar 2017 11:43:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=0gJ6KwLiCq8g7lUL+beZipnRA82paSmhe6B1AG1UYt8=; b=G0XBKLZbgxzS9SWOvAX0/mseQFohvS6l4O6pRCM7RK5M4RLzkP5kUyMRmnBHVGjXva DuZAKVcOkqF3eKFhVwWfyME37bZM1MmjJ3Aw/B7EgKm7MGZxJ6javGIvOzVVxiJ4wfWN Zod/mOBe/OljHfcc6aBjTJ9rcvP2E+6vWRD4HEAV4IUUlJePfnxQlXdNqrNdulLxgj15 H8CHwAmIxUqmaRmaom0EP6vGWU60b5lKRr3ZpL7KFbmdyvGHJ5+AWb0oqAUvdeusiOfD G/7k6rbqHvYDZsMl3CrIi4NO+Jt1+MCDH/K4eTBsbF2UFA05ohwpea562K9lGOWg3kNQ Jn6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=0gJ6KwLiCq8g7lUL+beZipnRA82paSmhe6B1AG1UYt8=; b=BcecQAbaCZGUl6CsSe2BcOui6X4/cBr1/dEJf5S+ueayqXQB8E3DGT62rIiG6RCd7d ePW6YIw5UHOPBSna/ZjliGtBATDJAlrLZ+uPXbDT7zi+w288NDviAU8D+N+2u+QzqlS2 T//YaKCmwfPENwrEHShYs5oJUuPk9oX+U0JRvs8wePRPDkdbA12CO5bjx1ERoVRfe+oE mVR2a1/IdlcIPsL8xLXG8mKhz0jVsC7DkQIklLw/8n2vLB8sDg3DZiusjoCx9MNtI5Zy dR7nrca+uO86RcozkGWQaGpZenj2K944Lweteb30tr+KzEyZZsztcy/YhlZKrkdfQfyZ FBEQ== X-Gm-Message-State: AFeK/H0B3yY5skq6SO6XEUvBmEWyX0xgOtlCkZw8ZeZvccsghkV855JSpLJx62QASHFF/w== X-Received: by 10.28.209.75 with SMTP id i72mr3221074wmg.31.1489862202788; Sat, 18 Mar 2017 11:36:42 -0700 (PDT) Received: from localhost ([2a02:c7f:c42b:f900:5e51:4fff:fee9:57af]) by smtp.gmail.com with ESMTPSA id 36sm14383390wrk.57.2017.03.18.11.36.41 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 18 Mar 2017 11:36:42 -0700 (PDT) Date: Sat, 18 Mar 2017 18:36:58 +0000 From: Thomas Gummerer To: Jeff King Cc: git@vger.kernel.org Subject: Re: [BUG] "git stash -- path" reports wrong unstaged changes Message-ID: <20170318183658.GC27158@hank> References: <20170317145039.dmcb3qyqbzfvtmgz@sigill.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170317145039.dmcb3qyqbzfvtmgz@sigill.intra.peff.net> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On 03/17, Jeff King wrote: > I used "git stash -- path" for the first time today and happened to > notice an oddity. If I run: > > git init -q repo > cd repo > > for i in one two; do > echo content >$i > git add $i > done > git commit -qm base > > for i in one two; do > echo change >$i > done > git stash -- one > > it says: > > Saved working directory and index state WIP on master: 20cfadf base > Unstaged changes after reset: > M one > M two > > Even though "one" no longer has unstaged changes. Yeah, this is clearly not right. Thanks for catching this before it got into any release. > If I run with GIT_TRACE=1, that message is generated by: > > git reset -- one > > which makes sense. At that stage we've just reset the index, but the > working tree file still has modifications. In the non-pathspec case we > run "git reset --hard", which takes care of the index and the working > tree. > > It's really "checkout-index" that cleans the working tree, but it > doesn't have porcelain finery like an "Unstaged changes" message. I > think the patch below would fix it, but I wonder if we can do it in a > way that doesn't involve calling diff-files twice. > > -Peff > > --- > diff --git a/git-stash.sh b/git-stash.sh > index 9c70662cc..9a4bb503a 100755 > --- a/git-stash.sh > +++ b/git-stash.sh > @@ -299,10 +299,15 @@ push_stash () { > then > if test $# != 0 > then > - git reset ${GIT_QUIET:+-q} -- "$@" > + git reset -q -- "$@" > git ls-files -z --modified -- "$@" | > git checkout-index -z --force --stdin > git clean --force ${GIT_QUIET:+-q} -d -- "$@" > + if test -z "$GIT_QUIET" && ! git diff-files --quiet > + then > + say "$(gettext "Unstaged changes after reset:")" > + git diff-files --name-status > + fi > else > git reset --hard ${GIT_QUIET:+-q} > fi This would mean the user gets something like in your case above: Unstaged changes after reset: M two As a user that doesn't know the internal implementation of push_stash, this would make me wonder why git stash would mention a file that is not provided as pathspec, but not the one that was provided in the pathspec argument. I think one option would be to to just keep quiet about the exact changes that git stash push makes, similar to what we do in the --include-untracked and in the -p case. The other option would be to find the files that are affected and print them, but that would probably be a bit too noisy especially in cases such as git stash push -- docs/*. Also from reading the code in the -p case, when --keep-index is given, the git reset there doesn't respect $GIT_QUIET at all, and also doesn't respect the pathspec argument, which seems like another bug. I can submit a patch series for those, but I won't get to it before tomorrow :)