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

[Phillip Wood <phillip.wood123@gmail.com>]

On 16/06/2026 18:04, K Jayatheerth wrote:
> Hi Phillip,
> Thanks for taking a look!
> 
>> 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);
>>> +
>>>    # ifdef USE_THE_REPOSITORY_VARIABLE
>>>    #  include "strbuf.h"
>>>    #  include "repository.h"
>>
> 
> 
>>>    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).

I think it is probably simplest to keep the existing enums and modify 
print_path() to convert them to the appropriate PATH_FORMAT_*. That way 
we can keep the option parsing code as is.

Thanks

Phillip

> 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