From: Junio C Hamano <gitster@pobox•com>
To: Ramkumar Ramachandra <artagnon@gmail•com>
Cc: Git List <git@vger•kernel.org>,
Daniel Barkalow <barkalow@iabervon•org>,
Jonathan Nieder <jrnieder@gmail•com>
Subject: Re: [PATCH v4 2/2] rerere: Libify "rerere clear" and "rerere gc"
Date: Sun, 08 May 2011 13:06:55 -0700 [thread overview]
Message-ID: <7v62plrm74.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: 1304839851-6477-3-git-send-email-artagnon@gmail.com
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
prev parent reply other threads:[~2011-05-08 20:07 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-11 8:51 [PATCH] rerere: Expose an API corresponding to 'clear' functionality Ramkumar Ramachandra
2011-04-11 18:36 ` Junio C Hamano
2011-04-13 13:18 ` [PATCH v2] " Ramkumar Ramachandra
2011-04-13 20:38 ` Jonathan Nieder
2011-05-06 6:36 ` [PATCH v3] " Ramkumar Ramachandra
2011-05-06 16:51 ` Junio C Hamano
2011-05-07 13:17 ` Ramkumar Ramachandra
2011-05-08 7:30 ` [PATCH v4 0/2] Libify rerere: clear and gc Ramkumar Ramachandra
2011-05-08 7:30 ` [PATCH v4 1/2] usage: Introduce error_errno corresponding to die_errno Ramkumar Ramachandra
2011-05-08 9:46 ` Ramkumar Ramachandra
2011-05-08 18:10 ` Junio C Hamano
2011-05-08 7:30 ` [PATCH v4 2/2] rerere: Libify "rerere clear" and "rerere gc" Ramkumar Ramachandra
2011-05-08 20:06 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7v62plrm74.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=artagnon@gmail$(echo .)com \
--cc=barkalow@iabervon$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=jrnieder@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox