Elijah Newren [Tue, 16 May 2023 06:34:07 +0000 (06:34 +0000)]
hash-ll, hashmap: move oidhash() to hash-ll
oidhash() was used by both hashmap and khash, which makes sense.
However, the location of this function in hashmap.[ch] meant that
khash.h had to depend upon hashmap.h, making people unfamiliar with
khash think that it was built upon hashmap. (Or at least, I personally
was confused for a while about this in the past.)
Move this function to hash-ll, so that khash.h can stop depending upon
hashmap.h.
This has another benefit as well: it allows us to remove hashmap.h's
dependency on hash-ll.h. While some callers of hashmap.h were making
use of oidhash, most were not, so this change provides another way to
reduce the number of includes.
Diff best viewed with `--color-moved`.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:34:06 +0000 (06:34 +0000)]
object-store-ll.h: split this header out of object-store.h
The vast majority of files including object-store.h did not need dir.h
nor khash.h. Split the header into two files, and let most just depend
upon object-store-ll.h, while letting the two callers that need it
depend on the full object-store.h.
After this patch:
$ git grep -h include..object-store | sort | uniq -c
2 #include "object-store.h"
129 #include "object-store-ll.h"
Diff best viewed with `--color-moved`.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:34:05 +0000 (06:34 +0000)]
khash: name the structs that khash declares
khash.h lets you instantiate custom hash types that map between two
types. These are defined as a struct, as you might expect, and khash
typedef's that to kh_foo_t. But it declares the struct anonymously,
which doesn't give a name to the struct type itself; there is no
"struct kh_foo". This has two small downsides:
- when using khash, we declare "kh_foo_t *the_foo". This is
unlike our usual naming style, which is "struct kh_foo *the_foo".
- you can't forward-declare a typedef of an unnamed struct type in
C. So we might do something like this in a header file:
struct kh_foo;
struct bar {
struct kh_foo *the_foo;
};
to avoid having to include the header that defines the real
kh_foo. But that doesn't work with the typedef'd name. Without the
"struct" keyword, the compiler doesn't know we mean that kh_foo is
a type.
So let's always give khash structs the name that matches our
conventions ("struct kh_foo" to match "kh_foo_t"). We'll keep doing
the typedef to retain compatibility with existing callers.
Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:34:04 +0000 (06:34 +0000)]
merge-ll: rename from ll-merge
A long term (but rather minor) pet-peeve of mine was the name
ll-merge.[ch]. I thought it made it harder to realize what stuff was
related to merging when I was working on the merge machinery and trying
to improve it.
Further, back in d1cbe1e6d8a ("hash-ll.h: split out of hash.h to remove
dependency on repository.h", 2023-04-22), we have split the portions of
hash.h that do not depend upon repository.h into a "hash-ll.h" (due to
the recommendation to use "ll" for "low-level" in its name[1], but which
I used as a suffix precisely because of my distaste for "ll-merge").
When we discussed adding additional "*-ll.h" files, a request was made
that we use "ll" consistently as either a prefix or a suffix. Since it
is already in use as both a prefix and a suffix, the only way to do so
is to rename some files.
Besides my distaste for the ll-merge.[ch] name, let me also note that
the files
ll-fsmonitor.h, ll-hash.h, ll-merge.h, ll-object-store.h, ll-read-cache.h
would have essentially nothing to do with each other and make no sense
to group. But giving them the common "ll-" prefix would group them. Using
"-ll" as a suffix thus seems just much more logical to me. Rename
ll-merge.[ch] to merge-ll.[ch] to achieve this consistency, and to
ensure we get a more logical grouping of files.
Elijah Newren [Tue, 16 May 2023 06:34:03 +0000 (06:34 +0000)]
git-compat-util.h: remove unneccessary include of wildmatch.h
The include of wildmatch.h in git-compat-util.h was added in cebcab189aa
(Makefile: add USE_WILDMATCH to use wildmatch as fnmatch, 2013-01-01) as
a way to be able to compile-time force any calls to fnmatch() to instead
invoke wildmatch(). The defines and inline function were removed in 70a8fc999d9 (stop using fnmatch (either native or compat), 2014-02-15),
and this include in git-compat-util.h has been unnecessary ever since.
Remove the include from git-compat-util.h, but add it to the .c files
that had omitted the direct #include they needed.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:34:02 +0000 (06:34 +0000)]
builtin.h: remove unneccessary includes
This also made it clear that a few .c files under builtin/ were
depending upon some headers but had forgotten to #include them. Add the
missing direct includes while at it.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:34:00 +0000 (06:34 +0000)]
diff.h: remove unnecessary include of oidset.h
This also made it clear that several .c files depended upon various
things that oidset included, but had omitted the direct #include for
those headers. Add those now.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:57 +0000 (06:33 +0000)]
cache.h: remove this no-longer-used header
Since this header showed up in some places besides just #include
statements, update/clean-up/remove those other places as well.
Note that compat/fsmonitor/fsm-path-utils-darwin.c previously got
away with violating the rule that all files must start with an include
of git-compat-util.h (or a short-list of alternate headers that happen
to include it first). This change exposed the violation and caused it
to stop building correctly; fix it by having it include
git-compat-util.h first, as per policy.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:56 +0000 (06:33 +0000)]
read-cache*.h: move declarations for read-cache.c functions from cache.h
For the functions defined in read-cache.c, move their declarations from
cache.h to a new header, read-cache-ll.h. Also move some related inline
functions from cache.h to read-cache.h. The purpose of the
read-cache-ll.h/read-cache.h split is that about 70% of the sites don't
need the inline functions and the extra headers they include.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:55 +0000 (06:33 +0000)]
repository.h: move declaration of the_index from cache.h
the_index is a global variable defined in repository.c; as such, its
declaration feels better suited living in repository.h rather than
cache.h. Move it.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:52 +0000 (06:33 +0000)]
preload-index.h: move declarations for preload-index.c from elsewhere
We already have a preload-index.c file; move the declarations for the
functions in that file into a new preload-index.h. These were
previously split between cache.h and repository.h.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:48 +0000 (06:33 +0000)]
statinfo: move stat_{data,validity} functions from cache/read-cache
These functions do not depend upon struct cache_entry or struct
index_state in any way, and it seems more logical to break them out into
this file, especially since statinfo.h already has the struct stat_data
declaration.
Diff best viewed with `--color-moved`.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:47 +0000 (06:33 +0000)]
read-cache: move shared add/checkout/commit code
The function add_files_to_cache(), plus associated helper functions,
were defined in builtin/add.c, but also shared with builtin/checkout.c
and builtin/commit.c. Move these shared functions to read-cache.c.
Diff best viewed with `--color-moved`.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:46 +0000 (06:33 +0000)]
add: modify add_files_to_cache() to avoid globals
The function add_files_to_cache() is used by all three of builtin/{add,
checkout, commit}.c. That suggests this is common library code, and
should be moved somewhere else, like read-cache.c. However, the
function and its helpers made use of two global variables that made
straight code movement difficult:
* the_index
* include_sparse
The latter was perhaps more problematic since it was only accessible in
builtin/add.c but was still affecting builtin/checkout.c and
builtin/commit.c without this fact being very clear from the code. I'm
not sure if the other two callers would want to add a `--sparse` flag
similar to add.c to get non-default behavior, but exposing this
dependence will help if we ever decide we do want to add such a flag.
Modify add_files_to_cache() and its helpers to accept the necessary
arguments instead of relying on globals.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:45 +0000 (06:33 +0000)]
read-cache: move shared commit and ls-files code
The function overlay_tree_on_index(), plus associated helper functions,
were defined in builtin/ls-files.c, but also shared with
builtin/commit.c. Move these shared functions to read-cache.c.
Diff best viewed with `--color-moved`.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:43 +0000 (06:33 +0000)]
init-db, clone: change unnecessary global into passed parameter
Much like the parent commit, this commit was prompted by a desire to
move the functions which builtin/init-db.c and builtin/clone.c share out
of the former file and into setup.c. A secondary issue that made it
difficult was the init_shared_repository global variable; replace it
with a simple parameter that is passed to the relevant functions.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:42 +0000 (06:33 +0000)]
init-db: remove unnecessary global variable
This commit was prompted by a desire to move the functions which
builtin/init-db.c and builtin/clone.c share out of the former file and
into setup.c. One issue that made it difficult was the
init_is_bare_repository global variable.
init_is_bare_repository's sole use in life it to cache a value in
init_db(), and then be used in create_default_files(). This is a bit
odd since init_db() directly calls create_default_files(), and is the
only caller of that function. Convert the global to a simple function
parameter instead.
(Of course, this doesn't fix the fact that this value is then ignored by
create_default_files(), as noted in a big TODO comment in that function,
but it at least includes no behavioral change other than getting rid of
a very questionable global variable.)
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Tue, 16 May 2023 06:33:41 +0000 (06:33 +0000)]
init-db: document existing bug with core.bare in template config
The comments in create_default_files() talks about reading config from
the config file in the specified `--templates` directory, which leads to
the question of whether core.bare could be set in such a config file and
thus whether the code is doing the right thing. It turns out, that it
doesn't; it unconditionally ignores core.bare in the config file in any
--templates directory. It is not clear to me that fixing it can be done
within this function; it seems to occur too late:
* create_default_files() is called by init_db()
* init_db() is called by both builtin/{clone.c,init-db.c}
* both callers of init_db() call set_git_work_tree() before init_db()
and in order to actual affect whether a repository is bear, we'd need to
somewhere reset these values, not just the is_bare_repository_cfg
setting.
I do not want to open this can of worms at this time; I'm trying to
clean up some headers, for which I need to move some functions, for
which I need to clean up some globals, and that's far enough down the
rabbit hole. So, simply document the issue with a careful TODO comment
and a few testcases.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teng Long [Sat, 27 May 2023 07:57:53 +0000 (15:57 +0800)]
notes.c: introduce "--[no-]stripspace" option
This commit introduces a new option "--[no-]stripspace" to git notes
append, git notes edit, and git notes add. This option allows users to
control whether the note message need to stripped out.
For the consideration of backward compatibility, let's look at the
behavior about "stripspace" in "git notes" command:
1. "Edit Message" case: using the default editor to edit the note
message.
In "edit" case, the edited message will always be stripped out, the
implementation which can be found in the "prepare_note_data()". In
addition, the "-c" option supports to reuse an existing blob as a
note message, then open the editor to make a further edition on it,
the edited message will be stripped.
This commit doesn't change the default behavior of "edit" case by
using an enum "notes_stripspace", only when "--no-stripspace" option
is specified, the note message will not be stripped out. If you do
not specify the option or you specify "--stripspace", clearly, the
note message will be stripped out.
2. "Assign Message" case: using the "-m"/"-F"/"-C" option to specify the
note message.
In "assign" case, when specify message by "-m" or "-F", the message
will be stripped out by default, but when specify message by "-C",
the message will be copied verbatim, in other word, the message will
not be stripped out. One more thing need to note is "the order of
the options matter", that is, if you specify "-C" before "-m" or
"-F", the reused message by "-C" will be stripped out together,
because everytime concat "-m" or "-F" message, the concated message
will be stripped together. Oppositely, if you specify "-m" or "-F"
before "-C", the reused message by "-C" will not be stripped out.
This commit doesn't change the default behavior of "assign" case by
extending the "stripspace" field in "struct note_msg", so we can
distinguish the different behavior of "-m"/"-F" and "-C" options
when we need to parse and concat the message.
Signed-off-by: Teng Long <dyroneteng@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teng Long [Sat, 27 May 2023 07:57:52 +0000 (15:57 +0800)]
notes.c: append separator instead of insert by pos
Rename "insert_separator" to "append_separator" and also remove the
"postion" argument, this serves two purpose:
The first is that when specifying more than one "-m" ( like "-F", etc)
to "git notes add" or "git notes append", the order of them matters,
which means we need to append the each separator and message in turn,
so we don't have to make the caller specify the position, the "append"
operation is enough and clear.
The second is that when we execute the "git notes append" subcommand,
we need to combine the "prev_note" and "current_note" to get the
final result. Before, we inserted a newline character at the beginning
of "current_note". Now, we will append a newline to the end of
"prev_note" instead, this will give the consisitent results.
Signed-off-by: Teng Long <dyroneteng@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When adding new notes or appending to an existing notes, we will
insert a blank line between the paragraphs, like:
$ git notes add -m foo -m bar
$ git notes show HEAD
foo
bar
The default behavour sometimes is not enough, the user may want
to use a custom delimiter between paragraphs, like when
specifying '-m', '-F', '-C', '-c' options. So this commit
introduce a new '--separator' option for 'git notes add' and
'git notes append', for example when executing:
$ git notes add -m foo -m bar --separator="-"
$ git notes show HEAD
foo
-
bar
a newline is added to the value given to --separator if it
does not end with one already. So when executing:
$ git notes add -m foo -m bar --separator="-"
and
$ export LF="
"
$ git notes add -m foo -m bar --separator="-$LF"
Both the two exections produce the same result.
The reason we use a "strbuf" array to concat but not "string_list", is
that the binary file content may contain '\0' in the middle, this will
cause the corrupt result if using a string to save.
Signed-off-by: Teng Long <dyroneteng@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teng Long [Sat, 27 May 2023 07:57:48 +0000 (15:57 +0800)]
notes.c: cleanup 'strbuf_grow' call in 'append_edit'
Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This
"strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if
needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0,
"\n");" will do the "grow" for us.
348f199b (builtin-notes: Refactor handling of -F option to allow
combining -m and -F, 2010-02-13) added these to mimic the code
introduced by 2347fae5 (builtin-notes: Add "append" subcommand for
appending to note objects, 2010-02-13) that reads in previous note
before the message. And the resulting code with explicit sizing is
carried to this day.
In the context of reading an existing note in, exact sizing may have
made sense, but because the resulting note needs cleansing with
stripspace() when appending with this option, such an exact sizing
does not buy us all that much in practice.
It may help avoiding overallocation due to ALLOC_GROW() slop, but
nobody can feed so many long messages for it to matter from the
command line.
Signed-off-by: Teng Long <dyroneteng@gmail.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git [-c log.follow=true] log [--follow] ':(glob)f**'" used to barf.
* jk/log-follow-with-non-literal-pathspec:
diff: detect pathspec magic not supported by --follow
diff: factor out --follow pathspec check
pathspec: factor out magic-to-name function
The value of config.worktree is per-repository, but has been kept
in a singleton global variable per process. This has been OK as
most Git operations interacted with a single repository at a time,
but not right for operations like recursive "grep" that want to
access multiple repositories from a single process without forking.
The global variable has been eliminated and made into a member in
the per-repository data structure.
* vd/worktree-config-is-per-repository:
repository: move 'repository_format_worktree_config' to repo scope
config: pass 'repo' directly to 'config_with_options()'
config: use gitdir to get worktree config
Junio C Hamano [Tue, 20 Jun 2023 22:53:12 +0000 (15:53 -0700)]
Merge branch 'tb/submodule-null-deref-fix'
"git submodule" code trusted the data coming from the config (and
the in-tree .gitmodules file) too much without validating, leading
to NULL dereference if the user mucks with a repository (e.g.
submodule.<name>.url is removed). This has been corrected.
Junio C Hamano [Tue, 20 Jun 2023 22:53:12 +0000 (15:53 -0700)]
Merge branch 'jc/test-modernization-2'
Test style updates.
* jc/test-modernization-2:
t9400-git-cvsserver-server: modernize test format
t9200-git-cvsexportcommit: modernize test format
t9104-git-svn-follow-parent: modernize test format
t9100-git-svn-basic: modernize test format
t7700-repack: modernize test format
t7600-merge: modernize test format
t7508-status: modernize test format
t7201-co: modernize test format
t7111-reset-table: modernize test format
t7110-reset-merge: modernize test format
Junio C Hamano [Tue, 20 Jun 2023 22:53:12 +0000 (15:53 -0700)]
Merge branch 'jc/test-modernization'
* jc/test-modernization:
t7101-reset-empty-subdirs: modernize test format
t6050-replace: modernize test format
t5306-pack-nobase: modernize test format
t5303-pack-corruption-resilience: modernize test format
t5301-sliding-window: modernize test format
t5300-pack-object: modernize test format
t4206-log-follow-harder-copies: modernize test format
t4202-log: modernize test format
t4004-diff-rename-symlink: modernize test format
t4003-diff-rename-1: modernize test format
t4002-diff-basic: modernize test format
t3903-stash: modernize test format
t3700-add: modernize test format
t3500-cherry: modernize test format
t1006-cat-file: modernize test format
t1002-read-tree-m-u-2way: modernize test format
t1001-read-tree-m-2way: modernize test format
t3210-pack-refs: modernize test format
t0030-stripspace: modernize test format
t0000-basic: modernize test format
Document more pseudo-refs and teach the command line completion
machinery to complete AUTO_MERGE.
* pb/complete-and-document-auto-merge-and-friends:
completion: complete AUTO_MERGE
Documentation: document AUTO_MERGE
git-merge.txt: modernize word choice in "True merge" section
completion: complete REVERT_HEAD and BISECT_HEAD
revisions.txt: document more special refs
revisions.txt: use description list for special refs
Junio C Hamano [Tue, 20 Jun 2023 22:53:10 +0000 (15:53 -0700)]
Merge branch 'ps/fetch-cleanups'
Code clean-up.
* ps/fetch-cleanups:
fetch: use `fetch_config` to store "submodule.fetchJobs" value
fetch: use `fetch_config` to store "fetch.parallel" value
fetch: use `fetch_config` to store "fetch.recurseSubmodules" value
fetch: use `fetch_config` to store "fetch.showForcedUpdates" value
fetch: use `fetch_config` to store "fetch.pruneTags" value
fetch: use `fetch_config` to store "fetch.prune" value
fetch: pass through `fetch_config` directly
fetch: drop unneeded NULL-check for `remote_ref`
fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value
Derrick Stolee [Tue, 20 Jun 2023 19:01:15 +0000 (19:01 +0000)]
packfile: delete .idx files before .pack files
When installing a packfile, we place the .pack file before the .idx
file. The intention is that Git scans for .idx files in the pack
directory and then loads the .pack files from that list.
However, when we delete packfiles, we do not do this in the reverse
order as we should. The unlink_pack_path() method deletes the .pack
followed by the .idx.
This creates a window where the process could be interrupted between
the .pack deletion and the .idx deletion, leaving the repository in a
state that looks strange, but isn't actually too problematic if we
assume the pack was safe to delete. The .idx without a .pack will cause
some overhead, but will not interrupt other Git processes.
This ordering was introduced into the 'git repack' builtin by a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15), though
we must be careful to track history through the code move in 8434e85d5f9
(repack: refactor pack deletion for future use, 2019-06-10) to see that.
This became more important after 73320e49add (builtin/repack.c: only
collect fully-formed packs, 2023-06-07) changed how 'git repack' scanned
for packfiles for use in the cruft pack process. It previously looked
for .pack files, but that was problematic due to the order that packs
are installed: repacks between the creation of a .pack and the creation
of its .idx would result in hard failures.
There is an independent proposal about what to do in the case of a .idx
without a .pack during this 'git repack' scenario, but this change is
focused on deleting .pack files more safely.
Modify the order to delete the .idx before the .pack. The rest of the
modifiers on the .pack should still come after the .pack so we know all
of the presumed properties of the packfile as long as it exists in the
filesystem, in case we wish to reinstate it by re-indexing the .pack
file.
Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Sat, 17 Jun 2023 20:43:17 +0000 (22:43 +0200)]
replace strbuf_expand() with strbuf_expand_step()
Avoid the overhead of passing context to a callback function of
strbuf_expand() by using strbuf_expand_step() in a loop instead. It
requires explicit handling of %% and unrecognized placeholders, but is
simpler, more direct and avoids void pointers.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Sat, 17 Jun 2023 20:42:26 +0000 (22:42 +0200)]
replace strbuf_expand_dict_cb() with strbuf_expand_step()
Avoid the overhead of setting up a dictionary and passing it via
strbuf_expand() to strbuf_expand_dict_cb() by using strbuf_expand_step()
in a loop instead. It requires explicit handling of %% and unrecognized
placeholders, but is more direct and simpler overall, and expands only
on demand.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Sat, 17 Jun 2023 20:41:44 +0000 (22:41 +0200)]
strbuf: factor out strbuf_expand_step()
Extract the part of strbuf_expand that finds the next placeholder into a
new function. It allows to build parsers without callback functions and
the overhead imposed by them.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Sat, 17 Jun 2023 05:15:59 +0000 (01:15 -0400)]
http: handle both "h2" and "h2h3" in curl info lines
When redacting auth tokens in trace output from curl, we look for http/2
headers of the form "h2h3 [header: value]". This comes from b637a41ebe
(http: redact curl h2h3 headers in info, 2022-11-11).
But the "h2h3" prefix changed to just "h2" in curl's fc2f1e547 (http2:
support HTTP/2 to forward proxies, non-tunneling, 2023-04-14). That's in
released version curl 8.1.0; linking against that version means we'll
fail to correctly redact the trace. Our t5559.17 notices and fails.
We can fix this by matching either prefix, which should handle both old
and new versions.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Let's mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix
promptly any new leak that may be introduced and triggered by them in
the future.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sat, 17 Jun 2023 06:41:40 +0000 (08:41 +0200)]
config: fix a leak in git_config_copy_or_rename_section_in_file
A branch can have its configuration spread over several configuration
sections. This situation was already foreseen in 52d59cc645 (branch:
add a --copy (-c) option to go with --move (-m), 2017-06-18) when
"branch -c" was introduced.
Unfortunately, a leak was also introduced:
$ git branch foo
$ cat >> .git/config <<EOF
[branch "foo"]
some-key-a = a value
[branch "foo"]
some-key-b = b value
[branch "foo"]
some-key-c = c value
EOF
$ git branch -c foo bar
Direct leak of 130 byte(s) in 2 object(s) allocated from:
... in xrealloc wrapper.c
... in strbuf_grow strbuf.c
... in strbuf_vaddf strbuf.c
... in strbuf_addf strbuf.c
... in store_create_section config.c
... in git_config_copy_or_rename_section_in_file config.c
... in git_config_copy_section_in_file config.c
... in git_config_copy_section config.c
... in copy_or_rename_branch builtin/branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Let's fix it.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sat, 17 Jun 2023 06:41:22 +0000 (08:41 +0200)]
branch: fix a leak in cmd_branch
In 98e7ab6d42 (for-each-ref: delay parsing of --sort=<atom> options,
2021-10-20) a new string_list was introduced to accumulate any
"branch.sort" setting.
That string_list is cleared in ref_sorting_options(), which is only
called when processing the "--list" sub-command. Therefore, with other
sub-command, while having any sort option set, a leak is produced, e.g.:
Direct leak of 384 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in string_list_append_nodup string-list.c
... in string_list_append string-list.c
... in git_branch_config builtin/branch.c
... in configset_iter config.c
... in repo_config config.c
... in git_config config.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Indirect leak of 20 byte(s) in 1 object(s) allocated from:
... in xstrdup wrapper.c
... in string_list_append string-list.c
... in git_branch_config builtin/branch.c
... in configset_iter config.c
... in repo_config config.c
... in git_config config.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
We don't have a common clean-up section in cmd_branch(). To avoid
refactoring, keep the fix simple, and while we find a better solution
which hopefuly will avoid entirely that string_list, when no sort
options are needed; let's squelch the leak sanitizer using UNLEAK().
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sat, 17 Jun 2023 06:41:08 +0000 (08:41 +0200)]
branch: fix a leak in setup_tracking
In bdaf1dfae7 (branch: new autosetupmerge option "simple" for matching
branches, 2022-04-29) a new exit for setup_tracking() missed the
clean-up, producing a leak.
$ git config branch.autoSetupMerge simple
$ git remote add local .
$ git update-ref refs/remotes/local/foo HEAD
$ git branch bar local/foo
Direct leak of 384 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in string_list_append_nodup string-list.c
... in find_tracked_branch branch.c
... in for_each_remote remote.c
... in setup_tracking branch.c
... in create_branch branch.c
... in cmd_branch builtinbranch.c
... in run_builtin git.c
Indirect leak of 24 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in strbuf_grow strbuf.c
... in strbuf_add strbuf.c
... in match_name_with_pattern remote.c
... in query_refspecs remote.c
... in remote_find_tracking remote.c
... in find_tracked_branch branch.c
... in for_each_remote remote.c
... in setup_tracking branch.c
... in create_branch branch.c
... in cmd_branch builtinbranch.c
... in run_builtin git.c
The return introduced in bdaf1dfae7 was to avoid setting up the
tracking, but even in that case it is still necessary to do the
clean-up. Let's do it.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sat, 17 Jun 2023 06:40:43 +0000 (08:40 +0200)]
rev-parse: fix a leak with --abbrev-ref
To handle "--abbrev-ref" we use shorten_unambiguous_ref(). This
function takes a refname and returns a shortened refname, which is a
newly allocated string that needs to be freed.
Unfortunately, the refname variable is reused to receive the shortened
one. Therefore, we lose the original refname, which needs to be freed
as well, producing a leak.
This leak can be reviewed with:
$ for a in {1..10}; do git branch foo_${a}; done
$ git rev-parse --abbrev-ref refs/heads/foo_{1..10}
Direct leak of 171 byte(s) in 10 object(s) allocated from:
... in xstrdup wrapper.c
... in expand_ref refs.c
... in repo_dwim_ref refs.c
... in show_rev builtin/rev-parse.c
... in cmd_rev_parse builtin/rev-parse.c
... in run_builtin git.c
We have this leak since a45d34691e (rev-parse: --abbrev-ref option to
shorten ref name, 2009-04-13) when "--abbrev-ref" was introduced.
Let's fix it.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Sat, 17 Jun 2023 04:26:24 +0000 (00:26 -0400)]
commit: pass --no-divider to interpret-trailers
When git-commit sees any "--trailer" options, it passes the
COMMIT_EDITMSG file through git-interpret-trailers. But it does so
without passing --no-divider, which means that interpret-trailers will
look for a "---" divider to signal the end of the commit message.
That behavior doesn't make any sense in this context; we know we have a
complete and solitary commit message, not something we have to further
parse. And as a result, we'll do the wrong thing if the commit message
contains a "---" marker (which otherwise is not syntactically
significant), inserting any new trailers at the wrong spot.
We can fix this by passing --no-divider. This is the exact situation for
which it was added in 1688c9a489 (interpret-trailers: allow suppressing
"---" divider, 2018-08-22). As noted in the message for that commit, it
just adds the mechanism, and further patches were needed to trigger it
from various callers. We did that back then in a few spots, like ffce7f590f (sequencer: ignore "---" divider when parsing trailers,
2018-08-22), but obviously missed this one.
Reported-by: <eric.frederich@siemens.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
M Hickford [Fri, 16 Jun 2023 19:55:06 +0000 (19:55 +0000)]
credential/libsecret: store new attributes
d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
and a5c76569e7 (credential: new attribute oauth_refresh_token)
introduced new credential attributes.
libsecret assumes attribute values are non-confidential and
unchanging, so we encode the new attributes in the secret, separated by
newline:
This is extensible and backwards compatible. The credential protocol
already assumes that attribute values do not contain newlines.
Alternatives considered: store password_expiry_utc in a libsecret
attribute. This has the problem that libsecret creates new items
rather than overwrites when attribute values change.
Signed-off-by: M Hickford <mirth.hickford@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Wed, 14 Jun 2023 19:35:58 +0000 (19:35 +0000)]
setup.c: don't setup in discover_git_directory()
discover_git_directory() started modifying the_repository in ebaf3bcf1ae
(repository: move global r_f_p_c to repo struct, 2021-06-17), when, in
the repository setup process, we started copying members from the
"struct repository_format" we're inspecting to the appropriate "struct
repository". However, discover_git_directory() isn't actually used in
the setup process (its only caller in the Git binary is
read_early_config()), so it shouldn't be doing this setup at all!
As explained by 16ac8b8db6 (setup: introduce the
discover_git_directory() function, 2017-03-13) and the comment on its
declaration, discover_git_directory() is intended to be an entrypoint
into setup.c machinery that allows the Git directory to be discovered
without side effects, e.g. so that read_early_config() can read
".git/config" before the_repository has been set up.
Fortunately, we didn't start to rely on this unintended behavior between
then and now, so we let's just remove it. It isn't harming anyone, but
it's confusing.
Signed-off-by: Glen Choo <chooglen@google.com> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
M Hickford [Thu, 15 Jun 2023 19:19:33 +0000 (19:19 +0000)]
credential: erase all matching credentials
`credential reject` sends the erase action to each helper, but the
exact behaviour of erase isn't specified in documentation or tests.
Some helpers (such as credential-store and credential-libsecret) delete
all matching credentials, others (such as credential-cache) delete at
most one matching credential.
Test that helpers erase all matching credentials. This behaviour is
easiest to reason about. Users expect that `echo
"url=https://example.com" | git credential reject` or `echo
"url=https://example.com\nusername=tim" | git credential reject` erase
all matching credentials.
Fix credential-cache.
Signed-off-by: M Hickford <mirth.hickford@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
While both git-rev-list(1) and git-log(1) support `--stdin`, it only
accepts commits and files. Most notably, it is impossible to pass any of
the pseudo-opts like `--all`, `--glob=` or others via stdin.
This makes it hard to use this function in certain scripted scenarios,
like when one wants to support queries against specific revisions, but
also against reference patterns. While this is theoretically possible by
using arguments, this may run into issues once we hit platform limits
with sufficiently large queries. And because `--stdin` cannot handle
pseudo-opts, the only alternative would be to use a mixture of arguments
and standard input, which is cumbersome.
Implement support for handling pseudo-opts in both commands to support
this usecase better. One notable restriction here is that `--stdin` only
supports "stuck" arguments in the form of `--glob=foo`. This is because
"unstuck" arguments would also require us to read the next line, which
would add quite some complexity to the code. This restriction should be
fine for scripted usage though.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
revision: small readability improvement for reading from stdin
The code that reads lines from standard input manually compares whether
the read line matches "--", which is a bit awkward to read. Furthermore,
we're about to extend the code to also support reading pseudo-options
via standard input, requiring more elaborate handling of lines with a
leading dash.
Refactor the code by hoisting out the check for "--" outside of the
block that checks for a leading dash.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Thu, 15 Jun 2023 17:33:39 +0000 (19:33 +0200)]
ls-tree: fix documentation of %x format placeholder
ls-tree --format expands %x followed by two hexadecimal digits to the
character indicated by that hexadecimal number, e.g.:
$ git ls-tree --format=%x41 HEAD | head -1
A
It rejects % followed by a hexadecimal digit, e.g.:
$ git ls-tree --format=%41 HEAD | head -1
fatal: bad ls-tree format: element '41' does not start with '('
This functionality is provided by strbuf_expand_literal_cb(), which has
not been changed since it was factored out by fd2015b323 (strbuf:
separate callback for strbuf_expand:ing literals, 2019-01-28).
Adjust the documentation accordingly.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Linus Arver [Thu, 15 Jun 2023 02:53:50 +0000 (02:53 +0000)]
doc: trailer: add more examples in DESCRIPTION
Be more up-front about what trailers are in practice with examples, to
give the reader a visual cue while they go on to read the rest of the
description.
Also add an example for multiline values.
Signed-off-by: Linus Arver <linusa@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Linus Arver [Thu, 15 Jun 2023 02:53:45 +0000 (02:53 +0000)]
doc: trailer: examples: avoid the word "message" by itself
Previously, "message" could mean the input, output, commit message, or
"internal body text inside the commit message" (in the EXAMPLES
section). Avoid overloading this term by using the appropriate meanings
explicitly.
Signed-off-by: Linus Arver <linusa@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Linus Arver [Thu, 15 Jun 2023 02:53:44 +0000 (02:53 +0000)]
doc: trailer: drop "commit message part" phrasing
The command can take inputs that are either just a commit message, or
an email-like output such as git-format-patch which includes a commit
message, "---" divider, and patch part. The existing explanation blends
these two inputs together in the first sentence
This command reads some patches or commit messages
which then necessitates using the "commit message part" phrasing (as
opposed to just "commit message") because the input is ambiguous per the
above definition.
This change separates the two input types and explains them separately,
and so there is no longer a need to use the "commit message part"
phrase.
Signed-off-by: Linus Arver <linusa@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan [Wed, 14 Jun 2023 21:31:45 +0000 (14:31 -0700)]
CodingGuidelines: use octal escapes, not hex
Extend the shell-scripting section of CodingGuidelines to suggest octal
escape sequences (e.g. "\302\242") over hexadecimal (e.g. "\xc2\xa2")
since the latter can be a source of portability problems.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Josip Sokcevic [Wed, 14 Jun 2023 16:48:57 +0000 (09:48 -0700)]
diff-lib: honor override_submodule_config flag bit
When `diff.ignoreSubmodules = all` is set and submodule commits are
manually staged (e.g. via `git-update-index`), `git-commit` should
record the commit with updated submodules.
`index_differs_from` is called from `prepare_to_commit` with flags set to
`override_submodule_config = 1`. `index_differs_from` then merges the
default diff flags and passed flags.
When `diff.ignoreSubmodules` is set to "all", `flags` ends up having
both `override_submodule_config` and `ignore_submodules` set to 1. This
results in `git-commit` ignoring staged commits.
This patch restores original `flags.ignore_submodule` if
`flags.override_submodule_config` is set.
Signed-off-by: Josip Sokcevic <sokcevic@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Tue, 13 Jun 2023 19:29:46 +0000 (12:29 -0700)]
Merge branch 'zh/ls-files-format-atoms'
Some atoms that can be used in "--format=<format>" for "git ls-tree"
were not supported by "git ls-files", even though they were relevant
in the context of the latter.
* zh/ls-files-format-atoms:
ls-files: align format atoms with ls-tree
Junio C Hamano [Tue, 13 Jun 2023 19:29:45 +0000 (12:29 -0700)]
Merge branch 'jc/pack-ref-exclude-include'
"git pack-refs" learns "--include" and "--exclude" to tweak the ref
hierarchy to be packed using pattern matching.
* jc/pack-ref-exclude-include:
pack-refs: teach pack-refs --include option
pack-refs: teach --exclude option to exclude refs from being packed
docs: clarify git-pack-refs --all will pack all refs
Junio C Hamano [Tue, 13 Jun 2023 19:29:45 +0000 (12:29 -0700)]
Merge branch 'sa/doc-ls-remote'
Doc update.
* sa/doc-ls-remote:
ls-remote doc: document the output format
ls-remote doc: explain what each example does
ls-remote doc: show peeled tags in examples
ls-remote doc: remove redundant --tags example
show-branch doc: say <ref>, not <reference>
show-ref doc: update for internal consistency
Junio C Hamano [Tue, 13 Jun 2023 19:29:44 +0000 (12:29 -0700)]
Merge branch 'jc/diff-s-with-other-options'
The "-s" (silent, squelch) option of the "diff" family of commands
did not interact with other options that specify the output format
well. This has been cleaned up so that it will clear all the
formatting options given before.
* jc/diff-s-with-other-options:
diff: fix interaction between the "-s" option and other options
Junio C Hamano [Tue, 13 Jun 2023 19:29:44 +0000 (12:29 -0700)]
Merge branch 'kh/keep-tag-editmsg-upon-failure'
"git tag" learned to leave the "$GIT_DIR/TAG_EDITMSG" file when the
command failed, so that the user can salvage what they typed.
* kh/keep-tag-editmsg-upon-failure:
tag: keep the message file in case ref transaction fails
t/t7004-tag: add regression test for successful tag creation
doc: tag: document `TAG_EDITMSG`
Rubén Justo [Sun, 11 Jun 2023 18:50:36 +0000 (20:50 +0200)]
branch: fix a leak in setup_tracking
The commit d3115660b4 (branch: add flags and config to inherit tracking,
2021-12-20) replaced in "struct tracking", the member "char *src" by a
new "struct string_list *srcs".
This caused a modification in find_tracked_branch(). The string
returned by remote_find_tracking(), previously assigned to "src", is now
added to the string_list "srcs".
That string_list is initialized with STRING_LIST_INIT_DUP, which means
that what is added is not the given string, but a duplicate. Therefore,
the string returned by remote_find_tracking() is leaked.
The leak can be reviewed with:
$ git branch foo
$ git remote add local .
$ git fetch local
$ git branch --track bar local/foo
Direct leak of 24 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in strbuf_grow strbuf.c
... in strbuf_add strbuf.c
... in match_name_with_pattern remote.c
... in query_refspecs remote.c
... in remote_find_tracking remote.c
... in find_tracked_branch branch.c
... in for_each_remote remote.c
... in setup_tracking branch.c
... in create_branch branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Let's fix the leak, using the "_nodup" API to add to the string_list.
This way, the string itself will be added instead of being strdup()'d.
And when the string_list is cleared, the string will be freed.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sun, 11 Jun 2023 18:50:27 +0000 (20:50 +0200)]
branch: fix a leak in check_tracking_branch
Let's fix a leak we have in check_tracking_branch() since the function
was introduced in 41c21f22d0 (branch.c: Validate tracking branches with
refspecs instead of refs/remotes/*, 2013-04-21).
The leak can be reviewed with:
$ git remote add local .
$ git update-ref refs/remotes/local/foo HEAD
$ git branch --track bar local/foo
Direct leak of 24 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in strbuf_grow strbuf.c
... in strbuf_add strbuf.c
... in match_name_with_pattern remote.c
... in query_refspecs remote.c
... in remote_find_tracking remote.c
... in check_tracking_branch branch.c
... in for_each_remote remote.c
... in validate_remote_tracking_branch branch.c
... in dwim_branch_start branch.c
... in create_branch branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sun, 11 Jun 2023 18:50:16 +0000 (20:50 +0200)]
branch: fix a leak in inherit_tracking
In d3115660b4 (branch: add flags and config to inherit tracking,
2021-12-20) a new option was introduced to allow creating a new branch,
inheriting the tracking of another branch.
The new code, strdup()'d the remote_name of the existing branch, but
unfortunately it was not freed, producing a leak.
$ git remote add local .
$ git update-ref refs/remotes/local/foo HEAD
$ git branch --track bar local/foo
branch 'bar' set up to track 'local/foo'.
$ git branch --track=inherit baz bar
branch 'baz' set up to track 'local/foo'.
Direct leak of 6 byte(s) in 1 object(s) allocated from:
... in xstrdup wrapper.c
... in inherit_tracking branch.c
... in setup_tracking branch.c
... in create_branch branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Actually, the string we're strdup()'ing is from the struct branch
returned by get_branch(). Which, in turn, retrieves the string from the
global "struct repository". This makes perfectly valid to use the
string throughout the entire execution of create_branch(). There is no
need to duplicate it.
Let's fix the leak, removing the strdup().
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sun, 11 Jun 2023 18:49:43 +0000 (20:49 +0200)]
branch: fix a leak in dwim_and_setup_tracking
In e89f151db1 (branch: move --set-upstream-to behavior to
dwim_and_setup_tracking(), 2022-01-28) the string returned by
dwim_branch_start() was not freed, resulting in a memory leak.
It can be reviewed with:
$ git remote add local .
$ git update-ref refs/remotes/local/foo HEAD
$ git branch --set-upstream-to local/foo foo
Direct leak of 23 byte(s) in 1 object(s) allocated from:
... in xstrdup wrapper.c
... in expand_ref refs.c
... in repo_dwim_ref refs.c
... in dwim_branch_start branch.c
... in dwim_and_setup_tracking branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Let's free it now.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rubén Justo [Sun, 11 Jun 2023 18:49:35 +0000 (20:49 +0200)]
remote: fix a leak in query_matches_negative_refspec
In c0192df630 (refspec: add support for negative refspecs, 2020-09-30)
query_matches_negative_refspec() was introduced.
The function was implemented as a two-loop process, where the former
loop accumulates and the latter evaluates. To accumulate, a string_list
is used.
Within the first loop, there are three cases where a string is added to
the string_list. Two of them add strings that do not need to be
freed. But in the third case, the string added is returned by
match_name_with_pattern(), which needs to be freed.
The string_list is initialized with STRING_LIST_INIT_NODUP, i.e. when
cleared, the strings added are not freed. Therefore, the string
returned by match_name_with_pattern() is not freed, so we have a leak.
$ git remote add local .
$ git update-ref refs/remotes/local/foo HEAD
$ git branch --track bar local/foo
Direct leak of 24 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in strbuf_grow strbuf.c
... in strbuf_add strbuf.c
... in match_name_with_pattern remote.c
... in query_matches_negative_refspec remote.c
... in query_refspecs remote.c
... in remote_find_tracking remote.c
... in find_tracked_branch branch.c
... in for_each_remote remote.c
... in setup_tracking branch.c
... in create_branch branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Direct leak of 24 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in strbuf_grow strbuf.c
... in strbuf_add strbuf.c
... in match_name_with_pattern remote.c
... in query_matches_negative_refspec remote.c
... in query_refspecs remote.c
... in remote_find_tracking remote.c
... in check_tracking_branch branch.c
... in for_each_remote remote.c
... in validate_remote_tracking_branch branch.c
... in dwim_branch_start branch.c
... in create_branch branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
An interesting point to note is that while string_list_append() is used
in the first two cases described, string_list_append_nodup() is used in
the third. This seems to indicate an intention to delegate the
responsibility for freeing the string, to the string_list. As if the
string_list had been initialized with STRING_LIST_INIT_DUP, i.e. the
strings are strdup()'d when added (except if the "_nodup" API is used)
and freed when cleared.
Switching to STRING_LIST_INIT_DUP fixes the leak and probably is what we
wanted to do originally. Let's do it.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Direct leak of 65 byte(s) in 1 object(s) allocated from:
... in xrealloc wrapper.c
... in strbuf_grow strbuf.c
... in strbuf_vaddf strbuf.c
... in strbuf_addf strbuf.c
... in store_create_section config.c
... in git_config_copy_or_rename_section_in_file config.c
... in git_config_copy_section_in_file config.c
... in git_config_copy_section config.c
... in copy_or_rename_branch builtin/branch.c
... in cmd_branch builtin/branch.c
... in run_builtin git.c
Let's fix that leak.
Signed-off-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Wed, 7 Jun 2023 23:01:00 +0000 (19:01 -0400)]
pack-bitmap.c: gracefully degrade on failure to load MIDX'd pack
When opening a MIDX bitmap, we the pack-bitmap machinery eagerly calls
`prepare_midx_pack()` on each of the packs contained in the MIDX. This
is done in order to populate the array of `struct packed_git *`s held by
the MIDX, which we need later on in `load_reverse_index()`, since it
calls `load_pack_revindex()` on each of the MIDX'd packs, and requires
that the caller provide a pointer to a `struct packed_git`.
When opening one of these packs fails, the pack-bitmap code will `die()`
indicating that it can't open one of the packs in the MIDX. This
indicates that the MIDX is somehow broken with respect to the current
state of the repository. When this is the case, we indeed cannot make
use of the MIDX bitmap to speed up reachability traversals.
However, it does not mean that we can't perform reachability traversals
at all. In other failure modes, that same function calls `warning()` and
then returns -1, indicating to its caller (`open_bitmap()`) that we
should either look for a pack bitmap if one is available, or perform
normal object traversal without using bitmaps at all.
There is no reason why this case should cause us to die. If we instead
continued (by jumping to `cleanup` as this patch does) and avoid using
bitmaps altogether, we may again try and query the MIDX, which will also
fail. But when trying to call `fill_midx_entry()` fails, it also returns
a signal of its failure, and prompts the caller to try and locate the
object elsewhere.
In other words, the normal object traversal machinery works fine in the
presence of a corrupt MIDX, so there is no reason that the MIDX bitmap
machinery should abort in that case when we could easily continue.
Note that we *could* in theory try again to load a MIDX bitmap after
calling `reprepare_packed_git()`. Even though the `prepare_packed_git()`
code is careful to avoid adding a pack that we already have,
`prepare_midx_pack()` is not. So if we got part of the way through
calling `prepare_midx_pack()` on a stale MIDX, and then tried again on a
fresh MIDX that contains some of the same packs, we would end up with a
loop through the `->next` pointer.
For now, let's do the simplest thing possible and fallback to the
non-bitmap code when we detect a stale MIDX so that the complete fix as
above can be implemented carefully.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Wed, 7 Jun 2023 22:58:17 +0000 (18:58 -0400)]
gc: introduce `gc.recentObjectsHook`
This patch introduces a new multi-valued configuration option,
`gc.recentObjectsHook` as a means to mark certain objects as recent (and
thus exempt from garbage collection), regardless of their age.
When performing a garbage collection operation on a repository with
unreachable objects, Git makes its decision on what to do with those
object(s) based on how recent the objects are or not. Generally speaking,
unreachable-but-recent objects stay in the repository, and older objects
are discarded.
However, we have no convenient way to keep certain precious, unreachable
objects around in the repository, even if they have aged out and would
be pruned. Our options today consist of:
- Point references at the reachability tips of any objects you
consider precious, which may be undesirable or infeasible if there
are many such objects.
- Track them via the reflog, which may be undesirable since the
reflog's lifetime is limited to that of the reference it's tracking
(and callers may want to keep those unreachable objects around for
longer).
- Extend the grace period, which may keep around other objects that
the caller *does* want to discard.
- Manually modify the mtimes of objects you want to keep. If those
objects are already loose, this is easy enough to do (you can just
enumerate and `touch -m` each one).
But if they are packed, you will either end up modifying the mtimes
of *all* objects in that pack, or be forced to write out a loose
copy of that object, both of which may be undesirable. Even worse,
if they are in a cruft pack, that requires modifying its `*.mtimes`
file by hand, since there is no exposed plumbing for this.
- Force the caller to construct the pack of objects they want
to keep themselves, and then mark the pack as kept by adding a
".keep" file. This works, but is burdensome for the caller, and
having extra packs is awkward as you roll forward your cruft pack.
This patch introduces a new option to the above list via the
`gc.recentObjectsHook` configuration, which allows the caller to
specify a program (or set of programs) whose output is treated as a set
of objects to treat as recent, regardless of their true age.
The implementation is straightforward. Git enumerates recent objects via
`add_unseen_recent_objects_to_traversal()`, which enumerates loose and
packed objects, and eventually calls add_recent_object() on any objects
for which `want_recent_object()`'s conditions are met.
This patch modifies the recency condition from simply "is the mtime of
this object more recent than the cutoff?" to "[...] or, is this object
mentioned by at least one `gc.recentObjectsHook`?".
Depending on whether or not we are generating a cruft pack, this allows
the caller to do one of two things:
- If generating a cruft pack, the caller is able to retain additional
objects via the cruft pack, even if they would have otherwise been
pruned due to their age.
- If not generating a cruft pack, the caller is likewise able to
retain additional objects as loose.
A potential alternative here is to introduce a new mode to alter the
contents of the reachable pack instead of the cruft one. One could
imagine a new option to `pack-objects`, say `--extra-reachable-tips`
that does the same thing as above, adding the visited set of objects
along the traversal to the pack.
But this has the unfortunate side-effect of altering the reachability
closure of that pack. If parts of the unreachable object graph mentioned
by one or more of the "extra reachable tips" programs is not closed,
then the resulting pack won't be either. This makes it impossible in the
general case to write out reachability bitmaps for that pack, since
closure is a requirement there.
Instead, keep these unreachable objects in the cruft pack (or set of
unreachable, loose objects) instead, to ensure that we can continue to
have a pack containing just reachable objects, which is always safe to
write a bitmap over.
Helped-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, 7 Jun 2023 22:58:12 +0000 (18:58 -0400)]
reachable.c: extract `obj_is_recent()`
When enumerating objects in order to add recent ones (i.e. those whose
mtime is strictly newer than the cutoff) as tips of a reachability
traversal, `add_recent_object()` discards objects which do not meet the
recency criteria.
The subsequent commit will make checking whether or not an object is
recent also consult the list of hooks in `pack.recentHook`. Isolate this
check in its own function to keep the additional complexity outside of
`add_recent_object()`.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Wed, 7 Jun 2023 10:16:17 +0000 (06:16 -0400)]
builtin/repack.c: only collect fully-formed packs
To partition the set of packs based on which ones are "kept" (either
they have a .keep file, or were otherwise marked via the `--keep-pack`
option) and "non-kept" ones (anything else), `git repack` uses its
`collect_pack_filenames()` function.
Ordinarily, we would rely on a convenience function such as
`get_all_packs()` to enumerate and partition the set of packs. But
`collect_pack_filenames()` uses `readdir()` directly to read the
contents of the "$GIT_DIR/objects/pack" directory, and adds each entry
ending in ".pack" to the appropriate list (either kept, or non-kept as
above).
This is subtly racy, since `collect_pack_filenames()` may see a pack
that is not fully staged (i.e., it is missing its ".idx" file).
Ordinarily, this doesn't cause a problem. But it can cause issues when
generating a cruft pack.
This is because `git repack` feeds (among other things) the list of
existing kept packs down to `git pack-objects --cruft` to indicate that
any kept packs will not be removed from the repository (so that the
cruft pack machinery can avoid packing objects that appear in those
packs as cruft).
But `read_cruft_objects()` lists packfiles by calling `get_all_packs()`.
So if a ".pack" file exists (necessary to get that pack to appear to
`collect_pack_filenames()`), but doesn't have a corresponding ".idx"
file (necessary to get that pack to appear via `get_all_packs()`), we'll
complain with:
Fix the above by teaching `collect_pack_filenames()` to only collect
packs with their corresponding `*.idx` files in place, indicating that
those packs have been fully staged.
There are a couple of things worth noting:
- Since each entry in the `extra_keep` list (which contains the
`--keep-pack` names) has a `*.pack` suffix, we'll have to swap the
suffix from ".pack" to ".idx", and compare that instead.
- Since we use the the `fname_kept_list` to figure out which packs to
delete (with `git repack -d`), we would have previously deleted a
`*.pack` with no index (since the existince of a ".pack" file is
necessary and sufficient to include that pack in the list of
existing non-kept packs).
Now we will leave it alone (since that pack won't appear in the
list). This is far more correct behavior, since we don't want
to race with a pack being staged. Deleting a partially staged pack
is unlikely, however, since the window of time between staging a
pack and moving its .idx file into place is miniscule.
Note that this window does *not* include the time it takes to
receive and index the pack, since the incoming data goes into
"$GIT_DIR/objects/tmp_pack_XXXXXX", which does not end in ".pack"
and is thus ignored by collect_pack_filenames().
In the future, this function should probably be rewritten as a callback
to `for_each_file_in_pack_dir()`, but this is the simplest change we
could do in the short-term.
Reported-by: Michael Haggerty <mhagger@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
