{"thread":{"id":"55657","subject":"[PATCH] builtin/gc: warn when core.commitGraph is disabled","startedAt":"2021-05-10T09:44:35Z","lastAt":"2021-05-13T11:45:09Z","messageCount":6,"participants":["lilinchao@oschina.cn","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424011","messageId":"510425b8b17411eb93770026b95c99cc@oschina.cn","threadId":"55657","inReplyTo":null,"subject":"[PATCH] builtin/gc: warn when core.commitGraph is disabled","fromName":"","fromEmail":"lilinchao@oschina.cn","sentAt":"2021-05-10T09:43:43Z","receivedAt":"2021-05-10T09:44:35Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":"From: Li Linchao <lilinchao@oschina.cn>\n\nThrow warning message when core.commitGraph is disabled in commit-graph\nmaintenance task.\n\nSigned-off-by: Li Linchao <lilinchao@oschina.cn>\n---\n builtin/gc.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 98a803196b..90684ca3b3 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -861,8 +861,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n {\n \tprepare_repo_settings(the_repository);\n-\tif (!the_repository->settings.core_commit_graph)\n+\tif (!the_repository->settings.core_commit_graph) {\n+\t\twarning(_(\"skipping commit-graph task because core.commitGraph is disabled\"));\n \t\treturn 0;\n+\t}\n \n \tclose_object_store(the_repository->objects);\n \tif (run_write_commit_graph(opts)) {\n-- \n2.31.1.442.g7e39198978\n\n"},{"id":"424022","messageId":"87tunau7ia.fsf@evledraar.gmail.com","threadId":"55657","inReplyTo":"510425b8b17411eb93770026b95c99cc@oschina.cn","subject":"Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-10T11:55:53Z","receivedAt":"2021-05-10T12:51:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 10 2021, lilinchao@oschina.cn wrote:\n\n> From: Li Linchao <lilinchao@oschina.cn>\n>\n> Throw warning message when core.commitGraph is disabled in commit-graph\n> maintenance task.\n\nWon't this cause the gc.log issue noted in\nhttps://lore.kernel.org/git/87r1l27rae.fsf@evledraar.gmail.com/\n\nMore importantly, I don't think this UX makes sense. We said we didn't\nwant it, so why warn about it?\n\nMaybe there are good reasons to, but this commit message / patch doesn't\nmake the case for it...\n\n\n> Signed-off-by: Li Linchao <lilinchao@oschina.cn>\n> ---\n>  builtin/gc.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 98a803196b..90684ca3b3 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -861,8 +861,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n>  static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n>  {\n>  \tprepare_repo_settings(the_repository);\n> -\tif (!the_repository->settings.core_commit_graph)\n> +\tif (!the_repository->settings.core_commit_graph) {\n> +\t\twarning(_(\"skipping commit-graph task because core.commitGraph is disabled\"));\n>  \t\treturn 0;\n> +\t}\n>  \n>  \tclose_object_store(the_repository->objects);\n>  \tif (run_write_commit_graph(opts)) {\n\n"},{"id":"424063","messageId":"xmqq4kfaqwyv.fsf@gitster.g","threadId":"55657","inReplyTo":"510425b8b17411eb93770026b95c99cc@oschina.cn","subject":"Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-10T18:12:40Z","receivedAt":"2021-05-10T18:12:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lilinchao@oschina.cn writes:\n\n> From: Li Linchao <lilinchao@oschina.cn>\n>\n> Throw warning message when core.commitGraph is disabled in commit-graph\n> maintenance task.\n\nWhy?  If I said, with core.commitGraph, that I do not want to have\nanything to do with commitGraph, why should I get disturbed with\nsuch a warning message?\n\n\n> Signed-off-by: Li Linchao <lilinchao@oschina.cn>\n> ---\n>  builtin/gc.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 98a803196b..90684ca3b3 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -861,8 +861,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n>  static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n>  {\n>  \tprepare_repo_settings(the_repository);\n> -\tif (!the_repository->settings.core_commit_graph)\n> +\tif (!the_repository->settings.core_commit_graph) {\n> +\t\twarning(_(\"skipping commit-graph task because core.commitGraph is disabled\"));\n>  \t\treturn 0;\n> +\t}\n>  \n>  \tclose_object_store(the_repository->objects);\n>  \tif (run_write_commit_graph(opts)) {\n"},{"id":"424089","messageId":"fa99dc5ab1fe11eb92230026b95c99cc@oschina.cn","threadId":"55657","inReplyTo":"ceb22d54b18611ebb304a4badb2c2b1147508@gmail.com","subject":"Re: Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2021-05-11T02:17:03Z","receivedAt":"2021-05-11T02:17:09Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":">\n>On Mon, May 10 2021, lilinchao@oschina.cn wrote:\n>\n>> From: Li Linchao <lilinchao@oschina.cn>\n>>\n>> Throw warning message when core.commitGraph is disabled in commit-graph\n>> maintenance task.\n>\n>Won't this cause the gc.log issue noted in\n>https://lore.kernel.org/git/87r1l27rae.fsf@evledraar.gmail.com/\n>\n>More importantly, I don't think this UX makes sense. We said we didn't\n>want it, so why warn about it?\n>\n>Maybe there are good reasons to, but this commit message / patch doesn't\n>make the case for it...\n> \nForgive me, I don't know any of your previous discussions.\nSorry for disturbing.\n\n>\n>> Signed-off-by: Li Linchao <lilinchao@oschina.cn>\n>> ---\n>>  builtin/gc.c | 4 +++-\n>>  1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index 98a803196b..90684ca3b3 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -861,8 +861,10 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n>>  static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n>>  {\n>>  prepare_repo_settings(the_repository);\n>> -\tif (!the_repository->settings.core_commit_graph)\n>> +\tif (!the_repository->settings.core_commit_graph) {\n>> +\twarning(_(\"skipping commit-graph task because core.commitGraph is disabled\"));\n>>  return 0;\n>> +\t}\n>> \n>>  close_object_store(the_repository->objects);\n>>  if (run_write_commit_graph(opts)) {\n>"},{"id":"424444","messageId":"b87b12b4b3c311eba1fd0024e87935e7@oschina.cn","threadId":"55657","inReplyTo":"87tunau7ia.fsf@evledraar.gmail.com","subject":"Re: Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2021-05-13T08:17:54Z","receivedAt":"2021-05-13T08:18:21Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":"\n>\n>On Mon, May 10 2021, lilinchao@oschina.cn wrote:\n>\n>> From: Li Linchao <lilinchao@oschina.cn>\n>>\n>> Throw warning message when core.commitGraph is disabled in commit-graph\n>> maintenance task.\n>\n>Won't this cause the gc.log issue noted in\n>https://lore.kernel.org/git/87r1l27rae.fsf@evledraar.gmail.com/\n>\n>More importantly, I don't think this UX makes sense. We said we didn't\n>want it, so why warn about it?\n>\n>Maybe there are good reasons to, but this commit message / patch doesn't\n>make the case for it...\n>\nUh, well, maybe I should argue for this patch a bit more.\n\nFirst this is in git maintenance task, I've read the link you post, and I feel it has nothing to do with maintenance task.\n\nSecond 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.\n\nThanks"},{"id":"424462","messageId":"87k0o2svru.fsf@evledraar.gmail.com","threadId":"55657","inReplyTo":"b87b12b4b3c311eba1fd0024e87935e7@oschina.cn","subject":"Re: [PATCH] builtin/gc: warn when core.commitGraph is disabled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-13T11:24:38Z","receivedAt":"2021-05-13T11:45:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 13 2021, lilinchao@oschina.cn wrote:\n\n>>\n>>On Mon, May 10 2021, lilinchao@oschina.cn wrote:\n>>\n>>> From: Li Linchao <lilinchao@oschina.cn>\n>>>\n>>> Throw warning message when core.commitGraph is disabled in commit-graph\n>>> maintenance task.\n>>\n>>Won't this cause the gc.log issue noted in\n>>https://lore.kernel.org/git/87r1l27rae.fsf@evledraar.gmail.com/\n>>\n>>More importantly, I don't think this UX makes sense. We said we didn't\n>>want it, so why warn about it?\n>>\n>>Maybe there are good reasons to, but this commit message / patch doesn't\n>>make the case for it...\n>>\n> Uh, well, maybe I should argue for this patch a bit more.\n\n> First this is in git maintenance task, I've read the link you post,\n> and I feel it has nothing to do with maintenance task.\n\nYes, maybe the issue I noted with gc.log being populated because\nsomething wrote to stderr won't happen here. I was just asking if you'd\ntaken it into account.\n\n> Second I hope the `commit-graph` task can do the same thing with\n> `incremental repack` task that to warn user when the related necessary\n> setting is not yet ready, instead of running quietly, but doing\n> nothing.\n\nI agree that if you ask git to --do-stuff and core.stuff=false then we\nshould probably emit a warning, \"but you disabled stuff!\".\n\nIn this case though, isn't the entry point just \"incremental\", we then\ncheck should_write_commit_graph (as an aside, and not new in your\nchange, shouldn't this \"is the config set\" be moved there & be the first\nthing we check?).\n\nThen we run maintenance_task_commit_graph, where we see that the config\nis disabled.\n\nSo aren't there users that want incremental maintenance, but have also\ndisabled core.commitGraph (or writeCommitGraph or whatever), that are\nthen going to get a warning about something they explicitly disabled?\n\nOr is this warning just for cases where the user has somehow scheduled a\n\"commit graph\" task, with config disabled, and the task can therefore\nnever do anything useful? I agree that in such a case it probably makes\nsense to warn (or just exit non-zero?).\n\nAnyway, I really do mean what I said about the \"issue\" being that the\ncommit message needs to \"make a case for it\". I.e. even as someone who's\nhacked on the gc.c code (although not since \"maintenance\" became a\nthing), I honestly don't know if this warning is OK, i.e. are we at the\npoint where we issue it where the user really wants the commit graph,\nbut we just have a misconfiguration?\n\nI *suspect* not, and that it's going to be annoying to people who really\ndon't want the commit-graph, but who run \"incremental\", but maybe I'm\nwrong.\n\nIf that is the case maybe there's still a case for saying something, but\nthat should probably be an advise(), not a warning().\n\nOr the whole thing is fine, I honestly don't know :)\n"}]}