| Message ID | 6834bdcaea838cc49f209efd785bf2bdf09e9c08.1659577543.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Generalize 'scalar diagnose' into 'git diagnose' and 'git bugreport --diagnose' | expand |
On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote: > From: Victoria Dye <vdye@github.com> > > Replace implementation of 'scalar diagnose' with an internal invocation of > 'git diagnose --all'. This simplifies the implementation of 'cmd_diagnose' > by making it a direct alias of 'git diagnose' and removes some code in > 'scalar.c' that is duplicated in 'builtin/diagnose.c'. The simplicity of the > alias also sets up a clean deprecation path for 'scalar diagnose' (in favor > of 'git diagnose'), if that is desired in the future. > > This introduces one minor change to the output of 'scalar diagnose', which > is that the prefix of the created zip archive is changed from 'scalar_' to > 'git-diagnostics-'. > > Signed-off-by: Victoria Dye <vdye@github.com> > --- > contrib/scalar/scalar.c | 29 +++++++---------------------- > 1 file changed, 7 insertions(+), 22 deletions(-) > > diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c > index b10955531ce..fe2a0e9decb 100644 > --- a/contrib/scalar/scalar.c > +++ b/contrib/scalar/scalar.c > @@ -11,7 +11,6 @@ > #include "dir.h" > #include "packfile.h" > #include "help.h" > -#include "diagnose.h" > > /* > * Remove the deepest subdirectory in the provided path string. Path must not > @@ -510,34 +509,20 @@ static int cmd_diagnose(int argc, const char **argv) > N_("scalar diagnose [<enlistment>]"), > NULL > }; > - struct strbuf zip_path = STRBUF_INIT; > - time_t now = time(NULL); > - struct tm tm; > + struct strbuf diagnostics_root = STRBUF_INIT; > int res = 0; > > argc = parse_options(argc, argv, NULL, options, > usage, 0); > > - setup_enlistment_directory(argc, argv, usage, options, &zip_path); > - > - strbuf_addstr(&zip_path, "/.scalarDiagnostics/scalar_"); > - strbuf_addftime(&zip_path, > - "%Y%m%d_%H%M%S", localtime_r(&now, &tm), 0, 0); > - strbuf_addstr(&zip_path, ".zip"); > - switch (safe_create_leading_directories(zip_path.buf)) { > - case SCLD_EXISTS: > - case SCLD_OK: > - break; > - default: > - error_errno(_("could not create directory for '%s'"), > - zip_path.buf); > - goto diagnose_cleanup; Just spotting this now, but we had ad error, but we "goto diagnose_cleanup", but that will use our "res = 0" above. Is this untested already or in this series (didn't go back to look). But maybe a moot point, the post-image replacement uses die().. > - } > + setup_enlistment_directory(argc, argv, usage, options, &diagnostics_root); > + strbuf_addstr(&diagnostics_root, "/.scalarDiagnostics"); > > - res = create_diagnostics_archive(&zip_path, 1); > + if (run_git("diagnose", "--all", "-s", "%Y%m%d_%H%M%S", > + "-o", diagnostics_root.buf, NULL) < 0) > + res = -1; The code handling here seems really odd, issues: * This *can* return -1, if start_command() fails, but that's by far the rarer case, usually it would be 0 or >0 (only <0 if we can't start the command at all). * You should not be returning -1 from cmd_*() in general (we have outstanding issues with it, but those should be fixed). It will yield an exit code of 255 (but it's not portable)). * If you're going to return -1 at all, why override <0 with -1, just "res = run_git(...)" instead? I think all-in-all this should be: res = run_git(...); Then: > > -diagnose_cleanup: > - strbuf_release(&zip_path); > + strbuf_release(&diagnostics_root); > return res; return res < 0 ? -res : res; Or whatever.
Ævar Arnfjörð Bjarmason wrote: > > On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote: > >> From: Victoria Dye <vdye@github.com> >> >> Replace implementation of 'scalar diagnose' with an internal invocation of >> 'git diagnose --all'. This simplifies the implementation of 'cmd_diagnose' >> by making it a direct alias of 'git diagnose' and removes some code in >> 'scalar.c' that is duplicated in 'builtin/diagnose.c'. The simplicity of the >> alias also sets up a clean deprecation path for 'scalar diagnose' (in favor >> of 'git diagnose'), if that is desired in the future. >> >> This introduces one minor change to the output of 'scalar diagnose', which >> is that the prefix of the created zip archive is changed from 'scalar_' to >> 'git-diagnostics-'. >> >> Signed-off-by: Victoria Dye <vdye@github.com> >> --- >> contrib/scalar/scalar.c | 29 +++++++---------------------- >> 1 file changed, 7 insertions(+), 22 deletions(-) >> >> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c >> index b10955531ce..fe2a0e9decb 100644 >> --- a/contrib/scalar/scalar.c >> +++ b/contrib/scalar/scalar.c >> @@ -11,7 +11,6 @@ >> #include "dir.h" >> #include "packfile.h" >> #include "help.h" >> -#include "diagnose.h" >> >> /* >> * Remove the deepest subdirectory in the provided path string. Path must not >> @@ -510,34 +509,20 @@ static int cmd_diagnose(int argc, const char **argv) >> N_("scalar diagnose [<enlistment>]"), >> NULL >> }; >> - struct strbuf zip_path = STRBUF_INIT; >> - time_t now = time(NULL); >> - struct tm tm; >> + struct strbuf diagnostics_root = STRBUF_INIT; >> int res = 0; >> >> argc = parse_options(argc, argv, NULL, options, >> usage, 0); >> >> - setup_enlistment_directory(argc, argv, usage, options, &zip_path); >> - >> - strbuf_addstr(&zip_path, "/.scalarDiagnostics/scalar_"); >> - strbuf_addftime(&zip_path, >> - "%Y%m%d_%H%M%S", localtime_r(&now, &tm), 0, 0); >> - strbuf_addstr(&zip_path, ".zip"); >> - switch (safe_create_leading_directories(zip_path.buf)) { >> - case SCLD_EXISTS: >> - case SCLD_OK: >> - break; >> - default: >> - error_errno(_("could not create directory for '%s'"), >> - zip_path.buf); >> - goto diagnose_cleanup; > > Just spotting this now, but we had ad error, but we "goto > diagnose_cleanup", but that will use our "res = 0" above. > > Is this untested already or in this series (didn't go back to look). But > maybe a moot point, the post-image replacement uses die().. Nice catch - this does appear to be a pre-existing bug in 'scalar diagnose'. Given that both 'git diagnose' and 'git bugreport --diagnose' handle this case more appropriately, though, I agree that it's a bit of a moot point and not worth the churn created by a bugfix patch. > >> - } >> + setup_enlistment_directory(argc, argv, usage, options, &diagnostics_root); >> + strbuf_addstr(&diagnostics_root, "/.scalarDiagnostics"); >> >> - res = create_diagnostics_archive(&zip_path, 1); >> + if (run_git("diagnose", "--all", "-s", "%Y%m%d_%H%M%S", >> + "-o", diagnostics_root.buf, NULL) < 0) >> + res = -1; > > The code handling here seems really odd, issues: > > * This *can* return -1, if start_command() fails, but that's by far the > rarer case, usually it would be 0 or >0 (only <0 if we can't start > the command at all). > > * You should not be returning -1 from cmd_*() in general (we have > outstanding issues with it, but those should be fixed). It will yield > an exit code of 255 (but it's not portable)). > > * If you're going to return -1 at all, why override <0 with -1, just > "res = run_git(...)" instead? Thanks for the info, I'll replace the hardcoded '-1' return value with something derived from 'res' in the next version. > > I think all-in-all this should be: > > res = run_git(...); > > Then: > >> >> -diagnose_cleanup: >> - strbuf_release(&zip_path); >> + strbuf_release(&diagnostics_root); >> return res; > > return res < 0 ? -res : res; > > Or whatever.
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index b10955531ce..fe2a0e9decb 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -11,7 +11,6 @@ #include "dir.h" #include "packfile.h" #include "help.h" -#include "diagnose.h" /* * Remove the deepest subdirectory in the provided path string. Path must not @@ -510,34 +509,20 @@ static int cmd_diagnose(int argc, const char **argv) N_("scalar diagnose [<enlistment>]"), NULL }; - struct strbuf zip_path = STRBUF_INIT; - time_t now = time(NULL); - struct tm tm; + struct strbuf diagnostics_root = STRBUF_INIT; int res = 0; argc = parse_options(argc, argv, NULL, options, usage, 0); - setup_enlistment_directory(argc, argv, usage, options, &zip_path); - - strbuf_addstr(&zip_path, "/.scalarDiagnostics/scalar_"); - strbuf_addftime(&zip_path, - "%Y%m%d_%H%M%S", localtime_r(&now, &tm), 0, 0); - strbuf_addstr(&zip_path, ".zip"); - switch (safe_create_leading_directories(zip_path.buf)) { - case SCLD_EXISTS: - case SCLD_OK: - break; - default: - error_errno(_("could not create directory for '%s'"), - zip_path.buf); - goto diagnose_cleanup; - } + setup_enlistment_directory(argc, argv, usage, options, &diagnostics_root); + strbuf_addstr(&diagnostics_root, "/.scalarDiagnostics"); - res = create_diagnostics_archive(&zip_path, 1); + if (run_git("diagnose", "--all", "-s", "%Y%m%d_%H%M%S", + "-o", diagnostics_root.buf, NULL) < 0) + res = -1; -diagnose_cleanup: - strbuf_release(&zip_path); + strbuf_release(&diagnostics_root); return res; }