Re: [RFC PATCH] rerere: fix overeager gc

Johannes Sixt [off-list ref] writes:

Am 7/1/2010 18:27, schrieb Junio C Hamano:

quoted

Johannes Sixt [off-list ref] writes:

quoted

Would it be possible to update the timestamp of preimage every time it is
used (e.g., in rerere.c:merge()), and check for that?

It would be _possible_, but we are _not_ modifying the file at that point,
so somehow that solution feels very wrong, no?

rr-cache is basically a static database. The fact that we have a file
named 'thisimage' is just an abuse - to put a temporary file somewhere.
Therefore, depending on lockfile infrastructure to change timestamps for
us while manipulating 'thisimage' should feel no less wrong, don't you
think so?

Not really.  

"This" in "thisimage" stands for "image involved in conflict we are
looking at _this moment_", and using the per-conflict-id directory to
house that file was a conscious design decision.  Because you are trying
to record a new data item (namely "the last time we saw this conflict")
somewhere in the rr-cache database, using the timestamp of that directory
to tell when was the last time that _this moment_ was makes sense, at
least to me.

If anything, "preimage" that has newer timestamp than "postimage" feels
wrong, as you would record pre, think for a while how the corresponding
post should look like, and then record post.

If we for whatever reason trust placing an extra timestamp on a regular
file more than using directory timestamp (which I think may be a valid
concern from portability point of view), I'd rather see "preimage"
timestamp to keep track of the time when we _first_ encountered the
particular conflict, and "postimage" used for recording the time when we
saw the conflict most recently.

Am 7/2/2010 19:25, schrieb Junio C Hamano:

If anything, "preimage" that has newer timestamp than "postimage" feels
wrong,...

Indeed; I agree.

If we for whatever reason trust placing an extra timestamp on a regular
file more than using directory timestamp (which I think may be a valid
concern from portability point of view),

Windows behaves well in this regard. Writing of thisimage must be
converted to lockfile infrastructure, of course.

I'd rather see "preimage"
timestamp to keep track of the time when we _first_ encountered the
particular conflict, and "postimage" used for recording the time when we
saw the conflict most recently.

That would be fine, too.

-- Hannes

Instead of this series, I'll use this for a while; i.e., check the
timestamp of thisimage, and if it does not exist, check the directory's
timestamp. It doesn't pass t4200.20 because the test doesn't set the
timestamps of thisimage and the directory into the past.

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 980d542..49fcbf7 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c

@@ -13,10 +13,14 @@ static const char git_rerere_usage[] =
 static int cutoff_noresolve = 15;
 static int cutoff_resolve = 60;
 
-static time_t rerere_created_at(const char *name)
+static time_t rerere_last_used_at(const char *name)
 {
 	struct stat st;
-	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
+	if (!stat(rerere_path(name, "thisimage"), &st))
+		return st.st_mtime;
+	if (errno == ENOENT && !stat(rerere_path(name, "."), &st))
+		return st.st_mtime;
+	return (time_t) 0;
 }
 
 static void unlink_rr_item(const char *name)

