Re: [GSoC PATCH v2 2/4] rev-parse: use format_path for path formatting

[Justin Tobler <jltobler@gmail.com>]

On 26/06/05 10:00PM, K Jayatheerth wrote:
> Now that the core path-formatting logic has been abstracted into
> format_path() inside path.c, remove the localized duplicate formatting
> mechanics from builtin/rev-parse.c.
> 
> Drop the usage of the old local format_type and default_type enums,
> 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.

> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
>  builtin/rev-parse.c | 103 ++++++++++----------------------------------
>  1 file changed, 23 insertions(+), 80 deletions(-)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 218b5f34d6..c78bdc04c1 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -632,73 +632,16 @@ static void handle_ref_opt(const char *pattern, const char *prefix)
>  	clear_ref_exclusions(&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,
> -};
> -
> -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,
> +		       int arg_path_format, enum path_format def_format)
>  {
[snip]
> +	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