Subject: Re: [PATCH v8 6/8] ls-tree.c: support --object-only option for "git-ls-tree"

From: Junio C Hamano <[email-masked]>
Date: Mon, 03 Jan 2022 17:21:02 -0800

Teng Long <[email-masked]> writes:

> A simple refactoring was done to the "show_tree" function, intead by
> using bitwise operations to recognize the format for printing to
> stdout. The reason for doing this is that we don't want to increase
> the readability difficulty with the addition of "-object-only",
> making this part of the logic easier to read and expand.

The resulting code looks unnecessarily complex and brittle; some
SHOW_FOO mean SHOW_FOO_ONLY_AND_NOTHING_ELSE while other SHOW_BAR
means SHOW_BAR_BUT_WE_MAY_SHOW_OTHER_THINGS_IN_LATER_PART, and the
distinction is not clear from their names (which means it is hard
to later extend and enhance the behaviour of the code).

> +	if (!(shown_bits ^ SHOW_FILE_NAME)) {

Is the use of XOR operator significant here?

I.e. "if (shown_bits & SHOW_FILE_NAME)" would have been a much more
natural way to guard "this is a block that shows the file name",
than "the result MUST BE all bits off if we flip SHOW_FILE_NAME bit
off".  If various SHOW_FOO bits are meant to be mutually exclusive,
then "if ((shown_bits & SHOW_FILE_NAME) == SHOW_FILE_NAME)" would
also make sense, but as I said upfront, it is unclear to me if
shown_bits are meant to be a collection of "this bit means this
field is shown (and it implies nothing else)", so I dunno.