On 26/06/09 10:11AM, K Jayatheerth wrote:

quoted

quoted

+     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