From: K Jayatheerth Date: Tue, 09 Jun 2026 04:41:38 GMT Subject: Re: [GSoC PATCH v2 3/4] repo: add path.gitdir with absolute and relative suffix formatting Message-ID: In-Reply-To: > > + if (format == PATH_FORMAT_UNMODIFIED) { > > + strbuf_addstr(buf, path); > > + return; > > + } > > + > > + if (format == PATH_FORMAT_RELATIVE) { > > nit: we could just continue the "else if" chain here instead of > restarting it. Ahh, good catch! True we can. > > + strbuf_realpath_forgiving(&canonical_buf, path, 1); > > + strbuf_addbuf(buf, &canonical_buf); > > Do we need `canonical_buf` here? Can we just add the path to `buf` > directly? > canonical_buf is necessary if I my understanding is correct. We can't pass buf directly to strbuf_realpath_forgiving() because it resets its destination buffer before writing. Since format_path() has append semantics, doing so would clobber any existing content in buf. The intermediate canonical_buf is needed to keep that safe. > > +void format_path(struct strbuf *buf, const char *path, > > + const char *prefix, enum path_format format); > > + > > Ok so in this patch we are just adding the new path formatting > interface and will integrate it in the next one. Overall the direction > of this patch looks good to me. Yup, that's correct! > > and update print_path() to act as a light wrapper around the new shared > > engine. Resolve user-provided formatting flags directly within rev-parse > > to pass the final determined path_format to format_path(). > > So if the format isn't explicitly set by the user via the > `--path-format` option, the default formatting strategy used depends on > the path being printed. IOW, there is no consistent default path format > here. > Yes, that's correct. > > + struct strbuf sb = STRBUF_INIT; > > + enum path_format fmt = (arg_path_format != -1) ? arg_path_format : def_format; > > hmmm, so `arg_path_format` specifies what the user-provided format and > acts as a sentinel to signal there is no value provided and the fallback > format needs to be used. This feels a tad bit awkward to me. > > I wonder if we should introduce a PATH_FORMAT_DEFAULT to the > `path_format` enum that maps to one of the existing enum values in > `path.c:format_path()`. Here in `print_path()`, we could then intercept > a PATH_FORMAT_DEFAULT value and override it to the specified > `def_format`. I'm not sure if this is ultimately that much better > though. > > -Justin You're right that the -1 is awkward it forces arg_path_format to be an int rather than the enum type itself, which loses type safety. PATH_FORMAT_DEFAULT is cleaner in that regard, but it pushes the "what does default mean?" question into format_path() which currently has no notion of a fallback. Since the fallback is call-site specific (each path type in rev-parse has its own default), I'd rather keep that logic in print_path() where the context lives. A middle ground would be adding PATH_FORMAT_DEFAULT to the enum but not handling it in format_path(). --- enum path_format_type format = PATH_FORMAT_DEFAULT; /* ... */ static void print_path(const char *path, const char *prefix, enum path_format_type format, enum path_format_type def_format) { struct strbuf sb = STRBUF_INIT; enum path_format_type fmt = (format == PATH_FORMAT_DEFAULT) ? def_format : format; format_path(&sb, path, prefix, fmt); puts(sb.buf); strbuf_release(&sb); } --- > > + format_path(buf, git_dir, startup_info->prefix, PATH_FORMAT_CANONICAL); > > For absolute paths, I don't think we actually need the prefix, but > providing it doesn't probably matter too much either way. > Yeah, true. Since the relative had the prefix I just added the prefix here too. It is consistent. > > > > +test_repo_info_path () { > > + field_name=$1 > > + expect_absolute_eval=$2 > > + expect_relative=$3 > > + env_prefix=$4 > > nit: I was a bit uncertain regarding the purpose of env_prefix here. > Since the env_prefix is not used by any tests yet, I wonder if it we > should delay adding it until the next patch. If we want to reduce churn > though, I think we could also swap the order of patch 3 and 4. > Good point I will actually swap 3 and 4 It is just better tbh. ( > > + cd test-repo/sub && > > + expect_absolute=$(eval "$expect_absolute_eval") && > > Can we just compute `expect_absolute` prior to passing it instead of > using eval here? > Yes, I plan to follow Lucas's suggestion from his review. passing a repo_name parameter and capturing $PWD before the cd to construct the absolute path at helper-call time. That avoids eval entirely and also addresses his other concerns about test isolation. Will fix in v3. > > +test_expect_success 'setup test repository layout for path fields' ' > > + git init test-repo && > > + mkdir -p test-repo/sub > > +' > > + > > +test_repo_info_path 'gitdir' 'echo "$(cd .. && pwd)/.git"' '../.git' > > hmmm, do we expect the path suffix to be the same between relative and > absolute paths for all test cases? If so, we could just have a single > `expect_path_suffix` argument and let the helper compute the appropriate > absolute and relative paths internally. > Yes it is consistent between absolute and relative. This is a good suggestion. Also aligns with what Lucas said. Thank you, This will help building v3 much smoother. Regards, - K Jayatheerth