Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time

From: Jeff King <peff@peff.net>
Date: Apr 14, 2020, 19:55 UTC
Message-ID: <20200414195535.GA1879302@coredump.intra.peff.net>
In-Reply-To: <5e95d37d.1c69fb81.2b4ec.ce9f@mx.google.com>

On Tue, Apr 14, 2020 at 04:27:26PM +0200, luciano.rocha@booking.com wrote:

Show 11 quoted lines

> Update freshen_file() to use a NULL `times', semantically equivalent to
> the currently setup, with an explicit `actime' and `modtime' set to the
> "current time", but with the advantage that it works with other files
> not owned by the current user.
> 
> Fixes an issue on shared repos with a split index, where eventually a
> user's operation creates a shared index, and another user will later do
> an operation that will try to update its freshness, but will instead
> raise a warning:
>   $ git status
>   warning: could not freshen shared index '.git/sharedindex.bd736fa10e0519593fefdb2aec253534470865b2'

Thanks, this makes sense. And as a bonus, it's less code. ;)

I don't recall having any particular reason not to use NULL in the original (I may simply not have been aware it was an option), and I can't find any discussion either way.

One observation:

Show 7 quoted lines

>  static int freshen_file(const char *fn)
>  {
> -	struct utimbuf t;
> -	t.actime = t.modtime = time(NULL);
> -	return !utime(fn, &t);
> +	return !utime(fn, NULL);
>  }

The old code was setting the time based on the system time from time(). We've occasionally hit cases where the filesystem time and the system time do not match exactly (this might be true on an NFS mount, for example).

It's not clear to me whether utime(NULL) would be using the system or filesystem time in such a case. If the former, then there's no change in behavior. If the latter, I'd argue that it's probably an _improvement_, since we're simulating the case that we wrote a new file with a new mtime.

-Peff