* [PATCH] builtin/gc: warn when core.commitGraph is disabled @ 2021-05-10 9:43 lilinchao 2021-05-10 11:55 ` Ævar Arnfjörð Bjarmason ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: lilinchao @ 2021-05-10 9:43 UTC (permalink / raw) To: git; +Cc: dstolee, gitster, Li Linchao From: Li Linchao <lilinchao@oschina.cn> Throw warning message when core.commitGraph is disabled in commit-graph maintenance task. Signed-off-by: Li Linchao <lilinchao@oschina.cn> --- builtin/gc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/gc.c b/builtin/gc.c index 98a803196b..90684ca3b3 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -861,8 +861,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) + if (!the_repository->settings.core_commit_graph) { + warning(_("skipping commit-graph task because core.commitGraph is disabled")); return 0; + } close_object_store(the_repository->objects); if (run_write_commit_graph(opts)) { -- 2.31.1.442.g7e39198978 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled 2021-05-10 9:43 [PATCH] builtin/gc: warn when core.commitGraph is disabled lilinchao @ 2021-05-10 11:55 ` Ævar Arnfjörð Bjarmason 2021-05-13 8:17 ` lilinchao 2021-05-10 18:12 ` Junio C Hamano [not found] ` <ceb22d54b18611ebb304a4badb2c2b1147508@gmail.com> 2 siblings, 1 reply; 6+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-05-10 11:55 UTC (permalink / raw) To: lilinchao; +Cc: git, dstolee, gitster 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... > Signed-off-by: Li Linchao <lilinchao@oschina.cn> > --- > builtin/gc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index 98a803196b..90684ca3b3 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -861,8 +861,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) > + if (!the_repository->settings.core_commit_graph) { > + warning(_("skipping commit-graph task because core.commitGraph is disabled")); > return 0; > + } > > close_object_store(the_repository->objects); > if (run_write_commit_graph(opts)) { ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled 2021-05-10 11:55 ` Ævar Arnfjörð Bjarmason @ 2021-05-13 8:17 ` lilinchao 2021-05-13 11:24 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 6+ messages in thread From: lilinchao @ 2021-05-13 8:17 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Derrick Stolee, Junio C Hamano > >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. 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. Thanks ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled 2021-05-13 8:17 ` lilinchao @ 2021-05-13 11:24 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 6+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-05-13 11:24 UTC (permalink / raw) To: lilinchao@oschina.cn; +Cc: git, Derrick Stolee, Junio C Hamano 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 :) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled 2021-05-10 9:43 [PATCH] builtin/gc: warn when core.commitGraph is disabled lilinchao 2021-05-10 11:55 ` Ævar Arnfjörð Bjarmason @ 2021-05-10 18:12 ` Junio C Hamano [not found] ` <ceb22d54b18611ebb304a4badb2c2b1147508@gmail.com> 2 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2021-05-10 18:12 UTC (permalink / raw) To: lilinchao; +Cc: git, dstolee lilinchao@oschina.cn writes: > From: Li Linchao <lilinchao@oschina.cn> > > Throw warning message when core.commitGraph is disabled in commit-graph > maintenance task. Why? If I said, with core.commitGraph, that I do not want to have anything to do with commitGraph, why should I get disturbed with such a warning message? > Signed-off-by: Li Linchao <lilinchao@oschina.cn> > --- > builtin/gc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index 98a803196b..90684ca3b3 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -861,8 +861,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) > + if (!the_repository->settings.core_commit_graph) { > + warning(_("skipping commit-graph task because core.commitGraph is disabled")); > return 0; > + } > > close_object_store(the_repository->objects); > if (run_write_commit_graph(opts)) { ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <ceb22d54b18611ebb304a4badb2c2b1147508@gmail.com>]
* Re: Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled [not found] ` <ceb22d54b18611ebb304a4badb2c2b1147508@gmail.com> @ 2021-05-11 2:17 ` lilinchao 0 siblings, 0 replies; 6+ messages in thread From: lilinchao @ 2021-05-11 2:17 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Junio C Hamano Cc: git, Derrick Stolee > >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... > Forgive me, I don't know any of your previous discussions. Sorry for disturbing. > >> Signed-off-by: Li Linchao <lilinchao@oschina.cn> >> --- >> builtin/gc.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/builtin/gc.c b/builtin/gc.c >> index 98a803196b..90684ca3b3 100644 >> --- a/builtin/gc.c >> +++ b/builtin/gc.c >> @@ -861,8 +861,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) >> + if (!the_repository->settings.core_commit_graph) { >> + warning(_("skipping commit-graph task because core.commitGraph is disabled")); >> return 0; >> + } >> >> close_object_store(the_repository->objects); >> if (run_write_commit_graph(opts)) { > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-13 11:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-10 9:43 [PATCH] builtin/gc: warn when core.commitGraph is disabled 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 2021-05-10 18:12 ` Junio C Hamano [not found] ` <ceb22d54b18611ebb304a4badb2c2b1147508@gmail.com> 2021-05-11 2:17 ` lilinchao
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).