Junio C Hamano [Mon, 24 Jun 2024 23:39:14 +0000 (16:39 -0700)]
Merge branch 'jc/add-i-retire-usebuiltin-config'
For over a year, setting add.interactive.useBuiltin configuration
variable did nothing but giving a "this does not do anything"
warning. Finally remove it.
Junio C Hamano [Mon, 24 Jun 2024 23:39:14 +0000 (16:39 -0700)]
Merge branch 'jc/no-default-attr-tree-in-bare'
Earlier we stopped using the tree of HEAD as the default source of
attributes in a bare repository, but failed to document it. This
has been corrected.
* jc/no-default-attr-tree-in-bare:
attr.tree: HEAD:.gitattributes is no longer the default in a bare repo
Junio C Hamano [Mon, 24 Jun 2024 23:39:13 +0000 (16:39 -0700)]
Merge branch 'tb/precompose-getcwd'
We forgot to normalize the result of getcwd() to NFC on macOS where
all other paths are normalized, which has been corrected. This still
does not address the case where core.precomposeUnicode configuration
is not defined globally.
* tb/precompose-getcwd:
macOS: ls-files path fails if path of workdir is NFD
The pseudo-merge reachability bitmap to help more efficient storage
of the reachability bitmap in a repository with too many refs has
been added.
* tb/pseudo-merge-reachability-bitmap: (26 commits)
pack-bitmap.c: ensure pseudo-merge offset reads are bounded
Documentation/technical/bitmap-format.txt: add missing position table
t/perf: implement performance tests for pseudo-merge bitmaps
pseudo-merge: implement support for finding existing merges
ewah: `bitmap_equals_ewah()`
pack-bitmap: extra trace2 information
pack-bitmap.c: use pseudo-merges during traversal
t/test-lib-functions.sh: support `--notick` in `test_commit_bulk()`
pack-bitmap: implement test helpers for pseudo-merge
ewah: implement `ewah_bitmap_popcount()`
pseudo-merge: implement support for reading pseudo-merge commits
pack-bitmap.c: read pseudo-merge extension
pseudo-merge: scaffolding for reads
pack-bitmap: extract `read_bitmap()` function
pack-bitmap-write.c: write pseudo-merge table
pseudo-merge: implement support for selecting pseudo-merge commits
config: introduce `git_config_double()`
pack-bitmap: make `bitmap_writer_push_bitmapped_commit()` public
pack-bitmap: implement `bitmap_writer_has_bitmapped_object_id()`
pack-bitmap-write: support storing pseudo-merge commits
...
René Scharfe [Mon, 24 Jun 2024 19:15:45 +0000 (21:15 +0200)]
diff: allow --color-moved with --no-ext-diff
We ignore the option --color-moved if an external diff program is
configured, presumably because its overhead is unnecessary in that case.
Respect the option if we don't actually use the external diff, though.
Reported-by: lolligerhans@gmx.de Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Wong [Sat, 22 Jun 2024 04:36:48 +0000 (04:36 +0000)]
object-file: fix leak on conversion failure
I'm not sure exactly how to trigger the leak, but it seems fairly
obvious that the `content' buffer should be freed even if
convert_object_file() fails. Noticed while working in this area
on unrelated things.
Signed-off-by: Eric Wong <e@80x24.org> Acked-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Sat, 22 Jun 2024 05:08:42 +0000 (22:08 -0700)]
fuzz: minimum fuzzers environment lacks libcURL
The "fuzz smoke test" job compiles various .o files to create
libgit.a and others, but the final build product of the fuzzer build
is *not* "git". Since the job is not interested in building a
working "git", it does not define any build flags, and among the
notable ones that are missing is NO_CURL---even though the CI
environment that runs the job does not have libcURL development
package installed.
This obviously leads to a build failure.
Pass NO_CURL=NoThanks to "make" to make sure things will build
correctly, if we add any conditional compilation with "#ifdef
NO_CURL ... #endif" in the codebase.
Junio C Hamano [Thu, 20 Jun 2024 22:45:16 +0000 (15:45 -0700)]
Merge branch 'ps/document-breaking-changes'
The structure of the document that records longer-term project
decisions to deprecate/remove/update various behaviour has been
outlined.
* ps/document-breaking-changes:
BreakingChanges: document that we do not plan to deprecate git-checkout
BreakingChanges: document removal of grafting
BreakingChanges: document upcoming change from "sha1" to "sha256"
docs: introduce document to announce breaking changes
Junio C Hamano [Thu, 20 Jun 2024 22:45:15 +0000 (15:45 -0700)]
Merge branch 'pw/rebase-i-error-message'
When the user adds to "git rebase -i" instruction to "pick" a merge
commit, the error experience is not pleasant. Such an error is now
caught earlier in the process that parses the todo list.
* pw/rebase-i-error-message:
rebase -i: improve error message when picking merge
rebase -i: pass struct replay_opts to parse_insn_line()
Junio C Hamano [Thu, 20 Jun 2024 22:45:13 +0000 (15:45 -0700)]
Merge branch 'ps/abbrev-length-before-setup-fix'
Setting core.abbrev too early before the repository set-up
(typically in "git clone") caused segfault, which as been
corrected.
* ps/abbrev-length-before-setup-fix:
object-name: don't try to abbreviate to lengths greater than hexsz
parse-options-cb: stop clamping "--abbrev=" to hash length
config: fix segfault when parsing "core.abbrev" without repo
"git format-patch --interdiff" for multi-patch series learned to
turn on cover letters automatically (unless told never to enable
cover letter with "--no-cover-letter" and such).
* rj/format-patch-auto-cover-with-interdiff:
format-patch: assume --cover-letter for diff in multi-patch series
t4014: cleanups in a few tests
Junio C Hamano [Thu, 20 Jun 2024 22:45:11 +0000 (15:45 -0700)]
Merge branch 'kn/update-ref-symref'
"git update-ref --stdin" learned to handle transactional updates of
symbolic-refs.
* kn/update-ref-symref:
update-ref: add support for 'symref-update' command
reftable: pick either 'oid' or 'target' for new updates
update-ref: add support for 'symref-create' command
update-ref: add support for 'symref-delete' command
update-ref: add support for 'symref-verify' command
refs: specify error for regular refs with `old_target`
refs: create and use `ref_update_expects_existing_old_ref()`
Junio C Hamano [Thu, 20 Jun 2024 22:45:09 +0000 (15:45 -0700)]
Merge branch 'tb/multi-pack-reuse-fix'
Assorted fixes to multi-pack-index code paths.
* tb/multi-pack-reuse-fix:
pack-revindex.c: guard against out-of-bounds pack lookups
pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse
midx-write.c: do not read existing MIDX with `packs_to_include`
"git diff --exit-code --ext-diff" learned to take the exit status
of the external diff driver into account when deciding the exit
status of the overall "git diff" invocation when configured to do
so.
* rs/diff-exit-code-with-external-diff:
diff: let external diffs report that changes are uninteresting
userdiff: add and use struct external_diff
t4020: test exit code with external diffs
version: --build-options reports OpenSSL version information
This change uses the OpenSSL supplied OPENSSL_VERSION_TEXT #define supplied
for this purpose by that project. If the #define is not present, the version
is not reported.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Wed, 19 Jun 2024 17:13:19 +0000 (19:13 +0200)]
commit: remove find_header_mem()
cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)
introduced find_header_mem() and turned find_commit_header() into a thin
wrapper. Since then, the latter has become the last remaining caller of
the former. Remove it to restore find_commit_header() to the state
before cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK
note in the process.
Signed-off-by: René Scharfe <l.s.r@web.de> Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Wed, 19 Jun 2024 12:52:55 +0000 (08:52 -0400)]
t5500: fix mistaken $SERVER reference in helper function
The end of t5500 contains two tests which use a single helper function,
fetch_filter_blob_limit_zero(). It takes a parameter to point to the
path of the server repository, which we store locally as $SERVER. The
first caller uses the relative path "server", while the second points
into the httpd document root.
Commit 07ef3c6604 (fetch test: use more robust test for filtered
objects, 2019-12-23) refactored some lines, but accidentally switched
"$SERVER" to "server" in one spot. That means the second caller is
looking at the server directory from the previous test rather than its
own.
This happens to work out because the "server" directory from the first
test is still hanging around, and the contents of the two are identical.
But it was clearly not the intended behavior, and is fragile to cleaning
up the leftovers from the first test.
Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
mingw: drop bogus (and unneeded) declaration of `_pgmptr`
In 08809c09aa13 (mingw: add a helper function to attach GDB to the
current process, 2020-02-13), I added a declaration that was not needed.
Back then, that did not matter, but now that the declaration of that
symbol was changed in mingw-w64's headers, it causes the following
compile error:
CC compat/mingw.o
compat/mingw.c: In function 'open_in_gdb':
compat/mingw.c:35:9: error: function declaration isn't a prototype [-Werror=strict-prototypes]
35 | extern char *_pgmptr;
| ^~~~~~
In file included from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/mm_malloc.h:27,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/xmmintrin.h:34,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/immintrin.h:31,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/x86intrin.h:32,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/winnt.h:1658,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/minwindef.h:163,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/windef.h:9,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/windows.h:69,
from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/winsock2.h:23,
from compat/../git-compat-util.h:215,
from compat/mingw.c:1:
compat/mingw.c:35:22: error: '__p__pgmptr' redeclared without dllimport attribute: previous dllimport ignored [-Werror=attributes]
35 | extern char *_pgmptr;
| ^~~~~~~
Let's just drop the declaration and get rid of this compile error.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Wed, 19 Jun 2024 13:02:56 +0000 (09:02 -0400)]
fetch-pack: fix segfault when fscking without --lock-pack
The fetch-pack internals have multiple options related to creating
".keep" lock-files for the received pack:
- if args.lock_pack is set, then we tell index-pack to create a .keep
file. In the fetch-pack plumbing command, this is triggered by
passing "-k" twice.
- if the caller passes in a pack_lockfiles string list, then we use it
to record the path of the keep-file created by index-pack. We get
that name by reading the stdout of index-pack. In the fetch-pack
command, this is triggered by passing the (undocumented) --lock-pack
option; without it, we pass in a NULL string list.
So it's possible to ask index-pack to create the lock-file (using "-k
-k") but not ask to record it (by avoiding "--lock-pack"). This worked
fine until 5476e1efde (fetch-pack: print and use dangling .gitmodules,
2021-02-22), but now it causes a segfault.
Before that commit, if pack_lockfiles was NULL, we wouldn't bother
reading the output from index-pack at all. But since that commit,
index-pack may produce extra output if we asked it to fsck. So even if
nobody cares about the lockfile path, we still need to read it to skip
to the output we do care about.
We correctly check that we didn't get a NULL lockfile path (which can
happen if we did not ask it to create a .keep file at all), but we
missed the case where the lockfile path is not NULL (due to "-k -k") but
the pack_lockfiles string_list is NULL (because nobody passed
"--lock-pack"), and segfault trying to add to the NULL string-list.
We can fix this by skipping the append to the string list when either
the value or the list is NULL. In that case we must also free the
lockfile path to avoid leaking it when it's non-NULL.
Nobody noticed the bug for so long because the transport code used by
"git fetch" always passes in a pack_lockfiles pointer, and remote-curl
(the main user of the fetch-pack plumbing command) always passes
--lock-pack.
Reported-by: Kirill Smelkov <kirr@nexedi.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 19 Jun 2024 03:00:19 +0000 (03:00 +0000)]
merge-ort: convert more error() cases to path_msg()
merge_submodule() stores errors using path_msg(), whereas other call
sites make use of the error() function. This is inconsistent, and
moving towards path_msg() seems more friendly for libification efforts
since it will allow the caller to determine whether the error messages
need to be printed.
Note that this deferred handling of error messages changes the error
message in a recursive merge from
error: failed to execute internal merge
to
From inner merge: error: failed to execute internal merge
which provides a little more information about the error which may be
useful. Since the recursive merge strategy still only shows the older
error, we had to adjust the new testcase introduced a few commits ago to
just search for the older message somewhere in the output.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 19 Jun 2024 03:00:18 +0000 (03:00 +0000)]
merge-ort: upon merge abort, only show messages causing the abort
When something goes wrong enough that we need to abort early and not
even attempt merging the remaining files, it probably does not make
sense to report conflicts messages for the subset of files we processed
before hitting the fatal error. Instead, only show the messages
associated with paths where we hit the fatal error. Also, print these
messages to stderr rather than stdout.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 19 Jun 2024 03:00:17 +0000 (03:00 +0000)]
merge-ort: loosen commented requirements
The comment above type_short_descriptions claimed that the order had to
match what was found in the conflict_info_and_types enum. Since
type_short_descriptions uses designated initializers, the order should
not actually matter; I am guessing that positional initializers may have
been under consideration when that comment was added, but the comment
was not updated when designated initializers were chosen.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 19 Jun 2024 03:00:16 +0000 (03:00 +0000)]
merge-ort: clearer propagation of failure-to-function from merge_submodule
The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
conflicted, -1 = failure-to-determine), but we often like to think of
it as binary (ignoring the possibility of a negative value) and use
constructs like '!clean' to reflect this. However, these constructs
can make codepaths more difficult to understand, unless we handle the
negative case early and return pre-emptively; do that in
handle_content_merge() to make the code a bit easier to read.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 19 Jun 2024 03:00:15 +0000 (03:00 +0000)]
merge-ort: fix type of local 'clean' var in handle_content_merge ()
handle_content_merge() returns an int. Every caller of
handle_content_merge() expects an int. However, we declare a local
variable 'clean' that we use for the return value to be unsigned. To
make matters worse, we also assign 'clean' the return value of
merge_submodule() in one codepath, which is defined to return an int.
It seems that the only reason to have 'clean' be unsigned was to allow a
cutesy bit manipulation operation to be well-defined. Fix the type of
the 'clean' local in handle_content_merge().
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 19 Jun 2024 03:00:14 +0000 (03:00 +0000)]
merge-ort: maintain expected invariant for priv member
The calling convention for the merge machinery is
One call to init_merge_options()
One or more calls to merge_incore_[non]recursive()
One call to merge_finalize()
(possibly indirectly via merge_switch_to_result())
Both merge_switch_to_result() and merge_finalize() expect
opt->priv == NULL && result->priv != NULL
which is supposed to be set up by our move_opt_priv_to_result_priv()
function. However, two codepaths dealing with error cases did not
execute this necessary logic, which could result in assertion failures
(or, if assertions were compiled out, could result in segfaults). Fix
the oversight and add a test that would have caught one of these
problems.
While at it, also tighten an existing test for a non-recursive merge
to verify that it fails with appropriate status. Most merge tests in
the testsuite check either for success or conflicts; those testing for
neither are rare and it is good to ensure they support the invariant
assumed by builtin/merge.c in this comment:
/*
* The backend exits with 1 when conflicts are
* left to be resolved, with 2 when it does not
* handle the given merge at all.
*/
So, explicitly check for the exit status of 2 in these cases.
Reported-by: Matt Cree <matt.cree@gearset.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 19 Jun 2024 03:00:13 +0000 (03:00 +0000)]
merge-ort: extract handling of priv member into reusable function
In preparation for a subsequent commit which will ensure we do not
forget to maintain our invariants for the priv member in error
codepaths, extract the necessary functionality out into a separate
function. This change is cosmetic at this point, and introduces no
changes beyond an extra assertion sanity check.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Xing Xin [Wed, 19 Jun 2024 04:07:33 +0000 (04:07 +0000)]
unbundle: extend object verification for fetches
The existing fetch.fsckObjects and transfer.fsckObjects configurations
were not fully applied to bundle-involved fetches, including direct
bundle fetches and bundle-uri enabled fetches. Furthermore, there was no
object verification support for unbundle.
This commit extends object verification support in `bundle.c:unbundle`
by adding the `VERIFY_BUNDLE_FSCK` option to `verify_bundle_flags`. When
this option is enabled, we append the `--fsck-objects` flag to
`git-index-pack`.
The `VERIFY_BUNDLE_FSCK` option is now used by bundle-involved fetches,
where we use `fetch-pack.c:fetch_pack_fsck_objects` to determine whether
to enable this option for `bundle.c:unbundle`, specifically in:
- `transport.c:fetch_refs_from_bundle` for direct bundle fetches.
- `bundle-uri.c:unbundle_from_file` for bundle-uri enabled fetches.
This addition ensures a consistent logic for object verification during
fetches. Tests have been added to confirm functionality in the scenarios
mentioned above.
Reviewed-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Xing Xin <xingxin.xx@bytedance.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently, we can use "transfer.fsckObjects" and the more specific
"fetch.fsckObjects" to control checks for broken objects in received
packs during fetches. However, these configurations were only
acknowledged by `fetch-pack.c:get_pack` and did not take effect in
direct bundle fetches or fetches with _bundle-uri_ enabled.
This commit exposes the fetch-then-transfer configuration logic by
adding a new function `fetch_pack_fsck_objects` in fetch-pack.h. This
new function is used to replace the assignment for `fsck_objects` in
`fetch-pack.c:get_pack`. In the next commit, this function will also be
used to extend fsck support for bundle-involved fetches.
Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Xing Xin <xingxin.xx@bytedance.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Xing Xin [Wed, 19 Jun 2024 04:07:31 +0000 (04:07 +0000)]
bundle-uri: verify oid before writing refs
When using the bundle-uri mechanism with a bundle list containing
multiple interrelated bundles, we encountered a bug where tips from
downloaded bundles were not discovered, thus resulting in rather slow
clones. This was particularly problematic when employing the
"creationTokens" heuristic.
To reproduce this issue, consider a repository with a single branch
"main" pointing to commit "A". Firstly, create a base bundle with:
git bundle create base.bundle main
Then, add a new commit "B" on top of "A", and create an incremental
bundle for "main":
git bundle create incr.bundle A..main
Now, generate a bundle list with the following content:
[bundle]
version = 1
mode = all
heuristic = creationToken
[bundle "base"]
uri = base.bundle
creationToken = 1
[bundle "incr"]
uri = incr.bundle
creationToken = 2
A fresh clone with the bundle list above should result in a reference
"refs/bundles/main" pointing to "B" in the new repository. However, git
would still download everything from the server, as if it had fetched
nothing locally.
So why the "refs/bundles/main" is not discovered? After some digging I
found that:
1. Bundles in bundle list are downloaded to local files via
`bundle-uri.c:download_bundle_list` or via
`bundle-uri.c:fetch_bundles_by_token` for the "creationToken"
heuristic.
2. Each bundle is unbundled via `bundle-uri.c:unbundle_from_file`, which
is called by `bundle-uri.c:unbundle_all_bundles` or called within
`bundle-uri.c:fetch_bundles_by_token` for the "creationToken"
heuristic.
3. To get all prerequisites of the bundle, the bundle header is read
inside `bundle-uri.c:unbundle_from_file` to by calling
`bundle.c:read_bundle_header`.
4. Then it calls `bundle.c:unbundle`, which calls
`bundle.c:verify_bundle` to ensure the repository contains all the
prerequisites.
5. `bundle.c:verify_bundle` calls `parse_object`, which eventually
invokes `packfile.c:prepare_packed_git` or
`packfile.c:reprepare_packed_git`, filling
`raw_object_store->packed_git` and setting `packed_git_initialized`.
6. If `bundle.c:unbundle` succeeds, it writes refs via
`refs.c:refs_update_ref` with `REF_SKIP_OID_VERIFICATION` set. Here
bundle refs which can target arbitrary objects are written to the
repository.
7. Finally, in `fetch-pack.c:do_fetch_pack_v2`, the functions
`fetch-pack.c:mark_complete_and_common_ref` and
`fetch-pack.c:mark_tips` are called with `OBJECT_INFO_QUICK` set to
find local tips for negotiation. The `OBJECT_INFO_QUICK` flag
prevents `packfile.c:reprepare_packed_git` from being called,
resulting in failures to parse OIDs that reside only in the latest
bundle.
In the example above, when unbunding "incr.bundle", "base.pack" is added
to `packed_git` due to prerequisites verification. However, "B" cannot
be found for negotiation because it exists in "incr.pack", which is not
included in `packed_git`.
Fix the bug by removing `REF_SKIP_OID_VERIFICATION` flag when writing
bundle refs. When `refs.c:refs_update_ref` is called to write the
corresponding bundle refs, it triggers `refs.c:ref_transaction_commit`.
This, in turn, invokes `refs.c:ref_transaction_prepare`, which calls
`transaction_prepare` of the refs storage backend. For files backend, it
is `files-backend.c:files_transaction_prepare`, and for reftable
backend, it is `reftable-backend.c:reftable_be_transaction_prepare`.
Both functions eventually call `object.c:parse_object`, which can invoke
`packfile.c:reprepare_packed_git` to refresh `packed_git`. This ensures
that bundle refs point to valid objects and that all tips from bundle
refs are correctly parsed during subsequent negotiations.
A set of negotiation-related tests for cloning with bundle-uri has been
included to demonstrate that downloaded bundles are utilized to
accelerate fetching.
Additionally, another test has been added to show that bundles with
incorrect headers, where refs point to non-existent objects, do not
result in any bundle refs being created in the repository.
Reviewed-by: Karthik Nayak <karthik.188@gmail.com> Reviewed-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Xing Xin <xingxin.xx@bytedance.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Wong [Tue, 18 Jun 2024 21:30:41 +0000 (21:30 +0000)]
t1006: ensure cat-file info isn't buffered by default
While working on buffering changes to `git cat-file' in a
separate patch, I inadvertently made the output of --batch-check
and the `info' command of --batch-command buffered as if
opt->buffer_output is turned on by default.
Buffering by default breaks some 3rd-party Perl scripts using
cat-file, but this breakage was not detected anywhere in our
test suite. Add a small Perl snippet to test this problem since
(AFAIK) other equivalent ways to test this behavior from Bourne
shell and/or awk would require racy sleeps, non-portable FIFOs
or tedious C code.
Signed-off-by: Eric Wong <e@80x24.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Mon, 17 Jun 2024 22:55:57 +0000 (15:55 -0700)]
Merge branch 'ps/no-writable-strings'
Building with "-Werror -Wwrite-strings" is now supported.
* ps/no-writable-strings: (27 commits)
config.mak.dev: enable `-Wwrite-strings` warning
builtin/merge: always store allocated strings in `pull_twohead`
builtin/rebase: always store allocated string in `options.strategy`
builtin/rebase: do not assign default backend to non-constant field
imap-send: fix leaking memory in `imap_server_conf`
imap-send: drop global `imap_server_conf` variable
mailmap: always store allocated strings in mailmap blob
revision: always store allocated strings in output encoding
remote-curl: avoid assigning string constant to non-const variable
send-pack: always allocate receive status
parse-options: cast long name for OPTION_ALIAS
http: do not assign string constant to non-const field
compat/win32: fix const-correctness with string constants
pretty: add casts for decoration option pointers
object-file: make `buf` parameter of `index_mem()` a constant
object-file: mark cached object buffers as const
ident: add casts for fallback name and GECOS
entry: refactor how we remove items for delayed checkouts
line-log: always allocate the output prefix
line-log: stop assigning string constant to file parent buffer
...
Junio C Hamano [Mon, 17 Jun 2024 22:55:56 +0000 (15:55 -0700)]
Merge branch 'jk/am-retry'
"git am" has a safety feature to prevent it from starting a new
session when there already is a session going. It reliably
triggers when a mbox is given on the command line, but it has to
rely on the tty-ness of the standard input. Add an explicit way to
opt out of this safety with a command line option.
Junio C Hamano [Mon, 17 Jun 2024 22:55:55 +0000 (15:55 -0700)]
Merge branch 'jc/varargs-attributes'
Varargs functions that are unannotated as printf-like or execl-like
have been annotated as such.
* jc/varargs-attributes:
__attribute__: add a few missing format attributes
__attribute__: mark some functions with LAST_ARG_MUST_BE_NULL
__attribute__: remove redundant attribute declaration for git_die_config()
__attribute__: trace2_region_enter_printf() is like "printf"
Junio C Hamano [Mon, 17 Jun 2024 22:55:55 +0000 (15:55 -0700)]
Merge branch 'ps/ref-storage-migration'
A new command has been added to migrate a repository that uses the
files backend for its ref storage to use the reftable backend, with
limitations.
* ps/ref-storage-migration:
builtin/refs: new command to migrate ref storage formats
refs: implement logic to migrate between ref storage formats
refs: implement removal of ref storages
worktree: don't store main worktree twice
reftable: inline `merged_table_release()`
refs/files: fix NULL pointer deref when releasing ref store
refs/files: extract function to iterate through root refs
refs/files: refactor `add_pseudoref_and_head_entries()`
refs: allow to skip creation of reflog entries
refs: pass storage format to `ref_store_init()` explicitly
refs: convert ref storage format to an enum
setup: unset ref storage when reinitializing repository version
Junio C Hamano [Mon, 17 Jun 2024 22:55:54 +0000 (15:55 -0700)]
Merge branch 'ps/check-docs-fix'
"make check-docs" noticed problems and reported to its output but
failed to signal its findings with its exit status, which has been
corrected.
* ps/check-docs-fix:
ci/test-documentation: work around SyntaxWarning in Python 3.12
gitlab-ci: add job to run `make check-docs`
Documentation/lint-manpages: bubble up errors
Makefile: extract script to lint missing/extraneous manpages
Junio C Hamano [Mon, 17 Jun 2024 22:55:52 +0000 (15:55 -0700)]
Merge branch 'ap/credential-clear-fix'
Upon expiration event, the credential subsystem forgot to clear
in-core authentication material other than password (whose support
was added recently), which has been corrected.
Junio C Hamano [Mon, 17 Jun 2024 22:55:52 +0000 (15:55 -0700)]
Merge branch 'jc/format-patch-with-range-diff'
The inter/range-diff output has been moved to the end of the patch
when format-patch adds it to a single patch, instead of writing it
before the patch text, to be consistent with what is done for a
cover letter for a multi-patch series.
* jc/format-patch-with-range-diff:
format-patch: move range/inter diff at the end of a single patch output
show_log: factor out interdiff/range-diff generation
Kyle Lippincott [Mon, 17 Jun 2024 20:00:24 +0000 (20:00 +0000)]
attr: fix msan issue in read_attr_from_index
Memory sanitizer (msan) is detecting a use of an uninitialized variable
(`size`) in `read_attr_from_index`:
==2268==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x5651f3416504 in read_attr_from_index git/attr.c:868:11
#1 0x5651f3415530 in read_attr git/attr.c
#2 0x5651f3413d74 in bootstrap_attr_stack git/attr.c:968:6
#3 0x5651f3413d74 in prepare_attr_stack git/attr.c:1004:2
#4 0x5651f3413d74 in collect_some_attrs git/attr.c:1199:2
#5 0x5651f3413144 in git_check_attr git/attr.c:1345:2
#6 0x5651f34728da in convert_attrs git/convert.c:1320:2
#7 0x5651f3473425 in would_convert_to_git_filter_fd git/convert.c:1373:2
#8 0x5651f357a35e in index_fd git/object-file.c:2630:34
#9 0x5651f357aa15 in index_path git/object-file.c:2657:7
#10 0x5651f35db9d9 in add_to_index git/read-cache.c:766:7
#11 0x5651f35dc170 in add_file_to_index git/read-cache.c:799:9
#12 0x5651f321f9b2 in add_files git/builtin/add.c:346:7
#13 0x5651f321f9b2 in cmd_add git/builtin/add.c:565:18
#14 0x5651f321d327 in run_builtin git/git.c:474:11
#15 0x5651f321bc9e in handle_builtin git/git.c:729:3
#16 0x5651f321a792 in run_argv git/git.c:793:4
#17 0x5651f321a792 in cmd_main git/git.c:928:19
#18 0x5651f33dde1f in main git/common-main.c:62:11
The issue exists because `size` is an output parameter from
`read_blob_data_from_index`, but it's only modified if
`read_blob_data_from_index` returns non-NULL. The read of `size` when
calling `read_attr_from_buf` unconditionally may read from an
uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
before reading from `size`, but by then it's already too late: the
uninitialized read will have happened already. Furthermore, there's no
guarantee that the compiler won't reorder things so that it checks
`size` before checking `!buf`.
Make the call to `read_attr_from_buf` conditional on `buf` being
non-NULL, ensuring that `size` is not read if it's never set.
Signed-off-by: Kyle Lippincott <spectral@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Fri, 14 Jun 2024 19:23:58 +0000 (15:23 -0400)]
pack-bitmap.c: ensure pseudo-merge offset reads are bounded
After reading the pseudo-merge extension's metadata table, we allocate
an array to store information about each pseudo-merge, including its
byte offset within the .bitmap file itself.
This is done like so:
pseudo_merge_ofs = index_end - 24 -
(index->pseudo_merges.nr * sizeof(uint64_t));
for (i = 0; i < index->pseudo_merges.nr; i++) {
index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs);
pseudo_merge_ofs += sizeof(uint64_t);
}
But if the pseudo-merge table is corrupt, we'll keep calling get_be64()
past the end of the pseudo-merge extension, potentially reading off the
end of the mmap'd region.
Prevent this by ensuring that we have at least `table_size - 24` many
bytes available to read (adding 24 to the left-hand side of our
inequality to account for the length of the metadata component).
This is sufficient to prevent us from reading off the end of the
pseudo-merge extension, and ensures that all of the get_be64() calls
below are in bounds.
Helped-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Fri, 14 Jun 2024 19:23:55 +0000 (15:23 -0400)]
Documentation/technical/bitmap-format.txt: add missing position table
While investigating a benign Coverity warning on the new pseudo-merge
implementation, I was struggling to understand the (paraphrased) below:
ofs = index_end - 24 - (index->pseudo_merges.nr * sizeof(uint64_t));
for (i = 0; i < index->pseudo_merges.nr; i++) {
index->pseudo_merges.v[i].at = get_be64(ofs);
ofs += sizeof(uint64_t);
}
, in pack-bitmap.c::load_bitmap_header(). Looking at the documentation,
the diagram describing the on-disk format (prior to this patch)
suggested that the optional extended lookup table immediately preceded
the trailing metadata portion.
If that were the case, that would make the above code from
load_bitmap_header() incorrect, as we'd be blindly reading into the
extended offset table.
But later on in the documentation there is a description of the
pseudo-merge position table as immediately preceding the trailing
metadata portion of the extension. And indeed, we do write the position
table in pack-bitmap-write.c:
/* write positions for all pseudo merges */
for (i = 0; i < writer->pseudo_merges_nr; i++)
hashwrite_be64(f, pseudo_merge_ofs[i]);
hex: guard declarations with `USE_THE_REPOSITORY_VARIABLE`
Guard declarations of functions that implicitly use `the_repository`
with `USE_THE_REPOSITORY_VARIABLE` such that callers don't accidentally
rely on that global variable.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t/helper: remove dependency on `the_repository` in "proc-receive"
The "proc-receive" test helper implicitly relies on `the_repository` via
`parse_oid_hex()`. This isn't necessary though, and in fact the whole
command does not depend on `the_repository` at all.
Stop setting up `the_repository` and use `parse_oid_hex_any()` to parse
object IDs.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t/helper: fix segfault in "oid-array" command without repository
The "oid-array" test helper can supposedly work without a Git
repository, but will in fact crash because `the_repository->hash_algo`
is not initialized. This is because `oid_pos()`, which is used by
`oid_array_lookup()`, depends on `the_hash_algo->rawsz`.
Ideally, we'd adapt `oid_pos()` to not depend on `the_hash_algo`
anymore. That is a bigger untertaking though, so instead we fall back to
SHA1 when there is no repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t/helper: use correct object hash in partial-clone helper
The `object_info()` function of the partial-clone helper is responsible
for checking the object ID of a repository other than `the_repository`.
We use `parse_oid_hex()` in this function though, which means that we
still depend on `the_repository->hash_algo`.
Fix this by using the object hash of the function-local repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
compat/fsmonitor: fix socket path in networked SHA256 repos
The IPC socket used by the fsmonitor on Darwin is usually contained in
the Git repository itself. When the repository is hosted on a networked
filesystem though, we instead create the socket path in the user's home
directory or the socket directory. In that case, we derive the path by
hashing the repository path.
But while we always use SHA1 to hash the repository path, we then end up
using `hash_to_hex()` to append the computed hash to the socket path.
This is wrong because `hash_to_hex()` uses the hash algorithm configured
in `the_repository`, which may not be SHA1. The consequence is that we
may append uninitialized bytes to the path when operating in a SHA256
repository.
Fix this bug by using `hash_to_hex_algop()` with SHA1.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
replace-object: use hash algorithm from passed-in repository
In `register_replace_ref()`, we pass in a repository but then use
`get_oid_hex()` to parse passed-in object IDs, which implicitly uses
`the_repository`. Fix this by using the hash algorithm from the
passed-in repository instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
protocol-caps: use hash algorithm from passed-in repository
In `send_info()`, we pass in a repository but then use `get_oid_hex()`
to parse passed-in object IDs, which implicitly uses `the_repository`.
Fix this by using the hash algorithm from the passed-in repository
instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `oidset_parse_file_carefully()` function implicitly depends on
`the_repository` when parsing object IDs. Fix this by having callers
pass in the hash algorithm to use.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
http-fetch: don't crash when parsing packfile without a repo
The git-http-fetch(1) command accepts a `--packfile=` option, which
allows the user to specify that it shall fetch a specific packfile,
only. The parameter here is the hash of the packfile, which is specific
to the object hash used by the repository. This requirement is implicit
though via our use of `parse_oid_hex()`, which internally uses
`the_repository`.
The git-http-fetch(1) command allows for there to be no repository
though, which only exists such that we can show usage via the "-h"
option. In that case though, starting with c8aed5e8da (repository: stop
setting SHA1 as the default object hash, 2024-05-07), `the_repository`
does not have its object hash initialized anymore and thus we would
crash when trying to parse the object ID outside of a repository.
Fix this issue by dying immediately when we see a "--packfile="
parameter when outside a Git repository. This is not a functional
regression as we would die later on with the same error anyway.
Add a test to detect the segfault. We use the "nongit" function to do
so, which we need to allow-list in `test_must_fail ()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "hash-ll.h" header was introduced via d1cbe1e6d8 (hash-ll.h: split
out of hash.h to remove dependency on repository.h, 2023-04-22) to make
explicit the split between hash-related functions that rely on the
global `the_repository`, and those that don't. This split is no longer
necessary now that we we have removed the reliance on `the_repository`.
Merge "hash-ll.h" back into "hash.h". This causes some code units to not
include "repository.h" anymore, which requires us to add some forward
declarations.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
There is an include cycle between "refs.h" and "repository.h" via
"commit.h", "object.h" and "hash.h". This has the effect that several
definitions of structs and enums will not be visible once we merge
"hash-ll.h" back into "hash.h" in the next commit.
The only reason that "repository.h" includes "refs.h" is the definition
of `enum ref_storage_format`. Move it into "repository.h" and have
"refs.h" include "repository.h" instead to fix the cycle.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use of the `the_repository` variable is deprecated nowadays, and we
slowly but steadily convert the codebase to not use it anymore. Instead,
callers should be passing down the repository to work on via parameters.
It is hard though to prove that a given code unit does not use this
variable anymore. The most trivial case, merely demonstrating that there
is no direct use of `the_repository`, is already a bit of a pain during
code reviews as the reviewer needs to manually verify claims made by the
patch author. The bigger problem though is that we have many interfaces
that implicitly rely on `the_repository`.
Introduce a new `USE_THE_REPOSITORY_VARIABLE` macro that allows code
units to opt into usage of `the_repository`. The intent of this macro is
to demonstrate that a certain code unit does not use this variable
anymore, and to keep it from new dependencies on it in future changes,
be it explicit or implicit
For now, the macro only guards `the_repository` itself as well as
`the_hash_algo`. There are many more known interfaces where we have an
implicit dependency on `the_repository`, but those are not guarded at
the current point in time. Over time though, we should start to add
guards as required (or even better, just remove them).
Define the macro as required in our code units. As expected, most of our
code still relies on the global variable. Nearly all of our builtins
rely on the variable as there is no way yet to pass `the_repository` to
their entry point. For now, declare the macro in "biultin.h" to keep the
required changes at least a little bit more contained.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hash: require hash algorithm in `empty_tree_oid_hex()`
The `empty_tree_oid_hex()` function use `the_repository` to derive the
hash function that shall be used. Require callers to pass in the hash
algorithm to get rid of this implicit dependency.
While at it, remove the unused `empty_blob_oid_hex()` function.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hash: require hash algorithm in `is_empty_{blob,tree}_oid()`
Both functions `is_empty_{blob,tree}_oid()` use `the_repository` to
derive the hash function that shall be used. Require callers to pass in
the hash algorithm to get rid of this implicit dependency.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hash: make `is_null_oid()` independent of `the_repository`
The function `is_null_oid()` uses `oideq(oid, null_oid())` to check
whether a given object ID is the all-zero object ID. `null_oid()`
implicitly relies on `the_repository` though to return the correct null
object ID.
Get rid of this dependency by always comparing the complete hash array
for being all-zeroes. This is possible due to the refactoring of object
IDs so that their hash arrays are always fully initialized.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hash: convert `oidcmp()` and `oideq()` to compare whole hash
With the preceding commit, the hash array of object IDs is now fully
zero-padded even when the hash algorithm's output is smaller than the
array length. With that, we can now adapt both `oidcmp()` and `oideq()`
to unconditionally memcmp(3P) the whole array instead of depending on
the hash size.
While it may feel inefficient to compare unused bytes for e.g. SHA-1, in
practice the compiler should now be able to produce code that is better
optimized both because we have no branch anymore, but also because the
size to compare is now known at compile time. Goldbolt spits out the
following assembly on an x86_64 platform with GCC 14.1 for the old and
new implementations of `oidcmp()`:
Interestingly, the compiler decides to split the comparisons into two so
that it first compares the lower half of the object ID for equality and
then the upper half. If the first check shows a difference, then we
wouldn't even end up comparing the second half.
In both cases, the new generated code is significantly shorter and has
way less branches. While I didn't benchmark the change, I'd be surprised
if the new code was slower.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `oidcmp()` and `oideq()` functions only compare the prefix length as
specified by the given hash algorithm. This mandates that the object IDs
have a valid hash algorithm set, or otherwise we wouldn't be able to
figure out that prefix. As we do not have a hash algorithm in many
cases, for example when handling null object IDs, this assumption cannot
always be fulfilled. We thus have a fallback in place that instead uses
`the_repository` to derive the hash function. This implicit dependency
is hidden away from callers and can be quite surprising, especially in
contexts where there may be no repository.
In theory, we can adapt those functions to always memcmp(3P) the whole
length of their hash arrays. But there exist a couple of sites where we
populate `struct object_id`s such that only the prefix of its hash that
is actually used by the hash algorithm is populated. The remaining bytes
are left uninitialized. The fact that those bytes are uninitialized also
leads to warnings under Valgrind in some places where we copy those
bytes.
Refactor callsites where we populate object IDs to always initialize all
bytes. This also allows us to get rid of `oidcpy_with_padding()`, for
one because the input is now fully initialized, and because `oidcpy()`
will now always copy the whole hash array.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hash: require hash algorithm in `oidread()` and `oidclr()`
Both `oidread()` and `oidclr()` use `the_repository` to derive the hash
function that shall be used. Require callers to pass in the hash
algorithm to get rid of this implicit dependency.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hash: require hash algorithm in `hasheq()`, `hashcmp()` and `hashclr()`
Many of our hash functions have two variants, one receiving a `struct
git_hash_algo` and one that derives it via `the_repository`. Adapt all
of those functions to always require the hash algorithm as input and
drop the variants that do not accept one.
As those functions are now independent of `the_repository`, we can move
them from "hash.h" to "hash-ll.h".
Note that both in this and subsequent commits in this series we always
just pass `the_repository->hash_algo` as input even if it is obvious
that there is a repository in the context that we should be using the
hash from instead. This is done to be on the safe side and not introduce
any regressions. All callsites should eventually be amended to use a
repo passed via parameters, but this is outside the scope of this patch
series.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hash: drop (mostly) unused `is_empty_{blob,tree}_sha1()` functions
The functions `is_empty_{blob,tree}_sha1()` are mostly unused, except
for a single callsite in "read-cache.c". Most callsites have long since
been converted to use the equivalents that accept a `struct object_id`
instead of a string.
Adapt the remaining callsite and drop those functions.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:42:03 +0000 (06:42 -0400)]
remote: drop checks for zero-url case
Now that the previous commit removed the possibility that a "struct
remote" will ever have zero url fields, we can drop a number of
redundant checks and untriggerable code paths.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:37:11 +0000 (06:37 -0400)]
remote: always require at least one url in a remote
When we return a struct from remote_get(), the result _almost_ always
has at least one url. In remotes_remote_get_1(), we do this:
if (name_given && !valid_remote(ret))
add_url_alias(remote_state, ret, name);
if (!valid_remote(ret))
return NULL;
So if the remote doesn't have a url, we give it one based on the name
(this is how unconfigured urls are used as remotes). And if that doesn't
work, we return NULL.
But there's a catch: valid_remote() checks that we have at least one url
_unless_ the remote.*.vcs field is set. This comes from c578f51d52 (Add
a config option for remotes to specify a foreign vcs, 2009-11-18), and
the whole idea was to support remote helpers that don't have their own
url.
However, that mode has been broken since 25d5cc488a (Pass unknown
protocols to external protocol handlers, 2009-12-09)! That commit
unconditionally looks at the url in get_helper(), causing a segfault
with something like:
git -c remote.foo.vcs=bar fetch foo
We could fix that now, of course. But given that it has been broken for
almost 15 years and nobody noticed, there's a better option. This weird
"there might not be a url" special case requires checks all over the
code base, and it's not clear if there are other similar segfaults
lurking. It would be nice if we could drop that special case.
So instead, let's let the "the remote name is the url" code kick in. If
you have "remote.foo.vcs", then your url (unless otherwise configured)
is "foo". This does have a visible effect compared to what 25d5cc488a
was trying to do. The idea back then is that for a remote without a url,
we'd run:
# only one command-line option!
git-remote-bar foo
whereas with our default url, now we'll run:
git-remote-bar foo foo
Again, in practice nobody can be relying on this because it has been
segfaulting for 15 years. We should consider just removing this "vcs"
config option entirely, but that would be a user-visible breakage. So by
fixing it this way, we can keep things working that have been working,
and simplify away one special case inside our code.
This fixes the segfault from 25d5cc488a (demonstrated by the test), and
we can build further cleanups on top.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:34:54 +0000 (06:34 -0400)]
t5801: test remote.*.vcs config
The usual way to trigger a remote helper is to use the "::" syntax from: 87422439d1 (Allow specifying the remote helper in the url, 2009-11-18).
Doing:
will run "git-remote-hg origin https://example.com/repo". Or you can
use the fallback handling from 25d5cc488a (Pass unknown protocols to
external protocol handlers, 2009-12-09):
git config remote.origin.url "foo://bar"
which will run "git-remote-foo origin foo://bar".
But there's a third way, from c578f51d52 (Add a config option for
remotes to specify a foreign vcs, 2009-11-18):
git config remote.origin.vcs foo
git config remote.origin.url bar
which will run "git-remote-foo origin bar". This is mostly redundant
with the other methods, except that it is supposed to allow you to run
without a URL at all. So:
git config remote.origin.vcs foo
would run "git-remote-foo origin" with no extra URL parameter (under the
assumption that the helper somehow knows how to access the remote repo).
However, this mode has been broken since 25d5cc488a, shortly after it
was added! That commit taught the transport code to always look at the
URL string to parse off the "foo::" bits, meaning it would always
segfault in the no-url case. You can see that with:
git -c remote.foo.vcs=bar fetch foo
Nobody seems to have noticed in the almost 15 years since, so presumably
it's not a well-used feature. And without that, arguably the whole
remote.*.vcs feature could be removed entirely, as it isn't offering
anything you couldn't do with the "helper::" syntax. But it _does_ work
if you have a URL, and it has been advertised in the documentation for
all that time. So we shouldn't just remove it without warning.
Likewise, even if we were going to deprecate it, we should avoid
breaking it in the meantime. Since there are no tests for it at all,
let's add a few basic ones:
- this syntax doesn't work well with "git clone" (another point
against it versus "helper::"). But we can use "clone -c" to set up
the config manually, passing the URL as usual to clone. This does
work, though note that I had to use --no-local in the test to avoid
broken interactions between the local code and the helper. In the
real world this would be a non-issue, since the remote URL would
generally not also be a local Git repo!
- likewise, we should be able to set up the config manually and fetch
into a repository. This also works.
- we can simulate a vcs that has no URL support by stuffing the remote
path into another environment variable. This should work, but
doesn't (it hits the segfault mentioned above).
In the first two cases, I took the extra step of checking GIT_TRACE
output to confirm that we actually ran the helper (since the URL is a
valid Git repo, the clone/fetch would appear to work even if we
didn't use the helper at all!).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:31:31 +0000 (06:31 -0400)]
t5801: make remote-testgit GIT_DIR setup more robust
Our tests use a fake helper that just imports from an existing Git
repository. We're fed the path to that repo on the command line, and
derive the GIT_DIR by tacking on "/.git".
This is wrong if the path is a bare repository, but that's OK since this
is just a limited test. But it's also wrong if the transport code feeds
us the actual .git directory itself (i.e., we expect "/path/to/repo" but
it gives us "/path/to/repo/.git"). None of the current tests do that,
but let's future-proof ourselves against adding a test that does.
We can instead ask "rev-parse" to set our GIT_DIR. Note that we have to
first unset other git variables from our environment. Coming into this
script, we'll have GIT_DIR set to the fetching repository, and we need
to "switch" to the remote one.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:31:22 +0000 (06:31 -0400)]
remote: allow resetting url list
Because remote.*.url is treated as a multi-valued key, there is no way
to override previous config. So for example if you have
remote.origin.url set to some wrong value, doing:
git -c remote.origin.url=right fetch
would not work. It would append "right" to the list, which means we'd
still fetch from "wrong" (since subsequent values are used only as push
urls).
Let's provide a mechanism to reset the list, like we do for other
multi-valued keys (e.g., credential.helper, http.extraheaders, and
merge.suppressDest all use this "empty string means reset" pattern).
Reported-by: Mathew George <mathewegeorge@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:30:05 +0000 (06:30 -0400)]
config: document remote.*.url/pushurl interaction
The documentation for these keys gives a very terse definition and
points you to the fetch/push manpages. But from reading those pages it
was not at all obvious to me that:
- these are keys that can be defined multiple times with meaningful
behavior (especially remote.*.url)
- the way that pushurl overrides url (the git-push page does mention
that "pushurl defaults to url", but it is not immediately clear what
a multi-valued url would do in that situation).
Let's try to summarize the current behavior.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:29:09 +0000 (06:29 -0400)]
remote: simplify url/pushurl selection
When we want to know the push urls for a remote, there is some simple
logic:
- if the user configured any remote.*.pushurl keys, then those make
the complete set of push urls
- otherwise we push to all urls in remote.*.url
Many spots implement this with a level of indirection, assigning to a
local url/url_nr pair. But since both arrays are now strvecs, we can
just use a pointer to select the appropriate strvec, shortening the code
a bit.
Even though this is now a one-liner, since it is application logic that
is present in so many places, it's worth abstracting a helper function.
In fact, we already have such a function, but it's local to
builtin/push.c. So we'll just make it available everywhere via remote.h.
There are two spots to pay special attention to here:
1. in builtin/remote.c's get_url(), we are selecting first based on
push_mode and then falling back to "url" when we're in push_mode
but no pushurl is defined. The updated code makes that much more
clear, compared to the original which had an "else" fall-through.
2. likewise in that file's set_url(), we _only_ respect push_mode,
sine the point is that we are adding to pushurl in that case
(whether it is empty or not). And thus it does not use our helper
function.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:28:01 +0000 (06:28 -0400)]
remote: use strvecs to store remote url/pushurl
Now that the url/pushurl fields of "struct remote" own their strings, we
can switch from bare arrays to strvecs. This has a few advantages:
- push/clear are now one-liners
- likewise the free+assigns in alias_all_urls() can use
strvec_replace()
- we now use size_t for storage, avoiding possible overflow
- this will enable some further cleanups in future patches
There's quite a bit of fallout in the code that reads these fields, as
it tends to access these arrays directly. But it's mostly a mechanical
replacement of "url_nr" with "url.nr", and "url[i]" with "url.v[i]",
with a few variations (e.g. "*url" could become "*url.v", but I used
"url.v[0]" for consistency).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:27:22 +0000 (06:27 -0400)]
remote: transfer ownership of memory in add_url(), etc
Many of the internal functions in remote.c take const strings and store
them forever in instances of "struct remote". Since the functions are
internal and callers are aware of the convention, this seems to mostly
work and not cause leaks. But there are some issues:
- it's impossible to clear any of the arrays, because the data
dependencies between them are too muddled (if you free() a string,
it might also be referenced from another array, causing a
user-after-free; but if you don't, that might be the last reference,
causing a leak).
This is mostly of interest for further refactoring and features, but
there's at least one spot that's already a problem. In alias_all_urls(),
we replace elements of remote->url and remote->pushurl with their
aliased forms, dropping references to the original.
- sometimes strings from outside callers make their way in. For
example, calling remote_get("foo") when there is no configured "foo"
remote will create a remote struct with the single url "foo". But
we'll do so by holding on to the string passed to remote_get()
forever.
In practice I think this works out because we'd usually pass in a
string that lasts the length of the program (a string literal, or
argv reference, or other data structure allocated in the main
function). But it's a rather subtle requirement.
Instead, let's have remote->url and remote->pushurl own their string
memory. They'll copy the const strings that are passed in, and callers
can stop making their own copies. Likewise, when we overwrite an entry,
we can free the memory it points to, fixing the leak mentioned above.
We'll leave the struct members as "const" since they are visible to the
outside world, and shouldn't usually be touched. This requires casting
on free() for now, but we'll clean that further in a future patch.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:26:16 +0000 (06:26 -0400)]
remote: refactor alias_url() memory ownership
The alias_url() function may return either a newly allocated string
(which the caller must take ownership of), or the original const "url"
parameter that was passed in.
This often works OK because callers are generally passing in a "url"
that they expect to retain ownership of anyway. So whether we got back
the original or a new string, we're always interested in storing it
forever. But I suspect there are some possible leaks here (e.g.,
add_url_alias() may end up discarding the original "url").
Whether there are active leaks or not, this is a confusing setup that
makes further refactoring of memory ownership harder. So instead of
returning the original string, return NULL, forcing callers to decide
what to do with it explicitly. We can then build further cleanups on top
of that.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 14 Jun 2024 10:25:25 +0000 (06:25 -0400)]
archive: fix check for missing url
Running "git archive --remote" checks that we have at least one url for
the remote. It does so by looking at remote.url[0], but that won't work;
if we have no url at all, then remote.url will be NULL, and we'll
segfault.
Check url_nr instead, which is a more direct way of asking what we
want.
You can trigger the segfault like this:
git -c remote.foo.vcs=bar archive --remote=foo
but I didn't bother adding a test. This is the tip of the iceberg for
no-url remotes, and a later patch will improve that situation. I just
wanted to clean up this bug so it didn't make further refactoring of
this code more confusing.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
BreakingChanges: document that we do not plan to deprecate git-checkout
The git-checkout(1) command is seen by many as hard to understand
because it connects two somewhat unrelated features: switching between
branches and restoring worktree files from arbitrary revisions. In 2019,
we thus implemented two new commands git-switch(1) and git-restore(1) to
split out these separate concerns into standalone functions.
This "replacement" of git-checkout(1) has repeatedly triggered concerns
for our userbase that git-checkout(1) will eventually go away. This is
not the case though: the use of that command is still widespread, and it
is not expected that this will change anytime soon.
Document that all three commands will remain for the foreseeable future.
This decision may be revisited in case we ever figure out that most
everyone has given up on any of the commands.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The grafting mechanism for objects has been deprecated in e650d0643b
(docs: mark info/grafts as outdated, 2014-03-05), which is more than a
decade ago. The mechanism can lead to hard-to-debug issues and has a
superior replacement with replace refs.
Follow through with the deprecation and mark grafts for removal in Git
3.0.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
BreakingChanges: document upcoming change from "sha1" to "sha256"
Starting with 8e42eb0e9a (doc: sha256 is no longer experimental,
2023-07-31), the "sha256" object format is no longer considered to be
experimental. Furthermore, the SHA-1 hash function is actively
recommended against by for example NIST and FIPS 140-2, and attacks
against it are becoming more practical both due to new weaknesses
(SHAppening, SHAttered, Shambles) and due to the ever-increasing
computing power. It is only a matter of time before it can be considered
to be broken completely.
Let's plan for this event by being active instead of waiting for it to
happend and announce that the default object format is going to change
from "sha1" to "sha256" with Git 3.0.
All major Git implementations (libgit2, JGit, go-git) support the
"sha256" object format and are thus prepared for this change. The most
important missing piece in the puzzle is support in forges. But while
GitLab recently gained experimental support for the "sha256" object
format though, to the best of my knowledge GitHub doesn't support it
yet. Ideally, announcing this upcoming change will encourage forges to
start building that support.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
docs: introduce document to announce breaking changes
Over time, Git has grown quite a lot. With this evolution, many ideas
that were sensible at the time they were introduced are not anymore and
are thus considered to be deprecated. And while some deprecations may be
noted in manpages, most of them are actually deprecated in the "hive
mind" of the Git community, only.
Introduce a new document that tracks such breaking changes, but also
deprecations which we are not willing to go through with, to address
this issue. This document serves multiple purposes:
- It is a way to facilitate discussion around proposed deprecations.
- It allows users to learn about deprecations and speak up in case
they have good reasons why a certain feature should not be
deprecated.
- It states intent and documents where the Git project wants to go,
both in the case where we want to deprecate, but also in the case
where we don't want to deprecate a specific feature.
The document is _not_ intended to cast every single discussion into
stone. It is supposed to be a living document that may change over time
when there are good reasons for it to change.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Thu, 13 Jun 2024 16:39:08 +0000 (09:39 -0700)]
Merge branch 'ps/ref-storage-migration' into ps/use-the-repository
* ps/ref-storage-migration:
builtin/refs: new command to migrate ref storage formats
refs: implement logic to migrate between ref storage formats
refs: implement removal of ref storages
worktree: don't store main worktree twice
reftable: inline `merged_table_release()`
refs/files: fix NULL pointer deref when releasing ref store
refs/files: extract function to iterate through root refs
refs/files: refactor `add_pseudoref_and_head_entries()`
refs: allow to skip creation of reflog entries
refs: pass storage format to `ref_store_init()` explicitly
refs: convert ref storage format to an enum
setup: unset ref storage when reinitializing repository version
Derrick Stolee [Tue, 11 Jun 2024 15:09:15 +0000 (15:09 +0000)]
commit-graph: increment progress indicator
This fixes a bug that was introduced by 368d19b0b7 (commit-graph:
refactor compute_topological_levels(), 2023-03-20): Previously, the
progress indicator was updated from `i + 1` where `i` is the loop
variable of the enclosing `for` loop. After this patch, the update used
`info->progress_cnt + 1` instead, however, unlike `i`, the
`progress_cnt` attribute was not incremented. Let's increment it.
Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
[jc: squashed in a test update from Patrick Steinhardt] Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
