HEAD's reflog entry for a renamed branch

HEAD's reflog entry for a renamed branch

[Kyle Meyer]

Hello,

I noticed that, after renaming the current branch, the corresponding
message in .git/logs/HEAD is empty.

For example, running

    $ mkdir test-repo
    $ cd test-repo
    $ git init
    $ echo abc >file.txt
    $ git add file.txt
    $ git commit -m"Add file.txt"
    $ git branch -m master new-master

resulted in the following reflogs:

   $ cat .git/logs/refs/heads/new-master
   00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	commit (initial): Add file.txt
   68730... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	Branch: renamed refs/heads/master to refs/heads/new-master

   $ cat .git/logs/HEAD
   00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	commit (initial): Add file.txt
   68730... 00000... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500

I expected the second line of .git/logs/HEAD to mirror the second line
of .git/logs/refs/heads/new-master.  Are the empty message and null sha1
in HEAD's entry intentional?

--
Kyle

Re: HEAD's reflog entry for a renamed branch

[Jeff King]

On Mon, Jan 16, 2017 at 06:17:29PM -0500, Kyle Meyer wrote:

> I noticed that, after renaming the current branch, the corresponding
> message in .git/logs/HEAD is empty.
> 
> For example, running
> 
>     $ mkdir test-repo
>     $ cd test-repo
>     $ git init
>     $ echo abc >file.txt
>     $ git add file.txt
>     $ git commit -m"Add file.txt"
>     $ git branch -m master new-master

Thanks for providing a simple reproduction recipe.

> resulted in the following reflogs:
> 
>    $ cat .git/logs/refs/heads/new-master
>    00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	commit (initial): Add file.txt
>    68730... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	Branch: renamed refs/heads/master to refs/heads/new-master
> 
>    $ cat .git/logs/HEAD
>    00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	commit (initial): Add file.txt
>    68730... 00000... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500
> 
> I expected the second line of .git/logs/HEAD to mirror the second line
> of .git/logs/refs/heads/new-master.  Are the empty message and null sha1
> in HEAD's entry intentional?

The null sha1 is correct, I think. The branch we were on went away, and
we use the null sha1 to indicate "no value" in both the creation and
deletion cases.

It's unfortunate that there's no message. This is because the rename
calls delete_ref() under the hood, but that function doesn't take a
reflog message argument at all. It usually doesn't matter because
deleting the ref will also delete the reflog.

But as your example shows, deletions _can_ get logged in the HEAD
reflog; the low-level ref code detects when HEAD points to the updated
ref and logs an entry there, too.

You can see the same thing with a simpler example:

  $ git init
  $ git commit -m foo --allow-empty
  $ git update-ref -d refs/heads/master
  $ cat .git/logs/HEAD
  00000...  8ffd1... Jeff King <peff@peff.net> 1485464779 -0500	commit (initial): foo
  8ffd1...  00000... Jeff King <peff@peff.net> 1485464787 -0500

This doesn't come up much because most porcelain commands will refuse to
delete the current branch (notice I had to use "update-ref" and not
"branch -d").

I'd say there are two potential improvements:

  - delete_ref() should take a reflog message argument, in case it
    updates the HEAD reflog (as a bonus, this will future-proof us for a
    day when we might keep reflogs for deleted refs).

  - "git branch -m" does seem to realize when we are renaming HEAD,
    because it updates HEAD to point to the new branch name. But it
    should probably insert another reflog entry mentioning the rename
    (we do for "git checkout foo", even when "foo" has the same sha1 as
    the current HEAD).

-Peff

Re: HEAD's reflog entry for a renamed branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It's unfortunate that there's no message. This is because the rename
> calls delete_ref() under the hood, but that function doesn't take a
> reflog message argument at all. It usually doesn't matter because
> deleting the ref will also delete the reflog.
>
> But as your example shows, deletions _can_ get logged in the HEAD
> reflog; the low-level ref code detects when HEAD points to the updated
> ref and logs an entry there, too.
> ...
> I'd say there are two potential improvements:
>
>   - delete_ref() should take a reflog message argument, in case it
>     updates the HEAD reflog (as a bonus, this will future-proof us for a
>     day when we might keep reflogs for deleted refs).

This sounds sensible.

>   - "git branch -m" does seem to realize when we are renaming HEAD,
>     because it updates HEAD to point to the new branch name. But it
>     should probably insert another reflog entry mentioning the rename
>     (we do for "git checkout foo", even when "foo" has the same sha1 as
>     the current HEAD).

This one I care less (not in the sense that I prefer it not done,
but in the sense that I do not mind it is left unfixed than the
other one you pointed out).

Re: HEAD's reflog entry for a renamed branch

[Jeff King]

On Thu, Jan 26, 2017 at 01:30:54PM -0800, Junio C Hamano wrote:

> >   - "git branch -m" does seem to realize when we are renaming HEAD,
> >     because it updates HEAD to point to the new branch name. But it
> >     should probably insert another reflog entry mentioning the rename
> >     (we do for "git checkout foo", even when "foo" has the same sha1 as
> >     the current HEAD).
> 
> This one I care less (not in the sense that I prefer it not done,
> but in the sense that I do not mind it is left unfixed than the
> other one you pointed out).

I wondered if it might affect how "git checkout -" works. But that
feature looks for reflogs like "checkout: moving from X to Y" to know to
move back to X.  So we are fine here. Even though the HEAD reflog does
not show us going _to_ new-master, we would see it in a later entry as
"from new-master to Y". What we are missing is "rename from master to
new-master", but that entry does not matter. There is no "master" to
go back to anymore. :)

-Peff

[PATCH 0/3] delete_ref(): support reflog messages

[Kyle Meyer]

[Sorry for the slow response.]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 16, 2017 at 06:17:29PM -0500, Kyle Meyer wrote:

[...]

>>    $ cat .git/logs/refs/heads/new-master
>>    00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	commit (initial): Add file.txt
>>    68730... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	Branch: renamed refs/heads/master to refs/heads/new-master
>> 
>>    $ cat .git/logs/HEAD
>>    00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500	commit (initial): Add file.txt
>>    68730... 00000... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500
>> 
>> I expected the second line of .git/logs/HEAD to mirror the second line
>> of .git/logs/refs/heads/new-master.  Are the empty message and null sha1
>> in HEAD's entry intentional?
>
> The null sha1 is correct, I think. The branch we were on went away, and
> we use the null sha1 to indicate "no value" in both the creation and
> deletion cases.

I see, thanks.

> I'd say there are two potential improvements:
>
>   - delete_ref() should take a reflog message argument, in case it
>     updates the HEAD reflog (as a bonus, this will future-proof us for a
>     day when we might keep reflogs for deleted refs).

I've tried to do this in the following patches.

>   - "git branch -m" does seem to realize when we are renaming HEAD,
>     because it updates HEAD to point to the new branch name. But it
>     should probably insert another reflog entry mentioning the rename
>     (we do for "git checkout foo", even when "foo" has the same sha1 as
>     the current HEAD).

I haven't worked out how to do this part yet.  I'm guessing the change
will involve modifying split_head_update().

If this is added, should it be instead of, rather than in addition to,
the deletion entry?  If a "Branch: renamed ..." entry is present, it
doesn't seem like the deletion entry is providing any additional
information.

  delete_refs(): accept a reflog message argument
  update-ref: pass reflog message argument to delete_refs
  rename_ref: replace empty deletion message in HEAD's log

 builtin/am.c           |  4 ++--
 builtin/branch.c       |  2 +-
 builtin/notes.c        |  4 ++--
 builtin/remote.c       |  4 ++--
 builtin/replace.c      |  2 +-
 builtin/reset.c        |  2 +-
 builtin/symbolic-ref.c |  2 +-
 builtin/tag.c          |  2 +-
 builtin/update-ref.c   |  2 +-
 fast-import.c          |  2 +-
 refs.c                 |  4 ++--
 refs.h                 |  2 +-
 refs/files-backend.c   | 12 +++++++++---
 t/t1400-update-ref.sh  |  9 +++++++++
 t/t3200-branch.sh      |  4 ++++
 transport.c            |  2 +-
 16 files changed, 39 insertions(+), 20 deletions(-)

-- 
Kyle

[PATCH 1/3] delete_refs(): accept a reflog message argument

[Kyle Meyer]

When the current branch is renamed with 'git branch -m/-M' or deleted
with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log
with an empty message.

In preparation for adding a more meaningful message to HEAD's log in
these cases, update delete_ref() to take a message argument and pass
it along to ref_transaction_delete().  Modify all callers to pass NULL
for the new message argument; no change in behavior is intended.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 builtin/am.c           | 4 ++--
 builtin/branch.c       | 2 +-
 builtin/notes.c        | 4 ++--
 builtin/remote.c       | 4 ++--
 builtin/replace.c      | 2 +-
 builtin/reset.c        | 2 +-
 builtin/symbolic-ref.c | 2 +-
 builtin/tag.c          | 2 +-
 builtin/update-ref.c   | 2 +-
 fast-import.c          | 2 +-
 refs.c                 | 4 ++--
 refs.h                 | 2 +-
 refs/files-backend.c   | 6 +++---
 transport.c            | 2 +-
 14 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578..e08c739d4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1049,7 +1049,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	} else {
 		write_state_text(state, "abort-safety", "");
 		if (!state->rebasing)
-			delete_ref("ORIG_HEAD", NULL, 0);
+			delete_ref("ORIG_HEAD", NULL, 0, NULL);
 	}
 
 	/*
@@ -2172,7 +2172,7 @@ static void am_abort(struct am_state *state)
 				has_curr_head ? &curr_head : NULL, 0,
 				UPDATE_REFS_DIE_ON_ERR);
 	else if (curr_branch)
-		delete_ref(curr_branch, NULL, REF_NODEREF);
+		delete_ref(curr_branch, NULL, REF_NODEREF, NULL);
 
 	free(curr_branch);
 	am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0..44f23208f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -252,7 +252,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		}
 
 		if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
-			       REF_NODEREF)) {
+			       REF_NODEREF, NULL)) {
 			error(remote_branch
 			      ? _("Error deleting remote-tracking branch '%s'")
 			      : _("Error deleting branch '%s'"),
diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad..991448d4e 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -681,9 +681,9 @@ static int merge_abort(struct notes_merge_options *o)
 	 * notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
 	 */
 
-	if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+	if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0, NULL))
 		ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
-	if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+	if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF, NULL))
 		ret += error(_("failed to delete ref NOTES_MERGE_REF"));
 	if (notes_merge_abort(o))
 		ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 5339ed6ad..bfa8a5189 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -691,7 +691,7 @@ static int mv(int argc, const char **argv)
 		read_ref_full(item->string, RESOLVE_REF_READING, oid.hash, &flag);
 		if (!(flag & REF_ISSYMREF))
 			continue;
-		if (delete_ref(item->string, NULL, REF_NODEREF))
+		if (delete_ref(item->string, NULL, REF_NODEREF, NULL))
 			die(_("deleting '%s' failed"), item->string);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
@@ -1248,7 +1248,7 @@ static int set_head(int argc, const char **argv)
 			head_name = xstrdup(states.heads.items[0].string);
 		free_remote_ref_states(&states);
 	} else if (opt_d && !opt_a && argc == 1) {
-		if (delete_ref(buf.buf, NULL, REF_NODEREF))
+		if (delete_ref(buf.buf, NULL, REF_NODEREF, NULL))
 			result |= error(_("Could not delete %s"), buf.buf);
 	} else
 		usage_with_options(builtin_remote_sethead_usage, options);
diff --git a/builtin/replace.c b/builtin/replace.c
index b58c714cb..d32d0a3ae 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -121,7 +121,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 static int delete_replace_ref(const char *name, const char *ref,
 			      const unsigned char *sha1)
 {
-	if (delete_ref(ref, sha1, 0))
+	if (delete_ref(ref, sha1, 0, NULL))
 		return 1;
 	printf("Deleted replace ref '%s'\n", name);
 	return 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfc..cccd3f099 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -256,7 +256,7 @@ static int reset_refs(const char *rev, const struct object_id *oid)
 		update_ref_oid(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
 			   UPDATE_REFS_MSG_ON_ERR);
 	} else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig->hash, 0);
+		delete_ref("ORIG_HEAD", old_orig->hash, 0, NULL);
 	set_reflog_message(&msg, "updating HEAD", rev);
 	update_ref_status = update_ref_oid(msg.buf, "HEAD", oid, orig, 0,
 				       UPDATE_REFS_MSG_ON_ERR);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 96eed9446..c9d5bd3e8 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -58,7 +58,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 			die("Cannot delete %s, not a symbolic ref", argv[0]);
 		if (!strcmp(argv[0], "HEAD"))
 			die("deleting '%s' is not allowed", argv[0]);
-		return delete_ref(argv[0], NULL, REF_NODEREF);
+		return delete_ref(argv[0], NULL, REF_NODEREF, NULL);
 	}
 
 	switch (argc) {
diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a967..850a0674c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -97,7 +97,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
 static int delete_tag(const char *name, const char *ref,
 		      const unsigned char *sha1, const void *cb_data)
 {
-	if (delete_ref(ref, sha1, 0))
+	if (delete_ref(ref, sha1, 0, NULL))
 		return 1;
 	printf(_("Deleted tag '%s' (was %s)\n"), name, find_unique_abbrev(sha1, DEFAULT_ABBREV));
 	return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7f30d3a76..a41f9adf1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		 */
 		return delete_ref(refname,
 				  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
-				  flags);
+				  flags, NULL);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  flags | create_reflog_flag,
diff --git a/fast-import.c b/fast-import.c
index 64fe602f0..07412a85a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1752,7 +1752,7 @@ static int update_branch(struct branch *b)
 
 	if (is_null_sha1(b->sha1)) {
 		if (b->delete)
-			delete_ref(b->name, NULL, 0);
+			delete_ref(b->name, NULL, 0, NULL);
 		return 0;
 	}
 	if (read_ref(b->name, old_sha1))
diff --git a/refs.c b/refs.c
index cd36b64ed..4c80ebf2e 100644
--- a/refs.c
+++ b/refs.c
@@ -592,7 +592,7 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
 }
 
 int delete_ref(const char *refname, const unsigned char *old_sha1,
-	       unsigned int flags)
+	       unsigned int flags, const char *msg)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
@@ -603,7 +603,7 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_sha1,
-				   flags, NULL, &err) ||
+				   flags, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
diff --git a/refs.h b/refs.h
index 9fbff90e7..81627a63d 100644
--- a/refs.h
+++ b/refs.h
@@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
  * be NULL_SHA1. flags is passed through to ref_transaction_delete().
  */
 int delete_ref(const char *refname, const unsigned char *old_sha1,
-	       unsigned int flags);
+	       unsigned int flags, const char *msg);
 
 /*
  * Delete the specified references. If there are any problems, emit
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c041d4ba2..ffa75d816 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2489,7 +2489,7 @@ static int files_delete_refs(struct ref_store *ref_store,
 	for (i = 0; i < refnames->nr; i++) {
 		const char *refname = refnames->items[i].string;
 
-		if (delete_ref(refname, NULL, flags))
+		if (delete_ref(refname, NULL, flags, NULL))
 			result |= error(_("could not remove reference %s"), refname);
 	}
 
@@ -2616,7 +2616,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
 
-	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
+	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
 		error("unable to delete old %s", oldrefname);
 		goto rollback;
 	}
@@ -2630,7 +2630,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 	 */
 	if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
 			   sha1, NULL) &&
