From: Jeff King <peff@peff.net>
Date: Thu, 09 Jul 2015 17:00:54 GMT
Subject: Re: [PATCH] clone: Make use of the strip_suffix() helper method
Message-ID: <20150709170054.GA15820@peff.net>
In-Reply-To: <0000014e73738297-cce3a38b-a85d-40be-b501-354686c25eee-000000@eu-west-1.amazonses.com>

On Thu, Jul 09, 2015 at 03:33:46PM +0000, Sebastian Schuberth wrote:

> @@ -174,19 +175,17 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
>  	 * Strip .{bundle,git}.
>  	 */
>  	if (is_bundle) {
> -		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
> -			end -= 7;
> +		strip_suffix(start, ".bundle", &len);
>  	} else {
> -		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
> -			end -= 4;
> +		strip_suffix(start, ".git", &len);
>  	}

Yay, always glad to see complicated string handling like this go away.
As the resulting conditional blocks are one-liners, I think you can drop
the curly braces, which will match our usual style:

  if (is_bundle)
	strip_suffix(start, ".bundle", &len);
  else
	strip_suffix(start, ".git", &len);

If you wanted to get really fancy, I think you could put a ternary
operator in the middle of the strip_suffix call. That makes it clear
that "len" is set in all code paths, but I think some people find
ternary operators unreadable. :)

>  	if (is_bare) {
>  		struct strbuf result = STRBUF_INIT;
> -		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
> +		strbuf_addf(&result, "%.*s.git", len, start);
>  		dir = strbuf_detach(&result, NULL);

This one can also be simplified using xstrfmt to:

  if (is_bare)
	dir = xstrfmt("%.*s.git", len, start);

Do we still need to cast "len" to an int to use it with "%.*" (it is
defined by the standard as an int, not a size_t)?

-Peff