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

[Jeff King <peff@peff.net>]

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

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

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