Another reftable test migrated to the unit-test framework.
* cp/unit-test-reftable-stack:
t-reftable-stack: add test for stack iterators
t-reftable-stack: add test for non-default compaction factor
t-reftable-stack: use reftable_ref_record_equal() to compare ref records
t-reftable-stack: use Git's tempfile API instead of mkstemp()
t: harmonize t-reftable-stack.c with coding guidelines
t: move reftable/stack_test.c to the unit testing framework
In the test "cc list is sanitized", we feed a commit with a variety of
trailers to send-email, and then check its output to see how it handled
them. For most of them, we are grepping for a specific mention of the
header, but there's a "BugID" header which we expect to be ignored. We
confirm this by grepping for "12345", the fake BugID, and making sure it
is not present.
But we can be fooled by false positives! I just tracked down a flaky
test failure here that was caused by matching this unrelated line in the
output:
<20240914090449.612345-1-author@example.com>
which will change from run to run based on the time, pid, etc.
Ideally we'd tighten the regex to make this more specifically, but since
the point is that it _shouldn't_ be mentioned, it's hard to say what the
right match would be (e.g., would there be a leading space?).
Instead, let's just choose a match that is much less likely to appear.
The actual content of the header isn't important, since it's supposed to
be ignored.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If you do something like this:
rm file_a
echo change >file_b
git jump diff
then we'll generate two quickfix entries for the diff, one for each
file. But the one for the deleted file is rather pointless. There's no
content to show since the file is gone, and in fact we open the editor
with the path /dev/null!
In vim, at least, the result is a confusing annoyance: the editor opens
with an empty buffer, and you have to skip past it to the useful
quickfix entry (after scratching your head and figuring out that no,
nothing is broken).
Let's skip such entries entirely. There's nothing useful to show, since
the point is that the file has been deleted.
It is possible that you could be doing a diff whose post-image is not
the working tree, and then you'd perhaps be jumping to the deleted
content (or at least something that was in the same spot). But I don't
think it's worth worrying about that case. For one thing, using git-jump
for such diffs is a bad idea in general, as it's going to sometimes move
you to the wrong spot. And two, a deletion is always going to have one
hunk starting at line 1, which is not that interesting to jump to in the
first place.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we generate a quickfix entry for a diff hunk, we provide just the
filename and line number along with the content, like:
file:1: contents of the line
This can be a problem if the line itself looks like a quickfix header.
For example (and this is adapted from a real-world case that bit me):
echo 'static_lease 10:11:12:13:14:15:16 10.0.0.1' >file
git add file
echo change >file
produces:
file:1: static_lease 10:11:12:13:14:15:16 10.0.0.1
which is ambiguous. It could be line 1 of "file", or line 11 of the file
"file:1: static_lease 10", and so on. In the case of vim's default
config, it seems to prefer the latter (you can configure "errorformat"
with a variety of patterns, but out of the box it matches some common
ones).
One easy way to fix this is to provide a column number, like:
file:1:1: static_lease 10:11:12:13:14:15:16 10.0.0.1
which causes vim to prefer line 1 of "file" again (due to the preference
order of the various patterns in the default errorformat).
There are other options. For example, at least in my version of vim,
wrapping the file in quotation marks like:
"file":1: static_lease 10:11:12:13:14:15:16 10.0.0.1
also works. That perhaps would the right thing even if you had the silly
file name "file:1:1: foo 10". But it's not clear what would happen if
you had a filename with quotes in it.
This feature is inherently scraping text, and there's bound to be some
ambiguities. I don't think it's worth worrying too much about unlikely
filenames, as its the file content that is more likely to introduce
unexpected characters.
So let's just go with the extra ":1" column specifier. We know this is
supported everywhere, as git-jump's "grep" mode already uses it (and
thus doesn't exhibit the same problem).
The "merge" mode is mostly immune to this, as it only matches "<<<<<<<"
conflict marker lines. It's possible of course to have a marker that
says "foo 10:11" later in the line, but in practice these will only have
branches and perhaps file names, so it's probably not worth worrying
about (and fixing it would involve passing --column to the system grep,
which may not be portable).
I also gave some thought as to whether we could put something more
useful than "1" in the column field for diffs. In theory we could find
the first changed character of the line, but this is tricky in practice.
You'd have to correlate before/after lines of the hunk to decide what
changed. So:
-this is a foo line
+this is a bar line
is easy (column 11). But:
-this is a foo line
+another line
+this is a bar line
is harder.
This commit certainly doesn't preclude trying to do something more
clever later, but it's a much deeper rabbit hole than just fixing the
syntactic ambiguity.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The transport-helper code generally relies on the
remote-helper to provide an informative message to the user
when it encounters an error. In the rare cases where the
helper does not do so, the output can be quite confusing.
E.g.:
$ git clone https://example.com/foo.git
Cloning into 'foo'...
$ echo $?
128
$ ls foo
/bin/ls: cannot access foo: No such file or directory
We tried to address this with 81d340d (transport-helper:
report errors properly, 2013-04-10).
But that makes the common case much more confusing. The
remote helper protocol's method for signaling normal errors
is to simply hang up. So when the helper does encounter a
routine error and prints something to stderr, the extra
error message is redundant and misleading. So we dropped it
again in 266f1fd (transport-helper: be quiet on read errors
from helpers, 2013-06-21).
This puts the uncommon case right back where it started. We
may be able to do a little better, though. It is common for
the helper to die during a "real" command, like fetching the
list of remote refs. It is not common for it to die during
the initial "capabilities" negotiation, right after we
start. Reporting failure here is likely to catch fundamental
problems that prevent the helper from running (and reporting
errors) at all. Anything after that is the responsibility of
the helper itself to report.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The interpret-trailers command failed to recognise the end of the
message when the commit log ends in an incomplete line.
* bl/trailers-and-incomplete-last-line-fix:
interpret-trailers: handle message without trailing newline
Cygwin does have /dev/tty support that is needed by things like
single-key input mode.
* rj/cygwin-has-dev-tty:
config.mak.uname: add HAVE_DEV_TTY to cygwin config section
In a few corner cases "git diff --exit-code" failed to report
"changes" (e.g., renamed without any content change), which has
been corrected.
* rs/diff-exit-code-fix:
diff: report dirty submodules as changes in builtin_diff()
diff: report copies and renames as changes in run_diff_cmd()
The environment GIT_ADVICE has been intentionally kept undocumented
to discourage its use by interactive users. Add documentation to
help tool writers.
* ds/doc-wholesale-disabling-advice-messages:
advice: recommend GIT_ADVICE=0 for tools
A file descriptor left open is now properly closed when "git
sparse-checkout" updates the sparse patterns.
* jk/sparse-fdleak-fix:
sparse-checkout: use fdopen_lock_file() instead of xfdopen()
sparse-checkout: check commit_lock_file when writing patterns
sparse-checkout: consolidate cleanup when writing patterns
"git verify-pack" and "git index-pack" started dying outside a
repository, which has been corrected.
* ps/index-pack-outside-repo-fix:
builtin/index-pack: fix segfaults when running outside of a repo
The code forgot to discard unnecessary in-core commit buffer data
for commits that "git log --skip=<number>" traversed but omitted
from the output, which has been corrected.
* jk/free-commit-buffer-of-skipped-commits:
revision: free commit buffers for skipped commits
With the repository variable available in the builtin function as an
argument, pass this down into helper functions instead of using the
global the_repository.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For builtins that do not operate on a repository, remove
the #define USE_THE_REPOSITORY.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of including USE_THE_REPOSITORY_VARIABLE by default on every
builtin, remove it from builtin.h and add it to all the builtins that
include builtin.h (by definition, that means all builtins/*.c).
Also, remove the include statement for repository.h since it gets
brought in through builtin.h.
The next step will be to migrate each builtin
from having to use the_repository.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In order to reduce the usage of the global the_repository, add a
parameter to builtin functions that will get passed a repository
variable.
This commit uses UNUSED on most of the builtin functions, as subsequent
commits will modify the actual builtins to pass the repository parameter
down.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The last test in t5512 we recently added seems to be flaky.
Running
$ make && cd t && sh ./t5512-ls-remote.sh --stress
shows that "git ls-remote foo::bar" exited with status 141, which
means we got a SIGPIPE. This test piece was introduced by 9e89dcb6
(builtin/ls-remote: fall back to SHA1 outside of a repo, 2024-08-02)
and is pretty much independent from all other tests in the script
(it can even run standalone with everything before it removed).
The transport-helper.c:get_helper() function tries to write to the
helper. As we can see the helper script is very short and can exit
even before it reads anything, when get_helper() tries to give the
first command, "capabilities", the helper may already be gone.
A trivial fix, presented here, is to make sure that the helper reads
the first command it is given, as what it writes later is a response
to that command.
I however would wonder if the interactions with the helper initiated
by get_helper() should be done on a non-blocking I/O (we do check
the return value from our write(2) system calls, do we?).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we open a repository with the "Directory" option, we use "rev-parse
--git-dir" to get the path relative to that directory, and then use
Cwd::abs_path() to make it absolute (since our process working directory
may not be the same).
These days we can just ask for "--absolute-git-dir" instead, which saves
us a little code. That option was added in Git v2.13.0 via a2f5a87626
(rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think
we make any promises about running mismatched versions of git and
Git.pm, but even if somebody tries it, that's sufficiently old that it
should be OK.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When opening a bare repository like:
Git->repository(Directory => '/path/to/bare.git');
we will incorrectly point the repository object at the _current_
directory, not the one specified by the option.
The bug was introduced by 20da61f25f (Git.pm: trust rev-parse to find
bare repositories, 2022-10-22). Before then, we'd ask "rev-parse
--git-dir" if it was a Git repo, and if it returned anything, we'd
correctly convert that result to an absolute path using File::Spec and
Cwd::abs_path(). If it didn't, we'd guess it might be a bare repository
and find it ourselves, which was wrong (rev-parse should find even a
bare repo, and our search circumvented some of its rules).
That commit dropped most of the custom bare-repo search code in favor of
using "rev-parse --is-bare-repository" and trusting the "--git-dir" it
returned. But it mistakenly left some of the bare-repo code path in
place, which was now broken. That code calls Cwd::abs_path($dir); prior
to 20da61f25f $dir contained the "Directory" option the user passed in.
But afterwards, it contains the output of "rev-parse --git-dir". And
since our tentative rev-parse command is invoked after changing
directory, it will always be the relative path "."! So we'll end up with
the absolute path of the process's current directory, not the Directory
option the caller asked for.
So the non-bare case is correct, but the bare one is broken. Our tests
only check the non-bare one, so we didn't notice. We can fix this by
running the same absolute-path fixup code for both sides.
Helped-by: Rodrigo <rodrigolive@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the preceding commits we had to convert the linux32 job to be based
on Ubuntu 20.04 instead of Ubuntu 16.04 due to a limitation in GitHub
Workflows. This was the only job left that still tested against this old
but supported Ubuntu version, and we have no other jobs that test with a
comparatively old Linux distribution.
Add a new job to GitLab CI that tests with Ubuntu 16.04 to cover the
resulting test gap. GitLab doesn't modify Docker images in the same way
GitHub does and thus doesn't fall prey to the same issue. There are two
compatibility issues uncovered by this:
- Ubuntu 16.04 does not support HTTP/2 in Apache. We thus cannot set
`GIT_TEST_HTTPD=true`, which would otherwise cause us to fail when
Apache fails to start.
- Ubuntu 16.04 cannot use recent JGit versions as they depend on a
more recent Java runtime than we have available. We thus disable
installing any kind of optional dependencies that do not come from
the package manager.
These two restrictions are fine though, as we only really care about
whether Git compiles and runs on such old distributions in the first
place.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Building older versions of Git may require tweaking some build knobs. In
particular, very old versions of Git will fail to build with recent
OpenSSL, because the bignum type switched from a struct to a pointer.
The i5500 interop test uses Git v1.0.0 by default, which triggers this
problem. You can work around it by setting NO_OPENSSL in your
GIT_TEST_MAKE_OPTS variable. But there are two downsides:
1. You have to know to do this, and it's not at all obvious.
2. That sets the options for _all_ versions of Git that we build. And
it's possible for two versions to require conflicting knobs. E.g.,
building with "make NO_OPENSSL=Nope OPENSSL_SHA1=Yes" causes
imap-send.c to barf, because it declares a fallback typedef for SSL.
This is something we may want to fix, but of course many historical
versions are affected, and the interop scripts should be flexible
enough to build everything.
So let's introduce per-version make options, along with the ability for
scripts to specify knobs that match their default versions. That should
make everything build out of the box, but also allow testers flexibility
if they are testing interoperability between non-default versions.
We'll set NO_OPENSSL by default for v1.0.0 in i5500. It doesn't have to
worry about the conflict with OPENSSL_SHA1 because imap-send did not
exist back then (but if it did, it could also just explicitly use a
different hash implementation).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git cat-file" works well with the sparse-index, and gets marked as
such.
* kl/cat-file-on-sparse-index:
builtin/cat-file: mark 'git cat-file' sparse-index compatible
t1092: allow run_on_* functions to use standard input
One-line messages to "die" and other helper functions will get LF
added by these helper functions, but many existing messages had an
unnecessary LF at the end, which have been corrected.
* jk/messages-with-excess-lf-fix:
drop trailing newline from warning/error/die messages
"git pack-refs --auto" for the files backend was too aggressive,
which has been a bit tamed.
* ps/pack-refs-auto-heuristics:
refs/files: use heuristic to decide whether to repack with `--auto`
t0601: merge tests for auto-packing of refs
wrapper: introduce `log2u()`
A data corruption bug when multi-pack-index is used and the same
objects are stored in multiple packfiles has been corrected.
* tb/multi-pack-reuse-fix:
builtin/pack-objects.c: do not open-code `MAX_PACK_OBJECT_HEADER`
pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse
builtin/pack-objects.c: translate bit positions during pack-reuse
pack-bitmap: tag bitmapped packs with their corresponding MIDX
t/t5332-multi-pack-reuse.sh: verify pack generation with --strict
"git verify-pack" and "git index-pack" started dying outside a
repository, which has been corrected.
* ps/index-pack-outside-repo-fix:
builtin/index-pack: fix segfaults when running outside of a repo
"git bundle unbundle" outside a repository triggered a BUG()
unnecessarily, which has been corrected.
* ps/bundle-outside-repo-fix:
bundle: default to SHA1 when reading bundle headers
builtin/bundle: have unbundle check for repo before opening its bundle
Junio C Hamano