Re: [GSoC Patch v5 2/4] rev-parse: use append_formatted_path() for path formatting

From: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
Date: Jun 16, 2026, 17:04 UTC
Message-ID: <CA+rGoLfhhRNrSReeJ1grhy+2K3BSrikTCNgGpCaGqc4fFp3Lfg@mail.gmail.com>
In-Reply-To: <0077b1ae-3c85-4b34-a0ac-766395157c4f@gmail.com>

Hi Phillip, Thanks for taking a look!

Show 15 quoted lines

> On 16/06/2026 05:49, K Jayatheerth wrote:
> > The path-formatting logic in builtin/rev-parse.c is tightly coupled
> > to that command and writes directly to stdout, making it impossible
> > for other builtins to reuse.
> >
> > Extract the core algorithm into append_formatted_path() in path.c
> > and expose a path_format enum in path.h so that any builtin can
> > format paths consistently without duplicating logic.
>
> Sorry I haven't had time to look at this series recently, it is looking
> much nicer now that we have a single enum. It would be helpful to
> explain why we need PATH_FORMAT_DEFAULT that acts exactly like
> PATH_FORMAT_UNMODIFIED. Looking at the next patch it seems this is still
> a wart in the api due to rev-parse wanting needing to distinguish the
> unmodified case from the default case.

t);

Show 5 quoted lines

> > +
> >   # ifdef USE_THE_REPOSITORY_VARIABLE
> >   #  include "strbuf.h"
> >   #  include "repository.h"
>

Show 14 quoted lines

> >   int cmd_rev_parse(int argc,
> > @@ -717,7 +661,7 @@ int cmd_rev_parse(int argc,
> >       const char *name = NULL;
> >       struct strbuf buf = STRBUF_INIT;
> >       int seen_end_of_options = 0;
> > -     enum format_type format = FORMAT_DEFAULT;
> > +     enum path_format arg_path_format = PATH_FORMAT_DEFAULT;
>
> This is the source of the api wart I referred to in the previous patch.
> Could we keep the existing enums and convert them into the appropriate
> PATH_FORMAT_* flag in print_path() above? I think we already have the
> logic to do that in the existing code. That would mean that other users
> of append_formatted_path() don't have to worry about the extra flag.
>

That is a much more elegant solution than the current one.

For v6, I will clean this up by keeping the fallback logic localized within builtin/rev-parse.c and removing PATH_FORMAT_DEFAULT entirely from enum path_format in path.h.

Instead, I'll re-introduce a small local enum (e.g., enum rev_parse_format) inside rev-parse.c to handle the command-line parsing state (tracking whether the user explicitly provided a flag or if we are still in a neutral/default state).

As you said, most of the logic is already present. In print_path(), we will check that local tracking enum. If it’s set to the local default, we can map it directly to the path-specific def_format before invoking append_formatted_path(). This ensures other users of the function don't have to worry about the extra flag.

I will send out the v6 series with these fixes shortly.

Regards,
- K Jayatheerth