Re: [PATCH 4/4] repo: add the field path.toplevel

[Tian Yuchen <a3205153416@gmail.com>]

Hi Lucas,

 > I'm CC'ing here:
 >
 > - brian, who was the original author of the `print_path` [1]
 > - Ayush, Tian, Jayatheerth, Soutrik and Pushkar, since they expressed 
 >   interested in contributing to git-repo-info in GSoC. (I hope that I
 >   didn't forget anyone)

Thank you for your thoughtful consideration!

 > 1. Add --path-format, just like we have in git-rev-parse
 > 2. Use what rev-parse uses by default
 > 3. Add keys for both relative and absolute formats

It makes me wonder if we can use Format Modifiers as in ref-filter.c...

	https://git-scm.com/docs/git-for-each-ref

...which allow us to control the output precisely. For example:

	%(path:relative)
	%(path:absolute)
	%(path:short)
	%(path:strip=2)...

Just a thought.

 > There are two enums used in rev-parse for deciding how paths will
 > be printed by the function `print_path`: `format_type` and
 > `default_type`. Even though there aren't any ambiguities yet, their
 > names aren't clear that those "types" are path types.

This one makes sense to me. Also reduce naming conflicts.

 > +void strbuf_add_path(struct strbuf *sb, const char *path, const char 
 > *prefix, enum path_format_type format, enum path_default_type def)

Isn't it a bit inappropriate for a generic character concatenation 
function to know about format and def? I don't think this should be the 
responsibility of a low-level function, at least not str_buf_add_path().

All functions starting with strbuf_add.. listed by git grep strbuf_add 
are mostly pure string concatenation operations. I believe this should 
be the case here as well.

 > +	if (!prefix && (format != PATH_FORMAT_DEFAULT || def != 
PATH_DEFAULT_RELATIVE_IF_SHARED))
 > +		prefix = cwd = xgetcwd();

I think the logic here shouldn't be tied to 
PATH_DEFAULT_RELATIVE_IF_SHARED, I believe the attribution here is the 
same as above.

 > +		prefix = cwd = xgetcwd()

Will there be a performance regression? Since xgetcwd() here is a system 
call, right? If prefix == NULL and the get repo info command is used to 
locate the top-level path among a large number of submodules, and this 
command will be executed multiple times.

In my opinion, upper-level commands should call xgetcwd() only once 
before entering the loop, then pass the obtained prefix as an argument 
to the underlying implementation.

> -typedef int get_value_fn(struct repository *repo, struct strbuf *buf);
> +typedef int get_value_fn(struct repository *repo, struct strbuf *buf,
> +			 const char *prefix, enum path_format_type format);
>  
>  enum output_format {
>  	FORMAT_TABLE,
> @@ -35,26 +36,46 @@ struct repo_info_field {
>  	get_value_fn *get_value;
>  };
>  
> -static int get_layout_bare(struct repository *repo UNUSED, struct strbuf *buf)
> +static int get_layout_bare(struct repository *repo UNUSED, struct strbuf *buf,
> +			   const char *prefix UNUSED,
> +			   enum path_format_type format UNUSED)
>  {
>  	strbuf_addstr(buf, is_bare_repository() ? "true" : "false");
>  	return 0;
>  }
>  
> -static int get_layout_shallow(struct repository *repo, struct strbuf *buf)
> +static int get_layout_shallow(struct repository *repo, struct strbuf *buf,
> +			      const char *prefix UNUSED,
> +			      enum path_format_type format UNUSED)
>  {
>  	strbuf_addstr(buf,
>  		      is_repository_shallow(repo) ? "true" : "false");
>  	return 0;
>  }
>  
> -static int get_object_format(struct repository *repo, struct strbuf *buf)
> +static int get_object_format(struct repository *repo, struct strbuf *buf,
> +			     const char *prefix UNUSED,
> +			     enum path_format_type format UNUSED)
>  {
>  	strbuf_addstr(buf, repo->hash_algo->name);
>  	return 0;
>  }
>  
> -static int get_references_format(struct repository *repo, struct strbuf *buf)
> +static int get_path_toplevel(struct repository *repo, struct strbuf *buf,
> +			     const char *prefix, enum path_format_type format)
> +{
> +	const char *work_tree = repo_get_work_tree(repo);
> +	if (work_tree)
> +		strbuf_add_path(buf, work_tree, prefix, format,
> +				PATH_DEFAULT_UNMODIFIED);
> +	else
> +		return error(_("this operation must be run in a work tree"));
> +	return 0;
> +}
> +
> +static int get_references_format(struct repository *repo, struct strbuf *buf,
> +				 const char *prefix UNUSED,
> +

I don't think we should add the two new parameters to all get_ functions 
here. As changed in your patch, functions like get_object_format don't 
really, need to know about prefix or format, so the corresponding 
parameters are marked as UNUSED. Imagine if more and more data needs to 
be retrieved by these get_ series functions in the future — is it really 
advisable to add unnecessary parameters to all remaining functions just 
for the sake of a few?

I'm not entirely sure about the above content either; I'm just throwing 
out ideas to spark discussion. (´～`)

Thanks again for starting this discussion!

Regards,

Yuchen