{"thread":{"id":"38535","subject":"[PATCH 4/8] reflog: fix documentation","startedAt":"2015-02-09T09:12:36Z","lastAt":"2015-02-14T05:58:05Z","messageCount":46,"participants":["Michael Haggerty","Stefan Beller","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"255805","messageId":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":null,"subject":"[PATCH 0/8] Fix some problems with reflog expiration","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:36Z","receivedAt":"2015-02-09T09:12:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In addition to a few cleanups, this patch series fixes some problems\nthat I noticed when working on mh/reflog-expire:\n\n* Ignore '--updateref' when expiring the reflog of a symbolic\n  reference, because the alternatives are all pretty silly. See the\n  log message for commit 6/8 for more information.\n\n* Ignore '--updateref' when *all* reflog entries are expired by \"git\n  reflog expire\" or \"git reflog delete\". Currently, this sets the\n  reference to 0{40}, which breaks the repository. (Another\n  alternative would be to delete the reference in this situation, but\n  that seemed too radical to me somehow.)\n\n* When expiring the reflog for a symbolic reference, lock the symbolic\n  reference rather than its referent.\n\nThis patch series applies on top of master merged together with\nsb/atomic-push, like the \"refs-have-new\" patch series that I just\nsubmitted. It is also available from my GitHub account [1] as branch\n\"expire-updateref-fixes\".\n\nThere is a minor conflict between this patch series and\n\"mh/refs-have-new\":\n\n<<<<<<< HEAD\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\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||||||| merged common ancestors\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=======\n\t\tif ((flags & REF_HAVE_NEW) && !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>>>>>>> refs-have-new\n\nIt can be resolved in the obvious way:\n\n<<<<<<<\n\t\tif ((flags & REF_HAVE_NEW) && !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\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>>>>>>>\n\nBy the way, both of these patch series conflict with\nsb/atomic-push-fix, which is in pu. My understanding is that Stefan\nwants to rework that patch series anyway, but if not I would be happy\nto show how to resolve the conflicts.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\nMichael Haggerty (8):\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: fix documentation\n  reflog: rearrange the manpage\n  reflog_expire(): ignore --updateref for symbolic references\n  reflog_expire(): never update a reference to null_sha1\n  reflog_expire(): lock symbolic refs themselves, not their referent\n\n Documentation/git-reflog.txt | 65 +++++++++++++++++++++++---------------------\n builtin/reflog.c             |  2 +-\n refs.c                       | 55 ++++++++++++++++++++-----------------\n 3 files changed, 65 insertions(+), 57 deletions(-)\n\n-- \n2.1.4\n"},{"id":"255804","messageId":"1423473164-6011-2-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/8] write_ref_sha1(): remove check for lock == NULL","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:37Z","receivedAt":"2015-02-09T09:12:37Z","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>\n---\n refs.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c5fa709..d1130e2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3080,10 +3080,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":"255806","messageId":"1423473164-6011-3-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/8] write_ref_sha1(): Move write elision test to callers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:38Z","receivedAt":"2015-02-09T09:12:38Z","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 d1130e2..651e37e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2878,7 +2878,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@@ -2894,7 +2893,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@@ -3080,10 +3078,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@@ -3797,15 +3791,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":"255803","messageId":"1423473164-6011-4-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:39Z","receivedAt":"2015-02-09T09:12:39Z","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 651e37e..b083858 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2259,7 +2259,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@@ -2298,13 +2297,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 * When the ref did not exist and we are creating it, make\n+\t * sure there is no existing packed ref whose name begins with\n+\t * our refname, nor a packed ref whose name is a proper prefix\n+\t * of 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@@ -2320,8 +2319,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":"255800","messageId":"1423473164-6011-5-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/8] reflog: fix documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:40Z","receivedAt":"2015-02-09T09:12:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Update the \"git reflog\" usage documentation in the manpage and the\ncommand help to match the current reality.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n Documentation/git-reflog.txt | 6 ++++--\n builtin/reflog.c             | 2 +-\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex 70791b9..b410ee6 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -17,9 +17,11 @@ 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+'git reflog expire' [--dry-run] [--verbose]\n+\t[--rewrite] [--updateref] [--stale-fix]\n \t[--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...\n-'git reflog delete' ref@\\{specifier\\}...\n+'git reflog delete' [--dry-run] [--verbose]\n+\t[--rewrite] [--updateref] ref@\\{specifier\\}...\n 'git reflog' ['show'] [log-options] [<ref>]\n \n Reflog is a mechanism to record when the tip of branches are\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 49c64f9..d89a6dd 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -13,7 +13,7 @@\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 [--verbose] [--dry-run] [--rewrite] [--updateref] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...\";\n static const char reflog_delete_usage[] =\n \"git reflog delete [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...\";\n \n-- \n2.1.4\n"},{"id":"255802","messageId":"1423473164-6011-6-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/8] reflog: rearrange the manpage","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:41Z","receivedAt":"2015-02-09T09:12:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rearrange the \"git reflog\" manpage to list more common operations\nearlier.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n Documentation/git-reflog.txt | 56 ++++++++++++++++++++++----------------------\n 1 file changed, 28 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex b410ee6..f15a48e 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -17,21 +17,21 @@ The command takes various subcommands, and different options\n depending on the subcommand:\n \n [verse]\n+'git reflog' ['show'] [log-options] [<ref>]\n 'git reflog expire' [--dry-run] [--verbose]\n \t[--rewrite] [--updateref] [--stale-fix]\n \t[--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...\n 'git reflog delete' [--dry-run] [--verbose]\n \t[--rewrite] [--updateref] ref@\\{specifier\\}...\n-'git reflog' ['show'] [log-options] [<ref>]\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+Reflog is a mechanism to record when the tips of branches are updated.\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 \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+This command manages the information recorded in the reflog.\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@@ -40,11 +40,11 @@ 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 \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+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 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@@ -53,19 +53,6 @@ and specify the _exact_ entry (e.g. \"`git reflog delete master@{2}`\").\n OPTIONS\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-\n --expire=<time>::\n \tEntries older than this time are pruned.  Without the\n \toption it is taken from configuration `gc.reflogExpire`,\n@@ -83,8 +70,18 @@ them.\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+--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 \n --updateref::\n \tUpdate the ref with the sha1 of the top reflog entry (i.e.\n@@ -98,6 +95,9 @@ them.\n --verbose::\n \tPrint extra information on screen.\n \n+--all::\n+\tInstead of listing <refs> explicitly, prune all refs.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\n-- \n2.1.4\n"},{"id":"255807","messageId":"1423473164-6011-7-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:42Z","receivedAt":"2015-02-09T09:12:42Z","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: there are still other problems in this code that will be fixed\nin 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 f15a48e..9b87b46 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -85,7 +85,8 @@ them.\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+\t<ref>@\\{0\\}) after expiring or deleting.  (This option is\n+\tignored for symbolic references.)\n \n --rewrite::\n \tWhile expiring or deleting, adjust each reflog entry to ensure\ndiff --git a/refs.c b/refs.c\nindex b083858..c0001da 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4025,6 +4025,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@@ -4036,7 +4037,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@@ -4073,10 +4074,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@@ -4087,7 +4096,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":"255801","messageId":"1423473164-6011-8-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 7/8] reflog_expire(): never update a reference to null_sha1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:43Z","receivedAt":"2015-02-09T09:12:43Z","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 the repository in an invalid state even temporarily,\nand we don't want leave an invalid state if other callers of \"git\nreflog expire|delete --updateref\" don't think to do the extra cleanup\nstep.\n\nSo, if \"git reflog expire|delete\" leaves no more entries in the\nreflog, just leave the reference un-updated.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nAnother alternative would be to delete the reference in this\nsituation. This behavior would be kindof logically consistent and\nhappens to be just the thing that \"git stash drop\" wants. It wouldn't\neven really be backwards-incompatible, given that the current code\nleaves a broken repository in this situation. However, this change\nwould require some special casing if the reference whose reflog is\nbeing expired is the current reference, and it somehow seems too\n\"lossy\" to me for some reason. I'm open to feedback on this point.\n\n refs.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c0001da..1b2a497 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4077,10 +4077,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":"255808","messageId":"1423473164-6011-9-git-send-email-mhagger@alum.mit.edu","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T09:12:44Z","receivedAt":"2015-02-09T09:12:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When processing the reflog of a symbolic ref, hold the lock on the\nsymbolic reference itself, not on the reference that it points to.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1b2a497..3fcf342 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4037,7 +4037,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, &type);\n+\tlock = lock_ref_sha1_basic(refname, sha1, NULL, REF_NODEREF, &type);\n \tif (!lock)\n \t\treturn error(\"cannot lock ref '%s'\", refname);\n \tif (!reflog_exists(refname)) {\n-- \n2.1.4\n"},{"id":"255835","messageId":"CAGZ79kak+K+ZquHoPLra+ABbtxWQLLuEj+z049sACHvjA2oZqg@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/8] Fix some problems with reflog expiration","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:57:52Z","receivedAt":"2015-02-09T18:57:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n\n> By the way, both of these patch series conflict with\n> sb/atomic-push-fix, which is in pu. My understanding is that Stefan\n> wants to rework that patch series anyway, but if not I would be happy\n> to show how to resolve the conflicts.\n\nHeh, I had it already reworked on Friday evening, but forgot to send it out\nfor review. As mentioned before, sb/atomic-push-fix is a misleading branch name\nas it actually enables large transactions [ \"large\" means > $(ulimit -n) ].\n\nI don't mind reworking that series again on top of either this or the other\npatch series you sent out, as I wasn't entirely happy with it anyway.\n(Naming is hard)\n"},{"id":"255848","messageId":"CAPig+cTksvo2A9nazpeg=BThOSPv7DsJ6hz51h=U_yvSNBGn5A@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 7/8] reflog_expire(): never update a reference to null_sha1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-09T20:55:17Z","receivedAt":"2015-02-09T20:55:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 9, 2015 at 4:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Currently, if --updateref is specified and the very last reflog entry\n> is expired or deleted, the reference's value is set to 0{40}. This is\n> an invalid state of the repository, and breaks, for example, \"git\n> fsck\" and \"git for-each-ref\".\n>\n> The only place we use --updateref in our own code is when dropping\n> stash entries. In that code, the very next step is to check if the\n> reflog has been made empty, and if so, delete the \"refs/stash\"\n> reference entirely. Thus that code path ultimately leaves the\n> repository in a valid state.\n>\n> But we don't want the repository in an invalid state even temporarily,\n> and we don't want leave an invalid state if other callers of \"git\n\ns/want/want to/\n\n> reflog expire|delete --updateref\" don't think to do the extra cleanup\n> step.\n>\n> So, if \"git reflog expire|delete\" leaves no more entries in the\n> reflog, just leave the reference un-updated.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n"},{"id":"255909","messageId":"CAGZ79kbCHAS3kbF_rwiGFxNCG1movygkcyOddELM7ikOM+wKBQ@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/8] write_ref_sha1(): remove check for lock == NULL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-10T22:52:23Z","receivedAt":"2015-02-10T22:52:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> None of the callers pass NULL to this function, and there doesn't seem\n> to be any usefulness to allowing them to do so.\n\nUsually I'd oppose this change, as it seems to be a good defensive\nmeasure. (I cannot assume future me or anybody knows what they're\ndoing), but as this function (write_ref_sha1) is not widely exposed\nany more since aae383db8 (Apr 28, refs.c: make write_ref_sha1 static),\nI think it's safe to assume changes affecting this call are well\nunderstood in the future.\n\nso\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c | 4 ----\n>  1 file changed, 4 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index c5fa709..d1130e2 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3080,10 +3080,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n>         static char term = '\\n';\n>         struct object *o;\n>\n> -       if (!lock) {\n> -               errno = EINVAL;\n> -               return -1;\n> -       }\n>         if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n>                 unlock_ref(lock);\n>                 return 0;\n> --\n> 2.1.4\n>\n"},{"id":"255910","messageId":"1423609951-8301-1-git-send-email-sbeller@google.com","threadId":"38535","inReplyTo":"1423473164-6011-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH] refs.c: get rid of force_write flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-10T23:12:31Z","receivedAt":"2015-02-10T23:12:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    When this patch series is applied, you only have 3 occurences of force_write.\n    \n    1. In the struct as an undocumented int.\n    2. In  lock_ref_sha1_basic:\n    \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n    \t\tlock->force_write = 1;\n    3: In ref_transaction_commit:\n    \t/* Perform updates first so live commits remain referenced */\n    \tfor (i = 0; i < n; i++) {\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\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}\n    \t}\n    \n    So maybe we can solve it even more elegant by omiting the first 2 occurences and\n    directly check the type and flags in ref_transaction_commit.\n    \n    Maybe this makes sense to go on top of that series?\n    \n    Thanks,\n    Stefan\n\n refs.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3fcf342..ae24502 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@@ -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 ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n-\t\tlock->force_write = 1;\n \n  retry:\n \tswitch (safe_create_leading_directories(ref_file)) {\n@@ -3788,8 +3785,10 @@ 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\t/* Ignore symbolic links when told not to dereference */\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\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.2.1.62.g3f15098\n"},{"id":"255911","messageId":"CAGZ79kbrFpgF6_dLYdgT2D0JjWggu8edjV2sgXER5btpmyjDNw@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-10T23:24:47Z","receivedAt":"2015-02-10T23:24:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> If a reference is missing, its SHA-1 will be null_sha1, which can't\n> possibly match a new value that ref_transaction_commit() is trying to\n> update it to. So there is no need to set force_write in this scenario.\n>\n\nThis commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not\nforce write of packed refs). And reading both commit messages, they\nseem to contradict each other. (Both agree on  \"If a reference is\nmissing, its SHA-1 will be null_sha1 as provided by resolve_ref\", but\nthe conclusion seems to be different.)\n\nOn the other hand, there is more than 6 years difference, so I guess\nthe meaning and implications of some variables and functions may have\nslightly changed.\n\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c | 15 ++++++---------\n>  1 file changed, 6 insertions(+), 9 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 651e37e..b083858 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2259,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>         int type, lflags;\n>         int mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n>         int resolve_flags = 0;\n> -       int missing = 0;\n>         int attempts_remaining = 3;\n>\n>         lock = xcalloc(1, sizeof(struct ref_lock));\n> @@ -2298,13 +2297,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>                         orig_refname, strerror(errno));\n>                 goto error_return;\n>         }\n> -       missing = is_null_sha1(lock->old_sha1);\n> -       /* When the ref did not exist and we are creating it,\n> -        * make sure there is no existing ref that is packed\n> -        * whose name begins with our refname, nor a ref whose\n> -        * name is a proper prefix of our refname.\n> +       /*\n> +        * When the ref did not exist and we are creating it, make\n> +        * sure there is no existing packed ref whose name begins with\n> +        * our refname, nor a packed ref whose name is a proper prefix\n> +        * of our refname.\n>          */\n> -       if (missing &&\n> +       if (is_null_sha1(lock->old_sha1) &&\n>              !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {\n>                 last_errno = ENOTDIR;\n>                 goto error_return;\n> @@ -2320,8 +2319,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>         lock->ref_name = xstrdup(refname);\n>         lock->orig_ref_name = xstrdup(orig_refname);\n>         ref_file = git_path(\"%s\", refname);\n> -       if (missing)\n> -               lock->force_write = 1;\n>         if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n>                 lock->force_write = 1;\n>\n> --\n> 2.1.4\n>\n"},{"id":"255912","messageId":"CAGZ79kYsJ9Zy32LYOu=JoJ2nOYZMjTnqX_YwPZB1rSmTU5d-0Q@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/8] reflog: fix documentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-10T23:25:36Z","receivedAt":"2015-02-10T23:25:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Update the \"git reflog\" usage documentation in the manpage and the\n> command help to match the current reality.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n> ---\n"},{"id":"255913","messageId":"CAGZ79kY+AN6fmbBfCBdkUXiBSgJusFa7Jv2OS=n=EPXBpzp8EQ@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 5/8] reflog: rearrange the manpage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-10T23:42:18Z","receivedAt":"2015-02-10T23:42:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> ---all::\n> -       Instead of listing <refs> explicitly, prune all refs.\n> +--stale-fix::\n> +       This revamps the logic -- the definition of \"broken commit\"\n> +       becomes: a commit that is not reachable from any of the refs and\n> +       there is a missing object among the commit, tree, or blob\n> +       objects reachable from it that is not reachable from any of the\n> +       refs.\n\n--stale-fix becomes more and more irrelevant over time,\nso why not put in at the very end even after --all ?\n\nThinking out loud:\n(--expire=,--expire-unreachable= and --stale-fix) look like a group\nand (--updateref --rewrite --verbose and --all) also feel like a group,\nso you wanted to keep --stale-fix after --expire-unreachable= ?\n\nWhile talking about this man page, we should also add --dry-run?\n"},{"id":"255914","messageId":"20150211000520.GA30561@peff.net","threadId":"38535","inReplyTo":"CAGZ79kbrFpgF6_dLYdgT2D0JjWggu8edjV2sgXER5btpmyjDNw@mail.gmail.com","subject":"Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-11T00:05:20Z","receivedAt":"2015-02-11T00:05:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:\n\n> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> > If a reference is missing, its SHA-1 will be null_sha1, which can't\n> > possibly match a new value that ref_transaction_commit() is trying to\n> > update it to. So there is no need to set force_write in this scenario.\n> >\n> \n> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not\n> force write of packed refs). And reading both commit messages, they\n> seem to contradict each other. (Both agree on  \"If a reference is\n> missing, its SHA-1 will be null_sha1 as provided by resolve_ref\", but\n> the conclusion seems to be different.)\n\nMost of the lines of 5bdd8d4a3062a that are being reverted here are\ncaching the is_null_sha1() check in the \"missing\" variable. And that's\na cleanup in this patch that is not strictly necessary (\"missing\" would\nonly be used once, so it becomes noise).\n\nThe interesting thing in the earlier commit was to use the null sha1 to\ncause a force-write, rather than lstat()ing the filesystem. And here we\nare saying the force-write is not necessary at all, no matter what\nstorage scheme is used. So I don't think there is any contradiction\nbetween the two.\n\nIs this patch correct that the force-write is not necessary? I think so.\nThe force-write flag comes from:\n\ncommit 732232a123e1e61e38babb1c572722bb8a189ba3\nAuthor: Shawn Pearce <spearce@spearce.org>\nDate:   Fri May 19 03:29:05 2006 -0400\n\n    Force writing ref if it doesn't exist.\n    \n    Normally we try to skip writing a ref if its value hasn't changed\n    but in the special case that the ref doesn't exist but the new\n    value is going to be 0{40} then force writing the ref anyway.\n\nbut I am not sure that logic still holds (if it ever did). We do not ever write\n0{40} into a ref value.\n\n-Peff\n"},{"id":"255915","messageId":"20150211000600.GB30561@peff.net","threadId":"38535","inReplyTo":"CAGZ79kbCHAS3kbF_rwiGFxNCG1movygkcyOddELM7ikOM+wKBQ@mail.gmail.com","subject":"Re: [PATCH 1/8] write_ref_sha1(): remove check for lock == NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-11T00:06:00Z","receivedAt":"2015-02-11T00:06:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 10, 2015 at 02:52:23PM -0800, Stefan Beller wrote:\n\n> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> > None of the callers pass NULL to this function, and there doesn't seem\n> > to be any usefulness to allowing them to do so.\n> \n> Usually I'd oppose this change, as it seems to be a good defensive\n> measure. (I cannot assume future me or anybody knows what they're\n> doing), but as this function (write_ref_sha1) is not widely exposed\n> any more since aae383db8 (Apr 28, refs.c: make write_ref_sha1 static),\n> I think it's safe to assume changes affecting this call are well\n> understood in the future.\n\nThanks, I was iffy on this change for the same reason, but after your\nexplanation, I too think it is reasonable.\n\n-Peff\n"},{"id":"255916","messageId":"CAGZ79kbz93ne8KVkeZfj9muomfcoTh7BUEyEo5ZZpYNG4RSYZg@mail.gmail.com","threadId":"38535","inReplyTo":"20150211000520.GA30561@peff.net","subject":"Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-11T00:07:26Z","receivedAt":"2015-02-11T00:07:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 10, 2015 at 4:05 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:\n>\n>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> > If a reference is missing, its SHA-1 will be null_sha1, which can't\n>> > possibly match a new value that ref_transaction_commit() is trying to\n>> > update it to. So there is no need to set force_write in this scenario.\n>> >\n>>\n>> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not\n>> force write of packed refs). And reading both commit messages, they\n>> seem to contradict each other. (Both agree on  \"If a reference is\n>> missing, its SHA-1 will be null_sha1 as provided by resolve_ref\", but\n>> the conclusion seems to be different.)\n>\n> Most of the lines of 5bdd8d4a3062a that are being reverted here are\n> caching the is_null_sha1() check in the \"missing\" variable. And that's\n> a cleanup in this patch that is not strictly necessary (\"missing\" would\n> only be used once, so it becomes noise).\n>\n> The interesting thing in the earlier commit was to use the null sha1 to\n> cause a force-write, rather than lstat()ing the filesystem. And here we\n> are saying the force-write is not necessary at all, no matter what\n> storage scheme is used. So I don't think there is any contradiction\n> between the two.\n>\n> Is this patch correct that the force-write is not necessary? I think so.\n> The force-write flag comes from:\n>\n> commit 732232a123e1e61e38babb1c572722bb8a189ba3\n> Author: Shawn Pearce <spearce@spearce.org>\n> Date:   Fri May 19 03:29:05 2006 -0400\n>\n>     Force writing ref if it doesn't exist.\n>\n>     Normally we try to skip writing a ref if its value hasn't changed\n>     but in the special case that the ref doesn't exist but the new\n>     value is going to be 0{40} then force writing the ref anyway.\n>\n> but I am not sure that logic still holds (if it ever did). We do not ever write\n> 0{40} into a ref value.\n>\n> -Peff\n\nMakes sense.\n"},{"id":"255918","messageId":"CAGZ79kZvM4FeHQ074kh7qhsz8cHgGaHxOruP7CM2DgPJErkA-w@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-11T00:44:41Z","receivedAt":"2015-02-11T00:44:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n\n> If we are expiring reflog entries for a symbolic reference, then how\n> should --updateref be handled if the newest reflog entry is expired?\n>\n> Option 1: Update the referred-to reference. (This is what the current\n> code does.) This doesn't make sense, because the referred-to reference\n> has its own reflog, which hasn't been rewritten.\n>\n> Option 2: Update the symbolic reference itself (as in, REF_NODEREF).\n> This would convert the symbolic reference into a non-symbolic\n> reference (e.g., detaching HEAD), which is surely not what a user\n> would expect.\n>\n> Option 3: Error out. This is plausible, but it would make the\n> following usage impossible:\n>\n>     git reflog expire ... --updateref --all\n>\n> Option 4: Ignore --updateref for symbolic references.\n>\n\nOk let me ask a question first about the symbolic refs.\n\nWe used to use symbolic links for that, but because of\nportability issues we decided to not make it a link, but rather\na standard file containing the pointing link (The content of\n.git/HEAD is \"ref: refs/heads/master\\n\" except when detached)\n\nSo this is the only distinction? Or is there also a concept of\nsymbolic links/pointers for the reflog handling?\n\n> We choose to implement option 4.\n\nYou're only saying why the other options are insane, not why this\nis sane.\n\nAlso I'd rather tend for option 3 than 4, as it is a safety measure\n(assuming we give a hint to the user what the problem is, and\nhow it is circumventable)\n\n>\n> Note: there are still other problems in this code that will be fixed\n> in a moment.\n>\n> Signed-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>\n> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\n> index f15a48e..9b87b46 100644\n> --- a/Documentation/git-reflog.txt\n> +++ b/Documentation/git-reflog.txt\n> @@ -85,7 +85,8 @@ them.\n>\n>  --updateref::\n>         Update the ref with the sha1 of the top reflog entry (i.e.\n> -       <ref>@\\{0\\}) after expiring or deleting.\n> +       <ref>@\\{0\\}) after expiring or deleting.  (This option is\n> +       ignored for symbolic references.)\n>\n>  --rewrite::\n>         While expiring or deleting, adjust each reflog entry to ensure\n> diff --git a/refs.c b/refs.c\n> index b083858..c0001da 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -4025,6 +4025,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n>         struct ref_lock *lock;\n>         char *log_file;\n>         int status = 0;\n> +       int type;\n>\n>         memset(&cb, 0, sizeof(cb));\n>         cb.flags = flags;\n> @@ -4036,7 +4037,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n>          * reference itself, plus we might need to update the\n>          * reference if --updateref was specified:\n>          */\n> -       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);\n> +       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);\n>         if (!lock)\n>                 return error(\"cannot lock ref '%s'\", refname);\n>         if (!reflog_exists(refname)) {\n> @@ -4073,10 +4074,18 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n>         (*cleanup_fn)(cb.policy_cb);\n>\n>         if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n> +               /*\n> +                * It doesn't make sense to adjust a reference pointed\n> +                * to by a symbolic ref based on expiring entries in\n> +                * the symbolic reference's reflog.\n> +                */\n> +               int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n> +                       ~(type & REF_ISSYMREF);\n> +\n>                 if (close_lock_file(&reflog_lock)) {\n>                         status |= error(\"couldn't write %s: %s\", log_file,\n>                                         strerror(errno));\n> -               } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n> +               } else if (update &&\n>                         (write_in_full(lock->lock_fd,\n>                                 sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n>                          write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n> @@ -4087,7 +4096,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n>                 } else if (commit_lock_file(&reflog_lock)) {\n>                         status |= error(\"unable to commit reflog '%s' (%s)\",\n>                                         log_file, strerror(errno));\n> -               } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n> +               } else if (update && commit_ref(lock)) {\n>                         status |= error(\"couldn't set %s\", lock->ref_name);\n>                 }\n>         }\n> --\n> 2.1.4\n>\n"},{"id":"255919","messageId":"CAGZ79kaBGAOt-R1=mSG5H-5p=2UWjZEesktVwQcDAWFC-OW2Eg@mail.gmail.com","threadId":"38535","inReplyTo":"1423473164-6011-9-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-11T00:49:22Z","receivedAt":"2015-02-11T00:49:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> When processing the reflog of a symbolic ref, hold the lock on the\n> symbolic reference itself, not on the reference that it points to.\n\nI am not sure if that makes sense.\nSo when expiring HEAD, you want to have a .git/HEAD.lock file\ninstead of a .git/refs/heads/master.lock file?\n\nWhat would happen if there is a concurrent modification\nto the master branch?\n\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 1b2a497..3fcf342 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -4037,7 +4037,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n>          * reference itself, plus we might need to update the\n>          * reference if --updateref was specified:\n>          */\n> -       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);\n> +       lock = lock_ref_sha1_basic(refname, sha1, NULL, REF_NODEREF, &type);\n>         if (!lock)\n>                 return error(\"cannot lock ref '%s'\", refname);\n>         if (!reflog_exists(refname)) {\n> --\n> 2.1.4\n>\n"},{"id":"255950","messageId":"xmqq61b8t65x.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"CAGZ79kaBGAOt-R1=mSG5H-5p=2UWjZEesktVwQcDAWFC-OW2Eg@mail.gmail.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-11T22:49:46Z","receivedAt":"2015-02-11T22:49:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> When processing the reflog of a symbolic ref, hold the lock on the\n>> symbolic reference itself, not on the reference that it points to.\n>\n> I am not sure if that makes sense.\n> So when expiring HEAD, you want to have a .git/HEAD.lock file\n> instead of a .git/refs/heads/master.lock file?\n>\n> What would happen if there is a concurrent modification\n> to the master branch?\n\nThe HEAD may be pointing at 'master' and the other party that is\ntrying to modify it would fail when it tries to update the reflog\nfor HEAD thanks to HEAD.lock being held by us.  The HEAD may be\npointing at 'next' and the other part that updates 'master' would\nnot try to modify HEAD reflog and we do not conflict.\n\nAt least, I think that is the rationale behind this change.\n"},{"id":"255952","messageId":"CAGZ79kaaQWRXhph=0g3SRHKXMoW8eAp7QG21yuWXWd7OW4M+uA@mail.gmail.com","threadId":"38535","inReplyTo":"xmqq61b8t65x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-11T23:25:38Z","receivedAt":"2015-02-11T23:25:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 11, 2015 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> When processing the reflog of a symbolic ref, hold the lock on the\n>>> symbolic reference itself, not on the reference that it points to.\n>>\n>> I am not sure if that makes sense.\n>> So when expiring HEAD, you want to have a .git/HEAD.lock file\n>> instead of a .git/refs/heads/master.lock file?\n>>\n>> What would happen if there is a concurrent modification\n>> to the master branch?\n>\n> The HEAD may be pointing at 'master' and the other party that is\n> trying to modify it would fail when it tries to update the reflog\n> for HEAD thanks to HEAD.lock being held by us.  The HEAD may be\n> pointing at 'next' and the other part that updates 'master' would\n> not try to modify HEAD reflog and we do not conflict.\n>\n> At least, I think that is the rationale behind this change.\n\nThat makes sense! Do we have documentation on symrefs?\n\nOriginally I was wondering if this would make things\ncomplicated for  symbolic branches which are not HEAD.\nThen you could update the branch pointed to, because it\nhas no lock as the lock is on the symref. On the other hand\nthis seems to be an improvement, as you cannot move the\nsymref itself, as it has the lock and we don't really have other\nsymrefs?\n"},{"id":"255982","messageId":"54DC93A6.7080003@alum.mit.edu","threadId":"38535","inReplyTo":"CAPig+cTksvo2A9nazpeg=BThOSPv7DsJ6hz51h=U_yvSNBGn5A@mail.gmail.com","subject":"Re: [PATCH 7/8] reflog_expire(): never update a reference to null_sha1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-12T11:51:02Z","receivedAt":"2015-02-12T11:51:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2015 09:55 PM, Eric Sunshine wrote:\n> On Mon, Feb 9, 2015 at 4:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> [...]\n>> But we don't want the repository in an invalid state even temporarily,\n>> and we don't want leave an invalid state if other callers of \"git\n> \n> s/want/want to/\n> [...]\n\nThanks; will fix.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255983","messageId":"54DC9817.7050309@alum.mit.edu","threadId":"38535","inReplyTo":"20150211000520.GA30561@peff.net","subject":"Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-12T12:09:59Z","receivedAt":"2015-02-12T12:09:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/11/2015 01:05 AM, Jeff King wrote:\n> On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:\n> \n>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> If a reference is missing, its SHA-1 will be null_sha1, which can't\n>>> possibly match a new value that ref_transaction_commit() is trying to\n>>> update it to. So there is no need to set force_write in this scenario.\n>>>\n>>\n>> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not\n>> force write of packed refs). And reading both commit messages, they\n>> seem to contradict each other. (Both agree on  \"If a reference is\n>> missing, its SHA-1 will be null_sha1 as provided by resolve_ref\", but\n>> the conclusion seems to be different.)\n> \n> Most of the lines of 5bdd8d4a3062a that are being reverted here are\n> caching the is_null_sha1() check in the \"missing\" variable. And that's\n> a cleanup in this patch that is not strictly necessary (\"missing\" would\n> only be used once, so it becomes noise).\n> \n> The interesting thing in the earlier commit was to use the null sha1 to\n> cause a force-write, rather than lstat()ing the filesystem. And here we\n> are saying the force-write is not necessary at all, no matter what\n> storage scheme is used. So I don't think there is any contradiction\n> between the two.\n> \n> Is this patch correct that the force-write is not necessary? I think so.\n> The force-write flag comes from:\n> \n> commit 732232a123e1e61e38babb1c572722bb8a189ba3\n> Author: Shawn Pearce <spearce@spearce.org>\n> Date:   Fri May 19 03:29:05 2006 -0400\n> \n>     Force writing ref if it doesn't exist.\n>     \n>     Normally we try to skip writing a ref if its value hasn't changed\n>     but in the special case that the ref doesn't exist but the new\n>     value is going to be 0{40} then force writing the ref anyway.\n> \n> but I am not sure that logic still holds (if it ever did). We do not ever write\n> 0{40} into a ref value.\n\nI don't understand that old commit, either. Maybe there was an idea of\nstoring 0{40} in a loose ref file to mark a packed reference as deleted?\nCC to Shawn Pearce in case he can shed some light on the situation.\n\nI still think that my change is OK, because we definitely don't want to\nwrite 0{40} to any loose reference file. The reference-reading code\ncan't deal with it, so this would break the repository.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255992","messageId":"54DCC40E.4040506@alum.mit.edu","threadId":"38535","inReplyTo":"CAGZ79kY+AN6fmbBfCBdkUXiBSgJusFa7Jv2OS=n=EPXBpzp8EQ@mail.gmail.com","subject":"Re: [PATCH 5/8] reflog: rearrange the manpage","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-12T15:17:34Z","receivedAt":"2015-02-12T15:17:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/11/2015 12:42 AM, Stefan Beller wrote:\n> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> ---all::\n>> -       Instead of listing <refs> explicitly, prune all refs.\n>> +--stale-fix::\n>> +       This revamps the logic -- the definition of \"broken commit\"\n>> +       becomes: a commit that is not reachable from any of the refs and\n>> +       there is a missing object among the commit, tree, or blob\n>> +       objects reachable from it that is not reachable from any of the\n>> +       refs.\n> \n> --stale-fix becomes more and more irrelevant over time,\n> so why not put in at the very end even after --all ?\n> \n> Thinking out loud:\n> (--expire=,--expire-unreachable= and --stale-fix) look like a group\n> and (--updateref --rewrite --verbose and --all) also feel like a group,\n> so you wanted to keep --stale-fix after --expire-unreachable= ?\n\nYes, that's what I was thinking.\n\nBut you are right that the docs could be improved even more, so in the\nre-roll I will make some further changes.\n\n> While talking about this man page, we should also add --dry-run?\n\nGood point. I'll fix that, too.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255993","messageId":"54DCC846.7080004@alum.mit.edu","threadId":"38535","inReplyTo":"1423609951-8301-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: get rid of force_write flag","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-12T15:35:34Z","receivedAt":"2015-02-12T15:35:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/11/2015 12:12 AM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n> Notes:\n>     When this patch series is applied, you only have 3 occurences of force_write.\n>     \n>     1. In the struct as an undocumented int.\n>     2. In  lock_ref_sha1_basic:\n>     \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n>     \t\tlock->force_write = 1;\n>     3: In ref_transaction_commit:\n>     \t/* Perform updates first so live commits remain referenced */\n>     \tfor (i = 0; i < n; i++) {\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\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}\n>     \t}\n>     \n>     So maybe we can solve it even more elegant by omiting the first 2 occurences and\n>     directly check the type and flags in ref_transaction_commit.\n>     \n>     Maybe this makes sense to go on top of that series?\n> [...]\n\nThis is a good idea; I'll add it in the re-roll.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255994","messageId":"54DCD015.2080002@alum.mit.edu","threadId":"38535","inReplyTo":"CAGZ79kZvM4FeHQ074kh7qhsz8cHgGaHxOruP7CM2DgPJErkA-w@mail.gmail.com","subject":"Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-12T16:08:53Z","receivedAt":"2015-02-12T16:08:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/11/2015 01:44 AM, Stefan Beller wrote:\n> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> \n>> If we are expiring reflog entries for a symbolic reference, then how\n>> should --updateref be handled if the newest reflog entry is expired?\n>>\n>> Option 1: Update the referred-to reference. (This is what the current\n>> code does.) This doesn't make sense, because the referred-to reference\n>> has its own reflog, which hasn't been rewritten.\n>>\n>> Option 2: Update the symbolic reference itself (as in, REF_NODEREF).\n>> This would convert the symbolic reference into a non-symbolic\n>> reference (e.g., detaching HEAD), which is surely not what a user\n>> would expect.\n>>\n>> Option 3: Error out. This is plausible, but it would make the\n>> following usage impossible:\n>>\n>>     git reflog expire ... --updateref --all\n>>\n>> Option 4: Ignore --updateref for symbolic references.\n>>\n> \n> Ok let me ask a question first about the symbolic refs.\n> \n> We used to use symbolic links for that, but because of\n> portability issues we decided to not make it a link, but rather\n> a standard file containing the pointing link (The content of\n> .git/HEAD is \"ref: refs/heads/master\\n\" except when detached)\n> \n> So this is the only distinction? Or is there also a concept of\n> symbolic links/pointers for the reflog handling?\n\nA symbolic reference can have a reflog just like a normal reference can.\n\nWhen a reference is updated through a symbolic reference, then\nwrite_ref_sha1() writes a reflog entry for both the reference and the\nsymbolic reference. Also (as an extra kludge), if *any* reference is\nupdated directly and it happens to be the current HEAD reference, then\nan entry is added to HEAD's reflog.\n\n\"HEAD\" is the only symbolic reference that is ever transferred across\nrepositories.\n\nSymbolic references are always stored loose (i.e., not in packed-refs).\n\nDoes that answer your questions?\n\n>> We choose to implement option 4.\n> \n> You're only saying why the other options are insane, not why this\n> is sane.\n> \n> Also I'd rather tend for option 3 than 4, as it is a safety measure\n> (assuming we give a hint to the user what the problem is, and\n> how it is circumventable)\n\nThis is a pretty exotic usage. I can't think of any real-life use case\nfor using \"--updateref\" together with a symbolic reference. In our\nentire code base, \"--updateref\" is only used a single time, in\n\"git-stash.sh\", and that is always for \"refs/stash\", which is never a\nsymbolic reference. \"git-stash\" itself is implemented in a very stylized\nway (\"stylized\" being a polite way of saying \"bizarre\"), and I doubt\nthat there are many more users of this option in the wild, let alone\n\"--updateref\" together with a symbolic reference.\n\nSo, honestly, I don't think it is worth the effort of deciding between 3\nvs. 4. Since 4 is easier to implement (and already implemented), I'd\nrather leave it as is. If you want to submit a patch implementing 3, I\nwon't argue against it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255995","messageId":"54DCDA42.2060800@alum.mit.edu","threadId":"38535","inReplyTo":"CAGZ79kaaQWRXhph=0g3SRHKXMoW8eAp7QG21yuWXWd7OW4M+uA@mail.gmail.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-12T16:52:18Z","receivedAt":"2015-02-12T16:52:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/12/2015 12:25 AM, Stefan Beller wrote:\n> On Wed, Feb 11, 2015 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>>> When processing the reflog of a symbolic ref, hold the lock on the\n>>>> symbolic reference itself, not on the reference that it points to.\n>>>\n>>> I am not sure if that makes sense.\n>>> So when expiring HEAD, you want to have a .git/HEAD.lock file\n>>> instead of a .git/refs/heads/master.lock file?\n>>>\n>>> What would happen if there is a concurrent modification\n>>> to the master branch?\n>>\n>> The HEAD may be pointing at 'master' and the other party that is\n>> trying to modify it would fail when it tries to update the reflog\n>> for HEAD thanks to HEAD.lock being held by us.  The HEAD may be\n>> pointing at 'next' and the other part that updates 'master' would\n>> not try to modify HEAD reflog and we do not conflict.\n>>\n>> At least, I think that is the rationale behind this change.\n> \n> That makes sense! Do we have documentation on symrefs?\n> \n> Originally I was wondering if this would make things\n> complicated for  symbolic branches which are not HEAD.\n> Then you could update the branch pointed to, because it\n> has no lock as the lock is on the symref. On the other hand\n> this seems to be an improvement, as you cannot move the\n> symref itself, as it has the lock and we don't really have other\n> symrefs?\n\nThe convention is that holding lock $GIT_DIR/$refname.lock (where\n$refname might be, for example, \"HEAD\" or \"refs/heads/master\") protects\ntwo things:\n\n* The loose-reference file $GIT_DIR/$refname\n* The reflog file $GIT_DIR/logs/$refname\n\nAnd this makes sense:\n\nSuppose that HEAD is refs/heads/master. These two thing have independent\nreflogs, so there is no reason that one process can't be expiring the\nreflog of HEAD while another expires the reflog of refs/heads/master.\n\nThe behavior before this patch was that the reflog for \"HEAD\" was\nmodified while holding the reflog for \"refs/heads/master\". This is too\nstrict and would make those two processes contend unnecessarily.\n\nI can't think of a reason that the current behavior is unsafe. But it's\nmore restrictive than necessary, and more confusing than necessary. My\nguess is that it was unintended (i.e., a bug). It dates from\n\n    68db31cc28 (2007-05-09) git-update-ref: add --no-deref option for\noverwriting/detaching ref\n\nwhich initially added the REF_NODEREF constant and probably forgot that\nthe new flag should be used in this invocation.\n\nHowever, another important question is whether other Git implementations\nhave copied this unusual locking policy. If so, that would be a reason\nnot to change it. I just pinged the libgit2 maintainer to find out their\npolicy. Maybe you could find out about JGit?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255996","messageId":"CAGZ79kbspWzoKFz50aPnpbPr8LXcKz9qAYTwoNe4dQfZCgt=zw@mail.gmail.com","threadId":"38535","inReplyTo":"54DCD015.2080002@alum.mit.edu","subject":"Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-12T17:04:37Z","receivedAt":"2015-02-12T17:04:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 12, 2015 at 8:08 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 02/11/2015 01:44 AM, Stefan Beller wrote:\n>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>\n>>> If we are expiring reflog entries for a symbolic reference, then how\n>>> should --updateref be handled if the newest reflog entry is expired?\n>>>\n>>> Option 1: Update the referred-to reference. (This is what the current\n>>> code does.) This doesn't make sense, because the referred-to reference\n>>> has its own reflog, which hasn't been rewritten.\n>>>\n>>> Option 2: Update the symbolic reference itself (as in, REF_NODEREF).\n>>> This would convert the symbolic reference into a non-symbolic\n>>> reference (e.g., detaching HEAD), which is surely not what a user\n>>> would expect.\n>>>\n>>> Option 3: Error out. This is plausible, but it would make the\n>>> following usage impossible:\n>>>\n>>>     git reflog expire ... --updateref --all\n>>>\n>>> Option 4: Ignore --updateref for symbolic references.\n>>>\n>>\n>> Ok let me ask a question first about the symbolic refs.\n>>\n>> We used to use symbolic links for that, but because of\n>> portability issues we decided to not make it a link, but rather\n>> a standard file containing the pointing link (The content of\n>> .git/HEAD is \"ref: refs/heads/master\\n\" except when detached)\n>>\n>> So this is the only distinction? Or is there also a concept of\n>> symbolic links/pointers for the reflog handling?\n>\n> A symbolic reference can have a reflog just like a normal reference can.\n\nYes I can understand that, maybe I was thinking one step\nfurther in the wrong direction. The question was rather:\nDo we have symbolic reflogs, i.e.\n$ cat .git/logs/<some_ref>:\nsymbolic log: find log in other reflog\n\n>\n> When a reference is updated through a symbolic reference, then\n> write_ref_sha1() writes a reflog entry for both the reference and the\n> symbolic reference. Also (as an extra kludge), if *any* reference is\n> updated directly and it happens to be the current HEAD reference, then\n> an entry is added to HEAD's reflog.\n\nYes because we cannot do inverse symref resolution, we have this kludge\n(with the long comment, \"This should do 99% of the time in theory\nand 100% in practise\") of checking if it is also HEAD whenever we\ntouch another ref.\n\n>\n> \"HEAD\" is the only symbolic reference that is ever transferred across\n> repositories.\n>\n> Symbolic references are always stored loose (i.e., not in packed-refs).\n>\n> Does that answer your questions?\n>\n>>> We choose to implement option 4.\n>>\n>> You're only saying why the other options are insane, not why this\n>> is sane.\n>>\n>> Also I'd rather tend for option 3 than 4, as it is a safety measure\n>> (assuming we give a hint to the user what the problem is, and\n>> how it is circumventable)\n>\n> This is a pretty exotic usage. I can't think of any real-life use case\n> for using \"--updateref\" together with a symbolic reference. In our\n> entire code base, \"--updateref\" is only used a single time, in\n> \"git-stash.sh\", and that is always for \"refs/stash\", which is never a\n> symbolic reference. \"git-stash\" itself is implemented in a very stylized\n> way (\"stylized\" being a polite way of saying \"bizarre\"), and I doubt\n> that there are many more users of this option in the wild, let alone\n> \"--updateref\" together with a symbolic reference.\n>\n> So, honestly, I don't think it is worth the effort of deciding between 3\n> vs. 4. Since 4 is easier to implement (and already implemented), I'd\n> rather leave it as is. If you want to submit a patch implementing 3, I\n> won't argue against it.\n\nI am not going to bring a patch for option 3. I just learned to be extra\nsuspicious if we want to drop something silently, so I wanted to\nunderstand in what circumstances we'd run into trouble with that.\nThat said:\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n>\n"},{"id":"255998","messageId":"CAGZ79kZgjRNS3zd4Tif6M66mjkP6-tDpy4FAtio8jiwqHxUtgw@mail.gmail.com","threadId":"38535","inReplyTo":"54DCDA42.2060800@alum.mit.edu","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-12T18:04:14Z","receivedAt":"2015-02-12T18:04:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 12, 2015 at 8:52 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 02/12/2015 12:25 AM, Stefan Beller wrote:\n>> On Wed, Feb 11, 2015 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>\n>>>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>>>> When processing the reflog of a symbolic ref, hold the lock on the\n>>>>> symbolic reference itself, not on the reference that it points to.\n>>>>\n>>>> I am not sure if that makes sense.\n>>>> So when expiring HEAD, you want to have a .git/HEAD.lock file\n>>>> instead of a .git/refs/heads/master.lock file?\n>>>>\n>>>> What would happen if there is a concurrent modification\n>>>> to the master branch?\n>>>\n>>> The HEAD may be pointing at 'master' and the other party that is\n>>> trying to modify it would fail when it tries to update the reflog\n>>> for HEAD thanks to HEAD.lock being held by us.  The HEAD may be\n>>> pointing at 'next' and the other part that updates 'master' would\n>>> not try to modify HEAD reflog and we do not conflict.\n>>>\n>>> At least, I think that is the rationale behind this change.\n>>\n>> That makes sense! Do we have documentation on symrefs?\n>>\n>> Originally I was wondering if this would make things\n>> complicated for  symbolic branches which are not HEAD.\n>> Then you could update the branch pointed to, because it\n>> has no lock as the lock is on the symref. On the other hand\n>> this seems to be an improvement, as you cannot move the\n>> symref itself, as it has the lock and we don't really have other\n>> symrefs?\n>\n> The convention is that holding lock $GIT_DIR/$refname.lock (where\n> $refname might be, for example, \"HEAD\" or \"refs/heads/master\") protects\n> two things:\n>\n> * The loose-reference file $GIT_DIR/$refname\n> * The reflog file $GIT_DIR/logs/$refname\n>\n> And this makes sense:\n>\n> Suppose that HEAD is refs/heads/master. These two thing have independent\n> reflogs, so there is no reason that one process can't be expiring the\n> reflog of HEAD while another expires the reflog of refs/heads/master.\n>\n> The behavior before this patch was that the reflog for \"HEAD\" was\n> modified while holding the reflog for \"refs/heads/master\". This is too\n> strict and would make those two processes contend unnecessarily.\n>\n> I can't think of a reason that the current behavior is unsafe. But it's\n> more restrictive than necessary, and more confusing than necessary. My\n> guess is that it was unintended (i.e., a bug). It dates from\n>\n>     68db31cc28 (2007-05-09) git-update-ref: add --no-deref option for\n> overwriting/detaching ref\n>\n> which initially added the REF_NODEREF constant and probably forgot that\n> the new flag should be used in this invocation.\n>\n> However, another important question is whether other Git implementations\n> have copied this unusual locking policy. If so, that would be a reason\n> not to change it. I just pinged the libgit2 maintainer to find out their\n> policy. Maybe you could find out about JGit?\n\nI could not really find reflog expiration for jgit for a while, but\nthen I stumbled upon this:\n\n    // TODO: implement reflog_expire(pm, repo);\n\nso I think it's safe to change it in git.git for now.\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n>\n"},{"id":"256010","messageId":"xmqq386asxzj.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"1423473164-6011-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/8] write_ref_sha1(): Move write elision test to callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-12T19:58:40Z","receivedAt":"2015-02-12T19:58:40Z","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> write_ref_sha1() previously skipped the write if the reference already\n> had the desired value, unless lock->force_write was set. Instead,\n> perform that test at the callers.\n>\n> Two of the callers (in rename_ref()) unconditionally set force_write\n> just before calling write_ref_sha1(), so they don't need the extra\n> check at all. Nor do they need to set force_write anymore.\n\nGood.  I recall that this convoluted \"the callee checks if the\nvalues are the same so we need to tell it not to do that\" logic\nlooked very disturbing.\n\n\n\n> The last caller, in ref_transaction_commit(), still needs the test.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c | 18 +++++++++---------\n>  1 file changed, 9 insertions(+), 9 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index d1130e2..651e37e 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2878,7 +2878,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> @@ -2894,7 +2893,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> @@ -3080,10 +3078,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> @@ -3797,15 +3791,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"},{"id":"256011","messageId":"xmqqy4o2rixp.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"54DCC40E.4040506@alum.mit.edu","subject":"Re: [PATCH 5/8] reflog: rearrange the manpage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-12T20:09:06Z","receivedAt":"2015-02-12T20:09:06Z","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> On 02/11/2015 12:42 AM, Stefan Beller wrote:\n>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> ---all::\n>>> -       Instead of listing <refs> explicitly, prune all refs.\n>>> +--stale-fix::\n>>> +       This revamps the logic -- the definition of \"broken commit\"\n>>> +       becomes: a commit that is not reachable from any of the refs and\n>>> +       there is a missing object among the commit, tree, or blob\n>>> +       objects reachable from it that is not reachable from any of the\n>>> +       refs.\n>> \n>> --stale-fix becomes more and more irrelevant over time,\n>> so why not put in at the very end even after --all ?\n>> \n>> Thinking out loud:\n>> (--expire=,--expire-unreachable= and --stale-fix) look like a group\n>> and (--updateref --rewrite --verbose and --all) also feel like a group,\n>> so you wanted to keep --stale-fix after --expire-unreachable= ?\n>\n> Yes, that's what I was thinking.\n>\n> But you are right that the docs could be improved even more, so in the\n> re-roll I will make some further changes.\n\nThe subcommands of \"git reflog\" live in their own world.  The\noptions that are applicable to \"git reflog expire\" do not have\nmuch to do with what you can give to \"git reflog show\".\n\nSo perhaps a good first step would be to split OPTIONS section\ninto three (or four---if we have options common to all of them),\nand list them under separate headings (i.e. \"options applicable to\nall subcommands\" (optional), \"options for 'expire'\", etc.)\n\nThanks.\n"},{"id":"256012","messageId":"xmqqtwyqrim6.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"54DCD015.2080002@alum.mit.edu","subject":"Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-12T20:16:01Z","receivedAt":"2015-02-12T20:16:01Z","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 a pretty exotic usage. I can't think of any real-life use case\n> for using \"--updateref\" together with a symbolic reference. In our\n> entire code base, \"--updateref\" is only used a single time, in\n> \"git-stash.sh\", and that is always for \"refs/stash\", which is never a\n> symbolic reference.\n\nI have been wondering what the valid situation where --updateref is\na right thing to do on _any_ ref, be it a real one or a symbolic,\nunless you are trying to manually fix a corrupted repository that\nlack random objects left and right.  We may expire old reflog\nentries and make stale objects disappear, but even when we purge all\nreflog entries by expiring anything that is older than a nanosecond\nago, I do not think of a situation where we see some reflog entry\nsurface as the \"latest\" entry that points at an object that is _not_\nat the tip of the actual ref.\n\nExcept for the stash, of course, as you pointed out.  We could drop\nthe tip (i.e. the latest) while keeping the other ones.\n\n> \"git-stash\" itself is implemented in a very stylized\n> way (\"stylized\" being a polite way of saying \"bizarre\"), and I doubt\n> that there are many more users of this option in the wild, let alone\n> \"--updateref\" together with a symbolic reference.\n>\n> So, honestly, I don't think it is worth the effort of deciding between 3\n> vs. 4.\n\nI agree with that assessment.  We might even want to start thinking\nabout a good strategy to remove the --updateref option command,\nwhich in turn would need to restructure how a stash is represented.\n"},{"id":"256024","messageId":"20150212215421.GA19618@peff.net","threadId":"38535","inReplyTo":"1423473164-6011-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-12T21:54:21Z","receivedAt":"2015-02-12T21:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 09, 2015 at 10:12:42AM +0100, Michael Haggerty wrote:\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\nIsn't this second clause tautological? Unless REF_ISSYMREF covers all\nbits in \"type\", then \"type & REF_ISSYMREF\" must always have at least one\nbit 0, and negating it becomes non-zero. Did you mean:\n\n   !(type & REF_ISSYMREF)\n\n?\n\n-Peff\n"},{"id":"256060","messageId":"54DE0B82.6090906@alum.mit.edu","threadId":"38535","inReplyTo":"20150212215421.GA19618@peff.net","subject":"Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-13T14:34:42Z","receivedAt":"2015-02-13T14:34:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/12/2015 10:54 PM, Jeff King wrote:\n> On Mon, Feb 09, 2015 at 10:12:42AM +0100, Michael Haggerty wrote:\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> \n> Isn't this second clause tautological? Unless REF_ISSYMREF covers all\n> bits in \"type\", then \"type & REF_ISSYMREF\" must always have at least one\n> bit 0, and negating it becomes non-zero. Did you mean:\n> \n>    !(type & REF_ISSYMREF)\n> \n> ?\n\nYou're right, of course. Thanks.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"256063","messageId":"54DE259C.4030001@alum.mit.edu","threadId":"38535","inReplyTo":"CAGZ79kZgjRNS3zd4Tif6M66mjkP6-tDpy4FAtio8jiwqHxUtgw@mail.gmail.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-13T16:26:04Z","receivedAt":"2015-02-13T16:26:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/12/2015 07:04 PM, Stefan Beller wrote:\n> On Thu, Feb 12, 2015 at 8:52 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> However, another important question is whether other Git implementations\n>> have copied this unusual locking policy. If so, that would be a reason\n>> not to change it. I just pinged the libgit2 maintainer to find out their\n>> policy. Maybe you could find out about JGit?\n> \n> I could not really find reflog expiration for jgit for a while, but\n> then I stumbled upon this:\n> \n>     // TODO: implement reflog_expire(pm, repo);\n> \n> so I think it's safe to change it in git.git for now.\n\nStefan: This locking policy doesn't only affect \"reflog expire\"; we also\nneed to lock the reflog when we add a new entry to it, for example when\nupdating refs/heads/master through HEAD. Do you know what JGit locks in\nthat scenario?\n\n\nI had a discussion in IRC [1] with Carlos Martín Nieto about how libgit2\nhandles this:\n\n* When libgit2 updates a reference through a symref, it locks the\npointed-to reference while updating the reflog for the symbolic ref. So,\nfor example, \"git commit\" would hold refs/heads/master.lock while\nupdating both logs/refs/heads/master and logs/HEAD. (This matches the\ncurrent Git behavior.)\n* libgit2 doesn't support \"reflog expire\", though it does have an API\nthat allows users to overwrite the reflog with a specified list of\nentries. That API locks the reference itself; i.e., HEAD.lock. This\ndisagrees with Git's current behavior.\n\nI also realized that Git's current policy is probably not tenable if one\nprocess is re-seating a symref at the same time as another process is\nexpiring its reflog. The \"git reflog expire HEAD\" might grab\n\"refs/heads/master.lock\" then start rewriting \"logs/HEAD\". Meanwhile,\n\"git checkout foo\" would grab \"HEAD.lock\" (not being blocked by the\n\"expire\" process), then rewrite it to \"ref: refs/heads/foo\", then grab\n\"refs/heads/foo.lock\" before updating \"logs/HEAD\". So both processes\ncould be writing \"logs/HEAD\" at the same time.\n\nIn fact, it's even worse. \"git checkout foo\" and \"git symbolic-ref HEAD\nrefs/heads/foo\" release \"HEAD.lock\" *before* updating logs/HEAD--in\nother words, they append to logs/HEAD without holding *any* lock.\n\nWhat is the best way forward?\n\nI think the current locking policy is untenable, even aside from the bug\nin \"symbolic-ref\".\n\nSwitching to holding only \"HEAD.lock\" while updating \"logs/HEAD\" is the\nright solution, but it would introduce an incompatibility with old\nversions of Git and libgit2 (and maybe JGit?) Probably nobody would\nnotice, but who knows?\n\nTherefore, I propose that we hold *both* \"HEAD.lock\" *and*\n\"refs/heads/master.lock\" when modifying \"logs/HEAD\" (even when running\n\"reflog expire\" or \"reflog delete\"). I think this will be compatible\nwith old versions of Git and libgit2, and it will also fix some design\nproblems mentioned above. Plus, it will prevent updates to the two\nreflogs from appearing out of order relative to each other.\n\nSomeday, when old clients have been retired, we can optionally switch to\nlocking *only* \"HEAD.lock\" when modifying \"logs/HEAD\".\n\nWhat do people think?\nMichael\n\n[1] https://github.com/libgit2/libgit2/issues/2902\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"256066","messageId":"CAGZ79kbn=0VwZ6t3zE=BFGVeZqe5=H43gHLEzD2-OCar107abw@mail.gmail.com","threadId":"38535","inReplyTo":"54DE259C.4030001@alum.mit.edu","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-13T17:16:30Z","receivedAt":"2015-02-13T17:16:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 13, 2015 at 8:26 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>\n> What is the best way forward?\n>\n> Switching to holding only \"HEAD.lock\" while updating \"logs/HEAD\" is the\n> right solution, but it would introduce an incompatibility with old\n> versions of Git and libgit2 (and maybe JGit?) Probably nobody would\n> notice, but who knows?\n>\n> Therefore, I propose that we hold *both* \"HEAD.lock\" *and*\n> \"refs/heads/master.lock\" when modifying \"logs/HEAD\" (even when running\n> \"reflog expire\" or \"reflog delete\"). I think this will be compatible\n> with old versions of Git and libgit2, and it will also fix some design\n> problems mentioned above. Plus, it will prevent updates to the two\n> reflogs from appearing out of order relative to each other.\n\nYeah, I agree on that. It's ok to lock both for now (it doesn't\nreally harm the user). But we really need to document it in the\nsource code why we lock both and that the 2nd lock can be removed\nafter time has passed, something like:\n\n/*\n * We need to lock both the symbolic ref as well as\n * the dereferenced ref for now because of inconsistencies with\n * older and other implementations of git. Originally only the\n * dereferenced ref lock was held, but discussion($gmane/263555)\n * turned out, we actually want to hold the lock on the symbolic ref.\n * For now hold both locks until all implementation hold the lock on\n * the symbolic ref. (Feb/2015)\n */\n\n>\n> Someday, when old clients have been retired, we can optionally switch to\n> locking *only* \"HEAD.lock\" when modifying \"logs/HEAD\".\n>\n> What do people think?\n> Michael\n>\n> [1] https://github.com/libgit2/libgit2/issues/2902\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n>\n"},{"id":"256067","messageId":"xmqqoaoxoffe.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"54DE259C.4030001@alum.mit.edu","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-13T18:05:25Z","receivedAt":"2015-02-13T18:05:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I also realized that Git's current policy is probably not tenable if one\n> process is re-seating a symref at the same time as another process is\n> expiring its reflog. The \"git reflog expire HEAD\" might grab\n> \"refs/heads/master.lock\" then start rewriting \"logs/HEAD\". Meanwhile,\n> \"git checkout foo\" would grab \"HEAD.lock\" (not being blocked by the\n> \"expire\" process), then rewrite it to \"ref: refs/heads/foo\", then grab\n> \"refs/heads/foo.lock\" before updating \"logs/HEAD\". So both processes\n> could be writing \"logs/HEAD\" at the same time.\n> ...\n> Switching to holding only \"HEAD.lock\" while updating \"logs/HEAD\" is the\n> right solution,...\n\nWe convinced ourselves that not locking the symref is wrong, but\nhave we actually convinced us that not locking the underlying ref,\nas long as we have a lock on the symref, is safe?\n\nTo protect you, the holder of a lock on refs/remotes/origin/HEAD\nthat happens to point at refs/remotes/origin/next, from somebody who\nis updating the underlying refs/remotes/origin/next directly without\ngoing through the symbolic ref (e.g. receive-pack), wouldn't the\nother party need to find any and all symbolic refs that point at the\nunderlying ref and take locks on them?\n\nAs dereferencing a symbolic ref in the forward direction is much\ncheaper than in the reverse, and because you need to dereference it\nanyway, I wonder if we want the endgame to be \"hold locks on both\",\nnot \"just hold a lock on the symlink\".\n"},{"id":"256068","messageId":"CAGZ79kZpCjcGeifbLztpNUSq7-3Yy2_GEVPGEQsxrgoZfLFU+g@mail.gmail.com","threadId":"38535","inReplyTo":"xmqqoaoxoffe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-13T18:21:22Z","receivedAt":"2015-02-13T18:21:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>\n>> I also realized that Git's current policy is probably not tenable if one\n>> process is re-seating a symref at the same time as another process is\n>> expiring its reflog. The \"git reflog expire HEAD\" might grab\n>> \"refs/heads/master.lock\" then start rewriting \"logs/HEAD\". Meanwhile,\n>> \"git checkout foo\" would grab \"HEAD.lock\" (not being blocked by the\n>> \"expire\" process), then rewrite it to \"ref: refs/heads/foo\", then grab\n>> \"refs/heads/foo.lock\" before updating \"logs/HEAD\". So both processes\n>> could be writing \"logs/HEAD\" at the same time.\n>> ...\n>> Switching to holding only \"HEAD.lock\" while updating \"logs/HEAD\" is the\n>> right solution,...\n>\n> We convinced ourselves that not locking the symref is wrong, but\n> have we actually convinced us that not locking the underlying ref,\n> as long as we have a lock on the symref, is safe?\n>\n> To protect you, the holder of a lock on refs/remotes/origin/HEAD\n> that happens to point at refs/remotes/origin/next, from somebody who\n> is updating the underlying refs/remotes/origin/next directly without\n> going through the symbolic ref (e.g. receive-pack), wouldn't the\n> other party need to find any and all symbolic refs that point at the\n> underlying ref and take locks on them?\n\nAs we're just modifying the ref log of HEAD in this case, we don't bother\nwith where the HEAD points to. The other party may change\nrefs/remotes/origin/next without us noticing, but we don't care here as\nall we do is rewriting the ref log for HEAD.\n\nIf the other party were to modify HEAD (point it to some other branch, or\nforward the branch pointed to), they'd be expected to lock HEAD and\nwould fail as we have the lock?\n\n\n>\n> As dereferencing a symbolic ref in the forward direction is much\n> cheaper than in the reverse, and because you need to dereference it\n> anyway, I wonder if we want the endgame to be \"hold locks on both\",\n> not \"just hold a lock on the symlink\".\n\nThat would be the safest option indeed.\n"},{"id":"256069","messageId":"xmqqk2zloeg1.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"CAGZ79kZpCjcGeifbLztpNUSq7-3Yy2_GEVPGEQsxrgoZfLFU+g@mail.gmail.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-13T18:26:38Z","receivedAt":"2015-02-13T18:26:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> We convinced ourselves that not locking the symref is wrong, but\n>> have we actually convinced us that not locking the underlying ref,\n>> as long as we have a lock on the symref, is safe?\n>>\n>> To protect you, the holder of a lock on refs/remotes/origin/HEAD\n>> that happens to point at refs/remotes/origin/next, from somebody who\n>> is updating the underlying refs/remotes/origin/next directly without\n>> going through the symbolic ref (e.g. receive-pack), wouldn't the\n>> other party need to find any and all symbolic refs that point at the\n>> underlying ref and take locks on them?\n>\n> As we're just modifying the ref log of HEAD in this case, we don't bother\n> with where the HEAD points to. The other party may change\n> refs/remotes/origin/next without us noticing, but we don't care here as\n> all we do is rewriting the ref log for HEAD.\n>\n> If the other party were to modify HEAD (point it to some other branch, or\n> forward the branch pointed to), they'd be expected to lock HEAD and\n> would fail as we have the lock?\n\nI was not talking about HEAD in what you are responding to, though.\nDon't we maintain a reflog on refs/remotes/origin/HEAD?  Is it OK to\nallow its entries to become inconsistent with the underlying ref?\n"},{"id":"256071","messageId":"CAGZ79kYRi3KYcvNQbkhP0uLFgpJzD+h=P+smOLQy2Msv0C_1kw@mail.gmail.com","threadId":"38535","inReplyTo":"xmqqk2zloeg1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-13T18:32:06Z","receivedAt":"2015-02-13T18:32:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 13, 2015 at 10:26 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>> We convinced ourselves that not locking the symref is wrong, but\n>>> have we actually convinced us that not locking the underlying ref,\n>>> as long as we have a lock on the symref, is safe?\n>>>\n>>> To protect you, the holder of a lock on refs/remotes/origin/HEAD\n>>> that happens to point at refs/remotes/origin/next, from somebody who\n>>> is updating the underlying refs/remotes/origin/next directly without\n>>> going through the symbolic ref (e.g. receive-pack), wouldn't the\n>>> other party need to find any and all symbolic refs that point at the\n>>> underlying ref and take locks on them?\n>>\n>> As we're just modifying the ref log of HEAD in this case, we don't bother\n>> with where the HEAD points to. The other party may change\n>> refs/remotes/origin/next without us noticing, but we don't care here as\n>> all we do is rewriting the ref log for HEAD.\n>>\n>> If the other party were to modify HEAD (point it to some other branch, or\n>> forward the branch pointed to), they'd be expected to lock HEAD and\n>> would fail as we have the lock?\n>\n> I was not talking about HEAD in what you are responding to, though.\n> Don't we maintain a reflog on refs/remotes/origin/HEAD?  Is it OK to\n> allow its entries to become inconsistent with the underlying ref?\n>\n\nYes we do have a HEAD ref log, and that ref log would differ from\nthe underlying ref log. I don't know if that is desired, but I would tend to\nnot like it.\n\nSo the other party updating the underlying ref would also need to lock\nHEAD then,\nwhich ... they don't. But they try writing to it nevertheless, in\nrefs.c line 3121-3150\nsee the /* special hack */ comment.\n"},{"id":"256072","messageId":"xmqqa90hocc5.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"CAGZ79kYRi3KYcvNQbkhP0uLFgpJzD+h=P+smOLQy2Msv0C_1kw@mail.gmail.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-13T19:12:10Z","receivedAt":"2015-02-13T19:12:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Feb 13, 2015 at 10:26 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>\n>>>> We convinced ourselves that not locking the symref is wrong, but\n>>>> have we actually convinced us that not locking the underlying ref,\n>>>> as long as we have a lock on the symref, is safe?\n>>>>\n>>>> To protect you, the holder of a lock on refs/remotes/origin/HEAD\n>>>> that happens to point at refs/remotes/origin/next, from somebody who\n>>>> is updating the underlying refs/remotes/origin/next directly without\n>>>> going through the symbolic ref (e.g. receive-pack), wouldn't the\n>>>> other party need to find any and all symbolic refs that point at the\n>>>> underlying ref and take locks on them?\n>>>\n>>> As we're just modifying the ref log of HEAD in this case, we don't bother\n>>> with where the HEAD points to. The other party may change\n>>> refs/remotes/origin/next without us noticing, but we don't care here as\n>>> all we do is rewriting the ref log for HEAD.\n>>>\n>>> If the other party were to modify HEAD (point it to some other branch, or\n>>> forward the branch pointed to), they'd be expected to lock HEAD and\n>>> would fail as we have the lock?\n>>\n>> I was not talking about HEAD in what you are responding to, though.\n>> Don't we maintain a reflog on refs/remotes/origin/HEAD?  Is it OK to\n>> allow its entries to become inconsistent with the underlying ref?\n>\n> Yes we do have a HEAD ref log, and that ref log would differ from\n> the underlying ref log. I don't know if that is desired, but I\n> would tend to not like it.\n\nHEAD (or refs/remotes/origin/HEAD) reflog and reflog for\nrefs/heads/master (or refs/remotes/origin/next) would have to be\ndifferent as long as we allow symbolic refs to be repointed to\ndifferent refs.  If HEAD refers to 'next' today, and at the tip of\nnext sits commit X, the reflog for both of them would record the\nfact that they were pointing at X.  If you repoint HEAD to point at\n'master' (e.g. \"git checkout master\") whose tip is at commit Y, then\nreflog for HEAD would record the fact that now HEAD points at Y, and\nreflogs for 'master' or 'next' would not change merely because you\nswitched where HEAD points at.\n\nAnd there is anything to like or not to like about it.\n\nAs we are trying to see a way to move forward to do the right thing\naround reflog, I was wondering if locking only the symbolic ref is a\nsensible endgame.  \"The right thing\" being:\n\n   When a symbolic ref S points at underlying ref R, and if R's tip\n   changes from X to Y, we would want to know from the reflog of S\n   that S used to point at X and then changed to point at Y.\n\nReplace S and R with either (HEAD, refs/heads/master) or\n(refs/remotes/origin/HEAD, refs/remotes/origin/next) in the above\nand we want both to be true.\n\nHow best to achieve that, and what is the set of right ref(s) to\ntake lock(s) on?  I am not very much interested in how incorrect\ntoday's code might behave.  That is not the central point when\ndiscussing what is the best way forward.\n\n> So the other party updating the underlying ref would also need to lock\n> HEAD then,\n\nYes, that is what I meant.  Your \"also\" can be read in two different\nways (\"other party, too\" or \"HEAD, too\"), though, and I think we\nwant both ;-).  That is why I hinted that it was iffy to state that\nwe only have to take the lock only on S and not on R, but only as a\nworkaround to keep older implementation out we take both---once they\nget extinct we can get away with by taking a lock only on S.\n\nWhen pushing to update 'master' and 'next' into a repository whose\n'HEAD' points at 'master', we would want to take locks on 'next' (no\nquestion), but is it sensible to take the lock on 'HEAD' and\ndeliberately leave 'master' unlocked?  Or is it more sensible to\ntake all locks on the underlying refs involved (i.e. 'next' and\n'master') and in addition any symbolic refs that are pointing at\nthese refs involved (i.e. 'HEAD')?\n"},{"id":"256078","messageId":"54DE5A72.6030106@alum.mit.edu","threadId":"38535","inReplyTo":"xmqqa90hocc5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-13T20:11:30Z","receivedAt":"2015-02-13T20:11:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/13/2015 08:12 PM, Junio C Hamano wrote:\n> [...]\n> As we are trying to see a way to move forward to do the right thing\n> around reflog, I was wondering if locking only the symbolic ref is a\n> sensible endgame.  \"The right thing\" being:\n> \n>    When a symbolic ref S points at underlying ref R, and if R's tip\n>    changes from X to Y, we would want to know from the reflog of S\n>    that S used to point at X and then changed to point at Y.\n\nLet's first talk about an ideal world if we had complete support for\nsymbolic references.\n\nYes, I agree with your principle. Moreover, suppose that S and S2 *both*\npoint at R, and there is a third symref S3 that points at symref S\n(symbolic refs can point at other symbolic refs!):\n\nX <- R <- S <- S3\n        \\\n         S2\n\nNow, if R updated from X to Y (regardless of whether the update is via R\ndirectly or via one of the symrefs), then each of the four reflogs (R,\nS, S2, and S3) should gain a new entry reflecting the update.\n\nIf S is reseated to point at R2 instead of R, then the reflogs for S and\nfor S3 should each gain new entries\n\nWhat locks should we hold? In my opinion, we should hold the locks on\nexactly those references (symbolic or regular) whose reflogs we want to\nchange. So in the first example, we should hold\n\n    $GIT_DIR/$R.lock\n    $GIT_DIR/$S.lock\n    $GIT_DIR/$S2.lock, and\n    $GIT_DIR/$S3.lock\n\nIdeally, we should acquire all of the locks before making any modifications.\n\n\nNow back to the real world. Currently, if R is changed *through* a\nsymbolic reference S, then the reflogs for both R and S are updated, but\nnot the reflogs for any other symbolic references that might point at R.\nIf R is changed directly, then no symref's reflogs are affected, except\nfor the special case that HEAD's reflog is changed if it points directly\nat R. This limitation is a hack to avoid having to walk symrefs\nbackwards to find any symrefs that might be pointing at R.\n\nIf a symref is reseated, then its reflog is changed but not that of any\nsymrefs that might be pointed at it.\n\nIt might actually not be extremely expensive to follow symrefs\nbackwards. Symbolic references cannot be packed, so we would only have\nto scan the loose references; we could ignore packed refs. But it would\nstill be a lot more expensive than just updating one file. I don't know\nthat it's worth it, given that symbolic references are used so sparingly.\n\nI think that the rule about locks as expressed above can be carried over\nthe the real world:\n\n    We should hold the locks on exactly those references (symbolic\n    or regular) whose reflogs we plan to change. We should acquire all\n    of the locks before making any changes.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"256090","messageId":"xmqq3869mqbf.fsf@gitster.dls.corp.google.com","threadId":"38535","inReplyTo":"54DE5A72.6030106@alum.mit.edu","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-13T21:53:08Z","receivedAt":"2015-02-13T21:53:08Z","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> Now back to the real world. Currently, if R is changed *through* a\n> symbolic reference S, then the reflogs for both R and S are updated, but\n> not the reflogs for any other symbolic references that might point at R.\n> If R is changed directly, then no symref's reflogs are affected, except\n> for the special case that HEAD's reflog is changed if it points directly\n> at R. This limitation is a hack to avoid having to walk symrefs\n> backwards to find any symrefs that might be pointing at R.\n\nYup.\n\n> It might actually not be extremely expensive to follow symrefs\n> backwards. Symbolic references cannot be packed, so we would only have\n> to scan the loose references; we could ignore packed refs. But it would\n> still be a lot more expensive than just updating one file. I don't know\n> that it's worth it, given that symbolic references are used so sparingly.\n\nI personally do not think it is worth it.  I further think that it\nwould be perfectly OK to do one of the following:\n\n    - We only maintain reflogs for $GIT_DIR/HEAD; no other symrefs\n      get their own reflog, and we only check $GIT_DIR/HEAD when\n      updating refs/heads/* and no other refs for direct reference\n      (i.e. HEAD -> refs/something/else -> refs/heads/master symref\n      chain is ignored).\n\n    - In addition to the above, we also maintain reflogs for\n      $GIT_DIR/refs/remotes/*/HEAD but support only when they\n      directly point into a remote tracking branch in the same\n      hierarchy.  $GIT_DIR/refs/remotes/foo/HEAD that points at\n      $GIT_DIR/refs/remotes/bar/master is ignored and will get an\n      undefined behaviour.\n\n> I think that the rule about locks as expressed above can be carried over\n> the the real world:\n>\n>     We should hold the locks on exactly those references (symbolic\n>     or regular) whose reflogs we plan to change. We should acquire all\n>     of the locks before making any changes.\n\nSure.\n"},{"id":"256098","messageId":"54DEE3ED.4020408@alum.mit.edu","threadId":"38535","inReplyTo":"xmqq3869mqbf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-14T05:58:05Z","receivedAt":"2015-02-14T05:58:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/13/2015 10:53 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Now back to the real world. Currently, if R is changed *through* a\n>> symbolic reference S, then the reflogs for both R and S are updated, but\n>> not the reflogs for any other symbolic references that might point at R.\n>> If R is changed directly, then no symref's reflogs are affected, except\n>> for the special case that HEAD's reflog is changed if it points directly\n>> at R. This limitation is a hack to avoid having to walk symrefs\n>> backwards to find any symrefs that might be pointing at R.\n> \n> Yup.\n> \n>> It might actually not be extremely expensive to follow symrefs\n>> backwards. Symbolic references cannot be packed, so we would only have\n>> to scan the loose references; we could ignore packed refs. But it would\n>> still be a lot more expensive than just updating one file. I don't know\n>> that it's worth it, given that symbolic references are used so sparingly.\n> \n> I personally do not think it is worth it.  I further think that it\n> would be perfectly OK to do one of the following:\n> \n>     - We only maintain reflogs for $GIT_DIR/HEAD; no other symrefs\n>       get their own reflog, and we only check $GIT_DIR/HEAD when\n>       updating refs/heads/* and no other refs for direct reference\n>       (i.e. HEAD -> refs/something/else -> refs/heads/master symref\n>       chain is ignored).\n> \n>     - In addition to the above, we also maintain reflogs for\n>       $GIT_DIR/refs/remotes/*/HEAD but support only when they\n>       directly point into a remote tracking branch in the same\n>       hierarchy.  $GIT_DIR/refs/remotes/foo/HEAD that points at\n>       $GIT_DIR/refs/remotes/bar/master is ignored and will get an\n>       undefined behaviour.\n\nYes. The first is approximately the status quo, except that you would\nlike explicitly to *suppress* creating reflogs for symbolic refs other\nthan HEAD even if a reference is altered via the symbolic ref.\n\nThe second makes sense, though I think reflogs for remote HEADs are far\nless useful than those for HEAD. So I think this is a low-priority project.\n\n>> I think that the rule about locks as expressed above can be carried over\n>> the the real world:\n>>\n>>     We should hold the locks on exactly those references (symbolic\n>>     or regular) whose reflogs we plan to change. We should acquire all\n>>     of the locks before making any changes.\n> \n> Sure.\n\nI forgot to mention that if we want to retain lock-compatibility with\nolder clients, then we *also* need to lock the reference pointed at by a\nsymbolic ref when modifying the symbolic ref's reflog. This is often\nimplied by the previous rule, but not when we reseat a symbolic\nreference or when we expire a symbolic reference's reflog.\n\nI will look at how hard this is to implement. If it is at all involved,\nthen I might drop this patch from the current patch series and defer it\nto another one.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}