Convert "refs.c" to stop using `the_repository` in favor of the repo
that gets passed in via `struct ref_store`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The macro TEST takes a single expression. If a test requires multiple
statements then they need to be placed in a function that's called in
the TEST expression.
Remove the cognitive overhead of defining and calling single-use
functions by using if_test instead.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The macro TEST takes a single expression. If a test requires multiple
statements then they need to be placed in a function that's called in
the TEST expression.
Remove the overhead of defining and calling single-use functions by
using if_test instead.
Run the tests in the order of definition. We can reorder them like that
because they are independent. Technically this changes the output, but
retains the meaning of a full run and allows for easier review e.g. with
diff option --ignore-all-space.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use the documented macro if_test instead of the internal functions
test__run_begin() and test__run_end(), which are supposed to be private
to the unit test framework.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The macro TEST only allows defining a test that consists of a single
expression. Add a new macro, if_test, which provides a way to define
unit tests that are made up of one or more statements.
if_test allows defining self-contained tests en bloc, a bit like
test_expect_success does for regular tests. It acts like a conditional;
the test body is executed if test_skip_all() had not been called before.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Checks outside of tests are caught at runtime and reported like this:
Assertion failed: (ctx.running), function test_assert, file test-lib.c, line 267.
The assert() call aborts the unit test and doesn't reveal the location
or even the type of the offending check, as test_assert() is called by
all of them.
Handle it like the opposite case, a test without any checks: Don't
abort, but report the location of the actual check, along with a message
explaining the situation. The output for example above becomes:
# BUG: check outside of test at t/helper/test-example-tap.c:75
... and the unit test program continues and indicates the error in its
exit code at the end.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Improve the readability of the expected output by using a here-doc for
the test body and replacing the unwieldy ${SQ} references with literal
single quotes.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The test_msg() calls only repeat information already present in test
descriptions and check definitions, which are shown automatically if
the checks fail. Remove the redundant messages to simplify the tests
and their output. Here it is with all of them failing before:
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:18
# when adding a brand-new object, NULL should be returned
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:21
# when adding a brand-new object, NULL should be returned
not ok 1 - Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.
# check "ret == &vars->decoration_a" failed at t/unit-tests/t-example-decorate.c:29
# when readding an already existing object, existing decoration should be returned
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:32
# when readding an already existing object, existing decoration should be returned
not ok 2 - When re-adding an already existing object, the old decoration is returned.
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:40
# lookup should return added declaration
# check "ret == &vars->decoration_b" failed at t/unit-tests/t-example-decorate.c:43
# lookup should return added declaration
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:46
# lookup for unknown object should return NULL
not ok 3 - Lookup returns the added declarations, or NULL if the object was never added.
# check "objects_noticed == 2" failed at t/unit-tests/t-example-decorate.c:58
# left: 1
# right: 2
# should have 2 objects
not ok 4 - The user can also loop through all entries.
1..4
... and here with the patch applied:
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:18
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:20
not ok 1 - Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.
# check "ret == &vars->decoration_a" failed at t/unit-tests/t-example-decorate.c:27
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:29
not ok 2 - When re-adding an already existing object, the old decoration is returned.
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:36
# check "ret == &vars->decoration_b" failed at t/unit-tests/t-example-decorate.c:38
# check "ret == NULL" failed at t/unit-tests/t-example-decorate.c:40
not ok 3 - Lookup returns the added declarations, or NULL if the object was never added.
# check "objects_noticed == 2" failed at t/unit-tests/t-example-decorate.c:51
# left: 1
# right: 2
not ok 4 - The user can also loop through all entries.
1..4
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When "git daemon" enters a repository, it chdir's to the requested
repository and then uses "." (the curent directory) to consult the
"is this repository considered safe?" when it is not owned by the
same owner as the process.
Make sure this access will be allowed by setting safe.directory to
".", as that was once advertised on the list as a valid workaround
to the overly tight safe.directory settings introduced by 2.45.1
(cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).
Also add simlar test to show what happens in the same setting if the
safe.directory is set to "*" instead of "."; in short, "." is a bit
tighter (as it is custom designed for git-daemon situation) than
"anything goes" settings given by "*".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/"
A request to check if /home/u/repository is safe would be rejected
if the safe.directory configuration allows /home/u/repo/ but not its
alias /home/u/repository/. Normalize the paths configured for the
safe.directory configuration variable before comparing them with the
path being checked.
Two and a half things to note, compared to the previous step to
normalize the actual path of the suspected repository, are:
- A configured safe.directory may be coming from .gitignore in the
home directory that may be shared across machines. The path
meant to match with an entry may not necessarily exist on all of
such machines, so not being able to convert them to real path on
this machine is *not* a condition that is worthy of warning.
Hence, we ignore a path that cannot be converted to a real path.
- A configured safe.directory is essentially a random string that
user throws at us, written completely unrelated to the directory
the current process happens to be in. Hence it makes little
sense to give a non-absolute path. Hence we ignore any
non-absolute paths, except for ".".
- The safe.directory set to "." was once advertised on the list as
a valid workaround for the regression caused by the overly tight
safe.directory check introduced in 2.45.1; we treat it to mean
"if we are at the top level of a repository, it is OK".
(cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/".
A request to check if /home/u/repo is safe would be rejected if the
safe.directory configuration allows /home/u/repository/ but not its
alias /home/u/repo/. Normalize the path being checked before
comparing with safe.directory value(s).
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The paths given in the safe.directory configuration variable are
allowed to contain "~user" (which interpolates to user's home
directory) and "%(prefix)" (which interpolates to the installation
location in RUNTIME_PREFIX-enabled builds, and a call to the
git_config_pathname() function is tasked to obtain a copy of the
path with these constructs interpolated.
The function, when it succeeds, always yields an allocated string in
the location given as the out-parameter; even when there is nothing
to interpolate in the original, a literal copy is made. The code
path that contains this caller somehow made two contradicting and
incorrect assumptions of the behaviour when there is no need for
interpolation, and was written with extra defensiveness against
two phantom risks that do not exist.
One wrong assumption was that the function might yield NULL when
there is no interpolation. This led to the use of an extra "check"
variable, conditionally holding either the interpolated or the
original string. The assumption was with us since 8959555c
(setup_git_directory(): add an owner check for the top-level
directory, 2022-03-02) originally introduced the safe.directory
feature.
Another wrong assumption was that the function might yield the same
pointer as the input when there is no interpolation. This led to a
conditional free'ing of the interpolated copy, that the conditional
never skipped, as we always received an allocated string.
Simplify the code by removing the extra defensiveness.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
4aa2c4753d (grep: -W: don't extend context to trailing empty lines,
2016-05-28) stopped showing empty lines at the end of function context
when using -W. Do the same for trailing empty lines at the end of
files, for consistency -- it doesn't matter whether a function section
is ended by the next function or the end of the file.
Test it by adding a trailing empty line to the file used by the test
"grep -W" and leave its expected output the same.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The get_one_patchid() function unconditionally takes a line that
matches the patch header (namely, a line that begins with a full
object name, possibly prefixed by "commit" or "From" plus a space)
as the beginning of a patch. Even when it is *not* looking for one
(namely, when the previous call found the patch header and returned,
and then we are called again to skip the log message and process the
patch whose header was found by the previous invocation).
As a consequence, a line in the commit log message that begins with
one of these patterns can be mistaken to start another patch, with
current message entirely skipped (because we haven't even reached
the patch at all).
Allow the caller to tell us if it called us already and saw the
patch header (in which case we shouldn't be looking for another one,
until we see the "diff" part of the patch; instead we simply should
be skipping these lines as part of the commit log message), and skip
the header processing logic when that is the case. In the helper
function, it also needs to flip this "are we looking for a header?"
bit, once it finished skipping the commit log message and started
processing the patches, as the patch header of the _next_ message is
the only clue in the input that the current patch is done.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The get_one_patchid() function reads input lines until it finds a
patch header (the line that begins a patch), whose beginning is one
of:
(1) an "<object name>", which is what "git diff-tree --stdin" shows;
(2) "commit <object name>", which is what "git log" shows; or
(3) "From <object name>", which is what "git log --format=email" shows.
When it finds such a line, it returns to the caller, reporting the
<object name> it found, and the size of the "patch" it processed.
The caller then calls the function again, which then ignores the
commit log message, and then processes the lines in the patch part
until it hits another "beginning of a patch".
The above logic was fairly easy to see until 2bb73ae8 (patch-id: use
starts_with() and skip_prefix(), 2016-05-28) reorganized the code,
which made another logic that has nothing to do with the "where does
the next patch begin?" logic, which came from 2485eab5
(git-patch-id: do not trip over "no newline" markers, 2011-02-17)
that ignores the "\ No newline at the end", rolled into the same
single if() statement.
Let's split it out. The "\ No newline at the end" marker is part of
the patch, should not appear before we start reading the patch part,
and does not belong to the detection of patch header.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We pass two independent Boolean flags (i.e. do we want the stable
variant of patch-id? do we want to hash the stuff verbatim?) into
the function as two separate parameters. Before adding the third
one and make the interface even wider, let's consolidate them into
a single flag word.
No changes in behaviour. Just a trivial interface change.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The caller passes a flag that is used to become no-op when calling
flush_current_id(). Instead of calling something that becomes a
no-op, teach the caller not to call it in the first place.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git patch-id" was first developed to read from "git diff-tree
--stdin -p" output. Later it was enhanced to read from "git
diff-tree --stdin -p -v", which was the downstream of an early
imitation of "git log" ("git rev-list" run in the upstream of a pipe
to feed the "diff-tree"). These days, we also read from "git
format-patch".
Their output begins slightly differently, but the patch-id computed
over them for the same commit should be the same. Ensure that we
won't accidentally break this expectation.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With "git notes add -C $blob", the given blob contents are to be made
into a note without involving an editor. But when "--allow-empty" is
given, the editor is invoked, which can cause problems for
non-interactive callers[1].
This behaviour started with 90bc19b3ae (notes.c: introduce
'--separator=<paragraph-break>' option, 2023-05-27), which changed
editor invocation logic to check for a zero length note_data buffer.
Restore the original behaviour of "git note" that takes the contents
given via the "-m", "-C", "-F" options without invoking an editor, by
checking for any prior parameter callbacks, indicated by a non-zero
note_data.msg_nr. Remove the now-unneeded note_data.given flag.
Add a test for this regression by checking whether GIT_EDITOR is
invoked alongside "git notes add -C $empty_blob --allow-empty"
[1] https://github.com/ddiss/icyci/issues/12
Signed-off-by: David Disseldorp <ddiss@suse.de>
[jc: enhanced the test with -m/-F options]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The comment surrounding check_pointer_eq() should explain about what
this function does instead of explaining check_int(). Correct this.
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.
As such, check-non-portable-shell.pl warns when it detects such usage.
However, a limitation of the check is that it only detects such
invocations when variable assignment (i.e. `VAR=val`) is the first thing
on the line. Thus, it can easily be fooled by an invocation such as:
echo X | VAR=val shell-func
Address this shortcoming by loosening the check so that the variable
assignment can be recognized even when not at the beginning of the line.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Most problems reported by check-non-portable-shell are accompanied by
advice suggesting how the test author can repair the problem. For
instance:
error: egrep/fgrep obsolescent (use grep -E/-F)
However, when one-shot variable assignment is detected when calling a
shell function (i.e. `VAR=val shell-func`), the problem is reported, but
no advice is given. The lack of advice is particularly egregious since
neither the problem nor the workaround are likely well-known by
newcomers to the project writing tests for the first time. Address this
shortcoming by recommending the use of `test_env` which is tailor made
for this specific use-case.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a0a630192d (t/check-non-portable-shell: detect "FOO=bar
shell_func", 2018-07-13) added the check for one-shot environment
variable assignment for shell functions, the primary reason given for
avoiding them was that, under some shells, the assignment outlives the
invocation of the shell function, thus could potentially negatively
impact subsequent commands in the same test, as well as subsequent
tests.
However, it has recently become apparent that this is not the only
potential problem with one-shot assignments and shell functions. Another
problem is that some shells do not actually export the variable to
commands which the function invokes[1]. More significantly, however, the
behavior of one-shot assignments with shell functions is not specified
by POSIX[2].
Given this new understanding, the presented error message ("assignment
extends beyond 'shell_func'") is too specific and potentially
misleading. Address this by emitting a less specific error message.
(Note that the wording "is not portable" is chosen over the more
specific "behavior not specified by POSIX" for consistency with almost
all other error message issued by this "lint" script.)
[1]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/
[2]: https://lore.kernel.org/git/xmqq34o19jj1.fsf@gitster.g/
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell, thus should be avoided.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The behavior of a one-shot environment variable assignment of the form
"VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell
function. Indeed the behavior differs between shell implementations and
even different versions of the same shell. One such problematic behavior
is that, with some shells, the assignment will outlive the invocation of
the function, thus may potentially impact subsequent commands in the
test, as well as subsequent tests. A common way to work around the
problem is to wrap a subshell around the one-shot assignment, thus
ensuring that the assignment is short-lived.
In this test, the subshell is employed precisely for this purpose; other
side-effects of the subshell, such as losing the effect of `test_tick`
which is invoked by `test_commit`, are immaterial.
These days, we can take advantage of `test_commit --author` to more
clearly convey that the test is interested only in overriding the author
of the commit.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The --format option on the git-ls-files man page states that `%xx`
interpolates to the character with hex code `xx`. This mirrors the
documentation and behavior of `git for-each-ref --format=...`. However,
in reality it requires the character with code `XX` to be specified as
`%xXX`, mirroring the behaviour of `git log --format`.
Signed-off-by: Jayson Rhynas <jayrhynas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The hashfile API is used to write out a "hashfile", which has a
final checksum (typically SHA-1) at the end. An in-core hashfile
structure has up to two file descriptors and a few buffers that can
only be freed by calling a helper function that is private to the
csum-file implementation.
The usual flow of a user of the API is to first open a file
descriptor for writing, obtain a hashfile associated with that write
file descriptor by calling either hashfd() or hashfd_check(), call
hashwrite() number of times to write data to the file, and then call
finalize_hashfile(), which appends th checksum to the end of the
file, closes file descriptors and releases associated buffers.
But what if a caller finds some error after calling hashfd() to
start the process and/or hashwrite() to send some data to the file,
and wants to abort the operation? The underlying file descriptor is
often managed by the tempfile API, so aborting will clean the file
out of the filesystem, but the resources associated with the in-core
hashfile structure is lost.
Introduce discard_hashfile() API function to allow them to release
the resources held by a hashfile structure the callers want to
dispose of, and use that in read-cache.c:do_write_index(), which is
a central place that writes the index file.
Mark t2107 as leak-free, as this leak in "update-index --cacheinfo"
test that deliberately makes it fail is now plugged.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a named object to read mailmap from is not a blob, the code
correctly errors out, but it forgot to free the object data before
doing so.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The documentation to "git rebase" says that the line numbers (in the
rebased change) may not exactly be the same as the line numbers the
change gets replayed on top of the new base, but uses a wrong noun
"fuzz". It should have said "offset".
They are both terms of art. "fuzz" is about context lines not
exactly matching. "offset" is about the difference in the location
that a change was taken from the original and the change gets
replayed on the target. "offset" is often inevitable and part of
normal life. "fuzz" on the other hand is often a sign of trouble
(and indeed "Git" refuses to apply a change with "fuzz", except
there are options to be fuzzy about whitespaces).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Make the print command trigger the pager when invoked using a capital
'P', to make it easier for the user to review long hunks.
Note that if the PAGER ends unexpectedly before we've been able to send
the payload, perhaps because the user is not interested in the whole
thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
terminate the interactive session for the user.
Therefore, we need to ignore a possible SIGPIPE signal. Add a test for
this, in addition to the test for normal operation.
For the SIGPIPE test, we need to make sure that we completely fill the
operating system's buffer, otherwise we might not trigger the SIGPIPE
signal. The normal size of this buffer in different OSs varies from a
few KBs to 1MB. Use a payload large enough to guarantee that we exceed
this limit.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.
That machinery, once set up, does not allow us to regain the original
stdio streams.
In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.
Modify the pager machinery so that we can use `setup_pager()` and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function `wait_for_pager()`. Make this
function reset the pager machinery before returning.
One specific point to note is that we avoid forking the pager in
`setup_pager()` if the configured pager is an empty string [*1*] or
simply "cat" [*2*]. In these cases, `setup_pager()` does nothing and
therefore `wait_for_pager()` should not be called.
We could modify `setup_pager()` to return an indication of these
situations, so we could avoid calling `wait_for_pager()`.
However, let's avoid transferring that responsibility to the caller and
instead treat the call to `wait_for_pager()` as a no-op when we know we
haven't forked the pager.
1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
2006-04-16)
2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)
Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We send errors to the pager since 61b80509e3 (sending errors to stdout
under $PAGER, 2008-02-16).
In a8335024c2 (pager: do not dup2 stderr if it is already redirected,
2008-12-15) an exception was introduced to avoid redirecting stderr if
it is not connected to a terminal.
In such exceptional cases, the close(STDERR_FILENO) we're doing in
close_pager_fds, is unnecessary.
Furthermore, in a subsequent commit we're going to introduce changes
that will involve using close_pager_fds multiple times.
With this in mind, controlling when we want to close stderr, become
sensible.
Let's close(STDERR_FILENO) only when necessary, and pave the way for the
upcoming changes.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a test for the 'p' command, which was introduced in 66c14ab592
(add-patch: introduce 'p' in interactive-patch, 2024-03-29).
Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
I saw some contributors hesitate to give a positive review on
patches by their coworkers. When written well, a positive review
does not have to be a hollow "looks good" that rubber stamps an
useless approval on a topic that is not interesting to others.
Let's add a few paragraphs to encourage positive reviews, which is a
bit harder to give than a review to point out things to improve.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Trivial change to indicate that branches and tags are real options
that can be used combined to get more information. This helps with
linting translations and prompting the user that the terms represent
options.
Signed-off-by: Alexander Shopov <ash@kambanaria.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
