Re: [PATCH] credential: new attribute password_expiry_utc

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 30, 2023, 00:59 UTC
Message-ID: <CAPig+cQPLMrUKp0aqLCknSYCs5TAso-VSBYsQbGZ8g8wgY2Liw@mail.gmail.com>
In-Reply-To: <pull.1443.git.git.1674914650588.gitgitgadget@gmail.com>

On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 10 quoted lines

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

Show 5 quoted lines

> 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);