Re: [PATCH 2/7] builtin/bugreport.c: create '--diagnose' option

[Junio C Hamano <gitster@pobox.com>]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Victoria Dye <vdye@github.com>
>
> Create a '--diagnose' option for 'git bugreport' to collect additional
> information about the repository and write it to a zipped archive.
>
> The "diagnose" functionality was originally implemented for Scalar in
> aa5c79a331 (scalar: implement `scalar diagnose`, 2022-05-28). However, the
> diagnostics gathered are not specific to Scalar-cloned repositories and
> could be useful when diagnosing issues in any Git repository.
>
> Note that, while this patch appears large, it is mostly copied directly out
> of 'scalar.c'. Specifically, the functions
>
> - dir_file_stats_objects()
> - dir_file_stats()
> - count_files()
> - loose_objs_stats()
> - add_directory_to_archiver()
> - get_disk_info()

Yup.  As this does not "move" code across from older place to the
new home, it takes a bit of processing to verify the above claim,
but

 $ git blame -C -C -C -s -b master.. -- builtin/bugreport.c

shows that these are largely verbatim copies.

> +#ifndef WIN32
> +#include <sys/statvfs.h>
> +#endif
> +
> +static int get_disk_info(struct strbuf *out)
> +{
> +#ifdef WIN32
> +	struct strbuf buf = STRBUF_INIT;
> +...
> +	strbuf_addf(out, "Available space on '%s': ", buf.buf);
> +	strbuf_humanise_bytes(out, avail2caller.QuadPart);
> +...
> +#else
> +...
> +	strbuf_addf(out, "Available space on '%s': ", buf.buf);
> +	strbuf_humanise_bytes(out, st_mult(stat.f_bsize, stat.f_bavail));
> +...
> +#endif
> +	return 0;
> +}

As a proper part of Git, this part should probably be factored out
so that a platform specific helper function, implemented in compat/
layer, grabs "available disk space" number in off_t and the caller
of the above function becomes

	strbuf_realpath(&dir, ".", 1);
	strbuf_addf(out, "Available space on '%s:' ", dir.buf);
	strbuf_humanise_bytes(out, get_disk_size(dir.buf));

or something, without having to have #ifdef droppings.

> +static int create_diagnostics_archive(struct strbuf *zip_path)
> +{

Large part of this function is also lifted from scalar, and it looks
OK.  One thing I noticed is that "res" is explicitly initialized to
0, but given that the way the code is structured to use the "we
process sequencially in successful case, and branch out by 'goto'
immediately when we see a breakage" pattern, it may be better to
initialize it to -1 (i.e. assume error), or even better, leave it
uninitialized (i.e. let the compiler notice if a jump to cleanup is
made without setting res appropriately).

> +diagnose_cleanup:
> +	if (archiver_fd >= 0) {
> +		close(1);
> +		dup2(stdout_fd, 1);
> +	}
> +	free(argv_copy);
> +	strvec_clear(&archiver_args);
> +	strbuf_release(&buf);

Hmph, stdout_fd is a copy of the file descriptor 1 that was saved
away at the beginning.  Then archiver_fd was created to write into
the zip archive, and during the bulk of the function it was dup2'ed
to the file descriptor 1, to make anything written to the latter
appear in the zip output.

When we successfully opened archive_fd but failed to dup2(), we may
close a wrong file desciptor 1 here, but we recover from that by
using the saved-away stdout_fd, so we'd be OK.  If we did dup2(),
then we would be OK, too.

I am wondering if archiver_fd itself is leaking here, though.

Also, if we failed to open archiver_fd, then we have stdout_fd
leaking here, I suspect.

> +	return res;
> +}

Other than that, looks good to me.