Skip to contentSkip to conversation

refs: add \t to reflog in the files backend

PATCH In review Han-Wen Nienhuys via GitGitGadget 3 participants 3 messages Jul 31, 2020

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 refs: add \t to reflog in the files backend

From: Han-Wen Nienhuys <[email protected]>

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.

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.

---
refs: add \t to reflog in the files backend

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.

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-688%2Fhanwen%2Ftab-reflog-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-688/hanwen/tab-reflog-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/688

 refs.c               | 1 -
 refs/files-backend.c | 4 +++-
 2 files changed, 3 insertions(+), 2 deletions(-)
refs.c +0 -1
index 89814c7be4..2dd851fe81 100644
@@ -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;
refs/files-backend.c +3 -1
index e0aba23eb2..985631f33e 100644
@@ -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);
+	}
 	strbuf_addch(&sb, '\n');
 	if (write_in_full(fd, sb.buf, sb.len) < 0)
 		ret = -1;

base-commit: e8ab941b671da6890181aea5b5755d1d9eea24ec

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 7 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

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.

#2 lore raw
In reply to Han-Wen Nienhuys via GitGitGadget
Quoted 17 lines · Han-Wen Nienhuys via GitGitGadget
refs.c +0 -1
index 89814c7be4..2dd851fe81 100644
@@ -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;
refs/files-backend.c +0 -0
index e0aba23eb2..985631f33e 100644

refs/files-backend.c static int log_ref_write_fd(int fd, const struct object_id *old_oid, · line 1634

Quoted 8 lines
refs/files-backend.c +3 -1
@@ -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