{"thread":{"id":"39747","subject":"[PATCH v6 1/7] refs.c: add err arguments to reflog functions","startedAt":"2015-06-29T20:17:49Z","lastAt":"2015-07-09T06:47:51Z","messageCount":31,"participants":["David Turner","Junio C Hamano","Eric Sunshine","Michael Haggerty"],"isPatch":true,"patchVersion":6,"patchTotal":7},"messages":[{"id":"265171","messageId":"1435609076-8592-1-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":null,"subject":"[PATCH v6 0/7] refs backend preamble","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:49Z","receivedAt":"2015-06-29T20:17:49Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Minor formatting fixes from Junio Hamano.\n"},{"id":"265170","messageId":"1435609076-8592-2-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v6 1/7] refs.c: add err arguments to reflog functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:50Z","receivedAt":"2015-06-29T20:17:50Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Add an err argument to log_ref_setup that can explain the reason\nfor a failure. This then eliminates the need to manage errno through\nthis function since we can just add strerror(errno) to the err string\nwhen meaningful. No callers relied on errno from this function for\nanything else than the error message.\n\nAlso add err arguments to private functions write_ref_to_lockfile,\nlog_ref_write_1, commit_ref_update. This again eliminates the need to\nmanage errno in these functions.\n\nUpdate of a patch by Ronnie Sahlberg.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n builtin/checkout.c |   8 ++--\n refs.c             | 111 ++++++++++++++++++++++++++++-------------------------\n refs.h             |   4 +-\n 3 files changed, 66 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex c018ab3..93f63d3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -624,16 +624,18 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tstruct strbuf log_file = STRBUF_INIT;\n \t\t\t\tint ret;\n \t\t\t\tconst char *ref_name;\n+\t\t\t\tstruct strbuf err = STRBUF_INIT;\n \n \t\t\t\tref_name = mkpath(\"refs/heads/%s\", opts->new_orphan_branch);\n \t\t\t\ttemp = log_all_ref_updates;\n \t\t\t\tlog_all_ref_updates = 1;\n-\t\t\t\tret = log_ref_setup(ref_name, &log_file);\n+\t\t\t\tret = log_ref_setup(ref_name, &log_file, &err);\n \t\t\t\tlog_all_ref_updates = temp;\n \t\t\t\tstrbuf_release(&log_file);\n \t\t\t\tif (ret) {\n-\t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s'\\n\"),\n-\t\t\t\t\t    opts->new_orphan_branch);\n+\t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s'. %s\\n\"),\n+\t\t\t\t\t\topts->new_orphan_branch, err.buf);\n+\t\t\t\t\tstrbuf_release(&err);\n \t\t\t\t\treturn;\n \t\t\t\t}\n \t\t\t}\ndiff --git a/refs.c b/refs.c\nindex fb568d7..c97ca02 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2975,9 +2975,11 @@ static int rename_ref_available(const char *oldname, const char *newname)\n \treturn ret;\n }\n \n-static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1);\n+static int write_ref_to_lockfile(struct ref_lock *lock,\n+\t\t\t\t const unsigned char *sha1, struct strbuf *err);\n static int commit_ref_update(struct ref_lock *lock,\n-\t\t\t     const unsigned char *sha1, const char *logmsg);\n+\t\t\t     const unsigned char *sha1, const char *logmsg,\n+\t\t\t     struct strbuf *err);\n \n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n@@ -3038,9 +3040,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t}\n \thashcpy(lock->old_oid.hash, orig_sha1);\n \n-\tif (write_ref_to_lockfile(lock, orig_sha1) ||\n-\t    commit_ref_update(lock, orig_sha1, logmsg)) {\n-\t\terror(\"unable to write current sha1 into %s\", newrefname);\n+\tif (write_ref_to_lockfile(lock, orig_sha1, &err) ||\n+\t    commit_ref_update(lock, orig_sha1, logmsg, &err)) {\n+\t\terror(\"unable to write current sha1 into %s: %s\", newrefname, err.buf);\n+\t\tstrbuf_release(&err);\n \t\tgoto rollback;\n \t}\n \n@@ -3056,9 +3059,11 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = 0;\n-\tif (write_ref_to_lockfile(lock, orig_sha1) ||\n-\t    commit_ref_update(lock, orig_sha1, NULL))\n-\t\terror(\"unable to write current sha1 into %s\", oldrefname);\n+\tif (write_ref_to_lockfile(lock, orig_sha1, &err) ||\n+\t    commit_ref_update(lock, orig_sha1, NULL, &err)) {\n+\t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n+\t\tstrbuf_release(&err);\n+\t}\n \tlog_all_ref_updates = flag;\n \n  rollbacklog:\n@@ -3113,8 +3118,8 @@ static int copy_msg(char *buf, const char *msg)\n \treturn cp - buf;\n }\n \n-/* This function must set a meaningful errno on failure */\n-int log_ref_setup(const char *refname, struct strbuf *sb_logfile)\n+/* This function will fill in *err and return -1 on failure */\n+int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)\n {\n \tint logfd, oflags = O_APPEND | O_WRONLY;\n \tchar *logfile;\n@@ -3129,9 +3134,8 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile)\n \t     starts_with(refname, \"refs/notes/\") ||\n \t     !strcmp(refname, \"HEAD\"))) {\n \t\tif (safe_create_leading_directories(logfile) < 0) {\n-\t\t\tint save_errno = errno;\n-\t\t\terror(\"unable to create directory for %s\", logfile);\n-\t\t\terrno = save_errno;\n+\t\t\tstrbuf_addf(err, \"unable to create directory for %s. \"\n+\t\t\t\t    \"%s\", logfile, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n \t\toflags |= O_CREAT;\n@@ -3144,20 +3148,16 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile)\n \n \t\tif (errno == EISDIR) {\n \t\t\tif (remove_empty_directories(logfile)) {\n-\t\t\t\tint save_errno = errno;\n-\t\t\t\terror(\"There are still logs under '%s'\",\n-\t\t\t\t      logfile);\n-\t\t\t\terrno = save_errno;\n+\t\t\t\tstrbuf_addf(err, \"There are still logs under \"\n+\t\t\t\t\t    \"'%s'\", logfile);\n \t\t\t\treturn -1;\n \t\t\t}\n \t\t\tlogfd = open(logfile, oflags, 0666);\n \t\t}\n \n \t\tif (logfd < 0) {\n-\t\t\tint save_errno = errno;\n-\t\t\terror(\"Unable to append to %s: %s\", logfile,\n-\t\t\t      strerror(errno));\n-\t\t\terrno = save_errno;\n+\t\t\tstrbuf_addf(err, \"Unable to append to %s: %s\",\n+\t\t\t\t    logfile, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n \t}\n@@ -3195,7 +3195,7 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n \n static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \t\t\t   const unsigned char *new_sha1, const char *msg,\n-\t\t\t   struct strbuf *sb_log_file)\n+\t\t\t   struct strbuf *sb_log_file, struct strbuf *err)\n {\n \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \tchar *log_file;\n@@ -3203,7 +3203,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n-\tresult = log_ref_setup(refname, sb_log_file);\n+\tresult = log_ref_setup(refname, sb_log_file, err);\n+\n \tif (result)\n \t\treturn result;\n \tlog_file = sb_log_file->buf;\n@@ -3216,26 +3217,25 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\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-\t\terrno = save_errno;\n+\t\tstrbuf_addf(err, \"Unable to append to %s. %s\", log_file,\n+\t\t\t    strerror(errno));\n \t\treturn -1;\n \t}\n \tif (close(logfd)) {\n-\t\tint save_errno = errno;\n-\t\terror(\"Unable to append to %s\", log_file);\n-\t\terrno = save_errno;\n+\t\tstrbuf_addf(err, \"Unable to append to %s. %s\", log_file,\n+\t\t\t    strerror(errno));\n \t\treturn -1;\n \t}\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+\t\t\t const unsigned char *new_sha1, const char *msg,\n+\t\t\t struct strbuf *err)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb);\n+\tint ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, err);\n \tstrbuf_release(&sb);\n \treturn ret;\n }\n@@ -3247,25 +3247,28 @@ int is_branch(const char *refname)\n \n /*\n  * Write sha1 into the open lockfile, then close the lockfile. On\n- * errors, rollback the lockfile and set errno to reflect the problem.\n+ * errors, rollback the lockfile, fill in *err and\n+ * return -1.\n  */\n static int write_ref_to_lockfile(struct ref_lock *lock,\n-\t\t\t\t const unsigned char *sha1)\n+\t\t\t\t const unsigned char *sha1, struct strbuf *err)\n {\n \tstatic char term = '\\n';\n \tstruct object *o;\n \n \to = parse_object(sha1);\n \tif (!o) {\n-\t\terror(\"Trying to write ref %s with nonexistent object %s\",\n-\t\t\tlock->ref_name, sha1_to_hex(sha1));\n+\t\tstrbuf_addf(err,\n+\t\t\t    \"Trying to write ref %s with nonexistent object %s\",\n+\t\t\t    lock->ref_name, sha1_to_hex(sha1));\n \t\tunlock_ref(lock);\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n \tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n-\t\terror(\"Trying to write non-commit object %s to branch %s\",\n-\t\t\tsha1_to_hex(sha1), lock->ref_name);\n+\t\tstrbuf_addf(err,\n+\t\t\t    \"Trying to write non-commit object %s to branch %s\",\n+\t\t\t    sha1_to_hex(sha1), lock->ref_name);\n \t\tunlock_ref(lock);\n \t\terrno = EINVAL;\n \t\treturn -1;\n@@ -3273,10 +3276,9 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \tif (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||\n \t    write_in_full(lock->lk->fd, &term, 1) != 1 ||\n \t    close_ref(lock) < 0) {\n-\t\tint save_errno = errno;\n-\t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n+\t\tstrbuf_addf(err,\n+\t\t\t    \"Couldn't write %s\", lock->lk->filename.buf);\n \t\tunlock_ref(lock);\n-\t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \treturn 0;\n@@ -3288,12 +3290,15 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n  * necessary, using the specified lockmsg (which can be NULL).\n  */\n static int commit_ref_update(struct ref_lock *lock,\n-\t\t\t     const unsigned char *sha1, const char *logmsg)\n+\t\t\t     const unsigned char *sha1, const char *logmsg,\n+\t\t\t     struct strbuf *err)\n {\n \tclear_loose_ref_cache(&ref_cache);\n-\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg) < 0 ||\n+\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0 ||\n \t    (strcmp(lock->ref_name, lock->orig_ref_name) &&\n-\t     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg) < 0)) {\n+\t     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {\n+\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n+\t\t\t    lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n@@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\t\t\t\t      head_sha1, &head_flag);\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t    !strcmp(head_ref, lock->ref_name))\n-\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg);\n+\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg,\n+\t\t\t\t      err);\n \t}\n \tif (commit_ref(lock)) {\n \t\terror(\"Couldn't set %s\", lock->ref_name);\n@@ -3336,6 +3342,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n \tint fd, len, written;\n \tchar *git_HEAD = git_pathdup(\"%s\", ref_target);\n \tunsigned char old_sha1[20], new_sha1[20];\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (logmsg && read_ref(ref_target, old_sha1))\n \t\thashclr(old_sha1);\n@@ -3384,8 +3391,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n #ifndef NO_SYMLINK_HEAD\n \tdone:\n #endif\n-\tif (logmsg && !read_ref(refs_heads_master, new_sha1))\n-\t\tlog_ref_write(ref_target, old_sha1, new_sha1, logmsg);\n+\tif (logmsg && !read_ref(refs_heads_master, new_sha1) &&\n+\t\tlog_ref_write(ref_target, old_sha1, new_sha1, logmsg, &err)) {\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t}\n \n \tfree(git_HEAD);\n \treturn 0;\n@@ -4021,14 +4031,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t * value, so we don't need to write it.\n \t\t\t\t */\n \t\t\t} else if (write_ref_to_lockfile(update->lock,\n-\t\t\t\t\t\t\t update->new_sha1)) {\n+\t\t\t\t\t\t\t update->new_sha1,\n+\t\t\t\t\t\t\t err)) {\n \t\t\t\t/*\n \t\t\t\t * The lock was freed upon failure of\n \t\t\t\t * write_ref_to_lockfile():\n \t\t\t\t */\n \t\t\t\tupdate->lock = NULL;\n-\t\t\t\tstrbuf_addf(err, \"cannot update the ref '%s'.\",\n-\t\t\t\t\t    update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t} else {\n@@ -4054,11 +4063,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (update->flags & REF_NEEDS_COMMIT) {\n \t\t\tif (commit_ref_update(update->lock,\n-\t\t\t\t\t      update->new_sha1, update->msg)) {\n+\t\t\t\t\t      update->new_sha1, update->msg, err)) {\n \t\t\t\t/* freed by commit_ref_update(): */\n \t\t\t\tupdate->lock = NULL;\n-\t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n-\t\t\t\t\t    update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t} else {\ndiff --git a/refs.h b/refs.h\nindex e82fca5..debdefc 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -226,9 +226,9 @@ int pack_refs(unsigned int flags);\n #define REF_NODEREF\t0x01\n \n /*\n- * Setup reflog before using. Set errno to something meaningful on failure.\n+ * Setup reflog before using. Fill in err and return -1 on failure.\n  */\n-int log_ref_setup(const char *refname, struct strbuf *logfile);\n+int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err);\n \n /** Reads log for the value of ref during at_time. **/\n extern int read_ref_at(const char *refname, unsigned int flags,\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"265173","messageId":"1435609076-8592-3-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v6 2/7] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:51Z","receivedAt":"2015-06-29T20:17:51Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Instead of directly writing to and reading from files in\n$GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD\nand REVERT_HEAD.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n branch.c                         |  4 ++--\n builtin/commit.c                 |  6 +++---\n builtin/merge.c                  |  2 +-\n contrib/completion/git-prompt.sh |  4 ++--\n git-gui/lib/commit.tcl           |  2 +-\n sequencer.c                      | 39 ++++++++++++++++++++-------------------\n t/t7509-commit.sh                |  4 ++--\n wt-status.c                      |  6 ++----\n 8 files changed, 33 insertions(+), 34 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex b002435..ec598aa 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -302,8 +302,8 @@ void create_branch(const char *head,\n \n void remove_branch_state(void)\n {\n-\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\tunlink(git_path(\"REVERT_HEAD\"));\n+\tdelete_ref(\"CHERRY_PICK_HEAD\", NULL, REF_NODEREF);\n+\tdelete_ref(\"REVERT_HEAD\", NULL, REF_NODEREF);\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_RR\"));\n \tunlink(git_path(\"MERGE_MSG\"));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b5b1158..53c7e90 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -168,7 +168,7 @@ static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path(\"MERGE_HEAD\")))\n \t\twhence = FROM_MERGE;\n-\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n+\telse if (ref_exists(\"CHERRY_PICK_HEAD\")) {\n \t\twhence = FROM_CHERRY_PICK;\n \t\tif (file_exists(git_path(SEQ_DIR)))\n \t\t\tsequencer_in_use = 1;\n@@ -1777,8 +1777,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t}\n \tref_transaction_free(transaction);\n \n-\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\tunlink(git_path(\"REVERT_HEAD\"));\n+\tdelete_ref(\"CHERRY_PICK_HEAD\", NULL, REF_NODEREF);\n+\tdelete_ref(\"REVERT_HEAD\", NULL, REF_NODEREF);\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n \tunlink(git_path(\"MERGE_MODE\"));\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 46aacd6..3e2ae2f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1206,7 +1206,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"You have not concluded your merge (MERGE_HEAD exists).\"));\n \t}\n-\tif (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n+\tif (ref_exists(\"CHERRY_PICK_HEAD\")) {\n \t\tif (advice_resolve_conflict)\n \t\t\tdie(_(\"You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\\n\"\n \t\t\t    \"Please, commit your changes before you merge.\"));\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 366f0bc..e2c5583 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -415,9 +415,9 @@ __git_ps1 ()\n \t\t\tfi\n \t\telif [ -f \"$g/MERGE_HEAD\" ]; then\n \t\t\tr=\"|MERGING\"\n-\t\telif [ -f \"$g/CHERRY_PICK_HEAD\" ]; then\n+\t\telif git rev-parse --quiet --verify \"CHERRY_PICK_HEAD\" >/dev/null; then\n \t\t\tr=\"|CHERRY-PICKING\"\n-\t\telif [ -f \"$g/REVERT_HEAD\" ]; then\n+\t\telif git rev-parse --quiet --verify \"REVERT_HEAD\" >/dev/null; then\n \t\t\tr=\"|REVERTING\"\n \t\telif [ -f \"$g/BISECT_LOG\" ]; then\n \t\t\tr=\"|BISECTING\"\ndiff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl\nindex 864b687..2b08b13 100644\n--- a/git-gui/lib/commit.tcl\n+++ b/git-gui/lib/commit.tcl\n@@ -409,7 +409,7 @@ A rescan will be automatically started now.\n \tcatch {file delete [gitdir MERGE_MSG]}\n \tcatch {file delete [gitdir SQUASH_MSG]}\n \tcatch {file delete [gitdir GITGUI_MSG]}\n-\tcatch {file delete [gitdir CHERRY_PICK_HEAD]}\n+\tcatch {git update-ref -d --no-deref CHERRY_PICK_HEAD}\n \n \t# -- Let rerere do its thing.\n \t#\ndiff --git a/sequencer.c b/sequencer.c\nindex f8421a8..44c43e5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -158,21 +158,22 @@ static void free_message(struct commit *commit, struct commit_message *msg)\n \tunuse_commit_buffer(commit, msg->message);\n }\n \n-static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)\n+static void write_cherry_pick_head(struct commit *commit, const char *ref)\n {\n-\tconst char *filename;\n-\tint fd;\n-\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tvoid *transaction;\n \n-\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction)\n+\t\tdie(_(\"Could not create transaction: %s\"), err.buf);\n \n-\tfilename = git_path(\"%s\", pseudoref);\n-\tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n-\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n-\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n-\tstrbuf_release(&buf);\n+\tif (ref_transaction_update(transaction, ref, commit->object.sha1,\n+\t\t\t\t   NULL, REF_NODEREF, NULL,\n+\t\t\t\t   &err))\n+\t\tdie(_(\"Could not write ref %s: %s\"), ref, err.buf);\n+\n+\tif (ref_transaction_commit(transaction, &err))\n+\t\tdie(_(\"Could not commit ref write %s: %s\"), ref, err.buf);\n }\n \n static void print_advice(int show_hint, struct replay_opts *opts)\n@@ -186,7 +187,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n \t\t * (typically rebase --interactive) wants to take care\n \t\t * of the commit itself so remove CHERRY_PICK_HEAD\n \t\t */\n-\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n+\t\tdelete_ref(\"CHERRY_PICK_HEAD\", NULL, REF_NODEREF);\n \t\treturn;\n \t}\n \n@@ -878,8 +879,8 @@ static int rollback_single_pick(void)\n {\n \tunsigned char head_sha1[20];\n \n-\tif (!file_exists(git_path(\"CHERRY_PICK_HEAD\")) &&\n-\t    !file_exists(git_path(\"REVERT_HEAD\")))\n+\tif (!ref_exists(\"CHERRY_PICK_HEAD\") &&\n+\t    !ref_exists(\"REVERT_HEAD\"))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n \tif (read_ref_full(\"HEAD\", 0, head_sha1, NULL))\n \t\treturn error(_(\"cannot resolve HEAD\"));\n@@ -1014,8 +1015,8 @@ static int continue_single_pick(void)\n {\n \tconst char *argv[] = { \"commit\", NULL };\n \n-\tif (!file_exists(git_path(\"CHERRY_PICK_HEAD\")) &&\n-\t    !file_exists(git_path(\"REVERT_HEAD\")))\n+\tif (!ref_exists(\"CHERRY_PICK_HEAD\") &&\n+\t    !ref_exists(\"REVERT_HEAD\"))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n \treturn run_command_v_opt(argv, RUN_GIT_CMD);\n }\n@@ -1030,8 +1031,8 @@ static int sequencer_continue(struct replay_opts *opts)\n \tread_populate_todo(&todo_list, opts);\n \n \t/* Verify that the conflict has been resolved */\n-\tif (file_exists(git_path(\"CHERRY_PICK_HEAD\")) ||\n-\t    file_exists(git_path(\"REVERT_HEAD\"))) {\n+\tif (ref_exists(\"CHERRY_PICK_HEAD\") ||\n+\t    ref_exists(\"REVERT_HEAD\")) {\n \t\tint ret = continue_single_pick();\n \t\tif (ret)\n \t\t\treturn ret;\ndiff --git a/t/t7509-commit.sh b/t/t7509-commit.sh\nindex 9ac7940..f7fd62c 100755\n--- a/t/t7509-commit.sh\n+++ b/t/t7509-commit.sh\n@@ -163,7 +163,7 @@ test_expect_success 'commit respects CHERRY_PICK_HEAD and MERGE_MSG' '\n \ttest_tick &&\n \tgit commit -am \"cherry-pick 1\" --author=\"Cherry <cherry@pick.er>\" &&\n \tgit tag cherry-pick-head &&\n-\tgit rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD &&\n+\tgit update-ref CHERRY_PICK_HEAD $(git rev-parse cherry-pick-head) &&\n \techo \"This is a MERGE_MSG\" >.git/MERGE_MSG &&\n \techo \"cherry-pick 1b\" >>foo &&\n \ttest_tick &&\n@@ -178,7 +178,7 @@ test_expect_success 'commit respects CHERRY_PICK_HEAD and MERGE_MSG' '\n '\n \n test_expect_success '--reset-author with CHERRY_PICK_HEAD' '\n-\tgit rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD &&\n+\tgit update-ref CHERRY_PICK_HEAD $(git rev-parse cherry-pick-head) &&\n \techo \"cherry-pick 2\" >>foo &&\n \ttest_tick &&\n \tgit commit -am \"cherry-pick 2\" --reset-author &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 9c686e6..661c1fb 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1336,8 +1336,7 @@ void wt_status_get_state(struct wt_status_state *state,\n \t\t\tstate->rebase_in_progress = 1;\n \t\tstate->branch = read_and_strip_branch(\"rebase-merge/head-name\");\n \t\tstate->onto = read_and_strip_branch(\"rebase-merge/onto\");\n-\t} else if (!stat(git_path(\"CHERRY_PICK_HEAD\"), &st) &&\n-\t\t\t!get_sha1(\"CHERRY_PICK_HEAD\", sha1)) {\n+\t} else if (!read_ref(\"CHERRY_PICK_HEAD\", sha1)) {\n \t\tstate->cherry_pick_in_progress = 1;\n \t\thashcpy(state->cherry_pick_head_sha1, sha1);\n \t}\n@@ -1345,8 +1344,7 @@ void wt_status_get_state(struct wt_status_state *state,\n \t\tstate->bisect_in_progress = 1;\n \t\tstate->branch = read_and_strip_branch(\"BISECT_START\");\n \t}\n-\tif (!stat(git_path(\"REVERT_HEAD\"), &st) &&\n-\t    !get_sha1(\"REVERT_HEAD\", sha1)) {\n+\tif (!read_ref(\"REVERT_HEAD\", sha1)) {\n \t\tstate->revert_in_progress = 1;\n \t\thashcpy(state->revert_head_sha1, sha1);\n \t}\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"265174","messageId":"1435609076-8592-4-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v6 3/7] bisect: treat BISECT_HEAD as a ref","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:52Z","receivedAt":"2015-06-29T20:17:52Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Instead of directly writing to and reading from files in\n$GIT_DIR, use ref API to interact with BISECT_HEAD.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n git-bisect.sh | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex ae3fec2..dddcc89 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -35,7 +35,7 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n \n bisect_head()\n {\n-\tif test -f \"$GIT_DIR/BISECT_HEAD\"\n+\tif bisect_head_exists\n \tthen\n \t\techo BISECT_HEAD\n \telse\n@@ -209,6 +209,10 @@ check_expected_revs() {\n \tdone\n }\n \n+bisect_head_exists() {\n+    git rev-parse --quiet --verify \"BISECT_HEAD\" >/dev/null\n+}\n+\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -310,7 +314,7 @@ bisect_next() {\n \tbisect_next_check good\n \n \t# Perform all bisection computation, display and checkout\n-\tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n+\tgit bisect--helper --next-all $(bisect_head_exists && echo --no-checkout)\n \tres=$?\n \n \t# Check if we should exit because bisection is finished\n@@ -377,7 +381,7 @@ bisect_reset() {\n \t\tusage ;;\n \tesac\n \n-\tif ! test -f \"$GIT_DIR/BISECT_HEAD\" && ! git checkout \"$branch\" --\n+\tif ! bisect_head_exists && ! git checkout \"$branch\" --\n \tthen\n \t\tdie \"$(eval_gettext \"Could not check out original HEAD '\\$branch'.\n Try 'git bisect reset <commit>'.\")\"\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"265172","messageId":"1435609076-8592-5-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v6 4/7] refs: Break out check for reflog autocreation","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:53Z","receivedAt":"2015-06-29T20:17:53Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"This is just for clarity.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n refs.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c97ca02..30e81ba 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3118,6 +3118,16 @@ static int copy_msg(char *buf, const char *msg)\n \treturn cp - buf;\n }\n \n+static int should_autocreate_reflog(const char *refname)\n+{\n+\tif (!log_all_ref_updates)\n+\t\treturn 0;\n+\treturn starts_with(refname, \"refs/heads/\") ||\n+\t\tstarts_with(refname, \"refs/remotes/\") ||\n+\t\tstarts_with(refname, \"refs/notes/\") ||\n+\t\t!strcmp(refname, \"HEAD\");\n+}\n+\n /* This function will fill in *err and return -1 on failure */\n int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)\n {\n@@ -3128,11 +3138,7 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf\n \tlogfile = sb_logfile->buf;\n \t/* make sure the rest of the function can't change \"logfile\" */\n \tsb_logfile = NULL;\n-\tif (log_all_ref_updates &&\n-\t    (starts_with(refname, \"refs/heads/\") ||\n-\t     starts_with(refname, \"refs/remotes/\") ||\n-\t     starts_with(refname, \"refs/notes/\") ||\n-\t     !strcmp(refname, \"HEAD\"))) {\n+\tif (should_autocreate_reflog(refname)) {\n \t\tif (safe_create_leading_directories(logfile) < 0) {\n \t\t\tstrbuf_addf(err, \"unable to create directory for %s. \"\n \t\t\t\t    \"%s\", logfile, strerror(errno));\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"265175","messageId":"1435609076-8592-6-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v6 5/7] refs: new public ref function: safe_create_reflog","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:54Z","receivedAt":"2015-06-29T20:17:54Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"The safe_create_reflog function creates a reflog, if it does not\nalready exist.\n\nThe log_ref_setup function becomes private and gains a force_create\nparameter to force the creation of a reflog even if log_all_ref_updates\nis false or the refname is not one of the special refnames.\n\nThe new parameter also reduces the need to store, modify, and restore\nthe log_all_ref_updates global before reflog creation.\n\nIn a moment, we will use this to add reflog creation commands to\ngit-reflog.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n builtin/checkout.c | 10 +---------\n refs.c             | 25 +++++++++++++++++++++----\n refs.h             |  2 +-\n 3 files changed, 23 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 93f63d3..9f68399 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -620,19 +620,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \tif (opts->new_branch) {\n \t\tif (opts->new_orphan_branch) {\n \t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n-\t\t\t\tint temp;\n-\t\t\t\tstruct strbuf log_file = STRBUF_INIT;\n-\t\t\t\tint ret;\n \t\t\t\tconst char *ref_name;\n \t\t\t\tstruct strbuf err = STRBUF_INIT;\n \n \t\t\t\tref_name = mkpath(\"refs/heads/%s\", opts->new_orphan_branch);\n-\t\t\t\ttemp = log_all_ref_updates;\n-\t\t\t\tlog_all_ref_updates = 1;\n-\t\t\t\tret = log_ref_setup(ref_name, &log_file, &err);\n-\t\t\t\tlog_all_ref_updates = temp;\n-\t\t\t\tstrbuf_release(&log_file);\n-\t\t\t\tif (ret) {\n+\t\t\t\tif (safe_create_reflog(ref_name, &err, 1)) {\n \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s'. %s\\n\"),\n \t\t\t\t\t\topts->new_orphan_branch, err.buf);\n \t\t\t\t\tstrbuf_release(&err);\ndiff --git a/refs.c b/refs.c\nindex 30e81ba..1e53ef0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3128,8 +3128,14 @@ static int should_autocreate_reflog(const char *refname)\n \t\t!strcmp(refname, \"HEAD\");\n }\n \n-/* This function will fill in *err and return -1 on failure */\n-int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)\n+/*\n+ * This function creates a reflog for a ref.  If force_create = 0, the\n+ * reflog will only be created for certain refs (those for which\n+ * should_autocreate_reflog returns non-zero.  Otherwise, it will be\n+ * created regardless of the ref name.  This function will fill in *err\n+ * and return -1 on failure\n+ */\n+static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)\n {\n \tint logfd, oflags = O_APPEND | O_WRONLY;\n \tchar *logfile;\n@@ -3138,7 +3144,7 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf\n \tlogfile = sb_logfile->buf;\n \t/* make sure the rest of the function can't change \"logfile\" */\n \tsb_logfile = NULL;\n-\tif (should_autocreate_reflog(refname)) {\n+\tif (force_create || should_autocreate_reflog(refname)) {\n \t\tif (safe_create_leading_directories(logfile) < 0) {\n \t\t\tstrbuf_addf(err, \"unable to create directory for %s. \"\n \t\t\t\t    \"%s\", logfile, strerror(errno));\n@@ -3173,6 +3179,17 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf\n \treturn 0;\n }\n \n+\n+int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)\n+{\n+\tint ret;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tret = log_ref_setup(refname, &sb, err, force_create);\n+\tstrbuf_release(&sb);\n+\treturn ret;\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@@ -3209,7 +3226,7 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n-\tresult = log_ref_setup(refname, sb_log_file, err);\n+\tresult = log_ref_setup(refname, sb_log_file, err, 0);\n \n \tif (result)\n \t\treturn result;\ndiff --git a/refs.h b/refs.h\nindex debdefc..3b90e16 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -228,7 +228,7 @@ int pack_refs(unsigned int flags);\n /*\n  * Setup reflog before using. Fill in err and return -1 on failure.\n  */\n-int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err);\n+int safe_create_reflog(const char *refname, struct strbuf *err, int force_create);\n \n /** Reads log for the value of ref during at_time. **/\n extern int read_ref_at(const char *refname, unsigned int flags,\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"265176","messageId":"1435609076-8592-7-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:55Z","receivedAt":"2015-06-29T20:17:55Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"These are necessary because alternate ref backends might store reflogs\nsomewhere other than .git/logs.  Code that now directly manipulates\n.git/logs should instead go through git-reflog.\n\nIn a moment, we will use these functions to make git stash work with\nalternate ref backends.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n Documentation/git-reflog.txt | 10 ++++++\n builtin/reflog.c             | 75 +++++++++++++++++++++++++++++++++++++++++++-\n t/t1411-reflog-show.sh       | 12 +++++++\n 3 files changed, 96 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex 5e7908e..2bf8aa6 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -23,6 +23,8 @@ depending on the subcommand:\n \t[--dry-run] [--verbose] [--all | <refs>...]\n 'git reflog delete' [--rewrite] [--updateref]\n \t[--dry-run] [--verbose] ref@\\{specifier\\}...\n+'git reflog create' <refs>...\n+'git reflog exists' <ref>\n \n Reference logs, or \"reflogs\", record when the tips of branches and\n other references were updated in the local repository. Reflogs are\n@@ -52,6 +54,14 @@ argument must be an _exact_ entry (e.g. \"`git reflog delete\n master@{2}`\"). This subcommand is also typically not used directly by\n end users.\n \n+The \"create\" subcommand creates a reflog for one or more refs. Most\n+refs (those under refs/heads, refs/remotes, and refs/tags) will\n+automatically have reflogs created. Other refs will not. This command\n+allows manual ref creation.\n+\n+The \"exists\" subcommand checks whether a ref has a reflog.  It exists\n+with zero status if the reflog exists, and non-zero status if it does\n+not.\n \n OPTIONS\n -------\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex c2eb8ff..3080865 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =\n \"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...\";\n static const char reflog_delete_usage[] =\n \"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...\";\n+static const char reflog_create_usage[] =\n+\"git reflog create <refs>...\";\n+static const char reflog_exists_usage[] =\n+\"git reflog exists <ref>\";\n \n static unsigned long default_reflog_expire;\n static unsigned long default_reflog_expire_unreachable;\n@@ -699,12 +703,75 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \treturn status;\n }\n \n+static int cmd_reflog_create(int argc, const char **argv, const char *prefix)\n+{\n+\tint i, status = 0, start = 0;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\tfor (i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tif (!strcmp(arg, \"--\")) {\n+\t\t\ti++;\n+\t\t\tbreak;\n+\t\t}\n+\t\telse if (arg[0] == '-')\n+\t\t\tusage(reflog_create_usage);\n+\t\telse\n+\t\t\tbreak;\n+\t}\n+\n+\tstart = i;\n+\n+\tif (argc - start < 1)\n+\t\treturn error(\"Nothing to create?\");\n+\n+\tfor (i = start; i < argc; i++) {\n+\t\tif (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))\n+\t\t\tdie(\"invalid ref format: %s\", argv[i]);\n+\t}\n+\tfor (i = start; i < argc; i++) {\n+\t\tif (safe_create_reflog(argv[i], &err, 1)) {\n+\t\t\terror(\"could not create reflog %s: %s\", argv[i],\n+\t\t\t      err.buf);\n+\t\t\tstatus = 1;\n+\t\t\tstrbuf_release(&err);\n+\t\t}\n+\t}\n+\treturn status;\n+}\n+\n+static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)\n+{\n+\tint i, start = 0;\n+\n+\tfor (i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tif (!strcmp(arg, \"--\")) {\n+\t\t\ti++;\n+\t\t\tbreak;\n+\t\t}\n+\t\telse if (arg[0] == '-')\n+\t\t\tusage(reflog_exists_usage);\n+\t\telse\n+\t\t\tbreak;\n+\t}\n+\n+\tstart = i;\n+\n+\tif (argc - start != 1)\n+\t\tusage(reflog_exists_usage);\n+\n+\tif (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))\n+\t\tdie(\"invalid ref format: %s\", argv[start]);\n+\treturn !reflog_exists(argv[start]);\n+}\n+\n /*\n  * main \"reflog\"\n  */\n \n static const char reflog_usage[] =\n-\"git reflog [ show | expire | delete ]\";\n+\"git reflog [ show | expire | delete | create | exists ]\";\n \n int cmd_reflog(int argc, const char **argv, const char *prefix)\n {\n@@ -724,5 +791,11 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[1], \"delete\"))\n \t\treturn cmd_reflog_delete(argc - 1, argv + 1, prefix);\n \n+\tif (!strcmp(argv[1], \"create\"))\n+\t\treturn cmd_reflog_create(argc - 1, argv + 1, prefix);\n+\n+\tif (!strcmp(argv[1], \"exists\"))\n+\t\treturn cmd_reflog_exists(argc - 1, argv + 1, prefix);\n+\n \treturn cmd_log_reflog(argc, argv, prefix);\n }\ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex 6f47c0d..6e1abe7 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -166,4 +166,16 @@ test_expect_success 'git log -g -p shows diffs vs. parents' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reflog exists works' '\n+\tgit reflog exists refs/heads/master &&\n+\t! git reflog exists refs/heads/nonexistent\n+'\n+\n+test_expect_success 'reflog create works' '\n+\tgit update-ref non-refs-dir HEAD &&\n+\t! git reflog exists non-refs-dir &&\n+\tgit reflog create non-refs-dir &&\n+\tgit reflog exists non-refs-dir\n+'\n+\n test_done\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"265177","messageId":"1435609076-8592-8-git-send-email-dturner@twopensource.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v6 7/7] git-stash: use git-reflog instead of creating files","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:17:56Z","receivedAt":"2015-06-29T20:17:56Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"This is in support of alternate ref backends which don't necessarily\nstore reflogs as files.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n git-stash.sh | 4 ++--\n refs.c       | 1 +\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 8e9e2cd..27155bc 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -184,7 +184,7 @@ store_stash () {\n \tfi\n \n \t# Make sure the reflog for stash is kept.\n-\t: >>\"$(git rev-parse --git-path logs/$ref_stash)\"\n+\tgit reflog create \"$ref_stash\"\n \tgit update-ref -m \"$stash_msg\" $ref_stash $w_commit\n \tret=$?\n \ttest $ret != 0 && test -z $quiet &&\n@@ -262,7 +262,7 @@ save_stash () {\n \t\tsay \"$(gettext \"No local changes to save\")\"\n \t\texit 0\n \tfi\n-\ttest -f \"$(git rev-parse --git-path logs/$ref_stash)\" ||\n+\tgit reflog exists $ref_stash ||\n \t\tclear_stash || die \"$(gettext \"Cannot initialize stash\")\"\n \n \tcreate_stash \"$stash_msg\" $untracked\ndiff --git a/refs.c b/refs.c\nindex 1e53ef0..0f240c9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3125,6 +3125,7 @@ static int should_autocreate_reflog(const char *refname)\n \treturn starts_with(refname, \"refs/heads/\") ||\n \t\tstarts_with(refname, \"refs/remotes/\") ||\n \t\tstarts_with(refname, \"refs/notes/\") ||\n+\t\t!strcmp(refname, \"refs/stash\") ||\n \t\t!strcmp(refname, \"HEAD\");\n }\n \n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"265183","messageId":"xmqq7fqm9ryv.fsf@gitster.dls.corp.google.com","threadId":"39747","inReplyTo":"1435609076-8592-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v6 0/7] refs backend preamble","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-29T20:31:36Z","receivedAt":"2015-06-29T20:31:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> Minor formatting fixes from Junio Hamano.\n\nThere is another.\n\nBy the way, \"unused variable\" is not a formatting fix.\n\n\n git-bisect.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex dddcc89..2fd8ea6 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -210,7 +210,7 @@ check_expected_revs() {\n }\n \n bisect_head_exists() {\n-    git rev-parse --quiet --verify \"BISECT_HEAD\" >/dev/null\n+\tgit rev-parse --quiet --verify \"BISECT_HEAD\" >/dev/null\n }\n \n bisect_skip() {\n"},{"id":"265187","messageId":"1435610933.30953.4.camel@twopensource.com","threadId":"39747","inReplyTo":"xmqq7fqm9ryv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 0/7] refs backend preamble","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-29T20:48:53Z","receivedAt":"2015-06-29T20:48:53Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"\nOn Mon, 2015-06-29 at 13:31 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > Minor formatting fixes from Junio Hamano.\n> \n> There is another.\n\nWould it be simpler for me to push this sort of fixup to github and and\njust mention that a new patchset is available?  If so, here it is: \n\nhttps://github.com/dturner-tw/git.git\ndturner/pluggable-backends-preamble\n\nIf not, I'll re-send.\n\nOf course for real reviews, I'll continue sending patches to the mailing\nlist using git send-email.\n\n> By the way, \"unused variable\" is not a formatting fix.\n\nIndeed, it is not.\n"},{"id":"265189","messageId":"xmqq381a9qih.fsf@gitster.dls.corp.google.com","threadId":"39747","inReplyTo":"1435609076-8592-8-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v6 7/7] git-stash: use git-reflog instead of creating files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-29T21:03:02Z","receivedAt":"2015-06-29T21:03:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"OK, I'll squash in the last bit and restart today's integration run.\nEverything looks much nicer than any previous versions ;-)\n\nThanks.\n"},{"id":"265202","messageId":"CAPig+cROJJNTcZnZfMP0meA8ZWGcSHcQCMTCkuC+kn_+OQZ-zA@mail.gmail.com","threadId":"39747","inReplyTo":"1435609076-8592-7-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-30T07:34:10Z","receivedAt":"2015-06-30T07:34:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 29, 2015 at 4:17 PM, David Turner <dturner@twopensource.com> wrote:\n> These are necessary because alternate ref backends might store reflogs\n> somewhere other than .git/logs.  Code that now directly manipulates\n> .git/logs should instead go through git-reflog.\n>\n> In a moment, we will use these functions to make git stash work with\n> alternate ref backends.\n>\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index c2eb8ff..3080865 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =\n> +static int cmd_reflog_create(int argc, const char **argv, const char *prefix)\n> +{\n> +       int i, status = 0, start = 0;\n> +       struct strbuf err = STRBUF_INIT;\n> +\n> +       for (i = 1; i < argc; i++) {\n> +               const char *arg = argv[i];\n> +               if (!strcmp(arg, \"--\")) {\n> +                       i++;\n> +                       break;\n> +               }\n> +               else if (arg[0] == '-')\n> +                       usage(reflog_create_usage);\n> +               else\n> +                       break;\n> +       }\n> +\n> +       start = i;\n> +\n> +       if (argc - start < 1)\n> +               return error(\"Nothing to create?\");\n> +\n> +       for (i = start; i < argc; i++) {\n> +               if (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))\n> +                       die(\"invalid ref format: %s\", argv[i]);\n> +       }\n> +       for (i = start; i < argc; i++) {\n> +               if (safe_create_reflog(argv[i], &err, 1)) {\n> +                       error(\"could not create reflog %s: %s\", argv[i],\n> +                             err.buf);\n> +                       status = 1;\n> +                       strbuf_release(&err);\n\nThis feels a bit dirty. While it's true that the current\nimplementation of strbuf_release() re-initializes the strbuf (so\nyou're not technically wrong by re-using it), that's an implementation\ndetail; and indeed, the strbuf_release() documentation says:\n\n    Release a string buffer and the memory it used. You should not\n    use the string buffer after using this function, unless you\n    initialize it again.\n\nAlternatives would be strbuf_reset() or declaring and releasing the\nstrbuf within the for-loop scope.\n\n> +               }\n> +       }\n> +       return status;\n> +}\n"},{"id":"265241","messageId":"1435679865.30953.6.camel@twopensource.com","threadId":"39747","inReplyTo":"CAPig+cROJJNTcZnZfMP0meA8ZWGcSHcQCMTCkuC+kn_+OQZ-zA@mail.gmail.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-30T15:57:45Z","receivedAt":"2015-06-30T15:57:45Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2015-06-30 at 03:34 -0400, Eric Sunshine wrote:\n> > +                       strbuf_release(&err);\n> \n> This feels a bit dirty. While it's true that the current\n\nThanks.  New patchset pushed to the branch on github:\n\nhttps://github.com/dturner-tw/git.git\ndturner/pluggable-backends-preamble\n"},{"id":"265246","messageId":"xmqqbnfx89iz.fsf@gitster.dls.corp.google.com","threadId":"39747","inReplyTo":"CAPig+cROJJNTcZnZfMP0meA8ZWGcSHcQCMTCkuC+kn_+OQZ-zA@mail.gmail.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T16:07:32Z","receivedAt":"2015-06-30T16:07:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> +       for (i = start; i < argc; i++) {\n>> +               if (safe_create_reflog(argv[i], &err, 1)) {\n>> +                       error(\"could not create reflog %s: %s\", argv[i],\n>> +                             err.buf);\n>> +                       status = 1;\n>> +                       strbuf_release(&err);\n>\n> This feels a bit dirty.\n\nHmm, I do not share that feeling.  I wouldn't be surprised if you\nfound a lot of existing codepaths that run _init() a strbuf once,\nwork on it and then _release() once a section of code is done with\nit, reuse it for further (unrelated) processing, knowing that\n_release() implicitly did _init() and the strbuf is ready to use\nafter that.  I thought that was a very well established pattern.\n\n> While it's true that the current\n> implementation of strbuf_release() re-initializes the strbuf (so\n> you're not technically wrong by re-using it), that's an implementation\n> detail; and indeed, the strbuf_release() documentation says:\n>\n>     Release a string buffer and the memory it used. You should not\n>     use the string buffer after using this function, unless you\n>     initialize it again.\n\nHmph. Perhaps the doc is wrong? ;-)\n\n> Alternatives would be strbuf_reset() or declaring and releasing the\n> strbuf within the for-loop scope.\n\nBecause _reset() just rewinds the .len pointer without deallocating,\nyou would need an extra _release() before it goes out of scope. If\nit is expected that the strbuf will be reused for a number of times,\nthe length of the string each iteration uses is similar, and you\nwill iterate the loop many times, \"_reset() each time and _release()\nto clean-up\" pattern would save many calls to realloc/free.\n"},{"id":"265263","messageId":"CAPig+cQ7tQV6uG0sVMsPJX=8V3g+zufRCYcdfDo4ixwwUp5hZA@mail.gmail.com","threadId":"39747","inReplyTo":"xmqqbnfx89iz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-30T18:20:58Z","receivedAt":"2015-06-30T18:20:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2015 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>>> +       for (i = start; i < argc; i++) {\n>>> +               if (safe_create_reflog(argv[i], &err, 1)) {\n>>> +                       error(\"could not create reflog %s: %s\", argv[i],\n>>> +                             err.buf);\n>>> +                       status = 1;\n>>> +                       strbuf_release(&err);\n>>\n>> This feels a bit dirty.\n>\n> Hmm, I do not share that feeling.  I wouldn't be surprised if you\n> found a lot of existing codepaths that run _init() a strbuf once,\n> work on it and then _release() once a section of code is done with\n> it, reuse it for further (unrelated) processing, knowing that\n> _release() implicitly did _init() and the strbuf is ready to use\n> after that.  I thought that was a very well established pattern.\n\nThat could the case, and I may not be familiar with code doing that.\n\nI have, however, seen plenty of code which uses strbuf_reset() in the\nway you describe.\n\n>> While it's true that the current\n>> implementation of strbuf_release() re-initializes the strbuf (so\n>> you're not technically wrong by re-using it), that's an implementation\n>> detail; and indeed, the strbuf_release() documentation says:\n>>\n>>     Release a string buffer and the memory it used. You should not\n>>     use the string buffer after using this function, unless you\n>>     initialize it again.\n>\n> Hmph. Perhaps the doc is wrong? ;-)\n\nPerhaps. I always interpreted the documentation as meaning that the\nproject reserved the right to change the underlying implementation.\n\n>> Alternatives would be strbuf_reset() or declaring and releasing the\n>> strbuf within the for-loop scope.\n>\n> Because _reset() just rewinds the .len pointer without deallocating,\n> you would need an extra _release() before it goes out of scope. If\n> it is expected that the strbuf will be reused for a number of times,\n> the length of the string each iteration uses is similar, and you\n> will iterate the loop many times, \"_reset() each time and _release()\n> to clean-up\" pattern would save many calls to realloc/free.\n\nYep, that's why I suggested strbuf_reset() as an alternative (and\nlikely would have chosen it myself).\n"},{"id":"265272","messageId":"xmqq3819c6ze.fsf@gitster.dls.corp.google.com","threadId":"39747","inReplyTo":"CAPig+cQ7tQV6uG0sVMsPJX=8V3g+zufRCYcdfDo4ixwwUp5hZA@mail.gmail.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T19:48:53Z","receivedAt":"2015-06-30T19:48:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>>> Alternatives would be strbuf_reset() or declaring and releasing the\n>>> strbuf within the for-loop scope.\n>>\n>> Because _reset() just rewinds the .len pointer without deallocating,\n>> you would need an extra _release() before it goes out of scope. If\n>> it is expected that the strbuf will be reused for a number of times,\n>> the length of the string each iteration uses is similar, and you\n>> will iterate the loop many times, \"_reset() each time and _release()\n>> to clean-up\" pattern would save many calls to realloc/free.\n>\n> Yep, that's why I suggested strbuf_reset() as an alternative (and\n> likely would have chosen it myself).\n\nOK, then let's do that by squashing this in.\n\n builtin/reflog.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3080865..e9ba600 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -733,10 +733,11 @@ static int cmd_reflog_create(int argc, const char **argv, const char *prefix)\n \t\tif (safe_create_reflog(argv[i], &err, 1)) {\n \t\t\terror(\"could not create reflog %s: %s\", argv[i],\n \t\t\t      err.buf);\n+\t\t\tstrbuf_reset(&err);\n \t\t\tstatus = 1;\n-\t\t\tstrbuf_release(&err);\n \t\t}\n \t}\n+\tstrbuf_release(&err);\n \treturn status;\n }\n \n"},{"id":"265273","messageId":"1435699146.30953.7.camel@twopensource.com","threadId":"39747","inReplyTo":"xmqq3819c6ze.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-30T21:19:06Z","receivedAt":"2015-06-30T21:19:06Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2015-06-30 at 12:48 -0700, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> >>> Alternatives would be strbuf_reset() or declaring and releasing the\n> >>> strbuf within the for-loop scope.\n> >>\n> >> Because _reset() just rewinds the .len pointer without deallocating,\n> >> you would need an extra _release() before it goes out of scope. If\n> >> it is expected that the strbuf will be reused for a number of times,\n> >> the length of the string each iteration uses is similar, and you\n> >> will iterate the loop many times, \"_reset() each time and _release()\n> >> to clean-up\" pattern would save many calls to realloc/free.\n> >\n> > Yep, that's why I suggested strbuf_reset() as an alternative (and\n> > likely would have chosen it myself).\n> \n> OK, then let's do that by squashing this in.\n> \n>  builtin/reflog.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n\nI squashed that into my repo on github:\n\nhttps://github.com/dturner-tw/git.git\non the branch dturner/pluggable-backends-preamble\n"},{"id":"265274","messageId":"xmqqy4j0c2e2.fsf@gitster.dls.corp.google.com","threadId":"39747","inReplyTo":"1435699146.30953.7.camel@twopensource.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T21:28:05Z","receivedAt":"2015-06-30T21:28:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> On Tue, 2015-06-30 at 12:48 -0700, Junio C Hamano wrote:\n>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> \n>> >>> Alternatives would be strbuf_reset() or declaring and releasing the\n>> >>> strbuf within the for-loop scope.\n>> >>\n>> >> Because _reset() just rewinds the .len pointer without deallocating,\n>> >> you would need an extra _release() before it goes out of scope. If\n>> >> it is expected that the strbuf will be reused for a number of times,\n>> >> the length of the string each iteration uses is similar, and you\n>> >> will iterate the loop many times, \"_reset() each time and _release()\n>> >> to clean-up\" pattern would save many calls to realloc/free.\n>> >\n>> > Yep, that's why I suggested strbuf_reset() as an alternative (and\n>> > likely would have chosen it myself).\n>> \n>> OK, then let's do that by squashing this in.\n>> \n>>  builtin/reflog.c | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> I squashed that into my repo on github:\n\nI'm already deep into today's integration cycle, so it is unlikely\nI'd pull that before I push the result out.  Please let me know if\nthe resulting tree looks wrong (I only queued it to be squashed,\nhaven't done the squashing two into one yet).\n\nThanks.  The 7-patch series with this fixup looks good to me.\n"},{"id":"265560","messageId":"559AA490.3080605@alum.mit.edu","threadId":"39747","inReplyTo":"1435609076-8592-2-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-06T15:53:52Z","receivedAt":"2015-07-06T15:53:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/29/2015 10:17 PM, David Turner wrote:\n> Add an err argument to log_ref_setup that can explain the reason\n> for a failure. This then eliminates the need to manage errno through\n> this function since we can just add strerror(errno) to the err string\n> when meaningful. No callers relied on errno from this function for\n> anything else than the error message.\n> \n> Also add err arguments to private functions write_ref_to_lockfile,\n> log_ref_write_1, commit_ref_update. This again eliminates the need to\n> manage errno in these functions.\n> \n> Update of a patch by Ronnie Sahlberg.\n\nFirst a general comment: we have a convention, not yet very well adhered\nto, that error messages should start with lower-case letters. I know\nthat in many cases you are just carrying along pre-existing error\nmessages that are capitalized. But at a minimum, please avoid adding new\nerror messages that are capitalized. And if you are feeling ambitious,\nfeel free to lower-case some existing ones :-)\n\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n>  builtin/checkout.c |   8 ++--\n>  refs.c             | 111 ++++++++++++++++++++++++++++-------------------------\n>  refs.h             |   4 +-\n>  3 files changed, 66 insertions(+), 57 deletions(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> [...]\n> diff --git a/refs.c b/refs.c\n> index fb568d7..c97ca02 100644\n> --- a/refs.c\n> +++ b/refs.c\n> [...]\n> @@ -3216,26 +3217,25 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\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> -\t\terrno = save_errno;\n> +\t\tstrbuf_addf(err, \"Unable to append to %s. %s\", log_file,\n> +\t\t\t    strerror(errno));\n>  \t\treturn -1;\n\nAbove, the call to close(logfd) could clobber errno. It would be better\nto exchange the calls.\n\n>  \t}\n>  \tif (close(logfd)) {\n> -\t\tint save_errno = errno;\n> -\t\terror(\"Unable to append to %s\", log_file);\n> -\t\terrno = save_errno;\n> +\t\tstrbuf_addf(err, \"Unable to append to %s. %s\", log_file,\n> +\t\t\t    strerror(errno));\n>  \t\treturn -1;\n>  \t}\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> +\t\t\t const unsigned char *new_sha1, const char *msg,\n> +\t\t\t struct strbuf *err)\n>  {\n>  \tstruct strbuf sb = STRBUF_INIT;\n> -\tint ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb);\n> +\tint ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, err);\n>  \tstrbuf_release(&sb);\n>  \treturn ret;\n>  }\n> [...]\n> @@ -3288,12 +3290,15 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n>   * necessary, using the specified lockmsg (which can be NULL).\n>   */\n>  static int commit_ref_update(struct ref_lock *lock,\n> -\t\t\t     const unsigned char *sha1, const char *logmsg)\n> +\t\t\t     const unsigned char *sha1, const char *logmsg,\n> +\t\t\t     struct strbuf *err)\n>  {\n>  \tclear_loose_ref_cache(&ref_cache);\n> -\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg) < 0 ||\n> +\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0 ||\n>  \t    (strcmp(lock->ref_name, lock->orig_ref_name) &&\n> -\t     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg) < 0)) {\n> +\t     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {\n> +\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n> +\t\t\t    lock->ref_name);\n>  \t\tunlock_ref(lock);\n>  \t\treturn -1;\n>  \t}\n\nSince you are passing err into log_ref_write(), I assume that it would\nalready have an error message written to it by the time you enter into\nthis block. Yet in the block you append more text to it.\n\nIt seems to me that you either want to clear err and replace it with\nyour own message, or create a new message that looks like\n\n    Cannot update the ref '%s': %s\n\nwhere the second \"%s\" is replaced with the error from log_ref_write().\n\n> @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,\n>  \t\t\t\t\t      head_sha1, &head_flag);\n>  \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n>  \t\t    !strcmp(head_ref, lock->ref_name))\n> -\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg);\n> +\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg,\n> +\t\t\t\t      err);\n\nHere you don't check for errors from log_ref_write(). So it might or\nmight not have written something to err. If it has, is that error\nhandled correctly?\n\n>  \t}\n>  \tif (commit_ref(lock)) {\n>  \t\terror(\"Couldn't set %s\", lock->ref_name);\n> @@ -3336,6 +3342,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n>  \tint fd, len, written;\n>  \tchar *git_HEAD = git_pathdup(\"%s\", ref_target);\n>  \tunsigned char old_sha1[20], new_sha1[20];\n> +\tstruct strbuf err = STRBUF_INIT;\n>  \n>  \tif (logmsg && read_ref(ref_target, old_sha1))\n>  \t\thashclr(old_sha1);\n> @@ -3384,8 +3391,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n>  #ifndef NO_SYMLINK_HEAD\n>  \tdone:\n>  #endif\n> -\tif (logmsg && !read_ref(refs_heads_master, new_sha1))\n> -\t\tlog_ref_write(ref_target, old_sha1, new_sha1, logmsg);\n> +\tif (logmsg && !read_ref(refs_heads_master, new_sha1) &&\n> +\t\tlog_ref_write(ref_target, old_sha1, new_sha1, logmsg, &err)) {\n> +\t\terror(\"%s\", err.buf);\n> +\t\tstrbuf_release(&err);\n> +\t}\n>  \n>  \tfree(git_HEAD);\n>  \treturn 0;\n> @@ -4021,14 +4031,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>  \t\t\t\t * value, so we don't need to write it.\n>  \t\t\t\t */\n>  \t\t\t} else if (write_ref_to_lockfile(update->lock,\n> -\t\t\t\t\t\t\t update->new_sha1)) {\n> +\t\t\t\t\t\t\t update->new_sha1,\n> +\t\t\t\t\t\t\t err)) {\n>  \t\t\t\t/*\n>  \t\t\t\t * The lock was freed upon failure of\n>  \t\t\t\t * write_ref_to_lockfile():\n>  \t\t\t\t */\n>  \t\t\t\tupdate->lock = NULL;\n> -\t\t\t\tstrbuf_addf(err, \"cannot update the ref '%s'.\",\n> -\t\t\t\t\t    update->refname);\n\nPreviously, the error generated here was \"cannot update the ref '%s'.\"\nBut now that you are letting write_ref_to_lockfile() fill err, the error\nmessage will be something from that function. This might be OK or it\nmight not. If you think it is, it would be worth a word or two of\njustification in the commit message.\n\n>  \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n>  \t\t\t\tgoto cleanup;\n>  \t\t\t} else {\n> @@ -4054,11 +4063,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>  \n>  \t\tif (update->flags & REF_NEEDS_COMMIT) {\n>  \t\t\tif (commit_ref_update(update->lock,\n> -\t\t\t\t\t      update->new_sha1, update->msg)) {\n> +\t\t\t\t\t      update->new_sha1, update->msg, err)) {\n>  \t\t\t\t/* freed by commit_ref_update(): */\n>  \t\t\t\tupdate->lock = NULL;\n> -\t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n> -\t\t\t\t\t    update->refname);\n\nSame story here: you use whatever error message that commit_ref_update()\nemitted rather than the one that was previously chosen here.\n\n>  \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n>  \t\t\t\tgoto cleanup;\n>  \t\t\t} else {\n> diff --git a/refs.h b/refs.h\n> index e82fca5..debdefc 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -226,9 +226,9 @@ int pack_refs(unsigned int flags);\n>  #define REF_NODEREF\t0x01\n>  \n>  /*\n> - * Setup reflog before using. Set errno to something meaningful on failure.\n> + * Setup reflog before using. Fill in err and return -1 on failure.\n>   */\n> -int log_ref_setup(const char *refname, struct strbuf *logfile);\n> +int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err);\n>  \n>  /** Reads log for the value of ref during at_time. **/\n>  extern int read_ref_at(const char *refname, unsigned int flags,\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265561","messageId":"559AA61E.1010308@alum.mit.edu","threadId":"39747","inReplyTo":"1435609076-8592-3-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v6 2/7] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-06T16:00:30Z","receivedAt":"2015-07-06T16:00:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/29/2015 10:17 PM, David Turner wrote:\n> Instead of directly writing to and reading from files in\n> $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD\n> and REVERT_HEAD.\n> \n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n>  branch.c                         |  4 ++--\n>  builtin/commit.c                 |  6 +++---\n>  builtin/merge.c                  |  2 +-\n>  contrib/completion/git-prompt.sh |  4 ++--\n>  git-gui/lib/commit.tcl           |  2 +-\n>  sequencer.c                      | 39 ++++++++++++++++++++-------------------\n>  t/t7509-commit.sh                |  4 ++--\n>  wt-status.c                      |  6 ++----\n>  8 files changed, 33 insertions(+), 34 deletions(-)\n> \n> [...]\n> diff --git a/sequencer.c b/sequencer.c\n> index f8421a8..44c43e5 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -158,21 +158,22 @@ static void free_message(struct commit *commit, struct commit_message *msg)\n>  \tunuse_commit_buffer(commit, msg->message);\n>  }\n>  \n> -static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)\n> +static void write_cherry_pick_head(struct commit *commit, const char *ref)\n>  {\n> -\tconst char *filename;\n> -\tint fd;\n> -\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tvoid *transaction;\n>  \n> -\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n> +\ttransaction = ref_transaction_begin(&err);\n> +\tif (!transaction)\n> +\t\tdie(_(\"Could not create transaction: %s\"), err.buf);\n>  \n> -\tfilename = git_path(\"%s\", pseudoref);\n> -\tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n> -\tif (fd < 0)\n> -\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n> -\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n> -\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n> -\tstrbuf_release(&buf);\n> +\tif (ref_transaction_update(transaction, ref, commit->object.sha1,\n> +\t\t\t\t   NULL, REF_NODEREF, NULL,\n> +\t\t\t\t   &err))\n> +\t\tdie(_(\"Could not write ref %s: %s\"), ref, err.buf);\n> +\n> +\tif (ref_transaction_commit(transaction, &err))\n> +\t\tdie(_(\"Could not commit ref write %s: %s\"), ref, err.buf);\n>  }\n\nI didn't check all the details, but this code looks a lot like what\nupdate_ref() does. Maybe you can use that function?\n\n>  static void print_advice(int show_hint, struct replay_opts *opts)\n> @@ -186,7 +187,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n> [...]\n\nOtherwise, looks good to me.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265565","messageId":"559AAB18.1090701@alum.mit.edu","threadId":"39747","inReplyTo":"1435609076-8592-6-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-06T16:21:44Z","receivedAt":"2015-07-06T16:21:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/29/2015 10:17 PM, David Turner wrote:\n> The safe_create_reflog function creates a reflog, if it does not\n> already exist.\n> \n> The log_ref_setup function becomes private and gains a force_create\n> parameter to force the creation of a reflog even if log_all_ref_updates\n> is false or the refname is not one of the special refnames.\n> \n> The new parameter also reduces the need to store, modify, and restore\n> the log_all_ref_updates global before reflog creation.\n> \n> In a moment, we will use this to add reflog creation commands to\n> git-reflog.\n> \n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n>  builtin/checkout.c | 10 +---------\n>  refs.c             | 25 +++++++++++++++++++++----\n>  refs.h             |  2 +-\n>  3 files changed, 23 insertions(+), 14 deletions(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 93f63d3..9f68399 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -620,19 +620,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n>  \tif (opts->new_branch) {\n>  \t\tif (opts->new_orphan_branch) {\n>  \t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n> -\t\t\t\tint temp;\n> -\t\t\t\tstruct strbuf log_file = STRBUF_INIT;\n> -\t\t\t\tint ret;\n>  \t\t\t\tconst char *ref_name;\n>  \t\t\t\tstruct strbuf err = STRBUF_INIT;\n>  \n>  \t\t\t\tref_name = mkpath(\"refs/heads/%s\", opts->new_orphan_branch);\n> -\t\t\t\ttemp = log_all_ref_updates;\n> -\t\t\t\tlog_all_ref_updates = 1;\n> -\t\t\t\tret = log_ref_setup(ref_name, &log_file, &err);\n> -\t\t\t\tlog_all_ref_updates = temp;\n> -\t\t\t\tstrbuf_release(&log_file);\n> -\t\t\t\tif (ret) {\n> +\t\t\t\tif (safe_create_reflog(ref_name, &err, 1)) {\n>  \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s'. %s\\n\"),\n>  \t\t\t\t\t\topts->new_orphan_branch, err.buf);\n>  \t\t\t\t\tstrbuf_release(&err);\n\nThis was probably already dangerous before your change, but...\n\nmkpath() returns a pointer to a static buffer. It is subject to being\noverwritten if any of a number of path-related functions is called. So\npassing it into a function is dangerous.\n\nInstead, you should store it into memory that you control, for example\nby using a strbuf and strbuf_addf().\n\nAlso, we usually call variables holding reference names \"refname\", not\n\"ref_name\". Maybe rename the variable while you are in the area.\n\n> diff --git a/refs.c b/refs.c\n> index 30e81ba..1e53ef0 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3128,8 +3128,14 @@ static int should_autocreate_reflog(const char *refname)\n>  \t\t!strcmp(refname, \"HEAD\");\n>  }\n>  \n> -/* This function will fill in *err and return -1 on failure */\n> -int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)\n> +/*\n> + * This function creates a reflog for a ref.  If force_create = 0, the\n> + * reflog will only be created for certain refs (those for which\n> + * should_autocreate_reflog returns non-zero.  Otherwise, it will be\n> + * created regardless of the ref name.  This function will fill in *err\n> + * and return -1 on failure\n> + */\n\nIt is preferable to write function docstrings in the imperative voice:\n\n    Create a reflog for a ref. If force_create == 0, only create\n    the reflog for certain refs...\n\n> +static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)\n>  {\n>  \tint logfd, oflags = O_APPEND | O_WRONLY;\n>  \tchar *logfile;\n> @@ -3138,7 +3144,7 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf\n>  \tlogfile = sb_logfile->buf;\n>  \t/* make sure the rest of the function can't change \"logfile\" */\n>  \tsb_logfile = NULL;\n> -\tif (should_autocreate_reflog(refname)) {\n> +\tif (force_create || should_autocreate_reflog(refname)) {\n>  \t\tif (safe_create_leading_directories(logfile) < 0) {\n>  \t\t\tstrbuf_addf(err, \"unable to create directory for %s. \"\n>  \t\t\t\t    \"%s\", logfile, strerror(errno));\n> @@ -3173,6 +3179,17 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf\n>  \treturn 0;\n>  }\n>  \n> +\n> +int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)\n> +{\n> +\tint ret;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tret = log_ref_setup(refname, &sb, err, force_create);\n> +\tstrbuf_release(&sb);\n> +\treturn ret;\n> +}\n> +\n\nIs it really necessary to have two functions, safe_create_reflog() and\nlog_ref_setup()? I don't see any of the callers doing anything special\nwith the sb_logfile argument from the latter, so maybe it could be\ninlined into safe_create_reflog()? Maybe I'm overlooking something.\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265571","messageId":"559AB200.5020108@alum.mit.edu","threadId":"39747","inReplyTo":"1435609076-8592-7-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-06T16:51:12Z","receivedAt":"2015-07-06T16:51:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/29/2015 10:17 PM, David Turner wrote:\n> These are necessary because alternate ref backends might store reflogs\n> somewhere other than .git/logs.  Code that now directly manipulates\n> .git/logs should instead go through git-reflog.\n> \n> In a moment, we will use these functions to make git stash work with\n> alternate ref backends.\n> \n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n>  Documentation/git-reflog.txt | 10 ++++++\n>  builtin/reflog.c             | 75 +++++++++++++++++++++++++++++++++++++++++++-\n>  t/t1411-reflog-show.sh       | 12 +++++++\n>  3 files changed, 96 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\n> index 5e7908e..2bf8aa6 100644\n> --- a/Documentation/git-reflog.txt\n> +++ b/Documentation/git-reflog.txt\n> @@ -23,6 +23,8 @@ depending on the subcommand:\n>  \t[--dry-run] [--verbose] [--all | <refs>...]\n>  'git reflog delete' [--rewrite] [--updateref]\n>  \t[--dry-run] [--verbose] ref@\\{specifier\\}...\n> +'git reflog create' <refs>...\n> +'git reflog exists' <ref>\n>  \n>  Reference logs, or \"reflogs\", record when the tips of branches and\n>  other references were updated in the local repository. Reflogs are\n> @@ -52,6 +54,14 @@ argument must be an _exact_ entry (e.g. \"`git reflog delete\n>  master@{2}`\"). This subcommand is also typically not used directly by\n>  end users.\n>  \n> +The \"create\" subcommand creates a reflog for one or more refs. Most\n> +refs (those under refs/heads, refs/remotes, and refs/tags) will\n> +automatically have reflogs created. Other refs will not. This command\n> +allows manual ref creation.\n> +\n> +The \"exists\" subcommand checks whether a ref has a reflog.  It exists\n> +with zero status if the reflog exists, and non-zero status if it does\n> +not.\n>  \n>  OPTIONS\n>  -------\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index c2eb8ff..3080865 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =\n>  \"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...\";\n>  static const char reflog_delete_usage[] =\n>  \"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...\";\n> +static const char reflog_create_usage[] =\n> +\"git reflog create <refs>...\";\n> +static const char reflog_exists_usage[] =\n> +\"git reflog exists <ref>\";\n>  \n>  static unsigned long default_reflog_expire;\n>  static unsigned long default_reflog_expire_unreachable;\n> @@ -699,12 +703,75 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n>  \treturn status;\n>  }\n>  \n> +static int cmd_reflog_create(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint i, status = 0, start = 0;\n\nIt looks like start is initialized unconditionally after the first loop,\nso the initialization here is a red herring.\n\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tconst char *arg = argv[i];\n> +\t\tif (!strcmp(arg, \"--\")) {\n> +\t\t\ti++;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\telse if (arg[0] == '-')\n> +\t\t\tusage(reflog_create_usage);\n> +\t\telse\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tstart = i;\n> +\n> +\tif (argc - start < 1)\n\n    if (start == argc)\n\nseems clearer, but that might just be me.\n\n> +\t\treturn error(\"Nothing to create?\");\n> +\n> +\tfor (i = start; i < argc; i++) {\n> +\t\tif (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))\n> +\t\t\tdie(\"invalid ref format: %s\", argv[i]);\n> +\t}\n> +\tfor (i = start; i < argc; i++) {\n> +\t\tif (safe_create_reflog(argv[i], &err, 1)) {\n> +\t\t\terror(\"could not create reflog %s: %s\", argv[i],\n> +\t\t\t      err.buf);\n> +\t\t\tstatus = 1;\n> +\t\t\tstrbuf_release(&err);\n> +\t\t}\n> +\t}\n> +\treturn status;\n> +}\n> +\n\nSo, I have a philosophical question here with a practical side...\n\nIt appears that this command allows you to create a reflog for a\nreference that doesn't yet exist. At first blush, this seems to make\nsense. Suppose you want the creation of a reference to be logged. Then\nyou can first turn on its reflog, then create it.\n\nBut I think this is going to create problems. Reflogs are rather\nintimately connected to their references. For example, writes to a\nreflog are guarded by locking the corresponding reference. And currently\na reflog file is deleted when the corresponding reference is deleted.\nAlso, there are constraints about which references can coexist; for\nexample, references \"refs/heads/foo\" and \"refs/heads/foo/bar\" cannot\nexist at the same time, because (at least when using the filesystem\nbackend), \"refs/heads/foo\" would have to be both a file and a directory\nat the same time. So if one of these references already exists, it would\nbe an error to create a reflog for the other one.\n\nSimilarly, if there is a reflog file for one of these references that\nwas created by this command, but there is not yet a corresponding\nreference, then any command that later tries to create the other\nreference will not be able to create the reflog for *that* reference\nbecause it will conflict with the existing reflog. I doubt that the\nreference creation is smart enough to deal with this situation.\n\nSo all in all, I think it is unwise to allow a reflog to be created\nwithout its corresponding reference.\n\nThis, in turn, suggests one or both of the following alternatives:\n\n1. Allow \"git reflog create\", but only for references that already exist.\n\n2. If we want to allow a reflog to be created at the same time as the\ncorresponding reference, the reference-creation commands (\"git branch\",\n\"git tag\", \"git update-ref\", and maybe some others) probably need a new\noption like \"--create-reflog\" (and, for symmetry, probably\n\"--no-create-reflog\").\n\nAt the API level, it might make sense for the ref-transaction functions\nto get a new \"REF_FORCE_CREATE_REFLOG\" flag or something.\n\n> +static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint i, start = 0;\n> +\n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tconst char *arg = argv[i];\n> +\t\tif (!strcmp(arg, \"--\")) {\n> +\t\t\ti++;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\telse if (arg[0] == '-')\n> +\t\t\tusage(reflog_exists_usage);\n> +\t\telse\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tstart = i;\n> +\n> +\tif (argc - start != 1)\n> +\t\tusage(reflog_exists_usage);\n> +\n> +\tif (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))\n> +\t\tdie(\"invalid ref format: %s\", argv[start]);\n> +\treturn !reflog_exists(argv[start]);\n> +}\n> +\n>  /*\n>   * main \"reflog\"\n>   */\n>  \n>  static const char reflog_usage[] =\n> -\"git reflog [ show | expire | delete ]\";\n> +\"git reflog [ show | expire | delete | create | exists ]\";\n>  \n>  int cmd_reflog(int argc, const char **argv, const char *prefix)\n>  {\n> @@ -724,5 +791,11 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)\n>  \tif (!strcmp(argv[1], \"delete\"))\n>  \t\treturn cmd_reflog_delete(argc - 1, argv + 1, prefix);\n>  \n> +\tif (!strcmp(argv[1], \"create\"))\n> +\t\treturn cmd_reflog_create(argc - 1, argv + 1, prefix);\n> +\n> +\tif (!strcmp(argv[1], \"exists\"))\n> +\t\treturn cmd_reflog_exists(argc - 1, argv + 1, prefix);\n> +\n>  \treturn cmd_log_reflog(argc, argv, prefix);\n>  }\n> diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\n> index 6f47c0d..6e1abe7 100755\n> --- a/t/t1411-reflog-show.sh\n> +++ b/t/t1411-reflog-show.sh\n> @@ -166,4 +166,16 @@ test_expect_success 'git log -g -p shows diffs vs. parents' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'reflog exists works' '\n> +\tgit reflog exists refs/heads/master &&\n> +\t! git reflog exists refs/heads/nonexistent\n> +'\n> +\n> +test_expect_success 'reflog create works' '\n> +\tgit update-ref non-refs-dir HEAD &&\n> +\t! git reflog exists non-refs-dir &&\n> +\tgit reflog create non-refs-dir &&\n> +\tgit reflog exists non-refs-dir\n> +'\n> +\n>  test_done\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265780","messageId":"1436308882.5521.15.camel@twopensource.com","threadId":"39747","inReplyTo":"559AA490.3080605@alum.mit.edu","subject":"Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-07T22:41:22Z","receivedAt":"2015-07-07T22:41:22Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote:\n> On 06/29/2015 10:17 PM, David Turner wrote:\n> > Add an err argument to log_ref_setup that can explain the reason\n> > for a failure. This then eliminates the need to manage errno through\n> > this function since we can just add strerror(errno) to the err string\n> > when meaningful. No callers relied on errno from this function for\n> > anything else than the error message.\n> > \n> > Also add err arguments to private functions write_ref_to_lockfile,\n> > log_ref_write_1, commit_ref_update. This again eliminates the need to\n> > manage errno in these functions.\n> > \n> > Update of a patch by Ronnie Sahlberg.\n> \n> First a general comment: we have a convention, not yet very well adhered\n> to, that error messages should start with lower-case letters. I know\n> that in many cases you are just carrying along pre-existing error\n> messages that are capitalized. But at a minimum, please avoid adding new\n> error messages that are capitalized. And if you are feeling ambitious,\n> feel free to lower-case some existing ones :-)\n\nI'll save lowercasing messages for other patches, but I'll try to take\ncare with new messages.\n\n...\n\n> Above, the call to close(logfd) could clobber errno. It would be better\n> to exchange the calls.\n\nDone, thanks.\n\n> Since you are passing err into log_ref_write(), I assume that it would\n> already have an error message written to it by the time you enter into\n> this block. Yet in the block you append more text to it.\n> \n> It seems to me that you either want to clear err and replace it with\n> your own message, or create a new message that looks like\n> \n>     Cannot update the ref '%s': %s\n> \n> where the second \"%s\" is replaced with the error from log_ref_write().\n\nDone, thanks.\n\n> > @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,\n> >  \t\t\t\t\t      head_sha1, &head_flag);\n> >  \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n> >  \t\t    !strcmp(head_ref, lock->ref_name))\n> > -\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg);\n> > +\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg,\n> > +\t\t\t\t      err);\n> \n> Here you don't check for errors from log_ref_write(). So it might or\n> might not have written something to err. If it has, is that error\n> handled correctly?\n\nThat was the case before this patch too. But I guess I might as well\ncheck.\n\n> Previously, the error generated here was \"cannot update the ref '%s'.\"\n> But now that you are letting write_ref_to_lockfile() fill err, the error\n> message will be something from that function. This might be OK or it\n> might not. If you think it is, it would be worth a word or two of\n> justification in the commit message.\n\nWill adjust commit message.\n"},{"id":"265785","messageId":"1436311118.5521.16.camel@twopensource.com","threadId":"39747","inReplyTo":"559AAB18.1090701@alum.mit.edu","subject":"Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-07T23:18:38Z","receivedAt":"2015-07-07T23:18:38Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2015-07-06 at 18:21 +0200, Michael Haggerty wrote:\n\n<snip> changes applied; will re-roll.\n\n> > +\n> > +int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)\n> > +{\n> > +\tint ret;\n> > +\tstruct strbuf sb = STRBUF_INIT;\n> > +\n> > +\tret = log_ref_setup(refname, &sb, err, force_create);\n> > +\tstrbuf_release(&sb);\n> > +\treturn ret;\n> > +}\n> > +\n> \n> Is it really necessary to have two functions, safe_create_reflog() and\n> log_ref_setup()? I don't see any of the callers doing anything special\n> with the sb_logfile argument from the latter, so maybe it could be\n> inlined into safe_create_reflog()? Maybe I'm overlooking something.\n\nlog_ref_write_1 does use the sb_logfile argument.\n"},{"id":"265788","messageId":"1436316577.5521.25.camel@twopensource.com","threadId":"39747","inReplyTo":"559AB200.5020108@alum.mit.edu","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:49:37Z","receivedAt":"2015-07-08T00:49:37Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:\n\n> > +{\n> > +\tint i, status = 0, start = 0;\n> \n> It looks like start is initialized unconditionally after the first loop,\n> so the initialization here is a red herring.\n\nWill fix.\n\n> So, I have a philosophical question here with a practical side...\n> \n> It appears that this command allows you to create a reflog for a\n> reference that doesn't yet exist. At first blush, this seems to make\n> sense. Suppose you want the creation of a reference to be logged. Then\n> you can first turn on its reflog, then create it.\n> \n> But I think this is going to create problems. Reflogs are rather\n> intimately connected to their references. For example, writes to a\n> reflog are guarded by locking the corresponding reference. And currently\n> a reflog file is deleted when the corresponding reference is deleted.\n> Also, there are constraints about which references can coexist; for\n> example, references \"refs/heads/foo\" and \"refs/heads/foo/bar\" cannot\n> exist at the same time, because (at least when using the filesystem\n> backend), \"refs/heads/foo\" would have to be both a file and a directory\n> at the same time. So if one of these references already exists, it would\n> be an error to create a reflog for the other one.\n> \n> Similarly, if there is a reflog file for one of these references that\n> was created by this command, but there is not yet a corresponding\n> reference, then any command that later tries to create the other\n> reference will not be able to create the reflog for *that* reference\n> because it will conflict with the existing reflog. I doubt that the\n> reference creation is smart enough to deal with this situation.\n> \n> So all in all, I think it is unwise to allow a reflog to be created\n> without its corresponding reference.\n> \n> This, in turn, suggests one or both of the following alternatives:\n> \n> 1. Allow \"git reflog create\", but only for references that already exist.\n\nThis turns out not to work for git stash, which wants to create a reflog\nfor stash creation.\n\n> 2. If we want to allow a reflog to be created at the same time as the\n> corresponding reference, the reference-creation commands (\"git branch\",\n> \"git tag\", \"git update-ref\", and maybe some others) probably need a new\n> option like \"--create-reflog\" (and, for symmetry, probably\n> \"--no-create-reflog\").\n\ngit branch should already autocreate reflogs, since the refs it creates\nare under refs/heads.\n\n> At the API level, it might make sense for the ref-transaction functions\n> to get a new \"REF_FORCE_CREATE_REFLOG\" flag or something.\n\nJunio was opposed to the converse flag, so I'm going to just add\nmanually add code to create reflogs.\n"},{"id":"265835","messageId":"559D0281.6040908@alum.mit.edu","threadId":"39747","inReplyTo":"1436308882.5521.15.camel@twopensource.com","subject":"Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-08T10:59:13Z","receivedAt":"2015-07-08T10:59:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/08/2015 12:41 AM, David Turner wrote:\n> On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote:\n>> On 06/29/2015 10:17 PM, David Turner wrote:\n>>> [...]\n>>> @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,\n>>>  \t\t\t\t\t      head_sha1, &head_flag);\n>>>  \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n>>>  \t\t    !strcmp(head_ref, lock->ref_name))\n>>> -\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg);\n>>> +\t\t\tlog_ref_write(\"HEAD\", lock->old_oid.hash, sha1, logmsg,\n>>> +\t\t\t\t      err);\n>>\n>> Here you don't check for errors from log_ref_write(). So it might or\n>> might not have written something to err. If it has, is that error\n>> handled correctly?\n> \n> That was the case before this patch too. But I guess I might as well\n> check.\n\nThis isn't about fixing a pre-existing bug. Your patch introduced a\nregression.\n\nBefore this patch, commit_ref_update() didn't really need to know\nwhether log_ref_write() failed, because it didn't take any further\nremedial action anyway. log_ref_write() would have already written an\nerror message to stderr, and that was all the error handling that was\nneeded.\n\nBut now, log_ref_write() *doesn't* write its error message to stderr; it\nonly stores it to `err`. So now it is the caller's responsibility to\ncheck whether there was an error, and if so write `err` to stderr.\nOtherwise the error message will get lost entirely, I think.\n\nI think your v7 of this patch goes too far, by turning a failure to\nwrite to the reflog into a failure of the whole transaction. The problem\nis that this failure comes too late, in the commit phase of the\ntransaction. Aborting at this late stage can leave some references\nchanged and others rolled back, violating the promise of atomicity.\n\nThe old code reported a failure to write the reflog to stderr but didn't\nfail the transaction. I think that behavior is more appropriate. The\nreflog is of lower importance than the references themselves. Junio, do\nyou agree?\n\nIf so, it might be that adding err arguments to the reflog functions\ndoes not bring any benefits.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265837","messageId":"559D03AC.4060706@alum.mit.edu","threadId":"39747","inReplyTo":"1436311118.5521.16.camel@twopensource.com","subject":"Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-08T11:04:12Z","receivedAt":"2015-07-08T11:04:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/08/2015 01:18 AM, David Turner wrote:\n> On Mon, 2015-07-06 at 18:21 +0200, Michael Haggerty wrote:\n> \n> <snip> changes applied; will re-roll.\n> \n>>> +\n>>> +int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)\n>>> +{\n>>> +\tint ret;\n>>> +\tstruct strbuf sb = STRBUF_INIT;\n>>> +\n>>> +\tret = log_ref_setup(refname, &sb, err, force_create);\n>>> +\tstrbuf_release(&sb);\n>>> +\treturn ret;\n>>> +}\n>>> +\n>>\n>> Is it really necessary to have two functions, safe_create_reflog() and\n>> log_ref_setup()? I don't see any of the callers doing anything special\n>> with the sb_logfile argument from the latter, so maybe it could be\n>> inlined into safe_create_reflog()? Maybe I'm overlooking something.\n> \n> log_ref_write_1 does use the sb_logfile argument.\n\nThanks for the clarification. I *did* overlook something.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265849","messageId":"559D22A1.9030100@alum.mit.edu","threadId":"39747","inReplyTo":"1436316577.5521.25.camel@twopensource.com","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-08T13:16:17Z","receivedAt":"2015-07-08T13:16:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/08/2015 02:49 AM, David Turner wrote:\n> On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:\n>> [...]\n>> So all in all, I think it is unwise to allow a reflog to be created\n>> without its corresponding reference.\n>>\n>> This, in turn, suggests one or both of the following alternatives:\n>>\n>> 1. Allow \"git reflog create\", but only for references that already exist.\n> \n> This turns out not to work for git stash, which wants to create a reflog\n> for stash creation.\n> \n>> 2. If we want to allow a reflog to be created at the same time as the\n>> corresponding reference, the reference-creation commands (\"git branch\",\n>> \"git tag\", \"git update-ref\", and maybe some others) probably need a new\n>> option like \"--create-reflog\" (and, for symmetry, probably\n>> \"--no-create-reflog\").\n> \n> git branch should already autocreate reflogs, since the refs it creates\n> are under refs/heads.\n\n`git branch` only autocreates reflogs if core.logAllRefUpdates is on.\nThat setting happens to be on by default in a non-bare repository but\nthe user might turn it off. And it is off by default in a bare repository.\n\nIn my opinion it would be nice for the user to be able to ask for a\nreflog to be created for a branch regardless of how\ncore.logAllRefUpdates is set. Though I'm not saying that you have to be\nthe one to implement that functionality :-)\n\n>> At the API level, it might make sense for the ref-transaction functions\n>> to get a new \"REF_FORCE_CREATE_REFLOG\" flag or something.\n> \n> Junio was opposed to the converse flag, so I'm going to just add\n> manually add code to create reflogs.\n\nUnfortunately I wasn't keeping up with earlier versions of this patch\nseries and now I can't find the email from Junio that you are referring\nto. If the earlier flag had the opposite (\"converse\"?) sense, like\nREF_INHIBIT_CREATE_REFLOG, then I agree that it wouldn't be an improvement.\n\nBut I think this functionality *has to* be implemented within ref\ntransactions for references that are just being created, because\n\n1. The reflog must *not* be created if the reference creation fails for\nsome reason. For example, the reflog shouldn't be created if the\nreference name has a D/F conflict with an existing one in the \"refs/foo\"\nvs. \"refs/foo/bar\" sense. (This conflict might not be obvious when\ncreating the reflog file because the other reference might not have its\nreflog turned on.) There are other reasons that a reference creation\nmight fail, and code outside of the refs API can't be expected to know\nall possibilities.\n\n2. On the other hand, the reflog for a newly-created reference *should*\nreflect the creation of the reference. So it would be awkward to require\nthe calling code to create the reference and *then* turn on the reflog.\n\nFor references that already exist, I see no problem with a command that\nturns on the reflog without adding any entries to it. Though if you\nimplement this, it would be prudent to check that existing\nreflog-handling code doesn't fail when confronted with an empty file; I\nthink empty reflog files are rare now and might not be well-tested.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265861","messageId":"xmqqr3oiy3pm.fsf@gitster.dls.corp.google.com","threadId":"39747","inReplyTo":"559D0281.6040908@alum.mit.edu","subject":"Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-08T17:11:17Z","receivedAt":"2015-07-08T17:11:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I think your v7 of this patch goes too far, by turning a failure to\n> write to the reflog into a failure of the whole transaction. The problem\n> is that this failure comes too late, in the commit phase of the\n> transaction. Aborting at this late stage can leave some references\n> changed and others rolled back, violating the promise of atomicity.\n\nYeah, that sounds problematic.\n\n> The old code reported a failure to write the reflog to stderr but didn't\n> fail the transaction. I think that behavior is more appropriate. The\n> reflog is of lower importance than the references themselves. Junio, do\n> you agree?\n\nThat is actually a loaded question.\n\nDo I agree that the current (i.e. before this change) behaviour is\nmore appropriate given the current choice of representation of refs\nand reflogs on the filesystem, treating a failure to update reflog\nas lower importance event and accept it as a limitation that it\ncannot abort the whole transaction atomically?  Compared to leaving\nthe repository in a half-updated state where some refs and their\nlogs are updated already, other remaining proposed updates are\nignored, and the transaction claims to have failed even though some\nthings have already changed and we cannot rollback, I would say that\nis a better compromise to treat reflog update as a lower importance.\n\nDo I agree that reflog writing should stay to be best-effort in the\nlonger term?  Not really.  If we are moving the ref API in the\ndirection where we can plug a backend that is different from the\ntraditional filesystem based one for storing refs, I think the\nbackend should also be responsible for storing logs for the refs it\nstores, and if the backend wants to promise atomicity, then we\nshould be able to fail the whole transaction when updates to refs\ncould proceed but updates to the log of one of these updated refs\ncannot.  So I do not agree to cast in stone \"the reflog is of lower\nimportance\" as a general rule.\n"},{"id":"265872","messageId":"1436386343.4542.14.camel@twopensource.com","threadId":"39747","inReplyTo":"559D22A1.9030100@alum.mit.edu","subject":"Re: [PATCH v6 6/7] git-reflog: add create and exists functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T20:12:23Z","receivedAt":"2015-07-08T20:12:23Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-08 at 15:16 +0200, Michael Haggerty wrote:\n> On 07/08/2015 02:49 AM, David Turner wrote:\n> > On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:\n> >> [...]\n> >> So all in all, I think it is unwise to allow a reflog to be created\n> >> without its corresponding reference.\n> >>\n> >> This, in turn, suggests one or both of the following alternatives:\n> >>\n> >> 1. Allow \"git reflog create\", but only for references that already exist.\n> > \n> > This turns out not to work for git stash, which wants to create a reflog\n> > for stash creation.\n> > \n> >> 2. If we want to allow a reflog to be created at the same time as the\n> >> corresponding reference, the reference-creation commands (\"git branch\",\n> >> \"git tag\", \"git update-ref\", and maybe some others) probably need a new\n> >> option like \"--create-reflog\" (and, for symmetry, probably\n> >> \"--no-create-reflog\").\n> > \n> > git branch should already autocreate reflogs, since the refs it creates\n> > are under refs/heads.\n> \n> `git branch` only autocreates reflogs if core.logAllRefUpdates is on.\n> That setting happens to be on by default in a non-bare repository but\n> the user might turn it off. And it is off by default in a bare repository.\n\nOh, right.  Well, we can always add that later, if anyone needs it.\n\n> In my opinion it would be nice for the user to be able to ask for a\n> reflog to be created for a branch regardless of how\n> core.logAllRefUpdates is set. Though I'm not saying that you have to be\n> the one to implement that functionality :-)\n> \n> >> At the API level, it might make sense for the ref-transaction functions\n> >> to get a new \"REF_FORCE_CREATE_REFLOG\" flag or something.\n> > \n> > Junio was opposed to the converse flag, so I'm going to just add\n> > manually add code to create reflogs.\n> \n> Unfortunately I wasn't keeping up with earlier versions of this patch\n> series and now I can't find the email from Junio that you are referring\n> to. If the earlier flag had the opposite (\"converse\"?) sense, like\n> REF_INHIBIT_CREATE_REFLOG, then I agree that it wouldn't be an improvement.\n> \n> But I think this functionality *has to* be implemented within ref\n> transactions for references that are just being created, because\n> \n> 1. The reflog must *not* be created if the reference creation fails for\n> some reason. For example, the reflog shouldn't be created if the\n> reference name has a D/F conflict with an existing one in the \"refs/foo\"\n> vs. \"refs/foo/bar\" sense. (This conflict might not be obvious when\n> creating the reflog file because the other reference might not have its\n> reflog turned on.) There are other reasons that a reference creation\n> might fail, and code outside of the refs API can't be expected to know\n> all possibilities.\n> \n> 2. On the other hand, the reflog for a newly-created reference *should*\n> reflect the creation of the reference. So it would be awkward to require\n> the calling code to create the reference and *then* turn on the reflog.\n\nYep, makes sense.\n\n> For references that already exist, I see no problem with a command that\n> turns on the reflog without adding any entries to it. Though if you\n> implement this, it would be prudent to check that existing\n> reflog-handling code doesn't fail when confronted with an empty file; I\n> think empty reflog files are rare now and might not be well-tested.\n\nI think empty reflog files are fine; I recall a few tests creating them\n(and git stash did so as well).\n\nBut I also don't need that command (I think), so I won't implement it\nright now.\n"},{"id":"265895","messageId":"559E1917.6000205@alum.mit.edu","threadId":"39747","inReplyTo":"xmqqr3oiy3pm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-09T06:47:51Z","receivedAt":"2015-07-09T06:47:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/08/2015 07:11 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> I think your v7 of this patch goes too far, by turning a failure to\n>> write to the reflog into a failure of the whole transaction. The problem\n>> is that this failure comes too late, in the commit phase of the\n>> transaction. Aborting at this late stage can leave some references\n>> changed and others rolled back, violating the promise of atomicity.\n> \n> Yeah, that sounds problematic.\n> \n>> The old code reported a failure to write the reflog to stderr but didn't\n>> fail the transaction. I think that behavior is more appropriate. The\n>> reflog is of lower importance than the references themselves. Junio, do\n>> you agree?\n> \n> That is actually a loaded question.\n> \n> Do I agree that the current (i.e. before this change) behaviour is\n> more appropriate given the current choice of representation of refs\n> and reflogs on the filesystem, treating a failure to update reflog\n> as lower importance event and accept it as a limitation that it\n> cannot abort the whole transaction atomically?  Compared to leaving\n> the repository in a half-updated state where some refs and their\n> logs are updated already, other remaining proposed updates are\n> ignored, and the transaction claims to have failed even though some\n> things have already changed and we cannot rollback, I would say that\n> is a better compromise to treat reflog update as a lower importance.\n> \n> Do I agree that reflog writing should stay to be best-effort in the\n> longer term?  Not really.  If we are moving the ref API in the\n> direction where we can plug a backend that is different from the\n> traditional filesystem based one for storing refs, I think the\n> backend should also be responsible for storing logs for the refs it\n> stores, and if the backend wants to promise atomicity, then we\n> should be able to fail the whole transaction when updates to refs\n> could proceed but updates to the log of one of these updated refs\n> cannot.  So I do not agree to cast in stone \"the reflog is of lower\n> importance\" as a general rule.\n\nJunio,\n\nYou make a good distinction. I was describing a compromise that we have\nto make now due to the limitations of the current ref/reflog backend.\nBut I agree 100% that a future storage backend that can do correct\nrollback of refs *and* reflogs can fail a transaction if the reflog\nupdates cannot be committed.\n\nThanks,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}