scalar: implement the subcommand "diagnose"

From: Johannes Schindelin <[email protected]>

Over the course of Scalar's development, it became obvious that there is a need for a command that can gather all kinds of useful information that can help identify the most typical problems with large worktrees/repositories.

The diagnose command is the culmination of this hard-won knowledge: it gathers the installed hooks, the config, a couple statistics describing the data shape, among other pieces of information, and then wraps everything up in a tidy, neat .zip archive.

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.

Signed-off-by Johannes Schindelin <[email protected]>

---
 contrib/scalar/scalar.c          | 170 +++++++++++++++++++++++++++++++
 contrib/scalar/scalar.txt        |  12 +++
 contrib/scalar/t/t9099-scalar.sh |  13 +++
 3 files changed, 195 insertions(+)

contrib/scalar/scalar.c +170 -0

index 1ce9c2b00e8..13f2b0f4d5a 100644
@@ -259,6 +259,108 @@ static int unregister_dir(void)
 	return res;
 }
 
+static int stage(const char *git_dir, struct strbuf *buf, const char *path)
+{
+	struct strbuf cacheinfo = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int res;
+
+	strbuf_addstr(&cacheinfo, "100644,");
+
+	cp.git_cmd = 1;
+	strvec_pushl(&cp.args, "--git-dir", git_dir,
+		     "hash-object", "-w", "--stdin", NULL);
+	res = pipe_command(&cp, buf->buf, buf->len, &cacheinfo, 256, NULL, 0);
+	if (!res) {
+		strbuf_rtrim(&cacheinfo);
+		strbuf_addch(&cacheinfo, ',');
+		/* We cannot stage `.git`, use `_git` instead. */
+		if (starts_with(path, ".git/"))
+			strbuf_addf(&cacheinfo, "_%s", path + 1);
+		else
+			strbuf_addstr(&cacheinfo, path);
+
+		child_process_init(&cp);
+		cp.git_cmd = 1;
+		strvec_pushl(&cp.args, "--git-dir", git_dir,
+			     "update-index", "--add", "--cacheinfo",
+			     cacheinfo.buf, NULL);
+		res = run_command(&cp);
+	}
+
+	strbuf_release(&cacheinfo);
+	return res;
+}
+
+static int stage_file(const char *git_dir, const char *path)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int res;
+
+	if (strbuf_read_file(&buf, path, 0) < 0)
+		return error(_("could not read '%s'"), path);
+
+	res = stage(git_dir, &buf, path);
+
+	strbuf_release(&buf);
+	return res;
+}
+
+static int stage_directory(const char *git_dir, const char *path, int recurse)
+{
+	int at_root = !*path;
+	DIR *dir = opendir(at_root ? "." : path);
+	struct dirent *e;
+	struct strbuf buf = STRBUF_INIT;
+	size_t len;
+	int res = 0;
+
+	if (!dir)
+		return error(_("could not open directory '%s'"), path);
+
+	if (!at_root)
+		strbuf_addf(&buf, "%s/", path);
+	len = buf.len;
+
+	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 && stage_file(git_dir, buf.buf)) ||
+		    (e->d_type == DT_DIR && recurse &&
+		     stage_directory(git_dir, buf.buf, recurse)))
+			res = -1;
+	}
+
+	closedir(dir);
+	strbuf_release(&buf);
+	return res;
+}
+
+static int index_to_zip(const char *git_dir)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf oid = STRBUF_INIT;
+
+	cp.git_cmd = 1;
+	strvec_pushl(&cp.args, "--git-dir", git_dir, "write-tree", NULL);
+	if (pipe_command(&cp, NULL, 0, &oid, the_hash_algo->hexsz + 1,
+			 NULL, 0))
+		return error(_("could not write temporary tree object"));
+
+	strbuf_rtrim(&oid);
+	child_process_init(&cp);
+	cp.git_cmd = 1;
+	strvec_pushl(&cp.args, "--git-dir", git_dir, "archive", "-o", NULL);
+	strvec_pushf(&cp.args, "%s.zip", git_dir);
+	strvec_pushl(&cp.args, oid.buf, "--", NULL);
+	strbuf_release(&oid);
+	return run_command(&cp);
+}
+
 /* printf-style interface, expects `<key>=<value>` argument */
 static int set_config(const char *fmt, ...)
 {

@@ -499,6 +601,73 @@ cleanup:
 	return res;
 }
 
+static int cmd_diagnose(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END(),
+	};
+	const char * const usage[] = {
+		N_("scalar diagnose [<enlistment>]"),
+		NULL
+	};
+	struct strbuf tmp_dir = STRBUF_INIT;
+	time_t now = time(NULL);
+	struct tm tm;
+	struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+	int res = 0;
+
+	argc = parse_options(argc, argv, NULL, options,
+			     usage, 0);
+
+	setup_enlistment_directory(argc, argv, usage, options, &buf);
+
+	strbuf_addstr(&buf, "/.scalarDiagnostics/scalar_");
+	strbuf_addftime(&buf, "%Y%m%d_%H%M%S", localtime_r(&now, &tm), 0, 0);
+	if (run_git("init", "-q", "-b", "dummy", "--bare", buf.buf, NULL)) {
+		res = error(_("could not initialize temporary repository: %s"),
+			    buf.buf);
+		goto diagnose_cleanup;
+	}
+	strbuf_realpath(&tmp_dir, buf.buf, 1);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "Collecting diagnostic info into temp folder %s\n\n",
+		    tmp_dir.buf);
+
+	get_version_info(&buf, 1);
+
+	strbuf_addf(&buf, "Enlistment root: %s\n", the_repository->worktree);
+	fwrite(buf.buf, buf.len, 1, stdout);
+
+	if ((res = stage(tmp_dir.buf, &buf, "diagnostics.log")))
+		goto diagnose_cleanup;
+
+	if ((res = stage_directory(tmp_dir.buf, ".git", 0)) ||
+	    (res = stage_directory(tmp_dir.buf, ".git/hooks", 0)) ||
+	    (res = stage_directory(tmp_dir.buf, ".git/info", 0)) ||
+	    (res = stage_directory(tmp_dir.buf, ".git/logs", 1)) ||
+	    (res = stage_directory(tmp_dir.buf, ".git/objects/info", 0)))
+		goto diagnose_cleanup;
+
+	res = index_to_zip(tmp_dir.buf);
+
+	if (!res)
+		res = remove_dir_recursively(&tmp_dir, 0);
+
+	if (!res)
+		printf("\n"
+		       "Diagnostics complete.\n"
+		       "All of the gathered info is captured in '%s.zip'\n",
+		       tmp_dir.buf);
+
+diagnose_cleanup:
+	strbuf_release(&tmp_dir);
+	strbuf_release(&path);
+	strbuf_release(&buf);
+
+	return res;
+}
+
 static int cmd_list(int argc, const char **argv)
 {
 	if (argc != 1)

@@ -800,6 +969,7 @@ static struct {
 	{ "reconfigure", cmd_reconfigure },
 	{ "delete", cmd_delete },
 	{ "version", cmd_version },
+	{ "diagnose", cmd_diagnose },
 	{ NULL, NULL},
 };
 

contrib/scalar/scalar.txt +12 -0

index f416d637289..22583fe046e 100644
@@ -14,6 +14,7 @@ scalar register [<enlistment>]
 scalar unregister [<enlistment>]
 scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
 scalar reconfigure [ --all | <enlistment> ]
+scalar diagnose [<enlistment>]
 scalar delete <enlistment>
 
 DESCRIPTION

@@ -129,6 +130,17 @@ reconfigure the enlistment.
 With the `--all` option, all enlistments currently registered with Scalar
 will be reconfigured. Use this option after each Scalar upgrade.
 
+Diagnose
+~~~~~~~~
+
+diagnose [<enlistment>]::
+    When reporting issues with Scalar, it is often helpful to provide the
+    information gathered by this command, including logs and certain
+    statistics describing the data shape of the current enlistment.
++
+The output of this command is a `.zip` file that is written into
+a directory adjacent to the worktree in the `src` directory.
+
 Delete
 ~~~~~~
 

contrib/scalar/t/t9099-scalar.sh +13 -0

index 2e1502ad45e..ecd06e207c2 100755
@@ -65,6 +65,19 @@ test_expect_success 'scalar clone' '
 	)
 '
 
