The code for rename_rename_2to1 conflicts (two files both being renamed to
the same filename) was dead since the rename/add path was always being
independently triggered for each of the renames instead. Further,
reviving the dead code showed that it was inherently buggy and would
always segfault -- among a few other bugs.
Move the else-if branch for the rename/rename block before the rename/add
block to make sure it is checked first, and fix up the rename/rename(2to1)
code segments to make it handle most cases. Work is still needed to
handle higher dimensional corner cases such as rename/rename/modify/modify
issues.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the recursive case (o->call_depth > 0), we do not modify the working
directory. However, when o->call_depth==0, file renames can mean we need
to delete the old filename from the working copy. Since there have been
lots of changes and mistakes here, let's go through the details. Let's
start with a simple explanation of what we are trying to achieve:
Original goal: If a file is renamed on the side of history being merged
into head, the filename serving as the source of that rename needs to be
removed from the working directory.
The path to getting the above statement implemented in merge-recursive took
several steps. The relevant bits of code may be instructive to keep in
mind for the explanation, especially since an English-only description
involves double negatives that are hard to follow. These bits of code are:
int remove_file(..., const char *path, int no_wd)
{
...
int update_working_directory = !o->call_depth && !no_wd;
and
remove_file(o, 1, ren1_src, <expression>);
Where the choice for <expression> has morphed over time:
65ac6e9 (merge-recursive: adjust to loosened "working file clobbered"
check 2006-10-27), introduced the "no_wd" parameter to remove_file() and
used "1" for <expression>. This meant ren1_src was never deleted, leaving
it around in the working copy.
In 8371234 (Remove uncontested renamed files during merge. 2006-12-13),
<expression> was changed to "index_only" (where index_only ==
!!o->call_depth; see b7fa51da). This was equivalent to using "0" for
<expression> (due to the early logic in remove_file), and is orthogonal to
the condition we actually want to check at this point; it resulted in the
source file being removed except when index_only was false. This was
problematic because the file could have been renamed on the side of history
including head, in which case ren1_src could correspond to an untracked
file that should not be deleted.
In 183d797 (Keep untracked files not involved in a merge. 2007-02-04),
<expression> was changed to "index_only || stage == 3". While this gives
correct behavior, the "index_only ||" portion of <expression> is
unnecessary and makes the code slightly harder to follow.
There were also two further changes to this expression, though without
any change in behavior. First in b7fa51d (merge-recursive: get rid of the
index_only global variable 2008-09-02), it was changed to "o->call_depth
|| stage == 3". (index_only == !!o->call_depth). Later, in 41d70bd6
(merge-recursive: Small code clarification -- variable name and comments),
this was changed to "o->call_depth || renamed_stage == 2" (where stage was
renamed to other_stage and renamed_stage == other_stage ^ 1).
So we ended with <expression> being "o->call_depth || renamed_stage == 2".
But the "o->call_depth ||" piece was unnecessary. We can remove it,
leaving us with <expression> being "renamed_stage == 2". This doesn't
change behavior at all, but it makes the code clearer. Which is good,
because it's about to get uglier.
Corrected goal: If a file is renamed on the side of history being merged
into head, the filename serving as the source of that rename needs to be
removed from the working directory *IF* that file is tracked in head AND
the file tracked in head is related to the original file.
Note that the only difference between the original goal and the corrected
goal is the two extra conditions added at the end. The first condition is
relevant in a rename/delete conflict. If the file was deleted on the
HEAD side of the merge and an untracked file of the same name was added to
the working copy, then without that extra condition the untracked file
will be erroneously deleted. This changes <expression> to "renamed_stage
== 2 || !was_tracked(ren1_src)".
The second additional condition is relevant in two cases.
The first case the second condition can occur is when a file is deleted
and a completely different file is added with the same name. To my
knowledge, merge-recursive has no mechanism for detecting deleted-and-
replaced-by-different-file cases, so I am simply punting on this
possibility.
The second case for the second condition to occur is when there is a
rename/rename/add-source conflict. That is, when the original file was
renamed on both sides of history AND the original filename is being
re-used by some unrelated (but tracked) content. This case also presents
some additional difficulties for us since we cannot currently detect these
rename/rename/add-source conflicts; as long as the rename detection logic
"optimizes" by ignoring filenames that are present at both ends of the
diff, these conflicts will go unnoticed. However, rename/rename conflicts
are handled by an entirely separate codepath not being discussed here, so
this case is not relevant for the line of code under consideration.
In summary:
Change <expression> from "o->call_depth || renamed_stage == 2" to
"renamed_stage == 2 || !was_tracked(ren1_src)", in order to remove
unnecessary code and avoid deleting untracked files.
96 lines of explanation in the changelog to describe a one-line fix...
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of having the process_renames logic update the stages in the index
for the rename destination, have the index updated after process_entry or
process_df_entry. This will also allow us to have process_entry determine
whether a file was tracked and existed in the working copy before the
merge started.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If there were several files conflicting below a directory corresponding
to a D/F conflict, and the file of that D/F conflict is in the way, we
want it to be removed. Since files of D/F conflicts are handled last,
they can be reinstated later and possibly with a new unique name.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teach the string-list API how to remove an entry in O(1) runtime by
moving the last entry to the vacated spot. As such, the routine works
only for unsorted lists.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Checking whether a filename was part of stage 0 or stage 2 is code that we
would like to be able to call from a few other places without also
lstat()-ing the file to see if it exists in the working copy.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rename make_room_for_directories_of_df_conflicts() to
record_df_conflict_files() to reflect the change in functionality.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Previously, we were using lstat() to determine if a directory was still
present after a merge (and thus in the way of adding a file). We should
have been using lstat() only to determine if untracked directories were in
the way (and then only when necessary to check for untracked directories);
we should instead using the index to determine if there is a tracked
directory in the way. Create a new function to do this and use it to
replace the existing checks for directories being in the way.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We cannot assume that directory/file conflicts will appear in sorted
order; for example, 'letters.txt' comes between 'letters' and
'letters/file'.
Thanks to Johannes for a pointer about qsort stability issues with
Windows and suggested code change.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a D/F conflict is introduced via an add/add conflict, when
o->call_depth > 0 we need to ensure that the higher stage entry from the
base stage is removed.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
make_room_for_directories_of_df_conflicts() is about making sure necessary
working directory changes can succeed. When o->call_depth > 0 (i.e. the
recursive case), we do not want to make any working directory changes so
this function should be skipped.
Note that make_room_for_directories_of_df_conflicts() is broken as has
been pointed out by Junio; it should NOT be unlinking files. What it
should do is keep track of files that could be unlinked if a directory
later needs to be written in their place. However, that work also is only
relevant in the non-recursive case, so this change is helpful either way.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We are only calling update_stages_options() one way really, so we can
consolidate the slightly different variants into one and remove some
parameters whose values are always the same.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Hopefully no one ever hits this error except when making large changes to
merge-recursive.c and debugging...
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is a testcase that was broken by b2c8c0a (merge-recursive: When we
detect we can skip an update, actually skip it 2011-02-28) and fixed by
6db4105 (Revert "Merge branch 'en/merge-recursive'" 2011-05-19). Include
this testcase to ensure we don't regress it again.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This testcase was part of en/merge-recursive that was reverted in 6db4105
(Revert "Merge branch 'en/merge-recursive'" 2011-05-19). While the other
changes in that series caused unfortunate breakage, this testcase is still
useful; reinstate it.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since this test later does a git add -A, we should clean out unnecessary
untracked files as part of our cleanup.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is another challenging testcase trying to exercise the virtual merge
base creation in the rename/rename(1to2) code. A testcase is added that
we should be able to merge cleanly, but which requires a virtual merge
base to be created that is aware of rename/rename(1to2)/add-source
conflicts and can handle those.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This test is mostly just designed for testing optimality of the virtual
merge base in the event of a rename/rename(1to2) conflict. The current
choice for resolving this in git seems somewhat confusing and suboptimal.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add testcases that cover three failures with current git merge, all
involving renaming one file on both sides of history:
Case 1:
If a single file is renamed to two different filenames on different sides
of history, there should be a conflict. Adding a new file on one of those
sides of history whose name happens to match the rename source should not
cause the merge to suddenly succeed.
Case 2:
If a single file is renamed on both sides of history but renamed
identically, there should not be a conflict. This works fine. However,
if one of those sides also added a new file that happened to match the
rename source, then that file should be left alone. Currently, the
rename/rename conflict handling causes that new file to become untracked.
Case 3:
If a single file is renamed to two different filenames on different sides
of history, there should be a conflict. This works currently. However,
if those renames also involve rename/add conflicts (i.e. there are new
files on one side of history that match the destination of the rename of
the other side of history), then the resulting conflict should be recorded
in the index, showing that there were multiple files with a given filename.
Currently, git silently discards one of file versions.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
rename/rename conflicts, both with one file being renamed to two different
files and with two files being renamed to the same file, should leave the
index and the working copy in a sane state with appropriate conflict
recording, auxiliary files, etc. Git seems to handle one of the two cases
alright, but has some problems with the two files being renamed to one
case. Add tests for both cases.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add testcases that cover a variety of merge issues with files being
renamed and modified on different sides of history, when there are
directories possibly conflicting with the rename location.
Case 1:
On one side of history, a file is modified and a new directory is added.
On the other side of history, the file is modified in a non-conflicting
way but is renamed to the location of the new directory.
Case 2:
[Same as case 1, but there is also a content conflict. In detail:]
On one side of history, a file is modified and a new directory is added.
On the other side of history, the file is modified in a conflicting way
and it is renamed to the location of the new directory.
Case 3:
[Similar to case 1, but the "conflicting" directory is the directory
where the file original resided. In detail:]
On one side of history, a file is modified. On the other side of history,
the file is modified in a non-conflicting way, but the directory it was
under is removed and the file is renamed to the location of the directory
it used to reside in (i.e. 'sub/file' gets renamed to 'sub'). This is
flagged as a directory/rename conflict, but should be able to be resolved
since the directory can be cleanly removed by the merge.
One branch renames a file and makes a file where the directory the renamed
file used to be in, and the other branch updates the file in
place. Merging them should resolve it cleanly as long as the content level
change on the branches do not overlap and rename is detected, or should
leave conflict without losing information.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There are cases where history should merge cleanly, and which current git
does merge cleanly despite not detecting a rename; however the merge
currently nukes files that should not be removed.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
An undetected rename can cause a silent success where a conflict should
have been detected, or can cause an erroneous conflict state where the
merge should have been resolvable. Add testcases for both.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If there is a cleanly resolvable rename/modify conflict AND there is a new
file introduced on the renamed side of the merge whose name happens to
match that of the source of the rename (but is otherwise unrelated to the
rename), then git fails to cleanly resolve the merge despite the fact that
the new file should not cause any problems.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Current git will nuke an untracked file during a rename/delete conflict if
(a) there is an untracked file whose name matches the source of a rename
and (b) the merge is done in a certain direction. Add a simple testcase
demonstrating this bug.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The following sequence of commands reveals an issue with error
reporting of relative paths:
$ mkdir sub
$ cd sub
$ git ls-files --error-unmatch ../bbbbb
error: pathspec 'b' did not match any file(s) known to git.
$ git commit --error-unmatch ../bbbbb
error: pathspec 'b' did not match any file(s) known to git.
This bug is visible only if the normalized path (i.e., the relative
path from the repository root) is longer than the prefix.
Otherwise, the code skips over the normalized path and reads from
an unused memory location which still contains a leftover of the
original command line argument.
So instead, use the existing facilities to deal with relative paths
correctly.
Also fix inconsistency between "checkout" and "commit", e.g.
$ cd Documentation
$ git checkout nosuch.txt
error: pathspec 'Documentation/nosuch.txt' did not match...
$ git commit nosuch.txt
error: pathspec 'nosuch.txt' did not match...
by propagating the prefix down the codepath that reports the error.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When running git describe --dirty the index should be refreshed. Previously
the cached index would cause describe to think that the index was dirty when,
in reality, it was just stale.
The issue was exposed by python setuptools which hardlinks files into another
directory when building a distribution.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As we seem to need this variable that holds a single LF character
in many places, define it in test-lib.sh and let the test scripts
use it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Git currently reports loose objects as 'corrupt' if they've been
deflated using a window size less than 32Kb, because the
experimental_loose_object() function doesn't recognise the header
byte as a zlib header. This patch makes the function tolerant of
all valid window sizes (15-bit to 8-bit) - but doesn't sacrifice
it's accuracy in distingushing the standard loose-object format
from the experimental (now abandoned) format.
On memory constrained systems zlib may use a much smaller window
size - working on Agit, I found that Android uses a 4KB window;
giving a header byte of 0x48, not 0x78. Consequently all loose
objects generated appear 'corrupt', which is why Agit is a read-only
Git client at this time - I don't want my client to generate Git
repos that other clients treat as broken :(
This patch makes Git tolerant of different deflate settings - it
might appear that it changes experimental_loose_object() to the point
where it could incorrectly identify the experimental format as the
standard one, but the two criteria (bitmask & checksum) can only
give a false result for an experimental object where both of the
following are true:
1) object size is exactly 8 bytes when uncompressed (bitmask)
2) [single-byte in-pack git type&size header] * 256
+ [1st byte of the following zlib header] % 31 = 0 (checksum)
As it happens, for all possible combinations of valid object type
(1-4) and window bits (0-7), the only time when the checksum will be
divisible by 31 is for 0x1838 - ie object type *1*, a Commit - which,
due the fields all Commit objects must contain, could never be as
small as 8 bytes in size.
Given this, the combination of the two criteria (bitmask & checksum)
always correctly determines the buffer format, and is more tolerant
than the previous version.
The alternative to this patch is simply removing support for the
experimental format, which I am also totally cool with.
References:
Android uses a 4KB window for deflation:
http://android.git.kernel.org/?p=platform/libcore.git;a=blob;f=luni/src/main/native/java_util_zip_Deflater.cpp;h=c0b2feff196e63a7b85d97cf9ae5bb2583409c28;hb=refs/heads/gingerbread#l53
Code snippet searching for false positives with the zlib checksum:
https://gist.github.com/1118177
Signed-off-by: Roberto Tyley <roberto.tyley@guardian.co.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some web-based email clients prepend whitespace to raw message
transcripts to workaround content-sniffing in some browsers. Adjust
the patch format detection logic to ignore leading whitespace.
So now you can apply patches from GMail with "git am" in three steps:
1. choose "show original"
2. tell the browser to "save as" (for example by pressing Ctrl+S)
3. run "git am" on the saved file
This fixes a regression introduced by v1.6.4-rc0~15^2~2 (git-am
foreign patch support: autodetect some patch formats, 2009-05-27).
GMail support was first introduced to "git am" by v1.5.4-rc0~274^2
(Make mailsplit and mailinfo strip whitespace from the start of the
input, 2007-11-01).
Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is useful to mark a submodule as unneeded by default. When this
option is set and the user wants to work with such a submodule he
needs to configure 'submodule.<name>.update=checkout' or pass the
--checkout option. Then the submodule can be handled like a normal
submodule.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Lets always initialize the 'update_module' variable with the final
value. This way we allow code which wants to check this configuration
early to do so right in the beginning of cmd_update().
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
fsck allows a name with > character in it like "name> <email>". Also for
"name email>" fsck says "missing space before email".
More precisely, it seeks for a first '<', checks that ' ' preceeds it.
Then seeks to '<' or '>' and checks that it is the '>'. Missing space is
reported if either '<' is not found or it's not preceeded with ' '.
Change it to following. Seek to '<' or '>', check that it is '<' and is
preceeded with ' '. Seek to '<' or '>' and check that it is '>'. So now
"name> <email>" is rejected as "bad name". More strict name check is the
only change in what is accepted.
Report 'missing space' only if '<' is found and is not preceeded with a
space.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
fsck reports "missing space before <email>" for committer string equal
to "name email>" or to "". It'd be nicer to say "missing email" for
the second string and "name is bad" (has > in it) for the first one.
Add a failing test for these messages.
For "name> <email>" no error is reported. Looks like a bug, so add
such a failing test."
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The documentation declares following identity format:
(<name> SP)? LT <email> GT
where name is any string without LF and LT characters.
But fast-import just accepts any string up to first GT
instead of checking the whole format, and moreover just
writes it as is to the commit object.
git-fsck checks for [^<\n]* <[^<>\n]*> format. Note that the
space is mandatory. And the space quirk is already handled via
extending the string to the left when needed.
Modify fast-import input identity format to a slightly stricter
one - deny LF, LT and GT in both <name> and <email>. And check
for it.
This is stricter then git-fsck as fsck accepts "Name> <email>"
currently, but soon fsck check will be adjusted likewise.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
fast-import format declares 'committer_name SP' to be optional in
'committer_name SP LT email GT'. But for a (commit) object SP is
obligatory while zero length committer_name is ok. git-fsck checks
that SP is present, so fast-import must prepend it if the name SP
part is omitted. It doesn't do so and thus for "LT email GT" ident
it writes a bad object.
Name cannot contain LT or GT, ident always comes after SP in fast-import.
So if ident starts with LT reuse the SP as if a valid 'SP LT email GT'
ident was passed.
This fixes a ident parsing bug for a well-formed fast-import input.
Though the parsing is still loose and can accept a ill-formed input.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Documentation/git-fast-import.txt says that git-fast-import is strict
about it's input format. But committer/author field parsing is a bit
loose. Invalid values can be unnoticed and written out to the commit,
either with format-conforming input or with non-format-conforming one.
Add one passing and one failing test for empty/absent committer name
with well-formed input. And a failed test with unnoticed ill-formed
input.
Reported-by: SASAKI Suguru <sss.sonik@gmail.com>
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently parse-options.o pulls quite a big bunch of dependencies.
his complicates it's usage in contrib/ because it pulls external
dependencies and it also increases executables size.
Split off less generic and more internal to git part of
parse-options.c to parse-options-cb.c.
Move prefix_filename function from setup.c to abspath.c. abspath.o
and wrapper.o pull each other, so it's unlikely to increase the
dependencies. It was a dependency of parse-options.o that pulled
many others.
Now parse-options.o pulls just abspath.o, ctype.o, strbuf.o, usage.o,
wrapper.o, libc directly and strlcpy.o indirectly.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
opterror and optbug functions are used by some of parsing routines
in parse-options.c to report errors and bugs respectively.
Export these functions to allow more custom parsing routines to use
them in a uniform way.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The call to test_expect_success is nested inside a function, whose
arguments the test code wants to access. But it is not specified that any
unexpanded $1, $2, $3, etc in the test code will access the surrounding
function's arguments. Rather, they will access the arguments of the
function that happens to eval the test code.
In this case, the reference is intended to supply '-m message' to a call of
'git commit --squash'. Remove it because -m is optional and the test case
does not check for it. There are tests in t7500 that check combinations of
--squash and -m.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
