From: Allan Caffee Date: Tue, 02 Aug 2011 22:38:08 GMT Subject: Re: [PATCH] describe: Refresh the index when run with --dirty Message-ID: In-Reply-To: <7v1ux3eapk.fsf@alter.siamese.dyndns.org> On Tue, Aug 2, 2011 at 5:59 PM, Junio C Hamano wrote: > Thanks. > > Here is a minor fix-up on top, that can be squashed in a re-roll (if you > plan to do one). Thanks for the patch. I'll squash this into v2. On Sun, Jul 31, 2011 at 11:51 PM, Jeff King wrote: > On Sun, Jul 31, 2011 at 09:52:41PM -0400, Allan Caffee wrote: > >> When running git describe --dirty the index should be refreshed.  Previously >> the cached index would cause describe to think that the index was dirty when, >> in reality, it was just stale. >> >> The issue was exposed by python setuptools which hardlinks files into another >> directory when building a distribution. > > Overall, looks good to me. A few minor nits, though: > >> diff --git a/builtin/describe.c b/builtin/describe.c >> index 66fc291..792af76 100644 >> --- a/builtin/describe.c >> +++ b/builtin/describe.c >> @@ -24,6 +24,7 @@ static int longformat; >>  static int abbrev = -1; /* unspecified */ >>  static int max_candidates = 10; >>  static struct hash_table names; >> +static struct lock_file index_lock; /* real index */ > > This line was presumably copied straight from builtin/commit.c. You can > drop the "real index" comment here. Commit may deal with multiple > indices, which is what this comment was clarifying, but here it doesn't > make any sense. > >>  static int always; >> @@ -399,6 +400,7 @@ static void describe(const char *arg, int last_one) >>  int cmd_describe(int argc, const char **argv, const char *prefix) >>  { >>       int contains = 0; >> +     int fd; > > If a variable is only going to be used for one deep conditional, IMHO > it's nice to declare it inside the conditional block, so readers of the > code don't have to wonder under what conditions fd is valid. > >> +             if (dirty) { >> +                     read_cache(); >> +                     refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); >> +                     fd = hold_locked_index(&index_lock, 0); >> +                     if (0 <= fd) >> +                             update_index_if_able(&the_index, &index_lock); > > A few questions about this read_cache call: > >  1. Should this actually be: > >          if (read_cache() < 0) >                  die("unable to read cache"); > >     ? I notice that cmd_status also does not check the error code. But >     it seems like if we fail to read, we would then potentially write >     out a bogus index. Probably unlikely, as failure to read probably >     implies failure to write. It definitely seems like writing out a bogus index would be bad, but even if both the read *and* the write fail we would still be potentially mislabeling it as "dirty" if we failed to it in the first place. It seems like, since they explicitly requested --dirty, we ought to give up here since we can't accurately respond. >  2. Should the read and refresh happen while we hold the lock? >     Otherwise our read-modify-update is not atomic, and we risk >     overwriting another index writer. Again, cmd_status suffers from >     the same problem, so this is not something you are introducing. Yeah it definitely is a race condition as far as I can tell. Should this be changed in cmd_status (builtin/commit.c:1227-1232) as well? -- Allan