* Re: [PATCH v2 4/6] trailer: teach find_patch_start about --no-divider
From: Junio C Hamano @ 2023-09-11 17:25 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <f5f507c4c6c4514af7dca35e307ca68e72435afb.1694240177.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Currently, find_patch_start only finds the start of the patch part of
> the input (by looking at the "---" divider) for cases where the
> "--no-divider" flag has not been provided. If the user provides this
> flag, we do not rely on find_patch_start at all and just call strlen()
> directly on the input.
>
> Instead, make find_patch_start aware of "--no-divider" and make it
> handle that case as well. This means we no longer need to call strlen at
> all and can just rely on the existing code in find_patch_start. By
> forcing callers to consider this important option, we avoid the kind of
> mistake described in be3d654343 (commit: pass --no-divider to
> interpret-trailers, 2023-06-17).
OK. The code pays attention to "---" so making it stop doing so
when the "--no-*" option is given will make the function responsible
for finding the beginning of the patch.
I wonder if we should rename this function while we are at it,
though. When "--no-divider" is given, the expected use case is
*not* to have a patch at all, and it is dubious that a function
whose name is find_patch_start() can possibly do anything useful.
The real purpose of this function is to find the end of the log
message, isn't it? And the caller uses the end of the log message
it found and gives it to find_trailer_start() and find_trailer_end()
as the upper boundary of the search for the trailer block.
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: René Scharfe @ 2023-09-11 20:12 UTC (permalink / raw)
To: Taylor Blau; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <ZP4NrVeqMtFTLEuf@nand.local>
Am 10.09.23 um 20:40 schrieb Taylor Blau:
> On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:
>> Add an int pointer, value_int, to struct option to provide a typed value
>> pointer for the various integer options. It allows type checks at
>> compile time, which is not possible with the void pointer, value. Its
>> use is optional for now.
>
> This is an interesting direction. I wonder about whether or not you'd
> consider changing the option structure to contain a tagged union type
> that represents some common cases we'd want from a parse-options
> callback, something like:
>
> struct option {
> /* ... */
> union {
> void *value;
> int *value_int;
> /* etc ... */
> } u;
> enum option_type t;
> };
>
> where option_type has some value corresponding to "void *", another for
> "int *", and so on.
In a hand-made struct option this would only provide a very limited form
of type safety. It reduces the number of incorrect types to choose from
from basically infinity to a handful, but still allows pointing the
union e.g. to an int for an option that takes a long or a string without
any compiler warning or error.
Convenience macros like OPT_CMDMODE could use the union to provide a
type safe interface, though, true. This might suffice for our purposes.
> Alternatively, perhaps you are thinking that we'd use both the value
> pointer and the value_int pointer to point at potentially different
> values in the same callback. I don't have strong feelings about it, but
> I'd just as soon encourage us to shy away from that approach, since
> assigning a single callback parameter to each function seems more
> organized.
Right, we only need one active value pointer per option.
>> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>> const char *s, *arg;
>> const int unset = flags & OPT_UNSET;
>> int err;
>> + int *value_int = opt->value_int ? opt->value_int : opt->value;
>>
>> if (unset && p->opt)
>> return error(_("%s takes no value"), optname(opt, flags));
>
> Reading this hunk, I wonder whether we even need a type tag (the
> option_type enum above) if each callback knows a priori what type it
> expects. But I think storing them together in a union makes sense to do.
Yes, option types (OPTION_INTEGER etc.) already imply a pointer type,
no additional tag needed.
René
^ permalink raw reply
* [PATCH v2 02/32] doc hash-function-transition: Replace compatObjectFormat with mapObjectFormat
From: Eric W. Biederman @ 2023-09-11 16:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, git
In-Reply-To: <xmqqy1hdi6hp.fsf@gitster.g>
Deeply and fundamentally the plan is to only operate one one hash
function for the core of git, to use only one hash function for what
is stored in the repository.
To avoid requring a flag day to transition hash functions for naming
objects, and to support being able to access objects using legacy object
names a mapping functionality will be provided.
We want to provide user facing configuration that is robust enough
that it can accomodate multiple different scenarios on how git
evolves and how people use their repositories.
There are two different ways it is envisioned to use mapped object
ids. The first is to require every object in the repository to have a
mapping, so that pushes and pulls from repositories using a different
hash algorithm can work. The second is to have an incomplete mapping
of object ids so that old references to objects in emails, commit
messages, bug trackers and are usable in a read-only manner
with tools like "git show".
The first way fundamentally needs every object in the repository to
have a mapping, which requires the repository to be marked incompatible
for writes fron older versions of git. Thus the mapObjectFormat option
is placed in [extensions].
The ext2 family of filesystems has 3 ways of describing new features
compatible, read-only-compatible, and incompatible. The current git
configurtation has compat (any feature mentioned anywhere in the
configuration outside of [extensions] section), and incompatible (any
configuration inside of the [extensions] section. It would be nice to
have a read-only compatible section for the mandatory mapping
function. Would it be worth adding it now so that we have it for
future extensions?
Having a mapping that is just used in a read-only mode for looking up
old objects with old object ids will be needed post-transition. Such
a mode does not require computing the old hash function or even
support automatically writing any new mappings. So it is completely
safe to enable in a backwards compatible mode. Fort that let's
use core.readObjectMap to make it clear the mappings only read.
I have documented that both of the options readObjectMap and
mapObjectFormat can be specified multiple times if that is needed to
support the desired configuration of git.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
Posting this to hopefully move the conversation forward. Unfortunately
I need something like this so I can tests so I guess now is the time to
resolve this detail.
.../technical/hash-function-transition.txt | 49 ++++++++++++++++---
1 file changed, 43 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/hash-function-transition.txt b/Documentation/technical/hash-function-transition.txt
index 4b937480848a..9f5c672d9ad1 100644
--- a/Documentation/technical/hash-function-transition.txt
+++ b/Documentation/technical/hash-function-transition.txt
@@ -149,13 +149,13 @@ Repository format extension
~~~~~~~~~~~~~~~~~~~~~~~~~~~
A SHA-256 repository uses repository format version `1` (see
Documentation/technical/repository-version.txt) with extensions
-`objectFormat` and `compatObjectFormat`:
+`objectFormat` and `mapObjectFormat`:
[core]
repositoryFormatVersion = 1
[extensions]
objectFormat = sha256
- compatObjectFormat = sha1
+ mapObjectFormat = sha1
The combination of setting `core.repositoryFormatVersion=1` and
populating `extensions.*` ensures that all versions of Git later than
@@ -171,6 +171,43 @@ repository, instead producing an error message.
objectformat
compatobjectformat
+Configurate for a future hash function transition would be:
+
+ [core]
+ repositoryFormatVersion = 1
+ [extensions]
+ objectFormat = futureHash
+ mapObjectFormat = sha256
+ mapObjectFormat = sha1
+
+Or possibly:
+
+ [core]
+ repositoryFormatVersion = 1
+ readObjectMap = sha1
+ [extensions]
+ objectFormat = futureHash
+ mapObjectFormat = sha256
+
+Or post transition to futureHash:
+
+ [core]
+ repositoryFormatVersion = 1
+ readObjectMap = sha1
+ readObjectMap = sha256
+ [extensions]
+ objectFormat = futureHash
+
+The difference between mapObjectFormat and readObjectMap would be that
+mapObjectFormat would ask git to read existing maps, but would not ask
+git to write or create them. Which is enough to support looking up
+old oids post transition, when they are only needed to support
+references in commit logs, bug trackers, emails and the like.
+
+Meanwhile with mapObjectFormat set every object in the entire
+repository would be required to have a bi-directional mapping from the
+the mapped object format to the repositories storage hash function.
+
See the "Transition plan" section below for more details on these
repository extensions.
@@ -682,7 +719,7 @@ Some initial steps can be implemented independently of one another:
- adding support for the PSRC field and safer object pruning
The first user-visible change is the introduction of the objectFormat
-extension (without compatObjectFormat). This requires:
+extension. This requires:
- teaching fsck about this mode of operation
- using the hash function API (vtable) when computing object names
@@ -690,7 +727,7 @@ extension (without compatObjectFormat). This requires:
- rejecting attempts to fetch from or push to an incompatible
repository
-Next comes introduction of compatObjectFormat:
+Next comes introduction of mapObjectFormat:
- implementing the loose-object-idx
- translating object names between object formats
@@ -724,9 +761,9 @@ Over time projects would encourage their users to adopt the "early
transition" and then "late transition" modes to take advantage of the
new, more futureproof SHA-256 object names.
-When objectFormat and compatObjectFormat are both set, commands
+When objectFormat and mapObjectFormat are both set, commands
generating signatures would generate both SHA-1 and SHA-256 signatures
by default to support both new and old users.
In projects using SHA-256 heavily, users could be encouraged to adopt
the "post-transition" mode to avoid accidentally making implicit use
--
2.41.0
^ permalink raw reply related
* [PATCH v6 8/9] repack: implement `--filter-to` for storing filtered out objects
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
A previous commit has implemented `git repack --filter=<filter-spec>` to
allow users to filter out some objects from the main pack and move them
into a new different pack.
It would be nice if this new different pack could be created in a
different directory than the regular pack. This would make it possible
to move large blobs into a pack on a different kind of storage, for
example cheaper storage.
Even in a different directory, this pack can be accessible if, for
example, the Git alternates mechanism is used to point to it. In fact
not using the Git alternates mechanism can corrupt a repo as the
generated pack containing the filtered objects might not be accessible
from the repo any more. So setting up the Git alternates mechanism
should be done before using this feature if the user wants the repo to
be fully usable while this feature is used.
In some cases, like when a repo has just been cloned or when there is no
other activity in the repo, it's Ok to setup the Git alternates
mechanism afterwards though. It's also Ok to just inspect the generated
packfile containing the filtered objects and then just move it into the
'.git/objects/pack/' directory manually. That's why it's not necessary
for this command to check that the Git alternates mechanism has been
already setup.
While at it, as an example to show that `--filter` and `--filter-to`
work well with other options, let's also add a test to check that these
options work well with `--max-pack-size`.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-repack.txt | 11 +++++++
builtin/repack.c | 10 +++++-
t/t7700-repack.sh | 62 ++++++++++++++++++++++++++++++++++++
3 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 6d5bec7716..8545a32667 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -155,6 +155,17 @@ depth is 4095.
a single packfile containing all the objects. See
linkgit:git-rev-list[1] for valid `<filter-spec>` forms.
+--filter-to=<dir>::
+ Write the pack containing filtered out objects to the
+ directory `<dir>`. Only useful with `--filter`. This can be
+ used for putting the pack on a separate object directory that
+ is accessed through the Git alternates mechanism. **WARNING:**
+ If the packfile containing the filtered out objects is not
+ accessible, the repo can become corrupt as it might not be
+ possible to access the objects in that packfile. See the
+ `objects` and `objects/info/alternates` sections of
+ linkgit:gitrepository-layout[5].
+
-b::
--write-bitmap-index::
Write a reachability bitmap index as part of the repack. This
diff --git a/builtin/repack.c b/builtin/repack.c
index ac70698a41..e0e1b52cf0 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -867,6 +867,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
int write_midx = 0;
const char *cruft_expiration = NULL;
const char *expire_to = NULL;
+ const char *filter_to = NULL;
struct option builtin_repack_options[] = {
OPT_BIT('a', NULL, &pack_everything,
@@ -919,6 +920,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
N_("write a multi-pack index of the resulting packs")),
OPT_STRING(0, "expire-to", &expire_to, N_("dir"),
N_("pack prefix to store a pack containing pruned objects")),
+ OPT_STRING(0, "filter-to", &filter_to, N_("dir"),
+ N_("pack prefix to store a pack containing filtered out objects")),
OPT_END()
};
@@ -1067,6 +1070,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (po_args.filter_options.choice)
strvec_pushf(&cmd.args, "--filter=%s",
expand_list_objects_filter_spec(&po_args.filter_options));
+ else if (filter_to)
+ die(_("option '%s' can only be used along with '%s'"), "--filter-to", "--filter");
if (geometry.split_factor)
cmd.in = -1;
@@ -1157,8 +1162,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
}
if (po_args.filter_options.choice) {
+ if (!filter_to)
+ filter_to = packtmp;
+
ret = write_filtered_pack(&po_args,
- packtmp,
+ filter_to,
find_pack_prefix(packdir, packtmp),
&keep_pack_list,
&names,
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 39e89445fd..48e92aa6f7 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -462,6 +462,68 @@ test_expect_success '--filter works with --pack-kept-objects and .keep packs' '
)
'
+test_expect_success '--filter-to stores filtered out objects' '
+ git -C bare.git repack -a -d &&
+ test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
+
+ git init --bare filtered.git &&
+ git -C bare.git -c repack.writebitmaps=false repack -a -d \
+ --filter=blob:none \
+ --filter-to=../filtered.git/objects/pack/pack &&
+ test_stdout_line_count = 1 ls bare.git/objects/pack/pack-*.pack &&
+ test_stdout_line_count = 1 ls filtered.git/objects/pack/pack-*.pack &&
+
+ commit_pack=$(test-tool -C bare.git find-pack -c 1 HEAD) &&
+ blob_pack=$(test-tool -C bare.git find-pack -c 0 HEAD:file1) &&
+ blob_hash=$(git -C bare.git rev-parse HEAD:file1) &&
+ test -n "$blob_hash" &&
+ blob_pack=$(test-tool -C filtered.git find-pack -c 1 $blob_hash) &&
+
+ echo $(pwd)/filtered.git/objects >bare.git/objects/info/alternates &&
+ blob_pack=$(test-tool -C bare.git find-pack -c 1 HEAD:file1) &&
+ blob_content=$(git -C bare.git show $blob_hash) &&
+ test "$blob_content" = "content1"
+'
+
+test_expect_success '--filter works with --max-pack-size' '
+ rm -rf filtered.git &&
+ git init --bare filtered.git &&
+ git init max-pack-size &&
+ (
+ cd max-pack-size &&
+ test_commit base &&
+ # two blobs which exceed the maximum pack size
+ test-tool genrandom foo 1048576 >foo &&
+ git hash-object -w foo &&
+ test-tool genrandom bar 1048576 >bar &&
+ git hash-object -w bar &&
+ git add foo bar &&
+ git commit -m "adding foo and bar"
+ ) &&
+ git clone --no-local --bare max-pack-size max-pack-size.git &&
+ (
+ cd max-pack-size.git &&
+ git -c repack.writebitmaps=false repack -a -d --filter=blob:none \
+ --max-pack-size=1M \
+ --filter-to=../filtered.git/objects/pack/pack &&
+ echo $(cd .. && pwd)/filtered.git/objects >objects/info/alternates &&
+
+ # Check that the 3 blobs are in different packfiles in filtered.git
+ test_stdout_line_count = 3 ls ../filtered.git/objects/pack/pack-*.pack &&
+ test_stdout_line_count = 1 ls objects/pack/pack-*.pack &&
+ foo_pack=$(test-tool find-pack -c 1 HEAD:foo) &&
+ bar_pack=$(test-tool find-pack -c 1 HEAD:bar) &&
+ base_pack=$(test-tool find-pack -c 1 HEAD:base.t) &&
+ test "$foo_pack" != "$bar_pack" &&
+ test "$foo_pack" != "$base_pack" &&
+ test "$bar_pack" != "$base_pack" &&
+ for pack in "$foo_pack" "$bar_pack" "$base_pack"
+ do
+ case "$foo_pack" in */filtered.git/objects/pack/*) true ;; *) return 1 ;; esac
+ done
+ )
+'
+
objdir=.git/objects
midx=$objdir/pack/multi-pack-index
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH] completion: commit: complete configured trailer tokens
From: Martin Ågren @ 2023-09-11 10:20 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, ZheNing Hu, Philippe Blain
In-Reply-To: <pull.1583.git.1694108551683.gitgitgadget@gmail.com>
Hi Philippe,
> Add a __git_trailer_tokens function to list the configured trailers
> tokens, and use it in _git_commit to suggest the configured tokens,
> suffixing the completion words with ':' so that the user only has to add
> the trailer value.
Makes sense.
I've never dabbled in the completion scripts, so take the following with
some salt.
> +__git_trailer_tokens ()
> +{
> + git config --name-only --get-regexp trailer.\*.key | awk -F. '{print $2}'
> +}
The rest of this script uses `__git config` rather than `git config`.
The purpose of `__git` seems to be to respect options given on the
command line, so I think we would want to use it here.
These "." in "trailer." and ".key" will match any character. We also
don't anchor this at beginning and end. Maybe tighten this a bit and use
'^trailer\..*\.key$' to behave better in the face of config such as
this:
[strailer]
skeying = "s"
[trailerx]
keyx = "x"
Another thing. Consider such a config:
[trailer "q.p"]
key = "Q-p-by"
The "trailer.q.p.key" config above ends up completing as just "q"
because of how you use `print $2`. I see that `git commit --trailer=`
itself is fairly relaxed here, so `--trailer=q` effectively ends up
picking up "q.p" in the end. Tightening that is obviously out of scope
here and I have no opinion if the current behavior there is intended.
But maybe we should be a bit less relaxed here and complete to "q.p"? At
any rate, it gets weird when you also have "trailer.q.x.key" in your
config but we still just suggest the one "q".
I see your patch is in next, but maybe some of this tightening might be
worthwhile doing on top of it?
Martin
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood @ 2023-09-11 10:00 UTC (permalink / raw)
To: Oswald Buddenhagen, phillip.wood
Cc: Jeff King, Phillip Wood via GitGitGadget, git,
Johannes Schindelin, Junio C Hamano
In-Reply-To: <ZP2U8TBNjKs5ebky@ugly>
On 10/09/2023 11:05, Oswald Buddenhagen wrote:
>> The child not dying is tricky, if it is in the same process group as
>> git then even if git dies the I think the shell will wait for the
>> child to exit before showing the prompt again so it is not clear to me
>> that the user is disadvantaged by git ignoring SIGINT in that case.
>>
> there is no such thing as waiting for grandchildren. the grandchild is
> reparented to init when the child exits.
>
> there is a situation were one can be deadlocked by a non-exiting
> grandchild: when doing a blocking read of the child's output past its
> exit, when the grandchild has inherited stdout. but that's a
> implementation bug in the parent. and not relevant here.
Yes I got carried away and thought that the shell waited for all the
processes in the foreground process group, but it can only wait on those
processes that it created.
> On Fri, Sep 08, 2023 at 02:11:51PM +0100, Phillip Wood wrote:
>> On 08/09/2023 10:59, Phillip Wood wrote:
>>>> I've never done it before, but from my reading we basically want to do
>>>> (in the forked process before we exec):
>>>>
>>>> setsid();
>>>> open("/dev/tty");
>>>
>>> Do we want a whole new session? As I understand it to launch a
>>> foreground job shells put the child in its own process group and then
>>> call tcsetpgrp() to change the foreground process group of the
>>> controlling terminal to that of the child.
>>
> this would indeed be the right way if we wanted to isolate the children
> more, but ...
>
>> It is better for handling SIGINT and SIGQUIT when we don't want git to
>> be killed but in complicates the handling of SIGTSTP and friends. [...]
>>
> ... this shows that we really don't want that; we don't want to
> replicate interactive shell behavior. that is even before the divergence
> on windows.
Yeah, I'm not enthusiastic about emulating the shell's job control in git.
> so i think your patch is approaching things the right way.
> though blocking signals doesn't appear right - to ensure git's own clean
> exit while it has no children, it must catch sigint anyway, and
> temporarily ignoring it around spawning children sounds racy.
There is an inevitable race between wait() returning and calling
signal() to restore the handlers for SIGINT and SIGQUIT, it is such a
small window I'm not sure it is a problem in practice. There is also a
race when creating the child but if we block signals before calling
fork, then ignore SIGINT and SIGQUIT in the parent before unblocking
them we're OK because the child will be killed as soon as it unblocks
signal by any signal received while the signals were blocks and we'll
detect that in the parent and exit. Currently editor.c just ignores the
signals after fork has returned in the parent which means it is
theoretically possible to kill git with SIGINT while the child is running.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH v6 1/9] pack-objects: allow `--filter` without `--stdout`
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)
taught `git pack-objects` to use `--filter`, but required the use of
`--stdout` since a partial clone mechanism was not yet in place to
handle missing objects. Since then, changes like 9e27beaa23
(promisor-remote: implement promisor_remote_get_direct(), 2019-06-25)
and others added support to dynamically fetch objects that were missing.
Even without a promisor remote, filtering out objects can also be useful
if we can put the filtered out objects in a separate pack, and in this
case it also makes sense for pack-objects to write the packfile directly
to an actual file rather than on stdout.
Remove the `--stdout` requirement when using `--filter`, so that in a
follow-up commit, repack can pass `--filter` to pack-objects to omit
certain objects from the resulting packfile.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-pack-objects.txt | 4 ++--
builtin/pack-objects.c | 8 ++------
t/t5317-pack-objects-filter-objects.sh | 8 ++++++++
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index dea7eacb0f..e32404c6aa 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -296,8 +296,8 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.
nevertheless.
--filter=<filter-spec>::
- Requires `--stdout`. Omits certain objects (usually blobs) from
- the resulting packfile. See linkgit:git-rev-list[1] for valid
+ Omits certain objects (usually blobs) from the resulting
+ packfile. See linkgit:git-rev-list[1] for valid
`<filter-spec>` forms.
--no-filter::
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 72241bdca4..e3e1d11640 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4399,12 +4399,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
if (!rev_list_all || !rev_list_reflog || !rev_list_index)
unpack_unreachable_expiration = 0;
- if (filter_options.choice) {
- if (!pack_to_stdout)
- die(_("cannot use --filter without --stdout"));
- if (stdin_packs)
- die(_("cannot use --filter with --stdin-packs"));
- }
+ if (stdin_packs && filter_options.choice)
+ die(_("cannot use --filter with --stdin-packs"));
if (stdin_packs && use_internal_rev_list)
die(_("cannot use internal rev list with --stdin-packs"));
diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
index b26d476c64..2ff3eef9a3 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -53,6 +53,14 @@ test_expect_success 'verify blob:none packfile has no blobs' '
! grep blob verify_result
'
+test_expect_success 'verify blob:none packfile without --stdout' '
+ git -C r1 pack-objects --revs --filter=blob:none mypackname >packhash <<-EOF &&
+ HEAD
+ EOF
+ git -C r1 verify-pack -v "mypackname-$(cat packhash).pack" >verify_result &&
+ ! grep blob verify_result
+'
+
test_expect_success 'verify normal and blob:none packfiles have same commits/trees' '
git -C r1 verify-pack -v ../all.pack >verify_result &&
grep -E "commit|tree" verify_result |
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH 3/4] unit-tests: do show relative file paths
From: Phillip Wood @ 2023-09-11 13:25 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git; +Cc: Johannes Schindelin
In-Reply-To: <2b4e36c05c9e01b1e489100531fd01515b0786ab.1693462532.git.gitgitgadget@gmail.com>
Hi Johannes
On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Visual C interpolates `__FILE__` with the absolute _Windows_ path of
> the source file. GCC interpolates it with the relative path, and the
> tests even verify that.
Oh, that's a pain
> So let's make sure that the unit tests only emit such paths.
Makes sense
> +#ifndef _MSC_VER
> +#define make_relative(location) location
> +#else
> +/*
> + * Visual C interpolates the absolute Windows path for `__FILE__`,
> + * but we want to see relative paths, as verified by t0080.
> + */
> +#include "strbuf.h"
> +#include "dir.h"
> +
> +static const char *make_relative(const char *location)
> +{
> + static const char *prefix;
> + static size_t prefix_len;
> + static struct strbuf buf = STRBUF_INIT;
So far test-lib.c avoids using things like struct strbuf that it will be
used to test. In this instance we're only using it on one particular
compiler so it may not matter so much. We could avoid it but I'm not
sure it is worth the extra complexity. One thing I noted in this patch
is that prefix is leaked but I'm not sure if you run any leak checkers
on the msvc build.
static const char *make_relative(const char *location)
{
static char prefix[] = __FILE__
static size_t *prefix_len = (size_t)-1;
static char buf[PATH_MAX];
if (prefix == (size_t)-1) {
const char *path = "\\t\unit-tests\\test-lib.c";
size_t path_len = strlen(path);
prefix_len = strlen(prefix);
if (prefix_len < path_len) ||
memcmp(prefix + prefix_len - path_len, path, path_len)
die(...);
prefix_len -= path_len - 1; /* keep trailing '\\' */
prefix[prefix_len] = '\0';
}
/* Does it not start with the expected prefix? */
if (fspathncmp(location, prefix, prefix_len))
return location;
if (strlen(location) - prefix_len > sizeof(buf) - 1)
die(...)
/* +1 to copy NUL terminator */
memcpy(buf, location + prefix_len, strlen(location) - prefix_len + 1);
convert_slashes(buf);
return buf;
}
Best Wishes
Phillip
^ permalink raw reply
* Re: [RFC] New configuration option "diff.statNameWidth"
From: Dragan Simic @ 2023-09-11 18:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <a879c9c7d3b9bdae9a49f67fbe6316fc@manjaro.org>
On 2023-09-03 05:43, Dragan Simic wrote:
> On 2023-09-03 00:16, Junio C Hamano wrote:
>> Having said all the above, once we start seeing the actual patches
>> and its sales pitch (aka proposed commit log message), we do guide
>> and help polishing the patch into a better shape, so it will not
>> be like the contributor has to work in the dark without knowing
>> what level of bar their contribution has to pass.
>
> Thanks, everything sounds great and welcoming to the new contributors,
> who of course need to be willing to put in the required amount of
> skill and effort.
I sent a patch to the git mailing list today, about five hours ago, but
it hasn't appeared on the list yet. Could something be wrong with the
mail server(s), as I also received no other messages from the list in
the last six hours or so?
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood @ 2023-09-11 10:14 UTC (permalink / raw)
To: phillip.wood, Oswald Buddenhagen
Cc: Jeff King, Phillip Wood via GitGitGadget, git,
Johannes Schindelin, Junio C Hamano
In-Reply-To: <80eb7631-e5c0-497e-b2a9-b1f8c8a4a306@gmail.com>
On 11/09/2023 11:00, Phillip Wood wrote:
> There is an inevitable race between wait() returning and calling
> signal() to restore the handlers for SIGINT and SIGQUIT,
In principle if we installed a signal handler to set a flag if a signal
is received while calling wait() and then once wait() returns
successfully see if the child was killed we can tell if the signal was
received while the child was alive. In practice if the child is catching
SIGINT or SIGQUIT we cannot rely on it re-raising the signal so that
wont work.
Best Wishes
Phillip
^ permalink raw reply
* Re: [bug] git clone command leaves orphaned ssh process
From: Max Amelchenko @ 2023-09-11 10:11 UTC (permalink / raw)
To: Taylor Blau; +Cc: Bagas Sanjaya, git, Hideaki Yoshifuji, Junio C Hamano
In-Reply-To: <ZP4PO+HkbsbuKact@nand.local>
Maybe it's connected also to the underlying infrastructure? We are
getting this in AWS lambda jobs and we're hitting a system limit of
max processes because of it.
Can you try running this inside this image public.ecr.aws/lambda/python ?
On Sun, Sep 10, 2023 at 9:47 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Sun, Sep 10, 2023 at 12:47:14PM +0300, Max Amelchenko wrote:
> > Output of second ps aux command (after running git clone):
> >
> > bash-4.2# ps aux
> >
> > USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
> >
> > root 1 0.0 0.0 715708 5144 pts/0 Ssl+ 09:43 0:00
> > /usr/local/bin/aws-lambda-rie /var/runtime/bootstrap
> >
> > root 14 0.0 0.0 114096 3088 pts/1 Ss 09:43 0:00 bash
> >
> > root 167 0.5 0.0 0 0 pts/1 Z 09:46 0:00 [ssh] <defunct>
> >
> > root 168 0.0 0.0 118296 3408 pts/1 R+ 09:46 0:00 ps aux
> >
> > See the added ssh defunct process.
>
> Hmm... I wasn't quite able to reproduce this locally. Below
> `git.compile` points to a Git executable built from the v2.40.1 tag
> corresponding to your bug report:
>
> $ host='ssh://*****@*****lab-prod.server.sim.cloud/terraform/modules/aws-eks'
> $ git.compile clone "$host" /tmp/x
> Cloning into '/tmp/x'...
> ssh: Could not resolve hostname *****lab-prod.server.sim.cloud: Name or service not known
> fatal: Could not read from remote repository.
>
> Please make sure you have the correct access rights
> and the repository exists.
>
> and then:
>
> $ ps aux | grep defunct
> ttaylorr 3688844 0.0 0.0 6340 2180 pts/1 S+ 14:45 0:00 grep --color defunct
>
> Thanks,
> Taylor
^ permalink raw reply
* [PATCH] diff --stat: add config option to limit filename width
From: Dragan Simic @ 2023-09-11 15:39 UTC (permalink / raw)
To: git
Add new configuration option diff.statNameWidth=<width> that is equivalent
to the command-line option --stat-name-width=<width>, but it is ignored
by format-patch. This follows the logic established by the already
existing configuration option diff.statGraphWidth=<width>.
Limiting the widths of names and graphs in the --stat output makes sense
for interactive work on wide terminals with many columns, hence the support
for these configuration options. They don't affect format-patch because
it already adheres to the traditional 80-column standard.
Update the documentation and add more tests to cover new configuration
option diff.statNameWidth=<width>. While there, perform a few minor code
and whitespace cleanups here and there, as spotted.
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
Documentation/config/diff.txt | 4 ++++
Documentation/diff-options.txt | 17 +++++++-------
builtin/diff.c | 1 +
builtin/log.c | 1 +
builtin/merge.c | 1 +
builtin/rebase.c | 1 +
diff.c | 11 +++++++--
graph.c | 1 -
t/t4052-stat-output.sh | 41 ++++++++++++++++++++++++++++------
9 files changed, 60 insertions(+), 18 deletions(-)
diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
index 35a7bf86d7..9391c77e55 100644
--- a/Documentation/config/diff.txt
+++ b/Documentation/config/diff.txt
@@ -52,6 +52,10 @@ directories with less than 10% of the total amount of changed files,
and accumulating child directory counts in the parent directories:
`files,10,cumulative`.
+diff.statNameWidth::
+ Limit the width of the filename part in --stat output. If set, applies
+ to all commands generating --stat output except format-patch.
+
diff.statGraphWidth::
Limit the width of the graph part in --stat output. If set, applies
to all commands generating --stat output except format-patch.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9f33f88771..6603470dbe 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -210,14 +210,15 @@ have to use `--diff-algorithm=default` option.
part. Maximum width defaults to terminal width, or 80 columns
if not connected to a terminal, and can be overridden by
`<width>`. The width of the filename part can be limited by
- giving another width `<name-width>` after a comma. The width
- of the graph part can be limited by using
- `--stat-graph-width=<width>` (affects all commands generating
- a stat graph) or by setting `diff.statGraphWidth=<width>`
- (does not affect `git format-patch`).
- By giving a third parameter `<count>`, you can limit the
- output to the first `<count>` lines, followed by `...` if
- there are more.
+ giving another width `<name-width>` after a comma or by setting
+ `diff.statNameWidth=<width>`. The width of the graph part can be
+ limited by using `--stat-graph-width=<width>` or by setting
+ `diff.statGraphWidth=<width>`. Using `--stat` or
+ `--stat-graph-width` affects all commands generating a stat graph,
+ while setting `diff.statNameWidth` or `diff.statGraphWidth`
+ does not affect `git format-patch`.
+ By giving a third parameter `<count>`, you can limit the output to
+ the first `<count>` lines, followed by `...` if there are more.
+
These parameters can also be set individually with `--stat-width=<width>`,
`--stat-name-width=<name-width>` and `--stat-count=<count>`.
diff --git a/builtin/diff.c b/builtin/diff.c
index 0b313549c7..c0f564273a 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -475,6 +475,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
/* Set up defaults that will apply to both no-index and regular diffs. */
rev.diffopt.stat_width = -1;
+ rev.diffopt.stat_name_width = -1;
rev.diffopt.stat_graph_width = -1;
rev.diffopt.flags.allow_external = 1;
rev.diffopt.flags.allow_textconv = 1;
diff --git a/builtin/log.c b/builtin/log.c
index 87088077d9..2901514778 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -175,6 +175,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
rev->verbose_header = 1;
rev->diffopt.flags.recursive = 1;
rev->diffopt.stat_width = -1; /* use full terminal width */
+ rev->diffopt.stat_name_width = -1; /* respect statNameWidth config */
rev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config */
rev->abbrev_commit = default_abbrev_commit;
rev->show_root_diff = default_show_root;
diff --git a/builtin/merge.c b/builtin/merge.c
index de68910177..fd6942d0ef 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -488,6 +488,7 @@ static void finish(struct commit *head_commit,
struct diff_options opts;
repo_diff_setup(the_repository, &opts);
opts.stat_width = -1; /* use full terminal width */
+ opts.stat_name_width = -1; /* respect statNameWidth config */
opts.stat_graph_width = -1; /* respect statGraphWidth config */
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..ed15accec9 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1804,6 +1804,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
/* We want color (if set), but no pager */
repo_diff_setup(the_repository, &opts);
opts.stat_width = -1; /* use full terminal width */
+ opts.stat_name_width = -1; /* respect statNameWidth config */
opts.stat_graph_width = -1; /* respect statGraphWidth config */
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
diff --git a/diff.c b/diff.c
index bccb018da4..353e3b2cc9 100644
--- a/diff.c
+++ b/diff.c
@@ -65,6 +65,7 @@ int diff_auto_refresh_index = 1;
static int diff_mnemonic_prefix;
static int diff_no_prefix;
static int diff_relative;
+static int diff_stat_name_width;
static int diff_stat_graph_width;
static int diff_dirstat_permille_default = 30;
static struct diff_options default_diff_options;
@@ -410,6 +411,10 @@ int git_diff_ui_config(const char *var, const char *value,
diff_relative = git_config_bool(var, value);
return 0;
}
+ if (!strcmp(var, "diff.statnamewidth")) {
+ diff_stat_name_width = git_config_int(var, value, ctx->kvi);
+ return 0;
+ }
if (!strcmp(var, "diff.statgraphwidth")) {
diff_stat_graph_width = git_config_int(var, value, ctx->kvi);
return 0;
@@ -2704,12 +2709,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
number_width = decimal_width(max_change) > number_width ?
decimal_width(max_change) : number_width;
+ if (options->stat_name_width == -1)
+ options->stat_name_width = diff_stat_name_width;
if (options->stat_graph_width == -1)
options->stat_graph_width = diff_stat_graph_width;
/*
- * Guarantee 3/8*16==6 for the graph part
- * and 5/8*16==10 for the filename part
+ * Guarantee 3/8*16 == 6 for the graph part
+ * and 5/8*16 == 10 for the filename part
*/
if (width < 16 + 6 + number_width)
width = 16 + 6 + number_width;
diff --git a/graph.c b/graph.c
index 2a9dc430fa..1ca34770ee 100644
--- a/graph.c
+++ b/graph.c
@@ -339,7 +339,6 @@ void graph_setup_line_prefix(struct diff_options *diffopt)
diffopt->output_prefix = diff_output_prefix_callback;
}
-
struct git_graph *graph_init(struct rev_info *opt)
{
struct git_graph *graph = xmalloc(sizeof(struct git_graph));
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 3ee27e277d..beb2ec2a55 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -49,34 +49,63 @@ log -1 --stat
EOF
cat >expect.60 <<-'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
EOF
cat >expect.6030 <<-'EOF'
...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
EOF
-cat >expect2.60 <<-'EOF'
+while read verb expect cmd args
+do
+ # No width limit applied when statNameWidth is ignored
+ case "$expect" in expect72|expect.6030)
+ test_expect_success "$cmd $verb statNameWidth config with long name" '
+ git -c diff.statNameWidth=30 $cmd $args >output &&
+ grep " | " output >actual &&
+ test_cmp $expect actual
+ ';;
+ esac
+ # Maximum width limit still applied when statNameWidth is ignored
+ case "$expect" in expect.60|expect.6030)
+ test_expect_success "$cmd --stat=width $verb statNameWidth config with long name" '
+ git -c diff.statNameWidth=30 $cmd $args --stat=60 >output &&
+ grep " | " output >actual &&
+ test_cmp $expect actual
+ ';;
+ esac
+done <<\EOF
+ignores expect72 format-patch -1 --stdout
+ignores expect.60 format-patch -1 --stdout
+respects expect.6030 diff HEAD^ HEAD --stat
+respects expect.6030 show --stat
+respects expect.6030 log -1 --stat
+EOF
+
+cat >expect.40 <<-'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
+EOF
+cat >expect2.40 <<-'EOF'
...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
EOF
cat >expect2.6030 <<-'EOF'
...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
EOF
while read expect cmd args
do
test_expect_success "$cmd --stat=width: a long name is given more room when the bar is short" '
git $cmd $args --stat=40 >output &&
grep " | " output >actual &&
- test_cmp $expect.60 actual
+ test_cmp $expect.40 actual
'
test_expect_success "$cmd --stat-width=width with long name" '
git $cmd $args --stat-width=40 >output &&
grep " | " output >actual &&
- test_cmp $expect.60 actual
+ test_cmp $expect.40 actual
'
- test_expect_success "$cmd --stat=...,name-width with long name" '
+ test_expect_success "$cmd --stat=width,name-width with long name" '
git $cmd $args --stat=60,30 >output &&
grep " | " output >actual &&
test_cmp $expect.6030 actual
@@ -94,7 +123,6 @@ expect show --stat
expect log -1 --stat
EOF
-
test_expect_success 'preparation for big change tests' '
>abcd &&
git add abcd &&
@@ -207,7 +235,6 @@ respects expect40 show --stat
respects expect40 log -1 --stat
EOF
-
cat >expect <<'EOF'
abcd | 1000 ++++++++++++++++++++++++++
EOF
^ permalink raw reply related
* Re: [RFC][PATCH 0/32] SHA256 and SHA1 interoperability
From: Eric W. Biederman @ 2023-09-11 16:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, brian m. carlson
In-Reply-To: <xmqq8r9di5ba.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>
>> I would like to see the SHA256 transition happen so I started playing
>> with the k2204-transition-interop branch of brian m. carlson's tree.
>
> I needed these tweaks to build the series standalone on 'master' (or
> 2.42). There are semantic merge conflicts with some topics in flight
> when this is merged to 'seen', so it may take me a bit more time to
> push the integration result.
Junio, brian for the very warm reception of this, it is very
encouraging.
I am not worried about what it will take time to get the changes I
posted into the integration. I had only envisioned them as good enough
to get the technical ideas across, and had never envisioned them as
being accepted as is.
What I am envisioning as my future directions are:
- Post non controversial cleanups, so they can be merged.
(I can only see about 4 of them the most significant is:
bulk-checkin: Only accept blobs)
- Sort out the configuration options
- Post the smallest patchset I can that will allow testing the code in
object-file-convert.c. Unfortunately for that I need configuration
options to enable the mapping.
In starting to write the tests I have already found a bug in
the conversion of tags (an extra newline is added), and I haven't
even gotten to testing the tricky bits with signatures.
- Once the object file conversion is tested and is solid work on
the more substantial pieces.
Does that sound like a reasonable plan?
Eric
^ permalink raw reply
* Re: [PATCH] diff --no-index: fix -R with stdin
From: Junio C Hamano @ 2023-09-11 19:04 UTC (permalink / raw)
To: René Scharfe; +Cc: Martin Storsjö, git, Phillip Wood
In-Reply-To: <22fdfa3b-f90e-afcc-667c-705fb7670245@web.de>
René Scharfe <l.s.r@web.de> writes:
> When -R is given, queue_diff() swaps the mode and name variables of the
> two files to produce a reverse diff. 1e3f26542a (diff --no-index:
> support reading from named pipes, 2023-07-05) added variables that
> indicate whether files are special, i.e named pipes or - for stdin.
> These new variables were not swapped, though, which broke the handling
> of stdin with with -R. Swap them like the other metadata variables.
>
> Reported-by: Martin Storsjö <martin@martin.st>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Great bug report, thank you!
Looking good. I wish all our bugs are this easy and obvious ;-)
> diff-no-index.c | 1 +
> t/t4053-diff-no-index.sh | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8aead3e332..e7041b89e3 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -232,6 +232,7 @@ static int queue_diff(struct diff_options *o,
> if (o->flags.reverse_diff) {
> SWAP(mode1, mode2);
> SWAP(name1, name2);
> + SWAP(special1, special2);
> }
>
> d1 = noindex_filespec(name1, mode1, special1);
> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
> index 6781cc9078..5f059f65fc 100755
> --- a/t/t4053-diff-no-index.sh
> +++ b/t/t4053-diff-no-index.sh
> @@ -224,6 +224,25 @@ test_expect_success "diff --no-index treats '-' as stdin" '
> test_must_be_empty actual
> '
>
> +test_expect_success "diff --no-index -R treats '-' as stdin" '
> + cat >expect <<-EOF &&
> + diff --git b/a/1 a/-
> + index $(git hash-object --stdin <a/1)..$ZERO_OID 100644
> + --- b/a/1
> + +++ a/-
> + @@ -1 +1 @@
> + -1
> + +x
> + EOF
> +
> + test_write_lines x | test_expect_code 1 \
> + git -c core.abbrev=no diff --no-index -R -- - a/1 >actual &&
> + test_cmp expect actual &&
> +
> + test_write_lines 1 | git diff --no-index -R -- a/1 - >actual &&
> + test_must_be_empty actual
> +'
> +
> test_expect_success 'diff --no-index refuses to diff stdin and a directory' '
> test_must_fail git diff --no-index -- - a </dev/null 2>err &&
> grep "fatal: cannot compare stdin to a directory" err
> --
> 2.42.0
^ permalink raw reply
* [PATCH v6 6/9] repack: add `--filter=<filter-spec>` option
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
This new option puts the objects specified by `<filter-spec>` into a
separate packfile.
This could be useful if, for example, some blobs take up a lot of
precious space on fast storage while they are rarely accessed. It could
make sense to move them into a separate cheaper, though slower, storage.
It's possible to find which new packfile contains the filtered out
objects using one of the following:
- `git verify-pack -v ...`,
- `test-tool find-pack ...`, which a previous commit added,
- `--filter-to=<dir>`, which a following commit will add to specify
where the pack containing the filtered out objects will be.
This feature is implemented by running `git pack-objects` twice in a
row. The first command is run with `--filter=<filter-spec>`, using the
specified filter. It packs objects while omitting the objects specified
by the filter. Then another `git pack-objects` command is launched using
`--stdin-packs`. We pass it all the previously existing packs into its
stdin, so that it will pack all the objects in the previously existing
packs. But we also pass into its stdin, the pack created by the previous
`git pack-objects --filter=<filter-spec>` command as well as the kept
packs, all prefixed with '^', so that the objects in these packs will be
omitted from the resulting pack. The result is that only the objects
filtered out by the first `git pack-objects` command are in the pack
resulting from the second `git pack-objects` command.
As the interactions with kept packs are a bit tricky, a few related
tests are added.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-repack.txt | 12 ++++
builtin/repack.c | 73 +++++++++++++++++++
t/t7700-repack.sh | 135 +++++++++++++++++++++++++++++++++++
3 files changed, 220 insertions(+)
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 4017157949..6d5bec7716 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -143,6 +143,18 @@ depth is 4095.
a larger and slower repository; see the discussion in
`pack.packSizeLimit`.
+--filter=<filter-spec>::
+ Remove objects matching the filter specification from the
+ resulting packfile and put them into a separate packfile. Note
+ that objects used in the working directory are not filtered
+ out. So for the split to fully work, it's best to perform it
+ in a bare repo and to use the `-a` and `-d` options along with
+ this option. Also `--no-write-bitmap-index` (or the
+ `repack.writebitmaps` config option set to `false`) should be
+ used otherwise writing bitmap index will fail, as it supposes
+ a single packfile containing all the objects. See
+ linkgit:git-rev-list[1] for valid `<filter-spec>` forms.
+
-b::
--write-bitmap-index::
Write a reachability bitmap index as part of the repack. This
diff --git a/builtin/repack.c b/builtin/repack.c
index 8de3009b9f..ac70698a41 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -21,6 +21,7 @@
#include "pack.h"
#include "pack-bitmap.h"
#include "refs.h"
+#include "list-objects-filter-options.h"
#define ALL_INTO_ONE 1
#define LOOSEN_UNREACHABLE 2
@@ -57,6 +58,7 @@ struct pack_objects_args {
int no_reuse_object;
int quiet;
int local;
+ struct list_objects_filter_options filter_options;
};
static int repack_config(const char *var, const char *value,
@@ -725,6 +727,57 @@ static int finish_pack_objects_cmd(struct child_process *cmd,
return finish_command(cmd);
}
+static int write_filtered_pack(const struct pack_objects_args *args,
+ const char *destination,
+ const char *pack_prefix,
+ struct string_list *keep_pack_list,
+ struct string_list *names,
+ struct string_list *existing_packs,
+ struct string_list *existing_kept_packs)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct string_list_item *item;
+ FILE *in;
+ int ret, i;
+ const char *caret;
+ const char *scratch;
+ int local = skip_prefix(destination, packdir, &scratch);
+
+ prepare_pack_objects(&cmd, args, destination);
+
+ strvec_push(&cmd.args, "--stdin-packs");
+
+ if (!pack_kept_objects)
+ strvec_push(&cmd.args, "--honor-pack-keep");
+ for (i = 0; i < keep_pack_list->nr; i++)
+ strvec_pushf(&cmd.args, "--keep-pack=%s",
+ keep_pack_list->items[i].string);
+
+ cmd.in = -1;
+
+ ret = start_command(&cmd);
+ if (ret)
+ return ret;
+
+ /*
+ * Here 'names' contains only the pack(s) that were just
+ * written, which is exactly the packs we want to keep. Also
+ * 'existing_kept_packs' already contains the packs in
+ * 'keep_pack_list'.
+ */
+ in = xfdopen(cmd.in, "w");
+ for_each_string_list_item(item, names)
+ fprintf(in, "^%s-%s.pack\n", pack_prefix, item->string);
+ for_each_string_list_item(item, existing_packs)
+ fprintf(in, "%s.pack\n", item->string);
+ caret = pack_kept_objects ? "" : "^";
+ for_each_string_list_item(item, existing_kept_packs)
+ fprintf(in, "%s%s.pack\n", caret, item->string);
+ fclose(in);
+
+ return finish_pack_objects_cmd(&cmd, names, local);
+}
+
static int write_cruft_pack(const struct pack_objects_args *args,
const char *destination,
const char *pack_prefix,
@@ -855,6 +908,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
N_("limits the maximum number of threads")),
OPT_STRING(0, "max-pack-size", &po_args.max_pack_size, N_("bytes"),
N_("maximum size of each packfile")),
+ OPT_PARSE_LIST_OBJECTS_FILTER(&po_args.filter_options),
OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
N_("repack objects in packs marked with .keep")),
OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
@@ -868,6 +922,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
OPT_END()
};
+ list_objects_filter_init(&po_args.filter_options);
+
git_config(repack_config, &cruft_po_args);
argc = parse_options(argc, argv, prefix, builtin_repack_options,
@@ -1008,6 +1064,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
strvec_push(&cmd.args, "--incremental");
}
+ if (po_args.filter_options.choice)
+ strvec_pushf(&cmd.args, "--filter=%s",
+ expand_list_objects_filter_spec(&po_args.filter_options));
+
if (geometry.split_factor)
cmd.in = -1;
else
@@ -1096,6 +1156,18 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
}
}
+ if (po_args.filter_options.choice) {
+ ret = write_filtered_pack(&po_args,
+ packtmp,
+ find_pack_prefix(packdir, packtmp),
+ &keep_pack_list,
+ &names,
+ &existing_nonkept_packs,
+ &existing_kept_packs);
+ if (ret)
+ goto cleanup;
+ }
+
string_list_sort(&names);
close_object_store(the_repository->objects);
@@ -1230,6 +1302,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
string_list_clear(&existing_nonkept_packs, 0);
string_list_clear(&existing_kept_packs, 0);
free_pack_geometry(&geometry);
+ list_objects_filter_release(&po_args.filter_options);
return ret;
}
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 27b66807cd..39e89445fd 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -327,6 +327,141 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '
test_must_be_empty actual
'
+test_expect_success 'repacking with a filter works' '
+ git -C bare.git repack -a -d &&
+ test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
+ git -C bare.git -c repack.writebitmaps=false repack -a -d --filter=blob:none &&
+ test_stdout_line_count = 2 ls bare.git/objects/pack/*.pack &&
+ commit_pack=$(test-tool -C bare.git find-pack -c 1 HEAD) &&
+ blob_pack=$(test-tool -C bare.git find-pack -c 1 HEAD:file1) &&
+ test "$commit_pack" != "$blob_pack" &&
+ tree_pack=$(test-tool -C bare.git find-pack -c 1 HEAD^{tree}) &&
+ test "$tree_pack" = "$commit_pack" &&
+ blob_pack2=$(test-tool -C bare.git find-pack -c 1 HEAD:file2) &&
+ test "$blob_pack2" = "$blob_pack"
+'
+
+test_expect_success '--filter fails with --write-bitmap-index' '
+ test_must_fail \
+ env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
+ git -C bare.git repack -a -d --write-bitmap-index --filter=blob:none
+'
+
+test_expect_success 'repacking with two filters works' '
+ git init two-filters &&
+ (
+ cd two-filters &&
+ mkdir subdir &&
+ test_commit foo &&
+ test_commit subdir_bar subdir/bar &&
+ test_commit subdir_baz subdir/baz
+ ) &&
+ git clone --no-local --bare two-filters two-filters.git &&
+ (
+ cd two-filters.git &&
+ test_stdout_line_count = 1 ls objects/pack/*.pack &&
+ git -c repack.writebitmaps=false repack -a -d \
+ --filter=blob:none --filter=tree:1 &&
+ test_stdout_line_count = 2 ls objects/pack/*.pack &&
+ commit_pack=$(test-tool find-pack -c 1 HEAD) &&
+ blob_pack=$(test-tool find-pack -c 1 HEAD:foo.t) &&
+ root_tree_pack=$(test-tool find-pack -c 1 HEAD^{tree}) &&
+ subdir_tree_hash=$(git ls-tree --object-only HEAD -- subdir) &&
+ subdir_tree_pack=$(test-tool find-pack -c 1 "$subdir_tree_hash") &&
+
+ # Root tree and subdir tree are not in the same packfiles
+ test "$commit_pack" != "$blob_pack" &&
+ test "$commit_pack" = "$root_tree_pack" &&
+ test "$blob_pack" = "$subdir_tree_pack"
+ )
+'
+
+prepare_for_keep_packs () {
+ git init keep-packs &&
+ (
+ cd keep-packs &&
+ test_commit foo &&
+ test_commit bar
+ ) &&
+ git clone --no-local --bare keep-packs keep-packs.git &&
+ (
+ cd keep-packs.git &&
+
+ # Create two packs
+ # The first pack will contain all of the objects except one blob
+ git rev-list --objects --all >objs &&
+ grep -v "bar.t" objs | git pack-objects pack &&
+ # The second pack will contain the excluded object and be kept
+ packid=$(grep "bar.t" objs | git pack-objects pack) &&
+ >pack-$packid.keep &&
+
+ # Replace the existing pack with the 2 new ones
+ rm -f objects/pack/pack* &&
+ mv pack-* objects/pack/
+ )
+}
+
+test_expect_success '--filter works with .keep packs' '
+ prepare_for_keep_packs &&
+ (
+ cd keep-packs.git &&
+
+ foo_pack=$(test-tool find-pack -c 1 HEAD:foo.t) &&
+ bar_pack=$(test-tool find-pack -c 1 HEAD:bar.t) &&
+ head_pack=$(test-tool find-pack -c 1 HEAD) &&
+
+ test "$foo_pack" != "$bar_pack" &&
+ test "$foo_pack" = "$head_pack" &&
+
+ git -c repack.writebitmaps=false repack -a -d --filter=blob:none &&
+
+ foo_pack_1=$(test-tool find-pack -c 1 HEAD:foo.t) &&
+ bar_pack_1=$(test-tool find-pack -c 1 HEAD:bar.t) &&
+ head_pack_1=$(test-tool find-pack -c 1 HEAD) &&
+
+ # Object bar is still only in the old .keep pack
+ test "$foo_pack_1" != "$foo_pack" &&
+ test "$bar_pack_1" = "$bar_pack" &&
+ test "$head_pack_1" != "$head_pack" &&
+
+ test "$foo_pack_1" != "$bar_pack_1" &&
+ test "$foo_pack_1" != "$head_pack_1" &&
+ test "$bar_pack_1" != "$head_pack_1"
+ )
+'
+
+test_expect_success '--filter works with --pack-kept-objects and .keep packs' '
+ rm -rf keep-packs keep-packs.git &&
+ prepare_for_keep_packs &&
+ (
+ cd keep-packs.git &&
+
+ foo_pack=$(test-tool find-pack -c 1 HEAD:foo.t) &&
+ bar_pack=$(test-tool find-pack -c 1 HEAD:bar.t) &&
+ head_pack=$(test-tool find-pack -c 1 HEAD) &&
+
+ test "$foo_pack" != "$bar_pack" &&
+ test "$foo_pack" = "$head_pack" &&
+
+ git -c repack.writebitmaps=false repack -a -d --filter=blob:none \
+ --pack-kept-objects &&
+
+ foo_pack_1=$(test-tool find-pack -c 1 HEAD:foo.t) &&
+ test-tool find-pack -c 2 HEAD:bar.t >bar_pack_1 &&
+ head_pack_1=$(test-tool find-pack -c 1 HEAD) &&
+
+ test "$foo_pack_1" != "$foo_pack" &&
+ test "$foo_pack_1" != "$bar_pack" &&
+ test "$head_pack_1" != "$head_pack" &&
+
+ # Object bar is in both the old .keep pack and the new
+ # pack that contained the filtered out objects
+ grep "$bar_pack" bar_pack_1 &&
+ grep "$foo_pack_1" bar_pack_1 &&
+ test "$foo_pack_1" != "$head_pack_1"
+ )
+'
+
objdir=.git/objects
midx=$objdir/pack/multi-pack-index
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH v2 2/6] trailer: split process_input_file into separate pieces
From: Junio C Hamano @ 2023-09-11 17:10 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <c00f4623d0b97cc8ed71ea018e6ecf6e21739b53.1694240177.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Currently, process_input_file does three things:
>
> (1) parse the input string for trailers,
> (2) print text before the trailers, and
> (3) calculate the position of the input where the trailers end.
>
> Rename this function to parse_trailers(), and make it only do
> (1). The caller of this function, process_trailers, becomes responsible
> for (2) and (3). These items belong inside process_trailers because they
> are both concerned with printing the surrounding text around
> trailers (which is already one of the immediate concerns of
> process_trailers).
Nicely explained and the resulting code reads well.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-09-11 20:11 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <ZP2X9roiaeEjzf24@ugly>
Am 10.09.23 um 12:18 schrieb Oswald Buddenhagen:
> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>> Convert the offending OPT_CMDMODE users and use the typed value_int
>> point in the macro's definition to enforce that type for future ones.
>>
> that defeats -Wswitch[-enum], though.
True. Though I don't fully understand these warnings (why not then
also warn about if without else?), but taking them away is a bit rude
to those who care.
> the pedantically correct solution would be using setter callbacks.
Or to use an int to point to and then copy into a companion enum
variable to after parsing, which would be my choice.
René
^ permalink raw reply
* [PATCH v6 2/9] t/helper: add 'find-pack' test-tool
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
In a following commit, we will make it possible to separate objects in
different packfiles depending on a filter.
To make sure that the right objects are in the right packs, let's add a
new test-tool that can display which packfile(s) a given object is in.
Let's also make it possible to check if a given object is in the
expected number of packfiles with a `--check-count <n>` option.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Makefile | 1 +
t/helper/test-find-pack.c | 50 ++++++++++++++++++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0080-find-pack.sh | 82 +++++++++++++++++++++++++++++++++++++++
5 files changed, 135 insertions(+)
create mode 100644 t/helper/test-find-pack.c
create mode 100755 t/t0080-find-pack.sh
diff --git a/Makefile b/Makefile
index 5776309365..742b76998e 100644
--- a/Makefile
+++ b/Makefile
@@ -800,6 +800,7 @@ TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
TEST_BUILTINS_OBJS += test-env-helper.o
TEST_BUILTINS_OBJS += test-example-decorate.o
TEST_BUILTINS_OBJS += test-fast-rebase.o
+TEST_BUILTINS_OBJS += test-find-pack.o
TEST_BUILTINS_OBJS += test-fsmonitor-client.o
TEST_BUILTINS_OBJS += test-genrandom.o
TEST_BUILTINS_OBJS += test-genzeros.o
diff --git a/t/helper/test-find-pack.c b/t/helper/test-find-pack.c
new file mode 100644
index 0000000000..e8bd793e58
--- /dev/null
+++ b/t/helper/test-find-pack.c
@@ -0,0 +1,50 @@
+#include "test-tool.h"
+#include "object-name.h"
+#include "object-store.h"
+#include "packfile.h"
+#include "parse-options.h"
+#include "setup.h"
+
+/*
+ * Display the path(s), one per line, of the packfile(s) containing
+ * the given object.
+ *
+ * If '--check-count <n>' is passed, then error out if the number of
+ * packfiles containing the object is not <n>.
+ */
+
+static const char *find_pack_usage[] = {
+ "test-tool find-pack [--check-count <n>] <object>",
+ NULL
+};
+
+int cmd__find_pack(int argc, const char **argv)
+{
+ struct object_id oid;
+ struct packed_git *p;
+ int count = -1, actual_count = 0;
+ const char *prefix = setup_git_directory();
+
+ struct option options[] = {
+ OPT_INTEGER('c', "check-count", &count, "expected number of packs"),
+ OPT_END(),
+ };
+
+ argc = parse_options(argc, argv, prefix, options, find_pack_usage, 0);
+ if (argc != 1)
+ usage(find_pack_usage[0]);
+
+ if (repo_get_oid(the_repository, argv[0], &oid))
+ die("cannot parse %s as an object name", argv[0]);
+
+ for (p = get_all_packs(the_repository); p; p = p->next)
+ if (find_pack_entry_one(oid.hash, p)) {
+ printf("%s\n", p->pack_name);
+ actual_count++;
+ }
+
+ if (count > -1 && count != actual_count)
+ die("bad packfile count %d instead of %d", actual_count, count);
+
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index abe8a785eb..41da40c296 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -31,6 +31,7 @@ static struct test_cmd cmds[] = {
{ "env-helper", cmd__env_helper },
{ "example-decorate", cmd__example_decorate },
{ "fast-rebase", cmd__fast_rebase },
+ { "find-pack", cmd__find_pack },
{ "fsmonitor-client", cmd__fsmonitor_client },
{ "genrandom", cmd__genrandom },
{ "genzeros", cmd__genzeros },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index ea2672436c..411dbf2db4 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -25,6 +25,7 @@ int cmd__dump_reftable(int argc, const char **argv);
int cmd__env_helper(int argc, const char **argv);
int cmd__example_decorate(int argc, const char **argv);
int cmd__fast_rebase(int argc, const char **argv);
+int cmd__find_pack(int argc, const char **argv);
int cmd__fsmonitor_client(int argc, const char **argv);
int cmd__genrandom(int argc, const char **argv);
int cmd__genzeros(int argc, const char **argv);
diff --git a/t/t0080-find-pack.sh b/t/t0080-find-pack.sh
new file mode 100755
index 0000000000..67b11216a3
--- /dev/null
+++ b/t/t0080-find-pack.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='test `test-tool find-pack`'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit one &&
+ test_commit two &&
+ test_commit three &&
+ test_commit four &&
+ test_commit five
+'
+
+test_expect_success 'repack everything into a single packfile' '
+ git repack -a -d --no-write-bitmap-index &&
+
+ head_commit_pack=$(test-tool find-pack HEAD) &&
+ head_tree_pack=$(test-tool find-pack HEAD^{tree}) &&
+ one_pack=$(test-tool find-pack HEAD:one.t) &&
+ three_pack=$(test-tool find-pack HEAD:three.t) &&
+ old_commit_pack=$(test-tool find-pack HEAD~4) &&
+
+ test-tool find-pack --check-count 1 HEAD &&
+ test-tool find-pack --check-count=1 HEAD^{tree} &&
+ ! test-tool find-pack --check-count=0 HEAD:one.t &&
+ ! test-tool find-pack -c 2 HEAD:one.t &&
+ test-tool find-pack -c 1 HEAD:three.t &&
+
+ # Packfile exists at the right path
+ case "$head_commit_pack" in
+ ".git/objects/pack/pack-"*".pack") true ;;
+ *) false ;;
+ esac &&
+ test -f "$head_commit_pack" &&
+
+ # Everything is in the same pack
+ test "$head_commit_pack" = "$head_tree_pack" &&
+ test "$head_commit_pack" = "$one_pack" &&
+ test "$head_commit_pack" = "$three_pack" &&
+ test "$head_commit_pack" = "$old_commit_pack"
+'
+
+test_expect_success 'add more packfiles' '
+ git rev-parse HEAD^{tree} HEAD:two.t HEAD:four.t >objects &&
+ git pack-objects .git/objects/pack/mypackname1 >packhash1 <objects &&
+
+ git rev-parse HEAD~ HEAD~^{tree} HEAD:five.t >objects &&
+ git pack-objects .git/objects/pack/mypackname2 >packhash2 <objects &&
+
+ head_commit_pack=$(test-tool find-pack HEAD) &&
+
+ # HEAD^{tree} is in 2 packfiles
+ test-tool find-pack HEAD^{tree} >head_tree_packs &&
+ grep "$head_commit_pack" head_tree_packs &&
+ grep mypackname1 head_tree_packs &&
+ ! grep mypackname2 head_tree_packs &&
+ test-tool find-pack --check-count 2 HEAD^{tree} &&
+ ! test-tool find-pack --check-count 1 HEAD^{tree} &&
+
+ # HEAD:five.t is also in 2 packfiles
+ test-tool find-pack HEAD:five.t >five_packs &&
+ grep "$head_commit_pack" five_packs &&
+ ! grep mypackname1 five_packs &&
+ grep mypackname2 five_packs &&
+ test-tool find-pack -c 2 HEAD:five.t &&
+ ! test-tool find-pack --check-count=0 HEAD:five.t
+'
+
+test_expect_success 'add more commits (as loose objects)' '
+ test_commit six &&
+ test_commit seven &&
+
+ test -z "$(test-tool find-pack HEAD)" &&
+ test -z "$(test-tool find-pack HEAD:six.t)" &&
+ test-tool find-pack --check-count 0 HEAD &&
+ test-tool find-pack -c 0 HEAD:six.t &&
+ ! test-tool find-pack -c 1 HEAD:seven.t
+'
+
+test_done
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH v5] sequencer: beautify subject of reverts of reverts
From: Kristoffer Haugsbakk @ 2023-09-11 20:12 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Junio C Hamano, Phillip Wood, git
In-Reply-To: <20230902072035.652549-1-oswald.buddenhagen@gmx.de>
On Sat, Sep 2, 2023, at 09:20, Oswald Buddenhagen wrote:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
Congrats on a nice series. It's very “lean and mean”—focused, not
excessive.
And I think I will remember the phrase “too nerdy” for a while. ;)
Maybe we will get this message template the next time we revert a
merge.[1]
> If you merge the updated side branch (with D at its tip), none of the
> changes made in A or B will be in the result, because they were reverted
> by W. That is what Alan saw.
>
> [...]
>
> In such a situation, you would want to first revert the previous revert,
> which would make the history look like this: ...
🔗 1: https://github.com/git/git/blob/master/Documentation/howto/revert-a-faulty-merge.txt
Cheers
--
Kristoffer
^ permalink raw reply
* [PATCH v6 5/9] pack-bitmap-write: rebuild using new bitmap when remapping
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
`git repack` is about to learn a new `--filter=<filter-spec>` option and
we will want to check that this option is incompatible with
`--write-bitmap-index`.
Unfortunately it appears that a test like:
test_expect_success '--filter fails with --write-bitmap-index' '
test_must_fail \
env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
git -C bare.git repack -a -d --write-bitmap-index --filter=blob:none
'
sometimes fail because when rebuilding bitmaps, it appears that we are
reusing existing bitmap information. So instead of detecting that some
objects are missing and erroring out as it should, the
`git repack --write-bitmap-index --filter=...` command succeeds.
Let's fix that by making sure we rebuild bitmaps using new bitmaps
instead of existing ones.
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
pack-bitmap-write.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index f6757c3cbf..f4ecdf8b0e 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -413,15 +413,19 @@ static int fill_bitmap_commit(struct bb_commit *ent,
if (old_bitmap && mapping) {
struct ewah_bitmap *old = bitmap_for_commit(old_bitmap, c);
+ struct bitmap *remapped = bitmap_new();
/*
* If this commit has an old bitmap, then translate that
* bitmap and add its bits to this one. No need to walk
* parents or the tree for this commit.
*/
- if (old && !rebuild_bitmap(mapping, old, ent->bitmap)) {
+ if (old && !rebuild_bitmap(mapping, old, remapped)) {
+ bitmap_or(ent->bitmap, remapped);
+ bitmap_free(remapped);
reused_bitmaps_nr++;
continue;
}
+ bitmap_free(remapped);
}
/*
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH v3 5/6] git-std-lib: introduce git standard library
From: Phillip Wood @ 2023-09-11 13:22 UTC (permalink / raw)
To: Calvin Wan, git; +Cc: nasamuffin, jonathantanmy, linusa, vdye
In-Reply-To: <20230908174443.1027716-5-calvinwan@google.com>
Hi Calvin
On 08/09/2023 18:44, Calvin Wan wrote:
> +ifndef GIT_STD_LIB
> LIB_OBJS += abspath.o
> LIB_OBJS += add-interactive.o
> LIB_OBJS += add-patch.o
> @@ -1196,6 +1198,27 @@ LIB_OBJS += write-or-die.o
> LIB_OBJS += ws.o
> LIB_OBJS += wt-status.o
> LIB_OBJS += xdiff-interface.o
> +else ifdef GIT_STD_LIB
> +LIB_OBJS += abspath.o
> +LIB_OBJS += ctype.o
> +LIB_OBJS += date.o
> +LIB_OBJS += hex-ll.o
> +LIB_OBJS += parse.o
> +LIB_OBJS += strbuf.o
> +LIB_OBJS += usage.o
> +LIB_OBJS += utf8.o
> +LIB_OBJS += wrapper.o
It is still not clear to me how re-using LIB_OBJS like this is
compatible with building libgit.a and git-stb-lib.a in a single make
process c.f. [1].
> +ifdef GIT_STD_LIB
> + BASIC_CFLAGS += -DGIT_STD_LIB
> + BASIC_CFLAGS += -DNO_GETTEXT
As I've said before [2] I think that being able to built git-std-lib.a
with gettext support is a prerequisite for using it to build git (just
like trace2 support is). If we cannot build git using git-std-lib then
the latter is likely to bit rot and so I don't think git-std-lib should
be merged until there is a demonstration of building git using it.
> +### Libified Git rules
> +
> +# git-std-lib
> +# `make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease STUB_PAGER=YesPlease`
> +STD_LIB = git-std-lib.a
> +
> +$(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
> + $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
This is much nicer that the previous version.
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3e7a59b5ff..14bf71c530 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -455,8 +455,8 @@ static inline int noop_core_config(const char *var UNUSED,
> #define platform_core_config noop_core_config
> #endif
>
> +#if !defined(__MINGW32__) && !defined(_MSC_VER) && !defined(GIT_STD_LIB)
> int lstat_cache_aware_rmdir(const char *path);
> -#if !defined(__MINGW32__) && !defined(_MSC_VER)
> #define rmdir lstat_cache_aware_rmdir
> #endif
I thought we'd agreed that this represents a change in behavior that
should be fixed c.f. [2]
> @@ -1462,14 +1464,17 @@ static inline int is_missing_file_error(int errno_)
> return (errno_ == ENOENT || errno_ == ENOTDIR);
> }
>
> +#ifndef GIT_STD_LIB
> int cmd_main(int, const char **);
>
> /*
> * Intercept all calls to exit() and route them to trace2 to
> * optionally emit a message before calling the real exit().
> */
> +
Nit: this blank line seems unnecessary
> int common_exit(const char *file, int line, int code);
> #define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
> +#endif
>
> /*
> * You can mark a stack variable with UNLEAK(var) to avoid it being
> diff --git a/stubs/pager.c b/stubs/pager.c
> diff --git a/stubs/pager.h b/stubs/pager.h
> new file mode 100644
> index 0000000000..b797910881
> --- /dev/null
> +++ b/stubs/pager.h
> @@ -0,0 +1,6 @@
> +#ifndef PAGER_H
> +#define PAGER_H
> +
> +int pager_in_use(void);
> +
> +#endif /* PAGER_H */
Is this file actually used for anything? pager_in_use() is already
declared in pager.h in the project root directory.
> diff --git a/wrapper.c b/wrapper.c
> index 7da15a56da..eeac3741cf 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -5,7 +5,6 @@
> #include "abspath.h"
> #include "parse.h"
> #include "gettext.h"
> -#include "repository.h"
It is probably worth splitting this change out with a commit message
explaining why the include is unneeded.
This is looking good, it would be really nice to see a demonstration of
building git using git-std-lib (with gettext support) in the next iteration.
Best Wishes
Phillip
[1]
https://lore.kernel.org/git/a0f04bd7-3a1e-b303-fd52-eee2af4d38b3@gmail.com/
[2]
https://lore.kernel.org/git/CAFySSZBMng9nEdCkuT5+fc6rfFgaFfU2E0NP3=jUQC1yRcUE6Q@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v3 1/1] range-diff: treat notes like `log`
From: Junio C Hamano @ 2023-09-11 19:55 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King
In-Reply-To: <a37dfb3748e23b4f5081bc9a3c80a5c546101f1d.1694383248.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>> To fix this explicitly set the output format of the internally executed
>> `git log` with `--pretty=medium`. Because that cancels `--notes`, add
>> explicitly `--notes` at the end.
>
> § Authors
>
> • Fix by Johannes
> • Tests by Kristoffer
>
> † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about
> showing notes, 2010-01-20).
>
> Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
OK, Dscho, does this round look acceptable to you?
It feels UGLY to iterate over args _without_ actually parsing them,
at least to me. Such a non-parsing look breaks at least in two ways
over time. (1) a mechanism may be introduced laster, similar to
"--", that allows other_arg->v[] array to mark "here is where the
dashed options end". Now the existing loop keeps reading to the end
and finds "--notes" that is not a dashed option but is part of the
normal command line arguments in "other arg". (2) Among the dashed
options passed in the other_arg->v[], there may be an option that
takes a string value, and a value that happens to be "--notes" is
mistaken as asking for "notes" (iow "git log -G --notes" is looking
for commits with changes that contain "double dash followed by en oh
tee ee es").
I think "git range-diff -G --notes" (or "-S --notes") shows that
this new non-parsing loop is already broken. It looks for a change
that has "--notes" correctly, but at the same time, triggers that
"ah, we have an explicit --notes so drop the implicit_notes_arg
flag" logic.
> range-diff.c | 13 +++++++++++--
> t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index 2e86063491..fbb81a92cc 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -41,12 +41,20 @@ static int read_patches(const char *range, struct string_list *list,
> struct child_process cp = CHILD_PROCESS_INIT;
> struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
> struct patch_util *util = NULL;
> - int in_header = 1;
> + int i, implicit_notes_arg = 1, in_header = 1;
> char *line, *current_filename = NULL;
> ssize_t len;
> size_t size;
> int ret = -1;
>
> + for (i = 0; other_arg && i < other_arg->nr; i++)
> + if (!strcmp(other_arg->v[i], "--notes") ||
> + starts_with(other_arg->v[i], "--notes=") ||
> + !strcmp(other_arg->v[i], "--no-notes")) {
> + implicit_notes_arg = 0;
> + break;
> + }
> +
> strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
> "--reverse", "--date-order", "--decorate=no",
> "--no-prefix", "--submodule=short",
> @@ -60,8 +68,9 @@ static int read_patches(const char *range, struct string_list *list,
> "--output-indicator-context=#",
> "--no-abbrev-commit",
> "--pretty=medium",
> - "--notes",
> NULL);
> + if (implicit_notes_arg)
> + strvec_push(&cp.args, "--notes");
> strvec_push(&cp.args, range);
> if (other_arg)
> strvec_pushv(&cp.args, other_arg->v);
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index b5f4d6a653..b33afa1c6a 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -662,6 +662,20 @@ test_expect_success 'range-diff with multiple --notes' '
> test_cmp expect actual
> '
>
> +# `range-diff` should act like `log` with regards to notes
> +test_expect_success 'range-diff with --notes=custom does not show default notes' '
> + git notes add -m "topic note" topic &&
> + git notes add -m "unmodified note" unmodified &&
> + git notes --ref=custom add -m "topic note" topic &&
> + git notes --ref=custom add -m "unmodified note" unmodified &&
> + test_when_finished git notes remove topic unmodified &&
> + test_when_finished git notes --ref=custom remove topic unmodified &&
> + git range-diff --notes=custom main..topic main..unmodified \
> + >actual &&
> + ! grep "## Notes ##" actual &&
> + grep "## Notes (custom) ##" actual
> +'
> +
> test_expect_success 'format-patch --range-diff does not compare notes by default' '
> git notes add -m "topic note" topic &&
> git notes add -m "unmodified note" unmodified &&
> @@ -679,6 +693,20 @@ test_expect_success 'format-patch --range-diff does not compare notes by default
> ! grep "note" 0000-*
> '
>
> +test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '
> + git notes add -m "topic note" topic &&
> + git notes --ref=custom add -m "topic note (custom)" topic &&
> + git notes add -m "unmodified note" unmodified &&
> + git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
> + test_when_finished git notes remove topic unmodified &&
> + test_when_finished git notes --ref=custom remove topic unmodified &&
> + git format-patch --notes=custom --cover-letter --range-diff=$prev \
> + main..unmodified >actual &&
> + test_when_finished "rm 000?-*" &&
> + grep "## Notes (custom) ##" 0000-* &&
> + ! grep "## Notes ##" 0000-*
> +'
> +
> test_expect_success 'format-patch --range-diff with --no-notes' '
> git notes add -m "topic note" topic &&
> git notes add -m "unmodified note" unmodified &&
^ permalink raw reply
* [PATCH v6 0/9] Repack objects into separate packfiles based on a filter
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder
In-Reply-To: <20230812000011.1227371-1-christian.couder@gmail.com>
# Intro
Last year, John Cai sent 2 versions of a patch series to implement
`git repack --filter=<filter-spec>` and later I sent 4 versions of a
patch series trying to do it a bit differently:
- https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
- https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/
In these patch series, the `--filter=<filter-spec>` removed the
filtered out objects altogether which was considered very dangerous
even though we implemented different safety checks in some of the
latter series.
In some discussions, it was mentioned that such a feature, or a
similar feature in `git gc`, or in a new standalone command (perhaps
called `git prune-filtered`), should put the filtered out objects into
a new packfile instead of deleting them.
Recently there were internal discussions at GitLab about either moving
blobs from inactive repos onto cheaper storage, or moving large blobs
onto cheaper storage. This lead us to rethink at repacking using a
filter, but moving the filtered out objects into a separate packfile
instead of deleting them.
So here is a new patch series doing that while implementing the
`--filter=<filter-spec>` option in `git repack`.
# Use cases for the new feature
This could be useful for example for the following purposes:
1) As a way for servers to save storage costs by for example moving
large blobs, or all the blobs, or all the blobs in inactive
repos, to separate storage (while still making them accessible
using for example the alternates mechanism).
2) As a way to use partial clone on a Git server to offload large
blobs to, for example, an http server, while using multiple
promisor remotes (to be able to access everything) on the client
side. (In this case the packfile that contains the filtered out
object can be manualy removed after checking that all the objects
it contains are available through the promisor remote.)
3) As a way for clients to reclaim some space when they cloned with
a filter to save disk space but then fetched a lot of unwanted
objects (for example when checking out old branches) and now want
to remove these unwanted objects. (In this case they can first
move the packfile that contains filtered out objects to a
separate directory or storage, then check that everything works
well, and then manually remove the packfile after some time.)
As the features and the code are quite different from those in the
previous series, I decided to start a new series instead of continuing
a previous one.
Also since version 2 of this new series, commit messages, don't
mention uses cases like 2) or 3) above, as people have different
opinions on how it should be done. How it should be done could depend
a lot on the way promisor remotes are used, the software and hardware
setups used, etc, so it seems more difficult to "sell" this series by
talking about such use cases. As use case 1) seems simpler and more
appealing, it makes more sense to only talk about it in the commit
messages.
# Changes since version 5
Thanks to Junio who reviewed or commented on versions 1, 2, 3, 4 and
5, and to Taylor who reviewed or commented on version 1, 3, 4 and 5!
Thanks also to Robert Coup who participated in the discussions related
to version 2 and Peff who participated in the discussions related to
version 4. There is only the following code change since version 5:
- Patch 5/9 (pack-bitmap-write: rebuild using new bitmap when
remapping) is new. It fixes a bitmap rebuilding issue that wasn't
triggered previously but got triggered by this series and caused CI
tests to fail. The patch is taken from a suggestion by Taylor in:
https://lore.kernel.org/git/ZNwFlcS3SOS9h77N@nand.local/
I checked that CI tests now passes in:
https://github.com/chriscool/git/actions/runs/6122146278
(There is a failure on 'win test (5)' with "failed: t7527.17
directory changes to a file", but it looks like it's not related to
the previous issue and also not related to to this series at all.)
Another change is that this series has been rebased on top of
94e83dcf5b (The seventh batch, 2023-09-07) to fix a few conflicts
related to changes in the geometry code, as can be seen in the
short range-diff below.
# Commit overview
(No changes in any of the patches compared to version 5, except that
patch 5/9 is new.)
* 1/9 pack-objects: allow `--filter` without `--stdout`
To be able to later repack with a filter we need `git pack-objects`
to write packfiles when it's filtering instead of just writing the
pack without the filtered out objects to stdout.
* 2/9 t/helper: add 'find-pack' test-tool
For testing `git repack --filter=...` that we are going to
implement, it's useful to have a test helper that can tell which
packfiles contain a specific object.
* 3/9 repack: refactor finishing pack-objects command
This is a small refactoring creating a new useful function, so that
`git repack --filter=...` will be able to reuse it.
* 4/9 repack: refactor finding pack prefix
This is another small refactoring creating a small function that
will be reused in the next patch.
* 5/9 pack-bitmap-write: rebuild using new bitmap when remapping
This patch is new in version 6. It fixes an issue when bitmaps are
rebuilt that was revealed by this series, and caused a CI test to
fail.
* 6/9 repack: add `--filter=<filter-spec>` option
This actually adds the `--filter=<filter-spec>` option. It uses one
`git pack-objects` process with the `--filter` option. And then
another `git pack-objects` process with the `--stdin-packs`
option.
* 7/9 gc: add `gc.repackFilter` config option
This is a gc config option so that `git gc` can also repack using a
filter and put the filtered out objects into a separate packfile.
* 8/9 repack: implement `--filter-to` for storing filtered out objects
For some use cases, it's interesting to create the packfile that
contains the filtered out objects into a separate location. This is
similar to the `--expire-to` option for cruft packfiles.
* 9/9 gc: add `gc.repackFilterTo` config option
This allows specifying the location of the packfile that contains
the filtered out objects when using `gc.repackFilter`.
# Range-diff since v5
1: bbcc368876 = 1: da931b5082 pack-objects: allow `--filter` without `--stdout`
2: f1b80e5728 = 2: 10504b3699 t/helper: add 'find-pack' test-tool
3: ffecc73960 ! 3: ee12eb8ad7 repack: refactor finishing pack-objects command
@@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
- struct pack_geometry *geometry = NULL;
+ struct pack_geometry geometry = { 0 };
- struct strbuf line = STRBUF_INIT;
struct tempfile *refs_snapshot = NULL;
int i, ext, ret;
4: 6c2f381a88 = 4: d197e0c370 repack: refactor finding pack prefix
-: ---------- > 5: abeef5fbad pack-bitmap-write: rebuild using new bitmap when remapping
5: 134700c2ce ! 6: 31ca2579d3 repack: add `--filter=<filter-spec>` option
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
+ strvec_pushf(&cmd.args, "--filter=%s",
+ expand_list_objects_filter_spec(&po_args.filter_options));
+
- if (geometry)
+ if (geometry.split_factor)
cmd.in = -1;
else
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
string_list_clear(&existing_nonkept_packs, 0);
string_list_clear(&existing_kept_packs, 0);
- clear_pack_geometry(geometry);
+ free_pack_geometry(&geometry);
+ list_objects_filter_release(&po_args.filter_options);
return ret;
6: d3365c7b48 = 7: fa70ae85f2 gc: add `gc.repackFilter` config option
7: 9a09382cd1 ! 8: e01ea3dd70 repack: implement `--filter-to` for storing filtered out objects
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
+ else if (filter_to)
+ die(_("option '%s' can only be used along with '%s'"), "--filter-to", "--filter");
- if (geometry)
+ if (geometry.split_factor)
cmd.in = -1;
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
}
8: a52e3a71db = 9: d6ff314189 gc: add `gc.repackFilterTo` config option
Christian Couder (9):
pack-objects: allow `--filter` without `--stdout`
t/helper: add 'find-pack' test-tool
repack: refactor finishing pack-objects command
repack: refactor finding pack prefix
pack-bitmap-write: rebuild using new bitmap when remapping
repack: add `--filter=<filter-spec>` option
gc: add `gc.repackFilter` config option
repack: implement `--filter-to` for storing filtered out objects
gc: add `gc.repackFilterTo` config option
Documentation/config/gc.txt | 16 ++
Documentation/git-pack-objects.txt | 4 +-
Documentation/git-repack.txt | 23 +++
Makefile | 1 +
builtin/gc.c | 10 ++
builtin/pack-objects.c | 8 +-
builtin/repack.c | 167 +++++++++++++++------
pack-bitmap-write.c | 6 +-
t/helper/test-find-pack.c | 50 +++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0080-find-pack.sh | 82 ++++++++++
t/t5317-pack-objects-filter-objects.sh | 8 +
t/t6500-gc.sh | 24 +++
t/t7700-repack.sh | 197 +++++++++++++++++++++++++
15 files changed, 547 insertions(+), 51 deletions(-)
create mode 100644 t/helper/test-find-pack.c
create mode 100755 t/t0080-find-pack.sh
--
2.42.0.167.gd6ff314189
^ permalink raw reply
* Re: [ANNOUNCE] Virtual Contributor's Summit 2023
From: Emily Shaffer @ 2023-09-11 20:36 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZMgVMYemepJCqXLs@nand.local>
On Mon, Jul 31, 2023 at 1:10 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Jul 31, 2023 at 10:56:35AM -0700, Emily Shaffer wrote:
> > On Wed, Jul 26, 2023 at 11:00 AM Taylor Blau <me@ttaylorr.com> wrote:
> > > > http://bit.ly/git-contributors-summit-2023
> >
> > Since it's virtual this year, my team is considering hosting a "watch
> > party" in Silicon Valley, to get at least a few folks to be able to
> > meet face to face, even though we can't travel. Basically, we'd book a
> > conference room at some Google office, invite local contributors to
> > come along, and dial into the summit from that room - nothing fancy,
> > no extra programming, maybe catering.
>
> Sounds fun, thanks for organizing. I think my only request would be that
> attendees sign up through the forms linked above, and that we limit
> attendees to folks who meet the criteria (active Git contributor, or
> planning on becoming one).
Now that we have dates (thanks Taylor!) I'm happy to confirm that
Google is indeed able to host a "watch party" at one of our offices in
Sunnyvale (specifics TBD). As Taylor emphasized, we're only able to
invite people who already meet the criteria he named and have signed
up on the main sheet. If you are one of those people and haven't
already gotten in touch with me, please do so, it would be fun to
spend some in person time :)
For those who did already get in touch with me, I'll send details
without the rest of the list shortly.
Thanks!
- Emily
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: René Scharfe @ 2023-09-11 20:11 UTC (permalink / raw)
To: Oleg Nesterov, Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Calvin Wan,
Carlo Marcelo Arenas Belón, Elijah Newren, Jeff King,
Linus Torvalds, Mathias Krause, Taylor Blau, git
In-Reply-To: <20230911121211.GA17401@redhat.com>
Am 11.09.23 um 14:12 schrieb Oleg Nesterov:
> show_funcname_line() returns when "lno <= opt->last_shown" and this
> is not right in that the ->last_shown line (which matched the pattern)
> can also have the actual function name we need to report.
>
> Change this code to check "lno < opt->last_shown". While at it, move
> this check up to avoid the unnecessary "find the previous bol" loop.
>
> Note that --lno can't underflow, lno==0 is not possible in this loop.
>
> Simple test-case:
>
> $ cat TEST.c
> void func(void);
>
> void func1(xxx)
> {
> use1(xxx);
> }
>
> void func2(xxx)
> {
> use2(xxx);
> }
>
> $ git grep --untracked -pn xxx TEST.c
>
> before the patch:
>
> TEST.c=1=void func(void);
> TEST.c:3:void func1(xxx)
> TEST.c:5: use1(xxx);
> TEST.c:8:void func2(xxx)
> TEST.c:10: use2(xxx);
>
> after the patch:
>
> TEST.c=1=void func(void);
> TEST.c:3:void func1(xxx)
> TEST.c=3=void func1(xxx)
> TEST.c:5: use1(xxx);
> TEST.c:8:void func2(xxx)
> TEST.c=8=void func2(xxx)
> TEST.c:10: use2(xxx);
>
> which looks much better to me.
Interesting idea to treat function lines as annotations of matches
instead of as special context. Showing lines twice feels wasteful, but
at least for -p it might be justifiable from that angle. Wouldn't you
have to repeat function line 3 before the match in line 8, though?
The reason for not repeating a matched function line was that it
doesn't add much information under the assumption that it's easy to
identify function lines visually. I assume this still holds, but
perhaps it doesn't for more complicated languages?
I have to admit that I almost never use --show-function, but instead
use the related --function-context a lot. So my practical experience
with the former option is very limited.
> grep.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
The patch would need to update Documentation/git-grep.txt as well to
reflect the changed output.
René
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox