[pull] master from git:master#98
Merged
pull[bot] merged 27 commits intoturkdevops:masterfrom Aug 29, 2025
Merged
Conversation
Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()` accept an array of records that should be added to the new table. Callers of this function are expected to also pass the number of such records to the function to tell it how many such records it is supposed to write. But while all callers pass in a `size_t`, which is a sensible choice, the function in fact accepts an `int` as argument, which is less so. Fix this. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The reftable writer accidentally uses the Git-specific `QSORT()` macro. This macro removes the need for the caller to provide the element size, but other than that it's mostly equivalent to `qsort()`. Replace the macro accordingly to make the library usable outside of Git. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a couple of forward declarations in the stack-related code of the reftable library. These declarations aren't really required, but are simply caused by unfortunate ordering. Reorder the code and remove the forward declarations. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
While perfectly legal, older compiler toolchains complain when
zero-initializing structs that contain nested structs with `{0}`:
/home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
struct reftable_addition empty = REFTABLE_ADDITION_INIT;
^~~~~~~~~~~~~~~~~~~~~~
/home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'
#define REFTABLE_ADDITION_INIT {0}
^
We had the discussion around whether or not we want to handle such bogus
compiler errors in the past already [1]. Back then we basically decided
that we do not care about such old-and-buggy compilers, so while we
could fix the issue by using `{{0}}` instead this is not the preferred
way to handle this in the Git codebase.
We have an easier fix though: we can just drop the macro altogether and
handle initialization of the struct in `reftable_stack_addition_init()`.
Callers are expected to call this function already, so this change even
simplifies the calling convention.
[1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/
Suggested-by: Carlo Arenas <carenas@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `reftable_stack_add()` function is a simple wrapper to lock the stack, add records to it via a callback and then commit the result. One problem with it though is that it doesn't accept any flags for creating the addition. This makes it impossible to automatically reload the stack in case it was modified before we managed to lock the stack. Add a `flags` field to plug this gap and pass it through accordingly. For now this new flag won't be used by us, but it will be used by libgit2. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we compact the reftable stack we first acquire the lock for the "tables.list" file and then reload the stack to check that it is still up-to-date. This is done by calling `stack_uptodate()`, which knows to return zero in case the stack is up-to-date, a positive value if it is not and a negative error code on unexpected conditions. We don't do proper error checking though, but instead we only check whether the returned error code is non-zero. If so, we simply bubble it up the calling stack, which means that callers may see an unexpected positive value. Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead. Handle this situation in `reftable_addition_commit()`, where we perform a best-effort auto-compaction. All other callsites of `stack_uptodate()` know to handle a positive return value and thus don't need to be fixed. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `flock` interface is implemented as part of "reftable/system.c" and thus needs to be implemented by the integrator between the reftable library and its parent code base. As such, we cannot rely on any specific implementation thereof. Regardless of that, users of the `flock` subsystem rely on `errno` being set to specific values. This is fragile and not documented anywhere and doesn't really make for a good interface. Refactor the code so that the implementations themselves are expected to return reftable-specific error codes. Our implementation of the `flock` subsystem already knows to do this for all error paths except one. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When creating a new addition via either `reftable_stack_new_addition()`
or its convenince wrapper `reftable_stack_add()` we:
1. Create the "tables.list.lock" file.
2. Verify that the current version of the "tables.list" file is
up-to-date.
3. Write the new table records if so.
By default, the second step would cause us to bail out if we see that
there has been a concurrent write to the stack that made our in-memory
copy of the stack out-of-date. This is a safety mechanism to not write
records to the stack based on outdated information.
The downside though is that concurrent writes may now cause us to bail
out, which is not a good user experience. In addition, this isn't even
necessary for us, as Git knows to perform all checks for the old state
of references under the lock. (Well, in all except one case: when we
expire the reflog we first create the log iterator before we create the
lock, but this ordering is fixed as part of this commit.)
Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`
flag. The effect of this flag is that we reload the stack after having
acquired the lock in case the stack is out-of-date. This plugs the race
with concurrent writers, but we continue performing the verifications of
the expected old state to catch actual conflicts in the references we
are about to write.
Adapt the remaining callsites that don't yet pass this flag to do so.
While at it, drop a needless manual reload.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We pass a "struct object_id" to describe_blob() by value. This isn't wrong, as an oid is composed only of copy-able values. But it's unusual; typically we pass structs by const pointer, including object_ids. Let's do so. It similarly makes sense for us to hold that pointer in the callback data (rather than yet another copy of the oid). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
If describe_blob() does not find the blob in question, it returns an empty strbuf, and we print an empty line. This differs from describe_commit(), which always either returns an answer or calls die() itself. As the blob function was bolted onto the command afterwards, I think its behavior is not intentional, and it is just a bug that it does not report an error. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When describing a blob, we search for it by traversing from HEAD. We do this by feeding the name HEAD to setup_revisions(). But if we are on an unborn branch, this will fail with a confusing message: $ git describe $blob fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' It is OK for this to be an error (we cannot find $blob in an empty traversal, so we'd eventually complain about that). But the error message could be more helpful. Let's resolve HEAD ourselves and pass the resolved object id to setup_revisions(). If resolving fails, then we can print a more useful message. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The changes added by 39fc408 (t/t1517: automate `git subcmd -h` tests outside a repository, 2025-08-08) to automatically loop over all "main" Git commands will, when run against an installed build using GIT_TEST_INSTALLED rather than the build in the build directory, include some extra git-gui commands that are installed by `make install`, or credential helpers that might be installed manually from the contrib directories. These fail the test, so record them as such. Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Several tests set a config variable in a sub-repo we chdir into via a subshell, like this: ( cd "$D" && cd two && git config foo.bar baz ) But they also clean up the variable with a when_finished directive outside of the subshell, like this: test_when_finished "git config unset foo.bar" At first glance, this shouldn't work! The cleanup clause cannot be run from the subshell (since environment changes there are lost by the time the test snippet finishes). But since the cleanup command runs outside the subshell, our working directory will not have been switched into "two". But it does work. Why? The answer is that an earlier test does a "cd two" that moves the whole test's working directory out of $TRASH_DIRECTORY and into "two". So the subshell is a bit of a red herring; we are already in the right directory! That's why we need the "cd $D" at the top of the shell, to put us back to a known spot. Let's make this cleanup code more explicitly specify where we expect the config command to run. That makes the script more robust against running a subset of the tests, and ultimately will make it easier to refactor the script to avoid these top-level chdirs. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Several tests in t5510 do a bare "cd subrepo", not in a subshell. This changes the working directory for subsequent tests. As a result, almost every test has to start with "cd $D" to go back to the top-level. Our usual style is to do per-test environment changes like this in a subshell, so that tests can assume they are starting at the top-level $TRASH_DIRECTORY. Let's switch to that style, which lets us drop all of that extra path-handling. Most cases can switch to using a subshell, but in a few spots we can simplify by doing "git init foo && git -C foo ...". We do have to make sure that we weren't intentionally touching the environment in any code which was moved into a subshell (e.g., with a test_when_finished), but that isn't the case for any of these tests. All of the references to the $D variable can go away, replaced generally with $PWD or $TRASH_DIRECTORY (if we use it inside a chdir'd subshell). Note in one test, "fetch --prune prints the remotes url", we make sure to use $(pwd) to get the Windows-style path on that platform (for the other tests, the exact form doesn't matter). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
These tests set config within a sub-repo using (cd two && git config), and then a separate test_when_finished outside the subshell to clean it up. We can't use test_config to do this, because the cleanup command it registers inside the subshell would be lost. Nor can we do it before entering the subshell, because the config has to be set after some other commands are run. Let's switch these tests to use "git -C" for each command instead of a subshell. That lets us use test_config (with -C also) at the appropriate part of the test. And we no longer need the manual cleanup command. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When given an expected "before" state, the ref-writing code will avoid overwriting any ref that does not match that expected state. We use the null oid as a sentinel value for "nothing should exist", and likewise that is the sentinel value we get when trying to read a ref that does not exist. But there's one corner case where this is ambiguous: dangling symrefs. Trying to read them will yield the null oid, but there is potentially something of value there: the dangling symref itself. For a normal recursive write, this is OK. Imagine we have a symref "FOO_HEAD" that points to a ref "refs/heads/bar" that does not exist, and we try to write to it with a create operation like: oid=$(git rev-parse HEAD) ;# or whatever git symbolic-ref FOO_HEAD refs/heads/bar echo "create FOO_HEAD $oid" | git update-ref --stdin The attempt to resolve FOO_HEAD will actually resolve "bar", yielding the null oid. That matches our expectation, and the write proceeds. This is correct, because we are not writing FOO_HEAD at all, but writing its destination "bar", which in fact does not exist. But what if the operation asked not to dereference symrefs? Like this: echo "create FOO_HEAD $oid" | git update-ref --no-deref --stdin Resolving FOO_HEAD would still result in a null oid, and the write will proceed. But it will overwrite FOO_HEAD itself, removing the fact that it ever pointed to "bar". This case is a little esoteric; we are clobbering a symref with a no-deref write of a regular ref value. But the same problem occurs when writing symrefs. For example: echo "symref-create FOO_HEAD refs/heads/other" | git update-ref --no-deref --stdin The "create" operation asked us to create FOO_HEAD only if it did not exist. But we silently overwrite the existing value. You can trigger this without using update-ref via the fetch followRemoteHEAD code. In "create" mode, it should not overwrite an existing value. But if you manually create a symref pointing to a value that does not yet exist (either via symbolic-ref or with "remote add -m"), create mode will happily overwrite it. Instead, we should detect this case and refuse to write. The correct specification to overwrite FOO_HEAD in this case is to provide an expected target ref value, like: echo "symref-update FOO_HEAD refs/heads/other ref refs/heads/bar" | git update-ref --no-deref --stdin Note that the non-symref "update" directive does not allow you to do this (you can only specify an oid). This is a weakness in the update-ref interface, and you'd have to overwrite unconditionally, like: echo "update FOO_HEAD $oid" | git update-ref --no-deref --stdin Likewise other symref operations like symref-delete do not accept the "ref" keyword. You should be able to do: echo "symref-delete FOO_HEAD ref refs/heads/bar" but cannot (and can only delete unconditionally). This patch doesn't address those gaps. We may want to do so in a future patch for completeness, but it's not clear if anybody actually wants to perform those operations. The symref update case (specifically, via followRemoteHEAD) is what I ran into in the wild. The code for the fix is relatively straight-forward given the discussion above. But note that we have to implement it independently for the files and reftable backends. The "old oid" checks happen as part of the locking process, which is implemented separately for each system. We may want to factor this out somehow, but it's beyond the scope of this patch. (Another curiosity is that the messages in the reftable code are marked for translation, but the ones in the files backend are not. I followed local convention in each case, but we may want to harmonize this at some point). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Gitk is now maintained by Johannes Sixt and the repository can be cloned from a new URL. b593581 (Update the official repo of gitk, 2024-12-24) could have updated this instance in the manual, too, but the opportunity was missed. Update it now. Do give credit to Paul Mackerras as the inventor of the program. Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When describing a blob, we traverse from HEAD, remembering each commit we saw, and then checking each blob to report the containing commit. But if we haven't seen any commits at all, we'll segfault (we store the "current" commit as an oid initialized to the null oid, causing lookup_commit_reference() to return NULL). This shouldn't be able to happen normally. We always start our traversal at HEAD, which must be a commit (a property which is enforced by the refs code). But you can trigger the segfault like this: blob=$(echo foo | git hash-object -w --stdin) echo $blob >.git/HEAD git describe $blob We can instead catch this case and return an empty result, which hits the usual "we didn't find $blob while traversing HEAD" error. This is a minor lie in that we did "find" the blob. And this even hints at a bigger problem in this code: what if the traversal pointed to the blob as _not_ part of a commit at all, but we had previously filled in the recorded "current commit"? One could imagine this happening due to a tag pointing directly to the blob in question. But that can't happen, because we only traverse from HEAD, never from any other refs. And the intent of the blob-describing code is to find blobs within commits. So I think this matches the original intent as closely as we can (and again, this segfault cannot be triggered without corrupting your repository!). The test here does not use the formula above, which works only for the files backend (and not reftables). Instead we use another loophole to create the bogus state using only Git commands. See the comment in the test for details. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
There's a call in describe_commit() to lookup_commit_reference(), but we don't check the return value. If it returns NULL, we'll segfault as we immediately dereference the result. In practice this can never happen, since all callers pass an oid which came from a "struct commit" already. So we can make this more obvious by just taking that commit struct in the first place. Reported-by: Cheng <prophecheng@stu.pku.edu.cn> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Discord is a great way of receiving help for members of the community that are not on the mailing list or not familiar with Libera. Adding it to the official documentation will aid discoverability of it. The link is the same as the one at https://git-scm.com/community. Signed-off-by: Daniele Sassoli <danielesassoli@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Test fix. * ad/t1517-short-help-tests-fix: t/t1517: mark tests that fail with GIT_TEST_INSTALLED
Code clean-ups. * ps/reftable-libgit2-cleanup: refs/reftable: always reload stacks when creating lock reftable: don't second-guess errors from flock interface reftable/stack: handle outdated stacks when compacting reftable/stack: allow passing flags to `reftable_stack_add()` reftable/stack: fix compiler warning due to missing braces reftable/stack: reorder code to avoid forward declarations reftable/writer: drop Git-specific `QSORT()` macro reftable/writer: fix type used for number of records
Discord has been added to the first contribution documentation as another way to ask for help. * ds/doc-community-discord: doc: add discord to ways of getting help
"git fetch" can clobber a symref that is dangling when the remote-tracking HEAD is set to auto update, which has been corrected. * jk/no-clobber-dangling-symref-with-fetch: refs: do not clobber dangling symrefs t5510: prefer "git -C" to subshell for followRemoteHEAD tests t5510: stop changing top-level working directory t5510: make confusing config cleanup more explicit
"git describe <blob>" misbehaves and/or crashes in some corner cases, which has been taught to exit with failure gracefully. * jk/describe-blob: describe: pass commit to describe_commit() describe: handle blob traversal with no commits describe: catch unborn branch in describe_blob() describe: error if blob not found describe: pass oid struct by const pointer
Manual page for "gitk" is updated with the current maintainer's name. * js/doc-gitk-history: doc/gitk: update reference to the external project
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.3)
Can you help keep this open source service alive? 💖 Please sponsor : )