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

[Victoria Dye <vdye@github.com>]

Æ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.