A given path should only ever be associated with a single registered
worktree. This invariant is enforced by refusing to create a new
worktree at a given path if that path already exists. For example:
$ git worktree add -q --detach foo
$ git worktree add -q --detach foo
fatal: 'foo' already exists
However, the check can be fooled, and the invariant broken, if the
path is missing. Continuing the example:
$ rm -fr foo
$ git worktree add -q --detach foo
$ git worktree list
... eadebfe [master]
.../foo eadebfe (detached HEAD)
.../foo eadebfe (detached HEAD)
This "corruption" leads to the unfortunate situation in which the
worktree can not be removed:
$ git worktree remove foo
fatal: validation failed, cannot remove working tree: '.../foo'
does not point back to '.git/worktrees/foo'
Nor can the bogus entry be pruned:
$ git worktree prune -v
$ git worktree list
... eadebfe [master]
.../foo eadebfe (detached HEAD)
.../foo eadebfe (detached HEAD)
without first deleting the worktree directory manually:
$ rm -fr foo
$ git worktree prune -v
Removing .../foo: gitdir file points to non-existent location
Removing .../foo1: gitdir file points to non-existent location
$ git worktree list
... eadebfe [master]
or by manually deleting the worktree entry in .git/worktrees.
To address this problem, upgrade "git worktree add" validation to
allow worktree creation only if the given path is not already
associated with an existing worktree (even if the path itself is
non-existent), thus preventing such bogus worktree entries from being
created in the first place.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Certain conditions must be met for a path to be a valid candidate as the
location of a new worktree; for instance, the path must not exist or
must be an empty directory. Although the number of conditions is small,
new conditions will soon be added so factor out the existing checks into
a separate function to avoid further bloating add_worktree().
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
prune_worktrees() and delete_git_dir() both remove worktree
administrative entries from .git/worktrees, and their implementations
are nearly identical. The only difference is that prune_worktrees() is
also capable of removing a bogus non-worktree-related file from
.git/worktrees.
Simplify by extending delete_git_dir() to handle the little bit of
extra functionality needed by prune_worktrees(), and drop the
effectively duplicate code from the latter.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is a pure code movement to avoid having to forward-declare the
function when new callers are subsequently added.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add the '--quiet' option to git worktree, as for the other git
commands. 'add' is the only command affected by it since all other
commands, except 'list', are currently silent by default.
[jc: appiled trivial fix-up to keep the tests from touching outside
the scratch area]
Helped-by: Martin Ågren <martin.agren@gmail.com>
Helped-by: Duy Nguyen <pclouds@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Pass the previously added "num_matches" struct value up to the callers
of unique_tracking_name(). This will allow callers to optionally print
better error messages in a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git worktree add" learned to check out an existing branch.
* tg/worktree-add-existing-branch:
worktree: teach "add" to check out existing branches
worktree: factor out dwim_branch function
worktree: improve message when creating a new worktree
worktree: remove extra members from struct add_opts
"git worktree remove" learned that "-f" is a shorthand for
"--force" option, just like for "git worktree add".
* sb/worktree-remove-opt-force:
worktree: accept -f as short for --force for removal
Currently 'git worktree add <path>' creates a new branch named after the
basename of the path by default. If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.
However we can do a little better than that, and check the branch out if
it is not checked out anywhere else. This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.
As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.
We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add <path>'. It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.
Factor it out now to reduce the patch noise in the next patch.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently 'git worktree add' produces output like the following:
Preparing ../foo (identifier foo)
HEAD is now at 26da330922 <title>
The '../foo' is the path where the worktree is created, which the user
has just given on the command line. The identifier is an internal
implementation detail, which is not particularly relevant for the user
and indeed isn't mentioned explicitly anywhere in the man page.
Instead of this message, print a message that gives the user a bit more
detail of what exactly 'git worktree' is doing. There are various dwim
modes which perform some magic under the hood, which should be
helpful to users. Just from the output of the command it is not always
visible to users what exactly has happened.
Help the users a bit more by modifying the "Preparing ..." message and
adding some additional information of what 'git worktree add' did under
the hood, while not displaying the identifier anymore.
Currently there are several different cases:
- 'git worktree add -b ...' or 'git worktree add <path>', both of
which create a new branch, either through the user explicitly
requesting it, or through 'git worktree add' implicitly creating
it. This will end up with the following output:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add -B ...', which may either create a new branch if
the branch with the given name does not exist yet, or resets an
existing branch to the current HEAD, or the commit-ish given.
Depending on which action is taken, we'll end up with the following
output:
Preparing worktree (resetting branch '<branch>'; was at caa68db14)
HEAD is now at 26da330922 <title>
or:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add --detach' or 'git worktree add <path>
<commit-ish>', both of which create a new worktree with a detached
HEAD, for which we will print the following output:
Preparing worktree (detached HEAD 26da330922)
HEAD is now at 26da330922 <title>
- 'git worktree add <path> <local-branch>', which checks out the
branch and prints the following output:
Preparing worktree (checking out '<local-branch>')
HEAD is now at 47007d5 <title>
Additionally currently the "Preparing ..." line is printed to stderr,
while the "HEAD is now at ..." line is printed to stdout by 'git reset
--hard', which is used internally by 'git worktree add'. Fix this
inconsistency by printing the "Preparing ..." message to stdout as
well. As "Preparing ..." is not an error, stdout also seems like the
more appropriate output stream.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There are two members of 'struct add_opts', which are only used inside
the 'add()' function, but being part of 'struct add_opts' they are
needlessly also passed to the 'add_worktree' function.
Make them local to the 'add()' function to make it clearer where they
are used.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Many commands support a "--force" option, frequently abbreviated as
"-f", however, "git worktree remove"'s hand-rolled OPT_BOOL forgets
to recognize the short form, despite git-worktree.txt documenting
"-f" as supported. Replace OPT_BOOL with OPT__FORCE, which provides
"-f" for free, and makes 'remove' consistent with 'add' option
parsing (which also specifies the PARSE_OPT_NOCOMPLETE flag).
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The way "git worktree prune" worked internally has been simplified,
by assuming how "git worktree move" moves an existing worktree to a
different place.
* nd/worktree-prune:
worktree prune: improve prune logic when worktree is moved
worktree: delete dead code
gc.txt: more details about what gc does
Conversion from uchar[20] to struct object_id continues.
* bc/object-id: (36 commits)
convert: convert to struct object_id
sha1_file: introduce a constant for max header length
Convert lookup_replace_object to struct object_id
sha1_file: convert read_sha1_file to struct object_id
sha1_file: convert read_object_with_reference to object_id
tree-walk: convert tree entry functions to object_id
streaming: convert istream internals to struct object_id
tree-walk: convert get_tree_entry_follow_symlinks internals to object_id
builtin/notes: convert static functions to object_id
builtin/fmt-merge-msg: convert remaining code to object_id
sha1_file: convert sha1_object_info* to object_id
Convert remaining callers of sha1_object_info_extended to object_id
packfile: convert unpack_entry to struct object_id
sha1_file: convert retry_bad_packed_offset to struct object_id
sha1_file: convert assert_sha1_type to object_id
builtin/mktree: convert to struct object_id
streaming: convert open_istream to use struct object_id
sha1_file: convert check_sha1_signature to struct object_id
sha1_file: convert read_loose_object to use struct object_id
builtin/index-pack: convert struct ref_delta_entry to object_id
...
Automatic detection of worktree relocation by a user (via 'mv', for
instance) was removed by 618244e160 (worktree: stop supporting moving
worktrees manually - 2016-01-22). Prior to that,
.git/worktrees/<tag>/gitdir was updated whenever the worktree was
accessed in order to let the pruning logic know that the worktree was
"active" even if it disappeared for a while (due to being located on
removable media, for instance).
"git worktree move" has come so we don't really need this, but since
it's easy to do, perhaps we could keep supporting manual worktree move
a bit longer. Notice that when a worktree is active, the "index" file
should be updated pretty often in common case. The logic is updated to
check for index mtime to see if the worktree is alive.
The old logic of checking gitdir's mtime is dropped because nobody
updates it anyway. The new corner case is, if the index file does not
exist, we immediately remove the stale worktree. But if the "index"
file does not exist, you may have a bigger problem.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This "link" was a feature in early iterations of multiple worktree
functionality for some reason it was dropped [1]. Since nobody creates
this "link", there's no need to check it.
This is mostly used to let the user moves a worktree manually [2]. If
you move a worktree within the same file system, this hard link count
lets us know the worktree is still there even if we don't know where it
is.
We support 'worktree move' now and don't need this anymore.
[1] last appearance in v4 message-id:
1393675983-3232-25-git-send-email-pclouds@gmail.com
and the reason in v5 was "revisit later", message-id:
1394246900-31535-1-git-send-email-pclouds@gmail.com
[2] 23af91d102 (prune: strategies for linked checkouts - 2014-11-30)
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teach parse-options API an option to help the completion script,
and make use of the mechanism in command line completion.
* nd/parseopt-completion: (45 commits)
completion: more subcommands in _git_notes()
completion: complete --{reuse,reedit}-message= for all notes subcmds
completion: simplify _git_notes
completion: don't set PARSE_OPT_NOCOMPLETE on --rerere-autoupdate
completion: use __gitcomp_builtin in _git_worktree
completion: use __gitcomp_builtin in _git_tag
completion: use __gitcomp_builtin in _git_status
completion: use __gitcomp_builtin in _git_show_branch
completion: use __gitcomp_builtin in _git_rm
completion: use __gitcomp_builtin in _git_revert
completion: use __gitcomp_builtin in _git_reset
completion: use __gitcomp_builtin in _git_replace
remote: force completing --mirror= instead of --mirror
completion: use __gitcomp_builtin in _git_remote
completion: use __gitcomp_builtin in _git_push
completion: use __gitcomp_builtin in _git_pull
completion: use __gitcomp_builtin in _git_notes
completion: use __gitcomp_builtin in _git_name_rev
completion: use __gitcomp_builtin in _git_mv
completion: use __gitcomp_builtin in _git_merge_base
...
"git worktree" learned move and remove subcommands.
* nd/worktree-move:
t2028: fix minor error and issues in newly-added "worktree move" tests
worktree remove: allow it when $GIT_WORK_TREE is already gone
worktree remove: new command
worktree move: refuse to move worktrees with submodules
worktree move: accept destination as directory
worktree move: new command
worktree.c: add update_worktree_location()
worktree.c: add validate_worktree()
Convert find_unique_abbrev and find_unique_abbrev_r to each take a
pointer to struct object_id.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git worktree add" learned to run the post-checkout hook, just like
"git clone" runs it upon the initial checkout.
* es/worktree-add-post-checkout-hook:
worktree: add: fix 'post-checkout' not knowing new worktree location
Although "git worktree add" learned to run the 'post-checkout' hook in
ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it
neglected to change to the directory of the newly-created worktree
before running the hook. Instead, the hook runs within the directory
from which the "git worktree add" command itself was invoked, which
effectively neuters the hook since it knows nothing about the new
worktree directory.
Further, ade546be47 failed to sanitize the environment before running
the hook, which means that user-assigned values of GIT_DIR and
GIT_WORK_TREE could mislead the hook about the location of the new
worktree. In the case of "git worktree add" being run from a bare
repository, the GIT_DIR="." assigned by Git itself leaks into the hook's
environment and breaks Git commands; this is so even when the working
directory is correctly changed to the new worktree before the hook runs
since ".", relative to the new worktree directory, does not point at the
bare repository.
Fix these problems by (1) changing to the new worktree's directory
before running the hook, and (2) sanitizing the environment of GIT_DIR
and GIT_WORK_TREE so hooks can't be confused by misleading values.
Enhance the t2025 'post-checkout' tests to verify that the hook is
indeed run within the correct directory and that Git commands invoked by
the hook compute Git-dir and top-level worktree locations correctly.
While at it, also add two new tests: (1) verify that the hook is run
within the correct directory even when the new worktree is created from
a sibling worktree (as opposed to the main worktree); (2) verify that
the hook is provided with correct context when the new worktree is
created from a bare repository (test provided by Lars Schneider).
Implementation Notes:
Rather than sanitizing the environment of GIT_DIR and GIT_WORK_TREE, an
alternative would be to set them explicitly, as is already done for
other Git commands run internally by "git worktree add". This patch opts
instead to sanitize the environment in order to clearly document that
the worktree is fully functional by the time the hook is run, thus does
not require special environmental overrides.
The hook is run manually, rather than via run_hook_le(), since it needs
to change the working directory to that of the worktree, and
run_hook_le() does not provide such functionality. As this is a one-off
case, adding 'run_hook' overloads which allow the directory to be set
does not seem warranted at this time.
Reported-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git worktree remove" basically consists of two things
- delete $GIT_WORK_TREE
- delete $GIT_DIR (which is $SUPER_GIT_DIR/worktrees/something)
If $GIT_WORK_TREE is already gone for some reason, we should be able
to finish the job by deleting $GIT_DIR.
Two notes:
- $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that
case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may
be in a usb stick somewhere. This is already handled because we
check for lock first.
- validate_worktree() is still called because it may do more checks in
future (and it already does something else, like checking main
worktree, but that's irrelevant in this case)
Noticed-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This command allows to delete a worktree. Like 'move' you cannot
remove the main worktree, or one with submodules inside [1].
For deleting $GIT_WORK_TREE, Untracked files or any staged entries are
considered precious and therefore prevent removal by default. Ignored
files are not precious.
When it comes to deleting $GIT_DIR, there's no "clean" check because
there should not be any valuable data in there, except:
- HEAD reflog. There is nothing we can do about this until somebody
steps up and implements the ref graveyard.
- Detached HEAD. Technically it can still be recovered. Although it
may be nice to warn about orphan commits like 'git checkout' does.
[1] We do 'git status' with --ignore-submodules=all for safety
anyway. But this needs a closer look by submodule people before we
can allow deletion. For example, if a submodule is totally clean,
but its repo not absorbed to the main .git dir, then deleting
worktree also deletes the valuable .submodule repo too.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Submodules contains .git files with relative paths. After a worktree
move, these files need to be updated or they may point to nowhere.
This is a bandage patch to make sure "worktree move" don't break
people's worktrees by accident. When .git file update code is in
place, this validate_no_submodules() could be removed.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Similar to "mv a b/", which is actually "mv a b/a", we extract basename
of source worktree and create a directory of the same name at
destination if dst path is a directory.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This command allows to relocate linked worktrees. Main worktree cannot
(yet) be moved.
There are two options to move the main worktree, but both have
complications, so it's not implemented yet. Anyway the options are:
- convert the main worktree to a linked one and move it away, leave
the git repository where it is. The repo essentially becomes bare
after this move.
- move the repository with the main worktree. The tricky part is make
sure all file descriptors to the repository are closed, or it may
fail on Windows.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The new completable options for "worktree add" are:
--checkout
--guess-remote
--lock
--track
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
--force option is most likely hidden from command line completion for
safety reasons. This is done by adding an extra flag
PARSE_OPT_NOCOMPLETE. Update OPT__FORCE() to accept additional
flags. Actual flag change comes later depending on individual
commands.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
c4738aed ("worktree: add can be created from any commit-ish",
2017-11-26) taught "git worktree add" to start a new worktree
with an arbitrary commit-ish checked out, not limited to a tip
of a branch.
"git worktree --help" was updated to describe this, but we forgot to
update "git worktree -h".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git worktree add" learned to run the post-checkout hook, just like
"git checkout" does, after the initial checkout.
* es/worktree-checkout-hook:
worktree: invoke post-checkout hook (unless --no-checkout)
git-clone and git-checkout both invoke the post-checkout hook following
a successful checkout, yet git-worktree neglects to do so even though it
too "checks out" the worktree. Fix this oversight.
Implementation note: The newly-created worktree may reference a branch
or be detached. In the latter case, a commit lookup is performed, though
the result is used only in a boolean sense to (a) determine if the
commit actually exists, and (b) assign either the branch name or commit
ID to HEAD. Since the post-commit hook needs to know the ID of the
checked-out commit, the lookup now needs to be done in all cases, rather
than only when detached. Consequently, a new boolean is needed to handle
(b) since the lookup result itself can no longer perform that role.
Reported-by: Matthew K Gumbel <matthew.k.gumbel@intel.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some users might want to have the --guess-remote option introduced in
the previous commit on by default, so they don't have to type it out
every time they create a new worktree.
Add a config option worktree.guessRemote that allows users to configure
the default behaviour for themselves.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently 'git worktree add <path>' creates a new branch named after the
basename of the <path>, that matches the HEAD of whichever worktree we
were on when calling "git worktree add <path>".
It's sometimes useful to have 'git worktree add <path> behave more like
the dwim machinery in 'git checkout <new-branch>', i.e. check if the new
branch name, derived from the basename of the <path>, uniquely matches
the branch name of a remote-tracking branch, and if so check out that
branch and set the upstream to the remote-tracking branch.
Add a new --guess-remote option that enables exactly that behaviour.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently 'git worktree add <path> <branch>', errors out when 'branch'
is not a local branch. It has no additional dwim'ing features that one
might expect.
Make it behave more like 'git checkout <branch>' when the branch doesn't
exist locally, but a remote tracking branch uniquely matches the desired
branch name, i.e. create a new branch from the remote tracking branch
and set the upstream to the remote tracking branch.
As 'git worktree add' currently just dies in this situation, there are
no backwards compatibility worries when introducing this feature.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently 'git worktree add' sets up tracking branches if '<branch>' is
a remote tracking branch, and doesn't set them up otherwise, as is the
default for 'git branch'.
This may or may not be what the user wants. Allow overriding this
behaviour with a --[no-]track flag that gets passed through to 'git
branch'.
We already respect branch.autoSetupMerge, as 'git worktree' just calls
'git branch' internally.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Convert the head_sha1 member to be head_oid instead. This is required
to convert resolve_ref_unsafe.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We try to read "len" bytes into a buffer and just assume
that it happened correctly. In practice this should usually
be the case, since we just stat'd the file to get the
length. But we could be fooled by transient errors or by
other processes racily truncating the file.
Let's be more careful. There's a slim chance this could
catch a real error, but it also prevents people and tools
from getting worried while reading the code.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
To read the "gitdir" file into memory, we stat the file and
allocate a buffer. But we store the size in an "int", which
may be truncated. We should use a size_t and xsize_t(),
which will detect truncation.
An overflow is unlikely for a "gitdir" file, but it's a good
practice to model.
Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
It's a common pattern in git commands to allocate some
memory that should last for the lifetime of the program and
then not bother to free it, relying on the OS to throw it
away.
This keeps the code simple, and it's fast (we don't waste
time traversing structures or calling free at the end of the
program). But it also triggers warnings from memory-leak
checkers like valgrind or LSAN. They know that the memory
was still allocated at program exit, but they don't know
_when_ the leaked memory stopped being useful. If it was
early in the program, then it's probably a real and
important leak. But if it was used right up until program
exit, it's not an interesting leak and we'd like to suppress
it so that we can see the real leaks.
This patch introduces an UNLEAK() macro that lets us do so.
To understand its design, let's first look at some of the
alternatives.
Unfortunately the suppression systems offered by
leak-checking tools don't quite do what we want. A
leak-checker basically knows two things:
1. Which blocks were allocated via malloc, and the
callstack during the allocation.
2. Which blocks were left un-freed at the end of the
program (and which are unreachable, but more on that
later).
Their suppressions work by mentioning the function or
callstack of a particular allocation, and marking it as OK
to leak. So imagine you have code like this:
int cmd_foo(...)
{
/* this allocates some memory */
char *p = some_function();
printf("%s", p);
return 0;
}
You can say "ignore allocations from some_function(),
they're not leaks". But that's not right. That function may
be called elsewhere, too, and we would potentially want to
know about those leaks.
So you can say "ignore the callstack when main calls
some_function". That works, but your annotations are
brittle. In this case it's only two functions, but you can
imagine that the actual allocation is much deeper. If any of
the intermediate code changes, you have to update the
suppression.
What we _really_ want to say is that "the value assigned to
p at the end of the function is not a real leak". But
leak-checkers can't understand that; they don't know about
"p" in the first place.
However, we can do something a little bit tricky if we make
some assumptions about how leak-checkers work. They
generally don't just report all un-freed blocks. That would
report even globals which are still accessible when the
leak-check is run. Instead they take some set of memory
(like BSS) as a root and mark it as "reachable". Then they
scan the reachable blocks for anything that looks like a
pointer to a malloc'd block, and consider that block
reachable. And then they scan those blocks, and so on,
transitively marking anything reachable from a global as
"not leaked" (or at least leaked in a different category).
So we can mark the value of "p" as reachable by putting it
into a variable with program lifetime. One way to do that is
to just mark "p" as static. But that actually affects the
run-time behavior if the function is called twice (you
aren't likely to call main() twice, but some of our cmd_*()
functions are called from other commands).
Instead, we can trick the leak-checker by putting the value
into _any_ reachable bytes. This patch keeps a global
linked-list of bytes copied from "unleaked" variables. That
list is reachable even at program exit, which confers
recursive reachability on whatever values we unleak.
In other words, you can do:
int cmd_foo(...)
{
char *p = some_function();
printf("%s", p);
UNLEAK(p);
return 0;
}
to annotate "p" and suppress the leak report.
But wait, couldn't we just say "free(p)"? In this toy
example, yes. But UNLEAK()'s byte-copying strategy has
several advantages over actually freeing the memory:
1. It's recursive across structures. In many cases our "p"
is not just a pointer, but a complex struct whose
fields may have been allocated by a sub-function. And
in some cases (e.g., dir_struct) we don't even have a
function which knows how to free all of the struct
members.
By marking the struct itself as reachable, that confers
reachability on any pointers it contains (including those
found in embedded structs, or reachable by walking
heap blocks recursively.
2. It works on cases where we're not sure if the value is
allocated or not. For example:
char *p = argc > 1 ? argv[1] : some_function();
It's safe to use UNLEAK(p) here, because it's not
freeing any memory. In the case that we're pointing to
argv here, the reachability checker will just ignore
our bytes.
3. Likewise, it works even if the variable has _already_
been freed. We're just copying the pointer bytes. If
the block has been freed, the leak-checker will skip
over those bytes as uninteresting.
4. Because it's not actually freeing memory, you can
UNLEAK() before we are finished accessing the variable.
This is helpful in cases like this:
char *p = some_function();
return another_function(p);
Writing this with free() requires:
int ret;
char *p = some_function();
ret = another_function(p);
free(p);
return ret;
But with unleak we can just write:
char *p = some_function();
UNLEAK(p);
return another_function(p);
This patch adds the UNLEAK() macro and enables it
automatically when Git is compiled with SANITIZE=leak. In
normal builds it's a noop, so we pay no runtime cost.
It also adds some UNLEAK() annotations to show off how the
feature works. On top of other recent leak fixes, these are
enough to get t0000 and t0001 to pass when compiled with
LSAN.
Note the case in commit.c which actually converts a
strbuf_release() into an UNLEAK. This code was already
non-leaky, but the free didn't do anything useful, since
we're exiting. Converting it to an annotation means that
non-leak-checking builds pay no runtime cost. The cost is
minimal enough that it's probably not worth going on a
crusade to convert these kinds of frees to UNLEAKS. I did it
here for consistency with the "sb" leak (though it would
have been equally correct to go the other way, and turn them
both into strbuf_release() calls).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A common pattern to free a piece of memory and assign NULL to the
pointer that used to point at it has been replaced with a new
FREE_AND_NULL() macro.
* ab/free-and-null:
*.[ch] refactoring: make use of the FREE_AND_NULL() macro
coccinelle: make use of the "expression" FREE_AND_NULL() rule
coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
coccinelle: make use of the "type" FREE_AND_NULL() rule
coccinelle: add a rule to make "type" code use FREE_AND_NULL()
git-compat-util: add a FREE_AND_NULL() wrapper around free(ptr); ptr = NULL
Fix configuration codepath to pay proper attention to commondir
that is used in multi-worktree situation, and isolate config API
into its own header file.
* bw/config-h:
config: don't implicitly use gitdir or commondir
config: respect commondir
setup: teach discover_git_directory to respect the commondir
config: don't include config.h by default
config: remove git_config_iter
config: create config.h
Replace occurrences of `free(ptr); ptr = NULL` which weren't caught by
the coccinelle rule. These fall into two categories:
- free/NULL assignments one after the other which coccinelle all put
on one line, which is functionally equivalent code, but very ugly.
- manually spotted occurrences where the NULL assignment isn't right
after the free() call.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Stop including config.h by default in cache.h. Instead only include
config.h in those files which require use of the config system.
Signed-off-by: Brandon Williams <bmwill@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some platforms have ulong that is smaller than time_t, and our
historical use of ulong for timestamp would mean they cannot
represent some timestamp that the platform allows. Invent a
separate and dedicated timestamp_t (so that we can distingiuish
timestamps and a vanilla ulongs, which along is already a good
move), and then declare uintmax_t is the type to be used as the
timestamp_t.
* js/larger-timestamps:
archive-tar: fix a sparse 'constant too large' warning
use uintmax_t for timestamps
date.c: abort if the system time cannot handle one of our timestamps
timestamp_t: a new data type for timestamps
PRItime: introduce a new "printf format" for timestamps
parse_timestamp(): specify explicitly where we parse timestamps
t0006 & t5000: skip "far in the future" test when time_t is too limited
t0006 & t5000: prepare for 64-bit timestamps
ref-filter: avoid using `unsigned long` for catch-all data type
The buffer allocated by shorten_unambiguous_ref() needs to be released.
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Git's source code assumes that unsigned long is at least as precise as
time_t. Which is incorrect, and causes a lot of problems, in particular
where unsigned long is only 32-bit (notably on Windows, even in 64-bit
versions).
So let's just use a more appropriate data type instead. In preparation
for this, we introduce the new `timestamp_t` data type.
By necessity, this is a very, very large patch, as it has to replace all
timestamps' data type in one go.
As we will use a data type that is not necessarily identical to `time_t`,
we need to be very careful to use `time_t` whenever we interact with the
system functions, and `timestamp_t` everywhere else.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Allow to lock a worktree immediately after it's created. This helps
prevent a race between "git worktree add; git worktree lock" and
"git worktree prune".
* nd/worktree-add-lock:
worktree add: add --lock option
