On Tue, Aug 2, 2011 at 5:59 PM, Junio C Hamano <gitster@xxxxxxxxx> 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 <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html