I want to implement the same outward facing API as found within
merge-recursive.h in a different merge strategy. However, that makes
names like MERGE_RECURSIVE_{NORMAL,OURS,THEIRS} look a little funny;
rename to MERGE_VARIANT_{NORMAL,OURS,THEIRS}.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
merge_options has several internal fields that should not be set or read
by external callers. This just complicates the API. Move them into an
opaque merge_options_internal struct that is defined only in
merge-recursive.c and keep these out of merge-recursive.h.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If opt->buffer_output is less than 2, then merge_trees(),
merge_recursive(), and merge_recursive_generic() are all supposed to
flush the opt->obuf output buffer to stdout and release any memory it
holds. merge_trees() did not do this. Move the logic that handles this
for merge_recursive_internal() to merge_finalize() so that all three
methods handle this requirement.
Note that this bug didn't cause any problems currently, because there
are only two callers of merge_trees() right now (a git grep for
'merge_trees(' is misleading because builtin/merge-tree.c also defines a
'merge_tree' function that is unrelated), and only one of those is
called with buffer_output less than 2 (builtin/checkout.c), but it set
opt->verbosity to 0, for which there is only currently one non-error
message that would be shown: "Already up to date!". However, that one
message can only occur when the merge is utterly trivial (the merge base
tree exactly matches the merge tree), and builtin/checkout.c already
attempts a trivial merge via unpack_trees() before falling back to
merge_trees().
Also, if opt->buffer_output is 2, then the caller is responsible to
handle showing any output in opt->obuf and for free'ing it. This
requirement might be easy to overlook, so add a comment to
merge-recursive.h pointing it out. (There are currently two callers
that set buffer_output to 2, both in sequencer.c, and both of which
handle this correctly.)
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The merge_options struct had lots of fields, making it a little
imposing, but the options naturally fall into multiple different groups.
Grouping similar options and adding a comment or two makes it easier to
read, easier for new folks to figure out which options are related, and
thus easier for them to find the options they need.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We provided users with the ability to state whether they wanted rename
detection, and to put a limit on how much CPU would be spent. Both of
these fields had multiple configuration parameters for setting them,
with one being a fallback and the other being an override. However,
instead of implementing the logic for how to combine the multiple
source locations into the appropriate setting at config loading time,
we loaded and tracked both values and then made the code combine them
every time it wanted to check the overall value. This had a few
minor drawbacks:
* it seems more complicated than necessary
* it runs the risk of people using the independent settings in the
future and breaking the intent of how the options are used
together
* it makes merge_options more complicated than necessary for other
potential users of the API
Fix these problems by moving the logic for combining the pairs of
options into a single value; make it apply at time-of-config-loading
instead of each-time-of-use.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
No substantive code changes (view this with diff --color-moved), but
a few small code cleanups:
* Move structs and an inline function only used by merge-recursive.c
into merge-recursive.c
* Re-order function declarations to be more logical
* Add or fix some explanatory comments
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In commit 259ccb6cc3 ("merge-recursive: rename merge_options argument
from 'o' to 'opt'", 2019-04-05), I renamed a bunch of function
arguments in merge-recursive.c, but forgot to make that same change to
merge-recursive.h. Make the two match.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
It is not at all clear what 'mr' was supposed to stand for, at least not
to me. Pick a clearer name for this variable.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
merge_trees(), merge_recursive(), and merge_recursive_generic() in
their function headers used four different names for the merge base or
list of merge bases they were passed:
* 'common'
* 'ancestors'
* 'ca'
* 'base_list'
They were able to refer to it four different ways instead of only three
by using a different name in the signature for the .c file than the .h
file. Change all of these to 'merge_base' or 'merge_bases'.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
No substantive code change, just add some line breaks to fix lines that
have grown in length due to various refactorings. Most remaining lines
of excessive length in merge-recursive include error messages and it's
not clear that splitting those improves things.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
write_tree_from_memory() appeared to be a merge-recursive special that
basically duplicated write_index_as_tree(). The two have a different
signature, but the bigger difference was just that write_index_as_tree()
would always unconditionally read the index off of disk instead of
working on the current in-memory index. So:
* split out common code into write_index_as_tree_internal()
* rename write_tree_from_memory() to write_inmemory_index_as_tree(),
make it call write_index_as_tree_internal(), and move it to
cache-tree.c
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Alternatively, you can view this as "make the merge functions behave
more similarly." merge-recursive has three different entry points:
merge_trees(), merge_recursive(), and merge_recursive_generic(). Two of
these would call diff_warn_rename_limit(), but merge_trees() didn't.
This lead to callers of merge_trees() needing to manually call
diff_warn_rename_limit() themselves. Move this to the new
merge_finalize() function to make sure that all three entry points run
this function.
Note that there are two external callers of merge_trees(), one in
sequencer.c and one in builtin/checkout.c. The one in sequencer.c is
cleaned up by this patch and just transfers where the call to
diff_warn_rename_limit() is made; the one in builtin/checkout.c is for
switching to a different commit and in the very rare case where the
warning might be triggered, it would probably be helpful to include
(e.g. if someone is modifying a file that has been renamed in moving to
the other commit, but there are so many renames between the commits that
the limit kicks in and none are detected, it may help to have an
explanation about why they got a delete/modify conflict instead of a
proper content merge in a renamed file).
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
merge_trees() took a results parameter that would only be written when
opt->call_depth was positive, which is never the case now that
merge_trees_internal() has been split from merge_trees(). Remove the
misleading and unused parameter from merge_trees().
While at it, add some comments explaining how the output of
merge_trees() and merge_recursive() differ.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We had a rule to enforce that the index matches head, but it was found
at the beginning of merge_trees() and would only trigger when
opt->call_depth was 0. Since merge_recursive() doesn't call
merge_trees() until after returning from recursing, this meant that the
check wasn't triggered by merge_recursive() until it had first finished
all the intermediate merges to create virtual merge bases. That is a
potentially huge amount of computation (and writing of intermediate
merge results into the .git/objects directory) before it errors out and
says, in effect, "Sorry, I can't do any merging because you have some
local changes that would be overwritten."
Further, not enforcing this requirement earlier allowed other bugs (such
as an unintentional unconditional dropping and reloading of the index in
merge_recursive() even when no recursion was necessary), to mask bugs in
other callers (which were fixed in the commit prior to this one).
Make sure we do the index == head check at the beginning of the merge,
and error out immediately if it fails. While we're at it, fix a small
leak in the show-the-error codepath.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is the bug that just won't die; there always seems to be another
form of it somewhere. See the commit message of 55f39cf755 ("merge:
fix misleading pre-merge check documentation", 2018-06-30) for a more
detailed explanation), but in short:
<quick summary>
builtin/merge.c contains this important requirement for merge
strategies:
...the index must be in sync with the head commit. The strategies are
responsible to ensure this.
This condition is important to enforce because there are two likely
failure cases when the index isn't in sync with the head commit:
* we silently throw away changes the user had staged before the merge
* we accidentally (and silently) include changes in the merge that
were not part of either of the branches/trees being merged
Discarding users' work and mis-merging are both bad outcomes, especially
when done silently, so naturally this rule was stated sternly -- but,
unfortunately totally ignored in practice unless and until actual bugs
were found. But, fear not: the bugs from this were fixed in commit
ee6566e8d7 ("[PATCH] Rewrite read-tree", 2005-09-05)
through a rewrite of read-tree (again, commit 55f39cf755 has a more
detailed explanation of how this affected merge). And it was fixed
again in commit
160252f816 ("git-merge-ours: make sure our index matches HEAD", 2005-11-03)
...and it was fixed again in commit
3ec62ad9ff ("merge-octopus: abort if index does not match HEAD", 2016-04-09)
...and again in commit
65170c07d4 ("merge-recursive: avoid incorporating uncommitted changes in a merge", 2017-12-21)
...and again in commit
eddd1a411d ("merge-recursive: enforce rule that index matches head before merging", 2018-06-30)
...with multiple testcases added to the testsuite that could be
enumerated in even more commits.
Then, finally, in a patch in the same series as the last fix above, the
documentation about this requirement was fixed in commit 55f39cf755
("merge: fix misleading pre-merge check documentation", 2018-06-30), and
we all lived happily ever after...
</quick summary>
Unfortunately, "ever after" apparently denotes a limited time and it
expired today. The merge-recursive rule to enforce that index matches
head was at the beginning of merge_trees() and would only trigger when
opt->call_depth was 0. Since merge_recursive() doesn't call
merge_trees() until after returning from recursing, this meant that the
check wasn't triggered by merge_recursive() until it had first finished
all the intermediate merges to create virtual merge bases. That is a
potentially HUGE amount of computation (and writing of intermediate
merge results into the .git/objects directory) before it errors out and
says, in effect, "Sorry, I can't do any merging because you have some
local changes that would be overwritten."
Trying to enforce that all of merge_trees(), merge_recursive(), and
merge_recursive_generic() checked the index == head condition earlier
resulted in a bunch of broken tests. It turns out that
merge_recursive() has code to drop and reload the cache while recursing
to create intermediate virtual merge bases, but unfortunately that code
runs even when no recursion is necessary. This unconditional dropping
and reloading of the cache masked a few bugs:
* builtin/merge-recursive.c: didn't even bother loading the index.
* builtin/stash.c: feels like a fake 'builtin' because it repeatedly
invokes git subprocesses all over the place, mixed with other
operations. In particular, invoking "git reset" will reset the
index on disk, but the parent process that invoked it won't
automatically have its in-memory index updated.
* t3030-merge-recursive.h: this test has always been broken in that it
didn't make sure to make index match head before running. But, it
didn't care about the index or even the merge result, just the
verbose output while running. While commit eddd1a411d
("merge-recursive: enforce rule that index matches head before
merging", 2018-06-30) should have uncovered this broken test, it
used a test_must_fail wrapper around the merge-recursive call
because it was known that the merge resulted in a rename/rename
conflict. Thus, that fix only made this test fail for a different
reason, and since the index == head check didn't happen until after
coming all the way back out of the recursion, the testcase had
enough information to pass the one check that it did perform.
So, load the index in builtin/merge-recursive.c, reload the in-memory
index in builtin/stash.c, and modify the t3030 testcase to correctly
setup the index and make sure that the test fails in the expected way
(meaning it reports a rename/rename conflict). This makes sure that
all callers actually make the index match head. The next commit will
then enforce the condition that index matches head earlier so this
problem doesn't return in the future.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit d7cf3a96e9 ("merge-recursive.c: remove implicit dependency on
the_repository", 2019-01-12) and follow-ups like commit 34e7771bc6
("Use the right 'struct repository' instead of the_repository",
2019-06-27), removed most implicit uses of the_repository. Convert
calls to get_commit_tree() to instead use repo_get_commit_tree() to get
rid of another.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There is a 'free_buf' label to which all but one of the error paths in
update_file_flags() jump; that error case involves a NULL buf and is
thus not a memory leak. However, make that error case execute the same
deallocation code anyway so that if anyone adds any additional memory
allocations or deallocations, then all error paths correctly deallocate
resources.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Improve code readability by introducing an enum to replace the
not-quite-boolean values taken on by detect_directory_renames.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In commit 7ca56aa076 ("merge-recursive: add a label for ancestor",
2010-03-20), a label was added for the '||||||' line to make it have
the more informative heading '|||||| merged common ancestors', with
the statement:
It would be nicer to use a more informative label. Perhaps someone
will provide one some day.
This chosen label was perfectly reasonable when recursiveness kicks in,
i.e. when there are multiple merge bases. (I can't think of a better
label in such cases.) But it is actually somewhat misleading when there
is a unique merge base or no merge base. Change this based on the
number of merge bases:
>=2: "merged common ancestors"
1: <abbreviated commit hash>
0: "<empty tree>"
Tests have also been added to check that we get the right ancestor name
for each of the three cases.
Also, since merge_recursive() and merge_trees() have polar opposite
pre-conditions for opt->ancestor, document merge_recursive()'s
pre-condition with an assertion. (An assertion was added to
merge_trees() already a few commits ago.) The differences in
pre-conditions stem from two factors: (1) merge_trees() does not recurse
and thus does not have multiple sub-merges to worry about -- each of
which would require a different value for opt->ancestor, (2)
merge_trees() is only passed trees rather than commits and thus cannot
internally guess as good of a label. Thus, while external callers of
merge_trees() are required to provide a non-NULL opt->ancestor,
merge_recursive() expects to set this value itself.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We always want our conflict hunks to be labelled so that users can know
where each came from. The previous commit fixed the one caller in the
codebase which was not setting opt->ancestor (and thus not providing a
label for the "merge base" conflict hunk in diff3-style conflict
markers); add an assertion to prevent future codepaths from also
overlooking this requirement.
Enforcing this requirement also allows us to simplify the code for
labelling the conflict hunks by no longer checking if the ancestor label
is NULL.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When running 'git checkout -m' and using diff3 style conflict markers,
we want all the conflict hunks (left-side, "common" or "merge base", and
right-side) to have label markers letting the user know where each came
from. The "common" hunk label (o.ancestor) came from
old_branch_info->name, but that is NULL when HEAD is detached, which
resulted in a blank label. Check for that case and provide an
abbreviated commit hash instead.
(Incidentally, this was the only case in the git codebase where
merge_trees() was called with opt->ancestor being NULL. A subsequent
commit will prevent similar problems by enforcing that merge_trees()
always be called with opt->ancestor != NULL.)
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In commit 8daec1df03 ("merge-recursive: switch from (oid,mode) pairs
to a diff_filespec", 2019-04-05), an assertion on a->path && b->path
was added for code readability to document that these both needed to be
non-NULL at this point in the code. However, the subsequent lines also
read o->path, so it should be included in the assert.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Inspired by 21416f0a07 ("restore: fix typo in docs", 2019-08-03), I ran
"git grep -E '(\b[a-zA-Z]+) \1\b' -- Documentation/" to find other cases
where words were duplicated, e.g. "the the", and in most cases removed
one of the repeated words.
There were many false positives by this grep command, including
deliberate repeated words like "really really" or valid uses of "that
that" which I left alone, of course.
I also did not correct any of the legitimate, accidentally repeated
words in old RelNotes.
Signed-off-by: Mark Rushakoff <mark.rushakoff@gmail.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
My IEE 'home for life' email service is being withdrawn on 30 Sept 2019.
Replace with my new email domain.
I also have a secondary (backup) 'home for life' through
<philipoakley@dunelm.org.uk>.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Philip Oakley <philipoakley@iee.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
-----BEGIN PGP SIGNATURE-----
iQIzBAABCAAdFiEE4fA2sf7nIh/HeOzvsLXohpav5ssFAl1NqkwACgkQsLXohpav
5ssjXQ//VsqRnuVu947TP0x/3vJzAuLSsTW1qE4kJUNQbRCRz64ejSiKiVlfDtpb
yk4rWbdnVVVCZwCUCNp421SAKWVWuFvEDhqd6JMe69DF1MqOwdn7gVleRiVa59Sv
2aVMzCO0FcRwUxuSkHogExJp94z2kyzL6EdAVYyalU8InR54cBML+in+gqtWToXE
7gGzAyu6g2Dv7/Wx2laohm05xppvbgsnrGqZvMhoYR1rl5pf9LlERvS/CjNl4FBc
mFqhJYgYjjvWfVPmv7WSce1JxlGd/AdDK0eMl6rnorHwSfDbeNsmvDT5a62YioQ7
9eC2/2woRom5T56NuEwobMYhpEG7ttlZDHEDg0YULSW7gbaNJdEoYJ78T0p7yQL3
HIljlg2H+l/I2wxeiMDg50oLCIWptT8d0E9TkEX89UkLq8Lc0XQeA7oxIM8HpjQy
n/Zx7sfDE4DVd7mFZb9UmzvHpzwXKl1NEy9a2/Mb7gRIUwO1DEHL8ATjar+j3AbO
uh3vOShC4u1Ya1vUOY7wRbmxfxIGIRiqRHtEmx60j1GCSDQMl71fTyO/QnAi71KH
CNzBRWauiyuJqwXQfzhZzXKLBDjfufoPudVHlWm0UC5oY3MXuLv9jUH6JAoaRP1U
46gauPfLinOeB0XQ4Uo3xbHJ6j2e91BLt2TzyQMMz0n6upGM9QE=
=hE45
-----END PGP SIGNATURE-----
Merge tag 'v2.23.0-rc2' of git://git.kernel.org/pub/scm/git/git
Git 2.23-rc2
* tag 'v2.23.0-rc2' of git://git.kernel.org/pub/scm/git/git: (63 commits)
Git 2.23-rc2
t0000: reword comments for "local" test
t: decrease nesting in test_oid_to_path
sha1-file: release strbuf after use
test-dir-iterator: use path argument directly
dir-iterator: release strbuf after use
commit-graph: release strbufs after use
l10n: reformat some localized strings for v2.23.0
merge-recursive: avoid directory rename detection in recursive case
commit-graph: fix bug around octopus merges
restore: fix typo in docs
doc: typo: s/can not/cannot/ and s/is does/does/
Git 2.23-rc1
log: really flip the --mailmap default
RelNotes/2.23.0: fix a few typos and other minor issues
RelNotes/2.21.1: typofix
log: flip the --mailmap default unconditionally
config: work around bug with includeif:onbranch and early config
A few more last-minute fixes
repack: simplify handling of auto-bitmaps and .keep files
...
Compilation fix.
* cb/xdiff-no-system-includes-in-dot-c:
xdiff: remove duplicate headers from xpatience.c
xdiff: remove duplicate headers from xhistogram.c
xdiff: drop system includes in xutils.c
The internal diff machinery can be made to read out of bounds while
looking for --funcion-context line in a corner case, which has been
corrected.
* jk/xdiff-clamp-funcname-context-index:
xdiff: clamp function context indices in post-image
