[PATCH] notes: allow merging from arbitrary references

Jacob Keller· Oct 1, 2015, 22:39 UTC · lore

From: Jacob Keller <jacob.keller@gmail.com>

Create a new expansion function, expand_loose_notes_ref which will expand any ref using get_sha1, but falls back to expand_notes_ref if this fails. The contents of the strbuf will be either the hex string of the sha1, or the expanded notes ref. It is expected to be re-expanded using get_sha1 inside the notes merge machinery, and there is no real error checking provided at this layer.

Since we now support merging from non-notes refs, remove the test case associated with that behavior. Add a test case for merging from a non-notes ref.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/notes.c        |  4 ++--
 notes.c                | 14 ++++++++++++++
 notes.h                |  8 ++++++++
 t/t3308-notes-merge.sh | 22 +++++++++++-----------
 4 files changed, 35 insertions(+), 13 deletions(-)

Show changes to 4 files +35 −13

builtin/notes.c, notes.c, notes.h, t/t3308-notes-merge.sh

diff --git a/builtin/notes.c b/builtin/notes.c
index 3608c6478528..a029c3b976cc 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -806,7 +806,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 
 	o.local_ref = default_notes_ref();
 	strbuf_addstr(&remote_ref, argv[0]);
-	expand_notes_ref(&remote_ref);
+	expand_loose_notes_ref(&remote_ref);
 	o.remote_ref = remote_ref.buf;
 
 	t = init_notes_check("merge");
@@ -833,7 +833,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 	}
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
-		    remote_ref.buf, default_notes_ref());
+		    argv[0], default_notes_ref());
 	strbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); /* skip "notes: " */
 
 	result = notes_merge(&o, t, result_sha1);
diff --git a/notes.c b/notes.c
index eacd2a61daf9..754405dcdb92 100644
--- a/notes.c
+++ b/notes.c
@@ -1300,3 +1300,17 @@ void expand_notes_ref(struct strbuf *sb)
 	else
 		strbuf_insert(sb, 0, "refs/notes/", 11);
 }
+
+void expand_loose_notes_ref(struct strbuf *sb)
+{
+	unsigned char object[20];
+
+	if (get_sha1(sb->buf, object)) {
+		/* fallback to expand_notes_ref */
+		expand_notes_ref(sb);
+	} else {
+		/* we got an object, so replace the strbuf with the hex string */
+		strbuf_reset(sb);
+		strbuf_addstr(sb, sha1_to_hex(object));
+	}
+}
diff --git a/notes.h b/notes.h
index 2a3f92338076..99944eb8507e 100644
--- a/notes.h
+++ b/notes.h
@@ -294,4 +294,12 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 /* Expand inplace a note ref like "foo" or "notes/foo" into "refs/notes/foo" */
 void expand_notes_ref(struct strbuf *sb);
 
+/*
+ * Similar to expand_notes_ref, but allows arbitrary refs to be expanded via
+ * get_sha1 first. If get_sha1 fails to find a ref, fall back to traditional
+ * expand_notes_ref. The contents of the strbuf will be suitable to attempt
+ * passing to get_sha1 again inside the notes machinery.
+ */
+void expand_loose_notes_ref(struct strbuf *sb);
+
 #endif
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b49bbea..19aed7ec953b 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -18,7 +18,9 @@ test_expect_success setup '
 	git notes add -m "Notes on 1st commit" 1st &&
 	git notes add -m "Notes on 2nd commit" 2nd &&
 	git notes add -m "Notes on 3rd commit" 3rd &&
