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