+SQ="'"
+test_expect_success UNZIP 'scalar diagnose' '
+	scalar diagnose cloned >out &&
+	sed -n "s/.*$SQ\\(.*\\.zip\\)$SQ.*/\\1/p" <out >zip_path &&
+	zip_path=$(cat zip_path) &&
+	test -n "$zip_path" &&
+	unzip -v "$zip_path" &&
+	folder=${zip_path%.zip} &&
+	test_path_is_missing "$folder" &&
+	unzip -p "$zip_path" diagnostics.log >out &&
+	test_file_not_empty out
+'
+
 test_expect_success 'scalar reconfigure' '
 	git init one/src &&
 	scalar register one &&

Am 26.01.22 um 09:41 schrieb Johannes Schindelin via GitGitGadget:

Quoted 8 lines

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.

git archive allows you to include untracked files in an archive with its option --add-file. You can see an example in Git's Makefile; search for GIT_ARCHIVE_EXTRA_FILES. It still requires a tree argument, but the empty tree object should suffice if you don't want to include any tracked files. It doesn't currently support streaming, though, i.e. files are fully read into memory, so it's impractical for huge ones.

Quoted 10 lines · René Scharfe

Am 26.01.22 um 09:41 schrieb Johannes Schindelin via GitGitGadget:

Quoted 8 lines

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.

Quoted 6 lines

git archive allows you to include untracked files in an archive with its option --add-file. You can see an example in Git's Makefile; search for GIT_ARCHIVE_EXTRA_FILES. It still requires a tree argument, but the empty tree object should suffice if you don't want to include any tracked files. It doesn't currently support streaming, though, i.e. files are fully read into memory, so it's impractical for huge ones.

Using --add-file would likely be preferable to setting up a temporary repository just to invoke git archive in it. Johannes would be the expert to ask whether or not big files are going to be a problem here (based on a cursory scan of the new functions in scalar.c, I don't expect this to be the case).

The new stage_directory() function _could_ add --add-file arguments in a loop around readdir(), but it might also be nice to add a new --add-directory function to git archive which would do the "heavy" lifting for us.

From: Johannes Schindelin <[email protected]>

Over the course of Scalar's development, it became obvious that there is a need for a command that can gather all kinds of useful information that can help identify the most typical problems with large worktrees/repositories.

The diagnose command is the culmination of this hard-won knowledge: it gathers the installed hooks, the config, a couple statistics describing the data shape, among other pieces of information, and then wraps everything up in a tidy, neat .zip archive.

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.

Signed-off-by Johannes Schindelin <[email protected]>

---
 contrib/scalar/scalar.c          | 170 +++++++++++++++++++++++++++++++
 contrib/scalar/scalar.txt        |  12 +++
 contrib/scalar/t/t9099-scalar.sh |  13 +++
 3 files changed, 195 insertions(+)

contrib/scalar/scalar.c +170 -0

index 1ce9c2b00e8..13f2b0f4d5a 100644
@@ -259,6 +259,108 @@ static int unregister_dir(void)
        return res;
 }
 
+static int stage(const char *git_dir, struct strbuf *buf, const char *path)
+{
+       struct strbuf cacheinfo = STRBUF_INIT;
+       struct child_process cp = CHILD_PROCESS_INIT;
+       int res;
+
+       strbuf_addstr(&cacheinfo, "100644,");
+
+       cp.git_cmd = 1;
+       strvec_pushl(&cp.args, "--git-dir", git_dir,
+                    "hash-object", "-w", "--stdin", NULL);
+       res = pipe_command(&cp, buf->buf, buf->len, &cacheinfo, 256, NULL, 0);
+       if (!res) {
+               strbuf_rtrim(&cacheinfo);
+               strbuf_addch(&cacheinfo, ',');
+               /* We cannot stage `.git`, use `_git` instead. */
+               if (starts_with(path, ".git/"))
+                       strbuf_addf(&cacheinfo, "_%s", path + 1);
+               else
+                       strbuf_addstr(&cacheinfo, path);
+
+               child_process_init(&cp);
+               cp.git_cmd = 1;
+               strvec_pushl(&cp.args, "--git-dir", git_dir,
+                            "update-index", "--add", "--cacheinfo",
+                            cacheinfo.buf, NULL);
+               res = run_command(&cp);
+       }
+
+       strbuf_release(&cacheinfo);
+       return res;
+}
+
+static int stage_file(const char *git_dir, const char *path)
+{
+       struct strbuf buf = STRBUF_INIT;
+       int res;
+
+       if (strbuf_read_file(&buf, path, 0) < 0)
+               return error(_("could not read '%s'"), path);
+
+       res = stage(git_dir, &buf, path);
+
+       strbuf_release(&buf);
+       return res;
+}
+
+static int stage_directory(const char *git_dir, const char *path, int recurse)
+{
+       int at_root = !*path;
+       DIR *dir = opendir(at_root ? "." : path);
+       struct dirent *e;
+       struct strbuf buf = STRBUF_INIT;
+       size_t len;
+       int res = 0;
+
+       if (!dir)
+               return error(_("could not open directory '%s'"), path);
+
+       if (!at_root)
+               strbuf_addf(&buf, "%s/", path);
+       len = buf.len;
+
+       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 && stage_file(git_dir, buf.buf)) ||
+                   (e->d_type == DT_DIR && recurse &&
+                    stage_directory(git_dir, buf.buf, recurse)))
+                       res = -1;
+       }
+
+       closedir(dir);
+       strbuf_release(&buf);
+       return res;
+}
+
+static int index_to_zip(const char *git_dir)
+{
+       struct child_process cp = CHILD_PROCESS_INIT;
+       struct strbuf oid = STRBUF_INIT;
+
+       cp.git_cmd = 1;
+       strvec_pushl(&cp.args, "--git-dir", git_dir, "write-tree", NULL);
+       if (pipe_command(&cp, NULL, 0, &oid, the_hash_algo->hexsz + 1,
+                        NULL, 0))
+               return error(_("could not write temporary tree object"));
+
+       strbuf_rtrim(&oid);
+       child_process_init(&cp);
+       cp.git_cmd = 1;
+       strvec_pushl(&cp.args, "--git-dir", git_dir, "archive", "-o", NULL);
+       strvec_pushf(&cp.args, "%s.zip", git_dir);
+       strvec_pushl(&cp.args, oid.buf, "--", NULL);
+       strbuf_release(&oid);
+       return run_command(&cp);
+}
+
 /* printf-style interface, expects `<key>=<value>` argument */
 static int set_config(const char *fmt, ...)
 {

@@ -499,6 +601,73 @@ cleanup:
        return res;
 }
 