-	    delete_ref(newrefname, NULL, REF_NODEREF)) {
+	    delete_ref(newrefname, NULL, REF_NODEREF, NULL)) {
 		if (errno==EISDIR) {
 			struct strbuf path = STRBUF_INIT;
 			int result;
diff --git a/transport.c b/transport.c
index d72e08948..e025d9b29 100644
--- a/transport.c
+++ b/transport.c
@@ -299,7 +299,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 		if (verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
-			delete_ref(rs.dst, NULL, 0);
+			delete_ref(rs.dst, NULL, 0, NULL);
 		} else
 			update_ref("update by push", rs.dst,
 					ref->new_oid.hash, NULL, 0, 0);
-- 
2.11.1

Re: [PATCH 1/3] delete_refs(): accept a reflog message argument

[Jeff King]

On Thu, Feb 16, 2017 at 10:57:58PM -0500, Kyle Meyer wrote:

> When the current branch is renamed with 'git branch -m/-M' or deleted
> with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log
> with an empty message.
> 
> In preparation for adding a more meaningful message to HEAD's log in
> these cases, update delete_ref() to take a message argument and pass
> it along to ref_transaction_delete().  Modify all callers to pass NULL
> for the new message argument; no change in behavior is intended.

Seems like a good first step.

> diff --git a/refs.h b/refs.h
> index 9fbff90e7..81627a63d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>   */
>  int delete_ref(const char *refname, const unsigned char *old_sha1,
> -	       unsigned int flags);
> +	       unsigned int flags, const char *msg);

Should the "msg" argument go at the beginning, to match update_ref()?

-Peff

Re: [PATCH 1/3] delete_refs(): accept a reflog message argument

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> diff --git a/refs.h b/refs.h
>> index 9fbff90e7..81627a63d 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>>   */
>>  int delete_ref(const char *refname, const unsigned char *old_sha1,
>> -	       unsigned int flags);
>> +	       unsigned int flags, const char *msg);
>
> Should the "msg" argument go at the beginning, to match update_ref()?

Probably.  rename/create have the message at the end but their
parameters are very different from update/delete.  The parameters
update and delete take are not identical, but we can view them as a
lot more similar than the other two.  So I think it makes sense for
delete to try matching update, even though trying to make all four
the same may proabably be pointless.

Re: [PATCH 1/3] delete_refs(): accept a reflog message argument

[Kyle Meyer]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>>> diff --git a/refs.h b/refs.h
>>> index 9fbff90e7..81627a63d 100644
>>> --- a/refs.h
>>> +++ b/refs.h
>>> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>>>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>>>   */
>>>  int delete_ref(const char *refname, const unsigned char *old_sha1,
>>> -	       unsigned int flags);
>>> +	       unsigned int flags, const char *msg);
>>
>> Should the "msg" argument go at the beginning, to match update_ref()?
>
> Probably.  rename/create have the message at the end but their
> parameters are very different from update/delete.  The parameters
> update and delete take are not identical, but we can view them as a
> lot more similar than the other two.  So I think it makes sense for
> delete to try matching update, even though trying to make all four
> the same may proabably be pointless.

I put "msg" after "flags" because that's where it occurs in
ref_transaction_delete(), but matching update_ref() makes sense.

-- 
Kyle

[PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Kyle Meyer]

Now that delete_refs() accepts a reflog message, pass the
user-provided message to delete_refs() rather than silently dropping
it.  The doesn't matter for the deleted ref's log because the log is
deleted along with the ref, but this entry will show up in HEAD's
reflog when deleting a checked out branch.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
I'm not sure if the test here (or in the next patch) is worth including.

 builtin/update-ref.c  | 2 +-
 t/t1400-update-ref.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a41f9adf1..f642acc22 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		 */
 		return delete_ref(refname,
 				  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
-				  flags, NULL);
+				  flags, msg);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  flags | create_reflog_flag,
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b0ffc0b57..65918d984 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+test_expect_success "deleting current branch adds message to HEAD's log" '
+	git update-ref $m $A &&
+	git symbolic-ref HEAD $m &&
+	git update-ref -mdelmsg -d $m &&
+	! test -f .git/$m &&
+	grep "delmsg$" .git/logs/HEAD >/dev/null
+'
+rm -f .git/$m
+
 test_expect_success 'update-ref does not create reflogs by default' '
 	test_when_finished "git update-ref -d $outside" &&
 	git update-ref $outside $A &&
-- 
2.11.1

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Jeff King]

On Thu, Feb 16, 2017 at 10:57:59PM -0500, Kyle Meyer wrote:

> Now that delete_refs() accepts a reflog message, pass the
> user-provided message to delete_refs() rather than silently dropping
> it.  The doesn't matter for the deleted ref's log because the log is
> deleted along with the ref, but this entry will show up in HEAD's
> reflog when deleting a checked out branch.

Sounds good.

> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index a41f9adf1..f642acc22 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>  		 */
>  		return delete_ref(refname,
>  				  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
> -				  flags, NULL);
> +				  flags, msg);

This looks obviously correct.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index b0ffc0b57..65918d984 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>  '
>  rm -f .git/$m
>  
> +test_expect_success "deleting current branch adds message to HEAD's log" '
> +	git update-ref $m $A &&
> +	git symbolic-ref HEAD $m &&
> +	git update-ref -mdelmsg -d $m &&
> +	! test -f .git/$m &&
> +	grep "delmsg$" .git/logs/HEAD >/dev/null
> +'
> +rm -f .git/$m

I think covering this with a test is good.

I don't know if it's also worth testing that deleting via HEAD also
writes the reflog. I.e.,:

  git update-ref -m delete-by-head -d HEAD

Some of the style here is a bit out-dated, but I think you are just
matching the surrounding tests.  So that's OK by me (though a patch to
modernize the whole thing would be welcome, too).

For reference, the two things I notice are:

  - we prefer test_path_is_missing to "! test -f" these days.

  - we don't redirect the output of grep (it's handled already in
    non-verbose mode, and in verbose mode we try to be...verbose).

-Peff

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ...

All good review comments.

I briefly wondered if recording the deletion of the current branch
in HEAD's reflog has much practical values (Porcelain "git branch"
would not even allow deletion in the first place), but because it is
a rare and unusual event, it probably makes more sense to have a
record of it.

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Kyle Meyer]

Jeff King <peff@peff.net> writes:

[...]

>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index b0ffc0b57..65918d984 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>>  '
>>  rm -f .git/$m
>>  
>> +test_expect_success "deleting current branch adds message to HEAD's log" '
>> +	git update-ref $m $A &&
>> +	git symbolic-ref HEAD $m &&
>> +	git update-ref -mdelmsg -d $m &&
>> +	! test -f .git/$m &&
>> +	grep "delmsg$" .git/logs/HEAD >/dev/null
>> +'
>> +rm -f .git/$m
>
> I think covering this with a test is good.
>
> I don't know if it's also worth testing that deleting via HEAD also
> writes the reflog. I.e.,:
>
>   git update-ref -m delete-by-head -d HEAD

Seems reasonable to cover this case as well.

> Some of the style here is a bit out-dated, but I think you are just
> matching the surrounding tests.  So that's OK by me (though a patch to
> modernize the whole thing would be welcome, too).

Right.  I'd be happy to follow up with a patch updating the style in
t1400-update-ref.sh.

> For reference, the two things I notice are:
>
>   - we prefer test_path_is_missing to "! test -f" these days.
>
>   - we don't redirect the output of grep (it's handled already in
>     non-verbose mode, and in verbose mode we try to be...verbose).

Would moving cleanup like "rm -f .git/$m" within the test's body using
test_when_finished also be preferred?

-- 
Kyle

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Jeff King]

On Fri, Feb 17, 2017 at 06:40:28PM -0500, Kyle Meyer wrote:

> > For reference, the two things I notice are:
> >
> >   - we prefer test_path_is_missing to "! test -f" these days.
> >
> >   - we don't redirect the output of grep (it's handled already in
> >     non-verbose mode, and in verbose mode we try to be...verbose).
> 
> Would moving cleanup like "rm -f .git/$m" within the test's body using
> test_when_finished also be preferred?

Yeah, that too.  I forgot to mention it.

-Peff

[PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

When the current branch is renamed, the deletion of the old ref is
recorded in HEAD's log with an empty message.  Now that delete_refs()
accepts a reflog message, provide a more descriptive message.  This
message will not be included in the reflog of the renamed branch, but
its log already covers the renaming event with a message of "Branch:
renamed ...".

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 refs/files-backend.c | 8 +++++++-
 t/t3200-branch.sh    | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ffa75d816..bb5df7ee6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2598,6 +2598,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	struct strbuf err = STRBUF_INIT;
+	struct strbuf logmsg_del = STRBUF_INIT;
 
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
@@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
 
-	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
+	strbuf_addf(&logmsg_del, "Deleted %s", oldrefname);
+
+	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, logmsg_del.buf)) {
 		error("unable to delete old %s", oldrefname);
+		strbuf_release(&logmsg_del);
 		goto rollback;
 	}
+	strbuf_release(&logmsg_del);
+
 
 	/*
 	 * Since we are doing a shallow lookup, sha1 is not the
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 8a833f354..4af7cd2b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,6 +139,10 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
 	test $(git rev-parse --abbrev-ref HEAD) = bam
 '
 
+test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' '
+	grep "Deleted refs/heads/baz$" .git/logs/HEAD >/dev/null
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
 	git checkout master &&
 	git worktree add -b baz bazdir &&
-- 
2.11.1

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Thu, Feb 16, 2017 at 10:58:00PM -0500, Kyle Meyer wrote:

> When the current branch is renamed, the deletion of the old ref is
> recorded in HEAD's log with an empty message.  Now that delete_refs()
> accepts a reflog message, provide a more descriptive message.  This
> message will not be included in the reflog of the renamed branch, but
> its log already covers the renaming event with a message of "Branch:
> renamed ...".

Right, makes sense. The code overall looks fine, though I have one
nit:

> @@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
>  		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
>  			oldrefname, strerror(errno));
>  
> -	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
> +	strbuf_addf(&logmsg_del, "Deleted %s", oldrefname);

We've been anything but consistent with our reflog messages in the past,
but I think the general guideline for new ones is to use "command:
action". Of course we don't _know_ the action here, because we're deep
in rename_ref().

Should we have the caller pass it in for us, as we do with delete_ref()
and update_ref()?

I see we actually already have a "logmsg" parameter. It already says
"Branch: renamed %s to %s". Could we just reuse that? I know that this
step is technically just the deletion, but I think it more accurately
describes the whole operation that the deletion is part of.

-Peff

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Feb 16, 2017 at 10:58:00PM -0500, Kyle Meyer wrote:
>
>> When the current branch is renamed, the deletion of the old ref is
>> recorded in HEAD's log with an empty message.  Now that delete_refs()
>> accepts a reflog message, provide a more descriptive message.  This
>> message will not be included in the reflog of the renamed branch, but
>> its log already covers the renaming event with a message of "Branch:
>> renamed ...".
>
> Right, makes sense. The code overall looks fine, though I have one
> nit:
>
>> @@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
>>  		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
>>  			oldrefname, strerror(errno));
>>  
>> -	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
>> +	strbuf_addf(&logmsg_del, "Deleted %s", oldrefname);
>
> We've been anything but consistent with our reflog messages in the past,
> but I think the general guideline for new ones is to use "command:
> action". Of course we don't _know_ the action here, because we're deep
> in rename_ref().
>
> Should we have the caller pass it in for us, as we do with delete_ref()
> and update_ref()?
>
> I see we actually already have a "logmsg" parameter. It already says
> "Branch: renamed %s to %s". Could we just reuse that? I know that this
> step is technically just the deletion, but I think it more accurately
> describes the whole operation that the deletion is part of.

True, but stepping back a bit,...

Do we even want these "internal" delete_ref() invocations to be
logged in HEAD's reflog?  I understand that this is inside the
implementation of renaming an old ref to a new ref, and reflog
message given to delete_ref() would matter only if the HEAD happens
to be pointing at old ref---but then HEAD will be repointed to the
new ref by somebody else [*1*] that called this function to rename
old to new and it _will_ log it.  So I am not sure if it is a good
thing to describe the deletion more readably with a message (which
is what this patch does) in the first place.  If we can just say
"don't log this deletion event in HEAD's reflog", wouldn't that be
more desirable?


[Footnote]

*1* Is the reason why the code in files_rename_ref() we are looking
    at does not adjust HEAD to point at the new ref is because it is
    just handing one ref-store and obviouvious to symrefs in other
    backends?

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Fri, Feb 17, 2017 at 09:50:54AM -0800, Junio C Hamano wrote:

> > I see we actually already have a "logmsg" parameter. It already says
> > "Branch: renamed %s to %s". Could we just reuse that? I know that this
> > step is technically just the deletion, but I think it more accurately
> > describes the whole operation that the deletion is part of.
> 
> True, but stepping back a bit,...
> 
> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog?  I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it.  So I am not sure if it is a good
> thing to describe the deletion more readably with a message (which
> is what this patch does) in the first place.  If we can just say
> "don't log this deletion event in HEAD's reflog", wouldn't that be
> more desirable?

Yes. I think the options are basically (in order of decreasing
preference in my opinion):

  1. Log a rename entry (same sha1, but note the rename in the free-form
     text).

  2. Log a delete (sha1 goes to null) followed by a creation (from null
     back to the original sha1).

  3. Log nothing at all for HEAD.

This does half of (2). If we do the second half, then I'd prefer it to
(3). But if we can do (1), that is better still (IMHO).

> *1* Is the reason why the code in files_rename_ref() we are looking
>     at does not adjust HEAD to point at the new ref is because it is
>     just handing one ref-store and obviouvious to symrefs in other
>     backends?

I'm actually confused about which bit of code is updating HEAD. I do not
see it either in files_rename_ref() or in the caller. Yet it clearly
happens. But that is the code that would know enough to do (1) or the
second half of (2) above.

-Peff

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Fri, Feb 17, 2017 at 02:43:50PM -0500, Jeff King wrote:

> Yes. I think the options are basically (in order of decreasing
> preference in my opinion):
> 
>   1. Log a rename entry (same sha1, but note the rename in the free-form
>      text).
> 
>   2. Log a delete (sha1 goes to null) followed by a creation (from null
>      back to the original sha1).
> 
>   3. Log nothing at all for HEAD.
> 
> This does half of (2). If we do the second half, then I'd prefer it to
> (3). But if we can do (1), that is better still (IMHO).
> 
> > *1* Is the reason why the code in files_rename_ref() we are looking
> >     at does not adjust HEAD to point at the new ref is because it is
> >     just handing one ref-store and obviouvious to symrefs in other
> >     backends?
> 
> I'm actually confused about which bit of code is updating HEAD. I do not
> see it either in files_rename_ref() or in the caller. Yet it clearly
> happens. But that is the code that would know enough to do (1) or the
> second half of (2) above.

Ah, I found it. It's in replace_each_worktree_head_symref() these days,
which does not bother to pass a log message.

So I think the second half of (2) is probably something like the patch
below.

Thinking on it more, we probably _do_ want two entries. Because the
operations are not atomic, it's possible that we may end up in a
half-way state after the first entry is written. And when debugging such
a case, I'd much rather see the first half of the operation logged than
nothing at all.

-Peff

---
diff --git a/branch.c b/branch.c
index b955d4f31..5c12036b0 100644
--- a/branch.c
+++ b/branch.c
@@ -345,7 +345,8 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
 	    branch, wt->path);
 }
 
-int replace_each_worktree_head_symref(const char *oldref, const char *newref)
+int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+				      const char *logmsg)
 {
 	int ret = 0;
 	struct worktree **worktrees = get_worktrees(0);
@@ -358,7 +359,7 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
 			continue;
 
 		if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-					     newref)) {
+					     newref, logmsg)) {
 			ret = -1;
 			error(_("HEAD of working tree %s is not updated"),
 			      worktrees[i]->path);
diff --git a/branch.h b/branch.h
index 3103eb9ad..b07788558 100644
--- a/branch.h
+++ b/branch.h
@@ -71,6 +71,7 @@ extern void die_if_checked_out(const char *branch, int ignore_current_worktree);
  * This will be used when renaming a branch. Returns 0 if successful, non-zero
  * otherwise.
  */
