* Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'
From: Eric Sunshine @ 2023-01-30 5:38 UTC (permalink / raw)
To: Teng Long; +Cc: avarab, git, tenglong.tl
In-Reply-To: <20230128115027.57250-1-tenglong.tl@alibaba-inc.com>
On Sat, Jan 28, 2023 at 6:50 AM Teng Long <dyroneteng@gmail.com> wrote:
> Eric Sunshine writes:
> > > Situation of note "removing" shouldn't happen in 'append_edit()',
> > > unless it's a bug. So, let's drop the unreachable "else" code
> > > in "append_edit()".
> > >
> > > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > > ---
> > > - } else {
> > > - fprintf(stderr, _("Removing note for object %s\n"),
> > > - oid_to_hex(&object));
> > > - remove_note(t, object.hash);
> > > - logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> > > + commit_notes(the_repository, t, logmsg);
> > > }
> > > - commit_notes(the_repository, t, logmsg);
> >
> > This change breaks removal of notes using "git notes edit". Prior to
> > this change, if you delete the content of a note using "git notes
> > edit", then the note is removed. Following this change, the note
> > remains, which contradicts documented[1] behavior.
>
> As the commit msg describes, the subcommands I understand should have clear
> responsibilities as possible (documentaion may have some effect in my mind). So,
> the removal opertion under "append subcommand" here is little wired to me, but
> your suggestion makes sense, this may have compatibility issues. Although I
> think it's weird that someone would use this in the presence of the remove
> subcommand, and my feeling is that this code is actually copied from the add
> method (introduced by 52694cdabbf68f19c8289416e7bb3bbef41d8d27), but I'm not
> sure.
>
> So, it's ok for me to drop this one.
It's unfortunate, perhaps, that the git-notes command-set is not
orthogonal, but it's another case of historic accretion. According to
the history, as originally implemented, git-notes only supported two
commands, "edit" and "show", and "edit" was responsible for _all_
mutation operations, including deletion. git-notes only grew a more
complete set of commands a couple years later.
At any rate, even though the original behaviors may not make as much
sense these days now that git-notes has a more complete set of
commands, we still need to be careful not to break existing workflows
and scripts (tooling). That's why it would be nice to beef up the
tests so that the existing behaviors don't get broken by changes such
as this patch wants to make. But, as mentioned earlier, the additional
tests probably shouldn't be part of this series, but rather can be
done as a separate series (by someone; it doesn't have to be you).
^ permalink raw reply
* Re: [PATCH] credential: new attribute password_expiry_utc
From: Eric Sunshine @ 2023-01-30 0:59 UTC (permalink / raw)
To: M Hickford via GitGitGadget; +Cc: git, M Hickford
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:
> 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);
^ permalink raw reply
* Re: [PATCH] credential: new attribute password_expiry_utc
From: Junio C Hamano @ 2023-01-29 20:17 UTC (permalink / raw)
To: M Hickford via GitGitGadget; +Cc: git, M Hickford
In-Reply-To: <pull.1443.git.git.1674914650588.gitgitgadget@gmail.com>
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: M Hickford <mirth.hickford@gmail.com>
>
> 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.
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;
> + }
> +
^ permalink raw reply
* Draft of Git Rev News edition 95
From: Christian Couder @ 2023-01-29 18:27 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen, Kaartic Sivaraam,
Taylor Blau, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, ZheNing Hu,
Michal Suchánek, Teng Long
Hi everyone!
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-95.md
Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:
https://github.com/git/git.github.io/issues/623
You can also reply to this email.
In general all kinds of contributions, for example proofreading,
suggestions for articles or links, help on the issues in GitHub,
volunteering for being interviewed and so on, are very much
appreciated.
I tried to Cc everyone who appears in this edition, but maybe I missed
some people, sorry about that.
Jakub, Markus, Kaartic and I plan to publish this edition on Tuesday
January 31st in the evening.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v3] Documentation: clarify that cache forgets credentials if the system restarts
From: Junio C Hamano @ 2023-01-29 17:26 UTC (permalink / raw)
To: Jeff King; +Cc: M Hickford via GitGitGadget, git, brian m. carlson, M Hickford
In-Reply-To: <Y9YWruI4CdbJ9Rjn@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Jan 28, 2023 at 08:13:34PM +0000, M Hickford via GitGitGadget wrote:
>
>> diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
>> index 432e159d952..f473994a864 100644
>> --- a/Documentation/git-credential-cache.txt
>> +++ b/Documentation/git-credential-cache.txt
>> @@ -14,10 +14,13 @@ git config credential.helper 'cache [<options>]'
>> DESCRIPTION
>> -----------
>>
>> -This command caches credentials in memory for use by future Git
>> -programs. The stored credentials never touch the disk, and are forgotten
>> -after a configurable timeout. The cache is accessible over a Unix
>> -domain socket, restricted to the current user by filesystem permissions.
>> +This command caches credentials for use by future Git programs.
>> +The stored credentials are kept in memory of the cache-daemon
>> +process (instead of written to a file) and are forgotten after a
>> +configurable timeout. Credentials are forgotten sooner if the
>> +cache-daemon dies, for example if the system restarts. The cache
>> +is accessible over a Unix domain socket, restricted to the current
>> +user by filesystem permissions.
>
> This version looks good to me.
Yup, it looks good to me, too. Thanks, all.
^ permalink raw reply
* Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Junio C Hamano @ 2023-01-29 17:15 UTC (permalink / raw)
To: Mathias Krause
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <adb5a43a-5081-4f60-d1ea-2a6511f858c0@grsecurity.net>
Mathias Krause <minipli@grsecurity.net> writes:
> ... While we might be able to compile the pattern and run it in
> interpreter mode, it'll likely have a *much* higher runtime.
> ...
> So this grep run eat up ~9.5 *hours* of CPU time. Do we really want to
> fall back to something like this for the pathological cases? ...Yeah, I
> don't think so either.
You may not, but I do not agree with you at all. The code should
not outsmart the user in such a case.
Even if the pattern the user came up with is impossible to grok for
a working JIT compiler, and it might be hard to grok for the
interpreter, what is the next step you recommend the user if you
refuse to fall back on the interprete? "Rewrite it to please the
JIT compiler"?
If that is the best pattern the user can produce to solve the
problem at hand, being able to give the user an answer in 9 hours is
much better than not being able to give anything at all.
Maybe after waiting for 5 minutes, the user gets bored and ^C, or
without killing it, open another terminal and try a different
patern, and in 9 hours, perhaps comes up with an equivalent (or
different but close enough) pattern that happens to run much faster,
at which time the user may kill the original one. In any of these
cases, by refusing to run, the code is not doing any service to the
user.
^ permalink raw reply
* Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Mathias Krause @ 2023-01-29 13:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <xmqqtu0b95oy.fsf@gitster.g>
On 27.01.23 19:46, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> What am I missing?
>>
>> Note that I've seen and recently re-read the discussion that leads to
>> https://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/
>>
>> I suspect that this auto-probe is related to solving "the user
>> thinks JIT is in use but because of failing JIT the user's pattern
>> is getting horrible performance" somehow. But I do not think a hard
>> failure is a good approach to help users in such a situation.
>
> I guess what I am saying is that the previous one that has been
> queued on 'seen' may be better. It should cover your original
> "SELinux and other mechanisms can render JIT unusable because they
> do not allow dynamic generation of code" use case.
It clearly does cover my use case but it has a bad impact on the runtime
of pathological patterns. But if you think that's not an issue, I'll
update the changelog of v1 accordingly and resent it.
Thanks,
Mathias
^ permalink raw reply
* Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Mathias Krause @ 2023-01-29 13:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <xmqq1qnfancf.fsf@gitster.g>
On 27.01.23 18:39, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Yes, the "instead of failing hard, fall back" makes sense. Just
>> that I do not see why the runtime test is a good thing to have. In
>> short, we are not in the business of catching bugs in pcre2_jit
>> implementations, so if they say they cannot compile the pattern (I
>> would even say I doubt the point of checking the return code to
>> ensure it is NOMEMORY), it would be fine to let the interpreter
>> codepath to inspect the pattern and diagnose problems with it, or
>> take the slow match without JIT.
>>
>> What am I missing?
>
> Note that I've seen and recently re-read the discussion that leads to
> https://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/
Ahh, so ignore my last advise in the previous Email. You already read it.
> I suspect that this auto-probe is related to solving "the user
> thinks JIT is in use but because of failing JIT the user's pattern
> is getting horrible performance" somehow. But I do not think a hard
> failure is a good approach to help users in such a situation.
Yes, it's exactly trying to detect this case and not cause a regression
for users expecting 'git grep' to make use of the JIT.
So, beside the W|X memory allocation error, other errors are likely only
to point out limitations of the JIT compiler, e.g. the example I gave in
https://lore.kernel.org/all/2b04b19a-a2bd-3dd5-6f21-ed0b0ad3e02f@grsecurity.net/,
which is, admitted, a made up example that can easily be worked around
by manually prefixing it with '(*NO_JIT)'. It would be a pain having to
do that for *every* pattern, but only doing it for the pathological
cases should be fine. Otherwise more users would have run into it
already and complained about it, no?
> After such a failure, the user can prefix "(*NO_JIT)" to the pattern
> and retry, or give up the operation altogether and not get a useful
> result, but wouldn't it be far more helpful to just fallback as if
> (*NO_JIT) was on from the beginning?
Sure, if it would be required for *every*, i.e. "normal" patterns. But
always doing it even for abusive ones doesn't feel right.
> Also I notice that p->pcre2_jit_on is per "struct grep_pat", so it
> is not like "once we see a pathological pattern, we turn off JIT
> completely for other patterns", right? That is, if you have
>
> git grep -P -e "$A" -e "$B"
>
> and we fail to compile "$A" (for whatever reason), we could still
> (attempt to) compile "$B". Perhaps $A was too complex or was
> incompatible with JIT combined with other options, but $B may be
> easy enough to still be JITtable, in which case we would match with
> the JITted version of $B with interpreted version of $A, instead of
> failing, right?
The current version of git would fail hard if it fails to JIT compile
"$A". My patch doesn't change that behavior and that's intentional, as I
share Ævar's thinking about falling back to the interpreter mode for
"complex patterns" (which means pathological cases, really) is a bad
idea. While we might be able to compile the pattern and run it in
interpreter mode, it'll likely have a *much* higher runtime.
Just to get you a glimpse of how much longer, this is what it takes
running the pathological pattern from above's example on git.git:
$ time git grep -P "(*NO_JIT)$(perl -e 'print "(.)" x 4000')"
Binary file git-gui/macosx/git-gui.icns matches
Binary file t/t5000/pax.tar matches
Binary file t/t5004/big-pack.zip matches
Binary file t/t5004/empty-with-pax-header.tar matches
real 44m42,150s
user 577m14,623s
sys 0m46,210s
So this grep run eat up ~9.5 *hours* of CPU time. Do we really want to
fall back to something like this for the pathological cases? ...Yeah, I
don't think so either.
Thanks,
Mathias
^ permalink raw reply
* Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Mathias Krause @ 2023-01-29 12:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <xmqqbkmk9bsn.fsf@gitster.g>
On 27.01.23 17:34, Junio C Hamano wrote:
> Mathias Krause <minipli@grsecurity.net> writes:
>
>> As having a functional PCRE2 JIT compiler is a legitimate use case for
>> performance reasons, we'll only do the fallback if the supposedly
>> available JIT is found to be non-functional by attempting to JIT compile
>> a very simple pattern. If this fails, JIT is deemed to be non-functional
>> and we do the interpreter fallback. For all other cases, i.e. the simple
>> pattern can be compiled but the user provided cannot, we fail hard as we
>> do now as the reason for the failure must be the pattern itself.
>
> I do not know if it is a good idea to rely on the "very simple
> pattern". The implementation of JIT could devise various ways to
> succeed for such simple patterns without having writable-executable
> piece of memory.
Well, if PCRE2 JIT ever changes to optimize this case, we would be back
to the error I'm seeing right now. But I doubt that PCRE2 will be doing
optimizations like that. The current implementation does the JIT memory
allocation test very early, even before looking at the pattern:
https://github.com/PCRE2Project/pcre2/blob/pcre2-10.42/src/pcre2_jit_compile.c#L14450-L14465
But I can add a call to pcre2_pattern_info(PCRE2_INFO_JITSIZE) if you
really like me to, but IMHO it's not needed.
> What happened to the earlier idea of falling back
> to the interpreted codepath, which will catch any bad pattern that
> has "the reason for the failure" by failing anyway?
Ævar's concerns about always falling back to the interpreter mode made
me change the patch like this. Basically what he's concerned about are
two things:
1/ "Crazy patterns" that fail the JIT but will work for the interpreter
can be a serve performance regression.
2/ Always falling back to interpreter mode might mask JIT API usage
errors, we'd like to see.
While 1/ could also be seen as a limitation of current 'git grep', I
share Ævar's extended runtime regression concerns. If, for example, some
web interface offers users to supply arbitrary grep patterns, abusing
the interpreter mode fallback will consume significant more CPU
resources than it does right now (which simply fails with an error).
>
>> +static int pcre2_jit_functional(void)
>> +{
>> + static int jit_working = -1;
>> + pcre2_code *code;
>> + size_t off;
>> + int err;
>> +
>> + if (jit_working != -1)
>> + return jit_working;
>> +
>> + /*
>> + * Try to JIT compile a simple pattern to probe if the JIT is
>> + * working in general. It might fail for systems where creating
>> + * memory mappings for runtime code generation is restricted.
>> + */
>> + code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL);
>> + if (!code)
>> + return 0;
>> +
>> + jit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;
>> + pcre2_code_free(code);
>
> I'd prefer not having to worry about: Or it might not fail for such
> systems, as the pattern is too simple and future versions of
> pcre2_compile() could have special case code.
See above, it's unlikely to happen.
>
>> @@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>> pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
>> if (p->pcre2_jit_on) {
>> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
>> - if (jitret)
>> + if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {
>> + /*
>> + * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)
>> + * indicated JIT support, the library might still
>> + * fail to generate JIT code for various reasons,
>> + * e.g. when SELinux's 'deny_execmem' or PaX's
>> + * MPROTECT prevent creating W|X memory mappings.
>> + *
>> + * Instead of faling hard, fall back to interpreter
>> + * mode, just as if the pattern was prefixed with
>> + * '(*NO_JIT)'.
>> + */
>> + p->pcre2_jit_on = 0;
>> + return;
>
> Yes, the "instead of failing hard, fall back" makes sense. Just
> that I do not see why the runtime test is a good thing to have.
It prevents the fallback from being abused and introducing new
regressions. So it's good to have.
> In
> short, we are not in the business of catching bugs in pcre2_jit
> implementations, so if they say they cannot compile the pattern (I
> would even say I doubt the point of checking the return code to
> ensure it is NOMEMORY), it would be fine to let the interpreter
> codepath to inspect the pattern and diagnose problems with it, or
> take the slow match without JIT.
Yeah, unfortunately they're not gonna fix what's a bug, IMHO. They think
it's a feature: https://github.com/PCRE2Project/pcre2/pull/157
Anyhow, the error code is very well documented, see pcre2_jit_compile(3)
"""
[...] The function can also return PCRE2_ERROR_NOMEMORY if
JIT is unable to allocate executable memory for the compiler,
even if it was because of a system security restriction.
"""
And that's very much in line with what the test in pcre2_jit_compile()'s
current implementation does.
> What am I missing?
Please have a look at Ævar's reasoning here:
https://lore.kernel.org/git/221220.86bknxwy9t.gmgdl@evledraar.gmail.com/
Thanks,
Mathias
^ permalink raw reply
* Re: [PATCH v3] Documentation: clarify that cache forgets credentials if the system restarts
From: Jeff King @ 2023-01-29 6:48 UTC (permalink / raw)
To: M Hickford via GitGitGadget; +Cc: git, brian m. carlson, M Hickford
In-Reply-To: <pull.1447.v3.git.1674936815117.gitgitgadget@gmail.com>
On Sat, Jan 28, 2023 at 08:13:34PM +0000, M Hickford via GitGitGadget wrote:
> diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
> index 432e159d952..f473994a864 100644
> --- a/Documentation/git-credential-cache.txt
> +++ b/Documentation/git-credential-cache.txt
> @@ -14,10 +14,13 @@ git config credential.helper 'cache [<options>]'
> DESCRIPTION
> -----------
>
> -This command caches credentials in memory for use by future Git
> -programs. The stored credentials never touch the disk, and are forgotten
> -after a configurable timeout. The cache is accessible over a Unix
> -domain socket, restricted to the current user by filesystem permissions.
> +This command caches credentials for use by future Git programs.
> +The stored credentials are kept in memory of the cache-daemon
> +process (instead of written to a file) and are forgotten after a
> +configurable timeout. Credentials are forgotten sooner if the
> +cache-daemon dies, for example if the system restarts. The cache
> +is accessible over a Unix domain socket, restricted to the current
> +user by filesystem permissions.
This version looks good to me.
-Peff
^ permalink raw reply
* Re: t5559 breaks with apache 2.4.55
From: Jeff King @ 2023-01-29 6:35 UTC (permalink / raw)
To: Todd Zullinger; +Cc: git
In-Reply-To: <Y9Jmfg/jlSszVep4@coredump.intra.peff.net>
On Thu, Jan 26, 2023 at 06:39:42AM -0500, Jeff King wrote:
> Yeah, after seeing that, I'm quite sure this is a mod_http2 issue. It
> would be nice to bisect within the mod_http2 history to find the
> culprit, but I'd first have to figure out how to build standalone apache
> modules. ;)
This turned out to be quite painless, so I bisected using the source at:
https://github.com/icing/mod_h2
Unfortunately, it's not super helpful for identifying the problem. The
culprit turns out to be 16ffed9692b, which has a 450-line diff. The
commit message is:
* refactored stream response handling to reflect the different phases
(response/data/trailers) more clearly and help resolving cpu busy loops.
But that does at least give me more confidence that the bug is in
mod_http2, and isn't, say, some intentional behavior change there that
happens to trigger a bug in curl.
I opened an issue here: https://github.com/icing/mod_h2/issues/243
So we'll see if that helps.
I re-read your earlier thread, and the version problems you have don't
quite line up. I think you were having issues with mod_http2 2.0.9, but
the older version (1.5.19) worked. I was OK with 2.0.9, but upgrading to
2.0.10 broke things. I'd hate to have to disable t5559 by default; it
does catch some cases that would trigger in the real-world (since real
sites like GitHub are using http2). But if it's too unreliable in
practice, that might be the path of least resistance.
-Peff
^ permalink raw reply
* [PATCH v3] Documentation: clarify that cache forgets credentials if the system restarts
From: M Hickford via GitGitGadget @ 2023-01-28 20:13 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Jeff King, M Hickford, M Hickford
In-Reply-To: <pull.1447.v2.git.1674936563549.gitgitgadget@gmail.com>
From: M Hickford <mirth.hickford@gmail.com>
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
Documentation: clarify that cache forgets credentials if the system
restarts
Make it obvious to readers unfamiliar with Unix sockets.
Signed-off-by: M Hickford mirth.hickford@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1447%2Fhickford%2Fpatch-2-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1447/hickford/patch-2-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1447
Range-diff vs v2:
1: e84d069cf19 ! 1: 09f4afae70c Documentation: clarify that cache forgets credentials if the system restarts
@@ Documentation/git-credential-cache.txt: git config credential.helper 'cache [<op
+The stored credentials are kept in memory of the cache-daemon
+process (instead of written to a file) and are forgotten after a
+configurable timeout. Credentials are forgotten sooner if the
-+cache-daemon dies, for example if the system restarts. The cached
++cache-daemon dies, for example if the system restarts. The cache
+is accessible over a Unix domain socket, restricted to the current
+user by filesystem permissions.
Documentation/git-credential-cache.txt | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
index 432e159d952..f473994a864 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -14,10 +14,13 @@ git config credential.helper 'cache [<options>]'
DESCRIPTION
-----------
-This command caches credentials in memory for use by future Git
-programs. The stored credentials never touch the disk, and are forgotten
-after a configurable timeout. The cache is accessible over a Unix
-domain socket, restricted to the current user by filesystem permissions.
+This command caches credentials for use by future Git programs.
+The stored credentials are kept in memory of the cache-daemon
+process (instead of written to a file) and are forgotten after a
+configurable timeout. Credentials are forgotten sooner if the
+cache-daemon dies, for example if the system restarts. The cache
+is accessible over a Unix domain socket, restricted to the current
+user by filesystem permissions.
You probably don't want to invoke this command directly; it is meant to
be used as a credential helper by other parts of Git. See
base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2] Documentation: clarify that cache forgets credentials if the system restarts
From: M Hickford via GitGitGadget @ 2023-01-28 20:09 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Jeff King, M Hickford, M Hickford
In-Reply-To: <pull.1447.git.1671610994375.gitgitgadget@gmail.com>
From: M Hickford <mirth.hickford@gmail.com>
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
Documentation: clarify that cache forgets credentials if the system
restarts
Make it obvious to readers unfamiliar with Unix sockets.
Signed-off-by: M Hickford mirth.hickford@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1447%2Fhickford%2Fpatch-2-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1447/hickford/patch-2-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1447
Range-diff vs v1:
1: 5032ddf99da ! 1: e84d069cf19 Documentation: clarify that cache forgets credentials if the system restarts
@@ Metadata
## Commit message ##
Documentation: clarify that cache forgets credentials if the system restarts
- Make it obvious to readers unfamiliar with Unix sockets.
-
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
## Documentation/git-credential-cache.txt ##
-@@ Documentation/git-credential-cache.txt: DESCRIPTION
+@@ Documentation/git-credential-cache.txt: git config credential.helper 'cache [<options>]'
+ DESCRIPTION
+ -----------
- This command caches credentials in memory for use by future Git
- programs. The stored credentials never touch the disk, and are forgotten
+-This command caches credentials in memory for use by future Git
+-programs. The stored credentials never touch the disk, and are forgotten
-after a configurable timeout. The cache is accessible over a Unix
-+after a configurable timeout. Credentials are forgotten sooner if you
-+log out or the system restarts. The cache is accessible over a Unix
- domain socket, restricted to the current user by filesystem permissions.
+-domain socket, restricted to the current user by filesystem permissions.
++This command caches credentials for use by future Git programs.
++The stored credentials are kept in memory of the cache-daemon
++process (instead of written to a file) and are forgotten after a
++configurable timeout. Credentials are forgotten sooner if the
++cache-daemon dies, for example if the system restarts. The cached
++is accessible over a Unix domain socket, restricted to the current
++user by filesystem permissions.
You probably don't want to invoke this command directly; it is meant to
+ be used as a credential helper by other parts of Git. See
Documentation/git-credential-cache.txt | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
index 432e159d952..9eef5d2cc10 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -14,10 +14,13 @@ git config credential.helper 'cache [<options>]'
DESCRIPTION
-----------
-This command caches credentials in memory for use by future Git
-programs. The stored credentials never touch the disk, and are forgotten
-after a configurable timeout. The cache is accessible over a Unix
-domain socket, restricted to the current user by filesystem permissions.
+This command caches credentials for use by future Git programs.
+The stored credentials are kept in memory of the cache-daemon
+process (instead of written to a file) and are forgotten after a
+configurable timeout. Credentials are forgotten sooner if the
+cache-daemon dies, for example if the system restarts. The cached
+is accessible over a Unix domain socket, restricted to the current
+user by filesystem permissions.
You probably don't want to invoke this command directly; it is meant to
be used as a credential helper by other parts of Git. See
base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68
--
gitgitgadget
^ permalink raw reply related
* [PATCH] Documentation: clarify that cache forgets credentials if the system restarts
From: M Hickford @ 2023-01-28 20:08 UTC (permalink / raw)
To: peff; +Cc: git, gitgitgadget, gitster, sandals, mirth.hickford
In-Reply-To: <Y6PDx7Ij4NA/kBB7@coredump.intra.peff.net>
Thanks Junio, Jeff and Brian for your replies. I'll send an updated patch based on Junio's text. To me, most important is to explain that credentials are forgotten early if the daemon exits, so that setting a timeout of 1 year is unlikely to work.
^ permalink raw reply
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: Elijah Newren @ 2023-01-28 16:45 UTC (permalink / raw)
To: William Sprent
Cc: William Sprent via GitGitGadget, git, Eric Sunshine,
Ævar Arnfjörð Bjarmason, Derrick Stolee,
Victoria Dye
In-Reply-To: <9dda8cde-7c96-a5f1-f271-951f8b348b80@unity3d.com>
On Fri, Jan 27, 2023 at 3:59 AM William Sprent <williams@unity3d.com> wrote:
>
> On 26/01/2023 04.25, Elijah Newren wrote:
> > On Wed, Jan 25, 2023 at 8:16 AM William Sprent <williams@unity3d.com> wrote:
> >>
> >> On 25/01/2023 06.11, Elijah Newren wrote:
> >>> It looks like Ævar and Victoria have both given really good reviews
> >>> already, but I think I spotted some additional things to comment on.
> >>>
> >>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
> >>> <gitgitgadget@gmail.com> wrote:
> >>>>
> >>>> From: William Sprent <williams@unity3d.com>
> >>>>
> >>>> There is currently no way to ask git the question "which files would be
> >>>> part of a sparse checkout of commit X with sparse checkout patterns Y".
> >>>> One use-case would be that tooling may want know whether sparse checkouts
> >>>> of two commits contain the same content even if the full trees differ.
> >>>
> >>> Could you say more about this usecase? Why does tooling need or want
> >>> to know this; won't a checkout of the new commit end up being quick
> >>> and simple? (I'm not saying your usecase is bad, just curious that it
> >>> had never occurred to me, and I'm afraid I'm still not sure what your
> >>> purpose might be.)
> >>>
> >>
> >> I'm thinking mainly about a monorepo context where there are a number of
> >> distinct 'units' that can be described with sparse checkout patterns.
> >> And perhaps there's some tooling that only wants to perform an action if
> >> the content of a 'unit' changes.
> >
> > So, you're basically wanting to do something like
> > git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT1
> >> sparse-files
> > git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT2
> >>> sparse-files
> > sort sparse-files | uniq >relevant-files
> > git diff --name-only $COMMIT1 $COMMIT2 >changed-files
> > and then checking whether relevant-files and changed-files have a
> > non-empty intersection?
>
> Well, the concrete use-case I'm exploring is something along the lines
> of using the content hashes of sparse checkouts as cache keys for resource
> heavy jobs (builds/tests/etc).
>
> So, that would be something along the lines of,
>
> git ls-tree -r --paths-matching-sparsity-file=<pattern-file> \
> | sha1sum > cache-key
>
> and then performing a lookup before performing an action (which would
> then only be done in the context of the sparse checkout). My thinking
> is that this only would require git and no additional tooling, which in
> turn makes it very easy to reproduce the state where the job took place.
>
>
> > Would that potentially be better handled by
> > git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore
> > --ignore-file=<pattern-file> --stdin
> > and seeing whether the output is non-empty? We'd have to add an
> > "--ignore-file" option to check-ignore to override reading of
> > .gitignore files and such, and it'd be slightly confusing because the
> > documentation talks about "ignored" files rather than "selected"
> > files, but that's a confusion point that has been with us ever since
> > the gitignore mechanism was repurposed for sparse checkouts. Or maybe
> > we could just add a check-sparsity helper, and then allow it to take
> > directories in-lieu of patterns.
>
> I don't think it necessarily would be better handled by that. But it would
> be workable. It would be a matter of collating the output of
>
> git ls-tree -r <commit>
>
> with
>
> git ls-tree --name-only -r <commit> | git check-ignore ...
>
> Which is less ergonomic. But it is also a less intrusive change.
>
> Really, the main thing is to expose the sparse filtering logic somehow, and
> allow for building tooling on top of it.
> > This seems nicer than opening a can of worms about letting every git
> > command specify a different set of sparsity rules.
>
> I think you are the better judge of how much of a can of worms that would
> be. I don't think it would be too out of line with how git acts in general
> though, as we have things like the the 'GIT_INDEX_FILE' env-var.
I agree you've got a reasonable usecase here. The integration you've
described sounds like something that could be independently composable
with several other commands, and you alluded to that in your commit
message. But is adding it to ls-tree the best spot? If we add it
there, then folks who want to similarly integrate such capabilities
with other commands (archive, diff, grep, rev-list --objects, etc.),
seem more likely to do so via direct integration. We already have a
very large can of worms to work on to make commands behave in ways
that are limited to sparse paths (see
Documentation/techncial/sparse-checkout.txt, namely the "behavior A"
stuff). As can be seen in that document, what to do for limiting
commands to sparse paths is obvious with some commands but has lots of
interesting edge cases for others (even with years of experience with
sparse checkouts, we had 3- and 4- way differing opinions on the
intended behavior for some commands when we started composing that
document a few months ago). If we had jumped straight to
implementation for some commands, we would have likely painted
ourselves into a corner for other commands. Adding another layer of
specifying an alternate set of sparsity rules will likely have
interesting knock-on effects that we should think through for all the
commands to ensure we aren't painting ourselves into a similar corner,
if we go down this route.
However, in the cases that sparse-checkout.txt document was
addressing, the behavior fundamentally needs to be integrated into all
the relevant commands to get user experience right. In your case, you
merely need a separate tool to be able to compose the output of
different commands together. So, exposing whether sparsity rules
would select various paths in a single separate command (maybe git
check-ignore, or a new sparse-checkout subcommand, or maybe just a new
subcommand similar to check-ignore) would avoid a lot of these issues,
and give us a single place to extend/improve as we learn about further
usecases.
[...]
> >> I agree that it is a bit awkward to have to "translate" the directories
> >> into patterns when wanting to use cone mode. I can try adding
> >> '--[no]-cone' flags and see how that feels. Together with Victoria's
> >> suggestions that would result in having the following flags:
> >>
> >> * --scope=(sparse|all)
> >> * --sparse-patterns-file=<path>
> >> * --[no]-cone: used together with --sparse-patterns-file to tell git
> >> whether to interpret the patterns given as directories (cone) or
> >> patterns (no-cone).
> >>
> >> Which seems like a lot at first glance. But it allows for passing
> >> directories instead of patterns for cone mode, and is similar to the
> >> behaviour of 'sparse-checkout set'.
> >>
> >> Does that seem like something that would make sense?
> >
> > --sparse-patterns-file still implies patterns; I think that would need
> > some rewording.
>
> Yeah. After sleeping on it, I also think that it becomes a difficult
> interface to work with, and you'll get different results with the same
> patterns whether you pass --cone or --no-cone, which seems error prone
> to me.
>
> For better or for worse, both cone and non-cone uses of sparse-checkouts
> end up producing pattern files. And those pattern files do unambiguously
> describe a filtering of the worktree whether it is in cone-mode or not.
Back when cone mode was introduced, I objected to reusing the existing
pattern format and/or files...but was assuaged that it was just an
implementation detail that wouldn't be exposed to users (since people
would interact with the 'sparse-checkout' command instead of direct
editing of $GIT_DIR/info/sparse-checkout). It's still a performance
penalty, because we waste time both turning directory names into
patterns when we write them out, and when we read them in by parsing
the patterns to see if they match cone mode rules and if so discard
the patterns and just build up our hashes. The patterns are nothing
more than an intermediary format we waste time translating to and
from, though once upon a time they existed so that if someone suddenly
started using an older (now ancient) version of git on their current
checkout then it could still hobble along with degraded performance
instead of providing an error. (We have introduced other changes to
the config and git index which would cause older git clients to just
fail to parse and throw an error, and even have defined mechanisms for
such erroring out. We could have taken advantage of that for this
feature too.)
Anyway, long story short, if you're going to start exposing users to
this implementation detail that was meant to be hidden (and do it in a
way that may be copied into several commands to boot), then I
definitely object.
> Given that 'ls-tree' is more of a plumbing command, I think it might still
> make sense to use the patterns. That would also make the interaction
> a bit more logical to me -- e.g. if you want to override the patterns
> you have to pass them in the same format as the ones that would be read
> by default.
No, sparsity specification should be provided by users the same way
they normally specify it (i.e. the way they pass it to
`sparse-checkout {add,set}`), not the way it's stored via some hidden
implementation detail.
I'd make an exception if you ended up using `git check-ignore` because
that command was specifically written about gitignore-style rules, and
git-ignore-style rules just happen to have been reused as-is for
non-cone-mode sparse-checkout rules. But if you go that route:
(1) you have to frame any extensions to check-ignore as things that
are useful for gitignore checking, and only incidentally useful to
sparse-checkouts
(2) you'd have to forgo any cone-mode optimizations
Going the check-ignore route seems like the easiest path to providing
the basic functionality you need right now. If your usecases remain
niche, that might be good enough for all time too. If your usecases
become popular, though, I expect someone would eventually tire of
using `check-ignore` and implement similar functionality along with
supporting cone-mode in a `git check-sparsity` or `git sparse-checkout
debug-rules` command or something like that.
^ permalink raw reply
* [PATCH v7 00/12] Enhance credential helper protocol to include auth headers
From: M Hickford @ 2023-01-28 14:28 UTC (permalink / raw)
To: gitgitgadget
Cc: avarab, chooglen, derrickstolee, git, git, lessleydennington,
mirth.hickford, mjcheetham, vdye
In-Reply-To: <pull.1352.v7.git.1674252530.gitgitgadget@gmail.com>
> Future work
> ===========
>
> In the future we can further expand the protocol to allow credential helpers
> decide the best authentication scheme. Today credential helpers are still
> only expected to return a username/password pair to Git, meaning the other
> authentication schemes that may be offered still need challenge responses
> sent via a Basic Authorization header. The changes outlined above still
> permit helpers to select and configure an available authentication mode, but
> require the remote for example to unpack a bearer token from a basic
> challenge.
>
> More careful consideration is required in the handling of custom
> authentication schemes which may not have a username, or may require
> arbitrary additional request header values be set.
>
> For example imagine a new "FooBar" authentication scheme that is surfaced in
> the following response:
>
> HTTP/1.1 401 Unauthorized
> WWW-Authenticate: FooBar realm="login.example", algs="ES256 PS256"
>
>
> With support for arbitrary authentication schemes, Git would call credential
> helpers with the following over standard input:
>
> protocol=https
> host=example.com
> wwwauth[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123"
>
>
> And then an enlightened credential helper could return over standard output:
>
> protocol=https
> host=example.com
> authtype=FooBar
> username=bob@id.example.com
> password=<FooBar credential>
> header[]=X-FooBar: 12345
> header[]=X-FooBar-Alt: ABCDEF
>
>
> Git would be expected to attach this authorization header to the next
> request:
>
> GET /info/refs?service=git-upload-pack HTTP/1.1
> Host: git.example
> Git-Protocol: version=2
> Authorization: FooBar <FooBar credential>
> X-FooBar: 12345
> X-FooBar-Alt: ABCDEF
Interesting! Can you tell us more about how you hope to use this at GitHub? Could this be used for OAuth 2.0 Demonstrating Proof-of-Possession at the Application Layer (DPoP)? https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop (some of the fields in your example look familiar).
Challenge responses are typically short lived [1]. What happens if a storage helper is configured before a challenge-response helper? We want to maintain composability of helpers.
[credential]
helper = storage # eg. cache or osxkeychain
helper = challenge-response # eg. oauth-dpop
Storage may return an expired challenge response stored earlier. This could be avoided by introducing an expiry attribute to the credential protocol. https://lore.kernel.org/git/pull.1443.git.git.1674914650588.gitgitgadget@gmail.com/T/#u
A monolithic helper configured alone doesn't have this problem -- it knows which parts of its output to store or discard.
Declaration of interest: I maintain a credential-generating OAuth helper composable with any storage helper. https://github.com/hickford/git-credential-oauth
[1] https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop#section-8
^ permalink raw reply
* [PATCH] credential: new attribute password_expiry_utc
From: M Hickford via GitGitGadget @ 2023-01-28 14:04 UTC (permalink / raw)
To: git; +Cc: M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
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>
---
credential: new attribute password_expiry_utc
Some passwords, such as a personal access token or OAuth access token,
may have an expiry date (as long as years for PATs or as short as hours
for an OAuth access token). Add a new credential attribute
password_expiry_utc.
If password has expired, credential fill no longer returns early, so
later helpers have opportunity to 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. Credential-generating helpers need only output the expiry
attribute. Storage helpers should store the expiry if they can.
Store expiry attribute in cache.
This is particularly useful when a storage helper and a
credential-generating helper are configured together, eg.
[credential]
helper = storage # eg. cache or osxkeychain
helper = generate # eg. oauth
Without this patch, credential fill may return an expired credential
from storage, causing authentication to fail. With this patch: a fresh
credential is generated if and only if the credential is expired.
Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16/files
Signed-off-by: M Hickford mirth.hickford@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v1
Pull-Request: https://github.com/git/git/pull/1443
Documentation/git-credential.txt | 4 ++++
builtin/credential-cache--daemon.c | 3 +++
credential.c | 21 +++++++++++++++++++++
credential.h | 1 +
4 files changed, 29 insertions(+)
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..15ace648bdd 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -144,6 +144,10 @@ Git understands the following attributes:
The credential's password, if we are asking it to be stored.
+`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.
+
`url`::
When this special attribute is read by `git credential`, the
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);
+ }
}
}
else if (!strcmp(action.buf, "exit")) {
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>
void credential_init(struct credential *c)
{
@@ -21,6 +22,7 @@ void credential_clear(struct credential *c)
free(c->path);
free(c->username);
free(c->password);
+ c->password_expiry_utc = 0;
string_list_clear(&c->helpers, 0);
credential_init(c);
@@ -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
+ c->password_expiry_utc = atoi(value);
} else if (!strcmp(key, "url")) {
credential_from_url(c, value);
} else if (!strcmp(key, "quit")) {
c->quit = !!git_config_bool("quit", value);
}
+
+ // 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"));
+ FREE_AND_NULL(c->username);
+ FREE_AND_NULL(c->password);
+ c->password_expiry_utc = 0;
+ }
+
/*
* Ignore other lines; we don't know what they mean, but
* this future-proofs us when later versions of git do
@@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path, 0);
credential_write_item(fp, "username", c->username, 0);
credential_write_item(fp, "password", c->password, 0);
+ if (c->password_expiry_utc != 0) {
+ int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc);
+ char* str = malloc( length + 1 );
+ snprintf( str, length + 1, "%ld", c->password_expiry_utc );
+ credential_write_item(fp, "password_expiry_utc", str, 0);
+ free(str);
+ }
}
static int run_credential_helper(struct credential *c,
diff --git a/credential.h b/credential.h
index f430e77fea4..e10f7c2b313 100644
--- a/credential.h
+++ b/credential.h
@@ -126,6 +126,7 @@ struct credential {
char *protocol;
char *host;
char *path;
+ time_t password_expiry_utc;
};
#define CREDENTIAL_INIT { \
base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'
From: Teng Long @ 2023-01-28 11:50 UTC (permalink / raw)
To: sunshine; +Cc: avarab, dyroneteng, git, tenglong.tl
In-Reply-To: <CAPig+cR5s3XzmY+L_jDW2g_PEgi5E791x0GuV+VPkxFA_6sB7A@mail.gmail.com>
Eric Sunshine writes:
> > Situation of note "removing" shouldn't happen in 'append_edit()',
> > unless it's a bug. So, let's drop the unreachable "else" code
> > in "append_edit()".
> >
> > The notes operation "append" is different with "add", the latter
> > supports to overwrite the existing note then let the "removing"
> > happen (e.g. execute `git notes add -f -F /dev/null` on an existing
> > note), but the former will not because it only does "appends" but
> > not doing "overwrites".
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > @@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> > if (add_note(t, &object, &new_note, combine_notes_overwrite))
> > BUG("combine_notes_overwrite failed");
> > logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
> > - } else {
> > - fprintf(stderr, _("Removing note for object %s\n"),
> > - oid_to_hex(&object));
> > - remove_note(t, object.hash);
> > - logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> > + commit_notes(the_repository, t, logmsg);
> > }
> > - commit_notes(the_repository, t, logmsg);
>
> This change breaks removal of notes using "git notes edit". Prior to
> this change, if you delete the content of a note using "git notes
> edit", then the note is removed. Following this change, the note
> remains, which contradicts documented[1] behavior.
>
> [1]: Unfortunately, perhaps, this behavior is documented under the
> "remove" subcommand rather than the "edit" subcommand, but it is
> nevertheless documented and has worked this way for ages, so this
> patch causes a regression.
As the commit msg describes, the subcommands I understand should have clear
responsibilities as possible (documentaion may have some effect in my mind). So,
the removal opertion under "append subcommand" here is little wired to me, but
your suggestion makes sense, this may have compatibility issues. Although I
think it's weird that someone would use this in the presence of the remove
subcommand, and my feeling is that this code is actually copied from the add
method (introduced by 52694cdabbf68f19c8289416e7bb3bbef41d8d27), but I'm not
sure.
So, it's ok for me to drop this one.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 2/5] notes.c: cleanup for "designated init" and "char ptr init"
From: Teng Long @ 2023-01-28 11:33 UTC (permalink / raw)
To: avarab; +Cc: dyroneteng, git, sunshine, tenglong.tl
In-Reply-To: <230112.867cxs2i83.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason writes:
> > From: Teng Long <dyroneteng@gmail.com>
> >
> > Let's do some cleanup for the following two places in "append_edit()".
> >
> > The first place is "char *logmsg;" need to be initialized with NULL.
> > The second place is "struct note_data d = { 0, 0, NULL, STRBUF_INIT };"
> > could be replaced with designated init format.
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > builtin/notes.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > index e57f024824..8ca55ec83e 100644
> > --- a/builtin/notes.c
> > +++ b/builtin/notes.c
> > @@ -566,9 +566,9 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> > struct notes_tree *t;
> > struct object_id object, new_note;
> > const struct object_id *note;
> > - char *logmsg;
> > + char *logmsg = NULL;
>
> This change isn't needed, and the compiler will check that we have it
> init'd.
>
> It *is* needed when combined with your 3/5, but even then it's the wrong
> solution. In that change you move the commit_notes() into that "if"
> branch that needs the "logmsg", and it's correct that you need to
> NULL-init it to safely pass it to free().
You are correct, will fix.
> But let's just declare the "logmsg" in that if block then, and move the
> free() over there, that reduces the scope it's in.
>
> > const char * const *usage;
> > - struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> > + struct note_data d = { .buf = STRBUF_INIT };
>
> This change is good, but then let's change the other such case in this
> file to a designated init too, and make the commit message etc. be about
> using designated init here.
OK.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 1/5] notes.c: cleanup 'strbuf_grow' call in 'append_edit'
From: Teng Long @ 2023-01-28 11:22 UTC (permalink / raw)
To: sunshine; +Cc: avarab, dyroneteng, git, tenglong.tl
In-Reply-To: <CAPig+cQKJxJCwk1GWtQ=LNNA=z9tQxYUwn9CMcXE4R9g8eKU7A@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:
> > Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This
> > "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if
> > needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0,
> > "\n");" will do the "grow" for us.
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > @@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> > char *prev_buf = read_object_file(note, &type, &size);
> >
> > - strbuf_grow(&d.buf, size + 1);
> > if (d.buf.len && prev_buf && size)
> > strbuf_insertstr(&d.buf, 0, "\n");
>
> Indeed, it's not clear why that was there in the first place. Digging
> through history doesn't shed any light on it. It was introduced by
> 2347fae50b (builtin-notes: Add "append" subcommand for appending to
> note objects, 2010-02-13)[1], but there's no explanation as to why it
> was coded that way. Best guess may be that the author originally
> inserted "\n" manually by direct manipulation of the strbuf rather
> than employing a strbuf function, but then switched to strbuf_insert()
> before submitting the series and forgot to remove the now-unnecessary
> strbuf_grow().
Yes, I have the same opinion with you, maybe original idea to savesome array
creation overhead? I'm not sure, but I think the deletion here is OK.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] scalar: only warn when background maintenance fails
From: Junio C Hamano @ 2023-01-28 0:32 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Derrick Stolee via GitGitGadget, git, vdye
In-Reply-To: <4913381a-769f-aba0-c04d-559d103e8396@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
>> The "maintain
>> their clone" certainly should include running periodic maintenance
>> tasks without them having to worry about it. It feels like this is
>> calling for an explicit "disable periodic maintenance tasks in this
>> repository" option to help these esoteric environments that disable
>> cron-like system services, while keeping the default safer,
>> i.e. fail loudly when the periodic maintenance tasks that the users
>> expect to happen cannot be enabled, or something.
>>
>> Perhaps I am not the primary audience, but hmph, I have a feeling
>> that this is not exactly going into a healthy direction.
>
> Here, we are in an environment where background maintenance is
> unavailable in an unexpected way. If that feature is not available
> to the user, should they not get the benefits of the others?
That is not what I was saying. I just have expected to see a way
for the user to give scalar an explicit "I understand that periodic
maintenance does not happen in this repository" consent, instead of
demoting an error detection for everybody to a warning that users
will just ignore.
^ permalink raw reply
* Re: [PATCH 3/3] scalar: only warn when background maintenance fails
From: Victoria Dye @ 2023-01-27 22:18 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget; +Cc: git, Derrick Stolee
In-Reply-To: <xmqqleln90ka.fsf@gitster.g>
Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> A user reported issues with 'scalar clone' and 'scalar register' when
>> working in an environment that had locked down the ability to run
>> 'crontab' or 'systemctl' in that those commands registered as _failures_
>> instead of opportunistically reporting a success with just a warning
>> about background maintenance.
>>
>> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a
>> successful background maintenance, but this is not a viable strategy for
>> long-term.
>>
>> Update 'scalar register' and 'scalar clone' to no longer fail by
>> modifying register_dir() to only warn when toggle_maintenance(1) fails.
>>
>> Since background maintenance is a "nice to have" and not a requirement
>> for a working repository, it is best to move this from hard error to
>> gentle warning.
>
> Wasn't the main selling point of scalar that users do not have to
> worry about various minute details of configuration settings to
> maintain their clone of projects on the larger side? The "maintain
> their clone" certainly should include running periodic maintenance
> tasks without them having to worry about it. It feels like this is
> calling for an explicit "disable periodic maintenance tasks in this
> repository" option to help these esoteric environments that disable
> cron-like system services, while keeping the default safer,
> i.e. fail loudly when the periodic maintenance tasks that the users
> expect to happen cannot be enabled, or something.
I see Stolee's approach as more in line with how FSMonitor is treated by
'scalar', which is "only turn it on if it's supported, but otherwise do
nothing" (the main difference here being that a warning is displayed if
maintenance can't be turned on). That still fits the stated goal of 'scalar'
("configure all the niche large-repo settings for me when I
clone/register"), but it makes 'scalar' more forgiving of system
configurations that don't support maintenance.
That said, this doesn't distinguish between "maintenance couldn't be turned
on because the system doesn't support it" and "maintenance couldn't be
turned on because of an internal error". The latter might still be worth
failing on, so maybe something like this would be more palatable?
--------8<--------8<--------8<--------
diff --git a/scalar.c b/scalar.c
index 6c52243cdf1..138780abe5f 100644
--- a/scalar.c
+++ b/scalar.c
@@ -252,6 +252,10 @@ static int stop_fsmonitor_daemon(void)
return 0;
}
+static int have_maintenance_support(void) {
+ /* check whether at least one scheduler is supported on the system */
+}
+
static int register_dir(void)
{
if (add_or_remove_enlistment(1))
@@ -260,7 +264,7 @@ static int register_dir(void)
if (set_recommended_config(0))
return error(_("could not set recommended config"));
- if (toggle_maintenance(1))
+ if (have_maintenance_support() && toggle_maintenance(1))
return error(_("could not turn on maintenance"));
if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
-------->8-------->8-------->8--------
>
> Perhaps I am not the primary audience, but hmph, I have a feeling
> that this is not exactly going into a healthy direction.
I don't think this change would be the start of a larger pattern, since most
Scalar-related settings aren't system-dependent (only FSMonitor and
maintenance are, AFAIK). What would be worse, I think, is setting the
maintenance behavior with a new command option - I could very easily see
*that* leading to a slippery slope of endless options to toggle 'scalar's
behaviors, completely defeating it's whole "set everything up for me"
purpose.
>
> Other two steps that led to this step looked quite sensible, though.
>
> Thanks.
^ permalink raw reply related
* Re: [PATCH 3/3] scalar: only warn when background maintenance fails
From: Derrick Stolee @ 2023-01-27 22:18 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget; +Cc: git, vdye
In-Reply-To: <xmqqleln90ka.fsf@gitster.g>
On 1/27/2023 3:36 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> A user reported issues with 'scalar clone' and 'scalar register' when
>> working in an environment that had locked down the ability to run
>> 'crontab' or 'systemctl' in that those commands registered as _failures_
>> instead of opportunistically reporting a success with just a warning
>> about background maintenance.
>>
>> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a
>> successful background maintenance, but this is not a viable strategy for
>> long-term.
>>
>> Update 'scalar register' and 'scalar clone' to no longer fail by
>> modifying register_dir() to only warn when toggle_maintenance(1) fails.
>>
>> Since background maintenance is a "nice to have" and not a requirement
>> for a working repository, it is best to move this from hard error to
>> gentle warning.
>
> Wasn't the main selling point of scalar that users do not have to
> worry about various minute details of configuration settings to
> maintain their clone of projects on the larger side?
Yes, and that includes things like partial clone, sparse-checkout,
and FS Monitor which are independent of this maintenance change.
> The "maintain
> their clone" certainly should include running periodic maintenance
> tasks without them having to worry about it. It feels like this is
> calling for an explicit "disable periodic maintenance tasks in this
> repository" option to help these esoteric environments that disable
> cron-like system services, while keeping the default safer,
> i.e. fail loudly when the periodic maintenance tasks that the users
> expect to happen cannot be enabled, or something.
>
> Perhaps I am not the primary audience, but hmph, I have a feeling
> that this is not exactly going into a healthy direction.
Here, we are in an environment where background maintenance is
unavailable in an unexpected way. If that feature is not available
to the user, should they not get the benefits of the others?
Not having background maintenance is definitely a downside, but it's
different from failing to connect to the server or being unable to
complete setting up the working directory. The warning communicates
the issue, so users can realize the problem exists and work to resolve
it in their own way.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH 3/3] scalar: only warn when background maintenance fails
From: Derrick Stolee @ 2023-01-27 22:14 UTC (permalink / raw)
To: Victoria Dye, Derrick Stolee via GitGitGadget, git; +Cc: gitster
In-Reply-To: <65b1735b-7ea6-5f7c-e1d9-6c986c7beb1d@github.com>
On 1/27/2023 5:06 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> if (toggle_maintenance(1))
>> - return error(_("could not turn on maintenance"));
>> + warning(_("could not turn on maintenance"));
>
> Should we do the same thing for 'unregister_dir()'? Unlike 'register_dir()',
> it doesn't break immediately (and finishes removing the enlistment), but it
> still returns a nonzero error code from 'scalar unregister'.
The interesting thing about unregister_dir() is that
toggle_maintenance(0) "turns of maintenance" by removing the
maintenance.repo config value pointing to this repository,
not by removing the maintenance schedule. Thus, we don't get
the failure in the same way.
>> -test_expect_success 'scalar clone fails when background maintenance fails' '
>> +test_expect_success 'scalar clone warns when background maintenance fails' '
>> GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
>> - test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
>> + scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
>> grep "could not turn on maintenance" err
>> '
>
> Similarly, it might be nice to show how 'scalar unregister' behaves when
> maintenance fails in the tests.
And the way I found out was by making the same tests, but since
they actually never failed or listed a warning (for this reason)
I left it out.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH 3/3] scalar: only warn when background maintenance fails
From: Victoria Dye @ 2023-01-27 22:06 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, Derrick Stolee
In-Reply-To: <d75780e0567b5f765816ab7522afe550ebaa3521.1674849963.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> A user reported issues with 'scalar clone' and 'scalar register' when
> working in an environment that had locked down the ability to run
> 'crontab' or 'systemctl' in that those commands registered as _failures_
> instead of opportunistically reporting a success with just a warning
> about background maintenance.
>
> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a
> successful background maintenance, but this is not a viable strategy for
> long-term.
>
> Update 'scalar register' and 'scalar clone' to no longer fail by
> modifying register_dir() to only warn when toggle_maintenance(1) fails.
>
> Since background maintenance is a "nice to have" and not a requirement
> for a working repository, it is best to move this from hard error to
> gentle warning.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> scalar.c | 2 +-
> t/t9210-scalar.sh | 4 ++--
> t/t9211-scalar-clone.sh | 4 ++--
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/scalar.c b/scalar.c
> index f25d5f1d0ef..ca19b95ce46 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -262,7 +262,7 @@ static int register_dir(void)
> return error(_("could not set recommended config"));
>
> if (toggle_maintenance(1))
> - return error(_("could not turn on maintenance"));
> + warning(_("could not turn on maintenance"));
Should we do the same thing for 'unregister_dir()'? Unlike 'register_dir()',
it doesn't break immediately (and finishes removing the enlistment), but it
still returns a nonzero error code from 'scalar unregister'.
>
> if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
> return error(_("could not start the FSMonitor daemon"));
> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
> index 13a4f6dbcf4..4432a30d10b 100755
> --- a/t/t9210-scalar.sh
> +++ b/t/t9210-scalar.sh
> @@ -104,10 +104,10 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '
> test_cmp_config -C test/src true core.fsmonitor
> '
>
> -test_expect_success 'scalar register fails when background maintenance fails' '
> +test_expect_success 'scalar register warns when background maintenance fails' '
> git init register-repo &&
> GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
> - test_must_fail scalar register register-repo 2>err &&
> + scalar register register-repo 2>err &&
> grep "could not turn on maintenance" err
> '
>
> diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
> index a6156da29ac..872ad1c9c2b 100755
> --- a/t/t9211-scalar-clone.sh
> +++ b/t/t9211-scalar-clone.sh
> @@ -174,9 +174,9 @@ test_expect_success 'progress without tty' '
> cleanup_clone $enlistment
> '
>
> -test_expect_success 'scalar clone fails when background maintenance fails' '
> +test_expect_success 'scalar clone warns when background maintenance fails' '
> GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
> - test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
> + scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
> grep "could not turn on maintenance" err
> '
Similarly, it might be nice to show how 'scalar unregister' behaves when
maintenance fails in the tests.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox