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

> > > +     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);
> }
> ---