Ævar Arnfjörð Bjarmason wrote:

On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote:

quoted

From: Victoria Dye <redacted>

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

quoted

-	}
+	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:

quoted

 
-diagnose_cleanup:
-	strbuf_release(&zip_path);
+	strbuf_release(&diagnostics_root);
 	return res;

	return res < 0 ? -res : res;

Or whatever.