-extern int replace_each_worktree_head_symref(const char *oldref, const char *newref);
+extern int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+					     const char *logmsg);
 
 #endif
diff --git a/builtin/branch.c b/builtin/branch.c
index cbaa6d03c..383005912 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -471,14 +471,15 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 
 	if (rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
-	strbuf_release(&logmsg);
 
 	if (recovery)
 		warning(_("Renamed a misnamed branch '%s' away"), oldref.buf + 11);
 
-	if (replace_each_worktree_head_symref(oldref.buf, newref.buf))
+	if (replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
 
+	strbuf_release(&logmsg);
+
 	strbuf_addf(&oldsection, "branch.%s", oldref.buf + 11);
 	strbuf_release(&oldref);
 	strbuf_addf(&newsection, "branch.%s", newref.buf + 11);
diff --git a/refs.h b/refs.h
index 9fbff90e7..b33035c4a 100644
--- a/refs.h
+++ b/refs.h
@@ -334,7 +334,8 @@ int create_symref(const char *refname, const char *target, const char *logmsg);
  * $GIT_DIR points to.
  * Return 0 if successful, non-zero otherwise.
  * */
-int set_worktree_head_symref(const char *gitdir, const char *target);
+int set_worktree_head_symref(const char *gitdir, const char *target,
+			     const char *logmsg);
 
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4f1a88f6d..fa8d08e3c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3024,7 +3024,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target)
+int set_worktree_head_symref(const char *gitdir, const char *target, const char *logmsg)
 {
 	static struct lock_file head_lock;
 	struct ref_lock *lock;
@@ -3052,7 +3052,7 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
 	lock->lk = &head_lock;
 	lock->ref_name = xstrdup(head_rel);
 
-	ret = create_symref_locked(lock, head_rel, target, NULL);
+	ret = create_symref_locked(lock, head_rel, target, logmsg);
 
 	unlock_ref(lock); /* will free lock */
 	strbuf_release(&head_path);

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Thinking on it more, we probably _do_ want two entries. Because the
> operations are not atomic, it's possible that we may end up in a
> half-way state after the first entry is written. And when debugging such
> a case, I'd much rather see the first half of the operation logged than
> nothing at all.

Yes, that sounds right.

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 17, 2017 at 02:43:50PM -0500, Jeff King wrote:
>
>> Yes. I think the options are basically (in order of decreasing
>> preference in my opinion):
>> 
>>   1. Log a rename entry (same sha1, but note the rename in the free-form
>>      text).
>> 
>>   2. Log a delete (sha1 goes to null) followed by a creation (from null
>>      back to the original sha1).
>> 
>>   3. Log nothing at all for HEAD.
>> 
>> This does half of (2). If we do the second half, then I'd prefer it to
>> (3). But if we can do (1), that is better still (IMHO).

[...]

>> I'm actually confused about which bit of code is updating HEAD. I do not
>> see it either in files_rename_ref() or in the caller. Yet it clearly
>> happens. But that is the code that would know enough to do (1) or the
>> second half of (2) above.
>
> Ah, I found it. It's in replace_each_worktree_head_symref() these days,
> which does not bother to pass a log message.
> 
> So I think the second half of (2) is probably something like the patch
> below.
>
> Thinking on it more, we probably _do_ want two entries. Because the
> operations are not atomic, it's possible that we may end up in a
> half-way state after the first entry is written. And when debugging such
> a case, I'd much rather see the first half of the operation logged than
> nothing at all.

OK, I'll have a go at replacing patch 3 with this approach.

-- 
Kyle

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

Junio C Hamano <gitster@pobox.com> writes:

[...]

> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog?  I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it.

I know the discussion has developed further, but just a note that I
think the last statement is inaccurate: currently, a rename will not be
recorded in HEAD's log.  "git branch -m" will show a renaming event in
the new branch's log, but the only trace of the event in HEAD's log is
the deletion entry with an empty message.

-- 
Kyle

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Junio C Hamano]

Kyle Meyer <kyle@kyleam.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
>> Do we even want these "internal" delete_ref() invocations to be
>> logged in HEAD's reflog?  I understand that this is inside the
>> implementation of renaming an old ref to a new ref, and reflog
>> message given to delete_ref() would matter only if the HEAD happens
>> to be pointing at old ref---but then HEAD will be repointed to the
>> new ref by somebody else [*1*] that called this function to rename
>> old to new and it _will_ log it.
>
> I know the discussion has developed further, but just a note that I
> think the last statement is inaccurate: currently, a rename will not be
> recorded in HEAD's log.  "git branch -m" will show a renaming event in
> the new branch's log, but the only trace of the event in HEAD's log is
> the deletion entry with an empty message.

Yes, I think Peff diagnosed it and suggested a fix.

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Fri, Feb 17, 2017 at 06:41:32PM -0500, Kyle Meyer wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> [...]
> 
> > Do we even want these "internal" delete_ref() invocations to be
> > logged in HEAD's reflog?  I understand that this is inside the
> > implementation of renaming an old ref to a new ref, and reflog
> > message given to delete_ref() would matter only if the HEAD happens
> > to be pointing at old ref---but then HEAD will be repointed to the
> > new ref by somebody else [*1*] that called this function to rename
> > old to new and it _will_ log it.
> 
> I know the discussion has developed further, but just a note that I
> think the last statement is inaccurate: currently, a rename will not be
> recorded in HEAD's log.  "git branch -m" will show a renaming event in
> the new branch's log, but the only trace of the event in HEAD's log is
> the deletion entry with an empty message.

Right. I assumed Junio was talking about the hypothetical behavior we'd
want.

Your response did make me think of one other option: if we updated HEAD
_before_ writing the new ref, then it would automatically get the
"create" half of the rename. IOW, if the rename process were:

  1. delete old; this writes a reflog to HEAD, per the magic
     HEAD-detection in commit_ref_update().

  2. update HEAD to point to new

  3. re-create new; this uses the same magic to write a HEAD reflog.

That's probably a crazy path to go, though. Right now steps (1) and (3)
happen in a low-level function, and step (2) happens outside of it
completely. Arguably it would be good to change that, but I think we
want to think of (1) and (3) as an atomic operation. Putting more things
which might fail between them is a bad idea.

So I think your existing patches (modulo the review comments), plus the
"something like this" that I sent on top are probably a better end
state.

-Peff

Re: [PATCH 0/3] delete_ref(): support reflog messages

[Jeff King]

On Thu, Feb 16, 2017 at 10:57:57PM -0500, Kyle Meyer wrote:

> [Sorry for the slow response.]

No problem. The pace of open source varies wildly. :)

> >   - "git branch -m" does seem to realize when we are renaming HEAD,
> >     because it updates HEAD to point to the new branch name. But it
> >     should probably insert another reflog entry mentioning the rename
> >     (we do for "git checkout foo", even when "foo" has the same sha1 as
> >     the current HEAD).
> 
> I haven't worked out how to do this part yet.  I'm guessing the change
> will involve modifying split_head_update().
> 
> If this is added, should it be instead of, rather than in addition to,
> the deletion entry?  If a "Branch: renamed ..." entry is present, it
> doesn't seem like the deletion entry is providing any additional
> information.

I think you could do an "instead of" that goes from sha1 X to X, and
just mentions the rename. Or you could add a second entry after the
delete that takes it from 0{40} back to X.

I suspect the latter is easier to do, and I doubt anybody would care
that much of the exact form. These entries aren't really doing anything
for reachability. They're just giving an audit log of what happened. So
I don't think anybody would really care unless they were debugging a
confusing situation by hand. And as long as there's enough information
to figure out what happened, they'll be happy.

-Peff

[PATCH v2 0/4] delete_ref: support reflog messages

[Kyle Meyer]

Thanks to Junio and Peff for their feedback on v1.

  https://public-inbox.org/git/20170217035800.13214-1-kyle@kyleam.com/T/#u