+static int cmd_diagnose(int argc, const char **argv)
+{
+       struct option options[] = {
+               OPT_END(),
+       };
+       const char * const usage[] = {
+               N_("scalar diagnose [<enlistment>]"),
+               NULL
+       };
+       struct strbuf tmp_dir = STRBUF_INIT;
+       time_t now = time(NULL);
+       struct tm tm;
+       struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+       int res = 0;
+
+       argc = parse_options(argc, argv, NULL, options,
+                            usage, 0);
+
+       setup_enlistment_directory(argc, argv, usage, options, &buf);
+
+       strbuf_addstr(&buf, "/.scalarDiagnostics/scalar_");
+       strbuf_addftime(&buf, "%Y%m%d_%H%M%S", localtime_r(&now, &tm), 0, 0);
+       if (run_git("init", "-q", "-b", "dummy", "--bare", buf.buf, NULL)) {
+               res = error(_("could not initialize temporary repository: %s"),
+                           buf.buf);
+               goto diagnose_cleanup;
+       }
+       strbuf_realpath(&tmp_dir, buf.buf, 1);
+
+       strbuf_reset(&buf);
+       strbuf_addf(&buf, "Collecting diagnostic info into temp folder %s\n\n",
+                   tmp_dir.buf);
+
+       get_version_info(&buf, 1);
+
+       strbuf_addf(&buf, "Enlistment root: %s\n", the_repository->worktree);
+       fwrite(buf.buf, buf.len, 1, stdout);
+
+       if ((res = stage(tmp_dir.buf, &buf, "diagnostics.log")))
+               goto diagnose_cleanup;
+
+       if ((res = stage_directory(tmp_dir.buf, ".git", 0)) ||
+           (res = stage_directory(tmp_dir.buf, ".git/hooks", 0)) ||
+           (res = stage_directory(tmp_dir.buf, ".git/info", 0)) ||
+           (res = stage_directory(tmp_dir.buf, ".git/logs", 1)) ||
+           (res = stage_directory(tmp_dir.buf, ".git/objects/info", 0)))
+               goto diagnose_cleanup;
+
+       res = index_to_zip(tmp_dir.buf);
+
+       if (!res)
+               res = remove_dir_recursively(&tmp_dir, 0);
+
+       if (!res)
+               printf("\n"
+                      "Diagnostics complete.\n"
+                      "All of the gathered info is captured in '%s.zip'\n",
+                      tmp_dir.buf);
+
+diagnose_cleanup:
+       strbuf_release(&tmp_dir);
+       strbuf_release(&path);
+       strbuf_release(&buf);
+
+       return res;
+}
+
 static int cmd_list(int argc, const char **argv)
 {
        if (argc != 1)

@@ -800,6 +969,7 @@ static struct {
        { "reconfigure", cmd_reconfigure },
        { "delete", cmd_delete },
        { "version", cmd_version },
+       { "diagnose", cmd_diagnose },
        { NULL, NULL},
 };
 

contrib/scalar/scalar.txt +12 -0

index f416d637289..22583fe046e 100644
@@ -14,6 +14,7 @@ scalar register [<enlistment>]
 scalar unregister [<enlistment>]
 scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
 scalar reconfigure [ --all | <enlistment> ]
+scalar diagnose [<enlistment>]
 scalar delete <enlistment>
 
 DESCRIPTION

@@ -129,6 +130,17 @@ reconfigure the enlistment.
 With the `--all` option, all enlistments currently registered with Scalar
 will be reconfigured. Use this option after each Scalar upgrade.
 
+Diagnose
+~~~~~~~~
+
+diagnose [<enlistment>]::
+    When reporting issues with Scalar, it is often helpful to provide the
+    information gathered by this command, including logs and certain
+    statistics describing the data shape of the current enlistment.
++
+The output of this command is a `.zip` file that is written into
+a directory adjacent to the worktree in the `src` directory.
+
 Delete
 ~~~~~~
 

contrib/scalar/t/t9099-scalar.sh +13 -1

index 2e1502ad45e..ecd06e207c2 100755
@@ -65,6 +65,19 @@ test_expect_success 'scalar clone' '
        )
 '
 
+SQ="'"
+test_expect_success UNZIP 'scalar diagnose' '
+       scalar diagnose cloned >out &&
+       sed -n "s/.*$SQ\\(.*\\.zip\\)$SQ.*/\\1/p" <out >zip_path &&
+       zip_path=$(cat zip_path) &&
+       test -n "$zip_path" &&
+       unzip -v "$zip_path" &&
+       folder=${zip_path%.zip} &&
+       test_path_is_missing "$folder" &&
+       unzip -p "$zip_path" diagnostics.log >out &&
+       test_file_not_empty out
+'
+
 test_expect_success 'scalar reconfigure' '
        git init one/src &&
        scalar register one &&
--

gitgitgadget

From: Matthew John Cheetham <[email protected]>

Teach the scalar diagnose command to gather file size information about pack files.

Signed-off-by Matthew John Cheetham <[email protected]>

---
 contrib/scalar/scalar.c          | 39 ++++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh |  2 ++
 2 files changed, 41 insertions(+)

contrib/scalar/scalar.c +10 -0

index e26fb2fc018..690933ffdf3 100644
@@ -653,6 +653,39 @@ cleanup:
 	return res;
 }
 
+static void dir_file_stats(struct strbuf *buf, const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *e;
+	struct stat e_stat;
+	struct strbuf file_path = STRBUF_INIT;
+	size_t base_path_len;
+
+	if (!dir)
+		return;

Quoted 29 lines · Matthew John Cheetham via GitGitGadget

From: Matthew John Cheetham <[email protected]>

Teach the scalar diagnose command to gather file size information about pack files.

Signed-off-by Matthew John Cheetham <[email protected]>

---
 contrib/scalar/scalar.c          | 39 ++++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh |  2 ++
 2 files changed, 41 insertions(+)

contrib/scalar/scalar.c +10 -0

index e26fb2fc018..690933ffdf3 100644
@@ -653,6 +653,39 @@ cleanup:
 	return res;
 }
 
