A more structured way to obtain execution trace has been added.
* jh/trace2:
trace2: add for_each macros to clang-format
trace2: t/helper/test-trace2, t0210.sh, t0211.sh, t0212.sh
trace2:data: add subverb for rebase
trace2:data: add subverb to reset command
trace2:data: add subverb to checkout command
trace2:data: pack-objects: add trace2 regions
trace2:data: add trace2 instrumentation to index read/write
trace2:data: add trace2 hook classification
trace2:data: add trace2 transport child classification
trace2:data: add trace2 sub-process classification
trace2:data: add editor/pager child classification
trace2:data: add trace2 regions to wt-status
trace2: collect Windows-specific process information
trace2: create new combined trace facility
trace2: Documentation/technical/api-trace2.txt
"git prune-packed" did not notice and complain against excess
arguments given from the command line, which now it does.
* rj/prune-packed-excess-args:
prune-packed: check for too many arguments
Output from "diff --cc" did not show the original paths when the
merge involved renames. A new option adds the paths in the
original trees to the output.
* en/combined-all-paths:
log,diff-tree: add --combined-all-paths option
Update the implementation of pack-redundant for performance in a
repository with many packfiles.
* sc/pack-redundant:
pack-redundant: consistent sort method
pack-redundant: rename pack_list.all_objects
pack-redundant: new algorithm to find min packs
pack-redundant: delete redundant code
pack-redundant: delay creation of unique_objects
t5323: test cases for git-pack-redundant
Four new configuration variables {author,committer}.{name,email}
have been introduced to override user.{name,email} in more specific
cases.
* wh/author-committer-ident-config:
config: allow giving separate author and committer idents
The diff machinery, one of the oldest parts of the system, which
long predates the parse-options API, uses fairly long and complex
handcrafted option parser. This is being rewritten to use the
parse-options API.
* nd/diff-parseopt:
diff.c: convert --raw
diff.c: convert -W|--[no-]function-context
diff.c: convert -U|--unified
diff.c: convert -u|-p|--patch
diff.c: prepare to use parse_options() for parsing
diff.h: avoid bit fields in struct diff_flags
diff.h: keep forward struct declarations sorted
parse-options: allow ll_callback with OPTION_CALLBACK
parse-options: avoid magic return codes
parse-options: stop abusing 'callback' for lowlevel callbacks
parse-options: add OPT_BITOP()
parse-options: disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
parse-options: add one-shot mode
parse-options.h: remove extern on function prototypes
"git checkout --no-overlay" can be used to trigger a new mode of
checking out paths out of the tree-ish, that allows paths that
match the pathspec that are in the current index and working tree
and are not in the tree-ish.
* tg/checkout-no-overlay:
revert "checkout: introduce checkout.overlayMode config"
checkout: introduce checkout.overlayMode config
checkout: introduce --{,no-}overlay option
checkout: factor out mark_cache_entry_for_checkout function
checkout: clarify comment
read-cache: add invalidate parameter to remove_marked_cache_entries
entry: support CE_WT_REMOVE flag in checkout_entry
entry: factor out unlink_entry function
move worktree tests to t24*
Add a GIT_TEST_STASH_USE_BUILTIN=false test mode which is equivalent
to running with stash.useBuiltin=false. This is needed to spot that
we're not introducing any regressions in the legacy stash version
while we're carrying both it and the new built-in version.
This imitates the equivalent treatment for the built-in rebase in
62c23938fa (tests: add a special setup where rebase.useBuiltin is off,
2018-11-14).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We recently converted the `git stash` command from Unix shell scripts
to builtins.
Let's end users a way out when they discover a bug in the
builtin command: `stash.useBuiltin`.
As the file name `git-stash` is already in use, let's rename the
scripted backend to `git-legacy-stash`.
To make the test suite pass with `stash.useBuiltin=false`, this commit
also backports rudimentary support for `-q` (but only *just* enough
to appease the test suite), and adds a super-ugly hack to force exit
code 129 for `git stash -h`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The old shell script `git-stash.sh` was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.
This commit also removes the word `helper` since now stash is
called directly and not by a shell script.
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commits introduces a optimization by avoiding calling the
same functions again. For example, `git stash push -u`
would call at some points the following functions:
* `check_changes()` (inside `do_push_stash()`)
* `do_create_stash()`, which calls: `check_changes()` and
`get_untracked_files()`
Note that `check_changes()` also calls `get_untracked_files()`.
So, `check_changes()` is called 2 times and `get_untracked_files()`
3 times.
The old function `check_changes()` now consists of two functions:
`get_untracked_files()` and `check_changes_tracked_files()`.
These are the call chains for `push` and `create`:
* `push_stash()` -> `do_push_stash()` -> `do_create_stash()`
* `create_stash()` -> `do_create_stash()`
To prevent calling the same functions over and over again,
`check_changes()` inside `do_create_stash()` is now placed
in the caller functions (`create_stash()` and `do_push_stash()`).
This way `check_changes()` and `get_untracked files()` are called
only one time.
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add stash save to the helper and delete functions which are no
longer needed (`show_help()`, `save_stash()`, `push_stash()`,
`create_stash()`, `clear_stash()`, `untracked_files()` and
`no_changes()`).
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There is a change in behaviour with this commit. When there was
no initial commit, the shell version of stash would still display
a message. This commit makes `push` to not display any message if
`--quiet` or `-q` is specified. Add tests for `--quiet`.
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add stash show to the helper and delete the show_stash, have_stash,
assert_stash_like, is_stash_like and parse_flags_and_rev functions
from the shell script now that they are no longer needed.
In shell version, although `git stash show` accepts `--index` and
`--quiet` options, it ignores them. In C, both options are passed
further to `git diff`.
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref functions from the shell script now that they
are no longer needed.
Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add stash branch to the helper and delete the apply_to_branch
function from the shell script.
Checkout does not currently provide a function for checking out
a branch as cmd_checkout does a large amount of sanity checks
first that we require here.
Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.
We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.
Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a builtin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
lets conversion get started without the other commands being
finished.
The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.
Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.
Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As transform_todo_file() is only needed inside of
rebase--interactive.c for `rebase -p', it is moved there from
sequencer.c.
The parameter r (repository) is dropped along the way.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
edit_todo_list() is changed to work on a todo_list, and to handle the
initial edition of the todo list (ie. making a backup of the todo
list).
It does not check for dropped commits yet, as todo_list_check() does not
take the commits that have already been processed by the rebase (ie. the
todo list is edited in the middle of a rebase session).
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As rearrange_squash_in_todo_file() is only needed inside of
rebase--interactive.c for `rebase -p', it is moved there from
sequencer.c.
The parameter r (repository) is dropped along the way, and the error
handling is slightly improved.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As sequencer_add_exec_commands() is only needed inside of
rebase--interactive.c for `rebase -p', it is moved there from
sequencer.c.
The parameter r (repository) is dropped along the way.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
complete_action() used functions that read the todo-list file, made some
changes to it, and wrote it back to the disk.
The previous commits were dedicated to separate the part that deals with
the file from the actual logic of these functions. Now that this is
done, we can call directly the "logic" functions to avoid useless file
access.
The parsing of the list has to be done by the caller. If the buffer of
the todo list provided by the caller is empty, a `noop' command is
directly added to the todo list, without touching the buffer.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This makes sequencer_make_script() write its script to a strbuf (ie. the
buffer of a todo_list) instead of a FILE. This reduce the amount of
read/write made by rebase interactive.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This refactors rearrange_squash() to work on a todo_list to avoid
redundant reads and writes. The function is renamed
todo_list_rearrange_squash().
The old version created a new buffer, which was directly written to the
disk. This new version creates a new item list by just copying items
from the old item list, without creating a new buffer. This eliminates
the need to reparse the todo list, but this also means its buffer cannot
be directly written to the disk.
As rebase -p still need to check the todo list from the disk, a new
function is introduced, rearrange_squash_in_todo_file().
complete_action() still uses rearrange_squash_in_todo_file() for now.
This will be changed in a future commit.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This refactors sequencer_add_exec_commands() to work on a todo_list to
avoid redundant reads and writes to the disk.
Instead of inserting the `exec' commands between the other commands and
re-parsing the buffer at the end, they are appended to the buffer once,
and a new list of items is created. Items from the old list are copied
across and new `exec' items are appended when necessary. This
eliminates the need to reparse the buffer, but this also means we have
to use todo_list_write_to_disk() to write the file.
todo_list_add_exec_commands() and sequencer_add_exec_commands() are
modified to take a string list instead of a string -- one item for each
command. This makes it easier to insert a new command to the todo list
for each command to execute.
sequencer_add_exec_commands() still reads the todo list from the disk,
as it is needed by rebase -p.
complete_action() still uses sequencer_add_exec_commands() for now.
This will be changed in a future commit.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The --connectivity-only option avoids opening every object, and instead
just marks reachable objects with a flag and compares this to the set
of all objects. This strategy is discussed in more detail in 3e3f8bd608
(fsck: prepare dummy objects for --connectivity-check, 2017-01-17).
This means that we report _every_ unreachable object as dangling.
Whereas in a full fsck, we'd have actually opened and parsed each of
those unreachable objects, marking their child objects with the USED
flag, to mean "this was mentioned by another object". And thus we can
report only the tip of an unreachable segment of the object graph as
dangling.
You can see this difference with a trivial example:
tree=$(git hash-object -t tree -w /dev/null)
one=$(echo one | git commit-tree $tree)
two=$(echo two | git commit-tree -p $one $tree)
Running `git fsck` will report only $two as dangling, but with
--connectivity-only, both commits (and the tree) are reported. Likewise,
using --lost-found would write all three objects.
We can make --connectivity-only work like the normal case by taking a
separate pass over the unreachable objects, parsing them and marking
objects they refer to as USED. That still avoids parsing any blobs,
though we do pay the cost to access any unreachable commits and trees
(which may or may not be noticeable, depending on how many you have).
If neither --dangling nor --lost-found is in effect, then we can skip
this step entirely, just like we do now. That makes "--connectivity-only
--no-dangling" just as fast as the current "--connectivity-only". I.e.,
we do the correct thing always, but you can still tweak the options to
make it faster if you don't care about dangling objects.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This fixes a regression in 7c0fe330d5 (rev-list: handle missing tree
objects properly, 2018-10-05) where rev-list will now complain about the
empty tree when it doesn't physically exist on disk.
Before that commit, we relied on the traversal code in list-objects.c to
walk through the trees. Since it uses parse_tree(), we'd do a normal
object lookup that includes looking in the set of "cached" objects
(which is where our magic internal empty-tree kicks in).
After that commit, we instead tell list-objects.c not to die on any
missing trees, and we check them ourselves using has_object_file(). But
that function uses OBJECT_INFO_SKIP_CACHED, which means we won't use our
internal empty tree.
This normally wouldn't come up. For most operations, Git will try to
write out the empty tree object as it would any other object. And
pack-objects in a push or fetch will send the empty tree (even if it's
virtual on the sending side). However, there are cases where this can
matter. One I found in the wild:
1. The root tree of a commit became empty by deleting all files,
without using an index. In this case it was done using libgit2's
tree builder API, but as the included test shows, it can easily be
done with regular git using hash-object.
The resulting repo works OK, as we'd avoid walking over our own
reachable commits for a connectivity check.
2. Cloning with --reference pointing to the repository from (1) can
trigger the problem, because we tell the other side we already have
that commit (and hence the empty tree), but then walk over it
during the connectivity check (where we complain about it missing).
Arguably the workflow in step (1) should be more careful about writing
the empty tree object if we're referencing it. But this workflow did
work prior to 7c0fe330d5, so let's restore it.
This patch makes the minimal fix, which is to swap out a direct call to
oid_object_info_extended(), minus the SKIP_CACHED flag, instead of
calling has_object_file(). This is all that has_object_file() is doing
under the hood. And there's little danger of unrelated fallout from
other unexpected "cached" objects, since there's only one call site that
ends such a cached object, and it's in git-blame.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The default SIGPIPE behavior can be useful for a command that generates
a lot of output: if the receiver of our output goes away, we'll be
notified asynchronously to stop generating it (typically by killing the
program).
But for a command like fetch, which is primarily concerned with
receiving data and writing it to disk, an unexpected SIGPIPE can be
awkward. We're already checking the return value of all of our write()
calls, and dying due to the signal takes away our chance to gracefully
handle the error.
On Linux, we wouldn't generally see SIGPIPE at all during fetch. If the
other side of the network connection hangs up, we'll see ECONNRESET. But
on OS X, we get a SIGPIPE, and the process is killed. This causes t5570
to racily fail, as we sometimes die by signal (instead of the expected
die() call) when the server side hangs up.
Let's ignore SIGPIPE during the network portion of the fetch, which will
cause our write() to return EPIPE, giving us consistent behavior across
platforms.
This fixes the test flakiness, but note that it stops short of fixing
the larger problem. The server side hit a fatal error, sent us an "ERR"
packet, and then hung up. We notice the failure because we're trying to
write to a closed socket. But by dying immediately, we never actually
read the ERR packet and report its content to the user. This is a (racy)
problem on all platforms. So this patch lays the groundwork from which
that problem might be fixed consistently, but it doesn't actually fix
it.
Note the placement of the SIGPIPE handling. The absolute minimal change
would be to ignore SIGPIPE only when we're writing. But twiddling the
signal handler for each write call is inefficient and maintenance
burden. On the opposite end of the spectrum, we could simply declare
that fetch does not need SIGPIPE handling, since it doesn't generate a
lot of output, and we could just ignore it at the start of cmd_fetch().
This patch takes a middle ground. It ignores SIGPIPE during the network
operation (which is admittedly most of the program, since the actual
network operations are all done under the hood by the transport code).
So it's still pretty coarse.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Technically, the scripted version set ORIG_HEAD only in two spots (which
really could have been one, because it called `git checkout $onto^0` to
start the rebase and also if it could take a shortcut, and in both cases
it called `git update-ref $orig_head`).
Practically, it *implicitly* reset ORIG_HEAD whenever `git reset --hard`
was called.
However, what we really want is that it is set exactly once, at the
beginning of the rebase.
So let's do that.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the case that the rebase boils down to a fast-forward, the built-in
rebase reset the working tree twice: once to start the rebase at `onto`,
then realizing that the original (pre-rebase) HEAD was an ancestor and
we basically already fast-forwarded to the post-rebase HEAD,
`reset_head()` was called to update the original ref and to point HEAD
back to it.
That second `reset_head()` call does not need to touch the working tree,
though, as it does not change the actual tip commit (and therefore the
working tree should stay unchanged anyway): only the ref needs to be
updated (because the rebase detached the HEAD, and we want to go back to
the branch on which the rebase was started).
But that second `reset_head()` was called without the flag to leave the
working tree alone (the reason: when that call was introduced, that flag
was not yet even thought of). Let's avoid that unnecessary work by
passing that flag.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
After we set up a `struct repository_format`, it owns various pieces of
allocated memory. We then either use those members, because we decide we
want to use the "candidate" repository format, or we discard the
candidate / scratch space. In the first case, we transfer ownership of
the memory to a few global variables. In the latter case, we just
silently drop the struct and end up leaking memory.
Introduce an initialization macro `REPOSITORY_FORMAT_INIT` and a
function `clear_repository_format()`, to be used on each side of
`read_repository_format()`. To have a clear and simple memory ownership,
let all users of `struct repository_format` duplicate the strings that
they take from it, rather than stealing the pointers.
Call `clear_...()` at the start of `read_...()` instead of just zeroing
the struct, since we sometimes enter the function multiple times. Thus,
it is important to initialize the struct before calling `read_...()`, so
document that. It's also important because we might not even call
`read_...()` before we call `clear_...()`, see, e.g., builtin/init-db.c.
Teach `read_...()` to clear the struct on error, so that it is reset to
a safe state, and document this. (In `setup_git_directory_gently()`, we
look at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we
weren't actually supposed to do per the API. After this commit, that's
ok.)
We inherit the existing code's combining "error" and "no version found".
Both are signalled through `version == -1` and now both cause us to
clear any partial configuration we have picked up. For "extensions.*",
that's fine, since they require a positive version number. For
"core.bare" and "core.worktree", we're already verifying that we have a
non-negative version number before using them.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When "--no-index" is in effect (or implied by the arguments), git-diff
jumps early to a special code path to perform that diff. This means we
miss out on some settings like enabling --ext-diff and --textconv by
default.
Let's jump to the no-index path _after_ we've done more setup on
rev.diffopt. Since some of the options don't affect us (e.g., items
related to the index), let's re-order the setup into two blocks (see the
in-code comments).
Note that we also need to stop re-initializing the diffopt struct in
diff_no_index(). This should not be necessary, as it will already have
been initialized by cmd_diff() (and there are no other callers). That in
turn lets us drop the "repository" argument from diff_no_index (which
never made much sense, since the whole point is that you don't need a
repository).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When studying the performance of 'git push' we would like to know
how much time is spent at various parts of the command. One area
that could cause performance trouble is 'git pack-objects'.
Add trace2 regions around the three main actions taken in this
command:
1. Enumerate objects.
2. Prepare pack.
3. Write pack-file.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Create a new unified tracing facility for git. The eventual intent is to
replace the current trace_printf* and trace_performance* routines with a
unified set of git_trace2* routines.
In addition to the usual printf-style API, trace2 provides higer-level
event verbs with fixed-fields allowing structured data to be written.
This makes post-processing and analysis easier for external tools.
Trace2 defines 3 output targets. These are set using the environment
variables "GIT_TR2", "GIT_TR2_PERF", and "GIT_TR2_EVENT". These may be
set to "1" or to an absolute pathname (just like the current GIT_TRACE).
* GIT_TR2 is intended to be a replacement for GIT_TRACE and logs command
summary data.
* GIT_TR2_PERF is intended as a replacement for GIT_TRACE_PERFORMANCE.
It extends the output with columns for the command process, thread,
repo, absolute and relative elapsed times. It reports events for
child process start/stop, thread start/stop, and per-thread function
nesting.
* GIT_TR2_EVENT is a new structured format. It writes event data as a
series of JSON records.
Calls to trace2 functions log to any of the 3 output targets enabled
without the need to call different trace_printf* or trace_performance*
routines.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The make_cover_letter() function is supposed to open a new file for
writing, and let the caller write into it via FILE *rev->diffopt.file
but because the function does not return anything, the caller does not
bother checking the return value.
Make sure it dies, instead of keep going with a NULL output
filestream and relying on it to cause a crash, when it fails to
open the file.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The resolve_ref_unsafe() function can, and sometimes will in the case
of this codepath, return the char * passed to it to the caller. In
this case we construct a strbuf, free it, and then continue using the
dst_name after that free().
The code being fixed dates back to da3efdb17b ("receive-pack: detect
aliased updates which can occur with symrefs", 2010-04-19). When it
was originally added it didn't have this bug, it was introduced when
it was subsequently modified to use strbuf in 6b01ecfe22 ("ref
namespaces: Support remote repositories via upload-pack and
receive-pack", 2011-07-08).
This is theoretically a security issue, the C standard makes no
guarantees that a value you use after free() hasn't been poked at or
changed by something else on the system, but in practice modern OSs
will have mapped the relevant page to this process, so nothing else
would have used it. We do no further allocations between the free()
and use-after-free, so we ourselves didn't corrupt or change the
value.
Jeff investigated that and found: "It probably would be an issue if
the allocation were larger. glibc at least will use mmap()/munmap()
after some cutoff[1], in which case we'd get a segfault from hitting
the unmapped page. But for small allocations, it just bumps brk() and
the memory is still available for further allocations after
free(). [...] If you had a sufficiently large refname you might be
able to trigger the bug [...]. I tried to push such a ref. I had to
manually make a packed-refs file with the long name to avoid
filesystem limits (though probably you could have a long a/b/c/ name
on ext4). But the result can't actually be pushed, because it all has
to fit into a 64k pkt-line as part of the push protocol.".
An a alternative and more succinct way of implementing this would have
been to do the strbuf_release() at the end of check_aliased_update()
and use "goto out" instead of the early "return" statements. Hopefully
this approach of using a helper instead makes it easier to follow.
1. Jeff: "Weirdly, the mmap() cutoff on my glibc system is 135168
bytes. Which is...2^17 + 2^12? 33 pages? I'm sure there's a good
reason for that, but I didn't dig into it."
Reported-by: 王健强 <jianqiang.wang@securitygossip.com>
Helped-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>
This adds value completion for a couple more paramters. To make it
easier to maintain these hard coded lists, add a comment at the original
list/code to remind people to update git-completion.bash too.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
