Re: [PATCH] rerere: Expose an API corresponding to 'clear' functionality

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2011, 18:36 UTC
Message-ID: <7vmxjwws8a.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1302511916-23320-1-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Expose a new function called rerere_clear, and rework
> 'builtin/rerere.c' to use this when the corresponding command-line
> argument is passed.

Show 19 quoted lines

> @@ -142,19 +134,14 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
>  		pathspec = get_pathspec(prefix, argv + 1);
>  		return rerere_forget(pathspec);
>  	}
> +	if (!strcmp(argv[0], "clear"))
> +		return rerere_clear();
>  
>  	fd = setup_rerere(&merge_rr, flags);
>  	if (fd < 0)
>  		return 0;
>  
> -	if (!strcmp(argv[0], "clear")) {
> -		for (i = 0; i < merge_rr.nr; i++) {
> -			const char *name = (const char *)merge_rr.items[i].util;
> -			if (!has_rerere_resolution(name))
> -				unlink_rr_item(name);
> -		}
> -		unlink_or_warn(git_path("MERGE_RR"));
> -	} else if (!strcmp(argv[0], "gc"))

In your version, the call of rerere_clear() is not protected by a check that used to return early in a repository that is configured not to use rerere. I don't see there is a corresponding early return in the new function rerere_clear() you created in rerere.c, either.

Would that cause a user-visible regression?

Show 9 quoted lines

> diff --git a/rerere.h b/rerere.h
> index 595f49f..b9ab839 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -16,8 +16,10 @@ extern void *RERERE_RESOLVED;
>  extern int setup_rerere(struct string_list *, int);
>  extern int rerere(int);
>  extern const char *rerere_path(const char *hex, const char *file);
> +extern void unlink_rr_item(const char *name);

The name was perfectly fine while it was a file-scope static, but is unacceptable as a public function. At least please spell "rerere" out somewhere to ensure that even first-time readers can tell what area of the system the function is about.