* Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions
From: Patrick Steinhardt @ 2024-01-23 15:18 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <87a5owqgzi.fsf@iotcl.com>
[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]
On Tue, Jan 23, 2024 at 03:08:00PM +0100, Toon Claes wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > [[PGP Signed Part:Undecided]]
> > [1. text/plain]
> > 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
> > @@ -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 &&
>
> Why did you not remove the use of `mkconfig` here?
I guess you mean stop using `mkconfig` and instead use git-config(1) to
manually write only the repository format version as well as extensions?
The problem here is that the resulting configuration would be dependent
on some environment variables, like `GIT_TEST_DEFAULT_HASH` and the
refdb equivalent `GIT_TEST_DEFAULT_REF_FORMAT` which do end up in the
repo's configuration. So I think it's actually preferable to overwrite
the complete ".git/config" so that is exactly in the expected state.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends
From: Patrick Steinhardt @ 2024-01-23 15:22 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <87ede8qhw9.fsf@iotcl.com>
[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]
On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > [[PGP Signed Part:Undecided]]
> > [1. text/plain]
> > 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>
> > ---
>
> > @@ -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)"
> > '
>
> From the commit message it was not clear to me this change was needed.
> Do you think it's worth it to add something to the commit message
> explaining you now need to copy the custom.conf into each seperate
> repository?
Good point in fact. The problem here is that before, CUSTOM_CONFIG_FILE
was using a relative path that wouldn't be found when cd'ing into the
respective subrepositories. By using `path-utils real_path` we resolve
the relative path to the full path, and thus we can find the file
regardless of our shell's current working directory.
Not sure whether this is worth a reroll, but in case you or others think
that it is then I'm happy to add this explanation.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Fatal error: commit-graph-chain.lock': File exists.
From: Thomas Braun @ 2024-01-23 15:01 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
Since 2.43.0 I'm seeing the following error message when I fetch from a remote:
$LC_ALL=C git fetch --all
remote: Enumerating objects: 446, done.
remote: Counting objects: 100% (446/446), done.
remote: Compressing objects: 100% (135/135), done.
remote: Total 446 (delta 328), reused 415 (delta 311), pack-reused 0
Receiving objects: 100% (446/446), 432.84 KiB | 1.01 MiB/s, done.
Resolving deltas: 100% (328/328), completed with 29 local objects.
From EEE:XXX/YYY
* [new branch] bugfix/ZZZ -> origin/ZZZ
* [new branch] bugfix/ZZZ -> origin/bugfix/ZZZ
+ 7244c9b68...6dad36582 feature/AAA -> origin/feature/AAA (forced update)
+ 7ce96b207...7bee890d6 feature/BBB -> origin/feature/BBB (forced update)
* [new branch] feature/CCC -> origin/feature/CCC
fd8c36a84..2ab4db03d main -> origin/main
fatal: Unable to create '/DDD/.git/objects/info/commit-graphs/commit-graph-chain.lock': File exists.
Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.
Observations:
- repo and gitforge independent
- only happens when fetch.writeCommitGraph is turned on
- git gc cleans up the stale lock file
- I think this is only happening when multiple branches on the remote have changed
I do have protocol.version set to 2.
I tried replicating the issue with an example but failed so far.
Does that ring a bell somewhere? Where should I start poking?
Thanks,
Thomas
[System Info]
git Version:
git version 2.43.0
cpu: x86_64
built from commit: 564d0252ca632e0264ed670534a51d18a689ef5d
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.1.0-13-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.55-1 (2023-09-29) x86_64
Compiler Info: gnuc: 12.2
libc Info: glibc: 2.36
$SHELL (typically, interactive shell): /bin/bash
^ permalink raw reply
* Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends
From: Christian Couder @ 2024-01-23 16:15 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <0552123fa30243d6d8d6b378991651dd6ade7de3.1704877233.git.ps@pks.im>
On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents.
Here "we" means "tests in t1300" I guess.
> 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.
But I am not sure that "we" in the above sentence is also "tests in
t1300". I think overwriting the repo format version and potential
extensions is done by other tests, right? Anyway it would be nice to
clarify this.
> 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.
Not sure here also what "we" is. When could refs be accessed via the
"files" backend even though the repo was initialized with the
"reftable" backend? Does this mean that some of the tests in t1300 try
to access refs via the "files" backend while we may want to run all
the tests using 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.
Not sure what exactly is discarding extensions. Also robust is not
very clear. It would be better to give at least an example of how
things could fail.
> 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
> + )
> "
Maybe, while at it, this test could be modernized to use single quotes
around the test code like:
test_expect_success 'check split_cmdline return' '
...
'
or is using double quotes still Ok?
> 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
> + )
> '
Here single quotes are used for example.
> 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)"
> '
This looks like a test modernization, but maybe it's part of making
the tests more robust. Anyway it might be a good idea to either talk a
bit about that in the commit message or to move it to a preparatory
commit if it's a modernization and other modernizations could be made
in that preparatory commit.
Otherwise this patch looks good to me.
^ permalink raw reply
* Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions
From: Christian Couder @ 2024-01-23 16:15 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <1284b70fab6dd85114cb24fc5c7b6c49e35eb135.1704877233.git.ps@pks.im>
On Wed, Jan 10, 2024 at 10:02 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> In t1302 we exercise logic around "core.repositoryFormatVersion" and
> extensions. These tests are not particularly robust against extensions
> like the newly introduced "refStorage" extension.
Here also it's not very clear what robust means. Some examples of how
things could fail would perhaps help.
> 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.
It would be nice to talk a bit about the failures that each of the
above changes could prevent.
> 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 () {
Before that hunk there is:
test_expect_success 'setup' '
test_oid_cache <<-\EOF &&
version sha1:0
version sha256:1
EOF
and it seems to me that the above test_oid_cache call is not needed
anymore, if `test_oid version` is not used anymore.
Otherwise this looks good to me.
^ permalink raw reply
* Re: [PATCH v2 0/6] t: mark "files"-backend specific tests
From: Christian Couder @ 2024-01-23 16:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1704877233.git.ps@pks.im>
Hi,
On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
> 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
I took a look at these 6 patches and only had comments for patches 1/6
and 3/6 so I replied only to those. The rest looks good to me.
Thanks!
^ permalink raw reply
* Re: [PATCH 7/7] Documentation: add "special refs" to the glossary
From: Phillip Wood @ 2024-01-23 16:27 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <2a0943a78d0db0f489962520536594845970e0b0.1705659748.git.ps@pks.im>
Hi Patrick
On 19/01/2024 10:40, Patrick Steinhardt wrote:
> Add the "special refs" term to our glossary.
Related to this the glossary entry for pseudorefs says
Pseudorefs are a class of files under `$GIT_DIR` which behave
like refs for the purposes of rev-parse, but which are treated
specially by git. Pseudorefs both have names that are all-caps,
and always start with a line consisting of a
<<def_SHA1,SHA-1>> followed by whitespace. So, HEAD is not a
pseudoref, because it is sometimes a symbolic ref. They might
optionally contain some additional data. `MERGE_HEAD` and
`CHERRY_PICK_HEAD` are examples. Unlike
<<def_per_worktree_ref,per-worktree refs>>, these files cannot
be symbolic refs, and never have reflogs. They also cannot be
updated through the normal ref update machinery. Instead,
they are updated by directly writing to the files. However,
they can be read as if they were refs, so `git rev-parse
MERGE_HEAD` will work.
which is very file-centric. We should probably update that as we're
moving away from filesystem access except for special refs.
Best Wishes
Phillip
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> Documentation/glossary-content.txt | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index f7d98c11e3..d71b199955 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -638,6 +638,20 @@ The most notable example is `HEAD`.
> An <<def_object,object>> used to temporarily store the contents of a
> <<def_dirty,dirty>> working directory and the index for future reuse.
>
> +[[def_special_ref]]special ref::
> + A ref that has different semantics than normal refs. These refs can be
> + accessed via normal Git commands but may not behave the same as a
> + normal ref in some cases.
> ++
> +The following special refs are known to Git:
> +
> + - "`FETCH_HEAD`" is written by linkgit:git-fetch[1] or linkgit:git-pull[1]. It
> + may refer to multiple object IDs. Each object ID is annotated with metadata
> + indicating where it was fetched from and its fetch status.
> +
> + - "`MERGE_HEAD`" is written by linkgit:git-merge[1] when resolving merge
> + conflicts. It contains all commit IDs which are being merged.
> +
> [[def_submodule]]submodule::
> A <<def_repository,repository>> that holds the history of a
> separate project inside another repository (the latter of
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Phillip Wood @ 2024-01-23 16:30 UTC (permalink / raw)
To: Patrick Steinhardt, Junio C Hamano; +Cc: Karthik Nayak, git
In-Reply-To: <Za-gF_Hp_lXViGWw@tanuki>
Hi Patrick
On 23/01/2024 11:16, Patrick Steinhardt wrote:
> On Mon, Jan 22, 2024 at 12:22:49PM -0800, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>> The list of hard coded exceptions also looks quite short, I
>>> can see MERGE_AUTOSTASH and BISECT_START are missing and there are
>>> probably others I've not thought of.
>>
>> I agree that it is something we need to fix.
>
> I've taken a deeper look at BISECT_START because I previously missed it
> in my conversion to make special refs become normal pseudo refs. But as
> it turns out, BISECT_START is not a ref at all.
> > Depending on how you execute git-bisect(1), it will either contain the
> object ID of the detached HEAD or the branch you're starting the bisect
> from. This information is used to switch back to that state when you
> abort the bisect. So far this sounds like a proper ref indeed. But in
> case you're starting from a branch it will not be a symref that points
> to this branch, but it will just contain the branch name. This is not a
> valid format that could be read as a loose ref, and thus this file is
> not a proper ref at all (except that sometimes it behaves like one when
> starting from a detached HEAD).
Thank you, I'd missed that
> My first hunch was to convert it so that it indeed always is a proper
> ref. But thinking about it a bit more I'm less convinced that this is
> sensible as it is deeply tied to the behaviour of git-bisect(1) and only
> represents its internal state. I thus came to the conclusion that it is
> more similar to the sequencer state that we have in ".git/rebase-merge"
> and ".git/rebase-apply" than anything else.
>
> So if we wanted to rectify this, I think the most sensible way to
> address this would be to introduce a new ".git/bisect-state" directory
> that contains all of git-bisect(1)'s state:
>
> - BISECT_TERMS -> bisect-state/terms
> - BISECT_LOG -> bisect-state/log
> - BISECT_START -> bisect-state/start
> - BISECT_RUN -> bisect-state/run
> - BISECT_FIRST_PARENT -> bisect-state/first-parent
> - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
>
> I think this would make for a much cleaner solution overall as things
> are neatly contained. Cleaning up after a bisect would thus only require
> a delete of ".git/bisect-state/" and we're done.
>
> Of course, this would be a backwards-incompatible change. We could
> transition to that newer schema by having newer Git versions recognize
> both ways to store the state, but only ever write the new schema. But
> I'm not sure whether it would ultimately be worth it.
I think that is a really good suggestion, it would bring bisect into
line with am, rebase, cherry-pick etc. which keep their state in a
subdirectory rather than cluttering up .git.
Best Wishes
Phillip
> Patrick
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: phillip.wood123 @ 2024-01-23 16:40 UTC (permalink / raw)
To: Karthik Nayak, phillip.wood, Junio C Hamano; +Cc: git
In-Reply-To: <CAOLa=ZQOcqGBJOSehok4BYGUE8RKtnE9eiJYogeT8E6NWZ25xw@mail.gmail.com>
Hi Karthik
On 23/01/2024 12:49, Karthik Nayak wrote:
> Hello Phillip,
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
> [...]
> So there is no way to make `is_pseudoref_syntax()` stricter without
> breaking backward compatibility. While we do want to reach that goal, we
> have to go about in the other way around, that i.e.
> 1. Fix all pseudorefs to have the '_HEAD' suffix.
I'm wary of renaming user facing pseudorefs like AUTO_MERGE. In the
future we should try and avoid adding any without a "_HEAD" suffix
> 2. Move bisect files to '$GIT_DIR/bisect-state' (see [1] for more
> details).
> After this, we can safely make `is_pseudoref_syntax()` stricter.
>
> Given this, I think for the next version, I'll do the following changes:
> 1. keep `is_pseudoref_syntax()` as is.
> 2. introduce `is_pseudoref()` which calls `is_pseudoref_syntax()` and
> also checks the content of the file.
We could perhaps make is_pseudoref() stricter from the start as we're
adding a new function, it could have a list of exceptions to the "a
pseudoref must end with '_HEAD'" rule like this patch. Being strict
about the pseudorefs we list with "git for-each-ref" might encourage
users to update any scripts they have that create "pseudorefs" that do
not match the new rules without us making any changes that actually
break those scripts.
> 3. replace use of `is_pseudoref_syntax()` with `is_pseudoref()` in this
> patch series.
That sounds like a good way forward to me, lets see if Junio agrees.
Best Wishes
Phillip
> [1]: https://public-inbox.org/git/20240119142705.139374-1-karthik.188@gmail.com/T/#m6e3790e30613fd68349708faaf5f4d9c704ba677
^ permalink raw reply
* Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends
From: Justin Tobler @ 2024-01-23 16:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Toon Claes, git, Taylor Blau, Eric Sunshine
In-Reply-To: <Za_ZrtRMEmVHu_o5@tanuki>
On Tue, Jan 23, 2024 at 9:22 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote:
> >
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > [[PGP Signed Part:Undecided]]
> > > [1. text/plain]
> > > 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>
> > > ---
> >
> > > @@ -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)"
> > > '
> >
> > From the commit message it was not clear to me this change was needed.
> > Do you think it's worth it to add something to the commit message
> > explaining you now need to copy the custom.conf into each seperate
> > repository?
>
> Good point in fact. The problem here is that before, CUSTOM_CONFIG_FILE
> was using a relative path that wouldn't be found when cd'ing into the
> respective subrepositories. By using `path-utils real_path` we resolve
> the relative path to the full path, and thus we can find the file
> regardless of our shell's current working directory.
>
> Not sure whether this is worth a reroll, but in case you or others think
> that it is then I'm happy to add this explanation.
I also found it unclear why the CUSTOM_CONFIG_FILE change was needed.
I had assumed it was a refactor to make the tests more robust. It might be
nice to explain it in the commit message. :)
-Justin
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Junio C Hamano @ 2024-01-23 17:32 UTC (permalink / raw)
To: Brian Lyles; +Cc: Kristoffer Haugsbakk, Taylor Blau, Elijah Newren, git
In-Reply-To: <CAHPHrSd8rLj_TDE11dYQW+51--8YC4rumnfT+v2bYr+K7AMQrQ@mail.gmail.com>
Brian Lyles <brianmlyles@gmail.com> writes:
> I do see that there are ~130 files with trailing whitespace in maint
> today, though I suspect that most of those are not intentional.
I got curious and took a look at files that has hits with "lines
that end with SP or HT":
$ git grep -l -e '[ ]$'
There are some that can be cleaned up, but many of them are
intentional.
For example, CODE_OF_CONDUCT.md has these two (shown with $$$)
I think can be removed without breaking markdown:
Community Impact Guidelines were inspired by $$$
[Mozilla's code of conduct enforcement ladder][Mozilla CoC].
For answers to common questions about this code of conduct, see the FAQ at
[https://www.contributor-covenant.org/faq][FAQ]. Translations are available $$$
at [https://www.contributor-covenant.org/translations][translations].
The one in Documentation/user-manual.txt is on borderline. They
appear in a sample output from a command the user is typing (again,
$$$ shows where the SP at the end of line is):
diff --git a/init-db.c b/init-db.c
index 65898fa..b002dc6 100644
--- a/init-db.c
+++ b/init-db.c
@@ -7,7 +7,7 @@
$$$
int main(int argc, char **argv)
...
As its purpose is to show human readers what they see in their
terminal should _look_ like, we _could_ do without the single space
on these otherwise empty lines, which denotes an empty line that
hasn't changed in the diff output. But it would no longer match
byte-for-byte with what we are trying to illustrate.
There are many hits from the above grep in t/t[0-9][0-9][0-9][0-9]/
directories; these are canonical/expected output used in tests and
the spaces at the end of these lines are expected.
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Junio C Hamano @ 2024-01-23 17:38 UTC (permalink / raw)
To: Phillip Wood; +Cc: Karthik Nayak, git
In-Reply-To: <ba721840-7b67-4822-8046-c0da4d3b9bde@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I not sure I quite understand what you mean here. Are you saying that
> scripts should stop using "git update-ref" and "git rev-parse" for
> anything that does not match the new pseudoref syntax?
Yes, I am saying that, and also that we should have been stricter on
what we accept and consider as pseudorefs---not just any file that
sits directly under $GIT_DIR/., but ideally we should have limited
to "^[A-Z]*_HEAD$" or something. The idea I am floating is to see if
such a tightening can be done now without hearing too many screams.
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Junio C Hamano @ 2024-01-23 17:44 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Phillip Wood, Karthik Nayak, git
In-Reply-To: <Za-gF_Hp_lXViGWw@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> My first hunch was to convert it so that it indeed always is a proper
> ref. But thinking about it a bit more I'm less convinced that this is
> sensible as it is deeply tied to the behaviour of git-bisect(1) and only
> represents its internal state. I thus came to the conclusion that it is
> more similar to the sequencer state that we have in ".git/rebase-merge"
> and ".git/rebase-apply" than anything else.
Fair enough.
> So if we wanted to rectify this, I think the most sensible way to
> address this would be to introduce a new ".git/bisect-state" directory
> that contains all of git-bisect(1)'s state:
>
> - BISECT_TERMS -> bisect-state/terms
> - BISECT_LOG -> bisect-state/log
> - BISECT_START -> bisect-state/start
> - BISECT_RUN -> bisect-state/run
> - BISECT_FIRST_PARENT -> bisect-state/first-parent
> - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
>
> I think this would make for a much cleaner solution overall as things
> are neatly contained. Cleaning up after a bisect would thus only require
> a delete of ".git/bisect-state/" and we're done.
And bisect-state/ needs to be marked as per-worktree hierarchy, I suppose.
> Of course, this would be a backwards-incompatible change.
As long as we ignore folks who switches versions of Git in the
middle of their "git bisect" session, we should be OK.
If we really cared the backward compatibility, the new version of
Git that knows and uses this new layout could notice these old-style
filenames and move them over to the new place under new names. From
there, everything should work (including things like "git bisect log").
Thanks.
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Junio C Hamano @ 2024-01-23 17:46 UTC (permalink / raw)
To: Karthik Nayak; +Cc: phillip.wood, git
In-Reply-To: <CAOLa=ZQOcqGBJOSehok4BYGUE8RKtnE9eiJYogeT8E6NWZ25xw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Given this, I think for the next version, I'll do the following changes:
> 1. keep `is_pseudoref_syntax()` as is.
> 2. introduce `is_pseudoref()` which calls `is_pseudoref_syntax()` and
> also checks the content of the file.
> 3. replace use of `is_pseudoref_syntax()` with `is_pseudoref()` in this
> patch series.
The content check in 2. was something that was mentioned in an
earlier discussion, lack of which I completely missed during the
review of this current round. Sounds very good to add that.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Junio C Hamano @ 2024-01-23 18:01 UTC (permalink / raw)
To: Phillip Wood; +Cc: brianmlyles, git, me, newren
In-Reply-To: <b897771e-c60e-4e41-bfae-3bcfdd832be1@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Thanks for the well explained commit message
;-)
>> The `--keep-redundant-commits` option will be documented as a deprecated
>> synonym of `--empty=keep`, and will be supported for backwards
>> compatibility for the time being.
>
> I'm not sure if we need to deprecate it as in "it will be removed in
> the future" or just reduce it prominence in favor of --empty
True, especially since --empty=keep is much less descriptive and the
part after "note that ..." below will take a long time before
sticking in readers' brain.
>> +--empty=(stop|drop|keep)::
>> + How to handle commits being cherry-picked that are redundant with
>> + changes already in the current history.
It might make it easier to understand if we moved the description in
'keep' that says something about --allow-empty here, as it should
apply to other two choices if I understand correctly. Let me try:
This option specifies how a commit that is not originally empty
but becomes a no-op when cherry-picked due to earlier changes
already applied or already in the current history. Regardless
of this this option, `cherry-pick` will fail on a commit that is
empty in the original history---see `--allow-empty` to pass them
intact.
or something. Then the description of `keep` can become just as
short as other two, e.g. a single sentence "The commit that becomes
empty will be kept".
>> ...
>> + The cherry-pick will stop when the empty commit is applied, allowing
>> + you to examine the commit. This is the default behavior.
>> +`drop`;;
>> + The empty commit will be dropped.
>> +`keep`;;
>> + The empty commit will be kept. Note that use of this option only
>> results in an empty commit when the commit was not initially empty,
>> but rather became empty due to a previous commit. Commits that were
>> initially empty will cause the cherry-pick to fail. To force the
>> inclusion of those commits use `--allow-empty`.
>> +--
>> ++
>> +Note that commits which start empty will cause the cherry-pick to fail (unless
>> +`--allow-empty` is specified).
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Junio C Hamano @ 2024-01-23 18:18 UTC (permalink / raw)
To: Phillip Wood; +Cc: brianmlyles, git, me, newren
In-Reply-To: <7bf5036b-9f55-4451-a13c-8a2c815dfbb7@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>> This implication of `--allow-empty` therefore seems incorrect: One
>> should be able to keep a commit that becomes empty without also being
>> forced to pick commits that start as empty.
>
> Do you have a practical example of where you want to keep the commits
> that become empty but not the ones that start empty? I agree there is
> a distinction but I think the common case is that the user wants to
> keep both types of empty commit or none. I'm not against giving the
> user the option to keep one or the other if it is useful but I'm wary
> of changing the default.
This may not a new issue introduced by this series, but one thing I
would be worried about the usability of the keep-redundant is that I
know it takes more than one tries of cherry-picking of the same
series, at least to me, to get a series right. The initial attempt
may make some commit empty and thanks to --keep-redundant they will
be kept, but I'll inevitably find more things I need to tweak and
cherry-pick the resulting series, possibly on a different base. And
to this second round of cherry-pick, these "were not, but now have
become empty" commits appear empty from the start.
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Junio C Hamano @ 2024-01-23 18:34 UTC (permalink / raw)
To: Sergey Organov; +Cc: Elijah Newren, git
In-Reply-To: <87a5ow9jb4.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Then how does one figure what "git clean -f -f" will do without actually
> doing it?
I think whoever came up with the bright idea of forcing twice
somehow does a totally different thing from forcing once should be
shot, twice ;-) It does not mesh well with the idea behind the
clean.requireForce setting to make you explicitly choose either '-f'
or '-n' to express your intent.
I wonder how feasible is it to deprecate that misfeature introduced
with a0f4afbe (clean: require double -f options to nuke nested git
repository and work tree, 2009-06-30) and migrate its users (which
is marked as "This is rarely what the user wants") to a new option,
say, --nested-repo-too so that the "dry-run" version of the
invocations become
git clean -n
git clean -n --nested-repo-too
and you can substitute "-n" with "-f" to actually perform it?
Anybody care to come up with a sensible migration plan?
Thanks.
^ permalink raw reply
* Subject: [PATCH] CoC: whitespace fix
From: Junio C Hamano @ 2024-01-23 18:41 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Taylor Blau, Elijah Newren, Brian Lyles
In-Reply-To: <xmqqa5owot07.fsf@gitster.g>
> For example, CODE_OF_CONDUCT.md has these two (shown with $$$)
> I think can be removed without breaking markdown:
>
> Community Impact Guidelines were inspired by $$$
> [Mozilla's code of conduct enforcement ladder][Mozilla CoC].
>
> For answers to common questions about this code of conduct, see the FAQ at
> [https://www.contributor-covenant.org/faq][FAQ]. Translations are available $$$
> at [https://www.contributor-covenant.org/translations][translations].
Before I forget...
------ >8 ----------- >8 ----------- >8 ----------- >8 ------
Subject: [PATCH] CoC: whitespace fix
Fix two lines with trailing whitespaces.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
CODE_OF_CONDUCT.md | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md
index 0215b1fd4c..e58917c50a 100644
--- a/CODE_OF_CONDUCT.md
+++ b/CODE_OF_CONDUCT.md
@@ -130,11 +130,11 @@ This Code of Conduct is adapted from the [Contributor Covenant][homepage],
version 2.0, available at
[https://www.contributor-covenant.org/version/2/0/code_of_conduct.html][v2.0].
-Community Impact Guidelines were inspired by
+Community Impact Guidelines were inspired by
[Mozilla's code of conduct enforcement ladder][Mozilla CoC].
For answers to common questions about this code of conduct, see the FAQ at
-[https://www.contributor-covenant.org/faq][FAQ]. Translations are available
+[https://www.contributor-covenant.org/faq][FAQ]. Translations are available
at [https://www.contributor-covenant.org/translations][translations].
[homepage]: https://www.contributor-covenant.org
--
2.43.0-386-ge02ecfcc53
^ permalink raw reply related
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Junio C Hamano @ 2024-01-23 18:49 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Taylor Blau, Elijah Newren, Brian Lyles
In-Reply-To: <xmqqa5owot07.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I got curious and took a look at files that has hits with "lines
> that end with SP or HT":
>
> $ git grep -l -e '[ ]$'
Another more interest way to check is to do this:
$ git diff --check $(git hash-object --stdin -t tree </dev/null)
which will not just catch trailing whitespaces but other whitespace
errors. Good thing is that the projects can even customize the rules
used for various paths using the attributes mechanism.
Here is a sample patch based on what the above command line found.
------ >8 ----------- >8 ----------- >8 ----------- >8 ------
Subject: [PATCH] docs: a few whitespace fixes
Some documentation files have 8-space indented lines where other
indented lines in the same list use a single HT to indent. As
Documentation/.gitattributes says *.txt files use the default
whitespace rules, let's fix some of them as a practice.
Note that git-add documentation has other instances of space
indented lines, but they are samples of manually aligned output
of the "git add -i" command and it would be better to keep them
as-is. Which in turn may mean we may want to loosen the whitespace
rules for some parts of the documentation files, but we currently do
not have such a feature. The attribute based whitespace rules apply
to the whole file.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/diff-format.txt | 8 ++++----
Documentation/git-add.txt | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git c/Documentation/diff-format.txt w/Documentation/diff-format.txt
index a3ae8747a2..bcaf9ca608 100644
--- c/Documentation/diff-format.txt
+++ w/Documentation/diff-format.txt
@@ -8,16 +8,16 @@ These commands all compare two sets of things; what is
compared differs:
git-diff-index <tree-ish>::
- compares the <tree-ish> and the files on the filesystem.
+ compares the <tree-ish> and the files on the filesystem.
git-diff-index --cached <tree-ish>::
- compares the <tree-ish> and the index.
+ compares the <tree-ish> and the index.
git-diff-tree [-r] <tree-ish-1> <tree-ish-2> [<pattern>...]::
- compares the trees named by the two arguments.
+ compares the trees named by the two arguments.
git-diff-files [<pattern>...]::
- compares the index and the files on the filesystem.
+ compares the index and the files on the filesystem.
The "git-diff-tree" command begins its output by printing the hash of
what is being compared. After that, all the commands print one output
diff --git c/Documentation/git-add.txt w/Documentation/git-add.txt
index ed44c1cb31..e1a5c27acd 100644
--- c/Documentation/git-add.txt
+++ w/Documentation/git-add.txt
@@ -73,7 +73,7 @@ in linkgit:gitglossary[7].
-v::
--verbose::
- Be verbose.
+ Be verbose.
-f::
--force::
^ permalink raw reply related
* [PATCH] reftable: honor core.fsync
From: John Cai via GitGitGadget @ 2024-01-23 18:51 UTC (permalink / raw)
To: git; +Cc: John Cai, John Cai
From: John Cai <johncai86@gmail.com>
While the reffiles backend honors configured fsync settings, the
reftable backend does not. Address this by fsyncing reftable files using
the write-or-die api's fsync_component() in two places: when we
add additional entries into the table, and when we close the reftable
writer.
This commits adds a flush function pointer as a new member of
reftable_writer because we are not sure that the first argument to the
*write function pointer always contains a file descriptor. In the case of
strbuf_add_void, the first argument is a buffer. This way, we can pass
in a corresponding flush function that knows how to flush depending on
which writer is being used.
This patch does not contain tests as they will need to wait for another
patch to start to exercise the reftable backend. At that point, the
tests will be added to observe that fsyncs are happening when the
reftable is in use.
Signed-off-by: John Cai <johncai86@gmail.com>
---
reftable: honor core.fsync
While the reffiles backend honors configured fsync settings, the
reftable backend does not. Address this by fsyncing reftable files using
the write-or-die api's fsync_component() in two places: when we add
additional entries into the table, and when we close the reftable
writer.
This commits adds a flush function pointer as a new member of
reftable_writer because we are not sure that the first argument to the
*write function pointer always contains a file descriptor. In the case
of strbuf_add_void, the first argument is a buffer. This way, we can
pass in a corresponding flush function that knows how to flush depending
on which writer is being used.
This patch does not contain tests as they will need to wait for another
patch to exercise the reftable backend in test. At that point, the tests
will be added to observe that fsyncs are happening when the reftable is
in use.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1654%2Fjohn-cai%2Fjc%2Ffsync-reftable-write-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1654/john-cai/jc/fsync-reftable-write-v1
Pull-Request: https://github.com/git/git/pull/1654
reftable/merged_test.c | 6 +++---
reftable/readwrite_test.c | 24 ++++++++++++------------
reftable/refname_test.c | 2 +-
reftable/reftable-writer.h | 1 +
reftable/stack.c | 16 +++++++++++++---
reftable/test_framework.c | 5 +++++
reftable/test_framework.h | 2 ++
reftable/writer.c | 8 ++++++++
reftable/writer.h | 1 +
9 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index 46908f738f7..bf090b474ed 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -42,7 +42,7 @@ static void write_test_table(struct strbuf *buf,
}
}
- w = reftable_new_writer(&strbuf_add_void, buf, &opts);
+ w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
reftable_writer_set_limits(w, min, max);
for (i = 0; i < n; i++) {
@@ -70,7 +70,7 @@ static void write_test_log_table(struct strbuf *buf,
.exact_log_message = 1,
};
struct reftable_writer *w = NULL;
- w = reftable_new_writer(&strbuf_add_void, buf, &opts);
+ w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
reftable_writer_set_limits(w, update_index, update_index);
for (i = 0; i < n; i++) {
@@ -412,7 +412,7 @@ static void test_default_write_opts(void)
struct reftable_write_options opts = { 0 };
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_ref_record rec = {
.refname = "master",
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index b8a32240164..6b99daeaf2a 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -51,7 +51,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,
.hash_id = hash_id,
};
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
struct reftable_ref_record ref = { NULL };
int i = 0, n;
struct reftable_log_record log = { NULL };
@@ -130,7 +130,7 @@ static void test_log_buffer_size(void)
.message = "commit: 9\n",
} } };
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
/* This tests buffer extension for log compression. Must use a random
hash, to ensure that the compressed part is larger than the original.
@@ -171,7 +171,7 @@ static void test_log_overflow(void)
.message = msg,
} } };
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
uint8_t hash1[GIT_SHA1_RAWSZ] = {1}, hash2[GIT_SHA1_RAWSZ] = { 2 };
@@ -202,7 +202,7 @@ static void test_log_write_read(void)
struct reftable_block_source source = { NULL };
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
const struct reftable_stats *stats = NULL;
reftable_writer_set_limits(w, 0, N);
for (i = 0; i < N; i++) {
@@ -294,7 +294,7 @@ static void test_log_zlib_corruption(void)
struct reftable_block_source source = { 0 };
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
const struct reftable_stats *stats = NULL;
uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
@@ -535,7 +535,7 @@ static void test_table_refs_for(int indexed)
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_iterator it = { NULL };
int j;
@@ -628,7 +628,7 @@ static void test_write_empty_table(void)
struct reftable_write_options opts = { 0 };
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_block_source source = { NULL };
struct reftable_reader *rd = NULL;
struct reftable_ref_record rec = { NULL };
@@ -666,7 +666,7 @@ static void test_write_object_id_min_length(void)
};
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_ref_record ref = {
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
@@ -701,7 +701,7 @@ static void test_write_object_id_length(void)
};
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_ref_record ref = {
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
@@ -735,7 +735,7 @@ static void test_write_empty_key(void)
struct reftable_write_options opts = { 0 };
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_ref_record ref = {
.refname = "",
.update_index = 1,
@@ -758,7 +758,7 @@ static void test_write_key_order(void)
struct reftable_write_options opts = { 0 };
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_ref_record refs[2] = {
{
.refname = "b",
@@ -801,7 +801,7 @@ static void test_write_multiple_indices(void)
struct reftable_reader *reader;
int err, i;
- writer = reftable_new_writer(&strbuf_add_void, &writer_buf, &opts);
+ writer = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);
reftable_writer_set_limits(writer, 1, 1);
for (i = 0; i < 100; i++) {
struct reftable_ref_record ref = {
diff --git a/reftable/refname_test.c b/reftable/refname_test.c
index 699e1aea412..b9cc62554ea 100644
--- a/reftable/refname_test.c
+++ b/reftable/refname_test.c
@@ -30,7 +30,7 @@ static void test_conflict(void)
struct reftable_write_options opts = { 0 };
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
- reftable_new_writer(&strbuf_add_void, &buf, &opts);
+ reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
struct reftable_ref_record rec = {
.refname = "a/b",
.value_type = REFTABLE_REF_SYMREF,
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index db8de197f6c..7c7cae5f99b 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -88,6 +88,7 @@ struct reftable_stats {
/* reftable_new_writer creates a new writer */
struct reftable_writer *
reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
+ int (*flush_func)(void *),
void *writer_arg, struct reftable_write_options *opts);
/* Set the range of update indices for the records we will add. When writing a
diff --git a/reftable/stack.c b/reftable/stack.c
index 7ffeb3ee107..ab295341cc4 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -8,6 +8,7 @@ license that can be found in the LICENSE file or at
#include "stack.h"
+#include "../write-or-die.h"
#include "system.h"
#include "merged.h"
#include "reader.h"
@@ -16,7 +17,6 @@ license that can be found in the LICENSE file or at
#include "reftable-record.h"
#include "reftable-merged.h"
#include "writer.h"
-
#include "tempfile.h"
static int stack_try_add(struct reftable_stack *st,
@@ -47,6 +47,13 @@ static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
return write_in_full(*fdp, data, sz);
}
+static int reftable_fd_flush(void *arg)
+{
+ int *fdp = (int *)arg;
+
+ return fsync_component(FSYNC_COMPONENT_REFERENCE, *fdp);
+}
+
int reftable_new_stack(struct reftable_stack **dest, const char *dir,
struct reftable_write_options config)
{
@@ -545,6 +552,9 @@ int reftable_addition_commit(struct reftable_addition *add)
goto done;
}
+ fsync_component_or_die(FSYNC_COMPONENT_REFERENCE, lock_file_fd,
+ get_tempfile_path(add->lock_file));
+
err = rename_tempfile(&add->lock_file, add->stack->list_file);
if (err < 0) {
err = REFTABLE_IO_ERROR;
@@ -639,7 +649,7 @@ int reftable_addition_add(struct reftable_addition *add,
goto done;
}
}
- wr = reftable_new_writer(reftable_fd_write, &tab_fd,
+ wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,
&add->stack->config);
err = write_table(wr, arg);
if (err < 0)
@@ -731,7 +741,7 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,
strbuf_addstr(temp_tab, ".temp.XXXXXX");
tab_fd = mkstemp(temp_tab->buf);
- wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
+ wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);
err = stack_write_compact(st, wr, first, last, config);
if (err < 0)
diff --git a/reftable/test_framework.c b/reftable/test_framework.c
index 04044fc1a0f..4066924eee4 100644
--- a/reftable/test_framework.c
+++ b/reftable/test_framework.c
@@ -20,3 +20,8 @@ ssize_t strbuf_add_void(void *b, const void *data, size_t sz)
strbuf_add(b, data, sz);
return sz;
}
+
+int noop_flush(void *arg)
+{
+ return 0;
+}
diff --git a/reftable/test_framework.h b/reftable/test_framework.h
index ee44f735aea..687390f9c23 100644
--- a/reftable/test_framework.h
+++ b/reftable/test_framework.h
@@ -56,4 +56,6 @@ void set_test_hash(uint8_t *p, int i);
*/
ssize_t strbuf_add_void(void *b, const void *data, size_t sz);
+int noop_flush(void *);
+
#endif
diff --git a/reftable/writer.c b/reftable/writer.c
index ee4590e20f8..92935baa703 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -121,6 +121,7 @@ static struct strbuf reftable_empty_strbuf = STRBUF_INIT;
struct reftable_writer *
reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
+ int (*flush_func)(void *),
void *writer_arg, struct reftable_write_options *opts)
{
struct reftable_writer *wp =
@@ -136,6 +137,7 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
wp->write = writer_func;
wp->write_arg = writer_arg;
wp->opts = *opts;
+ wp->flush = flush_func;
writer_reinit_block_writer(wp, BLOCK_TYPE_REF);
return wp;
@@ -603,6 +605,12 @@ int reftable_writer_close(struct reftable_writer *w)
put_be32(p, crc32(0, footer, p - footer));
p += 4;
+ err = w->flush(w->write_arg);
+ if (err < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
+ }
+
err = padded_write(w, footer, footer_size(writer_version(w)), 0);
if (err < 0)
goto done;
diff --git a/reftable/writer.h b/reftable/writer.h
index 09b88673d97..8d0df9cc528 100644
--- a/reftable/writer.h
+++ b/reftable/writer.h
@@ -16,6 +16,7 @@ license that can be found in the LICENSE file or at
struct reftable_writer {
ssize_t (*write)(void *, const void *, size_t);
+ int (*flush)(void *);
void *write_arg;
int pending_padding;
struct strbuf last_key;
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] reftable: honor core.fsync
From: Junio C Hamano @ 2024-01-23 19:31 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <pull.1654.git.git.1706035870956.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This commits adds a flush function pointer as a new member of
> reftable_writer because we are not sure that the first argument to the
> *write function pointer always contains a file descriptor. In the case of
> strbuf_add_void, the first argument is a buffer. This way, we can pass
> in a corresponding flush function that knows how to flush depending on
> which writer is being used.
A comment and a half.
* Can't the new "how to flush" go to the write-option structure?
If you represent "no flush" as a NULL pointer in the flush member,
most of the changes to the _test files can go, no?
* For a function
int func(int ac, char **av);
a literal pointer to it can legally be written as either
int (*funcp)(int, char **) = &func;
int (*funcp)(int, char **) = func;
but it is my understanding that this codebase prefers the latter,
a tradition which goes back to 2005 when Linus was still writing
a lot of code, i.e. the identifier that is the name of the
function, without & in front.
^ permalink raw reply
* Re: [PATCH] reftable: honor core.fsync
From: Kristoffer Haugsbakk @ 2024-01-23 21:06 UTC (permalink / raw)
To: Josh Soref; +Cc: John Cai, git
In-Reply-To: <pull.1654.git.git.1706035870956.gitgitgadget@gmail.com>
On Tue, Jan 23, 2024, at 19:51, John Cai via GitGitGadget wrote:
> This commits adds a flush function pointer as a new member of
I guess you meant singular “This commit”?
> This commits adds a flush function pointer as a new member of
> […]
> This patch does not contain tests as they will need to wait for another
Out of these two “This commit” is more true for the future `git log`.
--
Kristoffer Haugsbakk
^ permalink raw reply
* `git fetch` with protocol.version=1 misses tags that point to the fetched history
From: Josh Steadmon @ 2024-01-23 21:08 UTC (permalink / raw)
To: git; +Cc: bcmills, peff
At $DAYJOB, we got a bug report that Git 2.21.0 breaks Go's CI due to
not fetching all tags in the history. The bug reporter (Bryan, CCed) was
kind enough to bisect this failure down to 61c7711cfe (sha1-file: use
loose object cache for quick existence check, 2018-11-12). This was only
recently discovered because Go's CI was using Git v2.17.6.
More details can be found in the original bug report [1] and Go's issue
for the CI breakage [2].
[1] https://git.g-issues.gerritcodereview.com/issues/320678525
[2] https://github.com/golang/go/issues/56881
A log of the failing Go test follows:
vcs-test.golang.org rerouted to http://127.0.0.1:36865
https://vcs-test.golang.org rerouted to https://127.0.0.1:43597
=== RUN TestStat
=== PAUSE TestStat
=== CONT TestStat
=== RUN TestStat/gitrepo1/v1.2.4-annotated
=== PAUSE TestStat/gitrepo1/v1.2.4-annotated
=== CONT TestStat/gitrepo1/v1.2.4-annotated
git_test.go:166: mkdir -p /tmp/gitrepo-test-767558581/modcache/cache/vcs # git3 http://127.0.0.1:36865/git/gitrepo1
git_test.go:166: # lock /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0.lock
git_test.go:166: mkdir -p /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0 # git3 http://127.0.0.1:36865/git/gitrepo1
git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git init --bare
git_test.go:166: 0.011s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git init --bare
git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git remote add origin -- http://127.0.0.1:36865/git/gitrepo1
git_test.go:166: 0.008s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git remote add origin -- http://127.0.0.1:36865/git/gitrepo1
git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git tag -l
git_test.go:166: 0.008s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git tag -l
git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git ls-remote -q origin
2024/01/17 14:11:49 serving /git/gitrepo1/info/refs?service=git-upload-pack
2024/01/17 14:11:49 gitrepo1.txt:
> handle git
> env GIT_AUTHOR_NAME='Russ Cox'
> env GIT_AUTHOR_EMAIL='rsc@golang.org'
> env GIT_COMMITTER_NAME=$GIT_AUTHOR_NAME
> env GIT_COMMITTER_EMAIL=$GIT_AUTHOR_EMAIL
> git init
[stdout]
Initialized empty Git repository in /tmp/vcstest1128851619/git/gitrepo1/.git/
> at 2018-04-17T15:43:22-04:00
> unquote ''
> cp stdout README
> git add README
> git commit -a -m 'empty README'
[stdout]
[main (root-commit) ede458d] empty README
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 README
> git branch -m master
> git tag v1.2.3
> at 2018-04-17T15:45:48-04:00
> git branch v2
> git checkout v2
[stderr]
Switched to branch 'v2'
> echo 'v2'
[stdout]
v2
> cp stdout v2
> git add v2
> git commit -a -m 'v2'
[stdout]
[v2 76a00fb] v2
1 file changed, 1 insertion(+)
create mode 100644 v2
> git tag v2.3
> git tag v2.0.1
> git branch v2.3.4
> at 2018-04-17T16:00:19-04:00
> echo 'intermediate'
[stdout]
intermediate
> cp stdout foo.txt
> git add foo.txt
> git commit -a -m 'intermediate'
[stdout]
[v2 97f6aa5] intermediate
1 file changed, 1 insertion(+)
create mode 100644 foo.txt
> at 2018-04-17T16:00:32-04:00
> echo 'another'
[stdout]
another
> cp stdout another.txt
> git add another.txt
> git commit -a -m 'another'
[stdout]
[v2 9d02800] another
1 file changed, 1 insertion(+)
create mode 100644 another.txt
> git tag v2.0.2
> at 2018-04-17T16:16:52-04:00
> git checkout master
[stderr]
Switched to branch 'master'
> git branch v3
> git checkout v3
[stderr]
Switched to branch 'v3'
> mkdir v3/sub/dir
> echo 'v3/sub/dir/file'
[stdout]
v3/sub/dir/file
> cp stdout v3/sub/dir/file.txt
> git add v3
> git commit -a -m 'add v3/sub/dir/file.txt'
[stdout]
[v3 a8205f8] add v3/sub/dir/file.txt
1 file changed, 1 insertion(+)
create mode 100644 v3/sub/dir/file.txt
> at 2018-04-17T22:23:00-04:00
> git checkout master
[stderr]
Switched to branch 'master'
> git tag -a v1.2.4-annotated -m 'v1.2.4-annotated'
> git show-ref --tags --heads
[stdout]
ede458df7cd0fdca520df19a33158086a8a68e81 refs/heads/master
9d02800338b8a55be062c838d1f02e0c5780b9eb refs/heads/v2
76a00fb249b7f93091bc2c89a789dab1fc1bc26f refs/heads/v2.3.4
a8205f853c297ad2c3c502ba9a355b35b7dd3ca5 refs/heads/v3
ede458df7cd0fdca520df19a33158086a8a68e81 refs/tags/v1.2.3
b004e48a345a86ed7a2fb7debfa7e0b2f9b0dd91 refs/tags/v1.2.4-annotated
76a00fb249b7f93091bc2c89a789dab1fc1bc26f refs/tags/v2.0.1
9d02800338b8a55be062c838d1f02e0c5780b9eb refs/tags/v2.0.2
76a00fb249b7f93091bc2c89a789dab1fc1bc26f refs/tags/v2.3
> cmp stdout .git-refs
git_test.go:166: 0.194s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git ls-remote -q origin
git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' ede458df7cd0fdca520df19a33158086a8a68e81 --
git_test.go:166: 0.006s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' ede458df7cd0fdca520df19a33158086a8a68e81 --
git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git fetch -f --depth=1 origin refs/tags/v1.2.4-annotated:refs/tags/v1.2.4-annotated
2024/01/17 14:11:49 serving /git/gitrepo1/info/refs?service=git-upload-pack
2024/01/17 14:11:49 serving /git/gitrepo1/git-upload-pack
2024/01/17 14:11:49 serving /git/gitrepo1/git-upload-pack
git_test.go:166: 0.113s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git fetch -f --depth=1 origin refs/tags/v1.2.4-annotated:refs/tags/v1.2.4-annotated
git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' refs/tags/v1.2.4-annotated --
git_test.go:166: 0.007s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' refs/tags/v1.2.4-annotated --
git_test.go:661: Stat: incorrect info
have {Origin:<nil> Name:ede458df7cd0fdca520df19a33158086a8a68e81 Short:ede458df7cd0 Version:v1.2.4-annotated Time:2018-04-17 19:43:22 +0000 UTC Tags:[v1.2.4-annotated]}
want {Origin:<nil> Name:ede458df7cd0fdca520df19a33158086a8a68e81 Short:ede458df7cd0 Version:v1.2.4-annotated Time:2018-04-17 19:43:22 +0000 UTC Tags:[v1.2.3 v1.2.4-annotated]}
--- FAIL: TestStat (0.00s)
--- FAIL: TestStat/gitrepo1/v1.2.4-annotated (0.35s)
FAIL
FAIL cmd/go/internal/modfetch/codehost 0.398s
FAIL
^ permalink raw reply
* Re: sd/negotiate-trace-fix (was: What's cooking in git.git (Jan 2024, #06; Fri, 19))
From: Josh Steadmon @ 2024-01-23 21:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, delmerico
In-Reply-To: <xmqqbk9ghio5.fsf@gitster.g>
On 2024.01.19 17:56, Junio C Hamano wrote:
[snip]
> * sd/negotiate-trace-fix (2024-01-03) 1 commit
> - push: region_leave trace for negotiate_using_fetch
>
> Tracing fix.
>
> Waiting for a review response.
> cf. <xmqqbka27zu9.fsf@gitster.g>
> source: <20240103224054.1940209-1-delmerico@google.com>
Hi Junio,
Were there other open questions you had on this series? It looks like
Sam answered all the questions in your xmqqbka27zu9.fsf@gitster.g reply.
In your other followup you mentioned running CI with tracing enabled for
all tests. This is something that we suggested before [1] but it was
pretty thoroughly rejected for runtime reasons. Perhaps a more efficient
check for region_enter/leave pairs could be an option, though.
[1] https://lore.kernel.org/git/cover.1564009259.git.steadmon@google.com/
^ permalink raw reply
* Re: sd/negotiate-trace-fix
From: Junio C Hamano @ 2024-01-23 21:34 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, delmerico
In-Reply-To: <ZbAu49xkxEzJ3ZkX@google.com>
Josh Steadmon <steadmon@google.com> writes:
> On 2024.01.19 17:56, Junio C Hamano wrote:
> [snip]
>> * sd/negotiate-trace-fix (2024-01-03) 1 commit
>> - push: region_leave trace for negotiate_using_fetch
>>
>> Tracing fix.
>>
>> Waiting for a review response.
>> cf. <xmqqbka27zu9.fsf@gitster.g>
>> source: <20240103224054.1940209-1-delmerico@google.com>
>
> Hi Junio,
>
> Were there other open questions you had on this series? It looks like
> Sam answered all the questions in your xmqqbka27zu9.fsf@gitster.g reply.
Thanks for a reminder.
> In your other followup you mentioned running CI with tracing enabled for
> all tests. This is something that we suggested before [1] but it was
> pretty thoroughly rejected for runtime reasons. Perhaps a more efficient
> check for region_enter/leave pairs could be an option, though.
Yeah, perhaps. It does not sound like an issue that must block this
patch in either case.
Thanks.
^ 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