Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 4, 2013, 22:37 UTC
Message-ID: <xmqqtxfraj52.fsf@gitster.dls.corp.google.com>
In-Reply-To: <alpine.DEB.1.00.1311042316520.1191@s15462909.onlinehome-server.info>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 34 quoted lines

> Hi Junio,
>
> On Mon, 4 Nov 2013, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > I do not think anybody sane uses prefixcmp() or suffixcmp() for
>> > anything but checking with zero; in other words, I suspect that all
>> > uses of Xcmp() can be replaced with !!Xcmp(), so as a separate
>> > clean-up patch, we may at least want to make it clear that the
>> > callers should not expect anything but "does str have sfx as its
>> > suffix, yes or no?" by doing something like this:
>> >
>> >  int suffixcmp(const char *str, const char *suffix)
>> >  {
>> >  	int len = strlen(str), suflen = strlen(suffix);
>> >  	if (len < suflen)
>> >  		return -1;
>> >  	else
>> > -		return strcmp(str + len - suflen, suffix);
>> > +		return !!strcmp(str + len - suflen, suffix);
>> >  }
>> >
>> > I am not absolutely sure about doing the same to prefixcmp(),
>> > though. It could be used for ordering, even though no existing code
>> > seems to do so.
>> 
>> I just realized why this suggestion is incomplete; if we were to go
>> this route, we should rename the function to has_suffix() or
>> something. anything-cmp() ought to be usable as an ordering
>> comparison function, but suffixcmp() clearly isn't.
>
> I have to admit that I do not understand why a change in suffixcmp()'s
> behavior is needed.

I made the suggestion only because I do not understand why the function should order "f" and ".txt" in the

	"f" < ".txt"

order. Even worse, the other function postfixcmp() orders them the other way around.

If -1 returned from the function were an indication of error "The string does not even have that suffix", then I would have been a bit more sympathetic, and its current behaviour in that case could be argued as a special case of the broader return value "non-zero (from the ordinary strcmp() return codeflow) means the string does not have that suffix and zero means the string ends with the suffix".

But then, a function that pretends to be for ordering comparison, with a name that ends with cmp(), and then declaring that "no, this is not for ordering; the sign of the result does not matter--what only matters is if it returns zero or non-zero", feels quite schizophrenic, at least to me.

And my earlier suggestion to change the return value *is* not a right change. It still keeps the pretense of comparison for ordering (i.e. ...cmp() name), while returning a value that cannot possibly be used for ordering.

So I think the right patch should make the function like this:

	int has_suffix(const char *str, const char *suffix)
        {
        	int len = strlen(str);
                int suffix_len = strlen(suffix);
                if (len < suffix_len)
                	return 0;
		return !strcmp(str + len - suffix_len, suffix);
	}