GSoC 2026 Week 2: Feedback and Iteration

Alright buckle up this is gonna be detailed. Week 2 has been a relatively smoother ride for me than Week 1.

Since I submitted my patches to the mailing list,
I have received some really good feedback.

Before we delve into the feedback,
I would like to break down my patches and explain what is happening in the code.

So that we can understand the feedback properly.

There are mainly 4 patches.

Starting with Patch 1.

As we previously discussed, rev-parse and repo-info have some overlap.
The path.* functions already exist in some form in rev-parse.

Now, since the code already exists,
why rewrite it? No need, right?

Patch 1 is about that.

We take this function from rev-parse:

+void format_path(struct strbuf *buf, const char *path,
+		 const char *prefix, enum path_format format)
+{
+	if (format == PATH_FORMAT_UNMODIFIED) {
+		strbuf_addstr(buf, path);
+		return;
+	}
+
+	if (format == PATH_FORMAT_RELATIVE) {
+		struct strbuf relative_buf = STRBUF_INIT;
+		struct strbuf real_path = STRBUF_INIT;
+		struct strbuf real_prefix = STRBUF_INIT;
+		char *cwd = NULL;
+
+		/*
+		 * We don't ever produce a relative path if prefix is NULL,
+		 * so set the prefix to the current directory so that we can
+		 * produce a relative path whenever possible.
+		 */
+		if (!prefix)
+			prefix = cwd = xgetcwd();
+
+		if (!is_absolute_path(path)) {
+			strbuf_realpath_forgiving(&real_path, path, 1);
+			path = real_path.buf;
+		}
+		if (!is_absolute_path(prefix)) {
+			strbuf_realpath_forgiving(&real_prefix, prefix, 1);
+			prefix = real_prefix.buf;
+		}
+
+		strbuf_addstr(buf, relative_path(path, prefix, &relative_buf));
+
+		strbuf_release(&relative_buf);
+		strbuf_release(&real_path);
+		strbuf_release(&real_prefix);
+		free(cwd);
+	} else if (format == PATH_FORMAT_RELATIVE_IF_SHARED) {
+		struct strbuf relative_buf = STRBUF_INIT;
+
+		/*
+		 * If we're using RELATIVE_IF_SHARED mode, then we want an
+		 * absolute path unless the two share a common prefix, so don't
+		 * default the prefix to the current working directory. Doing so
+		 * would cause a relative path to always be produced if possible.
+		 */
+		strbuf_addstr(buf, relative_path(path, prefix, &relative_buf));
+		strbuf_release(&relative_buf);
+	} else if (format == PATH_FORMAT_CANONICAL) {
+		struct strbuf canonical_buf = STRBUF_INIT;
+
+		strbuf_realpath_forgiving(&canonical_buf, path, 1);
+		strbuf_addbuf(buf, &canonical_buf);
+
+		strbuf_release(&canonical_buf);
+	}
+}
+
 REPO_GIT_PATH_FUNC(squash_msg, "SQUASH_MSG")
 REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
 REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
diff --git a/path.h b/path.h
index 0434ba5e07..a78e0fc141 100644
--- a/path.h
+++ b/path.h
@@ -262,6 +262,36 @@ enum scld_error safe_create_leading_directories_no_share(char *path);
 int safe_create_file_with_leading_directories(struct repository *repo,
 					      const char *path);

+/**
+ * The formatting strategy to apply when writing a path into a buffer.
+ */
+enum path_format {
+	/* Output the path exactly as-is without any modifications. */
+	PATH_FORMAT_UNMODIFIED,
+
+	/* Output a path relative to the provided directory prefix. */
+	PATH_FORMAT_RELATIVE,
+
+	/* Output a relative path only if the path shares a root with the prefix. */
+	PATH_FORMAT_RELATIVE_IF_SHARED,
+
+	/* Output a fully resolved, absolute canonical path. */
+	PATH_FORMAT_CANONICAL
+};
+
+/**
+ * Format a path according to the specified formatting strategy and append
+ * the result to the given strbuf.
+ *
+ * `buf`    : The string buffer to append the formatted path to.
+ * `path`   : The path string that needs to be formatted.
+ * `prefix` : The directory prefix to calculate relative offsets against.
+ * Pass NULL to default to the current working directory where applicable.
+ * `format` : The formatting behavior rule to execute.
+ */
+void format_path(struct strbuf *buf, const char *path,
+		 const char *prefix, enum path_format format);
+
 # ifdef USE_THE_REPOSITORY_VARIABLE
 #  include "strbuf.h"
 #  include "repository.h"
 # endif
				

We are adding this function to path.c.

Why path.c, and what is path.c?

Good question.

