{"thread":{"id":"37409","subject":"Re: [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR","startedAt":"2014-08-24T13:22:41Z","lastAt":"2014-08-25T12:46:31Z","messageCount":2,"participants":["Gábor Szeder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"248201","messageId":"E1XLXkh-0002sL-IJ@iramx2.ira.uni-karlsruhe.de","threadId":"37409","inReplyTo":null,"subject":"Re: [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR","fromName":"Gábor Szeder","fromEmail":"szeder@ira.uka.de","sentAt":"2014-08-24T13:22:41Z","receivedAt":"2014-08-24T13:22:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Aug 23, 2014 12:26 PM, Jeff King <peff@peff.net> wrote:\n> Since dd0b72c (bash prompt: use bash builtins to check stash \n> state, 2011-04-01), git-prompt checks whether we have a \n> stash by looking for $GIT_DIR/refs/stash. Generally external \n> programs should never do this, because they would miss \n> packed-refs.\n\nNot sure whether the prompt script is external program or not, but doesn't matter, this is the right thing to do.\n\n> That commit claims that packed-refs does not pack \n> refs/stash, but that is not quite true. It does pack the \n> ref, but due to a bug, fails to prune the ref. When we fix \n> that bug, we would want to be doing the right thing here. \n>\n> Signed-off-by: Jeff King <peff@peff.net> \n> --- \n> I know we are pretty sensitive to forks in the prompt code (after all, \n> that was the point of dd0b72c). This patch is essentially a reversion of \n> this hunk of dd0b72c, and is definitely safe.\n\nI'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.\n\nBest,\nGábor"},{"id":"248224","messageId":"20140825124631.GC17288@peff.net","threadId":"37409","inReplyTo":"E1XLXkh-0002sL-IJ@iramx2.ira.uni-karlsruhe.de","subject":"Re: [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T12:46:31Z","receivedAt":"2014-08-25T12:46:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 24, 2014 at 08:22:41PM +0700, Gábor Szeder wrote:\n\n> On Aug 23, 2014 12:26 PM, Jeff King <peff@peff.net> wrote:\n> > Since dd0b72c (bash prompt: use bash builtins to check stash \n> > state, 2011-04-01), git-prompt checks whether we have a \n> > stash by looking for $GIT_DIR/refs/stash. Generally external \n> > programs should never do this, because they would miss \n> > packed-refs.\n> \n> Not sure whether the prompt script is external program or not, but\n> doesn't matter, this is the right thing to do.\n\nYeah, by external I just meant \"nothing outside of refs.c should make\nthis assumption\".\n\n> > That commit claims that packed-refs does not pack \n> > refs/stash, but that is not quite true. It does pack the \n> > ref, but due to a bug, fails to prune the ref. When we fix \n> > that bug, we would want to be doing the right thing here. \n> >\n> > Signed-off-by: Jeff King <peff@peff.net> \n> > --- \n> > I know we are pretty sensitive to forks in the prompt code (after all, \n> > that was the point of dd0b72c). This patch is essentially a reversion of \n> > this hunk of dd0b72c, and is definitely safe.\n> \n> I'm not sure, but if I remember correctly (don't have the means to\n> check it at the moment, sorry) in that commit I also added a 'git\n> pack-ref' invocation to the relevant test(s?) to guard us against\n> breakages due to changes in 'git pack-refs'.  If that is so, then I\n> think those invocations should be removed as well, as they'll become\n> useless.\n\nIt did add that change (that's actually how I noticed the problem!\nThank you for being thorough in dd0b72c). My inclination is to leave the\npack-refs invocations, as they protect against a certain class of errors\n(we are not doing the risky behavior now, but the purpose of the test\nsuite is to detect regressions; the next person to touch that code may\nnot be so careful as you were).\n\nI don't feel too strongly, though, so if we want them gone, I'm OK with\nthat.\n\n-Peff\n"}]}