From 81ad5513431c9c20f3ec41b6713d4ca602ed4a07 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:09 +0000 Subject: [PATCH 01/11] scalar-diagnose: use "$GIT_UNZIP" in test Use the "$GIT_UNZIP" test variable rather than verbatim 'unzip' to unzip the 'scalar diagnose' archive. Using "$GIT_UNZIP" is needed to run the Scalar tests on systems where 'unzip' is not in the system path. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/t/t9099-scalar.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh index 10b1172a8a..fac86a5755 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/contrib/scalar/t/t9099-scalar.sh @@ -109,14 +109,14 @@ test_expect_success UNZIP 'scalar diagnose' ' sed -n "s/.*$SQ\\(.*\\.zip\\)$SQ.*/\\1/p" zip_path && zip_path=$(cat zip_path) && test -n "$zip_path" && - unzip -v "$zip_path" && + "$GIT_UNZIP" -v "$zip_path" && folder=${zip_path%.zip} && test_path_is_missing "$folder" && - unzip -p "$zip_path" diagnostics.log >out && + "$GIT_UNZIP" -p "$zip_path" diagnostics.log >out && test_file_not_empty out && - unzip -p "$zip_path" packs-local.txt >out && + "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && grep "$(pwd)/.git/objects" out && - unzip -p "$zip_path" objects-local.txt >out && + "$GIT_UNZIP" -p "$zip_path" objects-local.txt >out && grep "^Total: [1-9]" out ' From 91be401945a58c95a61c94bd5ab5ed25d143803d Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:10 +0000 Subject: [PATCH 02/11] scalar-diagnose: avoid 32-bit overflow of size_t Avoid 32-bit size_t overflow when reporting the available disk space in 'get_disk_info' by casting the block size and available block count to 'off_t' before multiplying them. Without this change, 'st_mult' would (correctly) report a size_t overflow on 32-bit systems at or exceeding 2^32 bytes of available space. Note that 'off_t' is a 64-bit integer even on 32-bit systems due to the inclusion of '#define _FILE_OFFSET_BITS 64' in 'git-compat-util.h' (see b97e911643 (Support for large files on 32bit systems., 2007-02-17)). Helped-by: Junio C Hamano Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 97e71fe19c..0404645228 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -348,7 +348,7 @@ static int get_disk_info(struct strbuf *out) } strbuf_addf(out, "Available space on '%s': ", buf.buf); - strbuf_humanise_bytes(out, st_mult(stat.f_bsize, stat.f_bavail)); + strbuf_humanise_bytes(out, (off_t)stat.f_bsize * (off_t)stat.f_bavail); strbuf_addf(out, " (mount flags 0x%lx)\n", stat.f_flag); strbuf_release(&buf); #endif From ba307a50467aa0bc588f24a95a786014b42e3054 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:11 +0000 Subject: [PATCH 03/11] scalar-diagnose: add directory to archiver more gently If a directory added to the 'scalar diagnose' archiver does not exist, warn and return 0 from 'add_directory_to_archiver()' rather than failing with a fatal error. This handles a failure edge case where the '.git/logs' has not yet been created when running 'scalar diagnose', but extends to any situation where a directory may be missing in the '.git' dir. Now, when a directory is missing a warning is captured in the diagnostic logs. This provides a user with more complete information than if 'scalar diagnose' simply failed with an error. Helped-by: Junio C Hamano Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 0404645228..b9092f0b61 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -266,14 +266,20 @@ static int add_directory_to_archiver(struct strvec *archiver_args, const char *path, int recurse) { int at_root = !*path; - DIR *dir = opendir(at_root ? "." : path); + DIR *dir; struct dirent *e; struct strbuf buf = STRBUF_INIT; size_t len; int res = 0; - if (!dir) + dir = opendir(at_root ? "." : path); + if (!dir) { + if (errno == ENOENT) { + warning(_("could not archive missing directory '%s'"), path); + return 0; + } return error_errno(_("could not open directory '%s'"), path); + } if (!at_root) strbuf_addf(&buf, "%s/", path); From 435a2535b72aa0e3a2e152841fe79c5b65a6e8c0 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:12 +0000 Subject: [PATCH 04/11] scalar-diagnose: move 'get_disk_info()' to 'compat/' Move 'get_disk_info()' function into 'compat/'. Although Scalar-specific code is generally not part of the main Git tree, 'get_disk_info()' will be used in subsequent patches by additional callers beyond 'scalar diagnose'. This patch prepares for that change, at which point this platform-specific code should be part of 'compat/' as a matter of convention. The function is copied *mostly* verbatim, with two exceptions: * '#ifdef WIN32' is replaced with '#ifdef GIT_WINDOWS_NATIVE' to allow 'statvfs' to be used with Cygwin. * the 'struct strbuf buf' and 'int res' (as well as their corresponding cleanup & return) are moved outside of the '#ifdef' block. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- compat/disk.h | 56 +++++++++++++++++++++++++++++++++++++++++ contrib/scalar/scalar.c | 53 +------------------------------------- git-compat-util.h | 1 + 3 files changed, 58 insertions(+), 52 deletions(-) create mode 100644 compat/disk.h diff --git a/compat/disk.h b/compat/disk.h new file mode 100644 index 0000000000..50a32e3d8a --- /dev/null +++ b/compat/disk.h @@ -0,0 +1,56 @@ +#ifndef COMPAT_DISK_H +#define COMPAT_DISK_H + +#include "git-compat-util.h" + +static int get_disk_info(struct strbuf *out) +{ + struct strbuf buf = STRBUF_INIT; + int res = 0; + +#ifdef GIT_WINDOWS_NATIVE + char volume_name[MAX_PATH], fs_name[MAX_PATH]; + DWORD serial_number, component_length, flags; + ULARGE_INTEGER avail2caller, total, avail; + + strbuf_realpath(&buf, ".", 1); + if (!GetDiskFreeSpaceExA(buf.buf, &avail2caller, &total, &avail)) { + error(_("could not determine free disk size for '%s'"), + buf.buf); + res = -1; + goto cleanup; + } + + strbuf_setlen(&buf, offset_1st_component(buf.buf)); + if (!GetVolumeInformationA(buf.buf, volume_name, sizeof(volume_name), + &serial_number, &component_length, &flags, + fs_name, sizeof(fs_name))) { + error(_("could not get info for '%s'"), buf.buf); + res = -1; + goto cleanup; + } + strbuf_addf(out, "Available space on '%s': ", buf.buf); + strbuf_humanise_bytes(out, avail2caller.QuadPart); + strbuf_addch(out, '\n'); +#else + struct statvfs stat; + + strbuf_realpath(&buf, ".", 1); + if (statvfs(buf.buf, &stat) < 0) { + error_errno(_("could not determine free disk size for '%s'"), + buf.buf); + res = -1; + goto cleanup; + } + + strbuf_addf(out, "Available space on '%s': ", buf.buf); + strbuf_humanise_bytes(out, (off_t)stat.f_bsize * (off_t)stat.f_bavail); + strbuf_addf(out, " (mount flags 0x%lx)\n", stat.f_flag); +#endif + +cleanup: + strbuf_release(&buf); + return res; +} + +#endif /* COMPAT_DISK_H */ diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index b9092f0b61..607fedefd8 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -13,6 +13,7 @@ #include "help.h" #include "archive.h" #include "object-store.h" +#include "compat/disk.h" /* * Remove the deepest subdirectory in the provided path string. Path must not @@ -309,58 +310,6 @@ static int add_directory_to_archiver(struct strvec *archiver_args, return res; } -#ifndef WIN32 -#include -#endif - -static int get_disk_info(struct strbuf *out) -{ -#ifdef WIN32 - struct strbuf buf = STRBUF_INIT; - char volume_name[MAX_PATH], fs_name[MAX_PATH]; - DWORD serial_number, component_length, flags; - ULARGE_INTEGER avail2caller, total, avail; - - strbuf_realpath(&buf, ".", 1); - if (!GetDiskFreeSpaceExA(buf.buf, &avail2caller, &total, &avail)) { - error(_("could not determine free disk size for '%s'"), - buf.buf); - strbuf_release(&buf); - return -1; - } - - strbuf_setlen(&buf, offset_1st_component(buf.buf)); - if (!GetVolumeInformationA(buf.buf, volume_name, sizeof(volume_name), - &serial_number, &component_length, &flags, - fs_name, sizeof(fs_name))) { - error(_("could not get info for '%s'"), buf.buf); - strbuf_release(&buf); - return -1; - } - strbuf_addf(out, "Available space on '%s': ", buf.buf); - strbuf_humanise_bytes(out, avail2caller.QuadPart); - strbuf_addch(out, '\n'); - strbuf_release(&buf); -#else - struct strbuf buf = STRBUF_INIT; - struct statvfs stat; - - strbuf_realpath(&buf, ".", 1); - if (statvfs(buf.buf, &stat) < 0) { - error_errno(_("could not determine free disk size for '%s'"), - buf.buf); - strbuf_release(&buf); - return -1; - } - - strbuf_addf(out, "Available space on '%s': ", buf.buf); - strbuf_humanise_bytes(out, (off_t)stat.f_bsize * (off_t)stat.f_bavail); - strbuf_addf(out, " (mount flags 0x%lx)\n", stat.f_flag); - strbuf_release(&buf); -#endif - return 0; -} - /* printf-style interface, expects `=` argument */ static int set_config(const char *fmt, ...) { diff --git a/git-compat-util.h b/git-compat-util.h index 58d7708296..9a62e3a0d2 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -258,6 +258,7 @@ static inline int is_xplatform_dir_sep(int c) #include #include #include +#include #include #ifndef NO_SYS_SELECT_H #include From bb2c34956ad4fab1e7619327219fb5a5a49f571b Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:13 +0000 Subject: [PATCH 05/11] scalar-diagnose: move functionality to common location MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the core functionality of 'scalar diagnose' into a new 'diagnose.[c,h]' library to prepare for new callers in the main Git tree generating diagnostic archives. These callers will be introduced in subsequent patches. While this patch appears large, it is mostly made up of moving code out of 'scalar.c' and into 'diagnose.c'. Specifically, the functions - dir_file_stats_objects() - dir_file_stats() - count_files() - loose_objs_stats() - add_directory_to_archiver() are all copied verbatim from 'scalar.c'. The 'create_diagnostics_archive()' function is a mostly identical (partial) copy of 'cmd_diagnose()', with the primary changes being that 'zip_path' is an input and "Enlistment root" is corrected to "Repository root" in the archiver log. Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Makefile | 1 + contrib/scalar/scalar.c | 202 +------------------------------------ diagnose.c | 216 ++++++++++++++++++++++++++++++++++++++++ diagnose.h | 8 ++ 4 files changed, 227 insertions(+), 200 deletions(-) create mode 100644 diagnose.c create mode 100644 diagnose.h diff --git a/Makefile b/Makefile index 1624471bad..ad27a0bd70 100644 --- a/Makefile +++ b/Makefile @@ -932,6 +932,7 @@ LIB_OBJS += ctype.o LIB_OBJS += date.o LIB_OBJS += decorate.o LIB_OBJS += delta-islands.o +LIB_OBJS += diagnose.o LIB_OBJS += diff-delta.o LIB_OBJS += diff-merges.o LIB_OBJS += diff-lib.o diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 607fedefd8..3983def760 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -11,9 +11,7 @@ #include "dir.h" #include "packfile.h" #include "help.h" -#include "archive.h" -#include "object-store.h" -#include "compat/disk.h" +#include "diagnose.h" /* * Remove the deepest subdirectory in the provided path string. Path must not @@ -263,53 +261,6 @@ static int unregister_dir(void) return res; } -static int add_directory_to_archiver(struct strvec *archiver_args, - const char *path, int recurse) -{ - int at_root = !*path; - DIR *dir; - struct dirent *e; - struct strbuf buf = STRBUF_INIT; - size_t len; - int res = 0; - - dir = opendir(at_root ? "." : path); - if (!dir) { - if (errno == ENOENT) { - warning(_("could not archive missing directory '%s'"), path); - return 0; - } - return error_errno(_("could not open directory '%s'"), path); - } - - if (!at_root) - strbuf_addf(&buf, "%s/", path); - len = buf.len; - strvec_pushf(archiver_args, "--prefix=%s", buf.buf); - - while (!res && (e = readdir(dir))) { - if (!strcmp(".", e->d_name) || !strcmp("..", e->d_name)) - continue; - - strbuf_setlen(&buf, len); - strbuf_addstr(&buf, e->d_name); - - if (e->d_type == DT_REG) - strvec_pushf(archiver_args, "--add-file=%s", buf.buf); - else if (e->d_type != DT_DIR) - warning(_("skipping '%s', which is neither file nor " - "directory"), buf.buf); - else if (recurse && - add_directory_to_archiver(archiver_args, - buf.buf, recurse) < 0) - res = -1; - } - - closedir(dir); - strbuf_release(&buf); - return res; -} - /* printf-style interface, expects `=` argument */ static int set_config(const char *fmt, ...) { @@ -550,83 +501,6 @@ cleanup: return res; } -static void dir_file_stats_objects(const char *full_path, size_t full_path_len, - const char *file_name, void *data) -{ - struct strbuf *buf = data; - struct stat st; - - if (!stat(full_path, &st)) - strbuf_addf(buf, "%-70s %16" PRIuMAX "\n", file_name, - (uintmax_t)st.st_size); -} - -static int dir_file_stats(struct object_directory *object_dir, void *data) -{ - struct strbuf *buf = data; - - strbuf_addf(buf, "Contents of %s:\n", object_dir->path); - - for_each_file_in_pack_dir(object_dir->path, dir_file_stats_objects, - data); - - return 0; -} - -static int count_files(char *path) -{ - DIR *dir = opendir(path); - struct dirent *e; - int count = 0; - - if (!dir) - return 0; - - while ((e = readdir(dir)) != NULL) - if (!is_dot_or_dotdot(e->d_name) && e->d_type == DT_REG) - count++; - - closedir(dir); - return count; -} - -static void loose_objs_stats(struct strbuf *buf, const char *path) -{ - DIR *dir = opendir(path); - struct dirent *e; - int count; - int total = 0; - unsigned char c; - struct strbuf count_path = STRBUF_INIT; - size_t base_path_len; - - if (!dir) - return; - - strbuf_addstr(buf, "Object directory stats for "); - strbuf_add_absolute_path(buf, path); - strbuf_addstr(buf, ":\n"); - - strbuf_add_absolute_path(&count_path, path); - strbuf_addch(&count_path, '/'); - base_path_len = count_path.len; - - while ((e = readdir(dir)) != NULL) - if (!is_dot_or_dotdot(e->d_name) && - e->d_type == DT_DIR && strlen(e->d_name) == 2 && - !hex_to_bytes(&c, e->d_name, 1)) { - strbuf_setlen(&count_path, base_path_len); - strbuf_addstr(&count_path, e->d_name); - total += (count = count_files(count_path.buf)); - strbuf_addf(buf, "%s : %7d files\n", e->d_name, count); - } - - strbuf_addf(buf, "Total: %d loose objects", total); - - strbuf_release(&count_path); - closedir(dir); -} - static int cmd_diagnose(int argc, const char **argv) { struct option options[] = { @@ -637,12 +511,8 @@ static int cmd_diagnose(int argc, const char **argv) NULL }; struct strbuf zip_path = STRBUF_INIT; - struct strvec archiver_args = STRVEC_INIT; - char **argv_copy = NULL; - int stdout_fd = -1, archiver_fd = -1; time_t now = time(NULL); struct tm tm; - struct strbuf buf = STRBUF_INIT; int res = 0; argc = parse_options(argc, argv, NULL, options, @@ -663,79 +533,11 @@ static int cmd_diagnose(int argc, const char **argv) zip_path.buf); goto diagnose_cleanup; } - stdout_fd = dup(1); - if (stdout_fd < 0) { - res = error_errno(_("could not duplicate stdout")); - goto diagnose_cleanup; - } - 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, "Collecting diagnostic info\n\n"); - get_version_info(&buf, 1); - - strbuf_addf(&buf, "Enlistment root: %s\n", the_repository->worktree); - get_disk_info(&buf); - write_or_die(stdout_fd, buf.buf, buf.len); - strvec_pushf(&archiver_args, - "--add-virtual-file=diagnostics.log:%.*s", - (int)buf.len, buf.buf); - - strbuf_reset(&buf); - strbuf_addstr(&buf, "--add-virtual-file=packs-local.txt:"); - dir_file_stats(the_repository->objects->odb, &buf); - foreach_alt_odb(dir_file_stats, &buf); - strvec_push(&archiver_args, buf.buf); - - strbuf_reset(&buf); - strbuf_addstr(&buf, "--add-virtual-file=objects-local.txt:"); - loose_objs_stats(&buf, ".git/objects"); - strvec_push(&archiver_args, buf.buf); - - 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); - res = write_archive(archiver_args.nr, (const char **)argv_copy, NULL, - the_repository, NULL, 0); - if (res) { - error(_("failed to write archive")); - goto diagnose_cleanup; - } - - if (!res) - fprintf(stderr, "\n" - "Diagnostics complete.\n" - "All of the gathered info is captured in '%s'\n", - zip_path.buf); + res = create_diagnostics_archive(&zip_path); diagnose_cleanup: - if (archiver_fd >= 0) { - close(1); - dup2(stdout_fd, 1); - } - free(argv_copy); - strvec_clear(&archiver_args); strbuf_release(&zip_path); - strbuf_release(&buf); - return res; } diff --git a/diagnose.c b/diagnose.c new file mode 100644 index 0000000000..f0dcbfe1a2 --- /dev/null +++ b/diagnose.c @@ -0,0 +1,216 @@ +#include "cache.h" +#include "diagnose.h" +#include "compat/disk.h" +#include "archive.h" +#include "dir.h" +#include "help.h" +#include "strvec.h" +#include "object-store.h" +#include "packfile.h" + +static void dir_file_stats_objects(const char *full_path, size_t full_path_len, + const char *file_name, void *data) +{ + struct strbuf *buf = data; + struct stat st; + + if (!stat(full_path, &st)) + strbuf_addf(buf, "%-70s %16" PRIuMAX "\n", file_name, + (uintmax_t)st.st_size); +} + +static int dir_file_stats(struct object_directory *object_dir, void *data) +{ + struct strbuf *buf = data; + + strbuf_addf(buf, "Contents of %s:\n", object_dir->path); + + for_each_file_in_pack_dir(object_dir->path, dir_file_stats_objects, + data); + + return 0; +} + +static int count_files(char *path) +{ + DIR *dir = opendir(path); + struct dirent *e; + int count = 0; + + if (!dir) + return 0; + + while ((e = readdir(dir)) != NULL) + if (!is_dot_or_dotdot(e->d_name) && e->d_type == DT_REG) + count++; + + closedir(dir); + return count; +} + +static void loose_objs_stats(struct strbuf *buf, const char *path) +{ + DIR *dir = opendir(path); + struct dirent *e; + int count; + int total = 0; + unsigned char c; + struct strbuf count_path = STRBUF_INIT; + size_t base_path_len; + + if (!dir) + return; + + strbuf_addstr(buf, "Object directory stats for "); + strbuf_add_absolute_path(buf, path); + strbuf_addstr(buf, ":\n"); + + strbuf_add_absolute_path(&count_path, path); + strbuf_addch(&count_path, '/'); + base_path_len = count_path.len; + + while ((e = readdir(dir)) != NULL) + if (!is_dot_or_dotdot(e->d_name) && + e->d_type == DT_DIR && strlen(e->d_name) == 2 && + !hex_to_bytes(&c, e->d_name, 1)) { + strbuf_setlen(&count_path, base_path_len); + strbuf_addstr(&count_path, e->d_name); + total += (count = count_files(count_path.buf)); + strbuf_addf(buf, "%s : %7d files\n", e->d_name, count); + } + + strbuf_addf(buf, "Total: %d loose objects", total); + + strbuf_release(&count_path); + closedir(dir); +} + +static int add_directory_to_archiver(struct strvec *archiver_args, + const char *path, int recurse) +{ + int at_root = !*path; + DIR *dir; + struct dirent *e; + struct strbuf buf = STRBUF_INIT; + size_t len; + int res = 0; + + dir = opendir(at_root ? "." : path); + if (!dir) { + if (errno == ENOENT) { + warning(_("could not archive missing directory '%s'"), path); + return 0; + } + return error_errno(_("could not open directory '%s'"), path); + } + + if (!at_root) + strbuf_addf(&buf, "%s/", path); + len = buf.len; + strvec_pushf(archiver_args, "--prefix=%s", buf.buf); + + while (!res && (e = readdir(dir))) { + if (!strcmp(".", e->d_name) || !strcmp("..", e->d_name)) + continue; + + strbuf_setlen(&buf, len); + strbuf_addstr(&buf, e->d_name); + + if (e->d_type == DT_REG) + strvec_pushf(archiver_args, "--add-file=%s", buf.buf); + else if (e->d_type != DT_DIR) + warning(_("skipping '%s', which is neither file nor " + "directory"), buf.buf); + else if (recurse && + add_directory_to_archiver(archiver_args, + buf.buf, recurse) < 0) + res = -1; + } + + closedir(dir); + strbuf_release(&buf); + return res; +} + +int create_diagnostics_archive(struct strbuf *zip_path) +{ + struct strvec archiver_args = STRVEC_INIT; + char **argv_copy = NULL; + int stdout_fd = -1, archiver_fd = -1; + struct strbuf buf = STRBUF_INIT; + int res; + + stdout_fd = dup(STDOUT_FILENO); + if (stdout_fd < 0) { + res = error_errno(_("could not duplicate stdout")); + goto diagnose_cleanup; + } + + archiver_fd = xopen(zip_path->buf, O_CREAT | O_WRONLY | O_TRUNC, 0666); + if (dup2(archiver_fd, STDOUT_FILENO) < 0) { + res = error_errno(_("could not redirect output")); + goto diagnose_cleanup; + } + + init_zip_archiver(); + strvec_pushl(&archiver_args, "git-diagnose", "--format=zip", NULL); + + strbuf_reset(&buf); + strbuf_addstr(&buf, "Collecting diagnostic info\n\n"); + get_version_info(&buf, 1); + + strbuf_addf(&buf, "Repository root: %s\n", the_repository->worktree); + get_disk_info(&buf); + write_or_die(stdout_fd, buf.buf, buf.len); + strvec_pushf(&archiver_args, + "--add-virtual-file=diagnostics.log:%.*s", + (int)buf.len, buf.buf); + + strbuf_reset(&buf); + strbuf_addstr(&buf, "--add-virtual-file=packs-local.txt:"); + dir_file_stats(the_repository->objects->odb, &buf); + foreach_alt_odb(dir_file_stats, &buf); + strvec_push(&archiver_args, buf.buf); + + strbuf_reset(&buf); + strbuf_addstr(&buf, "--add-virtual-file=objects-local.txt:"); + loose_objs_stats(&buf, ".git/objects"); + strvec_push(&archiver_args, buf.buf); + + 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); + res = write_archive(archiver_args.nr, (const char **)argv_copy, NULL, + the_repository, NULL, 0); + if (res) { + error(_("failed to write archive")); + goto diagnose_cleanup; + } + + fprintf(stderr, "\n" + "Diagnostics complete.\n" + "All of the gathered info is captured in '%s'\n", + zip_path->buf); + +diagnose_cleanup: + if (archiver_fd >= 0) { + dup2(stdout_fd, STDOUT_FILENO); + close(stdout_fd); + close(archiver_fd); + } + free(argv_copy); + strvec_clear(&archiver_args); + strbuf_release(&buf); + + return res; +} diff --git a/diagnose.h b/diagnose.h new file mode 100644 index 0000000000..06dca69bda --- /dev/null +++ b/diagnose.h @@ -0,0 +1,8 @@ +#ifndef DIAGNOSE_H +#define DIAGNOSE_H + +#include "strbuf.h" + +int create_diagnostics_archive(struct strbuf *zip_path); + +#endif /* DIAGNOSE_H */ From 33cba726f03e1e06b10b85cf4a1cbd1017810486 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:14 +0000 Subject: [PATCH 06/11] diagnose.c: add option to configure archive contents Update 'create_diagnostics_archive()' to take an argument 'mode'. When archiving diagnostics for a repository, 'mode' is used to selectively include/exclude information based on its value. The initial options for 'mode' are: * DIAGNOSE_NONE: do not collect any diagnostics or create an archive (no-op). * DIAGNOSE_STATS: collect basic repository metadata (Git version, repo path, filesystem available space) as well as sizing and count statistics for the repository's objects and packfiles. * DIAGNOSE_ALL: collect basic repository metadata, sizing/count statistics, and copies of the '.git', '.git/hooks', '.git/info', '.git/logs', and '.git/objects/info' directories. These modes are introduced to provide users the option to collect diagnostics without the sensitive information included in copies of '.git' dir contents. At the moment, only 'scalar diagnose' uses 'create_diagnostics_archive()' (with a hardcoded 'DIAGNOSE_ALL' mode to match existing functionality), but more callers will be introduced in subsequent patches. Finally, refactor from a hardcoded set of 'add_directory_to_archiver()' calls to iterative invocations gated by 'DIAGNOSE_ALL'. This allows for easier future modification of the set of directories to archive and improves error reporting when 'add_directory_to_archiver()' fails. Helped-by: Derrick Stolee Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 2 +- diagnose.c | 39 +++++++++++++++++++++++++++++++-------- diagnose.h | 8 +++++++- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 3983def760..d538b8b8f1 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -534,7 +534,7 @@ static int cmd_diagnose(int argc, const char **argv) goto diagnose_cleanup; } - res = create_diagnostics_archive(&zip_path); + res = create_diagnostics_archive(&zip_path, DIAGNOSE_ALL); diagnose_cleanup: strbuf_release(&zip_path); diff --git a/diagnose.c b/diagnose.c index f0dcbfe1a2..9270056db2 100644 --- a/diagnose.c +++ b/diagnose.c @@ -8,6 +8,11 @@ #include "object-store.h" #include "packfile.h" +struct archive_dir { + const char *path; + int recursive; +}; + static void dir_file_stats_objects(const char *full_path, size_t full_path_len, const char *file_name, void *data) { @@ -132,13 +137,25 @@ static int add_directory_to_archiver(struct strvec *archiver_args, return res; } -int create_diagnostics_archive(struct strbuf *zip_path) +int create_diagnostics_archive(struct strbuf *zip_path, enum diagnose_mode mode) { struct strvec archiver_args = STRVEC_INIT; char **argv_copy = NULL; int stdout_fd = -1, archiver_fd = -1; struct strbuf buf = STRBUF_INIT; - int res; + int res, i; + struct archive_dir archive_dirs[] = { + { ".git", 0 }, + { ".git/hooks", 0 }, + { ".git/info", 0 }, + { ".git/logs", 1 }, + { ".git/objects/info", 0 } + }; + + if (mode == DIAGNOSE_NONE) { + res = 0; + goto diagnose_cleanup; + } stdout_fd = dup(STDOUT_FILENO); if (stdout_fd < 0) { @@ -177,12 +194,18 @@ int create_diagnostics_archive(struct strbuf *zip_path) loose_objs_stats(&buf, ".git/objects"); strvec_push(&archiver_args, buf.buf); - 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; + /* Only include this if explicitly requested */ + if (mode == DIAGNOSE_ALL) { + for (i = 0; i < ARRAY_SIZE(archive_dirs); i++) { + if (add_directory_to_archiver(&archiver_args, + archive_dirs[i].path, + archive_dirs[i].recursive)) { + res = error_errno(_("could not add directory '%s' to archiver"), + archive_dirs[i].path); + goto diagnose_cleanup; + } + } + } strvec_pushl(&archiver_args, "--prefix=", oid_to_hex(the_hash_algo->empty_tree), "--", NULL); diff --git a/diagnose.h b/diagnose.h index 06dca69bda..998775857a 100644 --- a/diagnose.h +++ b/diagnose.h @@ -3,6 +3,12 @@ #include "strbuf.h" -int create_diagnostics_archive(struct strbuf *zip_path); +enum diagnose_mode { + DIAGNOSE_NONE, + DIAGNOSE_STATS, + DIAGNOSE_ALL +}; + +int create_diagnostics_archive(struct strbuf *zip_path, enum diagnose_mode mode); #endif /* DIAGNOSE_H */ From 6783fd3cef0d6625e8a6d9d42d76042447078401 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:15 +0000 Subject: [PATCH 07/11] builtin/diagnose.c: create 'git diagnose' builtin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create a 'git diagnose' builtin to generate a standalone zip archive of repository diagnostics. 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 can be useful when diagnosing issues in any Git repository. Helped-by: Ævar Arnfjörð Bjarmason Helped-by: Derrick Stolee Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- .gitignore | 1 + Documentation/git-diagnose.txt | 50 +++++++++++++++++++++++++++++ Makefile | 1 + builtin.h | 1 + builtin/diagnose.c | 57 ++++++++++++++++++++++++++++++++++ git.c | 1 + t/t0092-diagnose.sh | 32 +++++++++++++++++++ 7 files changed, 143 insertions(+) create mode 100644 Documentation/git-diagnose.txt create mode 100644 builtin/diagnose.c create mode 100755 t/t0092-diagnose.sh diff --git a/.gitignore b/.gitignore index 42fd7253b4..80b530bbed 100644 --- a/.gitignore +++ b/.gitignore @@ -53,6 +53,7 @@ /git-cvsimport /git-cvsserver /git-daemon +/git-diagnose /git-diff /git-diff-files /git-diff-index diff --git a/Documentation/git-diagnose.txt b/Documentation/git-diagnose.txt new file mode 100644 index 0000000000..ce07dd0725 --- /dev/null +++ b/Documentation/git-diagnose.txt @@ -0,0 +1,50 @@ +git-diagnose(1) +================ + +NAME +---- +git-diagnose - Generate a zip archive of diagnostic information + +SYNOPSIS +-------- +[verse] +'git diagnose' [(-o | --output-directory) ] [(-s | --suffix) ] + +DESCRIPTION +----------- +Collects detailed information about the user's machine, Git client, and +repository state and packages that information into a zip archive. The +generated archive can then, for example, be shared with the Git mailing list to +help debug an issue or serve as a reference for independent debugging. + +The following information is captured in the archive: + + * 'git version --build-options' + * The path to the repository root + * The available disk space on the filesystem + * The name and size of each packfile, including those in alternate object + stores + * The total count of loose objects, as well as counts broken down by + `.git/objects` subdirectory + +This tool differs from linkgit:git-bugreport[1] in that it collects much more +detailed information with a greater focus on reporting the size and data shape +of repository contents. + +OPTIONS +------- +-o :: +--output-directory :: + Place the resulting diagnostics archive in `` instead of the + current directory. + +-s :: +--suffix :: + Specify an alternate suffix for the diagnostics archive name, to create + a file named 'git-diagnostics-'. This should take the + form of a strftime(3) format string; the current local time will be + used. + +GIT +--- +Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index ad27a0bd70..9ceaf55582 100644 --- a/Makefile +++ b/Makefile @@ -1154,6 +1154,7 @@ BUILTIN_OBJS += builtin/credential-cache.o BUILTIN_OBJS += builtin/credential-store.o BUILTIN_OBJS += builtin/credential.o BUILTIN_OBJS += builtin/describe.o +BUILTIN_OBJS += builtin/diagnose.o BUILTIN_OBJS += builtin/diff-files.o BUILTIN_OBJS += builtin/diff-index.o BUILTIN_OBJS += builtin/diff-tree.o diff --git a/builtin.h b/builtin.h index 40e9ecc848..8901a34d6b 100644 --- a/builtin.h +++ b/builtin.h @@ -144,6 +144,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix); int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix); int cmd_credential_store(int argc, const char **argv, const char *prefix); int cmd_describe(int argc, const char **argv, const char *prefix); +int cmd_diagnose(int argc, const char **argv, const char *prefix); int cmd_diff_files(int argc, const char **argv, const char *prefix); int cmd_diff_index(int argc, const char **argv, const char *prefix); int cmd_diff(int argc, const char **argv, const char *prefix); diff --git a/builtin/diagnose.c b/builtin/diagnose.c new file mode 100644 index 0000000000..832493bba6 --- /dev/null +++ b/builtin/diagnose.c @@ -0,0 +1,57 @@ +#include "builtin.h" +#include "parse-options.h" +#include "diagnose.h" + +static const char * const diagnose_usage[] = { + N_("git diagnose [-o|--output-directory ] [-s|--suffix ]"), + NULL +}; + +int cmd_diagnose(int argc, const char **argv, const char *prefix) +{ + struct strbuf zip_path = STRBUF_INIT; + time_t now = time(NULL); + struct tm tm; + char *option_output = NULL; + char *option_suffix = "%Y-%m-%d-%H%M"; + char *prefixed_filename; + + const struct option diagnose_options[] = { + OPT_STRING('o', "output-directory", &option_output, N_("path"), + N_("specify a destination for the diagnostics archive")), + OPT_STRING('s', "suffix", &option_suffix, N_("format"), + N_("specify a strftime format suffix for the filename")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, diagnose_options, + diagnose_usage, 0); + + /* Prepare the path to put the result */ + prefixed_filename = prefix_filename(prefix, + option_output ? option_output : ""); + strbuf_addstr(&zip_path, prefixed_filename); + strbuf_complete(&zip_path, '/'); + + strbuf_addstr(&zip_path, "git-diagnostics-"); + strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0); + strbuf_addstr(&zip_path, ".zip"); + + switch (safe_create_leading_directories(zip_path.buf)) { + case SCLD_OK: + case SCLD_EXISTS: + break; + default: + die_errno(_("could not create leading directories for '%s'"), + zip_path.buf); + } + + /* Prepare diagnostics */ + if (create_diagnostics_archive(&zip_path, DIAGNOSE_STATS)) + die_errno(_("unable to create diagnostics archive %s"), + zip_path.buf); + + free(prefixed_filename); + strbuf_release(&zip_path); + return 0; +} diff --git a/git.c b/git.c index e5d62fa5a9..0b9d8ef767 100644 --- a/git.c +++ b/git.c @@ -522,6 +522,7 @@ static struct cmd_struct commands[] = { { "credential-cache--daemon", cmd_credential_cache_daemon }, { "credential-store", cmd_credential_store }, { "describe", cmd_describe, RUN_SETUP }, + { "diagnose", cmd_diagnose, RUN_SETUP_GENTLY }, { "diff", cmd_diff, NO_PARSEOPT }, { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT }, { "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT }, diff --git a/t/t0092-diagnose.sh b/t/t0092-diagnose.sh new file mode 100755 index 0000000000..b6923726fd --- /dev/null +++ b/t/t0092-diagnose.sh @@ -0,0 +1,32 @@ +#!/bin/sh + +test_description='git diagnose' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh + +test_expect_success UNZIP 'creates diagnostics zip archive' ' + test_when_finished rm -rf report && + + git diagnose -o report -s test >out && + grep "Available space" out && + + zip_path=report/git-diagnostics-test.zip && + test_path_is_file "$zip_path" && + + # Check zipped archive content + "$GIT_UNZIP" -p "$zip_path" diagnostics.log >out && + test_file_not_empty out && + + "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && + grep ".git/objects" out && + + "$GIT_UNZIP" -p "$zip_path" objects-local.txt >out && + grep "^Total: [0-9][0-9]*" out && + + # Should not include .git directory contents by default + ! "$GIT_UNZIP" -l "$zip_path" | grep ".git/" + grep "^Total: [0-9][0-9]*" out +' + +test_done From 7ecf193f7d621f618b322498d884ee103a44522f Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:16 +0000 Subject: [PATCH 08/11] builtin/diagnose.c: add '--mode' option Create '--mode=' option in 'git diagnose' to allow users to optionally select non-default diagnostic information to include in the output archive. Additionally, document the currently-available modes, emphasizing the importance of not sharing a '--mode=all' archive publicly due to the presence of sensitive information. Note that the option parsing callback - 'option_parse_diagnose()' - is added to 'diagnose.c' rather than 'builtin/diagnose.c' so that it may be reused in future callers configuring a diagnostics archive. Helped-by: Derrick Stolee Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/git-diagnose.txt | 17 ++++++++++++++++- builtin/diagnose.c | 8 ++++++-- diagnose.c | 30 ++++++++++++++++++++++++++++++ diagnose.h | 3 +++ t/t0092-diagnose.sh | 30 +++++++++++++++++++++++++++++- 5 files changed, 84 insertions(+), 4 deletions(-) diff --git a/Documentation/git-diagnose.txt b/Documentation/git-diagnose.txt index ce07dd0725..3ec8cc7ad7 100644 --- a/Documentation/git-diagnose.txt +++ b/Documentation/git-diagnose.txt @@ -9,6 +9,7 @@ SYNOPSIS -------- [verse] 'git diagnose' [(-o | --output-directory) ] [(-s | --suffix) ] + [--mode=] DESCRIPTION ----------- @@ -17,7 +18,7 @@ repository state and packages that information into a zip archive. The generated archive can then, for example, be shared with the Git mailing list to help debug an issue or serve as a reference for independent debugging. -The following information is captured in the archive: +By default, the following information is captured in the archive: * 'git version --build-options' * The path to the repository root @@ -27,6 +28,9 @@ The following information is captured in the archive: * The total count of loose objects, as well as counts broken down by `.git/objects` subdirectory +Additional information can be collected by selecting a different diagnostic mode +using the `--mode` option. + This tool differs from linkgit:git-bugreport[1] in that it collects much more detailed information with a greater focus on reporting the size and data shape of repository contents. @@ -45,6 +49,17 @@ OPTIONS form of a strftime(3) format string; the current local time will be used. +--mode=(stats|all):: + Specify the type of diagnostics that should be collected. The default behavior + of 'git diagnose' is equivalent to `--mode=stats`. ++ +The `--mode=all` option collects everything included in `--mode=stats`, as well +as copies of `.git`, `.git/hooks`, `.git/info`, `.git/logs`, and +`.git/objects/info` directories. This additional information may be sensitive, +as it can be used to reconstruct the full contents of the diagnosed repository. +Users should exercise caution when sharing an archive generated with +`--mode=all`. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/diagnose.c b/builtin/diagnose.c index 832493bba6..cd260c2015 100644 --- a/builtin/diagnose.c +++ b/builtin/diagnose.c @@ -3,7 +3,7 @@ #include "diagnose.h" static const char * const diagnose_usage[] = { - N_("git diagnose [-o|--output-directory ] [-s|--suffix ]"), + N_("git diagnose [-o|--output-directory ] [-s|--suffix ] [--mode=]"), NULL }; @@ -12,6 +12,7 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix) struct strbuf zip_path = STRBUF_INIT; time_t now = time(NULL); struct tm tm; + enum diagnose_mode mode = DIAGNOSE_STATS; char *option_output = NULL; char *option_suffix = "%Y-%m-%d-%H%M"; char *prefixed_filename; @@ -21,6 +22,9 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix) N_("specify a destination for the diagnostics archive")), OPT_STRING('s', "suffix", &option_suffix, N_("format"), N_("specify a strftime format suffix for the filename")), + OPT_CALLBACK_F(0, "mode", &mode, N_("(stats|all)"), + N_("specify the content of the diagnostic archive"), + PARSE_OPT_NONEG, option_parse_diagnose), OPT_END() }; @@ -47,7 +51,7 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix) } /* Prepare diagnostics */ - if (create_diagnostics_archive(&zip_path, DIAGNOSE_STATS)) + if (create_diagnostics_archive(&zip_path, mode)) die_errno(_("unable to create diagnostics archive %s"), zip_path.buf); diff --git a/diagnose.c b/diagnose.c index 9270056db2..beb0a8741b 100644 --- a/diagnose.c +++ b/diagnose.c @@ -13,6 +13,36 @@ struct archive_dir { int recursive; }; +struct diagnose_option { + enum diagnose_mode mode; + const char *option_name; +}; + +static struct diagnose_option diagnose_options[] = { + { DIAGNOSE_STATS, "stats" }, + { DIAGNOSE_ALL, "all" }, +}; + +int option_parse_diagnose(const struct option *opt, const char *arg, int unset) +{ + int i; + enum diagnose_mode *diagnose = opt->value; + + if (!arg) { + *diagnose = unset ? DIAGNOSE_NONE : DIAGNOSE_STATS; + return 0; + } + + for (i = 0; i < ARRAY_SIZE(diagnose_options); i++) { + if (!strcmp(arg, diagnose_options[i].option_name)) { + *diagnose = diagnose_options[i].mode; + return 0; + } + } + + return error(_("invalid --%s value '%s'"), opt->long_name, arg); +} + static void dir_file_stats_objects(const char *full_path, size_t full_path_len, const char *file_name, void *data) { diff --git a/diagnose.h b/diagnose.h index 998775857a..7a4951a786 100644 --- a/diagnose.h +++ b/diagnose.h @@ -2,6 +2,7 @@ #define DIAGNOSE_H #include "strbuf.h" +#include "parse-options.h" enum diagnose_mode { DIAGNOSE_NONE, @@ -9,6 +10,8 @@ enum diagnose_mode { DIAGNOSE_ALL }; +int option_parse_diagnose(const struct option *opt, const char *arg, int unset); + int create_diagnostics_archive(struct strbuf *zip_path, enum diagnose_mode mode); #endif /* DIAGNOSE_H */ diff --git a/t/t0092-diagnose.sh b/t/t0092-diagnose.sh index b6923726fd..fca9b58489 100755 --- a/t/t0092-diagnose.sh +++ b/t/t0092-diagnose.sh @@ -26,7 +26,35 @@ test_expect_success UNZIP 'creates diagnostics zip archive' ' # Should not include .git directory contents by default ! "$GIT_UNZIP" -l "$zip_path" | grep ".git/" - grep "^Total: [0-9][0-9]*" out +' + +test_expect_success UNZIP '--mode=stats excludes .git dir contents' ' + test_when_finished rm -rf report && + + git diagnose -o report -s test --mode=stats >out && + + # Includes pack quantity/size info + "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && + grep ".git/objects" out && + + # Does not include .git directory contents + ! "$GIT_UNZIP" -l "$zip_path" | grep ".git/" +' + +test_expect_success UNZIP '--mode=all includes .git dir contents' ' + test_when_finished rm -rf report && + + git diagnose -o report -s test --mode=all >out && + + # Includes pack quantity/size info + "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && + grep ".git/objects" out && + + # Includes .git directory contents + "$GIT_UNZIP" -l "$zip_path" | grep ".git/" && + + "$GIT_UNZIP" -p "$zip_path" .git/HEAD >out && + test_file_not_empty out ' test_done From aac0e8ffeeed57cae2a047fd442f2125cbfc8e39 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:17 +0000 Subject: [PATCH 09/11] builtin/bugreport.c: create '--diagnose' option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create a '--diagnose' option for 'git bugreport' to collect additional information about the repository and write it to a zipped archive. The '--diagnose' option behaves effectively as an alias for simultaneously running 'git bugreport' and 'git diagnose'. In the documentation, users are explicitly recommended to attach the diagnostics alongside a bug report to provide additional context to readers, ideally reducing some back-and-forth between reporters and those debugging the issue. Note that '--diagnose' may take an optional string arg (either 'stats' or 'all'). If specified without the arg, the behavior corresponds to running 'git diagnose' without '--mode'. As with 'git diagnose', this default is intended to help reduce unintentional leaking of sensitive information). Users can also explicitly specify '--diagnose=(stats|all)' to generate the respective archive created by 'git diagnose --mode=(stats|all)'. Suggested-by: Ævar Arnfjörð Bjarmason Helped-by: Derrick Stolee Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/git-bugreport.txt | 18 +++++++++++++ builtin/bugreport.c | 27 ++++++++++++++++--- t/t0091-bugreport.sh | 48 +++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index d8817bf3ce..eca726e579 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -9,6 +9,7 @@ SYNOPSIS -------- [verse] 'git bugreport' [(-o | --output-directory) ] [(-s | --suffix) ] + [--diagnose[=]] DESCRIPTION ----------- @@ -31,6 +32,10 @@ The following information is captured automatically: - A list of enabled hooks - $SHELL +Additional information may be gathered into a separate zip archive using the +`--diagnose` option, and can be attached alongside the bugreport document to +provide additional context to readers. + This tool is invoked via the typical Git setup process, which means that in some cases, it might not be able to launch - for example, if a relevant config file is unreadable. In this kind of scenario, it may be helpful to manually gather @@ -49,6 +54,19 @@ OPTIONS named 'git-bugreport-'. This should take the form of a strftime(3) format string; the current local time will be used. +--no-diagnose:: +--diagnose[=]:: + Create a zip archive of supplemental information about the user's + machine, Git client, and repository state. The archive is written to the + same output directory as the bug report and is named + 'git-diagnostics-'. ++ +Without `mode` specified, the diagnostic archive will contain the default set of +statistics reported by `git diagnose`. An optional `mode` value may be specified +to change which information is included in the archive. See +linkgit:git-diagnose[1] for the list of valid values for `mode` and details +about their usage. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 9de32bc96e..530895be55 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -5,6 +5,7 @@ #include "compat/compiler.h" #include "hook.h" #include "hook-list.h" +#include "diagnose.h" static void get_system_info(struct strbuf *sys_info) @@ -59,7 +60,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit) } static const char * const bugreport_usage[] = { - N_("git bugreport [-o|--output-directory ] [-s|--suffix ]"), + N_("git bugreport [-o|--output-directory ] [-s|--suffix ] [--diagnose[=]"), NULL }; @@ -98,16 +99,21 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) int report = -1; time_t now = time(NULL); struct tm tm; + enum diagnose_mode diagnose = DIAGNOSE_NONE; char *option_output = NULL; char *option_suffix = "%Y-%m-%d-%H%M"; const char *user_relative_path = NULL; char *prefixed_filename; + size_t output_path_len; const struct option bugreport_options[] = { + OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"), + N_("create an additional zip archive of detailed diagnostics (default 'stats')"), + PARSE_OPT_OPTARG, option_parse_diagnose), OPT_STRING('o', "output-directory", &option_output, N_("path"), - N_("specify a destination for the bugreport file")), + N_("specify a destination for the bugreport file(s)")), OPT_STRING('s', "suffix", &option_suffix, N_("format"), - N_("specify a strftime format suffix for the filename")), + N_("specify a strftime format suffix for the filename(s)")), OPT_END() }; @@ -119,6 +125,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) option_output ? option_output : ""); strbuf_addstr(&report_path, prefixed_filename); strbuf_complete(&report_path, '/'); + output_path_len = report_path.len; strbuf_addstr(&report_path, "git-bugreport-"); strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0); @@ -133,6 +140,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) report_path.buf); } + /* Prepare diagnostics, if requested */ + if (diagnose != DIAGNOSE_NONE) { + struct strbuf zip_path = STRBUF_INIT; + strbuf_add(&zip_path, report_path.buf, output_path_len); + strbuf_addstr(&zip_path, "git-diagnostics-"); + strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0); + strbuf_addstr(&zip_path, ".zip"); + + if (create_diagnostics_archive(&zip_path, diagnose)) + die_errno(_("unable to create diagnostics archive %s"), zip_path.buf); + + strbuf_release(&zip_path); + } + /* Prepare the report contents */ get_bug_template(&buffer); diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh index 08f5fe9cae..b6d2f591ac 100755 --- a/t/t0091-bugreport.sh +++ b/t/t0091-bugreport.sh @@ -78,4 +78,52 @@ test_expect_success 'indicates populated hooks' ' test_cmp expect actual ' +test_expect_success UNZIP '--diagnose creates diagnostics zip archive' ' + test_when_finished rm -rf report && + + git bugreport --diagnose -o report -s test >out && + + zip_path=report/git-diagnostics-test.zip && + grep "Available space" out && + test_path_is_file "$zip_path" && + + # Check zipped archive content + "$GIT_UNZIP" -p "$zip_path" diagnostics.log >out && + test_file_not_empty out && + + "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && + grep ".git/objects" out && + + "$GIT_UNZIP" -p "$zip_path" objects-local.txt >out && + grep "^Total: [0-9][0-9]*" out && + + # Should not include .git directory contents by default + ! "$GIT_UNZIP" -l "$zip_path" | grep ".git/" +' + +test_expect_success UNZIP '--diagnose=stats excludes .git dir contents' ' + test_when_finished rm -rf report && + + git bugreport --diagnose=stats -o report -s test >out && + + # Includes pack quantity/size info + "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && + grep ".git/objects" out && + + # Does not include .git directory contents + ! "$GIT_UNZIP" -l "$zip_path" | grep ".git/" +' + +test_expect_success UNZIP '--diagnose=all includes .git dir contents' ' + test_when_finished rm -rf report && + + git bugreport --diagnose=all -o report -s test >out && + + # Includes .git directory contents + "$GIT_UNZIP" -l "$zip_path" | grep ".git/" && + + "$GIT_UNZIP" -p "$zip_path" .git/HEAD >out && + test_file_not_empty out +' + test_done From 672196a3073036be2eda683fc116af9a21a66aa6 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:18 +0000 Subject: [PATCH 10/11] scalar-diagnose: use 'git diagnose --mode=all' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace implementation of 'scalar diagnose' with an internal invocation of 'git diagnose --mode=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-'. Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index d538b8b8f1..68571ce195 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,19 @@ static int cmd_diagnose(int argc, const char **argv) N_("scalar diagnose []"), 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); + setup_enlistment_directory(argc, argv, usage, options, &diagnostics_root); + strbuf_addstr(&diagnostics_root, "/.scalarDiagnostics"); - 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; - } + res = run_git("diagnose", "--mode=all", "-s", "%Y%m%d_%H%M%S", + "-o", diagnostics_root.buf, NULL); - res = create_diagnostics_archive(&zip_path, DIAGNOSE_ALL); - -diagnose_cleanup: - strbuf_release(&zip_path); + strbuf_release(&diagnostics_root); return res; } From 43370b1e910f767b327046b1c4253c82e9695052 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Fri, 12 Aug 2022 20:10:19 +0000 Subject: [PATCH 11/11] scalar: update technical doc roadmap Update the Scalar roadmap to reflect the completion of generalizing 'scalar diagnose' into 'git diagnose' and 'git bugreport --diagnose'. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/technical/scalar.txt | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt index 08bc09c225..f6353375f0 100644 --- a/Documentation/technical/scalar.txt +++ b/Documentation/technical/scalar.txt @@ -84,6 +84,9 @@ series have been accepted: - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand. +- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose` + into `git diagnose` and `git bugreport --diagnose`. + Roughly speaking (and subject to change), the following series are needed to "finish" this initial version of Scalar: @@ -91,12 +94,6 @@ Roughly speaking (and subject to change), the following series are needed to and implement `scalar help`. At the end of this series, Scalar should be feature-complete from the perspective of a user. -- Generalize features not specific to Scalar: In the spirit of making Scalar - configure only what is needed for large repo performance, move common - utilities into other parts of Git. Some of this will be internal-only, but one - major change will be generalizing `scalar diagnose` for use with any Git - repository. - - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of `git`, including updates to build and install it with the rest of Git. This change will incorporate Scalar into the Git CI and test framework, as well as