[PATCH] notes: allow merging from arbitrary references

[PATCH] notes: allow merging from arbitrary references

[Jacob Keller]

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(-)

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

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

[Jacob Keller]

On Thu, Oct 1, 2015 at 3:39 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> 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

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

[Jacob Keller]

On Thu, Oct 1, 2015 at 3:39 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> 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

[PATCH] notes: allow merging from arbitrary references

[Jacob Keller]

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

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

[Johan Herland]

On Fri, Nov 13, 2015 at 5:34 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> 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 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, <johan@herland.net>
www.herland.net

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

[Jacob Keller]

On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland <johan@herland.net> 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.
>
> 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.
>

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

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

[Johan Herland]

On Mon, Nov 16, 2015 at 12:23 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland <johan@herland.net> wrote:
>> 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, <johan@herland.net>
www.herland.net

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

[Jacob Keller]

On Sun, Nov 15, 2015 at 11:55 PM, Johan Herland <johan@herland.net> 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
>

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

[Johan Herland]

On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller <jacob.keller@gmail.com> 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, <johan@herland.net>
www.herland.net

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

[Jacob Keller]

On Wed, Nov 18, 2015 at 2:29 PM, Johan Herland <johan@herland.net> wrote:
> On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller <jacob.keller@gmail.com> 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.
>

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.

>> 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.
>
>> 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, <johan@herland.net>
> www.herland.net

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jacob Keller]

On Tue, Nov 24, 2015 at 3:42 PM, Jeff King <peff@peff.net> wrote:
> 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:
>>
>> > ---
>> > 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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:
>> 
>> > ---
>> > 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. :)

I tried to queue
<1447432462-21192-1-git-send-email-jacob.e.keller@intel.com> from
Nov 13 directly on top of 'master', and t3310 does seem to fail.

I'll discard the topic for now, expecting the resurrection sometime
later.

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

[Jacob Keller]

On Fri, Dec 11, 2015 at 11:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I'll discard the topic for now, expecting the resurrection sometime
> later.
>

Hi,

Yes I fully intend to work this against master once I have some time
again. It may be a few weeks as I am extra busy for the holidays, and
the next posting will be fresh on the tip of the future-master :)

Regards,
Jake

[PATCH] notes: allow merging from arbitrary references

[Jacob Keller]

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

Create a new expansion function, expand_loose_notes_ref which will first
check whether the ref can be found using get_sha1. If it can't be found
then it will fallback to using expand_notes_ref. The content of the
strbuf will not be changed if the notes ref can be located using
get_sha1. Otherwise, it may be updated as done by expand_notes_ref.

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>
---
Dropping the version number since this is a resend of a very old patch
and Junio already said he had dropped it from queue.

 builtin/notes.c        |  4 ++--
 notes.c                | 10 ++++++++++
 notes.h                |  7 +++++++
 t/t3308-notes-merge.sh | 22 +++++++++++-----------
 4 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 515cebbeb8a3..0d3e49ef66a8 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 db77922130b4..086cc483e518 100644
--- a/notes.c
+++ b/notes.c
@@ -1303,3 +1303,13 @@ 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);
+	}
+}
diff --git a/notes.h b/notes.h
index 2a3f92338076..431f14378817 100644
--- a/notes.h
+++ b/notes.h
@@ -294,4 +294,11 @@ 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 will check whether the ref can be located
+ * via get_sha1 first, and only falls back to expand_notes_ref in the case
+ * where get_sha1 fails.
+ */
+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.3.505.g5cc1fd1

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

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Create a new expansion function, expand_loose_notes_ref which will first
> check whether the ref can be found using get_sha1. If it can't be found
> then it will fallback to using expand_notes_ref. The content of the
> strbuf will not be changed if the notes ref can be located using
> get_sha1. Otherwise, it may be updated as done by expand_notes_ref.
>
> 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>
> ---
> Dropping the version number since this is a resend of a very old patch
> and Junio already said he had dropped it from queue.

OK, will try to queue again (but I am cutting an -rc today so it may
have to wait a bit).  Those who have been involved in the notes topics
need to review it before the patch can make progress, though.

Thanks.


>
>  builtin/notes.c        |  4 ++--
>  notes.c                | 10 ++++++++++
>  notes.h                |  7 +++++++
>  t/t3308-notes-merge.sh | 22 +++++++++++-----------
>  4 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 515cebbeb8a3..0d3e49ef66a8 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 db77922130b4..086cc483e518 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -1303,3 +1303,13 @@ 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);
> +	}
> +}
> diff --git a/notes.h b/notes.h
> index 2a3f92338076..431f14378817 100644
> --- a/notes.h
> +++ b/notes.h
> @@ -294,4 +294,11 @@ 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 will check whether the ref can be located
> + * via get_sha1 first, and only falls back to expand_notes_ref in the case
> + * where get_sha1 fails.
> + */
> +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)' '

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

[Jacob Keller]

On Mon, Dec 28, 2015 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Create a new expansion function, expand_loose_notes_ref which will first
>> check whether the ref can be found using get_sha1. If it can't be found
>> then it will fallback to using expand_notes_ref. The content of the
>> strbuf will not be changed if the notes ref can be located using
>> get_sha1. Otherwise, it may be updated as done by expand_notes_ref.
>>
>> 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>
>> ---
>> Dropping the version number since this is a resend of a very old patch
>> and Junio already said he had dropped it from queue.
>
> OK, will try to queue again (but I am cutting an -rc today so it may
> have to wait a bit).  Those who have been involved in the notes topics
> need to review it before the patch can make progress, though.
>
> Thanks.
>
>

Some of them have reviewed previous versions, and I believe I have
Cc'ed them again with this patch.

http://article.gmane.org/gmane.comp.version-control.git/281323

Hopefully they will have some time to look at this again.

Regards,
Jake

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> OK, will try to queue again (but I am cutting an -rc today so it may
> have to wait a bit).  Those who have been involved in the notes topics
> need to review it before the patch can make progress, though.

Just FYI, with this the tip of 'pu' seems to fail a few tests in
t3310.