Elijah Newren [Sat, 19 Feb 2022 16:44:45 +0000 (16:44 +0000)]
sparse-checkout: reject arguments in cone-mode that look like patterns
In sparse-checkout add/set under cone mode, the arguments passed are
supposed to be directories rather than gitignore-style patterns.
However, given the amount of effort spent in the manual discussing
patterns, it is easy for users to assume they need to pass patterns such
as
/foo/*
or
!/bar/*/
or perhaps they really do ignore the directory rule and specify a
random gitignore-style pattern like
*.c
To help catch such mistakes, throw an error if any of the positional
arguments:
* starts with any of '/!'
* contains any of '*?[]'
Inform users they can pass --skip-checks if they have a directory that
really does have such special characters in its name. (We exclude '\'
because of sparse-checkout's special handling of backslashes; see
the MINGW test in t1091.46.)
Reviewed-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Sat, 19 Feb 2022 16:44:44 +0000 (16:44 +0000)]
sparse-checkout: error or warn when given individual files
The set and add subcommands accept multiple positional arguments.
The meaning of these arguments differs slightly in the two modes:
Cone mode only accepts directories. If given a file, it would
previously treat it as a directory, causing not just the file itself to
be included but all sibling files as well -- likely against users'
expectations. Throw an error if the specified path is a file in the
index. Provide a --skip-checks argument to allow users to override
(e.g. for the case when the given path IS a directory on another
branch).
Non-cone mode accepts general gitignore patterns. There are many
reasons to avoid this mode, but one possible reason to use it instead of
cone mode: to be able to select individual files within a directory.
However, if a file is passed to set/add in non-cone mode, you won't be
selecting a single file, you'll be selecting a file with the same name
in any directory. Thus users will likely want to prefix any paths they
specify with a leading '/' character; warn users if the patterns they
specify exactly name a file because it means they are likely missing
such a leading slash.
Reviewed-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Sat, 19 Feb 2022 16:44:43 +0000 (16:44 +0000)]
sparse-checkout: pay attention to prefix for {set, add}
In cone mode, non-option arguments to set & add are clearly paths, and
as such, we should pay attention to prefix.
In non-cone mode, it is not clear that folks intend to provide paths
since the inputs are gitignore-style patterns. Paying attention to
prefix would prevent folks from doing things like
git sparse-checkout add /.gitattributes
git sparse-checkout add '/toplevel-dir/*'
In fact, the former will result in
fatal: '/.gitattributes' is outside repository...
while the later will result in
fatal: Invalid path '/toplevel-dir': No such file or directory
despite the fact that both are valid gitignore-style patterns that would
select real files if added to the sparse-checkout file. This might lead
people to just use the path without the leading slash, potentially
resulting in them grabbing files with the same name throughout the
directory hierarchy contrary to their expectations. See also [1] and
[2]. Adding prefix seems to just be fraught with error; so for now
simply throw an error in non-cone mode when sparse-checkout set/add are
run from a subdirectory.
Elijah Newren [Sat, 19 Feb 2022 16:44:42 +0000 (16:44 +0000)]
sparse-checkout: correctly set non-cone mode when expected
commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
sparse-checkout mode", 2021-12-14) made the `set` command able to
initialize sparse-checkout mode, but it also had to function when
sparse-checkout mode was already setup and the user just wanted to
change the sparsity paths. So, if the user passed --cone or --no-cone,
then we should override the current setting, but if they didn't pass
either, we should use whatever the current cone mode setting is.
Unfortunately, there was a small error in the logic in that it would not
set the in-memory cone mode value (core_sparse_checkout_one) when
--no-cone was specified, but since it did set the config setting on
disk, any subsequent git invocation would correctly get non-cone mode.
As such, the error did not previously matter. However, a subsequent
commit will add some logic that depends on core_sparse_checkout_cone
being set to the correct mode, so make sure it is set consistently with
the config values we will be writing to disk.
Reviewed-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Sat, 19 Feb 2022 16:44:41 +0000 (16:44 +0000)]
sparse-checkout: correct reapply's handling of options
Commit 4e256731d6 ("sparse-checkout: enable reapply to take
--[no-]{cone,sparse-index}", 2021-12-14) made it so that reapply could
take additional options but added no tests. Tests would have shown that
the feature doesn't work because the initial values are set AFTER
parsing the command line options instead of before. Add a test and set
the initial value at the appropriate time.
Reviewed-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Sat, 12 Feb 2022 00:56:01 +0000 (16:56 -0800)]
Merge branch 'tg/fetch-prune-exit-code-fix'
When "git fetch --prune" failed to prune the refs it wanted to
prune, the command issued error messages but exited with exit
status 0, which has been corrected.
* tg/fetch-prune-exit-code-fix:
fetch --prune: exit with error if pruning fails
Junio C Hamano [Sat, 12 Feb 2022 00:55:58 +0000 (16:55 -0800)]
Merge branch 'jc/doc-log-messages'
Update the contributor-facing documents on proposed log messages.
* jc/doc-log-messages:
SubmittingPatches: explain why we care about log messages
CodingGuidelines: hint why we value clearly written log messages
SubmittingPatches: write problem statement in the log in the present tense
* ab/no-errno-from-resolve-ref-unsafe:
refs API: remove "failure_errno" from refs_resolve_ref_unsafe()
sequencer: don't use die_errno() on refs_resolve_ref_unsafe() failure
Junio C Hamano [Wed, 9 Feb 2022 22:21:01 +0000 (14:21 -0800)]
Merge branch 'js/sparse-vs-split-index'
Mark in various places in the code that the sparse index and the
split index features are mutually incompatible.
* js/sparse-vs-split-index:
split-index: it really is incompatible with the sparse index
t1091: disable split index
sparse-index: sparse index is disallowed when split index is active
Junio C Hamano [Wed, 9 Feb 2022 22:21:01 +0000 (14:21 -0800)]
Merge branch 'jt/clone-not-quite-empty'
Cloning from a repository that does not yet have any branches or
tags but has other refs resulted in a "remote transport reported
error", which has been corrected.
* jt/clone-not-quite-empty:
clone: support unusual remote ref configurations
Junio C Hamano [Wed, 9 Feb 2022 22:21:00 +0000 (14:21 -0800)]
Merge branch 'jt/sparse-checkout-leading-dir-fix'
"git sparse-checkout init" failed to write into $GIT_DIR/info
directory when the repository was created without one, which has
been corrected to auto-create it.
* jt/sparse-checkout-leading-dir-fix:
sparse-checkout: create leading directory
Junio C Hamano [Wed, 9 Feb 2022 22:21:00 +0000 (14:21 -0800)]
Merge branch 'ab/config-based-hooks-2'
More "config-based hooks".
* ab/config-based-hooks-2:
run-command: remove old run_hook_{le,ve}() hook API
receive-pack: convert push-to-checkout hook to hook.h
read-cache: convert post-index-change to use hook.h
commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
git-p4: use 'git hook' to run hooks
send-email: use 'git hook run' for 'sendemail-validate'
git hook run: add an --ignore-missing flag
hooks: convert worktree 'post-checkout' hook to hook library
hooks: convert non-worktree 'post-checkout' hook to hook library
merge: convert post-merge to use hook.h
am: convert applypatch-msg to use hook.h
rebase: convert pre-rebase to use hook.h
hook API: add a run_hooks_l() wrapper
am: convert {pre,post}-applypatch to use hook.h
gc: use hook library for pre-auto-gc hook
hook API: add a run_hooks() wrapper
hook: add 'run' subcommand
"git fetch --negotiate-only" is an internal command used by "git
push" to figure out which part of our history is missing from the
other side. It should never recurse into submodules even when
fetch.recursesubmodules configuration variable is set, nor it
should trigger "gc". The code has been tightened up to ensure it
only does common ancestry discovery and nothing else.
* gc/fetch-negotiate-only-early-return:
fetch: help translators by reusing the same message template
fetch --negotiate-only: do not update submodules
fetch: skip tasks related to fetching objects
fetch: use goto cleanup in cmd_fetch()
Junio C Hamano [Wed, 9 Feb 2022 22:20:59 +0000 (14:20 -0800)]
Merge branch 'tl/doc-cli-options-first'
We explain that revs come first before the pathspec among command
line arguments, but did not spell out that dashed options come
before other args, which has been corrected.
* tl/doc-cli-options-first:
git-cli.txt: clarify "options first and then args"
The conditional inclusion mechanism of configuration files using
"[includeIf <condition>]" learns to base its decision on the
URL of the remote repository the repository interacts with.
* jt/conditional-config-on-remote-url:
config: include file if remote URL matches a glob
config: make git_config_include() static
Junio C Hamano [Sat, 5 Feb 2022 17:42:32 +0000 (09:42 -0800)]
Merge branch 'ms/update-index-racy'
"git update-index --refresh" has been taught to deal better with
racy timestamps (just like "git status" already does).
* ms/update-index-racy:
update-index: refresh should rewrite index in case of racy timestamps
t7508: add tests capturing racy timestamp handling
t7508: fix bogus mtime verification
test-lib: introduce API for verifying file mtime
Junio C Hamano [Sat, 5 Feb 2022 17:42:31 +0000 (09:42 -0800)]
Merge branch 'ab/cat-file'
Assorted updates to "git cat-file", especially "-h".
* ab/cat-file:
cat-file: s/_/-/ in typo'd usage_msg_optf() message
cat-file: don't whitespace-pad "(...)" in SYNOPSIS and usage output
cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
cat-file: correct and improve usage information
cat-file: fix remaining usage bugs
cat-file: make --batch-all-objects a CMDMODE
cat-file: move "usage" variable to cmd_cat_file()
cat-file docs: fix SYNOPSIS and "-h" output
parse-options API: add a usage_msg_optf()
cat-file tests: test messaging on bad objects/paths
cat-file tests: test bad usage
Junio C Hamano [Sat, 5 Feb 2022 17:42:31 +0000 (09:42 -0800)]
Merge branch 'jc/qsort-s-alignment-fix'
Fix a hand-rolled alloca() imitation that may have violated
alignment requirement of data being sorted in compatibility
implementation of qsort_s() and stable qsort().
* jc/qsort-s-alignment-fix:
stable-qsort: avoid using potentially unaligned access
compat/qsort_s.c: avoid using potentially unaligned access
Junio C Hamano [Sat, 5 Feb 2022 17:42:30 +0000 (09:42 -0800)]
Merge branch 'rs/apply-symlinks-use-strset'
"git apply" (ab)used the util pointer of the string-list to keep
track of how each symbolic link needs to be handled, which has been
simplified by using strset.
* rs/apply-symlinks-use-strset:
apply: use strsets to track symlinks
Junio C Hamano [Sat, 5 Feb 2022 17:42:30 +0000 (09:42 -0800)]
Merge branch 'rs/grep-expr-cleanup'
Code clean-up.
* rs/grep-expr-cleanup:
grep: use grep_and_expr() in compile_pattern_and()
grep: extract grep_binexp() from grep_or_expr()
grep: use grep_not_expr() in compile_pattern_not()
grep: use grep_or_expr() in compile_pattern_or()
* jh/p4-spawning-external-commands-cleanup:
git-p4: don't print shell commands as python lists
git-p4: pass command arguments as lists instead of using shell
git-p4: don't select shell mode using the type of the command argument
Junio C Hamano [Sat, 5 Feb 2022 17:42:28 +0000 (09:42 -0800)]
Merge branch 'pb/pull-rebase-autostash-fix'
"git pull --rebase" ignored the rebase.autostash configuration
variable when the remote history is a descendant of our history,
which has been corrected.
* pb/pull-rebase-autostash-fix:
pull --rebase: honor rebase.autostash when fast-forwarding
Shaoxuan Yuan [Wed, 2 Feb 2022 07:28:44 +0000 (15:28 +0800)]
builtin/diff.c: fix "git-diff" usage string typo
Remove mistaken right square brackets from "git-diff"
usage string. Make the usage string conform to "git-diff"
documentation (Documentation/git-diff.txt).
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Thomas Gummerer [Mon, 31 Jan 2022 13:30:47 +0000 (13:30 +0000)]
fetch --prune: exit with error if pruning fails
When pruning refs fails, we print an error to stderr, but still
exit 0 from 'git fetch'. Since this is a genuine error, fetch
should be exiting with some non-zero exit code. Make it so.
The --prune option was introduced in f360d844de ("builtin-fetch: add
--prune option", 2009-11-10). Unfortunately it's unclear from that
commit whether ignoring the exit code was an oversight or
intentional, but it feels like an oversight.
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Robert Coup [Fri, 28 Jan 2022 14:36:02 +0000 (14:36 +0000)]
fetch: fix negotiate-only error message
The error message when invoking a negotiate-only fetch without providing
any tips incorrectly refers to a --negotiate-tip=* argument. Fix this to
use the actual argument, --negotiation-tip=*.
Signed-off-by: Robert Coup <robert@coup.net.nz> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Fri, 28 Jan 2022 01:58:18 +0000 (01:58 +0000)]
sparse-checkout: fix a couple minor memory leaks
These were introduced in commit 55dfcf9591 ("sparse-checkout: clear
tracked sparse dirs", 2021-09-08) and missed in my review at the time.
Plug the leaks.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Thu, 27 Jan 2022 19:02:57 +0000 (11:02 -0800)]
SubmittingPatches: write problem statement in the log in the present tense
We give a guidance for proposed log message to write problem
statement first, followed by the reasoning behind, and recipe for,
the solution. Clarify that we describe the situation _before_ the
proposed patch is applied in the present tense (not in the past
tense e.g. "we used to do X, but thanks to this commit we now do Y")
for consistency.
refs API: remove "failure_errno" from refs_resolve_ref_unsafe()
Remove the now-unused "failure_errno" parameter from the
refs_resolve_ref_unsafe() signature. In my recent 96f6623ada0 (Merge
branch 'ab/refs-errno-cleanup', 2021-11-29) series we made all of its
callers explicitly request the errno via an output parameter.
As that series shows all but one caller ended up passing in a
boilerplate "ignore_errno", since they only cared about whether the
return value was NULL or not, i.e. if the ref could be resolved.
There was one small issue with that series fixed with a follow-up in 31e39123695 (Merge branch 'ab/refs-errno-cleanup', 2022-01-14) a small
bug in that series was fixed.
After those two there was one caller left in sequencer.c that used the
"failure_errno', but as of the preceding commit it uses a boilerplate
"ignore_errno" instead.
This leaves the public refs API without any use of "failure_errno" at
all. We could still do with a bit of cleanup and generalization
between refs.c and refs/files-backend.c before the "reftable"
integration lands, but that's all internal to the reference code
itself.
So let's remove this output parameter. Not only isn't it used now, but
it's unlikely that we'll want it again in the future. We'd like to
slowly move the refs API to a more file-backend independent way of
communicating error codes, having it use a "failure_errno" was only
the first step in that direction. If this or any other function needs
to communicate what specifically is wrong with the requested "refname"
it'll be better to have the function set some output enum of
well-defined error states than piggy-backend on "errno".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
sequencer: don't use die_errno() on refs_resolve_ref_unsafe() failure
Change code that was faithfully migrated to the new "resolve_errno"
API in ed90f04155d (refs API: make resolve_ref_unsafe() not set errno,
2021-10-16) to stop caring about the errno at all.
When we fail to resolve "HEAD" after the sequencer runs it doesn't
really help to say what the "errno" value is, since the fake backend
errno may or may not reflect anything real about the state of the
".git/HEAD". With the upcoming reftable backend this fakery will
become even more pronounced.
So let's just die() instead of die_errno() here. This will also help
simplify the refs_resolve_ref_unsafe() API. This was the only user of
it that wasn't ignoring the "failure_errno" output parameter.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 26 Jan 2022 01:43:45 +0000 (01:43 +0000)]
sequencer, stash: fix running from worktree subdir
In commits bc3ae46b42 ("rebase: do not attempt to remove
startup_info->original_cwd", 2021-12-09) and 0fce211ccc ("stash: do not
attempt to remove startup_info->original_cwd", 2021-12-09), we wanted to
allow the subprocess to know which directory the parent process was
running from, so that the subprocess could protect it. However...
When run from a non-main worktree, setup_git_directory() will note
that the discovered git directory
(/PATH/TO/.git/worktree/non-main-worktree) does not match
DEFAULT_GIT_DIR_ENVIRONMENT (see setup_discovered_git_dir()), and
decide to set GIT_DIR in the environment. This matters because...
Whenever git is run with the GIT_DIR environment variable set, and
GIT_WORK_TREE not set, it presumes that '.' is the working tree. So...
This combination results in the subcommand being very confused about
the working tree. Fix it by also setting the GIT_WORK_TREE environment
variable along with setting cmd.dir.
A possibly more involved fix we could consider for later would be to
make setup.c set GIT_WORK_TREE whenever (a) it discovers both the git
directory and the working tree and (b) it decides to set GIT_DIR in the
environment. I did not attempt that here as such would be too big of a
change for a 2.35.1 release.
Test-case-by: Glen Choo <chooglen@google.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan [Mon, 24 Jan 2022 18:09:09 +0000 (10:09 -0800)]
clone: support unusual remote ref configurations
When cloning a branchless and tagless but not refless remote using
protocol v0 or v1, Git calls transport_fetch_refs() with an empty ref
list. This makes the clone fail with the message "remote transport
reported error".
Git should have refrained from calling transport_fetch_refs(), just like
it does in the case that the remote is refless. Therefore, teach Git to
do this.
In protocol v2, this does not happen because the client passes
ref-prefix arguments that filter out non-branches and non-tags in the
ref advertisement, making the remote appear empty.
Note that this bug concerns logic in builtin/clone.c and only affects
cloning, not fetching.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jessica Clarke [Sun, 23 Jan 2022 20:33:47 +0000 (20:33 +0000)]
mem-pool: don't assume uintmax_t is aligned enough for all types
mem_pool_alloc uses sizeof(uintmax_t) as a proxy for what should be
_Alignof(max_align_t) in C11. On most architectures this is sufficient
(though on m68k it is in fact overly strict, since the de-facto ABI,
which differs from the specified System V ABI, has the maximum alignment
of all types as 2 bytes), but on CHERI, and thus Arm's Morello
prototype, it is insufficient for any type that stores a pointer, which
must be aligned to 128 bits (on 64-bit architectures extended with
CHERI), whilst uintmax_t is a 64-bit integer.
Fix this by introducing our own approximation for max_align_t and a
means to compute _Alignof it without relying on C11. Currently this
union only contains uintmax_t and void *, but more types can be added as
needed.
Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Mon, 24 Jan 2022 17:14:46 +0000 (09:14 -0800)]
Merge branch 'ab/checkout-branch-info-leakfix'
We added an unrelated sanity checking that leads to a BUG() while
plugging a leak, which triggered in a repository with symrefs in
the local branch namespace that point at a ref outside. Partially
revert the change to avoid triggering the BUG().
* ab/checkout-branch-info-leakfix:
checkout: avoid BUG() when hitting a broken repository
split-index: it really is incompatible with the sparse index
... at least for now. So let's error out if we are even trying to
initialize the split index when the index is sparse, or when trying to
write the split index extension for a sparse index.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 61feddcdf28 (tests: disable GIT_TEST_SPLIT_INDEX for sparse index
tests, 2021-08-26), it was already called out that the split index
feature is incompatible with the sparse index feature, and its commit
message wondered aloud whether more checks would be required to ensure
that the split index and sparse index features aren't enabled at the
same time.
We are about to introduce such additional checks, and indeed, t1091
would utterly fail with them. Therefore, let's preemptively disable the
split index for the entirety of t1091.
This partially reverts above-mentioned patch because it covered only one
test case whereas we want to cover the entire test script.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
sparse-index: sparse index is disallowed when split index is active
In 6e773527b6b (sparse-index: convert from full to sparse, 2021-03-30),
we introduced initial support for a sparse index, and were careful to
avoid converting to a sparse index in the presence of a split index.
However, when we _just_ read a freshly-initialized index, it might not
contain a split index even if _writing_ it will add one by virtue of
being asked for via the `GIT_TEST_SPLIT_INDEX` variable.
We did not notice any problems with checking _only_ for `split_index`
(and not `GIT_TEST_SPLIT_INDEX`) right until both
`vd/sparse-sparsity-fix-on-read` _and_ `vd/sparse-reset` were merged.
Those two topics' interplay triggers a bug in conjunction with running
t1091.15 when `GIT_TEST_SPLIT_INDEX=true` in the following way:
`vd/sparse-sparsity-fix-on-read` ensures that the index is made sparse
right after reading, and `vd/sparse-reset` ensures that the index is
made non-sparse again unless running in the `--soft` mode. Since the
split index feature is incompatible with the sparse index feature, we
see a symptom like this:
fatal: position for replacement 4 exceeds base index size 4
Let's fix this by avoiding the conversion to a sparse index when
`GIT_TEST_SPLIT_INDEX=true`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Sat, 22 Jan 2022 00:58:30 +0000 (16:58 -0800)]
checkout: avoid BUG() when hitting a broken repository
When 9081a421 (checkout: fix "branch info" memory leaks, 2021-11-16)
cleaned up existing memory leaks, we added an unrelated sanity check
to ensure that a local branch is truly local and not a symref to
elsewhere that dies with BUG() otherwise. This was misguided in two
ways. First of all, such a tightening did not belong to a leak-fix
patch. And the condition it detected was *not* a bug in our program
but a problem in user data, where warning() or die() would have been
more appropriate.
As the condition is not fatal (the result of computing the local
branch name in the code that is involved in the faulty check is only
used as a textual label for the commit), let's revert the code to
the original state, i.e. strip "refs/heads/" to compute the local
branch name if possible, and otherwise leave it NULL. The consumer
of the information in merge_working_tree() is prepared to see NULL
in there and act accordingly.
Elijah Newren [Thu, 20 Jan 2022 07:47:15 +0000 (07:47 +0000)]
merge: fix memory leaks in cmd_merge()
There were two commit_lists created in cmd_merge() that were only
conditionally free()'d. Add a quick conditional call to
free_commit_list() for each of them at the end of the function.
Testing this commit against t6404 under valgrind shows that this patch
fixes the following two leaks:
16 bytes in 1 blocks are definitely lost in loss record 16 of 126
at 0x484086F: malloc (vg_replace_malloc.c:380)
by 0x69FFEB: do_xmalloc (wrapper.c:41)
by 0x6A0073: xmalloc (wrapper.c:62)
by 0x52A72D: commit_list_insert (commit.c:556)
by 0x47FC93: reduce_parents (merge.c:1114)
by 0x4801EE: collect_parents (merge.c:1214)
by 0x480B56: cmd_merge (merge.c:1465)
by 0x40686E: run_builtin (git.c:464)
by 0x406C51: handle_builtin (git.c:716)
by 0x406E96: run_argv (git.c:783)
by 0x40730A: cmd_main (git.c:914)
by 0x4E7DFA: main (common-main.c:56)
8 (16 direct, 32 indirect) bytes in 1 blocks are definitely lost in \
loss record 61 of 126
at 0x484086F: malloc (vg_replace_malloc.c:380)
by 0x69FFEB: do_xmalloc (wrapper.c:41)
by 0x6A0073: xmalloc (wrapper.c:62)
by 0x52A72D: commit_list_insert (commit.c:556)
by 0x52A8F2: commit_list_insert_by_date (commit.c:620)
by 0x5270AC: get_merge_bases_many_0 (commit-reach.c:413)
by 0x52716C: repo_get_merge_bases (commit-reach.c:438)
by 0x480E5A: cmd_merge (merge.c:1520)
by 0x40686E: run_builtin (git.c:464)
by 0x406C51: handle_builtin (git.c:716)
by 0x406E96: run_argv (git.c:783)
by 0x40730A: cmd_main (git.c:914)
There are still 3 leaks in chdir_notify_register() after this, but
chdir_notify_register() has been brought up on the list before and folks
were not a fan of fixing those, so I'm not touching them.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Thu, 20 Jan 2022 07:47:14 +0000 (07:47 +0000)]
merge-ort: fix memory leak in merge_ort_internal()
The documentation for merge_incore_recursive(), modelled after
merge_recursive(), notes that
merge_bases will be consumed (emptied) so make a copy if you need it
However, in merge_ort_internal() (which merge_incore_recursive() calls),
it runs
merged_merge_bases = pop_commit(&merge_bases);
...
for (iter = merge_bases; iter; iter = iter->next) {
...
}
In other words, it only consumes the *first* entry of merge_bases, and
the rest it iterates through. If it iterated through all of them, the
caller could be responsible for free'ing the memory. If it consumed all
of them, the current documentation would be correct and the callers
would need to do nothing. The current middle ground makes it impossible
for callers to avoid memory leaks, since any attempt to use the
merge_bases it passes in would result in a use-after-free.
It turns out this part of the code was copied from merge-recursive.c,
which has had the same bug for 15.5 years. However, since we are trying
to keep merge-recursive.c stable as we sunset it, let's just fix the
leak in in merge_ort_internal() by having it actually consume all the
elements of the merge_bases commit_list.
Testing this commit against t6404 (the first testcase specifically
about recursive merges) under valgrind shows that this patch fixes
the following leak:
32 (16 direct, 16 indirect) bytes in 1 blocks are definitely lost \
in loss record 49 of 126
at 0x484086F: malloc (vg_replace_malloc.c:380)
by 0x69FFEB: do_xmalloc (wrapper.c:41)
by 0x6A0073: xmalloc (wrapper.c:62)
by 0x52A72D: commit_list_insert (commit.c:556)
by 0x47EC86: try_merge_strategy (merge.c:751)
by 0x48143B: cmd_merge (merge.c:1679)
by 0x40686E: run_builtin (git.c:464)
by 0x406C51: handle_builtin (git.c:716)
by 0x406E96: run_argv (git.c:783)
by 0x40730A: cmd_main (git.c:914)
by 0x4E7DFA: main (common-main.c:56)
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan [Fri, 21 Jan 2022 17:44:41 +0000 (09:44 -0800)]
sparse-checkout: create leading directory
When creating the sparse-checkout file, Git does not create the leading
directory, "$GIT_DIR/info", if it does not exist. This causes problems
if the repository does not have that directory. Therefore, ensure that
the leading directory is created.
This is the only "open" in builtin/sparse-checkout.c that does not have
a leading directory check. (The other one in write_patterns_and_update()
does.)
Note that the test needs to explicitly specify a template when running
"git init" because the default template used in the tests has the
"info/" directory included.
Helped-by: Jose Lopes <jabolopes@google.com> Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Thu, 20 Jan 2022 21:58:43 +0000 (13:58 -0800)]
fetch: help translators by reusing the same message template
Follow the example set by 12909b6b (i18n: turn "options are
incompatible" into "cannot be used together", 2022-01-05) and use
the same message string to reduce the need for translation.
Reported-by: Jiang Xin <worldhello.net@gmail.com> Helped-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Thu, 20 Jan 2022 12:35:54 +0000 (13:35 +0100)]
branch,checkout: fix --track documentation
Document that the accepted variants of the --track option are --track,
--track=direct, and --track=inherit. The equal sign in the latter two
cannot be replaced with whitespace; in general optional arguments need
to be attached firmly to their option.
Put "direct" consistently before "inherit", if only for the reasons
that the former is the default, explained first in the documentation,
and comes before the latter alphabetically.
Mention both modes in the short help so that readers don't have to look
them up in the full documentation. They are literal strings and thus
untranslatable. PARSE_OPT_LITERAL_ARGHELP is inferred due to the pipe
and parenthesis characters, so we don't have to provide that flag
explicitly.
Mention that -t has the same effect as --track and --track=direct.
There is no way to specify inherit mode using the short option, because
short options generally don't accept optional arguments.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Josh Steadmon [Fri, 14 Jan 2022 03:33:36 +0000 (19:33 -0800)]
test-lib: unset trace2 parent envvars
The trace2 subsystem can inherit certain information from parent
processes via environment variables; e.g., the parent command name and
session ID. This allows trace2 to note when a command is the child
process of another Git process, and to adjust various pieces of output
accordingly.
This behavior breaks certain tests that examine trace2 output when the
tests run as a child of another git process, such as in `git rebase -x
"make test"`.
While we could fix this by unsetting the relevant variables in the
affected tests (currently t0210, t0211, t0212, and t6421), this would
leave other tests vulnerable to similar breakage if new test cases are
added which inspect trace2 output. So fix this in general by unsetting
GIT_TRACE2_PARENT_NAME and GIT_TRACE2_PARENT_SID in test-lib.sh.
Reported-by: Emily Shaffer <emilyshaffer@google.com> Helped-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
getcwd(mingw): handle the case when there is no cwd
A recent upstream topic introduced checks for certain Git commands that
prevent them from deleting the current working directory, introducing
also a regression test that ensures that commands such as `git version`
_can_ run without a current working directory.
While technically not possible on Windows via the regular Win32 API, we
do run the regression tests in an MSYS2 Bash which uses a POSIX
emulation layer (the MSYS2/Cygwin runtime) where a really evil hack
_does_ allow to delete a directory even if it is the current working
directory.
Therefore, Git needs to be prepared for a missing working directory,
even on Windows.
This issue was not noticed in upstream Git because there was no caller
that tried to discover a Git directory with a deleted current working
directory in the test suite. But in the microsoft/git fork, we do want
to run `pre-command`/`post-command` hooks for every command, even for
`git version`, which means that we make precisely such a call. The bug
is not in that `pre-command`/`post-command` feature, though, but in
`mingw_getcwd()` and needs to be addressed there.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Wed, 19 Jan 2022 00:00:56 +0000 (16:00 -0800)]
fetch --negotiate-only: do not update submodules
`git fetch --negotiate-only` is an implementation detail of push
negotiation and, unlike most `git fetch` invocations, does not actually
update the main repository. Thus it should not update submodules even
if submodule recursion is enabled.
This is not just slow, it is wrong e.g. push negotiation with
"submodule.recurse=true" will cause submodules to be updated because it
invokes `git fetch --negotiate-only`.
Fix this by disabling submodule recursion if --negotiate-only was given.
Since this makes --negotiate-only and --recurse-submodules incompatible,
check for this invalid combination and die.
This does not use the "goto cleanup" introduced in the previous commit
because we want to recurse through submodules whenever a ref is fetched,
and this can happen without introducing new objects.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Wed, 19 Jan 2022 00:00:55 +0000 (16:00 -0800)]
fetch: skip tasks related to fetching objects
cmd_fetch() does the following with the assumption that objects are
fetched:
* Run gc
* Write commit graphs (if enabled by fetch.writeCommitGraph=true)
However, neither of these tasks makes sense if objects are not fetched
e.g. `git fetch --negotiate-only` never fetches objects.
Speed up cmd_fetch() by bailing out early if we know for certain that
objects will not be fetched. cmd_fetch() can bail out early whenever
objects are not fetched, but for now this only considers
--negotiate-only.
The same optimization does not apply to `git fetch --dry-run` because
that actually fetches objects; the dry run refers to not updating refs.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Wed, 19 Jan 2022 00:00:54 +0000 (16:00 -0800)]
fetch: use goto cleanup in cmd_fetch()
Replace an early return with 'goto cleanup' in cmd_fetch() so that the
string_list is always cleared (the string_list_clear() call is purely
cleanup; the string_list is not reused). This makes cleanup consistent
so that a subsequent commit can use 'goto cleanup' to bail out early.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Wed, 19 Jan 2022 00:02:23 +0000 (16:02 -0800)]
Merge branch 'jc/freebsd-without-c99-only-build'
FreeBSD 13.0 headers have unconditional dependency on C11 language
features, and adding -std=gnu99 to DEVELOPER_CFLAGS would just
break the developer build.
* jc/freebsd-without-c99-only-build:
Makefile: FreeBSD cannot do C99-or-below build
Josh Steadmon [Tue, 18 Jan 2022 20:49:46 +0000 (12:49 -0800)]
branch,checkout: fix --track usage strings
As Ævar pointed out in [1], the use of PARSE_OPT_LITERAL_ARGHELP with a
list of allowed parameters is not recommended. Both git-branch and
git-checkout were changed in d311566 (branch: add flags and config to
inherit tracking, 2021-12-20) to use this discouraged combination for
their --track flags.
Fix this by removing PARSE_OPT_LITERAL_ARGHELP, and changing the arghelp
to simply be "mode". Users may discover allowed values in the manual
pages.
Jonathan Tan [Tue, 18 Jan 2022 17:47:40 +0000 (09:47 -0800)]
config: include file if remote URL matches a glob
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
Junio C Hamano [Tue, 18 Jan 2022 17:47:39 +0000 (09:47 -0800)]
Makefile: FreeBSD cannot do C99-or-below build
In "make DEVELOPER=YesPlease" builds, we try to help developers to
catch as many potential issues as they can by using -Wall and
turning compilation warnings into errors. In the same spirit, we
recently started adding -std=gnu99 to their CFLAGS, so that they can
notice when they accidentally used language features beyond C99.
It however turns out that FreeBSD 13.0 mistakenly uses C11 extension
in its system header files regardless of what __STDC_VERSION__ says,
which means that the platform (unless we tweak their system headers)
cannot be used for this purpose.
It seems that -std=gnu99 is only added conditionally even in today's
config.mak.dev, so it is fine if we dropped -std=gnu99 from there.
Which means that developers on FreeBSD cannot participate in vetting
use of features beyond C99, but there are developers on other
platforms who will, so it's not too bad.
We might want a more "fundamental" fix to make the platform capable
of taking -std=gnu99, like working around the use of unconditional
C11 extension in its system header files by supplying a set of
"replacement" definitions in our header files. We chose not to
pursue such an approach for two reasons at this point:
(1) The fix belongs to the FreeBSD project, not this project, and
such an upstream fix may happen hopefully in a not-too-distant
future.
(2) Fixing such a bug in system header files and working it around
can lead to unexpected breakages (other parts of their system
header files may not be expecting to see and do not work well
with our "replacement" definitions). This close to the final
release of this cycle, we have no time for that.
Elijah Newren [Mon, 17 Jan 2022 18:25:55 +0000 (18:25 +0000)]
merge-ort: avoid assuming all renames detected
In commit 8b09a900a1 ("merge-ort: restart merge with cached renames to
reduce process entry cost", 2021-07-16), we noted that in the merge-ort
steps of
collect_merge_info()
detect_and_process_renames()
process_entries()
that process_entries() was expensive, and we could often make it cheaper
by changing this to
collect_merge_info()
detect_and_process_renames()
<cache all the renames, and restart>
collect_merge_info()
detect_and_process_renames()
process_entries()
because the second collect_merge_info() would be cheaper (we could avoid
traversing into some directories), the second
detect_and_process_renames() would be free since we had already detected
all renames, and then process_entries() has far fewer entries to handle.
However, this was built on the assumption that the first
detect_and_process_renames() actually detected all potential renames.
If someone has merge.renameLimit set to some small value, that
assumption is violated which manifests later with the following message:
Turn off this cache-renames-and-restart whenever we cannot detect all
renames, and add a testcase that would have caught this problem.
Reported-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Elijah Newren <newren@gmail.com> Tested-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