path.c is a file that contains helper functions and can be used by multiple builtin files.

It is basically Chuck Norris for path helpers ;)

So we take our function from rev-parse
and add it there, as shown above, with some tiny changes of course.

That is Patch 1.

Now Patch 2 is where, since we added a global function,
we remove a good chunk of code from rev-parse.

-static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def)
+static void print_path(const char *path, const char *prefix,
+		       enum path_format_type format, enum path_default_type def)
 {
-	char *cwd = NULL;
-	/*
-	 * We don't ever produce a relative path if prefix is NULL, so set the
-	 * prefix to the current directory so that we can produce a relative
-	 * path whenever possible.  If we're using RELATIVE_IF_SHARED mode, then
-	 * we want an absolute path unless the two share a common prefix, so don't
-	 * set it in that case, since doing so causes a relative path to always
-	 * be produced if possible.
-	 */
-	if (!prefix && (format != FORMAT_DEFAULT || def != DEFAULT_RELATIVE_IF_SHARED))
-		prefix = cwd = xgetcwd();
-	if (format == FORMAT_DEFAULT && def == DEFAULT_UNMODIFIED) {
-		puts(path);
-	} else if (format == FORMAT_RELATIVE ||
-		  (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE)) {
-		/*
-		 * In order for relative_path to work as expected, we need to
-		 * make sure that both paths are absolute paths.  If we don't,
-		 * we can end up with an unexpected absolute path that the user
-		 * didn't want.
-		 */
-		struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT;
-		if (!is_absolute_path(path)) {
-			strbuf_realpath_forgiving(&realbuf, path,  1);
-			path = realbuf.buf;
-		}
-		if (!is_absolute_path(prefix)) {
-			strbuf_realpath_forgiving(&prefixbuf, prefix, 1);
-			prefix = prefixbuf.buf;
-		}
-		puts(relative_path(path, prefix, &buf));
-		strbuf_release(&buf);
-		strbuf_release(&realbuf);
-		strbuf_release(&prefixbuf);
-	} else if (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE_IF_SHARED) {
-		struct strbuf buf = STRBUF_INIT;
-		puts(relative_path(path, prefix, &buf));
-		strbuf_release(&buf);
-	} else {
-		struct strbuf buf = STRBUF_INIT;
-		strbuf_realpath_forgiving(&buf, path, 1);
-		puts(buf.buf);
-		strbuf_release(&buf);
-	}
-	free(cwd);
+	struct strbuf sb = STRBUF_INIT;
+
+	strbuf_add_path(&sb, path, prefix, format, def);
+	puts(sb.buf);
+
+	strbuf_release(&sb);
 }

Do you see how we still use puts()?

That is because the global helper only handles adding the path to an existing strbuf,
while the function in rev-parse.c also prints it to the output stream.

That's the difference.

Now that we have made rev-parse use the global helper,
we have cleared a path forward for repo.c to use the same function that rev-parse uses.

Patch 3 can now add path.gitdir keys to repo-info.

Since no feedback was about changing the key architecture itself,
I am not going to explain it again, as it is the same as in the previous blog.

Now we add functions that add the paths to the buffer.

static int get_path_gitdir_absolute(struct repository *repo, struct strbuf *buf)
{
	const char *git_dir = repo_get_git_dir(repo);

	if (!git_dir)
		return error(_("unable to get git directory"));

	strbuf_add_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_CANONICAL, PATH_DEFAULT_UNMODIFIED);
	return 0;
}

static int get_path_gitdir_relative(struct repository *repo, struct strbuf *buf)
{
	const char *git_dir = repo_get_git_dir(repo);

	if (!git_dir)
		return error(_("unable to get git directory"));

	strbuf_add_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_RELATIVE, PATH_DEFAULT_UNMODIFIED);
	return 0;
}

Why two functions?

It is pretty obvious: one is for the absolute path and the other is for the relative path.

Notice how we use the enums.

Something you should know is that Patch 1 also takes the enums from rev-parse:

enum path_format_type {
	PATH_FORMAT_DEFAULT,
	PATH_FORMAT_RELATIVE,
	PATH_FORMAT_CANONICAL
};

enum path_default_type {
	PATH_DEFAULT_RELATIVE,
	PATH_DEFAULT_RELATIVE_IF_SHARED,
	PATH_DEFAULT_CANONICAL,
	PATH_DEFAULT_UNMODIFIED
};

and adds them to path.h.

Patch 2 then removes them from rev-parse.c.

So the functions start using common enums.

The same applies to Patch 4.

It follows the same style.

Feedback

Since the submission of these patches,
a good chunk of feedback has been recorded, and more is still coming in.

The first round of feedback was around the overall design.

