git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] maintenance: core.commitGraph=false prevents writes
Date: Mon, 12 Oct 2020 10:30:33 -0700	[thread overview]
Message-ID: <xmqqwnzvpd1i.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <pull.749.git.1602509314545.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Mon, 12 Oct 2020 13:28:34 +0000")

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Recently, a user had an issue due to combining
> fetch.writeCommitGraph=true with core.commitGraph=false. The root bug
> has been resolved by preventing commit-graph writes when
> core.commitGraph is disabled. This happens inside the 'git commit-graph
> write' command, but we can be more aware of this situation and prevent
> that process from ever starting in the 'commit-graph' maintenance task.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     maintenance: core.commitGraph=false prevents writes
>     
>     As requested [1], this prevents the extra process when core.commitGraph
>     is disabled.

That's not a request.  I was just wondering aloud.

If you took inspiration from my thinking aloud, that is wonderful,
but the actual work to ensure it is not an idea that horribly breaks
some underlying assumption I didn't know about in the code and
deciding it is a good idea to do so is all done by you, so please
take the credit due.

>     This is based on ds/maintenance-commit-graph-auto-fix.
>     
>     [1] https://lore.kernel.org/git/xmqqft6nrtlw.fsf@gitster.c.googlers.com/
>     
>     Thanks, -Stolee

Hmph.  

There is a call to prepare_repo_settings() in cmd_gc().

I have to wonder if it should be done much earlier and in a more
central place, perhaps in cmd_maintenance() before anything else
happens.  Even though commit-graph may feel somewhat special only
because it is relatively new, it is not hard to imagine that other
maintenance tasks (both older ones and future ones) would eventually
want to have similar access to the feature settings.

It is OK to keep "the maintenance command works only in the single
repository", and not passing a "repo" that cmd_maintenance() would
prepare by calling prepare_repo_settings() down in the callchain, at
least right now, but we might want to consider doing so in the
future.

Thanks.


> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-749%2Fderrickstolee%2Fmaintenance-core-commit-graph-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-749/derrickstolee/maintenance-core-commit-graph-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/749
>
>  builtin/gc.c           | 4 ++++
>  t/t7900-maintenance.sh | 8 ++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 12ddb68bba..e80331c4e2 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -813,6 +813,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)
>  
>  static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)
>  {
> +	prepare_repo_settings(the_repository);
> +	if (!the_repository->settings.core_commit_graph)
> +		return 0;
> +
>  	close_object_store(the_repository->objects);
>  	if (run_write_commit_graph(opts)) {
>  		error(_("failed to write commit-graph"));
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index ee1f4a7ae4..9776154a2a 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -52,6 +52,14 @@ test_expect_success 'run --task=<task>' '
>  	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt
>  '
>  
> +test_expect_success 'core.commitGraph=false prevents write process' '
> +	GIT_TRACE2_EVENT="$(pwd)/no-commit-graph.txt" \
> +		git -c core.commitGraph=false maintenance run \
> +		--task=commit-graph 2>/dev/null &&
> +	test_subcommand ! git commit-graph write --split --reachable --no-progress \
> +		<no-commit-graph.txt
> +'
> +
>  test_expect_success 'commit-graph auto condition' '
>  	COMMAND="maintenance run --task=commit-graph --auto --quiet" &&
>  
>
> base-commit: 8f801804befa12a9c4ddff91275cf03612f1895d

  reply	other threads:[~2020-10-12 17:30 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-12 13:28 Derrick Stolee via GitGitGadget
2020-10-12 17:30 ` Junio C Hamano [this message]
2020-10-12 18:40   ` Derrick Stolee

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=xmqqwnzvpd1i.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --subject='Re: [PATCH] maintenance: core.commitGraph=false prevents writes' \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git