Re: [PATCH v2 09/10] scalar-diagnose: use 'git diagnose --all'

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.