On Sun, 29 Jan 2023 at 20:17, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "M Hickford via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: M Hickford <mirth.hickford@xxxxxxxxx>
> >
> > 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.
>
> It is unclear what you are describing in the above. The current
> behaviour without the patch? The behaviour of the code if this
> patch gets applied? Write it in such a way that it is clear why
> the patch is a good idea, not just "this would not hurt because it
> is backwards compatible".
>
> The usual way to do so is to sell your change in this order:
>
> - Give background information to help readers understand what you
> are going to write in the following explanation.
>
> - Describe the current behaviour without any change to the code;
>
> - Present a situation where the current code results in an
> undesirable outcome. What exactly happens, what visible effect it
> has to the user, how the code could do better to help the user?
>
> - Propose an updated behaviour that would behave better in the
> above sample situation presented.
>
Thanks for the guidance. Writing a better commit message clarified my
own thoughts.
> Curiously, what you wrote below the "---" line, that will not be
> part of the log message, looks to be organized better than the
> above. The first paragraph (except for the "Add ...") prepares the
> readers, It is still unclear if the second paragraph "when expired"
> describes what happens with the current code (i.e. highlighting why
> a change is needed) or what you want to happen with the patch, but
> the paragraph should first explain the problem in the current
> behaviour to motivate readers to learn why the updated code would
> lead to a better world. And follow that with the behaviour of the
> updated code and its effect (e.g. "without first trying a credential
> that is stale and see it fail before asking to reauthenticate, such
> a known-to-be-stale credential gets discarded automatically").
>
>
> > +`password_expiry_utc`::
> > +
> > + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970.
> > +
>
> A overly long line. Please follow Documentation/CodingGuidelines
> and Documentation/SubmittingPatches
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index f3c89831d4a..5cb8a186b45 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
> > if (e) {
> > fprintf(out, "username=%s\n", e->item.username);
> > fprintf(out, "password=%s\n", e->item.password);
> > + if (e->item.password_expiry_utc != 0) {
> > + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc);
> > + }
>
> Style (multiple issues, check CodingGuidelines):
>
> if (e->item.password_expiry_utc)
> fprintf(out, "... overly long format template ...",
> e->item.password_expiry_utc);
>
> * Using integral value or pointer value as a truth value does not
> require an explicit comparison with 0;
>
> * A single-statement block does not need {} around it;
>
> * Overly long line should be folded, with properly indented.
>
> > diff --git a/credential.c b/credential.c
> > index f6389a50684..0a3a9cbf0a2 100644
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -7,6 +7,7 @@
> > #include "prompt.h"
> > #include "sigchain.h"
> > #include "urlmatch.h"
> > +#include <time.h>
>
> Don't include system headers directly; often git-compat-util.h
> already has it, and if not, we need to find the right place to have
> it in git-compat-util.h file, as there are platforms that are
> finicky in inclusion order of the header files and definition of
> feature macros.
>
> > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c)
> > free(c->path);
> > free(c->username);
> > free(c->password);
> > + c->password_expiry_utc = 0;
>
> Not a huge deal, but if the rule is "an credential with expiry
> timestamp that is too old behaves as if it no longer exists or is
> valid", then a large integer, not zero, may serve as a better
> sentinel value for "this entry never expires". Instead of having to
> do
>
> if (expiry && expiry < time()) {
> ... expired ...
> }
>
> you can just do
>
> if (expiry < time()) {
> ... expired ...
> }
>
> and that would be simpler to understand for human readers, too.
>
> > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)
> > } else if (!strcmp(key, "path")) {
> > free(c->path);
> > c->path = xstrdup(value);
> > + } else if (!strcmp(key, "password_expiry_utc")) {
> > + // TODO: ignore if can't parse integer
>
> Do not use // comment. /* Our single-liner comment reads like this */
>
> > + c->password_expiry_utc = atoi(value);
>
> Don't use atoi(); make sure value is not followed by a non-number,
> e.g.
>
> const char *value = "43q";
> printf("%d<%s>\n", atoi(value), value);
>
> would give you 43<43q>, but you want to reject and silently ignore
> such an expiry timestamp.
>
> > + // if expiry date has passed, ignore password and expiry fields
>
> Ditto, but if you used a large value as sentinel for "never expires"
> and wrote it like this
>
> if (c->password_expiry_utc < time(NULL)) {
>
> then it is clear enough that you do not even need such a comment.
> The expression itself makes it clear what is going on (i.e. the
> current time comes later than the expiry_utc value on the number
> line hence it appears on the right to it, clearly showing that it
> has passed the threshold).
>
> > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {
> > + trace_printf(_("Password has expired.\n"));
> > + FREE_AND_NULL(c->username);
> > + FREE_AND_NULL(c->password);
> > + c->password_expiry_utc = 0;
> > + }
> > +
