Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it computes values for blobs instead of using
hard-coded hashes.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it computes variables for blobs and uses the
ZERO_OID variable instead of using hard-coded hashes.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Strip out the index lines in the diff before comparing them, as these
will differ between hash algorithms. This leads to a smaller, simpler
change than editing the index line.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
By providing aliases via --list-cmds=, we could simplify command
collection code in the script. We only issue one git command. Before
this patch that is "git config", after it's "git --list-cmds=". In
"git help" completion case we actually reduce one "git" process (for
getting guides) but that call was added in this series so it does not
really count.
A couple of bash functions are removed because they are not needed
anymore. __git_compute_all_commands() and $__git_all_commands stay
because they are still needed for completing pager.* config and
without "alias" group, the result is still cacheable.
There is a slight (good) change in _git_help() with this patch: before
"git help <tab>" shows external commands (as in _not_ part of git) as
well as part of $__git_all_commands. We have finer control over
command listing now and can exclude that because we can't provide a
man page for external commands anyway.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of maintaining a separate list of command classification,
which often could go out of date, let's centralize the information
back in git.
While the function in git-completion.bash implies "list porcelain
commands", that's not exactly what it does. It gets all commands (aka
--list-cmds=main,others) then exclude certain non-porcelain ones. We
could almost recreate this list two lists list-mainporcelain and
others. The non-porcelain-but-included-anyway is added by the third
category list-complete.
Note that the current completion script incorrectly classifies
filter-branch as porcelain and t9902 tests this behavior. We keep it
this way in t9902 because this test does not really care which
particular command is porcelain or plumbing, they're just names.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The help command currently hard codes the list of guides and their
summary in C. Let's move this list to command-list.txt. This lets us
extract summary lines from Documentation/git*.txt. This also
potentially lets us list guides in git.txt, but I'll leave that for
now.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This lists all recognized commands [1] by category. The group order
follows closely git.txt.
[1] We may actually show commands that are not built (e.g. if you set
NO_PERL you don't have git-instaweb but it's still listed here). I
ignore the problem because on Linux a git package could be split
anyway. The "git-core" package may not contain git-instaweb even if
it's built because it may end up in a separate package. We can't know
anyway.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Even if these are hidden options, let's make them a bit more generic
since we're introducing more listing types shortly. The code is
structured to allow combining multiple listing types together because
we will soon add more types the 'builtins'.
'parseopt' remains separate because it has separate (SPC) to match
git-completion.bash needs and will not combine with others.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If you run "config --blob" outside of a repository, then we
eventually try to resolve the blob name and hit a BUG().
Let's catch this earlier and provide a useful message.
Note that we could also catch this much lower in the stack,
in git_config_from_blob_ref(). That might cover other
callsites, too, but it's unclear whether those ones would
actually be bugs or not. So let's leave the low-level
functions to assume the caller knows what it's doing (and
BUG() if it turns out it doesn't).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The tests added in 2f271cd9cf (t9902-completion: add tests
demonstrating issues with quoted pathnames, 2018-05-08) and in
2ab6eab4fe (completion: improve handling quoted paths in 'git
ls-files's output, 2018-03-28) have a few shortcomings:
- All these tests use the 'test_completion' helper function, thus
they are exercising the whole completion machinery, although they
are only interested in how git-aware path completion, specifically
the __git_complete_index_file() function deals with unusual
characters in pathnames and on the command line.
- These tests can't satisfactorily test the case of pathnames
containing spaces, because 'test_completion' gets the words on the
command line as a single argument and it uses space as word
separator.
- Some of the tests are protected by different FUNNYNAMES_* prereqs
depending on whether they put backslashes and double quotes or
separator characters (FS, GS, RS, US) in pathnames, although a
filesystem not allowing one likely doesn't allow the others
either.
- One of the tests operates on paths containing '|' and '&'
characters without being protected by a FUNNYNAMES prereq, but
some filesystems (notably on Windows) don't allow these characters
in pathnames, either.
Replace these tests with basically equivalent, more focused tests that
call __git_complete_index_file() directly. Since this function only
looks at the current word to be completed, i.e. the $cur variable, we
can easily include pathnames containing spaces in the tests, so use
such pathnames instead of pathnames containing '|' and '&'. Finally,
use only a single FUNNYNAMES prereq for all kinds of special
characters.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teach fetch to generate ref-prefixes, to be used for server-side
filtering of the ref-advertisement, based on the configured fetch
refspec ('remote.<name>.fetch') when no user provided refspec exists.
Signed-off-by: Brandon Williams <bmwill@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The last two tests 'editor with a space' and 'core.editor with a
space' in 't7005-editor.sh' need the SPACES_IN_FILENAMES prereq to
ensure that they are only run on filesystems that allow, well, spaces
in filenames. However, we have been putting a space in the name of
the trash directory for just over a decade now, so we wouldn't be able
to run any of our tests on such a filesystem in the first place.
This prereq is therefore unnecessary, remove it.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it uses variables and command substitution for
trees instead of hard-coded hashes. This also has the benefit of making
it more obvious how the test works.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
These tests rely on creating packs with specially named objects which
are necessarily dependent on the hash used. Skip these tests if we're
not using SHA-1.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This test relies on objects with colliding short names which are
necessarily dependent on the hash used. Skip the test if we're not
using SHA-1.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This test relies on objects with colliding short names which are
necessarily dependent on the hash used. Skip the test if we're not
using SHA-1.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since this is a core test that tests basic functionality, annotate the
assertions that have dependencies on SHA-1 with the appropriate
prerequisite.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since this is a core test that tests basic functionality, annotate the
assertions that have dependencies on SHA-1 with the appropriate
prerequisite.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Switch all uses of $_x40 to $OID_REGEX so that they work correctly with
larger hashes. This commit was created by using the following sed
command to modify all files in the t directory except t/test-lib.sh:
sed -i 's/\$_x40/$OID_REGEX/g'
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently we have a variable, $_x40, which contains a regex that matches
a full 40-character hex constant. However, with NewHash, we'll have
object IDs that are longer than 40 characters. In such a case, $_x40
will be a confusing name. Create a $OID_REGEX variable which will
always reflect a regex matching the appropriate object ID, regardless of
the length of the current hash.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Switch all uses of $_z40 to $ZERO_OID so that they work correctly with
larger hashes. This commit was created by using the following sed
command to modify all files in the t directory except t/test-lib.sh:
sed -i 's/\$_z40/$ZERO_OID/g'
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently we have a variable, $_z40, which contains the all-zero object
ID. However, with NewHash, we'll have an all-zero object ID which is
longer than 40 hex characters. In such a case, $_z40 will be a
confusing name. Create a $ZERO_OID variable which will always reflect
the all-zeros object ID, regardless of the length of the current hash.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There are some basic tests in our codebase that test that we get fixed
SHA-1 values. These are valuable because they make sure that our SHA-1
implementation is free of bugs, but obviously these tests will fail with
a different hash.
There are also tests which intentionally produce objects that have
collisions when truncated to a certain length to test our handling of
these cases. These tests, too, will fail with a different hash.
Add an SHA1 prerequisite to annotate both of these types of tests and
disable them when we're using a different hash. In the future, we will
create versions of these tests which handle both SHA-1 and NewHash.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Do the preparatory fetch inside the test of ls-remote --symref to avoid
cluttering the test output and to be able to catch unexpected fetch
failures.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
After performing a merge that has conflicts git status will, by default,
attempt to detect renames which causes many objects to be examined. In a
virtualized repo, those objects do not exist locally so the rename logic
triggers them to be fetched from the server. This results in the status call
taking hours to complete on very large repos vs seconds with this patch.
Add a new config status.renames setting to enable turning off rename
detection during status and commit. This setting will default to the value
of diff.renames.
Add a new config status.renamelimit setting to to enable bounding the time
spent finding out inexact renames during status and commit. This setting
will default to the value of diff.renamelimit.
Add --no-renames command line option to status that enables overriding the
config setting from the command line. Add --find-renames[=<n>] command line
option to status that enables detecting renames and optionally setting the
similarity index.
Reviewed-by: Elijah Newren <newren@gmail.com>
Original-Patch-by: Alejandro Pauly <alpauly@microsoft.com>
Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
By default we want to fill the whole screen if possible, but we do not
want to use up _all_ terminal columns because the last character is
going hit the border, push the cursor over and wrap. Keep it at
default value zero, which will make print_columns() set the width at
term_columns() - 1.
This affects the test in t7004 because effective column width before
was 40 but now 39 so we need to compensate it by one or the output at
39 columns has a different layout.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
According to the documentation, it is possible to "specify 40 '0' or an
empty string as <oldvalue> to make sure that the ref you are creating
does not exist." But in the code for pseudorefs, we do not implement
this, as demonstrated by the failing tests added in the previous commit.
If we fail to read the old ref, we immediately die. But a failure to
read would actually be a good thing if we have been given the zero oid.
With the zero oid, allow -- and even require -- the ref-reading to fail.
This implements the "make sure that the ref ... does not exist" part of
the documentation and fixes both failing tests from the previous commit.
Since we have a `strbuf err` for collecting errors, let's use it and
signal an error to the caller instead of dying hard.
Reported-by: Rafael Ascensão <rafa.almas@gmail.com>
Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
I have not been able to find any tests around adding pseudorefs using
`git update-ref`. Add some as outlined in this table (original design by
Michael Haggerty; modified and extended by me):
Pre-update value | ref-update old OID | Expected result
-------------------|----------------------|----------------
missing | value | reject
missing | none given | accept
set | none given | accept
set | correct value | accept
set | wrong value | reject
missing | zero | accept *
set | zero | reject *
The tests marked with a * currently fail, despite git-update-ref(1)
claiming that it is possible to "specify 40 '0' or an empty string as
<oldvalue> to make sure that the ref you are creating does not exist."
These failing tests will be fixed in the next commit.
It is only natural to test deletion as well. Test deletion without an
old OID, with a correct one and with an incorrect one.
Suggested-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change the output emitted when an ambiguous object is encountered so
that we show tags first, then commits, followed by trees, and finally
blobs. Within each type we show objects in hashcmp() order. Before
this change the objects were only ordered by hashcmp().
The reason for doing this is that the output looks better as a result,
e.g. the v2.17.0 tag before this change on "git show e8f2" would
display:
hint: The candidates are:
hint: e8f2093055 tree
hint: e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
hint: e8f21d02f7 blob
hint: e8f21d577c blob
hint: e8f25a3a50 tree
hint: e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
hint: e8f2650052 tag v2.17.0
hint: e8f2867228 blob
hint: e8f28d537c tree
hint: e8f2a35526 blob
hint: e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
hint: e8f2cf6ec0 tree
Now we'll instead show:
hint: e8f2650052 tag v2.17.0
hint: e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
hint: e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
hint: e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
hint: e8f2093055 tree
hint: e8f25a3a50 tree
hint: e8f28d537c tree
hint: e8f2cf6ec0 tree
hint: e8f21d02f7 blob
hint: e8f21d577c blob
hint: e8f2867228 blob
hint: e8f2a35526 blob
Since we show the commit data in the output that's nicely aligned once
we sort by object type. The decision to show tags before commits is
pretty arbitrary. I don't want to order by object_type since there
tags come last after blobs, which doesn't make sense if we want to
show the most important things first.
I could display them after commits, but it's much less likely that
we'll display a tag, so if there is one it makes sense to show it
prominently at the top.
A note on the implementation: Derrick rightly pointed out[1] that
we're bending over backwards here in get_short_oid() to first
de-duplicate the list, and then emit it, but could simply do it in one
step.
The reason for that is that oid_array_for_each_unique() doesn't
actually require that the array be sorted by oid_array_sort(), it just
needs to be sorted in some order that guarantees that all objects with
the same ID are adjacent to one another, which (barring a hash
collision, which'll be someone else's problem) the sort_ambiguous()
function does.
I agree that would be simpler for this code, and had forgotten why I
initially wrote it like this[2]. But on further reflection I think
it's better to do more work here just so we're not underhandedly using
the oid-array API where we lie about the list being sorted. That would
break any subsequent use of oid_array_lookup() in subtle ways.
I could get around that by hacking the API itself to support this
use-case and documenting it, which I did as a WIP patch in [3], but I
think it's too much code smell just for this one call site. It's
simpler for the API to just introduce a oid_array_for_each() function
to eagerly spew out the list without sorting or de-duplication, and
then do the de-duplication and sorting in two passes.
1. https://public-inbox.org/git/20180501130318.58251-1-dstolee@microsoft.com/
2. https://public-inbox.org/git/876047ze9v.fsf@evledraar.gmail.com/
3. https://public-inbox.org/git/874ljrzctc.fsf@evledraar.gmail.com/
Helped-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
b2dc968e60 (t5516: refactor oddball tests, 2008-11-07) accidentaly
broke the &&-chain in the test 'push does not update local refs on
failure', but since it was in a subshell, chain-lint couldn't notice
it.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a while-at-it cleanup replacing a 'cd dir && <...> && cd ..' with a
subshell, commit 28391a80a9 (receive-pack: allow deletion of corrupt
refs, 2007-11-29) also moved the assignment of the $old_commit
variable to that subshell. This variable, however, is used outside of
that subshell as a parameter of check_push_result(), to check that a
ref still points to the commit where it is supposed to. With the
variable remaining unset outside the subshell check_push_result()
doesn't perform that check at all.
Use 'git -C <dir> cmd...', so we don't need to change directory, and
thus don't need the subshell either when setting $old_commit.
Furthermore, change check_push_result() to require at least three
parameters (the repo, the oid, and at least one ref), so it will catch
similar issues earlier should they ever arise.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The two JGit tests 'we can read jgit bitmaps' and 'jgit can read our
bitmaps' in 't5310-pack-bitmaps.sh' fail when run with
GIT_TEST_SPLIT_INDEX=YesPlease. Both tests create a clone of the test
repository to check bitmap interoperability with JGit. With split
indexes enabled the index in the clone repositories contains the
'link' extension, which JGit doesn't support and, consequently, an
exception aborts it:
<...>
org.eclipse.jgit.api.errors.JGitInternalException: DIRC extension 'link' not supported by this version.
at org.eclipse.jgit.dircache.DirCache.readFrom(DirCache.java:562)
<...>
Since testing bitmaps doesn't need a worktree in the first place,
let's just create bare clones for the two JGit tests, so the cloned
won't have an index, and these two tests can be executed even with
split index enabled.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous commit added code generation for all_cmd_desc[] which
includes almost everything we need to generate common command list.
Convert help code to use that array instead and drop common_cmds[] array.
The description of each common command group is removed from
command-list.txt. This keeps this file format simpler. common-cmds.h
will not be generated correctly after this change due to the
command-list.txt format change. But it does not matter and
common-cmds.h will be removed.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Placing `struct lock_file`s on the stack used to be a bad idea, because
the temp- and lockfile-machinery would keep a pointer into the struct.
But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05), we can safely have lockfiles on the stack. (This applies
even if a user returns early, leaving a locked lock behind.)
Each of these `struct lock_file`s is used from within a single function.
Move them into the respective functions to make the scope clearer and
drop the staticness.
For good measure, I have inspected these sites and come to believe that
they always release the lock, with the possible exception of bailing out
using `die()` or `exit()` or by returning from a `cmd_foo()`.
As pointed out by Jeff King, it would be bad if someone held on to a
`struct lock_file *` for some reason. After some grepping, I agree with
his findings: no-one appears to be doing that.
After this commit, the remaining occurrences of "static struct
lock_file" are locks that are used from within different functions. That
is, they need to remain static. (Short of more intrusive changes like
passing around pointers to non-static locks.)
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Die in case writing the index fails, so that the caller can notice
(instead of, say, being impressed by how performant the writing is).
While at it, note that after opening a lock with `LOCK_DIE_ON_ERROR`, we
do not need to worry about whether we succeeded. Also, we can move the
`struct lock_file` into the function and drop the staticness. (Placing
`struct lock_file`s on the stack used to be a bad idea, because the
temp- and lockfile-machinery would keep a pointer into the struct. But
after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can safely have lockfiles on the stack.)
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The test script 't6050-replace.sh' starts off with redirecting the
whole test script's stdin from /dev/null. This redirection has been
there since the test script was introduced in a3e8267225
(replace_object: add a test case, 2009-01-23), but the commit message
doesn't explain why it was deemed necessary. AFAICT, it has never
been necessary, and t6050 runs just fine and succeeds even without it,
not only the current version but past versions as well.
Besides being unnecessary, this redirection is also harmful, as it
prevents the test helper functions 'test_pause' and 'debug' from
working properly in t6050, because we can't enter any commands to the
shell and the debugger, respectively.
So let's remove that redirection.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add the ability to control rename detection for merge via a config setting.
This setting behaves the same and defaults to the value of diff.renames but only
applies to merge.
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The can-working-tree-updates-be-skipped check has had a long and blemished
history. The update can be skipped iff:
a) The merge is clean
b) The merge matches what was in HEAD (content, mode, pathname)
c) The target path is usable (i.e. not involved in D/F conflict)
Traditionally, we split b into parts:
b1) The merged result matches the content and mode found in HEAD
b2) The merged target path existed in HEAD
Steps a & b1 are easy to check; we have always gotten those right. While
it is easy to overlook step c, this was fixed seven years ago with commit
4ab9a157d069 ("merge_content(): Check whether D/F conflicts are still
present", 2010-09-20). merge-recursive didn't have a readily available
way to directly check step b2, so various approximations were used:
* In commit b2c8c0a76274 ("merge-recursive: When we detect we can skip
an update, actually skip it", 2011-02-28), it was noted that although
the code claimed it was skipping the update, it did not actually skip
the update. The code was made to skip it, but used lstat(path, ...)
as an approximation to path-was-tracked-in-index-before-merge.
* In commit 5b448b853030 ("merge-recursive: When we detect we can skip
an update, actually skip it", 2011-08-11), the problem with using
lstat was noted. It was changed to the approximation
path2 && strcmp(path, path2)
which is also wrong. !path2 || strcmp(path, path2) would have been
better, but would have fallen short with directory renames.
* In c5b761fb2711 ("merge-recursive: ensure we write updates for
directory-renamed file", 2018-02-14), the problem with the previous
approximation was noted and changed to
was_tracked(path)
That looks close to what we were trying to answer, but was_tracked()
as implemented at the time should have been named is_tracked(); it
returned something different than what we were looking for.
* To make matters more complex, fixing was_tracked() isn't sufficient
because the splitting of b into b1 and b2 is wrong. Consider the
following merge with a rename/add conflict:
side A: modify foo, add unrelated bar
side B: rename foo->bar (but don't modify the mode or contents)
In this case, the three-way merge of original foo, A's foo, and B's
bar will result in a desired pathname of bar with the same
mode/contents that A had for foo. Thus, A had the right mode and
contents for the file, and it had the right pathname present (namely,
bar), but the bar that was present was unrelated to the contents, so
the working tree update was not skippable.
Fix this by introducing a new function:
was_tracked_and_matches(o, path, &mfi.oid, mfi.mode)
and use it to directly check for condition b.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add several tests checking whether updates can be skipped in a merge.
Also add several similar testcases for where updates cannot be skipped in
a merge to make sure that we skip if and only if we should.
In particular:
* Testcase 1a (particularly 1a-check-L) would have pointed out the
problem Linus has been dealing with for year with his merges[1].
* Testcase 2a (particularly 2a-check-L) would have pointed out the
problem with my directory-rename-series before it broke master[2].
* Testcases 3[ab] (particularly 3a-check-L) provide a simpler testcase
than 12b of t6043 making that one easier to understand.
* There are several complementary testcases to make sure we're not just
fixing those particular issues while regressing in the opposite
direction.
* There are also a pair of tests for the special case when a merge
results in a skippable update AND the user has dirty modifications to
the path.
[1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/
[2] https://public-inbox.org/git/xmqqmuya43cs.fsf@gitster-ct.c.googlers.com/
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If a cherry-pick or merge with a rename results in a skippable update
(due to the merged content matching what HEAD already had), but the
working directory is dirty, avoid trying to refresh the index as that
will fail.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Four closely related changes all with the purpose of fixing error handling
in this function:
- fix reported function name in add_cacheinfo error messages
- differentiate between the two error messages
- abort early when we hit the error (stop ignoring return code)
- mark a test which was hitting this error as failing until we get the
right fix
In more detail...
In commit 0424138d5715 ("Fix bogus error message from merge-recursive
error path", 2007-04-01), it was noted that the name of the function which
the error message claimed it was reported from did not match the actual
function name. This was changed to something closer to the real function
name, but it still didn't match the actual function name. Fix the
reported name to match.
Second, the two errors in this function had identical messages, preventing
us from knowing which error had been triggered. Add a couple words to the
second error message to differentiate the two.
Next, make sure callers do not ignore the return code so that it will stop
processing further entries (processing further entries could result in
more output which could cause the error to scroll off the screen, or at
least be missed by the user) and make it clear the error is the cause of
the early abort. These errors should never be triggered in production; if
either one is, it represents a bug in the calling path somewhere and is
likely to have resulted in mis-merged content. The combination of
ignoring of the return code and continuing to print other standard
messages after hitting the error resulted in the following bug report from
Junio: "...the command pretends that everything went well and merged
cleanly in that path...[Behaving] in a buggy and unexplainable way is bad
enough, doing so silently is unexcusable." Fix this.
Finally, there was one test in the testsuite that did hit this error path,
but was passing anyway. This would have been easy to miss since it had a
test_must_fail and thus could have failed for the wrong reason, but in a
separate testing step I added an intentional NULL-dereference to the
codepath where these error messages are printed in order to flush out such
cases. I could modify that test to explicitly check for this error and
fail the test if it is hit, but since this test operates in a bit of a
gray area and needed other changes, I went for a different fix. The gray
area this test operates in is the following: If the merge of a certain
file results in the same version of the file that existed in HEAD, but
there are dirty modifications to the file, is that an error with a
"Refusing to overwrite existing file" expected, or a case where the merge
should succeed since we shouldn't have to touch the dirty file anyway?
Recent discussion on the list leaned towards saying it should be a
success. Therefore, change the expected behavior of this test to match.
As a side effect, this makes the failed-due-to-hitting-add_cacheinfo-error
very clear, and we can mark the test as test_expect_failure. A subsequent
commit will implement the necessary changes to get this test to pass
again.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If a file on one side of history was renamed, and merely modified on the
other side, then applying a directory rename to the modified side gives us
a rename/rename(1to2) conflict. We should only apply directory renames to
pairs representing either adds or renames.
Making this change means that a directory rename testcase that was
previously reported as a rename/delete conflict will now be reported as a
modify/delete conflict.
Reviewed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a testcase showing spurious rename/rename(1to2) conflicts occurring
due to directory rename detection.
Also add a pair of testcases dealing with moving directory hierarchies
around that were suggested by Stefan Beller as "food for thought" during
his review of an earlier patch series, but which actually uncovered a
bug. Round things out with a test that is a cross between the two
testcases that showed existing bugs in order to make sure we aren't
merely addressing problems in isolation but in general.
Reviewed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
