From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH] commit-graph: warn about incompatibilities only when trying to write
Date: Fri, 26 Feb 2021 17:00:25 +0100 [thread overview]
Message-ID: <87r1l27rae.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <pull.888.git.1614351036334.gitgitgadget@gmail.com>
On Fri, Feb 26 2021, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In c85eec7fc37 (commit-graph: when incompatible with graphs, indicate
> why, 2021-02-11), we started warning the user if they tried to write a
> commit-graph in a shallow repository, or one containing replace objects.
>
> However, this patch was a bit overzealous, as Git now _also_ warns when
> merely checking whether there _is_ a usable commit graph, not only when
> writing one.
>
> Let's suppress that warning unless we want to write a commit-graph.
Ah, so that's what it's for, as I suspected :) Unfortunately...
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> commit-graph: warn about incompatibilities only when trying to write
>
> As pointed out by Ævar in
> https://lore.kernel.org/git/87pn0o6y1e.fsf@evledraar.gmail.com, my
> recent patch to trigger warnings in repositories that are incompatible
> with the commit-graph was a bit too overzealous. Here is a fix for that.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-888%2Fdscho%2Fwarn-a-little-less-if-commit-graph-is-skipped-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-888/dscho/warn-a-little-less-if-commit-graph-is-skipped-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/888
>
> commit-graph.c | 20 ++++++++++++--------
> t/t5318-commit-graph.sh | 13 +++++++++++++
> 2 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 8fd480434353..245b7108e0d1 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -198,7 +198,7 @@ static struct commit_graph *alloc_commit_graph(void)
>
> extern int read_replace_refs;
>
> -static int commit_graph_compatible(struct repository *r)
> +static int commit_graph_compatible(struct repository *r, int quiet)
> {
> if (!r->gitdir)
> return 0;
> @@ -206,8 +206,9 @@ static int commit_graph_compatible(struct repository *r)
> if (read_replace_refs) {
> prepare_replace_object(r);
> if (hashmap_get_size(&r->objects->replace_map->map)) {
> - warning(_("repository contains replace objects; "
> - "skipping commit-graph"));
> + if (!quiet)
> + warning(_("repository contains replace "
> + "objects; skipping commit-graph"));
> return 0;
> }
> }
> @@ -215,12 +216,15 @@ static int commit_graph_compatible(struct repository *r)
> prepare_commit_graft(r);
> if (r->parsed_objects &&
> (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) {
> - warning(_("repository contains (deprecated) grafts; "
> - "skipping commit-graph"));
> + if (!quiet)
> + warning(_("repository contains (deprecated) grafts; "
> + "skipping commit-graph"));
> return 0;
> }
> if (is_repository_shallow(r)) {
> - warning(_("repository is shallow; skipping commit-graph"));
> + if (!quiet)
> + warning(_("repository is shallow; skipping "
> + "commit-graph"));
> return 0;
> }
>
> @@ -652,7 +656,7 @@ static int prepare_commit_graph(struct repository *r)
> */
> return 0;
>
> - if (!commit_graph_compatible(r))
> + if (!commit_graph_compatible(r, 1))
...doing this will cause "git gc --auto" to run into persistent
warnings. See a WIP patch-on-top in [1] below...
> return 0;
>
> prepare_alt_odb(r);
> @@ -2123,7 +2127,7 @@ int write_commit_graph(struct object_directory *odb,
> warning(_("attempting to write a commit-graph, but 'core.commitGraph' is disabled"));
> return 0;
> }
> - if (!commit_graph_compatible(the_repository))
> + if (!commit_graph_compatible(the_repository, 0))
> return 0;
>
> ctx = xcalloc(1, sizeof(struct write_commit_graph_context));
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 2ed0c1544da1..2699c55e9a93 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -741,4 +741,17 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '
> )
> '
>
> +test_expect_success 'warn about incompatibilities (only) when writing' '
> + git init warn &&
> + test_commit -C warn initial &&
> + test_commit -C warn second &&
> + git -C warn replace --graft second &&
> + test_config -C warn gc.writecommitgraph true &&
> +
> + git -C warn gc 2>err &&
> + test_i18ngrep "skipping commit-graph" err &&
> + git -C warn rev-list -1 second 2>err &&
> + test_i18ngrep ! "skipping commit-graph" err
...I think it makes sense to have a "test_line_count = 1" here for these
sorts of warnings we know/suspect might come up N times if we don't
carefully tweak things.
I think (but have not tested) that we'll still write it 2 times if you
have the graph configured to write on fetch, and we end up also running
"gc" without gc.autoDetach.
In any case, here's a WIP patch to fix/demonstrate the bug here,
consider it to have my SOB (but no need to retain the authorship).
Obviously needs amending (e.g. the "0 &&" case, just to reproduce the
bug), and you might not want the s/int/enum/ cleanup while we're at it.
Also makes sense to add to add this case to the "gc --auto" test,
i.e. if we have writeCommitGraph=true && a --depth=1 repo do we have no
gc.log at the end as we should.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ecf85376050..73dacf927f9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1924,13 +1924,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
if (fetch_write_commit_graph > 0 ||
(fetch_write_commit_graph < 0 &&
the_repository->settings.fetch_write_commit_graph)) {
- int commit_graph_flags = COMMIT_GRAPH_WRITE_SPLIT;
+ enum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_SPLIT | COMMIT_GRAPH_WRITE_WARNINGS;
if (progress)
- commit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;
+ flags |= COMMIT_GRAPH_WRITE_PROGRESS;
write_commit_graph_reachable(the_repository->objects->odb,
- commit_graph_flags,
+ flags,
NULL);
}
diff --git a/builtin/gc.c b/builtin/gc.c
index 64f2b52d6e2..9109898eacb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -593,7 +593,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
/*
* Auto-gc should be least intrusive as possible.
*/
- if (!need_to_gc())
+ if (0 && !need_to_gc())
return 0;
if (!quiet) {
if (detach_auto)
@@ -689,10 +689,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
}
prepare_repo_settings(the_repository);
- if (the_repository->settings.gc_write_commit_graph == 1)
+ if (the_repository->settings.gc_write_commit_graph) {
+ enum commit_graph_write_flags flags = 0;
+ if (!quiet && !daemonized)
+ flags |= (COMMIT_GRAPH_WRITE_PROGRESS |
+ COMMIT_GRAPH_WRITE_WARNINGS);
+
write_commit_graph_reachable(the_repository->objects->odb,
- !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
- NULL);
+ flags, NULL);
+ }
if (auto_gc && too_many_loose_objects())
warning(_("There are too many unreachable loose objects; "
diff --git a/commit-graph.c b/commit-graph.c
index 245b7108e0d..8afc75b2dbe 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -198,7 +198,8 @@ static struct commit_graph *alloc_commit_graph(void)
extern int read_replace_refs;
-static int commit_graph_compatible(struct repository *r, int quiet)
+static int commit_graph_compatible(struct repository *r,
+ enum commit_graph_write_flags flags)
{
if (!r->gitdir)
return 0;
@@ -206,7 +207,7 @@ static int commit_graph_compatible(struct repository *r, int quiet)
if (read_replace_refs) {
prepare_replace_object(r);
if (hashmap_get_size(&r->objects->replace_map->map)) {
- if (!quiet)
+ if (flags & COMMIT_GRAPH_WRITE_WARNINGS)
warning(_("repository contains replace "
"objects; skipping commit-graph"));
return 0;
@@ -216,13 +217,13 @@ static int commit_graph_compatible(struct repository *r, int quiet)
prepare_commit_graft(r);
if (r->parsed_objects &&
(r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) {
- if (!quiet)
+ if (flags & COMMIT_GRAPH_WRITE_WARNINGS)
warning(_("repository contains (deprecated) grafts; "
"skipping commit-graph"));
return 0;
}
if (is_repository_shallow(r)) {
- if (!quiet)
+ if (flags & COMMIT_GRAPH_WRITE_WARNINGS)
warning(_("repository is shallow; skipping "
"commit-graph"));
return 0;
@@ -2127,7 +2128,7 @@ int write_commit_graph(struct object_directory *odb,
warning(_("attempting to write a commit-graph, but 'core.commitGraph' is disabled"));
return 0;
}
- if (!commit_graph_compatible(the_repository, 0))
+ if (!commit_graph_compatible(the_repository, flags))
return 0;
ctx = xcalloc(1, sizeof(struct write_commit_graph_context));
diff --git a/commit-graph.h b/commit-graph.h
index f8e92500c6e..0d2a12a5e58 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -95,9 +95,10 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r);
enum commit_graph_write_flags {
COMMIT_GRAPH_WRITE_APPEND = (1 << 0),
COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),
- COMMIT_GRAPH_WRITE_SPLIT = (1 << 2),
- COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3),
- COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS = (1 << 4),
+ COMMIT_GRAPH_WRITE_WARNINGS = (1 << 2),
+ COMMIT_GRAPH_WRITE_SPLIT = (1 << 3),
+ COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),
+ COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS = (1 << 5),
};
enum commit_graph_split_flags {
next prev parent reply other threads:[~2021-02-26 16:02 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-26 14:50 [PATCH] commit-graph: warn about incompatibilities only when trying to write Johannes Schindelin via GitGitGadget
2021-02-26 16:00 ` Ævar Arnfjörð Bjarmason [this message]
2021-02-26 22:39 ` Junio C Hamano
2021-02-28 16:20 ` Ævar Arnfjörð Bjarmason
2021-03-01 17:18 ` Junio C Hamano
2021-03-01 22:10 ` Johannes Schindelin
2021-03-01 22:21 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r1l27rae.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).