Re: [PATCH] describe: Refresh the index when run with --dirty

> 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.