[PATCH v3 0/7] Fix some problems with reflog expiration

[PATCH v3 0/7] Fix some problems with reflog expiration

[Michael Haggerty]

This is v3 of the patch series. Thanks to Junio for his comments
about v2 [1]; I think this version addresses all of his points.

Changes since v2:

* Introduce a temporary in "struct ref_lock: delete the force_write
  member" to make a complicated boolean expression easier to
  understand.

* Split the documentation of options for "reflog expire" and "reflog
  delete", and make another couple tweaks to the "reflog"
  documentation.

* Add a NEEDSWORK comment suggesting that the "reflog" command should
  use parse_options() to process its command line.

[1] http://thread.gmane.org/gmane.comp.version-control.git/264586

Michael Haggerty (6):
  write_ref_sha1(): remove check for lock == NULL
  write_ref_sha1(): Move write elision test to callers
  lock_ref_sha1_basic(): do not set force_write for missing references
  reflog: improve and update documentation
  reflog_expire(): ignore --updateref for symbolic references
  reflog_expire(): never update a reference to null_sha1

Stefan Beller (1):
  struct ref_lock: delete the force_write member

 Documentation/git-reflog.txt | 144 ++++++++++++++++++++++++++-----------------
 builtin/reflog.c             |   9 +--
 refs.c                       |  65 ++++++++++---------
 3 files changed, 126 insertions(+), 92 deletions(-)

-- 
2.1.4

[PATCH v3 1/7] write_ref_sha1(): remove check for lock == NULL

[Michael Haggerty]

None of the callers pass NULL to this function, and there doesn't seem
to be any usefulness to allowing them to do so.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/refs.c b/refs.c
index ab2f2a9..e82d503 100644
--- a/refs.c
+++ b/refs.c
@@ -3079,10 +3079,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 	static char term = '\n';
 	struct object *o;
 
-	if (!lock) {
-		errno = EINVAL;
-		return -1;
-	}
 	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
 		unlock_ref(lock);
 		return 0;
-- 
2.1.4

[PATCH v3 2/7] write_ref_sha1(): Move write elision test to callers

[Michael Haggerty]

write_ref_sha1() previously skipped the write if the reference already
had the desired value, unless lock->force_write was set. Instead,
perform that test at the callers.

Two of the callers (in rename_ref()) unconditionally set force_write
just before calling write_ref_sha1(), so they don't need the extra
check at all. Nor do they need to set force_write anymore.

The last caller, in ref_transaction_commit(), still needs the test.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index e82d503..dd30bfa 100644
--- a/refs.c
+++ b/refs.c
@@ -2877,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		error("unable to lock %s for update", newrefname);
 		goto rollback;
 	}
-	lock->force_write = 1;
 	hashcpy(lock->old_sha1, orig_sha1);
 	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
 		error("unable to write current sha1 into %s", newrefname);
@@ -2893,7 +2892,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		goto rollbacklog;
 	}
 
-	lock->force_write = 1;
 	flag = log_all_ref_updates;
 	log_all_ref_updates = 0;
 	if (write_ref_sha1(lock, orig_sha1, NULL))
@@ -3079,10 +3077,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 	static char term = '\n';
 	struct object *o;
 
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
-		unlock_ref(lock);
-		return 0;
-	}
 	o = parse_object(sha1);
 	if (!o) {
 		error("Trying to write ref %s with nonexistent object %s",
@@ -3796,15 +3790,21 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (!is_null_sha1(update->new_sha1)) {
-			if (write_ref_sha1(update->lock, update->new_sha1,
-					   update->msg)) {
+			if (!update->lock->force_write &&
+			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
+				unlock_ref(update->lock);
+				update->lock = NULL;
+			} else if (write_ref_sha1(update->lock, update->new_sha1,
+						  update->msg)) {
 				update->lock = NULL; /* freed by write_ref_sha1 */
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
+			} else {
+				/* freed by write_ref_sha1(): */
+				update->lock = NULL;
 			}
-			update->lock = NULL; /* freed by write_ref_sha1 */
 		}
 	}
 
-- 
2.1.4

[PATCH v3 3/7] lock_ref_sha1_basic(): do not set force_write for missing references

[Michael Haggerty]

If a reference is missing, its SHA-1 will be null_sha1, which can't
possibly match a new value that ref_transaction_commit() is trying to
update it to. So there is no need to set force_write in this scenario.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index dd30bfa..3ed9ea6 100644
--- a/refs.c
+++ b/refs.c
@@ -2258,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int type, lflags;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 	int resolve_flags = 0;
-	int missing = 0;
 	int attempts_remaining = 3;
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2297,13 +2296,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			orig_refname, strerror(errno));
 		goto error_return;
 	}
