gitweb.git
t5318-commit-graph: remove unused variableSZEDER Gábor Fri, 22 Mar 2019 14:27:25 +0000 (15:27 +0100)

t5318-commit-graph: remove unused variable

This is a remnant from early versions of the commit-graph patch series
[1], when 'git commit-graph --write' printed the hash of the created
commit-graph file, and tests did look at the command's output, because
the commit-graph file's name included that hash as well.

[1] https://public-inbox.org/git/1517348383-112294-6-git-send-email-dstolee@microsoft.com/

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

checkout: prevent losing staged changes with --mergeNguyễn Thái Ngọc Duy Fri, 22 Mar 2019 09:31:38 +0000 (16:31 +0700)

checkout: prevent losing staged changes with --merge

When --merge is specified, we may need to do a real merge (instead of
three-way tree unpacking), the steps are best seen in git-checkout.sh
version before it's removed:

# Match the index to the working tree, and do a three-way.
git diff-files --name-only | git update-index --remove --stdin &&
work=`git write-tree` &&
git read-tree $v --reset -u $new || exit

git merge-recursive $old -- $new $work

# Do not register the cleanly merged paths in the index yet.
# this is not a real merge before committing, but just carrying
# the working tree changes along.
unmerged=`git ls-files -u`
git read-tree $v --reset $new
case "$unmerged" in
'') ;;
*)
(
z40=0000000000000000000000000000000000000000
echo "$unmerged" |
sed -e 's/^[0-7]* [0-9a-f]* /'"0 $z40 /"
echo "$unmerged"
) | git update-index --index-info
;;
esac

Notice the last 'read-tree --reset' step. We restore worktree back to
'new' tree after worktree's messed up by merge-recursive. If there are
staged changes before this whole command sequence is executed, they
are lost because they are unlikely part of the 'new' tree to be
restored.

There is no easy way to fix this. Elijah may have something up his
sleeves [1], but until then, check if there are staged changes and
refuse to run and lose them. The user would need to do "git reset" to
continue in this case.

A note about the test update. 'checkout -m' in that test will fail
because a deletion is staged. This 'checkout -m' was previously needed
to verify quietness behavior of unpack-trees. But a different check
has been put in place in the last patch. We can safely drop
'checkout -m' now.

[1] CABPp-BFoL_U=bzON4SEMaQSKU2TKwnOgNqjt5MUaOejTKGUJxw@mail.gmail.com

Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

read-tree: add --quietNguyễn Thái Ngọc Duy Fri, 22 Mar 2019 09:31:37 +0000 (16:31 +0700)

read-tree: add --quiet

read-tree is basically the front end of unpack-trees code and shoud
expose all of its functionality (unless it's designed for internal
use). This "opts.quiet" (formerly "opts.gently") was added for
builtin/checkout.c but there is no reason why other read-tree users
won't find this useful.

The test that is updated to run 'read-tree --quiet' was added because
unpack-trees was accidentally not being quiet [1] in 6a143aa2b2
(checkout -m: attempt merge when deletion of path was staged -
2014-08-12). Because checkout is the only "opts.quiet" user, there was
no other way to test quiet behavior. But we can now test it directly.

6a143aa2b2 was manually reverted to verify that read-tree --quiet
works correctly (i.e. test_must_be_empty fails).

[1] the commit message there say "errors out instead of performing a
merge" but I'm pretty sure the "performing a merge" happens anyway
even before that commit. That line should say "errors out
_in addition to_ performing a merge"

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

unpack-trees: rename "gently" flag to "quiet"Nguyễn Thái Ngọc Duy Fri, 22 Mar 2019 09:31:36 +0000 (16:31 +0700)

unpack-trees: rename "gently" flag to "quiet"

The gently flag was added in 17e4642667 (Add flag to make unpack_trees()
not print errors. - 2008-02-07) to suppress error messages. The name
"gently" does not quite express that. Granted, being quiet is gentle but
it could mean not performing some other actions. Rename the flag to
"quiet" to be more on point.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

unpack-trees: keep gently check inside add_rejected_pathNguyễn Thái Ngọc Duy Fri, 22 Mar 2019 09:31:35 +0000 (16:31 +0700)

unpack-trees: keep gently check inside add_rejected_path

This basically follows the footsteps of 6a143aa2b2 (checkout -m:
attempt merge when deletion of path was staged - 2014-08-12) where
there gently check is moved inside reject_merge() so that callers do
not accidentally forget it.

add_rejected_path() has the same usage pattern. All call sites check
gently first, then decide to call add_rejected_path() if needed. Move
the check inside.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

gc docs: clean grammar for "gc.bigPackThreshold"Ævar Arnfjörð Bjarmason Fri, 22 Mar 2019 09:32:34 +0000 (10:32 +0100)

gc docs: clean grammar for "gc.bigPackThreshold"

Clean up the grammar in the documentation for
"gc.bigPackThreshold". This documentation was added in 9806f5a7bf ("gc
--auto: exclude base pack if not enough mem to "repack -ad"",
2018-04-15).

Saying "the amount of memory estimated for" flows more smoothly than
the previous "the amount of memory is estimated not enough".

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

gc docs: stop noting "repack" flagsÆvar Arnfjörð Bjarmason Fri, 22 Mar 2019 09:32:33 +0000 (10:32 +0100)

gc docs: stop noting "repack" flags

Remove the mention of specific flags from the "gc" documentation, and
leave it at describing what we'll do instead. As seen in builtin/gc.c
we'll use various repack flags depending on what we detect we need to
do, so this isn't always accurate.

More importantly, a subsequent change is about to remove all this
documentation and replace it with an include of the gc.* docs in
git-config(1). By first changing this it's easier to reason about that
subsequent change.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

gc docs: modernize the advice for manually running... Ævar Arnfjörð Bjarmason Fri, 22 Mar 2019 09:32:32 +0000 (10:32 +0100)

gc docs: modernize the advice for manually running "gc"

The docs have been recommending that users need to run this manually,
but that hasn't been needed in practice for a long time except in
exceptional circumstances.

Let's instead have this reflect reality and say that most users don't
need to run this manually at all, while briefly describing the sorts
sort of cases where "gc" does need to be run manually.

Since we're recommending that users run this most of the and usually
don't need to tweak it, let's tone down the very prominent example of
the gc.auto=0 command. It's sufficient to point to the gc.auto
documentation below.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

t5551: mark half-auth no-op fetch test as v0-onlyJonathan Tan Fri, 22 Mar 2019 19:01:39 +0000 (12:01 -0700)

t5551: mark half-auth no-op fetch test as v0-only

When using protocol v0, upload-pack over HTTP permits a "half-auth"
configuration in which, at the web server layer, the info/refs path is
not protected by authentication but the git-upload-pack path is, so that
a user can perform fetches that do not download any objects without
authentication, but still needs authentication to download objects.

But protocol v2 does not support this, because both ref and pack are
obtained from the git-upload-pack path.

Mark the test verifying this behavior as protocol v0-only, with a
description of what needs to be done to make v2 support this.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

http: use normalize_curl_result() instead of manual... Jeff King Sun, 24 Mar 2019 12:13:16 +0000 (08:13 -0400)

http: use normalize_curl_result() instead of manual conversion

When we switched off CURLOPT_FAILONERROR in 17966c0a63 (http: avoid
disconnecting on 404s for loose objects, 2016-07-11), the fetch_object()
function started manually handling 404's. Since we now have
normalize_curl_result() for use elsewhere, we can use it here as well,
shortening the code.

Note that we lose the check for http/https in the URL here. None of the
other result-normalizing code paths bother with this. Looking at
missing_target(), which checks specifically for an FTP-specific CURLcode
and "http" code 550, it seems likely that git-over-ftp has been subtly
broken since 17966c0a63. This patch does nothing to fix that, but nor
should it make anything worse (in fact, it may be slightly better
because we'll actually recognize an error as such, rather than assuming
CURLE_OK means we actually got some data).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

http: normalize curl results for dumb loose and alterna... Jeff King Sun, 24 Mar 2019 12:09:46 +0000 (08:09 -0400)

http: normalize curl results for dumb loose and alternates fetches

If the dumb-http walker encounters a 404 when fetching a loose object,
it then looks at any http-alternates for the object. The 404 check is
implemented by missing_target(), which checks not only the http code,
but also that we got an http error from the CURLcode.

That broke when we stopped using CURLOPT_FAILONERROR in 17966c0a63
(http: avoid disconnecting on 404s for loose objects, 2016-07-11), since
our CURLcode will now be CURLE_OK. As a result, fetching over dumb-http
from a repository with alternates could result in Git printing "Unable
to find abcd1234..." and aborting.

We could probably fix this just by loosening missing_target(). However,
there's other code which looks at the curl result, and it would have to
be tweaked as well. Instead, let's just normalize the result the same
way the smart-http code does.

There's a similar case in processing the alternates (where we failover
from "info/http-alternates" to "info/alternates"). We'll give it the
same treatment.

After this patch, we should be hitting all code paths that need this
normalization (notably absent here is the http_pack_request path, but it
does not use FAILONERROR, nor missing_target()).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

http: factor out curl result code normalizationJeff King Sun, 24 Mar 2019 12:08:38 +0000 (08:08 -0400)

http: factor out curl result code normalization

We make some requests with CURLOPT_FAILONERROR and some without, and
then handle_curl_result() normalizes any failures to a uniform CURLcode.

There are some other code paths in the dumb-http walker which don't use
handle_curl_result(); let's pull the normalization into its own function
so it can be reused.

Arguably those code paths would benefit from the rest of
handle_curl_result(), notably the auth handling. But retro-fitting it
now would be a lot of work, and in practice it doesn't matter too much
(whatever authentication we needed to make the initial contact with the
server is generally sufficient for the rest of the dumb-http requests).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

