Re: [RFC PATCH] rerere: fix overeager gc

SZEDER Gábor [off-list ref] writes:

...  But I'm not sure I
can rely on that when gc'ing.

Looking at the timestamp of "thisimage" would probaboly be more sensible
than "preimage" alone, _if_ "thisimage" still exists.  It is rewritten
every time this particular conflict is observed; this is not necessarily
when the recorded resolution is _used_, but it may be close enough in
practice.

You probably would want to rename the helper as "last_checked_at", though.

After rerere does its work, however, "thisimage" does not have to stay
around (the user can remove it, or we could enhance "gc" to do so).

+		if (has_rerere_resolution(e->d_name)) {
+			then = rerere_last_used_at(e->d_name);
+			if (!then)
+				continue;

Here you already know that you have resolution (i.e. "postimage"), but
your new function cannot stat a corresponding "thisimage", so you err
on the safer side---but that means you may keep pre/post image pairs
forever if somebody removes otherwise unused "thisimage" from a distant
past.  Perhaps we should apply cutoff_noresolve to the entry here?

One possibility is to look at the timestamp of the directory itself
instead.  Then we can safely gc otherwise-unused "thisimage" file when
rerere is not in use.  I wonder if directory m_time timestamps are usable
for this purpose on non-POSIX platforms?

Am 6/29/2010 19:59, schrieb Junio C Hamano:

One possibility is to look at the timestamp of the directory itself
instead.  Then we can safely gc otherwise-unused "thisimage" file when
rerere is not in use.  I wonder if directory m_time timestamps are usable
for this purpose on non-POSIX platforms?

I don't think that will work at all: We only use fopen() to write
thisimage, which only truncates the file, but doesn't modify mtime of the
directory. Nor do we create any other (temporary) directory entries that
would modify the mtime.

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?

-- Hannes

Hi,

thank you both for your comments.


On Wed, Jun 30, 2010 at 08:12:30AM +0200, Johannes Sixt wrote:

Am 6/29/2010 19:59, schrieb Junio C Hamano:

quoted

One possibility is to look at the timestamp of the directory itself
instead.  Then we can safely gc otherwise-unused "thisimage" file when
rerere is not in use.  I wonder if directory m_time timestamps are usable
for this purpose on non-POSIX platforms?

I don't think that will work at all: We only use fopen() to write
thisimage, which only truncates the file, but doesn't modify mtime of the
directory. Nor do we create any other (temporary) directory entries that
would modify the mtime.

Indeed; on Linux I have:

drwxr-xr-x 2 szeder szeder 4096 2010-06-24 10:59 .git/rr-cache/13e67feeb07f97d6fccc2257d793d93ec4e730bf/

-rw-r--r-- 1 szeder szeder 3095 2010-06-30 04:56 .git/rr-cache/13e67feeb07f97d6fccc2257d793d93ec4e730bf/thisimage

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?

Will take a look.


Best,
Gábor

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

Prevent the loss of valuable merge resolutions by updating the
timestamp of the conflicted automerge result each time when
encountering the same merge conflict.

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

On Wed, Jun 30, 2010 at 08:12:30AM +0200, Johannes Sixt wrote:

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?

So, how about this?


 builtin/rerere.c  |    4 ++--
 rerere.c          |    8 +++++++-
 t/t4200-rerere.sh |    8 ++++++++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 0048f9e..4d4faae 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c

@@ -13,7 +13,7 @@ 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_checked_at(const char *name)
 {
 	struct stat st;
 	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;

@@ -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_checked_at(e->d_name);
 		if (!then)
 			continue;
 		cutoff = (has_rerere_resolution(e->d_name)

diff --git a/rerere.c b/rerere.c
index 2197890..1cc7c65 100644
--- a/rerere.c
+++ b/rerere.c

@@ -378,7 +378,13 @@ static int merge(const char *name, const char *path)
 	}
 	ret = ll_merge(&result, path, &base, NULL, &cur, "", &other, "", 0);
 	if (!ret) {
-		FILE *f = fopen(path, "w");
+		FILE *f;
+
+		if (utime(rerere_path(name, "preimage"), NULL) < 0)
+			warning("failed utime() on %s: %s",
+					rerere_path(name, "preimage"),
+					strerror(errno));
+		f = fopen(path, "w");
 		if (!f)
 			return error("Could not open %s: %s", path,
 				     strerror(errno));

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 70856d0..c01d930 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 preimage timestamp' '
+	git reset --hard &&
+	oldmtime=$(test-chmtime -v -42 $rr/preimage |cut -f 1) &&
+	test_must_fail git pull . first &&
+	newmtime=$(test-chmtime -v +0 $rr/preimage |cut -f 1) &&
+	test $oldmtime -lt $newmtime
+'
+
 rm $rr/postimage
 echo "$sha1	a1" | perl -pe 'y/\012/\000/' > .git/MERGE_RR
 

-- 
1.7.2.rc0.54.g4d821

Am 7/1/2010 11:36, schrieb SZEDER Gábor:

-static time_t rerere_created_at(const char *name)
+static time_t rerere_last_checked_at(const char *name)

rerere_last_used_at?

I think this should be outside of 'if (!ret)' condition because even if
the merge fails, the resolution was *used*.

Mental note: update mingw_utime to accept NULL for the second parameter...

-- Hannes

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

Prevent the loss of valuable merge resolutions by updating the
timestamp of the conflicted automerge result each time when
encountering the same merge conflict.

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

On Thu, Jul 01, 2010 at 12:10:55PM +0200, Johannes Sixt wrote:

rerere_last_used_at?

I think this should be outside of 'if (!ret)' condition because even if
the merge fails, the resolution was *used*.

Right on both points.


 builtin/rerere.c  |    4 ++--
 rerere.c          |    4 ++++
 t/t4200-rerere.sh |    8 ++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

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

@@ -13,7 +13,7 @@ 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;

@@ -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..0d8a167 100644
--- a/rerere.c
+++ b/rerere.c

@@ -389,6 +389,10 @@ static int merge(const char *name, const char *path)
 				     strerror(errno));
 	}
 
+	if (utime(rerere_path(name, "preimage"), NULL) < 0)
+		warning("failed utime() on %s: %s",
+				rerere_path(name, "preimage"), strerror(errno));
+
 out:
 	free(cur.ptr);
 	free(base.ptr);

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 70856d0..c01d930 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 preimage timestamp' '
+	git reset --hard &&
+	oldmtime=$(test-chmtime -v -42 $rr/preimage |cut -f 1) &&
+	test_must_fail git pull . first &&
+	newmtime=$(test-chmtime -v +0 $rr/preimage |cut -f 1) &&
+	test $oldmtime -lt $newmtime
+'
+
 rm $rr/postimage
 echo "$sha1	a1" | perl -pe 'y/\012/\000/' > .git/MERGE_RR
 

-- 
1.7.2.rc0.54.g4d821

POSIX sayeth:

  "If times is a null pointer, the access and modification
   times of the file shall be set to the current time."

Let's do so.

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

On Thu, Jul 01, 2010 at 12:10:55PM +0200, Johannes Sixt wrote:

Mental note: update mingw_utime to accept NULL for the second parameter...

Here it is, but I don't have mingw, so it's completely untested.

 compat/mingw.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 9a8e336..a54db74 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -304,8 +304,14 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)
 		goto revert_attrs;
 	}
 
