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

[Junio C Hamano <gitster@pobox.com>]

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

> 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);
	}