[3/3] diff.c: simplify diff_opt_break_rewrites()

Commit Message

Duy Nguyen Jan. 22, 2019, 12:26 a.m. UTC

Helped-by: Johannes Schindelin <[e-mail]>
---
 diff.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Junio C Hamano Jan. 22, 2019, 8:50 p.m. UTC | #1

Nguyễn Thái Ngọc Duy  <[e-mail]> writes:

> -	int opt1, opt2;
> +	int opt1, opt2 = 0;
>  
>  	BUG_ON_OPT_NEG(unset);
>  	if (!arg)
>  		arg = "";
>  	opt1 = parse_rename_score(&arg);
> -	switch (*arg) {
> -	case '\0':
> -		opt2 = 0;
> -		break;
> -	case '/':
> +	if (*arg == '/') {
>  		arg++;
>  		opt2 = parse_rename_score(&arg);
> -		break;
>  	}
>  	if (*arg != 0)
>  		return error(_("%s expects <n>/<m> form"), opt->long_name);

Good.  I was about to complain on the lack of "default" that catches
the error at the end of "<n>" in the switch(), but because this
follow-up validation is trying to catch "<n>" form (i.e. single
score without slash) whose "<n>" is malformed, and "<n>/<m>" form
whose "<m>" is malformed, which is kind of clever, but it is not
very easy to understand what is going on, it makes sense to get rid
of the switch() and make it if() statement.

It would make it even easier to follow if you did

	if (*arg == '/') {
		opt2 = ...;
		arg++;
	} else {
		opt2 = 0;
	}
	if (*arg)
		return error(...);

It makes it clear that opt2==0 means <n> form and not <n>/<m> form,
by having an explicit assignment while we parse what *arg points at,
without the initialization to 0 in the variable definition.

But this should be squashed in the original patch.  Having an "oops,
it was horribly unreadble---how about doing something like this?"
incremental is good during a discussion, and having a "what we have
seen is basically good and proven solid, but here is a small
improvement" incremental is good for code that is stable enough to
build on (read: at least in 'next'), but it is not particularly good
for a topic not yet in 'next' yet.

Junio C Hamano Jan. 22, 2019, 11:18 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> It would make it even easier to follow if you did
>
> 	if (*arg == '/') {
> 		opt2 = ...;
> 		arg++;
> 	} else {

Oops, this should read "else if (!*arg) {", of course, to match the
original.  Sorry for the noise.

> 		opt2 = 0;
> 	}

And then we'd want a blank line here to make it clear that the
previous if/else cascade has finished, and the error checking we see
next is not part of it.

> 	if (*arg)
> 		return error(...);
>
> It makes it clear that opt2==0 means <n> form and not <n>/<m> form,
> by having an explicit assignment while we parse what *arg points at,
> without the initialization to 0 in the variable definition.

Patch

diff --git a/diff.c b/diff.c
index da5ba835ce..2351571251 100644
--- a/diff.c
+++ b/diff.c
@@ -4814,20 +4814,15 @@  static int diff_opt_break_rewrites(const struct option *opt,
 				   const char *arg, int unset)
 {
 	int *break_opt = opt->value;
-	int opt1, opt2;
+	int opt1, opt2 = 0;
 
 	BUG_ON_OPT_NEG(unset);
 	if (!arg)
 		arg = "";
 	opt1 = parse_rename_score(&arg);
-	switch (*arg) {
-	case '\0':
-		opt2 = 0;
-		break;
-	case '/':
+	if (*arg == '/') {
 		arg++;
 		opt2 = parse_rename_score(&arg);
-		break;
 	}
 	if (*arg != 0)
 		return error(_("%s expects <n>/<m> form"), opt->long_name);