-	time_t_to_filetime(times->modtime, &mft);
-	time_t_to_filetime(times->actime, &aft);
+	if (times) {
+		time_t_to_filetime(times->modtime, &mft);
+		time_t_to_filetime(times->actime, &aft);
+	} else {
+		GetSystemTimeAsFileTime(&mft);
+		aft->dwLowDateTime = mft->dwLowDateTime;
+		aft->dwHighDateTime = mft->dwHighDateTime;
+	}
 	if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {
 		errno = EINVAL;
 		rc = -1;

-- 
1.7.2.rc0.54.g4d821

From: SZEDER Gábor <redacted>

POSIX sayeth:

  "If times is a null pointer, the access and modification
   times of the file shall be set to the current time."

Let's do so.

Signed-off-by: SZEDER Gábor <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
 >> Mental note: update mingw_utime to accept NULL for the second
 >> parameter...
 > 
 > Here it is, but I don't have mingw, so it's completely untested.

 Thanks. Here is a version that compiles; the interdiff is

 @@ -309,8 +309,7 @@
                 time_t_to_filetime(times->actime, &aft);
         } else {
                 GetSystemTimeAsFileTime(&mft);
 -               aft->dwLowDateTime = mft->dwLowDateTime;
 -               aft->dwHighDateTime = mft->dwHighDateTime;
 +               aft = mft;
         }
         if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {
                 errno = EINVAL;

 With this, the series passes the test suite on MinGW.

 -- Hannes

 compat/mingw.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 0722a6d..b6f0a7f 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -304,8 +304,13 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)
 		goto revert_attrs;
 	}
 
-	time_t_to_filetime(times->modtime, &mft);
-	time_t_to_filetime(times->actime, &aft);
+	if (times) {
+		time_t_to_filetime(times->modtime, &mft);
+		time_t_to_filetime(times->actime, &aft);
+	} else {
+		GetSystemTimeAsFileTime(&mft);
+		aft = mft;
+	}
 	if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {
 		errno = EINVAL;
 		rc = -1;

-- 
1.7.2.rc1.1057.g1270