[PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
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
---
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
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
Jeff King 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
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
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;
struc
Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
Jeff King 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
Junio C Hamano 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
Jeff King 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 writes: > Junio C Hamano 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
On Fri, Feb 17, 2017 at 06:41:32PM -0500, Kyle Meyer wrote: > Junio C Hamano 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
