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

From

Ramkumar Ramachandra <artagnon@gmail.com>

Date

May 7, 2011, 13:17 UTC

Message-ID

<20110507131745.GA29061@ramkum.desktop.amazon.com>

In-Reply-To

<7vr58b22ny.fsf@alter.siamese.dyndns.org>

Hi Junio,

Junio C Hamano writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > Libify the "rerere clear" into a simple function called rerere_clear
> > that takes no arguments, and returns the exit status.  Also export
> > unlink_rr_item as unlink_rerere_item so rerere_clear and the
> > un-libified "git rerere gc" can both use it.
> >
> > Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> > ---
> >  What changed since v2: Jonathan's review.
> 
> Are you sure this is the version you wanted to send?
> 
> You now return -1 from rerere_clear() when setup_rerere() says that the
> feature is not enabled, and this is propagated back to cmd_rerere(),
> causing the whole command to report a failure in its exit status, which
> seems to me a grave regression.  Your previous round got this part right,
> but it is broken in this round.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

diff --git a/rerere.c b/rerere.c
index aaca3b0..fda02f6 100644
--- a/rerere.c
+++ b/rerere.c
@@ -687,7 +687,7 @@ int rerere_clear(void)
 
 	fd = setup_rerere(&merge_rr, 0);
 	if (fd < 0)
-		return -1;
+		return 0;
 	for (i = 0; i < merge_rr.nr; i++) {
 		const char *name = (const char *)merge_rr.items[i].util;
 		if (!has_rerere_resolution(name))

> Also I seem to recall that Jonathan suggested that you do not have to
> expose unlink_rr_item() as an external symbol if you moved the garbage
> collection part from builtin/rerere.c to rerere.c but I do not see such a
> change in this patch.  I think the gc interface is a lot more reasonable
> API to expose to external callers ("git gc" may want to make an internal
> call to rerere_gc() moved to rerere.c, instead of spawning "git rerere gc"
> as an external command) than unlink_rerere_item() that is only useful for
> callers that are deep inside rerere specific codepath.

I'll quote Jonathan from the previous review:

"
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.
"

To the first part of the question: yes, that's the reason for exposing
unlink_rr_item as unlink_rerere_item.  Yet, I followed the latter
approach for the appeal of the minimal patch -- I should have said
this explicitly.  Anyway, I plan to post another patch cleaning up and
libifying rerere shortly.

Junio: If you feel that garbage_collect should be exposed in this
patch, I'll post an alternative version now, and you can pick the one
you like :)

-- Ram