From: Jeff King <peff@peff.net>
To: "Gábor Szeder" <szeder@ira.uka.de>
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
git@vger.kernel.org, Ronnie Sahlberg <sahlberg@google.com>
Subject: Re: [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR
Date: Mon, 25 Aug 2014 08:46:31 -0400 [thread overview]
Message-ID: <20140825124631.GC17288@peff.net> (raw)
In-Reply-To: <E1XLXkh-0002sL-IJ@iramx2.ira.uni-karlsruhe.de>
On Sun, Aug 24, 2014 at 08:22:41PM +0700, Gábor Szeder wrote:
> On Aug 23, 2014 12:26 PM, Jeff King <peff@peff.net> wrote:
> > Since dd0b72c (bash prompt: use bash builtins to check stash
> > state, 2011-04-01), git-prompt checks whether we have a
> > stash by looking for $GIT_DIR/refs/stash. Generally external
> > programs should never do this, because they would miss
> > packed-refs.
>
> Not sure whether the prompt script is external program or not, but
> doesn't matter, this is the right thing to do.
Yeah, by external I just meant "nothing outside of refs.c should make
this assumption".
> > That commit claims that packed-refs does not pack
> > refs/stash, but that is not quite true. It does pack the
> > ref, but due to a bug, fails to prune the ref. When we fix
> > that bug, we would want to be doing the right thing here.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > I know we are pretty sensitive to forks in the prompt code (after all,
> > that was the point of dd0b72c). This patch is essentially a reversion of
> > this hunk of dd0b72c, and is definitely safe.
>
> I'm not sure, but if I remember correctly (don't have the means to
> check it at the moment, sorry) in that commit I also added a 'git
> pack-ref' invocation to the relevant test(s?) to guard us against
> breakages due to changes in 'git pack-refs'. If that is so, then I
> think those invocations should be removed as well, as they'll become
> useless.
It did add that change (that's actually how I noticed the problem!
Thank you for being thorough in dd0b72c). My inclination is to leave the
pack-refs invocations, as they protect against a certain class of errors
(we are not doing the risky behavior now, but the purpose of the test
suite is to detect regressions; the next person to touch that code may
not be so careful as you were).
I don't feel too strongly, though, so if we want them gone, I'm OK with
that.
-Peff
next prev parent reply other threads:[~2014-08-25 12:46 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-24 13:22 [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR Gábor Szeder
2014-08-25 12:46 ` Jeff King [this message]
-- strict thread matches above, loose matches on Subject: below --
2014-08-23 5:23 [PATCH 0/5] fix pack-refs pruning of refs/foo Jeff King
2014-08-23 5:26 ` [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR Jeff King
Reply instructions:
You may reply publicly 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140825124631.GC17288@peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=mhagger@alum.mit.edu \
--cc=sahlberg@google.com \
--cc=szeder@ira.uka.de \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.