| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
"git repack" in a shallow clone did not correctly update the
shallow points in the repository, leading to a repository that
does not pass fsck.
* js/shallow-and-fetch-prune:
repack -ad: prune the list of shallow commits
shallow: offer to prune only non-existing entries
repack: point out a bug handling stale shallow info
|
|
The receive.denyCurrentBranch=updateInstead codepath kicked in even
when the push should have been rejected due to other reasons, such
as it does not fast-forward or the update-hook rejects it, which
has been corrected.
* jc/receive-deny-current-branch-fix:
receive: denyCurrentBranch=updateinstead should not blindly update
|
|
Under certain circumstances, "git diff D:/a/b/c D:/a/b/d" on
Windows would strip initial parts from the paths because they
were not recognized as absolute, which has been corrected.
* js/diff-notice-has-drive-prefix:
diff: don't attempt to strip prefix from absolute Windows paths
|
|
A mutex used in "git pack-objects" were not correctly initialized
and this caused "git repack" to dump core on Windows.
* js/pack-objects-mutex-init-fix:
pack-objects (mingw): initialize `packing_data` mutex in the correct spot
pack-objects (mingw): demonstrate a segmentation fault with large deltas
pack-objects: fix typo 'detla' -> 'delta'
|
|
The implementation of run_command() API on the UNIX platforms had a
bug that caused a command not on $PATH to be found in the current
directory.
* jk/run-command-notdot:
run-command: mark path lookup errors with ENOENT
|
|
"git log --graph" showing an octopus merge sometimes miscounted the
number of display columns it is consuming to show the merge and its
parent commits, which has been corrected.
* np/log-graph-octopus-fix:
log: fix coloring of certain octopus merge shapes
|
|
The codepath to support the experimental split-index mode had
remaining "racily clean" issues fixed.
* sg/split-index-racefix:
split-index: BUG() when cache entry refers to non-existing shared entry
split-index: smudge and add racily clean cache entries to split index
split-index: don't compare cached data of entries already marked for split index
split-index: count the number of deleted entries
t1700-split-index: date back files to avoid racy situations
split-index: add tests to demonstrate the racy split index problem
t1700-split-index: document why FSMONITOR is disabled in this test script
|
|
A partial clone that is configured to lazily fetch missing objects
will on-demand issue a "git fetch" request to the originating
repository to fill not-yet-obtained objects. The request has been
optimized for requesting a tree object (and not the leaf blob
objects contained in it) by telling the originating repository that
no blobs are needed.
* jt/non-blob-lazy-fetch:
fetch-pack: exclude blobs when lazy-fetching trees
fetch-pack: avoid object flags if no_dependents
|
|
A corner-case bugfix.
* sm/show-superproject-while-conflicted:
rev-parse: --show-superproject-working-tree should work during a merge
|
|
The code in "git status" sometimes hit an assertion failure. This
was caused by a structure that was reused without cleaning the data
used for the first run, which has been corrected.
* en/status-multiple-renames-to-the-same-target-fix:
commit: fix erroneous BUG, 'multiple renames on the same target? how?'
|
|
The mailmap file update.
* jn/mailmap-update:
mailmap: consistently normalize brian m. carlson's name
|
|
The recently introduced commit-graph auxiliary data is incompatible
with mechanisms such as replace & grafts that "breaks" immutable
nature of the object reference relationship. Disable optimizations
based on its use (and updating existing commit-graph) when these
incompatible features are in use in the repository.
* ds/commit-graph-with-grafts:
commit-graph: close_commit_graph before shallow walk
commit-graph: not compatible with uninitialized repo
commit-graph: not compatible with grafts
commit-graph: not compatible with replace objects
test-repository: properly init repo
commit-graph: update design document
refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
refs.c: migrate internal ref iteration to pass thru repository argument
|
|
Recently added "range-diff" had a corner-case bug to cause it
segfault, which has been corrected.
* tg/range-diff-corner-case-fix:
linear-assignment: fix potential out of bounds memory access
|
|
"git update-ref" learned to make both "--no-deref" and "--stdin"
work at the same time.
* en/update-ref-no-deref-stdin:
update-ref: allow --no-deref with --stdin
update-ref: fix type of update_flags variable to match its usage
|
|
Update error messages given by "git remote" and make them consistent.
* ms/remote-error-message-update:
builtin/remote: quote remote name on error to display empty name
|
|
The code to backfill objects in lazily cloned repository did not
work correctly, which has been corrected.
* jt/lazy-object-fetch-fix:
fetch-object: set exact_oid when fetching
fetch-object: unify fetch_object[s] functions
|
|
"git rebase" etc. in Git 2.19 fails to abort when given an empty
commit log message as result of editing, which has been corrected.
* en/sequencer-empty-edit-result-aborts:
sequencer: fix --allow-empty-message behavior, make it smarter
|
|
"git add ':(attr:foo)'" is not supported and is supposed to be
rejected while the command line arguments are parsed, but we fail
to reject such a command line upfront.
* nd/attr-pathspec-fix:
add: do not accept pathspec magic 'attr'
|
|
A corner case bugfix in "git rerere" code.
* en/rerere-multi-stage-1-fix:
rerere: avoid buffer overrun
t4200: demonstrate rerere segfault on specially crafted merge
|
|
Further fix for O_APPEND emulation on Windows
* js/mingw-o-append:
mingw: fix mingw_open_append to work with named pipes
t0051: test GIT_TRACE to a windows named pipe
|
|
Fix for a long-standing bug that leaves the index file corrupt when
it shrinks during a partial commit.
* jk/reopen-tempfile-truncate:
reopen_tempfile(): truncate opened file
|
|
When fsmonitor is in use, after operation on submodules updates
.gitmodules, we lost track of the fact that we did so and relied on
stale fsmonitor data.
* bp/mv-submodules-with-fsmonitor:
git-mv: allow submodules and fsmonitor to work together
|
|
"git rebase -i" did not clear the state files correctly when a run
of "squash/fixup" is aborted and then the user manually amended the
commit instead, which has been corrected.
* js/rebase-i-autosquash-fix:
rebase -i: be careful to wrap up fixup/squash chains
rebase -i --autosquash: demonstrate a problem skipping the last squash
|
|
"git interpret-trailers" and its underlying machinery had a buggy
code that attempted to ignore patch text after commit log message,
which triggered in various codepaths that will always get the log
message alone and never get such an input.
* jk/trailer-fixes:
append_signoff: use size_t for string offsets
sequencer: ignore "---" divider when parsing trailers
pretty, ref-filter: format %(trailers) with no_divider option
interpret-trailers: allow suppressing "---" divider
interpret-trailers: tighten check for "---" patch boundary
trailer: pass process_trailer_opts to trailer_info_get()
trailer: use size_t for iterating trailer list
trailer: use size_t for string offsets
|
|
`git repack` can drop unreachable commits without further warning,
making the corresponding entries in `.git/shallow` invalid, which causes
serious problems when deepening the branches.
One scenario where unreachable commits are dropped by `git repack` is
when a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.
Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
packed in the first place, which is an assumption at least some of Git's
code seems to make).
This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
unreachable, it is not a problem, but if they go missing, it *is* a
problem. One symptom of this problem is that a deepening fetch may now
fail with
fatal: error in object: unshallow <commit-hash>
To avoid this problem, let's prune the shallow list in `git repack` when
the `-d` option is passed, unless `-A` is passed, too (which would force
the now-unreachable objects to be turned into loose objects instead of
being deleted). Additionally, we also need to take `--keep-reachable`
and `--unpack-unreachable=<date>` into account.
Note: an alternative solution discussed during the review of this patch
was to teach `git fetch` to simply ignore entries in .git/shallow if the
corresponding commits do not exist locally. A quick test, however,
revealed that the .git/shallow file is written during a shallow *clone*,
in which case the commits do not exist, either, but the "shallow" line
*does* need to be sent. Therefore, this approach would be a lot more
finicky than the approach presented by the this patch.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
The `prune_shallow()` function wants a full reachability check to be
completed before it goes to work, to ensure that all unreachable entries
are removed from the shallow file.
However, in the upcoming patch we do not even want to go that far. We
really only need to remove entries corresponding to pruned commits, i.e.
to commits that no longer exist.
Let's support that use case.
Rather than extending the signature of `prune_shallow()` to accept
another Boolean, let's turn it into a bit field and declare constants,
for readability.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
A `git fetch --prune` can turn previously-reachable objects unreachable,
even commits that are in the `shallow` list. A subsequent `git repack
-ad` will then unceremoniously drop those unreachable commits, and the
`shallow` list will become stale. This means that when we try to fetch
with a larger `--depth` the next time, we may end up with:
fatal: error in object: unshallow <commit-hash>
Reported by Alejandro Pauly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
|
|
Since commit e3a434468f (run-command: use the
async-signal-safe execv instead of execvp, 2017-04-19),
prepare_cmd() does its own PATH lookup for any commands we
run (on non-Windows platforms).
However, its logic does not match the old execvp call when
we fail to find a matching entry in the PATH. Instead of
feeding the name directly to execv, execvp would consider
that an ENOENT error. By continuing and passing the name
directly to execv, we effectively behave as if "." was
included at the end of the PATH. This can have confusing and
even dangerous results.
The fix itself is pretty straight-forward. There's a new
test in t0061 to cover this explicitly, and I've also added
a duplicate of the ENOENT test to ensure that we return the
correct errno for this case.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
git diff can be invoked with absolute paths. Typically, this triggers
the --no-index case. Then the absolute paths remain in the file names
that are printed in the output.
There is one peculiarity, though: When the command is invoked from a
a sub-directory in a repository, then it is attempted to strip the
sub-directory from the beginning of relative paths. Yet, to detect a
relative path the code just checks for an initial forward slash.
This mistakes a Windows style path like "D:/base" as a relative path
and the output looks like this, for example:
D:\dir\test\one>git -P diff --numstat D:\dir\base D:\dir\diff
1 1 ir/{base => diff}/1.txt
where the correct output should be
D:\dir\test\one>git -P diff --numstat D:\dir\base D:\dir\diff
1 1 D:/dir/{base => diff}/1.txt
If the sub-directory where 'git diff' is invoked is sufficiently deep
that the prefix becomes longer than the path to be printed, then the
subsequent code accesses the path out of bounds.
Use is_absolute_path() to detect Windows style absolute paths.
One might wonder whether the check for a directory separator that
is visible in the patch context should be changed from == '/' to
is_dir_sep() or not. It turns out not to be necessary. That code
only ever investigates paths that have undergone pathspec
normalization, after which there are only forward slashes even on
Windows.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
The handling of receive.denyCurrentBranch=updateInstead was added to
a switch statement that handles other values of the variable, but
all the other case arms only checked a condition to reject the
attempted push, or let later logic in the same function to still
intervene, so that a push that does not fast-forward (which is
checked after the switch statement in question) is still rejected.
But the handling of updateInstead incorrectly took immediate effect,
without giving other checks a chance to intervene.
Instead of calling update_worktree() that causes the side effect
immediately, just note the fact that we will need to call the
function later, and first give other checks a chance to reject the
request. After the update-hook gets a chance to reject the push
(which happens as the last step in a series of checks), call
update_worktree() when we earlier detected the need to.
Reported-by: Rajesh Madamanchi
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
In 9ac3f0e5b3e4 (pack-objects: fix performance issues on packing large
deltas, 2018-07-22), a mutex was introduced that is used to guard the
call to set the delta size. This commit even added code to initialize
it, but at an incorrect spot: in `init_threaded_search()`, while the
call to `oe_set_delta_size()` (and hence to `packing_data_lock()`) can
happen in the call chain `check_object()` <- `get_object_details()` <-
`prepare_pack()` <- `cmd_pack_objects()`, which is long before the
`prepare_pack()` function calls `ll_find_deltas()` (which initializes
the threaded search).
Another tell-tale that the mutex was initialized in an incorrect spot is
that the function to initialize it lives in builtin/, while the code
that uses the mutex is defined in a libgit.a header file.
Let's use a more appropriate function: `prepare_packing_data()`, which
not only lives in libgit.a, but *has* to be called before the
`packing_data` struct is used that contains that mutex.
This fixes https://github.com/git-for-windows/git/issues/1839.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
There is a problem in the way 9ac3f0e5b3e4 (pack-objects: fix
performance issues on packing large deltas, 2018-07-22) initializes that
mutex in the `packing_data` struct. The problem manifests in a
segmentation fault on Windows, when a mutex (AKA critical section) is
accessed without being initialized. (With pthreads, you apparently do
not really have to initialize them?)
This was reported in https://github.com/git-for-windows/git/issues/1839.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
For octopus merges where the first parent edge immediately merges into
the next column to the left, the number of columns should be one less
than the usual case.
First parent to the left case:
| *-.
| |\ \
|/ / /
The usual case:
| *-.
| |\ \
| | | *
Also refactor the code to iterate over columns rather than dashes,
building from an initial patch suggested by Jeff King.
Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
When the split index feature is in use, then a cache entry is:
- either only present in the split index, in which case its 'index'
field must be 0,
- or it should refer to an existing entry in the shared index, i.e.
the 'index' field can't be greater than the size of the shared
index.
If a cache entry were to refer to a non-existing entry in the shared
index, then that's a sign of something being wrong in the index state,
either as a result of a bug in dealing with the split/shared index
entries, or perhaps a (potentially unrelated) memory corruption issue.
prepare_to_write_split_index() already has a condition to catch cache
entries with such bogus 'index' field, but instead of calling BUG() it
just sets cache entry's 'index = 0', and the entry will then be
written to the new split index.
Don't write a new index file from bogus index state, and call BUG()
upon encountering an cache entry referring to a non-existing shared
index entry.
Running the test suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes'
doesn't trigger this condition.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Ever since the split index feature was introduced [1], refreshing a
split index is prone to a variant of the classic racy git problem.
Consider the following sequence of commands updating the split index
when the shared index contains a racily clean cache entry, i.e. an
entry whose cached stat data matches with the corresponding file in
the worktree and the cached mtime matches that of the index:
echo "cached content" >file
git update-index --split-index --add file
echo "dirty worktree" >file # size stays the same!
# ... wait ...
git update-index --add other-file
Normally, when a non-split index is updated, then do_write_index()
(the function responsible for writing all kinds of indexes, "regular",
split, and shared) recognizes racily clean cache entries, and writes
them with smudged stat data, i.e. with file size set to 0. When
subsequent git commands read the index, they will notice that the
smudged stat data doesn't match with the file in the worktree, and
then go on to check the file's content and notice its dirtiness.
In the above example, however, in the second 'git update-index'
prepare_to_write_split_index() decides which cache entries stored only
in the shared index should be replaced in the new split index. Alas,
this function never looks out for racily clean cache entries, and
since the file's stat data in the worktree hasn't changed since the
shared index was written, it won't be replaced in the new split index.
Consequently, do_write_index() doesn't even get this racily clean
cache entry, and can't smudge its stat data. Subsequent git commands
will then see that the index has more recent mtime than the file and
that the (not smudged) cached stat data still matches with the file in
the worktree, and, ultimately, will erroneously consider the file
clean.
Modify prepare_to_write_split_index() to recognize racily clean cache
entries, and mark them to be added to the split index. Note that
there are two places where it should check raciness: first those cache
entries that are only stored in the shared index, and then those that
have been copied by unpack_trees() from the shared index while it
constructed a new index. This way do_write_index() will get these
racily clean cache entries as well, and will then write them with
smudged stat data to the new split index.
This change makes all tests in 't1701-racy-split-index.sh' pass, so
flip the two 'test_expect_failure' tests to success. Also add the '#'
(as in nr. of trial) to those tests' description that were omitted
when the tests expected failure.
Note that after this change if the index is split when it contains a
racily clean cache entry, then a smudged cache entry will be written
both to the new shared and to the new split indexes. This doesn't
affect regular git commands: as far as they are concerned this is just
an entry in the split index replacing an outdated entry in the shared
index. It did affect a few tests in 't1700-split-index.sh', though,
because they actually check which entries are stored in the split
index; a previous patch in this series has already made the necessary
adjustments in 't1700'. And racily clean cache entries and index
splitting are rare enough to not worry about the resulting duplicated
smudged cache entries, and the additional complexity required to
prevent them is not worth it.
Several tests failed occasionally when the test suite was run with
'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace
back to this racy split index problem, starting with those failing
more frequently, with a link to a failing Travis CI build job for
each. The highlighted line [2] shows when the racy file was written,
which is not always in the failing test but in a preceeding setup
test.
t3903-stash.sh:
https://travis-ci.org/git/git/jobs/385542084#L5858
t4024-diff-optimize-common.sh:
https://travis-ci.org/git/git/jobs/386531969#L3174
t4015-diff-whitespace.sh:
https://travis-ci.org/git/git/jobs/360797600#L8215
t2200-add-update.sh:
https://travis-ci.org/git/git/jobs/382543426#L3051
t0090-cache-tree.sh:
https://travis-ci.org/git/git/jobs/416583010#L3679
There might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and
others using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet.
[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge
branch 'nd/split-index', 2014-07-16).
[2] Note that those highlighted lines are in the 'after failure' fold,
and your browser might unhelpfully fold it up before you could
take a good look.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
When unpack_trees() constructs a new index, it copies cache entries
from the original index [1]. prepare_to_write_split_index() has to
deal with this, and it has a dedicated code path for copied entries
that are present in the shared index, where it compares the cached
data in the corresponding copied and original entries. If the cached
data matches, then they are considered the same; if it differs, then
the copied entry will be marked for inclusion as a replacement entry
in the just about to be written split index by setting the
CE_UPDATE_IN_BASE flag.
However, a cache entry already has its CE_UPDATE_IN_BASE flag set upon
reading the split index, if the entry already has a replacement entry
there, or upon refreshing the cached stat data, if the corresponding
file was modified. The state of this flag is then preserved when
unpack_trees() copies a cache entry from the shared index.
So modify prepare_to_write_split_index() to check the copied cache
entries' CE_UPDATE_IN_BASE flag first, and skip the thorough
comparison of cached data if the flag is already set. Those couple of
lines comparing the cached data would then have too many levels of
indentation, so extract them into a helper function.
Note that comparing the cached data in copied and original entries in
the shared index might actually be entirely unnecessary. In theory
all code paths refreshing the cached stat data of an entry in the
shared index should set the CE_UPDATE_IN_BASE flag in that entry, and
unpack_trees() should preserve this flag when copying cache entries.
This means that the cached data is only ever changed if the
CE_UPDATE_IN_BASE flag is set as well. Our test suite seems to
confirm this: instrumenting the conditions in question and running the
test suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes' showed that the
cached data in a copied entry differs from the data in the shared
entry only if its CE_UPDATE_IN_BASE flag is indeed set.
In practice, however, our test suite doesn't have 100% coverage,
GIT_TEST_SPLIT_INDEX is inherently random, and I certainly can't claim
to possess complete understanding of what goes on in unpack_trees()...
Therefore I kept the comparison of the cached data when
CE_UPDATE_IN_BASE is not set, just in case that an unnoticed or future
code path were to accidentally miss setting this flag upon refreshing
the cached stat data or unpack_trees() were to drop this flag while
copying a cache entry.
[1] Note that when unpack_trees() constructs the new index and decides
that a cache entry should now refer to different content than what
was recorded in the original index (e.g. 'git read-tree -m
HEAD^'), then that can't really be considered a copy of the
original, but rather the creation of a new entry. Notably and
pertinent to the split index feature, such a new entry doesn't
have a reference to the original's shared index entry anymore,
i.e. its 'index' field is set to 0. Consequently, such an entry
is treated by prepare_to_write_split_index() as an entry not
present in the shared index and it will be added to the new split
index, while the original entry will be marked as deleted, and
neither the above discussion nor the changes in this patch apply
to them.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
'struct split_index' contains the field 'nr_deletions', whose name
with the 'nr_' prefix suggests that it contains the number of deleted
cache entries. However, barring its initialization to 0, this field
is only ever set to 1, indicating that there is at least one deleted
entry, but not the number of deleted entries. Luckily, this doesn't
cause any issues (other than confusing the reader, that is), because
the only place reading this field uses it in the same sense, i.e.: 'if
(si->nr_deletions)'.
To avoid confusion, we could either rename this field to something
like 'has_deletions' to make its name match its role, or make it a
counter of deleted cache entries to match its name.
Let's make it a counter, to keep it in sync with the related field
'nr_replacements', which does contain the number of replaced cache
entries. This will also give developers debugging the split index
code easy access to the number of deleted cache entries.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
't1700-split-index.sh' checks that the index was split correctly under
various circumstances and that all the different ways to turn the
split index feature on and off work correctly. To do so, most of its
tests use 'test-tool dump-split-index' to see which files have their
cache entries in the split index. All these tests assume that all
cache entries are written to the shared index (called "base"
throughout these tests) when a new shared index is created. This is
an implementation detail: most git commands (basically all except 'git
update-index') don't care or know at all about split index or whether
a cache entry is stored in the split or shared index.
As demonstrated in the previous patch, refreshing a split index is
prone to a variant of the classic racy git issue. The next patch will
fix this issue, but while doing so it will also slightly change this
behaviour: only cache entries with mtime in the past will be written
only to the newly created shared index, but racily clean cache entries
will be written to the new split index (with smudged stat data).
While this upcoming change won't at all affect any git commands, it
will violate the above mentioned assumption of 't1700's tests. Since
these tests create or modify files and create or refresh the split
index in rapid succession, there are plenty of racily clean cache
entries to be dealt with, which will then be written to the new split
indexes, and, ultimately, will cause several tests in 't1700' to fail.
Let's prepare 't1700-split-index.sh' for this upcoming change and
modify its tests to avoid racily clean files by backdating the mtime
of any file modifications (and since a lot of tests create or modify
files, encapsulate it into a helper function).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Ever since the split index feature was introduced [1], refreshing a
split index is prone to a variant of the classic racy git problem.
There are a couple of unrelated tests in the test suite that
occasionally fail when run with 'GIT_TEST_SPLIT_INDEX=yes', but
't1700-split-index.sh', the only test script focusing solely on split
index, has never noticed this issue, because it only cares about how
the index is split under various circumstances and all the different
ways to turn the split index feature on and off.
Add a dedicated test script 't1701-racy-split-index.sh' to exercise
the split index feature in racy situations as well; kind of a
"t0010-racy-git.sh for split index" but with modern style (the tests
do everything in &&-chained list of commands in 'test_expect_...'
blocks, and use 'test_cmp' for more informative output on failure).
The tests cover the following sequences of index splitting, updating,
and racy file modifications, with the last two cases demonstrating the
racy split index problem:
1. Split the index while adding a racily clean file:
echo "cached content" >file
git update-index --split-index --add file
echo "dirty worktree" >file # size stays the same
This case already works properly. Even though the cache entry's
stat data matches with the modifid file in the worktree,
subsequent git commands will notice that the (split) index and
the file have the same mtime, and then will go on to check the
file's content and notice its dirtiness.
2. Add a racily clean file to an already split index:
git update-index --split-index
echo "cached content" >file
git update-index --add file
echo "dirty worktree" >file
This case already works properly. After the second 'git
update-index' writes the newly added file's cache entry to the
new split index, it basically works in the same way as case #1.
3. Split the index when it (i.e. the not yet splitted index)
contains a racily clean cache entry, i.e. an entry whose cached
stat data matches with the corresponding file in the worktree and
the cached mtime matches that of the index:
echo "cached content" >file
git update-index --add file
echo "dirty worktree" >file
# ... wait ...
git update-index --split-index --add other-file
This case already works properly. The shared index is written by
do_write_index(), i.e. the same function that is responsible for
writing "regular" and split indexes as well. This function
cleverly notices the racily clean cache entry, and writes the
entry to the new shared index with smudged stat data, i.e. file
size set to 0. When subsequent git commands read the index, they
will notice that the smudged stat data doesn't match with the
file in the worktree, and then go on to check the file's content
and notice its dirtiness.
4. Update the split index when it contains a racily clean cache
entry:
git update-index --split-index
echo "cached content" >file
git update-index --add file
echo "dirty worktree" >file
# ... wait ...
git update-index --add other-file
This case already works properly. After the second 'git
update-index' the newly added file's cache entry is only stored
in the split index. If a cache entry is present in the split
index (even if it is a replacement of an outdated entry in the
shared index), then it will always be included in the new split
index on subsequent split index updates (until the file is
removed or a new shared index is written), independently from
whether the entry is racily clean or not. When do_write_index()
writes the new split index, it notices the racily clean cache
entry, and smudges its stat date. Subsequent git commands
reading the index will notice the smudged stat data and then go
on to check the file's content and notice its dirtiness.
5. Update the split index when a racily clean cache entry is stored
only in the shared index:
echo "cached content" >file
git update-index --split-index --add file
echo "dirty worktree" >file
# ... wait ...
git update-index --add other-file
This case fails due to the racy split index problem. In the
second 'git update-index' prepare_to_write_split_index() decides,
among other things, which cache entries stored only in the shared
index should be replaced in the new split index. Alas, this
function never looks out for racily clean cache entries, and
since the file's stat data in the worktree hasn't changed since
the shared index was written, the entry won't be replaced in the
new split index. Consequently, do_write_index() doesn't even get
this racily clean cache entry, and can't smudge its stat data.
Subsequent git commands will then see that the index has more
recent mtime than the file and that the (not smudged) cached stat
data still matches with the file in the worktree, and,
ultimately, will erroneously consider the file clean.
6. Update the split index after unpack_trees() copied a racily clean
cache entry from the shared index:
echo "cached content" >file
git update-index --split-index --add file
echo "dirty worktree" >file
# ... wait ...
git read-tree -m HEAD
This case fails due to the racy split index problem. This
basically fails for the same reason as case #5 above, but there
is one important difference, which warrants the dedicated test.
While that second 'git update-index' in case #5 updates
index_state in place, in this case 'git read-tree -m' calls
unpack_trees(), which throws out the entire index, and constructs
a new one from the (potentially updated) copies of the original's
cache entries. Consequently, when prepare_to_write_split_index()
gets to work on this reconstructed index, it takes a different
code path than in case #5 when deciding which cache entries in
the shared index should be replaced. The result is the same,
though: the racily clean cache entry goes unnoticed, it isn't
added to the split index with smudged stat data, and subsequent
git commands will then erroneously consider the file clean.
Note that in the last two 'test_expect_failure' cases I omitted the
'#' (as in nr. of trial) from the tests' description on purpose for
now, as it breakes the TAP output [2]; it will be added at the end of
the series, when those two tests will be flipped to
'test_expect_success'.
[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge
branch 'nd/split-index', 2014-07-16).
[2] In the TAP output a '#' should separate the test's description
from the TODO directive emitted by 'test_expect_failure'. The
additional '#' in "#$trial" interferes with this, the test harness
won't recognize the TODO directive, and will report that those
tests failed unexpectedly.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
A partial clone with missing trees can be obtained using "git clone
--filter=tree:none <repo>". In such a repository, when a tree needs to
be lazily fetched, any tree or blob it directly or indirectly references
is fetched as well, regardless of whether the original command required
those objects, or if the local repository already had some of them.
This is because the fetch protocol, which the lazy fetch uses, does not
allow clients to request that only the wanted objects be sent, which
would be the ideal solution. This patch implements a partial solution:
specify the "blob:none" filter, somewhat reducing the fetch payload.
This change has no effect when lazily fetching blobs (due to how filters
work). And if lazily fetching a commit (such repositories are difficult
to construct and is not a use case we support very well, but it is
possible), referenced commits and trees are still fetched - only the
blobs are not fetched.
The necessary code change is done in fetch_pack() instead of somewhere
closer to where the "filter" instruction is written to the wire so that
only one part of the code needs to be changed in order for users of all
protocol versions to benefit from this optimization.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
When fetch_pack() is invoked as part of another Git command (due to a
lazy fetch from a partial clone, for example), it uses object flags that
may already be used by the outer Git command.
The commit that introduced the lazy fetch feature (88e2f9ed8e
("introduce fetch-object: fetch one promisor object", 2017-12-05)) tried
to avoid this overlap, but it did not avoid it totally. It was
successful in avoiding writing COMPLETE, but did not avoid reading
COMPLETE, and did not avoid writing and reading ALTERNATE.
Ensure that no flags are written or read by fetch_pack() in the case
where it is used to perform a lazy fetch. To do this, it is sufficient
to avoid checking completeness of wanted refs (unnecessary in the case
of lazy fetches), and to avoid negotiation-related work (in the current
implementation, already, no negotiation is performed). After that was
done, the lack of overlap was verified by checking all direct and
indirect usages of COMPLETE and ALTERNATE - that they are read or
written only if no_dependents is false.
There are other possible solutions to this issue:
(1) Split fetch-pack.{c,h} into a flag-using part and a non-flag-using
part, and whenever no_dependents is set, only use the
non-flag-using part.
(2) Make fetch_pack() be able to be used with arbitrary repository
objects. fetch_pack() should then create its own repository object
based on the given repository object, with its own object
hashtable, so that the flags do not conflict.
(1) is possible but invasive - some functions would need to be split;
and such invasiveness would potentially be unnecessary if we ever were
to need (2) anyway. (2) would be useful if we were to support, say,
submodules that were partial clones themselves, but I don't know when or
if the Git project plans to support those.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Invoking 'git rev-parse --show-superproject-working-tree' exits with
"fatal: BUG: returned path string doesn't match cwd?"
when the superproject has an unmerged entry for the current submodule,
instead of displaying the superproject's working tree.
The problem is due to the fact that when a merge of the submodule reference
is in progress, "git ls-files --stage —full-name <submodule-relative-path>”
returns three seperate entries for the submodule (one for each stage) rather
than a single entry; e.g.,
$ git ls-files --stage --full-name submodule-child-test
160000 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1 submodule-child-test
160000 f174d1dbfe863a59692c3bdae730a36f2a788c51 2 submodule-child-test
160000 e6178f3a58b958543952e12824aa2106d560f21d 3 submodule-child-test
The code in get_superproject_working_tree() expected exactly one entry to
be returned; this patch makes it use the first entry if multiple entries
are returned.
Test t1500-rev-parse is extended to cover this case.
Signed-off-by: Sam McKelvie <sammck@gmail.com>
Reviewed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
builtin/commit.c:prepare_to_commit() can call run_status() twice if
using the editor, including status, and the user attempts to record a
non-merge empty commit without explicit --allow-empty. If there is also
a rename involved as well (due to using 'git add -N'), then a BUG in
wt-status.c is triggered:
BUG: wt-status.c:476: multiple renames on the same target? how?
The reason we hit this bug is that both run_status() calls use the same
struct wt_status * (named s), and s->change is not freed between runs.
Changes are inserted into s with string_list_insert, which usually means
that the second run just recomputes all the same results and overwrites
what was computed the first time. However, ever since commit
176ea7479309 ("wt-status.c: handle worktree renames", 2017-12-27),
wt-status started checking for renames and copies but also added a
preventative check that d->rename_status wasn't already set and output a
BUG message if it was. The problem isn't that there are multiple rename
targets to a single path as the error implies, the problem is that 's'
is not freed/cleared between the two run_status() calls.
Ever since commit dc6b1d92ca9c ("wt-status: use settings from
git_diff_ui_config", 2018-05-04), which stopped hardcoding
DIFF_DETECT_RENAME and allowed users to ask for copy detection, this bug
has also been triggerable with a copy instead of a rename.
Fix the bug by clearing s->change. A better change might be to clean up
all of s between the two run_status() calls. A good first step towards
such a goal might be writing a function to free the necessary fields in
the wt_status * struct; a cursory glance at the code suggests all of its
allocated data is probably leaked. However, doing all that cleanup is a
bigger task for someone else interested to tackle; just fix the bug for
now.
Reported-by: Andrea Stacchiotti <andreastacchiotti@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
* maint-2.18:
Git 2.18.1
Git 2.17.2
fsck: detect submodule paths starting with dash
fsck: detect submodule urls starting with dash
Git 2.16.5
Git 2.15.3
Git 2.14.5
submodule-config: ban submodule paths that start with a dash
submodule-config: ban submodule urls that start with dash
submodule--helper: use "--" to signal end of clone options
|
|
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
* maint-2.17:
Git 2.17.2
fsck: detect submodule paths starting with dash
fsck: detect submodule urls starting with dash
Git 2.16.5
Git 2.15.3
Git 2.14.5
submodule-config: ban submodule paths that start with a dash
submodule-config: ban submodule urls that start with dash
submodule--helper: use "--" to signal end of clone options
|
Junio C Hamano
Johannes Schindelin
Jeff King
Johannes Sixt
Noam Postavsky
SZEDER Gábor
Jonathan Tan
Sam McKelvie
Elijah Newren