Re: [PATCH] credential: new attribute password_expiry_utc

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> If password has expired, credential fill no longer returns early,
> so later helpers can generate a fresh credential. This is backwards
> compatible -- no change in behaviour with helpers that discard the
> expiry attribute. The expiry logic is entirely in the git credential
> layer; compatible helpers simply store and return the expiry
> attribute verbatim.
>
> Store new attribute in cache.
>
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>

Just a few comments in addition to those already provided by Junio...

> diff --git a/credential.c b/credential.c
> @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)
> +               // if expiry date has passed, ignore password and expiry fields
> +               if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {
> +                       trace_printf(_("Password has expired.\n"));

Using `_(...)` marks a string for localization, but doing so is
undesirable for debugging messages which are meant for the developer,
not the end user (and it creates extra work for translators). No
existing[1] trace_printf() calls in the codebase use `_(...)`.

[1]: Unfortunately, a couple examples exist in
Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be
removed.

> @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp)
> +       if (c->password_expiry_utc != 0) {
> +               int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc);
> +               char* str = malloc( length + 1 );

Style in this project is `char *str`, not `char* str`. Also, drop
spaces around function arguments:

    char *str = malloc(length + 1);

> +               snprintf( str, length + 1, "%ld", c->password_expiry_utc );

Same.

> +               credential_write_item(fp, "password_expiry_utc", str, 0);
> +               free(str);
> +       }

xstrfmt() from strbuf.h can help simplify this entire block:

    char *s = xstrfmt("%ld", c->password_expiry_utc);
    credential_write_item(fp, "password_expiry_utc", str, 0);
    free(s);