Re: [PATCH] Remove redundant double exclamation points

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 15:19 UTC
Message-ID: <221219.868rj3za6s.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1401.git.git.1671459163559.gitgitgadget@gmail.com>

On Mon, Dec 19 2022, Rose via GitGitGadget wrote:

> From: Seija Kijin <doremylover123@gmail.com>
>
> S_ISDIR is a macro that involves a "==" comparison.

It does? The POSIX standard (https://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/stat.h.html) says:

	The following macros shall be provided to test whether a file is
	of the specified type. The value m supplied to the macros is the
	value of st_mode from a stat structure. The macro shall evaluate
	to a non-zero value if the test is true; 0 if the test is false.

The "non-zero" there seems to intentionally leave open that this may be defined e.g. as via a "&" test, as opposed to "==" which according to C99's 6.5.9.3 says:

	The == (equal to) and != (not equal to) operators are analogous
	to the relational operators except for their lower
	precedence.90) Each of the operators yields 1 if the specified
	relation is true and 0 if it is false. The result has type
	int. For any pair of operands, exactly one of the relations is
	true.

> This means the !! is redundant and not needed.

I think you're therefore introducing a bug here, this may work on your platform, but we have no guarantee that it'll work elsewhere.

I thought that it probably wouldn't matter, as we'd treat the argument as a boolean, but we don't. In within_depth() we proceed to use the passed-in 3rd argument (depth) as a counter, so it really does matter if it's 0, 1, or other non-zero.

Show 20 quoted lines

>  tree-walk.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tree-walk.c b/tree-walk.c
> index 74f4d710e8f..6b51d27ccb2 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -1040,9 +1040,9 @@ static enum interesting do_match(struct index_state *istate,
>  		    ps->max_depth == -1)
>  			return all_entries_interesting;
>  		return within_depth(base->buf + base_offset, baselen,
> -				    !!S_ISDIR(entry->mode),
> -				    ps->max_depth) ?
> -			entry_interesting : entry_not_interesting;
> +				    S_ISDIR(entry->mode), ps->max_depth) ?
> +			       entry_interesting :
> +			       entry_not_interesting;
>  	}
>  
>  	pathlen = tree_entry_len(entry);

Aside from whether or not this is a bug, could you please submit proposed refactorings of the git project via coccinelle patches if possible (as I suggested to you before).

I realize that it has a slight learning curve, but it makes writing & maintaining these so much easier, and it'll fix (mis)uses going forward, not just as a one-off.

So, as an example (and assuming this wasn't buggy), you'd do that in
this case as e.g. (untested, but you can see similar syntax in our
existing *.cocci files):
	
	@@
	@@
	- !!
	(
	S_ISDIR
	|
	S_ISFIFO
        // |
        // we'd continue to list the rest here...
	)
	  (...)