"git verify-pack" and "git index-pack" started dying outside a
repository, which has been corrected.
* ps/index-pack-outside-repo-fix:
builtin/index-pack: fix segfaults when running outside of a repo
The code forgot to discard unnecessary in-core commit buffer data
for commits that "git log --skip=<number>" traversed but omitted
from the output, which has been corrected.
* jk/free-commit-buffer-of-skipped-commits:
revision: free commit buffers for skipped commits
With the repository variable available in the builtin function as an
argument, pass this down into helper functions instead of using the
global the_repository.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For builtins that do not operate on a repository, remove
the #define USE_THE_REPOSITORY.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of including USE_THE_REPOSITORY_VARIABLE by default on every
builtin, remove it from builtin.h and add it to all the builtins that
include builtin.h (by definition, that means all builtins/*.c).
Also, remove the include statement for repository.h since it gets
brought in through builtin.h.
The next step will be to migrate each builtin
from having to use the_repository.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In order to reduce the usage of the global the_repository, add a
parameter to builtin functions that will get passed a repository
variable.
This commit uses UNUSED on most of the builtin functions, as subsequent
commits will modify the actual builtins to pass the repository parameter
down.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The last test in t5512 we recently added seems to be flaky.
Running
$ make && cd t && sh ./t5512-ls-remote.sh --stress
shows that "git ls-remote foo::bar" exited with status 141, which
means we got a SIGPIPE. This test piece was introduced by 9e89dcb6
(builtin/ls-remote: fall back to SHA1 outside of a repo, 2024-08-02)
and is pretty much independent from all other tests in the script
(it can even run standalone with everything before it removed).
The transport-helper.c:get_helper() function tries to write to the
helper. As we can see the helper script is very short and can exit
even before it reads anything, when get_helper() tries to give the
first command, "capabilities", the helper may already be gone.
A trivial fix, presented here, is to make sure that the helper reads
the first command it is given, as what it writes later is a response
to that command.
I however would wonder if the interactions with the helper initiated
by get_helper() should be done on a non-blocking I/O (we do check
the return value from our write(2) system calls, do we?).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we open a repository with the "Directory" option, we use "rev-parse
--git-dir" to get the path relative to that directory, and then use
Cwd::abs_path() to make it absolute (since our process working directory
may not be the same).
These days we can just ask for "--absolute-git-dir" instead, which saves
us a little code. That option was added in Git v2.13.0 via a2f5a87626
(rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think
we make any promises about running mismatched versions of git and
Git.pm, but even if somebody tries it, that's sufficiently old that it
should be OK.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When opening a bare repository like:
Git->repository(Directory => '/path/to/bare.git');
we will incorrectly point the repository object at the _current_
directory, not the one specified by the option.
The bug was introduced by 20da61f25f (Git.pm: trust rev-parse to find
bare repositories, 2022-10-22). Before then, we'd ask "rev-parse
--git-dir" if it was a Git repo, and if it returned anything, we'd
correctly convert that result to an absolute path using File::Spec and
Cwd::abs_path(). If it didn't, we'd guess it might be a bare repository
and find it ourselves, which was wrong (rev-parse should find even a
bare repo, and our search circumvented some of its rules).
That commit dropped most of the custom bare-repo search code in favor of
using "rev-parse --is-bare-repository" and trusting the "--git-dir" it
returned. But it mistakenly left some of the bare-repo code path in
place, which was now broken. That code calls Cwd::abs_path($dir); prior
to 20da61f25f $dir contained the "Directory" option the user passed in.
But afterwards, it contains the output of "rev-parse --git-dir". And
since our tentative rev-parse command is invoked after changing
directory, it will always be the relative path "."! So we'll end up with
the absolute path of the process's current directory, not the Directory
option the caller asked for.
So the non-bare case is correct, but the bare one is broken. Our tests
only check the non-bare one, so we didn't notice. We can fix this by
running the same absolute-path fixup code for both sides.
Helped-by: Rodrigo <rodrigolive@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the preceding commits we had to convert the linux32 job to be based
on Ubuntu 20.04 instead of Ubuntu 16.04 due to a limitation in GitHub
Workflows. This was the only job left that still tested against this old
but supported Ubuntu version, and we have no other jobs that test with a
comparatively old Linux distribution.
Add a new job to GitLab CI that tests with Ubuntu 16.04 to cover the
resulting test gap. GitLab doesn't modify Docker images in the same way
GitHub does and thus doesn't fall prey to the same issue. There are two
compatibility issues uncovered by this:
- Ubuntu 16.04 does not support HTTP/2 in Apache. We thus cannot set
`GIT_TEST_HTTPD=true`, which would otherwise cause us to fail when
Apache fails to start.
- Ubuntu 16.04 cannot use recent JGit versions as they depend on a
more recent Java runtime than we have available. We thus disable
installing any kind of optional dependencies that do not come from
the package manager.
These two restrictions are fine though, as we only really care about
whether Git compiles and runs on such old distributions in the first
place.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Building older versions of Git may require tweaking some build knobs. In
particular, very old versions of Git will fail to build with recent
OpenSSL, because the bignum type switched from a struct to a pointer.
The i5500 interop test uses Git v1.0.0 by default, which triggers this
problem. You can work around it by setting NO_OPENSSL in your
GIT_TEST_MAKE_OPTS variable. But there are two downsides:
1. You have to know to do this, and it's not at all obvious.
2. That sets the options for _all_ versions of Git that we build. And
it's possible for two versions to require conflicting knobs. E.g.,
building with "make NO_OPENSSL=Nope OPENSSL_SHA1=Yes" causes
imap-send.c to barf, because it declares a fallback typedef for SSL.
This is something we may want to fix, but of course many historical
versions are affected, and the interop scripts should be flexible
enough to build everything.
So let's introduce per-version make options, along with the ability for
scripts to specify knobs that match their default versions. That should
make everything build out of the box, but also allow testers flexibility
if they are testing interoperability between non-default versions.
We'll set NO_OPENSSL by default for v1.0.0 in i5500. It doesn't have to
worry about the conflict with OPENSSL_SHA1 because imap-send did not
exist back then (but if it did, it could also just explicitly use a
different hash implementation).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git cat-file" works well with the sparse-index, and gets marked as
such.
* kl/cat-file-on-sparse-index:
builtin/cat-file: mark 'git cat-file' sparse-index compatible
t1092: allow run_on_* functions to use standard input
One-line messages to "die" and other helper functions will get LF
added by these helper functions, but many existing messages had an
unnecessary LF at the end, which have been corrected.
* jk/messages-with-excess-lf-fix:
drop trailing newline from warning/error/die messages
"git pack-refs --auto" for the files backend was too aggressive,
which has been a bit tamed.
* ps/pack-refs-auto-heuristics:
refs/files: use heuristic to decide whether to repack with `--auto`
t0601: merge tests for auto-packing of refs
wrapper: introduce `log2u()`
A data corruption bug when multi-pack-index is used and the same
objects are stored in multiple packfiles has been corrected.
* tb/multi-pack-reuse-fix:
builtin/pack-objects.c: do not open-code `MAX_PACK_OBJECT_HEADER`
pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse
builtin/pack-objects.c: translate bit positions during pack-reuse
pack-bitmap: tag bitmapped packs with their corresponding MIDX
t/t5332-multi-pack-reuse.sh: verify pack generation with --strict
"git verify-pack" and "git index-pack" started dying outside a
repository, which has been corrected.
* ps/index-pack-outside-repo-fix:
builtin/index-pack: fix segfaults when running outside of a repo
"git bundle unbundle" outside a repository triggered a BUG()
unnecessarily, which has been corrected.
* ps/bundle-outside-repo-fix:
bundle: default to SHA1 when reading bundle headers
builtin/bundle: have unbundle check for repo before opening its bundle
The patch parser in "git patch-id" has been tightened to avoid
getting confused by lines that look like a patch header in the log
message.
cf. <Zqh2T_2RLt0SeKF7@tanuki>
* jc/patch-id:
patch-id: tighten code to detect the patch header
patch-id: rewrite code that detects the beginning of a patch
patch-id: make get_one_patchid() more extensible
patch-id: call flush_current_id() only when needed
t4204: patch-id supports various input format
If NO_OPENSSL is defined, then imap-send.c defines a fallback "SSL"
type, which is just a void pointer that remains NULL. This works, but it
has one problem: it is using the type name "SSL", which conflicts with
the upstream name, if some other part of the system happens to include
openssl. For example:
$ make NO_OPENSSL=Nope OPENSSL_SHA1=Yes imap-send.o
CC imap-send.o
imap-send.c:35:15: error: conflicting types for ‘SSL’; have ‘void *’
35 | typedef void *SSL;
| ^~~
In file included from /usr/include/openssl/evp.h:26,
from sha1/openssl.h:4,
from hash.h:10,
from object.h:4,
from commit.h:4,
from refs.h:4,
from setup.h:4,
from imap-send.c:32:
/usr/include/openssl/types.h:187:23: note: previous declaration of ‘SSL’ with type ‘SSL’ {aka ‘struct ssl_st’}
187 | typedef struct ssl_st SSL;
| ^~~
make: *** [Makefile:2761: imap-send.o] Error 1
This is not a terribly common combination in practice:
1. Why are we disabling openssl support but still using its sha1? The
answer is that you may use the same build options across many
versions, and some older versions of Git no longer build with
modern versions of openssl.
2. Why are we using a totally unsafe sha1 that does not detect
collisions? You're right, we shouldn't. But in preparation for
using unsafe sha1 for non-cryptographic checksums, it would be nice
to be able to turn it on without hassle.
We can make this work by adjusting the way imap-send handles its
fallback. One solution is something like this:
#ifdef NO_OPENSSL
#define git_SSL void *
#else
#define git_SSL SSL
#endif
But we can observe that we only need this definition in one spot: the
struct which holds the variable. So rather than play around with macros
that may cause unexpected effects, we can just directly use the correct
type in that struct.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The linux32 job runs inside a docker container with a 32-bit libc, etc.
This breaks any GitHub Actions scripts that are implemented in
javascript, because they ship with their own 64-bit version of Node.js
that's dynamically linked. They'll fail with a message like:
exec /__e/node20/bin/node: no such file or directory
because they can't find the runtime linker.
This hasn't been a problem until recently because we special-case older,
non-javascript versions of these actions for the linux32 job. But it
recently became an issue when our old version of actions/upload-artifact
was deprecated, causing the job to fail. We worked around that in
90f2c7240c (ci: remove 'Upload failed tests' directories' step from
linux32 jobs, 2024-09-09), but it meant a loss of functionality for that
job. And we may eventually run into the same deprecation problem with
actions/checkout, which can't just be removed.
We can solve the linking issue by installing the 64-bit libc and stdc++
packages before doing anything else. Coupled with the switch to a more
recent image in the previous patch, that lets us remove the
special-casing of the action scripts entirely.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The Xenial image we're using was released more than 8 years ago. This is
a problem for using some recent GitHub Actions scripts, as they require
Node.js 20, and all of the binaries they ship need glibc 2.28 or later.
We're not using them yet, but moving forward prepares us for a future
patch which will.
Xenial was actually the last official 32-bit Ubuntu release, but you can
still find i386 images for more recent releases. This patch uses Focal,
which was released in 2020 (and is the oldest one with glibc 2.28).
There are two small downsides here:
- while Xenial is pretty old, it is still in LTS support until April
2026. So there's probably some value in testing with such an old
system, and we're losing that.
- there are no i386 subversion packages in the Focal repository. So we
won't be able to test that (OTOH, we had never tested it until the
previous patch which unified the 32/64-bit dependency code).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The script to install dependencies has two separate entries for 32-bit
and 64-bit Ubuntu systems. This increases the maintenance burden since
both should need roughly the same packages.
That hasn't been too bad so far because we've stayed on the same 32-bit
image since 2017. Trying to move to a newer image revealed several
problems with the linux32 job:
- newer images complain about using "linux32 --32bit i386", due to
seccomp restrictions. We can loosen these with a docker option, but
I don't think running it is even doing anything. We use it only for
pretending to "apt" that we're on a 32-bit machine, but inside the
container image apt is already configured as a 32-bit system (even
though the kernel outside the container is obviously 64-bit). Using
the same apt invocation for both architectures just gets rid of this
call entirely.
- we set DEBIAN_FRONTEND to avoid hanging on packages that ask the
user questions. This wasn't a problem on the old image, but it is on
newer ones. The 64-bit stanza handles this already.
As a bonus, the 64-bit stanza uses "apt -q" instead of redirecting
output to /dev/null. This would have saved me a lot of debugging
time trying to figure out why it was hanging. :)
- the old image seems to have zlib-dev installed by default, but newer
ones do not.
In addition, there were probably many tests being skipped on the 32-bit
build because we didn't have support packages installed (e.g., gpg). Now
we'll run them.
We do need to keep some parts split off just for 64-bit systems: our p4
and lfs installs reference x86_64/amd64 binaries. The downloaded jgit
should work in theory, since it's just a jar file embedded in a shell
script that relies on the system java. But the system java in our image
is too old, so I've left it as 64-bit only for now.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We haven't used these scripts since 4a6e4b9602 (CI: remove Travis CI
support, 2021-11-23), as the GitHub Actions config has support for
directly running jobs within docker containers.
It's possible we might want to resurrect something like this in order to
be more agnostic to the CI platform. But it's not clear exactly what it
would look like. And in the meantime, it's just a maintenance burden as
we make changes to CI config, and is subject to bitrot. In fact it's
already broken; it references ci/install-docker-dependencies.sh, which
went away in 9cdeb34b96 (ci: merge scripts which install dependencies,
2024-04-12).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Stop storing the "core.notesRef" config value globally. Instead,
retrieve the value in `default_notes_ref()`. The code is never called in
a hot loop anyway, so doing this on every invocation should be perfectly
fine.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Same as the preceding commits, storing the "core.warnAmbiguousRefs"
value globally is misdesigned as this setting may be set per repository.
Move the logic into the repo-settings subsystem. The usual pattern here
is that users are expected to call `prepare_repo_settings()` before they
access the settings themselves. This seems somewhat fragile though, as
it is easy to miss and leads to somewhat ugly code patterns at the call
sites.
Instead, introduce a new function that encapsulates this logic for us.
This also allows us to change how exactly the lazy initialization works
in the future, e.g. by only partially initializing values as requested
by the caller.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Same as the preceding commit, storing the "core.preferSymlinkRefs" value
globally is misdesigned as this setting may be set per repository.
There is only a single user of this value anyway, namely the "files"
backend. So let's just remove the global variable and read the value of
this setting when initializing the backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The value of "core.logAllRefUpdates" is being stored in the global
variable `log_all_ref_updates`. This design is somewhat aged nowadays,
where it is entirely possible to access multiple repositories in the
same process which all have different values for this setting. So using
a single global variable to track it is plain wrong.
Remove the global variable. Instead, we now provide a new function part
of the repo-settings subsystem that parses the value for a specific
repository. While that may require us to read the value multiple times,
we work around this by reading it once when the ref backends are set up
and caching the value there.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In refs-related code we modify the global `log_all_ref_updates`
variable, which is done because `should_autocreate_reflog()` does not
accept passing an `enum log_refs_config` but instead accesses the global
variable. Adapt its interface such that the value is provided by the
caller, which allows us to compute the proper value locally without
having to modify global state.
This change requires us to move the enum to "repo-settings.h", or
otherwise we get compilation errors due to include cycles. We're about
to fully move this setting into the repo-settings subsystem anyway, so
this is fine.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In "branch.c" we modify the global `log_all_ref_updates` variable to
force creation of a reflog entry. Modifying global state like this is
discouraged, as it may have all kinds of consequences in other places of
our codebase.
Stop modifying the variable and pass the `REF_FORCE_CREATE_REFLOG` flag
instead. Setting this flag has a stronger meaning than setting the
config to `LOG_REFS_NORMAL`:
- `LOG_REFS_NORMAL` will ask us to only create reflog entries for
preexisting reflogs or branches, remote refs, note refs and HEAD.
- `REF_FORCE_CREATE_REFLOG` will unconditionally create a reflog and
is thus equivalent to `LOG_REFS_ALWAYS`.
But as we are in `create_branch()` and thus do not have to worry about
arbitrary references, but only about branches, `LOG_REFS_NORMAL` and
`LOG_REFS_ALWAYS` are indeed equivalent.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The default values for `struct repo_settings` are set up in
`prepare_repo_settings()`. This is somewhat different from how we
typically do this, namely by providing an `INIT` macro that sets up the
default values for us.
Refactor the code to do the same.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
While we have "repo-settings.c", we do not have a corresponding
"repo-settings.h" file. Instead, this functionality is part of the
"repository.h" header, making it hard to discover.
Split the declarations out of "repository.h" and create a standalone
header file with them.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In "environment.h" we have quite a lot of functions and variables that
either explicitly or implicitly depend on `the_repository`.
The implicit set of stateful declarations includes for example variables
which get populated when parsing a repository's Git configuration. This
set of variables is broken by design, as their state often depends on
the last repository config that has been parsed. So they may or may not
represent the state of `the_repository`.
Fixing that is quite a big undertaking, and later patches in this series
will demonstrate a solution for a first small set of those variables. So
for now, let's guard these with `USE_THE_REPOSITORY_VARIABLE` so that
callers are aware of the implicit dependency.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano