[PATCH] notes: allow merging from arbitrary references

From: Jacob Keller <redacted>

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 <redacted>
---
I do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

I'm interested in whether this is the right direction, as my longterm
goal is to be able to push/pull notes to a specific namespace (probably
refs/remote-notes/*, since actually modifying to use
refs/remotes/notes/* is difficult to send to users, and remote-notes
makes the most useful sense). The first part of this is allowing merge
to come from an arbitrary reference, as currently it is not really
possible to merge from refs/remote-notes as we'd need it to be.

 builtin/notes.c        |  4 ++--
 notes.c                | 14 ++++++++++++++
 notes.h                |  8 ++++++++
 t/t3308-notes-merge.sh | 22 +++++++++++-----------
 4 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index e0f5d308d206..4a86cc90ee92 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c

@@ -809,7 +809,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", NOTES_INIT_WRITABLE);

@@ -836,7 +836,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 358e2fdb74eb..c92c22aa217a 100644
--- a/notes.c
+++ b/notes.c

@@ -1306,3 +1306,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 e5d67fd3754a..658caf7d6e99 100644
--- a/notes.h
+++ b/notes.h

@@ -302,4 +302,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.1.264.gbab76a9

On Fri, Nov 13, 2015 at 5:34 PM, Jacob Keller [off-list ref] wrote:

From: Jacob Keller <redacted>

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 <redacted>
---
I do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

Looks good to me.

I'm interested in whether this is the right direction, as my longterm
goal is to be able to push/pull notes to a specific namespace (probably
refs/remote-notes/*, since actually modifying to use
refs/remotes/notes/* is difficult to send to users, and remote-notes
makes the most useful sense). The first part of this is allowing merge
to come from an arbitrary reference, as currently it is not really
possible to merge from refs/remote-notes as we'd need it to be.

Yes, I agree that merging from refs outside refs/notes/ should become possible.

A related topic that has been discussed (although I cannot remember if
any conclusion was reached) is whether to allow more notes operations
- specifically _read-only_ operations - on notes trees outside
refs/notes/. I believe this should also become possible, although I
haven't thoroughly examined all implications.

...Johan


-- 
Johan Herland, [off-list ref]
www.herland.net

On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland [off-list ref] wrote:

quoted

---
I do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

Looks good to me.

quoted

I'm interested in whether this is the right direction, as my longterm
goal is to be able to push/pull notes to a specific namespace (probably
refs/remote-notes/*, since actually modifying to use
refs/remotes/notes/* is difficult to send to users, and remote-notes
makes the most useful sense). The first part of this is allowing merge
to come from an arbitrary reference, as currently it is not really
possible to merge from refs/remote-notes as we'd need it to be.

Yes, I agree that merging from refs outside refs/notes/ should become possible.

Thanks.

A related topic that has been discussed (although I cannot remember if
any conclusion was reached) is whether to allow more notes operations
- specifically _read-only_ operations - on notes trees outside
refs/notes/. I believe this should also become possible, although I
haven't thoroughly examined all implications.

...Johan

This was discussed at some point on one of the versions of my patch.
The tricky part is in how to get it implemented correctly.

We need to be able to correctly handle DWIM logic for things, and
ensure that what we're operating on actually looks "note-like" since
we don't really want to perform read-only ops on refs that don't hold
notes like objects.

Regards,
Jake

On Mon, Nov 16, 2015 at 12:23 AM, Jacob Keller [off-list ref] wrote:

On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland [off-list ref] wrote:

quoted

A related topic that has been discussed (although I cannot remember if
any conclusion was reached) is whether to allow more notes operations
- specifically _read-only_ operations - on notes trees outside
refs/notes/. I believe this should also become possible, although I
haven't thoroughly examined all implications.

This was discussed at some point on one of the versions of my patch.
The tricky part is in how to get it implemented correctly.

We need to be able to correctly handle DWIM logic for things, and
ensure that what we're operating on actually looks "note-like" since
we don't really want to perform read-only ops on refs that don't hold
notes like objects.

I believe read-only operations on non-notes trees is harmless
(although suboptimal). When reading in a notes tree, the notes code
maintains non-note entries in a sorted linked list. Only paths that
contain exactly 40 hex characters (modulo '/') ends up as "notes"
(i.e. false positives). The rest ends up in the non-notes list. The
overwhelming majority of non-notes trees will have no "notes" in them
(zero false positives).

For those few trees that do contain note-like paths: since we never
write out the tree again, we don't end up corrupting the non-notes
tree itself (which would typically look like changing the "fanout" of
note-like paths, e.g. moving 'de/adbeef...' to 'deadbeef...'). Hence,
the only damage we can get from reading in a non-notes tree depend on
what we subsequently do with the "notes" information read from that
tree.

Again, since the number of "notes" read from a non-notes tree is
typically zero, the subsequent damage is typically, also, zero.

For "git notes merge", false positives from a non-notes tree are
merged into the first (proper) notes tree.

For "git log --notes", false positives end up being displayed as part
of the output. Note that here, a false positive must not only match
the above criteria (40 hex chars, modulo '/'), but must also correctly
name a commit that occurs in the log.

Are there other cases where a false positive would wreak considerable havoc?

Additionally, if we suspect that passing non-notes trees to read-only
operations will be a common error, we could add a simple heuristic to
the notes code, to warn (or even abort) if we strongly suspect that we
are reading in a non-notes tree. For example, if the ratio of
non-notes to notes entries goes above, say, 1:1 (or even 10:1), then
what we're reading is probably not a proper notes tree...


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Sun, Nov 15, 2015 at 11:55 PM, Johan Herland [off-list ref] wrote:

Additionally, if we suspect that passing non-notes trees to read-only
operations will be a common error, we could add a simple heuristic to
the notes code, to warn (or even abort) if we strongly suspect that we
are reading in a non-notes tree. For example, if the ratio of
non-notes to notes entries goes above, say, 1:1 (or even 10:1), then
what we're reading is probably not a proper notes tree...

I agree here for this part, a possible heuristic check would maybe be
valuable.. but not sure it's super worth the effort. I doubt it would
be a common error, and I don't think the issues above would actually
cause too many problems.

The main other issue is how to get notes DWIM things to work for all
cases where we want to use notes refs, since right now the DWIM is
basically done at the top level and only handles notes like things.
The problem with it is that if you specify a full ref that *isn't*
refs/notes, you will always prefix it with refs/notes, like so:

refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,

This makes it really difficult to expand a ref. However, Julio seemed
to think this was a possibly valuable expansion under normal
circumstances. The current solution is to try to do a normal lookup
first and only use the notes DWIM after we fail a lookup, which I
think is what the above patch attempts to do. This seems ok enough to
me.

Regards,
Jake

...Johan

On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller [off-list ref] wrote:

The main other issue is how to get notes DWIM things to work for all
cases where we want to use notes refs, since right now the DWIM is
basically done at the top level and only handles notes like things.
The problem with it is that if you specify a full ref that *isn't*
refs/notes, you will always prefix it with refs/notes, like so:

refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,

I am becoming convinced that this is a bug. I don't know anywhere else
in Git, where a fully qualified ref name (i.e. anything starting with
"refs/") is not interpreted verbatim. For the notes code to do just
that adds unnecessary confusion.

This makes it really difficult to expand a ref. However, Junio seemed
to think this was a possibly valuable expansion under normal
circumstances.

I doubt it. It carries its own set of problems in that refs/foo/bar
(=> refs/notes/refs/foo/bar) is treated differently from refs/notes/bar
(=> refs/notes/bar). If users _really_ want to create
refs/notes/refs/$whatever, they should have to be explicit about that
(i.e. we should require them to say refs/notes/refs/$whatever instead
of allowing them to lazily say refs/$whatever). (It even saves them
from a potential bug if their $whatever happens to start with "notes/",
in which case the current code already forces them to fully qualify...)

I realize this is a backwards-incompatible change in behavior, but I
don't think it'll matter much in practice. Given e.g.

  git notes --ref refs/foo list

when refs/foo and refs/notes/refs/foo is both missing:
  Current behavior: refs/notes/refs/foo lookup fails.
    Treat like empty notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup fails. Same behavior as current.

when refs/notes/refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup succeeds.
    Shows notes in that tree
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup succeeds. Same as current.

when refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup fails. Treat like empty
    notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0

when both refs/foo and refs/notes/refs/foo exist:
  Current behavior: refs/notes/refs/foo lookup succeeds. Shows notes
    in that tree
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0

In other words, this change requires both refs/foo and
refs/notes/refs/foo to be present in order to cause any real confusion.
And in that case, the proposed behavior forces you to use fully-
qualified refs (which will be interpreted as such) whereas the current
behavior takes what looks like a fully-qualified ref (refs/foo) and
interprets it like a notes-shorthand (-> refs/notes/refs/foo), which
I argue is probably more confusing to most users.

The current solution is to try to do a normal lookup
first and only use the notes DWIM after we fail a lookup, which I
think is what the above patch attempts to do. This seems ok enough to
me.

Yes, given $whatever, we should first lookup $whatever, and only
failing that, we should try refs/notes/$whatever. Maybe it's also
worth trying refs/$whatever (before refs/notes/$whatever), since that
would be consistent with what's currently done for other refs (e.g.
try "git log heads/master" or "git log tags/v2.6.3" in git.git).


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Wed, Nov 18, 2015 at 2:29 PM, Johan Herland [off-list ref] wrote:

On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller [off-list ref] wrote:

quoted

The main other issue is how to get notes DWIM things to work for all
cases where we want to use notes refs, since right now the DWIM is
basically done at the top level and only handles notes like things.
The problem with it is that if you specify a full ref that *isn't*
refs/notes, you will always prefix it with refs/notes, like so:

refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,

I am becoming convinced that this is a bug. I don't know anywhere else
in Git, where a fully qualified ref name (i.e. anything starting with
"refs/") is not interpreted verbatim. For the notes code to do just
that adds unnecessary confusion.

I agree, and I had a patch to change this behavior, but the main issue
being I think it didn't fallback to the suggested proposal below.

quoted

This makes it really difficult to expand a ref. However, Junio seemed
to think this was a possibly valuable expansion under normal
circumstances.

I doubt it. It carries its own set of problems in that refs/foo/bar
(=> refs/notes/refs/foo/bar) is treated differently from refs/notes/bar
(=> refs/notes/bar). If users _really_ want to create
refs/notes/refs/$whatever, they should have to be explicit about that
(i.e. we should require them to say refs/notes/refs/$whatever instead
of allowing them to lazily say refs/$whatever). (It even saves them
from a potential bug if their $whatever happens to start with "notes/",
in which case the current code already forces them to fully qualify...)

The question is whether we do:

a) check for refs/abc/xyz and fail if we can't find it or

b) check for refs/abc/xyz and then expand to refs/notes/refs/abc/xyz
if we can't?

I think that the first is generally preferable but with read-only ops
it's not a big deal since we won't be writing to notes trees.

I realize this is a backwards-incompatible change in behavior, but I
don't think it'll matter much in practice. Given e.g.

  git notes --ref refs/foo list

when refs/foo and refs/notes/refs/foo is both missing:
  Current behavior: refs/notes/refs/foo lookup fails.
    Treat like empty notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup fails. Same behavior as current.

when refs/notes/refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup succeeds.
    Shows notes in that tree
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup succeeds. Same as current.

when refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup fails. Treat like empty
    notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0

when both refs/foo and refs/notes/refs/foo exist:
  Current behavior: refs/notes/refs/foo lookup succeeds. Shows notes
    in that tree
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0

In other words, this change requires both refs/foo and
refs/notes/refs/foo to be present in order to cause any real confusion.
And in that case, the proposed behavior forces you to use fully-
qualified refs (which will be interpreted as such) whereas the current
behavior takes what looks like a fully-qualified ref (refs/foo) and
interprets it like a notes-shorthand (-> refs/notes/refs/foo), which
I argue is probably more confusing to most users.

quoted

The current solution is to try to do a normal lookup
first and only use the notes DWIM after we fail a lookup, which I
think is what the above patch attempts to do. This seems ok enough to
me.

Yes, given $whatever, we should first lookup $whatever, and only
failing that, we should try refs/notes/$whatever. Maybe it's also
worth trying refs/$whatever (before refs/notes/$whatever), since that
would be consistent with what's currently done for other refs (e.g.
try "git log heads/master" or "git log tags/v2.6.3" in git.git).

The biggest implementation issue here is that the notes code that does
DWIM happens before lookup of whether the ref exists, and the code
that does lookup of refs in the notes.c code won't fallback and try a
different expansion.

I think that is why my proposed change was dropped, if I remember correctly.

I am in agreement with you, and think we should use the proposed
behavior above, as it is very unlikely to cause any issues with
current cases, especially since we already try not to allow operation
on refs outside of the notes tree today.

Regards,
Jake

...Johan

--
Johan Herland, [off-list ref]
www.herland.net

On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:

---
I do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

Please keep topics branched from master where possible. And if not
possible, please indicate which topic in 'next' is required to build on.

We never merge 'next' itself, only individual topics from it. So I can't
just apply your patch on top of 'next'.

I did get it to apply on the current master with "am -3", but some tests
in t3310 seem to fail. Can you take a look?

I skimmed the discussion with Johan that followed this. Are we happy
with this as a first step, or would people rather look at re-working the
notes-ref lookups everywhere?

-Peff

On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:

On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:

quoted

---
I do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

Please keep topics branched from master where possible. And if not
possible, please indicate which topic in 'next' is required to build on.

We never merge 'next' itself, only individual topics from it. So I can't
just apply your patch on top of 'next'.

I did get it to apply on the current master with "am -3", but some tests
in t3310 seem to fail. Can you take a look?

I just noticed v2, which I missed earlier. But the same complaints
apply. :)

-Peff

On Tue, Nov 24, 2015 at 3:42 PM, Jeff King [off-list ref] wrote:

On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:

quoted

On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:

quoted

---
I do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

Please keep topics branched from master where possible. And if not
possible, please indicate which topic in 'next' is required to build on.

We never merge 'next' itself, only individual topics from it. So I can't
just apply your patch on top of 'next'.

I did get it to apply on the current master with "am -3", but some tests
in t3310 seem to fail. Can you take a look?

I just noticed v2, which I missed earlier. But the same complaints
apply. :)

-Peff

Yea.. sorry about that. I normally work off next since this is what I
use day to day for general git use, as I like to run the bleedy edge.

I can respin these on master, but it may take a bit of time as I am on
vacation at the moment.

I'm also curious if people would rather go the more difficult route
first or not.

Regards,
Jake