Sergey Organov [Mon, 23 Nov 2020 20:05:51 +0000 (23:05 +0300)]
glossary: improve "branch" definition
The old phrasing is at least questionable, if not wrong, as there are
a lot of branches out there that didn't see active development for
years, yet they are still branches, ready to become active again any
time.
Signed-off-by: Sergey Organov <sorganov@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Wed, 2 Dec 2020 02:34:11 +0000 (21:34 -0500)]
Makefile: mark git-maintenance as a builtin
We normally get the list of builtin commands by expanding BUILTIN_OBJS.
But for commands which are embedded inside another's source file (e.g.,
cmd_show() in builtin/log.c), the Makefile needs to be told explicitly
about them.
Since cmd_maintenance() is inside buitin/gc.c, it should be listed
explicitly in the BUILT_INS list in the Makefile. Not doing so isn't
_too_ tragic, as it simply means we will not make a git-maintenance
symlink in libexec/git-core. Since we encourage people to use the "git
foo" form, even in scripts which have put libexec into their PATH,
nobody seems to have noticed.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Matheus Tavares [Tue, 1 Dec 2020 23:45:04 +0000 (20:45 -0300)]
apply: don't use core.sharedRepository to create working tree files
core.sharedRepository defines which permissions Git should set when
creating files in $GIT_DIR, so that the repository may be shared with
other users. But (in its current form) the setting shouldn't affect how
files are created in the working tree. This is not respected by apply
and am (which uses apply), when creating leading directories:
$ cat d.patch
diff --git a/d/f b/d/f
new file mode 100644
index 0000000..e69de29
Apply without the setting:
$ umask 0077
$ git apply d.patch
$ ls -ld d
drwx------
Apply with the setting:
$ umask 0077
$ git -c core.sharedRepository=0770 apply d.patch
$ ls -ld d
drwxrws---
Only the leading directories are affected. That's because they are
created with safe_create_leading_directories(), which calls
adjust_shared_perm() to set the directories' permissions based on
core.sharedRepository. To fix that, let's introduce a variant of this
function that ignores the setting, and use it in apply. Also add a
regression test and a note in the function documentation about the use
of each variant according to the destination (working tree or git
dir).
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 1 Dec 2020 21:11:38 +0000 (13:11 -0800)]
banned.h: mark ctime_r() and asctime_r() as banned
The ctime_r() and asctime_r() functions are reentrant, but have
no check that the buffer we pass in is long enough (the manpage says it
"should have room for at least 26 bytes"). Since this is such an
easy-to-get-wrong interface, and since we have the much safer strftime()
as well as its more convenient strbuf_addftime() wrapper, let's ban both
of those.
Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Wed, 2 Dec 2020 11:46:01 +0000 (12:46 +0100)]
gitignore: remove entry for git serve
b7ce24d095 (Turn `git serve` into a test helper, 2019-04-18) demoted git
serve from a builtin command to a test helper. As a result the
git-serve binary is no longer built and thus doesn't have to be ignored
anymore.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Wed, 2 Dec 2020 02:26:24 +0000 (21:26 -0500)]
gitignore: drop duplicate entry for git-sh-i18n
This was accidentally added by e00cf070a4 (git-sh-i18n.sh: add no-op
gettext() and eval_gettext() wrappers, 2011-05-14), even though an
earlier commit in the same series had already done so.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Wed, 2 Dec 2020 02:47:56 +0000 (21:47 -0500)]
t7900: speed up expensive test
A test marked with EXPENSIVE creates two 2.5GB files and adds them to
the repository. This takes 194s to run on my machine, versus 2s when the
EXPENSIVE prereq isn't set. We can trim this down a bit by doing two
things:
- use "git commit --quiet" to avoid spending time generating a diff
summary (this actually only helps for the second commit, but I've
added it here to both for consistency). This shaves off 8s.
- set core.compression to 0. We know these files are full of random
bytes, and so won't compress (that's the point of the test!).
Spending cycles on zlib is pointless. This shaves off 122s.
After this, my total time to run the script is 64s. That won't help
normal runs without GIT_TEST_LONG set, of course, but it's easy enough
to do.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 1 Dec 2020 22:25:18 +0000 (22:25 +0000)]
stash: fix stash application in sparse-checkouts
sparse-checkouts are built on the patterns in the
$GIT_DIR/info/sparse-checkout file, where commands have modified
behavior for paths that do not match those patterns. The differences in
behavior, as far as the bugs concerned here, fall into three different
categories (with git subcommands that fall into each category listed):
* commands that only look at files matching the patterns:
* status
* diff
* clean
* update-index
* commands that remove files from the working tree that do not match
the patterns, and restore files that do match them:
* read-tree
* switch
* checkout
* reset (--hard)
* commands that omit writing files to the working tree that do not
match the patterns, unless those files are not clean:
* merge
* rebase
* cherry-pick
* revert
There are some caveats above, e.g. a plain `git diff` ignores files
outside the sparsity patterns but will show diffs for paths outside the
sparsity patterns when revision arguments are passed. (Technically,
diff is treating the sparse paths as matching HEAD.) So, there is some
internal inconsistency among these commands. There are also additional
commands that should behave differently in the face of sparse-checkouts,
as the sparse-checkout documentation alludes to, but the above is
sufficient for me to explain how `git stash` is affected.
What is relevant here is that logically 'stash' should behave like a
merge; it three-way merges the changes the user had in progress at stash
creation time, the HEAD at the time the stash was created, and the
current HEAD, in order to get the stashed changes applied to the current
branch. However, this simplistic view doesn't quite work in practice,
because stash tweaks it a bit due to two factors: (1) flags like
--keep-index and --include-untracked (why we used two different verbs,
'keep' and 'include', is a rant for another day) modify what should be
staged at the end and include more things that should be quasi-merged,
(2) stash generally wants changes to NOT be staged. It only provides
exceptions when (a) some of the changes had conflicts and thus we want
to use stages to denote the clean merges and higher order stages to
mark the conflicts, or (b) if there is a brand new file we don't want
it to become untracked.
stash has traditionally gotten this special behavior by first doing a
merge, and then when it's clean, applying a pipeline of commands to
modify the result. This series of commands for
unstaging-non-newly-added-files came from the following commands:
Looking back at the different types of special sparsity handling listed
at the beginning of this message, you may note that we have at least one
of each type covered here: merge, diff-index, and read-tree. The weird
mix-and-match led to 3 different bugs:
(1) If a path merged cleanly and it didn't match the sparsity patterns,
the merge backend would know to avoid writing it to the working tree and
keep the SKIP_WORKTREE bit, simply only updating it in the index.
Unfortunately, the subsequent commands would essentially undo the
changes in the index and thus simply toss the changes altogether since
there was nothing left in the working tree. This means the stash is
only partially applied.
(2) If a path existed in the worktree before `git stash apply` despite
having the SKIP_WORKTREE bit set, then the `git read-tree --reset` would
print an error message of the form
error: Entry 'modified' not uptodate. Cannot merge.
and cause stash to abort early.
(3) If there was a brand new file added by the stash, then the
diff-index command would save that pathname to the temporary file, the
read-tree --reset would remove it from the index, and the update-index
command would barf due to no such file being present in the working
copy; it would print a message of the form:
error: NEWFILE: does not exist and --remove not passed
fatal: Unable to process path NEWFILE
and then cause stash to abort early.
Basically, the whole idea of unstage-unless-brand-new requires special
care when you are dealing with a sparse-checkout. Fix these problems
by applying the following simple rule:
When we unstage files, if they have the SKIP_WORKTREE bit set,
clear that bit and write the file out to the working directory.
(*) If there's already a file present in the way, rename it first.
This fixes all three problems in t7012.13 and allows us to mark it as
passing.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 1 Dec 2020 22:25:17 +0000 (22:25 +0000)]
stash: remove unnecessary process forking
When stash was converted from shell to a builtin, it merely
transliterated the forking of various git commands from shell to a C
program that would fork the same commands. Some of those were converted
over to actual library calls, but much of the pipeline-of-commands
design still remains. Fix some of this by replacing the portion
corresponding to
into a library function that does the same thing. (The read-tree
--reset was already partially converted over to a library call, but as
an independent piece.) Note here that this came after a merge operation
was performed. The merge machinery always stages anything that cleanly
merges, and the above code only runs if there are no conflicts. Its
purpose is to make it so that when there are no conflicts, all the
changes from the stash are unstaged. However, that causes brand new
files from the stash to become untracked, so the code above first saves
those files off and then re-adds them afterwards.
We replace the whole series of commands with a simple function that will
unstage files that are not newly added. This doesn't fix any bugs in
the usage of these commands, it simply matches the existing behavior but
makes it into a single atomic operation that we can then operate on as a
whole. A subsequent commit will take advantage of this to fix issues
with these commands in sparse-checkouts.
This conversion incidentally fixes t3906.1, because the separate
update-index process would die with the following error messages:
error: uninitialized_sub: is a directory - add files inside instead
fatal: Unable to process path uninitialized_sub
The unstaging of the directory as a submodule meant it was no longer
tracked, and thus as an uninitialized directory it could not be added
back using `git update-index --add`, thus resulting in this error and
early abort. Most of the submodule tests in 3906 continue to fail after
this change, this change was just enough to push the first of those
tests to success.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 1 Dec 2020 22:25:16 +0000 (22:25 +0000)]
t7012: add a testcase demonstrating stash apply bugs in sparse checkouts
Applying stashes in sparse-checkouts, particularly when the patterns
used to define the sparseness have changed between when the stash was
created and when it is applied, has a number of bugs. The primary
problem is that stashes are sometimes only partially applied. In most
such cases, it does so silently without any warning or error being
displayed and with 0 exit status.
There are, however, a few cases when non-translated error messages are
shown and the stash application aborts early. The first is when there
are files present despite the SKIP_WORKTREE bit being set, in which case
the error message shown is:
error: Entry 'PATHNAME' not uptodate. Cannot merge.
The other situation is when a stash contains new files to add to the
working tree; in this case, the code aborts early but still has the
stash partially applied, and shows the following error message:
error: NEWFILE: does not exist and --remove not passed
fatal: Unable to process path NEWFILE
Add a test that can trigger all three of these problems. Have it
carefully check that the working copy and SKIP_WORKTREE bits are as
expected after the stash application. The test is currently marked as
expected to fail, but subsequent commits will implement the fixes and
toggle the expectation.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 1 Dec 2020 21:11:37 +0000 (13:11 -0800)]
banned.h: mark non-reentrant gmtime, etc as banned
The traditional gmtime(), localtime(), ctime(), and asctime() functions
return pointers to shared storage. This means they're not thread-safe,
and they also run the risk of somebody holding onto the result across
multiple calls (where each call invalidates the previous result).
All callers should be using their reentrant counterparts.
Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Tue, 1 Dec 2020 00:30:06 +0000 (19:30 -0500)]
builtin/bugreport.c: use thread-safe localtime_r()
To generate its filename, the 'git bugreport' builtin asks the system
for the current time with 'localtime()'. Since this uses a shared
buffer, it is not thread-safe.
Even though 'git bugreport' is not multi-threaded, using localtime() can
trigger some static analysis tools to complain, and a quick
Jeff King [Tue, 1 Dec 2020 12:15:13 +0000 (07:15 -0500)]
upload-pack: kill pack-objects helper on signal or exit
We spawn an external pack-objects process to actually send objects to
the remote side. If we are killed by a signal during this process, then
pack-objects may continue to run. As soon as it starts producing output
for the pack, it will see a failure writing to upload-pack and exit
itself. But before then, it may do significant work traversing the
object graph, compressing deltas, etc, which will all be pointless. So
let's make sure to kill as soon as we know that the caller will not read
the result.
There's no test here, since it's inherently racy, but here's an easy
reproduction is on a large-ish repo like linux.git:
- make sure you don't have pack bitmaps (since they make the enumerating
phase go quickly). For linux.git it takes ~30s or so to walk the
whole graph on my machine.
- run "git clone --no-local -q . dst"; the "-q" is important because
if pack-objects is writing progress to upload-pack (to get
multiplexed over the sideband to the client), then it will notice
pretty quickly the failure to write to stderr
- kill the client-side clone process in another terminal (don't use
^C, as that will send SIGINT to all of the processes)
- run "ps au | grep git" or similar to observe upload-pack dying
within 5 seconds (it will send a keepalive that will notice the
client has gone away)
- but you'll still see pack-objects consuming 100% CPU (and 1GB+ of
RAM) during the traversal and delta compression phases. It will exit
as soon as it starts to write the pack (when it will notice that
upload-pack went away).
With this patch, pack-objects exits as soon as upload-pack does.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
git imap-send does not parse the default git config settings and thus ignore
core.askpass value.
Rewrite config parsing to support core settings.
Reported-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Mon, 30 Nov 2020 22:49:45 +0000 (14:49 -0800)]
Merge branch 'sa/credential-store-timeout'
Multiple "credential-store" backends can race to lock the same
file, causing everybody else but one to fail---reattempt locking
with some timeout to reduce the rate of the failure.
* sa/credential-store-timeout:
crendential-store: use timeout when locking file
Junio C Hamano [Mon, 30 Nov 2020 22:49:41 +0000 (14:49 -0800)]
Merge branch 'js/t2106-cleanup'
A test script got cleaned up and then made not to depend on the
value of init.defaultBranch.
* js/t2106-cleanup:
t2106: ensure that the checkout fails for the expected reason
t2106: make test independent of the current main branch name
t2106: adjust style to the current conventions
René Scharfe [Mon, 30 Nov 2020 19:27:15 +0000 (20:27 +0100)]
fetch-pack: disregard invalid pack lockfiles
9da69a6539 (fetch-pack: support more than one pack lockfile, 2020-06-10)
started to use a string_list for pack lockfile names instead of a single
string pointer. It removed a NULL check from transport_unlock_pack() as
well, which is the function that eventually deletes these lockfiles and
releases their name strings.
index_pack_lockfile() can return NULL if it doesn't like the contents it
reads from the file descriptor passed to it. unlink(2) is declared to
not accept NULL pointers (at least with glibc). Undefined Behavior
Sanitizer together with Address Sanitizer detects a case where a NULL
lockfile name is passed to unlink(2) by transport_unlock_pack() in t1060
(make SANITIZE=address,undefined; cd t; ./t1060-object-corruption.sh).
Reinstate the NULL check to avoid undefined behavior, but put it right
at the source, so that the number of items in the string_list reflects
the number of valid lockfiles.
Signed-off-by: René Scharfe <l.s.r@web.de> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Martin Ågren [Sun, 29 Nov 2020 19:52:22 +0000 (20:52 +0100)]
MyFirstObjectWalk: drop `init_walken_defaults()`
In a recent commit, we stopped calling `init_grep_defaults()` from this
function. Thus, by the end of the tutorial, we still haven't added any
contents to this function. Let's remove it for simplicity.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Martin Ågren [Sun, 29 Nov 2020 19:52:21 +0000 (20:52 +0100)]
grep: copy struct in one fell swoop
We have a `struct grep_opt` with our defaults which we then copy into
the caller's struct. Rather than zeroing the target struct and copying
each element one by one, just copy everything at once. This leaves the
code simpler and more maintainable.
We don't have any ownership issues with what we're copying now and can
just greedily copy the whole thing. If and when we do need to handle
such elements (`char *`?), we must and can handle it appropriately. Make
sure to leave a comment to our future selves.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Nipunn Koorapati [Wed, 25 Nov 2020 22:11:37 +0000 (22:11 +0000)]
perf/fsmonitor: use test_must_be_empty helper
Simplify test and make error messages more clear here.
Per feedback from Junio in 33226af42b (t/perf/fsmonitor: improve error message if typoing hook
name, 2020-10-26)
Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> Acked-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rafael Silva [Thu, 26 Nov 2020 20:41:41 +0000 (20:41 +0000)]
maintenance: fix SEGFAULT when no repository
The "git maintenance run" and "git maintenance start/stop" commands
holds a file-based lock at the .git/maintenance.lock and
.git/schedule.lock respectively. These locks are used to ensure only
one maintenance process is executed at the time as both operations
involves writing data into the git repository.
The path to the lock file is built using
"the_repository->objects->odb->path" that results in SEGFAULT when we
have no repository available as "the_repository->objects->odb" is
set to NULL.
Let's teach maintenance command to use RUN_SETUP option that will
provide the validation and fail when running outside of a repository.
Hence fixing the SEGFAULT for all three operations and making the
behaviour consistent across all subcommands.
Setting the RUN_SETUP also provides the same protection for all
subcommands given that the "register" and "unregister" also requires to
be executed inside a repository.
Furthermore let's remove the local validation implemented by the
"register" and "unregister" as this will not be required anymore with
the new option.
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Pratyush Yadav [Fri, 27 Nov 2020 10:23:51 +0000 (15:53 +0530)]
git-gui: Only touch GITGUI_MSG when needed
In 4e55d19 (git-gui: Cleanup end-of-line whitespace in commit messages.,
2007-01-25), the logic to decide if GITGUI_MSG should be saved or
deleted was updated to not require the commit message buffer to be
modified. This fixes a situation where if the user quits and restarts
git-gui multiple times the commit message buffer was lost.
Unfortunately, the fix was not quite correct. The check for whether the
commit message buffer has been modified is useless. If the commit is
_not_ amend, then the check is never performed. If the commit is amend,
then saving the message does not matter anyway. Amend state is destroyed
on exit and the next time git-gui is opened it starts from scratch, but
with the older message retained in the buffer. If amend is selected,
the current message is over-written by the amend commit's message.
The correct fix would be to not touch GITGUI_MSG at all if the commit
message buffer is not modified. This way, the file is not deleted even
on multiple restarts. It has the added benefit of not writing the file
unnecessarily on every exit.
Junio C Hamano [Wed, 25 Nov 2020 23:24:52 +0000 (15:24 -0800)]
Merge branch 'pk/subsub-fetch-fix'
"git fetch" did not work correctly with nested submodules where the
innermost submodule that is not of interest got updated in the
upstream, which has been corrected.
* pk/subsub-fetch-fix:
submodules: fix of regression on fetching of non-init subsub-repo
Junio C Hamano [Wed, 25 Nov 2020 23:24:52 +0000 (15:24 -0800)]
Merge branch 'jk/4gb-idx'
The code was not prepared to deal with pack .idx file that is
larger than 4GB.
* jk/4gb-idx:
packfile: detect overflow in .idx file size checks
block-sha1: take a size_t length parameter
fsck: correctly compute checksums on idx files larger than 4GB
use size_t to store pack .idx byte offsets
compute pack .idx byte offsets using size_t
Junio C Hamano [Wed, 25 Nov 2020 23:24:52 +0000 (15:24 -0800)]
Merge branch 'jx/t5411-flake-fix'
The exchange between receive-pack and proc-receive hook did not
carefully check for errors.
* jx/t5411-flake-fix:
receive-pack: use default version 0 for proc-receive
receive-pack: gently write messages to proc-receive
t5411: new helper filter_out_user_friendly_and_stable_output
Junio C Hamano [Wed, 25 Nov 2020 23:24:52 +0000 (15:24 -0800)]
Merge branch 'sg/bisect-approximately-halfway'
"git bisect start/next" in a large span of history spends a lot of
time trying to come up with exactly the half-way point; this can be
optimized by stopping when we see a commit that is close enough to
the half-way point.
* sg/bisect-approximately-halfway:
bisect: loosen halfway() check for a large number of commits
Junio C Hamano [Wed, 25 Nov 2020 23:24:51 +0000 (15:24 -0800)]
Merge branch 'fc/bash-completion-alias-of-alias'
The command line completion script (in contrib/) learned to expand
commands that are alias of alias.
* fc/bash-completion-alias-of-alias:
completion: bash: improve alias loop detection
completion: bash: check for alias loop
completion: bash: support recursive aliases
Derrick Stolee [Wed, 25 Nov 2020 22:12:56 +0000 (22:12 +0000)]
maintenance: use 'git config --fixed-value'
When a repository's leading directories contain regex metacharacters,
the config calls for 'git maintenance register' and 'git maintenance
unregister' are not careful enough. Use the new --fixed-value option
to direct the config machinery to use exact string matches. This is a
more robust option than escaping these arguments in a piecemeal fashion.
For the test, require that we are not running on Windows since the '+'
and '*' characters are not allowed on that filesystem.
Reported-by: Emily Shaffer <emilyshaffer@google.com> Reported-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Wed, 25 Nov 2020 23:04:26 +0000 (15:04 -0800)]
Merge branch 'ds/config-literal-value' into ds/maintenance-part-3
* ds/config-literal-value:
config doc: value-pattern is not necessarily a regexp
config: implement --fixed-value with --get*
config: plumb --fixed-value into config API
config: add --fixed-value option, un-implemented
t1300: add test for --replace-all with value-pattern
t1300: test "set all" mode with value-pattern
config: replace 'value_regex' with 'value_pattern'
config: convert multi_replace to flags
Junio C Hamano [Wed, 25 Nov 2020 23:01:31 +0000 (15:01 -0800)]
config doc: value-pattern is not necessarily a regexp
The introductory part of the "git config --help" mentions the
optional value-pattern argument, but give no hint that it can be
something other than a regular expression (worse, it just says
"POSIX regexp", which usually means BRE but the regexp the command
takes is ERE). Also, it needs to be documented that the '!' prefix
to negate the match, which is only mentioned in this part of the
document, works only with regexp and not with the --fixed-value.
Derrick Stolee [Wed, 25 Nov 2020 22:12:55 +0000 (22:12 +0000)]
config: implement --fixed-value with --get*
The config builtin does its own regex matching of values for the --get,
--get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
to the get_value() method so we can initialize the value-pattern argument
as a fixed string instead of a regex pattern.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 25 Nov 2020 22:12:54 +0000 (22:12 +0000)]
config: plumb --fixed-value into config API
The git_config_set_multivar_in_file_gently() and related methods now
take a 'flags' bitfield, so add a new bit representing the --fixed-value
option from 'git config'. This alters the purpose of the value_pattern
parameter to be an exact string match. This requires some initialization
changes in git_config_set_multivar_in_file_gently() and a new strcmp()
call in the matches() method.
The new CONFIG_FLAGS_FIXED_VALUE flag is initialized in builtin/config.c
based on the --fixed-value option, and that needs to be updated in
several callers.
This patch only affects some of the modes of 'git config', and the rest
will be completed in the next change.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 25 Nov 2020 22:12:53 +0000 (22:12 +0000)]
config: add --fixed-value option, un-implemented
The 'git config' builtin takes a 'value-pattern' parameter for several
actions. This can cause confusion when expecting exact value matches
instead of regex matches, especially when the input string contains
metacharacters. While callers can escape the patterns themselves, it
would be more friendly to allow an argument to disable the pattern
matching in favor of an exact string match.
Add a new '--fixed-value' option that does not currently change the
behavior. The implementation will be filled in by later changes for
each appropriate action. For now, check and test that --fixed-value
will abort the command when included with an incompatible action or
without a 'value-pattern' argument.
The name '--fixed-value' was chosen over something simpler like
'--fixed' because some commands allow regular expressions on the
key in addition to the value.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 25 Nov 2020 22:12:52 +0000 (22:12 +0000)]
t1300: add test for --replace-all with value-pattern
The --replace-all option was added in 4ddba79d (git-config-set: add more
options) but was not tested along with the 'value-pattern' parameter.
Since we will be updating this option to optionally treat 'value-pattern'
as a fixed string, let's add a test here that documents the current
behavior.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 25 Nov 2020 22:12:51 +0000 (22:12 +0000)]
t1300: test "set all" mode with value-pattern
Without additional modifiers, 'git config <key> <value>' attempts
to set a single value in the .git/config file. When the
value-pattern parameter is supplied, this command behaves in a
non-trivial manner.
Consider 'git config <key> <value> <value-pattern>'. The expected
behavior is as follows:
1. If there are multiple existing values that match 'value-pattern',
then the command fails. Users should use --replace-all instead.
2. If there is no existing values match 'value-pattern', then the
'key=value' pair is appended, making this 'key' a multi-valued
config setting.
3. If there is one existing value that matches 'value-pattern', then
the new config has one entry where 'key=value'.
Add a test that demonstrates these options. Break from the existing
pattern in t1300-config.sh to use 'git config --file=<file>' instead of
modifying .git/config directly to prevent possibly incompatible repo
states. Also use 'git config --file=<file> --list' for config state
comparison instead of the config file format. This makes the tests
more readable.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 25 Nov 2020 22:12:50 +0000 (22:12 +0000)]
config: replace 'value_regex' with 'value_pattern'
The 'value_regex' argument in the 'git config' builtin is poorly named,
especially related to an upcoming change that allows exact string
matches instead of ERE pattern matches.
Perform a mostly mechanical change of every instance of 'value_regex' to
'value_pattern' in the codebase. This is only critical for documentation
and error messages, but it is best to be consistent inside the codebase,
too.
For documentation, use 'value-pattern' which is better punctuation. This
affects Documentation/git-config.txt and the usage in builtin/config.c,
which was already mixed between 'value_regex' and 'value-regex'.
I gave some thought to leaving the value_regex variables inside config.c
that are regex_t pointers. However, it is probably best to keep the name
consistent with the rest of the variables.
This does not update the translations inside the po/ directory, as that
creates conflicts with ongoing work. The input strings should
automatically update through automation, and a few of the output strings
currently use "[value_regex]" directly.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 25 Nov 2020 22:12:49 +0000 (22:12 +0000)]
config: convert multi_replace to flags
We will extend the flexibility of the config API. Before doing so, let's
take an existing 'int multi_replace' parameter and replace it with a new
'unsigned flags' parameter that can take multiple options as a bit field.
Update all callers that specified multi_replace to now specify the
CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the
documentation of git_config_set_multivar_in_file() including a clear
labeling of its arguments. Other config API methods in config.h require
only a change of the final parameter from 'int' to 'unsigned'.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Wed, 25 Nov 2020 17:17:33 +0000 (12:17 -0500)]
midx.c: protect against disappearing packs
When a packed object is stored in a multi-pack index, but that pack has
racily gone away, the MIDX code simply calls die(), when it could be
returning an error to the caller, which would in turn lead to
re-scanning the pack directory.
A pack can racily disappear, for example, due to a simultaneous 'git
repack -ad',
You can also reproduce this with two terminals, where one is running:
git init
while true; do
git commit -q --allow-empty -m foo
git repack -ad
git multi-pack-index write
done
(in effect, constantly writing new MIDXs), and the other is running:
obj=$(git rev-parse HEAD)
while true; do
echo $obj | git cat-file --batch-check='%(objectsize:disk)' || break
done
That will sometimes hit the error preparing packfile from
multi-pack-index message, which this patch fixes.
Right now, that path to discovering a missing pack looks something like
'find_pack_entry()' calling 'fill_midx_entry()' and eventually making
its way to call 'nth_midxed_pack_entry()'.
'nth_midxed_pack_entry()' already checks 'is_pack_valid()' and
propagates an error if the pack is invalid. So, this works if the pack
has gone away between calling 'prepare_midx_pack()' and before calling
'is_pack_valid()', but not if it disappears before then.
Catch the case where the pack has already disappeared before
'prepare_midx_pack()' by returning an error in that case, too.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Wed, 25 Nov 2020 17:17:28 +0000 (12:17 -0500)]
packfile.c: protect against disappearing indexes
In 17c35c8969 (packfile: skip loading index if in multi-pack-index,
2018-07-12) we stopped loading the .idx file for packs that are
contained within a multi-pack index.
This saves us the effort of loading an .idx and doing some lightweight
validity checks by way of 'packfile.c:load_idx()', but introduces a race
between processes that need to load the index (e.g., to generate a
reverse index) and processes that can delete the index.
will result in a segfault prior to this patch. What's happening here is
that we notice that the pack is in the multi-pack index, and so don't
check that it still has a .idx. When we then try and load that index to
generate a reverse index, we don't have it, so the call to
'find_pack_revindex()' in 'packfile.c:packed_object_info()' returns
NULL, and then dereferencing it causes a segfault.
Of course, we don't ever expect someone to remove the index file by
hand, or to be in a state where we never wrote it to begin with (yet
find that pack in the multi-pack-index). But, this can happen in a
timing race with 'git repack -ad', which removes all existing packs
after writing a new pack containing all of their objects.
Avoid this by reverting the hunk of 17c35c8969 which stops loading the
index when the pack is contained in a MIDX. This makes the latter half
of 17c35c8969 useless, since we'll always have a non-NULL
'p->index_data', in which case that if statement isn't guarding
anything.
These two together effectively revert 17c35c8969, and avoid the race
explained above.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Drew DeVault [Wed, 25 Nov 2020 21:01:45 +0000 (13:01 -0800)]
help.c: help.autocorrect=never means "do not compute suggestions"
While help.autocorrect can be set to 0 to decline auto-execution of
possibly mistyped commands, it still spends cycles to compute the
suggestions, and it wastes screen real estate.
Update help.autocorrect to accept the string "never" to just exit
with error upon mistyped commands to help users who prefer to never
see suggested corrections at all.
While at it, introduce "immediate" as a more readable way to
immediately execute the auto-corrected command, which can be done
with negative value.
Signed-off-by: Drew DeVault <sir@cmpwn.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Simão Afonso [Wed, 25 Nov 2020 18:31:23 +0000 (18:31 +0000)]
crendential-store: use timeout when locking file
When holding the lock for rewriting the credential file, use a timeout
to avoid race conditions when the credentials file needs to be updated
in parallel.
An example would be doing `fetch --all` on a repository with several
remotes that need credentials, using parallel fetching.
The timeout can be configured using "credentialStore.lockTimeoutMS",
defaulting to 1 second.
Signed-off-by: Simão Afonso <simao.afonso@powertools-tech.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan [Mon, 23 Nov 2020 20:45:22 +0000 (12:45 -0800)]
usage: add trace2 entry upon warning()
Emit a trace2 error event whenever warning() is called, just like when
die(), error(), or usage() is called.
This helps debugging issues that would trigger warnings but not errors.
In particular, this might have helped debugging an issue I encountered
with commit graphs at $DAYJOB [1].
There is a tradeoff between including potentially relevant messages and
cluttering up the trace output produced. I think that warning() messages
should be included in traces, because by its nature, Git is used over
multiple invocations of the Git tool, and a failure (currently traced)
in a Git invocation might be caused by an unexpected interaction in a
previous Git invocation that only has a warning (currently untraced) as
a symptom - as is the case in [1].
Junio C Hamano [Fri, 20 Nov 2020 17:52:26 +0000 (09:52 -0800)]
MyFirstContribition: answering questions is not the end of the story
A review exchange may begin with a reviewer asking "what did you
mean by this phrase in your log message (or here in the doc)?", the
author answering what was meant, and then the reviewer saying "ah,
that is what you meant---then the flow of the logic makes sense".
But that is not the happy end of the story. New contributors often
forget that the material that has been reviewed in the above exchange
is still unclear in the same way to the next person who reads it,
until it gets updated.
While we are in the vicinity, rephrase the verb "request" used to
refer to comments by reviewers to "suggest"---this matches the
contrast between "original" and "suggested" that appears later in
the same paragraph, and more importantly makes it clearer that it is
not like authors are to please reviewers' wishes but rather
reviewers are merely helping authors to polish their commits.
Reviewed-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t3404: do not depend on any specific default branch name
Now that we can override the default branch name in the tests via
`GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME`, we should avoid expecting a
particular hard-coded name.
So let's rename the initial branch immediately to `primary` and work
with that.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 24 Nov 2020 09:06:05 +0000 (04:06 -0500)]
submodule: fix fetch_in_submodule logic
Commit 1c1518071c (submodule: use "fetch" logic instead of custom remote
discovery, 2020-11-14) rewrote the logic in fetch_in_submodule to do:
elif test "$2" -ne ""
But this is nonsense in shell: -ne is for numeric comparisons. This
should be "=" or more idiomatically:
elif test -n "$2"
But once we fix that, many tests start failing. Because that commit
introduced another problem. The caller that passes 3 arguments looks
like this:
fetch_in_submodule "$sm_path" $depth "$sha1"
Note the unquoted $depth parameter. When it isn't set, the function will
see only 2 arguments, and the function has no idea if what it sees in $2
is an option to go on the command line, or a refspec to pass on stdin.
In the old code before that commit:
fetch_in_submodule () (
sanitize_submodule_env &&
cd "$1" &&
- case "$2" in
- '')
- git fetch ;;
- *)
- shift
- git fetch $(get_default_remote) "$@" ;;
- esac
we treated those the same, so it didn't matter. But in the new logic
(with my fix above):
+ if test $# -eq 3
+ then
+ echo "$3" | git fetch --stdin "$2"
+ elif test -n "$n"
+ then
+ git fetch "$2"
+ else
+ git fetch
+ fi
we use the number of parameters to distinguish the two. Let's insist
that the caller pass an empty string for positional parameter two if
they want to have a third parameter after it.
But that still leaves one problem. In the --stdin block, we
unconditionally pass "$2" to git-fetch, even if it's the empty string.
Rather than add another conditional, we can use :+ parameter expansion
to include it only if it's non-empty. In fact, we can do the same for
the elif, too, simplifying it further. Technically this is overkill,
since we know the --depth parameter will not have whitespace (and
indeed, most callers do not bother quoting it), but it doesn't hurt for
the function to be careful.
It's somewhat amazing that no tests were failing. I think what happened
is that:
- the 3-arg form rarely triggered; any call with a non-empty $depth
and a $sha1 would work, but one with an empty $depth would only have
2 arguments
- because of the wrong arguments to "test", the shell would complain
and exit non-zero. So we never ran the middle conditional at all
- that left every call running "git fetch" with no arguments. A
well-written test could have detected the distinction here, but in
practice omitting --depth just means fetching more commits, and
fetching everything (rather than a single sha1) works as long as the
commit in question is reachable
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Tue, 24 Nov 2020 04:16:43 +0000 (04:16 +0000)]
maintenance: include 'cron' details in docs
Advanced and expert users may want to know how 'git maintenance start'
schedules background maintenance in order to customize their own
schedules beyond what the maintenance.* config values allow. Start a new
set of sections in git-maintenance.txt that describe how 'cron' is used
to run these tasks.
This is particularly valuable for users who want to inspect what Git is
doing or for users who want to customize the schedule further. Having a
baseline can provide a way forward for users who have never worked with
cron schedules.
Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Tue, 24 Nov 2020 04:16:42 +0000 (04:16 +0000)]
maintenance: extract platform-specific scheduling
The existing schedule mechanism using 'cron' is supported by POSIX
platforms, but not Windows. It also works slightly differently on
macOS to significant detriment of the user experience. To allow for
new implementations on these platforms, extract a method that
performs the platform-specific scheduling mechanism. This will be
swapped at compile time with new implementations on specialized
platforms.
As we add this generality, rename GIT_TEST_CRONTAB to
GIT_TEST_MAINT_SCHEDULER. Further, this variable is now parsed as
"<scheduler>:<command>" so we can test platform-specific scheduling
logic even when not on the correct platform. By specifying the
<scheduler> in this string, we will be able to test all three sets of
Git logic from a Linux machine.
Co-authored-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Nate Avers [Mon, 23 Nov 2020 03:23:41 +0000 (22:23 -0500)]
notes.c: fix a segfault in notes_display_config()
If notes.displayRef is configured with no value[1], control should be
returned to the caller when notes.c:notes_display_config() checks if 'v'
is NULL. Otherwise, both git log --notes and git diff-tree --notes will
subsequently segfault when refs.h:has_glob_specials() calls strpbrk()
with a NULL first argument.
Junio C Hamano [Sat, 21 Nov 2020 23:14:39 +0000 (15:14 -0800)]
Merge branch 'pd/mergetool-nvimdiff'
Fix regression introduced when nvimdiff support in mergetool was added.
* pd/mergetool-nvimdiff:
mergetool: avoid letting `list_tool_variants` break user-defined setups
mergetools/bc: add `bc4` to the alias list for Beyond Compare
Junio C Hamano [Sat, 21 Nov 2020 23:14:38 +0000 (15:14 -0800)]
Merge branch 'en/strmap'
A specialization of hashmap that uses a string as key has been
introduced. Hopefully it will see wider use over time.
* en/strmap:
shortlog: use strset from strmap.h
Use new HASHMAP_INIT macro to simplify hashmap initialization
strmap: take advantage of FLEXPTR_ALLOC_STR when relevant
strmap: enable allocations to come from a mem_pool
strmap: add a strset sub-type
strmap: split create_entry() out of strmap_put()
strmap: add functions facilitating use as a string->int map
strmap: enable faster clearing and reusing of strmaps
strmap: add more utility functions
strmap: new utility functions
hashmap: provide deallocation function names
hashmap: introduce a new hashmap_partial_clear()
hashmap: allow re-use after hashmap_free()
hashmap: adjust spacing to fix argument alignment
hashmap: add usage documentation explaining hashmap_free[_entries]()
Junio C Hamano [Sat, 21 Nov 2020 23:14:38 +0000 (15:14 -0800)]
Merge branch 'jk/rev-parse-end-of-options'
"git rev-parse" learned the "--end-of-options" to help scripts to
safely take a parameter that is supposed to be a revision, e.g.
"git rev-parse --verify -q --end-of-options $rev".
* jk/rev-parse-end-of-options:
rev-parse: handle --end-of-options
rev-parse: put all options under the "-" check
rev-parse: don't accept options after dashdash
Martin Ågren [Sat, 21 Nov 2020 18:31:08 +0000 (19:31 +0100)]
grep: use designated initializers for `grep_defaults`
In 15fabd1bbd ("builtin/grep.c: make configuration callback more
reusable", 2012-10-09), we learned to fill a `static struct grep_opt
grep_defaults` which we can use as a blueprint for other such structs.
At the time, we didn't consider designated initializers to be widely
useable, but these days, we do. (See, e.g., cbc0f81d96 ("strbuf: use
designated initializers in STRBUF_INIT", 2017-07-10).)
Use designated initializers to let the compiler set up the struct and so
that we don't need to remember to call `init_grep_defaults()`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Martin Ågren [Sat, 21 Nov 2020 18:31:07 +0000 (19:31 +0100)]
grep: don't set up a "default" repo for grep
`init_grep_defaults()` fills a `static struct grep_opt grep_defaults`.
This struct is then used by `grep_init()` as a blueprint for other such
structs. Notably, `grep_init()` takes a `struct repo *` and assigns it
into the target struct.
As a result, it is unnecessary for us to take a `struct repo *` in
`init_grep_defaults()` as well. We assign it into the default struct and
never look at it again. And in light of how we return early if we have
already set up the default struct, it's not just unnecessary, but is
also a bit confusing: If we are called twice and with different repos,
is it a bug or a feature that we ignore the second repo?
Drop the repo parameter for `init_grep_defaults()`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Sat, 21 Nov 2020 00:29:21 +0000 (19:29 -0500)]
send-pack: kill pack-objects helper on signal or exit
We spawn an external pack-objects process to actually send
objects to the remote side. If we are killed by a signal
during this process, the pack-objects will keep running and
complete the push, which may surprise the user. We should
take it down when we go down.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Matheus Tavares [Fri, 20 Nov 2020 15:09:39 +0000 (12:09 -0300)]
worktree: fix order of arguments in error message
`git worktree add` (without --force) errors out when given a path
that is already registered as a worktree and the path is missing on
disk. But the `cmd` and `path` strings are switched on the error
message. Let's fix that.
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
gc: rename keep_base_pack variable for --keep-largest-pack
As noted in an earlier change the keep_base_pack variable name is a
relic from an earlier on-list version of ae4e89e549 ("gc: add
--keep-largest-pack option", 2018-04-15) before it was renamed to
--keep-largest-pack.
Let's change the variable name to avoid that confusion, it's easier to
read the code if there's a 1=1 mapping between the variable name and
option name.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
gc docs: change --keep-base-pack to --keep-largest-pack
The --keep-base-pack option never existed in git.git. It was the name
for the --keep-largest-pack option in earlier revisions of that series
before it landed as ae4e89e549 ("gc: add --keep-largest-pack option",
2018-04-15).
The later patches in that series[1][2] weren't changed to also refer
to --keep-largest-pack, so we've had this reference to a nonexisting
option ever since the feature initially landed.
2. 9806f5a7bf ("gc --auto: exclude base pack if not enough mem to
"repack -ad"", 2018-04-15)
Reported-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>