Junio C Hamano [Wed, 21 Nov 2018 13:58:00 +0000 (22:58 +0900)]
Merge branch 'ma/commit-graph-docs' into maint
Doc update.
* ma/commit-graph-docs:
Doc: refer to the "commit-graph file" with dash
git-commit-graph.txt: refer to "*commit*-graph file"
git-commit-graph.txt: typeset more in monospace
git-commit-graph.txt: fix bullet lists
Junio C Hamano [Wed, 21 Nov 2018 13:57:58 +0000 (22:57 +0900)]
Merge branch 'fe/doc-updates' into maint
Doc updates.
* fe/doc-updates:
git-describe.1: clarify that "human readable" is also git-readable
git-column.1: clarify initial description, provide examples
git-archimport.1: specify what kind of Arch we're talking about
Junio C Hamano [Wed, 21 Nov 2018 13:57:52 +0000 (22:57 +0900)]
Merge branch 'md/exclude-promisor-objects-fix' into maint
Operations on promisor objects make sense in the context of only a
small subset of the commands that internally use the revisions
machinery, but the "--exclude-promisor-objects" option were taken
and led to nonsense results by commands like "log", to which it
didn't make much sense. This has been corrected.
* md/exclude-promisor-objects-fix:
exclude-promisor-objects: declare when option is allowed
Documentation/git-log.txt: do not show --exclude-promisor-objects
Junio C Hamano [Wed, 21 Nov 2018 13:57:51 +0000 (22:57 +0900)]
Merge branch 'js/shallow-and-fetch-prune' into maint
"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
Junio C Hamano [Wed, 21 Nov 2018 13:57:51 +0000 (22:57 +0900)]
Merge branch 'jc/receive-deny-current-branch-fix' into maint
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
Junio C Hamano [Wed, 21 Nov 2018 13:57:50 +0000 (22:57 +0900)]
Merge branch 'js/diff-notice-has-drive-prefix' into maint
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
Junio C Hamano [Wed, 21 Nov 2018 13:57:49 +0000 (22:57 +0900)]
Merge branch 'np/log-graph-octopus-fix' into maint
"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
Junio C Hamano [Wed, 21 Nov 2018 13:57:48 +0000 (22:57 +0900)]
Merge branch 'sg/split-index-racefix' into maint
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
Junio C Hamano [Wed, 21 Nov 2018 13:57:48 +0000 (22:57 +0900)]
Merge branch 'jt/non-blob-lazy-fetch' into maint
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
Junio C Hamano [Wed, 21 Nov 2018 13:57:48 +0000 (22:57 +0900)]
Merge branch 'en/status-multiple-renames-to-the-same-target-fix' into maint
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?'
Junio C Hamano [Wed, 21 Nov 2018 13:57:47 +0000 (22:57 +0900)]
Merge branch 'ds/commit-graph-with-grafts' into maint
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
Junio C Hamano [Wed, 21 Nov 2018 13:57:45 +0000 (22:57 +0900)]
Merge branch 'nd/attr-pathspec-fix' into maint
"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'
Junio C Hamano [Wed, 21 Nov 2018 13:57:43 +0000 (22:57 +0900)]
Merge branch 'bp/mv-submodules-with-fsmonitor' into maint
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
Junio C Hamano [Wed, 21 Nov 2018 13:57:42 +0000 (22:57 +0900)]
Merge branch 'js/rebase-i-autosquash-fix' into maint
"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
Junio C Hamano [Wed, 21 Nov 2018 13:57:41 +0000 (22:57 +0900)]
Merge branch 'jk/trailer-fixes' into maint
"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
Jeff King [Tue, 30 Oct 2018 23:23:38 +0000 (19:23 -0400)]
cat-file: handle streaming failures consistently
There are three ways to convince cat-file to stream a blob:
- cat-file -p $blob
- cat-file blob $blob
- echo $batch | cat-file --batch
In the first two, we simply exit with the error code of
streaw_blob_to_fd(). That means that an error will cause us
to exit with "-1" (which we try to avoid) without printing
any kind of error message (which is confusing to the user).
Instead, let's match the third case, which calls die() on an
error. Unfortunately we cannot be more specific, as
stream_blob_to_fd() does not tell us whether the problem was
on reading (e.g., a corrupt object) or on writing (e.g.,
ENOSPC). That might be an opportunity for future work, but
for now we will at least exit with a sane message and exit
code.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 30 Oct 2018 23:23:12 +0000 (19:23 -0400)]
check_stream_sha1(): handle input underflow
This commit fixes an infinite loop when fscking large
truncated loose objects.
The check_stream_sha1() function takes an mmap'd loose
object buffer and streams 4k of output at a time, checking
its sha1. The loop quits when we've output enough bytes (we
know the size from the object header), or when zlib tells us
anything except Z_OK or Z_BUF_ERROR.
The latter is expected because zlib may run out of room in
our 4k buffer, and that is how it tells us to process the
output and loop again.
But Z_BUF_ERROR also covers another case: one in which zlib
cannot make forward progress because it needs more _input_.
This should never happen in this loop, because though we're
streaming the output, we have the entire deflated input
available in the mmap'd buffer. But since we don't check
this case, we'll just loop infinitely if we do see a
truncated object, thinking that zlib is asking for more
output space.
It's tempting to fix this by checking stream->avail_in as
part of the loop condition (and quitting if all of our bytes
have been consumed). But that assumes that once zlib has
consumed the input, there is nothing left to do. That's not
necessarily the case: it may have read our input into its
internal state, but still have bytes to output.
Instead, let's continue on Z_BUF_ERROR only when we see the
case we're expecting: the previous round filled our output
buffer completely. If it didn't (and we still saw
Z_BUF_ERROR), we know something is wrong and should break
out of the loop.
The bug comes from commit f6371f9210 (sha1_file: add
read_loose_object() function, 2017-01-13), which
reimplemented some of the existing loose object functions.
So it's worth checking if this bug was inherited from any of
those. The answers seems to be no. The two obvious
candidates are both OK:
1. unpack_sha1_rest(); this doesn't need to loop on
Z_BUF_ERROR at all, since it allocates the expected
output buffer in advance (which we can't do since we're
explicitly streaming here)
2. check_object_signature(); the streaming path relies on
the istream interface, which uses read_istream_loose()
for this case. That function uses a similar "is our
output buffer full" check with Z_BUF_ERROR (which is
where I stole it from for this patch!)
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 30 Oct 2018 23:18:51 +0000 (19:18 -0400)]
t1450: check large blob in trailing-garbage test
Commit cce044df7f (fsck: detect trailing garbage in all
object types, 2017-01-13) added two tests of trailing
garbage in a loose object file: one with a commit and one
with a blob. The point of having two is that blobs would
follow a different code path that streamed the contents,
instead of loading it into a buffer as usual.
At the time, merely being a blob was enough to trigger the
streaming code path. But since 7ac4f3a007 (fsck: actually
fsck blob data, 2018-05-02), we now only stream blobs that
are actually large. So since then, the streaming code path
is not tested at all for this case.
We can restore the original intent of the test by tweaking
core.bigFileThreshold to make our small blob seem large.
There's no easy way to externally verify that we followed
the streaming code path, but I did check before/after using
a temporary debug statement.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`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>
repack: point out a bug handling stale shallow info
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>
Jeff King [Wed, 24 Oct 2018 07:38:00 +0000 (03:38 -0400)]
run-command: mark path lookup errors with ENOENT
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>
Matthew DeVore [Tue, 23 Oct 2018 01:13:42 +0000 (18:13 -0700)]
exclude-promisor-objects: declare when option is allowed
The --exclude-promisor-objects option causes some funny behavior in at
least two commands: log and blame. It causes a BUG crash:
$ git log --exclude-promisor-objects
BUG: revision.c:2143: exclude_promisor_objects can only be used
when fetch_if_missing is 0
Aborted
[134]
Fix this such that the option is treated like any other unknown option.
The commands that must support it are limited, so declare in those
commands that the flag is supported. In particular:
pack-objects
prune
rev-list
The commands were found by searching for logic which parses
--exclude-promisor-objects outside of revision.c. Extra logic outside of
revision.c is needed because fetch_if_missing must be turned on before
revision.c sees the option or it will BUG-crash. The above list is
supported by the fact that no other command is introspectively invoked
by another command passing --exclude-promisor-object.
Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Matthew DeVore [Tue, 23 Oct 2018 01:13:41 +0000 (18:13 -0700)]
Documentation/git-log.txt: do not show --exclude-promisor-objects
Do not suggest that --exclude-promisor-objects is supported by git-log,
since it currently BUG-crashes and it's not necessary to support it.
Options that control behavior for promisor objects should be limited to
a small number of commands.
Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andreas Heiduk [Mon, 22 Oct 2018 20:45:45 +0000 (22:45 +0200)]
doc: fix indentation of listing blocks in gitweb.conf.txt
'gitweb.conf.txt' uses inconsistent indentation in listing blocks and a mix
of listing blocks and literal paragraphs. Both didn't look pretty in the
rendered HTML page.
Signed-off-by: Andreas Heiduk <asheiduk@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andreas Heiduk [Mon, 22 Oct 2018 20:45:44 +0000 (22:45 +0200)]
doc: fix descripion for 'git tag --format'
The '--format=<format>' is now listed in the 'OPTIONS' section, not only
the '<format>' string itself. The description moved up a few paragraphs
because '<format>' is not a standalone paramater but a parameter for the
option '--format'.
Signed-off-by: Andreas Heiduk <asheiduk@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Johannes Sixt [Fri, 19 Oct 2018 16:58:07 +0000 (18:58 +0200)]
diff: don't attempt to strip prefix from absolute Windows paths
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:
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>
Junio C Hamano [Fri, 19 Oct 2018 04:57:35 +0000 (13:57 +0900)]
receive: denyCurrentBranch=updateinstead should not blindly update
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>
pack-objects (mingw): initialize `packing_data` mutex in the correct spot
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>
pack-objects (mingw): demonstrate a segmentation fault with large deltas
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>
Tao Qingyun [Tue, 16 Oct 2018 14:54:28 +0000 (22:54 +0800)]
builtin/branch.c: remove useless branch_get
branch_get sometimes returns current_branch, which can be NULL (e.g., if
you're on a detached HEAD). Try:
$ git branch HEAD
fatal: no such branch 'HEAD'
$ git branch ''
fatal: no such branch ''
However, it seems weird that we'd check those cases here (and provide
such lousy messages). And indeed, dropping that and letting us
eventually hit create_branch() gives a much better message:
$ git branch HEAD
fatal: 'HEAD' is not a valid branch name.
$ git branch ''
fatal: '' is not a valid branch name.
Signed-off-by: Tao Qingyun <taoqy@ls-a.me> 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>
SZEDER Gábor [Thu, 11 Oct 2018 09:53:57 +0000 (11:53 +0200)]
split-index: BUG() when cache entry refers to non-existing shared entry
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>
SZEDER Gábor [Thu, 11 Oct 2018 09:43:09 +0000 (11:43 +0200)]
split-index: smudge and add racily clean cache entries to split index
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:
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.
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>
SZEDER Gábor [Thu, 11 Oct 2018 09:43:08 +0000 (11:43 +0200)]
split-index: don't compare cached data of entries already marked for split index
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>
SZEDER Gábor [Thu, 11 Oct 2018 09:43:07 +0000 (11:43 +0200)]
split-index: count the number of deleted entries
'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>
SZEDER Gábor [Thu, 11 Oct 2018 09:43:06 +0000 (11:43 +0200)]
t1700-split-index: date back files to avoid racy situations
'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>
SZEDER Gábor [Thu, 11 Oct 2018 09:43:05 +0000 (11:43 +0200)]
split-index: add tests to demonstrate the racy split index problem
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:
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:
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:
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:
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:
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>
doc: move git-rev-parse from porcelain to plumbing
git-rev-parse mostly seems like plumbing, and is more usd in
scripts than in regular use. Online it's often mentioned as
a plumbing command. Nonetheless it's listed under porcelain
interrogators in `man git`. It seems appropriate to formally
move git-rev-parse to plumbing interrogators.
Signed-off-by: Daniels Umanovskis <daniels@umanovskis.se> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Mihir Mehta [Wed, 10 Oct 2018 22:26:54 +0000 (17:26 -0500)]
doc: fix a typo and clarify a sentence
I noticed that git-merge-base was unlikely to actually be a git command,
and tried it in my shell. Seeing that it doesn't work, I cleaned up two
places in the docs where it appears.
Signed-off-by: Mihir Mehta <mihir@cs.utexas.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The sample code calls `get_revision()' followed by `graph_update()',
but the documentation and source code indicate that `get_revision()'
already calls `graph_update()' for you.
Signed-off-by: Michael Witten <mfwitten@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
