{"thread":{"id":"38113","subject":"[PATCH 01/23] refs.c: make ref_transaction_create a wrapper for ref_transaction_update","startedAt":"2014-12-04T23:08:12Z","lastAt":"2014-12-12T18:57:15Z","messageCount":63,"participants":["Michael Haggerty","Jonathan Nieder","Junio C Hamano","Stefan Beller","ronnie sahlberg","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":23},"messages":[{"id":"253107","messageId":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":null,"subject":"[PATCH 00/23] Add reflog_expire() to the references API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:12Z","receivedAt":"2014-12-04T23:08:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I propose this patch series as an alternative to Ronnie's \"reflog\ntransactions\" series.\n\nBoth alternatives have been discussed exhaustively on the list [1,2].\nMy opinion is that there is no need to allow arbitrary reflog changes\nvia the ref_transaction API, because just about the only things we\nwant to do are\n\n* Append an entry to a reflog when a reference is updated. This should\n (and is already) done as a side effect of the reference update.\n\n* Expire or delete old reflog entries based on relatively complicated\n logic, possibly repairing the remaining entries so as to preserve\n the continuity of the reflog chain.\n\nThis patch series shows that the latter can be done with a single,\nfairly simple, purpose-made function, expire_reflog(), in the\nreferences API. The policy for what reflog entries should be expired\nis specified by the caller via three callback functions that don't\nhave to know anything about how reflogs are stored. The locking,\niteration, repair, and writing is implemented within the references\nmodule, in a function that can easily be swapped out when pluggable\nreference backends are implemented.\n\nThe remaining reflog operations (enabling/disabling reflogs for a\nreference, renaming the reflog when a reference is renamed) are not\nespecially difficult but will be brought into the same framework in a\nfuture patch series.\n\nThe first few patches and the last few are taken from Ronnie's and\nStefan's work. I chose *not* to rename the ref_transaction functions\nfor obvious reasons. A couple of the later patches from their series\nwould make sense but are not duplicated here.\n\nThis branch is also available on GitHub:\n\n https://github.com/mhagger/git.git, branch reflog-expire-api-v1\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770\n[2] http://thread.gmane.org/gmane.comp.version-control.git/260731/focus=260767\n\nMichael Haggerty (17):\n expire_reflog(): remove unused parameter\n expire_reflog(): rename \"ref\" parameter to \"refname\"\n expire_reflog(): exit early if the reference has no reflog\n expire_reflog(): use a lock_file for rewriting the reflog file\n Extract function should_expire_reflog_ent()\n expire_reflog(): extract two policy-related functions\n expire_reflog(): add a \"flags\" argument\n expire_reflog(): move dry_run to flags argument\n expire_reflog(): move updateref to flags argument\n Rename expire_reflog_cb to expire_reflog_policy_cb\n struct expire_reflog_cb: a new callback data type\n expire_reflog(): pass flags through to expire_reflog_ent()\n expire_reflog(): move verbose to flags argument\n expire_reflog(): move rewrite to flags argument\n Move newlog and last_kept_sha1 to \"struct expire_reflog_cb\"\n expire_reflog(): treat the policy callback data as opaque\n reflog_expire(): new function in the reference API\n\nRonnie Sahlberg (5):\n refs.c: make ref_transaction_create a wrapper for\n ref_transaction_update\n refs.c: make ref_transaction_delete a wrapper for\n ref_transaction_update\n refs.c: add a function to append a reflog entry to a fd\n refs.c: remove unlock_ref/close_ref/commit_ref from the refs api\n lock_any_ref_for_update(): inline function\n\nStefan Beller (1):\n refs.c: don't expose the internal struct ref_lock in the header file\n\n builtin/reflog.c | 259 ++++++++++++++++++++++---------------------------------\n refs.c | 251 +++++++++++++++++++++++++++++++++++------------------\n refs.h | 74 ++++++++++------\n 3 files changed, 319 insertions(+), 265 deletions(-)\n\n-- \n2.1.3\n"},{"id":"253105","messageId":"1417734515-11812-2-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/23] refs.c: make ref_transaction_create a wrapper for ref_transaction_update","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:13Z","receivedAt":"2014-12-04T23:08:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Ronnie Sahlberg \n\nThe ref_transaction_update function can already be used to create refs by\npassing null_sha1 as the old_sha1 parameter. Simplify by replacing\ntransaction_create with a thin wrapper.\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \nReviewed-by: Michael Haggerty \nReviewed-by: Jonathan Nieder \nSigned-off-by: Michael Haggerty \n---\n refs.c | 27 ++-------------------------\n 1 file changed, 2 insertions(+), 25 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..005eb18 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t int flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n-\tstruct ref_update *update;\n-\n-\tassert(err);\n-\n-\tif (transaction->state != REF_TRANSACTION_OPEN)\n-\t\tdie(\"BUG: create called for transaction that is not open\");\n-\n-\tif (!new_sha1 || is_null_sha1(new_sha1))\n-\t\tdie(\"BUG: create ref with null new_sha1\");\n-\n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\tstrbuf_addf(err, \"refusing to create ref with bad name %s\",\n-\t\t\t refname);\n-\t\treturn -1;\n-\t}\n-\n-\tupdate = add_update(transaction, refname);\n-\n-\thashcpy(update->new_sha1, new_sha1);\n-\thashclr(update->old_sha1);\n-\tupdate->flags = flags;\n-\tupdate->have_old = 1;\n-\tif (msg)\n-\t\tupdate->msg = xstrdup(msg);\n-\treturn 0;\n+\treturn ref_transaction_update(transaction, refname, new_sha1,\n+\t\t\t\t null_sha1, flags, 1, msg, err);\n }\n \n int ref_transaction_delete(struct ref_transaction *transaction,\n-- \n2.1.3\n"},{"id":"253108","messageId":"1417734515-11812-3-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/23] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:14Z","receivedAt":"2014-12-04T23:08:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Ronnie Sahlberg \n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \nReviewed-by: Michael Haggerty \nReviewed-by: Jonathan Nieder \nSigned-off-by: Michael Haggerty \n---\n refs.c | 22 ++--------------------\n refs.h | 2 +-\n 2 files changed, 3 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 005eb18..05cb299 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err)\n {\n-\tstruct ref_update *update;\n-\n-\tassert(err);\n-\n-\tif (transaction->state != REF_TRANSACTION_OPEN)\n-\t\tdie(\"BUG: delete called for transaction that is not open\");\n-\n-\tif (have_old && !old_sha1)\n-\t\tdie(\"BUG: have_old is true but old_sha1 is NULL\");\n-\n-\tupdate = add_update(transaction, refname);\n-\tupdate->flags = flags;\n-\tupdate->have_old = have_old;\n-\tif (have_old) {\n-\t\tassert(!is_null_sha1(old_sha1));\n-\t\thashcpy(update->old_sha1, old_sha1);\n-\t}\n-\tif (msg)\n-\t\tupdate->msg = xstrdup(msg);\n-\treturn 0;\n+\treturn ref_transaction_update(transaction, refname, null_sha1,\n+\t\t\t\t old_sha1, flags, have_old, msg, err);\n }\n \n int update_ref(const char *action, const char *refname,\ndiff --git a/refs.h b/refs.h\nindex 2bc3556..7d675b7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -283,7 +283,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \n /*\n * Add a reference update to transaction. new_sha1 is the value that\n- * the reference should have after the update, or zeros if it should\n+ * the reference should have after the update, or null_sha1 if it should\n * be deleted. If have_old is true, then old_sha1 holds the value\n * that the reference should have had before the update, or zeros if\n * it must not have existed beforehand.\n-- \n2.1.3\n"},{"id":"253125","messageId":"1417734515-11812-4-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/23] refs.c: add a function to append a reflog entry to a fd","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:15Z","receivedAt":"2014-12-04T23:08:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Ronnie Sahlberg \n\nBreak out the code to create the string and writing it to the file\ndescriptor from log_ref_write and add it into a dedicated function\nlog_ref_write_fd. For now this is only used from log_ref_write,\nbut later on we will call this function from reflog transactions too,\nwhich means that we will end up with only a single place,\nwhere we write a reflog entry to a file instead of the current two\nplaces (log_ref_write and builtin/reflog.c).\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \nSigned-off-by: Michael Haggerty \n---\n refs.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 05cb299..150c980 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \treturn 0;\n }\n \n+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n+\t\t\t const unsigned char *new_sha1,\n+\t\t\t const char *committer, const char *msg)\n+{\n+\tint msglen, written;\n+\tunsigned maxlen, len;\n+\tchar *logrec;\n+\n+\tmsglen = msg ? strlen(msg) : 0;\n+\tmaxlen = strlen(committer) + msglen + 100;\n+\tlogrec = xmalloc(maxlen);\n+\tlen = sprintf(logrec, \"%s %s %s\\n\",\n+\t\t sha1_to_hex(old_sha1),\n+\t\t sha1_to_hex(new_sha1),\n+\t\t committer);\n+\tif (msglen)\n+\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n+\n+\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n+\tfree(logrec);\n+\tif (written != len)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg)\n {\n-\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n-\tunsigned maxlen, len;\n-\tint msglen;\n+\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \tchar log_file[PATH_MAX];\n-\tchar *logrec;\n-\tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \tlogfd = open(log_file, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tcommitter = git_committer_info(0);\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = sprintf(logrec, \"%s %s %s\\n\",\n-\t\t sha1_to_hex(old_sha1),\n-\t\t sha1_to_hex(new_sha1),\n-\t\t committer);\n-\tif (msglen)\n-\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n-\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n-\tfree(logrec);\n-\tif (written != len) {\n+\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n+\t\t\t\t git_committer_info(0), msg);\n+\tif (result) {\n \t\tint save_errno = errno;\n \t\tclose(logfd);\n \t\terror(\"Unable to append to %s\", log_file);\n-- \n2.1.3\n"},{"id":"253106","messageId":"1417734515-11812-5-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/23] expire_reflog(): remove unused parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:16Z","receivedAt":"2014-12-04T23:08:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It was called \"unused\", so at least it was self-consistent.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 2d85d26..3e11bee 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n \treturn 0;\n }\n \n-static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)\n+static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_data)\n {\n \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n \tstruct expire_reflog_cb cb;\n@@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tfor (i = 0; i < collected.nr; i++) {\n \t\t\tstruct collected_reflog *e = collected.e[i];\n \t\t\tset_reflog_expiry_param(&cb, explicit_expiry, e->reflog);\n-\t\t\tstatus |= expire_reflog(e->reflog, e->sha1, 0, &cb);\n+\t\t\tstatus |= expire_reflog(e->reflog, e->sha1, &cb);\n \t\t\tfree(e);\n \t\t}\n \t\tfree(collected.e);\n@@ -677,7 +677,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n-\t\tstatus |= expire_reflog(ref, sha1, 0, &cb);\n+\t\tstatus |= expire_reflog(ref, sha1, &cb);\n \t}\n \treturn status;\n }\n@@ -748,7 +748,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\t\tcb.expire_total = 0;\n \t\t}\n \n-\t\tstatus |= expire_reflog(ref, sha1, 0, &cb);\n+\t\tstatus |= expire_reflog(ref, sha1, &cb);\n \t\tfree(ref);\n \t}\n \treturn status;\n-- \n2.1.3\n"},{"id":"253126","messageId":"1417734515-11812-6-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/23] expire_reflog(): rename \"ref\" parameter to \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:17Z","receivedAt":"2014-12-04T23:08:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is our usual convention.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3e11bee..df302f4 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n \treturn 0;\n }\n \n-static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_data)\n+static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n {\n \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n \tstruct expire_reflog_cb cb;\n@@ -365,20 +365,20 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_da\n \t * we take the lock for the ref itself to prevent it from\n \t * getting updated.\n \t */\n-\tlock = lock_any_ref_for_update(ref, sha1, 0, NULL);\n+\tlock = lock_any_ref_for_update(refname, sha1, 0, NULL);\n \tif (!lock)\n-\t\treturn error(\"cannot lock ref '%s'\", ref);\n-\tlog_file = git_pathdup(\"logs/%s\", ref);\n-\tif (!reflog_exists(ref))\n+\t\treturn error(\"cannot lock ref '%s'\", refname);\n+\tlog_file = git_pathdup(\"logs/%s\", refname);\n+\tif (!reflog_exists(refname))\n \t\tgoto finish;\n \tif (!cmd->dry_run) {\n-\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", ref);\n+\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", refname);\n \t\tcb.newlog = fopen(newlog_path, \"w\");\n \t}\n \n \tcb.cmd = cmd;\n \n-\tif (!cmd->expire_unreachable || !strcmp(ref, \"HEAD\")) {\n+\tif (!cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n \t\ttip_commit = NULL;\n \t\tcb.unreachable_expire_kind = UE_HEAD;\n \t} else {\n@@ -407,7 +407,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_da\n \t\tmark_reachable(&cb);\n \t}\n \n-\tfor_each_reflog_ent(ref, expire_reflog_ent, &cb);\n+\tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n \n \tif (cb.unreachable_expire_kind != UE_ALWAYS) {\n \t\tif (cb.unreachable_expire_kind == UE_HEAD) {\n-- \n2.1.3\n"},{"id":"253123","messageId":"1417734515-11812-7-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/23] expire_reflog(): exit early if the reference has no reflog","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:18Z","receivedAt":"2014-12-04T23:08:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is very little cleanup needed if the reference has no reflog. If\nwe move the initialization of log_file down a bit, there's even less.\nSo instead of jumping to the cleanup code at the end of the function,\njust do the cleanup and return inline.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex df302f4..a282e60 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n \tlock = lock_any_ref_for_update(refname, sha1, 0, NULL);\n \tif (!lock)\n \t\treturn error(\"cannot lock ref '%s'\", refname);\n+\tif (!reflog_exists(refname)) {\n+\t\tunlock_ref(lock);\n+\t\treturn 0;\n+\t}\n \tlog_file = git_pathdup(\"logs/%s\", refname);\n-\tif (!reflog_exists(refname))\n-\t\tgoto finish;\n \tif (!cmd->dry_run) {\n \t\tnewlog_path = git_pathdup(\"logs/%s.lock\", refname);\n \t\tcb.newlog = fopen(newlog_path, \"w\");\n@@ -419,7 +421,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n \t\t\tclear_commit_marks(tip_commit, REACHABLE);\n \t\t}\n \t}\n- finish:\n+\n \tif (cb.newlog) {\n \t\tif (fclose(cb.newlog)) {\n \t\t\tstatus |= error(\"%s: %s\", strerror(errno),\n-- \n2.1.3\n"},{"id":"253109","messageId":"1417734515-11812-8-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:19Z","receivedAt":"2014-12-04T23:08:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We don't actually need the locking functionality, because we already\nhold the lock on the reference itself, which is how the reflog file is\nlocked. But the lock_file code still does some of the bookkeeping for\nus and is more careful than the old code here was.\n\nFor example:\n\n* Correctly handle the case that the reflog lock file already exists\n for some reason or cannot be opened.\n\n* Correctly clean up the lockfile if the program dies.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 37 ++++++++++++++++++++++---------------\n 1 file changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex a282e60..d344d45 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n \treturn 0;\n }\n \n+static struct lock_file reflog_lock;\n+\n static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n {\n \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n \tstruct expire_reflog_cb cb;\n \tstruct ref_lock *lock;\n-\tchar *log_file, *newlog_path = NULL;\n+\tchar *log_file;\n \tstruct commit *tip_commit;\n \tstruct commit_list *tips;\n \tint status = 0;\n@@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n \t\tunlock_ref(lock);\n \t\treturn 0;\n \t}\n+\n \tlog_file = git_pathdup(\"logs/%s\", refname);\n \tif (!cmd->dry_run) {\n-\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", refname);\n-\t\tcb.newlog = fopen(newlog_path, \"w\");\n+\t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n+\t\t\tgoto failure;\n+\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n+\t\tif (!cb.newlog)\n+\t\t\tgoto failure;\n \t}\n \n \tcb.cmd = cmd;\n@@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n \t}\n \n \tif (cb.newlog) {\n-\t\tif (fclose(cb.newlog)) {\n-\t\t\tstatus |= error(\"%s: %s\", strerror(errno),\n-\t\t\t\t\tnewlog_path);\n-\t\t\tunlink(newlog_path);\n+\t\tif (close_lock_file(&reflog_lock)) {\n+\t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n+\t\t\t\t\tstrerror(errno));\n \t\t} else if (cmd->updateref &&\n \t\t\t(write_in_full(lock->lock_fd,\n \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n@@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n \t\t\t\t\tlock->lk->filename.buf);\n-\t\t\tunlink(newlog_path);\n-\t\t} else if (rename(newlog_path, log_file)) {\n-\t\t\tstatus |= error(\"cannot rename %s to %s\",\n-\t\t\t\t\tnewlog_path, log_file);\n-\t\t\tunlink(newlog_path);\n+\t\t\trollback_lock_file(&reflog_lock);\n+\t\t} else if (commit_lock_file(&reflog_lock)) {\n+\t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n+\t\t\t\t\tlog_file, log_file);\n \t\t} else if (cmd->updateref && commit_ref(lock)) {\n \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n-\t\t} else {\n-\t\t\tadjust_shared_perm(log_file);\n \t\t}\n \t}\n-\tfree(newlog_path);\n \tfree(log_file);\n \tunlock_ref(lock);\n \treturn status;\n+\n+ failure:\n+\trollback_lock_file(&reflog_lock);\n+\tfree(log_file);\n+\tunlock_ref(lock);\n+\treturn -1;\n }\n \n static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)\n-- \n2.1.3\n"},{"id":"253124","messageId":"1417734515-11812-9-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/23] Extract function should_expire_reflog_ent()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:20Z","receivedAt":"2014-12-04T23:08:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extracted from expire_reflog_ent() a function that is solely\nresponsible for deciding whether a reflog entry should be expired. By\nseparating this \"business logic\" from the mechanics of actually\nexpiring entries, we are working towards the goal of encapsulating\nreflog expiry within the refs API, with policy decided by a callback\nfunction passed to it by its caller.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 70 +++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 42 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex d344d45..7bc6e0f 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -288,51 +288,65 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig\n \treturn !(commit->object.flags & REACHABLE);\n }\n \n-static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+/*\n+ * Return true iff the specified reflog entry should be expired.\n+ */\n+static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\t\t\t const char *email, unsigned long timestamp, int tz,\n+\t\t\t\t const char *message, void *cb_data)\n {\n \tstruct expire_reflog_cb *cb = cb_data;\n \tstruct commit *old, *new;\n \n \tif (timestamp < cb->cmd->expire_total)\n-\t\tgoto prune;\n-\n-\tif (cb->cmd->rewrite)\n-\t\tosha1 = cb->last_kept_sha1;\n+\t\treturn 1;\n \n \told = new = NULL;\n \tif (cb->cmd->stalefix &&\n \t (!keep_entry(&old, osha1) || !keep_entry(&new, nsha1)))\n-\t\tgoto prune;\n+\t\treturn 1;\n \n \tif (timestamp < cb->cmd->expire_unreachable) {\n \t\tif (cb->unreachable_expire_kind == UE_ALWAYS)\n-\t\t\tgoto prune;\n+\t\t\treturn 1;\n \t\tif (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))\n-\t\t\tgoto prune;\n+\t\t\treturn 1;\n \t}\n \n \tif (cb->cmd->recno && --(cb->cmd->recno) == 0)\n-\t\tgoto prune;\n-\n-\tif (cb->newlog) {\n-\t\tchar sign = (tz < 0) ? '-' : '+';\n-\t\tint zone = (tz < 0) ? (-tz) : tz;\n-\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n-\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n-\t\t\temail, timestamp, sign, zone,\n-\t\t\tmessage);\n-\t\thashcpy(cb->last_kept_sha1, nsha1);\n-\t}\n-\tif (cb->cmd->verbose)\n-\t\tprintf(\"keep %s\", message);\n+\t\treturn 1;\n+\n \treturn 0;\n- prune:\n-\tif (!cb->newlog)\n-\t\tprintf(\"would prune %s\", message);\n-\telse if (cb->cmd->verbose)\n-\t\tprintf(\"prune %s\", message);\n+}\n+\n+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *message, void *cb_data)\n+{\n+\tstruct expire_reflog_cb *cb = cb_data;\n+\n+\tif (cb->cmd->rewrite)\n+\t\tosha1 = cb->last_kept_sha1;\n+\n+\tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n+\t\t\t\t message, cb_data)) {\n+\t\tif (!cb->newlog)\n+\t\t\tprintf(\"would prune %s\", message);\n+\t\telse if (cb->cmd->verbose)\n+\t\t\tprintf(\"prune %s\", message);\n+\t} else {\n+\t\tif (cb->newlog) {\n+\t\t\tchar sign = (tz < 0) ? '-' : '+';\n+\t\t\tint zone = (tz < 0) ? (-tz) : tz;\n+\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n+\t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n+\t\t\t\temail, timestamp, sign, zone,\n+\t\t\t\tmessage);\n+\t\t\thashcpy(cb->last_kept_sha1, nsha1);\n+\t\t}\n+\t\tif (cb->cmd->verbose)\n+\t\t\tprintf(\"keep %s\", message);\n+\t}\n \treturn 0;\n }\n \n-- \n2.1.3\n"},{"id":"253112","messageId":"1417734515-11812-10-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/23] expire_reflog(): extract two policy-related functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:21Z","receivedAt":"2014-12-04T23:08:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract two functions, reflog_expiry_prepare() and\nreflog_expiry_cleanup(), from expire_reflog(). This is a further step\ntowards separating the code for deciding on expiration policy from the\ncode that manages the physical expiration.\n\nThis change requires a couple of local variables from expire_reflog()\nto be turned into fields of \"struct expire_reflog_cb\". More\nreorganization of the callback data will follow in later commits.\n\nSigned-off-by: Michael Haggerty \n---\nIn fact, the work done in reflog_expire_cleanup() doesn't really need\nto be done via a callback, because it doesn't need to be done while\nthe reference lock is held. But the symmetry between prepare and\ncleanup is kindof nice. Perhaps some future policy decision will want\nto do some final work under the reference lock?\n\nBut it would be easy to get rid of this third callback function and\nhave the callers do the work themselves after calling expire_reflog().\nI don't have a string feeling either way.\n\n builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 52 insertions(+), 42 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 7bc6e0f..ebfa635 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -43,6 +43,8 @@ struct expire_reflog_cb {\n \tunsigned long mark_limit;\n \tstruct cmd_reflog_expire_cb *cmd;\n \tunsigned char last_kept_sha1[20];\n+\tstruct commit *tip_commit;\n+\tstruct commit_list *tips;\n };\n \n struct collected_reflog {\n@@ -363,6 +365,54 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n \treturn 0;\n }\n \n+static void reflog_expiry_prepare(const char *refname,\n+\t\t\t\t const unsigned char *sha1,\n+\t\t\t\t struct expire_reflog_cb *cb)\n+{\n+\tif (!cb->cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n+\t\tcb->tip_commit = NULL;\n+\t\tcb->unreachable_expire_kind = UE_HEAD;\n+\t} else {\n+\t\tcb->tip_commit = lookup_commit_reference_gently(sha1, 1);\n+\t\tif (!cb->tip_commit)\n+\t\t\tcb->unreachable_expire_kind = UE_ALWAYS;\n+\t\telse\n+\t\t\tcb->unreachable_expire_kind = UE_NORMAL;\n+\t}\n+\n+\tif (cb->cmd->expire_unreachable <= cb->cmd->expire_total)\n+\t\tcb->unreachable_expire_kind = UE_ALWAYS;\n+\n+\tcb->mark_list = NULL;\n+\tcb->tips = NULL;\n+\tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n+\t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n+\t\t\tstruct commit_list *elem;\n+\t\t\tfor_each_ref(push_tip_to_list, &cb->tips);\n+\t\t\tfor (elem = cb->tips; elem; elem = elem->next)\n+\t\t\t\tcommit_list_insert(elem->item, &cb->mark_list);\n+\t\t} else {\n+\t\t\tcommit_list_insert(cb->tip_commit, &cb->mark_list);\n+\t\t}\n+\t\tcb->mark_limit = cb->cmd->expire_total;\n+\t\tmark_reachable(cb);\n+\t}\n+}\n+\n+static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n+{\n+\tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n+\t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n+\t\t\tstruct commit_list *elem;\n+\t\t\tfor (elem = cb->tips; elem; elem = elem->next)\n+\t\t\t\tclear_commit_marks(elem->item, REACHABLE);\n+\t\t\tfree_commit_list(cb->tips);\n+\t\t} else {\n+\t\t\tclear_commit_marks(cb->tip_commit, REACHABLE);\n+\t\t}\n+\t}\n+}\n+\n static struct lock_file reflog_lock;\n \n static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n@@ -371,8 +421,6 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n \tstruct expire_reflog_cb cb;\n \tstruct ref_lock *lock;\n \tchar *log_file;\n-\tstruct commit *tip_commit;\n-\tstruct commit_list *tips;\n \tint status = 0;\n \n \tmemset(&cb, 0, sizeof(cb));\n@@ -400,47 +448,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n \n \tcb.cmd = cmd;\n \n-\tif (!cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n-\t\ttip_commit = NULL;\n-\t\tcb.unreachable_expire_kind = UE_HEAD;\n-\t} else {\n-\t\ttip_commit = lookup_commit_reference_gently(sha1, 1);\n-\t\tif (!tip_commit)\n-\t\t\tcb.unreachable_expire_kind = UE_ALWAYS;\n-\t\telse\n-\t\t\tcb.unreachable_expire_kind = UE_NORMAL;\n-\t}\n-\n-\tif (cmd->expire_unreachable <= cmd->expire_total)\n-\t\tcb.unreachable_expire_kind = UE_ALWAYS;\n-\n-\tcb.mark_list = NULL;\n-\ttips = NULL;\n-\tif (cb.unreachable_expire_kind != UE_ALWAYS) {\n-\t\tif (cb.unreachable_expire_kind == UE_HEAD) {\n-\t\t\tstruct commit_list *elem;\n-\t\t\tfor_each_ref(push_tip_to_list, &tips);\n-\t\t\tfor (elem = tips; elem; elem = elem->next)\n-\t\t\t\tcommit_list_insert(elem->item, &cb.mark_list);\n-\t\t} else {\n-\t\t\tcommit_list_insert(tip_commit, &cb.mark_list);\n-\t\t}\n-\t\tcb.mark_limit = cmd->expire_total;\n-\t\tmark_reachable(&cb);\n-\t}\n-\n+\treflog_expiry_prepare(refname, sha1, &cb);\n \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n-\n-\tif (cb.unreachable_expire_kind != UE_ALWAYS) {\n-\t\tif (cb.unreachable_expire_kind == UE_HEAD) {\n-\t\t\tstruct commit_list *elem;\n-\t\t\tfor (elem = tips; elem; elem = elem->next)\n-\t\t\t\tclear_commit_marks(elem->item, REACHABLE);\n-\t\t\tfree_commit_list(tips);\n-\t\t} else {\n-\t\t\tclear_commit_marks(tip_commit, REACHABLE);\n-\t\t}\n-\t}\n+\treflog_expiry_cleanup(&cb);\n \n \tif (cb.newlog) {\n \t\tif (close_lock_file(&reflog_lock)) {\n-- \n2.1.3\n"},{"id":"253110","messageId":"1417734515-11812-11-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/23] expire_reflog(): add a \"flags\" argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:22Z","receivedAt":"2014-12-04T23:08:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We want to separate the options relevant to the expiry machinery from\nthe options affecting the expiration policy. So add a \"flags\" argument\nto expire_reflog() to hold the former.\n\nThe argument doesn't yet do anything.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex ebfa635..319f0d2 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -415,7 +415,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n \n static struct lock_file reflog_lock;\n \n-static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n+static int expire_reflog(const char *refname, const unsigned char *sha1,\n+\t\t\t unsigned int flags, void *cb_data)\n {\n \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n \tstruct expire_reflog_cb cb;\n@@ -627,6 +628,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \tunsigned long now = time(NULL);\n \tint i, status, do_all;\n \tint explicit_expiry = 0;\n+\tunsigned int flags = 0;\n \n \tdefault_reflog_expire_unreachable = now - 30 * 24 * 3600;\n \tdefault_reflog_expire = now - 90 * 24 * 3600;\n@@ -696,7 +698,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tfor (i = 0; i < collected.nr; i++) {\n \t\t\tstruct collected_reflog *e = collected.e[i];\n \t\t\tset_reflog_expiry_param(&cb, explicit_expiry, e->reflog);\n-\t\t\tstatus |= expire_reflog(e->reflog, e->sha1, &cb);\n+\t\t\tstatus |= expire_reflog(e->reflog, e->sha1, flags, &cb);\n \t\t\tfree(e);\n \t\t}\n \t\tfree(collected.e);\n@@ -710,7 +712,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n-\t\tstatus |= expire_reflog(ref, sha1, &cb);\n+\t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n \t}\n \treturn status;\n }\n@@ -729,6 +731,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n {\n \tstruct cmd_reflog_expire_cb cb;\n \tint i, status = 0;\n+\tunsigned int flags = 0;\n \n \tmemset(&cb, 0, sizeof(cb));\n \n@@ -781,7 +784,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\t\tcb.expire_total = 0;\n \t\t}\n \n-\t\tstatus |= expire_reflog(ref, sha1, &cb);\n+\t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n \t\tfree(ref);\n \t}\n \treturn status;\n-- \n2.1.3\n"},{"id":"253127","messageId":"1417734515-11812-12-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/23] expire_reflog(): move dry_run to flags argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:23Z","receivedAt":"2014-12-04T23:08:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The policy objects don't care about \"--dry-run\". So move it to\nexpire_reflog()'s flags parameter.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 319f0d2..a490193 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -22,7 +22,6 @@ static unsigned long default_reflog_expire_unreachable;\n \n struct cmd_reflog_expire_cb {\n \tstruct rev_info revs;\n-\tint dry_run;\n \tint stalefix;\n \tint rewrite;\n \tint updateref;\n@@ -415,6 +414,10 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n \n static struct lock_file reflog_lock;\n \n+enum expire_reflog_flags {\n+\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0\n+};\n+\n static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t\t unsigned int flags, void *cb_data)\n {\n@@ -439,7 +442,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t}\n \n \tlog_file = git_pathdup(\"logs/%s\", refname);\n-\tif (!cmd->dry_run) {\n+\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n \t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n \t\t\tgoto failure;\n \t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n@@ -453,7 +456,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n \treflog_expiry_cleanup(&cb);\n \n-\tif (cb.newlog) {\n+\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n \t\tif (close_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n \t\t\t\t\tstrerror(errno));\n@@ -644,7 +647,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n-\t\t\tcb.dry_run = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n \t\telse if (starts_with(arg, \"--expire=\")) {\n \t\t\tif (parse_expiry_date(arg + 9, &cb.expire_total))\n \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n@@ -738,7 +741,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n-\t\t\tcb.dry_run = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n \t\telse if (!strcmp(arg, \"--rewrite\"))\n \t\t\tcb.rewrite = 1;\n \t\telse if (!strcmp(arg, \"--updateref\"))\n-- \n2.1.3\n"},{"id":"253111","messageId":"1417734515-11812-13-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/23] expire_reflog(): move updateref to flags argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:24Z","receivedAt":"2014-12-04T23:08:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The policy objects don't care about \"--updateref\". So move it to\nexpire_reflog()'s flags parameter.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex a490193..597c547 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -24,7 +24,6 @@ struct cmd_reflog_expire_cb {\n \tstruct rev_info revs;\n \tint stalefix;\n \tint rewrite;\n-\tint updateref;\n \tint verbose;\n \tunsigned long expire_total;\n \tunsigned long expire_unreachable;\n@@ -415,7 +414,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n static struct lock_file reflog_lock;\n \n enum expire_reflog_flags {\n-\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0\n+\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n+\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1\n };\n \n static int expire_reflog(const char *refname, const unsigned char *sha1,\n@@ -460,7 +460,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\tif (close_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n \t\t\t\t\tstrerror(errno));\n-\t\t} else if (cmd->updateref &&\n+\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n \t\t\t(write_in_full(lock->lock_fd,\n \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n@@ -471,7 +471,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t} else if (commit_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n \t\t\t\t\tlog_file, log_file);\n-\t\t} else if (cmd->updateref && commit_ref(lock)) {\n+\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n \t\t}\n \t}\n@@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\telse if (!strcmp(arg, \"--rewrite\"))\n \t\t\tcb.rewrite = 1;\n \t\telse if (!strcmp(arg, \"--updateref\"))\n-\t\t\tcb.updateref = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n \t\telse if (!strcmp(arg, \"--all\"))\n \t\t\tdo_all = 1;\n \t\telse if (!strcmp(arg, \"--verbose\"))\n@@ -745,7 +745,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\telse if (!strcmp(arg, \"--rewrite\"))\n \t\t\tcb.rewrite = 1;\n \t\telse if (!strcmp(arg, \"--updateref\"))\n-\t\t\tcb.updateref = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n \t\telse if (!strcmp(arg, \"--verbose\"))\n \t\t\tcb.verbose = 1;\n \t\telse if (!strcmp(arg, \"--\")) {\n-- \n2.1.3\n"},{"id":"253114","messageId":"1417734515-11812-14-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 13/23] Rename expire_reflog_cb to expire_reflog_policy_cb","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:25Z","receivedAt":"2014-12-04T23:08:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is the first step towards separating the data needed by the\npolicy code from the data needed by the reflog expiration machinery.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 597c547..3538e4b 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -30,7 +30,7 @@ struct cmd_reflog_expire_cb {\n \tint recno;\n };\n \n-struct expire_reflog_cb {\n+struct expire_reflog_policy_cb {\n \tFILE *newlog;\n \tenum {\n \t\tUE_NORMAL,\n@@ -220,7 +220,7 @@ static int keep_entry(struct commit **it, unsigned char *sha1)\n * the expire_limit and queue them back, so that the caller can call\n * us again to restart the traversal with longer expire_limit.\n */\n-static void mark_reachable(struct expire_reflog_cb *cb)\n+static void mark_reachable(struct expire_reflog_policy_cb *cb)\n {\n \tstruct commit *commit;\n \tstruct commit_list *pending;\n@@ -259,7 +259,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)\n \tcb->mark_list = leftover;\n }\n \n-static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsigned char *sha1)\n+static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, unsigned char *sha1)\n {\n \t/*\n \t * We may or may not have the commit yet - if not, look it\n@@ -295,7 +295,7 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\t\t\t const char *email, unsigned long timestamp, int tz,\n \t\t\t\t const char *message, void *cb_data)\n {\n-\tstruct expire_reflog_cb *cb = cb_data;\n+\tstruct expire_reflog_policy_cb *cb = cb_data;\n \tstruct commit *old, *new;\n \n \tif (timestamp < cb->cmd->expire_total)\n@@ -323,7 +323,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tconst char *email, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n-\tstruct expire_reflog_cb *cb = cb_data;\n+\tstruct expire_reflog_policy_cb *cb = cb_data;\n \n \tif (cb->cmd->rewrite)\n \t\tosha1 = cb->last_kept_sha1;\n@@ -350,7 +350,8 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \treturn 0;\n }\n \n-static int push_tip_to_list(const char *refname, const unsigned char *sha1, int flags, void *cb_data)\n+static int push_tip_to_list(const char *refname, const unsigned char *sha1,\n+\t\t\t int flags, void *cb_data)\n {\n \tstruct commit_list **list = cb_data;\n \tstruct commit *tip_commit;\n@@ -365,7 +366,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n \n static void reflog_expiry_prepare(const char *refname,\n \t\t\t\t const unsigned char *sha1,\n-\t\t\t\t struct expire_reflog_cb *cb)\n+\t\t\t\t struct expire_reflog_policy_cb *cb)\n {\n \tif (!cb->cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n \t\tcb->tip_commit = NULL;\n@@ -397,7 +398,7 @@ static void reflog_expiry_prepare(const char *refname,\n \t}\n }\n \n-static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n+static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n {\n \tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n \t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n@@ -422,7 +423,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t\t unsigned int flags, void *cb_data)\n {\n \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n-\tstruct expire_reflog_cb cb;\n+\tstruct expire_reflog_policy_cb cb;\n \tstruct ref_lock *lock;\n \tchar *log_file;\n \tint status = 0;\n-- \n2.1.3\n"},{"id":"253119","messageId":"1417734515-11812-15-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 14/23] struct expire_reflog_cb: a new callback data type","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:26Z","receivedAt":"2014-12-04T23:08:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a new data type, \"struct expire_reflog_cb\", for holding the data\nthat expire_reflog() passes to expire_reflog_ent() via\nfor_each_reflog_ent(). For now it only holds a pointer to \"struct\nexpire_reflog_policy_cb\". In future commits we will move some data\nfrom the latter to the former.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 43 ++++++++++++++++++++++++++-----------------\n 1 file changed, 26 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3538e4b..5dfa53a 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -45,10 +45,15 @@ struct expire_reflog_policy_cb {\n \tstruct commit_list *tips;\n };\n \n+struct expire_reflog_cb {\n+\tvoid *policy_cb;\n+};\n+\n struct collected_reflog {\n \tunsigned char sha1[20];\n \tchar reflog[FLEX_ARRAY];\n };\n+\n struct collect_reflog_cb {\n \tstruct collected_reflog **e;\n \tint alloc;\n@@ -323,28 +328,29 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tconst char *email, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n-\tstruct expire_reflog_policy_cb *cb = cb_data;\n+\tstruct expire_reflog_cb *cb = cb_data;\n+\tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n \n-\tif (cb->cmd->rewrite)\n-\t\tosha1 = cb->last_kept_sha1;\n+\tif (policy_cb->cmd->rewrite)\n+\t\tosha1 = policy_cb->last_kept_sha1;\n \n \tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n-\t\t\t\t message, cb_data)) {\n-\t\tif (!cb->newlog)\n+\t\t\t\t message, policy_cb)) {\n+\t\tif (!policy_cb->newlog)\n \t\t\tprintf(\"would prune %s\", message);\n-\t\telse if (cb->cmd->verbose)\n+\t\telse if (policy_cb->cmd->verbose)\n \t\t\tprintf(\"prune %s\", message);\n \t} else {\n-\t\tif (cb->newlog) {\n+\t\tif (policy_cb->newlog) {\n \t\t\tchar sign = (tz < 0) ? '-' : '+';\n \t\t\tint zone = (tz < 0) ? (-tz) : tz;\n-\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n+\t\t\tfprintf(policy_cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n \t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n \t\t\t\temail, timestamp, sign, zone,\n \t\t\t\tmessage);\n-\t\t\thashcpy(cb->last_kept_sha1, nsha1);\n+\t\t\thashcpy(policy_cb->last_kept_sha1, nsha1);\n \t\t}\n-\t\tif (cb->cmd->verbose)\n+\t\tif (policy_cb->cmd->verbose)\n \t\t\tprintf(\"keep %s\", message);\n \t}\n \treturn 0;\n@@ -423,12 +429,15 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t\t unsigned int flags, void *cb_data)\n {\n \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n-\tstruct expire_reflog_policy_cb cb;\n+\tstruct expire_reflog_cb cb;\n+\tstruct expire_reflog_policy_cb policy_cb;\n \tstruct ref_lock *lock;\n \tchar *log_file;\n \tint status = 0;\n \n \tmemset(&cb, 0, sizeof(cb));\n+\tmemset(&policy_cb, 0, sizeof(policy_cb));\n+\tcb.policy_cb = &policy_cb;\n \n \t/*\n \t * we take the lock for the ref itself to prevent it from\n@@ -446,16 +455,16 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n \t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n \t\t\tgoto failure;\n-\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n-\t\tif (!cb.newlog)\n+\t\tpolicy_cb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n+\t\tif (!policy_cb.newlog)\n \t\t\tgoto failure;\n \t}\n \n-\tcb.cmd = cmd;\n+\tpolicy_cb.cmd = cmd;\n \n-\treflog_expiry_prepare(refname, sha1, &cb);\n+\treflog_expiry_prepare(refname, sha1, &policy_cb);\n \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n-\treflog_expiry_cleanup(&cb);\n+\treflog_expiry_cleanup(&policy_cb);\n \n \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n \t\tif (close_lock_file(&reflog_lock)) {\n@@ -463,7 +472,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t\t\t\tstrerror(errno));\n \t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n \t\t\t(write_in_full(lock->lock_fd,\n-\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n+\t\t\t\tsha1_to_hex(policy_cb.last_kept_sha1), 40) != 40 ||\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n-- \n2.1.3\n"},{"id":"253122","messageId":"1417734515-11812-16-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 15/23] expire_reflog(): pass flags through to expire_reflog_ent()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:27Z","receivedAt":"2014-12-04T23:08:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a flags field to \"struct expire_reflog_cb\", and pass the flags\nargument through to expire_reflog_ent(). In a moment we will start\nusing it to pass through flags that expire_reflog_ent() needs.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 5dfa53a..1512b67 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -46,6 +46,7 @@ struct expire_reflog_policy_cb {\n };\n \n struct expire_reflog_cb {\n+\tunsigned int flags;\n \tvoid *policy_cb;\n };\n \n@@ -437,6 +438,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \n \tmemset(&cb, 0, sizeof(cb));\n \tmemset(&policy_cb, 0, sizeof(policy_cb));\n+\tcb.flags = flags;\n \tcb.policy_cb = &policy_cb;\n \n \t/*\n-- \n2.1.3\n"},{"id":"253120","messageId":"1417734515-11812-17-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 16/23] expire_reflog(): move verbose to flags argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:28Z","receivedAt":"2014-12-04T23:08:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The policy objects don't care about \"--verbose\". So move it to\nexpire_reflog()'s flags parameter.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 1512b67..cc7a220 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -20,11 +20,16 @@ static const char reflog_delete_usage[] =\n static unsigned long default_reflog_expire;\n static unsigned long default_reflog_expire_unreachable;\n \n+enum expire_reflog_flags {\n+\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n+\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n+\tEXPIRE_REFLOGS_VERBOSE = 1 << 2\n+};\n+\n struct cmd_reflog_expire_cb {\n \tstruct rev_info revs;\n \tint stalefix;\n \tint rewrite;\n-\tint verbose;\n \tunsigned long expire_total;\n \tunsigned long expire_unreachable;\n \tint recno;\n@@ -339,7 +344,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\t\t\t message, policy_cb)) {\n \t\tif (!policy_cb->newlog)\n \t\t\tprintf(\"would prune %s\", message);\n-\t\telse if (policy_cb->cmd->verbose)\n+\t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"prune %s\", message);\n \t} else {\n \t\tif (policy_cb->newlog) {\n@@ -351,7 +356,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\t\t\tmessage);\n \t\t\thashcpy(policy_cb->last_kept_sha1, nsha1);\n \t\t}\n-\t\tif (policy_cb->cmd->verbose)\n+\t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"keep %s\", message);\n \t}\n \treturn 0;\n@@ -421,11 +426,6 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n \n static struct lock_file reflog_lock;\n \n-enum expire_reflog_flags {\n-\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n-\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1\n-};\n-\n static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t\t unsigned int flags, void *cb_data)\n {\n@@ -679,7 +679,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\telse if (!strcmp(arg, \"--all\"))\n \t\t\tdo_all = 1;\n \t\telse if (!strcmp(arg, \"--verbose\"))\n-\t\t\tcb.verbose = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_VERBOSE;\n \t\telse if (!strcmp(arg, \"--\")) {\n \t\t\ti++;\n \t\t\tbreak;\n@@ -697,10 +697,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t */\n \tif (cb.stalefix) {\n \t\tinit_revisions(&cb.revs, prefix);\n-\t\tif (cb.verbose)\n+\t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"Marking reachable objects...\");\n \t\tmark_reachable_objects(&cb.revs, 0, 0, NULL);\n-\t\tif (cb.verbose)\n+\t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tputchar('\\n');\n \t}\n \n@@ -759,7 +759,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\telse if (!strcmp(arg, \"--updateref\"))\n \t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n \t\telse if (!strcmp(arg, \"--verbose\"))\n-\t\t\tcb.verbose = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_VERBOSE;\n \t\telse if (!strcmp(arg, \"--\")) {\n \t\t\ti++;\n \t\t\tbreak;\n-- \n2.1.3\n"},{"id":"253113","messageId":"1417734515-11812-18-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 17/23] expire_reflog(): move rewrite to flags argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:29Z","receivedAt":"2014-12-04T23:08:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The policy objects don't care about \"--rewrite\". So move it to\nexpire_reflog()'s flags parameter.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex cc7a220..6294406 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -23,13 +23,13 @@ static unsigned long default_reflog_expire_unreachable;\n enum expire_reflog_flags {\n \tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n \tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n-\tEXPIRE_REFLOGS_VERBOSE = 1 << 2\n+\tEXPIRE_REFLOGS_VERBOSE = 1 << 2,\n+\tEXPIRE_REFLOGS_REWRITE = 1 << 3\n };\n \n struct cmd_reflog_expire_cb {\n \tstruct rev_info revs;\n \tint stalefix;\n-\tint rewrite;\n \tunsigned long expire_total;\n \tunsigned long expire_unreachable;\n \tint recno;\n@@ -337,7 +337,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \tstruct expire_reflog_cb *cb = cb_data;\n \tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n \n-\tif (policy_cb->cmd->rewrite)\n+\tif (cb->flags & EXPIRE_REFLOGS_REWRITE)\n \t\tosha1 = policy_cb->last_kept_sha1;\n \n \tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n@@ -673,7 +673,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\telse if (!strcmp(arg, \"--stale-fix\"))\n \t\t\tcb.stalefix = 1;\n \t\telse if (!strcmp(arg, \"--rewrite\"))\n-\t\t\tcb.rewrite = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_REWRITE;\n \t\telse if (!strcmp(arg, \"--updateref\"))\n \t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n \t\telse if (!strcmp(arg, \"--all\"))\n@@ -755,7 +755,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n \t\telse if (!strcmp(arg, \"--rewrite\"))\n-\t\t\tcb.rewrite = 1;\n+\t\t\tflags |= EXPIRE_REFLOGS_REWRITE;\n \t\telse if (!strcmp(arg, \"--updateref\"))\n \t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n \t\telse if (!strcmp(arg, \"--verbose\"))\n-- \n2.1.3\n"},{"id":"253121","messageId":"1417734515-11812-19-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 18/23] Move newlog and last_kept_sha1 to \"struct expire_reflog_cb\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:30Z","receivedAt":"2014-12-04T23:08:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"These members are not needed by the policy functions.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 6294406..01b76d0 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -36,7 +36,6 @@ struct cmd_reflog_expire_cb {\n };\n \n struct expire_reflog_policy_cb {\n-\tFILE *newlog;\n \tenum {\n \t\tUE_NORMAL,\n \t\tUE_ALWAYS,\n@@ -45,7 +44,6 @@ struct expire_reflog_policy_cb {\n \tstruct commit_list *mark_list;\n \tunsigned long mark_limit;\n \tstruct cmd_reflog_expire_cb *cmd;\n-\tunsigned char last_kept_sha1[20];\n \tstruct commit *tip_commit;\n \tstruct commit_list *tips;\n };\n@@ -53,6 +51,8 @@ struct expire_reflog_policy_cb {\n struct expire_reflog_cb {\n \tunsigned int flags;\n \tvoid *policy_cb;\n+\tFILE *newlog;\n+\tunsigned char last_kept_sha1[20];\n };\n \n struct collected_reflog {\n@@ -338,23 +338,23 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n \n \tif (cb->flags & EXPIRE_REFLOGS_REWRITE)\n-\t\tosha1 = policy_cb->last_kept_sha1;\n+\t\tosha1 = cb->last_kept_sha1;\n \n \tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n \t\t\t\t message, policy_cb)) {\n-\t\tif (!policy_cb->newlog)\n+\t\tif (!cb->newlog)\n \t\t\tprintf(\"would prune %s\", message);\n \t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"prune %s\", message);\n \t} else {\n-\t\tif (policy_cb->newlog) {\n+\t\tif (cb->newlog) {\n \t\t\tchar sign = (tz < 0) ? '-' : '+';\n \t\t\tint zone = (tz < 0) ? (-tz) : tz;\n-\t\t\tfprintf(policy_cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n+\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n \t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n \t\t\t\temail, timestamp, sign, zone,\n \t\t\t\tmessage);\n-\t\t\thashcpy(policy_cb->last_kept_sha1, nsha1);\n+\t\t\thashcpy(cb->last_kept_sha1, nsha1);\n \t\t}\n \t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"keep %s\", message);\n@@ -457,8 +457,8 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n \t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n \t\t\tgoto failure;\n-\t\tpolicy_cb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n-\t\tif (!policy_cb.newlog)\n+\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n+\t\tif (!cb.newlog)\n \t\t\tgoto failure;\n \t}\n \n@@ -474,7 +474,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t\t\t\tstrerror(errno));\n \t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n \t\t\t(write_in_full(lock->lock_fd,\n-\t\t\t\tsha1_to_hex(policy_cb.last_kept_sha1), 40) != 40 ||\n+\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n-- \n2.1.3\n"},{"id":"253115","messageId":"1417734515-11812-20-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 19/23] expire_reflog(): treat the policy callback data as opaque","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:31Z","receivedAt":"2014-12-04T23:08:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that expire_reflog() doesn't actually look in the\nexpire_reflog_policy_cb data structure, we can make it opaque:\n\n* Change its callers to pass it a pointer to an entire \"struct\n expire_reflog_policy_cb\".\n\n* Change it to pass the pointer through as a \"void *\".\n\n* Change the policy functions, reflog_expiry_prepare(),\n reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept\n \"void *cb_data\" arguments and cast them to \"struct\n expire_reflog_policy_cb\" internally.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 73 ++++++++++++++++++++++++++++----------------------------\n 1 file changed, 36 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 01b76d0..c30936bb 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -43,7 +43,7 @@ struct expire_reflog_policy_cb {\n \t} unreachable_expire_kind;\n \tstruct commit_list *mark_list;\n \tunsigned long mark_limit;\n-\tstruct cmd_reflog_expire_cb *cmd;\n+\tstruct cmd_reflog_expire_cb cmd;\n \tstruct commit *tip_commit;\n \tstruct commit_list *tips;\n };\n@@ -309,22 +309,22 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \tstruct expire_reflog_policy_cb *cb = cb_data;\n \tstruct commit *old, *new;\n \n-\tif (timestamp < cb->cmd->expire_total)\n+\tif (timestamp < cb->cmd.expire_total)\n \t\treturn 1;\n \n \told = new = NULL;\n-\tif (cb->cmd->stalefix &&\n+\tif (cb->cmd.stalefix &&\n \t (!keep_entry(&old, osha1) || !keep_entry(&new, nsha1)))\n \t\treturn 1;\n \n-\tif (timestamp < cb->cmd->expire_unreachable) {\n+\tif (timestamp < cb->cmd.expire_unreachable) {\n \t\tif (cb->unreachable_expire_kind == UE_ALWAYS)\n \t\t\treturn 1;\n \t\tif (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))\n \t\t\treturn 1;\n \t}\n \n-\tif (cb->cmd->recno && --(cb->cmd->recno) == 0)\n+\tif (cb->cmd.recno && --(cb->cmd.recno) == 0)\n \t\treturn 1;\n \n \treturn 0;\n@@ -378,9 +378,11 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1,\n \n static void reflog_expiry_prepare(const char *refname,\n \t\t\t\t const unsigned char *sha1,\n-\t\t\t\t struct expire_reflog_policy_cb *cb)\n+\t\t\t\t void *cb_data)\n {\n-\tif (!cb->cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n+\tstruct expire_reflog_policy_cb *cb = cb_data;\n+\n+\tif (!cb->cmd.expire_unreachable || !strcmp(refname, \"HEAD\")) {\n \t\tcb->tip_commit = NULL;\n \t\tcb->unreachable_expire_kind = UE_HEAD;\n \t} else {\n@@ -391,7 +393,7 @@ static void reflog_expiry_prepare(const char *refname,\n \t\t\tcb->unreachable_expire_kind = UE_NORMAL;\n \t}\n \n-\tif (cb->cmd->expire_unreachable <= cb->cmd->expire_total)\n+\tif (cb->cmd.expire_unreachable <= cb->cmd.expire_total)\n \t\tcb->unreachable_expire_kind = UE_ALWAYS;\n \n \tcb->mark_list = NULL;\n@@ -405,13 +407,15 @@ static void reflog_expiry_prepare(const char *refname,\n \t\t} else {\n \t\t\tcommit_list_insert(cb->tip_commit, &cb->mark_list);\n \t\t}\n-\t\tcb->mark_limit = cb->cmd->expire_total;\n+\t\tcb->mark_limit = cb->cmd.expire_total;\n \t\tmark_reachable(cb);\n \t}\n }\n \n-static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n+static void reflog_expiry_cleanup(void *cb_data)\n {\n+\tstruct expire_reflog_policy_cb *cb = cb_data;\n+\n \tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n \t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n \t\t\tstruct commit_list *elem;\n@@ -427,19 +431,16 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n static struct lock_file reflog_lock;\n \n static int expire_reflog(const char *refname, const unsigned char *sha1,\n-\t\t\t unsigned int flags, void *cb_data)\n+\t\t\t unsigned int flags, void *policy_cb_data)\n {\n-\tstruct cmd_reflog_expire_cb *cmd = cb_data;\n \tstruct expire_reflog_cb cb;\n-\tstruct expire_reflog_policy_cb policy_cb;\n \tstruct ref_lock *lock;\n \tchar *log_file;\n \tint status = 0;\n \n \tmemset(&cb, 0, sizeof(cb));\n-\tmemset(&policy_cb, 0, sizeof(policy_cb));\n \tcb.flags = flags;\n-\tcb.policy_cb = &policy_cb;\n+\tcb.policy_cb = policy_cb_data;\n \n \t/*\n \t * we take the lock for the ref itself to prevent it from\n@@ -462,11 +463,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n \t\t\tgoto failure;\n \t}\n \n-\tpolicy_cb.cmd = cmd;\n-\n-\treflog_expiry_prepare(refname, sha1, &policy_cb);\n+\treflog_expiry_prepare(refname, sha1, cb.policy_cb);\n \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n-\treflog_expiry_cleanup(&policy_cb);\n+\treflog_expiry_cleanup(cb.policy_cb);\n \n \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n \t\tif (close_lock_file(&reflog_lock)) {\n@@ -639,7 +638,7 @@ static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, c\n \n static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n {\n-\tstruct cmd_reflog_expire_cb cb;\n+\tstruct expire_reflog_policy_cb cb;\n \tunsigned long now = time(NULL);\n \tint i, status, do_all;\n \tint explicit_expiry = 0;\n@@ -653,25 +652,25 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \tdo_all = status = 0;\n \tmemset(&cb, 0, sizeof(cb));\n \n-\tcb.expire_total = default_reflog_expire;\n-\tcb.expire_unreachable = default_reflog_expire_unreachable;\n+\tcb.cmd.expire_total = default_reflog_expire;\n+\tcb.cmd.expire_unreachable = default_reflog_expire_unreachable;\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n \t\telse if (starts_with(arg, \"--expire=\")) {\n-\t\t\tif (parse_expiry_date(arg + 9, &cb.expire_total))\n+\t\t\tif (parse_expiry_date(arg + 9, &cb.cmd.expire_total))\n \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n \t\t}\n \t\telse if (starts_with(arg, \"--expire-unreachable=\")) {\n-\t\t\tif (parse_expiry_date(arg + 21, &cb.expire_unreachable))\n+\t\t\tif (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))\n \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n \t\t}\n \t\telse if (!strcmp(arg, \"--stale-fix\"))\n-\t\t\tcb.stalefix = 1;\n+\t\t\tcb.cmd.stalefix = 1;\n \t\telse if (!strcmp(arg, \"--rewrite\"))\n \t\t\tflags |= EXPIRE_REFLOGS_REWRITE;\n \t\telse if (!strcmp(arg, \"--updateref\"))\n@@ -695,11 +694,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t * even in older repository. We cannot trust what's reachable\n \t * from reflog if the repository was pruned with older git.\n \t */\n-\tif (cb.stalefix) {\n-\t\tinit_revisions(&cb.revs, prefix);\n+\tif (cb.cmd.stalefix) {\n+\t\tinit_revisions(&cb.cmd.revs, prefix);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"Marking reachable objects...\");\n-\t\tmark_reachable_objects(&cb.revs, 0, 0, NULL);\n+\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tputchar('\\n');\n \t}\n@@ -712,7 +711,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tfor_each_reflog(collect_reflog, &collected);\n \t\tfor (i = 0; i < collected.nr; i++) {\n \t\t\tstruct collected_reflog *e = collected.e[i];\n-\t\t\tset_reflog_expiry_param(&cb, explicit_expiry, e->reflog);\n+\t\t\tset_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);\n \t\t\tstatus |= expire_reflog(e->reflog, e->sha1, flags, &cb);\n \t\t\tfree(e);\n \t\t}\n@@ -726,7 +725,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n \t\t\tcontinue;\n \t\t}\n-\t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n+\t\tset_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);\n \t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n \t}\n \treturn status;\n@@ -736,15 +735,15 @@ static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tconst char *email, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n-\tstruct cmd_reflog_expire_cb *cb = cb_data;\n-\tif (!cb->expire_total || timestamp < cb->expire_total)\n-\t\tcb->recno++;\n+\tstruct expire_reflog_policy_cb *cb = cb_data;\n+\tif (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)\n+\t\tcb->cmd.recno++;\n \treturn 0;\n }\n \n static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n {\n-\tstruct cmd_reflog_expire_cb cb;\n+\tstruct expire_reflog_policy_cb cb;\n \tint i, status = 0;\n \tunsigned int flags = 0;\n \n@@ -791,12 +790,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \n \t\trecno = strtoul(spec + 2, &ep, 10);\n \t\tif (*ep == '}') {\n-\t\t\tcb.recno = -recno;\n+\t\t\tcb.cmd.recno = -recno;\n \t\t\tfor_each_reflog_ent(ref, count_reflog_ent, &cb);\n \t\t} else {\n-\t\t\tcb.expire_total = approxidate(spec + 2);\n+\t\t\tcb.cmd.expire_total = approxidate(spec + 2);\n \t\t\tfor_each_reflog_ent(ref, count_reflog_ent, &cb);\n-\t\t\tcb.expire_total = 0;\n+\t\t\tcb.cmd.expire_total = 0;\n \t\t}\n \n \t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n-- \n2.1.3\n"},{"id":"253116","messageId":"1417734515-11812-21-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 20/23] reflog_expire(): new function in the reference API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:32Z","receivedAt":"2014-12-04T23:08:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move expire_reflog() into refs.c and rename it to reflog_expire().\nTurn the three policy functions into function pointers that are passed\ninto reflog_expire(). Add function prototypes and documentation to\nrefs.h.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/reflog.c | 133 +++++++------------------------------------------------\n refs.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++\n refs.h | 45 +++++++++++++++++++\n 3 files changed, 174 insertions(+), 118 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex c30936bb..49c64f9 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -20,13 +20,6 @@ static const char reflog_delete_usage[] =\n static unsigned long default_reflog_expire;\n static unsigned long default_reflog_expire_unreachable;\n \n-enum expire_reflog_flags {\n-\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n-\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n-\tEXPIRE_REFLOGS_VERBOSE = 1 << 2,\n-\tEXPIRE_REFLOGS_REWRITE = 1 << 3\n-};\n-\n struct cmd_reflog_expire_cb {\n \tstruct rev_info revs;\n \tint stalefix;\n@@ -48,13 +41,6 @@ struct expire_reflog_policy_cb {\n \tstruct commit_list *tips;\n };\n \n-struct expire_reflog_cb {\n-\tunsigned int flags;\n-\tvoid *policy_cb;\n-\tFILE *newlog;\n-\tunsigned char last_kept_sha1[20];\n-};\n-\n struct collected_reflog {\n \tunsigned char sha1[20];\n \tchar reflog[FLEX_ARRAY];\n@@ -330,38 +316,6 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \treturn 0;\n }\n \n-static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n-{\n-\tstruct expire_reflog_cb *cb = cb_data;\n-\tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n-\n-\tif (cb->flags & EXPIRE_REFLOGS_REWRITE)\n-\t\tosha1 = cb->last_kept_sha1;\n-\n-\tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n-\t\t\t\t message, policy_cb)) {\n-\t\tif (!cb->newlog)\n-\t\t\tprintf(\"would prune %s\", message);\n-\t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n-\t\t\tprintf(\"prune %s\", message);\n-\t} else {\n-\t\tif (cb->newlog) {\n-\t\t\tchar sign = (tz < 0) ? '-' : '+';\n-\t\t\tint zone = (tz < 0) ? (-tz) : tz;\n-\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n-\t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n-\t\t\t\temail, timestamp, sign, zone,\n-\t\t\t\tmessage);\n-\t\t\thashcpy(cb->last_kept_sha1, nsha1);\n-\t\t}\n-\t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n-\t\t\tprintf(\"keep %s\", message);\n-\t}\n-\treturn 0;\n-}\n-\n static int push_tip_to_list(const char *refname, const unsigned char *sha1,\n \t\t\t int flags, void *cb_data)\n {\n@@ -428,75 +382,6 @@ static void reflog_expiry_cleanup(void *cb_data)\n \t}\n }\n \n-static struct lock_file reflog_lock;\n-\n-static int expire_reflog(const char *refname, const unsigned char *sha1,\n-\t\t\t unsigned int flags, void *policy_cb_data)\n-{\n-\tstruct expire_reflog_cb cb;\n-\tstruct ref_lock *lock;\n-\tchar *log_file;\n-\tint status = 0;\n-\n-\tmemset(&cb, 0, sizeof(cb));\n-\tcb.flags = flags;\n-\tcb.policy_cb = policy_cb_data;\n-\n-\t/*\n-\t * we take the lock for the ref itself to prevent it from\n-\t * getting updated.\n-\t */\n-\tlock = lock_any_ref_for_update(refname, sha1, 0, NULL);\n-\tif (!lock)\n-\t\treturn error(\"cannot lock ref '%s'\", refname);\n-\tif (!reflog_exists(refname)) {\n-\t\tunlock_ref(lock);\n-\t\treturn 0;\n-\t}\n-\n-\tlog_file = git_pathdup(\"logs/%s\", refname);\n-\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n-\t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n-\t\t\tgoto failure;\n-\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n-\t\tif (!cb.newlog)\n-\t\t\tgoto failure;\n-\t}\n-\n-\treflog_expiry_prepare(refname, sha1, cb.policy_cb);\n-\tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n-\treflog_expiry_cleanup(cb.policy_cb);\n-\n-\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n-\t\tif (close_lock_file(&reflog_lock)) {\n-\t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n-\t\t\t\t\tstrerror(errno));\n-\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n-\t\t\t(write_in_full(lock->lock_fd,\n-\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n-\t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n-\t\t\t close_ref(lock) < 0)) {\n-\t\t\tstatus |= error(\"Couldn't write %s\",\n-\t\t\t\t\tlock->lk->filename.buf);\n-\t\t\trollback_lock_file(&reflog_lock);\n-\t\t} else if (commit_lock_file(&reflog_lock)) {\n-\t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n-\t\t\t\t\tlog_file, log_file);\n-\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n-\t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n-\t\t}\n-\t}\n-\tfree(log_file);\n-\tunlock_ref(lock);\n-\treturn status;\n-\n- failure:\n-\trollback_lock_file(&reflog_lock);\n-\tfree(log_file);\n-\tunlock_ref(lock);\n-\treturn -1;\n-}\n-\n static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)\n {\n \tstruct collected_reflog *e;\n@@ -712,7 +597,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tfor (i = 0; i < collected.nr; i++) {\n \t\t\tstruct collected_reflog *e = collected.e[i];\n \t\t\tset_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);\n-\t\t\tstatus |= expire_reflog(e->reflog, e->sha1, flags, &cb);\n+\t\t\tstatus |= reflog_expire(e->reflog, e->sha1, flags,\n+\t\t\t\t\t\treflog_expiry_prepare,\n+\t\t\t\t\t\tshould_expire_reflog_ent,\n+\t\t\t\t\t\treflog_expiry_cleanup,\n+\t\t\t\t\t\t&cb);\n \t\t\tfree(e);\n \t\t}\n \t\tfree(collected.e);\n@@ -726,7 +615,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \t\tset_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);\n-\t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n+\t\tstatus |= reflog_expire(ref, sha1, flags,\n+\t\t\t\t\treflog_expiry_prepare,\n+\t\t\t\t\tshould_expire_reflog_ent,\n+\t\t\t\t\treflog_expiry_cleanup,\n+\t\t\t\t\t&cb);\n \t}\n \treturn status;\n }\n@@ -798,7 +691,11 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\t\tcb.cmd.expire_total = 0;\n \t\t}\n \n-\t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n+\t\tstatus |= reflog_expire(ref, sha1, flags,\n+\t\t\t\t\treflog_expiry_prepare,\n+\t\t\t\t\tshould_expire_reflog_ent,\n+\t\t\t\t\treflog_expiry_cleanup,\n+\t\t\t\t\t&cb);\n \t\tfree(ref);\n \t}\n \treturn status;\ndiff --git a/refs.c b/refs.c\nindex 150c980..5a12d37 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3943,3 +3943,117 @@ int ref_is_hidden(const char *refname)\n \t}\n \treturn 0;\n }\n+\n+struct expire_reflog_cb {\n+\tunsigned int flags;\n+\treflog_expiry_select_fn *select_fn;\n+\tvoid *policy_cb;\n+\tFILE *newlog;\n+\tunsigned char last_kept_sha1[20];\n+};\n+\n+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *message, void *cb_data)\n+{\n+\tstruct expire_reflog_cb *cb = cb_data;\n+\tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n+\n+\tif (cb->flags & EXPIRE_REFLOGS_REWRITE)\n+\t\tosha1 = cb->last_kept_sha1;\n+\n+\tif ((*cb->select_fn)(osha1, nsha1, email, timestamp, tz,\n+\t\t\t message, policy_cb)) {\n+\t\tif (!cb->newlog)\n+\t\t\tprintf(\"would prune %s\", message);\n+\t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n+\t\t\tprintf(\"prune %s\", message);\n+\t} else {\n+\t\tif (cb->newlog) {\n+\t\t\tchar sign = (tz < 0) ? '-' : '+';\n+\t\t\tint zone = (tz < 0) ? (-tz) : tz;\n+\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n+\t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n+\t\t\t\temail, timestamp, sign, zone,\n+\t\t\t\tmessage);\n+\t\t\thashcpy(cb->last_kept_sha1, nsha1);\n+\t\t}\n+\t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n+\t\t\tprintf(\"keep %s\", message);\n+\t}\n+\treturn 0;\n+}\n+\n+static struct lock_file reflog_lock;\n+\n+extern int reflog_expire(const char *refname, const unsigned char *sha1,\n+\t\t\t unsigned int flags,\n+\t\t\t reflog_expiry_prepare_fn prepare_fn,\n+\t\t\t reflog_expiry_select_fn select_fn,\n+\t\t\t reflog_expiry_cleanup_fn cleanup_fn,\n+\t\t\t void *policy_cb_data)\n+{\n+\tstruct expire_reflog_cb cb;\n+\tstruct ref_lock *lock;\n+\tchar *log_file;\n+\tint status = 0;\n+\n+\tmemset(&cb, 0, sizeof(cb));\n+\tcb.flags = flags;\n+\tcb.policy_cb = policy_cb_data;\n+\tcb.select_fn = select_fn;\n+\n+\t/*\n+\t * we take the lock for the ref itself to prevent it from\n+\t * getting updated.\n+\t */\n+\tlock = lock_any_ref_for_update(refname, sha1, 0, NULL);\n+\tif (!lock)\n+\t\treturn error(\"cannot lock ref '%s'\", refname);\n+\tif (!reflog_exists(refname)) {\n+\t\tunlock_ref(lock);\n+\t\treturn 0;\n+\t}\n+\n+\tlog_file = git_pathdup(\"logs/%s\", refname);\n+\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n+\t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n+\t\t\tgoto failure;\n+\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n+\t\tif (!cb.newlog)\n+\t\t\tgoto failure;\n+\t}\n+\n+\t(*prepare_fn)(refname, sha1, cb.policy_cb);\n+\tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n+\t(*cleanup_fn)(cb.policy_cb);\n+\n+\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n+\t\tif (close_lock_file(&reflog_lock)) {\n+\t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n+\t\t\t\t\tstrerror(errno));\n+\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n+\t\t\t(write_in_full(lock->lock_fd,\n+\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n+\t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n+\t\t\t close_ref(lock) < 0)) {\n+\t\t\tstatus |= error(\"Couldn't write %s\",\n+\t\t\t\t\tlock->lk->filename.buf);\n+\t\t\trollback_lock_file(&reflog_lock);\n+\t\t} else if (commit_lock_file(&reflog_lock)) {\n+\t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n+\t\t\t\t\tlog_file, log_file);\n+\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n+\t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n+\t\t}\n+\t}\n+\tfree(log_file);\n+\tunlock_ref(lock);\n+\treturn status;\n+\n+ failure:\n+\trollback_lock_file(&reflog_lock);\n+\tfree(log_file);\n+\tunlock_ref(lock);\n+\treturn -1;\n+}\ndiff --git a/refs.h b/refs.h\nindex 7d675b7..d777f76 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -353,4 +353,49 @@ int update_ref(const char *action, const char *refname,\n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n extern int ref_is_hidden(const char *);\n \n+enum expire_reflog_flags {\n+\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n+\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n+\tEXPIRE_REFLOGS_VERBOSE = 1 << 2,\n+\tEXPIRE_REFLOGS_REWRITE = 1 << 3\n+};\n+\n+/*\n+ * The following interface is used for reflog expiration. The caller\n+ * calls reflog_expire(), supplying it with three callback functions,\n+ * of the following types. The callback functions define the\n+ * expiration policy that is desired.\n+ *\n+ * reflog_expiry_prepare_fn -- Called once after the reference is\n+ * locked.\n+ *\n+ * reflog_expiry_select_fn -- Called once for each entry in the\n+ * existing reflog. It should return true iff that entry should be\n+ * pruned.\n+ *\n+ * reflog_expiry_cleanup_fn -- Called once before the reference is\n+ * unlocked again.\n+ */\n+typedef void reflog_expiry_prepare_fn(const char *refname,\n+\t\t\t\t const unsigned char *sha1,\n+\t\t\t\t void *cb_data);\n+typedef int reflog_expiry_select_fn(unsigned char *osha1, unsigned char *nsha1,\n+\t\t\t\t const char *email,\n+\t\t\t\t unsigned long timestamp, int tz,\n+\t\t\t\t const char *message, void *cb_data);\n+typedef void reflog_expiry_cleanup_fn(void *cb_data);\n+\n+/*\n+ * Expire reflog entries for the specified reference. sha1 is the old\n+ * value of the reference. flags is a combination of the constants in\n+ * enum expire_reflog_flags. The three function pointers are described\n+ * above. On success, return zero.\n+ */\n+extern int reflog_expire(const char *refname, const unsigned char *sha1,\n+\t\t\t unsigned int flags,\n+\t\t\t reflog_expiry_prepare_fn prepare_fn,\n+\t\t\t reflog_expiry_select_fn select_fn,\n+\t\t\t reflog_expiry_cleanup_fn cleanup_fn,\n+\t\t\t void *policy_cb_data);\n+\n #endif /* REFS_H */\n-- \n2.1.3\n"},{"id":"253128","messageId":"1417734515-11812-22-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 21/23] refs.c: remove unlock_ref/close_ref/commit_ref from the refs api","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:33Z","receivedAt":"2014-12-04T23:08:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Ronnie Sahlberg \n\nunlock|close|commit_ref can be made static since there are no more external\ncallers.\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \nSigned-off-by: Michael Haggerty \n---\n refs.c | 24 ++++++++++++------------\n refs.h | 9 ---------\n 2 files changed, 12 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5a12d37..c6ee775 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2090,6 +2090,16 @@ int refname_match(const char *abbrev_name, const char *full_name)\n \treturn 0;\n }\n \n+static void unlock_ref(struct ref_lock *lock)\n+{\n+\t/* Do not free lock->lk -- atexit() still looks at them */\n+\tif (lock->lk)\n+\t\trollback_lock_file(lock->lk);\n+\tfree(lock->ref_name);\n+\tfree(lock->orig_ref_name);\n+\tfree(lock);\n+}\n+\n /* This function should make sure errno is meaningful on error */\n static struct ref_lock *verify_lock(struct ref_lock *lock,\n \tconst unsigned char *old_sha1, int mustexist)\n@@ -2888,7 +2898,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \treturn 1;\n }\n \n-int close_ref(struct ref_lock *lock)\n+static int close_ref(struct ref_lock *lock)\n {\n \tif (close_lock_file(lock->lk))\n \t\treturn -1;\n@@ -2896,7 +2906,7 @@ int close_ref(struct ref_lock *lock)\n \treturn 0;\n }\n \n-int commit_ref(struct ref_lock *lock)\n+static int commit_ref(struct ref_lock *lock)\n {\n \tif (commit_lock_file(lock->lk))\n \t\treturn -1;\n@@ -2904,16 +2914,6 @@ int commit_ref(struct ref_lock *lock)\n \treturn 0;\n }\n \n-void unlock_ref(struct ref_lock *lock)\n-{\n-\t/* Do not free lock->lk -- atexit() still looks at them */\n-\tif (lock->lk)\n-\t\trollback_lock_file(lock->lk);\n-\tfree(lock->ref_name);\n-\tfree(lock->orig_ref_name);\n-\tfree(lock);\n-}\n-\n /*\n * copy the reflog message msg to buf, which has been allocated sufficiently\n * large, while cleaning up the whitespaces. Especially, convert LF to space,\ndiff --git a/refs.h b/refs.h\nindex d777f76..82611b5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -198,15 +198,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t\tconst unsigned char *old_sha1,\n \t\t\t\t\t\tint flags, int *type_p);\n \n-/** Close the file descriptor owned by a lock and return the status */\n-extern int close_ref(struct ref_lock *lock);\n-\n-/** Close and commit the ref locked by the lock */\n-extern int commit_ref(struct ref_lock *lock);\n-\n-/** Release any lock taken but not written. **/\n-extern void unlock_ref(struct ref_lock *lock);\n-\n /*\n * Setup reflog before using. Set errno to something meaningful on failure.\n */\n-- \n2.1.3\n"},{"id":"253117","messageId":"1417734515-11812-23-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 22/23] lock_any_ref_for_update(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:34Z","receivedAt":"2014-12-04T23:08:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Ronnie Sahlberg \n\nInline the function at its one remaining caller (which is within\nrefs.c) and remove it.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 9 +--------\n refs.h | 9 +--------\n 2 files changed, 2 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c6ee775..f3564cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2346,13 +2346,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \treturn NULL;\n }\n \n-struct ref_lock *lock_any_ref_for_update(const char *refname,\n-\t\t\t\t\t const unsigned char *old_sha1,\n-\t\t\t\t\t int flags, int *type_p)\n-{\n-\treturn lock_ref_sha1_basic(refname, old_sha1, NULL, flags, type_p);\n-}\n-\n /*\n * Write an entry to the packed-refs file for the specified refname.\n * If peeled is non-NULL, write it as the entry's peeled value.\n@@ -4007,7 +4000,7 @@ extern int reflog_expire(const char *refname, const unsigned char *sha1,\n \t * we take the lock for the ref itself to prevent it from\n \t * getting updated.\n \t */\n-\tlock = lock_any_ref_for_update(refname, sha1, 0, NULL);\n+\tlock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);\n \tif (!lock)\n \t\treturn error(\"cannot lock ref '%s'\", refname);\n \tif (!reflog_exists(refname)) {\ndiff --git a/refs.h b/refs.h\nindex 82611b5..174863d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -181,8 +181,7 @@ extern int is_branch(const char *refname);\n extern int peel_ref(const char *refname, unsigned char *sha1);\n \n /*\n- * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),\n- * ref_transaction_create(), etc.\n+ * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.\n * REF_NODEREF: act on the ref directly, instead of dereferencing\n * symbolic references.\n * REF_DELETING: tolerate broken refs\n@@ -191,12 +190,6 @@ extern int peel_ref(const char *refname, unsigned char *sha1);\n */\n #define REF_NODEREF\t0x01\n #define REF_DELETING\t0x02\n-/*\n- * This function sets errno to something meaningful on failure.\n- */\n-extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n-\t\t\t\t\t\tconst unsigned char *old_sha1,\n-\t\t\t\t\t\tint flags, int *type_p);\n \n /*\n * Setup reflog before using. Set errno to something meaningful on failure.\n-- \n2.1.3\n"},{"id":"253118","messageId":"1417734515-11812-24-git-send-email-mhagger@alum.mit.edu","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 23/23] refs.c: don't expose the internal struct ref_lock in the header file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-04T23:08:35Z","receivedAt":"2014-12-04T23:08:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Stefan Beller \n\nNow the struct ref_lock is used completely internally, so let's\nremove it from the header file.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Michael Haggerty \n---\n refs.c | 9 +++++++++\n refs.h | 9 ---------\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f3564cf..40c5591 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -6,6 +6,15 @@\n #include \"dir.h\"\n #include \"string-list.h\"\n \n+struct ref_lock {\n+\tchar *ref_name;\n+\tchar *orig_ref_name;\n+\tstruct lock_file *lk;\n+\tunsigned char old_sha1[20];\n+\tint lock_fd;\n+\tint force_write;\n+};\n+\n /*\n * How to handle various characters in refnames:\n * 0: An acceptable character for refs\ndiff --git a/refs.h b/refs.h\nindex 174863d..2957641 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -1,15 +1,6 @@\n #ifndef REFS_H\n #define REFS_H\n \n-struct ref_lock {\n-\tchar *ref_name;\n-\tchar *orig_ref_name;\n-\tstruct lock_file *lk;\n-\tunsigned char old_sha1[20];\n-\tint lock_fd;\n-\tint force_write;\n-};\n-\n /*\n * A ref_transaction represents a collection of ref updates\n * that should succeed or fail together.\n-- \n2.1.3\n"},{"id":"253129","messageId":"20141204232054.GC16345@google.com","threadId":"38113","inReplyTo":"1417734515-11812-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 04/23] expire_reflog(): remove unused parameter","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T23:20:54Z","receivedAt":"2014-12-04T23:20:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> It was called \"unused\", so at least it was self-consistent.\n\nThe missing context is that this was a callback function that had to\nmatch the each_ref_fn signature (where that parameter is 'flags')\nuntil v1.5.4~14 (reflog-expire: avoid creating new files in a\ndirectory inside readdir(3) loop, 2008-01-25). v1.5.4~14 forgot to\nclean up.\n\nWith or without a note in the commit message explaining that,\nReviewed-by: Jonathan Nieder \n"},{"id":"253131","messageId":"20141204232853.GD16345@google.com","threadId":"38113","inReplyTo":"1417734515-11812-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 04/23] expire_reflog(): remove unused parameter","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T23:28:53Z","receivedAt":"2014-12-04T23:28:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n> \treturn 0;\n> }\n> \n> -static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)\n> +static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_data)\n> {\n> \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n\nOn second thought: why not update the last parameter to be a 'struct\ncmd_reflog_expire_cb *' instead of 'void *' while at it, like this?\n\n builtin/reflog.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git i/builtin/reflog.c w/builtin/reflog.c\nindex 3e11bee..d860624 100644\n--- i/builtin/reflog.c\n+++ w/builtin/reflog.c\n@@ -349,9 +349,8 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n \treturn 0;\n }\n \n-static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_data)\n+static int expire_reflog(const char *ref, const unsigned char *sha1, struct cmd_reflog_expire_cb *cmd)\n {\n-\tstruct cmd_reflog_expire_cb *cmd = cb_data;\n \tstruct expire_reflog_cb cb;\n \tstruct ref_lock *lock;\n \tchar *log_file, *newlog_path = NULL;\n"},{"id":"253135","messageId":"20141204234422.GG16345@google.com","threadId":"38113","inReplyTo":"1417734515-11812-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 05/23] expire_reflog(): rename \"ref\" parameter to \"refname\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T23:44:22Z","receivedAt":"2014-12-04T23:44:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/reflog.c | 16 ++++++++--------\n> 1 file changed, 8 insertions(+), 8 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"253137","messageId":"xmqqtx1bq8on.fsf@gitster.dls.corp.google.com","threadId":"38113","inReplyTo":"1417734515-11812-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/23] Add reflog_expire() to the references API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-04T23:47:20Z","receivedAt":"2014-12-04T23:47:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I propose this patch series as an alternative to Ronnie's \"reflog\n> transactions\" series.\n\nAfter coasting my eyes over individual patches, it was delightful to\nlook at the endgame state of the expiry codepath in builtin/reflog.c\nthat is left ;-)\n"},{"id":"253138","messageId":"20141204234801.GH16345@google.com","threadId":"38113","inReplyTo":"1417734515-11812-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T23:48:02Z","receivedAt":"2014-12-04T23:48:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> \tlock = lock_any_ref_for_update(refname, sha1, 0, NULL);\n> \tif (!lock)\n> \t\treturn error(\"cannot lock ref '%s'\", refname);\n> +\tif (!reflog_exists(refname)) {\n> +\t\tunlock_ref(lock);\n> +\t\treturn 0;\n> +\t}\n> \tlog_file = git_pathdup(\"logs/%s\", refname);\n> -\tif (!reflog_exists(refname))\n> -\t\tgoto finish;\n\nOkay.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"253140","messageId":"20141204235323.GI16345@google.com","threadId":"38113","inReplyTo":"1417734515-11812-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T23:53:23Z","receivedAt":"2014-12-04T23:53:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> [Subject: expire_reflog(): exit early if the reference has no reflog]\n\nThe caller moves on to expire other reflogs, so it's not exiting.\n\"return early\", maybe?\n\nExcept the function already returned early. I think the purpose of\nthis patch is to simplify the no-reflog case by handling it separately.\n\nAnyway, that's just nitpicking about the subject line. With\ns/exit/return/ it should be clear that this is a refactoring change,\nwhich for someone looking at the shortlog is the important thing.\n\nThanks,\nJonathan\n"},{"id":"253143","messageId":"20141205002331.GJ16345@google.com","threadId":"38113","inReplyTo":"1417734515-11812-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-05T00:23:31Z","receivedAt":"2014-12-05T00:23:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> We don't actually need the locking functionality, because we already\n> hold the lock on the reference itself, which is how the reflog file is\n> locked. But the lock_file code still does some of the bookkeeping for\n> us and is more careful than the old code here was.\n\nAs you say, the ref lock takes care of mutual exclusion, so we do not\nhave to be too careful about compatibility with other tools that might\nnot know to lock the reflog. And this is not tying our hands for a\nfuture when I might want to lock logs/refs/heads/topic/1 while\nlogs/refs/heads/topic still exists as part of the implementation of\n\"git mv topic/1 topic\".\n\nStefan and I had forgotten about that guarantee when looking at that\nkind of operation --- thanks for the reminder.\n\nShould updates to the HEAD reflog acquire HEAD.lock? (They don't\ncurrently.)\n\n[...]\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n> \treturn 0;\n> }\n> \n> +static struct lock_file reflog_lock;\n\nIf this lockfile is only used in that one function, it can be declared\ninside the function.\n\nIf it is meant to be used throughout the 'git reflog' command, then it\ncan go near the top of the file.\n\n> +\n> static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n> {\n> \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n> \tstruct expire_reflog_cb cb;\n> \tstruct ref_lock *lock;\n> -\tchar *log_file, *newlog_path = NULL;\n> +\tchar *log_file;\n> \tstruct commit *tip_commit;\n> \tstruct commit_list *tips;\n> \tint status = 0;\n> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> \t\tunlock_ref(lock);\n> \t\treturn 0;\n> \t}\n> +\n> \tlog_file = git_pathdup(\"logs/%s\", refname);\n> \tif (!cmd->dry_run) {\n> -\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", refname);\n> -\t\tcb.newlog = fopen(newlog_path, \"w\");\n> +\t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n> +\t\t\tgoto failure;\n\nhold_lock_file_for_update doesn't print a message. Code to print one\nlooks like\n\n\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {\n\t\tunable_to_lock_message(log_file, errno, &err);\n\t\terror(\"%s\", err.buf);\n\t\tgoto failure;\n\t}\n\n(A patch in flight changes that to\n\n\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {\n\t\terror(\"%s\", err.buf);\n\t\tgoto failure;\n\t}\n\n)\n\n> +\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> +\t\tif (!cb.newlog)\n> +\t\t\tgoto failure;\n\nHm. lockfile.c::fdopen_lock_file ought to use xfdopen to make this\ncase impossible. And xfdopen should use try_to_free_routine() and\ntry again on failure.\n\n[...]\n> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> \t}\n> \n> \tif (cb.newlog) {\n> -\t\tif (fclose(cb.newlog)) {\n> -\t\t\tstatus |= error(\"%s: %s\", strerror(errno),\n> -\t\t\t\t\tnewlog_path);\n> -\t\t\tunlink(newlog_path);\n> +\t\tif (close_lock_file(&reflog_lock)) {\n> +\t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n> +\t\t\t\t\tstrerror(errno));\n\nStyle nit: error messages usually start with a lowercase letter\n(though I realize nearby examples are already inconsistent).\n\ncommit_lock_file() can take care of the close_lock_file automatically.\n\n[...]\n> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> \t\t\t close_ref(lock) < 0)) {\n> \t\t\tstatus |= error(\"Couldn't write %s\",\n> \t\t\t\t\tlock->lk->filename.buf);\n> -\t\t\tunlink(newlog_path);\n> -\t\t} else if (rename(newlog_path, log_file)) {\n> -\t\t\tstatus |= error(\"cannot rename %s to %s\",\n> -\t\t\t\t\tnewlog_path, log_file);\n> -\t\t\tunlink(newlog_path);\n> +\t\t\trollback_lock_file(&reflog_lock);\n> +\t\t} else if (commit_lock_file(&reflog_lock)) {\n> +\t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n> +\t\t\t\t\tlog_file, log_file);\n\nMost callers say \"unable to commit reflog '%s'\", log_file to hedge their\nbets in case the close failed (which may be what you were avoiding\nabove.\n\nerrno is meaningful when commit_lock_file fails, making a more\ndetailed diagnosis from strerror(errno) possible.\n\nThanks,\nJonathan\n"},{"id":"253148","messageId":"20141205021931.GA29570@google.com","threadId":"38113","inReplyTo":"20141205002331.GJ16345@google.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-05T02:19:31Z","receivedAt":"2014-12-05T02:19:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n> > We don't actually need the locking functionality, because we already\n> > hold the lock on the reference itself, which is how the reflog file is\n> > locked. But the lock_file code still does some of the bookkeeping for\n> > us and is more careful than the old code here was.\n> \n> As you say, the ref lock takes care of mutual exclusion, so we do not\n> have to be too careful about compatibility with other tools that might\n> not know to lock the reflog. And this is not tying our hands for a\n> future when I might want to lock logs/refs/heads/topic/1 while\n> logs/refs/heads/topic still exists as part of the implementation of\n> \"git mv topic/1 topic\".\n> \n> Stefan and I had forgotten about that guarantee when looking at that\n> kind of operation --- thanks for the reminder.\n\nI did not forget about it, I did not know about that in the first hand.\nWe don't seem to have documentation on it?\n\nSo sorry for heading in a direction, which would have been avoidable.\n\nThanks,\nStefan\n"},{"id":"253150","messageId":"CAN05THTH2XDaUmchehF0gY0-GSgD_O9rJH7F5Gc3dHLCPz3GsA@mail.gmail.com","threadId":"38113","inReplyTo":"1417734515-11812-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"ronnie sahlberg","fromEmail":"ronniesahlberg@gmail.com","sentAt":"2014-12-05T02:59:01Z","receivedAt":"2014-12-05T02:59:01Z","isPatch":true,"sender":{"key":"ronniesahlberg@gmail.com","avatar":null},"body":"On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty wrote:\n> We don't actually need the locking functionality, because we already\n> hold the lock on the reference itself,\n\nNo. You do need the lock.\nThe ref is locked only during transaction_commit()\n\nIf you don't want to lock the reflog file and instead rely on the lock\non the ref itself you will need to\nrework your patches so that the lock on the ref is taken already\nduring, for example, transaction_update_ref() instead.\n\nBut without doing those changes and moving the ref locking from\n_commit() to _update_ref() you will risk reflog corruption/surprises\nif two operations collide and both rewrite the reflog without any lock held.\n\n\n> which is how the reflog file is\n> locked. But the lock_file code still does some of the bookkeeping for\n> us and is more careful than the old code here was.\n>\n> For example:\n>\n> * Correctly handle the case that the reflog lock file already exists\n> for some reason or cannot be opened.\n>\n> * Correctly clean up the lockfile if the program dies.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/reflog.c | 37 ++++++++++++++++++++++---------------\n> 1 file changed, 22 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index a282e60..d344d45 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n> return 0;\n> }\n>\n> +static struct lock_file reflog_lock;\n> +\n> static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n> {\n> struct cmd_reflog_expire_cb *cmd = cb_data;\n> struct expire_reflog_cb cb;\n> struct ref_lock *lock;\n> - char *log_file, *newlog_path = NULL;\n> + char *log_file;\n> struct commit *tip_commit;\n> struct commit_list *tips;\n> int status = 0;\n> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> unlock_ref(lock);\n> return 0;\n> }\n> +\n> log_file = git_pathdup(\"logs/%s\", refname);\n> if (!cmd->dry_run) {\n> - newlog_path = git_pathdup(\"logs/%s.lock\", refname);\n> - cb.newlog = fopen(newlog_path, \"w\");\n> + if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n> + goto failure;\n> + cb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> + if (!cb.newlog)\n> + goto failure;\n> }\n>\n> cb.cmd = cmd;\n> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> }\n>\n> if (cb.newlog) {\n> - if (fclose(cb.newlog)) {\n> - status |= error(\"%s: %s\", strerror(errno),\n> - newlog_path);\n> - unlink(newlog_path);\n> + if (close_lock_file(&reflog_lock)) {\n> + status |= error(\"Couldn't write %s: %s\", log_file,\n> + strerror(errno));\n> } else if (cmd->updateref &&\n> (write_in_full(lock->lock_fd,\n> sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> close_ref(lock) < 0)) {\n> status |= error(\"Couldn't write %s\",\n> lock->lk->filename.buf);\n> - unlink(newlog_path);\n> - } else if (rename(newlog_path, log_file)) {\n> - status |= error(\"cannot rename %s to %s\",\n> - newlog_path, log_file);\n> - unlink(newlog_path);\n> + rollback_lock_file(&reflog_lock);\n> + } else if (commit_lock_file(&reflog_lock)) {\n> + status |= error(\"cannot rename %s.lock to %s\",\n> + log_file, log_file);\n> } else if (cmd->updateref && commit_ref(lock)) {\n> status |= error(\"Couldn't set %s\", lock->ref_name);\n> - } else {\n> - adjust_shared_perm(log_file);\n> }\n> }\n> - free(newlog_path);\n> free(log_file);\n> unlock_ref(lock);\n> return status;\n> +\n> + failure:\n> + rollback_lock_file(&reflog_lock);\n> + free(log_file);\n> + unlock_ref(lock);\n> + return -1;\n> }\n>\n> static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)\n> --\n> 2.1.3\n>\n"},{"id":"253162","messageId":"5481A88A.7050509@alum.mit.edu","threadId":"38113","inReplyTo":"20141204232853.GD16345@google.com","subject":"Re: [PATCH 04/23] expire_reflog(): remove unused parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-05T12:43:54Z","receivedAt":"2014-12-05T12:43:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/05/2014 12:28 AM, Jonathan Nieder wrote:> Michael Haggerty wrote:\n>> It was called \"unused\", so at least it was self-consistent.\n>\n> The missing context is that this was a callback function that had to\n> match the each_ref_fn signature [...]\n>\n> With or without a note in the commit message explaining that,\n> Reviewed-by: Jonathan Nieder \n\n[...]\n\n>> --- a/builtin/reflog.c\n>> +++ b/builtin/reflog.c\n>> @@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname,\nconst unsigned char *sha1, int\n>> \treturn 0;\n>> }\n>>\n>> -static int expire_reflog(const char *ref, const unsigned char *sha1,\nint unused, void *cb_data)\n>> +static int expire_reflog(const char *ref, const unsigned char *sha1,\nvoid *cb_data)\n>> {\n>> \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n>\n> On second thought: why not update the last parameter to be a 'struct\n> cmd_reflog_expire_cb *' instead of 'void *' while at it, like this?\n> [...]\n\nThanks for the explanation, the review, and the suggestion. I will\nexpand the commit to be \"don't implement each_ref_fn anymore\" and\nincorporate all of your suggestions.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253163","messageId":"5481CADA.7050507@alum.mit.edu","threadId":"38113","inReplyTo":"20141204235323.GI16345@google.com","subject":"Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-05T15:10:18Z","receivedAt":"2014-12-05T15:10:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/05/2014 12:53 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> [Subject: expire_reflog(): exit early if the reference has no reflog]\n> \n> The caller moves on to expire other reflogs, so it's not exiting.\n> \"return early\", maybe?\n> \n> Except the function already returned early. I think the purpose of\n> this patch is to simplify the no-reflog case by handling it separately.\n> \n> Anyway, that's just nitpicking about the subject line. With\n> s/exit/return/ it should be clear that this is a refactoring change,\n> which for someone looking at the shortlog is the important thing.\n\nGood suggestion. I will change the commit message.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253171","messageId":"20141205174737.GB29570@google.com","threadId":"38113","inReplyTo":"CAN05THTTba-1n12hBszJAU-O+wsbSFd5Lt+kMk7_MU_0C=wZGQ@mail.gmail.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-05T17:47:37Z","receivedAt":"2014-12-05T17:47:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 04, 2014 at 06:51:36PM -0800, ronnie sahlberg wrote:\n> On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty \n> wrote:\n> \n> > We don't actually need the locking functionality, because we already\n> > hold the lock on the reference itself,\n> \n> \n> No. You do need the lock.\n> The ref is locked only during transaction_commit()\n> \n\nMichael is saying, that you never want to modify the reflog, if you're not holding\nthe lock on the corresponding ref. Or in other words, you may modify the reflog, if\nthe corresponding ref is held. This used to be this way back then. \n\n> If you don't want to lock the reflog file and instead rely on the lock on\n> the ref itself you will need to\n> rework your patches so that the lock on the ref is taken already during,\n> for example, transaction_update_ref() instead.\n> \n> But without doing those changes and moving the ref locking from _commit()\n> to _update_ref() you will risk reflog corruption/surprises\n> if two operations collide and both rewrite the reflog without any lock held.\n> \n> \n> \n> \n> \n> > which is how the reflog file is\n> > locked. But the lock_file code still does some of the bookkeeping for\n> > us and is more careful than the old code here was.\n> >\n> > For example:\n> >\n> > * Correctly handle the case that the reflog lock file already exists\n> > for some reason or cannot be opened.\n> >\n> > * Correctly clean up the lockfile if the program dies.\n> >\n> > Signed-off-by: Michael Haggerty \n> > ---\n> > builtin/reflog.c | 37 ++++++++++++++++++++++---------------\n> > 1 file changed, 22 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/builtin/reflog.c b/builtin/reflog.c\n> > index a282e60..d344d45 100644\n> > --- a/builtin/reflog.c\n> > +++ b/builtin/reflog.c\n> > @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname,\n> > const unsigned char *sha1, int\n> > return 0;\n> > }\n> >\n> > +static struct lock_file reflog_lock;\n> > +\n> > static int expire_reflog(const char *refname, const unsigned char *sha1,\n> > void *cb_data)\n> > {\n> > struct cmd_reflog_expire_cb *cmd = cb_data;\n> > struct expire_reflog_cb cb;\n> > struct ref_lock *lock;\n> > - char *log_file, *newlog_path = NULL;\n> > + char *log_file;\n> > struct commit *tip_commit;\n> > struct commit_list *tips;\n> > int status = 0;\n> > @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const\n> > unsigned char *sha1, void *c\n> > unlock_ref(lock);\n> > return 0;\n> > }\n> > +\n> > log_file = git_pathdup(\"logs/%s\", refname);\n> > if (!cmd->dry_run) {\n> > - newlog_path = git_pathdup(\"logs/%s.lock\", refname);\n> > - cb.newlog = fopen(newlog_path, \"w\");\n> > + if (hold_lock_file_for_update(&reflog_lock, log_file, 0) <\n> > 0)\n> > + goto failure;\n> > + cb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> > + if (!cb.newlog)\n> > + goto failure;\n> > }\n> >\n> > cb.cmd = cmd;\n> > @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const\n> > unsigned char *sha1, void *c\n> > }\n> >\n> > if (cb.newlog) {\n> > - if (fclose(cb.newlog)) {\n> > - status |= error(\"%s: %s\", strerror(errno),\n> > - newlog_path);\n> > - unlink(newlog_path);\n> > + if (close_lock_file(&reflog_lock)) {\n> > + status |= error(\"Couldn't write %s: %s\", log_file,\n> > + strerror(errno));\n> > } else if (cmd->updateref &&\n> > (write_in_full(lock->lock_fd,\n> > sha1_to_hex(cb.last_kept_sha1), 40) != 40\n> > ||\n> > @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const\n> > unsigned char *sha1, void *c\n> > close_ref(lock) < 0)) {\n> > status |= error(\"Couldn't write %s\",\n> > lock->lk->filename.buf);\n> > - unlink(newlog_path);\n> > - } else if (rename(newlog_path, log_file)) {\n> > - status |= error(\"cannot rename %s to %s\",\n> > - newlog_path, log_file);\n> > - unlink(newlog_path);\n> > + rollback_lock_file(&reflog_lock);\n> > + } else if (commit_lock_file(&reflog_lock)) {\n> > + status |= error(\"cannot rename %s.lock to %s\",\n> > + log_file, log_file);\n> > } else if (cmd->updateref && commit_ref(lock)) {\n> > status |= error(\"Couldn't set %s\", lock->ref_name);\n> > - } else {\n> > - adjust_shared_perm(log_file);\n> > }\n> > }\n> > - free(newlog_path);\n> > free(log_file);\n> > unlock_ref(lock);\n> > return status;\n> > +\n> > + failure:\n> > + rollback_lock_file(&reflog_lock);\n> > + free(log_file);\n> > + unlock_ref(lock);\n> > + return -1;\n> > }\n> >\n> > static int collect_reflog(const char *ref, const unsigned char *sha1, int\n> > unused, void *cb_data)\n> > --\n> > 2.1.3\n> >\n> >\n"},{"id":"253176","messageId":"20141205190259.GA16682@google.com","threadId":"38113","inReplyTo":"1417734515-11812-10-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 09/23] expire_reflog(): extract two policy-related functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-05T19:02:59Z","receivedAt":"2014-12-05T19:02:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:21AM +0100, Michael Haggerty wrote:\n> Extract two functions, reflog_expiry_prepare() and\n> reflog_expiry_cleanup(), from expire_reflog(). This is a further step\n> towards separating the code for deciding on expiration policy from the\n> code that manages the physical expiration.\n> \n> This change requires a couple of local variables from expire_reflog()\n> to be turned into fields of \"struct expire_reflog_cb\". More\n> reorganization of the callback data will follow in later commits.\n> \n> Signed-off-by: Michael Haggerty \nReviewed-by: Stefan Beller \n\n> ---\n> In fact, the work done in reflog_expire_cleanup() doesn't really need\n> to be done via a callback, because it doesn't need to be done while\n> the reference lock is held. But the symmetry between prepare and\n> cleanup is kindof nice. Perhaps some future policy decision will want\n> to do some final work under the reference lock?\n> \n> But it would be easy to get rid of this third callback function and\n> have the callers do the work themselves after calling expire_reflog().\n> I don't have a string feeling either way.\n> \n> builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------\n> 1 file changed, 52 insertions(+), 42 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 7bc6e0f..ebfa635 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -43,6 +43,8 @@ struct expire_reflog_cb {\n> \tunsigned long mark_limit;\n> \tstruct cmd_reflog_expire_cb *cmd;\n> \tunsigned char last_kept_sha1[20];\n> +\tstruct commit *tip_commit;\n> +\tstruct commit_list *tips;\n> };\n> \n> struct collected_reflog {\n> @@ -363,6 +365,54 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n> \treturn 0;\n> }\n> \n> +static void reflog_expiry_prepare(const char *refname,\n> +\t\t\t\t const unsigned char *sha1,\n> +\t\t\t\t struct expire_reflog_cb *cb)\n> +{\n> +\tif (!cb->cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n> +\t\tcb->tip_commit = NULL;\n> +\t\tcb->unreachable_expire_kind = UE_HEAD;\n> +\t} else {\n> +\t\tcb->tip_commit = lookup_commit_reference_gently(sha1, 1);\n> +\t\tif (!cb->tip_commit)\n> +\t\t\tcb->unreachable_expire_kind = UE_ALWAYS;\n> +\t\telse\n> +\t\t\tcb->unreachable_expire_kind = UE_NORMAL;\n> +\t}\n> +\n> +\tif (cb->cmd->expire_unreachable <= cb->cmd->expire_total)\n> +\t\tcb->unreachable_expire_kind = UE_ALWAYS;\n> +\n> +\tcb->mark_list = NULL;\n> +\tcb->tips = NULL;\n> +\tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n> +\t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n> +\t\t\tstruct commit_list *elem;\n> +\t\t\tfor_each_ref(push_tip_to_list, &cb->tips);\n> +\t\t\tfor (elem = cb->tips; elem; elem = elem->next)\n> +\t\t\t\tcommit_list_insert(elem->item, &cb->mark_list);\n> +\t\t} else {\n> +\t\t\tcommit_list_insert(cb->tip_commit, &cb->mark_list);\n> +\t\t}\n> +\t\tcb->mark_limit = cb->cmd->expire_total;\n> +\t\tmark_reachable(cb);\n> +\t}\n> +}\n> +\n> +static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n> +{\n> +\tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n> +\t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n> +\t\t\tstruct commit_list *elem;\n> +\t\t\tfor (elem = cb->tips; elem; elem = elem->next)\n> +\t\t\t\tclear_commit_marks(elem->item, REACHABLE);\n> +\t\t\tfree_commit_list(cb->tips);\n> +\t\t} else {\n> +\t\t\tclear_commit_marks(cb->tip_commit, REACHABLE);\n> +\t\t}\n> +\t}\n> +}\n> +\n> static struct lock_file reflog_lock;\n> \n> static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n> @@ -371,8 +421,6 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> \tstruct expire_reflog_cb cb;\n> \tstruct ref_lock *lock;\n> \tchar *log_file;\n> -\tstruct commit *tip_commit;\n> -\tstruct commit_list *tips;\n> \tint status = 0;\n> \n> \tmemset(&cb, 0, sizeof(cb));\n> @@ -400,47 +448,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> \n> \tcb.cmd = cmd;\n> \n> -\tif (!cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n> -\t\ttip_commit = NULL;\n> -\t\tcb.unreachable_expire_kind = UE_HEAD;\n> -\t} else {\n> -\t\ttip_commit = lookup_commit_reference_gently(sha1, 1);\n> -\t\tif (!tip_commit)\n> -\t\t\tcb.unreachable_expire_kind = UE_ALWAYS;\n> -\t\telse\n> -\t\t\tcb.unreachable_expire_kind = UE_NORMAL;\n> -\t}\n> -\n> -\tif (cmd->expire_unreachable <= cmd->expire_total)\n> -\t\tcb.unreachable_expire_kind = UE_ALWAYS;\n> -\n> -\tcb.mark_list = NULL;\n> -\ttips = NULL;\n> -\tif (cb.unreachable_expire_kind != UE_ALWAYS) {\n> -\t\tif (cb.unreachable_expire_kind == UE_HEAD) {\n> -\t\t\tstruct commit_list *elem;\n> -\t\t\tfor_each_ref(push_tip_to_list, &tips);\n> -\t\t\tfor (elem = tips; elem; elem = elem->next)\n> -\t\t\t\tcommit_list_insert(elem->item, &cb.mark_list);\n> -\t\t} else {\n> -\t\t\tcommit_list_insert(tip_commit, &cb.mark_list);\n> -\t\t}\n> -\t\tcb.mark_limit = cmd->expire_total;\n> -\t\tmark_reachable(&cb);\n> -\t}\n> -\n> +\treflog_expiry_prepare(refname, sha1, &cb);\n> \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n> -\n> -\tif (cb.unreachable_expire_kind != UE_ALWAYS) {\n> -\t\tif (cb.unreachable_expire_kind == UE_HEAD) {\n> -\t\t\tstruct commit_list *elem;\n> -\t\t\tfor (elem = tips; elem; elem = elem->next)\n> -\t\t\t\tclear_commit_marks(elem->item, REACHABLE);\n> -\t\t\tfree_commit_list(tips);\n> -\t\t} else {\n> -\t\t\tclear_commit_marks(tip_commit, REACHABLE);\n> -\t\t}\n> -\t}\n> +\treflog_expiry_cleanup(&cb);\n> \n> \tif (cb.newlog) {\n> \t\tif (close_lock_file(&reflog_lock)) {\n> -- \n> 2.1.3\n> \n"},{"id":"253178","messageId":"20141205191829.GB16682@google.com","threadId":"38113","inReplyTo":"20141205002331.GJ16345@google.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-05T19:18:29Z","receivedAt":"2014-12-05T19:18:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n> > We don't actually need the locking functionality, because we already\n> > hold the lock on the reference itself, which is how the reflog file is\n> > locked. But the lock_file code still does some of the bookkeeping for\n> > us and is more careful than the old code here was.\n> \n> As you say, the ref lock takes care of mutual exclusion, so we do not\n> have to be too careful about compatibility with other tools that might\n> not know to lock the reflog. And this is not tying our hands for a\n> future when I might want to lock logs/refs/heads/topic/1 while\n> logs/refs/heads/topic still exists as part of the implementation of\n> \"git mv topic/1 topic\".\n> \n> Stefan and I had forgotten about that guarantee when looking at that\n> kind of operation --- thanks for the reminder.\n> \n> Should updates to the HEAD reflog acquire HEAD.lock? (They don't\n> currently.)\n> \n> [...]\n> > --- a/builtin/reflog.c\n> > +++ b/builtin/reflog.c\n> > @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n> > \treturn 0;\n> > }\n> > \n> > +static struct lock_file reflog_lock;\n> \n> If this lockfile is only used in that one function, it can be declared\n> inside the function.\n> \n> If it is meant to be used throughout the 'git reflog' command, then it\n> can go near the top of the file.\n> \n\nAfter the series completes, this lock is only used in reflog_expire.\nSo I'd rather move it inside the function? Then we could run the reflog_expire\nfunction in parallel for different locks in theory?\n\n> > +\n> > static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n> > {\n> > \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n> > \tstruct expire_reflog_cb cb;\n> > \tstruct ref_lock *lock;\n> > -\tchar *log_file, *newlog_path = NULL;\n> > +\tchar *log_file;\n> > \tstruct commit *tip_commit;\n> > \tstruct commit_list *tips;\n> > \tint status = 0;\n> > @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> > \t\tunlock_ref(lock);\n> > \t\treturn 0;\n> > \t}\n> > +\n> > \tlog_file = git_pathdup(\"logs/%s\", refname);\n> > \tif (!cmd->dry_run) {\n> > -\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", refname);\n> > -\t\tcb.newlog = fopen(newlog_path, \"w\");\n> > +\t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n> > +\t\t\tgoto failure;\n> \n> hold_lock_file_for_update doesn't print a message. Code to print one\n> looks like\n> \n> \tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {\n> \t\tunable_to_lock_message(log_file, errno, &err);\n> \t\terror(\"%s\", err.buf);\n> \t\tgoto failure;\n> \t}\n> \n> (A patch in flight changes that to\n> \n> \tif (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {\n> \t\terror(\"%s\", err.buf);\n> \t\tgoto failure;\n> \t}\n> \n> )\n> \n> > +\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> > +\t\tif (!cb.newlog)\n> > +\t\t\tgoto failure;\n> \n> Hm. lockfile.c::fdopen_lock_file ought to use xfdopen to make this\n> case impossible. And xfdopen should use try_to_free_routine() and\n> try again on failure.\n> \n> [...]\n> > @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> > \t}\n> > \n> > \tif (cb.newlog) {\n> > -\t\tif (fclose(cb.newlog)) {\n> > -\t\t\tstatus |= error(\"%s: %s\", strerror(errno),\n> > -\t\t\t\t\tnewlog_path);\n> > -\t\t\tunlink(newlog_path);\n> > +\t\tif (close_lock_file(&reflog_lock)) {\n> > +\t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n> > +\t\t\t\t\tstrerror(errno));\n> \n> Style nit: error messages usually start with a lowercase letter\n> (though I realize nearby examples are already inconsistent).\n> \n> commit_lock_file() can take care of the close_lock_file automatically.\n> \n> [...]\n> > @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n> > \t\t\t close_ref(lock) < 0)) {\n> > \t\t\tstatus |= error(\"Couldn't write %s\",\n> > \t\t\t\t\tlock->lk->filename.buf);\n> > -\t\t\tunlink(newlog_path);\n> > -\t\t} else if (rename(newlog_path, log_file)) {\n> > -\t\t\tstatus |= error(\"cannot rename %s to %s\",\n> > -\t\t\t\t\tnewlog_path, log_file);\n> > -\t\t\tunlink(newlog_path);\n> > +\t\t\trollback_lock_file(&reflog_lock);\n> > +\t\t} else if (commit_lock_file(&reflog_lock)) {\n> > +\t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n> > +\t\t\t\t\tlog_file, log_file);\n> \n> Most callers say \"unable to commit reflog '%s'\", log_file to hedge their\n> bets in case the close failed (which may be what you were avoiding\n> above.\n> \n> errno is meaningful when commit_lock_file fails, making a more\n> detailed diagnosis from strerror(errno) possible.\n> \n> Thanks,\n> Jonathan\n"},{"id":"253179","messageId":"xmqqoarhq4dc.fsf@gitster.dls.corp.google.com","threadId":"38113","inReplyTo":"20141205191829.GB16682@google.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-05T19:32:47Z","receivedAt":"2014-12-05T19:32:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> > +static struct lock_file reflog_lock;\n>> \n>> If this lockfile is only used in that one function, it can be declared\n>> inside the function.\n>> \n>> If it is meant to be used throughout the 'git reflog' command, then it\n>> can go near the top of the file.\n>> \n>\n> After the series completes, this lock is only used in reflog_expire.\n> So I'd rather move it inside the function? Then we could run the reflog_expire\n> function in parallel for different locks in theory?\n\nI am not sure about the \"parallel\" part, but I would imagine that it\nis an essential prerequisite to move this outside the \"client\" code\nif we want to later replace the backing storage of refs and reflogs\noutside the filesystem, so from that point of view, I think the\nsuggestion makes sense.\n\nThanks.\n"},{"id":"253180","messageId":"CAGZ79kbf_Ze6JR33Y6-N3JZTUQqTyqc-feQZ-__h7JkeUTzUvg@mail.gmail.com","threadId":"38113","inReplyTo":"xmqqoarhq4dc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-05T19:41:05Z","receivedAt":"2014-12-05T19:41:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 5, 2014 at 11:32 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> > +static struct lock_file reflog_lock;\n>>>\n>>> If this lockfile is only used in that one function, it can be declared\n>>> inside the function.\n>>>\n>>> If it is meant to be used throughout the 'git reflog' command, then it\n>>> can go near the top of the file.\n>>>\n>>\n>> After the series completes, this lock is only used in reflog_expire.\n>> So I'd rather move it inside the function? Then we could run the reflog_expire\n>> function in parallel for different locks in theory?\n>\n> I am not sure about the \"parallel\" part, but I would imagine that it\n> is an essential prerequisite to move this outside the \"client\" code\n> if we want to later replace the backing storage of refs and reflogs\n> outside the filesystem, so from that point of view, I think the\n> suggestion makes sense.\n>\n> Thanks.\n>\n\nSorry for the confusion. With parallel I mean, that in theory we could have\nmultiple threads running reflog expire at the same time in the same program.\nHaving the \"static struct lock_file reflog_lock;\" around, we can only\nprocess one\nreflog at a time, as that is holding the lock_file struct.\n\nI am not saying we want to go multi-threading any time soon if at all.\nJust that it would\nbe easier to do, if not having the lock file as a file-global variable\ninstead of a\nvariable inside a function.\n\nThanks,\nStefan\n"},{"id":"253185","messageId":"xmqqk325q0is.fsf@gitster.dls.corp.google.com","threadId":"38113","inReplyTo":"CAGZ79kbf_Ze6JR33Y6-N3JZTUQqTyqc-feQZ-__h7JkeUTzUvg@mail.gmail.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-05T20:55:55Z","receivedAt":"2014-12-05T20:55:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>>> After the series completes, this lock is only used in reflog_expire.\n>>> So I'd rather move it inside the function? Then we could run the reflog_expire\n>>> function in parallel for different locks in theory?\n>>\n>> I am not sure about the \"parallel\" part, but I would imagine that it\n>> is an essential prerequisite to move this outside the \"client\" code\n>> if we want to later replace the backing storage of refs and reflogs\n>> outside the filesystem, so from that point of view, I think the\n>> suggestion makes sense.\n>>\n>> Thanks.\n>>\n>\n> Sorry for the confusion. With parallel I mean,...\n\nThere is no confusion. I understand exactly what you meant.\n\nWhat I said was not sure was if \"parallel\" is a practical enough\npossiblity to include into the set of value propositions the\nsuggested change to move the lock out of the \"client\" may give us.\n\nIn other words, \"With this change, doing a parallel will become a\nlot easier\"---\"Really? It probably is not one of the harder part of\nthe problem if you really want to go parallel\" was the discourse I\nhad in my mind.\n\n;-)\n"},{"id":"253296","messageId":"54857871.5090805@alum.mit.edu","threadId":"38113","inReplyTo":"20141205021931.GA29570@google.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-08T10:07:45Z","receivedAt":"2014-12-08T10:07:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/05/2014 03:19 AM, Stefan Beller wrote:\n> On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:\n>> Michael Haggerty wrote:\n>>\n>>> We don't actually need the locking functionality, because we already\n>>> hold the lock on the reference itself, which is how the reflog file is\n>>> locked. But the lock_file code still does some of the bookkeeping for\n>>> us and is more careful than the old code here was.\n>>\n>> As you say, the ref lock takes care of mutual exclusion, so we do not\n>> have to be too careful about compatibility with other tools that might\n>> not know to lock the reflog. And this is not tying our hands for a\n>> future when I might want to lock logs/refs/heads/topic/1 while\n>> logs/refs/heads/topic still exists as part of the implementation of\n>> \"git mv topic/1 topic\".\n>>\n>> Stefan and I had forgotten about that guarantee when looking at that\n>> kind of operation --- thanks for the reminder.\n> \n> I did not forget about it, I did not know about that in the first hand.\n> We don't seem to have documentation on it?\n> \n> So sorry for heading in a direction, which would have been avoidable.\n\nThis isn't documented very well. I thought I saw a comment somewhere in\nthe code that stated it explicitly, but I can't find it now. In any\ncase, my understanding of the locking protocol for reflogs is:\n\n The reflog for \"$refname\", which is stored at\n \"$GIT_DIR/logs/$refname\", is locked by holding\n \"$GIT_DIR/refs/$refname.lock\", *even if the corresponding\n reference is packed*.\n\nThis implies that readers, who don't pay attention to locks, have to be\nprepared for the possibility that the reflog is in the middle of an\nupdate and that the last line is incomplete. This is handled by\nshow_one_reflog_ent(), which discards incomplete lines.\n\nThis protocol avoids the need to rewrite the reflog from scratch for\neach reference update.\n\nGiven how poorly-documented this point is, I wonder whether other\nimplementations of Git (e.g., libgit2, JGit, Dulwich, ...) got it right.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253298","messageId":"54858038.9090004@alum.mit.edu","threadId":"38113","inReplyTo":"CAN05THTH2XDaUmchehF0gY0-GSgD_O9rJH7F5Gc3dHLCPz3GsA@mail.gmail.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-08T10:40:56Z","receivedAt":"2014-12-08T10:40:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/05/2014 03:59 AM, ronnie sahlberg wrote:\n> On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty wrote:\n>> We don't actually need the locking functionality, because we already\n>> hold the lock on the reference itself,\n> \n> No. You do need the lock.\n> The ref is locked only during transaction_commit()\n> \n> If you don't want to lock the reflog file and instead rely on the lock\n> on the ref itself you will need to\n> rework your patches so that the lock on the ref is taken already\n> during, for example, transaction_update_ref() instead.\n> \n> But without doing those changes and moving the ref locking from\n> _commit() to _update_ref() you will risk reflog corruption/surprises\n> if two operations collide and both rewrite the reflog without any lock held.\n\nRonnie, I don't understand your comments.\n\nIt is a statement of fact (to the best of my knowledge) that reflogs are\nsupposed to be modified only under a lock on the corresponding\nreference, namely \"$GIT_DIR/refs/$refname.lock\". We do not require\nreflog writers to hold \"$GIT_DIR/logs/$refname.lock\".\n\nIn this function, \"$GIT_DIR/logs/$refname.lock\" happens to be the name\nof the temporary file being used to stage the new contents of the\nreflog. But that is more or less a coincidence; we could call the\ntemporary file whatever we want because it has no locking implications.\nHowever, what we want to do with the file in this code path (write a new\nversion then rename the new version on top of the old version, deleting\nthe temporary file if the program is interrupted) is the same as what we\ndo with lockfiles, so we use the lockfile code because it is convenient.\n\nThis patch series has nothing to do with ref_transaction_commit() or any\nof the transaction machinery. It has to do with expire_reflog(), which\nis invoked outside of any transaction.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253335","messageId":"5485B039.1070200@alum.mit.edu","threadId":"38113","inReplyTo":"20141205002331.GJ16345@google.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-08T14:05:45Z","receivedAt":"2014-12-08T14:05:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/05/2014 01:23 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> We don't actually need the locking functionality, because we already\n>> hold the lock on the reference itself, which is how the reflog file is\n>> locked. But the lock_file code still does some of the bookkeeping for\n>> us and is more careful than the old code here was.\n> \n> As you say, the ref lock takes care of mutual exclusion, so we do not\n> have to be too careful about compatibility with other tools that might\n> not know to lock the reflog. And this is not tying our hands for a\n> future when I might want to lock logs/refs/heads/topic/1 while\n> logs/refs/heads/topic still exists as part of the implementation of\n> \"git mv topic/1 topic\".\n> \n> Stefan and I had forgotten about that guarantee when looking at that\n> kind of operation --- thanks for the reminder.\n\nThis reminder is important (and forgettable) enough that I will add a\ncomment within the function explaining it.\n\n> Should updates to the HEAD reflog acquire HEAD.lock? (They don't\n> currently.)\n\nYes, they should; good catch. I assume that you are referring to the\ncode at the bottom of write_ref_sha1()? Or did you find a problem in\nthis patch series?\n\nIf the former, then I propose that we address this bug in a separate\npatch series.\n\n> [...]\n>> --- a/builtin/reflog.c\n>> +++ b/builtin/reflog.c\n>> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n>> \treturn 0;\n>> }\n>> \n>> +static struct lock_file reflog_lock;\n> \n> If this lockfile is only used in that one function, it can be declared\n> inside the function.\n> \n> If it is meant to be used throughout the 'git reflog' command, then it\n> can go near the top of the file.\n\nFor now it is only used within this function, so I will move it into the\nfunction as you suggest. (As you know, it does need to remain static,\nbecause of the way the lock_file module takes over ownership of these\nobjects.)\n\n>> +\n>> static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n>> {\n>> \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n>> \tstruct expire_reflog_cb cb;\n>> \tstruct ref_lock *lock;\n>> -\tchar *log_file, *newlog_path = NULL;\n>> +\tchar *log_file;\n>> \tstruct commit *tip_commit;\n>> \tstruct commit_list *tips;\n>> \tint status = 0;\n>> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n>> \t\tunlock_ref(lock);\n>> \t\treturn 0;\n>> \t}\n>> +\n>> \tlog_file = git_pathdup(\"logs/%s\", refname);\n>> \tif (!cmd->dry_run) {\n>> -\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", refname);\n>> -\t\tcb.newlog = fopen(newlog_path, \"w\");\n>> +\t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n>> +\t\t\tgoto failure;\n> \n> hold_lock_file_for_update doesn't print a message. Code to print one\n> looks like\n> \n> \tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {\n> \t\tunable_to_lock_message(log_file, errno, &err);\n> \t\terror(\"%s\", err.buf);\n> \t\tgoto failure;\n> \t}\n\nThanks; will add.\n\n> (A patch in flight changes that to\n> \n> \tif (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {\n> \t\terror(\"%s\", err.buf);\n> \t\tgoto failure;\n> \t}\n> \n> )\n\nThanks for the heads-up. The compiler will complain when the branches\nare merged, and hopefully the fix will be obvious.\n\n>> +\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n>> +\t\tif (!cb.newlog)\n>> +\t\t\tgoto failure;\n> \n> Hm. lockfile.c::fdopen_lock_file ought to use xfdopen to make this\n> case impossible. And xfdopen should use try_to_free_routine() and\n> try again on failure.\n\nThat sounds reasonable, but it is not manifestly obvious given that at\nleast one caller of fdopen_lock_file() (in fast-import.c) tries to\nrecover if fdopen_lock_file() fails. Let's address this in a separate\npatch series if that is OK with you. For now I will add explicit\nerror-reporting code here before \"goto failure\".\n\n> [...]\n>> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n>> \t}\n>> \n>> \tif (cb.newlog) {\n>> -\t\tif (fclose(cb.newlog)) {\n>> -\t\t\tstatus |= error(\"%s: %s\", strerror(errno),\n>> -\t\t\t\t\tnewlog_path);\n>> -\t\t\tunlink(newlog_path);\n>> +\t\tif (close_lock_file(&reflog_lock)) {\n>> +\t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n>> +\t\t\t\t\tstrerror(errno));\n> \n> Style nit: error messages usually start with a lowercase letter\n> (though I realize nearby examples are already inconsistent).\n\nThanks; will fix.\n\n> commit_lock_file() can take care of the close_lock_file automatically.\n\nThe existing code is a tiny bit safer: first make sure both files can be\nwritten, *then* rename each of them into place. If either write fails,\nthen both files will get rolled back. But if we switch to using\ncommit_lock_file(), then a failure when writing the reference would\nleave the reflog updated but the reference rolled back.\n\n> [...]\n>> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c\n>> \t\t\t close_ref(lock) < 0)) {\n>> \t\t\tstatus |= error(\"Couldn't write %s\",\n>> \t\t\t\t\tlock->lk->filename.buf);\n>> -\t\t\tunlink(newlog_path);\n>> -\t\t} else if (rename(newlog_path, log_file)) {\n>> -\t\t\tstatus |= error(\"cannot rename %s to %s\",\n>> -\t\t\t\t\tnewlog_path, log_file);\n>> -\t\t\tunlink(newlog_path);\n>> +\t\t\trollback_lock_file(&reflog_lock);\n>> +\t\t} else if (commit_lock_file(&reflog_lock)) {\n>> +\t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n>> +\t\t\t\t\tlog_file, log_file);\n> \n> Most callers say \"unable to commit reflog '%s'\", log_file to hedge their\n> bets in case the close failed (which may be what you were avoiding\n> above.\n> \n> errno is meaningful when commit_lock_file fails, making a more\n> detailed diagnosis from strerror(errno) possible.\n\nI will improve the error message.\n\nThanks for your detailed review!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253377","messageId":"20141208223330.GA25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-9-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 08/23] Extract function should_expire_reflog_ent()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:33:30Z","receivedAt":"2014-12-08T22:33:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:20AM +0100, Michael Haggerty wrote:\n> Extracted from expire_reflog_ent() a function that is solely\n> responsible for deciding whether a reflog entry should be expired. By\n> separating this \"business logic\" from the mechanics of actually\n> expiring entries, we are working towards the goal of encapsulating\n> reflog expiry within the refs API, with policy decided by a callback\n> function passed to it by its caller.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n\nThe comments below are just thoughts, which don't need to be\nincluded into this commit.\n\n\n> +\tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n> +\t\t\t\t message, cb_data)) {\n> +\t\tif (!cb->newlog)\n> +\t\t\tprintf(\"would prune %s\", message);\n> +\t\telse if (cb->cmd->verbose)\n> +\t\t\tprintf(\"prune %s\", message);\n\nWhile this commit is just shoveling code around, we don't want to introduce\nchanges here. So a question for a possible later follow up:\n\"git reflog\" is listed as an ancillary manipulator, which still is porcelain.\nSo we maybe want to translate \"[would] prune\"?\n\n\n> +\t\t\tchar sign = (tz < 0) ? '-' : '+';\n> +\t\t\tint zone = (tz < 0) ? (-tz) : tz;\n> +\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n> +\t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n> +\t\t\t\temail, timestamp, sign, zone,\n> +\t\t\t\tmessage);\n\nThis is fine for just moving code around and reviewing.\nI send a patch on top of this one to remove the manual calculation of the \nsign and zone and let the fprintf function figure it out.\n"},{"id":"253378","messageId":"20141208223511.GB25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 10/23] expire_reflog(): add a \"flags\" argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:35:11Z","receivedAt":"2014-12-08T22:35:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:22AM +0100, Michael Haggerty wrote:\n> We want to separate the options relevant to the expiry machinery from\n> the options affecting the expiration policy. So add a \"flags\" argument\n> to expire_reflog() to hold the former.\n> \n> The argument doesn't yet do anything.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: \n\n> ---\n> builtin/reflog.c | 11 +++++++----\n> 1 file changed, 7 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index ebfa635..319f0d2 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -415,7 +415,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n> \n> static struct lock_file reflog_lock;\n> \n> -static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)\n> +static int expire_reflog(const char *refname, const unsigned char *sha1,\n> +\t\t\t unsigned int flags, void *cb_data)\n> {\n> \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n> \tstruct expire_reflog_cb cb;\n> @@ -627,6 +628,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \tunsigned long now = time(NULL);\n> \tint i, status, do_all;\n> \tint explicit_expiry = 0;\n> +\tunsigned int flags = 0;\n> \n> \tdefault_reflog_expire_unreachable = now - 30 * 24 * 3600;\n> \tdefault_reflog_expire = now - 90 * 24 * 3600;\n> @@ -696,7 +698,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\tfor (i = 0; i < collected.nr; i++) {\n> \t\t\tstruct collected_reflog *e = collected.e[i];\n> \t\t\tset_reflog_expiry_param(&cb, explicit_expiry, e->reflog);\n> -\t\t\tstatus |= expire_reflog(e->reflog, e->sha1, &cb);\n> +\t\t\tstatus |= expire_reflog(e->reflog, e->sha1, flags, &cb);\n> \t\t\tfree(e);\n> \t\t}\n> \t\tfree(collected.e);\n> @@ -710,7 +712,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\t\tcontinue;\n> \t\t}\n> \t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n> -\t\tstatus |= expire_reflog(ref, sha1, &cb);\n> +\t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n> \t}\n> \treturn status;\n> }\n> @@ -729,6 +731,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct cmd_reflog_expire_cb cb;\n> \tint i, status = 0;\n> +\tunsigned int flags = 0;\n> \n> \tmemset(&cb, 0, sizeof(cb));\n> \n> @@ -781,7 +784,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> \t\t\tcb.expire_total = 0;\n> \t\t}\n> \n> -\t\tstatus |= expire_reflog(ref, sha1, &cb);\n> +\t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n> \t\tfree(ref);\n> \t}\n> \treturn status;\n> -- \n> 2.1.3\n> \n"},{"id":"253379","messageId":"20141208223845.GC25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-12-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 11/23] expire_reflog(): move dry_run to flags argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:38:45Z","receivedAt":"2014-12-08T22:38:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:23AM +0100, Michael Haggerty wrote:\n> The policy objects don't care about \"--dry-run\". So move it to\n> expire_reflog()'s flags parameter.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: \n\n> ---\n> builtin/reflog.c | 13 ++++++++-----\n> 1 file changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 319f0d2..a490193 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -22,7 +22,6 @@ static unsigned long default_reflog_expire_unreachable;\n> \n> struct cmd_reflog_expire_cb {\n> \tstruct rev_info revs;\n> -\tint dry_run;\n> \tint stalefix;\n> \tint rewrite;\n> \tint updateref;\n> @@ -415,6 +414,10 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n> \n> static struct lock_file reflog_lock;\n> \n> +enum expire_reflog_flags {\n> +\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0\n> +};\n> +\n> static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t\t unsigned int flags, void *cb_data)\n> {\n> @@ -439,7 +442,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t}\n> \n> \tlog_file = git_pathdup(\"logs/%s\", refname);\n> -\tif (!cmd->dry_run) {\n> +\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n> \t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n> \t\t\tgoto failure;\n> \t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> @@ -453,7 +456,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n> \treflog_expiry_cleanup(&cb);\n> \n> -\tif (cb.newlog) {\n> +\tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n> \t\tif (close_lock_file(&reflog_lock)) {\n> \t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n> \t\t\t\t\tstrerror(errno));\n> @@ -644,7 +647,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \tfor (i = 1; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n> -\t\t\tcb.dry_run = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n> \t\telse if (starts_with(arg, \"--expire=\")) {\n> \t\t\tif (parse_expiry_date(arg + 9, &cb.expire_total))\n> \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n> @@ -738,7 +741,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> \tfor (i = 1; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n> -\t\t\tcb.dry_run = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n> \t\telse if (!strcmp(arg, \"--rewrite\"))\n> \t\t\tcb.rewrite = 1;\n> \t\telse if (!strcmp(arg, \"--updateref\"))\n> -- \n> 2.1.3\n> \n"},{"id":"253380","messageId":"20141208224206.GD25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-13-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 12/23] expire_reflog(): move updateref to flags argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:42:06Z","receivedAt":"2014-12-08T22:42:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:24AM +0100, Michael Haggerty wrote:\n> The policy objects don't care about \"--updateref\". So move it to\n> expire_reflog()'s flags parameter.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: \n\n> ---\n> builtin/reflog.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index a490193..597c547 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -24,7 +24,6 @@ struct cmd_reflog_expire_cb {\n> \tstruct rev_info revs;\n> \tint stalefix;\n> \tint rewrite;\n> -\tint updateref;\n> \tint verbose;\n> \tunsigned long expire_total;\n> \tunsigned long expire_unreachable;\n> @@ -415,7 +414,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n> static struct lock_file reflog_lock;\n> \n> enum expire_reflog_flags {\n> -\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0\n> +\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> +\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1\n> };\n> \n> static int expire_reflog(const char *refname, const unsigned char *sha1,\n> @@ -460,7 +460,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\tif (close_lock_file(&reflog_lock)) {\n> \t\t\tstatus |= error(\"Couldn't write %s: %s\", log_file,\n> \t\t\t\t\tstrerror(errno));\n> -\t\t} else if (cmd->updateref &&\n> +\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n> \t\t\t(write_in_full(lock->lock_fd,\n> \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n> \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n> @@ -471,7 +471,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t} else if (commit_lock_file(&reflog_lock)) {\n> \t\t\tstatus |= error(\"cannot rename %s.lock to %s\",\n> \t\t\t\t\tlog_file, log_file);\n> -\t\t} else if (cmd->updateref && commit_ref(lock)) {\n> +\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n> \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n> \t\t}\n> \t}\n> @@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\telse if (!strcmp(arg, \"--rewrite\"))\n> \t\t\tcb.rewrite = 1;\n> \t\telse if (!strcmp(arg, \"--updateref\"))\n> -\t\t\tcb.updateref = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n> \t\telse if (!strcmp(arg, \"--all\"))\n> \t\t\tdo_all = 1;\n> \t\telse if (!strcmp(arg, \"--verbose\"))\n> @@ -745,7 +745,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> \t\telse if (!strcmp(arg, \"--rewrite\"))\n> \t\t\tcb.rewrite = 1;\n> \t\telse if (!strcmp(arg, \"--updateref\"))\n> -\t\t\tcb.updateref = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n> \t\telse if (!strcmp(arg, \"--verbose\"))\n> \t\t\tcb.verbose = 1;\n> \t\telse if (!strcmp(arg, \"--\")) {\n> -- \n> 2.1.3\n> \n"},{"id":"253381","messageId":"20141208224610.GE25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 13/23] Rename expire_reflog_cb to expire_reflog_policy_cb","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:46:10Z","receivedAt":"2014-12-08T22:46:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:25AM +0100, Michael Haggerty wrote:\n> This is the first step towards separating the data needed by the\n> policy code from the data needed by the reflog expiration machinery.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: \n> ---\n> builtin/reflog.c | 19 ++++++++++---------\n> 1 file changed, 10 insertions(+), 9 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 597c547..3538e4b 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -30,7 +30,7 @@ struct cmd_reflog_expire_cb {\n> \tint recno;\n> };\n> \n> -struct expire_reflog_cb {\n> +struct expire_reflog_policy_cb {\n> \tFILE *newlog;\n> \tenum {\n> \t\tUE_NORMAL,\n> @@ -220,7 +220,7 @@ static int keep_entry(struct commit **it, unsigned char *sha1)\n> * the expire_limit and queue them back, so that the caller can call\n> * us again to restart the traversal with longer expire_limit.\n> */\n> -static void mark_reachable(struct expire_reflog_cb *cb)\n> +static void mark_reachable(struct expire_reflog_policy_cb *cb)\n> {\n> \tstruct commit *commit;\n> \tstruct commit_list *pending;\n> @@ -259,7 +259,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)\n> \tcb->mark_list = leftover;\n> }\n> \n> -static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsigned char *sha1)\n> +static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, unsigned char *sha1)\n> {\n> \t/*\n> \t * We may or may not have the commit yet - if not, look it\n> @@ -295,7 +295,7 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \t\t\t\t const char *email, unsigned long timestamp, int tz,\n> \t\t\t\t const char *message, void *cb_data)\n> {\n> -\tstruct expire_reflog_cb *cb = cb_data;\n> +\tstruct expire_reflog_policy_cb *cb = cb_data;\n> \tstruct commit *old, *new;\n> \n> \tif (timestamp < cb->cmd->expire_total)\n> @@ -323,7 +323,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \t\tconst char *email, unsigned long timestamp, int tz,\n> \t\tconst char *message, void *cb_data)\n> {\n> -\tstruct expire_reflog_cb *cb = cb_data;\n> +\tstruct expire_reflog_policy_cb *cb = cb_data;\n> \n> \tif (cb->cmd->rewrite)\n> \t\tosha1 = cb->last_kept_sha1;\n> @@ -350,7 +350,8 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \treturn 0;\n> }\n> \n> -static int push_tip_to_list(const char *refname, const unsigned char *sha1, int flags, void *cb_data)\n> +static int push_tip_to_list(const char *refname, const unsigned char *sha1,\n> +\t\t\t int flags, void *cb_data)\n> {\n> \tstruct commit_list **list = cb_data;\n> \tstruct commit *tip_commit;\n> @@ -365,7 +366,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int\n> \n> static void reflog_expiry_prepare(const char *refname,\n> \t\t\t\t const unsigned char *sha1,\n> -\t\t\t\t struct expire_reflog_cb *cb)\n> +\t\t\t\t struct expire_reflog_policy_cb *cb)\n> {\n> \tif (!cb->cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n> \t\tcb->tip_commit = NULL;\n> @@ -397,7 +398,7 @@ static void reflog_expiry_prepare(const char *refname,\n> \t}\n> }\n> \n> -static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)\n> +static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n> {\n> \tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n> \t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n> @@ -422,7 +423,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t\t unsigned int flags, void *cb_data)\n> {\n> \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n> -\tstruct expire_reflog_cb cb;\n> +\tstruct expire_reflog_policy_cb cb;\n> \tstruct ref_lock *lock;\n> \tchar *log_file;\n> \tint status = 0;\n> -- \n> 2.1.3\n> \n"},{"id":"253382","messageId":"20141208224941.GF25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 14/23] struct expire_reflog_cb: a new callback data type","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:49:41Z","receivedAt":"2014-12-08T22:49:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:26AM +0100, Michael Haggerty wrote:\n> Add a new data type, \"struct expire_reflog_cb\", for holding the data\n> that expire_reflog() passes to expire_reflog_ent() via\n> for_each_reflog_ent(). For now it only holds a pointer to \"struct\n> expire_reflog_policy_cb\". In future commits we will move some data\n> from the latter to the former.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: \n> ---\n> builtin/reflog.c | 43 ++++++++++++++++++++++++++-----------------\n> 1 file changed, 26 insertions(+), 17 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 3538e4b..5dfa53a 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -45,10 +45,15 @@ struct expire_reflog_policy_cb {\n> \tstruct commit_list *tips;\n> };\n> \n> +struct expire_reflog_cb {\n> +\tvoid *policy_cb;\n> +};\n> +\n> struct collected_reflog {\n> \tunsigned char sha1[20];\n> \tchar reflog[FLEX_ARRAY];\n> };\n> +\n> struct collect_reflog_cb {\n> \tstruct collected_reflog **e;\n> \tint alloc;\n> @@ -323,28 +328,29 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \t\tconst char *email, unsigned long timestamp, int tz,\n> \t\tconst char *message, void *cb_data)\n> {\n> -\tstruct expire_reflog_policy_cb *cb = cb_data;\n> +\tstruct expire_reflog_cb *cb = cb_data;\n> +\tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n> \n> -\tif (cb->cmd->rewrite)\n> -\t\tosha1 = cb->last_kept_sha1;\n> +\tif (policy_cb->cmd->rewrite)\n> +\t\tosha1 = policy_cb->last_kept_sha1;\n> \n> \tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n> -\t\t\t\t message, cb_data)) {\n> -\t\tif (!cb->newlog)\n> +\t\t\t\t message, policy_cb)) {\n> +\t\tif (!policy_cb->newlog)\n> \t\t\tprintf(\"would prune %s\", message);\n> -\t\telse if (cb->cmd->verbose)\n> +\t\telse if (policy_cb->cmd->verbose)\n> \t\t\tprintf(\"prune %s\", message);\n> \t} else {\n> -\t\tif (cb->newlog) {\n> +\t\tif (policy_cb->newlog) {\n> \t\t\tchar sign = (tz < 0) ? '-' : '+';\n> \t\t\tint zone = (tz < 0) ? (-tz) : tz;\n> -\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n> +\t\t\tfprintf(policy_cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n> \t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n> \t\t\t\temail, timestamp, sign, zone,\n> \t\t\t\tmessage);\n> -\t\t\thashcpy(cb->last_kept_sha1, nsha1);\n> +\t\t\thashcpy(policy_cb->last_kept_sha1, nsha1);\n> \t\t}\n> -\t\tif (cb->cmd->verbose)\n> +\t\tif (policy_cb->cmd->verbose)\n> \t\t\tprintf(\"keep %s\", message);\n> \t}\n> \treturn 0;\n> @@ -423,12 +429,15 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t\t unsigned int flags, void *cb_data)\n> {\n> \tstruct cmd_reflog_expire_cb *cmd = cb_data;\n> -\tstruct expire_reflog_policy_cb cb;\n> +\tstruct expire_reflog_cb cb;\n> +\tstruct expire_reflog_policy_cb policy_cb;\n> \tstruct ref_lock *lock;\n> \tchar *log_file;\n> \tint status = 0;\n> \n> \tmemset(&cb, 0, sizeof(cb));\n> +\tmemset(&policy_cb, 0, sizeof(policy_cb));\n> +\tcb.policy_cb = &policy_cb;\n> \n> \t/*\n> \t * we take the lock for the ref itself to prevent it from\n> @@ -446,16 +455,16 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n> \t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n> \t\t\tgoto failure;\n> -\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> -\t\tif (!cb.newlog)\n> +\t\tpolicy_cb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> +\t\tif (!policy_cb.newlog)\n> \t\t\tgoto failure;\n> \t}\n> \n> -\tcb.cmd = cmd;\n> +\tpolicy_cb.cmd = cmd;\n> \n> -\treflog_expiry_prepare(refname, sha1, &cb);\n> +\treflog_expiry_prepare(refname, sha1, &policy_cb);\n> \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n> -\treflog_expiry_cleanup(&cb);\n> +\treflog_expiry_cleanup(&policy_cb);\n> \n> \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n> \t\tif (close_lock_file(&reflog_lock)) {\n> @@ -463,7 +472,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t\t\t\tstrerror(errno));\n> \t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n> \t\t\t(write_in_full(lock->lock_fd,\n> -\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n> +\t\t\t\tsha1_to_hex(policy_cb.last_kept_sha1), 40) != 40 ||\n> \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n> \t\t\t close_ref(lock) < 0)) {\n> \t\t\tstatus |= error(\"Couldn't write %s\",\n> -- \n> 2.1.3\n> \n"},{"id":"253383","messageId":"20141208225517.GG25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-16-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 15/23] expire_reflog(): pass flags through to expire_reflog_ent()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:55:17Z","receivedAt":"2014-12-08T22:55:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:27AM +0100, Michael Haggerty wrote:\n> Add a flags field to \"struct expire_reflog_cb\", and pass the flags\n> argument through to expire_reflog_ent(). In a moment we will start\n> using it to pass through flags that expire_reflog_ent() needs.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: \n\n> ---\n> builtin/reflog.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 5dfa53a..1512b67 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -46,6 +46,7 @@ struct expire_reflog_policy_cb {\n> };\n> \n> struct expire_reflog_cb {\n> +\tunsigned int flags;\n> \tvoid *policy_cb;\n> };\n> \n> @@ -437,6 +438,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \n> \tmemset(&cb, 0, sizeof(cb));\n> \tmemset(&policy_cb, 0, sizeof(policy_cb));\n> +\tcb.flags = flags;\n> \tcb.policy_cb = &policy_cb;\n> \n> \t/*\n> -- \n> 2.1.3\n> \n"},{"id":"253384","messageId":"20141208225630.GH25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-17-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 16/23] expire_reflog(): move verbose to flags argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:56:30Z","receivedAt":"2014-12-08T22:56:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:28AM +0100, Michael Haggerty wrote:\n> The policy objects don't care about \"--verbose\". So move it to\n> expire_reflog()'s flags parameter.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n\n> ---\n> builtin/reflog.c | 24 ++++++++++++------------\n> 1 file changed, 12 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 1512b67..cc7a220 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -20,11 +20,16 @@ static const char reflog_delete_usage[] =\n> static unsigned long default_reflog_expire;\n> static unsigned long default_reflog_expire_unreachable;\n> \n> +enum expire_reflog_flags {\n> +\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> +\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n> +\tEXPIRE_REFLOGS_VERBOSE = 1 << 2\n> +};\n> +\n> struct cmd_reflog_expire_cb {\n> \tstruct rev_info revs;\n> \tint stalefix;\n> \tint rewrite;\n> -\tint verbose;\n> \tunsigned long expire_total;\n> \tunsigned long expire_unreachable;\n> \tint recno;\n> @@ -339,7 +344,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \t\t\t\t message, policy_cb)) {\n> \t\tif (!policy_cb->newlog)\n> \t\t\tprintf(\"would prune %s\", message);\n> -\t\telse if (policy_cb->cmd->verbose)\n> +\t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tprintf(\"prune %s\", message);\n> \t} else {\n> \t\tif (policy_cb->newlog) {\n> @@ -351,7 +356,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \t\t\t\tmessage);\n> \t\t\thashcpy(policy_cb->last_kept_sha1, nsha1);\n> \t\t}\n> -\t\tif (policy_cb->cmd->verbose)\n> +\t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tprintf(\"keep %s\", message);\n> \t}\n> \treturn 0;\n> @@ -421,11 +426,6 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n> \n> static struct lock_file reflog_lock;\n> \n> -enum expire_reflog_flags {\n> -\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> -\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1\n> -};\n> -\n> static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t\t unsigned int flags, void *cb_data)\n> {\n> @@ -679,7 +679,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\telse if (!strcmp(arg, \"--all\"))\n> \t\t\tdo_all = 1;\n> \t\telse if (!strcmp(arg, \"--verbose\"))\n> -\t\t\tcb.verbose = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_VERBOSE;\n> \t\telse if (!strcmp(arg, \"--\")) {\n> \t\t\ti++;\n> \t\t\tbreak;\n> @@ -697,10 +697,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t */\n> \tif (cb.stalefix) {\n> \t\tinit_revisions(&cb.revs, prefix);\n> -\t\tif (cb.verbose)\n> +\t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tprintf(\"Marking reachable objects...\");\n> \t\tmark_reachable_objects(&cb.revs, 0, 0, NULL);\n> -\t\tif (cb.verbose)\n> +\t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tputchar('\\n');\n> \t}\n> \n> @@ -759,7 +759,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> \t\telse if (!strcmp(arg, \"--updateref\"))\n> \t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n> \t\telse if (!strcmp(arg, \"--verbose\"))\n> -\t\t\tcb.verbose = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_VERBOSE;\n> \t\telse if (!strcmp(arg, \"--\")) {\n> \t\t\ti++;\n> \t\t\tbreak;\n> -- \n> 2.1.3\n> \n"},{"id":"253385","messageId":"20141208225820.GI25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-18-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 17/23] expire_reflog(): move rewrite to flags argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:58:20Z","receivedAt":"2014-12-08T22:58:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:29AM +0100, Michael Haggerty wrote:\n> The policy objects don't care about \"--rewrite\". So move it to\n> expire_reflog()'s flags parameter.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n\n> ---\n> builtin/reflog.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index cc7a220..6294406 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -23,13 +23,13 @@ static unsigned long default_reflog_expire_unreachable;\n> enum expire_reflog_flags {\n> \tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> \tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n> -\tEXPIRE_REFLOGS_VERBOSE = 1 << 2\n> +\tEXPIRE_REFLOGS_VERBOSE = 1 << 2,\n> +\tEXPIRE_REFLOGS_REWRITE = 1 << 3\n> };\n> \n> struct cmd_reflog_expire_cb {\n> \tstruct rev_info revs;\n> \tint stalefix;\n> -\tint rewrite;\n> \tunsigned long expire_total;\n> \tunsigned long expire_unreachable;\n> \tint recno;\n> @@ -337,7 +337,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \tstruct expire_reflog_cb *cb = cb_data;\n> \tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n> \n> -\tif (policy_cb->cmd->rewrite)\n> +\tif (cb->flags & EXPIRE_REFLOGS_REWRITE)\n> \t\tosha1 = policy_cb->last_kept_sha1;\n> \n> \tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n> @@ -673,7 +673,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\telse if (!strcmp(arg, \"--stale-fix\"))\n> \t\t\tcb.stalefix = 1;\n> \t\telse if (!strcmp(arg, \"--rewrite\"))\n> -\t\t\tcb.rewrite = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_REWRITE;\n> \t\telse if (!strcmp(arg, \"--updateref\"))\n> \t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n> \t\telse if (!strcmp(arg, \"--all\"))\n> @@ -755,7 +755,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n> \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n> \t\telse if (!strcmp(arg, \"--rewrite\"))\n> -\t\t\tcb.rewrite = 1;\n> +\t\t\tflags |= EXPIRE_REFLOGS_REWRITE;\n> \t\telse if (!strcmp(arg, \"--updateref\"))\n> \t\t\tflags |= EXPIRE_REFLOGS_UPDATE_REF;\n> \t\telse if (!strcmp(arg, \"--verbose\"))\n> -- \n> 2.1.3\n> \n"},{"id":"253386","messageId":"20141208225915.GJ25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-19-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 18/23] Move newlog and last_kept_sha1 to \"struct expire_reflog_cb\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T22:59:15Z","receivedAt":"2014-12-08T22:59:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:30AM +0100, Michael Haggerty wrote:\n> These members are not needed by the policy functions.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n> ---\n> builtin/reflog.c | 20 ++++++++++----------\n> 1 file changed, 10 insertions(+), 10 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 6294406..01b76d0 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -36,7 +36,6 @@ struct cmd_reflog_expire_cb {\n> };\n> \n> struct expire_reflog_policy_cb {\n> -\tFILE *newlog;\n> \tenum {\n> \t\tUE_NORMAL,\n> \t\tUE_ALWAYS,\n> @@ -45,7 +44,6 @@ struct expire_reflog_policy_cb {\n> \tstruct commit_list *mark_list;\n> \tunsigned long mark_limit;\n> \tstruct cmd_reflog_expire_cb *cmd;\n> -\tunsigned char last_kept_sha1[20];\n> \tstruct commit *tip_commit;\n> \tstruct commit_list *tips;\n> };\n> @@ -53,6 +51,8 @@ struct expire_reflog_policy_cb {\n> struct expire_reflog_cb {\n> \tunsigned int flags;\n> \tvoid *policy_cb;\n> +\tFILE *newlog;\n> +\tunsigned char last_kept_sha1[20];\n> };\n> \n> struct collected_reflog {\n> @@ -338,23 +338,23 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \tstruct expire_reflog_policy_cb *policy_cb = cb->policy_cb;\n> \n> \tif (cb->flags & EXPIRE_REFLOGS_REWRITE)\n> -\t\tosha1 = policy_cb->last_kept_sha1;\n> +\t\tosha1 = cb->last_kept_sha1;\n> \n> \tif (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,\n> \t\t\t\t message, policy_cb)) {\n> -\t\tif (!policy_cb->newlog)\n> +\t\tif (!cb->newlog)\n> \t\t\tprintf(\"would prune %s\", message);\n> \t\telse if (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tprintf(\"prune %s\", message);\n> \t} else {\n> -\t\tif (policy_cb->newlog) {\n> +\t\tif (cb->newlog) {\n> \t\t\tchar sign = (tz < 0) ? '-' : '+';\n> \t\t\tint zone = (tz < 0) ? (-tz) : tz;\n> -\t\t\tfprintf(policy_cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n> +\t\t\tfprintf(cb->newlog, \"%s %s %s %lu %c%04d\\t%s\",\n> \t\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1),\n> \t\t\t\temail, timestamp, sign, zone,\n> \t\t\t\tmessage);\n> -\t\t\thashcpy(policy_cb->last_kept_sha1, nsha1);\n> +\t\t\thashcpy(cb->last_kept_sha1, nsha1);\n> \t\t}\n> \t\tif (cb->flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tprintf(\"keep %s\", message);\n> @@ -457,8 +457,8 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n> \t\tif (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)\n> \t\t\tgoto failure;\n> -\t\tpolicy_cb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> -\t\tif (!policy_cb.newlog)\n> +\t\tcb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\n> +\t\tif (!cb.newlog)\n> \t\t\tgoto failure;\n> \t}\n> \n> @@ -474,7 +474,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t\t\t\tstrerror(errno));\n> \t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n> \t\t\t(write_in_full(lock->lock_fd,\n> -\t\t\t\tsha1_to_hex(policy_cb.last_kept_sha1), 40) != 40 ||\n> +\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n> \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n> \t\t\t close_ref(lock) < 0)) {\n> \t\t\tstatus |= error(\"Couldn't write %s\",\n> -- \n> 2.1.3\n> \n"},{"id":"253387","messageId":"20141208231242.GK25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-20-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 19/23] expire_reflog(): treat the policy callback data as opaque","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T23:12:42Z","receivedAt":"2014-12-08T23:12:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:31AM +0100, Michael Haggerty wrote:\n> Now that expire_reflog() doesn't actually look in the\n> expire_reflog_policy_cb data structure, we can make it opaque:\n> \n> * Change its callers to pass it a pointer to an entire \"struct\n> expire_reflog_policy_cb\".\n> \n> * Change it to pass the pointer through as a \"void *\".\n> \n> * Change the policy functions, reflog_expiry_prepare(),\n> reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept\n> \"void *cb_data\" arguments and cast them to \"struct\n> expire_reflog_policy_cb\" internally.\n> \n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n> ---\n> builtin/reflog.c | 73 ++++++++++++++++++++++++++++----------------------------\n> 1 file changed, 36 insertions(+), 37 deletions(-)\n> \n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 01b76d0..c30936bb 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -43,7 +43,7 @@ struct expire_reflog_policy_cb {\n> \t} unreachable_expire_kind;\n> \tstruct commit_list *mark_list;\n> \tunsigned long mark_limit;\n> -\tstruct cmd_reflog_expire_cb *cmd;\n> +\tstruct cmd_reflog_expire_cb cmd;\n> \tstruct commit *tip_commit;\n> \tstruct commit_list *tips;\n> };\n> @@ -309,22 +309,22 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \tstruct expire_reflog_policy_cb *cb = cb_data;\n> \tstruct commit *old, *new;\n> \n> -\tif (timestamp < cb->cmd->expire_total)\n> +\tif (timestamp < cb->cmd.expire_total)\n> \t\treturn 1;\n> \n> \told = new = NULL;\n> -\tif (cb->cmd->stalefix &&\n> +\tif (cb->cmd.stalefix &&\n> \t (!keep_entry(&old, osha1) || !keep_entry(&new, nsha1)))\n> \t\treturn 1;\n> \n> -\tif (timestamp < cb->cmd->expire_unreachable) {\n> +\tif (timestamp < cb->cmd.expire_unreachable) {\n> \t\tif (cb->unreachable_expire_kind == UE_ALWAYS)\n> \t\t\treturn 1;\n> \t\tif (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))\n> \t\t\treturn 1;\n> \t}\n> \n> -\tif (cb->cmd->recno && --(cb->cmd->recno) == 0)\n> +\tif (cb->cmd.recno && --(cb->cmd.recno) == 0)\n> \t\treturn 1;\n> \n> \treturn 0;\n> @@ -378,9 +378,11 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1,\n> \n> static void reflog_expiry_prepare(const char *refname,\n> \t\t\t\t const unsigned char *sha1,\n> -\t\t\t\t struct expire_reflog_policy_cb *cb)\n> +\t\t\t\t void *cb_data)\n> {\n> -\tif (!cb->cmd->expire_unreachable || !strcmp(refname, \"HEAD\")) {\n> +\tstruct expire_reflog_policy_cb *cb = cb_data;\n> +\n> +\tif (!cb->cmd.expire_unreachable || !strcmp(refname, \"HEAD\")) {\n> \t\tcb->tip_commit = NULL;\n> \t\tcb->unreachable_expire_kind = UE_HEAD;\n> \t} else {\n> @@ -391,7 +393,7 @@ static void reflog_expiry_prepare(const char *refname,\n> \t\t\tcb->unreachable_expire_kind = UE_NORMAL;\n> \t}\n> \n> -\tif (cb->cmd->expire_unreachable <= cb->cmd->expire_total)\n> +\tif (cb->cmd.expire_unreachable <= cb->cmd.expire_total)\n> \t\tcb->unreachable_expire_kind = UE_ALWAYS;\n> \n> \tcb->mark_list = NULL;\n> @@ -405,13 +407,15 @@ static void reflog_expiry_prepare(const char *refname,\n> \t\t} else {\n> \t\t\tcommit_list_insert(cb->tip_commit, &cb->mark_list);\n> \t\t}\n> -\t\tcb->mark_limit = cb->cmd->expire_total;\n> +\t\tcb->mark_limit = cb->cmd.expire_total;\n> \t\tmark_reachable(cb);\n> \t}\n> }\n> \n> -static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n> +static void reflog_expiry_cleanup(void *cb_data)\n> {\n> +\tstruct expire_reflog_policy_cb *cb = cb_data;\n> +\n> \tif (cb->unreachable_expire_kind != UE_ALWAYS) {\n> \t\tif (cb->unreachable_expire_kind == UE_HEAD) {\n> \t\t\tstruct commit_list *elem;\n> @@ -427,19 +431,16 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)\n> static struct lock_file reflog_lock;\n> \n> static int expire_reflog(const char *refname, const unsigned char *sha1,\n> -\t\t\t unsigned int flags, void *cb_data)\n> +\t\t\t unsigned int flags, void *policy_cb_data)\n> {\n> -\tstruct cmd_reflog_expire_cb *cmd = cb_data;\n> \tstruct expire_reflog_cb cb;\n> -\tstruct expire_reflog_policy_cb policy_cb;\n> \tstruct ref_lock *lock;\n> \tchar *log_file;\n> \tint status = 0;\n> \n> \tmemset(&cb, 0, sizeof(cb));\n> -\tmemset(&policy_cb, 0, sizeof(policy_cb));\n> \tcb.flags = flags;\n> -\tcb.policy_cb = &policy_cb;\n> +\tcb.policy_cb = policy_cb_data;\n> \n> \t/*\n> \t * we take the lock for the ref itself to prevent it from\n> @@ -462,11 +463,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,\n> \t\t\tgoto failure;\n> \t}\n> \n> -\tpolicy_cb.cmd = cmd;\n> -\n> -\treflog_expiry_prepare(refname, sha1, &policy_cb);\n> +\treflog_expiry_prepare(refname, sha1, cb.policy_cb);\n> \tfor_each_reflog_ent(refname, expire_reflog_ent, &cb);\n> -\treflog_expiry_cleanup(&policy_cb);\n> +\treflog_expiry_cleanup(cb.policy_cb);\n> \n> \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n> \t\tif (close_lock_file(&reflog_lock)) {\n> @@ -639,7 +638,7 @@ static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, c\n> \n> static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> {\n> -\tstruct cmd_reflog_expire_cb cb;\n> +\tstruct expire_reflog_policy_cb cb;\n> \tunsigned long now = time(NULL);\n> \tint i, status, do_all;\n> \tint explicit_expiry = 0;\n> @@ -653,25 +652,25 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \tdo_all = status = 0;\n> \tmemset(&cb, 0, sizeof(cb));\n> \n> -\tcb.expire_total = default_reflog_expire;\n> -\tcb.expire_unreachable = default_reflog_expire_unreachable;\n> +\tcb.cmd.expire_total = default_reflog_expire;\n> +\tcb.cmd.expire_unreachable = default_reflog_expire_unreachable;\n> \n> \tfor (i = 1; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n> \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n> \t\telse if (starts_with(arg, \"--expire=\")) {\n> -\t\t\tif (parse_expiry_date(arg + 9, &cb.expire_total))\n> +\t\t\tif (parse_expiry_date(arg + 9, &cb.cmd.expire_total))\n> \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n> \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n> \t\t}\n> \t\telse if (starts_with(arg, \"--expire-unreachable=\")) {\n> -\t\t\tif (parse_expiry_date(arg + 21, &cb.expire_unreachable))\n> +\t\t\tif (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))\n> \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n> \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n> \t\t}\n> \t\telse if (!strcmp(arg, \"--stale-fix\"))\n> -\t\t\tcb.stalefix = 1;\n> +\t\t\tcb.cmd.stalefix = 1;\n> \t\telse if (!strcmp(arg, \"--rewrite\"))\n> \t\t\tflags |= EXPIRE_REFLOGS_REWRITE;\n> \t\telse if (!strcmp(arg, \"--updateref\"))\n> @@ -695,11 +694,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t * even in older repository. We cannot trust what's reachable\n> \t * from reflog if the repository was pruned with older git.\n> \t */\n> -\tif (cb.stalefix) {\n> -\t\tinit_revisions(&cb.revs, prefix);\n> +\tif (cb.cmd.stalefix) {\n> +\t\tinit_revisions(&cb.cmd.revs, prefix);\n> \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tprintf(\"Marking reachable objects...\");\n> -\t\tmark_reachable_objects(&cb.revs, 0, 0, NULL);\n> +\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n> \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n> \t\t\tputchar('\\n');\n> \t}\n> @@ -712,7 +711,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\tfor_each_reflog(collect_reflog, &collected);\n> \t\tfor (i = 0; i < collected.nr; i++) {\n> \t\t\tstruct collected_reflog *e = collected.e[i];\n> -\t\t\tset_reflog_expiry_param(&cb, explicit_expiry, e->reflog);\n> +\t\t\tset_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);\n> \t\t\tstatus |= expire_reflog(e->reflog, e->sha1, flags, &cb);\n> \t\t\tfree(e);\n> \t\t}\n> @@ -726,7 +725,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n> \t\t\tcontinue;\n> \t\t}\n> -\t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n> +\t\tset_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);\n> \t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n> \t}\n> \treturn status;\n> @@ -736,15 +735,15 @@ static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> \t\tconst char *email, unsigned long timestamp, int tz,\n> \t\tconst char *message, void *cb_data)\n> {\n> -\tstruct cmd_reflog_expire_cb *cb = cb_data;\n> -\tif (!cb->expire_total || timestamp < cb->expire_total)\n> -\t\tcb->recno++;\n> +\tstruct expire_reflog_policy_cb *cb = cb_data;\n> +\tif (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)\n> +\t\tcb->cmd.recno++;\n> \treturn 0;\n> }\n> \n> static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> {\n> -\tstruct cmd_reflog_expire_cb cb;\n> +\tstruct expire_reflog_policy_cb cb;\n> \tint i, status = 0;\n> \tunsigned int flags = 0;\n> \n> @@ -791,12 +790,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> \n> \t\trecno = strtoul(spec + 2, &ep, 10);\n> \t\tif (*ep == '}') {\n> -\t\t\tcb.recno = -recno;\n> +\t\t\tcb.cmd.recno = -recno;\n> \t\t\tfor_each_reflog_ent(ref, count_reflog_ent, &cb);\n> \t\t} else {\n> -\t\t\tcb.expire_total = approxidate(spec + 2);\n> +\t\t\tcb.cmd.expire_total = approxidate(spec + 2);\n> \t\t\tfor_each_reflog_ent(ref, count_reflog_ent, &cb);\n> -\t\t\tcb.expire_total = 0;\n> +\t\t\tcb.cmd.expire_total = 0;\n> \t\t}\n> \n> \t\tstatus |= expire_reflog(ref, sha1, flags, &cb);\n> -- \n> 2.1.3\n> \n"},{"id":"253388","messageId":"20141208233217.GL25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-21-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 20/23] reflog_expire(): new function in the reference API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T23:32:17Z","receivedAt":"2014-12-08T23:32:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:\n> Move expire_reflog() into refs.c and rename it to reflog_expire().\n> Turn the three policy functions into function pointers that are passed\n> into reflog_expire(). Add function prototypes and documentation to\n> refs.h.\n> \n> Signed-off-by: Michael Haggerty \n\nWith or without the nits fixed\nReviewed-by: Stefan Beller \nas the nits are not degrading functionality.\n\n> ---\n> builtin/reflog.c | 133 +++++++------------------------------------------------\n> refs.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++\n> refs.h | 45 +++++++++++++++++++\n> 3 files changed, 174 insertions(+), 118 deletions(-)\n> \n\n\n\n> +static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> +\t\tconst char *email, unsigned long timestamp, int tz,\n> +\t\tconst char *message, void *cb_data)\n\nNit: According to our Codingguidelines we want to indent it further, so it aligns with\nthe arguments from the first line.\n\n+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+ const char *email, unsigned long timestamp, int tz,\n+ const char *message, void *cb_data)\n\n> +\t}\n> +\treturn 0;\n\nWhy do we need the return value for expire_reflog_ent?\nThe \"return 0:\" at the very end of the function is the only return I see here.\n\n> +enum expire_reflog_flags {\n> +\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> +\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n> +\tEXPIRE_REFLOGS_VERBOSE = 1 << 2,\n> +\tEXPIRE_REFLOGS_REWRITE = 1 << 3\n> +};\n\nSometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be\n\nenum expire_reflog_flags {\n EXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n EXPIRE_REFLOGS_VERBOSE = 1 << 2,\n EXPIRE_REFLOGS_REWRITE = 1 << 3\n}\n\nDo we have a preference in the coding style on this one?\n\n\n\n\n> + *\n> + * reflog_expiry_select_fn -- Called once for each entry in the\n> + * existing reflog. It should return true iff that entry should be\n> + * pruned.\n\nAlso I know how we got here, I wonder if we should inverse the logic here\n(in a later patch). \"select\" sounds to me as if the line is selected to keep it.\nHowever the opposite is true. To actually select (keep) the line we need to return\n0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?\n"},{"id":"253389","messageId":"20141208233422.GM25562@google.com","threadId":"38113","inReplyTo":"1417734515-11812-23-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 22/23] lock_any_ref_for_update(): inline function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-08T23:34:22Z","receivedAt":"2014-12-08T23:34:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 05, 2014 at 12:08:34AM +0100, Michael Haggerty wrote:\n> From: Ronnie Sahlberg \n> \n> Inline the function at its one remaining caller (which is within\n> refs.c) and remove it.\n> \n\n\n> Signed-off-by: Michael Haggerty \n\nIt's originally from Ronnie, but his sign off is missing?\n\nIf that sign off is found again, \nReviewed-by: Stefan Beller \n\n> ---\n> refs.c | 9 +--------\n> refs.h | 9 +--------\n> 2 files changed, 2 insertions(+), 16 deletions(-)\n> \n"},{"id":"253426","messageId":"xmqqfvcomzib.fsf@gitster.dls.corp.google.com","threadId":"38113","inReplyTo":"54857871.5090805@alum.mit.edu","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T18:47:24Z","receivedAt":"2014-12-09T18:47:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This isn't documented very well. I thought I saw a comment somewhere in\n> the code that stated it explicitly, but I can't find it now. In any\n> case, my understanding of the locking protocol for reflogs is:\n>\n> The reflog for \"$refname\", which is stored at\n> \"$GIT_DIR/logs/$refname\", is locked by holding\n> \"$GIT_DIR/refs/$refname.lock\", *even if the corresponding\n> reference is packed*.\n>\n> This implies that readers, who don't pay attention to locks, have to be\n> prepared for the possibility that the reflog is in the middle of an\n> update and that the last line is incomplete. This is handled by\n> show_one_reflog_ent(), which discards incomplete lines.\n\nInteresting, as I think I saw Peff did something around that area\nrecently.\n\nI have some more thought around the \"transaction\" in general, but\nit will be in a separate message.\n\nThanks.\n"},{"id":"253429","messageId":"20141209185433.GB31158@peff.net","threadId":"38113","inReplyTo":"xmqqfvcomzib.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T18:54:34Z","receivedAt":"2014-12-09T18:54:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 10:47:24AM -0800, Junio C Hamano wrote:\n\n> Michael Haggerty writes:\n> \n> > This isn't documented very well. I thought I saw a comment somewhere in\n> > the code that stated it explicitly, but I can't find it now. In any\n> > case, my understanding of the locking protocol for reflogs is:\n> >\n> > The reflog for \"$refname\", which is stored at\n> > \"$GIT_DIR/logs/$refname\", is locked by holding\n> > \"$GIT_DIR/refs/$refname.lock\", *even if the corresponding\n> > reference is packed*.\n> >\n> > This implies that readers, who don't pay attention to locks, have to be\n> > prepared for the possibility that the reflog is in the middle of an\n> > update and that the last line is incomplete. This is handled by\n> > show_one_reflog_ent(), which discards incomplete lines.\n> \n> Interesting, as I think I saw Peff did something around that area\n> recently.\n\nYeah, and I had no idea about the truncated-line race which Michael\ndescribed at the time. It makes me glad that I took the time to do the\nmore careful thing[1] (fixing the reverse-reflog parser to properly\ninclude the newline when present) and not the quick-and-easy thing[2]\n(teaching show_one_reflog_ent to accept entries without newlines).\n\nWhat you have queued in jk/for-each-reflog-ent-reverse should be fine,\neven in light of what Michael said.\n\n-Peff\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/260849\n\n[2] http://article.gmane.org/gmane.comp.version-control.git/260807\n"},{"id":"253553","messageId":"5488E196.1000107@alum.mit.edu","threadId":"38113","inReplyTo":"20141208233422.GM25562@google.com","subject":"Re: [PATCH 22/23] lock_any_ref_for_update(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-11T00:13:10Z","receivedAt":"2014-12-11T00:13:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/09/2014 12:34 AM, Stefan Beller wrote:\n> On Fri, Dec 05, 2014 at 12:08:34AM +0100, Michael Haggerty wrote:\n>> From: Ronnie Sahlberg \n>>\n>> Inline the function at its one remaining caller (which is within\n>> refs.c) and remove it.\n>>\n> \n> \n>> Signed-off-by: Michael Haggerty \n> \n> It's originally from Ronnie, but his sign off is missing?\n> \n> If that sign off is found again, \n> Reviewed-by: Stefan Beller \n\nSorry for the confusion.\n\nThis patch ultimately descends from a patch by Ronnie that completely\ndeleted lock_any_ref_for_update() because (in the context that he wrote\nit) there were no remaining callers of that function.\n\nIn the context of this patch, there *is* a remaining caller. So this\npatch is different--it deletes the obvious stuff as in Ronnie's patch,\nbut it also inlines the function at its last caller. And the commit\nmessage is completely different.\n\nGiven those differences and the fact that the only overlap with Ronnie's\noriginal patch is the *deletion* of content, I thought it most\nappropriate to change the authorship of the patch. So I removed Ronnie's\nSigned-off-by line but I forgot to remove his authorship.\n\nIf anybody thinks it would be more appropriate to leave Ronnie as author\nof the new patch or leave his Signed-off-by line, I am totally OK with\ndoing so. I'm just trying to figure out what's right.\n\nOtherwise, I will change the authorship of the patch to myself in the\nupcoming reroll and include only my own Signed-off-by line.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253597","messageId":"548AA5E9.9090201@alum.mit.edu","threadId":"38113","inReplyTo":"20141208233217.GL25562@google.com","subject":"Re: [PATCH 20/23] reflog_expire(): new function in the reference API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-12T08:23:05Z","receivedAt":"2014-12-12T08:23:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/09/2014 12:32 AM, Stefan Beller wrote:\n> On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:\n>> Move expire_reflog() into refs.c and rename it to reflog_expire().\n>> Turn the three policy functions into function pointers that are passed\n>> into reflog_expire(). Add function prototypes and documentation to\n>> refs.h.\n>>\n>> Signed-off-by: Michael Haggerty \n> \n> With or without the nits fixed\n> Reviewed-by: Stefan Beller \n> as the nits are not degrading functionality.\n> \n>> ---\n>> builtin/reflog.c | 133 +++++++------------------------------------------------\n>> refs.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++\n>> refs.h | 45 +++++++++++++++++++\n>> 3 files changed, 174 insertions(+), 118 deletions(-)\n>>\n> \n> \n> \n>> +static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n>> +\t\tconst char *email, unsigned long timestamp, int tz,\n>> +\t\tconst char *message, void *cb_data)\n> \n> Nit: According to our Codingguidelines we want to indent it further, so it aligns with\n> the arguments from the first line.\n\nWill fix.\n\n> +static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n> + const char *email, unsigned long timestamp, int tz,\n> + const char *message, void *cb_data)\n> \n>> +\t}\n>> +\treturn 0;\n> \n> Why do we need the return value for expire_reflog_ent?\n> The \"return 0:\" at the very end of the function is the only return I see here.\n\nexpire_reflog_ent() is passed to for_each_reflog_ent() and therefore\nmust be an each_reflog_ent_fn. If it returns a nonzero value, the\niteration is ended prematurely and the value is returned to the caller\nof for_each_reflog_ent(). We don't ever want to end the iteration\nprematurely here, so we always return 0.\n\n>> +enum expire_reflog_flags {\n>> +\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n>> +\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n>> +\tEXPIRE_REFLOGS_VERBOSE = 1 << 2,\n>> +\tEXPIRE_REFLOGS_REWRITE = 1 << 3\n>> +};\n> \n> Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be\n> \n> enum expire_reflog_flags {\n> EXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n> EXPIRE_REFLOGS_VERBOSE = 1 << 2,\n> EXPIRE_REFLOGS_REWRITE = 1 << 3\n> }\n> \n> Do we have a preference in the coding style on this one?\n\nBoth styles are used in our codebase, and I don't think the style guide\nsays anything about it. My practice in such cases is:\n\n* If I'm modifying existing code, preserve the existing style (to avoid\nunnecessary churn)\n* If most of our code uses one style, then use that style\n* If our code uses both styles frequently, just use whatever style looks\nbetter to me\n\nIf and when somebody cares enough to build a consensus for one policy or\nthe other and to submit a patch to the CodingGuidelines I will be happy\nto follow it.\n\n>> + *\n>> + * reflog_expiry_select_fn -- Called once for each entry in the\n>> + * existing reflog. It should return true iff that entry should be\n>> + * pruned.\n> \n> Also I know how we got here, I wonder if we should inverse the logic here\n> (in a later patch). \"select\" sounds to me as if the line is selected to keep it.\n> However the opposite is true. To actually select (keep) the line we need to return\n> 0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?\n\nYes, that would be clearer. I will make the change.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253601","messageId":"20141212085022.GA11891@peff.net","threadId":"38113","inReplyTo":"548AA5E9.9090201@alum.mit.edu","subject":"Re: [PATCH 20/23] reflog_expire(): new function in the reference API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T08:50:22Z","receivedAt":"2014-12-12T08:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 12, 2014 at 09:23:05AM +0100, Michael Haggerty wrote:\n\n> On 12/09/2014 12:32 AM, Stefan Beller wrote:\n> >> +enum expire_reflog_flags {\n> >> +\tEXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> >> +\tEXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n> >> +\tEXPIRE_REFLOGS_VERBOSE = 1 << 2,\n> >> +\tEXPIRE_REFLOGS_REWRITE = 1 << 3\n> >> +};\n> > \n> > Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be\n> > \n> > enum expire_reflog_flags {\n> > EXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n> > EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n> > EXPIRE_REFLOGS_VERBOSE = 1 << 2,\n> > EXPIRE_REFLOGS_REWRITE = 1 << 3\n> > }\n> > \n> > Do we have a preference in the coding style on this one?\n\nI think vertically aligned lists look really nice. But they often wreak\nhavoc with diffs, because introducing one longer line means re-aligning\nthe whole thing. IMHO, it's not worth it (but if you're going to do it,\nleave lots of extra room for expansion).\n\nJust my two cents, of course. I don't recall this particular style point\ncoming up before.\n\n> Both styles are used in our codebase, and I don't think the style guide\n> says anything about it. My practice in such cases is:\n> \n> * If I'm modifying existing code, preserve the existing style (to avoid\n> unnecessary churn)\n> * If most of our code uses one style, then use that style\n> * If our code uses both styles frequently, just use whatever style looks\n> better to me\n\nI think that is a very good philosophy in general.\n\n-Peff\n"},{"id":"253645","messageId":"xmqqwq5wbss4.fsf@gitster.dls.corp.google.com","threadId":"38113","inReplyTo":"20141212085022.GA11891@peff.net","subject":"Re: [PATCH 20/23] reflog_expire(): new function in the reference API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-12T18:57:15Z","receivedAt":"2014-12-12T18:57:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > enum expire_reflog_flags {\n>> > EXPIRE_REFLOGS_DRY_RUN = 1 << 0,\n>> > EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,\n>> > EXPIRE_REFLOGS_VERBOSE = 1 << 2,\n>> > EXPIRE_REFLOGS_REWRITE = 1 << 3\n>> > }\n>> > \n>> > Do we have a preference in the coding style on this one?\n>\n> I think vertically aligned lists look really nice. But they often wreak\n> havoc with diffs, because introducing one longer line means re-aligning\n> the whole thing. IMHO, it's not worth it (but if you're going to do it,\n> leave lots of extra room for expansion).\n>\n> Just my two cents, of course. I don't recall this particular style point\n> coming up before.\n>\n>> Both styles are used in our codebase, and I don't think the style guide\n>> says anything about it. My practice in such cases is:\n>> \n>> * If I'm modifying existing code, preserve the existing style (to avoid\n>> unnecessary churn)\n>> * If most of our code uses one style, then use that style\n>> * If our code uses both styles frequently, just use whatever style looks\n>> better to me\n>\n> I think that is a very good philosophy in general.\n\nThanks.\n\nAbout the indentation on the second and subsequent lines of a\nlogical line that is split into multiple lines, we explicitly say\n\"Both ar valid, and we use both.\" Following the above three-bullet\nlist would be a good practice for this one, too.\n"}]}