+static void dir_file_stats(struct strbuf *buf, const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *e;
+	struct stat e_stat;
+	struct strbuf file_path = STRBUF_INIT;
+	size_t base_path_len;
+
+	if (!dir)
+		return;

contrib/scalar/scalar.c cleanup: · line 675

Quoted 10 lines

contrib/scalar/scalar.c +10 -0

+
+	strbuf_addstr(buf, "Contents of ");
+	strbuf_add_absolute_path(buf, path);
+	strbuf_addstr(buf, ":\n");
+
+	strbuf_add_absolute_path(&file_path, path);
+	strbuf_addch(&file_path, '/');
+	base_path_len = file_path.len;
+
+	while ((e = readdir(dir)) != NULL)

Hmm. Is there a reason that this couldn't use for_each_file_in_pack_dir() with a callback that just does the stat() and buffer manipulation?

I don't think it's critical either way, but it would eliminate some of the boilerplate that is shared between this implementation and the one that already exists in for_each_file_in_pack_dir().

Quoted 45 lines · Taylor Blau

Quoted 29 lines · Matthew John Cheetham via GitGitGadget

From: Matthew John Cheetham <[email protected]>

Teach the scalar diagnose command to gather file size information about pack files.

Signed-off-by Matthew John Cheetham <[email protected]>

---
 contrib/scalar/scalar.c          | 39 ++++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh |  2 ++
 2 files changed, 41 insertions(+)

contrib/scalar/scalar.c +10 -0

index e26fb2fc018..690933ffdf3 100644
@@ -653,6 +653,39 @@ cleanup:
 	return res;
 }
 
+static void dir_file_stats(struct strbuf *buf, const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *e;
+	struct stat e_stat;
+	struct strbuf file_path = STRBUF_INIT;
+	size_t base_path_len;
+
+	if (!dir)
+		return;

contrib/scalar/scalar.c cleanup: · line 675

Quoted 10 lines

contrib/scalar/scalar.c +10 -0

+
+	strbuf_addstr(buf, "Contents of ");
+	strbuf_add_absolute_path(buf, path);
+	strbuf_addstr(buf, ":\n");
+
+	strbuf_add_absolute_path(&file_path, path);
+	strbuf_addch(&file_path, '/');
+	base_path_len = file_path.len;
+
+	while ((e = readdir(dir)) != NULL)

Hmm. Is there a reason that this couldn't use for_each_file_in_pack_dir() with a callback that just does the stat() and buffer manipulation?

Quoted 3 lines

I don't think it's critical either way, but it would eliminate some of the boilerplate that is shared between this implementation and the one that already exists in for_each_file_in_pack_dir().

It's helpful to see if there are other crud files in the pack directory. This method is also extended in microsoft/git to scan the alternates directory (which we expect to exist as the "shared objects cache).

We might want to modify the implementation in this series to run dir_file_stats() on each odb in the_repository. This would give us the data for the shared object cache for free while being more general to other Git repos. (It would require us to do some reaction work in microsoft/git and be a change of behavior, but we are the only ones who have looked at these diagnose files before, so that change will be easy to manage.)

contrib/scalar/scalar.c static void dir_file_stats(struct strbuf *buf, const char *path) · line 730

Quoted 4 lines · Matthew John Cheetham via GitGitGadget

contrib/scalar/scalar.c +4 -0

+	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)) {

What is this call to hex_to_bytes() for? I assume it's checking to make sure the directory we're looking at is one of the shards of loose objects.

Similar to my suggestion on the previous patch, I think that we could get rid of this function entirely and replace it with a call to for_each_loose_file_in_objdir().

From: Matthew John Cheetham <[email protected]>

When operating at the scale that Scalar wants to support, certain data shapes are more likely to cause undesirable performance issues, such as large numbers or large sizes of loose objects.

By including statistics about this, scalar diagnose now makes it easier to identify such scenarios.

Signed-off-by Matthew John Cheetham <[email protected]>

---
 contrib/scalar/scalar.c          | 60 ++++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh |  2 ++
 2 files changed, 62 insertions(+)

contrib/scalar/scalar.c +32 -0

index 690933ffdf3..c0ad4948215 100644
@@ -686,6 +686,60 @@ static void dir_file_stats(struct strbuf *buf, const char *path)
        closedir(dir);
 }
 
+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);

contrib/scalar/scalar.c +11 -0

+
+       strbuf_release(&count_path);
+       closedir(dir);
+}
+
 static int cmd_diagnose(int argc, const char **argv)
 {
        struct option options[] = {

@@ -734,6 +788,12 @@ static int cmd_diagnose(int argc, const char **argv)
        if ((res = stage(tmp_dir.buf, &buf, "packs-local.txt")))
                goto diagnose_cleanup;
 
+       strbuf_reset(&buf);
+       loose_objs_stats(&buf, ".git/objects");
+
+       if ((res = stage(tmp_dir.buf, &buf, "objects-local.txt")))
+               goto diagnose_cleanup;
+
        if ((res = stage_directory(tmp_dir.buf, ".git", 0)) ||
            (res = stage_directory(tmp_dir.buf, ".git/hooks", 0)) ||
            (res = stage_directory(tmp_dir.buf, ".git/info", 0)) ||

contrib/scalar/t/t9099-scalar.sh +2 -1

index b1745851e31..f2ec156d819 100755
@@ -77,6 +77,8 @@ test_expect_success UNZIP 'scalar diagnose' '
        unzip -p "$zip_path" diagnostics.log >out &&
        test_file_not_empty out &&
        unzip -p "$zip_path" packs-local.txt >out &&
+       test_file_not_empty out &&
+       unzip -p "$zip_path" objects-local.txt >out &&
        test_file_not_empty out
 '
 
--

gitgitgadget

Quoted 6 lines · Matthew John Cheetham via GitGitGadget

From: Matthew John Cheetham <[email protected]>

When operating at the scale that Scalar wants to support, certain data shapes are more likely to cause undesirable performance issues, such as large numbers or large sizes of loose objects.

Makes sense.

Quoted 49 lines

By including statistics about this, scalar diagnose now makes it easier to identify such scenarios.

Signed-off-by Matthew John Cheetham <[email protected]>

---
 contrib/scalar/scalar.c          | 60 ++++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh |  2 ++
 2 files changed, 62 insertions(+)

contrib/scalar/scalar.c +32 -0

index 690933ffdf3..c0ad4948215 100644
@@ -686,6 +686,60 @@ static void dir_file_stats(struct strbuf *buf, const char *path)
        closedir(dir);
 }
 
+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);

contrib/scalar/scalar.c static void dir_file_stats(struct strbuf *buf, const char *path) · line 730

Quoted 10 lines

contrib/scalar/scalar.c +10 -0

+       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)) {

You only recurse into directories, ignoring individual files.

contrib/scalar/scalar.c static void dir_file_stats(struct strbuf *buf, const char *path) · line 734

Quoted 4 lines

contrib/scalar/scalar.c +4 -0

+                       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);

This shows the number of files within a directory.

