git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "lilinchao@oschina.cn" <lilinchao@oschina.cn>
Cc: git <git@vger.kernel.org>, Derrick Stolee <dstolee@microsoft.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled
Date: Thu, 13 May 2021 13:24:38 +0200	[thread overview]
Message-ID: <87k0o2svru.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <b87b12b4b3c311eba1fd0024e87935e7@oschina.cn>


On Thu, May 13 2021, lilinchao@oschina.cn wrote:

>>
>>On Mon, May 10 2021, lilinchao@oschina.cn wrote:
>>
>>> From: Li Linchao <lilinchao@oschina.cn>
>>>
>>> Throw warning message when core.commitGraph is disabled in commit-graph
>>> maintenance task.
>>
>>Won't this cause the gc.log issue noted in
>>https://lore.kernel.org/git/87r1l27rae.fsf@evledraar.gmail.com/
>>
>>More importantly, I don't think this UX makes sense. We said we didn't
>>want it, so why warn about it?
>>
>>Maybe there are good reasons to, but this commit message / patch doesn't
>>make the case for it...
>>
> Uh, well, maybe I should argue for this patch a bit more.

> First this is in git maintenance task, I've read the link you post,
> and I feel it has nothing to do with maintenance task.

Yes, maybe the issue I noted with gc.log being populated because
something wrote to stderr won't happen here. I was just asking if you'd
taken it into account.

> Second I hope the `commit-graph` task can do the same thing with
> `incremental repack` task that to warn user when the related necessary
> setting is not yet ready, instead of running quietly, but doing
> nothing.

I agree that if you ask git to --do-stuff and core.stuff=false then we
should probably emit a warning, "but you disabled stuff!".

In this case though, isn't the entry point just "incremental", we then
check should_write_commit_graph (as an aside, and not new in your
change, shouldn't this "is the config set" be moved there & be the first
thing we check?).

Then we run maintenance_task_commit_graph, where we see that the config
is disabled.

So aren't there users that want incremental maintenance, but have also
disabled core.commitGraph (or writeCommitGraph or whatever), that are
then going to get a warning about something they explicitly disabled?

Or is this warning just for cases where the user has somehow scheduled a
"commit graph" task, with config disabled, and the task can therefore
never do anything useful? I agree that in such a case it probably makes
sense to warn (or just exit non-zero?).

Anyway, I really do mean what I said about the "issue" being that the
commit message needs to "make a case for it". I.e. even as someone who's
hacked on the gc.c code (although not since "maintenance" became a
thing), I honestly don't know if this warning is OK, i.e. are we at the
point where we issue it where the user really wants the commit graph,
but we just have a misconfiguration?

I *suspect* not, and that it's going to be annoying to people who really
don't want the commit-graph, but who run "incremental", but maybe I'm
wrong.

If that is the case maybe there's still a case for saying something, but
that should probably be an advise(), not a warning().

Or the whole thing is fine, I honestly don't know :)

  reply	other threads:[~2021-05-13 11:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-10  9:43 lilinchao
2021-05-10 11:55 ` Ævar Arnfjörð Bjarmason
2021-05-13  8:17   ` lilinchao
2021-05-13 11:24     ` Ævar Arnfjörð Bjarmason [this message]
2021-05-10 18:12 ` Junio C Hamano
     [not found] ` <ceb22d54b18611ebb304a4badb2c2b1147508@gmail.com>
2021-05-11  2:17   ` lilinchao

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=87k0o2svru.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=lilinchao@oschina.cn \
    --subject='Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled' \
    /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