Re: [PATCH v2 2/3] read-cache.c: Optionally tolerate missing files in make_cache_entry

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2014, 20:39 UTC
Message-ID: <xmqqr47xqflf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <197b46e792c8640d19580f282834052a660d2c05.1390592626.git.brad.king@kitware.com>

Brad King <brad.king@kitware.com> writes:

> +extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh, int refresh_flags);

Why a new parameter? If refresh_flags can be ANY when refresh=NoThanks, shouldn't they be a single variable that tells the callee how the entry should be refreshed (e.g. "not at all", "normally", "missing is ok", etc.)?

Show 12 quoted lines

> +static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really,
> +					       int flags)
>  {
> -	return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
> +	int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
> +	int cache_errno = 0;
> +	struct cache_entry *new;
> +
> +	new = refresh_cache_ent(&the_index, ce, really, &cache_errno, NULL);
> +
> +	if(!new && not_new && cache_errno == ENOENT)
> +		return ce;

I think this is still one level too high in the abstraction chain. "int really" might be of type signed int by historical accidents, but it is "unsigned int options" for the underlying refresh_cache_ent(). I'd suggest renaming this to "unsigned int refresh_options" or something, and then define a new constatnt similar to the existing CE_MATCH_IGNORE_*.