@@ -53,7 +57,7 @@ static void garbage_collect(struct string_list *rr)
 	while ((e = readdir(dir))) {
 		if (is_dot_or_dotdot(e->d_name))
 			continue;
-		then = rerere_created_at(e->d_name);
+		then = rerere_last_used_at(e->d_name);
 		if (!then)
 			continue;
 		cutoff = (has_rerere_resolution(e->d_name)

'rerere gc' prunes resolutions of conflicted merges that occurred long
time ago, and when doing so it takes the creation time of the
conflicted automerge results into account.  This can cause the loss of
frequently used merge resolutions (e.g. long-living topic branches are
merged into a regularly rebuilt integration branch (think of git's
pu)) when they become old enough to exceed 'rerere gc's threshold.

To prevent the loss of valuable merge resolutions 'rerere' will (1)
write 'thisimage' into a temporary file and then rename it into its
final destination, thereby updating the directory timestamp each time
when encountering the same merge conflict, and (2) take the directory
timestamp, i.e. the time of the last usage into account when gc'ing.

Signed-off-by: SZEDER Gábor <redacted>
---

On Mon, Jul 05, 2010 at 08:02:34AM +0200, Johannes Sixt wrote:

Am 7/2/2010 19:25, schrieb Junio C Hamano:

quoted

If we for whatever reason trust placing an extra timestamp on a regular
file more than using directory timestamp (which I think may be a valid
concern from portability point of view),

Windows behaves well in this regard. Writing of thisimage must be
converted to lockfile infrastructure, of course.

So, here is the next take with the tmpfile-rename stuff to update the
directory timestamp.  If there are any portability issues wrt directory
timestamp updating, the new test should catch them early.

 builtin/rerere.c  |    6 +++---
 rerere.c          |   19 +++++++++++++++++--
 t/t4200-rerere.sh |   16 ++++++++++++----
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 980d542..bede3eb 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c

@@ -13,10 +13,10 @@ static const char git_rerere_usage[] =
 static int cutoff_noresolve = 15;
 static int cutoff_resolve = 60;
 
-static time_t rerere_created_at(const char *name)
+static time_t rerere_last_used_at(const char *name)
 {
 	struct stat st;
-	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
+	return stat(rerere_path(name, "."), &st) ? (time_t) 0 : st.st_mtime;
 }
 
 static void unlink_rr_item(const char *name)

@@ -53,7 +53,7 @@ static void garbage_collect(struct string_list *rr)
 	while ((e = readdir(dir))) {
 		if (is_dot_or_dotdot(e->d_name))
 			continue;
-		then = rerere_created_at(e->d_name);
+		then = rerere_last_used_at(e->d_name);
 		if (!then)
 			continue;
 		cutoff = (has_rerere_resolution(e->d_name)

diff --git a/rerere.c b/rerere.c
index d03a696..e415f54 100644
--- a/rerere.c
+++ b/rerere.c

@@ -363,13 +363,28 @@ static int find_conflict(struct string_list *conflict)
 
 static int merge(const char *name, const char *path)
 {
-	int ret;
+	int fd, ret;
 	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
 	mmbuffer_t result = {NULL, 0};
+	char *tmpfile;
+
+	tmpfile = xstrdup(rerere_path(name, "tmp_thisimage_XXXXXX"));
+	fd = git_mkstemp_mode(tmpfile, 0600);
+	if (fd < 0) {
+		error("unable to create temporary file %s for rerere: %s\n",
+				tmpfile, strerror(errno));
+		return 1;
+	}
+	close(fd);
 
-	if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
+	if (handle_file(path, NULL, tmpfile) < 0)
 		return 1;
 
+	if (move_temp_to_file(tmpfile, rerere_path(name, "thisimage")) < 0)
+		return 1;
+
+	free(tmpfile);
+
 	if (read_mmfile(&cur, rerere_path(name, "thisimage")) ||
 			read_mmfile(&base, rerere_path(name, "preimage")) ||
 			read_mmfile(&other, rerere_path(name, "postimage"))) {

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 70856d0..a425329 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh

@@ -144,6 +144,14 @@ test_expect_success 'rerere kicked in' "! grep ^=======$ a1"
 
 test_expect_success 'rerere prefers first change' 'test_cmp a1 expect'
 
+test_expect_success 'rerere updates directory timestamp' '
+	git reset --hard &&
+	oldmtime=$(test-chmtime -v -42 $rr |cut -f 1) &&
+	test_must_fail git pull . first &&
+	newmtime=$(test-chmtime -v +0 $rr |cut -f 1) &&
+	test $oldmtime -lt $newmtime
+'
+
 rm $rr/postimage
 echo "$sha1	a1" | perl -pe 'y/\012/\000/' > .git/MERGE_RR
 

@@ -165,16 +173,16 @@ just_over_15_days_ago=$((-1-15*86400))
 almost_60_days_ago=$((60-60*86400))
 just_over_60_days_ago=$((-1-60*86400))
 
-test-chmtime =$almost_60_days_ago $rr/preimage
-test-chmtime =$almost_15_days_ago $rr2/preimage
+test-chmtime =$almost_60_days_ago $rr
+test-chmtime =$almost_15_days_ago $rr2
 
 test_expect_success 'garbage collection (part1)' 'git rerere gc'
 
 test_expect_success 'young records still live' \
 	"test -f $rr/preimage && test -f $rr2/preimage"
 
-test-chmtime =$just_over_60_days_ago $rr/preimage
-test-chmtime =$just_over_15_days_ago $rr2/preimage
+test-chmtime =$just_over_60_days_ago $rr
+test-chmtime =$just_over_15_days_ago $rr2
 
 test_expect_success 'garbage collection (part2)' 'git rerere gc'
 

-- 
1.7.2.rc2.42.gfe876