-	git notes add -m "Notes on 4th commit" 4th
+	git notes add -m "Notes on 4th commit" 4th &&
+	# Copy notes to remote-notes
+	git fetch . refs/notes/*:refs/remote-notes/origin/*
 '
 
 commit_sha1=$(git rev-parse 1st^{commit})
@@ -66,7 +68,9 @@ test_expect_success 'verify initial notes (x)' '
 '
 
 cp expect_notes_x expect_notes_y
+cp expect_notes_x expect_notes_v
 cp expect_log_x expect_log_y
+cp expect_log_x expect_log_v
 
 test_expect_success 'fail to merge empty notes ref into empty notes ref (z => y)' '
 	test_must_fail git -c "core.notesRef=refs/notes/y" notes merge z
@@ -84,16 +88,12 @@ test_expect_success 'fail to merge into various non-notes refs' '
 	test_must_fail git -c "core.notesRef=refs/notes/foo^{bar" notes merge x
 '
 
-test_expect_success 'fail to merge various non-note-trees' '
-	git config core.notesRef refs/notes/y &&
-	test_must_fail git notes merge refs/notes &&
-	test_must_fail git notes merge refs/notes/ &&
-	test_must_fail git notes merge refs/notes/dir &&
-	test_must_fail git notes merge refs/notes/dir/ &&
-	test_must_fail git notes merge refs/heads/master &&
-	test_must_fail git notes merge x: &&
-	test_must_fail git notes merge x:foo &&
-	test_must_fail git notes merge foo^{bar
+test_expect_success 'merge non-notes ref into empty notes ref (remote-notes/origin/x => v)' '
+	git config core.notesRef refs/notes/v &&
+	git notes merge refs/remote-notes/origin/x &&
+	verify_notes v &&
+	# refs/remote-notes/origin/x and v should point to the same notes commit
+	test "$(git rev-parse refs/remote-notes/origin/x)" = "$(git rev-parse refs/notes/v)"
 '
 
 test_expect_success 'merge notes into empty notes ref (x => y)' '

-- 
2.6.0.rc3.238.gc07a1e8

Jacob Keller· Oct 1, 2015, 22:51 UTC · re: Jacob Keller · lore

Re: [PATCH] notes: allow merging from arbitrary references

On Thu, Oct 1, 2015 at 3:39 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:

Show 15 quoted lines

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Create a new expansion function, expand_loose_notes_ref which will
> expand any ref using get_sha1, but falls back to expand_notes_ref if
> this fails. The contents of the strbuf will be either the hex string of
> the sha1, or the expanded notes ref. It is expected to be re-expanded
> using get_sha1 inside the notes merge machinery, and there is no real
> error checking provided at this layer.
>
> Since we now support merging from non-notes refs, remove the test case
> associated with that behavior. Add a test case for merging from a
> non-notes ref.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---

I haven't yet figured out how to handle showing of notes on non-notes refs, since the core notes command uses expand_notes_ref very early on in the flow before we even know what kind of command we're using. I think this will take a larger structural change to get right.

For merges it's pretty straight forward. This patch may seem weird, but it's expected that the internal notes machinery will (re)expand the remote_ref again and properly error out if it doesn't exist. We need this since merging from a notes ref that doesn't exist isn't considered an error today (it just falls back to using an empty ref).

Note that this does have some weird consequences, in that if you typo a reference name such as refs/foo/bar which doesn't exist, then it will be expanded into refs/notes/refs/foo/bar, which I am not sure is the best approach, but otherwise things such as "x" don't properly expand... I think in practice this will always result in the "empty" notes ref being used for the source of the merge, so it may not be a big deal.

Regards, Jake

Jacob Keller· Oct 8, 2015, 00:28 UTC · re: Jacob Keller · lore

Re: [PATCH] notes: allow merging from arbitrary references

On Thu, Oct 1, 2015 at 3:39 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:

Show 15 quoted lines

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Create a new expansion function, expand_loose_notes_ref which will
> expand any ref using get_sha1, but falls back to expand_notes_ref if
> this fails. The contents of the strbuf will be either the hex string of
> the sha1, or the expanded notes ref. It is expected to be re-expanded
> using get_sha1 inside the notes merge machinery, and there is no real
> error checking provided at this layer.
>
> Since we now support merging from non-notes refs, remove the test case
> associated with that behavior. Add a test case for merging from a
> non-notes ref.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---

Anyone have any comments on this one? It's basically Junio's suggestion of just using get_sha1 directly for the remote side of the notes merge...

Regards, Jake

patchnotes: allow merging from arbitrary references