{"thread":{"id":"24231","subject":"[RFC PATCH] rerere: fix overeager gc","startedAt":"2010-06-29T11:38:18Z","lastAt":"2010-07-14T18:33:17Z","messageCount":24,"participants":["SZEDER Gábor","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"144456","messageId":"1277811498-17288-1-git-send-email-szeder@ira.uka.de","threadId":"24231","inReplyTo":null,"subject":"[RFC PATCH] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-06-29T11:38:18Z","receivedAt":"2010-06-29T11:38:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'rerere gc' prunes resolutions of conflicted merges that occurred long\ntime ago, and when doing so it takes the creation time of the merge\nconflict resolution into account.  This can cause the loss of\nfrequently used merge resolutions (e.g. long-living topic branches are\nmerged into a regularly rebuilt integration branch (think of git's\npu)) when they become old enough to exceed 'rerere gc's threshold.\n\nPrevent the loss of valuable merge resolutions by changing 'rerere gc'\nto take the time of last usage of the merge resolution into account\nwhen determining whether a merge resolution should be pruned.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nI was wondering that every once in a while when I got a merge conflict\nduring rebuilding my integration branch then it was usually followed\nwith a bunch of other conflicts as well, even though nothing really\nchanged around the conflicting areas.  Until today at last I noticed\nthat it happens right after doing a 'git gc'...\n\nRFC, because I would not say that I put in too much effort to fully\nunderstand how rerere works internally...  As far as I observed\nrerere's behaviour and understood its code, thisimage is always\nrewritten each time a merge resolution is used.  But I'm not sure I\ncan rely on that when gc'ing.\n\n builtin/rerere.c  |   15 +++++++++++++--\n t/t4200-rerere.sh |   18 +++++++++++++++++-\n 2 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 0048f9e..e095852 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -19,6 +19,12 @@ static time_t rerere_created_at(const char *name)\n \treturn stat(rerere_path(name, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n }\n \n+static time_t rerere_last_used_at(const char *name)\n+{\n+\tstruct stat st;\n+\treturn stat(rerere_path(name, \"thisimage\"), &st) ? (time_t) 0 : st.st_mtime;\n+}\n+\n static void unlink_rr_item(const char *name)\n {\n \tunlink(rerere_path(name, \"thisimage\"));\n@@ -56,8 +62,13 @@ static void garbage_collect(struct string_list *rr)\n \t\tthen = rerere_created_at(e->d_name);\n \t\tif (!then)\n \t\t\tcontinue;\n-\t\tcutoff = (has_rerere_resolution(e->d_name)\n-\t\t\t  ? cutoff_resolve : cutoff_noresolve);\n+\t\tif (has_rerere_resolution(e->d_name)) {\n+\t\t\tthen = rerere_last_used_at(e->d_name);\n+\t\t\tif (!then)\n+\t\t\t\tcontinue;\n+\t\t\tcutoff = cutoff_resolve;\n+\t\t} else\n+\t\t\tcutoff = cutoff_noresolve;\n \t\tif (then < now - cutoff * 86400)\n \t\t\tstring_list_append(e->d_name, &to_remove);\n \t}\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 70856d0..45c9df8 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -154,33 +154,49 @@ test_expect_success 'clear removed the directory' \"test ! -d $rr\"\n mkdir $rr\n echo Hello > $rr/preimage\n echo World > $rr/postimage\n+echo \"!\" > $rr/thisimage\n \n sha2=4000000000000000000000000000000000000000\n rr2=.git/rr-cache/$sha2\n mkdir $rr2\n echo Hello > $rr2/preimage\n \n+sha3=ffffffffffffffffffffffffffffffffffffffff\n+rr3=.git/rr-cache/$sha3\n+mkdir $rr3\n+echo Hello > $rr3/preimage\n+echo World > $rr3/postimage\n+echo \"!\" > $rr3/thisimage\n+\n almost_15_days_ago=$((60-15*86400))\n just_over_15_days_ago=$((-1-15*86400))\n almost_60_days_ago=$((60-60*86400))\n just_over_60_days_ago=$((-1-60*86400))\n \n test-chmtime =$almost_60_days_ago $rr/preimage\n+test-chmtime =$almost_60_days_ago $rr/thisimage\n test-chmtime =$almost_15_days_ago $rr2/preimage\n+test-chmtime =$almost_60_days_ago $rr3/preimage\n+test-chmtime =$almost_60_days_ago $rr3/thisimage\n \n test_expect_success 'garbage collection (part1)' 'git rerere gc'\n \n test_expect_success 'young records still live' \\\n-\t\"test -f $rr/preimage && test -f $rr2/preimage\"\n+\t\"test -f $rr/preimage && test -f $rr2/preimage && test -f $rr3/preimage\"\n \n test-chmtime =$just_over_60_days_ago $rr/preimage\n+test-chmtime =$just_over_60_days_ago $rr/thisimage\n test-chmtime =$just_over_15_days_ago $rr2/preimage\n+test-chmtime =$just_over_60_days_ago $rr3/preimage\n \n test_expect_success 'garbage collection (part2)' 'git rerere gc'\n \n test_expect_success 'old records rest in peace' \\\n \t\"test ! -f $rr/preimage && test ! -f $rr2/preimage\"\n \n+test_expect_success 'recently used records are still there' \\\n+\t\"test -f $rr3/preimage\"\n+\n test_expect_success 'file2 added differently in two branches' '\n \tgit reset --hard &&\n \tgit checkout -b fourth &&\n-- \n1.7.2.rc0.42.g400d\n"},{"id":"144479","messageId":"7vy6dx90uk.fsf@alter.siamese.dyndns.org","threadId":"24231","inReplyTo":"1277811498-17288-1-git-send-email-szeder@ira.uka.de","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-29T17:59:15Z","receivedAt":"2010-06-29T17:59:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder@ira.uka.de> writes:\n\n> ...  But I'm not sure I\n> can rely on that when gc'ing.\n\nLooking at the timestamp of \"thisimage\" would probaboly be more sensible\nthan \"preimage\" alone, _if_ \"thisimage\" still exists.  It is rewritten\nevery time this particular conflict is observed; this is not necessarily\nwhen the recorded resolution is _used_, but it may be close enough in\npractice.\n\nYou probably would want to rename the helper as \"last_checked_at\", though.\n\nAfter rerere does its work, however, \"thisimage\" does not have to stay\naround (the user can remove it, or we could enhance \"gc\" to do so).\n\n> +\t\tif (has_rerere_resolution(e->d_name)) {\n> +\t\t\tthen = rerere_last_used_at(e->d_name);\n> +\t\t\tif (!then)\n> +\t\t\t\tcontinue;\n\nHere you already know that you have resolution (i.e. \"postimage\"), but\nyour new function cannot stat a corresponding \"thisimage\", so you err\non the safer side---but that means you may keep pre/post image pairs\nforever if somebody removes otherwise unused \"thisimage\" from a distant\npast.  Perhaps we should apply cutoff_noresolve to the entry here?\n\nOne possibility is to look at the timestamp of the directory itself\ninstead.  Then we can safely gc otherwise-unused \"thisimage\" file when\nrerere is not in use.  I wonder if directory m_time timestamps are usable\nfor this purpose on non-POSIX platforms?\n"},{"id":"144503","messageId":"4C2AE04E.9090901@viscovery.net","threadId":"24231","inReplyTo":"7vy6dx90uk.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-06-30T06:12:30Z","receivedAt":"2010-06-30T06:12:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/29/2010 19:59, schrieb Junio C Hamano:\n> One possibility is to look at the timestamp of the directory itself\n> instead.  Then we can safely gc otherwise-unused \"thisimage\" file when\n> rerere is not in use.  I wonder if directory m_time timestamps are usable\n> for this purpose on non-POSIX platforms?\n\nI don't think that will work at all: We only use fopen() to write\nthisimage, which only truncates the file, but doesn't modify mtime of the\ndirectory. Nor do we create any other (temporary) directory entries that\nwould modify the mtime.\n\nWould it be possible to update the timestamp of preimage every time it is\nused (e.g., in rerere.c:merge()), and check for that?\n\n-- Hannes\n"},{"id":"144506","messageId":"20100630080109.GA4264@neumann","threadId":"24231","inReplyTo":"4C2AE04E.9090901@viscovery.net","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-06-30T08:01:09Z","receivedAt":"2010-06-30T08:01:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nthank you both for your comments.\n\n\nOn Wed, Jun 30, 2010 at 08:12:30AM +0200, Johannes Sixt wrote:\n> Am 6/29/2010 19:59, schrieb Junio C Hamano:\n> > One possibility is to look at the timestamp of the directory itself\n> > instead.  Then we can safely gc otherwise-unused \"thisimage\" file when\n> > rerere is not in use.  I wonder if directory m_time timestamps are usable\n> > for this purpose on non-POSIX platforms?\n> \n> I don't think that will work at all: We only use fopen() to write\n> thisimage, which only truncates the file, but doesn't modify mtime of the\n> directory. Nor do we create any other (temporary) directory entries that\n> would modify the mtime.\n\nIndeed; on Linux I have:\n\ndrwxr-xr-x 2 szeder szeder 4096 2010-06-24 10:59 .git/rr-cache/13e67feeb07f97d6fccc2257d793d93ec4e730bf/\n\n-rw-r--r-- 1 szeder szeder 3095 2010-06-30 04:56 .git/rr-cache/13e67feeb07f97d6fccc2257d793d93ec4e730bf/thisimage\n\n\n> Would it be possible to update the timestamp of preimage every time it is\n> used (e.g., in rerere.c:merge()), and check for that?\n\nWill take a look.\n\n\nBest,\nGábor\n"},{"id":"144519","messageId":"7v7hlg8s0e.fsf@alter.siamese.dyndns.org","threadId":"24231","inReplyTo":"4C2AE04E.9090901@viscovery.net","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-30T15:22:25Z","receivedAt":"2010-06-30T15:22:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 6/29/2010 19:59, schrieb Junio C Hamano:\n>> One possibility is to look at the timestamp of the directory itself\n>> instead.  Then we can safely gc otherwise-unused \"thisimage\" file when\n>> rerere is not in use.  I wonder if directory m_time timestamps are usable\n>> for this purpose on non-POSIX platforms?\n>\n> I don't think that will work at all: We only use fopen() to write\n> thisimage, which only truncates the file, but doesn't modify mtime of the\n> directory. Nor do we create any other (temporary) directory entries that\n> would modify the mtime.\n\nAh, I see; I don't mind a patch that fixes the creation of thisimage to\nfollow the \"create into temporary and then commit by renaming\" pattern.\n\nWould that solve this issue?\n"},{"id":"144533","messageId":"4C2B6574.1060902@viscovery.net","threadId":"24231","inReplyTo":"7v7hlg8s0e.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-06-30T15:40:36Z","receivedAt":"2010-06-30T15:40:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/30/2010 17:22, schrieb Junio C Hamano:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n>> Am 6/29/2010 19:59, schrieb Junio C Hamano:\n>>> One possibility is to look at the timestamp of the directory itself\n>>> instead.  Then we can safely gc otherwise-unused \"thisimage\" file when\n>>> rerere is not in use.  I wonder if directory m_time timestamps are usable\n>>> for this purpose on non-POSIX platforms?\n>>\n>> I don't think that will work at all: We only use fopen() to write\n>> thisimage, which only truncates the file, but doesn't modify mtime of the\n>> directory. Nor do we create any other (temporary) directory entries that\n>> would modify the mtime.\n> \n> Ah, I see; I don't mind a patch that fixes the creation of thisimage to\n> follow the \"create into temporary and then commit by renaming\" pattern.\n> \n> Would that solve this issue?\n\nI think so. On Windows, the directory's mtime is updated.\n\n-- Hannes\n"},{"id":"144575","messageId":"1277977009-22780-1-git-send-email-szeder@ira.uka.de","threadId":"24231","inReplyTo":"4C2AE04E.9090901@viscovery.net","subject":"[PATCH v2] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-01T09:36:49Z","receivedAt":"2010-07-01T09:36:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'rerere gc' prunes resolutions of conflicted merges that occurred long\ntime ago, and when doing so it takes the creation time of the\nconflicted automerge results into account.  This can cause the loss of\nfrequently used merge resolutions (e.g. long-living topic branches are\nmerged into a regularly rebuilt integration branch (think of git's\npu)) when they become old enough to exceed 'rerere gc's threshold.\n\nPrevent the loss of valuable merge resolutions by updating the\ntimestamp of the conflicted automerge result each time when\nencountering the same merge conflict.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nOn Wed, Jun 30, 2010 at 08:12:30AM +0200, Johannes Sixt wrote:\n> Would it be possible to update the timestamp of preimage every time it is\n> used (e.g., in rerere.c:merge()), and check for that?\n\nSo, how about this?\n\n\n builtin/rerere.c  |    4 ++--\n rerere.c          |    8 +++++++-\n t/t4200-rerere.sh |    8 ++++++++\n 3 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 0048f9e..4d4faae 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -13,7 +13,7 @@ static const char git_rerere_usage[] =\n static int cutoff_noresolve = 15;\n static int cutoff_resolve = 60;\n \n-static time_t rerere_created_at(const char *name)\n+static time_t rerere_last_checked_at(const char *name)\n {\n \tstruct stat st;\n \treturn stat(rerere_path(name, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n@@ -53,7 +53,7 @@ static void garbage_collect(struct string_list *rr)\n \twhile ((e = readdir(dir))) {\n \t\tif (is_dot_or_dotdot(e->d_name))\n \t\t\tcontinue;\n-\t\tthen = rerere_created_at(e->d_name);\n+\t\tthen = rerere_last_checked_at(e->d_name);\n \t\tif (!then)\n \t\t\tcontinue;\n \t\tcutoff = (has_rerere_resolution(e->d_name)\ndiff --git a/rerere.c b/rerere.c\nindex 2197890..1cc7c65 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -378,7 +378,13 @@ static int merge(const char *name, const char *path)\n \t}\n \tret = ll_merge(&result, path, &base, NULL, &cur, \"\", &other, \"\", 0);\n \tif (!ret) {\n-\t\tFILE *f = fopen(path, \"w\");\n+\t\tFILE *f;\n+\n+\t\tif (utime(rerere_path(name, \"preimage\"), NULL) < 0)\n+\t\t\twarning(\"failed utime() on %s: %s\",\n+\t\t\t\t\trerere_path(name, \"preimage\"),\n+\t\t\t\t\tstrerror(errno));\n+\t\tf = fopen(path, \"w\");\n \t\tif (!f)\n \t\t\treturn error(\"Could not open %s: %s\", path,\n \t\t\t\t     strerror(errno));\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 70856d0..c01d930 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -144,6 +144,14 @@ test_expect_success 'rerere kicked in' \"! grep ^=======$ a1\"\n \n test_expect_success 'rerere prefers first change' 'test_cmp a1 expect'\n \n+test_expect_success 'rerere updates preimage timestamp' '\n+\tgit reset --hard &&\n+\toldmtime=$(test-chmtime -v -42 $rr/preimage |cut -f 1) &&\n+\ttest_must_fail git pull . first &&\n+\tnewmtime=$(test-chmtime -v +0 $rr/preimage |cut -f 1) &&\n+\ttest $oldmtime -lt $newmtime\n+'\n+\n rm $rr/postimage\n echo \"$sha1\ta1\" | perl -pe 'y/\\012/\\000/' > .git/MERGE_RR\n \n-- \n1.7.2.rc0.54.g4d821\n"},{"id":"144578","messageId":"4C2C69AF.4010303@viscovery.net","threadId":"24231","inReplyTo":"1277977009-22780-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v2] rerere: fix overeager gc","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-07-01T10:10:55Z","receivedAt":"2010-07-01T10:10:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 7/1/2010 11:36, schrieb SZEDER Gábor:\n> -static time_t rerere_created_at(const char *name)\n> +static time_t rerere_last_checked_at(const char *name)\n\nrerere_last_used_at?\n\n> @@ -378,7 +378,13 @@ static int merge(const char *name, const char *path)\n>  \t}\n>  \tret = ll_merge(&result, path, &base, NULL, &cur, \"\", &other, \"\", 0);\n>  \tif (!ret) {\n> -\t\tFILE *f = fopen(path, \"w\");\n> +\t\tFILE *f;\n> +\n> +\t\tif (utime(rerere_path(name, \"preimage\"), NULL) < 0)\n> +\t\t\twarning(\"failed utime() on %s: %s\",\n> +\t\t\t\t\trerere_path(name, \"preimage\"),\n> +\t\t\t\t\tstrerror(errno));\n> +\t\tf = fopen(path, \"w\");\n\nI think this should be outside of 'if (!ret)' condition because even if\nthe merge fails, the resolution was *used*.\n\nMental note: update mingw_utime to accept NULL for the second parameter...\n\n-- Hannes\n"},{"id":"144586","messageId":"1277982466-29601-1-git-send-email-szeder@ira.uka.de","threadId":"24231","inReplyTo":"4C2C69AF.4010303@viscovery.net","subject":"[PATCH 1/2] mingw: utime() handles NULL times parameter","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-01T11:07:45Z","receivedAt":"2010-07-01T11:07:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"POSIX sayeth:\n\n  \"If times is a null pointer, the access and modification\n   times of the file shall be set to the current time.\"\n\nLet's do so.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nOn Thu, Jul 01, 2010 at 12:10:55PM +0200, Johannes Sixt wrote:\n> Mental note: update mingw_utime to accept NULL for the second parameter...\n\nHere it is, but I don't have mingw, so it's completely untested.\n\n compat/mingw.c |   10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 9a8e336..a54db74 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -304,8 +304,14 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tgoto revert_attrs;\n \t}\n \n-\ttime_t_to_filetime(times->modtime, &mft);\n-\ttime_t_to_filetime(times->actime, &aft);\n+\tif (times) {\n+\t\ttime_t_to_filetime(times->modtime, &mft);\n+\t\ttime_t_to_filetime(times->actime, &aft);\n+\t} else {\n+\t\tGetSystemTimeAsFileTime(&mft);\n+\t\taft->dwLowDateTime = mft->dwLowDateTime;\n+\t\taft->dwHighDateTime = mft->dwHighDateTime;\n+\t}\n \tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n \t\terrno = EINVAL;\n \t\trc = -1;\n-- \n1.7.2.rc0.54.g4d821\n"},{"id":"144585","messageId":"1277982466-29601-2-git-send-email-szeder@ira.uka.de","threadId":"24231","inReplyTo":"4C2C69AF.4010303@viscovery.net","subject":"[PATCH 2/2] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-01T11:07:46Z","receivedAt":"2010-07-01T11:07:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'rerere gc' prunes resolutions of conflicted merges that occurred long\ntime ago, and when doing so it takes the creation time of the\nconflicted automerge results into account.  This can cause the loss of\nfrequently used merge resolutions (e.g. long-living topic branches are\nmerged into a regularly rebuilt integration branch (think of git's\npu)) when they become old enough to exceed 'rerere gc's threshold.\n\nPrevent the loss of valuable merge resolutions by updating the\ntimestamp of the conflicted automerge result each time when\nencountering the same merge conflict.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nOn Thu, Jul 01, 2010 at 12:10:55PM +0200, Johannes Sixt wrote:\n> rerere_last_used_at?\n\n> I think this should be outside of 'if (!ret)' condition because even if\n> the merge fails, the resolution was *used*.\n\nRight on both points.\n\n\n builtin/rerere.c  |    4 ++--\n rerere.c          |    4 ++++\n t/t4200-rerere.sh |    8 ++++++++\n 3 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 980d542..03434a7 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -13,7 +13,7 @@ static const char git_rerere_usage[] =\n static int cutoff_noresolve = 15;\n static int cutoff_resolve = 60;\n \n-static time_t rerere_created_at(const char *name)\n+static time_t rerere_last_used_at(const char *name)\n {\n \tstruct stat st;\n \treturn stat(rerere_path(name, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n@@ -53,7 +53,7 @@ static void garbage_collect(struct string_list *rr)\n \twhile ((e = readdir(dir))) {\n \t\tif (is_dot_or_dotdot(e->d_name))\n \t\t\tcontinue;\n-\t\tthen = rerere_created_at(e->d_name);\n+\t\tthen = rerere_last_used_at(e->d_name);\n \t\tif (!then)\n \t\t\tcontinue;\n \t\tcutoff = (has_rerere_resolution(e->d_name)\ndiff --git a/rerere.c b/rerere.c\nindex d03a696..0d8a167 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -389,6 +389,10 @@ static int merge(const char *name, const char *path)\n \t\t\t\t     strerror(errno));\n \t}\n \n+\tif (utime(rerere_path(name, \"preimage\"), NULL) < 0)\n+\t\twarning(\"failed utime() on %s: %s\",\n+\t\t\t\trerere_path(name, \"preimage\"), strerror(errno));\n+\n out:\n \tfree(cur.ptr);\n \tfree(base.ptr);\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 70856d0..c01d930 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -144,6 +144,14 @@ test_expect_success 'rerere kicked in' \"! grep ^=======$ a1\"\n \n test_expect_success 'rerere prefers first change' 'test_cmp a1 expect'\n \n+test_expect_success 'rerere updates preimage timestamp' '\n+\tgit reset --hard &&\n+\toldmtime=$(test-chmtime -v -42 $rr/preimage |cut -f 1) &&\n+\ttest_must_fail git pull . first &&\n+\tnewmtime=$(test-chmtime -v +0 $rr/preimage |cut -f 1) &&\n+\ttest $oldmtime -lt $newmtime\n+'\n+\n rm $rr/postimage\n echo \"$sha1\ta1\" | perl -pe 'y/\\012/\\000/' > .git/MERGE_RR\n \n-- \n1.7.2.rc0.54.g4d821\n"},{"id":"144588","messageId":"4C2C8718.5080203@viscovery.net","threadId":"24231","inReplyTo":"1277982466-29601-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 1/2 fixed] mingw_utime(): handle NULL times parameter","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-07-01T12:16:24Z","receivedAt":"2010-07-01T12:16:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: SZEDER Gábor <szeder@ira.uka.de>\n\nPOSIX sayeth:\n\n  \"If times is a null pointer, the access and modification\n   times of the file shall be set to the current time.\"\n\nLet's do so.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n >> Mental note: update mingw_utime to accept NULL for the second\n >> parameter...\n > \n > Here it is, but I don't have mingw, so it's completely untested.\n\n Thanks. Here is a version that compiles; the interdiff is\n\n @@ -309,8 +309,7 @@\n                 time_t_to_filetime(times->actime, &aft);\n         } else {\n                 GetSystemTimeAsFileTime(&mft);\n -               aft->dwLowDateTime = mft->dwLowDateTime;\n -               aft->dwHighDateTime = mft->dwHighDateTime;\n +               aft = mft;\n         }\n         if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n                 errno = EINVAL;\n\n With this, the series passes the test suite on MinGW.\n\n -- Hannes\n\n compat/mingw.c |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0722a6d..b6f0a7f 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -304,8 +304,13 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tgoto revert_attrs;\n \t}\n \n-\ttime_t_to_filetime(times->modtime, &mft);\n-\ttime_t_to_filetime(times->actime, &aft);\n+\tif (times) {\n+\t\ttime_t_to_filetime(times->modtime, &mft);\n+\t\ttime_t_to_filetime(times->actime, &aft);\n+\t} else {\n+\t\tGetSystemTimeAsFileTime(&mft);\n+\t\taft = mft;\n+\t}\n \tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n \t\terrno = EINVAL;\n \t\trc = -1;\n-- \n1.7.2.rc1.1057.g1270\n"},{"id":"144609","messageId":"7v1vbn417d.fsf@alter.siamese.dyndns.org","threadId":"24231","inReplyTo":"4C2AE04E.9090901@viscovery.net","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-01T16:27:18Z","receivedAt":"2010-07-01T16:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Would it be possible to update the timestamp of preimage every time it is\n> used (e.g., in rerere.c:merge()), and check for that?\n\nIt would be _possible_, but we are _not_ modifying the file at that point,\nso somehow that solution feels very wrong, no?\n"},{"id":"144664","messageId":"4C2D7DF7.8030408@viscovery.net","threadId":"24231","inReplyTo":"7v1vbn417d.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-07-02T05:49:43Z","receivedAt":"2010-07-02T05:49:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 7/1/2010 18:27, schrieb Junio C Hamano:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> \n>> Would it be possible to update the timestamp of preimage every time it is\n>> used (e.g., in rerere.c:merge()), and check for that?\n> \n> It would be _possible_, but we are _not_ modifying the file at that point,\n> so somehow that solution feels very wrong, no?\n\nrr-cache is basically a static database. The fact that we have a file\nnamed 'thisimage' is just an abuse - to put a temporary file somewhere.\nTherefore, depending on lockfile infrastructure to change timestamps for\nus while manipulating 'thisimage' should feel no less wrong, don't you\nthink so?\n\n-- Hannes\n"},{"id":"144696","messageId":"7vmxu923up.fsf@alter.siamese.dyndns.org","threadId":"24231","inReplyTo":"4C2D7DF7.8030408@viscovery.net","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-02T17:25:18Z","receivedAt":"2010-07-02T17:25:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 7/1/2010 18:27, schrieb Junio C Hamano:\n>> Johannes Sixt <j.sixt@viscovery.net> writes:\n>> \n>>> Would it be possible to update the timestamp of preimage every time it is\n>>> used (e.g., in rerere.c:merge()), and check for that?\n>> \n>> It would be _possible_, but we are _not_ modifying the file at that point,\n>> so somehow that solution feels very wrong, no?\n>\n> rr-cache is basically a static database. The fact that we have a file\n> named 'thisimage' is just an abuse - to put a temporary file somewhere.\n> Therefore, depending on lockfile infrastructure to change timestamps for\n> us while manipulating 'thisimage' should feel no less wrong, don't you\n> think so?\n\nNot really.  \n\n\"This\" in \"thisimage\" stands for \"image involved in conflict we are\nlooking at _this moment_\", and using the per-conflict-id directory to\nhouse that file was a conscious design decision.  Because you are trying\nto record a new data item (namely \"the last time we saw this conflict\")\nsomewhere in the rr-cache database, using the timestamp of that directory\nto tell when was the last time that _this moment_ was makes sense, at\nleast to me.\n\nIf anything, \"preimage\" that has newer timestamp than \"postimage\" feels\nwrong, as you would record pre, think for a while how the corresponding\npost should look like, and then record post.\n\nIf we for whatever reason trust placing an extra timestamp on a regular\nfile more than using directory timestamp (which I think may be a valid\nconcern from portability point of view), I'd rather see \"preimage\"\ntimestamp to keep track of the time when we _first_ encountered the\nparticular conflict, and \"postimage\" used for recording the time when we\nsaw the conflict most recently.\n"},{"id":"144796","messageId":"4C31757A.1000207@viscovery.net","threadId":"24231","inReplyTo":"7vmxu923up.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-07-05T06:02:34Z","receivedAt":"2010-07-05T06:02:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 7/2/2010 19:25, schrieb Junio C Hamano:\n> If anything, \"preimage\" that has newer timestamp than \"postimage\" feels\n> wrong,...\n\nIndeed; I agree.\n\n> If we for whatever reason trust placing an extra timestamp on a regular\n> file more than using directory timestamp (which I think may be a valid\n> concern from portability point of view),\n\nWindows behaves well in this regard. Writing of thisimage must be\nconverted to lockfile infrastructure, of course.\n\n> I'd rather see \"preimage\"\n> timestamp to keep track of the time when we _first_ encountered the\n> particular conflict, and \"postimage\" used for recording the time when we\n> saw the conflict most recently.\n\nThat would be fine, too.\n\n-- Hannes\n"},{"id":"145091","messageId":"4C356FB4.6060308@viscovery.net","threadId":"24231","inReplyTo":"7vmxu923up.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-07-08T06:27:00Z","receivedAt":"2010-07-08T06:27:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Instead of this series, I'll use this for a while; i.e., check the\ntimestamp of thisimage, and if it does not exist, check the directory's\ntimestamp. It doesn't pass t4200.20 because the test doesn't set the\ntimestamps of thisimage and the directory into the past.\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 980d542..49fcbf7 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -13,10 +13,14 @@ static const char git_rerere_usage[] =\n static int cutoff_noresolve = 15;\n static int cutoff_resolve = 60;\n \n-static time_t rerere_created_at(const char *name)\n+static time_t rerere_last_used_at(const char *name)\n {\n \tstruct stat st;\n-\treturn stat(rerere_path(name, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n+\tif (!stat(rerere_path(name, \"thisimage\"), &st))\n+\t\treturn st.st_mtime;\n+\tif (errno == ENOENT && !stat(rerere_path(name, \".\"), &st))\n+\t\treturn st.st_mtime;\n+\treturn (time_t) 0;\n }\n \n static void unlink_rr_item(const char *name)\n@@ -53,7 +57,7 @@ static void garbage_collect(struct string_list *rr)\n \twhile ((e = readdir(dir))) {\n \t\tif (is_dot_or_dotdot(e->d_name))\n \t\t\tcontinue;\n-\t\tthen = rerere_created_at(e->d_name);\n+\t\tthen = rerere_last_used_at(e->d_name);\n \t\tif (!then)\n \t\t\tcontinue;\n \t\tcutoff = (has_rerere_resolution(e->d_name)\n"},{"id":"145131","messageId":"1278599758-18962-1-git-send-email-szeder@ira.uka.de","threadId":"24231","inReplyTo":"4C31757A.1000207@viscovery.net","subject":"[PATCH v4] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-08T14:35:58Z","receivedAt":"2010-07-08T14:35:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'rerere gc' prunes resolutions of conflicted merges that occurred long\ntime ago, and when doing so it takes the creation time of the\nconflicted automerge results into account.  This can cause the loss of\nfrequently used merge resolutions (e.g. long-living topic branches are\nmerged into a regularly rebuilt integration branch (think of git's\npu)) when they become old enough to exceed 'rerere gc's threshold.\n\nTo prevent the loss of valuable merge resolutions 'rerere' will (1)\nwrite 'thisimage' into a temporary file and then rename it into its\nfinal destination, thereby updating the directory timestamp each time\nwhen encountering the same merge conflict, and (2) take the directory\ntimestamp, i.e. the time of the last usage into account when gc'ing.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nOn Mon, Jul 05, 2010 at 08:02:34AM +0200, Johannes Sixt wrote:\n> Am 7/2/2010 19:25, schrieb Junio C Hamano:\n> > If we for whatever reason trust placing an extra timestamp on a regular\n> > file more than using directory timestamp (which I think may be a valid\n> > concern from portability point of view),\n> \n> Windows behaves well in this regard. Writing of thisimage must be\n> converted to lockfile infrastructure, of course.\n\nSo, here is the next take with the tmpfile-rename stuff to update the\ndirectory timestamp.  If there are any portability issues wrt directory\ntimestamp updating, the new test should catch them early.\n\n builtin/rerere.c  |    6 +++---\n rerere.c          |   19 +++++++++++++++++--\n t/t4200-rerere.sh |   16 ++++++++++++----\n 3 files changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 980d542..bede3eb 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -13,10 +13,10 @@ static const char git_rerere_usage[] =\n static int cutoff_noresolve = 15;\n static int cutoff_resolve = 60;\n \n-static time_t rerere_created_at(const char *name)\n+static time_t rerere_last_used_at(const char *name)\n {\n \tstruct stat st;\n-\treturn stat(rerere_path(name, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n+\treturn stat(rerere_path(name, \".\"), &st) ? (time_t) 0 : st.st_mtime;\n }\n \n static void unlink_rr_item(const char *name)\n@@ -53,7 +53,7 @@ static void garbage_collect(struct string_list *rr)\n \twhile ((e = readdir(dir))) {\n \t\tif (is_dot_or_dotdot(e->d_name))\n \t\t\tcontinue;\n-\t\tthen = rerere_created_at(e->d_name);\n+\t\tthen = rerere_last_used_at(e->d_name);\n \t\tif (!then)\n \t\t\tcontinue;\n \t\tcutoff = (has_rerere_resolution(e->d_name)\ndiff --git a/rerere.c b/rerere.c\nindex d03a696..e415f54 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -363,13 +363,28 @@ static int find_conflict(struct string_list *conflict)\n \n static int merge(const char *name, const char *path)\n {\n-\tint ret;\n+\tint fd, ret;\n \tmmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};\n \tmmbuffer_t result = {NULL, 0};\n+\tchar *tmpfile;\n+\n+\ttmpfile = xstrdup(rerere_path(name, \"tmp_thisimage_XXXXXX\"));\n+\tfd = git_mkstemp_mode(tmpfile, 0600);\n+\tif (fd < 0) {\n+\t\terror(\"unable to create temporary file %s for rerere: %s\\n\",\n+\t\t\t\ttmpfile, strerror(errno));\n+\t\treturn 1;\n+\t}\n+\tclose(fd);\n \n-\tif (handle_file(path, NULL, rerere_path(name, \"thisimage\")) < 0)\n+\tif (handle_file(path, NULL, tmpfile) < 0)\n \t\treturn 1;\n \n+\tif (move_temp_to_file(tmpfile, rerere_path(name, \"thisimage\")) < 0)\n+\t\treturn 1;\n+\n+\tfree(tmpfile);\n+\n \tif (read_mmfile(&cur, rerere_path(name, \"thisimage\")) ||\n \t\t\tread_mmfile(&base, rerere_path(name, \"preimage\")) ||\n \t\t\tread_mmfile(&other, rerere_path(name, \"postimage\"))) {\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 70856d0..a425329 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -144,6 +144,14 @@ test_expect_success 'rerere kicked in' \"! grep ^=======$ a1\"\n \n test_expect_success 'rerere prefers first change' 'test_cmp a1 expect'\n \n+test_expect_success 'rerere updates directory timestamp' '\n+\tgit reset --hard &&\n+\toldmtime=$(test-chmtime -v -42 $rr |cut -f 1) &&\n+\ttest_must_fail git pull . first &&\n+\tnewmtime=$(test-chmtime -v +0 $rr |cut -f 1) &&\n+\ttest $oldmtime -lt $newmtime\n+'\n+\n rm $rr/postimage\n echo \"$sha1\ta1\" | perl -pe 'y/\\012/\\000/' > .git/MERGE_RR\n \n@@ -165,16 +173,16 @@ just_over_15_days_ago=$((-1-15*86400))\n almost_60_days_ago=$((60-60*86400))\n just_over_60_days_ago=$((-1-60*86400))\n \n-test-chmtime =$almost_60_days_ago $rr/preimage\n-test-chmtime =$almost_15_days_ago $rr2/preimage\n+test-chmtime =$almost_60_days_ago $rr\n+test-chmtime =$almost_15_days_ago $rr2\n \n test_expect_success 'garbage collection (part1)' 'git rerere gc'\n \n test_expect_success 'young records still live' \\\n \t\"test -f $rr/preimage && test -f $rr2/preimage\"\n \n-test-chmtime =$just_over_60_days_ago $rr/preimage\n-test-chmtime =$just_over_15_days_ago $rr2/preimage\n+test-chmtime =$just_over_60_days_ago $rr\n+test-chmtime =$just_over_15_days_ago $rr2\n \n test_expect_success 'garbage collection (part2)' 'git rerere gc'\n \n-- \n1.7.2.rc2.42.gfe876\n"},{"id":"145173","messageId":"7vlj9lo6w6.fsf@alter.siamese.dyndns.org","threadId":"24231","inReplyTo":"4C31757A.1000207@viscovery.net","subject":"Re: [RFC PATCH] rerere: fix overeager gc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-09T00:06:49Z","receivedAt":"2010-07-09T00:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 7/2/2010 19:25, schrieb Junio C Hamano:\n>\n>> I'd rather see \"preimage\"\n>> timestamp to keep track of the time when we _first_ encountered the\n>> particular conflict, and \"postimage\" used for recording the time when we\n>> saw the conflict most recently.\n>\n> That would be fine, too.\n\nOk, then let's make it so ;-)\n"},{"id":"145402","messageId":"1278978124-3035-1-git-send-email-szeder@ira.uka.de","threadId":"24231","inReplyTo":"7vlj9lo6w6.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] mingw_utime(): handle NULL times parameter","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-12T23:42:03Z","receivedAt":"2010-07-12T23:42:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"POSIX sayeth:\n\n  \"If times is a null pointer, the access and modification\n   times of the file shall be set to the current time.\"\n\nLet's do so.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.c |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 9a8e336..24333cb 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -304,8 +304,13 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tgoto revert_attrs;\n \t}\n \n-\ttime_t_to_filetime(times->modtime, &mft);\n-\ttime_t_to_filetime(times->actime, &aft);\n+\tif (times) {\n+\t\ttime_t_to_filetime(times->modtime, &mft);\n+\t\ttime_t_to_filetime(times->actime, &aft);\n+\t} else {\n+\t\tGetSystemTimeAsFileTime(&mft);\n+\t\taft = mft;\n+\t}\n \tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n \t\terrno = EINVAL;\n \t\trc = -1;\n-- \n1.7.2.rc2.37.g5e8ef\n"},{"id":"145403","messageId":"1278978124-3035-2-git-send-email-szeder@ira.uka.de","threadId":"24231","inReplyTo":"7vlj9lo6w6.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-12T23:42:04Z","receivedAt":"2010-07-12T23:42:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'rerere gc' prunes resolutions of conflicted merges that occurred long\ntime ago, and when doing so it takes the creation time of the\nconflicted automerge results into account.  This can cause the loss of\nfrequently used conflict resolutions (e.g. long-living topic branches\nare merged into a regularly rebuilt integration branch (think of git's\npu)) when they become old enough to exceed 'rerere gc's threshold.\n\nTo prevent the loss of valuable merge resolutions 'rerere' will (1)\nupdate the timestamp of the recorded conflict resolution (i.e.\n'postimage') each time when encountering and resolving the same merge\nconflict, and (2) take this timestamp, i.e. the time of the last usage\ninto account when gc'ing.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nOn Thu, Jul 08, 2010 at 05:06:49PM -0700, Junio C Hamano wrote:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> \n> > Am 7/2/2010 19:25, schrieb Junio C Hamano:\n> >\n> >> I'd rather see \"preimage\"\n> >> timestamp to keep track of the time when we _first_ encountered the\n> >> particular conflict, and \"postimage\" used for recording the time when we\n> >> saw the conflict most recently.\n> >\n> > That would be fine, too.\n> \n> Ok, then let's make it so ;-)\n\nHere it is.  Now you have it all, take your pick (;\n\n builtin/rerere.c  |   15 +++++++++++++--\n rerere.c          |    8 +++++++-\n t/t4200-rerere.sh |   14 +++++++++++---\n 3 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 980d542..52e4b64 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -19,6 +19,12 @@ static time_t rerere_created_at(const char *name)\n \treturn stat(rerere_path(name, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n }\n \n+static time_t rerere_last_used_at(const char *name)\n+{\n+\tstruct stat st;\n+\treturn stat(rerere_path(name, \"postimage\"), &st) ? (time_t) 0 : st.st_mtime;\n+}\n+\n static void unlink_rr_item(const char *name)\n {\n \tunlink(rerere_path(name, \"thisimage\"));\n@@ -56,8 +62,13 @@ static void garbage_collect(struct string_list *rr)\n \t\tthen = rerere_created_at(e->d_name);\n \t\tif (!then)\n \t\t\tcontinue;\n-\t\tcutoff = (has_rerere_resolution(e->d_name)\n-\t\t\t  ? cutoff_resolve : cutoff_noresolve);\n+\t\tif (has_rerere_resolution(e->d_name)) {\n+\t\t\tthen = rerere_last_used_at(e->d_name);\n+\t\t\tif (!then)\n+\t\t\t\tcontinue;\n+\t\t\tcutoff = cutoff_resolve;\n+\t\t} else\n+\t\t\tcutoff = cutoff_noresolve;\n \t\tif (then < now - cutoff * 86400)\n \t\t\tstring_list_append(&to_remove, e->d_name);\n \t}\ndiff --git a/rerere.c b/rerere.c\nindex d03a696..1d95161 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -378,7 +378,13 @@ static int merge(const char *name, const char *path)\n \t}\n \tret = ll_merge(&result, path, &base, NULL, &cur, \"\", &other, \"\", 0);\n \tif (!ret) {\n-\t\tFILE *f = fopen(path, \"w\");\n+\t\tFILE *f;\n+\n+\t\tif (utime(rerere_path(name, \"postimage\"), NULL) < 0)\n+\t\t\twarning(\"failed utime() on %s: %s\",\n+\t\t\t\t\trerere_path(name, \"postimage\"),\n+\t\t\t\t\tstrerror(errno));\n+\t\tf = fopen(path, \"w\");\n \t\tif (!f)\n \t\t\treturn error(\"Could not open %s: %s\", path,\n \t\t\t\t     strerror(errno));\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 70856d0..093b138 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -132,6 +132,8 @@ test_expect_success 'commit succeeds' \\\n \n test_expect_success 'recorded postimage' \"test -f $rr/postimage\"\n \n+oldmtimepost=$(test-chmtime -v -60 $rr/postimage |cut -f 1)\n+\n test_expect_success 'another conflicting merge' '\n \tgit checkout -b third master &&\n \tgit show second^:a1 | sed \"s/To die: t/To die! T/\" > a1 &&\n@@ -144,6 +146,11 @@ test_expect_success 'rerere kicked in' \"! grep ^=======$ a1\"\n \n test_expect_success 'rerere prefers first change' 'test_cmp a1 expect'\n \n+test_expect_success 'rerere updates postimage timestamp' '\n+\tnewmtimepost=$(test-chmtime -v +0 $rr/postimage |cut -f 1) &&\n+\ttest $oldmtimepost -lt $newmtimepost\n+'\n+\n rm $rr/postimage\n echo \"$sha1\ta1\" | perl -pe 'y/\\012/\\000/' > .git/MERGE_RR\n \n@@ -165,15 +172,16 @@ just_over_15_days_ago=$((-1-15*86400))\n almost_60_days_ago=$((60-60*86400))\n just_over_60_days_ago=$((-1-60*86400))\n \n-test-chmtime =$almost_60_days_ago $rr/preimage\n+test-chmtime =$just_over_60_days_ago $rr/preimage\n+test-chmtime =$almost_60_days_ago $rr/postimage\n test-chmtime =$almost_15_days_ago $rr2/preimage\n \n test_expect_success 'garbage collection (part1)' 'git rerere gc'\n \n-test_expect_success 'young records still live' \\\n+test_expect_success 'young or recently used records still live' \\\n \t\"test -f $rr/preimage && test -f $rr2/preimage\"\n \n-test-chmtime =$just_over_60_days_ago $rr/preimage\n+test-chmtime =$just_over_60_days_ago $rr/postimage\n test-chmtime =$just_over_15_days_ago $rr2/preimage\n \n test_expect_success 'garbage collection (part2)' 'git rerere gc'\n-- \n1.7.2.rc2.37.g5e8ef\n"},{"id":"145408","messageId":"7v630k6wpg.fsf@alter.siamese.dyndns.org","threadId":"24231","inReplyTo":"1278978124-3035-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 2/2] rerere: fix overeager gc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-13T00:40:11Z","receivedAt":"2010-07-13T00:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder@ira.uka.de> writes:\n\n> 'rerere gc' prunes resolutions of conflicted merges that occurred long\n> time ago, and when doing so it takes the creation time of the\n> conflicted automerge results into account.  This can cause the loss of\n> frequently used conflict resolutions (e.g. long-living topic branches\n> are merged into a regularly rebuilt integration branch (think of git's\n> pu)) when they become old enough to exceed 'rerere gc's threshold.\n>\n> To prevent the loss of valuable merge resolutions 'rerere' will (1)\n> update the timestamp of the recorded conflict resolution (i.e.\n> 'postimage') each time when encountering and resolving the same merge\n> conflict, and (2) take this timestamp, i.e. the time of the last usage\n> into account when gc'ing.\n\nThanks.\n\n> +static time_t rerere_last_used_at(const char *name)\n> +{\n> +\tstruct stat st;\n> +\treturn stat(rerere_path(name, \"postimage\"), &st) ? (time_t) 0 : st.st_mtime;\n> +}\n\nDoesn't has_rerere_resolution() already do a stat on this path?  There are\nonly two allers of the function so it would probably make sense to pass a\npointer to struct stat from the caller to avoid one extra call to stat.\n"},{"id":"145531","messageId":"20100714121925.GG15270@neumann","threadId":"24231","inReplyTo":"7v630k6wpg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-14T12:19:25Z","receivedAt":"2010-07-14T12:19:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jul 12, 2010 at 05:40:11PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor <szeder@ira.uka.de> writes:\n> > +static time_t rerere_last_used_at(const char *name)\n> > +{\n> > +\tstruct stat st;\n> > +\treturn stat(rerere_path(name, \"postimage\"), &st) ? (time_t) 0 : st.st_mtime;\n> > +}\n> \n> Doesn't has_rerere_resolution() already do a stat on this path?  There are\n> only two allers of the function so it would probably make sense to pass a\n> pointer to struct stat from the caller to avoid one extra call to stat.\n\nrerere_last_used_at() returns 0 when the stat() on 'postimage' fails,\nexactly like has_rerere_resolution().  Consequently, we can use\nrerere_last_used_at() to determine whether a resolution exists, too.\nAnd if we do that, we dont have to \"pollute\" callers of\nhas_rerere_resolution() with superfluous struct stat variables.\n\nSo how about this squashed in instead?\n\n-- >8 --\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 52e4b64..1dc424b 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -50,7 +50,7 @@ static void garbage_collect(struct string_list *rr)\n \tDIR *dir;\n \tstruct dirent *e;\n \tint i, cutoff;\n-\ttime_t now = time(NULL), then;\n+\ttime_t now = time(NULL), then, then_post;\n \n \tgit_config(git_rerere_gc_config, NULL);\n \tdir = opendir(git_path(\"rr-cache\"));\n@@ -62,10 +62,9 @@ static void garbage_collect(struct string_list *rr)\n \t\tthen = rerere_created_at(e->d_name);\n \t\tif (!then)\n \t\t\tcontinue;\n-\t\tif (has_rerere_resolution(e->d_name)) {\n-\t\t\tthen = rerere_last_used_at(e->d_name);\n-\t\t\tif (!then)\n-\t\t\t\tcontinue;\n+\t\tthen_post = rerere_last_used_at(e->d_name);\n+\t\tif (then_post) {\n+\t\t\tthen = then_post;\n \t\t\tcutoff = cutoff_resolve;\n \t\t} else\n \t\t\tcutoff = cutoff_noresolve;\n"},{"id":"145553","messageId":"7v1vb6yqve.fsf@alter.siamese.dyndns.org","threadId":"24231","inReplyTo":"20100714121925.GG15270@neumann","subject":"Re: [PATCH 2/2] rerere: fix overeager gc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-14T16:23:17Z","receivedAt":"2010-07-14T16:23:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder@ira.uka.de> writes:\n\n> On Mon, Jul 12, 2010 at 05:40:11PM -0700, Junio C Hamano wrote:\n>> SZEDER Gábor <szeder@ira.uka.de> writes:\n>> > +static time_t rerere_last_used_at(const char *name)\n>> > +{\n>> > +\tstruct stat st;\n>> > +\treturn stat(rerere_path(name, \"postimage\"), &st) ? (time_t) 0 : st.st_mtime;\n>> > +}\n>> \n>> Doesn't has_rerere_resolution() already do a stat on this path?  There are\n>> only two allers of the function so it would probably make sense to pass a\n>> pointer to struct stat from the caller to avoid one extra call to stat.\n>\n> rerere_last_used_at() returns 0 when the stat() on 'postimage' fails,\n> exactly like has_rerere_resolution().  Consequently, we can use\n> rerere_last_used_at() to determine whether a resolution exists, too.\n\n\"When was this last used?\" is answered with \"infinitely long time ago\" for\nan item that does not exist.  Between the ones that have never been\nresolved (i.e. no \"postimage\") and the ones that have not been used for a\nlong time, there is no material difference in the expiration, as long as\n\"long time\" exceeds rerere-unresolved.  Ok; I can see the logic in that.\n\nI wonder if swapping the order of things to check may make it easier to\nread, though.  If it was used, we want to compare gc.rerereresolved with\nthat timestamp, and otherwise we want to compare gc.rerereunresolved with\nthe timestamp of the creation.  I.e. something like this...\n\n builtin/rerere.c |   14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 7e45afe..6d1b580 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -59,16 +59,16 @@ static void garbage_collect(struct string_list *rr)\n \twhile ((e = readdir(dir))) {\n \t\tif (is_dot_or_dotdot(e->d_name))\n \t\t\tcontinue;\n-\t\tthen = rerere_created_at(e->d_name);\n-\t\tif (!then)\n-\t\t\tcontinue;\n-\t\tif (has_rerere_resolution(e->d_name)) {\n-\t\t\tthen = rerere_last_used_at(e->d_name);\n+\n+\t\tthen = rerere_last_used_at(e->d_name);\n+\t\tif (then) {\n+\t\t\tcutoff = cutoff_resolve;\n+\t\t} else {\n+\t\t\tthen = rerere_created_at(e->d_name);\n \t\t\tif (!then)\n \t\t\t\tcontinue;\n-\t\t\tcutoff = cutoff_resolve;\n-\t\t} else\n \t\t\tcutoff = cutoff_noresolve;\n+\t\t}\n \t\tif (then < now - cutoff * 86400)\n \t\t\tstring_list_append(e->d_name, &to_remove);\n \t}\n"},{"id":"145574","messageId":"20100714183317.GI15270@neumann","threadId":"24231","inReplyTo":"7v1vb6yqve.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] rerere: fix overeager gc","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2010-07-14T18:33:17Z","receivedAt":"2010-07-14T18:33:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jul 14, 2010 at 09:23:17AM -0700, Junio C Hamano wrote:\n> I wonder if swapping the order of things to check may make it easier to\n> read, though.  If it was used, we want to compare gc.rerereresolved with\n> that timestamp, and otherwise we want to compare gc.rerereunresolved with\n> the timestamp of the creation.  I.e. something like this...\n\nThat's even better.\n\n\n>  builtin/rerere.c |   14 +++++++-------\n>  1 files changed, 7 insertions(+), 7 deletions(-)\n> \n> diff --git a/builtin/rerere.c b/builtin/rerere.c\n> index 7e45afe..6d1b580 100644\n> --- a/builtin/rerere.c\n> +++ b/builtin/rerere.c\n> @@ -59,16 +59,16 @@ static void garbage_collect(struct string_list *rr)\n>  \twhile ((e = readdir(dir))) {\n>  \t\tif (is_dot_or_dotdot(e->d_name))\n>  \t\t\tcontinue;\n> -\t\tthen = rerere_created_at(e->d_name);\n> -\t\tif (!then)\n> -\t\t\tcontinue;\n> -\t\tif (has_rerere_resolution(e->d_name)) {\n> -\t\t\tthen = rerere_last_used_at(e->d_name);\n> +\n> +\t\tthen = rerere_last_used_at(e->d_name);\n> +\t\tif (then) {\n> +\t\t\tcutoff = cutoff_resolve;\n> +\t\t} else {\n> +\t\t\tthen = rerere_created_at(e->d_name);\n>  \t\t\tif (!then)\n>  \t\t\t\tcontinue;\n> -\t\t\tcutoff = cutoff_resolve;\n> -\t\t} else\n>  \t\t\tcutoff = cutoff_noresolve;\n> +\t\t}\n>  \t\tif (then < now - cutoff * 86400)\n>  \t\t\tstring_list_append(e->d_name, &to_remove);\n>  \t}\n> \n> \n"}]}