{"thread":{"id":"38675","subject":"[PATCH v2 2/7] write_ref_sha1(): Move write elision test to callers","startedAt":"2015-03-02T09:29:50Z","lastAt":"2015-03-03T11:35:24Z","messageCount":13,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"256830","messageId":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":null,"subject":"[PATCH v2 0/7] Fix some problems with reflog expiration","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:50Z","receivedAt":"2015-03-02T09:29:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of the patch series. Thanks to Eric Sunshine, Stefan\nBeller, Peff, and Junio for their comments about v1 [1].\n\nThe two main changes since v1:\n\n* Don't change the locking policy for changing symbolic references.\n  Even though the existing policy is partly broken, the change\n  proposed in v1 would have introduced an incompatibility with older\n  versions of Git and with libgit2 and possibly other Git clients. I\n  will try to implement the transition plan described in [2], but I\n  will do it in a separate patch series.\n\n* I rewrote the \"git reflog\" documentation more completely, following\n  Junio's suggestion.\n\nOther changes since v1:\n\n* Rebased onto a more recent version of upstream master (because\n  master now includes mh/reflog-expire and sb/atomic-push). The rebase\n  was conflict-free.\n\n* Tweaked some commit messages and added some Reviewed-by lines.\n\n* Appended a patch by Stefan Beller to delete the \"force_write\" member\n  of \"struct ref_lock\".\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/263552\n[2] http://article.gmane.org/gmane.comp.version-control.git/263811\n\nMichael Haggerty (6):\n  write_ref_sha1(): remove check for lock == NULL\n  write_ref_sha1(): Move write elision test to callers\n  lock_ref_sha1_basic(): do not set force_write for missing references\n  reflog: improve and update documentation\n  reflog_expire(): ignore --updateref for symbolic references\n  reflog_expire(): never update a reference to null_sha1\n\nStefan Beller (1):\n  struct ref_lock: delete the force_write member\n\n Documentation/git-reflog.txt | 139 +++++++++++++++++++++++++------------------\n builtin/reflog.c             |   4 +-\n refs.c                       |  63 +++++++++++---------\n 3 files changed, 117 insertions(+), 89 deletions(-)\n\n-- \n2.1.4\n"},{"id":"256825","messageId":"1425288597-20547-2-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/7] write_ref_sha1(): remove check for lock == NULL","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:51Z","receivedAt":"2015-03-02T09:29:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"None of the callers pass NULL to this function, and there doesn't seem\nto be any usefulness to allowing them to do so.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ab2f2a9..e82d503 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3079,10 +3079,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \tstatic char term = '\\n';\n \tstruct object *o;\n \n-\tif (!lock) {\n-\t\terrno = EINVAL;\n-\t\treturn -1;\n-\t}\n \tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n \t\tunlock_ref(lock);\n \t\treturn 0;\n-- \n2.1.4\n"},{"id":"256824","messageId":"1425288597-20547-3-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/7] write_ref_sha1(): Move write elision test to callers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:52Z","receivedAt":"2015-03-02T09:29:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"write_ref_sha1() previously skipped the write if the reference already\nhad the desired value, unless lock->force_write was set. Instead,\nperform that test at the callers.\n\nTwo of the callers (in rename_ref()) unconditionally set force_write\njust before calling write_ref_sha1(), so they don't need the extra\ncheck at all. Nor do they need to set force_write anymore.\n\nThe last caller, in ref_transaction_commit(), still needs the test.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e82d503..dd30bfa 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2877,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\terror(\"unable to lock %s for update\", newrefname);\n \t\tgoto rollback;\n \t}\n-\tlock->force_write = 1;\n \thashcpy(lock->old_sha1, orig_sha1);\n \tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n \t\terror(\"unable to write current sha1 into %s\", newrefname);\n@@ -2893,7 +2892,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\tgoto rollbacklog;\n \t}\n \n-\tlock->force_write = 1;\n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = 0;\n \tif (write_ref_sha1(lock, orig_sha1, NULL))\n@@ -3079,10 +3077,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \tstatic char term = '\\n';\n \tstruct object *o;\n \n-\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n-\t\tunlock_ref(lock);\n-\t\treturn 0;\n-\t}\n \to = parse_object(sha1);\n \tif (!o) {\n \t\terror(\"Trying to write ref %s with nonexistent object %s\",\n@@ -3796,15 +3790,21 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n-\t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t   update->msg)) {\n+\t\t\tif (!update->lock->force_write &&\n+\t\t\t    !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n+\t\t\t\tunlock_ref(update->lock);\n+\t\t\t\tupdate->lock = NULL;\n+\t\t\t} else if (write_ref_sha1(update->lock, update->new_sha1,\n+\t\t\t\t\t\t  update->msg)) {\n \t\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\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+\t\t\t\t/* freed by write_ref_sha1(): */\n+\t\t\t\tupdate->lock = NULL;\n \t\t\t}\n-\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n \t\t}\n \t}\n \n-- \n2.1.4\n"},{"id":"256827","messageId":"1425288597-20547-4-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 3/7] lock_ref_sha1_basic(): do not set force_write for missing references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:53Z","receivedAt":"2015-03-02T09:29:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If a reference is missing, its SHA-1 will be null_sha1, which can't\npossibly match a new value that ref_transaction_commit() is trying to\nupdate it to. So there is no need to set force_write in this scenario.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex dd30bfa..3ed9ea6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2258,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint type, lflags;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = 0;\n-\tint missing = 0;\n \tint attempts_remaining = 3;\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n@@ -2297,13 +2296,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\torig_refname, strerror(errno));\n \t\tgoto error_return;\n \t}\n-\tmissing = is_null_sha1(lock->old_sha1);\n-\t/* When the ref did not exist and we are creating it,\n-\t * make sure there is no existing ref that is packed\n-\t * whose name begins with our refname, nor a ref whose\n-\t * name is a proper prefix of our refname.\n+\t/*\n+\t * If the ref did not exist and we are creating it, make sure\n+\t * there is no existing packed ref whose name begins with our\n+\t * refname, nor a packed ref whose name is a proper prefix of\n+\t * our refname.\n \t */\n-\tif (missing &&\n+\tif (is_null_sha1(lock->old_sha1) &&\n \t     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n@@ -2319,8 +2318,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tlock->ref_name = xstrdup(refname);\n \tlock->orig_ref_name = xstrdup(orig_refname);\n \tref_file = git_path(\"%s\", refname);\n-\tif (missing)\n-\t\tlock->force_write = 1;\n \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n \t\tlock->force_write = 1;\n \n-- \n2.1.4\n"},{"id":"256828","messageId":"1425288597-20547-5-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 4/7] struct ref_lock: delete the force_write member","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:54Z","receivedAt":"2015-03-02T09:29:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Stefan Beller <sbeller@google.com>\n\nInstead, compute the value when it is needed.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nEdited-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3ed9ea6..f2e9883 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -12,7 +12,6 @@ struct ref_lock {\n \tstruct lock_file *lk;\n \tunsigned char old_sha1[20];\n \tint lock_fd;\n-\tint force_write;\n };\n \n /*\n@@ -2318,8 +2317,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tlock->ref_name = xstrdup(refname);\n \tlock->orig_ref_name = xstrdup(orig_refname);\n \tref_file = git_path(\"%s\", refname);\n-\tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n-\t\tlock->force_write = 1;\n \n  retry:\n \tswitch (safe_create_leading_directories(ref_file)) {\n@@ -3787,8 +3784,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n-\t\t\tif (!update->lock->force_write &&\n-\t\t\t    !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n+\t\t\tif (!((update->type & REF_ISSYMREF)\n+\t\t\t      && (update->flags & REF_NODEREF))\n+\t\t\t    && !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n+\t\t\t\t/*\n+\t\t\t\t * The reference already has the desired\n+\t\t\t\t * value, so we don't need to write it.\n+\t\t\t\t */\n \t\t\t\tunlock_ref(update->lock);\n \t\t\t\tupdate->lock = NULL;\n \t\t\t} else if (write_ref_sha1(update->lock, update->new_sha1,\n-- \n2.1.4\n"},{"id":"256829","messageId":"1425288597-20547-6-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 5/7] reflog: improve and update documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:55Z","receivedAt":"2015-03-02T09:29:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Revamp the \"git reflog\" usage documentation in the manpage and the\ncommand help to match the current reality and improve its clarity:\n\n* Add documentation for some options that had been left out.\n\n* Group the subcommands and options more logically and move more\n  common subcommands/options higher.\n\n* Improve some explanations.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n Documentation/git-reflog.txt | 138 +++++++++++++++++++++++++------------------\n builtin/reflog.c             |   4 +-\n 2 files changed, 81 insertions(+), 61 deletions(-)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex 70791b9..3af9422 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -17,81 +17,101 @@ The command takes various subcommands, and different options\n depending on the subcommand:\n \n [verse]\n-'git reflog expire' [--dry-run] [--stale-fix] [--verbose]\n-\t[--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...\n-'git reflog delete' ref@\\{specifier\\}...\n 'git reflog' ['show'] [log-options] [<ref>]\n+'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]\n+\t[--rewrite] [--updateref] [--stale-fix]\n+\t[--dry-run] [--verbose] [--all | <refs>...]\n+'git reflog delete' [--rewrite] [--updateref]\n+\t[--dry-run] [--verbose] ref@\\{specifier\\}...\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+useful in various Git commands, to specify the old value of a\n+reference. For example, `HEAD@{2}` means \"where HEAD used to be two\n+moves ago\", `master@{one.week.ago}` means \"where master used to point\n+to one week ago\", and so on. See linkgit:gitrevisions[7] for more\n+details.\n+\n+This command manages the information recorded in the reflogs.\n+\n+The \"show\" subcommand (which is also the default, in the absence of\n+any subcommands) shows the log of the reference provided in the\n+command-line (or `HEAD`, by default). The reflog covers all recent\n+actions, and in addition the `HEAD` reflog records branch switching.\n+`git reflog show` is an alias for `git log -g --abbrev-commit\n+--pretty=oneline`; see linkgit:git-log[1] for more information.\n+\n+The \"expire\" subcommand prunes older reflog entries. Entries older\n+than `expire` time, or entries older than `expire-unreachable` time\n+and not reachable from the current tip, are removed from the reflog.\n+This is typically not used directly by end users -- instead, see\n+linkgit:git-gc[1].\n+\n+The \"delete\" subcommand deletes single entries from the reflog. Its\n+argument must be an _exact_ entry (e.g. \"`git reflog delete\n+master@{2}`\").\n \n-Reflog is a mechanism to record when the tip of branches are\n-updated.  This command is to manage the information recorded in it.\n \n-The subcommand \"expire\" is used to prune older reflog entries.\n-Entries older than `expire` time, or entries older than\n-`expire-unreachable` time and not reachable from the current\n-tip, are removed from the reflog.  This is typically not used\n-directly by the end users -- instead, see linkgit:git-gc[1].\n-\n-The subcommand \"show\" (which is also the default, in the absence of any\n-subcommands) will take all the normal log options, and show the log of\n-the reference provided in the command-line (or `HEAD`, by default).\n-The reflog will cover all recent actions (HEAD reflog records branch switching\n-as well).  It is an alias for `git log -g --abbrev-commit --pretty=oneline`;\n-see linkgit:git-log[1].\n+OPTIONS\n+-------\n \n-The reflog is useful in various Git commands, to specify the old value\n-of a reference. For example, `HEAD@{2}` means \"where HEAD used to be\n-two moves ago\", `master@{one.week.ago}` means \"where master used to\n-point to one week ago\", and so on. See linkgit:gitrevisions[7] for\n-more details.\n+Options for `show`\n+~~~~~~~~~~~~~~~~~~\n \n-To delete single entries from the reflog, use the subcommand \"delete\"\n-and specify the _exact_ entry (e.g. \"`git reflog delete master@{2}`\").\n+`git reflog show` accepts any of the options accepted by `git log`.\n \n \n-OPTIONS\n--------\n+Options for `expire` and/or `delete`\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \n---stale-fix::\n-\tThis revamps the logic -- the definition of \"broken commit\"\n-\tbecomes: a commit that is not reachable from any of the refs and\n-\tthere is a missing object among the commit, tree, or blob\n-\tobjects reachable from it that is not reachable from any of the\n-\trefs.\n-+\n-This computation involves traversing all the reachable objects, i.e. it\n-has the same cost as 'git prune'.  Fortunately, once this is run, we\n-should not have to ever worry about missing objects, because the current\n-prune and pack-objects know about reflogs and protect objects referred by\n-them.\n+--all::\n+\t(For the `expire` command only.) Process the reflogs of all\n+\treferences.\n \n --expire=<time>::\n-\tEntries older than this time are pruned.  Without the\n-\toption it is taken from configuration `gc.reflogExpire`,\n-\twhich in turn defaults to 90 days.  --expire=all prunes\n-\tentries regardless of their age; --expire=never turns off\n-\tpruning of reachable entries (but see --expire-unreachable).\n+\t(For the `expire` command only.) Prune entries older than the\n+\tspecified time. If this option is not specified, the\n+\texpiration time is taken from the configuration setting\n+\t`gc.reflogExpire`, which in turn defaults to 90 days.\n+\t`--expire=all` prunes entries regardless of their age;\n+\t`--expire=never` turns off pruning of reachable entries (but\n+\tsee `--expire-unreachable`).\n \n --expire-unreachable=<time>::\n-\tEntries older than this time and not reachable from\n-\tthe current tip of the branch are pruned.  Without the\n-\toption it is taken from configuration\n-\t`gc.reflogExpireUnreachable`, which in turn defaults to\n-\t30 days.  --expire-unreachable=all prunes unreachable\n-\tentries regardless of their age; --expire-unreachable=never\n-\tturns off early pruning of unreachable entries (but see\n-\t--expire).\n-\n---all::\n-\tInstead of listing <refs> explicitly, prune all refs.\n+\t(For the `expire` command only.) Prune entries older than\n+\t`<time>` that are not reachable from the current tip of the\n+\tbranch. If this option is not specified, the expiration time\n+\tis taken from the configuration setting\n+\t`gc.reflogExpireUnreachable`, which in turn defaults to 30\n+\tdays. `--expire-unreachable=all` prunes unreachable entries\n+\tregardless of their age; `--expire-unreachable=never` turns\n+\toff early pruning of unreachable entries (but see `--expire`).\n \n --updateref::\n-\tUpdate the ref with the sha1 of the top reflog entry (i.e.\n-\t<ref>@\\{0\\}) after expiring or deleting.\n+\tUpdate the reference to the value of the top reflog entry (i.e.\n+\t<ref>@\\{0\\}) if the previous top entry was pruned.\n \n --rewrite::\n-\tWhile expiring or deleting, adjust each reflog entry to ensure\n-\tthat the `old` sha1 field points to the `new` sha1 field of the\n-\tprevious entry.\n+\tWhile expiring or deleting, adjust each reflog entry's \"old\"\n+\tSHA-1 to be equal to the \"new\" SHA-1 field of the entry\n+\tpreceding it.\n+\n+--stale-fix::\n+\t(For the `expire` command only.) Prune any reflog entries that\n+\tpoint to \"broken commits\". A broken commit is a commit that is\n+\tnot reachable from any of the reference tips and that refers,\n+\tdirectly or indirectly, to a missing commit, tree, or blob\n+\tobject.\n++\n+This computation involves traversing all the reachable objects, i.e. it\n+has the same cost as 'git prune'.  It is primarily intended to fix\n+corruption caused by garbage collecting using older versions of Git,\n+which didn't protect objects referred to by reflogs.\n+\n+-n::\n+--dry-run::\n+\tDo not actually prune any entries; just show what would have\n+\tbeen pruned.\n \n --verbose::\n \tPrint extra information on screen.\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 49c64f9..dd68a72 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -13,9 +13,9 @@\n  */\n \n static const char reflog_expire_usage[] =\n-\"git reflog expire [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...\";\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 [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...\";\n+\"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...\";\n \n static unsigned long default_reflog_expire;\n static unsigned long default_reflog_expire_unreachable;\n-- \n2.1.4\n"},{"id":"256826","messageId":"1425288597-20547-7-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 6/7] reflog_expire(): ignore --updateref for symbolic references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:56Z","receivedAt":"2015-03-02T09:29:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If we are expiring reflog entries for a symbolic reference, then how\nshould --updateref be handled if the newest reflog entry is expired?\n\nOption 1: Update the referred-to reference. (This is what the current\ncode does.) This doesn't make sense, because the referred-to reference\nhas its own reflog, which hasn't been rewritten.\n\nOption 2: Update the symbolic reference itself (as in, REF_NODEREF).\nThis would convert the symbolic reference into a non-symbolic\nreference (e.g., detaching HEAD), which is surely not what a user\nwould expect.\n\nOption 3: Error out. This is plausible, but it would make the\nfollowing usage impossible:\n\n    git reflog expire ... --updateref --all\n\nOption 4: Ignore --updateref for symbolic references.\n\nWe choose to implement option 4.\n\nNote: another problem in this code will be fixed in a moment.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n Documentation/git-reflog.txt |  3 ++-\n refs.c                       | 15 ++++++++++++---\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex 3af9422..d49db7c 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -89,7 +89,8 @@ Options for `expire` and/or `delete`\n \n --updateref::\n \tUpdate the reference to the value of the top reflog entry (i.e.\n-\t<ref>@\\{0\\}) if the previous top entry was pruned.\n+\t<ref>@\\{0\\}) if the previous top entry was pruned.  (This\n+\toption is ignored for symbolic references.)\n \n --rewrite::\n \tWhile expiring or deleting, adjust each reflog entry's \"old\"\ndiff --git a/refs.c b/refs.c\nindex f2e9883..bd47d23 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4026,6 +4026,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \tstruct ref_lock *lock;\n \tchar *log_file;\n \tint status = 0;\n+\tint type;\n \n \tmemset(&cb, 0, sizeof(cb));\n \tcb.flags = flags;\n@@ -4037,7 +4038,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n \t */\n-\tlock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);\n \tif (!lock)\n \t\treturn error(\"cannot lock ref '%s'\", refname);\n \tif (!reflog_exists(refname)) {\n@@ -4074,10 +4075,18 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t(*cleanup_fn)(cb.policy_cb);\n \n \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n+\t\t/*\n+\t\t * It doesn't make sense to adjust a reference pointed\n+\t\t * to by a symbolic ref based on expiring entries in\n+\t\t * the symbolic reference's reflog.\n+\t\t */\n+\t\tint update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n+\t\t\t!(type & REF_ISSYMREF);\n+\n \t\tif (close_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"couldn't write %s: %s\", log_file,\n \t\t\t\t\tstrerror(errno));\n-\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n+\t\t} else if (update &&\n \t\t\t(write_in_full(lock->lock_fd,\n \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n@@ -4088,7 +4097,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t} else if (commit_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"unable to commit reflog '%s' (%s)\",\n \t\t\t\t\tlog_file, strerror(errno));\n-\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n+\t\t} else if (update && commit_ref(lock)) {\n \t\t\tstatus |= error(\"couldn't set %s\", lock->ref_name);\n \t\t}\n \t}\n-- \n2.1.4\n"},{"id":"256832","messageId":"1425288597-20547-8-git-send-email-mhagger@alum.mit.edu","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 7/7] reflog_expire(): never update a reference to null_sha1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-02T09:29:57Z","receivedAt":"2015-03-02T09:29:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Currently, if --updateref is specified and the very last reflog entry\nis expired or deleted, the reference's value is set to 0{40}. This is\nan invalid state of the repository, and breaks, for example, \"git\nfsck\" and \"git for-each-ref\".\n\nThe only place we use --updateref in our own code is when dropping\nstash entries. In that code, the very next step is to check if the\nreflog has been made empty, and if so, delete the \"refs/stash\"\nreference entirely. Thus that code path ultimately leaves the\nrepository in a valid state.\n\nBut we don't want to the repository in an invalid state even\ntemporarily, and we don't want to leave an invalid state if other\ncallers of \"git reflog expire|delete --updateref\" don't think to do\nthe extra cleanup step.\n\nSo, if \"git reflog expire|delete\" leaves no more entries in the\nreflog, just leave the reference unchanged.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex bd47d23..f6b04d9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4078,10 +4078,13 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t/*\n \t\t * It doesn't make sense to adjust a reference pointed\n \t\t * to by a symbolic ref based on expiring entries in\n-\t\t * the symbolic reference's reflog.\n+\t\t * the symbolic reference's reflog. Nor can we update\n+\t\t * a reference if there are no remaining reflog\n+\t\t * entries.\n \t\t */\n \t\tint update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n-\t\t\t!(type & REF_ISSYMREF);\n+\t\t\t!(type & REF_ISSYMREF) &&\n+\t\t\t!is_null_sha1(cb.last_kept_sha1);\n \n \t\tif (close_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"couldn't write %s: %s\", log_file,\n-- \n2.1.4\n"},{"id":"256869","messageId":"xmqqk2yz9ir9.fsf@gitster.dls.corp.google.com","threadId":"38675","inReplyTo":"1425288597-20547-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 4/7] struct ref_lock: delete the force_write member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-02T21:44:42Z","receivedAt":"2015-03-02T21:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Instead, compute the value when it is needed.\n\n> @@ -2318,8 +2317,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \tlock->ref_name = xstrdup(refname);\n>  \tlock->orig_ref_name = xstrdup(orig_refname);\n>  \tref_file = git_path(\"%s\", refname);\n> -\tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n> -\t\tlock->force_write = 1;\n>  \n>   retry:\n>  \tswitch (safe_create_leading_directories(ref_file)) {\n> @@ -3787,8 +3784,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>  \t\tstruct ref_update *update = updates[i];\n>  \n>  \t\tif (!is_null_sha1(update->new_sha1)) {\n> -\t\t\tif (!update->lock->force_write &&\n> -\t\t\t    !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n> +\t\t\tif (!((update->type & REF_ISSYMREF)\n> +\t\t\t      && (update->flags & REF_NODEREF))\n> +\t\t\t    && !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n> +\t\t\t\t/*\n> +\t\t\t\t * The reference already has the desired\n> +\t\t\t\t * value, so we don't need to write it.\n> +\t\t\t\t */\n>  \t\t\t\tunlock_ref(update->lock);\n>  \t\t\t\tupdate->lock = NULL;\n>  \t\t\t} else if (write_ref_sha1(update->lock, update->new_sha1,\n\nThe code before and after the change are equivalent.\n\nIt shouldn't be the case, but somehow I find the original slightly\neasier to understand.  The before and after says the same thing,\ni.e. the code used to be:\n\n - We say \"do the write-out without questioning\" when we are\n   updating a symbolic ref without dereferencing.\n\n - Do nothing and unlock if we are not told to \"do the write-out\n   without questioning\" and the update will be a no-op anyway.\n\nwhile the code after the change says:\n\n + Do nothing and unlock if we are not handling \"update a symbolic\n   ref without dereferencing\" and the update will be a no-op anyway.\n\nPerhaps the former has the same effect as \"avoid a single complex\nsentence and use two short sentences instead\".\n\nThe negation in the condition does not help, either.\n\n * If we are updating a symbolic ref without dereferencing, or if we\n   are updating with a different object name, we definitely have to\n   write.\n\nwould be easier to understand, perhaps?  I.e.\n\n\tif (hashcmp(update->lock->old_sha1, update->lock->new_sha1) ||\n\t    ((update->type & REF_ISSYMREF) && (update->flags & REF_NO_DEREF))) {\n\t\t/* do the write-out thing */\n\t} else {\n\t\t/* the request to update from the same to the same is a no-op */\n\t\tunlock_ref(update->lock);\n                update->lock = NULL;\n\t}\n\nI dunno.\n"},{"id":"256870","messageId":"xmqqfv9n9htg.fsf@gitster.dls.corp.google.com","threadId":"38675","inReplyTo":"1425288597-20547-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 5/7] reflog: improve and update documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-02T22:04:59Z","receivedAt":"2015-03-02T22:04:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> +Reference logs, or \"reflogs\", record when the tips of branches and\n> +other references were updated in the local repository. Reflogs are\n> +useful in various Git commands, to specify the old value of a\n> +reference. For example, `HEAD@{2}` means \"where HEAD used to be two\n> +moves ago\", `master@{one.week.ago}` means \"where master used to point\n> +to one week ago\", and so on. See linkgit:gitrevisions[7] for more\n> +details.\n\nLooks very good, especially the part that mentions \"in the local\nrepository\".  It seems to be a common novice misunderstanding what\n`master@{one.week.ago}` means, and it might be beneficial to be more\nverbose by saying \"where master used to point to one week ago in\nthis local repository\".\n\n> +The \"expire\" subcommand prunes older reflog entries. Entries older\n> +than `expire` time, or entries older than `expire-unreachable` time\n> +and not reachable from the current tip, are removed from the reflog.\n> +This is typically not used directly by end users -- instead, see\n> +linkgit:git-gc[1].\n> +\n> +The \"delete\" subcommand deletes single entries from the reflog. Its\n> +argument must be an _exact_ entry (e.g. \"`git reflog delete\n> +master@{2}`\").\n\nJust like \"expire\", \"delete\" should be accompanied by the same\n\"typically not\".  I do not think it is even worth mentioning that it\nexists merely as an implementation detail for likgit:git-stash[1]\nand for no other reason.\n\n\n> +Options for `expire` and/or `delete`\n> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nI think this started from a hope that these two share many, but\nlooking at the result I notice the shared ones are a tiny and\ntrivial minority.  It probably makes sense to retitle this section\n\"Options for expire\" (and remove \"For the expire command only\"), and\nadd an \"Options for delete\" section immediately after it that looks\nlike:\n\n\tOptions for `delete`\n        ~~~~~~~~~~~~~~~~~~~~\n\n\t--updateref::\n        --rewrite::\n        --dry-run::\n        \tThe `delete` command takes these options whose\n                meanings are the same as those for `expire`.\n\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 49c64f9..dd68a72 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -13,9 +13,9 @@\n>   */\n>  \n>  static const char reflog_expire_usage[] =\n> -\"git reflog expire [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...\";\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 [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...\";\n> +\"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...\";\n>  \n>  static unsigned long default_reflog_expire;\n>  static unsigned long default_reflog_expire_unreachable;\n\nThanks for being complete, but I sense that it may be time we\nswitched to parse-options here, which gives us the help string for\nfree.  Perhaps throw in a comment line before this hunk\n\n\t/* NEEDSWORK: switch to parse-options */\n\nor something to leave hint for other people?\n\nThanks.\n"},{"id":"256871","messageId":"xmqqbnkb9hmb.fsf@gitster.dls.corp.google.com","threadId":"38675","inReplyTo":"1425288597-20547-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 0/7] Fix some problems with reflog expiration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-02T22:09:16Z","receivedAt":"2015-03-02T22:09:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> This is v2 of the patch series. Thanks to Eric Sunshine, Stefan\n> Beller, Peff, and Junio for their comments about v1 [1].\n\nThanks for a pleasant read.  Will replace what has been queued.\n"},{"id":"256889","messageId":"54F59212.5040105@alum.mit.edu","threadId":"38675","inReplyTo":"xmqqk2yz9ir9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 4/7] struct ref_lock: delete the force_write member","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-03T10:50:58Z","receivedAt":"2015-03-03T10:50:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/02/2015 10:44 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Instead, compute the value when it is needed.\n> \n>> @@ -2318,8 +2317,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>>  \tlock->ref_name = xstrdup(refname);\n>>  \tlock->orig_ref_name = xstrdup(orig_refname);\n>>  \tref_file = git_path(\"%s\", refname);\n>> -\tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n>> -\t\tlock->force_write = 1;\n>>  \n>>   retry:\n>>  \tswitch (safe_create_leading_directories(ref_file)) {\n>> @@ -3787,8 +3784,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>>  \t\tstruct ref_update *update = updates[i];\n>>  \n>>  \t\tif (!is_null_sha1(update->new_sha1)) {\n>> -\t\t\tif (!update->lock->force_write &&\n>> -\t\t\t    !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n>> +\t\t\tif (!((update->type & REF_ISSYMREF)\n>> +\t\t\t      && (update->flags & REF_NODEREF))\n>> +\t\t\t    && !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n>> +\t\t\t\t/*\n>> +\t\t\t\t * The reference already has the desired\n>> +\t\t\t\t * value, so we don't need to write it.\n>> +\t\t\t\t */\n>>  \t\t\t\tunlock_ref(update->lock);\n>>  \t\t\t\tupdate->lock = NULL;\n>>  \t\t\t} else if (write_ref_sha1(update->lock, update->new_sha1,\n> \n> The code before and after the change are equivalent.\n> \n> It shouldn't be the case, but somehow I find the original slightly\n> easier to understand. [...]\n\nI had the same feeling; thanks for confirming it. How about I introduce\na temporary variable `overwriting_symref` as an aid to the reader? I\nthink this makes it pretty clear:\n\n>  \t\tif (!is_null_sha1(update->new_sha1)) {\n> -\t\t\tif (!update->lock->force_write &&\n> -\t\t\t    !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n> +\t\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n> +\t\t\t\t\t\t  (update->flags & REF_NODEREF));\n> +\n> +\t\t\tif (!overwriting_symref\n> +\t\t\t    && !hashcmp(update->lock->old_sha1, update->new_sha1)) {\n> +\t\t\t\t/*\n> +\t\t\t\t * The reference already has the desired\n> +\t\t\t\t * value, so we don't need to write it.\n> +\t\t\t\t */\n>  \t\t\t\tunlock_ref(update->lock);\n>  \t\t\t\tupdate->lock = NULL;\n>  \t\t\t} else if (write_ref_sha1(update->lock, update->new_sha1,\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"256891","messageId":"54F59C7C.5040705@alum.mit.edu","threadId":"38675","inReplyTo":"xmqqfv9n9htg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 5/7] reflog: improve and update documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-03T11:35:24Z","receivedAt":"2015-03-03T11:35:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/02/2015 11:04 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\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>> +useful in various Git commands, to specify the old value of a\n>> +reference. For example, `HEAD@{2}` means \"where HEAD used to be two\n>> +moves ago\", `master@{one.week.ago}` means \"where master used to point\n>> +to one week ago\", and so on. See linkgit:gitrevisions[7] for more\n>> +details.\n> \n> Looks very good, especially the part that mentions \"in the local\n> repository\".  It seems to be a common novice misunderstanding what\n> `master@{one.week.ago}` means, and it might be beneficial to be more\n> verbose by saying \"where master used to point to one week ago in\n> this local repository\".\n\nYes, that's good. I will change it.\n\n>> +The \"expire\" subcommand prunes older reflog entries. Entries older\n>> +than `expire` time, or entries older than `expire-unreachable` time\n>> +and not reachable from the current tip, are removed from the reflog.\n>> +This is typically not used directly by end users -- instead, see\n>> +linkgit:git-gc[1].\n>> +\n>> +The \"delete\" subcommand deletes single entries from the reflog. Its\n>> +argument must be an _exact_ entry (e.g. \"`git reflog delete\n>> +master@{2}`\").\n> \n> Just like \"expire\", \"delete\" should be accompanied by the same\n> \"typically not\".  I do not think it is even worth mentioning that it\n> exists merely as an implementation detail for likgit:git-stash[1]\n> and for no other reason.\n\nOK, will change.\n\n>> +Options for `expire` and/or `delete`\n>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> \n> I think this started from a hope that these two share many, but\n> looking at the result I notice the shared ones are a tiny and\n> trivial minority.  It probably makes sense to retitle this section\n> \"Options for expire\" (and remove \"For the expire command only\"), and\n> add an \"Options for delete\" section immediately after it that looks\n> like:\n> \n> \tOptions for `delete`\n>         ~~~~~~~~~~~~~~~~~~~~\n> \n> \t--updateref::\n>         --rewrite::\n>         --dry-run::\n>         \tThe `delete` command takes these options whose\n>                 meanings are the same as those for `expire`.\n\nEither way is a little bit awkward, because these two subcommands share\n4 out of 8 of their options. But since \"delete\" is really quite useless\nto end users, I stuck its options in a separate section as you\nsuggested, but inline in a paragraph, where they won't bother anybody.\n\n>> diff --git a/builtin/reflog.c b/builtin/reflog.c\n>> index 49c64f9..dd68a72 100644\n>> --- a/builtin/reflog.c\n>> +++ b/builtin/reflog.c\n>> @@ -13,9 +13,9 @@\n>>   */\n>>  \n>>  static const char reflog_expire_usage[] =\n>> -\"git reflog expire [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...\";\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 [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...\";\n>> +\"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...\";\n>>  \n>>  static unsigned long default_reflog_expire;\n>>  static unsigned long default_reflog_expire_unreachable;\n> \n> Thanks for being complete, but I sense that it may be time we\n> switched to parse-options here, which gives us the help string for\n> free.  Perhaps throw in a comment line before this hunk\n> \n> \t/* NEEDSWORK: switch to parse-options */\n> \n> or something to leave hint for other people?\n\nOK.\n\nThanks for your review! A reroll will be coming soon.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}