Re: [PATCH v6 2/3] bisect: rewrite `check_term_format` shell function in C

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Pranit Bauva <pranit.bauva@xxxxxxxxx> writes:

> +	/*
> +	 * In theory, nothing prevents swapping completely good and bad,
> +	 * but this situation could be confusing and hasn't been tested
> +	 * enough. Forbid it for now.
> +	 */
> +
> +	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
> +		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
> +		return error(_("can't change the meaning of the term '%s'"), term);

The above comes from below

> -	bad|new)
> -		if test "$2" != bad
> -		then
> -			die "$(eval_gettext "can't change the meaning ...

So it is not your fault, but it is quite hard to understand.

The original says "You are trying to use 'bad' (or 'new') for
something that is not 'bad'--which is confusing; do not do it".

I _think_ the reason I find C version harder to read is the use of
strcmp(orig_term, "bad") to say "orig_term is not BAD".  The shell
version visually tells with "!=" that the meaning of the new term is
*NOT* "bad", and does not ahve such a problem.

Perhaps if we rewrote it to

	if ((!strcmp(orig_term, "good") &&
             one_of(term, "bad", "new", NULL)) ||
             (!strcmp(orig_term, "bad") &&
             one_of(term, "good", "old", NULL)))

i.e. "If you are using "bad" or "new" to mean "good", that is not a
good idea", as a follow-up change after the dust settles, the result
might be easier to read.

But this is just commenting for future reference, not suggesting to
update this patch at all.  If we were to do the change, that must
come as a separate step.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]