Re: [PATCH 15/25] config: add git_config_get_timestamp()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 11, 2022, 13:32 UTC
Message-ID: <220311.86pmmshahy.gmgdl@evledraar.gmail.com>
In-Reply-To: <7ec7ba3f328f9d6fa7f919ce18a138840d44ee49.1645641063.git.gitgitgadget@gmail.com>

On Wed, Feb 23 2022, Derrick Stolee via GitGitGadget wrote:

Show 5 quoted lines

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

Show 5 quoted lines

> [...]
> +		*dest = git_config_timestamp(key, value);
> +		return 0;
> +	} else
> +		return 1;

Nit: missing {}

Show 49 quoted lines

>[...]
> --- 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 :)