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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2019, 20:50 UTC
Message-ID: <xmqqtvi0e7sv.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190122002635.9411-4-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 19 quoted lines

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