commit: improve error message in "-a <paths>" caseNguyễn Thái Ngọc Duy Wed, 20 Mar 2019 10:29:06 +0000 (17:29 +0700)

commit: improve error message in "-a <paths>" case

I did something stupid today and got

$ git commit -a --fixup= @^
fatal: Paths with -a does not make sense.

which didn't make any sense (at least for the first few seconds).

Include the first path(spec) in the error message to help spot the
problem quicker. Now it shows

fatal: paths '@^ ...' with -a does not make sense

which should ring some bell because @^ should clearly not be considered
a path.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

refs/files-backend: don't look at an aborted transactionJeff King Thu, 21 Mar 2019 09:28:54 +0000 (05:28 -0400)

refs/files-backend: don't look at an aborted transaction

When deleting refs, we hold packed-refs.lock and prepare a packed
transaction to drop the refs from the packed-refs file. If it turns out
that we don't need to rewrite the packed refs (e.g., because none of the
deletions were present in the file), then we abort the transaction.

If that abort succeeds, then the transaction struct will have been
freed, and we set our local pointer to NULL so we don't look at it
again.

However, if it fails, then the struct will _still_ have been freed
(because ref_transaction_abort() always frees). But we don't clean up
the pointer, and will jump to our cleanup code, which will try to abort
it again, causing a use-after-free.

It's actually impossible for this to trigger in practice, since
packed_transaction_abort() will never return anything but success. But
let's fix it anyway, since that's more than we should assume about the
packed-refs code (after all, we are already bothering to check for an
error result which cannot be triggered).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

refs/files-backend: handle packed transaction prepare... Jeff King Thu, 21 Mar 2019 09:28:44 +0000 (05:28 -0400)

refs/files-backend: handle packed transaction prepare failure

In files_transaction_prepare(), if we have to delete some refs, we use a
subordinate packed_transaction to do so. It's rare for that
sub-transaction's prepare step to fail, since we hold the packed-refs
lock. But if it does, we trigger a BUG() due to these steps:

- we've attached the packed transaction to the files transaction as
backend_data->packed_transaction

- when the prepare step fails, the packed transaction cleans itself
up, putting itself into the CLOSED state

- the error value from preparing the packed transaction lets us know
in files_transaction_prepare() that we should also clean up and
return an error. We call files_transaction_cleanup(), which tries to
abort backend_data->packed_transaction. Since it's already CLOSED,
that triggers an assertion in ref_transaction_abort().

We can fix that by disconnecting the packed transaction from the outer
files transaction, and then free-ing (not aborting!) it ourselves.

A few other options/alternatives I considered:

- we could just make it a noop to abort a CLOSED transaction. But that
seems less safe, since clearly this code expects (and enforces) a
particular set of state transitions.

- we could have files_transaction_cleanup() selectively call abort()
vs free() based on the state of the on the packed transaction.
That's basically a more restricted version of the above, but also
potentially unsafe.

- instead of disconnecting backend_data->packed_transaction on error,
we could wait to install it until we successfully prepare. That
might make the flow a little simpler, but it introduces a hassle.
Earlier parts of files_transaction_prepare() that encounter an error
will jump to the cleanup label, and expect that cleaning up the
outer transaction will clean up the packed transaction, too. We'd
have to adjust those sites to clean up the packed transaction.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

unicode: update the width tables to Unicode 12Beat Bolli Thu, 21 Mar 2019 21:06:17 +0000 (22:06 +0100)

unicode: update the width tables to Unicode 12

Now that Unicode 12 has been announced[0], update the character
width tables to the new version.

[0] http://blog.unicode.org/2019/03/announcing-unicode-standard-version-120.html

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

midx: during verify group objects by packfile to speed... Jeff Hostetler Thu, 21 Mar 2019 19:36:15 +0000 (12:36 -0700)

midx: during verify group objects by packfile to speed verification

Teach `multi-pack-index verify` to sort the set of object by
packfile so that only one packfile needs to be open at a time.

This is a performance improvement. Previously, objects were
verified in OID order. This essentially requires all packfiles
to be open at the same time. If the number of packfiles exceeds
the open file limit, packfiles would be LRU-closed and re-opened
many times.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

midx: add progress indicators in multi-pack-index verifyJeff Hostetler Thu, 21 Mar 2019 19:36:14 +0000 (12:36 -0700)

midx: add progress indicators in multi-pack-index verify

Add progress indicators to more parts of midx verify.
Use sparse progress indicator for object iteration.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

trace2:data: add trace2 data to midxJeff Hostetler Thu, 21 Mar 2019 19:36:13 +0000 (12:36 -0700)

trace2:data: add trace2 data to midx

Log multi-pack-index command mode.
Log number of objects and packfiles in the midx.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

progress: add sparse mode to force 100% complete messageJeff Hostetler Thu, 21 Mar 2019 19:36:11 +0000 (12:36 -0700)

progress: add sparse mode to force 100% complete message

Add new start_sparse_progress() and start_delayed_sparse_progress()
constructors and "sparse" flag to struct progress.

Teach stop_progress() to force a 100% complete progress message before
printing the final "done" message when "sparse" is set.

Calling display_progress() for every item in a large set can
be expensive. If callers try to filter this for performance
reasons, such as emitting every k-th item, progress would
not reach 100% unless they made a final call to display_progress()
with the item count before calling stop_progress().

Now this is automatic when "sparse" is set.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

trace2: write to directory targetsJosh Steadmon Thu, 21 Mar 2019 21:09:51 +0000 (14:09 -0700)

trace2: write to directory targets

When the value of a trace2 environment variable is an absolute path
referring to an existing directory, write output to files (one per
process) underneath the given directory. Files will be named according
to the final component of the trace2 SID, followed by a counter to avoid
potential collisions.

This makes it more convenient to collect traces for every git invocation
by unconditionally setting the relevant trace2 envvar to a constant
directory name.

Signed-off-by: Josh Steadmon <steadmon@google.com>
Reviewed-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

checkout.txt: note about losing staged changes with... Nguyễn Thái Ngọc Duy Tue, 19 Mar 2019 09:39:10 +0000 (16:39 +0700)

checkout.txt: note about losing staged changes with --merge

If you have staged changes in path A and perform 'checkout
--merge' (which could result in conflicts in a totally unrelated path
B), changes in A will be gone. Which is unexpected. We are supposed
to keep all changes, or kick and scream otherwise.

This is the result of how --merge is implemented, from the very first
day in 1be0659efc (checkout: merge local modifications while switching
branches., 2006-01-12):

1. a merge is done, unmerged entries are collected
2. a hard switch to a new branch is done, then unmerged entries added
back

There is no trivial fix for this. Going with 3-way merge one file at a
time loses rename detection. Going with 3-way merge by trees requires
teaching the algorithm to pick up staged changes. And even if we detect
staged changes with --merge and abort for safety, an option to continue
--merge is very weird. Such an option would keep worktree changes, but
drop staged changes.

Because the problem has been with us since the introduction of --merge
and everybody has been pretty happy (except Phillip, who found this
problem), I'll just take a note here to acknowledge it and wait for
merge wizards to come in and work their magic. There may be a way
forward [1].

[1] CABPp-BFoL_U=bzON4SEMaQSKU2TKwnOgNqjt5MUaOejTKGUJxw@mail.gmail.com

Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

unpack-trees: fix oneway_merge accidentally carry over... Nguyễn Thái Ngọc Duy Mon, 18 Mar 2019 11:38:22 +0000 (18:38 +0700)

unpack-trees: fix oneway_merge accidentally carry over stage index

Phillip found out that 'git checkout -f <branch>' does not restore
conflict/unmerged files correctly. All tracked files should be taken
from <branch> and all non-zero stages removed. Most of this is true,
except that the final file could be in stage one instead of zero.

"checkout -f" (among other commands) does this with one-way merge, which
is supposed to take stat info from the index and everything else from
the given tree. The add_entry(.., old, ...) call in oneway_merge()
though will keep stage index from the index.

