{"thread":{"id":"39808","subject":"[PATCH v7 1/8] refs.c: add err arguments to reflog functions","startedAt":"2015-07-08T00:55:56Z","lastAt":"2015-07-15T18:04:36Z","messageCount":30,"participants":["David Turner","Junio C Hamano","Michael Haggerty","Johannes Sixt"],"isPatch":true,"patchVersion":7,"patchTotal":8},"messages":[{"id":"265789","messageId":"1436316963-25520-1-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":null,"subject":"[PATCH v7 1/8] refs.c: add err arguments to reflog functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:55:56Z","receivedAt":"2015-07-08T00:55:56Z","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\nSome error messages change slightly.  For instance, we sometimes lose\n\"cannot update ref\" and instead only show the specific cause of ref\nupdate failure.\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             | 130 +++++++++++++++++++++++++++++------------------------\n refs.h             |   4 +-\n 3 files changed, 79 insertions(+), 63 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..e891bed 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\tstrbuf_addf(err, \"Unable to append to %s. %s\", log_file,\n+\t\t\t    strerror(errno));\n \t\tclose(logfd);\n-\t\terror(\"Unable to append to %s\", log_file);\n-\t\terrno = save_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,14 +3290,20 @@ 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+\tint result = 0;\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\tunlock_ref(lock);\n-\t\treturn -1;\n+\t     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {\n+\t\tchar *old_msg = strbuf_detach(err, NULL);\n+\t\tstrbuf_addf(err, \"Cannot update the ref '%s': '%s'\",\n+\t\t\t    lock->ref_name, old_msg);\n+\t\tfree(old_msg);\n+\t\tresult = -1;\n+\t\tgoto done;\n \t}\n \tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n \t\t/*\n@@ -3316,16 +3324,21 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\thead_ref = resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING,\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    !strcmp(head_ref, lock->ref_name)) {\n+\t\t\tif (log_ref_write(\"HEAD\", lock->old_oid.hash, sha1,\n+\t\t\t\t\t  logmsg, err)) {\n+\t\t\t\tresult = -1;\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t}\n \t}\n \tif (commit_ref(lock)) {\n \t\terror(\"Couldn't set %s\", lock->ref_name);\n-\t\tunlock_ref(lock);\n-\t\treturn -1;\n+\t\tresult = -1;\n \t}\n+done:\n \tunlock_ref(lock);\n-\treturn 0;\n+\treturn result;\n }\n \n int create_symref(const char *ref_target, const char *refs_heads_master,\n@@ -3336,6 +3349,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 +3398,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 +4038,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 +4070,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.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265791","messageId":"1436316963-25520-2-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:55:57Z","receivedAt":"2015-07-08T00:55:57Z","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                      | 31 ++++++++++---------------------\n t/t7509-commit.sh                |  4 ++--\n wt-status.c                      |  6 ++----\n 8 files changed, 23 insertions(+), 36 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..90396ba 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -158,21 +158,10 @@ 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-\n-\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\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+\tupdate_ref(NULL, ref, commit->object.sha1, NULL, REF_NODEREF,\n+\t\t   UPDATE_REFS_DIE_ON_ERR);\n }\n \n static void print_advice(int show_hint, struct replay_opts *opts)\n@@ -186,7 +175,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 +867,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 +1003,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 +1019,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.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265790","messageId":"1436316963-25520-3-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v7 3/8] bisect: treat BISECT_HEAD as a ref","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:55:58Z","receivedAt":"2015-07-08T00:55:58Z","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..2fd8ea6 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+\tgit 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.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265792","messageId":"1436316963-25520-4-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v7 4/8] refs: Break out check for reflog autocreation","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:55:59Z","receivedAt":"2015-07-08T00:55:59Z","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 e891bed..e694359 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.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265793","messageId":"1436316963-25520-5-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v7 5/8] refs: new public ref function: safe_create_reflog","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:56:00Z","receivedAt":"2015-07-08T00:56:00Z","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 | 16 +++++-----------\n refs.c             | 25 +++++++++++++++++++++----\n refs.h             |  2 +-\n 3 files changed, 27 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 93f63d3..c840f33 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -620,24 +620,18 @@ 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\tchar *refname;\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\trefname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n+\t\t\t\tif (safe_create_reflog(refname, &err, 1)) {\n+\t\t\t\t\tfree(refname);\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\tfree(refname);\n \t\t\t}\n \t\t}\n \t\telse\ndiff --git a/refs.c b/refs.c\nindex e694359..01b0af5 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+ * Create a reflog for a ref.  If force_create = 0, the reflog will\n+ * 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\n+ * *err 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.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265794","messageId":"1436316963-25520-6-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v7 6/8] git-reflog: add exists command","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:56:01Z","receivedAt":"2015-07-08T00:56:01Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Theis 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\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n Documentation/git-reflog.txt |  4 ++++\n builtin/reflog.c             | 33 ++++++++++++++++++++++++++++++++-\n t/t1411-reflog-show.sh       |  5 +++++\n 3 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex 5e7908e..4b08fc7 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -23,6 +23,7 @@ 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 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 +53,9 @@ 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 \"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..7ed0e85 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -13,6 +13,8 @@ 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_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 +701,38 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\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 | exists ]\";\n \n int cmd_reflog(int argc, const char **argv, const char *prefix)\n {\n@@ -724,5 +752,8 @@ 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], \"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..3eb4f10 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -166,4 +166,9 @@ 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_done\n-- \n2.0.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265796","messageId":"1436316963-25520-7-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v7 7/8] update-ref and tag: add --create-reflog arg","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:56:02Z","receivedAt":"2015-07-08T00:56:02Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Allow the creation of a ref (e.g. stash) with a reflog already in\nplace. For most refs (e.g. those under refs/heads), this happens\nautomatically, but for others, we need this option.\n\nCurrently, git does this by pre-creating the reflog, but alternate ref\nbackends might store reflogs somewhere other than .git/logs.  Code\nthat now directly manipulates .git/logs should instead use git\nplumbing commands.\n\nI also added --create-reflog to git tag, just for completeness.\n\nIn a moment, we will use this argument to make git stash work with\nalternate ref backends.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n Documentation/git-tag.txt        |  5 ++++-\n Documentation/git-update-ref.txt |  5 ++++-\n builtin/tag.c                    |  5 +++++\n builtin/update-ref.c             | 17 +++++++++++++++--\n t/t1400-update-ref.sh            | 38 ++++++++++++++++++++++++++++++++++++++\n t/t7004-tag.sh                   |  9 ++++++++-\n 6 files changed, 74 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 034d10d..2312980 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n-\t[--column[=<options>] | --no-column] [<pattern>...]\n+\t[--column[=<options>] | --no-column] [--create-reflog] [<pattern>...]\n \t[<pattern>...]\n 'git tag' -v <tagname>...\n \n@@ -143,6 +143,9 @@ This option is only applicable when listing tags without annotation lines.\n \tall, 'whitespace' removes just leading/trailing whitespace lines and\n \t'strip' removes both whitespace and commentary.\n \n+--create-reflog::\n+\tCreate a reflog for the tag.\n+\n <tagname>::\n \tThe name of the tag to create, delete, or describe.\n \tThe new tag name must pass all checks defined by\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex c8f5ae5..969bfab 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely\n SYNOPSIS\n --------\n [verse]\n-'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] <ref> <newvalue> [<oldvalue>] | --stdin [-z])\n+'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] [--create-reflog] <ref> <newvalue> [<oldvalue>] | --stdin [-z])\n \n DESCRIPTION\n -----------\n@@ -67,6 +67,9 @@ performs all modifications together.  Specify commands of the form:\n \tverify SP <ref> [SP <oldvalue>] LF\n \toption SP <opt> LF\n \n+With `--create-reflog`, update-ref will create a reflog for each ref\n+even if one would not ordinarily be created.\n+\n Quote fields containing whitespace as if they were strings in C source\n code; i.e., surrounded by double-quotes and with backslash escapes.\n Use 40 \"0\" characters or the empty string to specify a zero value.  To\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5f6cdc5..896f434 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -579,6 +579,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n \tint annotate = 0, force = 0, lines = -1;\n+\tint create_reflog = 0;\n \tint cmdmode = 0;\n \tconst char *msgfile = NULL, *keyid = NULL;\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n@@ -605,6 +606,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING('u', \"local-user\", &keyid, N_(\"key-id\"),\n \t\t\t\t\tN_(\"use another key to sign the tag\")),\n \t\tOPT__FORCE(&force, N_(\"replace the tag if exists\")),\n+\t\tOPT_BOOL(0, \"create-reflog\", &create_reflog, N_(\"create_reflog\")),\n \n \t\tOPT_GROUP(N_(\"Tag listing options\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n@@ -730,6 +732,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (annotate)\n \t\tcreate_tag(object, tag, &buf, &opt, prev, object);\n \n+\tif (create_reflog && safe_create_reflog(ref.buf, &err, 1))\n+\t\tdie(\"failed to create reflog for %s: %s\", ref.buf, err.buf);\n+\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, ref.buf, object, prev,\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 6763cf1..d570e5e 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -14,6 +14,7 @@ static const char * const git_update_ref_usage[] = {\n \n static char line_termination = '\\n';\n static int update_flags;\n+int create_reflog;\n static const char *msg;\n \n /*\n@@ -198,6 +199,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"update %s: extra input: %s\", refname, next);\n \n+\tif (create_reflog && safe_create_reflog(refname, &err, 1))\n+\t\tdie(\"failed to create reflog for %s: %s\", refname, err.buf);\n+\n \tif (ref_transaction_update(transaction, refname,\n \t\t\t\t   new_sha1, have_old ? old_sha1 : NULL,\n \t\t\t\t   update_flags, msg, &err))\n@@ -230,6 +234,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"create %s: extra input: %s\", refname, next);\n \n+\tif (create_reflog && safe_create_reflog(refname, &err, 1))\n+\t\tdie(\"failed to create reflog for %s: %s\", refname, err.buf);\n+\n \tif (ref_transaction_create(transaction, refname, new_sha1,\n \t\t\t\t   update_flags, msg, &err))\n \t\tdie(\"%s\", err.buf);\n@@ -361,6 +368,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tN_(\"update <refname> not the one it points to\")),\n \t\tOPT_BOOL('z', NULL, &end_null, N_(\"stdin has NUL-terminated arguments\")),\n \t\tOPT_BOOL( 0 , \"stdin\", &read_stdin, N_(\"read updates from stdin\")),\n+\t\tOPT_BOOL( 0 , \"create-reflog\", &create_reflog, N_(\"create_reflog\")),\n \t\tOPT_END(),\n \t};\n \n@@ -421,7 +429,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \n \tif (no_deref)\n \t\tflags = REF_NODEREF;\n-\tif (delete)\n+\tif (delete) {\n \t\t/*\n \t\t * For purposes of backwards compatibility, we treat\n \t\t * NULL_SHA1 as \"don't care\" here:\n@@ -429,7 +437,12 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\treturn delete_ref(refname,\n \t\t\t\t  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,\n \t\t\t\t  flags);\n-\telse\n+\t} else {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tif (create_reflog && safe_create_reflog(refname, &err, 1))\n+\t\t\tdie(\"failed to create reflog for %s: %s\", refname, err.buf);\n+\n \t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n \t\t\t\t  flags, UPDATE_REFS_DIE_ON_ERR);\n+\t}\n }\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d787bf5..9d21c19 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -23,6 +23,7 @@ test_expect_success setup '\n m=refs/heads/master\n n_dir=refs/heads/gu\n n=$n_dir/fixes\n+outside=foo\n \n test_expect_success \\\n \t\"create $m\" \\\n@@ -74,6 +75,24 @@ test_expect_success \"delete $m (by HEAD)\" '\n '\n rm -f .git/$m\n \n+test_expect_success 'update-ref does not create reflogs by default' '\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n+test_expect_success 'update-ref creates reflogs with --create-reflog' '\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref --create-reflog $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\tgit reflog exists $outside\n+'\n+\n test_expect_success \\\n \t\"create $m (by HEAD)\" \\\n \t\"git update-ref HEAD $A &&\n@@ -472,6 +491,25 @@ test_expect_success 'stdin create ref works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stdin does not create reflogs by default' '\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\techo \"create $outside $m\" >stdin &&\n+\tgit update-ref --stdin <stdin &&\n+\tgit rev-parse $m >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n+test_expect_success 'stdin creates reflogs with --create-reflog' '\n+\techo \"create $outside $m\" >stdin &&\n+\tgit update-ref --create-reflog --stdin <stdin &&\n+\tgit rev-parse $m >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\tgit reflog exists $outside\n+'\n+\n test_expect_success 'stdin succeeds with quoted argument' '\n \tgit update-ref -d $a &&\n \techo \"create $a \\\"$m\\\"\" >stdin &&\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex d1ff5c9..426d285 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -51,7 +51,14 @@ test_expect_success 'creating a tag using default HEAD should succeed' '\n \techo foo >foo &&\n \tgit add foo &&\n \tgit commit -m Foo &&\n-\tgit tag mytag\n+\tgit tag mytag &&\n+\ttest_must_fail git reflog exists refs/tags/mytag\n+'\n+\n+test_expect_success 'creating a tag with --create-reflog should create reflog' '\n+\ttest_when_finished \"git tag -d tag_with_reflog\" &&\n+\tgit tag --create-reflog tag_with_reflog &&\n+\tgit reflog exists refs/tags/tag_with_reflog\n '\n \n test_expect_success 'listing all tags if one exists should succeed' '\n-- \n2.0.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265795","messageId":"1436316963-25520-8-git-send-email-dturner@twopensource.com","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"[PATCH v7 8/8] git-stash: use update-ref --create-reflog instead of creating files","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T00:56:03Z","receivedAt":"2015-07-08T00:56:03Z","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 | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 8e9e2cd..1d5ba7a 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -183,9 +183,7 @@ store_stash () {\n \t\tstash_msg=\"Created via \\\"git stash store\\\".\"\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 update-ref -m \"$stash_msg\" $ref_stash $w_commit\n+\tgit update-ref --create-reflog -m \"$stash_msg\" $ref_stash $w_commit\n \tret=$?\n \ttest $ret != 0 && test -z $quiet &&\n \tdie \"$(eval_gettext \"Cannot update \\$ref_stash with \\$w_commit\")\"\n@@ -262,7 +260,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\n-- \n2.0.5.499.g01f6352.dirty-twtrsrc\n"},{"id":"265810","messageId":"xmqqoajnyuom.fsf@gitster.dls.corp.google.com","threadId":"39808","inReplyTo":"1436316963-25520-8-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v7 8/8] git-stash: use update-ref --create-reflog instead of creating files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-08T07:28:41Z","receivedAt":"2015-07-08T07:28:41Z","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> This is in support of alternate ref backends which don't necessarily\n> store reflogs as files.\n>\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n\nThanks.  The last round was already a pleasant read, and I didn't\nspot anything questionable in the interdiff between the last one and\nthis round (although I admit that I maybe a lot less careful at this\ntime of the day ;-).\n\nMichael?\n"},{"id":"265813","messageId":"xmqqio9vyugx.fsf@gitster.dls.corp.google.com","threadId":"39808","inReplyTo":"1436316963-25520-8-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v7 8/8] git-stash: use update-ref --create-reflog instead of creating files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-08T07:33:18Z","receivedAt":"2015-07-08T07:33:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By the way, a series with 8 patches, especially that needed more\nthan a couple of rerolls, deserve a separate cover-letter, if only\nto describe what was changed since the previous rounds and pointers\nto them to help reviewers.\n\nThanks.\n"},{"id":"265839","messageId":"559D0B42.1040607@alum.mit.edu","threadId":"39808","inReplyTo":"1436316963-25520-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v7 1/8] refs.c: add err arguments to reflog functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-08T11:36:34Z","receivedAt":"2015-07-08T11:36:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/08/2015 02:55 AM, 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> Some error messages change slightly.  For instance, we sometimes lose\n> \"cannot update ref\" and instead only show the specific cause of ref\n> update failure.\n\nDid you check that the new error messages are at least as clear to the\nuser as the old ones? (Sometimes errors from deep in the call stack\nprovide details but don't make it clear how the details connect back to\nthe action that the user was trying to do.)\n\n> Update of a patch by Ronnie Sahlberg.\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             | 130 +++++++++++++++++++++++++++++------------------------\n>  refs.h             |   4 +-\n>  3 files changed, 79 insertions(+), 63 deletions(-)\n> \n> [...]\n> diff --git a/refs.c b/refs.c\n> index fb568d7..e891bed 100644\n> --- a/refs.c\n> +++ b/refs.c\n> [...]\n> @@ -3288,14 +3290,20 @@ 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> +\tint result = 0;\n\nPlease put a blank line between local variable definitions and the start\nof code.\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\tunlock_ref(lock);\n> -\t\treturn -1;\n> +\t     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {\n> +\t\tchar *old_msg = strbuf_detach(err, NULL);\n> +\t\tstrbuf_addf(err, \"Cannot update the ref '%s': '%s'\",\n> +\t\t\t    lock->ref_name, old_msg);\n> +\t\tfree(old_msg);\n> +\t\tresult = -1;\n> +\t\tgoto done;\n>  \t}\n\nI see this code already did what I complained about in my earlier email\n[*]: fail a reference transaction after it is already partly committed.\nIn my opinion this is incorrect for the reasons stated there. But you\ndon't have to consider it to be in the scope of your patch series.\n\n[*] http://article.gmane.org/gmane.comp.version-control.git/273668\n\n>  \tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n>  \t\t/*\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265840","messageId":"559D0E65.6080403@alum.mit.edu","threadId":"39808","inReplyTo":"1436316963-25520-5-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v7 5/8] refs: new public ref function: safe_create_reflog","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-08T11:49:57Z","receivedAt":"2015-07-08T11:49:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/08/2015 02:56 AM, 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 | 16 +++++-----------\n>  refs.c             | 25 +++++++++++++++++++++----\n>  refs.h             |  2 +-\n>  3 files changed, 27 insertions(+), 16 deletions(-)\n> \n> [...]\n> diff --git a/refs.c b/refs.c\n> index e694359..01b0af5 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> + * Create a reflog for a ref.  If force_create = 0, the reflog will\n> + * 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\n> + * *err 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\nSorry for being unclear in the last round. I meant that the whole\ndocstring should be in the imperative voice, like\n\n/*\n * Create a reflog for a ref. If force_create = 0, only create the\n * reflog for certain refs (those for which should_autocreate_reflog\n * returns non-zero). Otherwise, create it regardless of the reference\n * name. Fill in *err and return -1 on failure.\n */\n\nOtherwise, this patch looks good.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265850","messageId":"559D293A.9040307@alum.mit.edu","threadId":"39808","inReplyTo":"1436316963-25520-7-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v7 7/8] update-ref and tag: add --create-reflog arg","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-08T13:44:26Z","receivedAt":"2015-07-08T13:44:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Please see my email about v6 [*] for an argument for why, at the API\nlevel, the create-reflog functionality for new references needs to be\nimplemented within the ref_transaction API.\n\n[*] http://article.gmane.org/gmane.comp.version-control.git/273682\n\nOn 07/08/2015 02:56 AM, David Turner wrote:\n> update-ref and tag: add --create-reflog arg\n\n`--create-reflog` is an \"option\", not an \"arg\".\n\n> Allow the creation of a ref (e.g. stash) with a reflog already in\n> place. For most refs (e.g. those under refs/heads), this happens\n> automatically, but for others, we need this option.\n\nAs I mentioned, this fact is only true if core.logAllRefUpdates is\nturned on (or by default in non-bare repos).\n\n> Currently, git does this by pre-creating the reflog, but alternate ref\n> backends might store reflogs somewhere other than .git/logs.  Code\n> that now directly manipulates .git/logs should instead use git\n> plumbing commands.\n> \n> I also added --create-reflog to git tag, just for completeness.\n> \n> In a moment, we will use this argument to make git stash work with\n> alternate ref backends.\n\nFor a moment I thought that there should be a corresponding\n`--no-create-reflog` option. But it would be pretty pointless, because\nif this call to `update-ref` would want to create a reflog, then so\nwould *every* subsequent update to the reference. So the effect of\n`--no-create-reflog` could at best be temporary.\n\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n>  Documentation/git-tag.txt        |  5 ++++-\n>  Documentation/git-update-ref.txt |  5 ++++-\n>  builtin/tag.c                    |  5 +++++\n>  builtin/update-ref.c             | 17 +++++++++++++++--\n>  t/t1400-update-ref.sh            | 38 ++++++++++++++++++++++++++++++++++++++\n>  t/t7004-tag.sh                   |  9 ++++++++-\n>  6 files changed, 74 insertions(+), 5 deletions(-)\n> \n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index 034d10d..2312980 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -13,7 +13,7 @@ SYNOPSIS\n>  \t<tagname> [<commit> | <object>]\n>  'git tag' -d <tagname>...\n>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n> -\t[--column[=<options>] | --no-column] [<pattern>...]\n> +\t[--column[=<options>] | --no-column] [--create-reflog] [<pattern>...]\n>  \t[<pattern>...]\n>  'git tag' -v <tagname>...\n>  \n> @@ -143,6 +143,9 @@ This option is only applicable when listing tags without annotation lines.\n>  \tall, 'whitespace' removes just leading/trailing whitespace lines and\n>  \t'strip' removes both whitespace and commentary.\n>  \n> +--create-reflog::\n> +\tCreate a reflog for the tag.\n> +\n>  <tagname>::\n>  \tThe name of the tag to create, delete, or describe.\n>  \tThe new tag name must pass all checks defined by\n> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\n> index c8f5ae5..969bfab 100644\n> --- a/Documentation/git-update-ref.txt\n> +++ b/Documentation/git-update-ref.txt\n> @@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] <ref> <newvalue> [<oldvalue>] | --stdin [-z])\n> +'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] [--create-reflog] <ref> <newvalue> [<oldvalue>] | --stdin [-z])\n>  \n>  DESCRIPTION\n>  -----------\n> @@ -67,6 +67,9 @@ performs all modifications together.  Specify commands of the form:\n>  \tverify SP <ref> [SP <oldvalue>] LF\n>  \toption SP <opt> LF\n>  \n> +With `--create-reflog`, update-ref will create a reflog for each ref\n> +even if one would not ordinarily be created.\n> +\n>  Quote fields containing whitespace as if they were strings in C source\n>  code; i.e., surrounded by double-quotes and with backslash escapes.\n>  Use 40 \"0\" characters or the empty string to specify a zero value.  To\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 5f6cdc5..896f434 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -579,6 +579,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \tstruct create_tag_options opt;\n>  \tchar *cleanup_arg = NULL;\n>  \tint annotate = 0, force = 0, lines = -1;\n> +\tint create_reflog = 0;\n>  \tint cmdmode = 0;\n>  \tconst char *msgfile = NULL, *keyid = NULL;\n>  \tstruct msg_arg msg = { 0, STRBUF_INIT };\n> @@ -605,6 +606,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_STRING('u', \"local-user\", &keyid, N_(\"key-id\"),\n>  \t\t\t\t\tN_(\"use another key to sign the tag\")),\n>  \t\tOPT__FORCE(&force, N_(\"replace the tag if exists\")),\n> +\t\tOPT_BOOL(0, \"create-reflog\", &create_reflog, N_(\"create_reflog\")),\n>  \n>  \t\tOPT_GROUP(N_(\"Tag listing options\")),\n>  \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n> @@ -730,6 +732,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \tif (annotate)\n>  \t\tcreate_tag(object, tag, &buf, &opt, prev, object);\n>  \n> +\tif (create_reflog && safe_create_reflog(ref.buf, &err, 1))\n> +\t\tdie(\"failed to create reflog for %s: %s\", ref.buf, err.buf);\n> +\n\nThis reflog should not be left around if the tag creation ultimately\nfails for any reason.\n\n>  \ttransaction = ref_transaction_begin(&err);\n>  \tif (!transaction ||\n>  \t    ref_transaction_update(transaction, ref.buf, object, prev,\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 6763cf1..d570e5e 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -14,6 +14,7 @@ static const char * const git_update_ref_usage[] = {\n>  \n>  static char line_termination = '\\n';\n>  static int update_flags;\n> +int create_reflog;\n>  static const char *msg;\n>  \n>  /*\n> @@ -198,6 +199,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n>  \tif (*next != line_termination)\n>  \t\tdie(\"update %s: extra input: %s\", refname, next);\n>  \n> +\tif (create_reflog && safe_create_reflog(refname, &err, 1))\n> +\t\tdie(\"failed to create reflog for %s: %s\", refname, err.buf);\n> +\n\nIt is valid to pass the empty string or 0{40} to the `update` command as\nthe \"new\" value, in which case the reference will be deleted. In that\ncase, no reflog should be created.\n\n>  \tif (ref_transaction_update(transaction, refname,\n>  \t\t\t\t   new_sha1, have_old ? old_sha1 : NULL,\n>  \t\t\t\t   update_flags, msg, &err))\n> @@ -230,6 +234,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n>  \tif (*next != line_termination)\n>  \t\tdie(\"create %s: extra input: %s\", refname, next);\n>  \n> +\tif (create_reflog && safe_create_reflog(refname, &err, 1))\n> +\t\tdie(\"failed to create reflog for %s: %s\", refname, err.buf);\n> +\n>  \tif (ref_transaction_create(transaction, refname, new_sha1,\n>  \t\t\t\t   update_flags, msg, &err))\n>  \t\tdie(\"%s\", err.buf);\n\nShould the \"verify\" command also create a reflog, at least if the\nreference is not being verified to be missing?\n\n> @@ -361,6 +368,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\tN_(\"update <refname> not the one it points to\")),\n>  \t\tOPT_BOOL('z', NULL, &end_null, N_(\"stdin has NUL-terminated arguments\")),\n>  \t\tOPT_BOOL( 0 , \"stdin\", &read_stdin, N_(\"read updates from stdin\")),\n> +\t\tOPT_BOOL( 0 , \"create-reflog\", &create_reflog, N_(\"create_reflog\")),\n>  \t\tOPT_END(),\n>  \t};\n>  \n> @@ -421,7 +429,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (no_deref)\n>  \t\tflags = REF_NODEREF;\n> -\tif (delete)\n> +\tif (delete) {\n>  \t\t/*\n>  \t\t * For purposes of backwards compatibility, we treat\n>  \t\t * NULL_SHA1 as \"don't care\" here:\n> @@ -429,7 +437,12 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n>  \t\treturn delete_ref(refname,\n>  \t\t\t\t  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,\n>  \t\t\t\t  flags);\n> -\telse\n> +\t} else {\n> +\t\tstruct strbuf err = STRBUF_INIT;\n> +\t\tif (create_reflog && safe_create_reflog(refname, &err, 1))\n> +\t\t\tdie(\"failed to create reflog for %s: %s\", refname, err.buf);\n\nThis reflog also should not be left around if the reference does not\nexist at the end of this command.\n\n> +\n>  \t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n>  \t\t\t\t  flags, UPDATE_REFS_DIE_ON_ERR);\n> +\t}\n>  }\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index d787bf5..9d21c19 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265852","messageId":"559D2A9C.80401@alum.mit.edu","threadId":"39808","inReplyTo":"1436316963-25520-8-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v7 8/8] git-stash: use update-ref --create-reflog instead of creating files","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-08T13:50:20Z","receivedAt":"2015-07-08T13:50:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I apologize for getting involved so late in the evolution of this patch\nseries. Thanks for being so patient with the back-and-forth!\n\nI'm really glad that you are working on this. It is important that this\narea gets cleaned up if we are ever to support alternate reference\nbackends. And I'm happy that we will be getting new commands and command\noptions to deal with reflogs, because that was one case were users\npreviously had to muck about within the .git directory, which is a very\nbad thing. We need to train users never to look behind the curtain!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265862","messageId":"559D6208.8090607@kdbg.org","threadId":"39808","inReplyTo":"1436316963-25520-2-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-07-08T17:46:48Z","receivedAt":"2015-07-08T17:46:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.07.2015 um 02:55 schrieb David Turner:\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...\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 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\"\n\nWe are trying very hard not to spawn any new processes in __git_ps1(). \nSo, I raise a moderate veto against this hunk.\n\n-- Hannes\n"},{"id":"265869","messageId":"1436382962.4542.8.camel@twopensource.com","threadId":"39808","inReplyTo":"559D6208.8090607@kdbg.org","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T19:16:02Z","receivedAt":"2015-07-08T19:16:02Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-08 at 19:46 +0200, Johannes Sixt wrote:\n> Am 08.07.2015 um 02:55 schrieb David Turner:\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> ...\n> > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> > index 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\"\n> \n> We are trying very hard not to spawn any new processes in __git_ps1(). \n> So, I raise a moderate veto against this hunk.\n\nDo you have an alternate suggestion about how to accomplish the same\nthing? Here are my ideas:\n\nWe could special-case CHERRY_PICK_HEAD and REVERT_HEAD to be files\nindependent of the ref backend, but that tends to complicate the\nbackends.  I think this is a mistake.\n\nWe could reduce the number from two to one by providing a new\ngit-am-status command which outputs one of \"CHERRY-PICKING\",\n\"REVERTING\", or \"\" (or maybe it would also handle rebase and am).  We\ncould also generalize it to \"git-prompt-helper\" or something by moving\nthat entire bunch of if statements inside.  This would replace calls to\n\"git describe\".\n\nBut you probably have a better idea.\n"},{"id":"265871","messageId":"1436385716.4542.11.camel@twopensource.com","threadId":"39808","inReplyTo":"559D0B42.1040607@alum.mit.edu","subject":"Re: [PATCH v7 1/8] refs.c: add err arguments to reflog functions","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T20:01:56Z","receivedAt":"2015-07-08T20:01:56Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-08 at 13:36 +0200, Michael Haggerty wrote:\n> On 07/08/2015 02:55 AM, 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> > Some error messages change slightly.  For instance, we sometimes lose\n> > \"cannot update ref\" and instead only show the specific cause of ref\n> > update failure.\n> \n> Did you check that the new error messages are at least as clear to the\n> user as the old ones? (Sometimes errors from deep in the call stack\n> provide details but don't make it clear how the details connect back to\n> the action that the user was trying to do.)\n\nThey seem clear enough to me, but I guess there's no reason not to keep\nthe \"cannot update ref\" bit.  Will fix, along with the rest.\n"},{"id":"265873","messageId":"1436386899.4542.16.camel@twopensource.com","threadId":"39808","inReplyTo":"559D293A.9040307@alum.mit.edu","subject":"Re: [PATCH v7 7/8] update-ref and tag: add --create-reflog arg","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T20:21:39Z","receivedAt":"2015-07-08T20:21:39Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-08 at 15:44 +0200, Michael Haggerty wrote:\n<snip>\n> Should the \"verify\" command also create a reflog, at least if the\n> reference is not being verified to be missing?\n\nI don't see why it should. \"verify\" does not sound like a command that\nshould change anything.\n"},{"id":"265880","messageId":"559D92CF.7000408@kdbg.org","threadId":"39808","inReplyTo":"1436382962.4542.8.camel@twopensource.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-07-08T21:14:55Z","receivedAt":"2015-07-08T21:14:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.07.2015 um 21:16 schrieb David Turner:\n> On Wed, 2015-07-08 at 19:46 +0200, Johannes Sixt wrote:\n>> Am 08.07.2015 um 02:55 schrieb David Turner:\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>> ...\n>>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n>>> index 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\"\n>>\n>> We are trying very hard not to spawn any new processes in __git_ps1().\n>> So, I raise a moderate veto against this hunk.\n>\n> Do you have an alternate suggestion about how to accomplish the same\n> thing? Here are my ideas:\n>\n> We could special-case CHERRY_PICK_HEAD and REVERT_HEAD to be files\n> independent of the ref backend, but that tends to complicate the\n> backends.  I think this is a mistake.\n>\n> We could reduce the number from two to one by providing a new\n> git-am-status command which outputs one of \"CHERRY-PICKING\",\n> \"REVERTING\", or \"\" (or maybe it would also handle rebase and am).  We\n> could also generalize it to \"git-prompt-helper\" or something by moving\n> that entire bunch of if statements inside.  This would replace calls to\n> \"git describe\".\n>\n> But you probably have a better idea.\n\nIsn't it mere coincidence that the content of these two files looks like \na non-packed ref? Wouldn't it be better to consider the two akin to \nMERGE_HEAD (which is not a ref because it records more than just a \ncommit name)?\n\n-- Hannes\n"},{"id":"265884","messageId":"xmqq4mlexmh3.fsf@gitster.dls.corp.google.com","threadId":"39808","inReplyTo":"559D92CF.7000408@kdbg.org","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-08T23:23:36Z","receivedAt":"2015-07-08T23:23:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> We could reduce the number from two to one by providing a new\n>> git-am-status command which outputs one of \"CHERRY-PICKING\",\n>> \"REVERTING\", or \"\" (or maybe it would also handle rebase and am).  We\n>> could also generalize it to \"git-prompt-helper\" or something by moving\n>> that entire bunch of if statements inside.  This would replace calls to\n>> \"git describe\".\n>>\n>> But you probably have a better idea.\n>\n> Isn't it mere coincidence that the content of these two files looks\n> like a non-packed ref? Wouldn't it be better to consider the two akin\n> to MERGE_HEAD (which is not a ref because it records more than just a\n> commit name)?\n\nThat is an excellent thought that seems to have escaped everybody\ninvolved in the review.\n\nThese things do not behave like refs.  They do not want reflogs (and\neven if they had, the log would not mean much), and if we want to\nadd more information on the cherry-pick and revert in progress, they\nare the most natural place to do so.\n\nAnother thing that makes me vote for treating them just like\nFETCH_HEAD, MERGE_HEAD and other ALL_CAPS files like COMMIT_MSG is\nwhat should happen in a repository with more than one working trees.\nYou do not want to share what \"CHERRY_PICK_HEAD\" means across them\nonly because they happen to record an object name.\n\nThanks for a bit of sanity.\n"},{"id":"265886","messageId":"1436398884.4542.25.camel@twopensource.com","threadId":"39808","inReplyTo":"559D92CF.7000408@kdbg.org","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T23:41:24Z","receivedAt":"2015-07-08T23:41:24Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-08 at 23:14 +0200, Johannes Sixt wrote:\n> Am 08.07.2015 um 21:16 schrieb David Turner:\n> > On Wed, 2015-07-08 at 19:46 +0200, Johannes Sixt wrote:\n> >> Am 08.07.2015 um 02:55 schrieb David Turner:\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> >> ...\n> >>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> >>> index 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\"\n> >>\n> >> We are trying very hard not to spawn any new processes in __git_ps1().\n> >> So, I raise a moderate veto against this hunk.\n> >\n> > Do you have an alternate suggestion about how to accomplish the same\n> > thing? Here are my ideas:\n> >\n> > We could special-case CHERRY_PICK_HEAD and REVERT_HEAD to be files\n> > independent of the ref backend, but that tends to complicate the\n> > backends.  I think this is a mistake.\n> >\n> > We could reduce the number from two to one by providing a new\n> > git-am-status command which outputs one of \"CHERRY-PICKING\",\n> > \"REVERTING\", or \"\" (or maybe it would also handle rebase and am).  We\n> > could also generalize it to \"git-prompt-helper\" or something by moving\n> > that entire bunch of if statements inside.  This would replace calls to\n> > \"git describe\".\n> >\n> > But you probably have a better idea.\n> \n> Isn't it mere coincidence that the content of these two files looks like \n> a non-packed ref? Wouldn't it be better to consider the two akin to \n> MERGE_HEAD (which is not a ref because it records more than just a \n> commit name)?\n\nThere appears to be a bit of code that assumes they can be read as refs,\nand the tests test this functionality.  Indeed, I wrote most of this\ncode by replacing the backend and fixing failing tests.\n\nAt least some people seem to use the rev-ness of CHERRY_PICK_HEAD:\nhttps://mnaoumov.wordpress.com/2014/12/03/bulk-cherry-picking-process-and-magic-ref-cherry_pick_head/\n\nSo I think it's best to keep the feature.\n"},{"id":"265887","messageId":"1436399058.4542.26.camel@twopensource.com","threadId":"39808","inReplyTo":"xmqq4mlexmh3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-08T23:44:18Z","receivedAt":"2015-07-08T23:44:18Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-08 at 16:23 -0700, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> >> We could reduce the number from two to one by providing a new\n> >> git-am-status command which outputs one of \"CHERRY-PICKING\",\n> >> \"REVERTING\", or \"\" (or maybe it would also handle rebase and am).  We\n> >> could also generalize it to \"git-prompt-helper\" or something by moving\n> >> that entire bunch of if statements inside.  This would replace calls to\n> >> \"git describe\".\n> >>\n> >> But you probably have a better idea.\n> >\n> > Isn't it mere coincidence that the content of these two files looks\n> > like a non-packed ref? Wouldn't it be better to consider the two akin\n> > to MERGE_HEAD (which is not a ref because it records more than just a\n> > commit name)?\n> \n> That is an excellent thought that seems to have escaped everybody\n> involved in the review.\n> \n> These things do not behave like refs.  They do not want reflogs (and\n> even if they had, the log would not mean much), and if we want to\n> add more information on the cherry-pick and revert in progress, they\n> are the most natural place to do so.\n> \n> Another thing that makes me vote for treating them just like\n> FETCH_HEAD, MERGE_HEAD and other ALL_CAPS files like COMMIT_MSG is\n> what should happen in a repository with more than one working trees.\n> You do not want to share what \"CHERRY_PICK_HEAD\" means across them\n> only because they happen to record an object name.\n\nI didn't see this until after I had sent my previous message.  I think\nthe \"multiple working trees\" argument is strong enough that I will\nchange the code (and tests). \n"},{"id":"265892","messageId":"xmqqmvz5x4cc.fsf@gitster.dls.corp.google.com","threadId":"39808","inReplyTo":"1436399058.4542.26.camel@twopensource.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-09T05:55:15Z","receivedAt":"2015-07-09T05:55:15Z","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> I didn't see this until after I had sent my previous message.  I think\n> the \"multiple working trees\" argument is strong enough that I will\n> change the code (and tests). \n\nNot just code, but we probably should step back a bit and clearly\ndefine what we are trying to achieve.  I _think_ what we want are:\n\n - Everything under refs/* and their associated logs would be handed\n   off to the \"pluggable ref backend\" when one is in use.\n\n - All ref-like things with one-level ALL_CAPS names are per working\n   tree.\n\n   - They do not participate in \"prunable?\" reachability\n     computation.\n   - They (typically) do not want \"logs\".\n   - Each may have extra information specific to it.\n   - You can however read an object name off of them.\n\nOne possible and straight-forward implementation to achieve\n\"ref-like things with one-level ALL_CAPS names are per working tree\"\nis to declare that they will not be handed off to the backend, but\nwill always be implemented as files immediately under $GIT_DIR/.\n\nBut note that there is no fundamental reason we have to do it that\nway; an alternative would be to allow backends to store these things\nper working tree, but then the interface to drive backends need to\ntell them which working tree we are working from.\n\nUnlike branches, HEAD must be per working tree; the \"pluggable ref\nbackend\" needs to be able handle HEAD when you introduce it.  So\nfrom that point of view, \"multiple working tree\" is *not* a valid\nargument why they *have* to be implemented as files under $GIT_DIR/.\nIf you plan to let the pluggable ref backend to handle HEAD, you\nmust have a solution for per working tree ref-like things anyway.\n\nAs to J6t's \"no excessive plumbing invocations\", I think the right\napproach is to have a single \"git prompt--helper\" command that does\nwhat the current script does to compute $r.  \"we want to keep\npeeking into files under $GIT_DIR/\" alone is not a valid enough\nreason to constrain us (I am fine if the solution we find\nappropriate for the 'multiple working trees' and other issues ends\nup being \"we solve it by having them as files in $GIT_DIR\" for other\nreasons, though).\n"},{"id":"265947","messageId":"1436478800.4542.61.camel@twopensource.com","threadId":"39808","inReplyTo":"xmqqmvz5x4cc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-09T21:53:20Z","receivedAt":"2015-07-09T21:53:20Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-08 at 22:55 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > I didn't see this until after I had sent my previous message.  I think\n> > the \"multiple working trees\" argument is strong enough that I will\n> > change the code (and tests). \n> \n> Not just code, but we probably should step back a bit and clearly\n> define what we are trying to achieve.  I _think_ what we want are:\n> \n>  - Everything under refs/* and their associated logs would be handed\n>    off to the \"pluggable ref backend\" when one is in use.\n> \n>  - All ref-like things with one-level ALL_CAPS names are per working\n>    tree.\n> \n>    - They do not participate in \"prunable?\" reachability\n>      computation.\n>\n>    - They (typically) do not want \"logs\".\n\nExcept HEAD definitely does. \n\n>    - Each may have extra information specific to it.\n>    - You can however read an object name off of them.\n> \n> One possible and straight-forward implementation to achieve\n> \"ref-like things with one-level ALL_CAPS names are per working tree\"\n> is to declare that they will not be handed off to the backend, but\n> will always be implemented as files immediately under $GIT_DIR/.\n> \n> But note that there is no fundamental reason we have to do it that\n> way; an alternative would be to allow backends to store these things\n> per working tree, but then the interface to drive backends need to\n> tell them which working tree we are working from.\n> \n> Unlike branches, HEAD must be per working tree; the \"pluggable ref\n> backend\" needs to be able handle HEAD when you introduce it.\n\nI actually punted on this in my implementation, because at the time,\ngit-new-workdir was only in contrib.  But since the new worktree stuff,\nmultiple worktrees have first-class support, so I'll have to update the\ncode to handle it.\n\n> So\n> from that point of view, \"multiple working tree\" is *not* a valid\n> argument why they *have* to be implemented as files under $GIT_DIR/.\n> If you plan to let the pluggable ref backend to handle HEAD, you\n> must have a solution for per working tree ref-like things anyway.\n\nOK, here's my current best idea:\n\n1. A \"pseudoref\" is an all-caps file in $GIT_DIR/ that always contains\nat least a SHA1.  CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because\nHEAD might be a symbolic ref, it is not a pseudoref. \n\nRefs backends do not manage pseudorefs.  Instead, when a pseudoref (an\nall-caps ref containing no slashes) is requested (e.g. git rev-parse\nFETCH_HEAD) the generic refs code checks for the existence of that\nfile and if it exists, returns immediately without hitting the backend.\nThe generic code will refuse to allow updates to pseudorefs.\n\n2. The pluggable refs backend manages all refs other than HEAD.\n\n3. The \"files\" backend always manages HEAD.  This allows for a reflog\nand for HEAD to be a symbolic ref.\n\nThe major complication here is ref transactions -- what if there's a\ntransaction that wants to update e.g. both HEAD and refs/heads/master?\n\nIt may be the case that this never happens; I have not actually audited\nthe code to figure it out.  If someone knows for sure that it does not\nhappen, please say so. But assuming it does happen, here's my idea:\n\nIf the refs backend is the files backend, we can simply treat HEAD like\nany other ref.\n\nIf the refs backend is different, then the refs code needs to hold a\nfiles-backend transaction for HEAD, which it will commit immediately\nafter the other transaction succeeds.  We can stick a pointer to the\nextra transaction in the generic struct ref_transaction, which (as\nMichael Haggerty suggests) specific backends will extend.\n\nA failure to commit either transaction will be reported as a failure,\nand we'll give an additional inconsistent state warning if the main\ntransaction succeeds but the HEAD transaction fails.\n\nI don't love this idea -- it seems like kind of a hack.  But it's the\nbest I can come up with.\n\nWhat do other folks think?\n\n> As to J6t's \"no excessive plumbing invocations\", I think the right\n> approach is to have a single \"git prompt--helper\" command that does\n> what the current script does to compute $r.  \"we want to keep\n> peeking into files under $GIT_DIR/\" alone is not a valid enough\n> reason to constrain us (I am fine if the solution we find\n> appropriate for the 'multiple working trees' and other issues ends\n> up being \"we solve it by having them as files in $GIT_DIR\" for other\n> reasons, though).\n\nAgree.\n"},{"id":"265948","messageId":"xmqqbnflugsw.fsf@gitster.dls.corp.google.com","threadId":"39808","inReplyTo":"1436478800.4542.61.camel@twopensource.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-09T22:06:39Z","receivedAt":"2015-07-09T22:06:39Z","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> OK, here's my current best idea:\n>\n> 1. A \"pseudoref\" is an all-caps file in $GIT_DIR/ that always contains\n> at least a SHA1.  CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because\n> HEAD might be a symbolic ref, it is not a pseudoref. \n>\n> Refs backends do not manage pseudorefs.  Instead, when a pseudoref (an\n> all-caps ref containing no slashes) is requested (e.g. git rev-parse\n> FETCH_HEAD) the generic refs code checks for the existence of that\n> file and if it exists, returns immediately without hitting the backend.\n> The generic code will refuse to allow updates to pseudorefs.\n>\n> 2. The pluggable refs backend manages all refs other than HEAD.\n>\n> 3. The \"files\" backend always manages HEAD.  This allows for a reflog\n> and for HEAD to be a symbolic ref.\n>\n> The major complication here is ref transactions -- what if there's a\n> transaction that wants to update e.g. both HEAD and refs/heads/master?\n\nAn update to the current branch (e.g. \"git commit\") does involve at\nleast update to the reflog of HEAD, the current branch somewhere in\nrefs/heads/ and its log, so it is not \"what if\" but is a norm [*1*].\n\n>\n> It may be the case that this never happens; I have not actually audited\n> the code to figure it out.  If someone knows for sure that it does not\n> happen, please say so. But assuming it does happen, here's my idea:\n>\n> If the refs backend is the files backend, we can simply treat HEAD like\n> any other ref.\n>\n> If the refs backend is different, then the refs code needs to hold a\n> files-backend transaction for HEAD, which it will commit immediately\n> after the other transaction succeeds.  We can stick a pointer to the\n> extra transaction in the generic struct ref_transaction, which (as\n> Michael Haggerty suggests) specific backends will extend.\n>\n> A failure to commit either transaction will be reported as a failure,\n> and we'll give an additional inconsistent state warning if the main\n> transaction succeeds but the HEAD transaction fails.\n\nYeah, I was thinking along those lines, too.  Thanks for clearly\nwriting it down.\n\n> What do other folks think?\n\nMe too ;-)\n\n\n[Footnote]\n\n*1* But that is not a complaint; I do not have a better idea myself\neither.\n"},{"id":"265967","messageId":"559F4A55.1070309@alum.mit.edu","threadId":"39808","inReplyTo":"xmqqbnflugsw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-10T04:30:13Z","receivedAt":"2015-07-10T04:30:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/10/2015 12:06 AM, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n>> OK, here's my current best idea:\n>>\n>> 1. A \"pseudoref\" is an all-caps file in $GIT_DIR/ that always contains\n>> at least a SHA1.  CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because\n>> HEAD might be a symbolic ref, it is not a pseudoref. \n>>\n>> Refs backends do not manage pseudorefs.  Instead, when a pseudoref (an\n>> all-caps ref containing no slashes) is requested (e.g. git rev-parse\n>> FETCH_HEAD) the generic refs code checks for the existence of that\n>> file and if it exists, returns immediately without hitting the backend.\n>> The generic code will refuse to allow updates to pseudorefs.\n>>\n>> 2. The pluggable refs backend manages all refs other than HEAD.\n>>\n>> 3. The \"files\" backend always manages HEAD.  This allows for a reflog\n>> and for HEAD to be a symbolic ref.\n>>\n>> The major complication here is ref transactions -- what if there's a\n>> transaction that wants to update e.g. both HEAD and refs/heads/master?\n> \n> An update to the current branch (e.g. \"git commit\") does involve at\n> least update to the reflog of HEAD, the current branch somewhere in\n> refs/heads/ and its log, so it is not \"what if\" but is a norm [*1*].\n\nThe updating of symlink reflogs in general, and particularly that of\nHEAD, is not done very cleanly. You can see the code in\n`commit_ref_update()` (some of it helpfully commented to be a \"Special\nhack\"):\n\n* If a reference is modified through a symlink, the symlink is locked\nrather than the reference itself.\n* If a reference is modified directly, and HEAD points at it, then the\nHEAD reflog is amended without locking HEAD.\n\nAside from the lack of proper locking, which could result in races with\nother processes, we also have the problem that the same reference that\nis being changed via one of these implicit updates could *also* be being\nchanged directly in the same transaction. Such an update would evade the\n`ref_update_reject_duplicates()` check.\n\nPreviously my thinking was that the locking should be done differently:\nwhen the transaction is being processed, extra ref_update records could\nbe created for the extra reference(s) that have to be modified, then\nthese could be handled more straightforwardly. So supposing that HEAD\npoints at refs/heads/master,\n\n* An update of HEAD would be turned into a reflog update and also add a\nsynthetic update to refs/heads/master.\n* An update of refs/heads/master would add a synthetic update to the\nHEAD reflog\n\nThe first point would obviously apply to any updates via symbolic refs.\nThe second one should too, thought this is a case that we currently punt\non to avoid the need to do reverse symbolic ref lookups.\n\n>> It may be the case that this never happens; I have not actually audited\n>> the code to figure it out.  If someone knows for sure that it does not\n>> happen, please say so. But assuming it does happen, here's my idea:\n>>\n>> If the refs backend is the files backend, we can simply treat HEAD like\n>> any other ref.\n>>\n>> If the refs backend is different, then the refs code needs to hold a\n>> files-backend transaction for HEAD, which it will commit immediately\n>> after the other transaction succeeds.  We can stick a pointer to the\n>> extra transaction in the generic struct ref_transaction, which (as\n>> Michael Haggerty suggests) specific backends will extend.\n>>\n>> A failure to commit either transaction will be reported as a failure,\n>> and we'll give an additional inconsistent state warning if the main\n>> transaction succeeds but the HEAD transaction fails.\n> \n> Yeah, I was thinking along those lines, too.  Thanks for clearly\n> writing it down.\n> \n>> What do other folks think?\n> \n> Me too ;-)\n\nI don't have an answer right now, and I have to get on an airplane in a\nfew hours so I can't think hard about it at the moment. But let me also\nbraindump another vague plan that I have had for a long time:\noverlayable reference storage schemes. Think of the way that loose refs\nare currently overlaid on top of packed refs. I think it might be useful\nto support overlaying more generally.\n\nIn this particular case there could be a workspace-local reference\nstorage that only handles HEAD and perhaps some of the other\npseudoreferences. That could be overlaid onto loose reference storage\n(which would then only concern itself with references under \"refs/\"),\nwhich would in turn be overlaid onto packed refs. The workspace-local\nreference storage layer would have evil special-cased code for dealing\nwith the references that live outside of \"refs/\".\n\nA `ref_transaction_commit()` would be broken into phases: first each of\nthe stacked backends would be asked to verify that the transaction is\npossible and acquire any necessary locks, then each backend would get\nthe final \"commit\" command.\n\nThis construct would make it easy for different backends to share the\nsame implementation for HEAD (and potentially other workspace-local)\nreferences, by simply layering that one storage mechanism on top of\ntheir own.\n\nThat would probably be overengineering if it were only used to deal with\nHEAD, but I think it is a nice general mechanism that could have other\napplications.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"265997","messageId":"1436551147.4542.72.camel@twopensource.com","threadId":"39808","inReplyTo":"559F4A55.1070309@alum.mit.edu","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-10T17:59:07Z","receivedAt":"2015-07-10T17:59:07Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2015-07-10 at 06:30 +0200, Michael Haggerty wrote:\n> On 07/10/2015 12:06 AM, Junio C Hamano wrote:\n> > David Turner <dturner@twopensource.com> writes:\n> > \n> >> OK, here's my current best idea:\n> >>\n> >> 1. A \"pseudoref\" is an all-caps file in $GIT_DIR/ that always contains\n> >> at least a SHA1.  CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because\n> >> HEAD might be a symbolic ref, it is not a pseudoref. \n> >>\n> >> Refs backends do not manage pseudorefs.  Instead, when a pseudoref (an\n> >> all-caps ref containing no slashes) is requested (e.g. git rev-parse\n> >> FETCH_HEAD) the generic refs code checks for the existence of that\n> >> file and if it exists, returns immediately without hitting the backend.\n> >> The generic code will refuse to allow updates to pseudorefs.\n> >>\n> >> 2. The pluggable refs backend manages all refs other than HEAD.\n> >>\n> >> 3. The \"files\" backend always manages HEAD.  This allows for a reflog\n> >> and for HEAD to be a symbolic ref.\n> >>\n> >> The major complication here is ref transactions -- what if there's a\n> >> transaction that wants to update e.g. both HEAD and refs/heads/master?\n> > \n> > An update to the current branch (e.g. \"git commit\") does involve at\n> > least update to the reflog of HEAD, the current branch somewhere in\n> > refs/heads/ and its log, so it is not \"what if\" but is a norm [*1*].\n> \n> The updating of symlink reflogs in general, and particularly that of\n> HEAD, is not done very cleanly. You can see the code in\n> `commit_ref_update()` (some of it helpfully commented to be a \"Special\n> hack\"):\n> \n> * If a reference is modified through a symlink, the symlink is locked\n> rather than the reference itself.\n> * If a reference is modified directly, and HEAD points at it, then the\n> HEAD reflog is amended without locking HEAD.\n> \n> Aside from the lack of proper locking, which could result in races with\n> other processes, we also have the problem that the same reference that\n> is being changed via one of these implicit updates could *also* be being\n> changed directly in the same transaction. Such an update would evade the\n> `ref_update_reject_duplicates()` check.\n> \n> Previously my thinking was that the locking should be done differently:\n> when the transaction is being processed, extra ref_update records could\n> be created for the extra reference(s) that have to be modified, then\n> these could be handled more straightforwardly. So supposing that HEAD\n> points at refs/heads/master,\n> \n> * An update of HEAD would be turned into a reflog update and also add a\n> synthetic update to refs/heads/master.\n> * An update of refs/heads/master would add a synthetic update to the\n> HEAD reflog\n> \n> The first point would obviously apply to any updates via symbolic refs.\n> The second one should too, thought this is a case that we currently punt\n> on to avoid the need to do reverse symbolic ref lookups.\n\nAll of this is worth fixing, but I don't know that it needs to be fixed\nbefore ref backends hit.  What do you think?\n\n> >> It may be the case that this never happens; I have not actually audited\n> >> the code to figure it out.  If someone knows for sure that it does not\n> >> happen, please say so. But assuming it does happen, here's my idea:\n> >>\n> >> If the refs backend is the files backend, we can simply treat HEAD like\n> >> any other ref.\n> >>\n> >> If the refs backend is different, then the refs code needs to hold a\n> >> files-backend transaction for HEAD, which it will commit immediately\n> >> after the other transaction succeeds.  We can stick a pointer to the\n> >> extra transaction in the generic struct ref_transaction, which (as\n> >> Michael Haggerty suggests) specific backends will extend.\n> >>\n> >> A failure to commit either transaction will be reported as a failure,\n> >> and we'll give an additional inconsistent state warning if the main\n> >> transaction succeeds but the HEAD transaction fails.\n> > \n> > Yeah, I was thinking along those lines, too.  Thanks for clearly\n> > writing it down.\n> > \n> >> What do other folks think?\n> > \n> > Me too ;-)\n> \n> I don't have an answer right now, and I have to get on an airplane in a\n> few hours so I can't think hard about it at the moment. But let me also\n> braindump another vague plan that I have had for a long time:\n> overlayable reference storage schemes. Think of the way that loose refs\n> are currently overlaid on top of packed refs. I think it might be useful\n> to support overlaying more generally.\n> \n> In this particular case there could be a workspace-local reference\n> storage that only handles HEAD and perhaps some of the other\n> pseudoreferences. That could be overlaid onto loose reference storage\n> (which would then only concern itself with references under \"refs/\"),\n> which would in turn be overlaid onto packed refs. The workspace-local\n> reference storage layer would have evil special-cased code for dealing\n> with the references that live outside of \"refs/\".\n> \n> A `ref_transaction_commit()` would be broken into phases: first each of\n> the stacked backends would be asked to verify that the transaction is\n> possible and acquire any necessary locks, then each backend would get\n> the final \"commit\" command.\n> \n> This construct would make it easy for different backends to share the\n> same implementation for HEAD (and potentially other workspace-local)\n> references, by simply layering that one storage mechanism on top of\n> their own.\n> \n> That would probably be overengineering if it were only used to deal with\n> HEAD, but I think it is a nice general mechanism that could have other\n> applications.\n\nInteresting concept.  I think the semantics could get rather\ncomplicated, but maybe it's worth thinking about.\n\nBut for now, I think it would be better to special-case pseudorefs, with\nthe option to expand that to full layering later if we see a need.\n"},{"id":"266102","messageId":"1436848402.5074.21.camel@twopensource.com","threadId":"39808","inReplyTo":"559F4A55.1070309@alum.mit.edu","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-14T04:33:22Z","receivedAt":"2015-07-14T04:33:22Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"[j6t to bcc as it looks like his concerns have been addressed]\n\nOn Fri, 2015-07-10 at 06:30 +0200, Michael Haggerty wrote:\n> On 07/10/2015 12:06 AM, Junio C Hamano wrote:\n> > David Turner <dturner@twopensource.com> writes:\n> > \n> >> OK, here's my current best idea:\n> >>\n> >> 1. A \"pseudoref\" is an all-caps file in $GIT_DIR/ that always contains\n> >> at least a SHA1.  CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because\n> >> HEAD might be a symbolic ref, it is not a pseudoref. \n> >>\n> >> Refs backends do not manage pseudorefs.  Instead, when a pseudoref (an\n> >> all-caps ref containing no slashes) is requested (e.g. git rev-parse\n> >> FETCH_HEAD) the generic refs code checks for the existence of that\n> >> file and if it exists, returns immediately without hitting the backend.\n> >> The generic code will refuse to allow updates to pseudorefs.\n> >>\n> >> 2. The pluggable refs backend manages all refs other than HEAD.\n> >>\n> >> 3. The \"files\" backend always manages HEAD.  This allows for a reflog\n> >> and for HEAD to be a symbolic ref.\n> >>\n> >> The major complication here is ref transactions -- what if there's a\n> >> transaction that wants to update e.g. both HEAD and refs/heads/master?\n> > \n> > An update to the current branch (e.g. \"git commit\") does involve at\n> > least update to the reflog of HEAD, the current branch somewhere in\n> > refs/heads/ and its log, so it is not \"what if\" but is a norm [*1*].\n> \n> The updating of symlink reflogs in general, and particularly that of\n> HEAD, is not done very cleanly. You can see the code in\n> `commit_ref_update()` (some of it helpfully commented to be a \"Special\n> hack\"):\n> \n> * If a reference is modified through a symlink, the symlink is locked\n> rather than the reference itself.\n> * If a reference is modified directly, and HEAD points at it, then the\n> HEAD reflog is amended without locking HEAD.\n> \n> Aside from the lack of proper locking, which could result in races with\n> other processes, we also have the problem that the same reference that\n> is being changed via one of these implicit updates could *also* be being\n> changed directly in the same transaction. Such an update would evade the\n> `ref_update_reject_duplicates()` check.\n\nOn reflection, I'm not sure my approach makes sense.  The problem is\nthat refs backends internally manage recursive updates to symbolic refs,\nso it is not easy to send update for HEAD to one backend while sending\nthe corresponding refs/heads/master updates to a different one. \n\nSo I am thinking instead that backends should be required to manage\nupdates to HEAD themselves, and that some functions from refs-be-files\nwould be made generic to help with this.  \n\nFor the LMDB backend, I could put most of that code at the LMDB access\nlayer (which presently simply converts all LMDB errors other than\nNOT_FOUND to calls to die()).  I would intercept reads and writes to\nHEAD and logs/HEAD and replace them with calls to the appropriate\nfunctions.  So, for instance, the LMDB backend would still decide\nwhether to write HEAD, but it would delegate to the files backend code\nto actually write it.  Reflog updates are a bit more complicated,\nbecause we might end up using a different reflog format for LMDB vs for\nthe files backend, but since all reflog updates are controlled by the\nbackend, it should still be possible to handle this cleanly.\n\nOn reflection, I think that this would also be a reasonable approach to\ntake for stash, which does not fall into any of the listed categories.\nIt's not a pseudoref because it's not all-caps and it starts with refs/.\nUnlike other pseudorefs, it needs a reflog.  But like HEAD and unlike\nother refs, it (and its reflog) wants to be per-worktree. Are there\nother ref-like-things in this category (which I'll call \"per-worktree\nrefs\")?  I guess one set of these is submodules' HEADs.  I have been\nplanning on punting on these because it looks to me like that's what the\nfiles backend does.  So, let's leave those aside.  I don't think of any\nothers but maybe I'm missing something.\n\nI'm also not sure how worktrees handle HEAD's reflog -- there doesn't\nseem to be anything worktree-specific in refs.c.  But perhaps I'm just\nnot understanding that bit of the code.\n"},{"id":"266152","messageId":"xmqqd1ztxubj.fsf@gitster.dls.corp.google.com","threadId":"39808","inReplyTo":"1436848402.5074.21.camel@twopensource.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-15T16:24:32Z","receivedAt":"2015-07-15T16:24:32Z","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> So I am thinking instead that backends should be required to manage\n> updates to HEAD themselves, and that some functions from refs-be-files\n> would be made generic to help with this.  \n> ...\n> For the LMDB backend, I could put most of that code at the LMDB access\n> ...\n> backend, it should still be possible to handle this cleanly.\n\nSounds sensible.\n\n> On reflection, I think that this would also be a reasonable approach to\n> take for stash, which does not fall into any of the listed categories.\n> It's not a pseudoref because it's not all-caps and it starts with refs/.\n> Unlike other pseudorefs, it needs a reflog.  But like HEAD and unlike\n> other refs, it (and its reflog) wants to be per-worktree.\n\nI think we want stash, unlike HEAD, to be shared across worktrees,\nand contrib/workdir gets this right.\n\nSo there is nothing that makes refs/stash like CHERRY_PICK_HEAD at\nall.\n\n> Are there\n> other ref-like-things in this category (which I'll call \"per-worktree\n> refs\")?  I guess one set of these is submodules' HEADs.\n\nI've been assuming that a secondary worktree of superproject will\nget its own & corresponding secondary worktree of a submodule, which\nwould automatically give per-worktree \"submodule's HEADs\" and\neverything else that has to be per-worktree.  Am I missing something\nor are there any more need for underlying machinery than what we\ncurrently have for secondary worktrees for a single project tree?\n"},{"id":"266156","messageId":"1436983476.7540.4.camel@twopensource.com","threadId":"39808","inReplyTo":"xmqqd1ztxubj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-15T18:04:36Z","receivedAt":"2015-07-15T18:04:36Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-15 at 09:24 -0700, Junio C Hamano wrote:\n> > On reflection, I think that this would also be a reasonable approach to\n> > take for stash, which does not fall into any of the listed categories.\n> > It's not a pseudoref because it's not all-caps and it starts with refs/.\n> > Unlike other pseudorefs, it needs a reflog.  But like HEAD and unlike\n> > other refs, it (and its reflog) wants to be per-worktree.\n> \n> I think we want stash, unlike HEAD, to be shared across worktrees,\n> and contrib/workdir gets this right.\n> \n> So there is nothing that makes refs/stash like CHERRY_PICK_HEAD at\n> all.\n\nI just did a brief survey here and folks here agree with you.  So, I'll\nadjust my patches accordingly.\n\n> > Are there\n> > other ref-like-things in this category (which I'll call \"per-worktree\n> > refs\")?  I guess one set of these is submodules' HEADs.\n> \n> I've been assuming that a secondary worktree of superproject will\n> get its own & corresponding secondary worktree of a submodule, which\n> would automatically give per-worktree \"submodule's HEADs\" and\n> everything else that has to be per-worktree.  Am I missing something\n> or are there any more need for underlying machinery than what we\n> currently have for secondary worktrees for a single project tree?\n\nThat sounds good to me, but judging by the docs, that's not yet\nimplemented.  So I'm going to worry about that when it is implemented.\n"}]}