completion: add a GIT_COMPLETION_SHOW_ALL_COMMANDS
Add a GIT_COMPLETION_SHOW_ALL_COMMANDS=1 configuration setting to go
with the existing GIT_COMPLETION_SHOW_ALL=1 added in c099f579b98 (completion: add GIT_COMPLETION_SHOW_ALL env var,
2020-08-19).
This will include plumbing commands such as "cat-file" in "git <TAB>"
and "git c<TAB>" completion. Without/with this I have 134 and 243
completion with git <TAB>, respectively.
It was already possible to do this by tweaking
GIT_TESTING_PORCELAIN_COMMAND_LIST= from the outside, that testing
variable was added in 84a97131065 (completion: let git provide the
completable command list, 2018-05-20). Doing this before loading
git-completion.bash worked:
But such testing variables are not meant to be used from the outside,
and we make no guarantees that those internal won't change. So let's
expose this as a dedicated configuration knob.
It would be better to teach --list-cmds=* a new category which would
include all of these groups, but that's a larger change that we can
leave for some other time.
completion tests: re-source git-completion.bash in a subshell
Change tests of git-completion.bash that re-source it to do so inside
a subshell. Re-sourcing it will clobber variables it sets, and in the
case of the "GIT_COMPLETION_SHOW_ALL=1" test added in ca2d62b7879 (parse-options: don't complete option aliases by default,
2021-07-16) change the behavior of the completion persistently.
Aside from the addition of "(" and ")" on new lines this is an
indentation-only change, only the "(" and ")" lines are changed under
"git diff -w".
So let's change that test, and for good measure do the same for the
three tests that precede it, which were added in 8b0eaa41f23 (completion: clear cached --options when sourcing the
completion script, 2018-03-22). The may not be wrong, but doing this
establishes a more reliable pattern for future tests, which might use
these as a template to copy.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Shaoxuan Yuan [Wed, 2 Feb 2022 06:43:00 +0000 (14:43 +0800)]
t/lib-read-tree-m-3way: indent with tabs
As Documentation/CodingGuidelines says, our shell scripts
(including tests) are to use HT for indentation, but this script
uses 4-column indent with SP. Fix this.
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Shaoxuan Yuan [Wed, 2 Feb 2022 06:42:59 +0000 (14:42 +0800)]
t/lib-read-tree-m-3way: modernize style
Many invocations of the test_expect_success command in this
file are written in old style where the command, an optional
prerequisite, and the test title are written on separate
lines, and the executable script string begins on its own
line, and these lines are pasted together with backslashes
as necessary.
An invocation of the test_expect_success command in modern
test scripts however writes the prerequisite and the title
on the same line as the test_expect_success command itself,
and ends the line with a single quote that begins the
executable script string.
Update the style for uniformity.
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
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>
Jerry Zhang [Wed, 2 Feb 2022 04:19:45 +0000 (20:19 -0800)]
patch-id: fix scan_hunk_header on diffs with 1 line of before/after
Normally diffs will contain a hunk header of the format
"@@ -2,2 +2,15 @@ code". However when there is only 1 line of
change, the unified diff format allows for the second comma
separated value to be omitted in either before or after
line counts.
This can produce hunk headers that look like
"@@ -2 +2,18 @@ code" or "@@ -2,2 +2 @@ code".
As a result, scan_hunk_header mistakenly returns the line
number as line count, which then results in unpredictable
parsing errors with the rest of the patch, including giving
multiple lines of output for a single commit.
Fix by explicitly setting line count to 1 when there is
no comma, and add a test.
apply.c contains this same logic except it is correct. A
worthwhile future project might be to unify these two diff
parsers so they both benefit from fixes.
Signed-off-by: Jerry Zhang <jerry@skydio.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jerry Zhang [Wed, 2 Feb 2022 04:20:15 +0000 (20:20 -0800)]
patch-id: fix antipatterns in tests
Clean up the tests for patch-id by moving file preparation
tasks inside the test body and redirecting files directly into
stdin instead of using 'cat'.
Signed-off-by: Jerry Zhang <jerry@skydio.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:37 +0000 (02:37 +0000)]
diff-merges: avoid history simplifications when diffing merges
Doing diffs for merges are special; they should typically avoid history
simplification. For example, with
git log --diff-merges=first-parent -- path
the default history simplification would remove merge commits from
consideration if the file "path" matched the second parent. That is
counter to what the user wants when looking for first-parent diffs.
Similar comments can be made for --diff-merges=separate (which diffs
against both parents) and --diff-merges=remerge (which diffs against a
remerge of the merge commit).
However, history simplification still makes sense if not doing diffing
merges, and it also makes sense for the combined and dense-combined
forms of diffing merges (because both of those are defined to only show
a diff when the merge result at the relevant paths differs from *both*
parents).
So, for separate, first-parent, and remerge styles of diff-merges, turn
off history simplification.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:36 +0000 (02:37 +0000)]
merge-ort: mark conflict/warning messages from inner merges as omittable
A recursive merge involves merging the merge bases of the two branches
being merged. Such an inner merge can itself generate conflict notices.
While such notices may be useful when initially trying to create a
merge, they seem to just be noise when investigating merges later with
--remerge-diff. (Especially when both sides of the outer merge resolved
the conflict the same way leading to no overall conflict.) Remove them.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:35 +0000 (02:37 +0000)]
show, log: include conflict/warning messages in --remerge-diff headers
Conflicts such as modify/delete, rename/rename, or file/directory are
not representable via content conflict markers, and the normal output
messages notifying users about these were dropped with --remerge-diff.
While we don't want these messages randomly shown before the commit
and diff headers, we do want them to still be shown; include them as
part of the diff headers instead.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:34 +0000 (02:37 +0000)]
diff: add ability to insert additional headers for paths
When additional headers are provided, we need to
* add diff_filepairs to diff_queued_diff for each paths in the
additional headers map which, unless that path is part of
another diff_filepair already found in diff_queued_diff
* format the headers (colorization, line_prefix for --graph)
* make sure the various codepaths that attempt to return early
if there are "no changes" take into account the headers that
need to be shown.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:33 +0000 (02:37 +0000)]
merge-ort: format messages slightly different for use in headers
When users run
git show --remerge-diff $MERGE_COMMIT
or
git log -p --remerge-diff ...
stdout is not an appropriate location to dump conflict messages, but we
do want to provide them to users. We will include them in the diff
headers instead...but for that to work, we need for any multiline
messages to replace newlines with both a newline and a space. Add a new
flag to signal when we want these messages modified in such a fashion,
and use it in path_msg() to modify these messages this way. Also, allow
a special prefix to be specified for these headers.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:32 +0000 (02:37 +0000)]
merge-ort: mark a few more conflict messages as omittable
path_msg() has the ability to mark messages as omittable, designed for
remerge-diff where we'll instead be showing conflict messages as diff
headers for a subsequent diff. While all these messages are very useful
when trying to create a merge initially, early use with the
--remerge-diff feature (the only user of this omittable conflict message
capability), suggests that the particular messages marked in this commit
are just noise when trying to see what changes users made to create a
merge commit. Mark them as omittable.
Note that there were already a few messages marked as omittable in
merge-ort when doing a remerge-diff, because the development of
--remerge-diff preceded the upstreaming of merge-ort and I was trying to
ensure merge-ort could handle all the necessary requirements. See
commit c5a6f65527 ("merge-ort: add modify/delete handling and delayed
output processing", 2020-12-03) for the initial details. For some
examples of already-marked-as-omittable messages, see either
"Auto-merging <path>" or some of the submodule update hints. This
commit just adds two more messages that should also be omittable.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:31 +0000 (02:37 +0000)]
merge-ort: capture and print ll-merge warnings in our preferred fashion
Instead of immediately printing ll-merge warnings to stderr, we save
them in our output strbuf. Besides allowing us to move these warnings
to a special file for --remerge-diff, this has two other benefits for
regular merges done by merge-ort:
* The deferral of messages ensures we can print all messages about
any given path together (merge-recursive was known to sometimes
intersperse messages about other paths, particularly when renames
were involved).
* The deferral of messages means we can avoid printing spurious
conflict messages when we just end up aborting due to local user
modifications in the way. (In contrast to merge-recursive.c which
prematurely checks for local modifications in the way via
unpack_trees() and gets the check wrong both in terms of false
positives and false negatives relative to renames, merge-ort does
not perform the local modifications in the way check until the
checkout() step after the full merge has been computed.)
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:30 +0000 (02:37 +0000)]
ll-merge: make callers responsible for showing warnings
Since some callers may want to send warning messages to somewhere other
than stdout/stderr, stop printing "warning: Cannot merge binary files"
from ll-merge and instead modify the return status of ll_merge() to
indicate when a merge of binary files has occurred. Message printing
probably does not belong in a "low-level merge" anyway.
This commit continues printing the message as-is, just from the callers
instead of within ll_merge(). Future changes will start handling the
message differently in the merge-ort codepath.
There was one special case here: the callers in rerere.c do NOT check
for and print such a message; since those code paths explicitly skip
over binary files, there is no reason to check for a return status of
LL_MERGE_BINARY_CONFLICT or print the related message.
Note that my methodology included first modifying ll_merge() to return
a struct, so that the compiler would catch all the callers for me and
ensure I had modified all of them. After modifying all of them, I then
changed the struct to an enum.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:29 +0000 (02:37 +0000)]
log: clean unneeded objects during `log --remerge-diff`
The --remerge-diff option will need to create new blobs and trees
representing the "automatic merge" state. If one is traversing a
long project history, one can easily get hundreds of thousands of
loose objects generated during `log --remerge-diff`. However, none of
those loose objects are needed after we have completed our diff
operation; they can be summarily deleted.
Add a new helper function to tmp_objdir to discard all the contained
objects, and call it after each merge is handled.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 02:37:28 +0000 (02:37 +0000)]
show, log: provide a --remerge-diff capability
When this option is specified, we remerge all (two parent) merge commits
and diff the actual merge commit to the automatically created version,
in order to show how users removed conflict markers, resolved the
different conflict versions, and potentially added new changes outside
of conflict regions in order to resolve semantic merge problems (or,
possibly, just to hide other random changes).
This capability works by creating a temporary object directory and
marking it as the primary object store. This makes it so that any blobs
or trees created during the automatic merge are easily removable
afterwards by just deleting all objects from the temporary object
directory.
There are a few ways that this implementation is suboptimal:
* `log --remerge-diff` becomes slow, because the temporary object
directory can fill with many loose objects while running
* the log output can be muddied with misplaced "warning: cannot merge
binary files" messages, since ll-merge.c unconditionally writes those
messages to stderr while running instead of allowing callers to
manage them.
* important conflict and warning messages are simply dropped; thus for
conflicts like modify/delete or rename/rename or file/directory which
are not representable with content conflict markers, there may be no
way for a user of --remerge-diff to know that there had been a
conflict which was resolved (and which possibly motivated other
changes in the merge commit).
* when fixing the previous issue, note that some unimportant conflict
and warning messages might start being included. We should instead
make sure these remain dropped.
Subsequent commits will address these issues.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 03:42:40 +0000 (03:42 +0000)]
repo-settings: rename the traditional default fetch.negotiationAlgorithm
Give the traditional default fetch.negotiationAlgorithm the name
'consecutive'. Also allow a choice of 'default' to have Git decide
between the choices (currently, picking 'skipping' if
feature.experimental is true and 'consecutive' otherwise). Update the
documentation accordingly.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 03:42:39 +0000 (03:42 +0000)]
repo-settings: fix error handling for unknown values
In commit af3a67de01 ("negotiator: unknown fetch.negotiationAlgorithm
should error out", 2018-08-01), error handling for an unknown
fetch.negotiationAlgorithm was added with the code die()ing. This was
also added to the documentation for the fetch.negotiationAlgorithm
option, to make it explicit that the code would die on unknown values.
This behavior was lost with commit aaf633c2ad ("repo-settings: create
feature.experimental setting", 2019-08-13). Restore it so that the
behavior again matches the documentation.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Feb 2022 03:42:38 +0000 (03:42 +0000)]
repo-settings: fix checking for fetch.negotiationAlgorithm=default
In commit 3050b6dfc75d (repo-settings.c: simplify the setup,
2021-09-21), the branch for handling fetch.negotiationAlgorithm=default
was deleted. Since this value is documented in
Documentation/config/fetch.txt, restore the check for this value.
Note that this change caused an observable bug: if someone sets
feature.experimental=true in config, and then passes "-c
fetch.negotiationAlgorithm=default" on the command line in an attempt to
override the config, then the override is ignored. Fix the bug by not
ignoring the value of "default".
Technically, before commit 3050b6dfc75d, repo-settings would treat any
fetch.negotiationAlgorithm value other than "skipping" or "noop" as a
request for "default", but I think it probably makes more sense to
ignore such broken requests and leave fetch.negotiationAlgorithm with
the default value rather than the value of "default". (If that sounds
confusing, note that "default" is usually the default value, but when
feature.experimental=true, "skipping" is the default value.)
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
perl Git.pm: don't ignore signalled failure in _cmd_close()
Fix misbehavior in Git.pm that dates back to the very first version of
the library in git.git added in b1edc53d062 (Introduce Git.pm (v4),
2006-06-24). When we fail to execute a command we shouldn't ignore all
signals, those can happen e.g. if abort() is called, or if the command
segfaults.
Because of this we'd consider e.g. a command that died due to LSAN
exiting with abort() successful, as is the case with the tests listed
as running successfully with SANITIZE=leak in 9081a421a6d (checkout:
fix "branch info" memory leaks, 2021-11-16). We did run them
successfully, but only because we ignored these errors.
This was then made worse by the use of "abort_on_error=1" for LSAN
added in 85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,
2017-09-05). Doing that makes sense, but without providing that option
we'd have a "$? >> 8" of "23" on failure, with abort_on_error=1 we'll
get "0".
All of our tests pass even without the SIGPIPE exception being added
here, but as the code appears to have been trying to ignore it let's
keep ignoring it for now.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Chen Bojun [Sat, 29 Jan 2022 06:35:38 +0000 (14:35 +0800)]
receive-pack: purge temporary data if no command is ready to run
When pushing a hidden ref, e.g.:
$ git push origin HEAD:refs/hidden/foo
"receive-pack" will reject our request with an error message like this:
! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)
The remote side ("git-receive-pack") will not create the hidden ref as
expected, but the pack file sent by "git-send-pack" is left inside the
remote repository. I.e. the quarantine directory is not purged as it
should be.
Add a checkpoint before calling "tmp_objdir_migrate()" and after calling
the "pre-receive" hook to purge that temporary data in the quarantine
area when there is no command ready to run.
The reason we do not add the checkpoint before the "pre-receive" hook,
but after it, is that the "pre-receive" hook is called with a switch-off
"skip_broken" flag, and all commands, even broken ones, should be fed
by calling "feed_receive_hook()".
Add a new test case in t5516 as well.
Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Helped-by: Teng Long <dyroneteng@gmail.com> Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Sat, 29 Jan 2022 00:04:44 +0000 (16:04 -0800)]
builtin/branch: consolidate action-picking logic in cmd_branch()
Consolidate the logic for deciding when to create a new branch in
cmd_branch(), and save the result for reuse. Besides making the function
more explicit, this allows us to validate options that can only be used
when creating a branch. Such an option does not exist yet, but one will
be introduced in a subsequent commit.
Helped-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Glen Choo <chooglen@google.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Sat, 29 Jan 2022 00:04:43 +0000 (16:04 -0800)]
branch: add a dry_run parameter to create_branch()
Add a dry_run parameter to create_branch() such that dry_run = 1 will
validate a new branch without trying to create it. This will be used in
`git branch --recurse-submodules` to ensure that the new branch can be
created in all submodules.
Signed-off-by: Glen Choo <chooglen@google.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Sat, 29 Jan 2022 00:04:42 +0000 (16:04 -0800)]
branch: make create_branch() always create a branch
With the previous commit, there are no more invocations of
create_branch() that do not create a branch because:
* BRANCH_TRACK_OVERRIDE is no longer passed
* clobber_head_ok = true and force = false is never passed
Assert these situations, delete dead code and ensure that we're handling
clobber_head_ok and force correctly by introducing tests for `git branch
--force`. As a result, create_branch() now always creates a branch.
Helped-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Glen Choo <chooglen@google.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Sat, 29 Jan 2022 00:04:41 +0000 (16:04 -0800)]
branch: move --set-upstream-to behavior to dwim_and_setup_tracking()
This commit is preparation for a future commit that will simplify
create_branch() so that it always creates a branch. This will allow
create_branch() to accept a dry_run parameter (which is needed for "git
branch --recurse-submodules").
create_branch() used to always create a branch, but 4fc5006676 (Add
branch --set-upstream, 2010-01-18) changed it to also be able to set
tracking information without creating a branch.
Refactor the code that sets tracking information into its own functions
dwim_branch_start() and dwim_and_setup_tracking(). Also change an
invocation of create_branch() in cmd_branch() in builtin/branch.c to use
dwim_and_setup_tracking(), since that invocation is only for setting
tracking information (in "git branch --set-upstream-to").
As of this commit, create_branch() is no longer invoked in a way that
does not create branches.
Helped-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Glen Choo <chooglen@google.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Thomas Koutcher [Tue, 1 Feb 2022 17:26:04 +0000 (18:26 +0100)]
subtree: force merge commit
When `merge.ff` is set to `only` in .gitconfig, `git subtree pull` will
fail with error `fatal: Not possible to fast-forward, aborting.`, but
the command does want to make merges in these places. Add `--no-ff`
argument to `git merge` to enforce this behaviour.
Signed-off-by: Thomas Koutcher <thomas.koutcher@online.fr> Reviewed-by: Johannes Altmanninger <aclopte@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Han-Wen Nienhuys [Mon, 31 Jan 2022 17:50:18 +0000 (17:50 +0000)]
t1405: explictly delete reflogs for reftable
Deleting a ref in reftable just records a (ObjectID => ZeroID)
transaction in the reflog. To ensure 'for_each_reflog()' test below
works, explictly delete reflogs for deleted refs.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.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>
scalar: accept -C and -c options before the subcommand
The `git` executable has these two very useful options:
-C <directory>:
switch to the specified directory before performing any actions
-c <key>=<value>:
temporarily configure this setting for the duration of the
specified scalar subcommand
With this commit, we teach the `scalar` executable the same trick.
Note: It might look like a good idea to try to reuse the
`handle_options()` function in `git.c` instead of replicating only the
`-c`/`-C` part. However, that function is not only not in `libgit.a`, it
is also intricately entangled with the rest of the code in `git.c` that
is necessary e.g. to handle `--paginate`. Besides, no other option
handled by that `handle_options()` function is relevant to Scalar,
therefore the cost of refactoring vastly would outweigh the benefit.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 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>
diff-filter: be more careful when looking for negative bits
The `--diff-filter=<bits>` option allows to filter the diff by certain
criteria, for example `R` to only show renamed files. It also supports
negating a filter via a down-cased letter, i.e. `r` to show _everything
but_ renamed files.
However, the code is a bit overzealous when trying to figure out whether
`git diff` should start with all diff-filters turned on because the user
provided a lower-case letter: if the `--diff-filter` argument starts
with an upper-case letter, we must not start with all bits turned on.
Even worse, it is possible to specify the diff filters in multiple,
separate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.
Let's accumulate the include/exclude filters independently, and only
special-case the "only exclude filters were specified" case after
parsing the options altogether.
Note: The code replaced by this commit took pains to avoid setting any
unused bits of `options->filter`. That was unnecessary, though, as all
accesses happen via the `filter_bit_tst()` function using specific bits,
and setting the unused bits has no effect. Therefore, we can simplify
the code by using `~0` (or in this instance, `~<unwanted-bit>`).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
docs(diff): lose incorrect claim about `diff-files --diff-filter=A`
Originally, before we had `--intent-to-add`, there was no way that `git
diff-files` could see added files: if a file did not exist in the index,
`git diff-files` would not show it because it looks only at worktree
files when there is an index entry at the same path.
We used this example in the documentation of the diff options to explain
that not every `--diff-filter=<option>` has an effect in all scenarios.
Even when we added `--intent-to-add`, the comment was still correct,
because initially we showed such files as modified instead of added.
However, when that bug was fixed in feea6946a5b (diff-files: treat
"i-t-a" files as "not-in-index", 2020-06-20), the comment in the
documentation became incorrect.
Let's just remove it.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
stash: remove documentation for `stash.useBuiltin`
In 8a2cd3f5123 (stash: remove the stash.useBuiltin setting, 2020-03-03),
we removed the setting, and for a couple of major versions, we still
documented the setting, telling users that it is gone.
We can now safely remove even the documentation.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 90a6bb98d11 (legacy stash -p: respect the add.interactive.usebuiltin
setting, 2019-12-21), we added support to use the built-in `add -p` from
the scripted `stash -p`.
In 8a2cd3f5123 (stash: remove the stash.useBuiltin setting, 2020-03-03),
we retired the scripted `stash` (including the scripted `stash -p`).
Therefore this support is no longer necessary.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-sh-setup: remove remnant bits referring to `git-legacy-stash`
In 8a2cd3f5123 (stash: remove the stash.useBuiltin setting, 2020-03-03),
we removed `git-legacy-stash.sh`. But `git-sh-setup.sh` somehow still
thinks about it. Let's just not.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 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.
object-name: re-use "struct strbuf" in show_ambiguous_object()
Reduce the allocations done by show_ambiguous_object() by moving the
"desc" strbuf into the "struct ambiguous_output" introduced in the
preceding commit.
This doesn't matter for optimization purposes, but since we're
accumulating a "struct strbuf advice" anyway let's follow that pattern
and add a "struct strbuf sb", we can then strbuf_reset() it rather
than calling strbuf_release() for each call to
show_ambiguous_object().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
object-name: iterate ambiguous objects before showing header
Change the "The candidates are" header that's shown for ambiguous
objects to be shown after we've iterated over all of the objects.
If we get any errors while doing so we don't want to split up the the
header and the list as a result. The two will now be printed together,
as shown in the updated testcase.
As we're accumulating the lines into as "struct strbuf" before
emitting them we need to add a trailing newline to the call in
show_ambiguous_object(). This and the change from "The candidates
are:" to "The candidates are:\n%s" helps to give translators more
context.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Make the ambiguous tag object output nicer in the case of tag objects
such as ebf3c04b262 (Git 2.32, 2021-06-06) by including the date in
the "tagger" header. I.e.:
$ git rev-parse b7e68
error: short object ID b7e68 is ambiguous
hint: The candidates are:
hint: b7e68c41d92 tag 2021-06-06 - v2.32.0
hint: b7e68ae18e0 commit 2019-12-23 - bisect: use the standard 'if (!var)' way to check for 0
hint: b7e68f6b413 tree
hint: b7e68490b97 blob
b7e68
[...]
Before this we'd emit a "tag" line without a date, e.g.:
object-name: make ambiguous object output translatable
Change the output of show_ambiguous_object() added in [1] and last
tweaked in [2] and the preceding commit to be more friendly to
translators.
By being able to customize the "<SP><SP>%s\n" format we're even ready
for RTL languages, who'd presumably like to change that to
"%s<SP><SP>\n".
In the case of the existing "tag [tag could not be parsed]" output
we'll now instead emit "[bad tag, could not parse it]". This is
consistent with the "[bad object]" output. Rephrasing the message like
this is possible because we're not unconditionally adding the
type_name() at the beginning.
1. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
2. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
object-name: explicitly handle bad tags in show_ambiguous_object()
Follow-up the handling of OBJ_BAD in the preceding commit and
explicitly handle those cases where parse_tag() fails, or we don't end
up with a non-NULL pointer in in tag->tag.
If we run into such a tag we'd previously be silent about it. We
really should also be handling these batter in parse_tag_buffer() by
being more eager to emit an error(), instead of silently aborting with
"return -1;".
One example of such a tag is the one that's tested for in
"t3800-mktag.sh", where the code takes the "size <
the_hash_algo->hexsz + 24" branch.
But in lieu of earlier missing "error" output let's show the user
something to indicate why we're not showing a tag message in these
cases, now instead of showing:
object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
Amend the "unknown type" handling in the code that displays the
ambiguous object list to assert() that we're either going to get the
"real" object types we can pass to type_name(), or a -1 (OBJ_BAD)
return value from oid_object_info().
See [1] for the current output, and [1] for the commit that added the
"unknown type" handling.
We are never going to get an "unknown type" in the sense of custom
types crafted with "hash-object --literally", since we're not using
the OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag.
If we manage to otherwise unpack such an object without errors we'll
die() in parse_loose_header_extended() called by sort_ambiguous()
before we get to show_ambiguous_object(), as is asserted by the test
added in the preceding commit.
So saying "unknown type" here was always misleading, we really meant
to say that we had a failure parsing the object at all, i.e. that we
had repository corruption. If the problem is only that it's type is
unknown we won't reach this code.
So let's emit a generic "[bad object]" instead. As our tests added in
the preceding commit show, we'll have emitted various "error" output
already in those cases.
We should do better in the truly "unknown type" cases, which we'd need
to handle if we were passing down the OBJECT_INFO_ALLOW_UNKNOWN_TYPE
flag. But let's leave that for some future improvement. In a
subsequent commit I'll improve the output we do show, and not having
to handle the "unknown type" (as in OBJECT_INFO_ALLOW_UNKNOWN_TYPE)
simplifies that change.
1. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
2. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
object-name tests: add tests for ambiguous object blind spots
Extend the tests for ambiguous objects to check how we handle objects
where we return OBJ_BAD when trying to parse them. As noted in [1] we
have a blindspot when it comes to this behavior.
Since we need to add new test data here let's extend these tests to be
tested under SHA-256, in d7a2fc82491 (t1512: skip test if not using
SHA-1, 2018-05-13) all of the existing tests were skipped, as they
rely on specific SHA-1 object IDs.
For these tests it only matters that the first 4 characters of the OID
prefix are the same for both SHA-1 and SHA-256. This uses strings that
I mined, and have the same prefix when hashed with both.
We "test_cmp" the full output to guard against any future regressions,
and because a subsequent commit will tweak it. Showing a diff of how
the output changes is helpful to explain those subsequent commits.
The "sed" invocation in test_cmp_failed_rev_parse() doesn't need a
"/g" because under both SHA-1 and SHA-256 we'll wildcard match any
trailing part of the OID after our known starting prefix. We'd like to
convert all of that to just "..." for the "test_cmp" which follows.
Taylor Blau [Tue, 25 Jan 2022 22:41:20 +0000 (17:41 -0500)]
pack-bitmap.c: gracefully fallback after opening pack/MIDX
When opening a MIDX/pack-bitmap, we call open_midx_bitmap_1() or
open_pack_bitmap_1() respectively in a loop over the set of MIDXs/packs.
By design, these functions are supposed to be called over every pack and
MIDX, since only one of them should have a valid bitmap.
Ordinarily we return '0' from these two functions in order to indicate
that we successfully loaded a bitmap To signal that we couldn't load a
bitmap corresponding to the MIDX/pack (either because one doesn't exist,
or because there was an error with loading it), we can return '-1'. In
either case, the callers each enumerate all MIDXs/packs to ensure that
at most one bitmap per-kind is present.
But when we fail to load a bitmap that does exist (for example, loading
a MIDX bitmap without finding a corresponding reverse index), we'll
return -1 but leave the 'midx' field non-NULL. So when we fallback to
loading a pack bitmap, we'll complain that the bitmap we're trying to
populate already is "opened", even though it isn't.
Rectify this by setting the '->pack' and '->midx' field back to NULL as
appropriate. Two tests are added: one to ensure that the MIDX-to-pack
bitmap fallback works, and another to ensure we still complain when
there are multiple pack bitmaps in a repository.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:17 +0000 (17:41 -0500)]
midx: read `RIDX` chunk when present
When a MIDX contains the new `RIDX` chunk, ensure that the reverse index
is read from it instead of the on-disk .rev file. Since we need to
encode the object order in the MIDX itself for correctness reasons,
there is no point in storing the same data again outside of the MIDX.
So, this patch stops writing separate .rev files, and reads it out of
the MIDX itself. This is possible to do with relatively little new code,
since the format of the RIDX chunk is identical to the data in the .rev
file. In other words, we can implement this by pointing the
`revindex_data` field at the reverse index chunk of the MIDX instead of
the .rev file without any other changes.
Note that we have two knobs that are adjusted for the new tests:
GIT_TEST_MIDX_WRITE_REV and GIT_TEST_MIDX_READ_RIDX. The former controls
whether the MIDX .rev is written at all, and the latter controls whether
we read the MIDX's RIDX chunk.
Both are necessary to ensure that the test added at the beginning of
this series continues to work. This is because we always need to write
the RIDX chunk in the MIDX in order to change its checksum, but we want
to make sure reading the existing .rev file still works (since the RIDX
chunk takes precedence by default).
Arguably this isn't a very interesting mode to test, because the
precedence rules mean that we'll always read the RIDX chunk over the
.rev file. But it makes it impossible for a user to induce corruption in
their repository by adjusting the test knobs (since if we had an
either/or knob they could stop writing the RIDX chunk, allowing them to
tweak the MIDX's object order without changing its checksum).
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:15 +0000 (17:41 -0500)]
t/lib-bitmap.sh: parameterize tests over reverse index source
To prepare for reading the reverse index data out of the MIDX itself,
teach the `test_rev_exists` function to take an expected "source" for
the reverse index data.
When given "rev", it asserts that the MIDX's `.rev` file exists, and is
loaded when verifying the integrity of its bitmaps. Otherwise, it
ensures that trace2 reports the source of the reverse index data as the
same string which was given to test_rev_exists().
The following patch will implement reading the reverse index data from
the MIDX itself.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:13 +0000 (17:41 -0500)]
t5326: move tests to t/lib-bitmap.sh
In t5326, we have a handful of tests that we would like to run twice:
once using the MIDX's new `RIDX` chunk as the source of the
reverse-index cache, and once using the separate `.rev` file.
But because these tests mutate the state of the underlying repository,
and then make assumptions about those mutations occurring in a certain
sequence, simply running the tests twice in the same repository is
awkward.
Instead, extract the core of interesting tests into t/lib-bitmap.sh to
prepare for them to be run twice, each in a separate test script. This
means that they can each operate on a separate repository, removing any
concerns about mutating state.
For now, this patch is a strict cut-and-paste of some tests from t5326.
The tests which did not move are not interesting with respect to the
source of their reverse index data.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:10 +0000 (17:41 -0500)]
t5326: extract `test_rev_exists`
To determine which source of data is used for the MIDX's reverse index
cache, introduce a helper which forces loading the reverse index, and
then looks for the special trace2 event introduced in a previous commit.
For now, this helper just looks for when the legacy MIDX .rev file was
loaded, but in a subsequent commit will become parameterized over the
the reverse index's source.
This function replaces checking for the existence of the .rev file. We
could write a similar helper to ensure that the .rev file is cleaned up
after repacking, but it will make subsequent tests more difficult to
write, and provides marginal value since we already check that the MIDX
.bitmap file is removed.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:08 +0000 (17:41 -0500)]
t5326: drop unnecessary setup
The core.multiPackIndex config became true by default back in 18e449f86b
(midx: enable core.multiPackIndex by default, 2020-09-25), so it is no
longer necessary to enable it explicitly.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:05 +0000 (17:41 -0500)]
pack-revindex.c: instrument loading on-disk reverse index
In a subsequent commit, we'll use the MIDX's new 'RIDX' chunk as a
source for the reverse index's data. But it will be useful for tests to
be able to determine whether the reverse index was loaded from the
separate .rev file, or from a chunk within the MIDX.
To instrument this, add a trace2 event which the tests can look for in
order to determine the reverse index's source.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:03 +0000 (17:41 -0500)]
midx.c: make changing the preferred pack safe
The previous patch demonstrates a bug where a MIDX's auxiliary object
order can become out of sync with a MIDX bitmap.
This is because of two confounding factors:
- First, the object order is stored in a file which is named according
to the multi-pack index's checksum, and the MIDX does not store the
object order. This means that the object order can change without
altering the checksum.
- But the .rev file is moved into place with finalize_object_file(),
which link(2)'s the file into place instead of renaming it. For us,
that means that a modified .rev file will not be moved into place if
MIDX's checksum was unchanged.
This fix is to force the MIDX's checksum to change when the preferred
pack changes but the set of packs contained in the MIDX does not. In
other words, when the object order changes, the MIDX's checksum needs to
change with it (regardless of whether the MIDX is tracking the same or
different packs).
This prevents a race whereby changing the object order (but not the
packs themselves) enables a reader to see the new .rev file with the old
MIDX, or similarly seeing the new bitmap with the old object order.
But why can't we just stop hardlinking the .rev into place instead
adding additional data to the MIDX? Suppose that's what we did. Then
when we go to generate the new bitmap, we'll load the old MIDX bitmap,
along with the MIDX that it references. That's fine, since the new MIDX
isn't moved into place until after the new bitmap is generated. But the
new object order *has* been moved into place. So we'll read the old
bitmaps in the new order when generating the new bitmap file, meaning
that without this secondary change, bitmap generation itself would
become a victim of the race described here.
This can all be prevented by forcing the MIDX's checksum to change when
the object order does. By embedding the entire object order into the
MIDX, we do just that. That is, the MIDX's checksum will change in
response to any perturbation of the underlying object order. In t5326,
this will cause the MIDX's checksum to update (even without changing the
set of packs in the MIDX), preventing the stale read problem.
Note that this makes it safe to continue to link(2) the MIDX .rev file
into place, since it is now impossible to have a .rev file that is
out-of-sync with the MIDX whose checksum it references. (But we will do
away with MIDX .rev files later in this series anyway, so this is
somewhat of a moot point).
In theory, it is possible to store a "fingerprint" of the full object
order here, so long as that fingerprint changes at least as often as the
full object order does. Some possibilities here include storing the
identity of the preferred pack, along with the mtimes of the
non-preferred packs in a consistent order. But storing a limited part of
the information makes it difficult to reason about whether or not there
are gaps between the two that would cause us to get bitten by this bug
again.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 25 Jan 2022 22:41:01 +0000 (17:41 -0500)]
t5326: demonstrate bitmap corruption after permutation
This patch demonstrates a cause of bitmap corruption that can occur when
the contents of the multi-pack index does not change, but the underlying
object order does.
In this example, we have a MIDX containing two packs, each with a
distinct set of objects (pack A corresponds to the tree, blob, and
commit from the first patch, and pack B corresponds to the second
patch).
First, a MIDX is written where the 'A' pack is preferred. As expected,
the bitmaps generated there are in-tact. But then, we generate an
identical MIDX with a different object order: this time preferring pack
'B'.
Due to a bug which will be explained and fixed in the following commit,
the MIDX is updated, but the .rev file is not, causing the .bitmap file
to be read incorrectly. Specifically, the .bitmap file will contain
correct data, but the auxiliary object order in the .rev file is stale,
causing readers to get confused by reading the new bitmaps using the old
object order.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
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>
Phillip Wood [Wed, 26 Jan 2022 13:05:49 +0000 (13:05 +0000)]
rebase -m: don't fork git checkout
Now that reset_head() can handle the initial checkout of onto
correctly use it in the "merge" backend instead of forking "git
checkout". This opens the way for us to stop calling the
post-checkout hook in the future. Not running "git checkout" means
that "rebase -i/m" no longer recurse submodules when checking out
"onto" (thanks to Philippe Blain for pointing this out). As the rest
of rebase does not know what to do with submodules this is probably a
good thing. When using merge-ort rebase ought be able to handle
submodules correctly if it parsed the submodule config, such a change
is left for a future patch series.
The "apply" based rebase has avoided forking git checkout
since ac7f467fef ("builtin/rebase: support running "git rebase
<upstream>"", 2018-08-07). The code that handles the checkout was
moved into libgit by b309a97108 ("reset: extract reset_head() from
rebase", 2020-04-07).
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:48 +0000 (13:05 +0000)]
rebase --apply: set ORIG_HEAD correctly
At the start of a rebase, ORIG_HEAD is updated to the tip of the
branch being rebased. Unfortunately reset_head() always uses the
current value of HEAD for this which is incorrect if the rebase is
started with "git rebase <upstream> <branch>" as in that case
ORIG_HEAD should be updated to <branch>. This only affects the "apply"
backend as the "merge" backend does not yet use reset_head() for the
initial checkout. Fix this by passing in orig_head when calling
reset_head() and add some regression tests.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:47 +0000 (13:05 +0000)]
rebase --apply: fix reflog
move_to_original_branch() passes the message intended for the branch
reflog as `orig_head_msg`. Fix this by adding a `branch_msg` member to
struct reset_head_opts and add a regression test. Note that these
reflog messages do not respect GIT_REFLOG_ACTION. They are not alone
in that and will be fixed in a future series.
The "merge" backend already has tests that check both the branch and
HEAD reflogs.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:46 +0000 (13:05 +0000)]
reset_head(): take struct rebase_head_opts
This function takes a confusingly large number of parameters which
makes it difficult to remember which order to pass them in. The
following commits will add a couple more parameters which makes the
problem worse. To address this change the function to take a struct of
options. Using a struct means that it is no longer necessary to
remember which order to pass the parameters in and anyone reading the
code can easily see which value is passed to each parameter.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:45 +0000 (13:05 +0000)]
rebase: cleanup reset_head() calls
If ORIG_HEAD is not set by passing RESET_ORIG_HEAD then there is no
need to pass anything for reflog_orig_head. In addition to the callers
fixed in this commit move_to_original_branch() also passes
reflog_orig_head without setting ORIG_HEAD. That caller is mistakenly
passing the message it wants to put in the branch reflog which is not
currently possible so we delay fixing that caller until we can pass
the message as the branch reflog.
A later commit will make it a BUG() to pass reflog_orig_head without
RESET_ORIG_HEAD, that changes cannot be done here as it needs to wait
for move_to_original_branch() to be fixed first.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:44 +0000 (13:05 +0000)]
create_autostash(): remove unneeded parameter
The default_reflog parameter of create_autostash() is passed to
reset_head(). However as creating a stash does not involve updating
any refs the parameter is not used by reset_head(). Removing the
parameter from create_autostash() simplifies the callers.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:43 +0000 (13:05 +0000)]
reset_head(): make default_reflog_action optional
This parameter is only needed when a ref is going to be updated and
the caller does not pass an explicit reflog message. Callers that are
only discarding uncommitted changes in the working tree such as such
as "rebase --skip" or create_autostash() do not update any refs so
should not have to worry about passing this parameter.
This change is not intended to have any user visible changes. The
pointer comparison between `oid` and `&head_oid` checks that the
caller did not pass an oid to be checked out. As no callers pass
RESET_HEAD_RUN_POST_CHECKOUT_HOOK without passing an oid there are
no changes to when the post-checkout hook is run. As update_ref() only
updates the ref if the oid passed to it differs from the current ref
there are no changes to when HEAD is updated.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:42 +0000 (13:05 +0000)]
reset_head(): factor out ref updates
In the next commit we will stop trying to update HEAD when we are
removing uncommitted changes from the working tree. Move the code that
updates the refs to its own function in preparation for that.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:41 +0000 (13:05 +0000)]
reset_head(): remove action parameter
The only use of the action parameter is to setup the error messages
for unpack_trees(). All but two cases pass either "checkout" or
"reset". The case that passes "reset --hard" would be better passing
"reset" so that the error messages match the builtin reset command
like all the other callers that are doing a reset. The case that
passes "Fast-forwarded" is only updating HEAD and so the parameter is
unused in that case as it does not call unpack_trees(). The value to
pass to setup_unpack_trees_porcelain() can be determined by checking
whether flags contains RESET_HEAD_HARD without the caller having to
specify it.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:40 +0000 (13:05 +0000)]
rebase --apply: don't run post-checkout hook if there is an error
The hook should only be run if the worktree and refs were successfully
updated. This primarily affects "rebase --apply" but also "rebase
--merge" when it fast-forwards.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:39 +0000 (13:05 +0000)]
rebase: do not remove untracked files on checkout
If "git rebase [--apply|--merge] <upstream> <branch>" detects that
<upstream> is an ancestor of <branch> then it will fast-forward and
checkout <branch>. Normally a checkout or picking a commit during a
rebase will refuse to overwrite untracked files, however rebase does
overwrite untracked files when checking out <branch>.
The fix is to only set reset in `unpack_tree_opts` if flags contains
`RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
but it will be extended in the next commit to check that the
post-checkout hook is not run when the checkout fails.
The test for `!detach_head` dates back to the
original implementation of reset_head() in ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
2018-08-07) and was correct until e65123a71d
("builtin rebase: support `git rebase <upstream> <switch-to>`",
2018-09-04) started using reset_head() to checkout <switch-to> when
fast-forwarding.
Note that 480d3d6bf9 ("Change unpack_trees' 'reset' flag into an
enum", 2021-09-27) also fixes this bug as it changes reset_head() to
never remove untracked files. I think this fix is still worthwhile as
it makes it clear that the same settings are used for detached and
non-detached checkouts.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:38 +0000 (13:05 +0000)]
rebase: pass correct arguments to post-checkout hook
If a rebase started with "rebase [--apply|--merge] <upstream> <branch>"
detects that <upstream> is an ancestor of <branch> then it fast-forwards
and checks out <branch>. Unfortunately in that case it passed the null
oid as the first argument to the post-checkout hook rather than the oid
of HEAD.
A side effect of this change is that the call to update_ref() which
updates HEAD now always receives the old value of HEAD. This provides
protection against another process updating HEAD during the checkout.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Wed, 26 Jan 2022 13:05:37 +0000 (13:05 +0000)]
t5403: refactor rebase post-checkout hook tests
These tests only test the default backend and do not check that the
arguments passed to the hook are correct. Fix this by running the
tests with both backends and adding checks for the hook arguments.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> 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>
We have a copy of uncompress2() implementation in compat/ so that we
can build with an older version of zlib that lack the function, and
the build procedure selects if it is used via the NO_UNCOMPRESS2
$(MAKE) variable. This is yet another "annoying" knob the porters
need to tweak on platforms that are not common enough to have the
default set in the config.mak.uname file.
Attempt to instead ask the system header <zlib.h> to decide if we
need the compatibility implementation. This is a deviation from the
way we have been handling the "compatiblity" features so far, and if
it can be done cleanly enough, it could work as a model for features
that need compatibility definition we discover in the future. With
that goal in mind, avoid expedient but ugly hacks, like shoving the
code that is conditionally compiled into an unrelated .c file, which
may not work in future cases---instead, take an approach that uses a
file that is independently compiled and stands on its own.
Compile and link compat/zlib-uncompress2.c file unconditionally, but
conditionally hide the implementation behind #if/#endif when zlib
version is 1.2.9 or newer, and unconditionally archive the resulting
object file in the libgit.a to be picked up by the linker.
There are a few things to note in the shape of the code base after
this change:
- We no longer use NO_UNCOMPRESS2 knob; if the system header
<zlib.h> claims a version that is more cent than the library
actually is, this would break, but it is easy to add it back when
we find such a system.
- The object file compat/zlib-uncompress2.o is always compiled and
archived in libgit.a, just like a few other compat/ object files
already are.
- The inclusion of <zlib.h> is done in <git-compat-util.h>; we used
to do so from <cache.h> which includes <git-compat-util.h> as the
first thing it does, so from the *.c codes, there is no practical
change.
- Until objects in libgit.a that is already used gains a reference
to the function, the reftable code will be the only one that
wants it, so libgit.a on the linker command line needs to appear
once more at the end to satisify the mutual dependency.
- Beat found a trick used by OpenSSL to avoid making the
conditionally-compiled object truly empty (apparently because
they had to deal with compilers that do not want to see an
effectively empty input file). Our compat/zlib-uncompress2.c
file borrows the same trick for portabilty.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Helped-by: Beat Bolli <dev+git@drbeat.li> 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>
projects / thirdparty / git.git / log
