When we iterate over the list of loose objects to check, we
get the actual path of each object. But we then throw it
away and pass just the sha1 to fsck_sha1(), which will do a
fresh lookup. Usually it would find the same object, but it
may not if an object exists both as a loose and a packed
object. We may end up checking the packed object twice, and
never look at the loose one.
In practice this isn't too terrible, because if fsck doesn't
complain, it means you have at least one good copy. But
since the point of fsck is to look for corruption, we should
be thorough.
The new read_loose_object() interface can help us get the
data from disk, and then we replace parse_object() with
parse_object_buffer(). As a bonus, our error messages now
mention the path to a corrupted object, which should make it
easier to track down errors when they do happen.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
It's surprisingly hard to ask the sha1_file code to open a
_specific_ incarnation of a loose object. Most of the
functions take a sha1, and loop over the various object
types (packed versus loose) and locations (local versus
alternates) at a low level.
However, some tools like fsck need to look at a specific
file. This patch gives them a function they can use to open
the loose object at a given path.
The implementation unfortunately ends up repeating bits of
related functions, but there's not a good way around it
without some major refactoring of the whole sha1_file stack.
We need to mmap the specific file, then partially read the
zlib stream to know whether we're streaming or not, and then
finally either stream it or copy the data to a buffer.
We can do that by assembling some of the more arcane
internal sha1_file functions, but we end up having to
essentially reimplement unpack_sha1_file(), along with the
streaming bits of check_sha1_signature().
Still, most of the ugliness is contained in the new
function, and the interface is clean enough that it may be
reusable (though it seems unlikely anything but git-fsck
would care about opening a specific file).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The code paths in fsck for packed and loose objects are
quite different, and it is not immediately obvious that the
packed case behaves well. In particular:
1. The fsck_loose() function always returns "0" to tell the
iterator to keep checking more objects. Whereas
fsck_obj_buffer() (which handles packed objects)
returns -1. This is OK, because the callback machinery
for verify_pack() does not stop when it sees a non-zero
return.
2. The fsck_loose() function sets the ERROR_OBJECT bit
when fsck_obj() fails, whereas fsck_obj_buffer() sets it
only when it sees a corrupt object. This turns out not
to matter. We don't actually do anything with this bit
except exit the program with a non-zero code, and that
is handled already by the non-zero return from the
function.
So there are no bugs here, but it was certainly confusing to
me. And we do not test either of the properties in t1450
(neither that a non-corruption error will caused a non-zero
exit for a packed object, nor that we keep going after
seeing the first error). Let's test both of those
conditions, so that we'll notice if any of those assumptions
becomes invalid.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
sha1_file: fix error message for alternate objects
When we fail to open a corrupt loose object, we report an
error and mention the filename via sha1_file_name().
However, that function will always give us a path in the
local repository, whereas the corrupt object may have come
from an alternate. The result is a very misleading error
message.
Teach the open_sha1_file() and stat_sha1_file() helpers to
pass back the path they found, so that we can report it
correctly.
Note that the pointers we return go to static storage (e.g.,
from sha1_file_name()), which is slightly dangerous.
However, these helpers are static local helpers, and the
names are used for immediately generating error messages.
The simplicity is an acceptable tradeoff for the danger.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit 90cf590f5 (fsck: optionally show more helpful info
for broken links, 2016-07-17) added a remove_loose_object()
helper, but we already had a remove_object() helper that did
the same thing. Let's combine these into one.
The implementations had a few subtle differences, so I've
tried to take the best of both:
- the original used "sed", but the newer version avoids
spawning an extra process
- the original processed "$*", which was nonsense, as it
assumed only a single sha1. Use "$1" to make that more
clear.
- the newer version ran an extra rev-parse, but it was not
necessary; it's sole caller already converted the
argument into a raw sha1
- the original used "rm -f", whereas the new one uses
"rm". The latter is better because it may notice a bug
or other unexpected failure in the test. (The original
does check that the object exists before we remove it,
which is good, but that's a subset of the possible
unexpected conditions).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* as/merge-attr-sleep:
t6026: clarify the point of "kill $(cat sleep.pid)"
t6026: ensure that long-running script really is
Revert "t6026-merge-attr: don't fail if sleep exits early"
Revert "t6026-merge-attr: ensure that the merge driver was called"
t6026-merge-attr: ensure that the merge driver was called
t6026-merge-attr: don't fail if sleep exits early
Merge branch 'ak/sh-setup-dot-source-i18n-fix' into maint
Recent update to git-sh-setup (a library of shell functions that
are used by our in-tree scripted Porcelain commands) included
another shell library git-sh-i18n without specifying where it is,
relying on the $PATH. This has been fixed to be more explicit by
prefixing $(git --exec-path) output in front.
* ak/sh-setup-dot-source-i18n-fix:
git-sh-setup: be explicit where to dot-source git-sh-i18n from.
Merge branch 'jk/daemon-path-ok-check-truncation' into maint
"git daemon" used fixed-length buffers to turn URL to the
repository the client asked for into the server side directory
path, using snprintf() to avoid overflowing these buffers, but
allowed possibly truncated paths to the directory. This has been
tightened to reject such a request that causes overlong path to be
required to serve.
* jk/daemon-path-ok-check-truncation:
daemon: detect and reject too-long paths
Merge branch 'mm/send-email-cc-cruft-after-address' into maint
"git send-email" attempts to pick up valid e-mails from the
trailers, but people in real world write non-addresses there, like
"Cc: Stable <add@re.ss> # 4.8+", which broke the output depending
on the availability and vintage of Mail::Address perl module.
* mm/send-email-cc-cruft-after-address:
Git.pm: add comment pointing to t9000
t9000-addresses: update expected results after fix
parse_mailboxes: accept extra text after <...> address
Merge branch 'cp/completion-negative-refs' into maint
The command-line completion script (in contrib/) learned to
complete "git cmd ^mas<HT>" to complete the negative end of
reference to "git cmd ^master".
* cp/completion-negative-refs:
completion: support excluding refs
Extract a small helper out of the function that reads the authors
script file "git am" internally uses.
This by itself is not useful until a second caller appears in the
future for "rebase -i" helper.
Since 650c44925 (common-main: call git_extract_argv0_path(),
2016-07-01), the argv[0] that is seen in cmd_main() of
individual programs is always the basename of the
executable, as common-main strips off the full path. This
can produce confusing results for git-daemon, which wants to
re-exec itself.
For instance, if the program was originally run as
"/usr/lib/git/git-daemon", it will try just re-execing
"git-daemon", which will find the first instance in $PATH.
If git's exec-path has not been prepended to $PATH, we may
find the git-daemon from a different version (or no
git-daemon at all).
Normally this isn't a problem. Git commands are run as "git
daemon", the git wrapper puts the exec-path at the front of
$PATH, and argv[0] is already "daemon" anyway. But running
git-daemon via its full exec-path, while not really a
recommended method, did work prior to 650c44925. Let's make
it work again.
The real goal of 650c44925 was not to munge argv[0], but to
reliably set the argv0_path global. The only reason it
munges at all is that one caller, the git.c wrapper,
piggy-backed on that computation to find the command
basename. Instead, let's leave argv[0] untouched in
common-main, and have git.c do its own basename computation.
While we're at it, let's drop the return value from
git_extract_argv0_path(). It was only ever used in this one
callsite, and its dual purposes is what led to this
confusion in the first place.
Note that by changing the interface, the compiler can
confirm for us that there are no other callers storing the
return value. But the compiler can't tell us whether any of
the cmd_main() functions (besides git.c) were relying on the
basename munging. However, we can observe that prior to 650c44925, no other cmd_main() functions did that munging,
and no new cmd_main() functions have been introduced since
then. So we can't be regressing any of those cases.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Merge tag 'l10n-2.11.0-rnd3' of git://github.com/git-l10n/git-po
l10n-2.11.0-rnd3
* tag 'l10n-2.11.0-rnd3' of git://github.com/git-l10n/git-po:
l10n: de.po: translate 210 new messages
l10n: fix unmatched single quote in error message
Since b9605bc4f2 ("config: only read .git/config from configured
repos", 2016-09-12), we do not read from ".git/config" unless we
know we are in a repository. "git archive" however didn't do the
repository discovery and instead relied on the old behaviour.
Teach the command to run a "gentle" version of repository discovery
so that local configuration variables are honoured.
[jc: stole tests from peff] Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since b9605bc4f2 ("config: only read .git/config from configured
repos", 2016-09-12), we do not read from ".git/config" unless we
know we are in a repository. "git mailinfo" however didn't do the
repository discovery and instead relied on the old behaviour. This
was mostly OK because it was merely run as a helper program by other
porcelain scripts that first chdir's up to the root of the working
tree.
Teach the command to run a "gentle" version of repository discovery
so that local configuration variables like mailinfo.scissors are
honoured.
In commit 1462450 ("trailer: allow non-trailers in trailer block",
2016-10-21), functionality was added (and tested [1]) to allow
non-trailer lines in trailer blocks, as long as those blocks contain at
least one Git-generated or user-configured trailer, and consists of at
least 25% trailers. The documentation was updated to mention this new
functionality, but did not mention "user-configured trailer".
Further update the documentation to also mention "user-configured
trailer".
[1] "with non-trailer lines mixed with a configured trailer" in
t/t7513-interpret-trailers.sh
Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When 84c9dc2 (commit: allow core.commentChar=auto for character auto
selection, 2014-05-17) extended the core.commentChar functionality to
allow for the value 'auto', it forgot that rebase -i was already taught to
handle core.commentChar, and in turn forgot to let rebase -i handle that
new value gracefully.
Reported by Taufiq Hoven.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The way "git stripspace" reads the configuration was not quite
kosher, in that the code forgot to probe for a possibly existing
repository (note: stripspace is designed to be usable outside the
repository as well). It read .git/config only when it was run from
the top-level of the working tree by accident. A recent change b9605bc4f2 ("config: only read .git/config from configured repos",
2016-09-12) stopped reading the repository-local configuration file
".git/config" unless the repository discovery process is done, so
that .git/config is never read even when run from the top-level,
exposing the old bug more.
When rebasing interactively with a commentChar defined in the
current repository's config, the help text at the bottom of the edit
script potentially used an incorrect comment character. This was not
only funny-looking, but also resulted in tons of warnings like this
one:
Warning: the command isn't recognized in the following line
- #
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
rebase -i: highlight problems with core.commentchar
The interactive rebase does not currently play well with
core.commentchar. Let's add some tests to highlight those problems
that will be fixed in the remainder of the series.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fixed unmatched single quote introduced by commit:
* f56fffef9a sequencer: teach write_message() to append an optional LF
Signed-off-by: Jiang Xin <worldhello.net@gmail.com> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
for-each-ref: do not segv with %(HEAD) on an unborn branch
The code to flip between "*" and " " prefixes depending on what
branch is checked out used in --format='%(HEAD)' did not consider
that HEAD may resolve to an unborn branch and dereferenced a NULL.
This will become a lot easier to trigger as the codepath will be
used to reimplement "git branch [--list]" in the future.
diffcore-delta: remove unused parameter to diffcore_count_changes()
The delta_limit parameter to diffcore_count_changes() has been unused
since commit ba23bbc8e ("diffcore-delta: make change counter to byte
oriented again.", 2006-03-04).
Remove the parameter and adjust all callers.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* as/merge-attr-sleep:
t6026: clarify the point of "kill $(cat sleep.pid)"
t6026: ensure that long-running script really is
Revert "t6026-merge-attr: don't fail if sleep exits early"
Revert "t6026-merge-attr: ensure that the merge driver was called"
t6026-merge-attr: ensure that the merge driver was called
t6026-merge-attr: don't fail if sleep exits early
The redirection of the standard error stream to a temporary file is
a leftover cruft during debugging. Remove it.
Besides, it is reported by folks on the Windows that the test is
flaky with this redirection; somebody gets confused and this
merely-redirected-to file gets marked as delete-pending by git.exe
and makes it finish with a non-zero exit status when "git checkout"
finishes. Windows folks may want to figure that one out, but for
the purpose of this test, it shouldn't become a show-stopper.
t6026: clarify the point of "kill $(cat sleep.pid)"
We lengthened the time the leftover process sleeps in the previous
commit to make sure it will be there while 'git merge' runs and
finishes. It therefore needs to be killed before leaving the test.
And it needs to be killed even when 'git merge' fails, so it has to
be triggered via test_when_finished mechanism.
Explain all that in a large comment, and move the use site of
test_when_finished to immediately before 'git merge' invocation,
where the process is spawned.
Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
We have to use $PWD instead of $(pwd) because on Windows the latter
would add a C: style path to bash's Unix-style $PATH variable, which
becomes confused by the colon after the drive letter. ($PWD is a
Unix-style path.)
In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
assembles a Unix-style path list with the colon as separators. It
converts the value to a Windows-style path list with the semicolon as
path separator when it forwards the variable to git.exe. The same
confusion happens when bash's original value is contaminated with
Windows style paths.
Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
With the introduction of the $GIT_COMMON_DIR variable, the
repository layout manual was changed to reflect the location for
many files in case the variable is set. While adding the new
locations, one typo snuck in regarding the location of the
'info/' folder, which is falsely claimed to reside at
"$GIT_COMMON_DIR/index".
Fix the typo to point to "$GIT_COMMON_DIR/info/" instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When making sure that background tasks are cleaned up in 5babb5b
(t6026-merge-attr: clean up background process at end of test case,
2016-09-07), we considered to let the background task sleep longer, just
to be certain that it will still be running when we want to kill it
after the test.
Sadly, the assumption appears not to hold true that the test case passes
quickly enough to kill the background task within a second.
Simply increase it to an hour. No system can be possibly slow enough to
make above-mentioned assumption incorrect.
Reported by Andreas Schwab.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The point of the test is that the stray process was still running
when 'git merge' did its thing through its completion, so a failure
to "kill" it means we didn't give a condition to the test to trigger
a possible future breakage. Appending "|| :" to the "kill" is
sweeping a test-bug under the rug.
Test portability improvements and cleanups for t0021.
* jk/filter-process-fix:
t0021: fix filehandle usage on older perl
t0021: use $PERL_PATH for rot13-filter.pl
t0021: put $TEST_ROOT in $PATH
t0021: use write_script to create rot13 shell script
TravisCI changed their default macOS image from 10.10 to 10.11 [1].
Unfortunately the HTTPD tests do not run out of the box using the
pre-installed Apache web server anymore. Therefore we enable these
tests only for Linux and disable them for macOS.
Apple removed the OpenSSL header files in macOS 10.11 and above. OpenSSL
was deprecated since macOS 10.7.
Set `NO_OPENSSL` and `APPLE_COMMON_CRYPTO` to `YesPlease` as default for
macOS. It is possible to override this and use OpenSSL by defining
`NO_APPLE_COMMON_CRYPTO`.
Original-patch-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Lars Schneider <larsxschneider@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This function used to have the caller pass in the current
value of HEAD, in order to make sure we didn't clobber HEAD.
In 55c4a6730, that logic moved to validate_new_branchname(),
which just resolves HEAD itself. The parameter to
create_branch is now unused.
Since we have to update and re-wrap the docstring describing
the parameters anyway, let's take this opportunity to break
it out into a list, which makes it easier to find the
parameters.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
sequencer.c:632:14: warning: comparison of constant 2 with
expression of type 'const enum todo_command' is always
true [-Wtautological-constant-out-of-range-compare]
if (command < ARRAY_SIZE(todo_command_strings))
This is because "command" is an enum that may only have two
values (0 and 1) and the array in question has two elements.
As it turns out, clang is actually wrong here, at least
according to its own bug tracker:
https://llvm.org/bugs/show_bug.cgi?id=16154
But it's still worth working around this, as the warning is
present with -Wall, meaning we fail compilation with "make
DEVELOPER=1".
Casting the enum to size_t sufficiently unconfuses clang. As
a bonus, it also catches any possible out-of-bounds access
if the enum takes on a negative value (which shouldn't
happen either, but again, this is a defensive check).
Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end
of test case") added a kill command to clean up after the test, but this
can fail if the sleep command exits before the cleanup is executed.
Ignore the error from the kill command.
Signed-off-by: Andreas Schwab <schwab@suse.de> Signed-off-by: Jeff King <peff@peff.net>
alternates: re-allow relative paths from environment
Commit 670c359da (link_alt_odb_entry: handle normalize_path
errors, 2016-10-03) regressed the handling of relative paths
in the GIT_ALTERNATE_OBJECT_DIRECTORIES variable. It's not
entirely clear this was ever meant to work, but it _has_
worked for several years, so this commit restores the
original behavior.
When we get a path in GIT_ALTERNATE_OBJECT_DIRECTORIES, we
add it the path to the list of alternate object directories
as if it were found in objects/info/alternates, but with one
difference: we do not provide the link_alt_odb_entry()
function with a base for relative paths. That function
doesn't turn it into an absolute path, and we end up feeding
the relative path to the strbuf_normalize_path() function.
Most relative paths break out of the top-level directory
(e.g., "../foo.git/objects"), and thus normalizing fails.
Prior to 670c359da, we simply ignored the error, and due to
the way normalize_path_copy() was implemented it happened to
return the original path in this case. We then accessed the
alternate objects using this relative path.
By storing the relative path in the alt_odb list, the path
is relative to wherever we happen to be at the time we do an
object lookup. That means we look from $GIT_DIR in a bare
repository, and from the top of the worktree in a non-bare
repository.
If this were being designed from scratch, it would make
sense to pick a stable location (probably $GIT_DIR, or even
the object directory) and use that as the relative base,
turning the result into an absolute path. However, given
the history, at this point the minimal fix is to match the
pre-670c359da behavior.
We can do this simply by ignoring the error when we have no
relative base and using the original value (which we now
reliably have, thanks to strbuf_normalize_path()).
That still leaves us with a relative path that foils our
duplicate detection, and may act strangely if we ever
chdir() later in the process. We could solve that by storing
an absolute path based on getcwd(). That may be a good
future direction; for now we'll do just the minimum to fix
the regression.
The new t5615 script demonstrates the fix in its final three
tests. Since we didn't have any tests of the alternates
environment variable at all, it also adds some tests of
absolute paths.
Reported-by: Bryan Turner <bturner@atlassian.com> Signed-off-by: Jeff King <peff@peff.net>
The rot13-filter.pl script calls methods on implicitly
defined filehandles (STDOUT, and the result of an open()
call). Prior to perl 5.13, these methods are not
automatically loaded, and perl will complain with:
Can't locate object method "flush" via package "IO::Handle"
Let's explicitly load IO::File (which inherits from
IO::Handle). That's more than we need for just "flush", but
matches what perl has done since:
The rot13-filter.pl script hardcodes "#!/usr/bin/perl", and
does not respect $PERL_PATH at all. That is a problem if the
system does not have perl at that path, or if it has a perl
that is too old to run a complicated script like the
rot13-filter (but PERL_PATH points to a more modern one).
We can fix this by using write_script() to create a new copy
of the script with the correct #!-line. In theory we could
move the whole script inside t0021-conversion.sh rather than
having it as an auxiliary file, but it's long enough that
it just makes things harder to read.
As a bonus, we can stop using the full path to the script in
the filter-process config we add (because the trash
directory is in our PATH). Not only is this shorter, but it
sidesteps any shell-quoting issues. The original was broken
when $TEST_DIRECTORY contained a space, because it was
interpolated in the outer script.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>