Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote:
>
>> From: Victoria Dye <vdye@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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.
