Re: [PATCH v2 09/10] scalar-diagnose: use 'git diagnose --all'
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 4, 2022, 06:54 UTC
- Message-ID
- <220804.86r11webp8.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <6834bdcaea838cc49f209efd785bf2bdf09e9c08.1659577543.git.gitgitgadget@gmail.com>
On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote:
Show 57 quoted lines
> 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()..
Show 8 quoted lines
> - }
> + 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:
Show 5 quoted lines
> > -diagnose_cleanup: > - strbuf_release(&zip_path); > + strbuf_release(&diagnostics_root); > return res;
return res < 0 ? -res : res;
Or whatever.