contrib/scalar/scalar.c static void dir_file_stats(struct strbuf *buf, const char *path) · line 737

Quoted 3 lines

contrib/scalar/scalar.c +3 -0

+               }
+
+       strbuf_addf(buf, "Total: %d loose objects", total);

and this shows the total number of files across all the directories.

But the commit message suggested you also wanted to check for large sizes of loose objects. Did that get ripped out at some point with the commit message not being updated, or is it perhaps going to be included later?

Quoted 37 lines

contrib/scalar/scalar.c +11 -0

+
+       strbuf_release(&count_path);
+       closedir(dir);
+}
+
 static int cmd_diagnose(int argc, const char **argv)
 {
        struct option options[] = {

@@ -734,6 +788,12 @@ static int cmd_diagnose(int argc, const char **argv)
        if ((res = stage(tmp_dir.buf, &buf, "packs-local.txt")))
                goto diagnose_cleanup;
 
+       strbuf_reset(&buf);
+       loose_objs_stats(&buf, ".git/objects");
+
+       if ((res = stage(tmp_dir.buf, &buf, "objects-local.txt")))
+               goto diagnose_cleanup;
+
        if ((res = stage_directory(tmp_dir.buf, ".git", 0)) ||
            (res = stage_directory(tmp_dir.buf, ".git/hooks", 0)) ||
            (res = stage_directory(tmp_dir.buf, ".git/info", 0)) ||

contrib/scalar/t/t9099-scalar.sh +2 -1

index b1745851e31..f2ec156d819 100755
@@ -77,6 +77,8 @@ test_expect_success UNZIP 'scalar diagnose' '
        unzip -p "$zip_path" diagnostics.log >out &&
        test_file_not_empty out &&
        unzip -p "$zip_path" packs-local.txt >out &&
+       test_file_not_empty out &&
+       unzip -p "$zip_path" objects-local.txt >out &&
        test_file_not_empty out
 '
 
--

gitgitgadget

Over the course of the years, we developed a sub-command that gathers diagnostic data into a .zip file that can then be attached to bug reports. This sub-command turned out to be very useful in helping Scalar developers identify and fix issues.

Quoted 4 lines · Johannes Schindelin via GitGitGadget

Over the course of the years, we developed a sub-command that gathers diagnostic data into a .zip file that can then be attached to bug reports. This sub-command turned out to be very useful in helping Scalar developers identify and fix issues.

For historical context: The 'diagnose' command was implemented in VFS for Git and ported to the C# version of Scalar before 'git bugreport' existed, but they serve very similar purposes.

I wonder if 'scalar diagnose' could include some of the information captured by 'git bugreport' or whether this implementation of 'diagnose' could help inform 'git bugreport' in any way.

CC'ing Emily for thoughts.

Thanks,
-Stolee

From: Johannes Schindelin <[email protected]>

With the --add-file-with-content=<path>:<content> option, git archive now supports use cases where relatively trivial files need to be added that do not exist on disk.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Documentation/git-archive.txt | 11 ++++++++
 archive.c                     | 51 +++++++++++++++++++++++++++++------
 t/t5003-archive-zip.sh        | 12 +++++++++
 3 files changed, 66 insertions(+), 8 deletions(-)

Documentation/git-archive.txt +0 -0

index bc4e76a7834..1b52a0a65a1 100644
@@ -61,6 +61,17 @@ OPTIONS
 	by concatenating the value for `--prefix` (if any) and the

Documentation/git-archive.txt +0 -0

 --worktree-attributes::
 	Look for attributes in .gitattributes files in the working tree
 	as well (see <<ATTRIBUTES>>).

archive.c +9 -2

index a3bbb091256..172efd690c3 100644
@@ -263,6 +263,7 @@ static int queue_or_write_archive_entry(const struct object_id *oid,
 struct extra_file_info {
 	char *base;
 	struct stat stat;
+	void *content;
 };
 
 int write_archive_entries(struct archiver_args *args,

@@ -337,7 +338,13 @@ int write_archive_entries(struct archiver_args *args,
 		strbuf_addstr(&path_in_archive, basename(path));
 
 		strbuf_reset(&content);
-		if (strbuf_read_file(&content, path, info->stat.st_size) < 0)
+		if (info->content)
+			err = write_entry(args, &fake_oid, path_in_archive.buf,
+					  path_in_archive.len,
+					  info->stat.st_mode,
+					  info->content, info->stat.st_size);
+		else if (strbuf_read_file(&content, path,
+					  info->stat.st_size) < 0)
 			err = error_errno(_("could not read '%s'"), path);
 		else
 			err = write_entry(args, &fake_oid, path_in_archive.buf,

@@ -493,6 +500,7 @@ static void extra_file_info_clear(void *util, const char *str)
 {
 	struct extra_file_info *info = util;
 	free(info->base);
+	free(info->content);
 	free(info);
 }
 

@@ -514,14 +522,38 @@ static int add_file_cb(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 
-	path = prefix_filename(args->prefix, arg);

archive.c +28 -0

+		path = prefix_filename(args->prefix, arg);
+		if (stat(path, &info->stat))
+			die(_("File not found: %s"), path);
+		if (!S_ISREG(info->stat.st_mode))
+			die(_("Not a regular file: %s"), path);
+		info->content = NULL; /* read the file later */
+	} else {
+		const char *colon = strchr(arg, ':');
+		char *p;
+
+		if (!colon)
+			die(_("missing colon: '%s'"), arg);
+
+		p = xstrndup(arg, colon - arg);
+		if (!args->prefix)
+			path = p;
+		else {
+			path = prefix_filename(args->prefix, p);
+			free(p);
+		}
+		memset(&info->stat, 0, sizeof(info->stat));
+		info->stat.st_mode = S_IFREG | 0644;
+		info->content = xstrdup(colon + 1);
+		info->stat.st_size = strlen(info->content);
+	}
+	item = string_list_append_nodup(&args->extra_files, path);
+	item->util = info;
+
 	return 0;
 }
 

archive.c +1 -0

+		  add_file_cb, (intptr_t)&base },
 		OPT_STRING('o', "output", &output, N_("file"),
 			N_("write the archive to this file")),
 		OPT_BOOL(0, "worktree-attributes", &worktree_attributes,

t/t5003-archive-zip.sh +12 -0

index 1e6d18b140e..8ff1257f1a0 100755
@@ -206,6 +206,18 @@ test_expect_success 'git archive --format=zip --add-file' '
 check_zip with_untracked
 check_added with_untracked untracked untracked
 
+test_expect_success UNZIP 'git archive --format=zip --add-file-with-content' '
+	git archive --format=zip >with_file_with_content.zip \
+		--add-file-with-content=hello:world $EMPTY_TREE &&
+	test_when_finished "rm -rf tmp-unpack" &&
+	mkdir tmp-unpack && (
+		cd tmp-unpack &&
+		"$GIT_UNZIP" ../with_file_with_content.zip &&
+		test_path_is_file hello &&
+		test world = $(cat hello)
+	)
+'
+
 test_expect_success 'git archive --format=zip --add-file twice' '
 	echo untracked >untracked &&
 	git archive --format=zip --prefix=one/ --add-file=untracked \

We could use that option in Git's own Makefile to add the file named "version", which contains $GIT_VERSION. Hmm, but it also contains a terminating newline, which would be a bit tricky (but not impossible) to add. Would it make sense to add one automatically if it's missing (e.g. with strbuf_complete_line)? Not sure.

Quoted 5 lines · René Scharfe

We could use that option in Git's own Makefile to add the file named "version", which contains $GIT_VERSION. Hmm, but it also contains a terminating newline, which would be a bit tricky (but not impossible) to add. Would it make sense to add one automatically if it's missing (e.g. with strbuf_complete_line)? Not sure.

I do not think it is a good UI to give raw file content from the command line, which will be usable only for trivial, even single liner files, and forces people to learn two parallel option, one for trivial ones and the other for contents with meaningful size.

"--add-blob=<path>:<blob-object-name>" may be another option, useful when you have done "hash-object -w" already, and can be used to add single-liner, or an entire novel.

In any case, "--add-file=<file>", which we already have, would be more appropriate feature to use to record our "version" file, so there is no need to change our Makefile for it.

Quoted 6 lines

Quoted 5 lines

We could use that option in Git's own Makefile to add the file named "version", which contains $GIT_VERSION. Hmm, but it also contains a terminating newline, which would be a bit tricky (but not impossible) to add. Would it make sense to add one automatically if it's missing (e.g. with strbuf_complete_line)? Not sure.

Quoted 4 lines

I do not think it is a good UI to give raw file content from the command line, which will be usable only for trivial, even single liner files, and forces people to learn two parallel option, one for trivial ones and the other for contents with meaningful size.

Nevertheless, it is still the most elegant way that I can think of to generate a diagnostic .zip file without messing up the very things that are to be diagnosed: the repository and the worktree.

Quoted 11 lines · Johannes Schindelin

Quoted 10 lines

Quoted 5 lines

We could use that option in Git's own Makefile to add the file named "version", which contains $GIT_VERSION. Hmm, but it also contains a terminating newline, which would be a bit tricky (but not impossible) to add. Would it make sense to add one automatically if it's missing (e.g. with strbuf_complete_line)? Not sure.

I do not think it is a good UI to give raw file content from the command line, which will be usable only for trivial, even single liner files, and forces people to learn two parallel option, one for trivial ones and the other for contents with meaningful size.

Quoted 3 lines

Nevertheless, it is still the most elegant way that I can think of to generate a diagnostic .zip file without messing up the very things that are to be diagnosed: the repository and the worktree.

Puzzled. Are you feeding contents of a .zip file from the command line?

I was mostly worried about busting command line argument limit by trying to feed too many bytes, as the ceiling is fairly low on some platforms.

Another worry was that when <contents> can have arbitrary bytes, with --opt=<path>:<contents> syntax, the input becomes ambiguous (i.e. "which colon is the <path> separator?"), without some way to escape a colon in the payload.

For a single-liner, --add-file-with-contents=<path>:<contents> would be an OK way, and my comment was not a strong objection against this new option existing. It was primarily an objection against changing the way to add the 'version' file in our "make dist" procedure to use it anyway.

But now I think about it more, I am becoming less happy about it existing in the first place.

This will throw another monkey wrench to Konstantin's plan [*] to make "git archive" output verifiable with the signature on original Git objects, but it is not a new problem ;-)

[Reference]

* https://lore.kernel.org/git/[email protected]/

Quoted 6 lines

Quoted 3 lines

Nevertheless, it is still the most elegant way that I can think of to generate a diagnostic .zip file without messing up the very things that are to be diagnosed: the repository and the worktree.

Puzzled. Are you feeding contents of a .zip file from the command line?

Kind of. Command line arguments are built and handed to write_archive() in-process. It's done by patch 3 and extended by 5 and 6.

The number of files is relatively low and they aren't huge, right?

Quoted 4 lines

Another worry was that when <contents> can have arbitrary bytes, with --opt=<path>:<contents> syntax, the input becomes ambiguous (i.e. "which colon is the <path> separator?"), without some way to escape a colon in the payload.

The first colon is the separator here.

Quoted 5 lines

This will throw another monkey wrench to Konstantin's plan [*] to make "git archive" output verifiable with the signature on original Git objects, but it is not a new problem ;-)

Quoted 3 lines

[Reference]

* https://lore.kernel.org/git/[email protected]/

I don't see the conflict: If an untracked file is added to an archive using --add-file, --add-file-with-content, or ZIP or tar then we'd *want* the verification against a signed commit or tag to fail, no? A different signature would be required for the non-tracked parts.

Quoted 1 line · René Scharfe

The number of files is relatively low and they aren't huge, right?

As long as it is expected to fit on the command line, that's fine. But if the question is "it is OK to add a new option with known limitation", then it should be stated a bit differently.

Quoted 9 lines

Quoted 8 lines

This will throw another monkey wrench to Konstantin's plan [*] to make "git archive" output verifiable with the signature on original Git objects, but it is not a new problem ;-)

[Reference]

* https://lore.kernel.org/git/[email protected]/

Quoted 4 lines

I don't see the conflict: If an untracked file is added to an archive using --add-file, --add-file-with-content, or ZIP or tar then we'd *want* the verification against a signed commit or tag to fail, no? A different signature would be required for the non-tracked parts.

Yes, which is exactly how this (and existing --add-file) makes Konstantin's plan much less useful.

Quoted 2 lines

Yes, which is exactly how this (and existing --add-file) makes Konstantin's plan much less useful.

People added untracked files to archives before --add-file existed.

--add-file-with-content could be used to add the .GIT_ARCHIVE_SIG file.

Additional untracked files would need a manifest to specify which files are (not) covered by the signed commit/tag. Or the .GIT_ARCHIVE_SIG files could be added just after the signed files as a rule, before any other untracked files, as some kind of a separator.

Just listing untracked files and verifying the others might still be useful. Warning about untracked files shadowing tracked ones would be very useful.

Some equivalent to the .GIT_ARCHIVE_SIG file containing a signature of the untracked files could optionally be added at the end to allow full verification -- but would require signing at archive creation time.

Quoted 2 lines · René Scharfe

Quoted 2 lines

Yes, which is exactly how this (and existing --add-file) makes Konstantin's plan much less useful.

Quoted 4 lines

People added untracked files to archives before --add-file existed.

--add-file-with-content could be used to add the .GIT_ARCHIVE_SIG file.

Quoted 4 lines

Additional untracked files would need a manifest to specify which files are (not) covered by the signed commit/tag. Or the .GIT_ARCHIVE_SIG files could be added just after the signed files as a rule, before any other untracked files, as some kind of a separator.

Or if people do not _exclude_ tracked files from the archive, then the verifier who has a tarball and a Git tree object can consult the tree object to see which ones are added untracked cruft.

Quoted 3 lines

Some equivalent to the .GIT_ARCHIVE_SIG file containing a signature of the untracked files could optionally be added at the end to allow full verification -- but would require signing at archive creation time.

Yeah, and at that point, it is not much more convenient than just signing the whole archive (sans the SIG part, obviously), which is what people have always done ;-)

Quoted 3 lines

Or if people do not _exclude_ tracked files from the archive, then the verifier who has a tarball and a Git tree object can consult the tree object to see which ones are added untracked cruft.

True, but if you have the tree objects then you probably also have the blobs and don't need the archive? Or is this some kind of sparse checkout scenario?

Quoted 7 lines · René Scharfe

Quoted 3 lines

Or if people do not _exclude_ tracked files from the archive, then the verifier who has a tarball and a Git tree object can consult the tree object to see which ones are added untracked cruft.

True, but if you have the tree objects then you probably also have the blobs and don't need the archive? Or is this some kind of sparse checkout scenario?

My phrasing was too loose. This is a "how to verify a distro tarball" (without having a copy of the project repository, but with some common tools like "git") scenario.

The verifier has a tarball. In addition, the verifier knows the object name of the Git tree object the tarball was taken from, and somehow trusts that the object name is genuine. We can do either "untar + git-add . && git write-tree" or its equivalent to see how the contents hashes to the expected tree (or not).

How the verifier trusts the object name is out of scope (it may come from a copy of a signed tag object and a copy of the commit object that the tag points at and the contents of signed tag object, with its known format, would allow you to write a stand alone tool to verify the PGP signature).

Line-end normalization and smudge filter rules may get in the way, if we truly did "untar" to the filesystem, but I thought "git archive" didn't do smudge conversion and core.crlf handling when creating the archive?

Quoted 5 lines

The verifier has a tarball. In addition, the verifier knows the object name of the Git tree object the tarball was taken from, and somehow trusts that the object name is genuine. We can do either "untar + git-add . && git write-tree" or its equivalent to see how the contents hashes to the expected tree (or not).

...
Right, but the tree hash does not directly allow to see which objects are tracked or not.

So we do need some extra "manifest" to declare what's untracked etc., if we allow --add-file etc. to munge the tree when creating a tarball out of it.

Am 13.02.22 um 07:25 schrieb Junio C Hamano:

Quoted 4 lines

So we do need some extra "manifest" to declare what's untracked etc., if we allow --add-file etc. to munge the tree when creating a tarball out of it.

Right, or get that information from the order of files in the archive, by having tracked files come first, then the signature file with a certain name and then untracked files.

Am 06.02.22 um 23:39 schrieb Johannes Schindelin via GitGitGadget:

Quoted 21 lines

From: Johannes Schindelin <[email protected]>

With the --add-file-with-content=<path>:<content> option, git archive now supports use cases where relatively trivial files need to be added that do not exist on disk.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Documentation/git-archive.txt | 11 ++++++++
 archive.c                     | 51 +++++++++++++++++++++++++++++------
 t/t5003-archive-zip.sh        | 12 +++++++++
 3 files changed, 66 insertions(+), 8 deletions(-)

Documentation/git-archive.txt +0 -0

index bc4e76a7834..1b52a0a65a1 100644
@@ -61,6 +61,17 @@ OPTIONS
 	by concatenating the value for `--prefix` (if any) and the
 	basename of <file>.
 

Documentation/git-archive.txt OPTIONS · line 71

Quoted 8 lines

Documentation/git-archive.txt +8 -0

+--add-file-with-content=<path>:<content>::
+	Add the specified contents to the archive.  Can be repeated to add
+	multiple files.  The path of the file in the archive is built
+	by concatenating the value for `--prefix` (if any) and the
+	basename of <file>.
++
+The `<path>` cannot contain any colon, the file mode is limited to
+a regular file, and the option may be subject platform-dependent

s/subject/& to/

Documentation/git-archive.txt OPTIONS · line 74

Quoted 3 lines

Documentation/git-archive.txt +3 -0

+command-line limits. For non-trivial cases, write an untracked file
+and use `--add-file` instead.
+

We could use that option in Git's own Makefile to add the file named "version", which contains $GIT_VERSION.

Hmm, but it also contains a terminating newline, which would be a bit tricky (but not impossible) to add. Would it make sense to add one automatically if it's missing (e.g. with strbuf_complete_line)? Not sure.

Quoted 43 lines

Documentation/git-archive.txt +0 -0

 --worktree-attributes::
 	Look for attributes in .gitattributes files in the working tree
 	as well (see <<ATTRIBUTES>>).

archive.c +9 -2

index a3bbb091256..172efd690c3 100644
@@ -263,6 +263,7 @@ static int queue_or_write_archive_entry(const struct object_id *oid,
 struct extra_file_info {
 	char *base;
 	struct stat stat;
+	void *content;
 };
 
 int write_archive_entries(struct archiver_args *args,

@@ -337,7 +338,13 @@ int write_archive_entries(struct archiver_args *args,
 		strbuf_addstr(&path_in_archive, basename(path));
 
 		strbuf_reset(&content);
-		if (strbuf_read_file(&content, path, info->stat.st_size) < 0)
+		if (info->content)
+			err = write_entry(args, &fake_oid, path_in_archive.buf,
+					  path_in_archive.len,
+					  info->stat.st_mode,
+					  info->content, info->stat.st_size);
+		else if (strbuf_read_file(&content, path,
+					  info->stat.st_size) < 0)
 			err = error_errno(_("could not read '%s'"), path);
 		else
 			err = write_entry(args, &fake_oid, path_in_archive.buf,

@@ -493,6 +500,7 @@ static void extra_file_info_clear(void *util, const char *str)
 {
 	struct extra_file_info *info = util;
 	free(info->base);
+	free(info->content);
 	free(info);
 }
 

@@ -514,14 +522,38 @@ static int add_file_cb(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 
-	path = prefix_filename(args->prefix, arg);

archive.c static int add_file_cb(const struct option *opt, const char *arg, int unset) · line 528

Quoted 10 lines

archive.c +3 -6

-	item = string_list_append_nodup(&args->extra_files, path);
-	item->util = info = xmalloc(sizeof(*info));
+	info = xmalloc(sizeof(*info));
 	info->base = xstrdup_or_null(base);
-	if (stat(path, &info->stat))
-		die(_("File not found: %s"), path);
-	if (!S_ISREG(info->stat.st_mode))
-		die(_("Not a regular file: %s"), path);
+
+	if (strcmp(opt->long_name, "add-file-with-content")) {

Equivalent to:

	if (!strcmp(opt->long_name, "add-file")) {

I mention that because the inequality check confused me a bit at first.

Quoted 33 lines

archive.c +28 -0

+		path = prefix_filename(args->prefix, arg);
+		if (stat(path, &info->stat))
+			die(_("File not found: %s"), path);
+		if (!S_ISREG(info->stat.st_mode))
+			die(_("Not a regular file: %s"), path);
+		info->content = NULL; /* read the file later */
+	} else {
+		const char *colon = strchr(arg, ':');
+		char *p;
+
+		if (!colon)
+			die(_("missing colon: '%s'"), arg);
+
+		p = xstrndup(arg, colon - arg);
+		if (!args->prefix)
+			path = p;
+		else {
+			path = prefix_filename(args->prefix, p);
+			free(p);
+		}
+		memset(&info->stat, 0, sizeof(info->stat));
+		info->stat.st_mode = S_IFREG | 0644;
+		info->content = xstrdup(colon + 1);
+		info->stat.st_size = strlen(info->content);
+	}
+	item = string_list_append_nodup(&args->extra_files, path);
+	item->util = info;
+
 	return 0;
 }
 

archive.c static int parse_archive_args(int argc, const char **argv, · line 590

Quoted 5 lines

archive.c +2 -0

@@ -554,6 +586,9 @@ static int parse_archive_args(int argc, const char **argv,
 		{ OPTION_CALLBACK, 0, "add-file", args, N_("file"),
 		  N_("add untracked file to archive"), 0, add_file_cb,
 		  (intptr_t)&base },
+		{ OPTION_CALLBACK, 0, "add-file-with-content", args,
+		  N_("file"), N_("add untracked file to archive"), 0,

                      ^^^^

"<file>" seems wrong, because there is no actual file. It should rather be "<name>:<content>" for the virtual one, right?

From: Johannes Schindelin <[email protected]>

Over the course of Scalar's development, it became obvious that there is a need for a command that can gather all kinds of useful information that can help identify the most typical problems with large worktrees/repositories.

The diagnose command is the culmination of this hard-won knowledge: it gathers the installed hooks, the config, a couple statistics describing the data shape, among other pieces of information, and then wraps everything up in a tidy, neat .zip archive.

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.

Signed-off-by Johannes Schindelin <[email protected]>

---
 contrib/scalar/scalar.c          | 143 +++++++++++++++++++++++++++++++
 contrib/scalar/scalar.txt        |  12 +++
 contrib/scalar/t/t9099-scalar.sh |  14 +++
 3 files changed, 169 insertions(+)

contrib/scalar/scalar.c +91 -0

index 00dcd4b50ef..30ce0799c7a 100644
@@ -11,6 +11,7 @@
 #include "dir.h"
 #include "packfile.h"
 #include "help.h"
+#include "archive.h"
 
 /*
  * Remove the deepest subdirectory in the provided path string. Path must not

@@ -261,6 +262,44 @@ 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 = opendir(at_root ? "." : path);
+	struct dirent *e;
+	struct strbuf buf = STRBUF_INIT;
+	size_t len;
+	int res = 0;
+
+	if (!dir)
+		return error(_("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)
+			res = -1;
+		else if (recurse)
+		     add_directory_to_archiver(archiver_args, buf.buf, recurse);
+	}
+
+	closedir(dir);
+	strbuf_release(&buf);
+	return res;
+}
+
 /* printf-style interface, expects `<key>=<value>` argument */
 static int set_config(const char *fmt, ...)
 {

@@ -501,6 +540,109 @@ cleanup:
 	return res;
 }
 
+static int cmd_diagnose(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END(),
+	};
+	const char * const usage[] = {
+		N_("scalar diagnose [<enlistment>]"),
+		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 path = STRBUF_INIT, buf = STRBUF_INIT;
+	size_t off;
+	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;
+	}
+	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);
+

contrib/scalar/scalar.c +4 -0

+
+	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)) ||

contrib/scalar/scalar.c +16 -0

+
+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(&path);
+	strbuf_release(&buf);
+
+	return res;
+}
+
 static int cmd_list(int argc, const char **argv)
 {
 	if (argc != 1)

@@ -802,6 +944,7 @@ static struct {
 	{ "reconfigure", cmd_reconfigure },
 	{ "delete", cmd_delete },
 	{ "version", cmd_version },
+	{ "diagnose", cmd_diagnose },
 	{ NULL, NULL},
 };
 

contrib/scalar/scalar.txt +12 -0

index f416d637289..22583fe046e 100644
@@ -14,6 +14,7 @@ scalar register [<enlistment>]
 scalar unregister [<enlistment>]
 scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
 scalar reconfigure [ --all | <enlistment> ]
+scalar diagnose [<enlistment>]
 scalar delete <enlistment>
 
 DESCRIPTION

@@ -129,6 +130,17 @@ reconfigure the enlistment.
 With the `--all` option, all enlistments currently registered with Scalar
 will be reconfigured. Use this option after each Scalar upgrade.
 
+Diagnose
+~~~~~~~~
+
+diagnose [<enlistment>]::
+    When reporting issues with Scalar, it is often helpful to provide the
+    information gathered by this command, including logs and certain
+    statistics describing the data shape of the current enlistment.
++
+The output of this command is a `.zip` file that is written into
+a directory adjacent to the worktree in the `src` directory.
+
 Delete
 ~~~~~~
 

contrib/scalar/t/t9099-scalar.sh +14 -0

index 9d83fdf25e8..bbd07a44426 100755
@@ -90,4 +90,18 @@ test_expect_success '`scalar [...] <dir>` errors out when dir is missing' '
 	grep "cloned. does not exist" err
 '
 
+SQ="'"
+test_expect_success UNZIP 'scalar diagnose' '
+	scalar clone "file://$(pwd)" cloned --single-branch &&
+	scalar diagnose cloned >out &&
+	sed -n "s/.*$SQ\\(.*\\.zip\\)$SQ.*/\\1/p" <out >zip_path &&
+	zip_path=$(cat zip_path) &&
+	test -n "$zip_path" &&
+	unzip -v "$zip_path" &&
+	folder=${zip_path%.zip} &&
+	test_path_is_missing "$folder" &&
+	unzip -p "$zip_path" diagnostics.log >out &&
+	test_file_not_empty out
+'
+
 test_done

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 9 lines

contrib/scalar/scalar.c +9 -0

+	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);
+

contrib/scalar/scalar.c cleanup: · line 604

Quoted 10 lines

contrib/scalar/scalar.c +10 -0

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

Quoted 4 lines

contrib/scalar/scalar.c +4 -0

+
+	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)) ||

contrib/scalar/scalar.c cleanup: · line 618

Quoted 10 lines

contrib/scalar/scalar.c +10 -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.

contrib/scalar/scalar.c cleanup: · line 620

Quoted 2 lines

contrib/scalar/scalar.c +2 -0

+	res = write_archive(archiver_args.nr, (const char **)argv_copy, NULL,
+			    the_repository, NULL, 0);

Ah -- no shell means no command line length limits. :)

Quoted 4 lines

contrib/scalar/scalar.c +4 -0

+	if (res) {
+		error(_("failed to write archive"));
+		goto diagnose_cleanup;
+	}

contrib/scalar/scalar.c cleanup: · line 630

Quoted 6 lines

contrib/scalar/scalar.c +6 -0

+
+	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?

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.

contrib/scalar/scalar.c cleanup: · line 636

Quoted 7 lines

contrib/scalar/scalar.c +6 -0

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