Pranit Bauva [off-list ref] 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.
On Fri, May 13, 2016 at 4:06 AM, Junio C Hamano [off-list ref] wrote:
Pranit Bauva [off-list ref] writes:
quoted
+ /*
+ * 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
quoted
- 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.
Not just that, it would also be fundamentally more correct as there is
a difference between " !strcmp("good") " and " strcmp("bad") ". Yours
approach suggests that it should be "good" specifically which is
technically more correct as then only it would be sensible to check
whether it is trying to change the meaning of the term.
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.
Will do this change in a separate patch after the dust settles.
Regards,
Pranit Bauva