Changes from v1:

 * "msg" is now positioned as the first argument to delete_ref() to
   match update_ref().

   20170217081205.zn7j6d5cffgdvfbn@sigill.intra.peff.net

 * A "delete by head" test case has been added for the update-ref
   change.

   20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net

 * The added tests no longer send grep's output to /dev/null.

   20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net

 * Renaming the current branch is represented by two entries in HEAD's
   log, both which reuse the log message passed to rename_ref().
   
   20170217083112.vn7m4udsopmlvnn5@sigill.intra.peff.net,
   20170217195549.z6uyy7hbbhj5avh7@sigill.intra.peff.net

 * Corrected a few places in the commit messages where "delete_refs"
   was written instead of "delete_ref".


Kyle Meyer (4):
  delete_ref: accept a reflog message argument
  update-ref: pass reflog message to delete_ref()
  rename_ref: replace empty message in HEAD's log
  branch: record creation of renamed branch in HEAD's log

 branch.c               |  5 +++--
 branch.h               |  3 ++-
 builtin/am.c           |  4 ++--
 builtin/branch.c       |  7 ++++---
 builtin/notes.c        |  4 ++--
 builtin/remote.c       |  4 ++--
 builtin/replace.c      |  2 +-
 builtin/reset.c        |  2 +-
 builtin/symbolic-ref.c |  2 +-
 builtin/tag.c          |  2 +-
 builtin/update-ref.c   |  2 +-
 fast-import.c          |  2 +-
 refs.c                 |  6 +++---
 refs.h                 |  7 ++++---
 refs/files-backend.c   | 10 +++++-----
 t/t1400-update-ref.sh  | 18 ++++++++++++++++++
 t/t3200-branch.sh      |  6 ++++++
 transport.c            |  2 +-
 18 files changed, 58 insertions(+), 30 deletions(-)

-- 
2.11.1

[PATCH v2 1/4] delete_ref: accept a reflog message argument

[Kyle Meyer]

When the current branch is renamed with 'git branch -m/-M' or deleted
with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log
with an empty message.  In preparation for adding a more meaningful
message to HEAD's log in these cases, update delete_ref() to take a
message argument and pass it along to ref_transaction_delete().
Modify all callers to pass NULL for the new message argument; no
change in behavior is intended.

Note that this is relevant for HEAD's log but not for the deleted
ref's log, which is currently deleted along with the ref.  Even if it
were not, an entry for the deletion wouldn't be present in the deleted
ref's log.  files_transaction_commit() writes to the log if
REF_NEEDS_COMMIT or REF_LOG_ONLY are set, but lock_ref_for_update()
doesn't set REF_NEEDS_COMMIT for the deleted ref because REF_DELETING
is set.  In contrast, the update for HEAD has REF_LOG_ONLY set by
split_head_update(), resulting in the deletion being logged.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 builtin/am.c           | 4 ++--
 builtin/branch.c       | 2 +-
 builtin/notes.c        | 4 ++--
 builtin/remote.c       | 4 ++--
 builtin/replace.c      | 2 +-
 builtin/reset.c        | 2 +-
 builtin/symbolic-ref.c | 2 +-
 builtin/tag.c          | 2 +-
 builtin/update-ref.c   | 2 +-
 fast-import.c          | 2 +-
 refs.c                 | 6 +++---
 refs.h                 | 4 ++--
 refs/files-backend.c   | 6 +++---
 transport.c            | 2 +-
 14 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578..f7a7a971f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1049,7 +1049,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	} else {
 		write_state_text(state, "abort-safety", "");
 		if (!state->rebasing)
-			delete_ref("ORIG_HEAD", NULL, 0);
+			delete_ref(NULL, "ORIG_HEAD", NULL, 0);
 	}
 
 	/*
@@ -2172,7 +2172,7 @@ static void am_abort(struct am_state *state)
 				has_curr_head ? &curr_head : NULL, 0,
 				UPDATE_REFS_DIE_ON_ERR);
 	else if (curr_branch)
-		delete_ref(curr_branch, NULL, REF_NODEREF);
+		delete_ref(NULL, curr_branch, NULL, REF_NODEREF);
 
 	free(curr_branch);
 	am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0..8f8242e07 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -251,7 +251,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			goto next;
 		}
 
-		if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
+		if (delete_ref(NULL, name, is_null_sha1(sha1) ? NULL : sha1,
 			       REF_NODEREF)) {
 			error(remote_branch
 			      ? _("Error deleting remote-tracking branch '%s'")
diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad..4b492abd4 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -681,9 +681,9 @@ static int merge_abort(struct notes_merge_options *o)
 	 * notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
 	 */
 
-	if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+	if (delete_ref(NULL, "NOTES_MERGE_PARTIAL", NULL, 0))
 		ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
-	if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+	if (delete_ref(NULL, "NOTES_MERGE_REF", NULL, REF_NODEREF))
 		ret += error(_("failed to delete ref NOTES_MERGE_REF"));
 	if (notes_merge_abort(o))
 		ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 5339ed6ad..2b415911b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -691,7 +691,7 @@ static int mv(int argc, const char **argv)
 		read_ref_full(item->string, RESOLVE_REF_READING, oid.hash, &flag);
 		if (!(flag & REF_ISSYMREF))
 			continue;
-		if (delete_ref(item->string, NULL, REF_NODEREF))
+		if (delete_ref(NULL, item->string, NULL, REF_NODEREF))
 			die(_("deleting '%s' failed"), item->string);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
@@ -1248,7 +1248,7 @@ static int set_head(int argc, const char **argv)
 			head_name = xstrdup(states.heads.items[0].string);
 		free_remote_ref_states(&states);
 	} else if (opt_d && !opt_a && argc == 1) {
-		if (delete_ref(buf.buf, NULL, REF_NODEREF))
+		if (delete_ref(NULL, buf.buf, NULL, REF_NODEREF))
 			result |= error(_("Could not delete %s"), buf.buf);
 	} else
 		usage_with_options(builtin_remote_sethead_usage, options);
diff --git a/builtin/replace.c b/builtin/replace.c
index b58c714cb..226d0f952 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -121,7 +121,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 static int delete_replace_ref(const char *name, const char *ref,
 			      const unsigned char *sha1)
 {
-	if (delete_ref(ref, sha1, 0))
+	if (delete_ref(NULL, ref, sha1, 0))
 		return 1;
 	printf("Deleted replace ref '%s'\n", name);
 	return 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfc..fc3b906c4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -256,7 +256,7 @@ static int reset_refs(const char *rev, const struct object_id *oid)
 		update_ref_oid(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
 			   UPDATE_REFS_MSG_ON_ERR);
 	} else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig->hash, 0);
+		delete_ref(NULL, "ORIG_HEAD", old_orig->hash, 0);
 	set_reflog_message(&msg, "updating HEAD", rev);
 	update_ref_status = update_ref_oid(msg.buf, "HEAD", oid, orig, 0,
 				       UPDATE_REFS_MSG_ON_ERR);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 96eed9446..70addef15 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -58,7 +58,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 			die("Cannot delete %s, not a symbolic ref", argv[0]);
 		if (!strcmp(argv[0], "HEAD"))
 			die("deleting '%s' is not allowed", argv[0]);
