| Message ID | 21d8f9dc2b4ddc8ac3f4e8f6b21bfb762fc6ab77.1710972293.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pack-bitmap: pseudo-merge reachability bitmaps | expand |
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
On Wed, Apr 10, 2024 at 01:54:32PM -0400, Jeff King wrote: > > 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? Wow, yes, definitely -- this patch can be absolutely dropped. I suspect what happened here is that this patch is a relic from before I introduced pseudo-merge.c::pseudo_merge_config(), which is a standard callback for `git_config()`. I *think* the reason for the change is that I wanted to use the parse_config_key() function to parse different named pseudo-merge groups separately. The idea of having multiple named pseudo-merge groups was introduced after I wrote this patch, and I suspect that I never realized that this patch could be dropped as a result. Thanks for catching, I can cleanly just drop this patch from the series and everything works as expected. Thanks, Taylor
diff --git a/config.c b/config.c index 3cfeb3d8bd9..8512da92273 100644 --- a/config.c +++ b/config.c @@ -2627,6 +2627,24 @@ int repo_config_get_pathname(struct repository *repo, return ret; } +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; +} + /* Read values into protected_config. */ static void read_protected_config(void) { diff --git a/config.h b/config.h index 5dba984f770..619db01bc27 100644 --- a/config.h +++ b/config.h @@ -576,6 +576,8 @@ int repo_config_get_maybe_bool(struct repository *repo, const char *key, int *dest); int repo_config_get_pathname(struct repository *repo, const char *key, const char **dest); +int repo_config_get_expiry(struct repository *repo, + const char *key, const char **dest); /* * Functions for reading protected config. By definition, protected
Callers interested in parsing an approxidate from configuration currently make use of the `git_config_get_expiry()` function via the standard `git_config()` callback. 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). 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. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- config.c | 18 ++++++++++++++++++ config.h | 2 ++ 2 files changed, 20 insertions(+)