Re: [PATCH 7/8] pretty: refactor parsing of magic

From: Martin Ågren <martin.agren@gmail.com>
Date: Mar 20, 2025, 16:12 UTC
Message-ID: <CAN0heSosT5gVHZ3t7APJ0rGXD_agU8NQBE=t0KNK+C0huY5niw@mail.gmail.com>
In-Reply-To: <Z9vdVP4edeaRawsz@pks.im>

On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:

Show 21 quoted lines

>
> On Wed, Mar 19, 2025 at 08:23:40AM +0100, Martin Ågren wrote:
> > +enum magic {
> > +     NO_MAGIC,
> > +     ADD_LF_BEFORE_NON_EMPTY,
> > +     DEL_LF_BEFORE_EMPTY,
> > +     ADD_SP_BEFORE_NON_EMPTY
> > +};
> > +
>
> It would be nice to give all of these enums a common prefix, e.g.:
>
>     enum magic {
>             MAGIC_NONE,
>             MAGIC_ADD_LF_BEFORE_NON_EMPTY,
>             MAGIC_DEL_LF_BEFORE_EMPTY,
>             MAGIC_ADD_SP_BEFORE_NON_EMPTY
>     };
>
> Makes it easier to see that things belong together and it provides
> proper namespacing.

Agreed, good point.

> On the other hand you simply retain existing names. I don't insist on
> the refactoring, but still thing it would be nice as the enum has wider
> scope now.

Right. It's only file-scoped, but that's still a bigger scope... I'll rename them to give them all a common prefix as suggested.

Show 12 quoted lines

> It took me a bit to figure out why this is equivalent to what we had
> before. But:
>
>   - If `parse_magic()` returns bigger than 1 we'd have exited early, so
>     this return here is never hit.
>
>   - If it returns `0` we have hit `NO_MAGIC`, and we have another early
>     return for this case.
>
> So we only end up here in case `consumed = parse_magic(...)` is 1, and
> then we add the result from `format_and_pad_commit()` to that value.
> Which means that the refactoring is true to the original spirit.

If there's anything in particular you think should be called out in the commit message to assist future readers, just let me know. I'll take your points above as inspiration for things to highlight better.

There's also the return value "2", which is a bit, well, magic. Or at least fairly arbitrary. I kind of preferred it over switching to a signed type in this one spot though.

Thanks for all your very helpful comments.

Martin