refs: add \t to reflog in the files backend

[hanwen]

commit 523fa69 (Jul 10, 2020) "reflog: cleanse messages in the
refs.c layer" centralized reflog normalizaton. However, the
normalizaton added a leading "\t" to the message. This is an artifact
of the reflog storage format in the files backend, so it should be
added there.

Routines that parse back the reflog (such as grab_nth_branch_switch)
expect the "\t" to not be in the message, so without this fix, git
with reftable cannot process the "@{-1}" syntax.

Signed-off-by: Han-Wen Nienhuys hanwen@google.com

Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

[hanwen]

/submit

[gitgitgadget]

Submitted as pull.688.git.1596195370757.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> commit 523fa69c3 (Jul 10, 2020) "reflog: cleanse messages in the
> refs.c layer" centralized reflog normalizaton.  However, the
> normalizaton added a leading "\t" to the message. This is an artifact
> of the reflog storage format in the files backend, so it should be
> added there.

I agree with you that it makes sense to move the logic to add "\t"
before the log message from generic layer to the backend.  Thanks
for spotting and correcting.

Will queue.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Fri, Jul 31, 2020 at 11:36:10AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

> diff --git a/refs.c b/refs.c
> index 89814c7be4..2dd851fe81 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -907,7 +907,6 @@ static void copy_reflog_msg(struct strbuf *sb, const char *msg)
>  	char c;
>  	int wasspace = 1;
>  
> -	strbuf_addch(sb, '\t');
>  	while ((c = *msg++)) {
>  		if (wasspace && isspace(c))
>  			continue;
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index e0aba23eb2..985631f33e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1628,8 +1628,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
>  	int ret = 0;
>  
>  	strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);
> -	if (msg && *msg)
> +	if (msg && *msg) {
> +		strbuf_addch(&sb, '\t');
>  		strbuf_addstr(&sb, msg);
> +	}

I wondered here whether the existing code would always write the tab,
even for an entry with no message, since we seem to unconditionally add
the tab in the hunk above.

But it looks like we only call copy_reflog_msg() if it's non-empty:

  static char *normalize_reflog_message(const char *msg)
  {
          struct strbuf sb = STRBUF_INIT;
  
          if (msg && *msg)
                  copy_reflog_msg(&sb, msg);
          return strbuf_detach(&sb, NULL);
  }

so this is retaining that behavior (and indeed, doing a simple "git
update-ref" confirms that we currently omit the tab in this case).

So the patch looks good (and the intent seems obviously good to me, as
well).

-Peff

[gitgitgadget]

This branch is now known as hn/reftable.

[gitgitgadget]

This patch series was integrated into seen via git@92775f5.

[gitgitgadget]

This patch series was integrated into seen via git@cdd6e4a.

[gitgitgadget]

This patch series was integrated into next via git@9e34be9.

[gitgitgadget]

This patch series was integrated into seen via git@dc3c6fb.

[gitgitgadget]

This patch series was integrated into next via git@dc3c6fb.

[gitgitgadget]

This patch series was integrated into master via git@dc3c6fb.

[gitgitgadget]

Closed via dc3c6fb.