Re: [PATCH v4 05/10] ref-filter: add support to sort by version

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 25, 2015, 22:40 UTC
Message-ID: <xmqqd1zfkgi3.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1437764685-8633-6-git-send-email-Karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 27 quoted lines

> @@ -1180,19 +1181,17 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>  
>  	get_ref_atom_value(&state, a, s->atom, &va);
>  	get_ref_atom_value(&state, b, s->atom, &vb);
> -	switch (cmp_type) {
> -	case FIELD_STR:
> +	if (s->version)
> +		cmp = versioncmp(va->s, vb->s);
> +	else if (cmp_type == FIELD_STR)
>  		cmp = strcmp(va->s, vb->s);
> -		break;
> -	default:
> -		if (va->ul < vb->ul)
> -			cmp = -1;
> -		else if (va->ul == vb->ul)
> -			cmp = 0;
> -		else
> -			cmp = 1;
> -		break;
> -	}
> +	else if (va->ul < vb->ul)
> +		cmp = -1;
> +	else if (va->ul == vb->ul)
> +		cmp = 0;
> +	else
> +		cmp = 1;
> +

So there are generally three kinds of comparison possible:

    - if it is to be compared as versions, do versioncmp
    - if it is to be compared as strings, do strcmp
    - if it is to be compared as numbers, do <=> but because
      we are writing in C, not in Perl, do so as if/else/else

Having understood that, the above is not really easy to read and extend. We should structure the above more like this:

	if (s->version)
        	... versioncmp
	else if (... FIELD_STR)
        	... strcmp
	else {
		if (a < b)
                	...
		else if (a == b)
                	...
		else
                	...
        }

so that it would be obvious how this code need to be updated when we need to add yet another kind of comparison.

Without looking at the callers, s->version looks like a misdesign that should be updated to use the same cmp_type mechanism? That would lead to even more obvious construct that is easy to enhance, i.e.

	switch (cmp_type) {
        case CMP_VERSION:
        	...
	case CMP_STRING:
        	...
	case CMP_NUMBER:
        	...
        }
        
I dunno.

Other than that (and the structure of that "format-state" stuff we discussed separately), the series was a pleasant read.

Thanks.