-		return delete_ref(argv[0], NULL, REF_NODEREF);
+		return delete_ref(NULL, argv[0], NULL, REF_NODEREF);
 	}
 
 	switch (argc) {
diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a967..c23d6d4bc 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -97,7 +97,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
 static int delete_tag(const char *name, const char *ref,
 		      const unsigned char *sha1, const void *cb_data)
 {
-	if (delete_ref(ref, sha1, 0))
+	if (delete_ref(NULL, ref, sha1, 0))
 		return 1;
 	printf(_("Deleted tag '%s' (was %s)\n"), name, find_unique_abbrev(sha1, DEFAULT_ABBREV));
 	return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7f30d3a76..86d006d36 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -433,7 +433,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		 * For purposes of backwards compatibility, we treat
 		 * NULL_SHA1 as "don't care" here:
 		 */
-		return delete_ref(refname,
+		return delete_ref(NULL, refname,
 				  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
 				  flags);
 	else
diff --git a/fast-import.c b/fast-import.c
index 64fe602f0..6c13472c4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1752,7 +1752,7 @@ static int update_branch(struct branch *b)
 
 	if (is_null_sha1(b->sha1)) {
 		if (b->delete)
-			delete_ref(b->name, NULL, 0);
+			delete_ref(NULL, b->name, NULL, 0);
 		return 0;
 	}
 	if (read_ref(b->name, old_sha1))
diff --git a/refs.c b/refs.c
index cd36b64ed..053d23a90 100644
--- a/refs.c
+++ b/refs.c
@@ -591,8 +591,8 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
 	return 0;
 }
 
-int delete_ref(const char *refname, const unsigned char *old_sha1,
-	       unsigned int flags)
+int delete_ref(const char *msg, const char *refname,
+	       const unsigned char *old_sha1, unsigned int flags)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
@@ -603,7 +603,7 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_sha1,
-				   flags, NULL, &err) ||
+				   flags, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
diff --git a/refs.h b/refs.h
index 9fbff90e7..5880886a7 100644
--- a/refs.h
+++ b/refs.h
@@ -276,8 +276,8 @@ int reflog_exists(const char *refname);
  * exists, regardless of its old value. It is an error for old_sha1 to
  * be NULL_SHA1. flags is passed through to ref_transaction_delete().
  */
-int delete_ref(const char *refname, const unsigned char *old_sha1,
-	       unsigned int flags);
+int delete_ref(const char *msg, const char *refname,
+	       const unsigned char *old_sha1, unsigned int flags);
 
 /*
  * Delete the specified references. If there are any problems, emit
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c041d4ba2..299eb4d8a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2489,7 +2489,7 @@ static int files_delete_refs(struct ref_store *ref_store,
 	for (i = 0; i < refnames->nr; i++) {
 		const char *refname = refnames->items[i].string;
 
-		if (delete_ref(refname, NULL, flags))
+		if (delete_ref(NULL, refname, NULL, flags))
 			result |= error(_("could not remove reference %s"), refname);
 	}
 
@@ -2616,7 +2616,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
 
-	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
+	if (delete_ref(NULL, oldrefname, orig_sha1, REF_NODEREF)) {
 		error("unable to delete old %s", oldrefname);
 		goto rollback;
 	}
@@ -2630,7 +2630,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 	 */
 	if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
 			   sha1, NULL) &&
-	    delete_ref(newrefname, NULL, REF_NODEREF)) {
+	    delete_ref(NULL, newrefname, NULL, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			struct strbuf path = STRBUF_INIT;
 			int result;
diff --git a/transport.c b/transport.c
index d72e08948..269352b22 100644
--- a/transport.c
+++ b/transport.c
@@ -299,7 +299,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 		if (verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
-			delete_ref(rs.dst, NULL, 0);
+			delete_ref(NULL, rs.dst, NULL, 0);
 		} else
 			update_ref("update by push", rs.dst,
 					ref->new_oid.hash, NULL, 0, 0);
-- 
2.11.1

Re: [PATCH v2 1/4] delete_ref: accept a reflog message argument

[Duy Nguyen]

On Tue, Feb 21, 2017 at 8:10 AM, Kyle Meyer <kyle@kyleam.com> wrote:
> diff --git a/refs.h b/refs.h
> index 9fbff90e7..5880886a7 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -276,8 +276,8 @@ int reflog_exists(const char *refname);
>   * exists, regardless of its old value. It is an error for old_sha1 to
>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>   */
> -int delete_ref(const char *refname, const unsigned char *old_sha1,
> -              unsigned int flags);
> +int delete_ref(const char *msg, const char *refname,
> +              const unsigned char *old_sha1, unsigned int flags);

Is it just me who thinks it's weird that msg comes in front here? The
key identity, refname, should be the first argument imo. You'll
probably want to update the comment block above if msg can be NULL. We
have _very_ good documentation in this file, let's keep it uptodate.
-- 
Duy

Re: [PATCH v2 1/4] delete_ref: accept a reflog message argument

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Feb 21, 2017 at 8:10 AM, Kyle Meyer <kyle@kyleam.com> wrote:
>> diff --git a/refs.h b/refs.h
>> index 9fbff90e7..5880886a7 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -276,8 +276,8 @@ int reflog_exists(const char *refname);
>>   * exists, regardless of its old value. It is an error for old_sha1 to
>>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>>   */
>> -int delete_ref(const char *refname, const unsigned char *old_sha1,
>> -              unsigned int flags);
>> +int delete_ref(const char *msg, const char *refname,
>> +              const unsigned char *old_sha1, unsigned int flags);
>
> Is it just me who thinks it's weird that msg comes in front here?

You and anybody who didn't read what was discussed earlier, methinks
;-)  cf. <20170217081205.zn7j6d5cffgdvfbn@sigill.intra.peff.net>

> ... You'll
> probably want to update the comment block above if msg can be
> NULL.

Good suggestion.

Thanks for taking a look at this topic.  IIRC a recent update to one
of your topics introduced a new conflicts with this one.

Re: [PATCH v2 1/4] delete_ref: accept a reflog message argument

[Kyle Meyer]

Duy Nguyen <pclouds@gmail.com> writes:

> You'll probably want to update the comment block above if msg can be
> NULL. We have _very_ good documentation in this file, let's keep it
> uptodate.

Looking at how other functions in refs.h document their "msg" or
"logmsg" parameter, none seem to mention it explicitly.  update_ref
refers to ref_transaction_update, and
ref_transaction_{update,create,delete,verify} refer to ref.h's
"Reference transaction updates" comment.

delete_ref's docstring already mentions that "flag" is passed to
ref_transaction_delete, so perhaps it should mention "msg" here as
well.

-- >8 --
Subject: [PATCH] delete_ref: mention "msg" parameter in docstring

delete_ref() was recently extended to take a "msg" argument.
---
 refs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.h b/refs.h
index e529f4c3a..988750218 100644
--- a/refs.h
+++ b/refs.h
@@ -274,7 +274,8 @@ int reflog_exists(const char *refname);
  * verify that the current value of the reference is old_sha1 before
  * deleting it. If old_sha1 is NULL, delete the reference if it
  * exists, regardless of its old value. It is an error for old_sha1 to
- * be NULL_SHA1. flags is passed through to ref_transaction_delete().
+ * be NULL_SHA1. msg and flags are passed through to
+ * ref_transaction_delete().
  */
 int delete_ref(const char *msg, const char *refname,
 	       const unsigned char *old_sha1, unsigned int flags);
-- 
2.11.1

[PATCH v2 2/4] update-ref: pass reflog message to delete_ref()

[Kyle Meyer]

Now that delete_ref() accepts a reflog message, pass the user-provided
message to delete_ref() rather than silently dropping it.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 builtin/update-ref.c  |  2 +-
 t/t1400-update-ref.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 86d006d36..0b2ecf41a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -433,7 +433,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		 * For purposes of backwards compatibility, we treat
 		 * NULL_SHA1 as "don't care" here:
 		 */
-		return delete_ref(NULL, refname,
+		return delete_ref(msg, refname,
 				  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
 				  flags);
 	else
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b0ffc0b57..6e112fb5f 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -85,6 +85,24 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+test_expect_success "deleting current branch adds message to HEAD's log" '
+	git update-ref $m $A &&
+	git symbolic-ref HEAD $m &&
+	git update-ref -m delete-$m -d $m &&
+	! test -f .git/$m &&
+	grep "delete-$m$" .git/logs/HEAD
+'
+rm -f .git/$m
+
+test_expect_success "deleting by HEAD adds message to HEAD's log" '
+	git update-ref $m $A &&
+	git symbolic-ref HEAD $m &&
+	git update-ref -m delete-by-head -d HEAD &&
+	! test -f .git/$m &&
+	grep "delete-by-head$" .git/logs/HEAD
+'
+rm -f .git/$m
+
 test_expect_success 'update-ref does not create reflogs by default' '
 	test_when_finished "git update-ref -d $outside" &&
 	git update-ref $outside $A &&
-- 
2.11.1

[PATCH v2 3/4] rename_ref: replace empty message in HEAD's log

[Kyle Meyer]

When the current branch is renamed, the deletion of the old ref is
recorded in HEAD's log with an empty message.  Now that delete_ref()
accepts a reflog message, provide a more descriptive message by
passing along the log message that is given to rename_ref().

The next step will be to extend HEAD's log to also include the second
part of the rename, the creation of the new branch.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 refs/files-backend.c | 2 +-
 t/t3200-branch.sh    | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 299eb4d8a..f6e7c192c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2616,7 +2616,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
 
-	if (delete_ref(NULL, oldrefname, orig_sha1, REF_NODEREF)) {
+	if (delete_ref(logmsg, oldrefname, orig_sha1, REF_NODEREF)) {
 		error("unable to delete old %s", oldrefname);
 		goto rollback;
 	}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 8a833f354..0dbc54003 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,6 +139,11 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
 	test $(git rev-parse --abbrev-ref HEAD) = bam
 '
 
+test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' '
+        msg="Branch: renamed refs/heads/baz to refs/heads/bam" &&
+	grep " 0\{40\}.*$msg$" .git/logs/HEAD
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
 	git checkout master &&
 	git worktree add -b baz bazdir &&
-- 
2.11.1

[PATCH v2 4/4] branch: record creation of renamed branch in HEAD's log

[Kyle Meyer]

Renaming the current branch adds an event to the current branch's log
and to HEAD's log.  However, the logged entries differ.  The entry in
the branch's log represents the entire renaming operation (the old and
new hash are identical), whereas the entry in HEAD's log represents
the deletion only (the new sha1 is null).

Extend replace_each_worktree_head_symref(), whose only caller is
branch_rename(), to take a reflog message argument.  This allows the
creation of the new ref to be recorded in HEAD's log.  As a result,
the renaming event is represented by two entries (a deletion and a
creation entry) in HEAD's log.

It's a bit unfortunate that the branch's log and HEAD's log now
represent the renaming event in different ways.  Given that the
renaming operation is not atomic, the two-entry form is a more
accurate representation of the operation and is more useful for
debugging purposes if a failure occurs between the deletion and
creation events.  It would make sense to move the branch's log to the
two-entry form, but this would involve changes to how the rename is
carried out and to how the update flags and reflogs are processed for
deletions, so it may not be worth the effort.

Based-on-patch-by: Jeff King <peff@peff.net>
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 branch.c             | 5 +++--
 branch.h             | 3 ++-
 builtin/branch.c     | 5 +++--
 refs.h               | 3 ++-
 refs/files-backend.c | 4 ++--
 t/t3200-branch.sh    | 5 +++--
 6 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/branch.c b/branch.c
index b955d4f31..5c12036b0 100644
--- a/branch.c
+++ b/branch.c
@@ -345,7 +345,8 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
 	    branch, wt->path);
 }
 
