On 2020-11-09 at 14:46:13, Johannes Schindelin wrote:

On Fri, 9 Oct 2020, brian m. carlson wrote:

quoted

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ed200c8af1..ec62b4cd16 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c

@@ -583,6 +583,76 @@ static void handle_ref_opt(const char *pattern, const char *prefix)
 	clear_ref_exclusion(&ref_excludes);
 }

+enum format_type {
+	/* We would like a relative path. */
+	FORMAT_RELATIVE,
+	/* We would like a canonical absolute path. */
+	FORMAT_CANONICAL,
+	/* We would like the default behavior. */
+	FORMAT_DEFAULT,
+};
+
+enum default_type {
+	/* Our default is a relative path. */
+	DEFAULT_RELATIVE,
+	/* Our default is a relative path if there's a shared root. */
+	DEFAULT_RELATIVE_IF_SHARED,
+	/* Our default is a canonical absolute path. */
+	DEFAULT_CANONICAL,
+	/* Our default is not to modify the item. */
+	DEFAULT_UNMODIFIED,
+};

I wonder whether it would make sense to consolidate these two enums into a
single one.

Technically, we can, but because there are cases in each one which don't
make sense in the other, we end up with a situation that is hard to
reason about in print_path, which is, by this point, already a little
complex.  So I think I'd prefer not to consolidate them.

quoted

 int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 {
 	int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;

@@ -595,6 +665,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	struct object_context unused;
 	struct strbuf buf = STRBUF_INIT;
 	const int hexsz = the_hash_algo->hexsz;
+	enum format_type format = FORMAT_DEFAULT;

 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);

@@ -650,8 +721,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			if (!argv[i + 1])
 				die("--git-path requires an argument");
 			strbuf_reset(&buf);
-			puts(relative_path(git_path("%s", argv[i + 1]),
-					   prefix, &buf));
+			print_path(git_path("%s", argv[i + 1]), prefix, format, DEFAULT_RELATIVE_IF_SHARED);

One thing that the original code did was to reuse the same `strbuf`. Not
sure whether this matters in practice.

I don't think it does.  I'll make sure to free it, though, since
strbuf_reset doesn't do that.

quoted

 			i++;
 			continue;
 		}

@@ -683,6 +753,16 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					show_file(arg, 0);
 				continue;
 			}
+			if (opt_with_value(arg, "--path-format", &arg)) {
+				if (!strcmp(arg, "absolute")) {
+					format = FORMAT_CANONICAL;
+				} else if (!strcmp(arg, "relative")) {
+					format = FORMAT_RELATIVE;
+				} else {
+					die("unknown argument to --path-format: %s", arg);
+				}
+				continue;
+			}
 			if (!strcmp(arg, "--default")) {
 				def = argv[++i];
 				if (!def)

@@ -803,7 +883,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			if (!strcmp(arg, "--show-toplevel")) {
 				const char *work_tree = get_git_work_tree();
 				if (work_tree)
-					puts(work_tree);
+					print_path(work_tree, prefix, format, DEFAULT_CANONICAL);

The way `print_path()`'s code is structured, it is not immediately obvious
to me whether the patch changes behavior here. I _suspect_ that we're now
calling `strbuf_realpath_missing()` and react to its return value, which
is different from before.

Wouldn't make `DEFAULT_UNMODIFIED` make more sense here?

It's documented to show the absolute path of the top of the repository,
so it should be safe to do either one.  Will switch.

quoted

 				else
 					die("this operation must be run in a work tree");
 				continue;

@@ -811,7 +891,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			if (!strcmp(arg, "--show-superproject-working-tree")) {
 				struct strbuf superproject = STRBUF_INIT;
 				if (get_superproject_working_tree(&superproject))
-					puts(superproject.buf);
+					print_path(superproject.buf, prefix, format, DEFAULT_CANONICAL);

Shouldn't this be `DEFAULT_UNMODIFIED`?

Same thing as above.  Will change.

quoted

@@ -868,14 +950,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				}
 				cwd = xgetcwd();
 				len = strlen(cwd);
-				printf("%s%s.git\n", cwd, len && cwd[len-1] != '/' ? "/" : "");
+				strbuf_reset(&buf);
+				strbuf_addf(&buf, "%s%s.git", cwd, len && cwd[len-1] != '/' ? "/" : "");

So `DEFAULT_CANONICAL` ensures a trailing `/`? I do not see that in
`print_path()`'s implementation, and also, I would love to see a different
name for that ("canonical", from my Java past, suggests something like
"real path" to me).

I don't think that's what's happening here.  I believe the intent is to
insert a slash between the current working directory and the ".git"
component if and only if the former lacks one.  My code doesn't change
that.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US