Re: [GSoC PATCH v2 3/4] repo: add path.gitdir with absolute and relative suffix formatting

[Justin Tobler <jltobler@gmail.com>]

On 26/06/09 10:11AM, K Jayatheerth wrote:
> > > +     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.
> 
> 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);
> }
> ---

Intercepting PATH_FORMAT_DEFAULT in print_path() and overriding it to
the appropriate default needed for the specific path printed by
git-rev-parse(1), as shown above, seems reasonable to me.

But I do think that PATH_FORMAT_DEFAULT should have an actual default in
format_path(). Otherwise we would have an enum value that requires
callers to explicitly handle prior to invoking format_path() which would
also be rather awkward. IMO, it probably wouldn't be a big deal to just
say PATH_FORMAT_DEFAULT is treated as PATH_FORMAT_UNMODIFIED when passed
to format_path() and document it. In practice, our rev-parse use-case
would always replace PATH_FORMAT_DEFAULT with the appropriate value
prior to invoking format_path().

-Justin