-int replace_each_worktree_head_symref(const char *oldref, const char *newref)
+int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+				      const char *logmsg)
 {
 	int ret = 0;
 	struct worktree **worktrees = get_worktrees(0);
@@ -358,7 +359,7 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
 			continue;
 
 		if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-					     newref)) {
+					     newref, logmsg)) {
 			ret = -1;
 			error(_("HEAD of working tree %s is not updated"),
 			      worktrees[i]->path);
diff --git a/branch.h b/branch.h
index 3103eb9ad..b07788558 100644
--- a/branch.h
+++ b/branch.h
@@ -71,6 +71,7 @@ extern void die_if_checked_out(const char *branch, int ignore_current_worktree);
  * This will be used when renaming a branch. Returns 0 if successful, non-zero
  * otherwise.
  */
-extern int replace_each_worktree_head_symref(const char *oldref, const char *newref);
+extern int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+					     const char *logmsg);
 
 #endif
diff --git a/builtin/branch.c b/builtin/branch.c
index 8f8242e07..e1f97dcfc 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -579,14 +579,15 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 
 	if (rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
-	strbuf_release(&logmsg);
 
 	if (recovery)
 		warning(_("Renamed a misnamed branch '%s' away"), oldref.buf + 11);
 
-	if (replace_each_worktree_head_symref(oldref.buf, newref.buf))
+	if (replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
 
+	strbuf_release(&logmsg);
+
 	strbuf_addf(&oldsection, "branch.%s", oldref.buf + 11);
 	strbuf_release(&oldref);
 	strbuf_addf(&newsection, "branch.%s", newref.buf + 11);
diff --git a/refs.h b/refs.h
index 5880886a7..e529f4c3a 100644
--- a/refs.h
+++ b/refs.h
@@ -334,7 +334,8 @@ int create_symref(const char *refname, const char *target, const char *logmsg);
  * $GIT_DIR points to.
  * Return 0 if successful, non-zero otherwise.
  * */
-int set_worktree_head_symref(const char *gitdir, const char *target);
+int set_worktree_head_symref(const char *gitdir, const char *target,
+			     const char *logmsg);
 
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f6e7c192c..42b137bb1 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3055,7 +3055,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target)
+int set_worktree_head_symref(const char *gitdir, const char *target, const char *logmsg)
 {
 	static struct lock_file head_lock;
 	struct ref_lock *lock;
@@ -3083,7 +3083,7 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
 	lock->lk = &head_lock;
 	lock->ref_name = xstrdup(head_rel);
 
-	ret = create_symref_locked(lock, head_rel, target, NULL);
+	ret = create_symref_locked(lock, head_rel, target, logmsg);
 
 	unlock_ref(lock); /* will free lock */
 	strbuf_release(&head_path);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 0dbc54003..d84837d08 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,9 +139,10 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
 	test $(git rev-parse --abbrev-ref HEAD) = bam
 '
 
-test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' '
+test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD' '
         msg="Branch: renamed refs/heads/baz to refs/heads/bam" &&
-	grep " 0\{40\}.*$msg$" .git/logs/HEAD
+	grep " 0\{40\}.*$msg$" .git/logs/HEAD &&
+	grep "^0\{40\}.*$msg$" .git/logs/HEAD
 '
 
 test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
-- 
2.11.1

Re: [PATCH v2 0/4] delete_ref: support reflog messages

[Jeff King]

On Mon, Feb 20, 2017 at 08:10:31PM -0500, Kyle Meyer wrote:

> Kyle Meyer (4):
>   delete_ref: accept a reflog message argument
>   update-ref: pass reflog message to delete_ref()
>   rename_ref: replace empty message in HEAD's log
>   branch: record creation of renamed branch in HEAD's log

These look good to me. I think you did a nice job of summarizing the
discussion in the commit messages.

-Peff

Re: [PATCH v2 0/4] delete_ref: support reflog messages

[Junio C Hamano]

Kyle Meyer <kyle@kyleam.com> writes:

> Kyle Meyer (4):
>   delete_ref: accept a reflog message argument
>   update-ref: pass reflog message to delete_ref()
>   rename_ref: replace empty message in HEAD's log
>   branch: record creation of renamed branch in HEAD's log

These looked reasonable.  I had to resolve conflicts with another
topic in flight that removed set_worktree_head_symref() function
while queuing, and I think I resolved it correctly, but please
double check what is pushed out on 'pu'.

Thanks.

Re: [PATCH v2 0/4] delete_ref: support reflog messages

[Kyle Meyer]

Junio C Hamano <gitster@pobox.com> writes:

> These looked reasonable.  I had to resolve conflicts with another
> topic in flight that removed set_worktree_head_symref() function
> while queuing, and I think I resolved it correctly, but please
> double check what is pushed out on 'pu'.

The merge looks right to me, thanks.  Thanks also for touching up the
tab/space issue in t3200-branch.sh.

-- 
Kyle