public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Johannes Schindelin <Johannes.Schindelin@gmx•de>
Cc: Christian Couder <chriscool@tuxfamily•org>,
	git@vger•kernel.org, Avery Pennarun <apenwarr@gmail•com>
Subject: Re: [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead
Date: Mon, 04 Nov 2013 14:37:13 -0800	[thread overview]
Message-ID: <xmqqtxfraj52.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <alpine.DEB.1.00.1311042316520.1191@s15462909.onlinehome-server.info> (Johannes Schindelin's message of "Mon, 4 Nov 2013 23:17:27 +0100 (CET)")

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

     prev parent reply	other threads:[~2013-11-04 22:37 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-03 20:13 [PATCH] builtin/remote: remove postfixcmp() and use suffixcmp() instead Christian Couder
2013-11-04 17:56 ` Jonathan Nieder
2013-11-04 19:19 ` Junio C Hamano
2013-11-04 20:16   ` Christian Couder
2013-11-04 20:21   ` Junio C Hamano
2013-11-04 22:17     ` Johannes Schindelin
2013-11-04 22:37       ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqtxfraj52.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=Johannes.Schindelin@gmx$(echo .)de \
    --cc=apenwarr@gmail$(echo .)com \
    --cc=chriscool@tuxfamily$(echo .)org \
    --cc=git@vger$(echo .)kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox