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

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

[Ramkumar Ramachandra]

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

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Ref: http://thread.gmane.org/gmane.comp.version-control.git/171255/focus=171273

 builtin/rerere.c |   19 +++----------------
 rerere.c         |   24 ++++++++++++++++++++++++
 rerere.h         |    2 ++
 3 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 8235885..f3956bf 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -28,14 +28,6 @@ static time_t rerere_last_used_at(const char *name)
 	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
-static void unlink_rr_item(const char *name)
-{
-	unlink(rerere_path(name, "thisimage"));
-	unlink(rerere_path(name, "preimage"));
-	unlink(rerere_path(name, "postimage"));
-	rmdir(git_path("rr-cache/%s", name));
-}
-
 static int git_rerere_gc_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "gc.rerereresolved"))
@@ -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"))
+	if (!strcmp(argv[0], "gc"))
 		garbage_collect(&merge_rr);
 	else if (!strcmp(argv[0], "status"))
 		for (i = 0; i < merge_rr.nr; i++)
diff --git a/rerere.c b/rerere.c
index 22dfc84..f8da9bd 100644
--- a/rerere.c
+++ b/rerere.c
@@ -25,6 +25,14 @@ const char *rerere_path(const char *hex, const char *file)
 	return git_path("rr-cache/%s/%s", hex, file);
 }
 
+void unlink_rr_item(const char *name)
+{
+	unlink(rerere_path(name, "thisimage"));
+	unlink(rerere_path(name, "preimage"));
+	unlink(rerere_path(name, "postimage"));
+	rmdir(git_path("rr-cache/%s", name));
+}
+
 int has_rerere_resolution(const char *hex)
 {
 	struct stat st;
@@ -671,3 +679,19 @@ int rerere_forget(const char **pathspec)
 	}
 	return write_rr(&merge_rr, fd);
 }
+
+int rerere_clear(void)
+{
+	int i, fd;
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
+	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);
+	}
+	string_list_clear(&merge_rr, 1);
+	unlink_or_warn(git_path("MERGE_RR"));
+	return 0;
+}
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);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
+extern int rerere_clear(void);
 extern int rerere_remaining(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
-- 
1.7.4.rc1.7.g2cf08.dirty

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

[Junio C Hamano]

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.

> @@ -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?

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

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

[Ramkumar Ramachandra]

Expose a new function called rerere_clear, and make 'builtin/rerere.c'
use this when the corresponding command-line argument is passed.  As a
side-effect, also expose unlink_rr_item as unlink_rerere_item.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Junio: Thanks for the review.  Is this version alright?

 builtin/rerere.c |   21 ++++-----------------
 rerere.c         |   27 +++++++++++++++++++++++++++
 rerere.h         |    2 ++
 3 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 8235885..8f939b3 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -28,14 +28,6 @@ static time_t rerere_last_used_at(const char *name)
 	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
-static void unlink_rr_item(const char *name)
-{
-	unlink(rerere_path(name, "thisimage"));
-	unlink(rerere_path(name, "preimage"));
-	unlink(rerere_path(name, "postimage"));
-	rmdir(git_path("rr-cache/%s", name));
-}
-
 static int git_rerere_gc_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "gc.rerereresolved"))
@@ -76,7 +68,7 @@ static void garbage_collect(struct string_list *rr)
 			string_list_append(&to_remove, e->d_name);
 	}
 	for (i = 0; i < to_remove.nr; i++)
-		unlink_rr_item(to_remove.items[i].string);
+		unlink_rerere_item(to_remove.items[i].string);
 	string_list_clear(&to_remove, 0);
 }
 
@@ -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"))
+	if (!strcmp(argv[0], "gc"))
 		garbage_collect(&merge_rr);
 	else if (!strcmp(argv[0], "status"))
 		for (i = 0; i < merge_rr.nr; i++)
diff --git a/rerere.c b/rerere.c
index 22dfc84..6821f33 100644
--- a/rerere.c
+++ b/rerere.c
@@ -25,6 +25,14 @@ const char *rerere_path(const char *hex, const char *file)
 	return git_path("rr-cache/%s/%s", hex, file);
 }
 
