* Re: [PATCH] doc/cat-file: make synopsis and description less confusing
From: Junio C Hamano @ 2023-10-09 20:02 UTC (permalink / raw)
To: Jeff King; +Cc: Štěpán Němec, git, avarab
In-Reply-To: <20231009183352.GA3270793@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Oct 09, 2023 at 07:56:04PM +0200, Štěpán Němec wrote:
>
>> > Yup, that is a good suggestion. Do you want to wrap all of this
>> > discussion up as a patch?
>>
>> Certainly, here it is. I just wanted to hash out some of the details
>> first and found the plain text more fit for that purpose.
>
> This looks good to me. Thanks for taking the time to collect and fix all
> of the issues.
>
> -Peff
Thanks, both. Will take a look.
^ permalink raw reply
* Re: [PATCH v4 0/3] diff-merges: introduce '--dd' option
From: Junio C Hamano @ 2023-10-09 20:02 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Elijah Newren
In-Reply-To: <20231009160535.236523-1-sorganov@gmail.com>
Sergey Organov <sorganov@gmail.com> writes:
> Updates in v4:
>
> * Removed "(which see)" reference from documentation that caused
> confusion.
>
> * Removed explanation why it's --dd and not simply -d from commit
> message.
>
> * Refined --remerge-diff short description according to Junio and
> Elijah comments.
>
> * Added explanation of --dd purpose.
>
> * Fixed style and syntax of "on,m::" description.
Will replace. Thanks.
^ permalink raw reply
* Re: [PATCH 07/15] commit-graph: new filter ver. that fixes murmur3
From: Junio C Hamano @ 2023-10-09 19:52 UTC (permalink / raw)
To: Taylor Blau; +Cc: SZEDER Gábor, Jonathan Tan, git, Jeff King
In-Reply-To: <ZSRVJ3iASRaDGc80@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> However, I am not entirely sure I agree with you that this is a "new"
> issue. At least in the sense that Git (on 'master') does not currently
> know how to deal with Bloom filters that have different settings across
> commit-graph layers.
>
> IOW, you could produce this problem today using the test you wrote in
> <20201015132147.GB24954@szeder.dev> using different values of the
> GIT_BLOOM_SETTINGS environment variables as a proxy for different values
> of the commitGraph.changedPathsVersion configuration variable introduced
> in this series.
>
> So I think that this series makes it easier to fall into that trap, but
> the trap itself is not new. I think a reasonable stopgap (which IIUC you
> have suggested earlier) is to prevent writing a new commit-graph layer
> with a different hash version than the previous layer.
What we probably want more urgently than that stopgap is to perhaps
teach the code pretend as if commit-graph did not exist when we
detect multiple layers use different hash versions (or perhaps only
use the base layer and ignore the rest as an anti-pessimization), to
protect correctness first, no?
^ permalink raw reply
* Re: [PATCH 07/15] commit-graph: new filter ver. that fixes murmur3
From: Taylor Blau @ 2023-10-09 19:31 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Jonathan Tan, git, Junio C Hamano, Jeff King
In-Reply-To: <ZSRD0tK3bk67aDw4@nand.local>
On Mon, Oct 09, 2023 at 02:17:54PM -0400, Taylor Blau wrote:
> On Sun, Oct 08, 2023 at 04:35:23PM +0200, SZEDER Gábor wrote:
> > > Hmm. I am confused -- are you saying that this series breaks existing
> > > functionality, or merely does not patch an existing breakage? I *think*
> > > that it's the latter,
> >
> > It's neither: the new functionality added in this series is broken.
> >
> > > since this test case fails identically on master,
> > > but I am not sure.
> >
> > Not sure what test you are referring to. My test demonstrating the
> > breakage succeeds when adaped to master, because master doesn't
> > understand the commitgraph.changedPathsVersion=2 setting, and keeps
> > writing v1 Bloom filter chunks instead, so all commit-graphs layers
> > contain the same version.
>
> I was referring to the test you sent back in:
>
> https://public-inbox.org/git/20201015132147.GB24954@szeder.dev/
>
> but I think that I should have been looking at the one you sent more
> recently in:
>
> https://lore.kernel.org/git/20230830200218.GA5147@szeder.dev/
OK, I think that I now have my head wrapped around what you're saying.
However, I am not entirely sure I agree with you that this is a "new"
issue. At least in the sense that Git (on 'master') does not currently
know how to deal with Bloom filters that have different settings across
commit-graph layers.
IOW, you could produce this problem today using the test you wrote in
<20201015132147.GB24954@szeder.dev> using different values of the
GIT_BLOOM_SETTINGS environment variables as a proxy for different values
of the commitGraph.changedPathsVersion configuration variable introduced
in this series.
So I think that this series makes it easier to fall into that trap, but
the trap itself is not new. I think a reasonable stopgap (which IIUC you
have suggested earlier) is to prevent writing a new commit-graph layer
with a different hash version than the previous layer.
How does that sound?
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 03/25] documentation: fix typos
From: Junio C Hamano @ 2023-10-09 19:01 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Elijah Newren via GitGitGadget, git, Elijah Newren
In-Reply-To: <92beac82-f0fd-452b-858f-453cdf21b71f@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> On 08/10/2023 07:45, Elijah Newren via GitGitGadget wrote:
> [snip]
>> diff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt
>> index 870e00f2982..42afb953e8c 100644
>> --- a/Documentation/gitformat-pack.txt
>> +++ b/Documentation/gitformat-pack.txt
>> @@ -17,8 +17,8 @@ $GIT_DIR/objects/pack/multi-pack-index
>> DESCRIPTION
>> -----------
>>
>> -The Git pack format is now Git stores most of its primary repository
>> -data. Over the lietime af a repository loose objects (if any) and
>> +The Git pack format is how Git stores most of its primary repository
>> +data. Over the lietime of a repository loose objects (if any) and
>
> Hmm, this tyop jumped out at me while this patch
> floated past... (at least I assume it is a typo!):
>
> s/lietime/lifetime/
>
> ATB,
> Ramsay Jones
Thanks, will squash in.
^ permalink raw reply
* Re: Outreachy Introduction and Interest in Contributing to the Git Community
From: Junio C Hamano @ 2023-10-09 18:54 UTC (permalink / raw)
To: Doreen Wanyama; +Cc: git
In-Reply-To: <CANhBNnu6jbhNqAowBzyfcKDamXtcqCtZpY=QniTGTEJoCrwWOA@mail.gmail.com>
Doreen Wanyama <doreenwanyama20@gmail.com> writes:
> Dear Git community,
>
> I hope you are all doing well. I am writing to show my interest in
> working in the project titled move existing tests to a unit testing
> framework. This is because I have always been intrigued by the work
> the git community does and hence I am interested in being part of
> this. I have gone through the links provided about getting started on
> this. I spent yesterday evening and a better part of today trying to
> understand the resources. As of now I would like to start working on a
> microproject since I understand this is the first step. I am finding
> it difficult though to start. Someone to please help me understand how
> I should go about this or how I should go about finding my first
> microproject. Just a brief explanation will help.
> Thank you in advance.
>
>
> Best regards,
>
> Doreen Wanyama
Do you have access to the mailing list archive?
Visit
https://lore.kernel.org/git/?q=Outreachy+d%3A20230920..
with your browser to see how other applicants are starting their
journey and being helped in finding their microprojects. All the
suggestions they are given equally applies to you.
Welcome to the Git development community.
Have fun.
^ permalink raw reply
* Re: [PATCH v6] merge-tree: add -X strategy option
From: Jeff King @ 2023-10-09 18:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
In-Reply-To: <xmqq4jiz3env.fsf@gitster.g>
On Mon, Oct 09, 2023 at 10:10:28AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I agree that struct-copying is an unusual pattern, and we'd potentially
> > run into problems with duplication. But I think it is even trickier than
> > that here. We also go on to actually _modify_ opt in this function,
> > assigning to various members (both directly, and I think the merge code
> > itself will write to opt->priv).
> >
> > So if we use a pointer (rather than struct assignment), those changes
> > will persist in the merge_options struct that was passed in. Which is
> > also weird.
> >
> > Between the two, I think using a pointer is probably the least-weird.
> > This real_merge() function is only called once, and is a static-local
> > helper for cmd_merge_tree(). So the two functions work as a single unit,
> > and munging "opt" is not a big deal.
>
> It is called once per --stdin input to perform many merges in a row.
> The most obvious "structure to pointer to structure" conversion
> below seems to break an assertion (which is not very surprising, as
> it happens inside that --stdin loop), so I am tempted to revert the
> whole thing for now.
Oops, I totally missed the loop around the call to real_merge(). So
yeah, I think this is rather tricky.
Before Izzy's patch, real_merge() always makes its own fresh
merge_options. After, we have a template merge_options that we copy, but
we are assuming that a shallow struct copy is OK (probably true, but an
anti-pattern that may bite us later). If we add Phillip's suggestion on
top, then we do not copy at all, and end up reusing the same options
struct (which is definitely wrong).
I don't think there are any bugs with the state at the current tip of
ty/merge-tree-strategy-options, but if we want to make it safer, I think
we have two options:
- delay the conversion of the "xopts" list into merge_options until we
initialize it in real_merge(). This avoids breaking abstraction
boundaries, but does mean that the sanity-checking of the options
happens a little later (but not much in practice).
- provide a copy_merge_options() function, which makes this kind of
"set up a template and then copy it" pattern official. It can be a
struct assignment for now, but it at least alerts anybody adding new
options to the notion that a deep copy might be required.
Option 1 looks something like this (a lot of the hunks are just
reverting the tip of that branch, so squashed in it would be even
smaller):
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 7024b5ce2e..f9dbbdb867 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -415,7 +415,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
- struct merge_options merge_options;
+ struct strvec xopts;
};
static int real_merge(struct merge_tree_options *o,
@@ -425,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
{
struct commit *parent1, *parent2;
struct commit_list *merge_bases = NULL;
- struct merge_options opt = o->merge_options;
+ struct merge_options opt;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
@@ -439,11 +439,17 @@ static int real_merge(struct merge_tree_options *o,
help_unknown_ref(branch2, "merge-tree",
_("not something we can merge"));
+ init_merge_options(&opt, the_repository);
+
opt.show_rename_progress = 0;
opt.branch1 = branch1;
opt.branch2 = branch2;
+ for (size_t x = 0; x < o->xopts.nr; x++)
+ if (parse_merge_opt(&opt, o->xopts.v[x]))
+ die(_("unknown strategy option: -X%s"), o->xopts.v[x]);
+
if (merge_base) {
struct commit *base_commit;
struct tree *base_tree, *parent1_tree, *parent2_tree;
@@ -512,8 +518,7 @@ static int real_merge(struct merge_tree_options *o,
int cmd_merge_tree(int argc, const char **argv, const char *prefix)
{
- struct merge_tree_options o = { .show_messages = -1 };
- struct strvec xopts = STRVEC_INIT;
+ struct merge_tree_options o = { .show_messages = -1,.xopts = STRVEC_INIT };
int expected_remaining_argc;
int original_argc;
const char *merge_base = NULL;
@@ -549,24 +554,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
- OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ OPT_STRVEC('X', "strategy-option", &o.xopts, N_("option=value"),
N_("option for selected merge strategy")),
OPT_END()
};
- /* Init merge options */
- init_merge_options(&o.merge_options, the_repository);
-
/* Parse arguments */
original_argc = argc - 1; /* ignoring argv[0] */
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
- if (xopts.nr && o.mode == MODE_TRIVIAL)
+ if (o.xopts.nr && o.mode == MODE_TRIVIAL)
die(_("--trivial-merge is incompatible with all other options"));
- for (int x = 0; x < xopts.nr; x++)
- if (parse_merge_opt(&o.merge_options, xopts.v[x]))
- die(_("unknown strategy option: -X%s"), xopts.v[x]);
/* Handle --stdin */
if (o.use_stdin) {
^ permalink raw reply related
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Junio C Hamano @ 2023-10-09 18:49 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <20231009011546.509-1-naomi.ibeh69@gmail.com>
Naomi Ibe <naomi.ibeh69@gmail.com> writes:
> Subject: Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using d
Subject: [OUTREACHY][PATCH 1/1] builtin/add.c: clean up die() messages
> From: Naomi <naomi.ibeh69@gmail.com>
The name and address on this line come from your commit object,
which in turn would have came from your configuration. You have
[user]
name = Naomi
email = naomi.ibeh69@gmail.com
somewhere in your configuration file, perhaps in $HOME/.gitconfig or
somewhere. When contributiong to this project, you want that "name"
line to also include your family name, as it should match what you
write on your Signed-off-by: line. A focused way to do so without
affecting your author identity for other projects is to add
[user]
name = Naomi Ibe
in .git/config of the repository that you use to contribute to this
project, e.g.,
$ cd ... to the working tree of your clone of git you work in ...
$ git config user.name "Naomi Ibe"
The space above your Sign off is to fill the details you omitted on
the title of the message (which would say something about "conform
to guidelines" or "clean up" without mentioning what rule the
original violates), making the whole thing something like:
builtin/add.c: clean up die() messages
As described in the CodingGuidelines document, a single line
message given to die() and its friends should not capitalize its
first word, and should not add full-stop at the end.
Signed-off-by: ...
> Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> ---
> builtin/add.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Thanks. Otherwise the patch looks good.
>
> diff --git a/builtin/add.c b/builtin/add.c
> index c27254a5cd..5126d2ede3 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
> git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>
> if (repo_read_index(the_repository) < 0)
> - die(_("Could not read the index"));
> + die(_("could not read the index"));
>
> repo_init_revisions(the_repository, &rev, prefix);
> rev.diffopt.context = 7;
> @@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
> die(_("editing patch failed"));
>
> if (stat(file, &st))
> - die_errno(_("Could not stat '%s'"), file);
> + die_errno(_("could not stat '%s'"), file);
> if (!st.st_size)
> - die(_("Empty patch. Aborted."));
> + die(_("empty patch. aborted"));
>
> child.git_cmd = 1;
> strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
> NULL);
> if (run_command(&child))
> - die(_("Could not apply '%s'"), file);
> + die(_("could not apply '%s'"), file);
>
> unlink(file);
> free(file);
> @@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> finish:
> if (write_locked_index(&the_index, &lock_file,
> COMMIT_LOCK | SKIP_IF_UNCHANGED))
> - die(_("Unable to write new index file"));
> + die(_("unable to write new index file"));
>
> dir_clear(&dir);
> clear_pathspec(&pathspec);
^ permalink raw reply
* Re: [PATCH] doc/cat-file: make synopsis and description less confusing
From: Jeff King @ 2023-10-09 18:33 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git, avarab
In-Reply-To: <20231009175604.2361700-1-stepnem@smrk.net>
On Mon, Oct 09, 2023 at 07:56:04PM +0200, Štěpán Němec wrote:
> > Yup, that is a good suggestion. Do you want to wrap all of this
> > discussion up as a patch?
>
> Certainly, here it is. I just wanted to hash out some of the details
> first and found the plain text more fit for that purpose.
This looks good to me. Thanks for taking the time to collect and fix all
of the issues.
-Peff
^ permalink raw reply
* [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Doreen Wanyama @ 2023-10-09 18:31 UTC (permalink / raw)
To: git
Dear Git community,
I hope you are all doing well. I am writing to show my interest in
working in the project titled move existing tests to a unit testing
framework. This is because I have always been intrigued by the work
the git community does and hence I am interested in being part of
this. I have gone through the links provided about getting started on
this. I spent yesterday evening and a better part of today trying to
understand the resources. As of now I would like to start working on a
microproject since I understand this is the first step. I am finding
it difficult though to start. Someone to please help me understand how
I should go about this or how I should go about finding my first
microproject. Just a brief explanation will help.
Thank you in advance.
Best regards,
Doreen Wanyama
^ permalink raw reply
* Re: [PATCH] pretty: fix ref filtering for %(decorate) formats
From: Junio C Hamano @ 2023-10-09 18:24 UTC (permalink / raw)
To: Andy Koppe; +Cc: git
In-Reply-To: <20231008202307.1568477-1-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
> Mark pretty formats containing "%(decorate" as requiring decoration in
> userformat_find_requirements(), same as "%d" and "%D".
Ah, of course. The patch makes sense.
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 16626e4fe9..5aabc9f7d8 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -590,9 +590,9 @@ test_expect_success 'pretty format %decorate' '
> git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
> test_cmp expect2 actual2 &&
>
> - echo "[ HEAD -> foo; tag: bar; qux ]" >expect3 &&
> - git log --format="%(decorate:prefix=[ ,suffix= ],separator=%x3B )" \
> - -1 >actual3 &&
> + echo "[ bar; qux; foo ]" >expect3 &&
> + git log --format="%(decorate:prefix=[ ,suffix= ],separator=%x3B ,tag=)" \
> + --decorate-refs=refs/ -1 >actual3 &&
> test_cmp expect3 actual3 &&
The original test shares the same, but is the order of multiple
decorations expected to be stable? I feel a bit uneasy to see a
test that insists multiple things come out in a hardcoded order.
It is not making anything _worse_, so let's take the patch as-is.
Thanks.
> # Try with a typo (in "separator"), in which case the placeholder should
^ permalink raw reply
* Re: [PATCH 07/15] commit-graph: new filter ver. that fixes murmur3
From: Taylor Blau @ 2023-10-09 18:17 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Jonathan Tan, git, Junio C Hamano, Jeff King
In-Reply-To: <20231008143523.GA18858@szeder.dev>
On Sun, Oct 08, 2023 at 04:35:23PM +0200, SZEDER Gábor wrote:
> > Hmm. I am confused -- are you saying that this series breaks existing
> > functionality, or merely does not patch an existing breakage? I *think*
> > that it's the latter,
>
> It's neither: the new functionality added in this series is broken.
>
> > since this test case fails identically on master,
> > but I am not sure.
>
> Not sure what test you are referring to. My test demonstrating the
> breakage succeeds when adaped to master, because master doesn't
> understand the commitgraph.changedPathsVersion=2 setting, and keeps
> writing v1 Bloom filter chunks instead, so all commit-graphs layers
> contain the same version.
I was referring to the test you sent back in:
https://public-inbox.org/git/20201015132147.GB24954@szeder.dev/
but I think that I should have been looking at the one you sent more
recently in:
https://lore.kernel.org/git/20230830200218.GA5147@szeder.dev/
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/4] ref-cache.c: fix prefix matching in ref iteration
From: Junio C Hamano @ 2023-10-09 18:15 UTC (permalink / raw)
To: Victoria Dye; +Cc: Patrick Steinhardt, Victoria Dye via GitGitGadget, git
In-Reply-To: <3585d72f-9f06-d190-ad5a-bec6db3f647f@github.com>
Victoria Dye <vdye@github.com> writes:
> I originally operated on the assumption that it was the first case, which is
> why I didn't include a test in this patch. Commands like 'for-each-ref',
> 'show-ref', etc. either use an empty prefix or a directory prefix with a
> trailing slash, which won't trigger this issue.
Ah, yes, I didn't mention it but I suspected as such (i.e. the code
is structured in such a way that this broken implementation does not
matter to the current callers).
> I encountered the problem
> while working on a builtin that filtered refs by a user-specified prefix -
> the results included refs that should not have been matched, which led me to
> this fix.
OK, perfectly understandable.
> Scanning through the codebase again, though, I do see a way to replicate the
> issue:
>
> $ git update-ref refs/bisect/b HEAD
> $ git rev-parse --abbrev-ref --bisect
> refs/bisect/b
>
> Because 'rev-parse --bisect' uses the "refs/bisect/bad" prefix (no trailing
> slash) and does no additional filtering in its 'for_each_fullref_in'
> callback, refs like "refs/bisect/b" and "refs/bisect/ba" are (incorrectly)
> matched. I'll re-roll with the added test.
Good find. Thanks!
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: Taylor Blau @ 2023-10-09 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqfs2j1xdo.fsf@gitster.g>
On Mon, Oct 09, 2023 at 11:09:07AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Sat, Oct 07, 2023 at 01:20:02AM -0700, Junio C Hamano wrote:
> >> * tb/repack-max-cruft-size (2023-10-05) 4 commits
> >> (merged to 'next' on 2023-10-06 at b3ca6df3b9)
> >> + builtin/repack.c: avoid making cruft packs preferred
> >> + builtin/repack.c: implement support for `--max-cruft-size`
> >> + builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
> >> + t7700: split cruft-related tests to t7704
> >>
> >> "git repack" learned "--max-cruft-size" to prevent cruft packs from
> >> growing without bounds.
> >>
> >> Will merge to 'master'.
> >> source: <cover.1696293862.git.me@ttaylorr.com>
> >> source: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
> >
> > Thanks. On a semi-related note, did you want to pick up my patch in
> >
> > https://lore.kernel.org/git/035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com/
> >
> > ? That should address a performance bug that occurs when a repository
> > (incorrectly) chooses a cruft pack as its preferred pack source when
> > writing a MIDX bitmap, significantly impeding the pack-reuse mechanism.
>
> Isn't that in the above list already as b3ca6df3b9^2?
Oops, duh. I hadn't expected you to group that patch in with the rest of
tb/repack-max-cruft-size.
I'll put on my glasses the next time before suggesting you dropped one
of my patches on the floor... ;-)
Thanks,
Taylor
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: Junio C Hamano @ 2023-10-09 18:09 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZSQnVnK0k3bdk5zX@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Sat, Oct 07, 2023 at 01:20:02AM -0700, Junio C Hamano wrote:
>> * tb/repack-max-cruft-size (2023-10-05) 4 commits
>> (merged to 'next' on 2023-10-06 at b3ca6df3b9)
>> + builtin/repack.c: avoid making cruft packs preferred
>> + builtin/repack.c: implement support for `--max-cruft-size`
>> + builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
>> + t7700: split cruft-related tests to t7704
>>
>> "git repack" learned "--max-cruft-size" to prevent cruft packs from
>> growing without bounds.
>>
>> Will merge to 'master'.
>> source: <cover.1696293862.git.me@ttaylorr.com>
>> source: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
>
> Thanks. On a semi-related note, did you want to pick up my patch in
>
> https://lore.kernel.org/git/035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com/
>
> ? That should address a performance bug that occurs when a repository
> (incorrectly) chooses a cruft pack as its preferred pack source when
> writing a MIDX bitmap, significantly impeding the pack-reuse mechanism.
Isn't that in the above list already as b3ca6df3b9^2?
^ permalink raw reply
* [PATCH] doc/cat-file: make synopsis and description less confusing
From: Štěpán Němec @ 2023-10-09 17:56 UTC (permalink / raw)
To: peff; +Cc: git, avarab
In-Reply-To: <20231009155603.GA3251575@coredump.intra.peff.net>
The DESCRIPTION's "first form" is actually the 1st, 2nd, 3rd and 5th
form in SYNOPSIS, the "second form" is the 4th one.
Interestingly, this state of affairs was introduced in
97fe7250753b (cat-file docs: fix SYNOPSIS and "-h" output, 2021-12-28)
with the claim of "Now the two will match again." ("the two" being
DESCRIPTION and SYNOPSIS)...
The description also suffers from other correctness and clarity issues,
e.g., the "first form" paragraph discusses -p, -s and -t, but leaves out
-e, which is included in the corresponding SYNOPSIS section; the second
paragraph mentions <format>, which doesn't occur in SYNOPSIS at all, and
of the three batch options, really only describes the behavior of
--batch-check. Also the mention of "drivers" seems an implementation
detail not adding much clarity in a short summary (and isn't expanded
upon in the rest of the man page, either).
Rather than trying to maintain one-to-one (or N-to-M) correspondence
between the DESCRIPTION and SYNOPSIS forms, creating duplication and
providing opportunities for error, shorten the former into a concise
summary describing the two general modes of operation: batch and
non-batch, leaving details to the subsequent manual sections.
While here, fix a grammar error in the description of -e and make the
following further minor improvements:
NAME:
shorten ("content or type and size" isn't the whole story; say
"details" and leave the actual details to later sections)
SYNOPSIS and --help:
move the (--textconv | --filters) form before --batch, closer
to the other non-batch forms
Signed-off-by: Štěpán Němec <stepnem@smrk.net>
---
On Mon, 09 Oct 2023 11:56:03 -0400
Jeff King wrote:
> On Mon, Oct 09, 2023 at 10:36:51AM +0200, Štěpán Němec wrote:
>
>> > Ah, true, I was thinking that the DESCRIPTION section would be the first
>> > thing users would read, but I didn't notice the headline. I agree that
>> > what it says is probably sufficient (though arguably "type and size" is
>> > slightly inaccurate there; I said "details" in my proposed text but
>> > maybe that is too vague).
>>
>> We could also leave the NAME vague(r) and put an additional sentence at
>> the beginning of DESCRIPTION:
>
> Yup, that is a good suggestion. Do you want to wrap all of this
> discussion up as a patch?
Certainly, here it is. I just wanted to hash out some of the details
first and found the plain text more fit for that purpose.
Thanks.
To anyone not following the whole thread: this is a slightly more
ambitious version of [1] ("[PATCH] doc/cat-file: clarify description
regarding various command forms"), intended to supersede it.
[1]
https://lore.kernel.org/git/20231003082513.3003520-1-stepnem@smrk.net/
Documentation/git-cat-file.txt | 30 ++++++++++++++----------------
builtin/cat-file.c | 4 ++--
2 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 0e4936d18263..bd95a6c10a7d 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -3,8 +3,7 @@ git-cat-file(1)
NAME
----
-git-cat-file - Provide content or type and size information for repository objects
-
+git-cat-file - Provide contents or details of repository objects
SYNOPSIS
--------
@@ -12,25 +11,24 @@ SYNOPSIS
'git cat-file' <type> <object>
'git cat-file' (-e | -p) <object>
'git cat-file' (-t | -s) [--allow-unknown-type] <object>
+'git cat-file' (--textconv | --filters)
+ [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]
'git cat-file' (--batch | --batch-check | --batch-command) [--batch-all-objects]
[--buffer] [--follow-symlinks] [--unordered]
[--textconv | --filters] [-Z]
-'git cat-file' (--textconv | --filters)
- [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]
DESCRIPTION
-----------
-In its first form, the command provides the content or the type of an object in
-the repository. The type is required unless `-t` or `-p` is used to find the
-object type, or `-s` is used to find the object size, or `--textconv` or
-`--filters` is used (which imply type "blob").
+Output the contents or other properties such as size, type or delta
+information of one or more objects.
-In the second form, a list of objects (separated by linefeeds) is provided on
-stdin, and the SHA-1, type, and size of each object is printed on stdout. The
-output format can be overridden using the optional `<format>` argument. If
-either `--textconv` or `--filters` was specified, the input is expected to
-list the object names followed by the path name, separated by a single
-whitespace, so that the appropriate drivers can be determined.
+This command can operate in two modes, depending on whether an option
+from the `--batch` family is specified.
+
+In non-batch mode, the command provides information on an object
+named on the command line.
+
+In batch mode, arguments are read from standard input.
OPTIONS
-------
@@ -51,8 +49,8 @@ OPTIONS
-e::
Exit with zero status if `<object>` exists and is a valid
- object. If `<object>` is of an invalid format exit with non-zero and
- emits an error on stderr.
+ object. If `<object>` is of an invalid format, exit with non-zero
+ status and emit an error on stderr.
-p::
Pretty-print the contents of `<object>` based on its type.
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 694c8538df2f..ea8ad601ecc0 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -922,11 +922,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
N_("git cat-file <type> <object>"),
N_("git cat-file (-e | -p) <object>"),
N_("git cat-file (-t | -s) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--textconv | --filters)\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
N_("git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\n"
" [--buffer] [--follow-symlinks] [--unordered]\n"
" [--textconv | --filters] [-Z]"),
- N_("git cat-file (--textconv | --filters)\n"
- " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
NULL
};
const struct option options[] = {
base-commit: 3a06386e314565108ad56a9bdb8f7b80ac52fb69
--
2.42.0
^ permalink raw reply related
* Outreachy Introduction and Interest in Contributing to the Git Community
From: Doreen Wanyama @ 2023-10-09 17:38 UTC (permalink / raw)
To: git
Dear Git community,
I hope you are all doing well. I am writing to show my interest in
working in the project titled move existing tests to a unit testing
framework. This is because I have always been intrigued by the work
the git community does and hence I am interested in being part of
this. I have gone through the links provided about getting started on
this. I spent yesterday evening and a better part of today trying to
understand the resources. As of now I would like to start working on a
microproject since I understand this is the first step. I am finding
it difficult though to start. Someone to please help me understand how
I should go about this or how I should go about finding my first
microproject. Just a brief explanation will help.
Thank you in advance.
Best regards,
Doreen Wanyama
^ permalink raw reply
* Re: [PATCH v7 2/3] unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-10-09 17:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: phillip.wood123, git, linusa, calvinwan, rsbecker
In-Reply-To: <xmqq350hw6n7.fsf@gitster.g>
On 2023.08.17 17:12, Junio C Hamano wrote:
>
> What I am going to utter from here on are not complaints but purely
> meant as questions.
>
> Would the resulting output and maintainability of the tests change
> (improve, or worsen) if we introduce
>
> static void assert_empty_strbuf(struct strbuf *buf)
> {
> check_uint(buf->len, ==, 0);
> check_uint(buf->alloc, ==, 0);
> check(buf.buf == strbuf_slopbuf);
> check_char(buf.buf[0], ==, '\0');
> }
>
> and call it from the setup() function to ensure that
> strbuf_release(&buf) it calls after running customer test f() brings
> the buffer in a reasonably initialized state? The t_static_init()
> test should be able to say
>
> static void t_static_init(void)
> {
> struct strbuf buf = STRBUF_INIT;
> assert_empty_strbuf(&buf);
> }
>
> if we did so, but is that a good thing or a bad thing (e.g. it may
> make it harder to figure out where the real error came from, because
> of the "line number" thing will not easily capture the caller of the
> caller, perhaps)?
I am unsure whether or not this is an improvement. While it would
certainly help readability and reduce duplication if this were
production code, in test code it can often be more valuable to be
verbose and explicit, so that individual broken test cases can be
quickly understood without having to do a lot of cross referencing.
I'll hold off on adding any more utility functions in t-strbuf for V8,
but if you or other folks feel strongly about it we can address it in
V9.
> > + check_char(buf.buf[0], ==, '\0');
> > +}
>
> > +static void t_dynamic_init(void)
> > +{
> > + struct strbuf buf;
> > +
> > + strbuf_init(&buf, 1024);
> > + check_uint(buf.len, ==, 0);
> > + check_uint(buf.alloc, >=, 1024);
> > + check_char(buf.buf[0], ==, '\0');
>
> Is it sensible to check buf.buf is not slopbuf at this point, or
> does it make the test TOO intimate with the current implementation
> detail?
Yes, I think this is too much of an internal detail. None of the users
of strbuf ever reference it directly. Presumably for library-ish code,
we should stick to testing just the user-observable parts, not the
implementation.
> > + check_char(buf->buf[0], ==, ch);
> > + check_char(buf->buf[1], ==, '\0');
> > +}
>
> In any case, this t_addch() REQUIRES that incoming buf is empty,
> doesn't it? I do not think it is sensible. I would have expected
> that it would be more like
>
> t_addch(struct strbuf *buf, void *data)
> {
> char ch = *(char *)data;
> size_t orig_alloc = buf->alloc;
> size_t orig_len = buf->len;
>
> if (!assert_sane_strbuf(buf))
> return;
> strbuf_addch(buf, ch);
> if (!assert_sane_strbuf(buf))
> return;
> check_uint(buf->len, ==, orig_len + 1);
> check_uint(buf->alloc, >=, orig_alloc);
> check_char(buf->buf[buf->len - 1], ==, ch);
> check_char(buf->buf[buf->len], ==, '\0');
> }
>
> to ensure that we can add a ch to a strbuf with any existing
> contents and get a one-byte longer contents than before, with the
> last byte of the buffer becoming 'ch' and still NUL terminated.
>
> And we protect ourselves with a helper that checks if the given
> strbuf looks *sane*.
Yeah, in general I think this is a good improvement, but again I'm not
sure if it's worth adding additional helpers. I'll try to rework this a
bit in V8.
> static int assert_sane_strbuf(struct strbuf *buf)
> {
> /* can use slopbuf only when the length is 0 */
> if (buf->buf == strbuf_slopbuf)
> return (buf->len == 0);
> /* everybody else must have non-NULL buffer */
> if (buf->buf == NULL)
> return 0;
> /*
> * alloc must be at least 1 byte larger than len
> * for the terminating NUL at the end.
> */
> return ((buf->len + 1 <= buf->alloc) &&
> (buf->buf[buf->len] == '\0'));
> }
>
> You can obviously use your check_foo() for the individual checks
> done in this function to get a more detailed diagnosis, but because
> I have confused myself enough by thinking about their polarity, I
> wrote this in barebones comparison instead.
>
^ permalink raw reply
* Re: [PATCH] repack: free existing_cruft array after use
From: Junio C Hamano @ 2023-10-09 17:28 UTC (permalink / raw)
To: Taylor Blau; +Cc: Jeff King, git, Patrick Steinhardt, Eric Sunshine
In-Reply-To: <ZSNWZ9+Q2eOpy91A@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote:
>> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:
>> ...
>> Coverity (using the just-merged-to-next version of the workflow file!)
>> flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in
>> 'next', I think we'd want this on top:
>
> Woohoo! I'm glad that this is already paying dividends.
> ...
> Thanks, I can't believe I missed this when writing this function. The
> fix looks obviously correct to me, so this has my:
>
> Acked-by: Taylor Blau <me@ttaylorr.com>
>
> Thanks,
Will queue. Thanks.
^ permalink raw reply
* Re: [RFC PATCH] Not computing changed path filter for root commits
From: Taylor Blau @ 2023-10-09 17:26 UTC (permalink / raw)
To: Jonathan Tan; +Cc: SZEDER Gábor, git
In-Reply-To: <ZSQ2XwbTM4DDLfJq@nand.local>
On Mon, Oct 09, 2023 at 01:20:31PM -0400, Taylor Blau wrote:
> On Mon, Oct 02, 2023 at 03:55:46PM -0700, Jonathan Tan wrote:
> > As for the second issue, we can probably solve this by being defensive
> > in rev_same_tree_as_empty() by only using the Bloom filter when the
> > commit has no parents. Not sure if this is being overly defensive,
> > though.
>
> I am also unsure whether we are being overly defensive here or not. But
> I agree that it does feel safer to apply something like:
Never mind, we are being overly defensive there. The goal here is to
avoid using a commit's Bloom filter in cases where we are acting as if a
commit is at the root of history, but in fact has parents.
This only happens when we return REV_TREE_NEW from a call to
`rev_compare_tree(revs, p, commit, nth_parent)`. But we'll only get
REV_TREE_NEW back if
repo_get_commit_tree(the_repository, p);
returns NULL. But when we call rev_same_tree_as_empty(revs, p) in the
REV_TREE_NEW case, we return early as follows:
struct tree *t1 = repo_get_commit_tree(revs, p);
if (!t1)
return 0;
So we won't even consult the Bloom filter in that case, since t1 is NULL
for the same reason as what caused rev_compare_tree() to return
REV_TREE_NEW in the first place.
I am still dumbfounded by how we would ever get REV_TREE_NEW in the
first place, but if we did, I think we would be OK here.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Junio C Hamano @ 2023-10-09 17:24 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, Elijah Newren, git, Eric W. Biederman
In-Reply-To: <20231008173329.GA1557002@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Yes, the bulk-checkin mechanism suffers from an even worse problem which
>> is the pack it creates will contain no deltas whatsoever. The contents
>> of the pack are just getting written as-is, so there's no fancy
>> delta-ficiation going on.
>
> I wonder how big a deal this would be in practice for merges.
> ...
Thanks for your experiments ;-)
The reason why bulk-checkin mechanism does not attempt deltifying
(as opposed to fast-import that attempts to deltify with the
immediately previous object and only with that single object) is
exactly the same. It was done to support the initial check-in,
which by definition lacks the delta opportunity along the time axis.
As you describe, such a delta-less pack would risk missed
deltification opportunity when running a repack (without "-f"), as
the opposite of the well known "reuse delta" heuristics, aka "this
object was stored in the base form, it is likely that the previous
pack-object tried but did not find a good delta base for it, let's
not waste time retrying that" heuristics would get in the way.
^ permalink raw reply
* RE: [EXTERNAL] Re: Microsoft Smart App Control - Git - git-bash.exe File Unsigned
From: Rolland Swing (Insight Global LLC) @ 2023-10-09 17:21 UTC (permalink / raw)
To: brian m. carlson; +Cc: git@vger.kernel.org, Anthony Chuang
In-Reply-To: <ZSCvaWuPJ1peZ3KF@tapette.crustytoothpaste.net>
Thanks Brian - I'll reach out to them via their issue tracker.
Thanks,
Rolland
-----Original Message-----
From: brian m. carlson <sandals@crustytoothpaste.net>
Sent: Friday, October 6, 2023 6:08 PM
To: Rolland Swing (Insight Global LLC) <v-roswing@microsoft.com>
Cc: git@vger.kernel.org; Anthony Chuang <anchuang@microsoft.com>
Subject: [EXTERNAL] Re: Microsoft Smart App Control - Git - git-bash.exe File Unsigned
On 2023-10-05 at 20:41:39, Rolland Swing (Insight Global LLC) wrote:
> Hi Git Team,
Hey,
> We're part of the Microsoft team that owns Smart App Control (https://learn.microsoft.com/en-us/windows/apps/develop/smart-app-control/overview), which requires applications to sign all of their executable files (exe, dll, msi, tmp, and a few other file formats).
>
> We found during internal testing and/or from user feedback that your app, git-bash.exe, is not correctly signed.
>
> Block Event: FileName: \Device\HarddiskVolume7\Program
> Files\Git\git-bash.exe
> Calling Process: \Device\HarddiskVolume7\Windows\explorer.exe
> Sha256 Hash:
> 42F2E685686FB6356A195709AF912C7B9D424466BD7C6D69258AADA5E80AC3C2
The Git project doesn't distribute any binaries at all. We distribute only source code. Many distributors compile these to produce binaries.
The project you are probably thinking of is Git for Windows, which, while related, is a separate project. They do indeed distribute binaries, and this looks like a binary that's theirs. If you'd like to contact them, you can use their issue tracker
(https://github.com/git-for-windows/git/issues) to inquire.
However, I will note that a cursory search there found https://github.com/git-for-windows/git/issues/798, where the maintainer points out that there are over 400 exe files and 250 dll files, which would make signing them all excessively burdensome. I expect the upcoming requirements for HSM-backed keys for Windows code signing may make that even slower and more burdensome. That being said, perhaps with automation, the maintainer may feel differently than they did in 2016, so it might be worth asking again.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
^ permalink raw reply
* Re: [RFC PATCH] Not computing changed path filter for root commits
From: Taylor Blau @ 2023-10-09 17:20 UTC (permalink / raw)
To: Jonathan Tan; +Cc: SZEDER Gábor, git
In-Reply-To: <20231002225546.409837-1-jonathantanmy@google.com>
On Mon, Oct 02, 2023 at 03:55:46PM -0700, Jonathan Tan wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > diff --git a/revision.c b/revision.c
> > index 2f4c53ea20..1d36df49e2 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -837,14 +837,24 @@ static int rev_compare_tree(struct rev_info *revs,
> > static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
> > {
> > struct tree *t1 = repo_get_commit_tree(the_repository, commit);
> > + int bloom_ret = 1;
> >
> > if (!t1)
> > return 0;
> >
> > + if (revs->bloom_keys_nr) {
> > + bloom_ret = check_maybe_different_in_bloom_filter(revs, commit);
> > + if (!bloom_ret)
> > + return 1;
> > + }
> > +
> > tree_difference = REV_TREE_SAME;
> > revs->pruning.flags.has_changes = 0;
> > diff_tree_oid(NULL, &t1->object.oid, "", &revs->pruning);
> >
> > + if (bloom_ret == 1 && tree_difference == REV_TREE_SAME)
> > + count_bloom_filter_false_positive++;
> > +
> > return tree_difference == REV_TREE_SAME;
> > }
>
> I'll concentrate on getting this patch in, and will look at (and
> discuss) the other Bloom filter-related emails later.
Sounds good. I know that I have some pending mail from SZEDER as well,
so I'll try and apply any feedback from both of you before sending a
reroll so that we can get this all done in one shot.
> This looks good, possibly except a code path in try_to_simplify_commit()
> that calls this rev_same_tree_as_empty() function when
> rev_compare_tree() between a commit and its parent returns REV_TREE_NEW.
> So there are 2 issues: How can rev_compare_tree() ever return
> REV_TREE_NEW? And it doesn't seem right to check Bloom filters in this
> code path, since rev_same_tree_as_empty() was invoked here while we are
> enumerating through a commit's parents, which necessarily implies that
> the commit has parents, but here we're using the Bloom filter as if the
> commit is known to have no parents.
> As for the first issue, rev_compare_tree() returns REV_TREE_NEW when the
> parent's tree is NULL. I'm not sure how this can happen - the tree can
> be NULL if the parent commit is not parsed, but at this point I think
> that it has been parsed. And I think every commit has a tree. This goes
> back all the way to 3a5e860815 (revision: make tree comparison functions
> take commits rather than trees, 2008-11-03) and even beyond that (I
> didn't dig further).
The more I think about this, the more confused I become ;-). I was able
to track this all the way back to Linus's 461cf59f89 (rev-list: stop
when the file disappears, 2006-01-18), but am convinced that both of
these cases (we have an analogous one for when t2 is NULL and we return
REV_TREE_OLD) are dead code.
I replaced the "if (!t1) return REV_TREE_NEW;" with "if (!t1)
BUG("oops")", and was able to get the whole test suite to pass. So... I
am pretty sure that this is dead code, but not sure enough to remove it
myself ;-).
To your point about seeing the tree as NULL before parsing the parent, I
don't think that is the case here, since we parse the parent immediately
before calling rev_compare_tree() (and indeed Git will refuse to read
commit objects which do not list a tree).
> As for the second issue, we can probably solve this by being defensive
> in rev_same_tree_as_empty() by only using the Bloom filter when the
> commit has no parents. Not sure if this is being overly defensive,
> though.
I am also unsure whether we are being overly defensive here or not. But
I agree that it does feel safer to apply something like:
--- 8< ---
diff --git a/revision.c b/revision.c
index 3d78ea6a9a..21b3085465 100644
--- a/revision.c
+++ b/revision.c
@@ -834,7 +834,8 @@ static int rev_compare_tree(struct rev_info *revs,
return tree_difference;
}
-static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
+static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit,
+ int use_bloom_filter)
{
struct tree *t1 = repo_get_commit_tree(the_repository, commit);
int bloom_ret = 1;
@@ -842,7 +843,7 @@ static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
if (!t1)
return 0;
- if (revs->bloom_keys_nr) {
+ if (use_bloom_filter && revs->bloom_keys_nr) {
bloom_ret = check_maybe_different_in_bloom_filter(revs, commit);
if (!bloom_ret)
return 1;
@@ -892,7 +893,7 @@ static int compact_treesame(struct rev_info *revs, struct commit *commit, unsign
if (nth_parent != 0)
die("compact_treesame %u", nth_parent);
old_same = !!(commit->object.flags & TREESAME);
- if (rev_same_tree_as_empty(revs, commit))
+ if (rev_same_tree_as_empty(revs, commit, 1))
commit->object.flags |= TREESAME;
else
commit->object.flags &= ~TREESAME;
@@ -988,7 +989,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
return;
if (!commit->parents) {
- if (rev_same_tree_as_empty(revs, commit))
+ if (rev_same_tree_as_empty(revs, commit, 1))
commit->object.flags |= TREESAME;
return;
}
@@ -1069,7 +1070,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
case REV_TREE_NEW:
if (revs->remove_empty_trees &&
- rev_same_tree_as_empty(revs, p)) {
+ rev_same_tree_as_empty(revs, p, 0)) {
/* We are adding all the specified
* paths from this parent, so the
* history beyond this parent is not
--- >8 ---
on top.
> There is also the issue that count_bloom_filter_false_positive is
> incremented even when no Bloom filters are present, but I think this is
> fine (it matches the behavior of rev_compare_tree()).
Yep, agreed.
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH v6] merge-tree: add -X strategy option
From: Junio C Hamano @ 2023-10-09 17:10 UTC (permalink / raw)
To: Jeff King; +Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
In-Reply-To: <20231009155315.GA3252778@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I agree that struct-copying is an unusual pattern, and we'd potentially
> run into problems with duplication. But I think it is even trickier than
> that here. We also go on to actually _modify_ opt in this function,
> assigning to various members (both directly, and I think the merge code
> itself will write to opt->priv).
>
> So if we use a pointer (rather than struct assignment), those changes
> will persist in the merge_options struct that was passed in. Which is
> also weird.
>
> Between the two, I think using a pointer is probably the least-weird.
> This real_merge() function is only called once, and is a static-local
> helper for cmd_merge_tree(). So the two functions work as a single unit,
> and munging "opt" is not a big deal.
It is called once per --stdin input to perform many merges in a row.
The most obvious "structure to pointer to structure" conversion
below seems to break an assertion (which is not very surprising, as
it happens inside that --stdin loop), so I am tempted to revert the
whole thing for now.
Thanks.
git: merge-ort.c:5110: merge_incore_recursive: Assertion `opt->ancestor == NULL' failed.
./test-lib.sh: line 1067: 738791 Done printf "c1 c3\nc2 -- c1 c3\nc2 c3"
738792 Aborted | git -C repo merge-tree --stdin > actual
builtin/merge-tree.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git c/builtin/merge-tree.c w/builtin/merge-tree.c
index 7024b5ce2e..1cb1fba2de 100644
--- c/builtin/merge-tree.c
+++ w/builtin/merge-tree.c
@@ -425,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
{
struct commit *parent1, *parent2;
struct commit_list *merge_bases = NULL;
- struct merge_options opt = o->merge_options;
+ struct merge_options *opt = &o->merge_options;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
@@ -439,10 +439,10 @@ static int real_merge(struct merge_tree_options *o,
help_unknown_ref(branch2, "merge-tree",
_("not something we can merge"));
- opt.show_rename_progress = 0;
+ opt->show_rename_progress = 0;
- opt.branch1 = branch1;
- opt.branch2 = branch2;
+ opt->branch1 = branch1;
+ opt->branch2 = branch2;
if (merge_base) {
struct commit *base_commit;
@@ -452,11 +452,11 @@ static int real_merge(struct merge_tree_options *o,
if (!base_commit)
die(_("could not lookup commit '%s'"), merge_base);
- opt.ancestor = merge_base;
+ opt->ancestor = merge_base;
base_tree = repo_get_commit_tree(the_repository, base_commit);
parent1_tree = repo_get_commit_tree(the_repository, parent1);
parent2_tree = repo_get_commit_tree(the_repository, parent2);
- merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
+ merge_incore_nonrecursive(opt, base_tree, parent1_tree, parent2_tree, &result);
} else {
/*
* Get the merge bases, in reverse order; see comment above
@@ -467,7 +467,7 @@ static int real_merge(struct merge_tree_options *o,
if (!merge_bases && !o->allow_unrelated_histories)
die(_("refusing to merge unrelated histories"));
merge_bases = reverse_commit_list(merge_bases);
- merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+ merge_incore_recursive(opt, merge_bases, parent1, parent2, &result);
}
if (result.clean < 0)
@@ -501,12 +501,12 @@ static int real_merge(struct merge_tree_options *o,
}
if (show_messages) {
putchar(line_termination);
- merge_display_update_messages(&opt, line_termination == '\0',
+ merge_display_update_messages(opt, line_termination == '\0',
&result);
}
if (o->use_stdin)
putchar(line_termination);
- merge_finalize(&opt, &result);
+ merge_finalize(opt, &result);
return !result.clean; /* result.clean < 0 handled above */
}
^ permalink raw reply related
* Re: [PATCH v3 1/3] diff-merges: improve --diff-merges documentation
From: Elijah Newren @ 2023-10-09 17:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sergey Organov, git
In-Reply-To: <xmqqmswv3p11.fsf@gitster.g>
On Fri, Oct 6, 2023 at 11:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Elijah Newren <newren@gmail.com> writes:
> >
> >> In my opinion, --remerge-diff does this better; wouldn't we want a
> >> ...
> > Between -c and --cc, I do not think there is anything that makes us
> > favor -c over --cc. While the algorithm to decide which hunks out
> > of -c's output to omit was being polished, comparison with -c served
> > a good way to give baseline, but once --cc has become solid, I do
> > not think I've used -c myself.
Perhaps, then, the user manual should either omit -c, or recommend
users use --cc instead?
> > I personally find that a very trivial merge resolution is far easier
> > to read with --cc than --remerge-diff, the latter being way too
> > verbose.
Ah, indeed, for those that know the --cc output format well (it takes
a bit to figure out for newcomers), your example demonstrates this
nicely. Thanks.
> > Also, --cc and -c should work inside a read-only repository where
> > you only have read access to. If remerge needs to write some
> > objects to the repository, then you'd need some hack to give a
> > writable object store overlay via the alternate odb mechanism, or
> > something, right?
Well, it does use a temporary object store with the alternate odb
mechanism already, but I don't think there's any code to allow the
user to input the location for the temporary store, and thus we'd
probably attempt to write it underneath the same read-only directory.
So, yes, read-only repositories would likely be problematic for
--remerge-diff.
However, are read-only repositories worth mentioning in the documentation here?
> Well, the above did not come out as well as I intended, as I forgot
> to prefix it with something I thought was obvious from what I said
> in the recent discussion in the earlier iteration of this topic,
> where I said that it would be "--remerge-diff", if I were to pick an
> option that is so useful that it deserves short and sweet single
> letter. Narutally, it came after we gained experience with "--cc",
> so it would be surprising if it did worse. Just like it is natural
> to expect that "--cc" would give more useful output than "-m -p"
> that predates everybody else.
>
> In short, I would say "--remerge-diff" would give output that is the
> easiest to grok among the three modern variants to show the changes
> a merge introduces.
>
> The above two cases, where I said cc does better than remerge-diff,
> were meant as _exceptions_ for that general sentiment.
Thanks, this is useful. This does make me wonder, though: Should we
perhaps guide users as to what we recommend (and recommend against) in
this documentation?
If we have lots of options and they all shine on different usecases,
it makes sense to just provide a long list of possibilities for users.
But if we generally feel that one is entirely supplanted by another
(e.g. -c by --cc) it seems beneficial to mention that, and if we
generally feel that one will often be clearer or more useful than the
others (e.g. --remerge-diff), it seems beneficial to recommend it.
Thoughts?
Also, perhaps this would be best to include in a follow-up series (as
it appears from Sergey's latest iteration that we are leaving other
tweaks for a later series anyway), if we do decide we want to do it...
^ 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