Patrick Steinhardt <ps@xxxxxx> writes: >> No changes in behaviour. Just a trivial interface change. >> >> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> >> ... >> +#define GOPID_STABLE 01 >> +#define GOPID_VERBATIM 02 >> + > > This certainly is a worthwhile change. I have to wonder about code style > though: > > - Using 01 and 02 as constants feels somewhat weird to me. Don't we > typically use `(1 << 0)` and `(1 << 1)` for such binary flags? > > - What is our preferred style nowadays? Do we prefer defines over > enums? I rather had the feeling that enums are the go-to style for > things like this nowadays. > > It would also be nice to have documentation for the flags. For an internal implementation detail that does not even cross file boundaries with descriptive _STABLE and _VERBATIM that corresponds to the member names of config structure? I doubt it. > In any case, all of these are really just smallish nits and I think that > this is a strict improvement regardless of whether we massage the style > or not. > ... > I was wondering whether we could use `OPT_BIT()` here to set those as > flags directly. I guess that would require a bit more refactoring, but > if we also converted `struct patch_id_opts` to have a `flags` field then > this might overall be easier to read than the weird massaging of opts > that we did before and after your change. As a longer direction, I envision that most of the implementation we see in this file and what diff.c:diff_get_patch_id() does should be refactored and one of them should just go. So until that happens, I am inclined to keep the changes to this file to an absolute minimum.
