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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Ramkumar Ramachandra wrote:

> Expose a new function called rerere_clear,

The commit message is a good chance to quickly explain the API.  What
does the function do?  When would one call it?  Is it equivalent to
running "git rerere clear"?  Any gotchas?

> and make 'builtin/rerere.c'
> use this when the corresponding command-line argument is passed.

Didn't "git rerere" already use the functionality of this function?
I'm not sure what this part means.

> As a
> side-effect, also expose unlink_rr_item as unlink_rerere_item.

This is not a side-effect; you did it directly.

I think the reason for this is that rerere_gc is not being exposed at
the same time, right?  I suppose if I were doing it, I would have
moved that to rerere.c, too and kept unlink_rr_item static, but there
is also appeal in a minimal patch.  It would be clearer to say
something to the effect that we

	Also export unlink_rr_item as unlink_rerere_item so
	rerere_clear and the un-libified "git rerere gc" can
	both use it.

[...]
> +++ b/builtin/rerere.c
> @@ -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);
[...]
> +++ b/rerere.c
> @@ -671,3 +679,22 @@ int rerere_clear(void)
[...]
> +	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
> +	if (fd < 0)
> +		return 0;

Why RERERE_NOAUTOUPDATE instead of 0?  (Of course it doesn't matter in
practice.   Maybe "0" would convey that more clearly?)

> +
> +	for (i = 0; i < merge_rr.nr; i++) {
> +		const char *name = (const char *)merge_rr.items[i].util;
> +		if (!has_rerere_resolution(name))
> +			unlink_rerere_item(name);
> +	}
> +	string_list_clear(&merge_rr, 1);
> +	unlink_or_warn(git_path("MERGE_RR"));
> +	return 0;
> +}

The write_lock is never rolled back.  "git rerere" won't care since it
exits moments later and the atexit handler is called, but others might
mind that they can't perform any more rerere operations afterwards. :)

Thanks and hope that helps.
Jonathan