* [PATCH 0/1] Fix error message in git-show-ref(1) --exists
From: Toon Claes @ 2024-01-10 14:15 UTC (permalink / raw)
To: git; +Cc: Toon Claes
References can validly contain forward slashes in their name. With the ref files
backend, these are stored as a directory tree. This means when you look up a
reference, you might find a directory where you expected a file.
This causes the option --exists, recently added to git-show-ref(1), to return
the following error:
error: failed to look up reference: Is a directory
Other backends, like reftables, might store refs with forward slashes
differently. So they will not encounter the same error.
To make the error consistent across refs backend implementations, and to be more
clear to user, hide the error about having found a directory as a generic error:
error: reference does not exist
Toon Claes (1):
builtin/show-ref: treat directory directory as non-existing in
--exists
builtin/show-ref.c | 2 +-
t/t1403-show-ref.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
2.42.1
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Dragan Simic @ 2024-01-10 14:18 UTC (permalink / raw)
To: Jeff King; +Cc: Rubén Justo, Git List
In-Reply-To: <20240110110226.GC16674@coredump.intra.peff.net>
On 2024-01-10 12:02, Jeff King wrote:
> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
>
>> Using advise_if_enabled() to display an advice will automatically
>> include instructions on how to disable the advice, along with the
>> main advice:
>>
>> hint: use --reapply-cherry-picks to include skipped commits
>> hint: Disable this message with "git config advice.skippedCherryPicks
>> false"
>>
>> This can become distracting or noisy over time, while the user may
>> still want to receive the main advice.
>>
>> Let's have a switch to allow disabling this automatic advice.
>
> If I'm reading your patch correctly, this is a single option that
> controls the extra line for _all_ advice messages. But I'd have
> expected
> this to be something you'd want to set on a per-message basis. Here's
> my
> thinking.
>
> The original idea for advice messages was that they might be verbose
> and
> annoying, but if you had one that showed up a lot you'd choose to shut
> it up individually. But you wouldn't do so for _all_ messages, because
> you might benefit from seeing others (including new ones that get
> added). The "Disable this..." part was added later to help you easily
> know which config option to tweak.
Just to chime in and support this behavior of the advice messages.
Basically, you don't want to have them all disabled at the same time,
but to have per-message enable/disable granularity. I'd guess that some
of the messages are quite usable even to highly experienced users, and
encountering newly added messages is also very useful. Thus, having
them all disabled wouldn't be the best idea.
> The expectation was that you'd fall into one of two categories:
>
> 1. You don't see the message often enough to care, so you do nothing.
>
> 2. You do find it annoying, so you disable this instance.
>
> Your series proposes a third state:
>
> 3. You find the actual hint useful, but the verbosity of "how to shut
> it up" is too much for you.
>
> That make sense to me, along with being able to partially shut-up a
> message. But wouldn't you still need the "how to shut up" hint for
> _other_ messages, since it's customized for each situation?
>
> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,
> you run "git config advice.adviseOff false".
>
> But now you run into another hint, like:
>
> $ git foo
> hint: you can use --empty-commits to deal with isn't as good as
> --xyzzy
>
> and you want to disable it entirely. Which advice.* config option does
> so? You're stuck trying to find it in the manpage (which is tedious but
> also kind of tricky since you're now guessing which name goes with
> which
> message). You probably do want:
>
> hint: Disable this message with "git config advice.xyzzy false"
>
> in that case (at which point you may decide to silence it completely or
> partially).
>
> Which implies to me that the value of advice.* should be a tri-state to
> match the cases above: true, false, or a "minimal" / "quiet" mode
> (there
> might be a better name). And then you'd do:
>
> git config advice.skippedCherryPicks minimal
>
> (or whatever it is called) to get the mode you want, without affecting
> advice.xyzzy.
>
>> advice.c | 3 ++-
>> advice.h | 3 ++-
>> t/t0018-advice.sh | 8 ++++++++
>> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> Speaking of manpages, we'd presumably need an update to
> Documentation/config/advice.txt. :)
>
> -Peff
^ permalink raw reply
* [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists
From: Toon Claes @ 2024-01-10 14:15 UTC (permalink / raw)
To: git; +Cc: Toon Claes
In-Reply-To: <20240110141559.387815-1-toon@iotcl.com>
Recently [1] the option --exists was added to git-show-ref(1). When you
use this option against a ref that doesn't exist, but it is a parent
directory of an existing ref, you're getting the following error:
$ git show-ref --exists refs/heads
error: failed to look up reference: Is a directory
This error is confusing to the user. Instead, print the same error as
when the ref was not found:
error: reference does not exist
[1]: 9080a7f178 (builtin/show-ref: add new mode to check for reference
existence, 2023-10-31)
Signed-off-by: Toon Claes <toon@iotcl.com>
---
builtin/show-ref.c | 2 +-
t/t1403-show-ref.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index aaa2c39b2f..79955c2856 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -238,7 +238,7 @@ static int cmd_show_ref__exists(const char **refs)
if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
&unused_oid, &unused_referent, &unused_type,
&failure_errno)) {
- if (failure_errno == ENOENT) {
+ if (failure_errno == ENOENT || failure_errno == EISDIR) {
error(_("reference does not exist"));
ret = 2;
} else {
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index ec1957b709..d0a8f7b121 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -262,9 +262,9 @@ test_expect_success '--exists with non-commit object' '
test_expect_success '--exists with directory fails with generic error' '
cat >expect <<-EOF &&
- error: failed to look up reference: Is a directory
+ error: reference does not exist
EOF
- test_expect_code 1 git show-ref --exists refs/heads 2>err &&
+ test_expect_code 2 git show-ref --exists refs/heads 2>err &&
test_cmp expect err
'
--
2.42.1
^ permalink raw reply related
* [PATCH] t4129: prevent loss of exit code due to the use of pipes
From: Chandra Pratap via GitGitGadget @ 2024-01-10 12:54 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
From: Chandra Pratap <chandrapratap3519@gmail.com>
Piping the output of git commands like git-ls-files to another
command (grep in this case) hides the exit code returned by
these commands. Prevent this by storing the output of git-ls-files
to a temporary file and then "grep-ping" from that file. Replace
grep with test_grep as the latter is more verbose when it fails.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
t4129: prevent loss of exit code due to the use of pipes
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1636%2FChand-ra%2Ftest_pipe-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1636/Chand-ra/test_pipe-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1636
t/t4129-apply-samemode.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index e7a7295f1b6..ffabeafa213 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -41,7 +41,8 @@ test_expect_success FILEMODE 'same mode (index only)' '
chmod +x file &&
git add file &&
git apply --cached patch-0.txt &&
- git ls-files -s file | grep "^100755"
+ git ls-files -s file > ls-files-output &&
+ test_grep "^100755" ls-files-output
'
test_expect_success FILEMODE 'mode update (no index)' '
@@ -60,7 +61,8 @@ test_expect_success FILEMODE 'mode update (with index)' '
test_expect_success FILEMODE 'mode update (index only)' '
git reset --hard &&
git apply --cached patch-1.txt &&
- git ls-files -s file | grep "^100755"
+ git ls-files -s file > ls-files-output &&
+ test_grep "^100755" ls-files-output
'
test_expect_success FILEMODE 'empty mode is rejected' '
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-10 12:40 UTC (permalink / raw)
To: Junio C Hamano, Taylor Blau, Jeff King; +Cc: Git List
In-Reply-To: <xmqq8r4yf897.fsf@gitster.g>
On 09-ene-2024 14:32:20, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> that configuration knob is probably misnamed, as you
> pointed out).
Agree. Maybe we have a better name: "showDisableInstructions".
> I would understand if the proposed change were to change the
> "advice.<key>" configuration variable from a Boolean to a tristate
> (yes = default, no = disabled, always = give advice without
> instruction), though. IOW, the message might look like so:
>
> hint: use --reapply-cherry-picks to include skipped commits
> hint: setting advice.skippedCherryPicks to 'false' disables this message
> hint: and setting it to 'always' removes this additional instruction.
That's an interesting twist ... and intuitive it seems, as Peff also
came up with a similar approach.
But do we need, or want, that fine grain?
Using advise_if_enabled() allows us to display a hint while
automatically adding the option to disable it, _explicitly_ (so far*),
to the user.
But it happens that advise_if_enabled() itself has a hint to give.
My goal in this series is about giving the user the possibility to
disable _that_ hint (in the hint).
The user choosing to disable that hint is telling us: "I know I can
disable a hint, stop telling me so, please". So I don't think
this opens up a new risk or problem finding how to disable a hint.
$ git -c advice.showDisableInstructions=1 command_with_a_no_longer_welcomed_hint
Is there a need to make that "hint in the hint" customizable _per hint_?
That probably means that a new "make-it-always-for-all" may be desirable
alongside the new tristate yes-no-always ...
I dunno.
* perhaps this multi-value possibility could be a path
to explore what Taylor outlined in another message in this thread:
the possibility of a "one-shot" hint.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-10 12:11 UTC (permalink / raw)
To: Taylor Blau; +Cc: Git List, Junio C Hamano, Jeff King
In-Reply-To: <ZZ2QGYBBmK8cSYBD@nand.local>
On 09-ene-2024 13:27:37, Taylor Blau wrote:
> > + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 },
>
> The name seems to imply that setting `advice.adviceOff=true` would cause
> Git to suppress the turn-off instructions. But...
>
> > };
> >
> > static const char turn_off_instructions[] =
> > @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
> >
> > strbuf_vaddf(&buf, advice, params);
> >
> > - if (display_instructions)
> > + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
> > strbuf_addf(&buf, turn_off_instructions, key);
>
> ...it looks like the opposite is true. I guess the "adviceOff" part of
> this new configuration option suggests "show me advice on how to turn
> off advice of xyz kind in the future".
>
> Perhaps a clearer alternative might be "advice.showDisableInstructions"
> or something?
Yeah. We can then use ADVICE_SHOW_DISABLE_INSTRUCTIONS, which is also a
better name than the current ADVICE_ADVICE_OFF. Thanks.
I'll reroll also with a description of it in
Documentation/config/advice.txt, as Jeff has pointed out.
> I don't know, coming up with a direct/clear name of this
> configuration is challenging for me.
Well ... I'm not going to show the previous names I've been considering
for this ;-)
>
> >
> > for (cp = buf.buf; *cp; cp = np) {
> > diff --git a/advice.h b/advice.h
> > index 2affbe1426..1f2eef034e 100644
> > --- a/advice.h
> > +++ b/advice.h
> > @@ -10,7 +10,7 @@ struct string_list;
> > * Add the new config variable to Documentation/config/advice.txt.
> > * Call advise_if_enabled to print your advice.
> > */
> > - enum advice_type {
> > +enum advice_type {
> > ADVICE_ADD_EMBEDDED_REPO,
> > ADVICE_ADD_EMPTY_PATHSPEC,
> > ADVICE_ADD_IGNORED_FILE,
> > @@ -50,6 +50,7 @@ struct string_list;
> > ADVICE_WAITING_FOR_EDITOR,
> > ADVICE_SKIPPED_CHERRY_PICKS,
> > ADVICE_WORKTREE_ADD_ORPHAN,
> > + ADVICE_ADVICE_OFF,
> > };
> >
> > int git_default_advice_config(const char *var, const char *value);
> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> > index c13057a4ca..0b6a8b4a10 100755
> > --- a/t/t0018-advice.sh
> > +++ b/t/t0018-advice.sh
> > @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
> > test_must_be_empty actual
> > '
> >
> > +test_expect_success 'advice without the instructions to disable it' '
> > + cat >expect <<-\EOF &&
> > + hint: This is a piece of advice
> > + EOF
> > + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
> > + test_cmp expect actual
> > +'
>
> Looking at this test, I wonder why we don't imitate the existing style
> of:
>
> test_config advice.adviceOff false &&
> test-tool advise "This is a piece of advice" 2>actual &&
> test_cmp expect actual
As a reference, implicitly, that is:
git config advice.adviceOff false &&
test_when_finished "git config --unset-all advice.adviceOff" &&
test-tool advise "This is a piece of advice" 2>actual &&
test_cmp expect actual
I think the proposed test is better and understandable enough.
As a matter of fact, when I was thinking how to write the test I was
expecting to have a working "-c" in test-tool, as if we have a (not
expected) "git-advise".
So ...
>
> instead of teaching the test-tool helpers how to interpret `-c`
> arguments. Doing so would allow us to drop the first couple of patches
> in this series and simplify things a bit.
... IMHO the first two patches allows us to write simpler and more
intuitive tests based on test-tool.
I hope this argument persuades you, but I am not against your proposal.
>
> Thanks,
> Taylor
Thank you for reviewing this series.
^ permalink raw reply
* Re: Limited operations in unsafe repositories
From: Jeff King @ 2024-01-10 12:05 UTC (permalink / raw)
To: brian m. carlson; +Cc: git
In-Reply-To: <ZZr-JLxubCvWe0EU@tapette.crustytoothpaste.net>
On Sun, Jan 07, 2024 at 07:40:20PM +0000, brian m. carlson wrote:
> I had looked at sending a patch to make `git rev-parse` operate in a
> special mode where it's impossible to invoke any binaries at all, but
> unfortunately, `get_superproject_working_tree` invokes binaries, so
> that's not possible. (If anyone is interested in picking this up, there
> is a start on it, failing many tests, in the `rev-parse-safe-directory`
> on my GitHub remote.)
>
> I guess I'm looking for us to provide some basic functionality that is
> guaranteed to work in this case, including `git rev-parse` and `git
> config -l`. I don't think it's useful for every program that wants to
> work with Git to need to implement its own repository discovery and
> config parsing, and those are essential needs for tooling that needs to
> work with untrusted repositories.
If I understand correctly, you want to have some very limited code paths
that we know are OK to run even in an untrusted repo. My concern there
would be that it's easy for "basic functionality" to accidentally call
into less-limited code, which suddenly becomes a vulnerability.
My thinking is to flip that around: run all code, but put protection in
the spots that do unsafe things, like loading config or examining
hooks. I.e., a patch like this:
diff --git a/config.c b/config.c
index 9ff6ae1cb9..c7bbd6bdda 100644
--- a/config.c
+++ b/config.c
@@ -2026,7 +2026,7 @@ static int do_git_config_sequence(const struct config_options *opts,
if (!opts->git_dir != !opts->commondir)
BUG("only one of commondir and git_dir is non-NULL");
- if (opts->commondir) {
+ if (opts->commondir && is_safe_repository())
repo_config = mkpathdup("%s/config", opts->commondir);
worktree_config = mkpathdup("%s/config.worktree", opts->git_dir);
} else {
diff --git a/hook.c b/hook.c
index f6306d72b3..4fcfd82dc5 100644
--- a/hook.c
+++ b/hook.c
@@ -12,6 +12,9 @@ const char *find_hook(const char *name)
{
static struct strbuf path = STRBUF_INIT;
+ if (!is_safe_repository())
+ return NULL;
+
strbuf_reset(&path);
strbuf_git_path(&path, "hooks/%s", name);
if (access(path.buf, X_OK) < 0) {
where is_safe_repository() is something like:
- if git_env_bool("GIT_ASSUME_SAFE", 1) is true, return true
immediately
- otherwise, see if we are matched by safe.directory
And then running:
git --assume-unsafe rev-parse ...
would set that variable to "0".
And then most functionality would just work, modulo trusting repo hooks
and config. I mentioned "loading config" earlier, but of course that
patch is just touching the usual "load all config sources" code. We'd
still allow parsing .git/config for repo discovery, and something like:
git --assume-unsafe config -l --local
would still work if the caller knew they would be careful with the
output.
The downside, of course, is that we might fail to include other
dangerous spots. Those two are the big ones, I think, but would we want
to prevent people from pointing to /dev/mem in info/alternates? I dunno.
It certainly is more protection than we offer today, but maybe saying
"this is safe" would produce a false sense of security.
I do think something like git-prompt.sh could benefit here (it could use
--assume-unsafe for all invocations so you don't get surprised just by
chdir-ing into a downloaded tarball).
-Peff
^ permalink raw reply related
* Re: [PATCH] index-pack: spawn threads atomically
From: Jeff King @ 2024-01-10 11:44 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZZgvUyQK6X/MacDC@nand.local>
On Fri, Jan 05, 2024 at 11:33:23AM -0500, Taylor Blau wrote:
> - test_must_fail git index-pack --fix-thin --stdin <recoverable.pack
> + test_must_fail git index-pack --threads=1 --fix-thin --stdin <recoverable.pack
> [...]
> For what it's worth, I'm fine with either approach, mostly to avoid
> tying up more of the list's time discussing the options. But I have a
> vague preference towards `--threads=1` since it doesn't require us to
> touch production code.
That's quite tempting, actually. The flip side, though, is that the test
no longer reflects the production code as well. That is, in the real
world we'd still call exit() from a thread. That obviously works OK now
(modulo LSan), but if we ever had a regression where that left us in an
inconsistent state, we'd be less likely to notice it. Feels kind of
unlikely in practice, though.
I dunno. I guess the real least-bad thing is seeing if LSan can be
fixed to handle this atomically. I haven't even reported it there.
If do go with "--threads=1", I suspect several tests in that file need
it.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-10 11:39 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20240110110226.GC16674@coredump.intra.peff.net>
On 10-ene-2024 06:02:26, Jeff King wrote:
> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
>
> > Using advise_if_enabled() to display an advice will automatically
> > include instructions on how to disable the advice, along with the
> > main advice:
> >
> > hint: use --reapply-cherry-picks to include skipped commits
> > hint: Disable this message with "git config advice.skippedCherryPicks false"
> >
> > This can become distracting or noisy over time, while the user may
> > still want to receive the main advice.
> >
> > Let's have a switch to allow disabling this automatic advice.
>
> If I'm reading your patch correctly, this is a single option that
> controls the extra line for _all_ advice messages. But I'd have expected
> this to be something you'd want to set on a per-message basis. Here's my
> thinking.
>
> The original idea for advice messages was that they might be verbose and
> annoying, but if you had one that showed up a lot you'd choose to shut
> it up individually. But you wouldn't do so for _all_ messages, because
> you might benefit from seeing others (including new ones that get
> added). The "Disable this..." part was added later to help you easily
> know which config option to tweak.
>
> The expectation was that you'd fall into one of two categories:
>
> 1. You don't see the message often enough to care, so you do nothing.
>
> 2. You do find it annoying, so you disable this instance.
>
> Your series proposes a third state:
>
> 3. You find the actual hint useful, but the verbosity of "how to shut
> it up" is too much for you.
>
> That make sense to me, along with being able to partially shut-up a
> message. But wouldn't you still need the "how to shut up" hint for
> _other_ messages, since it's customized for each situation?
>
> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,
> you run "git config advice.adviseOff false".
>
> But now you run into another hint, like:
>
> $ git foo
> hint: you can use --empty-commits to deal with isn't as good as --xyzzy
>
> and you want to disable it entirely. Which advice.* config option does
> so? You're stuck trying to find it in the manpage (which is tedious but
> also kind of tricky since you're now guessing which name goes with which
> message). You probably do want:
>
> hint: Disable this message with "git config advice.xyzzy false"
>
> in that case (at which point you may decide to silence it completely or
> partially).
>
> Which implies to me that the value of advice.* should be a tri-state to
> match the cases above: true, false, or a "minimal" / "quiet" mode (there
> might be a better name). And then you'd do:
>
> git config advice.skippedCherryPicks minimal
>
> (or whatever it is called) to get the mode you want, without affecting
> advice.xyzzy.
Your reasoning is sensible, but I'm not sure if we want that level of
granularity. My guts doesn't feel it, though I'm not opposed.
In the message before yours in this thread, Junio proposed a similar
approach, and I've been thinking about it. Let me answer to your
comments from that message too.
>
> > advice.c | 3 ++-
> > advice.h | 3 ++-
> > t/t0018-advice.sh | 8 ++++++++
> > 3 files changed, 12 insertions(+), 2 deletions(-)
>
> Speaking of manpages, we'd presumably need an update to
> Documentation/config/advice.txt. :)
This has made me jump first to this message in the thread ... I missed
this. Thank you!
>
> -Peff
^ permalink raw reply
* Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph
From: Jeff King @ 2024-01-10 11:39 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Scott Leggett
In-Reply-To: <ZZg3YIEDCjbbGyVX@nand.local>
On Fri, Jan 05, 2024 at 12:07:44PM -0500, Taylor Blau wrote:
> > I do think there's some fragility here that we might want to follow up
> > on. We have a global commit_graph_data_slab that contains graph
> > positions, and our global commit structs depend on the that slab
> > remaining valid. But close_commit_graph() is just about closing _one_
> > object store's graph. So it's dangerous to call that function and clear
> > the slab without also throwing away any "struct commit" we might have
> > parsed that depends on it.
>
> I definitely agree that there seems like a high risk of another
> potentially-dangerous bug happening as a result of this area.
>
> One thing that sticks out to me is that we have a scope mismatch
> between the commit structs, the raw_object_store, and the commit slabs.
> Slabs are tied to the lifetime of the raw_object_store, but the commit
> objects are tied to the global lifetime of the process. I wonder if it
> would make sense to have a slab per object-store, and require that you
> pass both the commit *and* the object-store you're looking it up in as
> arguments to any slab-related functions.
Right, that scope mismatch is the crux of the issue. I had imagined that
if you're really closing the object database you might want to just
throw away all of the object structs. But we don't have a way to do that
(and in general I'd be slightly scared if we did, since lots of code
likes to think of object pointers as immutable).
I don't know if your solution works, though. If I "partially" load a
commit (i.e., its "parsed" flag is 1, but maybe_tree is still NULL, not
having been loaded from the graph yet), then that commit object depends
on the slab. If we later ask the commit about its tree, then it needs to
refer to the slab to see if it came from the graph. Right now that
happens in the global slab. If we did it in a per-object-store slab,
we'd need each commit to know which object store it came from!
Of course we do have a "struct repository" argument in
repo_get_commit_tree(). So...maybe that's enough? It still doesn't get
us all the way there, though. In this case the culprit was submodules,
and we really would be closing an alternate repo. But we have actual
calls to close_commit_graph() sprinkled around. Any code which closes
any commit-graph and then still accesses a "struct commit" is
potentially buggy.
I think a simpler solution would be that upon clearing the slab, we
either "finish" each commit (filling in the maybe_tree field) or
"unparse" it (unsetting the parsed flag, and then doing a regular and/or
graph lookup if it is accessed again later).
It should be easy-ish to iterate through the slab and look at the
commits that are mentioned in it. Though maybe not? Each commit knows
its slab-id, but I'm not sure if we have a master list of commits to go
the other way.
So yet another alternative is: don't clear the slab. Mark the entries as
"this came from a graph, but we don't know the correct graph position
anymore" (presumably with a new sentinel value for data->graph_pos). And
then functions like repo_get_commit_tree() would know that they need to
try harder (e.g., doing the unparse then and going back to the object
store to parse again).
Hmm. Actually, maybe COMMIT_NOT_FROM_GRAPH already functions that way.
If we see a commit that is parsed but has a NULL maybe_tree, then if we
see COMMIT_NOT_FROM_GRAPH we assume it's just a corrupt commit and
return NULL. But we could just try harder to re-parse it then. Like:
diff --git a/commit.c b/commit.c
index ef679a0b93..909898cf7f 100644
--- a/commit.c
+++ b/commit.c
@@ -423,6 +423,24 @@ struct tree *repo_get_commit_tree(struct repository *r,
if (commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)
return get_commit_tree_in_graph(r, commit);
+ /*
+ * This is either a corrupt commit, or one which we partially loaded
+ * from a graph file but then subsequently threw away the graph data.
+ *
+ * Optimistically assume it's the latter and try to reload from
+ * scratch. This gives a performance penalty if it really is a corrupt
+ * commit, but presumably that happens rarely.
+ *
+ * This will throw away the parents list, which is potentially sketchy.
+ * A better version of this would just unset commit->object.parsed
+ * and then do a minimal version of parse_commit() that _just_ loads
+ * the tree data (and/or graph position if available).
+ *
+ * Naturally we'd need to drop the "const" from our commit above, too.
+ */
+ unparse_commit(r, &commit->object.oid);
+ repo_parse_commit(r, commit);
+
return NULL;
}
I dunno. I do feel like this is a lurking maintenance headache, and
might even be a triggerable bug. But without knowing of a way that it
happens in the current code base, it feels like it would be easy to make
things worse rather than better.
I'm content to leave it for now and let this discussion serve as a
reference if somebody does manage to run into it in practice.
-Peff
^ permalink raw reply related
* Re: [GSOC][RFC] Add more builtin patterns for userdiff, as Mircroproject.
From: Christian Couder @ 2024-01-10 11:33 UTC (permalink / raw)
To: Sergius Nyah; +Cc: git, gitster@pobox.com
In-Reply-To: <CANAnif95ux=vCNCKbVw0q_vYamQRkbFqSa9_-u6xRvK6r+2a+Q@mail.gmail.com>
Hi Sergius,
On Tue, Jan 9, 2024 at 8:59 PM Sergius Nyah <sergiusnyah@gmail.com> wrote:
>
> Hello everyone,
> I'm Sergius, a Computer Science undergraduate student, and I want to
> begin Contributing to the Git project. So far, I've gone through
> Matheus' tutorial on First steps Contributing to Git, and I found it
> very helpful. I've also read the Contribution guidelines keenly and
> built Git from source.
Thanks for your interest in contributing to Git!
> In accordance to the contributor guidelines, I came across this
> Mircoproject idea from: https://git.github.io/SoC-2022-Microprojects/
s/Mircoproject/microproject/
There is a similar typo in the subject of your email too.
> which I'm willing to work on. It talked about enhancing Git's
> "userdiff" feature in "userdiff.c" which is crucial for identifying
> function names in various programming languages, thereby improving the
> readability of "git diff" outputs.
>
> From my understanding, the project involves extending the `userdiff`
> feature to support additional programming languages that are currently
> not covered such as Shell, Swift, Go and the others.
As far as I can see in userdiff.c, Golang and Bash seem to be supported.
> Here is a sample of how a language is defined in `userdiff.c`:
>
> > #define PATTERNS(lang, rx, wrx) { \
> > .name = lang, \
> > .binary = -1, \
> > .funcname = { \
> > .pattern = rx, \
> > .cflags = REG_EXTENDED, \
> > }, \
> > .word_regex = wrx "|[^[:space:]]|[\xc0-\xff][\x80-\xbf]+", \
> > .word_regex_multi_byte = wrx "|[^[:space:]]", \
> > }
>
> In this code, `lang` is the name of the language, `rx` is the regular
> expression for identifying function names, and `wrx` is the word
> regex.
>
> Approach: I Identified the Programming Languages that are not
> currently supported by the userdiff feature by reviewing the existing
> patterns in userdiff.c and comparing them with some popular
> programming languages.
> For each supported language, I would define a regular expression that
> could help identify function names in that language. This could
> include researching each language's syntax and testing their
> expressions to ensure that they work well.
In your microproject, you only need to add support for ONE language
that is not supported yet. Please don't try to do more than that.
> Also, I'd add a new IPATTERN definition for each language to the
> "userdiff.c" file, then rebuild Git and test the changes by creating a
> repo with files in the newly supported languages then run "git diff"
> to ensure the line @@ ... @@ produces their correct function names.
> Then submit a patch.
Except for my comments above, this looks like a good plan. Thanks.
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Jeff King @ 2024-01-10 11:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Haller, git
In-Reply-To: <xmqq34v7lmb3.fsf@gitster.g>
On Mon, Jan 08, 2024 at 10:20:00AM -0800, Junio C Hamano wrote:
> Stefan Haller <lists@haller-berlin.de> writes:
>
> > Our git client (lazygit) has a need to store per-repo config files that
> > override the global one, much like git itself. The easiest way to do
> > that is to store those in a .git/lazygit.cfg file, and I'm wondering if
> > there's any reason why this is a bad idea?
>
> An obvious alternative is to have .lazygit directory next to .git directory
> which would give you a bigger separation, which can cut both ways.
Just to spell out one of those ways: unlike ".git", we will happily
check out ".lazygit" from an untrusted remote repository. That may be a
feature if you want to be able to share project-specific config, or it
might be a terrible security vulnerability if lazygit config files can
trigger arbitrary code execution.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Jeff King @ 2024-01-10 11:02 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <d6099d78-43c6-4709-9121-11f84228cf91@gmail.com>
On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the
> main advice:
>
> hint: use --reapply-cherry-picks to include skipped commits
> hint: Disable this message with "git config advice.skippedCherryPicks false"
>
> This can become distracting or noisy over time, while the user may
> still want to receive the main advice.
>
> Let's have a switch to allow disabling this automatic advice.
If I'm reading your patch correctly, this is a single option that
controls the extra line for _all_ advice messages. But I'd have expected
this to be something you'd want to set on a per-message basis. Here's my
thinking.
The original idea for advice messages was that they might be verbose and
annoying, but if you had one that showed up a lot you'd choose to shut
it up individually. But you wouldn't do so for _all_ messages, because
you might benefit from seeing others (including new ones that get
added). The "Disable this..." part was added later to help you easily
know which config option to tweak.
The expectation was that you'd fall into one of two categories:
1. You don't see the message often enough to care, so you do nothing.
2. You do find it annoying, so you disable this instance.
Your series proposes a third state:
3. You find the actual hint useful, but the verbosity of "how to shut
it up" is too much for you.
That make sense to me, along with being able to partially shut-up a
message. But wouldn't you still need the "how to shut up" hint for
_other_ messages, since it's customized for each situation?
E.g., suppose that after getting annoyed at advice.skippedCherryPicks,
you run "git config advice.adviseOff false".
But now you run into another hint, like:
$ git foo
hint: you can use --empty-commits to deal with isn't as good as --xyzzy
and you want to disable it entirely. Which advice.* config option does
so? You're stuck trying to find it in the manpage (which is tedious but
also kind of tricky since you're now guessing which name goes with which
message). You probably do want:
hint: Disable this message with "git config advice.xyzzy false"
in that case (at which point you may decide to silence it completely or
partially).
Which implies to me that the value of advice.* should be a tri-state to
match the cases above: true, false, or a "minimal" / "quiet" mode (there
might be a better name). And then you'd do:
git config advice.skippedCherryPicks minimal
(or whatever it is called) to get the mode you want, without affecting
advice.xyzzy.
> advice.c | 3 ++-
> advice.h | 3 ++-
> t/t0018-advice.sh | 8 ++++++++
> 3 files changed, 12 insertions(+), 2 deletions(-)
Speaking of manpages, we'd presumably need an update to
Documentation/config/advice.txt. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] t7450: test submodule urls
From: Jeff King @ 2024-01-10 10:38 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <cf7848edffca27931aad02c0652adf2715320d35.1704822817.git.gitgitgadget@gmail.com>
On Tue, Jan 09, 2024 at 05:53:36PM +0000, Victoria Dye via GitGitGadget wrote:
> +#define TEST_TOOL_CHECK_URL_USAGE \
> + "test-tool submodule check-url <url>"
I don't think this command-line "<url>" mode works at all. Your
underlying function can handle either stdin or arguments:
> -static int check_name(int argc, const char **argv)
> +static int check_submodule(int argc, const char **argv, check_fn_t check_fn)
> {
> if (argc > 1) {
> while (*++argv) {
> - if (check_submodule_name(*argv) < 0)
> + if (check_fn(*argv) < 0)
> return 1;
> }
> } else {
> struct strbuf buf = STRBUF_INIT;
> while (strbuf_getline(&buf, stdin) != EOF) {
> - if (!check_submodule_name(buf.buf))
> + if (!check_fn(buf.buf))
> printf("%s\n", buf.buf);
> }
> strbuf_release(&buf);
...but the new caller rejects them before we get there:
> +static int cmd__submodule_check_url(int argc, const char **argv)
> +{
> + struct option options[] = {
> + OPT_END()
> + };
> + argc = parse_options(argc, argv, "test-tools", options,
> + submodule_check_url_usage, 0);
> + if (argc)
> + usage_with_options(submodule_check_url_usage, options);
> +
> + return check_submodule(argc, argv, check_submodule_url);
> }
So you'd want at least:
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index da89d265f0..6b964c88ab 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -88,8 +88,6 @@ static int cmd__submodule_check_url(int argc, const char **argv)
};
argc = parse_options(argc, argv, "test-tools", options,
submodule_check_url_usage, 0);
- if (argc)
- usage_with_options(submodule_check_url_usage, options);
return check_submodule(argc, argv, check_submodule_url);
}
but then that reveals another mismatch. In check_submodule() above we
expect argv[0] to be uninteresting (i.e., the name of the program), but
parse_options() will already have thrown it away. So we silently fail to
check the first option (which is especially bad since the only output is
the exit code, and thus the skipped one looks the same as one that
validated correctly).
All of this is inherited from the existing check_name() code, which I
think has all of the same bugs. The test scripts all just use the stdin
mode, so they don't notice. It's not too hard to fix, but maybe it's
worth just ripping out the unreachable code.
-Peff
^ permalink raw reply related
* Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs
From: Jeff King @ 2024-01-10 10:23 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <pull.1635.git.1704822817.gitgitgadget@gmail.com>
On Tue, Jan 09, 2024 at 05:53:34PM +0000, Victoria Dye via GitGitGadget wrote:
> While testing 'git fsck' checks on .gitmodules URLs, I noticed that some
> invalid URLs were passing the checks. Digging into it a bit more, the issue
> turned out to be that 'credential_from_url_gently()' parses certain URLs
> (like "http://example.com:something/deeper/path") incorrectly, in a way that
> appeared to return a valid result.
I don't think that checks was ever intended to be an overall URL-quality
check. The reason we used the credential code in the fsck check is that
we were checking for URLs which triggered a specific credential-related
vulnerability.
I don't mind tightening things further as long as:
1. We are not allowing any cases that the credential code would have
forbidden (i.e., something that might let the vulnerability slip
through, since ultimately it is the credential code which will need
to be protected). You ported over the newline check, which is the
main thing. It's possible that there is some difference between the
two parsers that may allow an invalid input to create a newline for
one but not the other, but having now looked over the code, I don't
think so.
And I think one could argue that the security-importance of the
fsck check has mostly run its course. The real fix was in the
credential code itself, and the matching fsck change was mostly
about protecting downstream clients until they were upgraded. Now
that it's been several years, there's not as much value there.
2. It is not making it harder for users to work with repositories that
may contain malformed URLs that _aren't_ vulnerabilities. It sounds
like the specific cases you found already don't work at all with
Git, so presumably nobody is using them. By making it an fsck
check, though, any mistakes that are embedded in history (even if
they are now corrected) will make it a pain to use the repository
with sites that enable transfer.fsckObjects.
My gut feeling is that this is probably OK in practice. If it does
cause pain, we might consider loosening the fsck.gitmodulesUrl
severity (under the notion from above that it is no longer a
critical security check). But if it doesn't cause real-world pain,
being pickier is probably better (it may save us from a
vulnerability down the road).
-Peff
^ permalink raw reply
* Re: Bug: Git spawns subprocesses on windows
From: Domen Golob @ 2024-01-10 9:29 UTC (permalink / raw)
To: brian m. carlson, Domen Golob, git
In-Reply-To: <ZZ3PB9mq8GTpGITC@tapette.crustytoothpaste.net>
Thanks for your answer, indeed this does not happen on Unix, and I
don't have this issue there.
Will report the bug to the windows team.
Thanks.
On Tue, 9 Jan 2024 at 23:56, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2024-01-09 at 15:33:43, Domen Golob wrote:
> > Hello,
> > the problem is exactly the same as what was reported here on stackoverflow:
> > c# - Git for windows seems to create sub-processes that never die -
> > Stack Overflow
> > https://stackoverflow.com/questions/69579065/git-for-windows-seems-to-create-sub-processes-that-never-die
> >
> > Additionally I have found out that:
> > - a Git for Windows subprocess is created for each repository and
> > every time a git command is issued this process grows in size and it
> > never dies.
> > - you cannot delete the .git folder from the terminal, but it works
> > via file explorer
> > - deleting the .git folder kills the Git for windows process
> > - creating changes in several repos creates multiple processes, and
> > sometimes the process is created as a subprocess
>
> You probably want to contact Git for Windows at
> https://github.com/git-for-windows/git. The reason I suggest that is
> that this is usually not a behaviour we see on Unix, and seems to be
> Windows-specific.
>
> I don't know if it's possible to see command-line arguments of processes
> on Windows like it is with `ps` on Unix, but including that information
> if possible will be very useful to the maintainers. Without knowing
> that information, it's very unlikely that anyone will be able to help
> you since we don't know what's going on.
>
> There are some possibilities of what's going on here:
>
> * git gc is running.
> * git maintenance or the fsmonitor is configured and is running.
> * You have a non-default antivirus or monitoring software that causes
> processes to hang or not complete, so one of Git's subprocesses can't
> exit. If you're using such software, we usually recommend completely
> removing it completely (disabling it is usually not sufficient),
> rebooting, and testing again to make sure it's not the problem.
> * You have some other process which is running which spawns Git commands
> (editors, content indexers, etc.).
> * We actually have a bug and some process is not waited for correctly,
> and zombie processes manifest differently on Windows than on Unix.
> * Something else I haven't considered.
>
> However, as I said, you'll probably want to contact the Git for Windows
> folks through their issue tracker.
> --
> brian m. carlson (he/him or they/them)
> Toronto, Ontario, CA
^ permalink raw reply
* [PATCH v2 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific
From: Patrick Steinhardt @ 2024-01-10 9:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704877233.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.
Mark the test suites to depend on the REFFILES backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1409-avoid-packing-refs.sh | 6 ++++++
t/t3210-pack-refs.sh | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
# Add an identifying mark to the packed-refs file header line. This
# shouldn't upset readers, and it should be omitted if the file is
# ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
test_expect_success 'enable reflogs' '
git config core.logallrefupdates true
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 5/6] t5526: break test submodule differently
From: Patrick Steinhardt @ 2024-01-10 9:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704877233.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]
In 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
recursive fetch with submodule in the case where the submodule is broken
due to whatever reason. The test to exercise that the fix works breaks
the submodule by deleting its `HEAD` reference, which will cause us to
not detect the directory as a Git repository.
While this is perfectly fine in theory, this way of breaking the repo
becomes problematic with the current efforts to introduce another refdb
backend into Git. The new reftable backend has a stub HEAD file that
always contains "ref: refs/heads/.invalid" so that tools continue to be
able to detect such a repository. But as the reftable backend will never
delete this file even when asked to delete `HEAD` the current way to
delete the `HEAD` reference will stop working.
Adapt the code to instead delete the objects database. Going back with
this new way to cause breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5526-fetch-submodules.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
git -C dst fetch --recurse-submodules &&
# Break the receiving submodule
- test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
+ rm -r dst/sub/.git/objects &&
# NOTE: without the fix the following tests will recurse forever!
# They should terminate with an error.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 4/6] t1419: mark test suite as files-backend specific
From: Patrick Steinhardt @ 2024-01-10 9:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704877233.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]
With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
any interest to the caller, which can avoid quite some allocations and
object lookups.
This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
file, but not for loose references. Consequently, all callers must still
filter emitted refs with those exclude patterns.
The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.
An alternative would be to introduce a new prereq that tells us whether
the backend under test supports exclude patterns or not. But this does
feel a bit overblown:
- It would either map to the REFFILES prereq, in which case it feels
overengineered because the prereq is only ever relevant to t1419.
- Otherwise, it could auto-detect whether the backend supports exclude
patterns. But this could lead to silent failures in case the support
for this feature breaks at any point in time.
It should thus be good enough to just use the REFFILES prereq for now.
If future backends ever grow support for exclude patterns we can easily
add their respective prereq as another condition for this test suite to
execute.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1419-exclude-refs.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
+ test_done
+fi
+
for_each_ref__exclude () {
GIT_TRACE2_PERF=1 test-tool ref-store main \
for-each-ref--exclude "$@" >actual.raw
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/6] t1302: make tests more robust with new extensions
From: Patrick Steinhardt @ 2024-01-10 9:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704877233.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]
In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
like the newly introduced "refStorage" extension.
Refactor the tests to be more robust:
- Check the DEFAULT_REPO_FORMAT prereq to determine the expected
repository format version. This helps to ensure that we only need to
update the prereq in a central place when new extensions are added.
- Use a separate repository to rewrite ".git/config" to test
combinations of the repository format version and extensions. This
ensures that we don't break the main test repository.
- Do not rewrite ".git/config" when exercising the "preciousObjects"
extension.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1302-repo-version.sh | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 179474fa65..7c680c91c4 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -28,7 +28,12 @@ test_expect_success 'setup' '
'
test_expect_success 'gitdir selection on normal repos' '
- test_oid version >expect &&
+ if test_have_prereq DEFAULT_REPO_FORMAT
+ then
+ echo 0
+ else
+ echo 1
+ fi >expect &&
git config core.repositoryformatversion >actual &&
git -C test config core.repositoryformatversion >actual2 &&
test_cmp expect actual &&
@@ -79,8 +84,13 @@ mkconfig () {
while read outcome version extensions; do
test_expect_success "$outcome version=$version $extensions" "
- mkconfig $version $extensions >.git/config &&
- check_${outcome}
+ test_when_finished 'rm -rf extensions' &&
+ git init extensions &&
+ (
+ cd extensions &&
+ mkconfig $version $extensions >.git/config &&
+ check_${outcome}
+ )
"
done <<\EOF
allow 0
@@ -94,7 +104,8 @@ allow 1 noop-v1
EOF
test_expect_success 'precious-objects allowed' '
- mkconfig 1 preciousObjects >.git/config &&
+ git config core.repositoryFormatVersion 1 &&
+ git config extensions.preciousObjects 1 &&
check_allow
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific
From: Patrick Steinhardt @ 2024-01-10 9:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704877233.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.
Mark the test accordingly with the REFFILES prereq.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1301-shared-repo.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
test_cmp expect actual
'
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
umask 077 &&
git config core.sharedRepository group &&
git reflog expire --all &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends
From: Patrick Steinhardt @ 2024-01-10 9:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704877233.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4258 bytes --]
The t1300 test suite exercises the git-config(1) tool. To do so we
overwrite ".git/config" to contain custom contents. While this is easy
enough to do, it may create problems when using a non-default repository
format because we also overwrite the repository format version as well
as any potential extensions. With the upcoming "reftable" ref backend
the result is that we may try to access refs via the "files" backend
even though the repository has been initialized with the "reftable"
backend.
Refactor tests which access the refdb to be more robust by using their
own separate repositories, which allows us to be more careful and not
discard required extensions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------
1 file changed, 48 insertions(+), 26 deletions(-)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e2752134..53c3d65823 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
'
test_expect_success 'check split_cmdline return' "
- git config alias.split-cmdline-fix 'echo \"' &&
- test_must_fail git split-cmdline-fix &&
- echo foo > foo &&
- git add foo &&
- git commit -m 'initial commit' &&
- git config branch.main.mergeoptions 'echo \"' &&
- test_must_fail git merge main
+ test_when_finished 'rm -rf repo' &&
+ git init repo &&
+ (
+ cd repo &&
+ git config alias.split-cmdline-fix 'echo \"' &&
+ test_must_fail git split-cmdline-fix &&
+ echo foo >foo &&
+ git add foo &&
+ git commit -m 'initial commit' &&
+ git config branch.main.mergeoptions 'echo \"' &&
+ test_must_fail git merge main
+ )
"
test_expect_success 'git -c "key=value" support' '
@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '
'
test_expect_success 'aliases can be CamelCased' '
- test_config alias.CamelCased "rev-parse HEAD" &&
- git CamelCased >out &&
- git rev-parse HEAD >expect &&
- test_cmp expect out
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ git config alias.CamelCased "rev-parse HEAD" &&
+ git CamelCased >out &&
+ git rev-parse HEAD >expect &&
+ test_cmp expect out
+ )
'
test_expect_success 'git -c does not split values on equals' '
@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
'
test_expect_success 'set up custom config file' '
- CUSTOM_CONFIG_FILE="custom.conf" &&
- cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+ cat >"custom.conf" <<-\EOF &&
[user]
custom = true
EOF
+ CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
'
test_expect_success !MINGW 'set up custom config file with special name characters' '
@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '
'
test_expect_success '--show-origin blob' '
- blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
- cat >expect <<-EOF &&
- blob:$blob user.custom=true
- EOF
- git config --blob=$blob --show-origin --list >output &&
- test_cmp expect output
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+ cat >expect <<-EOF &&
+ blob:$blob user.custom=true
+ EOF
+ git config --blob=$blob --show-origin --list >output &&
+ test_cmp expect output
+ )
'
test_expect_success '--show-origin blob ref' '
- cat >expect <<-\EOF &&
- blob:main:custom.conf user.custom=true
- EOF
- git add "$CUSTOM_CONFIG_FILE" &&
- git commit -m "new config file" &&
- git config --blob=main:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
- test_cmp expect output
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ cat >expect <<-\EOF &&
+ blob:main:custom.conf user.custom=true
+ EOF
+ cp "$CUSTOM_CONFIG_FILE" custom.conf &&
+ git add custom.conf &&
+ git commit -m "new config file" &&
+ git config --blob=main:custom.conf --show-origin --list >output &&
+ test_cmp expect output
+ )
'
test_expect_success '--show-origin with --default' '
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 0/6] t: mark "files"-backend specific tests
From: Patrick Steinhardt @ 2024-01-10 9:01 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704802213.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5069 bytes --]
Hi,
this is the second version of my patch series that addresses tests which
are specific to the "files" backend. Changes compared to v1:
- I've rewritten the patch addressing t1300 to not mark tests as
repository format specific anymoreg. Instead, we now create separate
repos for relevant tests where we are more careful to not discard
extensions.
- I've made the casing of config options more consistent.
- I've extended some commit messages to hopefully explain better why
I'm doing things the way I do them and fixed some typos.
Thanks for your feedback!
Patrick
Patrick Steinhardt (6):
t1300: make tests more robust with non-default ref backends
t1301: mark test for `core.sharedRepository` as reffiles specific
t1302: make tests more robust with new extensions
t1419: mark test suite as files-backend specific
t5526: break test submodule differently
t: mark tests regarding git-pack-refs(1) to be backend specific
t/t1300-config.sh | 74 +++++++++++++++++++++++------------
t/t1301-shared-repo.sh | 2 +-
t/t1302-repo-version.sh | 19 +++++++--
t/t1409-avoid-packing-refs.sh | 6 +++
t/t1419-exclude-refs.sh | 6 +++
t/t3210-pack-refs.sh | 6 +++
t/t5526-fetch-submodules.sh | 2 +-
7 files changed, 83 insertions(+), 32 deletions(-)
Range-diff against v1:
1: ec1b5bdd17 < -: ---------- t1300: mark tests to require default repo format
-: ---------- > 1: 0552123fa3 t1300: make tests more robust with non-default ref backends
2: 68e308c200 = 2: 384250fec2 t1301: mark test for `core.sharedRepository` as reffiles specific
3: 9af1e418d4 ! 3: 1284b70fab t1302: make tests more robust with new extensions
@@ t/t1302-repo-version.sh: allow 1 noop-v1
test_expect_success 'precious-objects allowed' '
- mkconfig 1 preciousObjects >.git/config &&
-+ git config core.repositoryformatversion 1 &&
++ git config core.repositoryFormatVersion 1 &&
+ git config extensions.preciousObjects 1 &&
check_allow
'
4: d7c6b8b2a7 ! 4: c6062b612c t1419: mark test suite as files-backend specific
@@ Commit message
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
- any interest to the caller, which can avoid quite some allocaitons and
+ any interest to the caller, which can avoid quite some allocations and
object lookups.
This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
- file, but not for loose references. Consequentially, all callers must
- still filter emitted refs with those exclude patterns.
+ file, but not for loose references. Consequently, all callers must still
+ filter emitted refs with those exclude patterns.
The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.
+ An alternative would be to introduce a new prereq that tells us whether
+ the backend under test supports exclude patterns or not. But this does
+ feel a bit overblown:
+
+ - It would either map to the REFFILES prereq, in which case it feels
+ overengineered because the prereq is only ever relevant to t1419.
+
+ - Otherwise, it could auto-detect whether the backend supports exclude
+ patterns. But this could lead to silent failures in case the support
+ for this feature breaks at any point in time.
+
+ It should thus be good enough to just use the REFFILES prereq for now.
+ If future backends ever grow support for exclude patterns we can easily
+ add their respective prereq as another condition for this test suite to
+ execute.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## t/t1419-exclude-refs.sh ##
5: 51e494a50e ! 5: ae08afc459 t5526: break test submodule differently
@@ Commit message
delete the `HEAD` reference will stop working.
Adapt the code to instead delete the objects database. Going back with
- this new way to cuase breakage confirms that it triggers the infinite
+ this new way to cause breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.
6: a9620f329d = 6: df648be535 t: mark tests regarding git-pack-refs(1) to be backend specific
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* New Feature Request: Rating control (stars) · Issue #3054 · microsoft/AdaptiveCards · GitHub
From: ross nicholas Oneil thomas @ 2024-01-10 8:15 UTC (permalink / raw)
To: GitHub Developer Support, Github email, zimmermanda@sec.gov,
digital_identity@nist.gov, GitHub, Ross Nicholas Oneil Thomas,
ross nicholas Oneil thomas, jmap-owner@ietf.org
HelloO World 12/10/2023
From: base_64;
ATTENTION NEEDS CORRECTED
Adaptive card is and was altered for GitHub and software is like it compared back to normal! I would mess with it but I’m about to hold management at GitHub liable for potentially having a scam or phishing attack on my corporate accounts and corporation, GitHub.
Soundly I have no power of attorney, no property restrictions or control over me legal or financial and or mentally.
Please make this account back to what should be of as for the adaptive card edited modeled by Matt whoever,
Thanks domain owner,
Ross Nicholas Oneil Thomas
California
DOB:11/14/1988
https://github.com/microsoft/AdaptiveCards/issues/3054
Ross Nicholas Oneil Thomas
www.github.com
www.coinbase.com
www.jsnull.com
(559-816-2950
^ permalink raw reply
* Re: [PATCH 5/6] t5526: break test submodule differently
From: Patrick Steinhardt @ 2024-01-10 7:41 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cTB5OH1hCD-EagxNAcaw1=RR7yCeeZ_AzeqHtFTGxT-0g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]
On Tue, Jan 09, 2024 at 02:23:55PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > In 10f5c52656 (submodule: avoid auto-discovery in
> > prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
> > recursive fetch with submodule in the case where the submodule is broken
> > due to whatever reason. The test to exercise that the fix works breaks
> > the submodule by deleting its `HEAD` reference, which will cause us to
> > not detect the directory as a Git repository.
> >
> > While this is perfectly fine in theory, this way of breaking the repo
> > becomes problematic with the current efforts to introduce another refdb
> > backend into Git. The new reftable backend has a stub HEAD file that
> > always contains "ref: refs/heads/.invalid" so that tools continue to be
> > able to detect such a repository. But as the reftable backend will never
> > delete this file even when asked to delete `HEAD` the current way to
> > delete the `HEAD` reference will stop working.
>
> This patch is not the appropriate place to bikeshed (but since this is
> the first time I've read or paid attention to it), if I saw "ref:
> refs/heads/.invalid", the word ".invalid" would make me think
> something was broken in my repository. Would it make sense to use some
> less alarming word, such as perhaps ".placeholder", ".stand-in",
> ".synthesized" or even the name of the non-file-based backend in use?
Well, something _is_ broken in your repository in case Git ever tries to
read the "HEAD" placeholder in a reftable-enabled repository. But I
guess you rather come from the angle of using `cat HEAD` as a user. I do
agree that using a better hint could help users, but this detail has
already been recorded as such in "Documentation/technical/reftable.txt".
We can of course change this to be "ref: refs/heads/.reftable", but as
you already say this is something that should be discussed in a separate
thread.
> > Adapt the code to instead delete the objects database. Going back with
> > this new way to cuase breakage confirms that it triggers the infinite
> > recursion just the same, and there are no equivalent ongoing efforts to
> > replace the object database with an alternate backend.
>
> s/cuase/cause/
>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
> > # Break the receiving submodule
> > - test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
> > + rm -r dst/sub/.git/objects &&
>
> If there is no guarantee that .git/objects will exist when any
> particular backend is in use, would it be more robust to use -f here,
> as well?
>
> rm -rf dst/sub/.git/objects &&
`.git/objects` always exists in a healthy repository. If it doesn't,
then `is_git_directory()` would return a false-ish value and we wouldn't
recognize the repository as such. Or are you saying that this could
potentially change in the future if there was ever to be an alternate
ODB format? If so that is a valid remark, but the test would break
regardless of whether we use `-f` or not: if a missing ".git/objects"
directory does not lead to a corrupted repository then the whole premise
of the test is broken.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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