Re: [PATCH v2 3/6] Implement `scalar diagnose`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 4447 bytes --]

Hi René,

On Mon, 7 Feb 2022, René Scharfe wrote:

> > Note: originally, Scalar was implemented in C# using the .NET API, where
> > we had the luxury of a comprehensive standard library that includes
> > basic functionality such as writing a `.zip` file. In the C version, we
> > lack such a commodity. Rather than introducing a dependency on, say,
> > libzip, we slightly abuse Git's `archive` command: Instead of writing
> > the `.zip` file directly, we stage the file contents in a Git index of a
> > temporary, bare repository, only to let `git archive` have at it, and
> > finally removing the temporary repository.
> >
> > Also note: Due to the frequently-spawned `git hash-object` processes,
> > this command is quite a bit slow on Windows. Should it turn out to be a
> > big problem, the lack of a batch mode of the `hash-object` command could
> > potentially be worked around via using `git fast-import` with a crafted
> > `stdin`.
>
> The two paragraphs above are not in sync with the patch.

Whoopsie!

> > +	archiver_fd = xopen(zip_path.buf, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> > +	if (archiver_fd < 0 || dup2(archiver_fd, 1) < 0) {
> > +		res = error_errno(_("could not redirect output"));
> > +		goto diagnose_cleanup;
> > +	}
> > +
> > +	init_zip_archiver();
> > +	strvec_pushl(&archiver_args, "scalar-diagnose", "--format=zip", NULL);
> > +
> > +	strbuf_reset(&buf);
> > +	strbuf_addstr(&buf,
> > +		      "--add-file-with-content=diagnostics.log:"
> > +		      "Collecting diagnostic info\n\n");
> > +	get_version_info(&buf, 1);
> > +
> > +	strbuf_addf(&buf, "Enlistment root: %s\n", the_repository->worktree);
> > +	off = strchr(buf.buf, ':') + 1 - buf.buf;
> > +	write_or_die(stdout_fd, buf.buf + off, buf.len - off);
> > +	strvec_push(&archiver_args, buf.buf);
>
> Fun trick to reuse the buffer for both the ZIP entry and stdout. :)  I'd
> have omitted the option from buf and added it like this, for simplicity:
>
> 	strvec_pushf(&archiver_args,
> 		     "--add-file-with-content=diagnostics.log:%s", buf.buf);
>
> Just a thought.

Oh, that's even better. I did not like that `off` pattern at all but
forgot to think of `pushf()`. Thanks!

> > +
> > +	if ((res = add_directory_to_archiver(&archiver_args, ".git", 0)) ||
> > +	    (res = add_directory_to_archiver(&archiver_args, ".git/hooks", 0)) ||
> > +	    (res = add_directory_to_archiver(&archiver_args, ".git/info", 0)) ||
> > +	    (res = add_directory_to_archiver(&archiver_args, ".git/logs", 1)) ||
> > +	    (res = add_directory_to_archiver(&archiver_args, ".git/objects/info", 0)))
> > +		goto diagnose_cleanup;
> > +
> > +	strvec_pushl(&archiver_args, "--prefix=",
> > +		     oid_to_hex(the_hash_algo->empty_tree), "--", NULL);
> > +
> > +	/* `write_archive()` modifies the `argv` passed to it. Let it. */
> > +	argv_copy = xmemdupz(archiver_args.v,
> > +			     sizeof(char *) * archiver_args.nr);
>
> Leaking the whole thing would be fine as well for this command, but
> cleaning up is tidier, of course.
>
> > +	res = write_archive(archiver_args.nr, (const char **)argv_copy, NULL,
> > +			    the_repository, NULL, 0);
>
> Ah -- no shell means no command line length limits. :)

Yes!!!

It also makes the command a ridiculous amount faster on Windows.

> > +	if (res) {
> > +		error(_("failed to write archive"));
> > +		goto diagnose_cleanup;
> > +	}
> > +
> > +	if (!res)
> > +		printf("\n"
> > +		       "Diagnostics complete.\n"
> > +		       "All of the gathered info is captured in '%s'\n",
> > +		       zip_path.buf);
>
> Is this message appended to the ZIP file or does it go to stdout?

It goes to `stdout`, this is for the user who runs `scalar diagnose`.

Hmm.

Now that you pointed it out, I think I want it to go to `stderr` instead.

> In any case: mixing write(2) and stdio(3) is not a good idea.  Using
> fwrite(3) instead of write_or_die above and doing the stdout dup(2)
> dance only tightly around the write_archive call would help, I think.

Sure, but let's print this message to `stderr` instead, that'll be much
cleaner, right?

Alternatively, I think I'd rather move the `printf()` below...

>
> > +
> > +diagnose_cleanup:
> > +	if (archiver_fd >= 0) {
> > +		close(1);
> > +		dup2(stdout_fd, 1);
> > +	}

... this re-redirection.

What do you think? `stdout` or `stderr`?

Thank you for your review!
Dscho