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

From: Jeff King <peff@peff.net>
Date: Aug 1, 2011, 03:51 UTC
Message-ID: <20110801035153.GA2207@sigill.intra.peff.net>
In-Reply-To: <1312163561-77072-1-git-send-email-allan.caffee@gmail.com>

On Sun, Jul 31, 2011 at 09:52:41PM -0400, Allan Caffee wrote:

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

  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.

  3. Is there any reason not to use the multi-threaded
     read_cache_preload here?

-Peff