Thanks Eric for the review
On Mon, 30 Jan 2023 at 00:59, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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.
