Jeff King [Mon, 8 May 2023 19:04:57 +0000 (15:04 -0400)]
t: drop "verbose" helper function
We have a small helper function called "verbose", with the idea that you
can write:
verbose foo
to get a message to stderr when the "foo" command fails, even if it does
not produce any output itself. This goes back to 8ad1652418 (t5304: use
helper to report failure of "test foo = bar", 2014-10-10). It does work,
but overall it has not been a big success for two reasons:
1. Test writers have to remember to put it there (and the resulting
test code is longer as a result).
2. It doesn't handle the opposite case (we expect "foo" to fail, but
it succeeds), leading to inconsistencies in tests (which you can
see in many hunks of this patch, e.g. ones involving "has_cr").
Most importantly, we added a136f6d8ff (test-lib.sh: support -x option
for shell-tracing, 2014-10-10) at the same time, and it does roughly the
same thing. The output is not quite as succinct as "verbose", and you
have to watch out for stray shell-traces ending up in stderr. But it
solves both of the problems above, and has clearly become the preferred
tool.
Let's consider the "verbose" function a failed experiment and remove the
last few callers (which are all many years old, and have been dwindling
as we remove them from scripts we touch for other reasons). It will be
one less thing for new test writers to see and wonder if they should be
using themselves.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Mon, 8 May 2023 19:01:46 +0000 (15:01 -0400)]
t7001: use "ls-files --format" instead of "cut"
Since ls-files recently learned a "--format" option, we can use that
rather than asking for all of "--stage" and then pulling out the bits we
want with "cut". That's simpler and avoids two extra processes (one for
cut, and one for the subshell to hold the intermediate result).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Mon, 8 May 2023 19:01:12 +0000 (15:01 -0400)]
t7001: avoid git on upstream of pipe
We generally avoid git on the left-hand side of a pipe, because it loses
the exit code of the command (and thus we'd miss things like segfaults
or unexpected failures). In the cases in t7001, we wouldn't expect
failures (they are just inspecting the repository state, and are not the
main point of the test), but it doesn't hurt to be careful.
In all but one case here we're piping "ls-files --stage" to cut off the
pathname (since we compare entries before and after moving). Let's pull
that into a helper function to avoid repeating the slightly awkward
replacement.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Shuqi Liang [Mon, 8 May 2023 20:21:40 +0000 (16:21 -0400)]
t1092: update a write-tree test
* 'on all' in the title of the test 'write-tree on all' was unclear;
remove it.
* Add a baseline 'test_all_match git write-tree' before making any
changes to the index, providing a reference point for the 'write-tree'
prior to any modifications.
* Add a comparison of the output of 'git status --porcelain=v2' to test
the working tree after 'write-tree' exits.
* Ensure SKIP_WORKTREE files weren't materialized on disk by using
"test_path_is_missing".
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 8 May 2023 17:38:12 +0000 (13:38 -0400)]
pack-bitmap.c: use commit boundary during bitmap traversal
When reachability bitmap coverage exists in a repository, Git will use a
different (and hopefully faster) traversal to compute revision walks.
Consider a set of positive and negative tips (which we'll refer to with
their standard bitmap parlance by "wants", and "haves"). In order to
figure out what objects exist between the tips, the existing traversal
in `prepare_bitmap_walk()` does something like:
1. Consider if we can even compute the set of objects with bitmaps,
and fall back to the usual traversal if we cannot. For example,
pathspec limiting traversals can't be computed using bitmaps (since
they don't know which objects are at which paths). The same is true
of certain kinds of non-trivial object filters.
2. If we can compute the traversal with bitmaps, partition the
(dereferenced) tips into two object lists, "haves", and "wants",
based on whether or not the objects have the UNINTERESTING flag,
respectively.
3. Fall back to the ordinary object traversal if either (a) there are
more than zero haves, none of which are in the bitmapped pack or
MIDX, or (b) there are no wants.
4. Construct a reachability bitmap for the "haves" side by walking
from the revision tips down to any existing bitmaps, OR-ing in any
bitmaps as they are found.
5. Then do the same for the "wants" side, stopping at any objects that
appear in the "haves" bitmap.
6. Filter the results if any object filter (that can be easily
computed with bitmaps alone) was given, and then return back to the
caller.
When there is good bitmap coverage relative to the traversal tips, this
walk is often significantly faster than an ordinary object traversal
because it can visit far fewer objects.
But in certain cases, it can be significantly *slower* than the usual
object traversal. Why? Because we need to compute complete bitmaps on
either side of the walk. If either one (or both) of the sides require
walking many (or all!) objects before they get to an existing bitmap,
the extra bitmap machinery is mostly or all overhead.
One of the benefits, however, is that even if the walk is slower, bitmap
traversals are guaranteed to provide an *exact* answer. Unlike the
traditional object traversal algorithm, which can over-count the results
by not opening trees for older commits, the bitmap walk builds an exact
reachability bitmap for either side, meaning the results are never
over-counted.
But producing non-exact results is OK for our traversal here (both in
the bitmap case and not), as long as the results are over-counted, not
under.
Relaxing the bitmap traversal to allow it to produce over-counted
results gives us the opportunity to make some significant improvements.
Instead of the above, the new algorithm only has to walk from the
*boundary* down to the nearest bitmap, instead of from each of the
UNINTERESTING tips.
The boundary-based approach still has degenerate cases, but we'll show
in a moment that it is often a significant improvement.
The new algorithm works as follows:
1. Build a (partial) bitmap of the haves side by first OR-ing any
bitmap(s) that already exist for UNINTERESTING commits between the
haves and the boundary.
2. For each commit along the boundary, add it as a fill-in traversal
tip (where the traversal terminates once an existing bitmap is
found), and perform fill-in traversal.
3. Build up a complete bitmap of the wants side as usual, stopping any
time we intersect the (partial) haves side.
4. Return the results.
And is more-or-less equivalent to using the *old* algorithm with this
invocation:
The new result performs significantly better in many cases, particularly
when the distance from the boundary commit(s) to an existing bitmap is
shorter than the distance from (all of) the have tips to the nearest
bitmapped commit.
Note that when using the old bitmap traversal algorithm, the results can
be *slower* than without bitmaps! Under the new algorithm, the result is
computed faster with bitmaps than without (at the cost of over-counting
the true number of objects in a similar fashion as the non-bitmap
traversal):
# (Computing the number of tagged objects not on any branches
# without bitmaps).
$ time git rev-list --count --objects --tags --not --branches
20
real 0m1.388s
user 0m1.092s
sys 0m0.296s
# (Computing the same query using the old bitmap traversal).
$ time git rev-list --count --objects --tags --not --branches --use-bitmap-index
19
The new algorithm is still slower than not using bitmaps at all, but it
is nearly a 15-fold improvement over the existing traversal.
In a more realistic setting (using my local copy of git.git), I can
observe a similar (if more modest) speed-up:
$ argv="--count --objects --branches --not --tags"
hyperfine \
-n 'no bitmaps' "git.compile rev-list $argv" \
-n 'existing traversal' "git.compile rev-list --use-bitmap-index $argv" \
-n 'boundary traversal' "git.compile -c pack.useBitmapBoundaryTraversal=true rev-list --use-bitmap-index $argv"
Benchmark 1: no bitmaps
Time (mean ± σ): 124.6 ms ± 2.1 ms [User: 103.7 ms, System: 20.8 ms]
Range (min … max): 122.6 ms … 133.1 ms 22 runs
Benchmark 2: existing traversal
Time (mean ± σ): 368.6 ms ± 3.0 ms [User: 325.3 ms, System: 43.1 ms]
Range (min … max): 365.1 ms … 374.8 ms 10 runs
Benchmark 3: boundary traversal
Time (mean ± σ): 167.6 ms ± 0.9 ms [User: 139.5 ms, System: 27.9 ms]
Range (min … max): 166.1 ms … 169.2 ms 17 runs
Summary
'no bitmaps' ran
1.34 ± 0.02 times faster than 'boundary traversal'
2.96 ± 0.05 times faster than 'existing traversal'
Here, the new algorithm is also still slower than not using bitmaps, but
represents a more than 2-fold improvement over the existing traversal in
a more modest example.
Since this algorithm was originally written (nearly a year and a half
ago, at the time of writing), the bitmap lookup table shipped, making
the new algorithm's result more competitive. A few other future
directions for improving bitmap traversal times beyond not using bitmaps
at all:
- Decrease the cost to decompress and OR together many bitmaps
together (particularly when enumerating the uninteresting side of
the walk). Here we could explore more efficient bitmap storage
techniques, like Roaring+Run and/or use SIMD instructions to speed
up ORing them together.
- Store pseudo-merge bitmaps, which could allow us to OR together
fewer "summary" bitmaps (which would also help with the above).
Helped-by: Jeff King <peff@peff.net> Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 8 May 2023 17:38:09 +0000 (13:38 -0400)]
pack-bitmap.c: extract `fill_in_bitmap()`
To prepare for the boundary-based bitmap walk to perform a fill-in
traversal using the boundary of either side as the tips, extract routine
used to perform fill-in traversal by `find_objects()` so that it can be
used in both places.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Nico Rieck [Tue, 24 Jan 2023 11:23:17 +0000 (11:23 +0000)]
gitk: escape file paths before piping to git log
We just started piping the file paths via `stdin` instead of passing
them via the command-line, to avoid running into command-line
limitations.
However, since we now pipe the file paths, we need to take care of
special characters.
This fixes https://github.com/git-for-windows/git/issues/2293
Signed-off-by: Nico Rieck <nico.rieck@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
To avoid running into command line limitations, some of Git's commands
support the `--stdin` option.
Let's use exactly this option in the three rev-list/log invocations in
gitk that would otherwise possibly run the danger of trying to invoke a
too-long command line.
While it is easy to redirect either stdin or stdout in Tcl/Tk scripts,
what we need here is both. We need to capture the output, yet we also
need to pipe in the revs/files arguments via stdin (because stdin does
not have any limit, unlike the command line). To help this, we use the
neat Tcl feature where you can capture stdout and at the same time feed
a fixed string as stdin to the spawned process.
One non-obvious aspect about this change is that the `--stdin` option
allows to specify revs, the double-dash, and files, but *no* other
options such as `--not`. This is addressed by prefixing the "negative"
revs with `^` explicitly rather than relying on the `--not` option
(thanks for coming up with that idea, Max!).
This fixes https://github.com/git-for-windows/git/issues/1987
Analysis-and-initial-patch-by: Max Kirillov <max@max630.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teng Long [Sat, 6 May 2023 11:34:08 +0000 (19:34 +0800)]
push: introduce '--branches' option
The '--all' option of git-push built-in cmd support to push all branches
(refs under refs/heads) to remote. Under the usage, a user can easlily
work in some scenarios, for example, branches synchronization and batch
upload.
The '--all' was introduced for a long time, meanwhile, git supports to
customize the storage location under "refs/". when a new git user see
the usage like, 'git push origin --all', we might feel like we're
pushing _all_ the refs instead of just branches without looking at the
documents until we found the related description of it or '--mirror'.
To ensure compatibility, we cannot rename '--all' to another name
directly, one way is, we can try to add a new option '--heads' which be
identical with the functionality of '--all' to let the user understand
the meaning of representation more clearly. Actually, We've more or less
named options this way already, for example, in 'git-show-ref' and 'git
ls-remote'.
At the same time, we fix a related issue about the wrong help
information of '--all' option in code and add some test cases in
t5523, t5543 and t5583.
Signed-off-by: Teng Long <dyroneteng@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
John Cai [Sat, 6 May 2023 04:15:29 +0000 (04:15 +0000)]
attr: teach "--attr-source=<tree>" global option to "git"
Earlier, 47cfc9bd (attr: add flag `--source` to work with tree-ish,
2023-01-14) taught "git check-attr" the "--source=<tree>" option to
allow it to read attribute files from a tree-ish, but did so only
for the command. Just like "check-attr" users wanted a way to use
attributes from a tree-ish and not from the working tree files,
users of other commands (like "git diff") would benefit from the
same.
Undo most of the UI change the commit made, while keeping the
internal logic to read attributes from a given tree-ish. Expose the
internal logic via a new "--attr-source=<tree>" command line option
given to "git", so that it can be used with any git command that
runs as part of the main git process.
Additionally, add an environment variable GIT_ATTR_SOURCE that is set
when --attr-source is passed in, so that subprocesses use the same value
for the attributes source tree.
Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
John Cai [Sat, 6 May 2023 04:14:11 +0000 (04:14 +0000)]
name-rev: make --stdin hidden
In 34ae3b70 (name-rev: deprecate --stdin in favor of --annotate-stdin),
we renamed --stdin to --annotate-stdin for the sake of a clearer name
for the option, and added text that indicates --stdin is deprecated. The
next step is to hide --stdin completely.
Make the option hidden. Also, update documentation to remove all
mentions of --stdin.
Signed-off-by: "John Cai" <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Sat, 6 May 2023 21:29:55 +0000 (14:29 -0700)]
t9800: correct misuse of 'show -s --raw' in a test
There is $(git show -s --raw --pretty=format:%at HEAD) in this test
that is meant to grab the author time of the commit. We used to
have a bug in the command line option parser of the diff family of
commands, where "show -s --raw" was identical to "show -s".
With the "-s" bug fixed, "show -s --raw" would mean the same thing
as "show --raw", i.e. show the output from the diff machinery in the
"raw" format. And this test will start failing, so fix it before
that happens.
Jeff King [Fri, 5 May 2023 21:16:10 +0000 (17:16 -0400)]
doc-diff: drop SOURCE_DATE_EPOCH override
The original doc-diff script set SOURCE_DATE_EPOCH to make asciidoc's
output deterministic. Otherwise, the mtime of the source files would end
up in the footer of the manpage, causing noisy and uninteresting diff
hunks.
But this has been unused since 28fde3a1f4 (doc: set actual revdate for
manpages, 2023-04-13), as the footer uses the externally-specified
GIT_DATE instead (that needs to be set consistently, too, which it now
is as of the previous commit).
Asciidoc sets several automatic attributes based on the mtime (or manual
epoch), so it's still possible to write a document that would need
SOURCE_DATE_EPOCH set to be deterministic. But if we wrote such a thing,
it's probably a mistake, and we're better off having doc-diff loudly
show it.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Fri, 5 May 2023 16:59:52 +0000 (09:59 -0700)]
diff: fix interaction between the "-s" option and other options
Sergey Organov noticed and reported "--patch --no-patch --raw"
behaves differently from just "--raw". It turns out that there are
a few interesting bugs in the implementation and documentation.
* First, the documentation for "--no-patch" was unclear that it
could be read to mean "--no-patch" countermands an earlier
"--patch" but not other things. The intention of "--no-patch"
ever since it was introduced at d09cd15d (diff: allow --no-patch
as synonym for -s, 2013-07-16) was to serve as a synonym for
"-s", so "--raw --patch --no-patch" should have produced no
output, but it can be (mis)read to allow showing only "--raw"
output.
* Then the interaction between "-s" and other format options were
poorly implemented. Modern versions of Git uses one bit each to
represent formatting options like "--patch", "--stat" in a single
output_format word, but for historical reasons, "-s" also is
represented as another bit in the same word. This allows two
interesting bugs to happen, and we have both X-<.
(1) After setting a format bit, then setting NO_OUTPUT with "-s",
the code to process another "--<format>" option drops the
NO_OUTPUT bit to allow output to be shown again. However,
the code to handle "-s" only set NO_OUTPUT without unsetting
format bits set earlier, so the earlier format bit got
revealed upon seeing the second "--<format>" option. This is
the problem Sergey observed.
(2) After setting NO_OUTPUT with "-s", code to process
"--<format>" option can forget to unset NO_OUTPUT, leaving
the command still silent.
It is tempting to change the meaning of "--no-patch" to mean
"disable only the patch format output" and reimplement "-s" as "not
showing anything", but it would be an end-user visible change in
behavior. Let's fix the interactions of these bits to first make
"-s" work as intended.
The fix is conceptually very simple.
* Whenever we set DIFF_FORMAT_FOO because we saw the "--foo"
option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is
given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to
do so in some of the options and caused (2) above.
* When processing "-s" option, we should not just set
DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.
We didn't do so and retained format bits set by options
previously seen, causing (1) above.
It is even more tempting to lose NO_OUTPUT bit and instead take
output_format word being 0 as its replacement, but that would break
the mechanism "git show" uses to default to "--patch" output, where
the distinction between telling the command to be silent with "-s"
and having no output format specified on the command line matters,
and an explicit output format given on the command line should not
be "combined" with the default "--patch" format.
So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we
may want to replace it with OPTION_GIVEN bit, and
* make "--patch", "--raw", etc. set DIFF_FORMAT_$format bit and
DIFF_FORMAT_OPTION_GIVEN bit on for each format. "--no-raw",
etc. will set off DIFF_FORMAT_$format bit but still record the
fact that we saw an option from the command line by setting
DIFF_FORMAT_OPTION_GIVEN bit.
* make "-s" (and its synonym "--no-patch") clear all other bits
and set only the DIFF_FORMAT_OPTION_GIVEN bit on.
which I suspect would make the code much cleaner without breaking
any end-user expectations.
Once that is in place, transitioning "--no-patch" to mean the
counterpart of "--patch", just like "--no-raw" only defeats an
earlier "--raw", would be quite simple at the code level. The
social cost of migrating the end-user expectations might be too
great for it to be worth, but at least the "GIVEN" bit clean-up
alone may be worth it.
Junio C Hamano [Fri, 5 May 2023 21:19:17 +0000 (14:19 -0700)]
diff: plug leaks in dirstat
The array of dirstat_file contained in the dirstat_dir structure is
not freed after the processing ends. Unfortunately, the member that
points at the array, .files, is incremented as the gather_dirstat()
function recursively walks it, and this needs to be plugged by
remembering the beginning of the array before gather_dirstat() mucks
with it and freeing it after we are done.
We can mark t4047 as leak-free. t4000, which is marked as
leak-free, now can exercise dirstat in it, which will happen next.
Earlier we changed the manual page formatting machinery to use the
dates from the commit the documentation source was taken from,
instead of the date the manual page was produced. When "doc-diff"
compares two commits from different dates, the different dates from
the two commits would result in unnecessary differences in the
output because of the change.
Compensate by setting a fixed date when "doc-diff" formats the pages
to be compared to work around this issue.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
DocBook Stylesheets limit the size of the manpage titles for some
reason.
Even some of the longest git commands have no trouble fitting in 80
character terminals, so it's not clear why we would want to limit titles
to 20 characters, especially when modern terminals are much bigger.
Alex Henrie [Wed, 3 May 2023 04:46:56 +0000 (22:46 -0600)]
docs: rewrite the documentation of the text and eol attributes
These two sentences are confusing because the description of the text
attribute sounds exactly the same as the description of the text=auto
attribute:
"Setting the text attribute on a path enables end-of-line normalization"
"When text is set to "auto", the path is marked for automatic
end-of-line conversion"
Unless the reader is already familiar with the two variants, there's a
high probability that they will think that "end-of-line normalization"
is the same thing as "automatic end-of-line conversion".
It's also not clear that the phrase "When the file has been committed
with CRLF, no conversion is done" in the paragraph for text=auto does
not apply equally to the bare text attribute which is described earlier.
Moreover, it falsely implies that normalization is only suppressed if
the file has been committed. In fact, running `git add` on a CRLF file,
adding the text=auto attribute to the file, and running `git add` again
does not do anything to the line endings either.
On top of that, in several places the documentation for the eol
attribute sounds like either it does not affect normalization on checkin
or it forces normalization on checkin. It also sounds like setting eol
(or setting a config variable) is required to turn on conversion on
checkout, but the text attribute can turn on conversion on checkout by
itself if eol is unspecified.
Rephrase the documentation of text, text=auto, eol, eol=crlf, and eol=lf
to be clear about how they are the same, how they are different, and in
what cases conversion is performed.
Helped-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Mon, 17 Apr 2023 19:10:44 +0000 (21:10 +0200)]
t2019: don't create unused files
Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of
"git checkout" to files "stdout" and "stderr". Several assertions are
made using file "stderr". File "stdout", however, is unused.
Don't redirect standard output of "git checkout" to file "stdout" in
t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Mon, 17 Apr 2023 19:10:43 +0000 (21:10 +0200)]
t1502: don't create unused files
Three tests in file t1502-rev-parse-parseopt.sh use three redirections
with invocation of "git rev-parse --parseopt --". All three tests
redirect standard output to file "out" and file "spec" to standard
input. Two of the tests redirect standard output a second time to file
"actual", and the third test redirects standard error to file "err".
These tests check contents of files "actual" and "err", but don't use
the files named "out" for assertions. The two tests that redirect to
standard output twice might also be confusing to the reader.
Don't redirect standard output of "git rev-parse" to file "out" in
t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.
Acked-by: Øystein Walle <oystwa@gmail.com> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Mon, 17 Apr 2023 19:10:42 +0000 (21:10 +0200)]
t1450: don't create unused files
Test 'fsck error and recovery on invalid object type' in file
t1450-fsck.sh redirects output of a failing "git fsck" invocation to
files "out" and "err" to assert presence of error messages in the output
of the command. Commit 31deb28f5e (fsck: don't hard die on invalid
object types, 2021-10-01) changed the way assertions in this test are
performed. The test doesn't compare the whole standard error with
prepared file "err.expect" and it doesn't assert that standard output is
empty.
Don't create unused files "err.expect" and "out" in test 'fsck error and
recovery on invalid object type'.
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Mon, 17 Apr 2023 19:10:41 +0000 (21:10 +0200)]
t1300: don't create unused files
Three tests in t1300-config.sh check that "git config --get" barfs when
syntax errors are present in the config file. The tests redirect
standard output and standard error of "git config --get" to files,
"actual" and "error" correspondingly. They assert presence of an error
message in file "error". However, these tests don't use file "actual"
for assertions.
Don't redirect standard output of "git config --get" to file "actual" in
t1300-config.sh to avoid creating unnecessary files.
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Mon, 17 Apr 2023 19:10:40 +0000 (21:10 +0200)]
t1300: fix config file syntax error descriptions
Three tests in t1300-config.sh check that "git config --get" barfs when
the config file contains various syntax errors: key=value pair without
equals sign, broken section line, and broken value string. The sample
config files include a comment describing the kind of broken syntax.
This description seems to have been copy-pasted from the "broken section
line" sample to the other two samples.
Fix descriptions of broken config file syntax in samples used in
t1300-config.sh.
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Mon, 17 Apr 2023 19:10:39 +0000 (21:10 +0200)]
t0300: don't create unused file
Test 'credential config with partial URLs' in t0300-credentials.sh
contains three "git credential fill" invocations. For two of the
invocations, the test asserts presence or absence of string "yep" in the
standard output. For the third test it checks for an error message in
standard error.
Don't redirect standard output of "git credential" to file "stdout" in
t0300-credentials.sh to avoid creating an unnecessary file when only
standard error is checked.
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Felipe Contreras [Tue, 18 Apr 2023 01:18:28 +0000 (19:18 -0600)]
doc: remove custom callouts format
The code to render callouts for manpages comes from 17 years ago: 776e994af5 (Properly render asciidoc "callouts" in git man pages.,
2006-04-28), and it was needed back then, but DocBook Stylesheets added
support for that in 2008 [1], since 1.74.0 it hasn't been necessary.
What's worse: the format of the upstream callouts is much nicer than our
hacked version.
1. Changes in the working tree not yet staged for the next
commit.
2. Changes between the index and your last commit; what you
would be committing if you run git commit without -a
option.
3. Changes in the working tree since your last commit; what
you would be committing if you run git commit -a
1. Changes in the working tree not yet staged for the next commit.
2. Changes between the index and your last commit; what you would
be committing if you run git commit without -a option.
3. Changes in the working tree since your last commit; what you
would be committing if you run git commit -a
Let's drop our unnecessary inferior custom format and use the official
one.
[1] https://sourceforge.net/p/docbook/code/7842/
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Tue, 2 May 2023 17:13:35 +0000 (10:13 -0700)]
Merge branch 'tb/ban-strtok'
Mark strtok() and strtok_r() to be banned.
* tb/ban-strtok:
banned.h: mark `strtok()` and `strtok_r()` as banned
t/helper/test-json-writer.c: avoid using `strtok()`
t/helper/test-oidmap.c: avoid using `strtok()`
t/helper/test-hashmap.c: avoid using `strtok()`
string-list: introduce `string_list_setlen()`
string-list: multi-delimiter `string_list_split_in_place()`
Junio C Hamano [Tue, 2 May 2023 17:13:34 +0000 (10:13 -0700)]
Merge branch 'jk/blame-fake-commit-label'
The output given by "git blame" that attributes a line to contents
taken from the file specified by the "--contents" option shows it
differently from a line attributed to the working tree file.
* jk/blame-fake-commit-label:
blame: use different author name for fake commit generated by --contents
The completion script used to use bare "read" without the "-r"
option to read the contents of various state files, which risked
getting confused with backslashes in them. This has been
corrected.
* ek/completion-use-read-r-to-read-literally:
completion: suppress unwanted unescaping of `read`
René Scharfe [Mon, 1 May 2023 19:51:57 +0000 (21:51 +0200)]
test-ctype: check EOF
The character classifiers are supposed to allow passing EOF to them, a
negative value. It isn't part of any character class. Extend the tests
to cover that.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Tue, 2 May 2023 13:27:22 +0000 (13:27 +0000)]
fsck: use local repository
In 0d30feef3c5 (fsck: create scaffolding for rev-index checks,
2023-04-17) and later 5a6072f631d (fsck: validate .rev file header,
2023-04-17), the check_pack_rev_indexes() method was created with a
'struct repository *r' parameter. However, this parameter was unused and
instead 'the_repository' was used in its place.
Fix this situation with the obvious replacement.
Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Tue, 2 May 2023 13:27:21 +0000 (13:27 +0000)]
fsck: verify checksums of all .bitmap files
If a filesystem-level corruption occurs in a .bitmap file, Git can react
poorly. This could take the form of a run-time error due to failing to
parse an EWAH bitmap or be more subtle such as returning the wrong set
of objects to a fetch or clone.
A natural first response to either of these kinds of errors is to run
'git fsck' to see if any files are corrupt. This currently ignores all
.bitmap files.
Add checks to 'git fsck' for all .bitmap files that are currently
associated with a multi-pack-index or pack file. Verify their checksums
using the hashfile API.
We iterate through all multi-pack-indexes and pack-files to be sure to
check all .bitmap files, not just the one that would be read by the
process. For example, a multi-pack-index bitmap overrules a pack-bitmap.
However, if the multi-pack-index is removed, the pack-bitmap may be
selected instead. Be thorough to include every file that could become
active in such a way. This includes checking files in alternates.
There is potential that we could extend this effort to check the
structure of the reachability bitmaps themselves, but it is very
expensive to do so. At minimum, it's as expensive as generating the
bitmaps in the first place, and that's assuming that we don't use the
trivial algorithm of verifying each bitmap individually. The trivial
algorithm will result in quadratic behavior (number of objects times
number of bitmapped commits) while the bitmap building operation
constructs a lattice of commits to build bitmaps incrementally and then
generate the final bitmaps from a subset of those commits.
If we were to extend 'git fsck' to check .bitmap file contents more
closely like this, then we would likely want to hide it behind an option
that signals the user is more willing to do expensive operations such as
this.
For testing, set up a repository with a pack-bitmap _and_ a
multi-pack-index bitmap. This requires some file movement to avoid
deleting the pack-bitmap during the repack that creates the
multi-pack-index bitmap. We can then verify that 'git fsck' is checking
all files, not just the "active" bitmap.
Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We need to provide `--trailer sign` since the command won’t output
anything if you don’t give it an input and/or a
`--trailer`. Furthermore, the message which already contains an s-o-b is
wrong:
$ git interpret-trailers --trailer sign <msg.txt Signed-off-by: Alice <alice@example.com> Signed-off-by: Alice <alice@example.com>
This can’t be what was originally intended.
So change the messages in this example to use the typical
“subject/message” file.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
doc: interpret-trailers: don’t use deprecated config
`command` has been deprecated since commit c364b7ef51 (trailer: add new
.cmd config option, 2021-05-03).
Use the commit message of c364b7ef51 as a guide to replace the use of
`$ARG` and to use a script instead of an inline command.[1] Also,
explicitly trigger the command by passing in `--trailer=see`, since
this config is not automatically used.[2]
[1]: “Instead of "$ARG", users can refer to the value as positional
argument, $1, in their scripts.”
[2]: “At the same time, in order to allow `git interpret-trailers` to
better simulate the behavior of `git command -s`,
'trailer.<token>.cmd' will not automatically execute.”
Acked-by: ZheNing Hu <adlternative@gmail.com> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use input redirection instead of invoking cat(1) on a single file. This
is more straightforward, saves a process, and often makes the line
shorter.
Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
doc: interpret-trailers: don’t use heredoc in examples
This file contains four instances of trailing spaces from its inception
in commit [1]. These spaces might be intentional, since a user would be
prompted with `> ` in an interactive session. On the one hand, this is a
whitespace error according to `git diff --check`; on the other hand, the
raw documentation—it makes no difference in the rendered output—is just
staying faithful to the simulation of the interactive prompt.
Let’s get rid of these whitespace errors and also make the examples more
friendly to cut-and-paste by replacing the heredocs with files which are
shown with cat(1).
[1]: dfd66ddf5a (Documentation: add documentation for 'git
interpret-trailers', 2014-10-13)
Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Mon, 1 May 2023 17:30:37 +0000 (10:30 -0700)]
setup: trace bare repository setups
safe.bareRepository=explicit is a safer default mode of operation, since
it guards against the embedded bare repository attack [1]. Most end
users don't use bare repositories directly, so they should be able to
set safe.bareRepository=explicit, with the expectation that they can
reenable bare repositories by specifying GIT_DIR or --git-dir.
However, the user might use a tool that invokes Git on bare repositories
without setting GIT_DIR (e.g. "go mod" will clone bare repositories
[2]), so even if a user wanted to use safe.bareRepository=explicit, it
wouldn't be feasible until their tools learned to set GIT_DIR.
To make this transition easier, add a trace message to note when we
attempt to set up a bare repository without setting GIT_DIR. This allows
users and tool developers to audit which of their tools are problematic
and report/fix the issue. When they are sufficiently confident, they
would switch over to "safe.bareRepository=explicit".
Note that this uses trace2_data_string(), which isn't supported by the
"normal" GIT_TRACE2 target, only _EVENT or _PERF.
Taylor Blau [Mon, 1 May 2023 15:54:06 +0000 (11:54 -0400)]
contrib/credential: embiggen fixed-size buffer in wincred
As in previous commits, harden the wincred credential helper against the
aforementioned protocol injection attack.
Unlike the approached used for osxkeychain and libsecret, where a
fixed-size buffer was replaced with `getline()`, we must take a
different approach here. There is no `getline()` equivalent in Windows,
and the function is not available to us with ordinary compiler settings.
Instead, allocate a larger (still fixed-size) buffer in which to process
each line. The value of 100 KiB is chosen to match the maximum-length
header that curl will allow, CURL_MAX_HTTP_HEADER.
To ensure that we are reading complete lines at a time, and that we
aren't susceptible to a similar injection attack (albeit with more
padding), ensure that each read terminates at a newline (i.e., that no
line is more than 100 KiB long).
Note that it isn't sufficient to turn the old loop into something like:
the credential helper would fill its buffer after reading up through the
first '\r', call fgets() again, and then see "host=example.com\r\n" on
its line.
Note that the original code was written in a way that would trim an
arbitrary number of "\r" and "\n" from the end of the string. We should
get only a single "\n" (since the point of `fgets()` is to return the
buffer to us when it sees one), and likewise would not expect to see
more than one associated "\r". The new code trims a single "\r\n", which
matches the original intent.
Tested-by: Matthew John Cheetham <mjcheetham@outlook.com> Helped-by: Matthew John Cheetham <mjcheetham@outlook.com> Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 1 May 2023 15:54:03 +0000 (11:54 -0400)]
contrib/credential: avoid fixed-size buffer in libsecret
The libsecret credential helper reads the newline-delimited
protocol stream one line at a time by repeatedly calling fgets() into a
fixed-size buffer, and is thus affected by the vulnerability described
in the previous commit.
To mitigate this attack, avoid using a fixed-size buffer, and instead
rely on getline() to allocate a buffer as large as necessary to fit the
entire content of the line, preventing any protocol injection.
In most parts of Git we don't assume that every platform has getline().
But libsecret is primarily used on Linux, where we do already assume it
(using a knob in config.mak.uname). POSIX also added getline() in 2008,
so we'd expect other recent Unix-like operating systems to have it
(e.g., FreeBSD also does).
Note that the buffer was already allocated on the heap in this case, but
we'll swap `g_free()` for `free()`, since it will now be allocated by
the system `getline()`, rather than glib's `g_malloc()`.
Tested-by: Jeff King <peff@peff.net> Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The libsecret credential helper does not mark its build artifact as
ignored, so running "make" results in a dirty working tree.
Mark the "git-credential-libsecret" binary as ignored to avoid the above.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
libgnome-keyring was deprecated in 2014 (in favor of libsecret), more
than nine years ago [1].
The credential helper implemented using libgnome-keyring has had a small
handful of commits since 2013, none of which implemented or changed any
functionality. The last commit to do substantial work in this area was 15f7221686 (contrib/git-credential-gnome-keyring.c: support really
ancient gnome-keyring, 2013-09-23), just shy of nine years ago.
This credential helper suffers from the same `fgets()`-related injection
attack (using the new "wwwauth[]" feature) as in the previous commit.
Instead of patching it, let's remove this helper as deprecated.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 1 May 2023 15:53:54 +0000 (11:53 -0400)]
contrib/credential: avoid fixed-size buffer in osxkeychain
The macOS Keychain-based credential helper reads the newline-delimited
protocol stream one line at a time by repeatedly calling fgets() into a
fixed-size buffer, and is thus affected by the vulnerability described
in the previous commit.
To mitigate this attack, avoid using a fixed-size buffer, and instead
rely on getline() to allocate a buffer as large as necessary to fit the
entire content of the line, preventing any protocol injection.
We solved a similar problem in a5bb10fd5e (config: avoid fixed-sized
buffer when renaming/deleting a section, 2023-04-06) by switching to
strbuf_getline(). We can't do that here because the contrib helpers do
not link with the rest of Git, and so can't use a strbuf. But we can use
the system getline() directly, which works similarly.
In most parts of Git we don't assume that every platform has getline().
But this helper is run only on OS X, and that platform added support in
10.7 ("Lion") which was released in 2011.
Tested-by: Taylor Blau <me@ttaylorr.com> Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 1 May 2023 15:53:51 +0000 (11:53 -0400)]
t/lib-credential.sh: ensure credential helpers handle long headers
Add a test ensuring that the "wwwauth[]" field cannot be used to
inject malicious data into the credential helper stream.
Many of the credential helpers in contrib/credential read the
newline-delimited protocol stream one line at a time by repeatedly
calling fgets() into a fixed-size buffer.
This assumes that each line is no more than 1024 characters long, since
each iteration of the loop assumes that it is parsing starting at the
beginning of a new line in the stream. However, similar to a5bb10fd5e
(config: avoid fixed-sized buffer when renaming/deleting a section,
2023-04-06), if a line is longer than 1024 characters, a malicious actor
can embed another command within an existing line, bypassing the usual
checks introduced in 9a6bbee800 (credential: avoid writing values with
newlines, 2020-03-11).
As with the problem fixed in that commit, specially crafted input can
cause the helper to return the credential for the wrong host, letting an
attacker trick the victim into sending credentials for one host to
another.
Luckily, all parts of the credential helper protocol that are available
in a tagged release of Git are immune to this attack:
- "protocol" is restricted to known values, and is thus immune.
- "host" is immune because curl will reject hostnames that have a '='
character in them, which would be required to carry out this attack.
- "username" is immune, because the buffer characters to fill out the
first `fgets()` call would pollute the `username` field, causing the
credential helper to return nothing (because it would match a
username if present, and the username of the credential to be stolen
is likely not 1024 characters).
- "password" is immune because providing a password instructs
credential helpers to avoid filling credentials in the first place.
- "path" is similar to username; if present, it is not likely to match
any credential the victim is storing. It's also not enabled by
default; the victim would have to set credential.useHTTPPath
explicitly.
However, the new "wwwauth[]" field introduced via 5f2117b24f
(credential: add WWW-Authenticate header to cred requests, 2023-02-27)
can be used to inject data into the credential helper stream. For
example, running:
being sent to the credential helper. If we tweak that "1024" to align
our output with the helper's buffer size and the rest of the data on the
line, it can cause the helper to see "host=victim.com" on its own line,
allowing motivated attackers to exfiltrate credentials belonging to
"victim.com".
The below test demonstrates these failures and provides us with a test
to ensure that our fix is correct. That said, it has a couple of
shortcomings:
- it's in t0303, since that's the only mechanism we have for testing
random helpers. But that means nobody is going to run it under
normal circumstances.
- to get the attack right, it has to line up the stuffed name with the
buffer size, so we depend on the exact buffer size. I parameterized
it so it could be used to test other helpers, but in practice it's
not likely for anybody to do that.
Still, it's the best we can do, and will help us confirm the presence of
the problem (and our fixes) in the new few patches.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 1 May 2023 15:53:48 +0000 (11:53 -0400)]
credential.c: store "wwwauth[]" values in `credential_read()`
Teach git-credential to read "wwwauth[]" value(s) when parsing the
output of a credential helper.
These extra headers are not needed for Git's own HTTP support to use the
feature internally, but the feature would not be available for a
scripted caller (say, git-remote-mediawiki providing the header in the
same way).
As a bonus, this also makes it easier to use wwwauth[] in synthetic
credential inputs in our test suite.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sometimes, adding a header different than CC or TO is desirable; for
example, when using Debbugs, it is best to use 'X-Debbugs-Cc' headers
to keep people in CC; this is an example use case enabled by the new
'--header-cmd' option.
The header unfolding logic is extracted to a subroutine so that it can
be reused; a test is added for coverage.
Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t/t3501-revert-cherry-pick.sh: clarify scope of the file
The file started out as a test for picks and reverts with renames, but
has been subsequently populated with all kinds of basic tests, in
accordance with its generic name. Adjust the description to reflect
that.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Fri, 28 Apr 2023 23:03:03 +0000 (16:03 -0700)]
Merge branch 'tb/enable-cruft-packs-by-default'
When "gc" needs to retain unreachable objects, packing them into
cruft packs (instead of exploding them into loose object files) has
been offered as a more efficient option for some time. Now the use
of cruft packs has been made the default and no longer considered
an experimental feature.
* tb/enable-cruft-packs-by-default:
repository.h: drop unused `gc_cruft_packs`
builtin/gc.c: make `gc.cruftPacks` enabled by default
t/t9300-fast-import.sh: prepare for `gc --cruft` by default
t/t6500-gc.sh: add additional test cases
t/t6500-gc.sh: refactor cruft pack tests
t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default
t/t5304-prune.sh: prepare for `gc --cruft` by default
builtin/gc.c: ignore cruft packs with `--keep-largest-pack`
builtin/repack.c: fix incorrect reference to '-C'
pack-write.c: plug a leak in stage_tmp_packfiles()
Junio C Hamano [Fri, 28 Apr 2023 23:03:03 +0000 (16:03 -0700)]
Merge branch 'jk/gpg-trust-level-fix'
The "%GT" placeholder for the "--format" option of "git log" and
friends caused BUG() to trigger on a commit signed with an unknown
key, which has been corrected.
* jk/gpg-trust-level-fix:
gpg-interface: set trust level of missing key to "undefined"
messages: capitalization and punctuation exceptions
These are conscious violations of the usual rules for error messages,
based on this reasoning:
- If an error message is directly followed by another sentence, it
needs to be properly terminated with a period, lest the grammar
looks broken and becomes hard to read.
- That second sentence isn't actually an error message any more, so
it should abide to conventional language rules for good looks and
legibility. Arguably, these should be converted to advice
messages (which the user can squelch, too), but that's a much
bigger effort to get right.
- Neither of these apply to the first hunk in do_exec(), but this
two-line message looks just too much like a real sentence to not
terminate it. Also, leaving it alone would make it asymmetrical
to the other hunk.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
N_() is meant to be used on strings that are subsequently _()'d, which
isn't the case here.
The affected construct is a bit questionable from an i18n perspective,
as it pieces together a sentence from separate strings. However, it
doesn't appear to be that bad, as the "assembly instructions" are in a
translatable message as well. Lacking specific complaints from
translators, it doesn't seem worth changing this.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Thu, 27 Apr 2023 23:00:59 +0000 (16:00 -0700)]
Merge branch 'tb/pack-revindex-on-disk'
The on-disk reverse index that allows mapping from the pack offset
to the object name for the object stored at the offset has been
enabled by default.
* tb/pack-revindex-on-disk:
t: invert `GIT_TEST_WRITE_REV_INDEX`
config: enable `pack.writeReverseIndex` by default
pack-revindex: introduce `pack.readReverseIndex`
pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK
pack-revindex: make `load_pack_revindex` take a repository
t5325: mark as leak-free
pack-write.c: plug a leak in stage_tmp_packfiles()
Jeff King [Thu, 27 Apr 2023 08:17:24 +0000 (04:17 -0400)]
parse_commit(): describe more date-parsing failure modes
The previous few commits improved the parsing of dates in malformed
commit objects. But there's one big case left implicit: we may still
feed garbage to parse_timestamp(). This is preferable to trying to be
more strict, but let's document the thinking in a comment.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The comment in parse_commit_date() claims that parse_timestamp() will
not walk past the end of the buffer we've been given, since it will hit
the newline at "eol" and stop. This is usually true, when dateptr
contains actual numbers to parse. But with a line like:
committer name <email> \n
with just whitespace, and no numbers, parse_timestamp() will consume
that newline as part of the leading whitespace, and we may walk past our
"tail" pointer (which itself is set from the "size" parameter passed in
to parse_commit_buffer()).
In practice this can't cause us to walk off the end of an array, because
we always add an extra NUL byte to the end of objects we load from disk
(as a defense against exactly this kind of bug). However, you can see
the behavior in action when "committer" is the final header (which it
usually is, unless there's an encoding) and the subject line can be
parsed as an integer. We walk right past the newline on the committer
line, as well as the "\n\n" separator, and mistake the subject for the
timestamp.
We can solve this by trimming the whitespace ourselves, making sure that
it has some non-whitespace to parse. Note that we need to be a bit
careful about the definition of "whitespace" here, as our isspace()
doesn't match exotic characters like vertical tab or formfeed. We can
work around that by checking for an actual number (see the in-code
comment). This is slightly more restrictive than the current code, but
in practice the results are either the same (we reject "foo" as "0", but
so would parse_timestamp()) or extremely unlikely even for broken
commits (parse_timestamp() would allow "\v123" as "123", but we'll now
make it "0").
I did also allow "-" here, which may be controversial, as we don't
currently support negative timestamps. My reasoning was two-fold. One,
the design of parse_timestamp() is such that we should be able to easily
switch it to handling signed values, and this otherwise creates a
hard-to-find gotcha that anybody doing that work would get tripped up
on. And two, the status quo is that we currently parse them, though the
result of course ends up as a very large unsigned value (which is likely
to just get clamped to "0" for display anyway, since our date routines
can't handle it).
The new test checks the commit parser (via "--until") for both vanilla
spaces and the vertical-tab case. I also added a test to check these
against the pretty-print formatter, which uses split_ident_line(). It's
not subject to the same bug, because it already insists that there be
one or more digits in the timestamp.
Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Thu, 27 Apr 2023 08:14:09 +0000 (04:14 -0400)]
parse_commit(): parse timestamp from end of line
To find the committer timestamp, we parse left-to-right looking for the
closing ">" of the email, and then expect the timestamp right after
that. But we've seen some broken cases in the wild where this fails, but
we _could_ find the timestamp with a little extra work. E.g.:
This means that features that rely on the committer timestamp, like
--since or --until, will treat the commit as happening at time 0 (i.e.,
1970).
This is doubly confusing because the pretty-print parser learned to
handle these in 03818a4a94 (split_ident: parse timestamp from end of
line, 2013-10-14). So printing them via "git show", etc, makes
everything look normal, but --until, etc are still broken (despite the
fact that that commit explicitly mentioned --until!).
So let's use the same trick as 03818a4a94: find the end of the line, and
parse back to the final ">". In theory we could use split_ident_line()
here, but it's actually a bit more strict. In particular, it requires a
valid time-zone token, too. That should be present, of course, but we
wouldn't want to break --until for cases that are working currently.
We might want to teach split_ident_line() to become more lenient there,
but it would require checking its many callers (since right now they can
assume that if date_start is non-NULL, so is tz_start).
So for now we'll just reimplement the same trick in the commit parser.
The test is in t4212, which already covers similar cases, courtesy of 03818a4a94. We'll just adjust the broken commit to munge both the author
and committer timestamps. Note that we could match (author|committer)
here, but alternation can't be used portably in sed. Since we wouldn't
expect to see ">" except as part of an ident line, we can just match
that character on any line.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 24 Apr 2023 22:20:26 +0000 (18:20 -0400)]
banned.h: mark `strtok()` and `strtok_r()` as banned
`strtok()` has a couple of drawbacks that make it undesirable to have
any new instances. In addition to being thread-unsafe, it also
encourages confusing data flows, where `strtok()` may be called from
multiple functions with its first argument as NULL, making it unclear
from the immediate context which string is being tokenized.
Now that we have removed all instances of `strtok()` from the tree,
let's ban `strtok()` to avoid introducing new ones in the future. If new
callers should arise, they are encouraged to use
`string_list_split_in_place()` (and `string_list_remove_empty_items()`,
if applicable).
string_list_split_in_place() is not a perfect drop-in replacement
for `strtok_r()`, particularly if the caller is processing a string with
an arbitrary number of tokens, and wants to process each token one at a
time.
But there are no instances of this in Git's tree which are more
well-suited to `strtok_r()` than the friendlier
`string_list_split_in_place()`, so ban `strtok_r()`, too.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Han Xin [Wed, 26 Apr 2023 13:15:04 +0000 (21:15 +0800)]
negotiator/skipping: fix some problems in mark_common()
The mark_common() method in negotiator/skipping.c was converted
from recursive to iterative in 4654134976f (negotiator/skipping:
avoid stack overflow, 2022-10-25), but there is some more work
to do:
1. prio_queue() should be used with clear_prio_queue(), otherwise there
will be a memory leak.
2. It does not do duplicate protection before prio_queue_put().
(The COMMON bit would work here, too.)
3. When it translated from recursive to iterative it kept "return"
statements that should probably be "continue" statements.
4. It does not attempt to parse commits, and instead returns
immediately when finding an unparsed commit. This is something
that it did in its original version, so maybe it is by design,
but it doesn't match the doc comment for the method.
Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Han Xin <hanxin.hx@bytedance.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Han Xin [Wed, 26 Apr 2023 13:15:03 +0000 (21:15 +0800)]
negotiator/default: avoid stack overflow
mark_common() in negotiator/default.c may overflow the stack due to
recursive function calls. Avoid this by instead recursing using a
heap-allocated data structure.
This is the same case as 4654134976f (negotiator/skipping: avoid
stack overflow, 2022-10-25)
Reported-by: Xin Xing <xingxin.xx@bytedance.com> Signed-off-by: Han Xin <hanxin.hx@bytedance.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Wed, 26 Apr 2023 15:13:55 +0000 (08:13 -0700)]
doc: GIT_DEFAULT_HASH is and will be ignored during "clone"
The phrasing "is currently ignored" was prone to be misinterpreted
as if we were wishing if it were honored. Rephrase it to make it
clear that the experimental variable will be ignored.
In the longer term, after/when we allow incremental/over-the-wire
migration of the object-format, i.e. cloning from an SHA-1
repository to create an SHA-256 repository (or vice versa) and
fetching and pushing between them would bidirectionally convert the
object format on the fly, it is likely that we would teach a new
option "--object-format" to "git clone" to say "you would use
whatever object format the origin uses by default, but this time, I
am telling you to use this format on our side, doing on-the-fly
object format conversion as needed". So it is perfectly OK to
ignore the settings of this experimental variable, even after such
an extension happens that makes it necessary for us to have a way to
create a new repository that uses different object format from the
origin repository.
Junio C Hamano [Tue, 25 Apr 2023 20:56:20 +0000 (13:56 -0700)]
Merge branch 'ps/fix-geom-repack-with-alternates'
Geometric repacking ("git repack --geometric=<n>") in a repository
that borrows from an alternate object database had various corner
case bugs, which have been corrected.
* ps/fix-geom-repack-with-alternates:
repack: disable writing bitmaps when doing a local repack
repack: honor `-l` when calculating pack geometry
t/helper: allow chmtime to print verbosely without modifying mtime
pack-objects: extend test coverage of `--stdin-packs` with alternates
pack-objects: fix error when same packfile is included and excluded
pack-objects: fix error when packing same pack twice
pack-objects: split out `--stdin-packs` tests into separate file
repack: fix generating multi-pack-index with only non-local packs
repack: fix trying to use preferred pack in alternates
midx: fix segfault with no packs and invalid preferred pack
The sendemail-validate validate hook learned to pass the total
number of input files and where in the sequence each invocation is
via environment variables.
* rj/send-email-validate-hook-count-messages:
send-email: export patch counters in validate environment
Junio C Hamano [Tue, 25 Apr 2023 20:56:20 +0000 (13:56 -0700)]
Merge branch 'jk/protocol-cap-parse-fix'
The code to parse capability list for v0 on-wire protocol fell into
an infinite loop when a capability appears multiple times, which
has been corrected.
* jk/protocol-cap-parse-fix:
v0 protocol: use size_t for capability length/offset
t5512: test "ls-remote --heads --symref" filtering with v0 and v2
t5512: allow any protocol version for filtered symref test
t5512: add v2 support for "ls-remote --symref" test
v0 protocol: fix sha1/sha256 confusion for capabilities^{}
t5512: stop referring to "v1" protocol
v0 protocol: fix infinite loop when parsing multi-valued capabilities
Junio C Hamano [Tue, 25 Apr 2023 20:56:19 +0000 (13:56 -0700)]
Merge branch 'en/header-split-cache-h'
Header clean-up.
* en/header-split-cache-h: (24 commits)
protocol.h: move definition of DEFAULT_GIT_PORT from cache.h
mailmap, quote: move declarations of global vars to correct unit
treewide: reduce includes of cache.h in other headers
treewide: remove double forward declaration of read_in_full
cache.h: remove unnecessary includes
treewide: remove cache.h inclusion due to pager.h changes
pager.h: move declarations for pager.c functions from cache.h
treewide: remove cache.h inclusion due to editor.h changes
editor: move editor-related functions and declarations into common file
treewide: remove cache.h inclusion due to object.h changes
object.h: move some inline functions and defines from cache.h
treewide: remove cache.h inclusion due to object-file.h changes
object-file.h: move declarations for object-file.c functions from cache.h
treewide: remove cache.h inclusion due to git-zlib changes
git-zlib: move declarations for git-zlib functions from cache.h
treewide: remove cache.h inclusion due to object-name.h changes
object-name.h: move declarations for object-name.c functions from cache.h
treewide: remove unnecessary cache.h inclusion
treewide: be explicit about dependence on mem-pool.h
treewide: be explicit about dependence on oid-array.h
...
Taylor Blau [Mon, 24 Apr 2023 22:20:23 +0000 (18:20 -0400)]
t/helper/test-json-writer.c: avoid using `strtok()`
Apply similar treatment as in the previous commit to remove usage of
`strtok()` from the "oidmap" test helper.
Each of the different commands that the "json-writer" helper accepts
pops the next space-delimited token from the current line and interprets
it as a string, integer, or double (with the exception of the very first
token, which is the command itself).
To accommodate this, split the line in place by the space character, and
pass the corresponding string_list to each of the specialized `get_s()`,
`get_i()`, and `get_d()` functions.
`get_i()` and `get_d()` are thin wrappers around `get_s()` that convert
their result into the appropriate type by either calling `strtol()` or
`strtod()`, respectively. In `get_s()`, we mark the token as "consumed"
by incrementing the `consumed_nr` counter, indicating how many tokens we
have read up to that point.
Because each of these functions needs the string-list parts, the number
of tokens consumed, and the line number, these three are wrapped up in
to a struct representing the line state.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Mon, 24 Apr 2023 22:20:17 +0000 (18:20 -0400)]
t/helper/test-hashmap.c: avoid using `strtok()`
Avoid using the non-reentrant `strtok()` to separate the parts of each
incoming command. Instead of replacing it with `strtok_r()`, let's
instead use the more friendly pair of `string_list_split_in_place()` and
`string_list_remove_empty_items()`.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
is preferable over calling `string_list_clear()` on every iteration of
the loop. This is because `string_list_clear()` causes us free our
existing `items` array. This means that every time we call
`string_list_split_in_place()`, the string-list internals re-allocate
the same size array.
Since in the above example we do not care about the individual parts
after processing each line, it is much more efficient to pretend that
there aren't any elements in the `string_list` by setting `list->nr` to
0 while leaving the list of elements allocated as-is.
This allows `string_list_split_in_place()` to overwrite any existing
entries without needing to free and re-allocate them.
However, setting `list->nr` manually is not safe in all instances. There
are a couple of cases worth worrying about:
- If the `string_list` is initialized with `strdup_strings`,
truncating the list can lead to overwriting strings which are
allocated elsewhere. If there aren't any other pointers to those
strings other than the ones inside of the `items` array, they will
become unreachable and leak.
(We could ourselves free the truncated items between
string_list->items[nr] and `list->nr`, but no present or future
callers would benefit from this additional complexity).
- If the given `nr` is larger than the current value of `list->nr`,
we'll trick the `string_list` into a state where it thinks there are
more items allocated than there actually are, which can lead to
undefined behavior if we try to read or write those entries.
Guard against both of these by introducing a helper function which
guards assignment of `list->nr` against each of the above conditions.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Enhance `string_list_split_in_place()` to accept multiple characters as
delimiters instead of a single character.
Instead of using `strchr(2)` to locate the first occurrence of the given
delimiter character, `string_list_split_in_place_multi()` uses
`strcspn(2)` to move past the initial segment of characters comprised of
any characters in the delimiting set.
When only a single delimiting character is provided, `strpbrk(2)` (which
is implemented with `strcspn(2)`) has equivalent performance to
`strchr(2)`. Modern `strcspn(2)` implementations treat an empty
delimiter or the singleton delimiter as a special case and fall back to
calling strchrnul(). Both glibc[1] and musl[2] implement `strcspn(2)`
this way.
This change is one step to removing `strtok(2)` from the tree. Note that
`string_list_split_in_place()` is not a strict replacement for
`strtok()`, since it will happily turn sequential delimiter characters
into empty entries in the resulting string_list. For example:
Jacob Keller [Mon, 24 Apr 2023 19:35:08 +0000 (12:35 -0700)]
blame: use different author name for fake commit generated by --contents
When the --contents option is used with git blame, and the contents of
the file have lines which can't be annotated by the history being
blamed, the user will see an author of "Not Committed Yet". This is
similar to the way blame handles working tree contents when blaming
without a revision.
This is slightly confusing since this data isn't the working copy and
while it is technically "not committed yet", its also coming from an
external file. Replace this author name with "External file
(--contents)" to better differentiate such lines from actual working
copy lines.
Suggested-by: Junio C Hamano <gitster@pobox.com> Suggested-by: Glen Choo <chooglen@google.com> Signed-off-by: Jacob Keller <jacob.keller@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Sun, 23 Apr 2023 13:46:49 +0000 (15:46 +0200)]
t1300: add tests for missing keys
There are several tests in t1300-config.sh that validate failing
invocations of "git config". However, there are no tests that check
what happens when "git config" is asked to retrieve a value for a
missing key.
Add tests that check this for various combinations of "<section>.<key>"
and "<section>.<subsection>.<key>".
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Sun, 23 Apr 2023 13:46:48 +0000 (15:46 +0200)]
t1300: check stderr for "ignores pairs" tests
Tests "git config ignores pairs ..." in t1300-config.sh validate that
"git config" ignores various kinds of supplied pairs of environment
variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* depending on
GIT_CONFIG_COUNT. By "ignores" here we mean that "git config" abides by
the value of environment variable GIT_CONFIG_COUNT and doesn't use
key-value pairs outside of the supplied GIT_CONFIG_COUNT when trying to
produce a value for config key "pair.one".
These tests also validate that "git config" doesn't complain about
mismatched environment variables to standard error. This is validated
by redirecting the standard error to a file called "error" and asserting
that it is empty. However, two of these tests incorrectly redirect to
standard output while calling the file "error", and test 'git config
ignores pairs exceeding count' doesn't validate standard error at all.
Fix these tests by redirecting standard error to file "error" and
asserting its emptiness.
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Andrei Rybak [Sun, 23 Apr 2023 13:46:47 +0000 (15:46 +0200)]
t1300: drop duplicate test
There are two almost identical tests called 'git config ignores pairs
with zero count' in file t1300-config.sh. Drop the first of these and
keep the one that contains more assertions.
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
merge-ort: fix calling merge_finalize() with no intermediate merge
If some code sets up the data structures for a merge, but then never
actually performs one before calling merge_finalize(), then
merge_finalize() wouldn't notice that result->priv was NULL and
return early, resulting in following that NULL pointer and getting
a segfault. There is currently no code in the git codebase that does
this, but this issue was found during testing of some proposed patches
that had the following structure:
where some flags could cause the code to have N=0, i.e. doing no merges.
Add a check for result->priv being NULL and return early to avoid a
segfault in these kinds of cases.
While at it, ensure the FREE_AND_NULL() in the function does something
useful with the nulling aspect, namely sets result->priv to NULL rather
than a mere temporary.
Reported-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
