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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 13, 2011, 20:38 UTC
Message-ID: <20110413203843.GC30630@elie>
In-Reply-To: <1302700718-19093-1-git-send-email-artagnon@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.

[...]

Show 9 quoted lines

> +++ 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?)

Show 10 quoted lines

> +
> +	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