| Message ID | 20231014135302.13095-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | diagnose: require repository | expand |
On Sat, Oct 14, 2023, at 15:53, Martin Ågren wrote: > Switch from the gentle setup to requiring a git directory. Without a git > repo, there isn't really much to diagnose. This patch works for me. Thanks.
Martin Ågren <martin.agren@gmail.com> writes: > When `git diagnose` is run from outside a repo, it begins collecting > various information before eventually hitting a segmentation fault, > leaving an incomplete zip file behind. > > Switch from the gentle setup to requiring a git directory. Without a git > repo, there isn't really much to diagnose. > > We could possibly do a best-effort collection of information about the > machine and then give up. That would roughly be today's behavior but > with a controlled exit rather than a segfault. However, the purpose of > this tool is largely to create a zip archive. Rather than creating an > empty zip file or no zip file at all, and having to explain that > behavior, it seems more helpful to bail out clearly and early with a > succinct error message. Without having thought things through, offhand I agree with your "no repository? there is nothing worth tarring up then" assessment. Because "git bugreport --diag" unconditionally spawns "git diagnose", the former may also want to be extra careful, perhaps like the attached patch. builtin/bugreport.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git c/builtin/bugreport.c w/builtin/bugreport.c index d2ae5c305d..ac9e05fcf7 100644 --- c/builtin/bugreport.c +++ w/builtin/bugreport.c @@ -146,6 +146,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) report_path.buf); } + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) { + warning(_("no repository--diagnostic output disabled")); + diagnose = DIAGNOSE_NONE; + } + /* Prepare diagnostics, if requested */ if (diagnose != DIAGNOSE_NONE) { struct strbuf zip_path = STRBUF_INIT;
On Sat, 14 Oct 2023 at 19:15, Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > > Switch from the gentle setup to requiring a git directory. Without a git > > repo, there isn't really much to diagnose. > > > > We could possibly do a best-effort collection of information about the > > machine and then give up. That would roughly be today's behavior but > > with a controlled exit rather than a segfault. However, the purpose of > > this tool is largely to create a zip archive. Rather than creating an > > empty zip file or no zip file at all, and having to explain that Correcting myself: The zip archive would actually contain `diagnostics.log` with some general info about the machine and Git build. > > behavior, it seems more helpful to bail out clearly and early with a > > succinct error message. > > Without having thought things through, offhand I agree with your "no > repository? there is nothing worth tarring up then" assessment. > > Because "git bugreport --diag" unconditionally spawns "git > diagnose", the former may also want to be extra careful, perhaps > like the attached patch. Good point. TBH, I had no idea about `git bugreport --diagnose`. > + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) { > + warning(_("no repository--diagnostic output disabled")); > + diagnose = DIAGNOSE_NONE; > + } > + When the user explicitly provides that option, it seems unfortunate to me to drop it. Yes, we'd warn, but `git bugreport` then pops a text editor, so you would only see the warning after finishing up the report. (Maybe. By the time you quit your editor, you might not consider checking the terminal for warnings and such.) So I'm inclined to instead just die if we see the option outside a repo. If `diagnose` the command fundamentally requires a repo (as with my patch) it seems surprising to me to not have `--diagnose` the option behave the same. Martin
Martin Ågren <martin.agren@gmail.com> writes: > Correcting myself: The zip archive would actually contain > `diagnostics.log` with some general info about the machine and Git > build. So it could contain some useful information without a specific repository, perhaps. > Good point. TBH, I had no idea about `git bugreport --diagnose`. You are not alone ;-) I didn't, either. Before responding to your patch, that is. >> + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) { >> + warning(_("no repository--diagnostic output disabled")); >> + diagnose = DIAGNOSE_NONE; >> + } >> + > > When the user explicitly provides that option, it seems unfortunate to > me to drop it. Yes, we'd warn, but `git bugreport` then pops a text > editor, so you would only see the warning after finishing up the report. > (Maybe. By the time you quit your editor, you might not consider > checking the terminal for warnings and such.) > > So I'm inclined to instead just die if we see the option outside a repo. > If `diagnose` the command fundamentally requires a repo (as with my > patch) it seems surprising to me to not have `--diagnose` the option > behave the same. I have no strong opinion. Victoria is on Cc: already, whose name appears a lot more often than mine in the shortlog for "diagnose" stuff, so I'll defer to her area expertise. Thanks.
Martin Ågren wrote: >>> behavior, it seems more helpful to bail out clearly and early with a >>> succinct error message. >> >> Without having thought things through, offhand I agree with your "no >> repository? there is nothing worth tarring up then" assessment. >> >> Because "git bugreport --diag" unconditionally spawns "git >> diagnose", the former may also want to be extra careful, perhaps >> like the attached patch. > > Good point. TBH, I had no idea about `git bugreport --diagnose`. > >> + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) { >> + warning(_("no repository--diagnostic output disabled")); >> + diagnose = DIAGNOSE_NONE; >> + } >> + > > When the user explicitly provides that option, it seems unfortunate to > me to drop it. Yes, we'd warn, but `git bugreport` then pops a text > editor, so you would only see the warning after finishing up the report. > (Maybe. By the time you quit your editor, you might not consider > checking the terminal for warnings and such.) > > So I'm inclined to instead just die if we see the option outside a repo. > If `diagnose` the command fundamentally requires a repo (as with my > patch) it seems surprising to me to not have `--diagnose` the option > behave the same. I agree - it was an oversight on my part to not firmly require the existence of a repository with 'git diagnose', and the same applies to 'bugreport --diagnose'. For reference, there is one other usage of 'git diagnose' (in 'scalar diagnose'). However, it's already guarded by 'setup_git_directory()' so it shouldn't need to be updated. > > Martin
diff --git a/t/t0092-diagnose.sh b/t/t0092-diagnose.sh index 133e5747d6..49671d35a2 100755 --- a/t/t0092-diagnose.sh +++ b/t/t0092-diagnose.sh @@ -5,6 +5,11 @@ test_description='git diagnose' TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh +test_expect_success 'nothing to diagnose without repo' ' + nongit test_must_fail git diagnose 2>err && + grep "not a git repository" err +' + test_expect_success UNZIP 'creates diagnostics zip archive' ' test_when_finished rm -rf report && diff --git a/git.c b/git.c index c67e44dd82..ff04a74bbd 100644 --- a/git.c +++ b/git.c @@ -525,7 +525,7 @@ static struct cmd_struct commands[] = { { "credential-cache--daemon", cmd_credential_cache_daemon }, { "credential-store", cmd_credential_store }, { "describe", cmd_describe, RUN_SETUP }, - { "diagnose", cmd_diagnose, RUN_SETUP_GENTLY }, + { "diagnose", cmd_diagnose, RUN_SETUP }, { "diff", cmd_diff, NO_PARSEOPT }, { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT }, { "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
When `git diagnose` is run from outside a repo, it begins collecting various information before eventually hitting a segmentation fault, leaving an incomplete zip file behind. Switch from the gentle setup to requiring a git directory. Without a git repo, there isn't really much to diagnose. We could possibly do a best-effort collection of information about the machine and then give up. That would roughly be today's behavior but with a controlled exit rather than a segfault. However, the purpose of this tool is largely to create a zip archive. Rather than creating an empty zip file or no zip file at all, and having to explain that behavior, it seems more helpful to bail out clearly and early with a succinct error message. Reported-by: ks1322 ks1322 <ks1322@gmail.com> Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- Thanks for the report. This could be one way of fixing this. I haven't found anything in the original submission [1] discussing this "_GENTLY". I didn't see anything in the implementation or the tests suggesting that it was intentional to run outside a git repo. [1] https://lore.kernel.org/git/xmqqzgg1nz6v.fsf@gitster.g/t/#mc66904caab6bc79e57eaf5063df268b2725b6fcc t/t0092-diagnose.sh | 5 +++++ git.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)