This is normally not a problem if the entry from the index is
normal (stage #0). But if there is a conflict, stage #0 does not exist
and we'll get stage #1 entry as "old" variable, which gets recorded in
the final index. Fix it by clearing stage mask.

This bug probably comes from b5b425074e (git-read-tree: make one-way
merge also honor the "update" flag, 2005-06-07). Before this commit, we
may create the final ("dst") index entry from the one in index, but we
do clear CE_STAGEMASK.

I briefly checked two- and three-way merge functions. I think we don't
have the same problem in those.

Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

parse_opt_ref_sorting: always use with NONEG flagJeff King Wed, 20 Mar 2019 20:22:15 +0000 (16:22 -0400)

parse_opt_ref_sorting: always use with NONEG flag

The "--sort" parameter of for-each-ref, etc, does not handle negation,
and instead returns an error to the parse-options code. But neither
piece of code prints anything for the user, which may leave them
confused:

$ git for-each-ref --no-sort
$ echo $?
129

As the comment in the callback function notes, this probably should
clear the list, which would make it consistent with other list-like
options (i.e., anything that uses OPT_STRING_LIST currently).
Unfortunately that's a bit tricky due to the way the ref-filter code
works. But in the meantime, let's at least make the error a little less
confusing:

- switch to using PARSE_OPT_NONEG in the option definition, which will
cause the options code to produce a useful message

- since this was cut-and-pasted to four different spots, let's define
a single OPT_REF_SORT() macro that we can use everywhere

- the callback can use BUG_ON_OPT_NEG() to make sure the correct flags
are used (incidentally, this also satisfies -Wunused-parameters,
since we're now looking at "unset")

- expand the comment into a NEEDSWORK to make it clear that the
direction is right, but the details need to be worked out

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

completion: use __git when calling --list-cmdsTodd Zullinger Wed, 20 Mar 2019 18:03:29 +0000 (14:03 -0400)

completion: use __git when calling --list-cmds

As we made --list-cmds read the local configuration file in an
earlier step, the completion.commands variable respects repo-level
configuration. Use __git which ensures that the proper repo config
is consulted if the command line contains 'git -C /some/other/repo'.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

completion: fix multiple command removalsJeff King Wed, 20 Mar 2019 18:03:28 +0000 (14:03 -0400)

completion: fix multiple command removals

Commit 6532f3740b ("completion: allow to customize the completable
command list", 2018-05-20) tried to allow multiple space-separated
entries in completion.commands. To do this, it copies each parsed token
into a strbuf so that the result is NUL-terminated.

However, for tokens starting with "-", it accidentally passes the
original non-terminated string, meaning that only the final one worked.
Switch to using the strbuf.

Reported-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

t9902: test multiple removals via completion.commandsTodd Zullinger Wed, 20 Mar 2019 18:03:27 +0000 (14:03 -0400)

t9902: test multiple removals via completion.commands

6532f3740b ("completion: allow to customize the completable command
list", 2018-05-20) added the completion.commands config variable.
Multiple commands may be added or removed, separated by a space.

Demonstrate the failure of multiple removals.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

git: read local config in --list-cmdsJeff King Wed, 20 Mar 2019 18:03:26 +0000 (14:03 -0400)

git: read local config in --list-cmds

Normally code that is checking config before we've decided to do
setup_git_directory() would use read_early_config(), which uses
discover_git_directory() to tentatively see if we're in a repo,
and if so to add it to the config sequence.

But list_cmds() uses the caching configset mechanism which
rightly does not use read_early_config(), because it has no
idea if it's being called early.

Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

stash: setup default diff output format if necessaryThomas Gummerer Wed, 20 Mar 2019 22:49:55 +0000 (22:49 +0000)

stash: setup default diff output format if necessary

In the scripted 'git stash show' when no arguments are passed, we just
pass '--stat' to 'git diff'. When any argument is passed to 'stash
show', we no longer pass '--stat' to 'git diff', and pass whatever
flags are passed directly through to 'git diff'.

By default 'git diff' shows the patch output. So when a user uses
'git stash show --patience', they would be shown the diff as expected,
using the patience algorithm. '--patience' in this case only changes
the diff algorithm, but does not cause 'git diff' to show the diff by
itself. The diff is shown because that's the default behaviour of
'git diff'.

In the C version of 'git stash show', we try to emulate that behaviour
using the internal diff API. However we forgot to set up the default
output format, in case it wasn't set by any of the flags that were
passed through. So 'git stash show --patience' in the builtin version
of stash would be completely silent, while it would show the diff in
the scripted version.

The same thing would happen for other flags that only affect the way a
patch is displayed, rather than switching to a different output format
than the default one.

Fix this by setting up the default output format for 'git diff'.

Reported-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

pretty: drop unused strbuf from parse_padding_placeholder()Jeff King Wed, 20 Mar 2019 08:16:42 +0000 (04:16 -0400)

pretty: drop unused strbuf from parse_padding_placeholder()

Unlike other parts of the --pretty user-format expansion,
this function is not actually writing to the output, but
instead just storing the padding values into a context
struct. We don't need to be passed a strbuf at all.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

pretty: drop unused "type" parameter in needs_rfc2047_e... Jeff King Wed, 20 Mar 2019 08:16:36 +0000 (04:16 -0400)

pretty: drop unused "type" parameter in needs_rfc2047_encoding()

The "should we encode" check was split off from add_rfc2047() into its
own function in 41dd00bad3 (format-patch: fix rfc2047 address encoding
with respect to rfc822 specials, 2012-10-18). But only the "add" half
needs to know the rfc2047_type, since it only affects _how_ we encode,
not whether we do.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

parse-options: drop unused ctx parameter from show_gitc... Jeff King Wed, 20 Mar 2019 08:16:27 +0000 (04:16 -0400)

parse-options: drop unused ctx parameter from show_gitcomp()

The completion display doesn't actually care about where we are in the
parsing. It's generated completely from the set of available options. So
we don't need to see the parse-options context struct at all.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

fetch_pack(): drop unused parametersJeff King Wed, 20 Mar 2019 08:16:14 +0000 (04:16 -0400)

fetch_pack(): drop unused parameters

We don't need the caller of fetch_pack() to pass in "dest", which is the
remote URL. Since ba227857d2 (Reduce the number of connects when
fetching, 2008-02-04), the caller is responsible for calling
git_connect() itself, and our "dest" parameter is unused.

That commit also started passing us the resulting "conn" child_process
from git_connect(). But likewise, we do not need do anything with it.
The descriptors in "fd" are enough for us, and the caller is responsible
for cleaning up "conn".

We can just drop both parameters.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

report_path_error(): drop unused prefix parameterJeff King Wed, 20 Mar 2019 08:15:48 +0000 (04:15 -0400)

report_path_error(): drop unused prefix parameter

This hasn't been used since 17ddc66e70 (convert report_path_error to
take struct pathspec, 2013-07-14), as the names in the struct will have
already been prefixed when they were parsed.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

unpack-trees: drop unused error_type parametersJeff King Wed, 20 Mar 2019 08:15:27 +0000 (04:15 -0400)

unpack-trees: drop unused error_type parameters

The verify_clean_subdirectory() helper takes an error_type parameter
from the caller, but doesn't actually use it. Instead, when it calls
add_rejected_path() it passes NOT_UPTODATE_DIR, its own custom error
type which is more specific than what the caller provides. Likewise for
verify_clean_submodule(), which always passes WOULD_LOSE_SUBMODULE.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

unpack-trees: drop name_entry from traverse_by_cache_tree()Jeff King Wed, 20 Mar 2019 08:15:07 +0000 (04:15 -0400)

unpack-trees: drop name_entry from traverse_by_cache_tree()

We pull the names from the existing index rather than the tree entry,
which is after all the point of this function. Let's drop the unused
"names" parameter.

Note that we leave the "nr_names" parameter, as it tells us how many
trees we are traversing (and thus how many index stages to set up).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

test-date: drop unused "now" parameter from parse_dates()Jeff King Wed, 20 Mar 2019 08:14:30 +0000 (04:14 -0400)

test-date: drop unused "now" parameter from parse_dates()

We only need the current time for relative dates like "5
minutes ago", and those are parsed only through approxidate,
not the strict parser used by parse_dates().

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

update-index: drop unused prefix_length parameter from... Jeff King Wed, 20 Mar 2019 08:14:07 +0000 (04:14 -0400)

update-index: drop unused prefix_length parameter from do_reupdate()

The prefix is always a NUL-terminated string, and we just end up passing
it along to parse_pathspec() anyway (which does not even take a length).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

log: drop unused "len" from show_tagger()Jeff King Wed, 20 Mar 2019 08:14:01 +0000 (04:14 -0400)

log: drop unused "len" from show_tagger()

We pass the length of the found "tagger" line to show_tagger(), but it
does not use it; instead, it passes the string to pp_user_info(), which
reads until newline or NUL. This is OK for our purposes because we
always read the object contents into a buffer with an extra NUL (and
indeed, our sole caller already relies on this by using starts_with).

Let's drop the ignored parameter. And while we're touching the caller,
let's use skip_prefix() to avoid a magic number.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

log: drop unused rev_info from early outputJeff King Wed, 20 Mar 2019 08:13:42 +0000 (04:13 -0400)

log: drop unused rev_info from early output

The early output code passes around a rev_info struct but doesn't need
it. The setup step only turns on global signal handlers, and the
"estimate" step is done completely from the rev->commits list that is
passed in separately.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

revision: drop some unused "revs" parametersJeff King Wed, 20 Mar 2019 08:13:36 +0000 (04:13 -0400)

revision: drop some unused "revs" parameters

There are several internal helpers that take a rev_info struct but don't
actually look at it. While one could argue that all helpers in
revision.c should take a rev_info struct for consistency, dropping the
unused parameter makes it clear that they don't actually depend on any
other rev options.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

The third batchJunio C Hamano Wed, 20 Mar 2019 06:19:48 +0000 (15:19 +0900)

The third batch

Signed-off-by: Junio C Hamano <gitster@pobox.com>

Merge branch 'br/commit-tree-parseopt'Junio C Hamano Wed, 20 Mar 2019 06:16:08 +0000 (15:16 +0900)

Merge branch 'br/commit-tree-parseopt'

The command line parser of "git commit-tree" has been rewritten to
use the parse-options API.

* br/commit-tree-parseopt:
commit-tree: utilize parse-options api

Merge branch 'jk/config-type-color-ends-with-lf'Junio C Hamano Wed, 20 Mar 2019 06:16:07 +0000 (15:16 +0900)

Merge branch 'jk/config-type-color-ends-with-lf'

"git config --type=color ..." is meant to replace "git config --get-color"
but there is a slight difference that wasn't documented, which is
now fixed.

* jk/config-type-color-ends-with-lf:
config: document --type=color output is a complete line

Merge branch 'ma/clear-repository-format'Junio C Hamano Wed, 20 Mar 2019 06:16:07 +0000 (15:16 +0900)

Merge branch 'ma/clear-repository-format'

The setup code has been cleaned up to avoid leaks around the
repository_format structure.

* ma/clear-repository-format:
setup: fix memory leaks with `struct repository_format`
setup: free old value before setting `work_tree`

Merge branch 'jk/virtual-objects-do-exist'Junio C Hamano Wed, 20 Mar 2019 06:16:07 +0000 (15:16 +0900)

Merge branch 'jk/virtual-objects-do-exist'

A recent update broke "is this object available to us?" check for
well-known objects like an empty tree (which should yield "yes",
even when there is no on-disk object for an empty tree), which has
been corrected.

* jk/virtual-objects-do-exist:
rev-list: allow cached objects in existence check

Merge branch 'jk/no-sigpipe-during-network-transport'Junio C Hamano Wed, 20 Mar 2019 06:16:06 +0000 (15:16 +0900)

Merge branch 'jk/no-sigpipe-during-network-transport'

On platforms where "git fetch" is killed with SIGPIPE (e.g. OSX),
the upload-pack that runs on the other end that hangs up after
detecting an error could cause "git fetch" to die with a signal,
which led to a flakey test. "git fetch" now ignores SIGPIPE during
the network portion of its operation (this is not a problem as we
check the return status from our write(2)s).

* jk/no-sigpipe-during-network-transport:
fetch: ignore SIGPIPE during network operation
fetch: avoid calling write_or_die()

Merge branch 'jk/fsck-doc'Junio C Hamano Wed, 20 Mar 2019 06:16:05 +0000 (15:16 +0900)

Merge branch 'jk/fsck-doc'

"git fsck --connectivity-only" omits computation necessary to sift
the objects that are not reachable from any of the refs into
unreachable and dangling. This is now enabled when dangling
objects are requested (which is done by default, but can be
overridden with the "--no-dangling" option).

* jk/fsck-doc:
fsck: always compute USED flags for unreachable objects
doc/fsck: clarify --connectivity-only behavior

Merge branch 'js/stress-test-ui-tweak'Junio C Hamano Wed, 20 Mar 2019 06:16:05 +0000 (15:16 +0900)

Merge branch 'js/stress-test-ui-tweak'

Dev support.

* js/stress-test-ui-tweak:
tests: introduce --stress-jobs=<N>
tests: let --stress-limit=<N> imply --stress

Merge branch 'js/rebase-orig-head-fix'Junio C Hamano Wed, 20 Mar 2019 06:16:05 +0000 (15:16 +0900)

Merge branch 'js/rebase-orig-head-fix'

"git rebase" that was reimplemented in C did not set ORIG_HEAD
correctly, which has been corrected.

* js/rebase-orig-head-fix:
built-in rebase: set ORIG_HEAD just once, before the rebase
built-in rebase: demonstrate that ORIG_HEAD is not set correctly
built-in rebase: use the correct reflog when switching branches
built-in rebase: no need to check out `onto` twice

Merge branch 'jk/bisect-final-output'Junio C Hamano Wed, 20 Mar 2019 06:16:04 +0000 (15:16 +0900)

Merge branch 'jk/bisect-final-output'

The final report from "git bisect" used to show the suspected
culprit using a raw "diff-tree", with which there is no output for
a merge commit. This has been updated to use a more modern and
human readable output that still is concise enough.

* jk/bisect-final-output:
bisect: make diff-tree output prettier
bisect: fix internal diff-tree config loading
bisect: use string arguments to feed internal diff-tree

Merge branch 'ab/makefile-help-devs-more'Junio C Hamano Wed, 20 Mar 2019 06:16:04 +0000 (15:16 +0900)

Merge branch 'ab/makefile-help-devs-more'

CFLAGS now can be tweaked when invoking Make while using
DEVELOPER=YesPlease; this did not work well before.

* ab/makefile-help-devs-more:
Makefile: allow for combining DEVELOPER=1 and CFLAGS="..."
Makefile: move the setting of *FLAGS closer to "include"
Makefile: Move *_LIBS assignment into its own section
Makefile: add/remove comments at top and tweak whitespace
Makefile: move "strip" assignment down from flags
Makefile: remove an out-of-date comment

trace2: NULL is not allowed for va_listTorsten Bögershausen Tue, 19 Mar 2019 17:13:46 +0000 (17:13 +0000)

trace2: NULL is not allowed for va_list

Some compilers don't allow NULL to be passed for a va_list,
and e.g. "gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516"
errors out like this:
trace2/tr2_tgt_event.c:193:18:
error: invalid operands to binary &&
(have ‘int’ and ‘va_list {aka __va_list}’)
if (fmt && *fmt && ap) {
^^
I couldn't find any hints that va_list and pointers can be mixed,
and no hints that they can't either. Morten Welinder comments:

"C99, Section 7.15, simply says that va_list "is an object type suitable for
holding information needed by the macros va_start, va_end, and
va_copy". So clearly not guaranteed to be mixable with pointers...

The portable solution is to use "va_list" everywhere in the callchain.
As a consequence, both trace2_region_enter_fl() and trace2_region_leave_fl()
now take a variable argument list.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Acked-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

rebase: remove the rebase.useBuiltin settingÆvar Arnfjörð Bjarmason Mon, 18 Mar 2019 11:01:52 +0000 (12:01 +0100)

rebase: remove the rebase.useBuiltin setting

Remove the rebase.useBuiltin setting, which was added as an escape
hatch to disable the builtin version of rebase first released with Git
2.20.

See [1] for the initial implementation of rebase.useBuiltin, and [2]
and [3] for the documentation and corresponding
GIT_TEST_REBASE_USE_BUILTIN option.

Carrying the legacy version is a maintenance burden as seen in
7e097e27d3 ("legacy-rebase: backport -C<n> and --whitespace=<option>
checks", 2018-11-20) and 9aea5e9286 ("rebase: fix regression in
rebase.useBuiltin=false test mode", 2019-02-13). Since the built-in
version has been shown to be stable enough let's remove the legacy
version.

As noted in [3] having use_builtin_rebase() shell out to get its
config doesn't make any sense anymore, that was done for the purposes
of spawning the legacy rebase without having modified any global
state. Let's instead handle this case in rebase_config().

There's still a bunch of references to git-legacy-rebase in po/*.po,
but those will be dealt with in time by the i18n effort.

Even though this configuration variable only existed two releases
let's not entirely delete the entry from the docs, but note its
absence. Individual versions of git tend to be around for a while due
to distro packaging timelines, so e.g. if we're "lucky" a given
version like 2.21 might be installed on say OSX for half a decade.

That'll mean some people probably setting this in config, and then
when they later wonder if it's needed they can Google search the
config option name or check it in git-config. It also allows us to
refer to the docs from the warning for details.

1. 55071ea248 ("rebase: start implementing it as a builtin",
2018-08-07)
2. d8d0a546f0 ("rebase doc: document rebase.useBuiltin", 2018-11-14)
3. 62c23938fa ("tests: add a special setup where rebase.useBuiltin is
off", 2018-11-14)
3. https://public-inbox.org/git/nycvar.QRO.7.76.6.1903141544110.41@tvgsbejvaqbjf.bet/

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

cherry-pick --continue: remember optionsPhillip Wood Wed, 13 Mar 2019 18:26:15 +0000 (18:26 +0000)

cherry-pick --continue: remember options

Remember --allow-empty, --allow-empty-message and
--keep-redundant-commits when cherry-pick stops for a conflict
resolution.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

cherry-pick: demonstrate option amnesiaPhillip Wood Wed, 13 Mar 2019 18:26:14 +0000 (18:26 +0000)

cherry-pick: demonstrate option amnesia

When cherry-pick stops for a conflict resolution it forgets
--allow-empty --allow-empty-message and --keep-redundant-commits.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

sequencer: break some long linesPhillip Wood Wed, 13 Mar 2019 18:26:13 +0000 (18:26 +0000)

sequencer: break some long lines

reformat save_opts() to remove excessively long lines.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

doc-diff: add `--cut-header-footer`Martin Ågren Sun, 17 Mar 2019 18:36:03 +0000 (19:36 +0100)

doc-diff: add `--cut-header-footer`

AsciiDoc and Asciidoctor do not agree on what to write in the header and
footer of each man-page, i.e., the very first and the very last line of
*.[157]. Those differences can certainly be interesting in their own
right, but they clutter the output of `./doc-diff --from-asciidoc
--to-asciidoctor HEAD HEAD` quite a bit since the diff contains some
10-15 lines of noise per file diffed.

Teach doc-diff to cut away the first two and last two lines, i.e., the
header/footer and the empty line immediately following/preceding it.
Because Asciidoctor uses an extra empty line compared to AsciiDoc,
remove one more line at each end of the file, but only if it's empty.

An alternative approach might be to pass down `--no-header-footer`,
which both AsciiDoc and Asciidoctor understand, but it has some
drawbacks. First of all, the result doesn't build -- `xmlto` stumbles on
the resulting xml since it has multiple root elements. Second, it cuts
too much -- dropping the header loses the synopsis, which would be
interesting to diff.

Like in the previous commit, encode this option into the directory name
of the "installed" and "rendered" files. Otherwise, we wouldn't be able
to trust that what we use out of that cache actually corresponds to the
options given for this run. (We could optimize this caching a little
since this flag doesn't affect the contents of "installed" at all, but
let's punt on that.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

doc-diff: support diffing from/to AsciiDoc(tor)Martin Ågren Sun, 17 Mar 2019 18:36:02 +0000 (19:36 +0100)

doc-diff: support diffing from/to AsciiDoc(tor)

Provide `--from-asciidoctor` and `--to-asciidoctor` to select that the
"from" resp. "to" commit should be built with Asciidoctor, and provide
an `--asciidoctor` shortcut for giving both. Similarly, provide
--{from-,to-,}asciidoc for explicitly selecting AsciiDoc.

Implement this using the USE_ASCIIDOCTOR flag. Let's not enforce a
default here, but instead just let the Makefile fall back on whatever is
in config.mak, so that `./doc-diff foo bar` without any of of these new
options behaves exactly like it did before this commit.

Encode the choice into the directory names of our "installed" and
"rendered" files, so that we can run `./doc-diff --from-asciidoc
--to-asciidoctor HEAD HEAD` without our two runs stomping on each other.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

doc-diff: let `render_tree()` take an explicit director... Martin Ågren Sun, 17 Mar 2019 18:36:01 +0000 (19:36 +0100)

doc-diff: let `render_tree()` take an explicit directory name

In `render_tree()`, `$1` is documented to be the commit-ish/oid and we
use it as that with `git checkout`, but we mostly use it to form the
name of various directories. To separate these concerns, and because we
are about to construct the directory names a bit differently, take two
distinct arguments instead.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Doc: auto-detect changed build flagsMartin Ågren Sun, 17 Mar 2019 18:36:00 +0000 (19:36 +0100)

Doc: auto-detect changed build flags

If you build the documentation switching between different options,
e.g., to build with both Asciidoc and Asciidoctor, you'll probably find
yourself running `make -C Documentation clean` either too often (wasting
time) or too rarely (getting mixed builds).

Track the flags we're using in the documentation build, similar to how
the main Makefile tracks CFLAGS and prefix flags. Track ASCIIDOC_COMMON
directly rather than its individual components -- that should make it
harder to forget to update the tracking if/when we modify the build
commands.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

t4038-diff-combined: quote paths with whitespaceTodd Zullinger Sun, 17 Mar 2019 23:02:39 +0000 (19:02 -0400)

t4038-diff-combined: quote paths with whitespace

d76ce4f734 ("log,diff-tree: add --combined-all-paths option",
2019-02-07) added tests for files containing tabs.

When the tests are run with bash, the lack of quoting during the file
setup causes 'ambiguous redirect' errors.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

reflog tests: test for the "points nowhere" warningÆvar Arnfjörð Bjarmason Fri, 15 Mar 2019 15:59:56 +0000 (16:59 +0100)

reflog tests: test for the "points nowhere" warning

The "git reflog expire" command when given an unknown reference has
since 4264dc15e1 ("git reflog expire", 2006-12-19) when this command
was implemented emit an error, but this has never been tested for.

Let's test for it, also under gc.reflogExpire{Unreachable,}=never in
case a future change is tempted to take shortcuts in the presence of
such config.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

reflog tests: make use of "test_config" idiomÆvar Arnfjörð Bjarmason Fri, 15 Mar 2019 15:59:55 +0000 (16:59 +0100)

reflog tests: make use of "test_config" idiom

Change a couple of tests that weren't using the helper to use it. This
makes the trailing "--unset" unnecessary.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

gc: refactor a "call me once" patternÆvar Arnfjörð Bjarmason Fri, 15 Mar 2019 15:59:54 +0000 (16:59 +0100)

gc: refactor a "call me once" pattern

Change an idiom we're using to ensure that gc_before_repack() only
does work once (see 62aad1849f ("gc --auto: do not lock refs in the
background", 2014-05-25)) to be more obvious.

Nothing except this function cares about the "pack_refs" and
"prune_reflogs" variables, so let's not leave the reader wondering if
they're being zero'd out for later use somewhere else.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

gc: convert to using the_hash_algoÆvar Arnfjörð Bjarmason Fri, 15 Mar 2019 15:59:53 +0000 (16:59 +0100)

gc: convert to using the_hash_algo

There's been a lot of changing of the hardcoded "40" values to
the_hash_algo->hexsz, but we've so far missed this one where we
hardcoded 38 for the loose object file length.

This is because a SHA-1 like abcde[...] gets turned into
objects/ab/cde[...]. There's no reason to suppose the same won't be
the case for SHA-256, and reading between the lines in
hash-function-transition.txt the format is planned to be the same.

In the future we may want to further modify this code for the hash
function transition. There's a potential pathological case here where
we'll only consider the loose objects for the currently active hash,
but objects for that hash will share a directory storage with the
other hash.

Thus we could theoretically have e.g. 1k SHA-1 loose objects, and 1
million SHA-256 objects. Then not notice that we need to pack them
because we're currently using SHA-1, even though our FS may be
straining under the stress of such humongous directories.

So assuming that "gc" eventually learns to pack up both SHA-1 and
SHA-256 objects regardless of what the current the_hash_algo is,
perhaps this check should be changed to consider all files in
objects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both
SHA-1 and SHA-256).

But none of that is something we need to worry about now, and
supporting both 38 and 62 characters depending on "the_hash_algo"
removes another case of SHA-1 hardcoding.

As noted in [1] I'm making no effort to somehow remove the hardcoding
for "2" as in "use the first two hexdigits for the directory
name". There's no indication that that'll ever change, and somehow
generalizing it here would be a drop in the ocean, so there's no point
in doing that. It also couldn't be done without coming up with some
generalized version of the magical "objects/17" directory. See [2] for
a discussion of that directory.

1. https://public-inbox.org/git/874l84ber7.fsf@evledraar.gmail.com/

2. https://public-inbox.org/git/87k1mta9x5.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

perf-lib.sh: rely on test-lib.sh for --tee handlingJeff King Fri, 15 Mar 2019 06:14:17 +0000 (02:14 -0400)

perf-lib.sh: rely on test-lib.sh for --tee handling

Since its inception, the perf-lib.sh script has manually handled the
"--tee" option (and other options which imply it, like "--valgrind")
with a cut-and-pasted block from test-lib.sh. That block has grown stale
over the years, and has at least three problems:

1. It uses $SHELL to re-exec the script, whereas the version in
test-lib.sh learned to use $TEST_SHELL_PATH.

2. It does an ad-hoc search of the "$*" string, whereas test-lib.sh
learned to carefully parse the arguments left to right.

3. It never learned about --verbose-log (which also implies --tee),
so it would not trigger for that option.

This last one was especially annoying, because t/perf/run uses the
GIT_TEST_OPTS from your config.mak to run the perf scripts. So if you've
set, say, "-x --verbose-log" there, it will be passed as part of most
perf runs. And while this script doesn't recognize the option, the
test-lib.sh that we source _does_, and the behavior ends up being much
more annoying:

- as the comment at the top of the block says, we have to run this
tee code early, before we start munging variables (it says
GIT_BUILD_DIR, but the problematic variable is actually
GIT_TEST_INSTALLED).

- since we don't recognize --verbose-log, we don't trigger the block.
We go on to munge GIT_TEST_INSTALLED, converting it from a relative
to an absolute path.

- then we source test-lib.sh, which _does_ recognize --verbose-log. It
re-execs the script, which runs again. But this time with an
absolute version of GIT_TEST_INSTALLED.

- As a result, we copy the absolute version of GIT_TEST_INSTALLED into
perf_results_prefix. Instead of writing our results to the expected
"test-results/build_1234abcd.p1234-whatever.times", we instead write
them to "test-results/_full_path_to_repo_t_perf_build_1234...".

The aggregate.perl script doesn't expect this, and so it prints
"<missing>" for each result (even though it spent considerable time
running the tests!).

We can solve all of these in one blow by just deleting our custom
handling, and relying on the inclusion of test-lib.sh to handle --tee,
--verbose-log, etc.

There's one catch, though. We want to handle GIT_TEST_INSTALLED after
we've included test-lib.sh, since we want it un-munged in the re-exec'd
version of the script. But if we want to convert it from a relative
to an absolute path, we must do so before we load test-lib.sh, since it
will change our working directory. So we compute the absolute directory
first, store it away, then include test-lib.sh, and finally assign to
GIT_TEST_INSTALLED as appropriate.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

doc: fix typos in man pagesAlexander Blesius Sat, 16 Mar 2019 10:34:39 +0000 (11:34 +0100)

doc: fix typos in man pages

Signed-off-by: Alexander Blesius <alexander+git@blesius.eu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

fix pack protocol example client/server communicationMike Hommey Sat, 16 Mar 2019 08:57:07 +0000 (17:57 +0900)

fix pack protocol example client/server communication

The pkt-line formatted lines contained the wrong pkt-len.

Signed-off-by: Mike Hommey <mh@glandium.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

pack-objects: default to writing bitmap hash-cacheJeff King Fri, 15 Mar 2019 06:25:28 +0000 (02:25 -0400)

pack-objects: default to writing bitmap hash-cache

Enabling pack.writebitmaphashcache should always be a performance win.
It costs only 4 bytes per object on disk, and the timings in ae4f07fbcc
(pack-bitmap: implement optional name_hash cache, 2013-12-21) show it
improving fetch and partial-bitmap clone times by 40-50%.

The only reason we didn't enable it by default at the time is that early
versions of JGit's bitmap reader complained about the presence of
optional header bits it didn't understand. But that was changed in
JGit's d2fa3987a (Use bitcheck to check for presence of OPT_FULL option,
2013-10-30), which made it into JGit v3.5.0 in late 2014.

So let's turn this option on by default. It's backwards-compatible with
all versions of Git, and if you are also using JGit on the same
repository, you'd only run into problems using a version that's almost 5
years old.

We'll drop the manual setting from all of our test scripts, including
perf tests. This isn't strictly necessary, but it has two advantages:

1. If the hash-cache ever stops being enabled by default, our perf
regression tests will notice.

2. We can use the modified perf tests to show off the behavior of an
otherwise unconfigured repo, as shown below.

These are the results of a few of a perf tests against linux.git that
showed interesting results. You can see the expected speedup in 5310.4,
which was noted in ae4f07fbcc. Curiously, 5310.8 did not improve (and
actually got slower), despite seeing the opposite in ae4f07fbcc.
I don't have an explanation for that.

The tests from p5311 did not exist back then, but do show improvements
(a smaller pack due to better deltas, which we found in less time).

Test HEAD^ HEAD
-------------------------------------------------------------------------------------
5310.4: simulated fetch 7.39(22.70+0.25) 5.64(11.43+0.22) -23.7%
5310.8: clone (partial bitmap) 18.45(24.83+1.19) 19.94(28.40+1.36) +8.1%
5311.31: server (128 days) 0.41(1.13+0.05) 0.34(0.72+0.02) -17.1%
5311.32: size (128 days) 7.4M 7.0M -4.8%
5311.33: client (128 days) 1.33(1.49+0.06) 1.29(1.37+0.12) -3.0%

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

t5310: correctly remove bitmaps for jgit testJeff King Fri, 15 Mar 2019 06:22:44 +0000 (02:22 -0400)

t5310: correctly remove bitmaps for jgit test

We use "jgit gc" to generate a pack bitmap file, and then make sure our
implementation can read it. To prepare the repo before running jgit, we
try to "rm -f" any existing bitmap files. But we got the path wrong;
we're in a bare repo, so looking in ".git/" finds nothing. Our "rm"
doesn't complain because of the "-f", and when we run "rev-list" there
are two bitmap files (ours and jgit's).

Our reader implementation will ignore one of the bitmap files, but it's
likely non-deterministic which one we will use. We'd prefer the one with
the more recent timestamp (just because of the way the packed_git list
is sorted), but in most test runs they'd have identical timestamps.

So this was probably actually testing something useful about 50% of the
time, and other half just testing that we could read our own bitmaps
(which is covered elsewhere).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

repack: enable bitmaps by default on bare reposEric Wong Thu, 14 Mar 2019 09:12:54 +0000 (09:12 +0000)

repack: enable bitmaps by default on bare repos

A typical use case for bare repos is for serving clones and
fetches to clients. Enable bitmaps by default on bare repos to
make it easier for admins to host git repos in a performant way.

Signed-off-by: Eric Wong <e@80x24.org>
Helped-by: Jeff King <peff@peff.net>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

glossary: add definition for overlayThomas Gummerer Sun, 17 Mar 2019 20:19:56 +0000 (20:19 +0000)

glossary: add definition for overlay

Add a definition for what overlay means in the context of git, to
clarify the recently introduced overlay-mode in git checkout.

Helped-by: Elijah Newren <newren@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

difftool: allow running outside Git worktrees with... Johannes Schindelin Thu, 14 Mar 2019 11:25:04 +0000 (04:25 -0700)

difftool: allow running outside Git worktrees with --no-index

As far as this developer can tell, the conversion from a Perl script to
a built-in caused the regression in the difftool that it no longer runs
outside of a Git worktree (with `--no-index`, of course).

It is a bit embarrassing that it took over two years after retiring the
Perl version to discover this regression, but at least we now know, and
can do something, about it.

This fixes https://github.com/git-for-windows/git/issues/2123

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

parse-options: make OPT_ARGUMENT() more usefulJohannes Schindelin Thu, 14 Mar 2019 11:25:04 +0000 (04:25 -0700)

parse-options: make OPT_ARGUMENT() more useful

`OPT_ARGUMENT()` is intended to keep the specified long option in `argv`
and not to do anything else.

However, it would make a lot of sense for the caller to know whether
this option was seen at all or not. For example, we want to teach `git
difftool` to work outside of any Git worktree, but only when
`--no-index` was specified.

Note: nothing in Git uses OPT_ARGUMENT(). Even worse, looking through
the commit history, one can easily see that nothing even
ever used it, apart from the regression test.

So not only do we make `OPT_ARGUMENT()` more useful, we are also about
to introduce its first real user!

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

difftool: remove obsolete (and misleading) commentJohannes Schindelin Thu, 14 Mar 2019 11:25:03 +0000 (04:25 -0700)

difftool: remove obsolete (and misleading) comment

We will always spawn something from `git difftool`, so we will always
have to set `GIT_DIR` and `GIT_WORK_TREE`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Merge git://ozlabs.org/~paulus/gitkJunio C Hamano Mon, 18 Mar 2019 02:18:49 +0000 (11:18 +0900)

Merge git://ozlabs.org/~paulus/gitk

* git://ozlabs.org/~paulus/gitk:
gitk: Update Bulgarian translation (317t)

merge: tweak --rerere-autoupdate documentationPhillip Wood Thu, 14 Mar 2019 19:12:33 +0000 (19:12 +0000)

merge: tweak --rerere-autoupdate documentation

Spell out --no-rerere-autoupdate explictly to make searching
easier. This matches the other --no options in the man page.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

am/cherry-pick/rebase/revert: document --rerere-autoupdatePhillip Wood Thu, 14 Mar 2019 19:12:32 +0000 (19:12 +0000)

am/cherry-pick/rebase/revert: document --rerere-autoupdate

This option was missing from the man pages of these commands.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

gitk: Update Bulgarian translation (317t)Alexander Shopov Wed, 13 Mar 2019 12:06:33 +0000 (13:06 +0100)

gitk: Update Bulgarian translation (317t)

Signed-off-by: Alexander Shopov <ash@kambanaria.org>
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

gc: remove redundant check for gc_auto_thresholdÆvar Arnfjörð Bjarmason Wed, 13 Mar 2019 23:54:35 +0000 (00:54 +0100)

gc: remove redundant check for gc_auto_threshold

Checking gc_auto_threshold in too_many_loose_objects() was added in
17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.",
2007-09-17) when need_to_gc() itself was also reliant on
gc_auto_pack_limit before its early return:

gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0

When that check was simplified to just checking "gc_auto_threshold <=
0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by
assigning 0 to gc.auto", 2008-03-19) this unreachable code should have
been removed. We only call too_many_loose_objects() from within
need_to_gc() itself, which will return if this condition holds, and in
cmd_gc() which will return before ever getting to "auto_gc &&
too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true
earlier in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

get_oid(): when an object was not found, try harderJohannes Schindelin Wed, 13 Mar 2019 10:16:34 +0000 (03:16 -0700)

get_oid(): when an object was not found, try harder

It is quite possible that the loose object cache gets stale when new
objects are written. In that case, get_oid() would potentially say that
it cannot find a given object, even if it should find it.

Let's blow away the loose object cache as well as the read packs and try
again in that case.

Note: this does *not* affect the code path that was introduced to help
avoid looking for the same non-existing objects (which made some
operations really expensive via NFS): that code path is handled by the
`OBJECT_INFO_QUICK` flag (which does not even apply to `get_oid()`,
which has no equivalent flag, at least at the time this patch was
written).

This incidentally fixes the problem identified earlier where an
interactive rebase wanted to re-read (and validate) the todo list after
an `exec` command modified it.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

sequencer: move stale comment into correct locationJohannes Schindelin Wed, 13 Mar 2019 10:16:33 +0000 (03:16 -0700)

sequencer: move stale comment into correct location

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

sequencer: improve error message when an OID could... Johannes Schindelin Wed, 13 Mar 2019 10:16:32 +0000 (03:16 -0700)

sequencer: improve error message when an OID could not be parsed

The interactive rebase simply complains about an "invalid line" when the
object hash of, say, a `pick` line could not be parsed.

Let's tell the user what happened in a little more detail.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

rebase -i: demonstrate obscure loose object cache bugJohannes Schindelin Wed, 13 Mar 2019 10:16:31 +0000 (03:16 -0700)

rebase -i: demonstrate obscure loose object cache bug

We specifically support `exec` commands in `git rebase -i`'s todo lists
to rewrite the very same todo list. Of course, we need to validate that
todo list when re-reading it.

It is also totally legitimate to extend the todo list by `pick` lines
using short names of commits that were created only after the rebase
started.

And this is where the loose object cache interferes with this feature:
if *some* loose object was read whose hash shares the same first two
digits with a commit that was not yet created when that loose object was
created, then we fail to find that new commit by its short name in
`get_oid()`, and the interactive rebase fails with an obscure error
message like:

error: invalid line 1: pick 6568fef
error: please fix this using 'git rebase --edit-todo'.

Let's first demonstrate that this is actually a bug in a new regression
test, in a separate commit so that other developers who do not believe
me can cherry-pick it to confirm the problem.

This new regression test generates two commits whose hashes share the
first two hex digits (so that their corresponding loose objects live in
the same subdirectory of .git/objects/, and are therefore supposed to be
in the same loose object cache bin).

It then picks the first, to make sure that the loose object cache is
initialized and cached that object directory, then generates the second
commit and picks it, too. Since the commit was generated in a different
process than the sequencer that wants to pick it, the loose object cache
had no chance of being updated in the meantime.

Technically, we would need only one `exec` command in this regression
test case, but for ease of implementation, it uses a pseudo-recursive
call to the same script.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

t9811-git-p4-label-import: fix pipeline negationSZEDER Gábor Wed, 13 Mar 2019 12:24:19 +0000 (13:24 +0100)

t9811-git-p4-label-import: fix pipeline negation

In 't9811-git-p4-label-import.sh' the test 'tag that cannot be
exported' runs

!(p4 labels | grep GIT_TAG_ON_A_BRANCH)

to check that the given string is not printed by 'p4 labels'. This is
problematic, because according to POSIX [1]:

"If the pipeline begins with the reserved word ! and command1 is a
subshell command, the application shall ensure that the ( operator
at the beginning of command1 is separated from the ! by one or more
<blank> characters. The behavior of the reserved word ! immediately
followed by the ( operator is unspecified."

While most common shells still interpret this '!' as "negate the exit
code of the last command in the pipeline", 'mksh/lksh' don't and
interpret it as a negative file name pattern instead. As a result
they attempt to run a command made up of the pathnames in the current
directory (it contains a single directory called 'main'), which, of
course, fails the test.

We could fix it simply by adding a space between the '!' and '(', but
instead let's fix it by removing the unnecessary subshell.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_02

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

git p4 test: disable '-x' tracing in the p4d watchdog... SZEDER Gábor Wed, 13 Mar 2019 12:24:18 +0000 (13:24 +0100)

git p4 test: disable '-x' tracing in the p4d watchdog loop

According to the comments in 't/lib-git-p4.sh', sometimes 'p4d' seems
to hang, and to deal with that 'start_p4d' starts a watchdog process
to kill it after a long-enough timeout ($P4D_TIMEOUT, defaults to
300s). This watchdog process is implemented as a background subshell
loop iterating once every second until the timeout expires, producing
a few lines of trace output on each iteration when the test script is
run with '-x' tracing enabled. The watchdog loop's trace gets
intermixed with the real test output and trace, and makes that harder
to read.

Send the trace output of this loop to /dev/null to avoid polluting the
real test output.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

git p4 test: simplify timeout handlingSZEDER Gábor Wed, 13 Mar 2019 12:24:17 +0000 (13:24 +0100)

git p4 test: simplify timeout handling

'lib-git-p4.sh' uses timeouts in a watchdog process to kill a
potentially stuck 'p4d' process and for certain cleanup operation
between tests. It does so by first computing when the timeout should
expire, and then repeatedly asking for the current time in seconds
until it exceeds the expiration time, and for portability reasons it
uses a one-liner Python script to ask for the current time.

Replace these timeouts with downcounters, which, though not
necessarily shorter, are much simpler, at least in the sense that they
don't execute the Python interpreter every second.

After this change the helper function with that Python one-liner has
no callers left, remove it.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

git p4 test: clean up the p4d cleanup functionsSZEDER Gábor Wed, 13 Mar 2019 12:24:16 +0000 (13:24 +0100)

git p4 test: clean up the p4d cleanup functions

Confusingly, the 'git p4' tests used two cleanup functions:

- 'kill_p4d' was run in the last test before 'test_done', and it not
only killed 'p4d', but it killed the watchdog process, and cleaned
up after 'p4d' as well by removing all directories used by the P4
daemon and client.

This cleanup is not necessary right before 'test_done', because
the whole trash directory is about to get removed anyway, but it
is necessary in 't9801-git-p4-branch.sh', which uses 'kill_p4d' to
stop 'p4d' before re-starting it in the middle of the test script.

- 'cleanup' was run in the trap on EXIT, and it killed 'p4d', but,
it didn't kill the watchdog process, and, contrarily to its name,
didn't perform any cleanup whatsoever.

Make it clearer what's going on by renaming and simplifying the
cleanup functions, so in the end we'll have:

- 'stop_p4d_and_watchdog' replaces 'cleanup' as it will try to live
up to its name and stop both the 'p4d' and the watchdog processes,
and as the sole function registered with 'test_atexit' it will be
responsible for no leaving any stray processes behind after 'git p4'
tests were finished or interrupted.

- 'stop_and_cleanup_p4d' replaces 'kill_p4d' as it will stop 'p4d'
(and the watchdog) and remove all directories used by the P4
daemon and cliean, so it can be used mid-script to stop and then
re-start 'p4d'.

Note that while 'cleanup' sent a single SIGKILL to 'p4d', 'kill_p4d'
was quite brutal, as it first sent SIGTERM to the daemon repeatedly,
either until its pid disappeared or until a given timeout was up, and
then it sent SIGKILL repeatedly, for good measure. This is overkill
(pardon the pun): a single SIGKILL should be able to take down any
process in a sensible state, and if a process were to somehow end up
stuck in the dreaded uninterruptible sleep state then even repeated
SIGKILLs won't bring immediate help. So ditch all the repeated
SIGTERM/SIGKILL parts, and use a single SIGKILL to stop 'p4d', and
make sure that there are no races between asynchron signal delivery
and subsequent restart of 'p4d' by waiting for it to die.

With this change the 'retry_until_fail' helper has no callers left,
remove it.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

git p4 test: use 'test_atexit' to kill p4d and the... Johannes Schindelin Wed, 13 Mar 2019 12:24:15 +0000 (13:24 +0100)

git p4 test: use 'test_atexit' to kill p4d and the watchdog process

Use 'test_atexit' to run cleanup commands to stop 'p4d' at the end of
the test script or upon interrupt or failure, as it is shorter,
simpler, and more robust than registering such cleanup commands in the
trap on EXIT in the test scripts.

Note that one of the test scripts, 't9801-git-p4-branch.sh', stops and
then re-starts 'p4d' twice in the middle of the script; take care that
the cleanup functions to stop 'p4d' are only registered once.

Note also that 'git p4' tests invoke different functions in the trap
on EXIT ('cleanup') and in the last test before 'test_done'
('kill_p4d'). Register both of these functions with 'test_atexit' for
now, and a a later patch in this series will then clean up the
redundancy.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

t0301-credential-cache: use 'test_atexit' to stop the... SZEDER Gábor Wed, 13 Mar 2019 12:24:14 +0000 (13:24 +0100)

t0301-credential-cache: use 'test_atexit' to stop the credentials helper

Use 'test_atexit' to run cleanup commands to stop the credentials
helper at the end of the test script or upon interrupt or failure, as
it is shorter, simpler, and more robust than registering such cleanup
commands in the trap on EXIT in the test scripts.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

tests: use 'test_atexit' to stop httpdSZEDER Gábor Wed, 13 Mar 2019 12:24:13 +0000 (13:24 +0100)

tests: use 'test_atexit' to stop httpd

Use 'test_atexit' to run cleanup commands to stop httpd at the end of
the test script or upon interrupt or failure, as it is shorter,
simpler, and more robust than registering such cleanup commands in the
trap on EXIT in the test scripts.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

git-daemon: use 'test_atexit` to stop 'git-daemon'Johannes Schindelin Wed, 13 Mar 2019 12:24:12 +0000 (13:24 +0100)

git-daemon: use 'test_atexit` to stop 'git-daemon'

Use 'test_atexit' to run cleanup commands to stop 'git-daemon' at the
end of the test script or upon interrupt or failure, as it is shorter,
simpler, and more robust than registering such cleanup commands in the
trap on EXIT in the test scripts.

Note that in 't5570-git-daemon.sh' the daemon is stopped and then
re-started in the middle of the test script; take care that the
cleanup functions to stop the daemon are only registered once.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

test-lib: introduce 'test_atexit'Johannes Schindelin Wed, 13 Mar 2019 12:24:11 +0000 (13:24 +0100)

test-lib: introduce 'test_atexit'

When running Apache, 'git daemon', or p4d, we want to kill them at the
end of the test script, otherwise a leftover daemon process will keep
its port open indefinitely, and thus will interfere with subsequent
executions of the same test script.

So far, we stop these daemon processes "manually", i.e.:

- by registering functions or commands in the trap on EXIT to stop
the daemon while preserving the last seen exit code before the
trap (to deal with a failure when run with '--immediate' or with
interrupts by ctrl-C),

- and by invoking these functions/commands last thing before
'test_done' (and sometimes restoring the test framework's default
trap on EXIT, to prevent the daemons from being killed twice).

On one hand, we do this inconsistently, e.g. 'git p4' tests invoke
different functions in the trap on EXIT and in the last test before
'test_done', and they neither restore the test framework's default trap
on EXIT nor preserve the last seen exit code. On the other hand, this
is error prone, because, as shown in a previous patch in this series,
any output from the cleanup commands in the trap on EXIT can prevent a
proper cleanup when a test script run with '--verbose-log' and certain
shells, notably 'dash', is interrupted.

Let's introduce 'test_atexit', which is loosely modeled after
'test_when_finished', but has a broader scope: rather than running the
commands after the current test case, run them when the test script
finishes, and also run them when the test is interrupted, or exits
early in case of a failure while the '--immediate' option is in
effect.

When running the cleanup commands at the end of a successful test,
then they will be run in 'test_done' before it removes the trash
directory, i.e. the cleanup commands will still be able to access any
pidfiles or socket files in there. When running the cleanup commands
after an interrupt or failure with '--immediate', then they will be
run in the trap on EXIT. In both cases they will be run in
'test_eval_', i.e. both standard error and output of all cleanup
commands will go where they should according to the '-v' or
'--verbose-log' options, and thus won't cause any troubles when
interrupting a test script run with '--verbose-log'.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

t/lib-git-daemon: make sure to kill the 'git-daemon... SZEDER Gábor Wed, 13 Mar 2019 12:24:10 +0000 (13:24 +0100)

t/lib-git-daemon: make sure to kill the 'git-daemon' process

After 'start_git_daemon' starts 'git daemon' (note the space in the
middle) in the background, it saves the background process' PID, so
the daemon can be stopped at the end of the test script. However,
'git-daemon' is not a builtin but a dashed external command, which
means that the dashless 'git daemon' executes the dashed 'git-daemon'
command, and, consequently, the PID recorded is not the PID of the
"real" daemon process, but that of the main 'git' wrapper. Now, if a
test script involving 'git daemon' is interrupted by ctrl-C, then only
the main 'git' process is stopped, but the real daemon process tends
to survive somehow, and keeps on running in the background
indefinitely, keeping the daemon's port to itself, and thus preventing
subsequent runs of the same test script.

Work this around by running 'git daemon' with the '--pidfile=...'
option to save the PID of the real daemon process, and kill that
process in 'stop_git_daemon' as well.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

test-lib: fix interrupt handling with 'dash' and '... SZEDER Gábor Wed, 13 Mar 2019 12:24:09 +0000 (13:24 +0100)

test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'

When a test script run with 'dash' and '--verbose-log -x' is
interrupted by ctrl-C, SIGTERM, or closing the terminal window, then
most of the time the registered EXIT trap actions are not executed.
This is an annoying issue with tests involving daemons, because they
should run cleanup commands to kill those daemon processes in the trap
on EXIT, but since these cleanup commands are not executed, the
daemons are left alive and keep their port open, thus interfering with
subsequent execution of the same test script.

The cause of this issue is the subtle combination of several factors
(bear with me, or skip over the indented part):

- Even when the test script is interrupted, the cleanup commands are
not run in the trap on INT, TERM, or HUP, but in the trap on EXIT
after the trap on the signals invokes 'exit' [1].

- According to POSIX [2]:

"The environment in which the shell executes a trap on EXIT
shall be identical to the environment immediately after the last
command executed before the trap on EXIT was taken."

Pertinent to the issue at hand is that all open file descriptors
and the state of '-x' tracing should be preserved. All shells
I've tried [3] preserve '-x'. Unfortunately, however:

- 'dash' doesn't conform to this when it comes to open file
descriptors: even when standard output and/or error are
redirected somewhere when 'exit' is invoked, anything written
to them in the trap on EXIT goes to the script's original
stdout and stderr [4].

We can't dismiss this with a simple "it doesn't conform to
POSIX, so we don't care", because 'dash' is the default
/bin/sh in some of the more popular Linux distros.

- As far as I can tell, POSIX doesn't explicitly say anything
about the environment of trap actions for various signals.

In practice it seems that most shells behave sensibly and
preserve both open file descriptors and the state of '-x'
tracing for the traps on INT, TERM, and HUP, including even
'dash'. The exceptions are 'mksh' and 'lksh': they do
preserve '-x', but not the open file descriptors.

- When a test script run with '-x' tracing enabled is interrupted,
then it's very likely that the signal arrives mid-test, i.e.:

- while '-x' tracing is enabled, and, consequently, our trap
actions on INT, TERM, HUP, and EXIT will produce trace output
as well.

- while standard output and error are redirected to a log file,
to the test script's original standard output and error, or to
/dev/null, depending on whether the test script was run with
'--verbose-log', '-v', or neither. According to the above, we
can't rely on these redirections still be in effect when
running the traps on INT, TERM, HUP, and/or EXIT.

- When a test script is run with '--verbose-log', then the test
script is re-executed with its standard output and error piped
into 'tee', in order to send the "regular" non-verbose test's
output both to the terminal and to the log file. When the test is
interrupted, then the signal interrupts the downstream 'tee' as
well.

Putting these together, when a test script run with 'dash' and
'--verbose-log -x' is interrupted, then 'dash' tries to write the
trace output from the EXIT trap to the script's original standard
error, but it very likely can't, because the 'tee' downstream of the
pipe is interrupted as well. This causes the shell running the test
script to die because of SIGPIPE, without running any of the commands
in the EXIT trap.

Disable '-x' tracing in the trap on INT, TERM, and HUP to avoid this
issue, as it disables tracing in the chained trap on EXIT as well.
Wrap it in a '{ ... } 2>/dev/null' block, so the trace of the command
disabling the tracing doesn't go to standard error either [5].

Note that it's not only '-x' tracing that can be problematic, but any
shell builtin, e.g. 'echo', that writes to standard output or error in
the trap on EXIT, while a test running with 'dash' and '--verbose-log'
(even without '-x') is interrupted. As far as I can tell, this is not
an issue at the moment:

- The cleanup commands to stop the credential-helper, Apache, or
'p4d' don't use any such shell builtins.

- stop_git_daemon() does use 'say' and 'error', both wrappers around
'echo', but it redirects 'say' to fd 3, i.e. to the log file, and
while 'error' does write to standard output, it comes only after
the daemon was killed.

- The non-builtin commands that actually stop the daemons ('kill',
'apache2 -k stop', 'git credential-cache exit') are silent, so they
won't get SIGPIPE before finishing their job.

[1] The trap on EXIT must run cleanup commands, because we want to
stop any daemons when a test script run with '--immediate' fails
and exits early with error. By chaining up the trap on signals to
the trap on EXIT we can deal with cleanup commands a bit simpler,
because the tests involving daemons only have to set a single
trap.

[2] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap

[3] The shells I tried: dash, Bash, ksh, ksh93, mksh, lksh, yash,
BusyBox sh, FreeBSD /bin/sh, NetBSD /bin/sh.

[4] $ cat trap-output.sh
#!/bin/sh
trap "echo output; echo error >&2" EXIT
{ exit; } >OUT 2>ERR
$ dash ./trap-output.sh
output
error
$ wc -c OUT ERR
0 OUT
0 ERR

On a related note, 'ksh', 'ksh93', and BusyBox sh don't conform to
the specs in this respect, either.

[5] This '{ set +x; } 2>/dev/null' trick won't help those shells that
show trace output for any redirections and don't preserve open
file descriptors for the trap on INT, TERM and HUP. The only such
shells I'm aware of are 'mksh' and 'lksh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

submodule: explain first attempt failure clearlyJonathan Tan Wed, 13 Mar 2019 17:57:38 +0000 (10:57 -0700)

submodule: explain first attempt failure clearly

When cloning with --recurse-submodules a superproject with at least one
submodule with HEAD pointing to an unborn branch, the clone goes
something like this:

Cloning into 'test'...
<messages about cloning of superproject>
Submodule '<name>' (<uri>) registered for path '<submodule path>'
Cloning into '<submodule path>'...
fatal: Couldn't find remote ref HEAD
Unable to fetch in submodule path '<submodule path>'
<messages about fetching with SHA-1>
From <uri>
* branch <hash> -> FETCH_HEAD
Submodule path '<submodule path>': checked out '<hash>'

In other words, first, a fetch is done with no hash arguments (that is,
a fetch of HEAD) resulting in a "Couldn't find remote ref HEAD" error;
then, a fetch is done given a hash, which succeeds.

The fetch given a hash was added in fb43e31f2b ("submodule: try harder
to fetch needed sha1 by direct fetching sha1", 2016-02-24), and the
"Unable to fetch..." message was downgraded from a fatal error to a
notice in e30d833671 ("git-submodule.sh: try harder to fetch a
submodule", 2018-05-16).

This commit improves the notice to be clearer that we are retrying the
fetch, and that the previous messages (in particular, the fatal errors
from fetch) do not necessarily indicate that the whole command fails. In
other words:

- If the HEAD-fetch succeeds and we then have the commit we want,
git-submodule prints no explanation.
- If the HEAD-fetch succeeds and we do not have the commit we want, but
the hash-fetch succeeds, git-submodule prints no explanation.
- If the HEAD-fetch succeeds and we do not have the commit we want, but
the hash-fetch fails, git-submodule prints a fatal error.
- If the HEAD-fetch fails, fetch prints a fatal error, and
git-submodule informs the user that it will retry by fetching
specific commits by hash.
- If the hash-fetch then succeeds, git-submodule prints no
explanation (besides the ones already printed).
- If the HEAD-fetch then fails, git-submodule prints a fatal error.

It could be said that we should just eliminate the HEAD-fetch
altogether, but that changes some behavior (in particular, some refs
that were opportunistically updated would no longer be), so I have left
that alone for now.

There is an analogous situation with the fetching code in fetch_finish()
and surrounding functions. For now, I have added a NEEDSWORK.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Makefile: fix unaligned loads in sha1dc with UBSanJeff King Tue, 12 Mar 2019 21:06:26 +0000 (17:06 -0400)

Makefile: fix unaligned loads in sha1dc with UBSan

The sha1dc library uses unaligned loads on platforms that support them.
This is normally what you'd want for performance, but it does cause
UBSan to complain when we compile with SANITIZE=undefined. Just like we
set -DNO_UNALIGNED_LOADS for our own code in that case, we should set
-DSHA1DC_FORCE_ALIGNED_ACCESS.

Of course that does nothing without pulling in the patches from sha1dc
to respect that define. So let's do that, too, updating both the
submodule link and our in-tree copy (from the same commit).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

doc: format pathnames and URLs as monospace.Corentin BOMPARD Wed, 6 Mar 2019 13:04:46 +0000 (14:04 +0100)

doc: format pathnames and URLs as monospace.

Applying CodingGuidelines about monospace on pathnames and URLs.

See Documentation/CodingGuidelines.txt for more information.

Signed-off-by: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
Signed-off-by: Nathan BERBEZIER <nathan.berbezier@etu.univ-lyon1.fr>
Signed-off-by: Pablo CHABANNE <pablo.chabanne@etu.univ-lyon1.fr>
Signed-off-by: Matthieu MOY <matthieu.moy@univ-lyon1.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

doc/CodingGuidelines: URLs and paths as monospaceCorentin BOMPARD Wed, 6 Mar 2019 13:04:45 +0000 (14:04 +0100)

doc/CodingGuidelines: URLs and paths as monospace

The current documentation uses both quotes (italics) and backquotes
(monospace) to render URLs and pathnames, which is inconsistent.
Document a best practice in CodingGuidelines to help reduce
inconsistencies in the future.

We set the best practice to using backquotes, since:

* It is already an established practice. For exemple:

$ git grep "'[^']/*[^']'" | wc -l
206

$ git grep '`[^`]/*[^`]`' | wc -l
690

There are false positives on both sides, but after a cursory look at
the output of both, it doesn't seem the false positive rate is really
higher in the second case.

At least, this shows that the existing documentation uses
inconsistent formatting, and that it would be good to do
something about it.

* It may be debatable whether path names need to be typed in
monospace but having them in italics is really unusual.

Signed-off-by: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
Signed-off-by: Nathan BERBEZIER <nathan.berbezier@etu.univ-lyon1.fr>
Signed-off-by: Pablo CHABANNE <pablo.chabanne@etu.univ-lyon1.fr>
Signed-off-by: Matthieu MOY <matthieu.moy@univ-lyon1.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>