-	missing = is_null_sha1(lock->old_sha1);
-	/* When the ref did not exist and we are creating it,
-	 * make sure there is no existing ref that is packed
-	 * whose name begins with our refname, nor a ref whose
-	 * name is a proper prefix of our refname.
+	/*
+	 * If the ref did not exist and we are creating it, make sure
+	 * there is no existing packed ref whose name begins with our
+	 * refname, nor a packed ref whose name is a proper prefix of
+	 * our refname.
 	 */
-	if (missing &&
+	if (is_null_sha1(lock->old_sha1) &&
 	     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
 		last_errno = ENOTDIR;
 		goto error_return;
@@ -2319,8 +2318,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	lock->ref_name = xstrdup(refname);
 	lock->orig_ref_name = xstrdup(orig_refname);
 	ref_file = git_path("%s", refname);
-	if (missing)
-		lock->force_write = 1;
 	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
 		lock->force_write = 1;
 
-- 
2.1.4

[PATCH v3 4/7] struct ref_lock: delete the force_write member

[Michael Haggerty]

From: Stefan Beller <sbeller@google.com>

Instead, compute the value when it is needed.

Signed-off-by: Stefan Beller <sbeller@google.com>
Edited-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 3ed9ea6..7a2f53f 100644
--- a/refs.c
+++ b/refs.c
@@ -12,7 +12,6 @@ struct ref_lock {
 	struct lock_file *lk;
 	unsigned char old_sha1[20];
 	int lock_fd;
-	int force_write;
 };
 
 /*
@@ -2318,8 +2317,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	lock->ref_name = xstrdup(refname);
 	lock->orig_ref_name = xstrdup(orig_refname);
 	ref_file = git_path("%s", refname);
-	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
-		lock->force_write = 1;
 
  retry:
 	switch (safe_create_leading_directories(ref_file)) {
@@ -3787,8 +3784,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (!is_null_sha1(update->new_sha1)) {
-			if (!update->lock->force_write &&
-			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
+			int overwriting_symref = ((update->type & REF_ISSYMREF) &&
+						  (update->flags & REF_NODEREF));
+
+			if (!overwriting_symref
+			    && !hashcmp(update->lock->old_sha1, update->new_sha1)) {
+				/*
+				 * The reference already has the desired
+				 * value, so we don't need to write it.
+				 */
 				unlock_ref(update->lock);
 				update->lock = NULL;
 			} else if (write_ref_sha1(update->lock, update->new_sha1,
-- 
2.1.4

[PATCH v3 5/7] reflog: improve and update documentation

[Michael Haggerty]

Revamp the "git reflog" usage documentation in the manpage and the
command help to match the current reality and improve its clarity:

* Add documentation for some options that had been left out.

* Group the subcommands and options more logically and move more
  common subcommands/options higher.

* Improve some explanations.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-reflog.txt | 143 +++++++++++++++++++++++++------------------
 builtin/reflog.c             |   9 +--
 2 files changed, 88 insertions(+), 64 deletions(-)

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index 70791b9..730106c 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -17,85 +17,112 @@ The command takes various subcommands, and different options
 depending on the subcommand:
 
 [verse]
-'git reflog expire' [--dry-run] [--stale-fix] [--verbose]
-	[--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...
-'git reflog delete' ref@\{specifier\}...
 'git reflog' ['show'] [log-options] [<ref>]
+'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]
+	[--rewrite] [--updateref] [--stale-fix]
+	[--dry-run] [--verbose] [--all | <refs>...]
+'git reflog delete' [--rewrite] [--updateref]
+	[--dry-run] [--verbose] ref@\{specifier\}...
+
+Reference logs, or "reflogs", record when the tips of branches and
+other references were updated in the local repository. Reflogs are
+useful in various Git commands, to specify the old value of a
+reference. For example, `HEAD@{2}` means "where HEAD used to be two
+moves ago", `master@{one.week.ago}` means "where master used to point
+to one week ago in this local repository", and so on. See
+linkgit:gitrevisions[7] for more details.
+
+This command manages the information recorded in the reflogs.
+
+The "show" subcommand (which is also the default, in the absence of
+any subcommands) shows the log of the reference provided in the
+command-line (or `HEAD`, by default). The reflog covers all recent
+actions, and in addition the `HEAD` reflog records branch switching.
+`git reflog show` is an alias for `git log -g --abbrev-commit
+--pretty=oneline`; see linkgit:git-log[1] for more information.
+
+The "expire" subcommand prunes older reflog entries. Entries older
+than `expire` time, or entries older than `expire-unreachable` time
+and not reachable from the current tip, are removed from the reflog.
+This is typically not used directly by end users -- instead, see
+linkgit:git-gc[1].
+
+The "delete" subcommand deletes single entries from the reflog. Its
+argument must be an _exact_ entry (e.g. "`git reflog delete
+master@{2}`"). This subcommand is also typically not used directly by
+end users.
 
-Reflog is a mechanism to record when the tip of branches are
-updated.  This command is to manage the information recorded in it.
 
-The subcommand "expire" is used to prune older reflog entries.
-Entries older than `expire` time, or entries older than
-`expire-unreachable` time and not reachable from the current
-tip, are removed from the reflog.  This is typically not used
-directly by the end users -- instead, see linkgit:git-gc[1].
-
-The subcommand "show" (which is also the default, in the absence of any
-subcommands) will take all the normal log options, and show the log of
-the reference provided in the command-line (or `HEAD`, by default).
-The reflog will cover all recent actions (HEAD reflog records branch switching
-as well).  It is an alias for `git log -g --abbrev-commit --pretty=oneline`;
-see linkgit:git-log[1].
+OPTIONS
+-------
 
-The reflog is useful in various Git commands, to specify the old value
-of a reference. For example, `HEAD@{2}` means "where HEAD used to be
-two moves ago", `master@{one.week.ago}` means "where master used to
-point to one week ago", and so on. See linkgit:gitrevisions[7] for
-more details.
+Options for `show`
+~~~~~~~~~~~~~~~~~~
 
-To delete single entries from the reflog, use the subcommand "delete"
-and specify the _exact_ entry (e.g. "`git reflog delete master@{2}`").
+`git reflog show` accepts any of the options accepted by `git log`.
 
 
-OPTIONS
--------
+Options for `expire`
+~~~~~~~~~~~~~~~~~~~~
 
---stale-fix::
-	This revamps the logic -- the definition of "broken commit"
-	becomes: a commit that is not reachable from any of the refs and
-	there is a missing object among the commit, tree, or blob
-	objects reachable from it that is not reachable from any of the
-	refs.
-+
-This computation involves traversing all the reachable objects, i.e. it
-has the same cost as 'git prune'.  Fortunately, once this is run, we
-should not have to ever worry about missing objects, because the current
-prune and pack-objects know about reflogs and protect objects referred by
-them.
+--all::
+	Process the reflogs of all references.
 
 --expire=<time>::
-	Entries older than this time are pruned.  Without the
-	option it is taken from configuration `gc.reflogExpire`,
-	which in turn defaults to 90 days.  --expire=all prunes
-	entries regardless of their age; --expire=never turns off
-	pruning of reachable entries (but see --expire-unreachable).
+	Prune entries older than the specified time. If this option is
+	not specified, the expiration time is taken from the
+	configuration setting `gc.reflogExpire`, which in turn
+	defaults to 90 days. `--expire=all` prunes entries regardless
+	of their age; `--expire=never` turns off pruning of reachable
+	entries (but see `--expire-unreachable`).
 
 --expire-unreachable=<time>::
-	Entries older than this time and not reachable from
-	the current tip of the branch are pruned.  Without the
-	option it is taken from configuration
-	`gc.reflogExpireUnreachable`, which in turn defaults to
-	30 days.  --expire-unreachable=all prunes unreachable
-	entries regardless of their age; --expire-unreachable=never
+	Prune entries older than `<time>` that are not reachable from
+	the current tip of the branch. If this option is not
+	specified, the expiration time is taken from the configuration
+	setting `gc.reflogExpireUnreachable`, which in turn defaults
+	to 30 days. `--expire-unreachable=all` prunes unreachable
+	entries regardless of their age; `--expire-unreachable=never`
 	turns off early pruning of unreachable entries (but see
-	--expire).
-
---all::
-	Instead of listing <refs> explicitly, prune all refs.
+	`--expire`).
 
 --updateref::
-	Update the ref with the sha1 of the top reflog entry (i.e.
-	<ref>@\{0\}) after expiring or deleting.
+	Update the reference to the value of the top reflog entry (i.e.
+	<ref>@\{0\}) if the previous top entry was pruned.
 
 --rewrite::
-	While expiring or deleting, adjust each reflog entry to ensure
-	that the `old` sha1 field points to the `new` sha1 field of the
-	previous entry.
+	If a reflog entry's predecessor is pruned, adjust its "old"
+	SHA-1 to be equal to the "new" SHA-1 field of the entry that
+	now precedes it.
+
+--stale-fix::
+	Prune any reflog entries that point to "broken commits". A
+	broken commit is a commit that is not reachable from any of
+	the reference tips and that refers, directly or indirectly, to
+	a missing commit, tree, or blob object.
++
+This computation involves traversing all the reachable objects, i.e. it
+has the same cost as 'git prune'.  It is primarily intended to fix
+corruption caused by garbage collecting using older versions of Git,
+which didn't protect objects referred to by reflogs.
+
+-n::
+--dry-run::
+	Do not actually prune any entries; just show what would have
+	been pruned.
 
 --verbose::
 	Print extra information on screen.
 
+
+Options for `delete`
+~~~~~~~~~~~~~~~~~~~~
+
+`git reflog delete` accepts options `--updateref`, `--rewrite`, `-n`,
+`--dry-run`, and `--verbose`, with the same meanings as when they are
+used with `expire`.
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 49c64f9..8182b64 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -8,14 +8,11 @@
 #include "revision.h"
 #include "reachable.h"
 
-/*
- * reflog expire
- */
-
+/* NEEDSWORK: switch to using parse_options */
 static const char reflog_expire_usage[] =
-"git reflog expire [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
+"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...";
 static const char reflog_delete_usage[] =
-"git reflog delete [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...";
+"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...";
 
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
-- 
2.1.4

[PATCH v3 6/7] reflog_expire(): ignore --updateref for symbolic references

[Michael Haggerty]

If we are expiring reflog entries for a symbolic reference, then how
should --updateref be handled if the newest reflog entry is expired?

Option 1: Update the referred-to reference. (This is what the current
code does.) This doesn't make sense, because the referred-to reference
has its own reflog, which hasn't been rewritten.

Option 2: Update the symbolic reference itself (as in, REF_NODEREF).
This would convert the symbolic reference into a non-symbolic
reference (e.g., detaching HEAD), which is surely not what a user
would expect.

Option 3: Error out. This is plausible, but it would make the
following usage impossible:

    git reflog expire ... --updateref --all

Option 4: Ignore --updateref for symbolic references.

We choose to implement option 4.

Note: another problem in this code will be fixed in a moment.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-reflog.txt |  3 ++-
 refs.c                       | 15 ++++++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index 730106c..5e7908e 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -88,7 +88,8 @@ Options for `expire`
 
 --updateref::
 	Update the reference to the value of the top reflog entry (i.e.
-	<ref>@\{0\}) if the previous top entry was pruned.
+	<ref>@\{0\}) if the previous top entry was pruned.  (This
+	option is ignored for symbolic references.)
 
 --rewrite::
 	If a reflog entry's predecessor is pruned, adjust its "old"
diff --git a/refs.c b/refs.c
index 7a2f53f..48bb9e8 100644
--- a/refs.c
+++ b/refs.c
@@ -4028,6 +4028,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	struct ref_lock *lock;
 	char *log_file;
 	int status = 0;
+	int type;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.flags = flags;
@@ -4039,7 +4040,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);
 	if (!lock)
 		return error("cannot lock ref '%s'", refname);
 	if (!reflog_exists(refname)) {
@@ -4076,10 +4077,18 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	(*cleanup_fn)(cb.policy_cb);
 
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+		/*
+		 * It doesn't make sense to adjust a reference pointed
+		 * to by a symbolic ref based on expiring entries in
+		 * the symbolic reference's reflog.
+		 */
+		int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+			!(type & REF_ISSYMREF);
+
 		if (close_lock_file(&reflog_lock)) {
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+		} else if (update &&
 			(write_in_full(lock->lock_fd,
 				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
 			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
@@ -4090,7 +4099,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 		} else if (commit_lock_file(&reflog_lock)) {
 			status |= error("unable to commit reflog '%s' (%s)",
 					log_file, strerror(errno));
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
+		} else if (update && commit_ref(lock)) {
 			status |= error("couldn't set %s", lock->ref_name);
 		}
 	}
-- 
2.1.4

[PATCH v3 7/7] reflog_expire(): never update a reference to null_sha1

[Michael Haggerty]

Currently, if --updateref is specified and the very last reflog entry
is expired or deleted, the reference's value is set to 0{40}. This is
an invalid state of the repository, and breaks, for example, "git
fsck" and "git for-each-ref".

The only place we use --updateref in our own code is when dropping
stash entries. In that code, the very next step is to check if the
reflog has been made empty, and if so, delete the "refs/stash"
reference entirely. Thus that code path ultimately leaves the
repository in a valid state.

But we don't want to the repository in an invalid state even
temporarily, and we don't want to leave an invalid state if other
callers of "git reflog expire|delete --updateref" don't think to do
the extra cleanup step.

So, if "git reflog expire|delete" leaves no more entries in the
reflog, just leave the reference unchanged.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 48bb9e8..d6244b6 100644
--- a/refs.c
+++ b/refs.c
@@ -4080,10 +4080,13 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 		/*
 		 * It doesn't make sense to adjust a reference pointed
 		 * to by a symbolic ref based on expiring entries in
-		 * the symbolic reference's reflog.
+		 * the symbolic reference's reflog. Nor can we update
+		 * a reference if there are no remaining reflog
+		 * entries.
 		 */
 		int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			!(type & REF_ISSYMREF);
+			!(type & REF_ISSYMREF) &&
+			!is_null_sha1(cb.last_kept_sha1);
 
 		if (close_lock_file(&reflog_lock)) {
 			status |= error("couldn't write %s: %s", log_file,
-- 
2.1.4

Re: [PATCH v3 0/7] Fix some problems with reflog expiration

[Stefan Beller]

On Tue, Mar 3, 2015 at 3:43 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This is v3 of the patch series. Thanks to Junio for his comments
> about v2 [1]; I think this version addresses all of his points.
>
> Changes since v2:
>
> * Introduce a temporary in "struct ref_lock: delete the force_write
>   member" to make a complicated boolean expression easier to
>   understand.
>
> * Split the documentation of options for "reflog expire" and "reflog
>   delete", and make another couple tweaks to the "reflog"
>   documentation.
>
> * Add a NEEDSWORK comment suggesting that the "reflog" command should
>   use parse_options() to process its command line.
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/264586

The whole series is
Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks for adjusting the commit from me to fit in better into the series.
Thanks,
Stefan

>
> Michael Haggerty (6):
>   write_ref_sha1(): remove check for lock == NULL
>   write_ref_sha1(): Move write elision test to callers
>   lock_ref_sha1_basic(): do not set force_write for missing references
>   reflog: improve and update documentation
>   reflog_expire(): ignore --updateref for symbolic references
>   reflog_expire(): never update a reference to null_sha1
>
> Stefan Beller (1):
>   struct ref_lock: delete the force_write member
>
>  Documentation/git-reflog.txt | 144 ++++++++++++++++++++++++++-----------------
>  builtin/reflog.c             |   9 +--
>  refs.c                       |  65 ++++++++++---------
>  3 files changed, 126 insertions(+), 92 deletions(-)
>
> --
> 2.1.4
>