{"thread":{"id":"38093","subject":"[PATCHv2 0/4] refs.c: use transactions for the reflog","startedAt":"2014-12-02T07:46:38Z","lastAt":"2014-12-03T22:52:01Z","messageCount":12,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"252840","messageId":"1417506402-24257-1-git-send-email-sbeller@google.com","threadId":"38093","inReplyTo":null,"subject":"[PATCHv2 0/4] refs.c: use transactions for the reflog","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-02T07:46:38Z","receivedAt":"2014-12-02T07:46:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is the core part of the refs-transactions-reflog series[1],\nwhich was in discussion for a bit already.\n\nThe idea is to have the reflog being part of the transactions, which\nthe refs are already using, so the we're moving towards a database\nlike API in the long run. This makes git easier to maintain as well\nas opening the possibility to replace the backend with a real database.\n\nThe first patch is essentially just some sed magic with reformatting\nthe code, so the naming convention fits better, because the transactions\nwill handle both the refs as well as the reflog after this series.\n\nThe second patch is also just a rename patch.\n\nThe meat and most of the lines of code are found in the 3rd patch.\nIt was completely rewritten from scratch using ideas provided by\nJonathan, thanks! We'll treat the refs and reflogs a bit differently\nwithin the transaction updates data structure, as the hard part for\nthe refs is during the commit phase, while the hard part for the reflogs\nis during the preparation phase now.\n\nWe're using a temporary file for creating the new reflog and rename it\ninto place afterwards utilizing the lock file API.\n\nThe last patch makes actually use of the reflog transaction API.\n\nAny comments would be appreciated,\nThanks,\nStefan\n"},{"id":"252841","messageId":"1417506402-24257-2-git-send-email-sbeller@google.com","threadId":"38093","inReplyTo":"1417506402-24257-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/4] refs.c: rename the transaction functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-02T07:46:39Z","receivedAt":"2014-12-02T07:46:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nRename the transaction functions. Remove the leading ref_ from the\nnames and append _ref to the names for functions that create/delete/\nupdate sha1 refs.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\nThis commit can be reproduced via\nfind . -name \"*.[ch]\" -print | xargs sed -i ' \\\n\ts/REF_TRANSACTION_OPEN/TRANSACTION_OPEN/; \\\n\ts/REF_TRANSACTION_CLOSED/TRANSACTION_CLOSED/; \\\n\ts/ref_transaction_begin/transaction_begin/; \\\n\ts/ref_transaction_commit/transaction_commit/; \\\n\ts/ref_transaction_create/transaction_create_ref/; \\\n\ts/ref_transaction_delete/transaction_delete_ref/; \\\n\ts/ref_transaction_free/transaction_free/; \\\n\ts/ref_transaction_update/transaction_update_ref/; \\\n\ts/ref_transaction/transaction/'\nmodulo white space changes for alignment.\n\nNo changes since sending it 5 days ago.\n\n branch.c               | 13 +++++----\n builtin/commit.c       | 10 +++----\n builtin/fetch.c        | 12 ++++----\n builtin/receive-pack.c | 13 ++++-----\n builtin/replace.c      | 10 +++----\n builtin/tag.c          | 10 +++----\n builtin/update-ref.c   | 26 ++++++++---------\n fast-import.c          | 22 +++++++-------\n refs.c                 | 78 +++++++++++++++++++++++++-------------------------\n refs.h                 | 36 +++++++++++------------\n sequencer.c            | 12 ++++----\n walker.c               | 10 +++----\n 12 files changed, 126 insertions(+), 126 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 4bab55a..c8462de 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -279,16 +279,17 @@ void create_branch(const char *head,\n \t\tlog_all_ref_updates = 1;\n \n \tif (!dont_change_ref) {\n-\t\tstruct ref_transaction *transaction;\n+\t\tstruct transaction *transaction;\n \t\tstruct strbuf err = STRBUF_INIT;\n \n-\t\ttransaction = ref_transaction_begin(&err);\n+\t\ttransaction = transaction_begin(&err);\n \t\tif (!transaction ||\n-\t\t    ref_transaction_update(transaction, ref.buf, sha1,\n-\t\t\t\t\t   null_sha1, 0, !forcing, msg, &err) ||\n-\t\t    ref_transaction_commit(transaction, &err))\n+\t\t    transaction_update_ref(transaction, ref.buf, sha1,\n+\t\t\t\t\t   null_sha1, 0, !forcing, msg,\n+\t\t\t\t\t   &err) ||\n+\t\t    transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n-\t\tref_transaction_free(transaction);\n+\t\ttransaction_free(transaction);\n \t\tstrbuf_release(&err);\n \t}\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e108c53..f50b7df 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1673,7 +1673,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstruct stat statbuf;\n \tstruct commit *current_head = NULL;\n \tstruct commit_extra_header *extra = NULL;\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n@@ -1804,17 +1804,17 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstrbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));\n \tstrbuf_insert(&sb, strlen(reflog_msg), \": \", 2);\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, \"HEAD\", sha1,\n+\t    transaction_update_ref(transaction, \"HEAD\", sha1,\n \t\t\t\t   current_head\n \t\t\t\t   ? current_head->object.sha1 : NULL,\n \t\t\t\t   0, !!current_head, sb.buf, &err) ||\n-\t    ref_transaction_commit(transaction, &err)) {\n+\t    transaction_commit(transaction, &err)) {\n \t\trollback_index_files();\n \t\tdie(\"%s\", err.buf);\n \t}\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \n \tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n \tunlink(git_path(\"REVERT_HEAD\"));\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7b84d35..0be0b09 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -404,7 +404,7 @@ static int s_update_ref(const char *action,\n {\n \tchar msg[1024];\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret, df_conflict = 0;\n \n@@ -414,23 +414,23 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, ref->name, ref->new_sha1,\n+\t    transaction_update_ref(transaction, ref->name, ref->new_sha1,\n \t\t\t\t   ref->old_sha1, 0, check_old, msg, &err))\n \t\tgoto fail;\n \n-\tret = ref_transaction_commit(transaction, &err);\n+\tret = transaction_commit(transaction, &err);\n \tif (ret) {\n \t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n \t\tgoto fail;\n \t}\n \n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn 0;\n fail:\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 32fc540..397abc9 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -838,26 +838,25 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n \telse {\n \t\tstruct strbuf err = STRBUF_INIT;\n-\t\tstruct ref_transaction *transaction;\n+\t\tstruct transaction *transaction;\n \n \t\tif (shallow_update && si->shallow_ref[cmd->index] &&\n \t\t    update_shallow_ref(cmd, si))\n \t\t\treturn \"shallow error\";\n \n-\t\ttransaction = ref_transaction_begin(&err);\n+\t\ttransaction = transaction_begin(&err);\n \t\tif (!transaction ||\n-\t\t    ref_transaction_update(transaction, namespaced_name,\n+\t\t    transaction_update_ref(transaction, namespaced_name,\n \t\t\t\t\t   new_sha1, old_sha1, 0, 1, \"push\",\n \t\t\t\t\t   &err) ||\n-\t\t    ref_transaction_commit(transaction, &err)) {\n-\t\t\tref_transaction_free(transaction);\n-\n+\t\t    transaction_commit(transaction, &err)) {\n+\t\t\ttransaction_free(transaction);\n \t\t\trp_error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t\treturn \"failed to update ref\";\n \t\t}\n \n-\t\tref_transaction_free(transaction);\n+\t\ttransaction_free(transaction);\n \t\tstrbuf_release(&err);\n \t\treturn NULL; /* good */\n \t}\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 85d39b5..5a7ab1f 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -155,7 +155,7 @@ static int replace_object_sha1(const char *object_ref,\n \tunsigned char prev[20];\n \tenum object_type obj_type, repl_type;\n \tchar ref[PATH_MAX];\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n \tobj_type = sha1_object_info(object, NULL);\n@@ -169,14 +169,14 @@ static int replace_object_sha1(const char *object_ref,\n \n \tcheck_ref_valid(object, prev, ref, sizeof(ref), force);\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, ref, repl, prev,\n+\t    transaction_update_ref(transaction, ref, repl, prev,\n \t\t\t\t   0, 1, NULL, &err) ||\n-\t    ref_transaction_commit(transaction, &err))\n+\t    transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \treturn 0;\n }\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e633f4e..5f3554b 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -583,7 +583,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tconst char *msgfile = NULL, *keyid = NULL;\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n \tstruct commit_list *with_commit = NULL;\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n@@ -730,13 +730,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (annotate)\n \t\tcreate_tag(object, tag, &buf, &opt, prev, object);\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, ref.buf, object, prev,\n+\t    transaction_update_ref(transaction, ref.buf, object, prev,\n \t\t\t\t   0, 1, NULL, &err) ||\n-\t    ref_transaction_commit(transaction, &err))\n+\t    transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \tif (force && !is_null_sha1(prev) && hashcmp(prev, object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n \ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 6c9be05..af08dd9 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -175,7 +175,7 @@ static int parse_next_sha1(struct strbuf *input, const char **next,\n  * depending on how line_termination is set.\n  */\n \n-static const char *parse_cmd_update(struct ref_transaction *transaction,\n+static const char *parse_cmd_update(struct transaction *transaction,\n \t\t\t\t    struct strbuf *input, const char *next)\n {\n \tstruct strbuf err = STRBUF_INIT;\n@@ -198,7 +198,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"update %s: extra input: %s\", refname, next);\n \n-\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n+\tif (transaction_update_ref(transaction, refname, new_sha1, old_sha1,\n \t\t\t\t   update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n@@ -209,7 +209,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \treturn next;\n }\n \n-static const char *parse_cmd_create(struct ref_transaction *transaction,\n+static const char *parse_cmd_create(struct transaction *transaction,\n \t\t\t\t    struct strbuf *input, const char *next)\n {\n \tstruct strbuf err = STRBUF_INIT;\n@@ -229,7 +229,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"create %s: extra input: %s\", refname, next);\n \n-\tif (ref_transaction_create(transaction, refname, new_sha1,\n+\tif (transaction_create_ref(transaction, refname, new_sha1,\n \t\t\t\t   update_flags, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n@@ -240,7 +240,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \treturn next;\n }\n \n-static const char *parse_cmd_delete(struct ref_transaction *transaction,\n+static const char *parse_cmd_delete(struct transaction *transaction,\n \t\t\t\t    struct strbuf *input, const char *next)\n {\n \tstruct strbuf err = STRBUF_INIT;\n@@ -264,7 +264,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"delete %s: extra input: %s\", refname, next);\n \n-\tif (ref_transaction_delete(transaction, refname, old_sha1,\n+\tif (transaction_delete_ref(transaction, refname, old_sha1,\n \t\t\t\t   update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n@@ -275,7 +275,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \treturn next;\n }\n \n-static const char *parse_cmd_verify(struct ref_transaction *transaction,\n+static const char *parse_cmd_verify(struct transaction *transaction,\n \t\t\t\t    struct strbuf *input, const char *next)\n {\n \tstruct strbuf err = STRBUF_INIT;\n@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"verify %s: extra input: %s\", refname, next);\n \n-\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n+\tif (transaction_update_ref(transaction, refname, new_sha1, old_sha1,\n \t\t\t\t   update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n@@ -320,7 +320,7 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)\n \treturn next + 8;\n }\n \n-static void update_refs_stdin(struct ref_transaction *transaction)\n+static void update_refs_stdin(struct transaction *transaction)\n {\n \tstruct strbuf input = STRBUF_INIT;\n \tconst char *next;\n@@ -376,9 +376,9 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \n \tif (read_stdin) {\n \t\tstruct strbuf err = STRBUF_INIT;\n-\t\tstruct ref_transaction *transaction;\n+\t\tstruct transaction *transaction;\n \n-\t\ttransaction = ref_transaction_begin(&err);\n+\t\ttransaction = transaction_begin(&err);\n \t\tif (!transaction)\n \t\t\tdie(\"%s\", err.buf);\n \t\tif (delete || no_deref || argc > 0)\n@@ -386,9 +386,9 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin(transaction);\n-\t\tif (ref_transaction_commit(transaction, &err))\n+\t\tif (transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n-\t\tref_transaction_free(transaction);\n+\t\ttransaction_free(transaction);\n \t\tstrbuf_release(&err);\n \t\treturn 0;\n \t}\ndiff --git a/fast-import.c b/fast-import.c\nindex d0bd285..152d944 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1687,7 +1687,7 @@ found_entry:\n static int update_branch(struct branch *b)\n {\n \tstatic const char *msg = \"fast-import\";\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tunsigned char old_sha1[20];\n \tstruct strbuf err = STRBUF_INIT;\n \n@@ -1713,17 +1713,17 @@ static int update_branch(struct branch *b)\n \t\t\treturn -1;\n \t\t}\n \t}\n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n+\t    transaction_update_ref(transaction, b->name, b->sha1, old_sha1,\n \t\t\t\t   0, 1, msg, &err) ||\n-\t    ref_transaction_commit(transaction, &err)) {\n-\t\tref_transaction_free(transaction);\n+\t    transaction_commit(transaction, &err)) {\n+\t\ttransaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t\treturn -1;\n \t}\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn 0;\n }\n@@ -1745,9 +1745,9 @@ static void dump_tags(void)\n \tstruct tag *t;\n \tstruct strbuf ref_name = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction) {\n \t\tfailure |= error(\"%s\", err.buf);\n \t\tgoto cleanup;\n@@ -1756,17 +1756,17 @@ static void dump_tags(void)\n \t\tstrbuf_reset(&ref_name);\n \t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n \n-\t\tif (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n+\t\tif (transaction_update_ref(transaction, ref_name.buf, t->sha1,\n \t\t\t\t\t   NULL, 0, 0, msg, &err)) {\n \t\t\tfailure |= error(\"%s\", err.buf);\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n-\tif (ref_transaction_commit(transaction, &err))\n+\tif (transaction_commit(transaction, &err))\n \t\tfailure |= error(\"%s\", err.buf);\n \n  cleanup:\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \tstrbuf_release(&ref_name);\n \tstrbuf_release(&err);\n }\ndiff --git a/refs.c b/refs.c\nindex 150c980..f0f0d23 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -26,7 +26,7 @@ static unsigned char refname_disposition[256] = {\n };\n \n /*\n- * Used as a flag to ref_transaction_delete when a loose ref is being\n+ * Used as a flag to transaction_delete_ref when a loose ref is being\n  * pruned.\n  */\n #define REF_ISPRUNING\t0x0100\n@@ -2533,23 +2533,23 @@ static void try_remove_empty_parents(char *name)\n /* make sure nobody touched the ref, and unlink */\n static void prune_ref(struct ref_to_prune *r)\n {\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_delete(transaction, r->name, r->sha1,\n+\t    transaction_delete_ref(transaction, r->name, r->sha1,\n \t\t\t\t   REF_ISPRUNING, 1, NULL, &err) ||\n-\t    ref_transaction_commit(transaction, &err)) {\n-\t\tref_transaction_free(transaction);\n+\t    transaction_commit(transaction, &err)) {\n+\t\ttransaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t\treturn;\n \t}\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \tstrbuf_release(&err);\n \ttry_remove_empty_parents(r->name);\n }\n@@ -2711,20 +2711,20 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n \n int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n {\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_delete(transaction, refname, sha1, delopt,\n+\t    transaction_delete_ref(transaction, refname, sha1, delopt,\n \t\t\t\t   sha1 && !is_null_sha1(sha1), NULL, &err) ||\n-\t    ref_transaction_commit(transaction, &err)) {\n+\t    transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n-\t\tref_transaction_free(transaction);\n+\t\ttransaction_free(transaction);\n \t\tstrbuf_release(&err);\n \t\treturn 1;\n \t}\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn 0;\n }\n@@ -3543,9 +3543,9 @@ struct ref_update {\n  *         an active transaction or if there is a failure while building\n  *         the transaction thus rendering it failed/inactive.\n  */\n-enum ref_transaction_state {\n-\tREF_TRANSACTION_OPEN   = 0,\n-\tREF_TRANSACTION_CLOSED = 1\n+enum transaction_state {\n+\tTRANSACTION_OPEN   = 0,\n+\tTRANSACTION_CLOSED = 1\n };\n \n /*\n@@ -3553,21 +3553,21 @@ enum ref_transaction_state {\n  * consist of checks and updates to multiple references, carried out\n  * as atomically as possible.  This structure is opaque to callers.\n  */\n-struct ref_transaction {\n+struct transaction {\n \tstruct ref_update **updates;\n \tsize_t alloc;\n \tsize_t nr;\n-\tenum ref_transaction_state state;\n+\tenum transaction_state state;\n };\n \n-struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n+struct transaction *transaction_begin(struct strbuf *err)\n {\n \tassert(err);\n \n-\treturn xcalloc(1, sizeof(struct ref_transaction));\n+\treturn xcalloc(1, sizeof(struct transaction));\n }\n \n-void ref_transaction_free(struct ref_transaction *transaction)\n+void transaction_free(struct transaction *transaction)\n {\n \tint i;\n \n@@ -3582,7 +3582,7 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \tfree(transaction);\n }\n \n-static struct ref_update *add_update(struct ref_transaction *transaction,\n+static struct ref_update *add_update(struct transaction *transaction,\n \t\t\t\t     const char *refname)\n {\n \tsize_t len = strlen(refname);\n@@ -3594,7 +3594,7 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n \treturn update;\n }\n \n-int ref_transaction_update(struct ref_transaction *transaction,\n+int transaction_update_ref(struct transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   const unsigned char *old_sha1,\n@@ -3605,7 +3605,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \n \tassert(err);\n \n-\tif (transaction->state != REF_TRANSACTION_OPEN)\n+\tif (transaction->state != TRANSACTION_OPEN)\n \t\tdie(\"BUG: update called for transaction that is not open\");\n \n \tif (have_old && !old_sha1)\n@@ -3629,23 +3629,23 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \treturn 0;\n }\n \n-int ref_transaction_create(struct ref_transaction *transaction,\n+int transaction_create_ref(struct transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n-\treturn ref_transaction_update(transaction, refname, new_sha1,\n+\treturn transaction_update_ref(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+int transaction_delete_ref(struct transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *old_sha1,\n \t\t\t   int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n-\treturn ref_transaction_update(transaction, refname, null_sha1,\n+\treturn transaction_update_ref(transaction, refname, null_sha1,\n \t\t\t\t      old_sha1, flags, have_old, msg, err);\n }\n \n@@ -3653,17 +3653,17 @@ int update_ref(const char *action, const char *refname,\n \t       const unsigned char *sha1, const unsigned char *oldval,\n \t       int flags, enum action_on_err onerr)\n {\n-\tstruct ref_transaction *t;\n+\tstruct transaction *t;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\tt = ref_transaction_begin(&err);\n+\tt = transaction_begin(&err);\n \tif (!t ||\n-\t    ref_transaction_update(t, refname, sha1, oldval, flags,\n+\t    transaction_update_ref(t, refname, sha1, oldval, flags,\n \t\t\t\t   !!oldval, action, &err) ||\n-\t    ref_transaction_commit(t, &err)) {\n+\t    transaction_commit(t, &err)) {\n \t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n \n-\t\tref_transaction_free(t);\n+\t\ttransaction_free(t);\n \t\tswitch (onerr) {\n \t\tcase UPDATE_REFS_MSG_ON_ERR:\n \t\t\terror(str, refname, err.buf);\n@@ -3678,7 +3678,7 @@ int update_ref(const char *action, const char *refname,\n \t\treturn 1;\n \t}\n \tstrbuf_release(&err);\n-\tref_transaction_free(t);\n+\ttransaction_free(t);\n \treturn 0;\n }\n \n@@ -3706,8 +3706,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \treturn 0;\n }\n \n-int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t   struct strbuf *err)\n+int transaction_commit(struct transaction *transaction,\n+\t\t       struct strbuf *err)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3716,11 +3716,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \tassert(err);\n \n-\tif (transaction->state != REF_TRANSACTION_OPEN)\n+\tif (transaction->state != TRANSACTION_OPEN)\n \t\tdie(\"BUG: commit called for transaction that is not open\");\n \n \tif (!n) {\n-\t\ttransaction->state = REF_TRANSACTION_CLOSED;\n+\t\ttransaction->state = TRANSACTION_CLOSED;\n \t\treturn 0;\n \t}\n \n@@ -3799,7 +3799,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n-\ttransaction->state = REF_TRANSACTION_CLOSED;\n+\ttransaction->state = TRANSACTION_CLOSED;\n \n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\ndiff --git a/refs.h b/refs.h\nindex 7d675b7..556adfd 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -11,22 +11,22 @@ struct ref_lock {\n };\n \n /*\n- * A ref_transaction represents a collection of ref updates\n+ * A transaction represents a collection of ref updates\n  * that should succeed or fail together.\n  *\n  * Calling sequence\n  * ----------------\n- * - Allocate and initialize a `struct ref_transaction` by calling\n- *   `ref_transaction_begin()`.\n+ * - Allocate and initialize a `struct transaction` by calling\n+ *   `transaction_begin()`.\n  *\n  * - List intended ref updates by calling functions like\n- *   `ref_transaction_update()` and `ref_transaction_create()`.\n+ *   `transaction_update_ref()` and `transaction_create_ref()`.\n  *\n- * - Call `ref_transaction_commit()` to execute the transaction.\n+ * - Call `transaction_commit()` to execute the transaction.\n  *   If this succeeds, the ref updates will have taken place and\n  *   the transaction cannot be rolled back.\n  *\n- * - At any time call `ref_transaction_free()` to discard the\n+ * - At any time call `transaction_free()` to discard the\n  *   transaction and free associated resources.  In particular,\n  *   this rolls back the transaction if it has not been\n  *   successfully committed.\n@@ -42,7 +42,7 @@ struct ref_lock {\n  * The message is appended to err without first clearing err.\n  * err will not be '\\n' terminated.\n  */\n-struct ref_transaction;\n+struct transaction;\n \n /*\n  * Bit values set in the flags argument passed to each_ref_fn():\n@@ -181,8 +181,8 @@ 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 lock_any_ref_for_update(), transaction_update_ref(),\n+ * transaction_create_ref(), etc.\n  * REF_NODEREF: act on the ref directly, instead of dereferencing\n  *              symbolic references.\n  * REF_DELETING: tolerate broken refs\n@@ -269,13 +269,13 @@ enum action_on_err {\n \n /*\n  * Begin a reference transaction.  The reference transaction must\n- * be freed by calling ref_transaction_free().\n+ * be freed by calling transaction_free().\n  */\n-struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n+struct transaction *transaction_begin(struct strbuf *err);\n \n /*\n  * The following functions add a reference check or update to a\n- * ref_transaction.  In all of them, refname is the name of the\n+ * transaction.  In all of them, refname is the name of the\n  * reference to be affected.  The functions make internal copies of\n  * refname and msg, so the caller retains ownership of these parameters.\n  * flags can be REF_NODEREF; it is passed to update_ref_lock().\n@@ -291,7 +291,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n  * means that the transaction as a whole has failed and will need to be\n  * rolled back.\n  */\n-int ref_transaction_update(struct ref_transaction *transaction,\n+int transaction_update_ref(struct transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   const unsigned char *old_sha1,\n@@ -307,7 +307,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n  * means that the transaction as a whole has failed and will need to be\n  * rolled back.\n  */\n-int ref_transaction_create(struct ref_transaction *transaction,\n+int transaction_create_ref(struct transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   int flags, const char *msg,\n@@ -321,7 +321,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n  * means that the transaction as a whole has failed and will need to be\n  * rolled back.\n  */\n-int ref_transaction_delete(struct ref_transaction *transaction,\n+int transaction_delete_ref(struct transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *old_sha1,\n \t\t\t   int flags, int have_old, const char *msg,\n@@ -337,13 +337,13 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n #define TRANSACTION_NAME_CONFLICT -1\n /* All other errors. */\n #define TRANSACTION_GENERIC_ERROR -2\n-int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t   struct strbuf *err);\n+int transaction_commit(struct transaction *transaction,\n+\t\t       struct strbuf *err);\n \n /*\n  * Free an existing transaction and all associated data.\n  */\n-void ref_transaction_free(struct ref_transaction *transaction);\n+void transaction_free(struct transaction *transaction);\n \n /** Lock a ref and then write its file */\n int update_ref(const char *action, const char *refname,\ndiff --git a/sequencer.c b/sequencer.c\nindex a03d4fa..f888005 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -238,7 +238,7 @@ static int error_dirty_index(struct replay_opts *opts)\n static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \t\t\tint unborn, struct replay_opts *opts)\n {\n-\tstruct ref_transaction *transaction;\n+\tstruct transaction *transaction;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \n@@ -248,13 +248,13 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \n \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n \n-\ttransaction = ref_transaction_begin(&err);\n+\ttransaction = transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, \"HEAD\",\n+\t    transaction_update_ref(transaction, \"HEAD\",\n \t\t\t\t   to, unborn ? null_sha1 : from,\n \t\t\t\t   0, 1, sb.buf, &err) ||\n-\t    ref_transaction_commit(transaction, &err)) {\n-\t\tref_transaction_free(transaction);\n+\t    transaction_commit(transaction, &err)) {\n+\t\ttransaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&sb);\n \t\tstrbuf_release(&err);\n@@ -263,7 +263,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \n \tstrbuf_release(&sb);\n \tstrbuf_release(&err);\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \treturn 0;\n }\n \ndiff --git a/walker.c b/walker.c\nindex f149371..f1d5e9b 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -253,7 +253,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n {\n \tstruct strbuf refname = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n-\tstruct ref_transaction *transaction = NULL;\n+\tstruct transaction *transaction = NULL;\n \tunsigned char *sha1 = xmalloc(targets * 20);\n \tchar *msg = NULL;\n \tint i, ret = -1;\n@@ -261,7 +261,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tsave_commit_buffer = 0;\n \n \tif (write_ref) {\n-\t\ttransaction = ref_transaction_begin(&err);\n+\t\ttransaction = transaction_begin(&err);\n \t\tif (!transaction) {\n \t\t\terror(\"%s\", err.buf);\n \t\t\tgoto done;\n@@ -298,7 +298,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\t\tcontinue;\n \t\tstrbuf_reset(&refname);\n \t\tstrbuf_addf(&refname, \"refs/%s\", write_ref[i]);\n-\t\tif (ref_transaction_update(transaction, refname.buf,\n+\t\tif (transaction_update_ref(transaction, refname.buf,\n \t\t\t\t\t   &sha1[20 * i], NULL, 0, 0,\n \t\t\t\t\t   msg ? msg : \"fetch (unknown)\",\n \t\t\t\t\t   &err)) {\n@@ -306,7 +306,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\t\tgoto done;\n \t\t}\n \t}\n-\tif (ref_transaction_commit(transaction, &err)) {\n+\tif (transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tgoto done;\n \t}\n@@ -314,7 +314,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tret = 0;\n \n done:\n-\tref_transaction_free(transaction);\n+\ttransaction_free(transaction);\n \tfree(msg);\n \tfree(sha1);\n \tstrbuf_release(&err);\n-- \n2.2.0\n"},{"id":"252843","messageId":"1417506402-24257-3-git-send-email-sbeller@google.com","threadId":"38093","inReplyTo":"1417506402-24257-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/4] refs.c: rename transaction.updates to transaction.ref_updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-02T07:46:40Z","receivedAt":"2014-12-02T07:46:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The updates are only holding refs not reflogs, so express it to the reader.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n* only renaming, no extra code in this patch.\n* new to the series.\n\n refs.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f0f0d23..58de60c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3554,7 +3554,7 @@ enum transaction_state {\n  * as atomically as possible.  This structure is opaque to callers.\n  */\n struct transaction {\n-\tstruct ref_update **updates;\n+\tstruct ref_update **ref_updates;\n \tsize_t alloc;\n \tsize_t nr;\n \tenum transaction_state state;\n@@ -3575,10 +3575,10 @@ void transaction_free(struct transaction *transaction)\n \t\treturn;\n \n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tfree(transaction->updates[i]->msg);\n-\t\tfree(transaction->updates[i]);\n+\t\tfree(transaction->ref_updates[i]->msg);\n+\t\tfree(transaction->ref_updates[i]);\n \t}\n-\tfree(transaction->updates);\n+\tfree(transaction->ref_updates);\n \tfree(transaction);\n }\n \n@@ -3589,8 +3589,8 @@ static struct ref_update *add_update(struct transaction *transaction,\n \tstruct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);\n \n \tstrcpy((char *)update->refname, refname);\n-\tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n-\ttransaction->updates[transaction->nr++] = update;\n+\tALLOC_GROW(transaction->ref_updates, transaction->nr + 1, transaction->alloc);\n+\ttransaction->ref_updates[transaction->nr++] = update;\n \treturn update;\n }\n \n@@ -3712,7 +3712,7 @@ int transaction_commit(struct transaction *transaction,\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n \tint n = transaction->nr;\n-\tstruct ref_update **updates = transaction->updates;\n+\tstruct ref_update **updates = transaction->ref_updates;\n \n \tassert(err);\n \n-- \n2.2.0\n"},{"id":"252842","messageId":"1417506402-24257-4-git-send-email-sbeller@google.com","threadId":"38093","inReplyTo":"1417506402-24257-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/4] refs.c: add a transaction function to append a reflog entry","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-02T07:46:41Z","receivedAt":"2014-12-02T07:46:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When performing a reflog transaction update, only write to the reflog iff\nmsg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform\nan update that only truncates but does not write. This change only affects\nwhether or not a reflog entry should be generated and written. If msg == NULL\nthen no such entry will be written.\n\nOrthogonal to this we have a boolean flag REFLOG_TRUNCATE which is used to\ntell the transaction system to \"truncate the reflog and thus discard all\nprevious users\".\n\nAt the current time the only place where we use msg == NULL is also the\nplace, where we use REFLOG_TRUNCATE. Even though these two settings are\ncurrently only ever used together it still makes sense to have them through\ntwo separate knobs.\n\nThis allows future consumers of this API that may want to do things\ndifferently. For example someone can do:\n  msg=\"Reflog truncated by Bob because ...\" + REFLOG_TRUNCATE\nand have it truncate the log and have it start fresh with an initial message\nthat explains the log was truncated. This API allows that.\n\nDuring one transaction we allow to make multiple reflog updates to the\nsame ref. This means we only need to lock the reflog once, during the first\nupdate that touches the reflog, and that all further updates can just write the\nreflog entry since the reflog is already locked.\n\nThis allows us to write code such as:\n\n  t = transaction_begin()\n  transaction_reflog_update(t, \"foo\", REFLOG_TRUNCATE, NULL);\n  loop-over-something...\n     transaction_reflog_update(t, \"foo\", 0, <message>);\n  transaction_commit(t)\n\nwhere we first truncate the reflog and then build the new content one line\nat a time.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n\n---\nThis is a complete rewrite of the patch you would have found earlier on\nthe list. The approach was changed to deal with the reflogs differently\nand not toss them into the array containing all the ref_updates, but\nkeep them in a separate string list.\n\nAs I am borrowing some text for the commit message and some ideas how to approach\nthe problem, I still added Ronnies sign off.\n\n refs.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n refs.h |  21 +++++++++++\n 2 files changed, 146 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 58de60c..2861486 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3557,6 +3557,12 @@ struct transaction {\n \tstruct ref_update **ref_updates;\n \tsize_t alloc;\n \tsize_t nr;\n+\n+\t/*\n+\t * Sorted list of reflogs to be committed,\n+\t * the util points to the lock_file\n+\t */\n+\tstruct string_list reflog_updates;\n \tenum transaction_state state;\n };\n \n@@ -3564,7 +3570,10 @@ struct transaction *transaction_begin(struct strbuf *err)\n {\n \tassert(err);\n \n-\treturn xcalloc(1, sizeof(struct transaction));\n+\tstruct transaction *ret = xcalloc(1, sizeof(struct transaction));\n+\tstring_list_init(&ret->reflog_updates, 1);\n+\n+\treturn ret;\n }\n \n void transaction_free(struct transaction *transaction)\n@@ -3629,6 +3638,112 @@ int transaction_update_ref(struct transaction *transaction,\n \treturn 0;\n }\n \n+/* Returns a fd, -1 on error. */\n+static int get_reflog_updates_fd(struct transaction *transaction,\n+\t\t\t\t const char *refname,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tchar *path;\n+\tstruct lock_file *lock;\n+\tstruct string_list_item *item = string_list_insert(\n+\t\t\t\t\t\t&transaction->reflog_updates,\n+\t\t\t\t\t\trefname);\n+\tif (!item->util) {\n+\t\titem->util = xcalloc(1, sizeof(struct lock_file));\n+\t\tlock = item->util;\n+\t\tpath = git_path(\"logs/locks/%s\", refname);\n+\t\tif (safe_create_leading_directories(path)) {\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t    \"creating temporary directories %s failed.\",\n+\t\t\t\t    path);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\tif (hold_lock_file_for_update(lock, path, 0) < 0) {\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t    \"creating temporary file %s failed.\",\n+\t\t\t\t    path);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\n+\tlock = item->util;\n+\n+\treturn lock->fd;\n+}\n+\n+int transaction_update_reflog(struct transaction *transaction,\n+\t\t\t      const char *refname,\n+\t\t\t      const unsigned char *new_sha1,\n+\t\t\t      const unsigned char *old_sha1,\n+\t\t\t      const char *email,\n+\t\t\t      unsigned long timestamp, int tz,\n+\t\t\t      const char *msg, int flags,\n+\t\t\t      struct strbuf *err)\n+{\n+\t/*\n+\t * We cannot handle reflogs the same way we handle refs because of\n+\t * naming conflicts in the file system.\n+\t *\n+\t * If renaming a ref from foo/foo to foo or the other way round,\n+\t * we need to be careful as we need the basic foo/ from being a\n+\t * directory to be a file or vice versa. When handling the refs\n+\t * this can be solved easily by first recording all we want into\n+\t * the packed refs file and then deleting all the loose refs. By\n+\t * doing it that way, we always have a valid state on disk.\n+\t *\n+\t * We don't have an equivalent of a packed refs file when dealing\n+\t * with reflog updates, but the API for updating the refs turned\n+\t * out to be conveniant, so let's introduce an intermediate file\n+\t * outside the $GIT_DIR/logs/refs/heads/ directory helping us\n+\t * avoiding this naming conflict for the reflogs. The intermediate\n+\t * lock file, in which we build up the new reflog will live under\n+\t * $GIT_DIR/logs/lock/refs/heads/ so the files\n+\t *       $GIT_DIR/logs/lock/refs/heads/foo.lock and\n+\t *       $GIT_DIR/logs/lock/refs/heads/foo/foo.lock\n+\t * do not collide.\n+\t */\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tif (transaction->state != TRANSACTION_OPEN)\n+\t\tdie(\"BUG: update_reflog called for transaction that is not open\");\n+\n+\tfd = get_reflog_updates_fd(transaction, refname, err);\n+\tif (!fd)\n+\t\tgoto failure;\n+\n+\tif (flags & REFLOG_TRUNCATE) {\n+\t\tif (lseek(fd, 0, SEEK_SET) < 0 ||\n+\t\t\tftruncate(fd, 0)) {\n+\t\t\tstrbuf_addf(err, \"Could not truncate reflog: %s. %s\",\n+\t\t\t\t    refname, strerror(errno));\n+\t\t\tgoto failure;\n+\t\t}\n+\t}\n+\tif (email)\n+\t\tstrbuf_addf(&buf, \"%s %lu %+05d\", email, timestamp, tz);\n+\n+\tif (msg &&\n+\t    log_ref_write_fd(fd, old_sha1, new_sha1, buf.buf, msg)) {\n+\t\tstrbuf_addf(err, \"Could not write to reflog: %s. %s\",\n+\t\t\t    refname, strerror(errno));\n+\t\tgoto failure;\n+\t}\n+\tstrbuf_release(&buf);\n+\n+\treturn 0;\n+failure:\n+\tstrbuf_release(&buf);\n+\t/*\n+\t * As we are using the lock file API, any stale files left behind will\n+\t * be taken care of, no need to do anything here.\n+\t */\n+\n+\ttransaction->state = TRANSACTION_CLOSED;\n+\treturn 1;\n+}\n+\n int transaction_create_ref(struct transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n@@ -3713,13 +3828,14 @@ int transaction_commit(struct transaction *transaction,\n \tconst char **delnames;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->ref_updates;\n+\tstruct string_list_item *item;\n \n \tassert(err);\n \n \tif (transaction->state != TRANSACTION_OPEN)\n \t\tdie(\"BUG: commit called for transaction that is not open\");\n \n-\tif (!n) {\n+\tif (!n && !transaction->reflog_updates.nr) {\n \t\ttransaction->state = TRANSACTION_CLOSED;\n \t\treturn 0;\n \t}\n@@ -3796,6 +3912,13 @@ int transaction_commit(struct transaction *transaction,\n \t}\n \tfor (i = 0; i < delnum; i++)\n \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\n+\t/* Commit all reflog updates*/\n+\tfor_each_string_list_item(item, &transaction->reflog_updates) {\n+\t\tstruct lock_file *lock = item->util;\n+\t\tcommit_lock_file_to(lock, git_path(\"logs/%s\", item->string));\n+\t}\n+\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\ndiff --git a/refs.h b/refs.h\nindex 556adfd..9f70b89 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -328,6 +328,27 @@ int transaction_delete_ref(struct transaction *transaction,\n \t\t\t   struct strbuf *err);\n \n /*\n+ * Flags controlling transaction_update_reflog().\n+ * REFLOG_TRUNCATE: Truncate the reflog.\n+ *\n+ * Flags >= 0x100 are reserved for internal use.\n+ */\n+#define REFLOG_TRUNCATE 0x01\n+/*\n+ * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set\n+ * this update will first truncate the reflog before writing the entry.\n+ * If msg is NULL no update will be written to the log.\n+ */\n+int transaction_update_reflog(struct transaction *transaction,\n+\t\t\t      const char *refname,\n+\t\t\t      const unsigned char *new_sha1,\n+\t\t\t      const unsigned char *old_sha1,\n+\t\t\t      const char *email,\n+\t\t\t      unsigned long timestamp, int tz,\n+\t\t\t      const char *msg, int flags,\n+\t\t\t      struct strbuf *err);\n+\n+/*\n  * Commit all of the changes that have been queued in transaction, as\n  * atomically as possible.\n  *\n-- \n2.2.0\n"},{"id":"252844","messageId":"1417506402-24257-5-git-send-email-sbeller@google.com","threadId":"38093","inReplyTo":"1417506402-24257-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/4] reflog.c: use a reflog transaction when writing during expire","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-02T07:46:42Z","receivedAt":"2014-12-02T07:46:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nUse a transaction for all updates during expire_reflog.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nno changes since sending it 5 days ago.\n\n builtin/reflog.c | 86 ++++++++++++++++++++++++--------------------------------\n 1 file changed, 37 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 2d85d26..55f3023 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -33,7 +33,8 @@ struct cmd_reflog_expire_cb {\n };\n \n struct expire_reflog_cb {\n-\tFILE *newlog;\n+\tstruct transaction *t;\n+\tconst char *refname;\n \tenum {\n \t\tUE_NORMAL,\n \t\tUE_ALWAYS,\n@@ -290,7 +291,7 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig\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+\t\tconst char *message, void *cb_data, struct strbuf *err)\n {\n \tstruct expire_reflog_cb *cb = cb_data;\n \tstruct commit *old, *new;\n@@ -316,20 +317,18 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\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+\tif (cb->t) {\n+\t\tif (transaction_update_reflog(cb->t, cb->refname, nsha1, osha1,\n+\t\t\t\t\t      email, timestamp, tz, message, 0,\n+\t\t\t\t\t      err))\n+\t\t\treturn -1;\n \t\thashcpy(cb->last_kept_sha1, nsha1);\n \t}\n \tif (cb->cmd->verbose)\n \t\tprintf(\"keep %s\", message);\n \treturn 0;\n  prune:\n-\tif (!cb->newlog)\n+\tif (!cb->t)\n \t\tprintf(\"would prune %s\", message);\n \telse if (cb->cmd->verbose)\n \t\tprintf(\"prune %s\", message);\n@@ -353,29 +352,27 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\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 \tstruct commit *tip_commit;\n \tstruct commit_list *tips;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint status = 0;\n \n \tmemset(&cb, 0, sizeof(cb));\n+\tcb.refname = ref;\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(ref, 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\tgoto finish;\n-\tif (!cmd->dry_run) {\n-\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", ref);\n-\t\tcb.newlog = fopen(newlog_path, \"w\");\n+\tcb.t = transaction_begin(&err);\n+\tif (!cb.t) {\n+\t\tstatus |= error(\"%s\", err.buf);\n+\t\tgoto cleanup;\n+\t}\n+\tif (transaction_update_reflog(cb.t, cb.refname, null_sha1, null_sha1,\n+\t\t\t\t      NULL, 0, 0, NULL, REFLOG_TRUNCATE,\n+\t\t\t\t      &err)) {\n+\t\tstatus |= error(\"%s\", err.buf);\n+\t\tgoto cleanup;\n \t}\n-\n \tcb.cmd = cmd;\n \n \tif (!cmd->expire_unreachable || !strcmp(ref, \"HEAD\")) {\n@@ -407,7 +404,10 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\tmark_reachable(&cb);\n \t}\n \n-\tfor_each_reflog_ent(ref, expire_reflog_ent, &cb);\n+\tif (for_each_reflog_ent(ref, expire_reflog_ent, &cb)) {\n+\t\tstatus |= error(\"%s\", err.buf);\n+\t\tgoto cleanup;\n+\t}\n \n \tif (cb.unreachable_expire_kind != UE_ALWAYS) {\n \t\tif (cb.unreachable_expire_kind == UE_HEAD) {\n@@ -420,32 +420,20 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t}\n \t}\n  finish:\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\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-\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\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} 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+\tif (!cmd->dry_run) {\n+\t\tif (cmd->updateref &&\n+\t\t    transaction_update_ref(cb.t, cb.refname,\n+\t\t\t\t\t   cb.last_kept_sha1, sha1,\n+\t\t\t\t\t   0, 1, NULL, &err)) {\n+\t\t\tstatus |= error(\"%s\", err.buf);\n+\t\t\tgoto cleanup;\n \t\t}\n+\t\tif (transaction_commit(cb.t, &err))\n+\t\t\tstatus |= error(\"%s\", err.buf);\n \t}\n-\tfree(newlog_path);\n-\tfree(log_file);\n-\tunlock_ref(lock);\n+ cleanup:\n+\ttransaction_free(cb.t);\n+\tstrbuf_release(&err);\n \treturn status;\n }\n \n-- \n2.2.0\n"},{"id":"252911","messageId":"20141203022444.GD6527@google.com","threadId":"38093","inReplyTo":"1417506402-24257-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/4] refs.c: rename the transaction functions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T02:24:44Z","receivedAt":"2014-12-03T02:24:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> No changes since sending it 5 days ago.\n>\n>  branch.c               | 13 +++++----\n>  builtin/commit.c       | 10 +++----\n>  builtin/fetch.c        | 12 ++++----\n>  builtin/receive-pack.c | 13 ++++-----\n>  builtin/replace.c      | 10 +++----\n>  builtin/tag.c          | 10 +++----\n>  builtin/update-ref.c   | 26 ++++++++---------\n>  fast-import.c          | 22 +++++++-------\n>  refs.c                 | 78 +++++++++++++++++++++++++-------------------------\n>  refs.h                 | 36 +++++++++++------------\n>  sequencer.c            | 12 ++++----\n>  walker.c               | 10 +++----\n>  12 files changed, 126 insertions(+), 126 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"252913","messageId":"20141203024301.GE6527@google.com","threadId":"38093","inReplyTo":"1417506402-24257-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] refs.c: rename transaction.updates to transaction.ref_updates","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T02:43:01Z","receivedAt":"2014-12-03T02:43:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> The updates are only holding refs not reflogs, so express it to the reader.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  refs.c | 14 +++++++-------\n>  1 file changed, 7 insertions(+), 7 deletions(-)\n\nMakes sense.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"252915","messageId":"20141203031519.GF6527@google.com","threadId":"38093","inReplyTo":"1417506402-24257-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/4] refs.c: add a transaction function to append a reflog entry","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T03:15:19Z","receivedAt":"2014-12-03T03:15:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> When performing a reflog transaction update, only write to the reflog iff\n> msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform\n> an update that only truncates but does not write. This change only affects\n> whether or not a reflog entry should be generated and written. If msg == NULL\n> then no such entry will be written.\n>\n> Orthogonal to this we have a boolean flag REFLOG_TRUNCATE which is used to\n> tell the transaction system to \"truncate the reflog and thus discard all\n> previous users\".\n>\n> At the current time the only place where we use msg == NULL is also the\n> place, where we use REFLOG_TRUNCATE. Even though these two settings are\n> currently only ever used together it still makes sense to have them through\n> two separate knobs.\n>\n> This allows future consumers of this API that may want to do things\n> differently. For example someone can do:\n>   msg=\"Reflog truncated by Bob because ...\" + REFLOG_TRUNCATE\n> and have it truncate the log and have it start fresh with an initial message\n> that explains the log was truncated. This API allows that.\n>\n> During one transaction we allow to make multiple reflog updates to the\n> same ref. This means we only need to lock the reflog once, during the first\n> update that touches the reflog, and that all further updates can just write the\n> reflog entry since the reflog is already locked.\n\nI'm having trouble parsing all of the above.  Can you explain the\nmotivation of the patch in a sentence or so?  Afterward that, if the\nAPI is not self-explanatory, there could be a short explanation of it\n(e.g., a list of functions and how they get used).\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3557,6 +3557,12 @@ struct transaction {\n>  \tstruct ref_update **ref_updates;\n>  \tsize_t alloc;\n>  \tsize_t nr;\n> +\n> +\t/*\n> +\t * Sorted list of reflogs to be committed,\n> +\t * the util points to the lock_file\n> +\t */\n> +\tstruct string_list reflog_updates;\n\nGrammar nit: where there is a comma here, there should be the end of a\nsentence.\n\n[...]\n> @@ -3564,7 +3570,10 @@ struct transaction *transaction_begin(struct strbuf *err)\n>  {\n>  \tassert(err);\n>  \n> -\treturn xcalloc(1, sizeof(struct transaction));\n> +\tstruct transaction *ret = xcalloc(1, sizeof(struct transaction));\n> +\tstring_list_init(&ret->reflog_updates, 1);\n\nCan do\n\n\tret->reflog_updates.strdup_strings = 1;\n\ninstead, since calloc already zeroed the memory.\n\n[...]\n> +/* Returns a fd, -1 on error. */\n> +static int get_reflog_updates_fd(struct transaction *transaction,\n> +\t\t\t\t const char *refname,\n> +\t\t\t\t struct strbuf *err)\n> +{\n> +\tchar *path;\n> +\tstruct lock_file *lock;\n> +\tstruct string_list_item *item = string_list_insert(\n> +\t\t\t\t\t\t&transaction->reflog_updates,\n> +\t\t\t\t\t\trefname);\n> +\tif (!item->util) {\n\nIt can be clearer to handle the simple case first:\n\n\tif (item->util) {\n\t\tlock = item->util;\n\t\treturn lock->fd;\n\t}\n\n\titem->util = xcalloc(...);\n\n> +\t\titem->util = xcalloc(1, sizeof(struct lock_file));\n> +\t\tlock = item->util;\n> +\t\tpath = git_path(\"logs/locks/%s\", refname);\n> +\t\tif (safe_create_leading_directories(path)) {\n> +\t\t\tstrbuf_addf(err,\n> +\t\t\t\t    \"creating temporary directories %s failed.\",\n> +\t\t\t\t    path);\n\nLooking at other callers, looks like something like\n\n\tif (scld(path)) {\n\t\tstrbuf_addf(err, \"could not create leading directories of '%s': %s\",\n\t\t\t    path, strerror(errno));\n\nis common.  That way, the message includes details from errno, it's\nclear that one of the leading directories to $path, not $path itself,\nwas what could not be created, and there is no trailing '.' at the end\nof the message.\n\n> +\t\tif (hold_lock_file_for_update(lock, path, 0) < 0) {\n> +\t\t\tstrbuf_addf(err,\n> +\t\t\t\t    \"creating temporary file %s failed.\",\n> +\t\t\t\t    path);\n\nhold_lock_file_for_update() is weird and has its own special error\nmessage writing function:\n\n\t\t\tunable_to_lock_message(path, errno, err);\n\nThat lets it give advice to the user about why writing the .lock\nfile failed and how to fix it.\n\nI have a series that simplifies by making it write directly to a\nstrbuf passed as an argument, but that's orthogonal to this patch.\n\n[...]\n> +int transaction_update_reflog(struct transaction *transaction,\n> +\t\t\t      const char *refname,\n> +\t\t\t      const unsigned char *new_sha1,\n> +\t\t\t      const unsigned char *old_sha1,\n> +\t\t\t      const char *email,\n> +\t\t\t      unsigned long timestamp, int tz,\n> +\t\t\t      const char *msg, int flags,\n> +\t\t\t      struct strbuf *err)\n\nThis is an intimidating list of arguments.  Would it make sense to\npack them into a struct, or to make the list less intimidating\nsome other way (e.g. combining email + timestamp + tz into an\nident string)?\n\n[...]\n> +\tfd = get_reflog_updates_fd(transaction, refname, err);\n> +\tif (!fd)\n> +\t\tgoto failure;\n\nif (fd < 0)\n\n[...]\n> +\tif (flags & REFLOG_TRUNCATE) {\n> +\t\tif (lseek(fd, 0, SEEK_SET) < 0 ||\n> +\t\t\tftruncate(fd, 0)) {\n> +\t\t\tstrbuf_addf(err, \"Could not truncate reflog: %s. %s\",\n> +\t\t\t\t    refname, strerror(errno));\n\nOdd error message format (using '.' to separate the refname from\nstrerror(errno) is unusual).  Errors normally are supposed to start\nwith a lowercase letter, like\n\n\tcannot truncate reflog '%s': %s\n\n> +\t\t\tgoto failure;\n> +\t\t}\n> +\t}\n\nHow does this cause the reflog to be populated in the !(flags &\nREFLOG_TRUNCATE) case?\n\nMaybe I am misunderstanding the API.  If I use\ntransaction_update_reflog() and have not updated the reflog\npreviously, isn't this supposed to just append a new entry to the\nreflog?\n\n[...]\n> +failure:\n> +\tstrbuf_release(&buf);\n> +\t/*\n> +\t * As we are using the lock file API, any stale files left behind will\n> +\t * be taken care of, no need to do anything here.\n> +\t */\n\nThat's only true if the caller is going to exit instead of proceeding\nto other work.\n\nWith current callers, I assume that's true.  So should this comment\nsay something like \"No need to roll back stale lock files because\nthe caller will exit soon\"?  Or should this roll back the lockfile\nanyway, in case the caller wants to try again?\n\n> +\n> +\ttransaction->state = TRANSACTION_CLOSED;\n> +\treturn 1;\n\nThe normal convention is -1 on error.\n\n[...]\n> @@ -3796,6 +3912,13 @@ int transaction_commit(struct transaction *transaction,\n>  \t}\n>  \tfor (i = 0; i < delnum; i++)\n>  \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n> +\n> +\t/* Commit all reflog updates*/\n> +\tfor_each_string_list_item(item, &transaction->reflog_updates) {\n> +\t\tstruct lock_file *lock = item->util;\n> +\t\tcommit_lock_file_to(lock, git_path(\"logs/%s\", item->string));\n\nNeat.\n\nThis seems like a good starting point.  Other code paths still write\nto reflogs directly --- is there a plan to get them to use this code,\ntoo, in a followup patch (e.g., by making write_ref_sha1() or\nlog_ref_write() use its own small transaction for the reflog update)?\n\nThanks and hope that helps,\nJonathan\n"},{"id":"252916","messageId":"20141203031826.GG6527@google.com","threadId":"38093","inReplyTo":"1417506402-24257-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/4] reflog.c: use a reflog transaction when writing during expire","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T03:18:26Z","receivedAt":"2014-12-03T03:18:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n[...]\n> @@ -290,7 +291,7 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig\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> +\t\tconst char *message, void *cb_data, struct strbuf *err)\n\nThis doesn't match the signature of each_reflog_ent_fn.  Would putting\nerr in the cb_data struct work?\n\nCurious,\nJonathan\n"},{"id":"252968","messageId":"xmqqlhmoip6x.fsf@gitster.dls.corp.google.com","threadId":"38093","inReplyTo":"1417506402-24257-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/4] refs.c: use transactions for the reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T18:05:42Z","receivedAt":"2014-12-03T18:05:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This is the core part of the refs-transactions-reflog series[1],\n> which was in discussion for a bit already.\n\nI think what 3/4 attempts to do is very sensible, but I agree with\nJonathan that it needs a bit more polishing.\n\nAm planning to queue only the first two for today's integration\ncycle on 'pu'.  Thanks.\n"},{"id":"252999","messageId":"20141203222852.GB26810@google.com","threadId":"38093","inReplyTo":"20141203031519.GF6527@google.com","subject":"Re: [PATCH 3/4] refs.c: add a transaction function to append a reflog entry","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-03T22:28:52Z","receivedAt":"2014-12-03T22:28:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 02, 2014 at 07:15:19PM -0800, Jonathan Nieder wrote:\n\nI'll think about rewriting the commit message, so it is easier to digest.\nI'll follow your suggestions except for the following annotations\n\n> \n> > +int transaction_update_reflog(struct transaction *transaction,\n> > +\t\t\t      const char *refname,\n> > +\t\t\t      const unsigned char *new_sha1,\n> > +\t\t\t      const unsigned char *old_sha1,\n> > +\t\t\t      const char *email,\n> > +\t\t\t      unsigned long timestamp, int tz,\n> > +\t\t\t      const char *msg, int flags,\n> > +\t\t\t      struct strbuf *err)\n> \n> This is an intimidating list of arguments.  Would it make sense to\n> pack them into a struct, or to make the list less intimidating\n> some other way (e.g. combining email + timestamp + tz into an\n> ident string)?\n\nIt's true, that is's a huge list. One of the reasons Ronnie gave, \nwas having symmetry in reflog.c:expire_reflog_ent, which has a very\nsimilar signature, the  email + timestamp + tz are passed in as separate\narguments.\n\nexpire_reflog_ent is being called from within reflog.c:expire_reflog\n\n\tif (for_each_reflog_ent(ref, expire_reflog_ent, &cb)) {\n\t\tstatus |= error(\"%s\", err.buf);\n\t\tgoto cleanup;\n\t}\n\nnow if, we dive into the for_each_reflog_ent function, it's essentially \njust calling show_one_reflog_ent on each reflog entry.\nIn there we are trying to separate a reflogs entry into the fields\n(new sha1, old sha1, committer, email, time, timezone, message) by the\nlovely expression\n\n\t/* old SP new SP name <email> SP time TAB msg LF */\n\tif (sb->len < 83 || sb->buf[sb->len - 1] != '\\n' ||\n\t    get_sha1_hex(sb->buf, osha1) || sb->buf[40] != ' ' ||\n\t    get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||\n\t    !(email_end = strchr(sb->buf + 82, '>')) ||\n\t    email_end[1] != ' ' ||\n\t    !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n\t    !message || message[0] != ' ' ||\n\t    (message[1] != '+' && message[1] != '-') ||\n\t    !isdigit(message[2]) || !isdigit(message[3]) ||\n\t    !isdigit(message[4]) || !isdigit(message[5]))\n\t\treturn 0; /* corrupt? */\n\nso I wonder if it actually makes sense to first separate it out into fields,\nand then joining to an ident string again. Is there some kind of struct already,\nwhich groups together identifying information, such as name, email, timestamp + zone,\nwhich could be reused here for passing on the data?\n\nAnyway, this is a first step on transactioning the reflog code, so it is minimally\ninvasive, not changing a lot of business logic. If you look into the next patch,\n(reflog.c: use a reflog transaction when writing during expire)\nyou'll see that it's essentially just a replacement of printfs to calling\nthe function.\n\n-\tif (cb->newlog) {\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+\tif (cb->t) {\n+\t\tif (transaction_update_reflog(cb->t, cb->refname, nsha1, osha1,\n+\t\t\t\t\t      email, timestamp, tz, message, 0,\n+\t\t\t\t\t      cb->err))\n\nand the errorhandling was removed.\nIf we want to change the function signature of transaction_update_reflog, \nI'd do it in a follow up?\n\n\n\n> \n\n> \n> [...]\n> > +\tif (flags & REFLOG_TRUNCATE) {\n> > +\t\tif (lseek(fd, 0, SEEK_SET) < 0 ||\n> > +\t\t\tftruncate(fd, 0)) {\n> > +\t\t\tstrbuf_addf(err, \"Could not truncate reflog: %s. %s\",\n> > +\t\t\t\t    refname, strerror(errno));\n> \n> Odd error message format (using '.' to separate the refname from\n> strerror(errno) is unusual).  Errors normally are supposed to start\n> with a lowercase letter, like\n> \n> \tcannot truncate reflog '%s': %s\n> \n> > +\t\t\tgoto failure;\n> > +\t\t}\n> > +\t}\n> \n> How does this cause the reflog to be populated in the !(flags &\n> REFLOG_TRUNCATE) case?\n> \n> Maybe I am misunderstanding the API.  If I use\n> transaction_update_reflog() and have not updated the reflog\n> previously, isn't this supposed to just append a new entry to the\n> reflog?\n> \n\nHe, that's indeed a good catch. I was investigating the API again myself and\nthe REFLOG_TRUNCATE flag is only set on the very first call to transaction_update_reflog\nand the subsequent calls are rebuilding the reflog by adding the unpruned lines \nagain line by line.\n\nI am wondering if we should actually put that into the same function now.\nAs the new reflog is build on the calls by transaction_update_reflog, it's actually\ndepending on the order strictly as opposed to the refs.\nMaybe we want to have two separate functions\n\ttransaction_update_begin_reflog(...)\n\ttransaction_update_reflog_add_line(...)\n\ninstead? Then a reflog update could look like this:\n\tt=transaction_begin(...)\n\t\n\ttransaction_update_begin_reflog(t, refname)\n\tfor_each_reflog_entry_for_refname(entry, refname):\n\t\tif (want_to_keep(entry):\n\t\t\ttransaction_update_reflog_add_line(entry, refname)\n\n\ttransaction_commit(...)\n\nThe logic of want_to_keep is currently in reflog.c:expire_reflog_ent(...),\nnot sure if there are other places.\n\n\n> [...]\n> > +failure:\n> > +\tstrbuf_release(&buf);\n> > +\t/*\n> > +\t * As we are using the lock file API, any stale files left behind will\n> > +\t * be taken care of, no need to do anything here.\n> > +\t */\n> \n> That's only true if the caller is going to exit instead of proceeding\n> to other work.\n> \n> With current callers, I assume that's true.  So should this comment\n> say something like \"No need to roll back stale lock files because\n> the caller will exit soon\"?  Or should this roll back the lockfile\n> anyway, in case the caller wants to try again?\n\nI am not sure if we ever want transactions be tried again without the user \nexplicitely rerunning the command?\n\nAs we're operating on a lockfile, which was just created by us, and now in \nthe failure command, we're likely to die? Maybe I remove the comment altogether, \nas the wondering reader will look into the API for lock files, when looking \nfor problems here.\n\n> > +\t/* Commit all reflog updates*/\n> > +\tfor_each_string_list_item(item, &transaction->reflog_updates) {\n> > +\t\tstruct lock_file *lock = item->util;\n> > +\t\tcommit_lock_file_to(lock, git_path(\"logs/%s\", item->string));\n> \n> Neat.\n> \n> This seems like a good starting point.  Other code paths still write\n> to reflogs directly --- is there a plan to get them to use this code,\n> too, in a followup patch (e.g., by making write_ref_sha1() or\n> log_ref_write() use its own small transaction for the reflog update)?\n> \n\nThat's the plan, but not part of this patch series yet.\n\n> Thanks and hope that helps,\n> Jonathan\n\nThanks for the comments,\nStefan\n"},{"id":"253001","messageId":"20141203225201.GH6527@google.com","threadId":"38093","inReplyTo":"20141203222852.GB26810@google.com","subject":"Re: [PATCH 3/4] refs.c: add a transaction function to append a reflog entry","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T22:52:01Z","receivedAt":"2014-12-03T22:52:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Tue, Dec 02, 2014 at 07:15:19PM -0800, Jonathan Nieder wrote:\n>> Stefan Beller wrote:\n\n>>> +int transaction_update_reflog(struct transaction *transaction,\n>>> +\t\t\t      const char *refname,\n>>> +\t\t\t      const unsigned char *new_sha1,\n>>> +\t\t\t      const unsigned char *old_sha1,\n>>> +\t\t\t      const char *email,\n>>> +\t\t\t      unsigned long timestamp, int tz,\n>>> +\t\t\t      const char *msg, int flags,\n>>> +\t\t\t      struct strbuf *err)\n>>\n>> This is an intimidating list of arguments.  Would it make sense to\n>> pack them into a struct, or to make the list less intimidating\n>> some other way (e.g. combining email + timestamp + tz into an\n>> ident string)?\n>\n> It's true, that is's a huge list.\n[...]\n> If we want to change the function signature of transaction_update_reflog,\n> I'd do it in a follow up?\n\nThe important change here is that it is moving from a static to a public\nfunction.  In static functions, having a signature that is hard to work\nwith is okay because the possibility of confusion is restricted to\ncallers coordinating between each other in the same file.  Now that the\nfunction is becoming public, it is time to think about the API.\n\n[...]\n>> Maybe I am misunderstanding the API.  If I use\n>> transaction_update_reflog() and have not updated the reflog\n>> previously, isn't this supposed to just append a new entry to the\n>> reflog?\n>\n> He, that's indeed a good catch. I was investigating the API again myself and\n> the REFLOG_TRUNCATE flag is only set on the very first call to transaction_update_reflog\n> and the subsequent calls are rebuilding the reflog by adding the unpruned lines\n> again line by line.\n\nTaking a step back, there are two ways this API can be used:\n\n * commands like \"git reflog expire\" that filter the reflog want to\n   create an entirely new reflog and copy entries one by one into it\n\n * commands like \"git update-ref\" that append to the reflog want to\n   have the reflog as-is, plus some new entries added one-by-one\n\nFor that second use, the only operation needed is\n\n\ttransaction_update_reflog\n\nwhich would add a line to the reflog.  The first use can have that plus\n\n\ttransaction_truncate_reflog\n\nwhich would clear the reflog.\n\nIf I understood the API proposed by Ronnie, it combined those two\noperations into a single function, transaction_truncate_reflog(flag).\nThe flag could be REFLOG_TRUNCATE to mean \"truncate the reflog first\",\nanalagous to the O_TRUNC flag in open(2).\n\nEither API (the two-function version with no flag or one-function\nversion with flag) is basically equivalent --- they can be implemented\nin terms of each other.  I slightly prefer the two-function API since\nit makes the sequence of operations more obvious (\"first truncate,\nthen append this, then append that, then...\").\n\n[...]\n>>> +failure:\n>>> +\tstrbuf_release(&buf);\n>>> +\t/*\n>>> +\t * As we are using the lock file API, any stale files left behind will\n>>> +\t * be taken care of, no need to do anything here.\n>>> +\t */\n>>\n>> That's only true if the caller is going to exit instead of proceeding\n>> to other work.\n>>\n>> With current callers, I assume that's true.  So should this comment\n>> say something like \"No need to roll back stale lock files because\n>> the caller will exit soon\"?  Or should this roll back the lockfile\n>> anyway, in case the caller wants to try again?\n>\n> I am not sure if we ever want transactions be tried again without the user\n> explicitely rerunning the command?\n>\n> As we're operating on a lockfile, which was just created by us, and now in\n> the failure command, we're likely to die? Maybe I remove the comment altogether,\n> as the wondering reader will look into the API for lock files, when looking\n> for problems here.\n\nIt is tempting to roll back the lock file and keeping the transaction in OPEN\nstate would be best.  That way, the caller can keep going if they want\nto --- it is as if the failing call never happened.\n\n... except that rolling back the lockfile would mean rolling back previous\nupdates to the same reflog that had succeeded, too.\n\nSo I think closing the transaction like you do is the right thing to\ndo.  Rolling back the lockfile wouldn't be useful since there are\nstill other lockfiles to take care of from previous updates to other\nreflogs that haven't been rolled back.  The comment should say that\nthe caller is going to exit.\n\nThanks,\nJonathan\n"}]}