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

[Kyle Meyer]

Junio C Hamano  writes:

> Jeff King  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

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

[Junio C Hamano]

Jeff King  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

[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 -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

[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 -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 
---
 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 ? _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, 
);
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();
} 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