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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2022, 01:21 UTC
Message-ID: <xmqqh7akgutd.fsf@gitster.g>
In-Reply-To: <cb881183cb7179e3e7e983b7c9ffe115f845115c.1641043500.git.dyroneteng@gmail.com>

Teng Long <dyroneteng@gmail.com> writes:

Show 5 quoted lines

> 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.