One of the questions I asked in the cover letter was whether we should still keep a --path-format flag.

Lucas pointed out that if we are already exposing both .absolute and .relative variants as separate keys, then having a separate formatting flag does not make much sense. The format is already encoded in the key itself.

Another discussion was about whether we should provide a default key such as path.gitdir without any suffix.

Lucas highlighted both sides of the argument.

On the positive side, some paths naturally make more sense in either absolute or relative form, and always typing .absolute or .relative can feel a little verbose.

On the other hand, introducing a default means users now need to remember what that default actually is. More importantly, git repo info gives us a chance to avoid some of the ambiguities that already exist elsewhere instead of carrying them forward.

He also pointed out that if both absolute and relative variants exist as independent keys, then --all should print both of them. After all, --all should mean exactly that.

Another piece of feedback came while reviewing the tests.

Lucas noticed that I had semi-hardcoded the expected absolute path in the test helper:

expect_absolute=$(cd .. && pwd)/.git

While this works for gitdir, it would not work well for future path fields such as the repository root or a superproject working tree.

Instead, the expected absolute path should be passed as a parameter, just like the relative path already is.

Junio later replied to the thread and agreed with Lucas's observation, which was nice to see.

More testing feedback followed for Patch 4.

Lucas suggested looking at the existing git rev-parse tests and checking for corner cases.

For example, the documentation for --git-common-dir says:

Show $GIT_COMMON_DIR if defined, else $GIT_DIR

My tests only covered the basic case and did not exercise both branches of that behavior.

The suggestion was to mirror the coverage that already exists in rev-parse, which makes a lot of sense. If we are exposing equivalent information, then we should be testing equivalent edge cases as well.

There was also a small nitpick regarding the patch subject prefix.

Instead of:

[GSoC][PATCH]

Lucas suggested using:

[GSoC PATCH]

This is a tiny detail, but following project conventions makes patch series easier to read and keeps everything consistent with the rest of the mailing list.

The most significant technical feedback came from Phillip Wood.

Phillip's main concern was the API introduced in Patch 1.

The original API exposed two enums:

enum path_format_type
enum path_default_type

along with:

strbuf_add_path()

His observation was that the API felt very specific to rev-parse and was difficult to understand in isolation.

Instead, he suggested simplifying everything into a single enum that directly describes the desired formatting behavior.

Something along the lines of:

enum path_format {
	PATH_FORMAT_ABSOLUTE,
	PATH_FORMAT_CANONICAL,
	PATH_FORMAT_RELATIVE,
	PATH_FORMAT_RELATIVE_IF_SHARED,
	PATH_FORMAT_UNMODIFIED,
};

This would make the API easier to understand because callers would simply specify the format they want instead of mixing formatting modes with default behaviors.

He also suggested renaming the helper to something more generic like:

format_path()

rather than strbuf_add_path().

Another point Phillip raised was documentation.

Since this helper would now live in a public header and be used by multiple callers, it should clearly document what each parameter does and what behavior the function guarantees.

I thought this feedback was particularly valuable because it improved the API itself rather than just the implementation.

Phillip also questioned one of the design decisions discussed in the cover letter.

I had mentioned that I preferred:

path.gitdir.absolute
path.gitdir.relative

over:

path.absolute.gitdir
path.relative.gitdir

Part of my reasoning was that the latter would disrupt the lexicographical ordering of the field list.

Phillip pointed out that this is not actually true as long as the entries themselves are inserted in the correct order.

For example:

path.absolute.commondir
path.absolute.gitdir
path.relative.commondir
path.relative.gitdir

would still be perfectly sorted.

That was a fair point.

My real preference was more about grouping related keys together. I personally find it easier to discover both variants when they sit next to each other rather than having all absolute keys grouped separately from all relative keys.

Finally, Junio provided feedback on the cover letter itself.

This was probably one of the most useful lessons I took away from this round.

The original cover letter started by saying that the first two patches were self-explanatory and that readers should focus on the third and fourth patches.

Junio pointed out that this is exactly backwards.

The purpose of a cover letter is to explain what the series is about and convince people that it is worth reading.

At that point, readers have not yet looked at the patches and may not even know what problem is being solved.

Telling them to go read the first two patches themselves defeats the purpose of the cover letter.

He also pointed out that I referenced previous discussions without properly summarizing them.

That unintentionally excludes readers who were not part of those discussions or simply do not remember them.

This was a really useful reminder that communication is just as important as the code itself.

Overall, the feedback was extremely constructive.

Some comments improved the API design.
Some improved the tests.
Some improved the cover letter.
And some challenged the assumptions behind the design itself.

A large portion of this feedback made its way directly into v2 of the series, which is exactly how the review process is supposed to work.