* 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
* 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
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
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
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
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
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
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
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?'
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
"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'
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
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
"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
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
'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: 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>
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>
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>
fetch-pack: exclude blobs when lazy-fetching trees
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>
oidset_insert() returns 1 if the object ID is already in the set and
doesn't add it again, or 0 if it hadn't been present. Make use of that
fact instead of checking with an extra oidset_contains() call.
Signed-off-by: Rene Scharfe <l.s.r@web.de> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
config.txt: correct the note about uploadpack.packObjectsHook
Document for uploadpack.packObjectsHook is added in [1] and consists
of two paragraphs, the second one is quite important about where this
variable can stay.
When the paragraph about uploadpack.allowFilter is added in [2], it's
added in between the two paragraphs. This makes the "this is non-repo
level config" note incorrectly apply to allowFilter instead of
packObjectsHook. Move allowFilter paragraph down to fix this.
[1] 20b20a22f8 (upload-pack: provide a hook for running pack-objects -
2016-05-18)
git doc: direct bug reporters to mailing list archive
The mailing list archive can help a user encountering a bug to tell
whether a recent regression has already been reported and whether a
longstanding bug has already had some discussion to start their
thinking.
Based-on-patch-by: Martin Ågren <martin.agren@gmail.com> Improved-by: Junio C Hamano <gitster@pobox.com> Improved-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous patch suggested the strbuf header to be the leading example
of how we would want our APIs to be documented. This may lead to some
scrutiny of that code and the coding style (which is different from the
API documentation style) and hence might be taken as an example on how
to format code as well.
So let's format strbuf.h in a way that we'd like to see:
* omit the extern keyword from function declarations
* name all parameters (usually the parameters are obvious from its type,
but consider exceptions like
`int strbuf_getwholeline_fd(struct strbuf *, int, int);`
* break overly long lines
Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
It makes it harder to let the API description and the reality drift
apart if the doc is kept close to the implementation or the header
of the API. We have been slowly migrating API docs out of the
Documentation/technical/api-* to *.h files, and the development
community generally considers that how inline docs in strbuf.h is
done the best current practice.
We recommend documenting in the header over documenting near the
implementation to encourage people to write the docs that are
readable without peeking at the implemention.
Helped-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t7005-editor: quote filename to fix whitespace-issue
Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES
prereq, 2018-05-14) removed code for detecting whether spaces in
filenames work. Since we rely on spaces throughout the test suite
("trash directory.t1234-foo"), testing whether we can use the filename
"e space.sh" was redundant and unnecessary.
In simplifying the code, though, this introduced a regression around how
spaces are handled, not in the /name/ of the editor script, but /in/ the
script itself. The script just does `echo space >$1`, where $1 is for
example "/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG".
With most shells, or with Bash in posix mode, $1 will not be subjected
to field splitting. But if we invoke Bash directly, which will happen if
we build Git with SHELL_PATH=/bin/bash, it will detect and complain
about an "ambiguous redirect". More details can be found in [1], thanks
to SZEDER Gábor.
Make sure that the editor script quotes "$1" to remove the ambiguity.
Signed-off-by: Alexander Pyhalov <apyhalov@gmail.com> Commit-message-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
rev-parse: --show-superproject-working-tree should work during a merge
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.,
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>
t1400: drop debug `echo` to actually execute `test`
Instead of running `test "foo" = "$(bar)"`, we prefix the whole thing
with `echo`. Comparing to nearby tests makes it clear that this is just
debug leftover. This line has actually been modified four times since it
was introduced in e52290428b (General ref log reading improvements.,
2006-05-19) and the `echo` has always survived. Let's finally drop it.
This script could need some more cleanups. This is just an immediate fix
so that we actually test what we intend to.
All other hits for `git grep "\<echo test " -- t/` seem fine. They want
to create some input or expected output data.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The file processed by `git commit-graph` is referred to as the
"commit-graph file", also with a dash. We have a few references to the
"commit graph file", though, without the dash. These occur in
git-commit-graph.txt as well as in Doc/technical/commit-graph.txt. Fix
them.
Do not change the references to the "commit graph" (without "... file")
as a data structure.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>