* js/ci-coverity, was Re: What's cooking in git.git (Oct 2023, #02; Wed, 4)
From: Jeff King @ 2023-10-05 17:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <xmqqpm1ulzoh.fsf@gitster.g>
On Wed, Oct 04, 2023 at 04:45:34PM -0700, Junio C Hamano wrote:
> * js/ci-coverity (2023-09-25) 7 commits
> - SQUASH???
> - coverity: detect and report when the token or project is incorrect
> - coverity: allow running on macOS
> - coverity: support building on Windows
> - coverity: allow overriding the Coverity project
> - coverity: cache the Coverity Build Tool
> - ci: add a GitHub workflow to submit Coverity scans
>
> GitHub CI workflow has learned to trigger Coverity check.
>
> Looking good.
> source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
I think that has been sitting at "Looking good" for a few iterations.
IMHO it is ready to progress, with the SQUASH applied on the final
patch.
-Peff
^ permalink raw reply
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Jeff King @ 2023-10-05 17:18 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git, avarab
In-Reply-To: <20231005132018+0200.47596-stepnem@smrk.net>
On Thu, Oct 05, 2023 at 01:20:18PM +0200, Štěpán Němec wrote:
> So how about we just butcher the DESCRIPTION completely;
> [...]
> DESCRIPTION
> 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.
>
> [That's all for a summary, read the following sections for details.]
Yeah, I think that is a big improvement over the status quo. I might
also be worth starting with a single-sentence overview of what is common
to both modes. Something like:
Output the contents or details of one or more objects. 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 a single object
given on the command line.
In batch mode, arguments are read from standard input.
> > I think this got a bit inaccurate with "--batch-command", which is a
> > whole different mode itself compared to --batch and --batch-check. I
> > don't think your patch is really making anything worse, but arguably
> > there are three "major modes" here.
>
> This is not obvious to me (the "three major modes" part). AIUI it's
> still mainly a batch (read from stdin) vs. non-batch (args on command
> line) dichotomy. The details differ (just args vs. command + args), but
> so does e.g. -e differ in providing information via exit code rather
> than stdout.
Yeah, I think you understand it correctly. But what the current text
(both before and after your proposed patch) says about batch mode is:
In batch mode, a list of objects (separated by linefeeds) is provided
on stdin, [...]
which I think is not really true of --batch-command. But the rewrite you
suggest above takes care of that nicely, I think.
> (But please note I'm not trying to pose as an expert here: this all
> started with me coming to git-cat-file(1) to _learn_ about cat-file
> and finding the description more than a little confusing.)
That is a very valuable perspective. I am probably too much an expert in
cat-file, and it has rotted my brain. ;)
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
From: Jeff King @ 2023-10-05 17:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <xmqqfs2qp3bg.fsf@gitster.g>
On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: John Cai <johncai86@gmail.com>
> >
> > 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git",
> > 2023-05-06) provided the ability to pass in a treeish as the attr
> > source. In the context of serving Git repositories as bare repos like we
> > do at GitLab however, it would be easier to point --attr-source to HEAD
> > for all commands by setting it once.
> >
> > Add a new config attr.tree that allows this.
>
> Hmph, I wonder if we want to go all the way to emulate how the
> mailmap.blob was done, including
>
> - Default the value of attr.tree to HEAD in a bare repository;
>
> - Notice but ignore errors if the attr.tree does not point at a
> tree object, and pretend as if attr.tree specified an empty tree;
>
> which does not seem to be in this patch. With such a change,
> probably we do not even need [2/2] of the series, perhaps?
Oh good, this was exactly what I was going to write in a review, so now
I don't have to. :)
Even though it creates behavior differences between attr.tree and
--attr-source, I think that is justifiable. Config options apply across
a wider set of contexts, so loosening the error handling may make sense
where it would not for a command-line option. But we should document
both that, and also how the two interact (I assume "git --attr-source"
would override attr.tree completely).
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
From: Taylor Blau @ 2023-10-05 17:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Junio C Hamano, Eric Sunshine
In-Reply-To: <ZR6ednOcuK6qizch@tanuki>
On Thu, Oct 05, 2023 at 01:31:02PM +0200, Patrick Steinhardt wrote:
> On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote:
> > The repack builtin takes a `--max-pack-size` command-line argument which
> > it uses to feed into any of the pack-objects children that it may spawn
> > when generating a new pack.
> >
> > This option is parsed with OPT_STRING, meaning that we'll accept
> > anything as input, punting on more fine-grained validation until we get
> > down into pack-objects.
> >
> > This is fine, but it's wasteful to spend an entire sub-process just to
> > figure out that one of its option is bogus. Instead, parse the value of
> > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the
> > knonw-good result down to pack-objects.
>
> Tiny nit: s/knonw/known.
>
> Other than that this patch looks good to me.
Oops, good eyes. Perhaps Junio can tweak this when queuing, but if he
doesn't, I don't think it'd be the end of the world...
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 0/6] some "commit-graph verify" fixes for chains
From: Taylor Blau @ 2023-10-05 17:38 UTC (permalink / raw)
To: Jeff King; +Cc: git, Derrick Stolee
In-Reply-To: <20230928043746.GB57926@coredump.intra.peff.net>
On Thu, Sep 28, 2023 at 12:37:46AM -0400, Jeff King wrote:
> On Tue, Sep 26, 2023 at 01:54:52AM -0400, Jeff King wrote:
>
> > [1/6]: commit-graph: factor out chain opening function
> > [2/6]: commit-graph: check mixed generation validation when loading chain file
> > [3/6]: t5324: harmonize sha1/sha256 graph chain corruption
> > [4/6]: commit-graph: detect read errors when verifying graph chain
> > [5/6]: commit-graph: tighten chain size check
> > [6/6]: commit-graph: report incomplete chains during verification
>
> Here's a re-roll that fixes a missed case in patch 6 (I sent more
> details elsewhere in the thread).
Thanks, and apologies for taking a little longer than I would have liked
to review both of these rounds. This round looks great to me (and it's
already in 'next').
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 01/10] t6700: mark test as leak-free
From: Taylor Blau @ 2023-10-05 17:40 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003202609.GA7812@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:26:09PM -0400, Jeff King wrote:
> This test has never leaked since it was added. Let's annotate it to make
> sure it stays that way (and to reduce noise when looking for other
> leak-free scripts after we fix some leaks).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Obviously not directly related to the rest; this could be spun off to
> its own series, or put atop jk/tree-name-and-depth-limit and merged from
> there.
I wondered how I missed this in tb/mark-more-tests-as-leak-free, but the
answer is trivial: that topic preceded your tree depth stuff ;-).
This change makes good sense, and I don't think it's worth spinning off
into its own series.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 03/10] merge: free result of repo_get_merge_bases()
From: Taylor Blau @ 2023-10-05 17:42 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003202724.GC7812@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:27:24PM -0400, Jeff King wrote:
> We call repo_get_merge_bases(), which allocates a commit_list, but never
> free the result, causing a leak.
>
> The obvious solution is to free it, but we need to look at the contents
> of the first item to decide whether to leave the loop. One option is to
> free it in both code paths. But since the commit that the list points to
> is longer-lived than the list itself, we can just dereference it
> immediately, free the list, and then continue with the existing logic.
> This is about the same amount of code, but keeps the list management all
> in one place.
>
> This lets us mark a number of merge-related test scripts as leak-free.
Wow, getting 10 newly leak-free tests for half as many lines of code is
terrific. Woohoo!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size`
From: Taylor Blau @ 2023-10-05 17:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Junio C Hamano, Eric Sunshine
In-Reply-To: <ZR6nKzflu_18JnoG@tanuki>
On Thu, Oct 05, 2023 at 02:08:11PM +0200, Patrick Steinhardt wrote:
> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:
> [snip]
> > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> > index 90806fd26a..fa0541b416 100644
> > --- a/Documentation/git-gc.txt
> > +++ b/Documentation/git-gc.txt
> > @@ -59,6 +59,13 @@ be performed as well.
> > cruft pack instead of storing them as loose objects. `--cruft`
> > is on by default.
> >
> > +--max-cruft-size=<n>::
> > + When packing unreachable objects into a cruft pack, limit the
> > + size of new cruft packs to be at most `<n>`. Overrides any
>
> We should probably mention the unit here, which is bytes.
Perhaps, though I'm OK with omitting it in the name of brevity, but only
since we link off to the relevant section in the git-repack(1)
documentation (which does include the units there).
> > +static void collapse_small_cruft_packs(FILE *in, size_t max_size,
> > + struct existing_packs *existing)
> > +{
> > + struct packed_git **existing_cruft, *p;
> > + struct strbuf buf = STRBUF_INIT;
> > + size_t total_size = 0;
> > + size_t existing_cruft_nr = 0;
> > + size_t i;
> > +
> > + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);
> > +
> > + for (p = get_all_packs(the_repository); p; p = p->next) {
> > + if (!(p->is_cruft && p->pack_local))
> > + continue;
> > +
> > + strbuf_reset(&buf);
> > + strbuf_addstr(&buf, pack_basename(p));
> > + strbuf_strip_suffix(&buf, ".pack");
> > +
> > + if (!string_list_has_string(&existing->cruft_packs, buf.buf))
> > + continue;
> > +
> > + if (existing_cruft_nr >= existing->cruft_packs.nr)
> > + BUG("too many cruft packs (found %"PRIuMAX", but knew "
> > + "of %"PRIuMAX")",
> > + (uintmax_t)existing_cruft_nr + 1,
> > + (uintmax_t)existing->cruft_packs.nr);
> > + existing_cruft[existing_cruft_nr++] = p;
> > + }
> > +
> > + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);
> > +
> > + for (i = 0; i < existing_cruft_nr; i++) {
> > + size_t proposed;
> > +
> > + p = existing_cruft[i];
> > + proposed = st_add(total_size, p->pack_size);
> > +
> > + if (proposed <= max_size) {
> > + total_size = proposed;
> > + fprintf(in, "-%s\n", pack_basename(p));
> > + } else {
> > + retain_cruft_pack(existing, p);
> > + fprintf(in, "%s\n", pack_basename(p));
> > + }
> > + }
> > +
> > + for (i = 0; i < existing->non_kept_packs.nr; i++)
> > + fprintf(in, "-%s.pack\n",
> > + existing->non_kept_packs.items[i].string);
>
> As far as I can see, the non-kept packs are passed to
> git-pack-objects(1) both in the cases where we do collapse small cruft
> packs and where we don't. Is there any particular reason why we handle
> those in both code paths separately instead of merging that logic? Is
> the ordering of packfiles important here?
No particularly good reason. The ordering isn't important, and you could do something like this:
--- 8< ---
diff --git a/builtin/repack.c b/builtin/repack.c
index 04770b15fe..6e17fc3f51 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -905,10 +905,6 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,
}
}
- for (i = 0; i < existing->non_kept_packs.nr; i++)
- fprintf(in, "-%s.pack\n",
- existing->non_kept_packs.items[i].string);
-
strbuf_release(&buf);
}
@@ -959,14 +955,13 @@ static int write_cruft_pack(const struct pack_objects_args *args,
in = xfdopen(cmd.in, "w");
for_each_string_list_item(item, names)
fprintf(in, "%s-%s.pack\n", pack_prefix, item->string);
- if (args->max_pack_size && !cruft_expiration) {
+ if (args->max_pack_size && !cruft_expiration)
collapse_small_cruft_packs(in, args->max_pack_size, existing);
- } else {
- for_each_string_list_item(item, &existing->non_kept_packs)
- fprintf(in, "-%s.pack\n", item->string);
+ else
for_each_string_list_item(item, &existing->cruft_packs)
fprintf(in, "-%s.pack\n", item->string);
- }
+ for_each_string_list_item(item, &existing->non_kept_packs)
+ fprintf(in, "-%s.pack\n", item->string);
for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s.pack\n", item->string);
fclose(in);
--- >8 ---
But I think that having a small amount of duplication is a fair price to
pay for being able to see the whole input given to pack-objects outlined
in a single function.
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph()
From: Taylor Blau @ 2023-10-05 17:42 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003202752.GD7812@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:27:52PM -0400, Jeff King wrote:
> When closing and freeing a commit-graph, the main entry point is
> close_commit_graph(), which then uses close_commit_graph_one() to
> recurse through the base_graph links and free each one.
>
> Commit 957ba814bf (commit-graph: when closing the graph, also release
> the slab, 2021-09-08) put the call to clear the slab into the recursive
> function, but this is pointless: there's only a single global slab
> variable. It works OK in practice because clearing the slab is
> idempotent, but it makes the code harder to reason about and refactor.
Well reasoned and explained, this change makes perfect sense to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain()
From: Taylor Blau @ 2023-10-05 17:44 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003203004.GF7812@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:30:04PM -0400, Jeff King wrote:
> When adding a graph to a chain, we do some consistency checks and then
> if everything looks good, set g->base_graph to add a link to the chain.
> But when we added a new consistency check in 209250ef38 (commit-graph.c:
> prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_
> we've already set g->base_graph. So we might return failure, even though
> we actually added to the chain.
>
> This hasn't caused a bug yet, because after failing to add to the chain,
> we discard the failed graph struct completely, leaking it. But in order
> to fix that, it's important that the struct be in a consistent and
> predictable state after the failure.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> commit-graph.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 2f75ecd9ae..2c72a554c2 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,
> cur_g = cur_g->base_graph;
> }
>
> - g->base_graph = chain;
> -
> if (chain) {
> if (unsigned_add_overflows(chain->num_commits,
> chain->num_commits_in_base)) {
> @@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,
> g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;
> }
>
> + g->base_graph = chain;
> +
Oops. That looks like my fault. Thanks for catching, this switch makes
sense to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Štěpán Němec @ 2023-10-05 17:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, avarab
In-Reply-To: <20231005171827.GC975921@coredump.intra.peff.net>
On Thu, 5 Oct 2023 13:18:27 -0400
Jeff King wrote:
> On Thu, Oct 05, 2023 at 01:20:18PM +0200, Štěpán Němec wrote:
>
>> So how about we just butcher the DESCRIPTION completely;
>> [...]
>> DESCRIPTION
>> 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.
>>
>> [That's all for a summary, read the following sections for details.]
>
> Yeah, I think that is a big improvement over the status quo. I might
> also be worth starting with a single-sentence overview of what is common
> to both modes. Something like:
>
> Output the contents or details of one or more objects. [...]
I thought about that when proposing the rewrite, but feel that it would
again just duplicate what's said elsewhere, in this case even before,
not after, in the very first line of the man page:
git-cat-file - Provide content or type and size information for
repository objects
> 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 a single object
> given on the command line.
^^^^^
Any particular reason you prefer "given" to "named"? However absurd a
notion of giving an actual object on the command line might seem, to me
"named" is better in that it leaves no room for such misinterpretation.
And the <object> description in OPTIONS talks about "ways to spell
object names", building on the same concept.
--
Štěpán
^ permalink raw reply
* Re: [PATCH 08/10] commit-graph: free write-context entries before overwriting
From: Taylor Blau @ 2023-10-05 17:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003203055.GH7812@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:30:55PM -0400, Jeff King wrote:
> diff --git a/commit-graph.c b/commit-graph.c
> index 4aa2f294f1..744b7eb1a3 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
> free(graph_name);
> }
>
> + free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
> ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));
> final_graph_name = get_split_graph_filename(ctx->odb,
> ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
> + free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
> ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
>
> result = rename(ctx->graph_name, final_graph_name);
This hunk makes sense. It might be nice in the future to do something
like:
--- 8< ---
diff --git a/commit-graph.c b/commit-graph.c
index 5e8a3a5085..cadccbe276 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -59,7 +59,7 @@ void git_test_write_commit_graph_or_die(void)
#define GRAPH_EXTRA_EDGES_NEEDED 0x80000000
#define GRAPH_EDGE_LAST_MASK 0x7fffffff
-#define GRAPH_PARENT_NONE 0x70000000
+tdefine GRAPH_PARENT_NONE 0x70000000
#define GRAPH_LAST_EDGE 0x80000000
@@ -1033,11 +1033,11 @@ struct write_commit_graph_context {
uint64_t progress_cnt;
char *base_graph_name;
- int num_commit_graphs_before;
- int num_commit_graphs_after;
- char **commit_graph_filenames_before;
- char **commit_graph_filenames_after;
- char **commit_graph_hash_after;
+ struct {
+ size_t nr;
+ char **fname;
+ char **hash;
+ } graphs_before, graphs_after;
uint32_t new_num_commits_in_base;
struct commit_graph *new_base_graph;
--- >8 ---
...making the corresponding changes throughout the rest of the file. But
that is definitely out of scope here, and could easily be left for
another day.
#leftoverbits
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH 0/10] some commit-graph leak fixes
From: Taylor Blau @ 2023-10-05 17:52 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:25:04PM -0400, Jeff King wrote:
> I noticed while working on the jk/commit-graph-verify-fix topic that
> free_commit_graph() leaks any slices of a commit-graph-chain except for
> the first. I naively hoped that fixing that would make t5324 leak-free,
> but it turns out there were a number of other leaks, so I fixed those,
> too. A couple of them were in the merge code, which in turn means a
> bunch of new test scripts are now leak-free.
>
> Even though I saw the problem on that other topic, there's no dependency
> here; this series can be applied directly to master (or possibly even
> maint, though I didn't try).
Thanks for carefully finding and explaining these various leaks. The
series is a definite improvement, and after reviewing closely I couldn't
find anything worth changing. LGTM!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 25/32] pack-compat-map: Add support for .compat files of a packfile
From: Taylor Blau @ 2023-10-05 18:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric W. Biederman, git, brian m. carlson
In-Reply-To: <xmqqfs3li5ly.fsf@gitster.g>
On Sun, Sep 10, 2023 at 11:30:49PM -0700, Junio C Hamano wrote:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>
> > diff --git a/pack-write.c b/pack-write.c
> > index b19ddf15b284..f22eea964f77 100644
> > --- a/pack-write.c
> > +++ b/pack-write.c
> > @@ -12,6 +12,7 @@
> > #include "pack-revindex.h"
> > #include "path.h"
> > #include "strbuf.h"
> > +#include "object-file-convert.h"
> > ...
> > +/*
> > + * The *hash contains the pack content hash.
> > + * The objects array is passed in sorted.
> > + */
> > +const char *write_compat_map_file(const char *compat_map_name,
> > + struct pack_idx_entry **objects,
> > + int nr_objects, const unsigned char *hash)
>
> Include "pack-compat-map.h"; otherwise the compiler would complain
> for missing prototypes.
Likewise this is missing an entry in the .gitignore:
--- >8 ---
diff --git a/.gitignore b/.gitignore
index 5e56e471b3..7f5a93a6f6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@
/git-shortlog
/git-show
/git-show-branch
+/git-show-compat-map
/git-show-index
/git-show-ref
/git-sparse-checkout
--- 8< ---
Thanks,
Taylor
^ permalink raw reply related
* Re: js/ci-coverity, was Re: What's cooking in git.git (Oct 2023, #02; Wed, 4)
From: Junio C Hamano @ 2023-10-05 18:44 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20231005170859.GB975921@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Oct 04, 2023 at 04:45:34PM -0700, Junio C Hamano wrote:
>
>> * js/ci-coverity (2023-09-25) 7 commits
>> - SQUASH???
>> - coverity: detect and report when the token or project is incorrect
>> - coverity: allow running on macOS
>> - coverity: support building on Windows
>> - coverity: allow overriding the Coverity project
>> - coverity: cache the Coverity Build Tool
>> - ci: add a GitHub workflow to submit Coverity scans
>>
>> GitHub CI workflow has learned to trigger Coverity check.
>>
>> Looking good.
>> source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
>
> I think that has been sitting at "Looking good" for a few iterations.
> IMHO it is ready to progress, with the SQUASH applied on the final
> patch.
Ah, yes, unless I use some magic phrase (like "Will merge to" or
"Expecting") there in the report, entries tend to be left in the
noise. Thanks for noticing and pinging. Very much appreciated.
^ permalink raw reply
* Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
From: John Cai @ 2023-10-05 19:46 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, John Cai via GitGitGadget, git
In-Reply-To: <20231005170703.GA975921@coredump.intra.peff.net>
Hi Peff,
On 5 Oct 2023, at 13:07, Jeff King wrote:
> On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:
>
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: John Cai <johncai86@gmail.com>
>>>
>>> 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git",
>>> 2023-05-06) provided the ability to pass in a treeish as the attr
>>> source. In the context of serving Git repositories as bare repos like we
>>> do at GitLab however, it would be easier to point --attr-source to HEAD
>>> for all commands by setting it once.
>>>
>>> Add a new config attr.tree that allows this.
>>
>> Hmph, I wonder if we want to go all the way to emulate how the
>> mailmap.blob was done, including
>>
>> - Default the value of attr.tree to HEAD in a bare repository;
>>
>> - Notice but ignore errors if the attr.tree does not point at a
>> tree object, and pretend as if attr.tree specified an empty tree;
>>
>> which does not seem to be in this patch. With such a change,
>> probably we do not even need [2/2] of the series, perhaps?
>
> Oh good, this was exactly what I was going to write in a review, so now
> I don't have to. :)
>
> Even though it creates behavior differences between attr.tree and
> --attr-source, I think that is justifiable. Config options apply across
> a wider set of contexts, so loosening the error handling may make sense
> where it would not for a command-line option.
Makes sense. Will adjust in the next version.
> But we should document both that, and also how the two interact (I assume
> "git --attr-source" would override attr.tree completely).
Yes, --attr-source would take precedence over attr.tree
>
> -Peff
thanks!
John
^ permalink raw reply
* Re: [PATCH v2 1/5] doc: fix some typos, grammar and wording issues
From: Junio C Hamano @ 2023-10-05 20:17 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git, Eric Sunshine
In-Reply-To: <20231005090055.3097783-1-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
> ---
> v2: Feedback from Eric and Junio, implement most of Eric's suggestions.
> Range-diff with two inline comments follows:
Looking good.
The above comment was meant to apply to the whole series, but
because you lack the [0/5] cover letter, I am replying to [1/5].
You may want to run git-format-patch with --cover-letter when
sending a series with multiple patches.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
From: Junio C Hamano @ 2023-10-05 20:22 UTC (permalink / raw)
To: Taylor Blau; +Cc: Patrick Steinhardt, git, Jeff King, Eric Sunshine
In-Reply-To: <ZR7yOgvk3PCtYIR2@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Thu, Oct 05, 2023 at 01:31:02PM +0200, Patrick Steinhardt wrote:
>> On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote:
>> > The repack builtin takes a `--max-pack-size` command-line argument which
>> > it uses to feed into any of the pack-objects children that it may spawn
>> > when generating a new pack.
>> >
>> > This option is parsed with OPT_STRING, meaning that we'll accept
>> > anything as input, punting on more fine-grained validation until we get
>> > down into pack-objects.
>> >
>> > This is fine, but it's wasteful to spend an entire sub-process just to
>> > figure out that one of its option is bogus. Instead, parse the value of
>> > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the
>> > knonw-good result down to pack-objects.
>>
>> Tiny nit: s/knonw/known.
>>
>> Other than that this patch looks good to me.
>
> Oops, good eyes. Perhaps Junio can tweak this when queuing, but if he
> doesn't, I don't think it'd be the end of the world...
I cannot do so "when queuing" anymore, but let me see if it is still
outside 'next' and then run "rebase -i" on it.
This is a bit different from what I would have written (I would have
passed the original string to underlying subprocess for it to sanity
check the value to its own liking, after we checked if it makes
sense when interpreted as magnitude, so that the underlying
subprocess, if it has stricter limit than we know of, can complain
with the string the end-user gave it, not the result of us turning
into a large integer string. But that is minor.
Thanks, both.
^ permalink raw reply
* Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size`
From: Junio C Hamano @ 2023-10-05 20:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git, Jeff King, Eric Sunshine
In-Reply-To: <ZR6nKzflu_18JnoG@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:
> [snip]
>> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
>> index 90806fd26a..fa0541b416 100644
>> --- a/Documentation/git-gc.txt
>> +++ b/Documentation/git-gc.txt
>> @@ -59,6 +59,13 @@ be performed as well.
>> cruft pack instead of storing them as loose objects. `--cruft`
>> is on by default.
>>
>> +--max-cruft-size=<n>::
>> + When packing unreachable objects into a cruft pack, limit the
>> + size of new cruft packs to be at most `<n>`. Overrides any
>
> We should probably mention the unit here, which is bytes.
Good suggestion. I'll tweak "at most `<n>`" to "& bytes" or
something.
^ permalink raw reply
* Re: [PATCH v2] builtin/repack.c: avoid making cruft packs preferred
From: Eric Sunshine @ 2023-10-05 20:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git, Jeff King, Junio C Hamano
In-Reply-To: <ZR6ajQa8bh5HKsnr@tanuki>
On Thu, Oct 5, 2023 at 10:46 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Tue, Oct 03, 2023 at 05:54:19PM -0400, Taylor Blau wrote:
> > + /* The largest pack was repacked, meaning that either
> > + * one or two packs exist depending on whether the
> > + * repository has a cruft pack or not.
>
> Nit: this comment will grow stale soonish once your patch series lands
> that introduces a maximum packfile size for cruft packs as there can be
> arbitrarily many cruft packs from thereon.
>
> The rest of this patch series looks good to me and makes sense. I don't
> really think that this comment here is worth a reroll.
If you do happen to reroll for some reason, though, perhaps take care
of the minor style violation, as well. Use:
/*
* Multi-line comment
* style.
*/
rather than:
/* Multi-line comment
* style.
*/
^ permalink raw reply
* [OUTREACHY] Indication Of Interest To Contribute To Git
From: Naomi Ibe @ 2023-10-05 20:24 UTC (permalink / raw)
To: git
Dear Community,
I am Naomi Ibe , and I am sending this mail to indicate my interest
to contribute meaningfully in whatever capacity I can to the Git
community.
Git has always been a part of my software engineering journey right
from day one and I would love to understand more about it, while still
helping it grow in whatever little capacity I can.
I understand that Christian Couder is going to be our mentor and I
would be delighted to work with him. I am really eager to learn and
begin contributions.
Thanks a lot for this opportunity.
Best regards,
Naomi
^ permalink raw reply
* Microsoft Smart App Control - Git - git-bash.exe File Unsigned
From: Rolland Swing (Insight Global LLC) @ 2023-10-05 20:41 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Anthony Chuang
In-Reply-To: <SJ1PR21MB3699CA030DE035CA42582AF5E3CAA@SJ1PR21MB3699.namprd21.prod.outlook.com>
Hi Git Team,
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
Signing your app is in your best interest, as it positively identifies you as the developer of your application to your customers installing and running your apps, and they can rest assured that your app hasn't been tampered with. For the purposes of Smart App Control, all app binaries including .exe, .dll, .tmp files, and uninstallers all need to be signed.
For more information on code signing, please refer to: https://learn.microsoft.com/en-us/windows/apps/develop/smart-app-control/code-signing-for-smart-app-control
Please confirm if the unsigned file(s) will be signed in this or a future build, or if there is no intent to rectify the unsigned executable file(s).
Thanks,
Rolland Swing | Azure E&P Program Manager
Enterprise & Security - Platform Integrity Team
mailto:v-roswing@microsoft.com
^ permalink raw reply
* Re: [PATCH] merge-ort: initialize repo in index state
From: Junio C Hamano @ 2023-10-05 20:56 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <pull.1583.git.git.1696519349407.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> initialize_attr_index() does not initialize the repo member of
> attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=<tree>"
> global option to "git", 2023-05-06), this became a problem because
> istate->repo gets passed down the call chain starting in
> git_check_attr(). This gets passed all the way down to
> replace_refs_enabled(), which segfaults when accessing r->gitdir.
>
> Fix this by initializing the repository in the index state.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> ---
Nice spotting.
> +test_expect_success '3-way merge with --attr-source' '
> + test_when_finished rm -rf 3-way &&
> + git init 3-way &&
> + (
> + cd 3-way &&
> + test_commit initial file1 foo &&
> + base=$(git rev-parse HEAD) &&
> + git checkout -b brancha &&
> + echo bar>>file1 &&
We need a space before but not after ">>".
> + git commit -am "adding bar" &&
> + source=$(git rev-parse HEAD) &&
> + echo baz>>file1 &&
Ditto.
> + git commit -am "adding baz" &&
> + merge=$(git rev-parse HEAD) &&
Sorry, but I got lost. We have the $base commit on the default
initial branch, from which forked branch-A which we created two
commits to add lines "bar" and "baz" to file1. We are calling the
tip of this branch-A $merge, and the parent of $merge is called
$source.
> + test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \
> + --merge-base "$base" --end-of-options "$source" "$merge" >out &&
So, is this asking "merge-tree" to merge "$source" and "$merge", one
of which is the direct parent of the other one? Aren't we missing a
"checkout @{-1}" before we add "baz" or something?
If the attitude taken by this test is "we do not really care if the
attempted merge is meaningless and would never happen in the real
world, as the only thing we care is to see "git" not to segfault",
then we probably shouldn't even check ...
> + grep "Merge conflict in file1" out
... if we failed due to conflict with a "grep" like this. As "out"
is a binary file (thanks to the use of "-z" on the command line of
"merge-tree" invocation), I am not sure if you can rely on "grep" to
find this error message in 'out', depending on your implementation
of "grep".
On the other hand, the new test can do a more realistic merge
between two commits, where having an attribute in some tree object
(which preferrably is *not* HEAD and .gitattribute does not exist in
the working tree) given to the --attr-source option does make a
difference, and verify the contents of the "file1" recorded in the
resulting tree. That way, the test can verify that the attributes
are read from the right place without segfaulting.
> + )
> +'
> +
> test_expect_success 'file change A, B (same)' '
> git reset --hard initial &&
> test_commit "change-a-b-same-A" "initial-file" "AAA" &&
>
> base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Sergey Organov @ 2023-10-05 20:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzs1mkma.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
[...]
>>> If I have to pick a candidate for "get me diff" that is the most
>>> useful among those currently are available, it is "give patches to
>>> all single-parent commit, and show tricky conflict resolution part
>>> only for merge commits".
>>
>> I'm afraid you need to pick a candidate that will be natural for '-d',
>> not just most useful output for your workflows, whatever it happens to
>> be.
>
> Literal match to word "diff" does not necessarily mean it is useful,
Sure, who argues? I don't.
> and short-and-sweet single-letter option name is primarily about
> letting users reach useful features with minimum typing [*1*], so you
> cannot avoid "most useful" being a large part of the equation.
I don't try to avoid "most useful" either, quite opposite. With whom do
you argue?
I just pointed that a short-cut would better be natural (or mnemonic)
/as well/, so you probably don't actually want:
-d:
give patches to all single-parent commits, and show tricky conflict
resolution part only for merge commits.
, or do you?
Overall, as an example, I'd understand if you had deflected the patch
with "let's rather use -d for '--decorate=short', or '--date=relative'",
or something like that, but you don't, leaving me uncertain about your
actual worries and intentions.
Anyway, I re-submitted the patches avoiding precious, too hard to
deserve single-letter option.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Jeff King @ 2023-10-05 21:01 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git, avarab
In-Reply-To: <20231005194852+0200.262756-stepnem@smrk.net>
On Thu, Oct 05, 2023 at 07:48:52PM +0200, Štěpán Němec wrote:
> > Yeah, I think that is a big improvement over the status quo. I might
> > also be worth starting with a single-sentence overview of what is common
> > to both modes. Something like:
> >
> > Output the contents or details of one or more objects. [...]
>
> I thought about that when proposing the rewrite, but feel that it would
> again just duplicate what's said elsewhere, in this case even before,
> not after, in the very first line of the man page:
>
> git-cat-file - Provide content or type and size information for
> repository objects
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).
> > 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 a single object
> > given on the command line.
> ^^^^^
> Any particular reason you prefer "given" to "named"? However absurd a
> notion of giving an actual object on the command line might seem, to me
> "named" is better in that it leaves no room for such misinterpretation.
> And the <object> description in OPTIONS talks about "ways to spell
> object names", building on the same concept.
Nope, I didn't even do that replacement consciously (I was just fleshing
out my example, and ended up deciding nothing else needed to be
changed). So "named" is fine by me.
Thanks.
-Peff
^ 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