From: René Scharfe <l.s.r@web.de>
Date: Thu, 06 Apr 2017 23:07:38 GMT
Subject: Re: [PATCH v6 1/3] read-cache: add strcmp_offset function
Message-ID: <97be34a6-ecdb-9820-1315-4cb5ed3b9101@web.de>
In-Reply-To: <20170406163442.36463-2-git@jeffhostetler.com>

Am 06.04.2017 um 18:34 schrieb git@jeffhostetler.com:
> diff --git a/read-cache.c b/read-cache.c
> index 9054369..e8f1900 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -887,6 +887,26 @@ static int has_file_name(struct index_state *istate,
>   	return retval;
>   }
>   
> +
> +/*
> + * Like strcmp(), but also return the offset of the first change.
> + * If strings are equal, return the length.
> + */
> +int strcmp_offset(const char *s1, const char *s2, int *first_change)
> +{
> +	int k;
> +
> +	if (!first_change)
> +		return strcmp(s1, s2);
> +
> +	for (k = 0; s1[k] == s2[k]; k++)
> +		if (s1[k] == '\0')
> +			break;
> +
> +	*first_change = k;
> +	return ((unsigned char *)s1)[k] - ((unsigned char *)s2)[k];
> +}

I like how this is shaping up to become a reusable function, but I only 
found one other possible use case (in read-cache.c::ce_write_entry), 
which somehow irritates me.  Either I didn't look hard enough (likely), 
we haven't got fitting code just yet, or this function isn't meant to be 
exported -- in the latter case its interface doesn't have to be polished 
as much.  (Just a thought, don't let me stop you.)

Assuming strcmp_offset() is a library-grade function then its 
first_change parameter should point to a size_t instead of an int, no?

Casting away the const qualifier in the return line is a bit iffy.  Why 
not cast after dereferencing, like this?

	return (unsigned char)s1[k] - (unsigned char)s2[k];

René