+void unlink_rerere_item(const char *name)
+{
+	unlink(rerere_path(name, "thisimage"));
+	unlink(rerere_path(name, "preimage"));
+	unlink(rerere_path(name, "postimage"));
+	rmdir(git_path("rr-cache/%s", name));
+}
+
 int has_rerere_resolution(const char *hex)
 {
 	struct stat st;
@@ -671,3 +679,22 @@ int rerere_forget(const char **pathspec)
 	}
 	return write_rr(&merge_rr, fd);
 }
+
+int rerere_clear(void)
+{
+	int i, fd;
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
+	if (fd < 0)
+		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))
+			unlink_rerere_item(name);
+	}
+	string_list_clear(&merge_rr, 1);
+	unlink_or_warn(git_path("MERGE_RR"));
+	return 0;
+}
diff --git a/rerere.h b/rerere.h
index 595f49f..cd2cdb2 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_rerere_item(const char *name);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
+extern int rerere_clear(void);
 extern int rerere_remaining(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
-- 
1.7.4.rc1.7.g2cf08.dirty

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

[Jonathan Nieder]

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

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

[Ramkumar Ramachandra]

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.

 builtin/rerere.c |   21 ++++-----------------
 rerere.c         |   27 +++++++++++++++++++++++++++
 rerere.h         |    2 ++
 3 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 8235885..8f939b3 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -28,14 +28,6 @@ static time_t rerere_last_used_at(const char *name)
 	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
-static void unlink_rr_item(const char *name)
-{
-	unlink(rerere_path(name, "thisimage"));
-	unlink(rerere_path(name, "preimage"));
-	unlink(rerere_path(name, "postimage"));
-	rmdir(git_path("rr-cache/%s", name));
-}
-
 static int git_rerere_gc_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "gc.rerereresolved"))
@@ -76,7 +68,7 @@ static void garbage_collect(struct string_list *rr)
 			string_list_append(&to_remove, e->d_name);
 	}
 	for (i = 0; i < to_remove.nr; i++)
-		unlink_rr_item(to_remove.items[i].string);
+		unlink_rerere_item(to_remove.items[i].string);
 	string_list_clear(&to_remove, 0);
 }
 
@@ -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"))
+	if (!strcmp(argv[0], "gc"))
 		garbage_collect(&merge_rr);
 	else if (!strcmp(argv[0], "status"))
 		for (i = 0; i < merge_rr.nr; i++)
diff --git a/rerere.c b/rerere.c
index 22dfc84..aaca3b0 100644
--- a/rerere.c
+++ b/rerere.c
@@ -25,6 +25,14 @@ const char *rerere_path(const char *hex, const char *file)
 	return git_path("rr-cache/%s/%s", hex, file);
 }
 
+void unlink_rerere_item(const char *name)
+{
+	unlink(rerere_path(name, "thisimage"));
+	unlink(rerere_path(name, "preimage"));
+	unlink(rerere_path(name, "postimage"));
+	rmdir(git_path("rr-cache/%s", name));
+}
+
 int has_rerere_resolution(const char *hex)
 {
 	struct stat st;
@@ -671,3 +679,22 @@ int rerere_forget(const char **pathspec)
 	}
 	return write_rr(&merge_rr, fd);
 }
+
+int rerere_clear(void)
+{
+	int i, fd;
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+	fd = setup_rerere(&merge_rr, 0);
+	if (fd < 0)
+		return -1;
+	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"));
+	rollback_lock_file(&write_lock);
+	return 0;
+}
diff --git a/rerere.h b/rerere.h
index 595f49f..cd2cdb2 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_rerere_item(const char *name);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
+extern int rerere_clear(void);
 extern int rerere_remaining(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
-- 
1.7.5.GIT

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

[Junio C Hamano]

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.

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.

What is going on?

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

[Ramkumar Ramachandra]

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.

Ugh, I'm not sure how this change crept in- sorry :|
Could you please squash this diff into the patch?

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

[PATCH v4 0/2] Libify rerere: clear and gc

[Ramkumar Ramachandra]

Hi,

This is an alternate version of the v3 patch: Instead of exposing
unlink_rr_item, I have chosen to libify "rerere gc" in order to libify
"rerere clear", and it may be argued that this is a pleasant
side-effect.  I've also included another patch to replace the
"die_errno" call with a new "error_errno" call, and I hope this will
be used in other places as well.  Note one more subtle change: I've
chosen to pass "flags" as an argument to both functions for the sake
of consistency; if and when rerere grows more command-line options,
"flags" can be replaced by a struct containing those parsed options.

Cons: This patch is much larger than v3.

Thanks to: Junio and Jonathan.

Ramkumar Ramachandra (2):
  usage: Introduce error_errno corresponding to die_errno
  rerere: Libify "rerere clear" and "rerere gc"

 builtin/rerere.c  |   81 ++-----------------------------------------
 git-compat-util.h |    1 +
 rerere.c          |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 rerere.h          |    2 +
 usage.c           |   10 +++++
 5 files changed, 116 insertions(+), 77 deletions(-)

-- 
1.7.5.GIT

[PATCH v4 1/2] usage: Introduce error_errno corresponding to die_errno

[Ramkumar Ramachandra]

The error function always returns the constant value -1, and this does
not convey enough information about the nature of the error.  This
patch introduces a new function error_errno that functions exactly
like the error function, except that it returns `errorno` instead of a
constant value.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Is "-errno" correct? This will determine how error-handling will be
 in a more libified version of Git, and I think it's important to get
 this right.

 git-compat-util.h |    1 +
 usage.c           |   10 ++++++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 40498b3..7b82038 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -241,6 +241,7 @@ extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf,
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
diff --git a/usage.c b/usage.c
index b5e67e3..c89efcc 100644
--- a/usage.c
+++ b/usage.c
@@ -107,6 +107,16 @@ int error(const char *err, ...)
 	return -1;
 }
 
+int error_errno(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	error_routine(err, params);
+	va_end(params);
+	return -errno;
+}
+
 void warning(const char *warn, ...)
 {
 	va_list params;
-- 
1.7.5.GIT

Re: [PATCH v4 1/2] usage: Introduce error_errno corresponding to die_errno

[Ramkumar Ramachandra]

Hi,

Ramkumar Ramachandra writes:
> The error function always returns the constant value -1, and this does
> not convey enough information about the nature of the error.  This
> patch introduces a new function error_errno that functions exactly
> like the error function, except that it returns `errorno` instead of a
> constant value.

An extra note: this could have been implemented differently, as seen
in my sequencer series [1].  However, after some thought, I think this
is how we want error handling to work.  The name may be a little
troubling because it doesn't behave like die_errno: does anyone have
suggestions for a different name?

[1]: http://article.gmane.org/gmane.comp.version-control.git/171262

-- Ram

Re: [PATCH v4 1/2] usage: Introduce error_errno corresponding to die_errno

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 40498b3..7b82038 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -241,6 +241,7 @@ extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf,
>  extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
> +extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));

Anybody remotely familiar with die_errno() would at least expect that the
new and improved error_errno() function would give the errno neatly
formatted via strerror() in the message, but that is not what the function
does.

> diff --git a/usage.c b/usage.c
> index b5e67e3..c89efcc 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -107,6 +107,16 @@ int error(const char *err, ...)
>  	return -1;
>  }
>  
> +int error_errno(const char *err, ...)
> +{
> +	va_list params;
> +
> +	va_start(params, err);
> +	error_routine(err, params);
> +	va_end(params);
> +	return -errno;
> +}

Can error_routine() do something to cause errno to change?

For that matter, I suspect that the caller who would _care_ about what
kind of error it got would need to save it away itself anyway.  For
example, it is not entirely clear if the callsite of this function in your
other patch is correct.  Can rollback_lock_file() cause errno to change?

	git_config(git_rerere_gc_config, NULL);
	dir = opendir(git_path("rr-cache"));
	if (!dir) {
+		int err = errno;
-		rollback_lock_file(&write_lock);
-		return error_errno("Unable to open rr-cache directory");
+		error("Untable to open rr-cache directory: %s", strerror(err));
+		return -err;
	}

I would prefer to do without introducing a confusingly named API function
whose first and only usecase does not even suggest it would be useful.

[PATCH v4 2/2] rerere: Libify "rerere clear" and "rerere gc"

[Ramkumar Ramachandra]

Libify "rerere clear" and "rerere gc" into simple functions called
rerere_clear and rerere_garbage_collect; both functions take flags as
a lone argument and return the exit status.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Have I cleaned up all the locks correctly? The tests should probably
 be updated to catch unclean exists- I'll probably work on this
 shortly.

 builtin/rerere.c |   81 ++------------------------------------------
 rerere.c         |   99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rerere.h         |    2 +
 3 files changed, 105 insertions(+), 77 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 8235885..e2abdaa 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -12,74 +12,6 @@ static const char * const rerere_usage[] = {
 	NULL,
 };
 
-/* these values are days */
-static int cutoff_noresolve = 15;
-static int cutoff_resolve = 60;
-
-static time_t rerere_created_at(const char *name)
-{
-	struct stat st;
-	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
-}
-
-static time_t rerere_last_used_at(const char *name)
-{
-	struct stat st;
-	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
-}
-
-static void unlink_rr_item(const char *name)
-{
-	unlink(rerere_path(name, "thisimage"));
-	unlink(rerere_path(name, "preimage"));
-	unlink(rerere_path(name, "postimage"));
-	rmdir(git_path("rr-cache/%s", name));
-}
-
-static int git_rerere_gc_config(const char *var, const char *value, void *cb)
-{
-	if (!strcmp(var, "gc.rerereresolved"))
-		cutoff_resolve = git_config_int(var, value);
-	else if (!strcmp(var, "gc.rerereunresolved"))
-		cutoff_noresolve = git_config_int(var, value);
-	else
-		return git_default_config(var, value, cb);
-	return 0;
-}
-
-static void garbage_collect(struct string_list *rr)
-{
-	struct string_list to_remove = STRING_LIST_INIT_DUP;
-	DIR *dir;
-	struct dirent *e;
-	int i, cutoff;
-	time_t now = time(NULL), then;
-
-	git_config(git_rerere_gc_config, NULL);
-	dir = opendir(git_path("rr-cache"));
-	if (!dir)
-		die_errno("unable to open rr-cache directory");
-	while ((e = readdir(dir))) {
-		if (is_dot_or_dotdot(e->d_name))
-			continue;
-
-		then = rerere_last_used_at(e->d_name);
-		if (then) {
-			cutoff = cutoff_resolve;
-		} else {
-			then = rerere_created_at(e->d_name);
-			if (!then)
-				continue;
-			cutoff = cutoff_noresolve;
-		}
-		if (then < now - cutoff * 86400)
-			string_list_append(&to_remove, e->d_name);
-	}
-	for (i = 0; i < to_remove.nr; i++)
-		unlink_rr_item(to_remove.items[i].string);
-	string_list_clear(&to_remove, 0);
-}
-
 static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)
 {
 	int i;
@@ -142,20 +74,15 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 		pathspec = get_pathspec(prefix, argv + 1);
 		return rerere_forget(pathspec);
 	}
+	else if (!strcmp(argv[0], "clear"))
+		return rerere_clear(flags);
+	else if (!strcmp(argv[0], "gc"))
+		return rerere_garbage_collect(flags);
 
 	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"))
-		garbage_collect(&merge_rr);
 	else if (!strcmp(argv[0], "status"))
 		for (i = 0; i < merge_rr.nr; i++)
 			printf("%s\n", merge_rr.items[i].string);
diff --git a/rerere.c b/rerere.c
index 22dfc84..18c7413 100644
--- a/rerere.c
+++ b/rerere.c
@@ -20,6 +20,10 @@ static int rerere_autoupdate;
 
 static char *merge_rr_path;
 
