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
