Re: [PATCH 02/24] config: repo_config_get_expiry()

From: Jeff King <peff@peff.net>
Date: Apr 10, 2024, 17:54 UTC
Message-ID: <20240410175432.GA2260545@coredump.intra.peff.net>
In-Reply-To: <21d8f9dc2b4ddc8ac3f4e8f6b21bfb762fc6ab77.1710972293.git.me@ttaylorr.com>

On Wed, Mar 20, 2024 at 06:05:05PM -0400, Taylor Blau wrote:

Show 9 quoted lines

> Introduce a `repo_config_get_expiry()` variant in the style of functions
> introduced by 3b256228a6 (config: read config from a repository object,
> 2017-06-22) to read a single value without requiring the git_config()
> callback-style approach.
> 
> This new function is similar to the existing implementation in
> `git_config_get_expiry()`, however it differs in that it fills out a
> `timestamp_t` value through a pointer, instead of merely checking and
> discarding the result (and returning it as a string).

The existing git_config_get_expiry() is a funny interface. That makes me wonder how its existing callers handle this issue. In most cases we are just grabbing values for git-gc to pass along as strings to sub-programs. The only other case resolves via approxidate immediately, in get_shared_index_expire_date(). Which sort of leaks the string, though it is technically stuffed away in a global that we never look at.

So I can see the value of an interface which just returns the parsed timestamp and handles the string itself. Weirdly we even have git_config_get_expiry_in_days() which works like that, but always scales the timestamp! So we could implement that in terms of this new function.

But...

Show 17 quoted lines

> +int repo_config_get_expiry(struct repository *repo,
> +			   const char *key, const char **dest)
> +{
> +	int ret;
> +
> +	git_config_check_init(repo);
> +
> +	ret = repo_config_get_string(repo, key, (char **)dest);
> +	if (ret)
> +		return ret;
> +	if (strcmp(*dest, "now")) {
> +		timestamp_t now = approxidate("now");
> +		if (approxidate(*dest) >= now)
> +			git_die_config(key, _("Invalid %s: '%s'"), key, *dest);
> +	}
> +	return ret;
> +}

...does this actually do what the commit message says? It looks identical to git_config_get_expiry() except that it takes a repository parameter?

> This function will gain its first caller in a subsequent commit to parse
> a "threshold" parameter for excluding too-recent commits from
> pseudo-merge groups.

So presumably you call approxidate() there in that new caller. Looks like that would be patch 10. But I don't see a call to the new function at all! It just uses git_config_expiry_date(), which does what you need (it doesn't use the configset, but it looks like you ended up doing a config callback approach anyway).

So can this patch be dropped?

-Peff