+/* these values are days */
+static int cutoff_noresolve = 15;
+static int cutoff_resolve = 60;
+
 const char *rerere_path(const char *hex, const char *file)
 {
 	return git_path("rr-cache/%s/%s", hex, file);
@@ -31,6 +35,37 @@ int has_rerere_resolution(const char *hex)
 	return !stat(rerere_path(hex, "postimage"), &st);
 }
 
+static time_t rerere_created_at(const char *name)
+{
+	struct stat st;
+	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
+}
+
+static time_t rerere_last_used_at(const char *name)
+{
+	struct stat st;
+	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
+}
+
+static void unlink_rr_item(const char *name)
+{
+	unlink(rerere_path(name, "thisimage"));
+	unlink(rerere_path(name, "preimage"));
+	unlink(rerere_path(name, "postimage"));
+	rmdir(git_path("rr-cache/%s", name));
+}
+
+static int git_rerere_gc_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "gc.rerereresolved"))
+		cutoff_resolve = git_config_int(var, value);
+	else if (!strcmp(var, "gc.rerereunresolved"))
+		cutoff_noresolve = git_config_int(var, value);
+	else
+		return git_default_config(var, value, cb);
+	return 0;
+}
+
 static void read_rr(struct string_list *rr)
 {
 	unsigned char sha1[20];
@@ -623,6 +658,51 @@ int rerere(int flags)
 	return do_plain_rerere(&merge_rr, fd);
 }
 
+int rerere_garbage_collect(int flags)
+{
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+	struct string_list to_remove = STRING_LIST_INIT_DUP;
+
+	DIR *dir;
+	struct dirent *e;
+	int i, fd, cutoff;
+	time_t now = time(NULL), then;
+
+	fd = setup_rerere(&merge_rr, flags);
+	if (fd < 0)
+		return 0;
+
+	git_config(git_rerere_gc_config, NULL);
+	dir = opendir(git_path("rr-cache"));
+	if (!dir) {
+		rollback_lock_file(&write_lock);
+		return error_errno("Unable to open rr-cache directory");
+	}
+	while ((e = readdir(dir))) {
+		if (is_dot_or_dotdot(e->d_name))
+			continue;
+
+		then = rerere_last_used_at(e->d_name);
+		if (then)
+			cutoff = cutoff_resolve;
+		else {
+			then = rerere_created_at(e->d_name);
+			if (!then)
+				continue;
+			cutoff = cutoff_noresolve;
+		}
+		if (then < now - cutoff * 86400)
+			string_list_append(&to_remove, e->d_name);
+	}
+	for (i = 0; i < to_remove.nr; i++)
+		unlink_rr_item(to_remove.items[i].string);
+
+	string_list_clear(&merge_rr, 1);
+	string_list_clear(&to_remove, 0);
+	rollback_lock_file(&write_lock);
+	return 0;
+}
+
 static int rerere_forget_one_path(const char *path, struct string_list *rr)
 {
 	const char *filename;
@@ -671,3 +751,22 @@ int rerere_forget(const char **pathspec)
 	}
 	return write_rr(&merge_rr, fd);
 }
+
+int rerere_clear(int flags)
+{
+	int i, fd;
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+	fd = setup_rerere(&merge_rr, flags);
+	if (fd < 0)
+		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))
+			unlink_rr_item(name);
+	}
+	string_list_clear(&merge_rr, 1);
+	unlink_or_warn(git_path("MERGE_RR"));
+	rollback_lock_file(&write_lock);
+	return 0;
+}
diff --git a/rerere.h b/rerere.h
index 595f49f..59849f6 100644
--- a/rerere.h
+++ b/rerere.h
@@ -15,10 +15,12 @@ extern void *RERERE_RESOLVED;
 
 extern int setup_rerere(struct string_list *, int);
 extern int rerere(int);
+extern int rerere_garbage_collect(int);
 extern const char *rerere_path(const char *hex, const char *file);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
 extern int rerere_remaining(struct string_list *);
+extern int rerere_clear(int);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
 	"update the index with reused conflict resolution if possible")
-- 
1.7.5.GIT

Re: [PATCH v4 2/2] rerere: Libify "rerere clear" and "rerere gc"

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/rerere.c b/rerere.c
> index 22dfc84..18c7413 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -623,6 +658,51 @@ int rerere(int flags)
>  	return do_plain_rerere(&merge_rr, fd);
>  }
>  
> +int rerere_garbage_collect(int flags)
> +{
> +	struct string_list merge_rr = STRING_LIST_INIT_DUP;
> +	struct string_list to_remove = STRING_LIST_INIT_DUP;
> +
> +	DIR *dir;
> +	struct dirent *e;
> +	int i, fd, cutoff;
> +	time_t now = time(NULL), then;
> +
> +	fd = setup_rerere(&merge_rr, flags);
> +	if (fd < 0)
> +		return 0;

Not a good taste in API design, I would have to say.  The callers may
already want to have interacted with the rerere mechanism before calling
into this function and that is exactly why setup_rerere() is a public API
from the beginning.  I would prefer to see you make it the caller's
responsibility just like the original code you moved from builtin/rerere.c
did.

> +	git_config(git_rerere_gc_config, NULL);
> +	dir = opendir(git_path("rr-cache"));
> +	if (!dir) {
> +		rollback_lock_file(&write_lock);
> +		return error_errno("Unable to open rr-cache directory");
> +	}

I have already commented on this part and showed an alternative.

Whatever you do, how about starting from just a simple code movement,
without any change in behaviour?

That not only makes it easier to review your subsequent changes on top of
the result, but the original "large code movement" patch infinitely easier
to review, as "blame rerere.c" can tell us that almost all the lines came
from builtin/rerere.c without any new bug slipping in.

-- >8 --
Subject: [PATCH] rerere: libify rerere_clear() and rerere_gc()

This moves the two features from builtin/rerere.c to a more library-ish
portion of the codebase.  No behaviour change.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/rerere.c |   77 +------------------------------------------------
 rerere.c         |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rerere.h         |    2 +
 3 files changed, 88 insertions(+), 75 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 8235885..08213c7 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -12,74 +12,6 @@ static const char * const rerere_usage[] = {
 	NULL,
 };
 
-/* these values are days */
-static int cutoff_noresolve = 15;
-static int cutoff_resolve = 60;
-
-static time_t rerere_created_at(const char *name)
-{
-	struct stat st;
-	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
-}
-
-static time_t rerere_last_used_at(const char *name)
-{
-	struct stat st;
-	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
-}
-
-static void unlink_rr_item(const char *name)
-{
-	unlink(rerere_path(name, "thisimage"));
-	unlink(rerere_path(name, "preimage"));
-	unlink(rerere_path(name, "postimage"));
-	rmdir(git_path("rr-cache/%s", name));
-}
-
-static int git_rerere_gc_config(const char *var, const char *value, void *cb)
-{
-	if (!strcmp(var, "gc.rerereresolved"))
-		cutoff_resolve = git_config_int(var, value);
-	else if (!strcmp(var, "gc.rerereunresolved"))
-		cutoff_noresolve = git_config_int(var, value);
-	else
-		return git_default_config(var, value, cb);
-	return 0;
-}
-
-static void garbage_collect(struct string_list *rr)
-{
-	struct string_list to_remove = STRING_LIST_INIT_DUP;
-	DIR *dir;
-	struct dirent *e;
-	int i, cutoff;
-	time_t now = time(NULL), then;
-
-	git_config(git_rerere_gc_config, NULL);
-	dir = opendir(git_path("rr-cache"));
-	if (!dir)
-		die_errno("unable to open rr-cache directory");
-	while ((e = readdir(dir))) {
-		if (is_dot_or_dotdot(e->d_name))
-			continue;
-
-		then = rerere_last_used_at(e->d_name);
-		if (then) {
-			cutoff = cutoff_resolve;
-		} else {
-			then = rerere_created_at(e->d_name);
-			if (!then)
-				continue;
-			cutoff = cutoff_noresolve;
-		}
-		if (then < now - cutoff * 86400)
-			string_list_append(&to_remove, e->d_name);
-	}
-	for (i = 0; i < to_remove.nr; i++)
-		unlink_rr_item(to_remove.items[i].string);
-	string_list_clear(&to_remove, 0);
-}
-
 static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)
 {
 	int i;
@@ -148,14 +80,9 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 		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"));
+		rerere_clear(&merge_rr);
 	} else if (!strcmp(argv[0], "gc"))
-		garbage_collect(&merge_rr);
+		rerere_gc(&merge_rr);
 	else if (!strcmp(argv[0], "status"))
 		for (i = 0; i < merge_rr.nr; i++)
 			printf("%s\n", merge_rr.items[i].string);
diff --git a/rerere.c b/rerere.c
index 22dfc84..dee2cb1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -671,3 +671,87 @@ int rerere_forget(const char **pathspec)
 	}
 	return write_rr(&merge_rr, fd);
 }
+
+static time_t rerere_created_at(const char *name)
+{
+	struct stat st;
+	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
+}
+
+static time_t rerere_last_used_at(const char *name)
+{
+	struct stat st;
+	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
+}
+
+static void unlink_rr_item(const char *name)
+{
+	unlink(rerere_path(name, "thisimage"));
+	unlink(rerere_path(name, "preimage"));
+	unlink(rerere_path(name, "postimage"));
+	rmdir(git_path("rr-cache/%s", name));
+}
+
+struct rerere_gc_config_cb {
+	int cutoff_noresolve;
+	int cutoff_resolve;
+};
+
+static int git_rerere_gc_config(const char *var, const char *value, void *cb)
+{
+	struct rerere_gc_config_cb *cf = cb;
+
+	if (!strcmp(var, "gc.rerereresolved"))
+		cf->cutoff_resolve = git_config_int(var, value);
+	else if (!strcmp(var, "gc.rerereunresolved"))
+		cf->cutoff_noresolve = git_config_int(var, value);
+	else
+		return git_default_config(var, value, cb);
+	return 0;
+}
+
+void rerere_gc(struct string_list *rr)
+{
+	struct string_list to_remove = STRING_LIST_INIT_DUP;
+	DIR *dir;
+	struct dirent *e;
+	int i, cutoff;
+	time_t now = time(NULL), then;
+	struct rerere_gc_config_cb cf = { 15, 60 };
+
+	git_config(git_rerere_gc_config, &cf);
+	dir = opendir(git_path("rr-cache"));
+	if (!dir)
+		die_errno("unable to open rr-cache directory");
+	while ((e = readdir(dir))) {
+		if (is_dot_or_dotdot(e->d_name))
+			continue;
+
+		then = rerere_last_used_at(e->d_name);
+		if (then) {
+			cutoff = cf.cutoff_resolve;
+		} else {
+			then = rerere_created_at(e->d_name);
+			if (!then)
+				continue;
+			cutoff = cf.cutoff_noresolve;
+		}
+		if (then < now - cutoff * 86400)
+			string_list_append(&to_remove, e->d_name);
+	}
+	for (i = 0; i < to_remove.nr; i++)
+		unlink_rr_item(to_remove.items[i].string);
+	string_list_clear(&to_remove, 0);
+}
+
+void rerere_clear(struct string_list *merge_rr)
+{
+	int i;
+
+	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"));
+}
diff --git a/rerere.h b/rerere.h
index 595f49f..fcd8bc1 100644
--- a/rerere.h
+++ b/rerere.h
@@ -19,6 +19,8 @@ extern const char *rerere_path(const char *hex, const char *file);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
 extern int rerere_remaining(struct string_list *);
+extern void rerere_clear(struct string_list *);
+extern void rerere_gc(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
 	"update the index with reused conflict resolution if possible")
-- 
1.7.5.1.268.gce5bd