| Message ID | 7ec7ba3f328f9d6fa7f919ce18a138840d44ee49.1645641063.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bundle URIs | expand |
On Wed, Feb 23 2022, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> > > The existing config parsing methods do not include a way to consistently > parse timestamps across all platforms. Recall that "unsigned long" is > 32 bits on 64-bit Windows, so git_config_get_ulong() is insufficient. They do. I think you've just missed the slightly non-obviously-named expiry-date type: $ for t in $(date +%s) 2.days.ago 1.week.ago; do echo $t = $(git -c x.y=$t config --type expiry-date --get x.y); done 1647005519 = 1647005519 2.days.ago = 1646832719 1.week.ago = 1646400719 AFAICT there's no need for these methods, except perhaps as some wrapper for the existing git_config_expiry_date(), perhaps to have a flag passed down to only allow epochs, and not the human-readable format? But for the TOC proposal I don't see why it wouldn't allow the human-readable format. You can arguably shoot yourself in the foot with 2.days.ago, but you can also provide absolute times in that format. In any case, peeling out this patch from this series to fast-track it if you do need it sounds like a good idea, it would need the relevant bits of the CLI UX implemented & docs added. There's also tests that you'll find by grepping for "expiry-date" to extend. > RFC-QUESTION: Would this be better to use uintmax_t, which could be cast > to timestamp_t or other types more robust than "unsigned long"? I think it's certainly legitimate to re-visit that when we're talking about making it part of a network format. But I think we should just go for timestamp_t, it will break 32 bit clients around the year 2038, but I think that probably the intersection between clients who'll find this useful and those still using 32 bit systems then will be pretty much zero :) So I think the only thing we need for bundle-uri if it has a TOC with a timestamp is to have it die/fallback properly if it can't parse the values. > [...] > + *dest = git_config_timestamp(key, value); > + return 0; > + } else > + return 1; Nit: missing {} >[...] > --- a/config.h > +++ b/config.h > @@ -206,6 +206,7 @@ int config_with_options(config_fn_t fn, void *, > > int git_parse_ssize_t(const char *, ssize_t *); > int git_parse_ulong(const char *, unsigned long *); > +int git_parse_timestamp(const char *, timestamp_t *); > > /** > * Same as `git_config_bool`, except that it returns -1 on error rather > @@ -226,6 +227,11 @@ int64_t git_config_int64(const char *, const char *); > */ > unsigned long git_config_ulong(const char *, const char *); > > +/** > + * Identical to `git_config_int`, but for (unsigned) timestamps. > + */ > +timestamp_t git_config_timestamp(const char *name, const char *value); > + > ssize_t git_config_ssize_t(const char *, const char *); > > /** > @@ -469,6 +475,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest > int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest); > int git_configset_get_int(struct config_set *cs, const char *key, int *dest); > int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest); > +int git_configset_get_timestamp(struct config_set *cs, const char *key, timestamp_t *dest); > int git_configset_get_bool(struct config_set *cs, const char *key, int *dest); > int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest); > int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest); > @@ -489,6 +496,8 @@ int repo_config_get_int(struct repository *repo, > const char *key, int *dest); > int repo_config_get_ulong(struct repository *repo, > const char *key, unsigned long *dest); > +int repo_config_get_timestamp(struct repository *repo, > + const char *key, timestamp_t *dest); > int repo_config_get_bool(struct repository *repo, > const char *key, int *dest); > int repo_config_get_bool_or_int(struct repository *repo, > @@ -558,6 +567,11 @@ int git_config_get_int(const char *key, int *dest); > */ > int git_config_get_ulong(const char *key, unsigned long *dest); > > +/** > + * Similar to `git_config_get_int` but for (unsigned) timestamps. > + */ > +int git_config_get_timestamp(const char *key, timestamp_t *dest); > + Not your fault, but it's really tiresome that whenever we need to add stuff to this API it's this mostly copy/pasting of related methods. I had some recent-ish patches that did something similar & it was a chore :)
diff --git a/config.c b/config.c index e0c03d154c9..84021b7d504 100644 --- a/config.c +++ b/config.c @@ -1228,6 +1228,15 @@ int git_parse_ulong(const char *value, unsigned long *ret) return 1; } +int git_parse_timestamp(const char *value, timestamp_t *ret) +{ + uintmax_t tmp; + if (!git_parse_unsigned(value, &tmp, maximum_unsigned_value_of_type(timestamp_t))) + return 0; + *ret = tmp; + return 1; +} + int git_parse_ssize_t(const char *value, ssize_t *ret) { intmax_t tmp; @@ -1296,6 +1305,14 @@ unsigned long git_config_ulong(const char *name, const char *value) return ret; } +timestamp_t git_config_timestamp(const char *name, const char *value) +{ + timestamp_t ret; + if (!git_parse_timestamp(value, &ret)) + die_bad_number(name, value); + return ret; +} + ssize_t git_config_ssize_t(const char *name, const char *value) { ssize_t ret; @@ -2328,6 +2345,16 @@ int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned lon return 1; } +int git_configset_get_timestamp(struct config_set *cs, const char *key, timestamp_t *dest) +{ + const char *value; + if (!git_configset_get_value(cs, key, &value)) { + *dest = git_config_timestamp(key, value); + return 0; + } else + return 1; +} + int git_configset_get_bool(struct config_set *cs, const char *key, int *dest) { const char *value; @@ -2471,6 +2498,13 @@ int repo_config_get_ulong(struct repository *repo, return git_configset_get_ulong(repo->config, key, dest); } +int repo_config_get_timestamp(struct repository *repo, + const char *key, timestamp_t *dest) +{ + git_config_check_init(repo); + return git_configset_get_timestamp(repo->config, key, dest); +} + int repo_config_get_bool(struct repository *repo, const char *key, int *dest) { @@ -2544,6 +2578,11 @@ int git_config_get_ulong(const char *key, unsigned long *dest) return repo_config_get_ulong(the_repository, key, dest); } +int git_config_get_timestamp(const char *key, timestamp_t *dest) +{ + return repo_config_get_timestamp(the_repository, key, dest); +} + int git_config_get_bool(const char *key, int *dest) { return repo_config_get_bool(the_repository, key, dest); diff --git a/config.h b/config.h index ab0106d2875..a6e4d35da0a 100644 --- a/config.h +++ b/config.h @@ -206,6 +206,7 @@ int config_with_options(config_fn_t fn, void *, int git_parse_ssize_t(const char *, ssize_t *); int git_parse_ulong(const char *, unsigned long *); +int git_parse_timestamp(const char *, timestamp_t *); /** * Same as `git_config_bool`, except that it returns -1 on error rather @@ -226,6 +227,11 @@ int64_t git_config_int64(const char *, const char *); */ unsigned long git_config_ulong(const char *, const char *); +/** + * Identical to `git_config_int`, but for (unsigned) timestamps. + */ +timestamp_t git_config_timestamp(const char *name, const char *value); + ssize_t git_config_ssize_t(const char *, const char *); /** @@ -469,6 +475,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest); int git_configset_get_int(struct config_set *cs, const char *key, int *dest); int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest); +int git_configset_get_timestamp(struct config_set *cs, const char *key, timestamp_t *dest); int git_configset_get_bool(struct config_set *cs, const char *key, int *dest); int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest); int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest); @@ -489,6 +496,8 @@ int repo_config_get_int(struct repository *repo, const char *key, int *dest); int repo_config_get_ulong(struct repository *repo, const char *key, unsigned long *dest); +int repo_config_get_timestamp(struct repository *repo, + const char *key, timestamp_t *dest); int repo_config_get_bool(struct repository *repo, const char *key, int *dest); int repo_config_get_bool_or_int(struct repository *repo, @@ -558,6 +567,11 @@ int git_config_get_int(const char *key, int *dest); */ int git_config_get_ulong(const char *key, unsigned long *dest); +/** + * Similar to `git_config_get_int` but for (unsigned) timestamps. + */ +int git_config_get_timestamp(const char *key, timestamp_t *dest); + /** * Finds and parses the value into a boolean value, for the configuration * variable `key` respecting keywords like "true" and "false". Integer