Re: [PATCH] credential: new attribute password_expiry_utc

[M Hickford <mirth.hickford@gmail.com>]

Thanks Eric for the review

On Mon, 30 Jan 2023 at 00:59, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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 `_(...)`.

Done in patch v3.

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

Neat. Done in patch v3.