* [PATCH v2 2/5] doc/diff-options: improve wording of the log.diffMerges mention
From: Štěpán Němec @ 2023-10-05 9:00 UTC (permalink / raw)
To: git
In-Reply-To: <20231005090055.3097783-1-stepnem@smrk.net>
Fix the grammar ("which default value is") and reword to match other
similar descriptions (say "configuration variable" instead of
"parameter", link to git-config(1)).
Signed-off-by: Štěpán Němec <stepnem@smrk.net>
---
Documentation/diff-options.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index ee256ec077be..48a5012748dd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -53,9 +53,9 @@ ifdef::git-log[]
-m:::
This option makes diff output for merge commits to be shown in
the default format. `-m` will produce the output only if `-p`
- is given as well. The default format could be changed using
- `log.diffMerges` configuration parameter, which default value
- is `separate`.
+ is given as well. The default format can be specified using
+ the configuration variable `log.diffMerges` (see
+ linkgit:git-config[1]). It defaults to `separate`.
+
--diff-merges=first-parent:::
--diff-merges=1:::
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v2] builtin/repack.c: avoid making cruft packs preferred
From: Patrick Steinhardt @ 2023-10-05 11:14 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]
On Tue, Oct 03, 2023 at 05:54:19PM -0400, Taylor Blau wrote:
[snip]
> @@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,
> if (preferred)
> strvec_pushf(&cmd.args, "--preferred-pack=%s",
> pack_basename(preferred));
> + else if (names->nr) {
> + /* 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.
> + * Select the non-cruft one as preferred to encourage
> + * pack-reuse among packs containing reachable objects
> + * over unreachable ones.
> + *
> + * (Note we could write multiple packs here if
> + * `--max-pack-size` was given, but any one of them
> + * will suffice, so pick the first one.)
> + */
Well, okay, you kind of acknowledge this here.
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.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Junio C Hamano @ 2023-10-05 10:25 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <871qeay6tz.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Just for better understanding: does it mean that *any* addition of
> one-letter option is prohibited from any existing Git command? Cause it
> definitely sounds this way.
No, we just prefer to think twice before giving short-and-sweet
single letter option to a feature that is not proven useful, and
during the discussion it has become rather clear that the proposed
combination of options goes against helping normal Git users by
forcing them to read an aggregated and redundant patch for a merge
when they already see patches for individual commits on the side
branch that was merged.
>> 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,
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 am wondering if we can have a generalized "personal command option
alias" mechanism implemented. Then you can give '-[a-z]'[*2*] to
whatever combination you like without affecting others by only
futzing with your $HOME/.gitconfig file.
[Footnote]
*1* ...or if we are mimicking options of an existing external
commmand, we need to match them, but in this case it does not
apply.
*2* If we were to do this for real, it may make sense to carve out
option namespace so that end-user aliases cannot overlap with
official set of options. For example, if you can never get a
string that matches "^-![a-z]$" recognised as an official
command line option by our parsers, then '-!' followed by a
single letter would be a good candidate for our "personal
command option alias" to work with, and '-!d' might be
something you want to use your personal option alias, without
having to worry about a later version of Git using the option
officially for something else.
Another thing that need to be considered when designing such a
"command option alias" feature is how to express the set of
commands an alias applies to, as some underlying commands may
share the same set of options. For example, having to say
optionAlias.bisect.1 = --first-parent
optionAlias.blame.1 = --first-parent
optionAlias.log.1 = --first-parent
is a bit too cumbersome to allow me to say "git bisect -!1",
"git blame -!1", and "git log -!1". But I am not offhand sure
if a much simpler
optionAlias.1 = --first-parent
is sufficient. "git commit -!1" would expand into "git commit
--first-parent" and would give you an error with the usage
message, which may not be too bad. I dunno.
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
From: Patrick Steinhardt @ 2023-10-05 11:31 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Junio C Hamano, Eric Sunshine
In-Reply-To: <9ec999882d790aa770aba8c0916b9260661af9be.1696293862.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]
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.
Patrick
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> builtin/repack.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 529e13120d..8a5bbb9cba 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -51,7 +51,7 @@ struct pack_objects_args {
> const char *window_memory;
> const char *depth;
> const char *threads;
> - const char *max_pack_size;
> + unsigned long max_pack_size;
> int no_reuse_delta;
> int no_reuse_object;
> int quiet;
> @@ -242,7 +242,7 @@ static void prepare_pack_objects(struct child_process *cmd,
> if (args->threads)
> strvec_pushf(&cmd->args, "--threads=%s", args->threads);
> if (args->max_pack_size)
> - strvec_pushf(&cmd->args, "--max-pack-size=%s", args->max_pack_size);
> + strvec_pushf(&cmd->args, "--max-pack-size=%lu", args->max_pack_size);
> if (args->no_reuse_delta)
> strvec_pushf(&cmd->args, "--no-reuse-delta");
> if (args->no_reuse_object)
> @@ -946,7 +946,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> N_("limits the maximum delta depth")),
> OPT_STRING(0, "threads", &po_args.threads, N_("n"),
> N_("limits the maximum number of threads")),
> - OPT_STRING(0, "max-pack-size", &po_args.max_pack_size, N_("bytes"),
> + OPT_MAGNITUDE(0, "max-pack-size", &po_args.max_pack_size,
> N_("maximum size of each packfile")),
> OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
> N_("repack objects in packs marked with .keep")),
> --
> 2.42.0.310.g9604b54f73.dirty
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 4/5] doc/gitk: s/sticked/stuck/
From: Štěpán Němec @ 2023-10-05 9:00 UTC (permalink / raw)
To: git
In-Reply-To: <20231005090055.3097783-1-stepnem@smrk.net>
The terminology was changed in b0d12fc9b23a (Use the word 'stuck'
instead of 'sticked').
Signed-off-by: Štěpán Němec <stepnem@smrk.net>
---
Documentation/gitk.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index d50e9ed10e04..c2213bb77b38 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -26,7 +26,7 @@ changes each commit introduces are shown. Finally, it supports some
gitk-specific options.
gitk generally only understands options with arguments in the
-'sticked' form (see linkgit:gitcli[7]) due to limitations in the
+'stuck' form (see linkgit:gitcli[7]) due to limitations in the
command-line parser.
rev-list options and arguments
--
2.42.0
^ permalink raw reply related
* [PATCH] merge-ort: initialize repo in index state
From: John Cai via GitGitGadget @ 2023-10-05 15:22 UTC (permalink / raw)
To: git; +Cc: John Cai, John Cai
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>
---
merge-ort: initialize repo in index state
initialize_attr_index() does not initialize the repo member of
attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=" 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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v1
Pull-Request: https://github.com/git/git/pull/1583
merge-ort.c | 1 +
t/t4300-merge-tree.sh | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..172dc7d497d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
struct index_state *attr_index = &opt->priv->attr_index;
struct cache_entry *ce;
+ attr_index->repo = the_repository;
attr_index->initialized = 1;
if (!opt->renormalize)
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 57c4f26e461..254453fff9c 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -86,6 +86,26 @@ EXPECTED
test_cmp expected actual
'
+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 &&
+ git commit -am "adding bar" &&
+ source=$(git rev-parse HEAD) &&
+ echo baz>>file1 &&
+ git commit -am "adding baz" &&
+ merge=$(git rev-parse HEAD) &&
+ test_must_fail git --attr-source=HEAD merge-tree -z --write-tree \
+ --merge-base "$base" --end-of-options "$source" "$merge" >out &&
+ grep "Merge conflict in file1" out
+ )
+'
+
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
--
gitgitgadget
^ permalink raw reply related
* [PATCH] t4014: replace "test -f" with "test_path_is_file"
From: Vinayak Dev @ 2023-10-05 8:17 UTC (permalink / raw)
To: gitster; +Cc: git, Vinayak Dev
From: Vinayak Dev <vinayakdev.sci@gmail.com>
Many tests in the codebase use test -[e|f|d].. commands
to check for various conditions. However, the test command
upon failure simply exits with a non-zero exit code(usually 1).
Therefore, replace instances of "test -f" from t/t4014-format-patch.sh
with the function test_path_is_file() defined in t/test-lib-functions.sh
that exits with a debugging-friendly diagnostic message upon failure.
Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>
---
t/t4014-format-patch.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 0a4ab36c3a..5f7d0836d6 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -763,7 +763,7 @@ test_expect_success 'format-patch from a subdirectory (1)' '
false
;;
esac &&
- test -f "$filename"
+ test_path_is_file "$filename"
'
test_expect_success 'format-patch from a subdirectory (2)' '
@@ -782,7 +782,7 @@ test_expect_success 'format-patch from a subdirectory (2)' '
;;
esac &&
basename=$(expr "$filename" : ".*/\(.*\)") &&
- test -f "sub/$basename"
+ test_path_is_file "sub/$basename"
'
test_expect_success 'format-patch from a subdirectory (3)' '
@@ -794,7 +794,7 @@ test_expect_success 'format-patch from a subdirectory (3)' '
git format-patch -1 -o "$TRASH_DIRECTORY"
) &&
basename=$(expr "$filename" : ".*/\(.*\)") &&
- test -f "$basename"
+ test_path_is_file "$basename"
'
test_expect_success 'format-patch --in-reply-to' '
base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
--
2.42.0
^ permalink raw reply related
* Wording "fetch" to change to "pull" or "merge" ti suggest the proper action to take when pull fails?
From: MDW @ 2023-10-05 14:31 UTC (permalink / raw)
To: git
Hello
I noticed that the working upon a rejected pull request is "fetch
first".
However, AFAIK fetching will not help with the pull.
So I suggest to change that wording.
Example:
```bash
$ git push
To ssh://git.exemple.com/user/repo.git
! [rejected] main -> main (fetch first)
error: failed to push some refs to 'ssh://git.exemple.com/user/repo.git'
```
Kind regards
Mario
--
GDPR: Personal data is provided only for this communication.
I refuse unsollicited subscriptions to mailing lists
and I refuse commercial use of this personal data.
^ permalink raw reply
* 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
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