[PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes

[PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes

[Jacob Keller]

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

The primary purpose of this series is to allow fetching remote notes
into a namespace such as "refs/remote-notes/<remote>/foo". Currently,
git-notes refuses to operate on refs outside of refs/notes/* including
merging from them, listing or showing them. This makes it difficult to
share notes from a remote repository.

Fix expand_notes_ref to not always prepend refs/notes to fully qualified
refs. Allow git-notes actions which do not specify NOTES_INIT_WRITABLE
to be performed on refs outside of refs/notes/*

Future work will include more coupling of "refs/remote-notes/<remote>"
into git-notes so that you can specify refs as "<remote>/foo" similar to
how remotes work for branches.

In addition, long term goal is to make it default to fetch notes into
refs/remote-notes/<remote>/*, and possibly to add some status for
tracking similar to how tracking branches work.

The one downside currently is that a test case for prevention of merge
from "refs/heads/master" had to be removed as "git notes merge
refs/heads/master" now works. I am not sure how this could be fixed.. I
did not find any way to tell if a treeish actually was a notes tree or
not...

This topic depends on mh/notes-allow-reading-treeish and actually
expands what this topic allowed before. Previously, treeishes such as
notes@{1} were made allowable, but the ref still had to be found under
refs/notes.

No documentation changes were made as from the looks of it,
documentation for --ref and core.notesRef is already correct despite the
previous behavior of expand_notes_ref. In that sense, documentation was
wrong before.

Jacob Keller (2):
  notes: don't expand qualified refs in expand_notes_ref
  notes: allow non-writable actions on refs outside of refs/notes

 builtin/notes.c        | 11 ++++++-----
 notes.c                |  4 ++--
 t/t3308-notes-merge.sh |  1 -
 3 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.6.0.rc2.248.g5b5be23

[PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Jacob Keller]

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

The documentation for --refs says that it will treat unqualified refs as
under refs/notes. Current behavior is to prefix refs/notes to all
strings that do not start with refs/notes or notes/, resulting in
performing actions on refs such as "refs/notes/refs/foo/bar" instead of
attempting to perform actions on "refs/foo/bar". A future patch will
introduce the idea of performing non-writable actions to refs outside of
refs/notes. Change the behavior of expand_notes_ref to leave qualified
refs under refs/* alone.

In addition, fix git notes merge <ref> to prevent merges from refs which
are not under refs/notes, in a similar way to how we already check note
refs in init_notes_check. This is required in order to keep current
tests passing without change. A future patch will change the behavior of
git notes merge so that it can merge from a ref outside of refs/notes.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/notes.c | 4 ++++
 notes.c         | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 6371d536d164..0f55d38983f0 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -810,6 +810,10 @@ 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);
+	if (!starts_with(remote_ref.buf, "refs/notes"))
+		die("Refusing to merge notes from %s (outside of refs/notes/)",
+		    remote_ref.buf);
+
 	o.remote_ref = remote_ref.buf;
 
 	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
diff --git a/notes.c b/notes.c
index 6ef347ca3ac4..d49168fb3f01 100644
--- a/notes.c
+++ b/notes.c
@@ -1296,8 +1296,8 @@ int copy_note(struct notes_tree *t,
 
 void expand_notes_ref(struct strbuf *sb)
 {
-	if (starts_with(sb->buf, "refs/notes/"))
-		return; /* we're happy */
+	if (starts_with(sb->buf, "refs/"))
+		return; /* fully qualified, so we're happy */
 	else if (starts_with(sb->buf, "notes/"))
 		strbuf_insert(sb, 0, "refs/", 5);
 	else
-- 
2.6.0.rc2.248.g5b5be23

[PATCH 2/2] notes: allow non-writable actions on refs outside of refs/notes

[Jacob Keller]

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

Allow non-destructive notes actions which do not require write
permission to be performed on refs outside of refs/notes/. The primary
advantage of this is to allow fetching remote refs to such location as
"refs/remote-notes/<remote>/foo" and then performing merges into
refs/notes/

It is not reasonable to put remote notes inside refs/notes as users may
already have conflicting names inside the notes namespace.

Remove one test case regarding merge from refs/heads/master, which will
now pass under current code. It may be worth looking how to prevent
some of these more obviously wrong merges.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/notes.c        | 13 +++++--------
 t/t3308-notes-merge.sh |  1 -
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 0f55d38983f0..ff056014d953 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -333,14 +333,14 @@ static struct notes_tree *init_notes_check(const char *subcommand,
 					   int flags)
 {
 	struct notes_tree *t;
-	const char *ref;
 	init_notes(NULL, NULL, NULL, flags);
 	t = &default_notes_tree;
 
-	ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;
-	if (!starts_with(ref, "refs/notes/"))
-		die("Refusing to %s notes in %s (outside of refs/notes/)",
-		    subcommand, ref);
+	if (flags & NOTES_INIT_WRITABLE) {
+		if (!starts_with(t->update_ref, "refs/notes"))
+			die("Refusing to %s notes in %s (outside of refs/notes/)",
+			    subcommand, t->update_ref);
+	}
 	return t;
 }
 
@@ -810,9 +810,6 @@ 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);
-	if (!starts_with(remote_ref.buf, "refs/notes"))
-		die("Refusing to merge notes from %s (outside of refs/notes/)",
-		    remote_ref.buf);
 
 	o.remote_ref = remote_ref.buf;
 
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b49bbea..f0feb64bae6e 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -90,7 +90,6 @@ test_expect_success 'fail to merge various non-note-trees' '
 	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
-- 
2.6.0.rc2.248.g5b5be23

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Junio C Hamano]

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

> From: Jacob Keller <jacob.keller@gmail.com>
>
> The documentation for --refs says that it will treat unqualified refs as
> under refs/notes. Current behavior is to prefix refs/notes to all
> strings that do not start with refs/notes or notes/, resulting in
> performing actions on refs such as "refs/notes/refs/foo/bar" instead of
> attempting to perform actions on "refs/foo/bar".

That actually sounds like a sensible thing to do, if you replace
'foo' with 'heads', for example, i.e. refs/notes/refs/heads/bar is a
notes about commits reachable from the branch whose name is 'bar'.

So given "refs/heads/bar", which is unqualified in the context of
talking about references that hold notes trees, the current
behaviour to turn it into "refs/notes/refs/heads/bar" is very
sensible, I would think.

Re: [PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes

[Mike Hommey]

On Wed, Sep 16, 2015 at 03:06:32PM -0700, Jacob Keller wrote:
> This topic depends on mh/notes-allow-reading-treeish and actually
> expands what this topic allowed before. Previously, treeishes such as
> notes@{1} were made allowable, but the ref still had to be found under
> refs/notes.

I haven't found the time to finish that topic yet, but I haven't
forgotten it. Sorry about that.

Mike

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Jacob Keller]

On Wed, Sep 16, 2015 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> The documentation for --refs says that it will treat unqualified refs as
>> under refs/notes. Current behavior is to prefix refs/notes to all
>> strings that do not start with refs/notes or notes/, resulting in
>> performing actions on refs such as "refs/notes/refs/foo/bar" instead of
>> attempting to perform actions on "refs/foo/bar".
>
> That actually sounds like a sensible thing to do, if you replace
> 'foo' with 'heads', for example, i.e. refs/notes/refs/heads/bar is a
> notes about commits reachable from the branch whose name is 'bar'.
>
> So given "refs/heads/bar", which is unqualified in the context of
> talking about references that hold notes trees, the current
> behaviour to turn it into "refs/notes/refs/heads/bar" is very
> sensible, I would think.
>

The end goal is to allow refs inside "refs/remote-notes/".. it seems
really weird that other fully qualified refs don't get "expanded" in
the same way as notes, and documentation does not make it explicit
that this is how it would work. I think that users who actually want
this behavior are already free to say "refs/notes/refs/heads/bar"...
that wouldn't change..

How would you propose allowing merging from "refs/remote-notes/<remote>/bar"?

We could easily just hard-code acceptance of refs/remote-notes/ as
well as refs/notes... But that felt weird to me...

But honestly I don't really care how it is done as long as we can "git
notes show", "git notes list" on refs/remote-notes/<origin>/commits
(or similar, remote-notes may not be the actual location if someone
came up with a better name?)

How would you propose we allow that?

If we keep the current behavior of "expand_notes_ref" then we
absolutely can't because use of "--ref" will auto expand
"refs/remote-notes/<origin>/commits" into
"refs/notes/refs/remote-notes/<origin>/commits" which wouldn't work...

Regards,
Jake

Re: [PATCH 0/2] notes: allow read only notes actions on refs outside of refs/notes

[Jacob Keller]

On Wed, Sep 16, 2015 at 3:36 PM, Mike Hommey <mh@glandium.org> wrote:
> On Wed, Sep 16, 2015 at 03:06:32PM -0700, Jacob Keller wrote:
>> This topic depends on mh/notes-allow-reading-treeish and actually
>> expands what this topic allowed before. Previously, treeishes such as
>> notes@{1} were made allowable, but the ref still had to be found under
>> refs/notes.
>
> I haven't found the time to finish that topic yet, but I haven't
> forgotten it. Sorry about that.
>
> Mike

That's ok. I don't think any of the remaining re-works left for your
patch impact mine..? I haven't really read it, but I know I make use
if NOTES_INIT_WRITABLE is the main reason.

Regards,
Jake

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Jacob Keller]

On Wed, Sep 16, 2015 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> The documentation for --refs says that it will treat unqualified refs as
>> under refs/notes. Current behavior is to prefix refs/notes to all
>> strings that do not start with refs/notes or notes/, resulting in
>> performing actions on refs such as "refs/notes/refs/foo/bar" instead of
>> attempting to perform actions on "refs/foo/bar".
>
> That actually sounds like a sensible thing to do, if you replace
> 'foo' with 'heads', for example, i.e. refs/notes/refs/heads/bar is a
> notes about commits reachable from the branch whose name is 'bar'.
>
> So given "refs/heads/bar", which is unqualified in the context of
> talking about references that hold notes trees, the current
> behaviour to turn it into "refs/notes/refs/heads/bar" is very
> sensible, I would think.
>

I never got any better suggestion on how to allow the behavior
desired, which is to enable merging from a non-notes location, in
order to provide a standard location for remote notes, ie:
refs/remote-notes/<remote>/<ref>

Any suggestions, if you're against this particular change?

Regards,
Jake

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Junio C Hamano]

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

> I never got any better suggestion on how to allow the behavior
> desired, which is to enable merging from a non-notes location, in
> order to provide a standard location for remote notes, ie:
> refs/remote-notes/<remote>/<ref>

Step back a bit and think again.  I think you are blinded by your
refs/remote-notes/*.

It is fine to wish that

    $ notes merge refs/notes/commits refs/remote-notes/origin/commits

to work, but you shouldn't force your users to use remote-tracking
refs in the first place.  Your users should be allowed to do this as
well:

    $ fetch origin refs/notes/commits
    $ THEIRS=$(git rev-parse FETCH_HEAD)
    $ notes merge refs/notes/commits $THEIRS

We need to realize that "notes merge" involves two notes trees and
they are of different nature.  The notes tree you are merging into
and recording the result (the destination), which will be a local
note, and the other notes tree you obtained from elsewhere and
update that local note with (the source).

The current code before your patch limits the allowed pair of notes
trees by insisting that both appear as the tips of refs somewhere in
refs/notes/*.  For allowing to merge from outside refs/notes/, you
need to loosen the location the latter notes tree, the one to update
your local notes tree with, can come from.  But that does not mean
you would want to loosen the location where the resulting notes tree
can go.

I think the proposed patch conflates them, and that conflation does
not help anything.  The rule of that function used to be "It must
come from refs/notes/ and nowhere else".  That made sense in the old
world order where both must be from refs/notes/, and the rule still
makes sense in the new world order for the destination of the merge.

The new rule of that function after the proposed patch says "It must
come from either refs/notes or refs/ somewhere".  This does not make
sense for the destination because it is too loose (and we didn't see
any justification why loosening it is a good idea), and it does not
make sense for the source because it still is too tight.  It should
be able to take anything get_sha1() understands (including $THEIRS
in the above example).

In addition you might also want to allow additional DWIMs from X to
refs/remote-notes/*/X as well as from X to refs/notes/X, but that is
secondary and should be done as a follow-up "nice to have" change,
because both "notes/remote-notes/origin/commits" and "notes/commits"
would be understood by get_sha1() already, and it is questinable if
it is a good idea to introduce special DWIMs that kick in only when
the commands are talking about notes in the first place (an equally
questionable alternative is to teach get_sha1() about refs/notes/*
and refs/remote-notes/*/*, which will make the disambiguation noisy
in the normal non-notes codepath---my knee-jerk reaction is to
suggest not to go there, either).

In any case, to get us closer to that endgame, change in the
proposed patch does not help.  It tries to cover two different cases
with a logic that is not a good match to either.  You need to have
two separate helpers to interpret the source and the destination.

Calls expand_notes_ref() made on a command line argument that
specifies the source (which I think is similar to what the other
recent topic calls "read-only") should be made to calls to a more
lenient version (and you can start with get_sha1() for that purpose
without introducing your own DWIMs in the first step), while leaving
calls to expand_notes_ref() for destination and the implementation
of expand_notes_ref() itself unmolested, so that we can keep the
safety in expands_notes_ref() that makes sure that the destination
of a local operation is under refs/notes/*.

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Jacob Keller]

On Tue, Sep 22, 2015 at 7:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The current code before your patch limits the allowed pair of notes
> trees by insisting that both appear as the tips of refs somewhere in
> refs/notes/*.  For allowing to merge from outside refs/notes/, you
> need to loosen the location the latter notes tree, the one to update
> your local notes tree with, can come from.  But that does not mean
> you would want to loosen the location where the resulting notes tree
> can go.

I do not loosen where the written location can go. My patch series
sort of does the opposite of where you suggest. I change
expand_notes_ref to be more "loose" but not still keep the current
checks in place which ensure that refs outside of refs/notes don't
write to them.

>
> I think the proposed patch conflates them, and that conflation does
> not help anything.  The rule of that function used to be "It must
> come from refs/notes/ and nowhere else".  That made sense in the old
> world order where both must be from refs/notes/, and the rule still
> makes sense in the new world order for the destination of the merge.
>

Yes, I don't change the destination rule, though I do change how we
can "DWIM" so that "attempting" to merge into "refs/balagrg/foo" does
not actually merge into "refs/notes/refs/balagrg/foo". I think this is
sane.

The actual "init_notes_check" code prevents merging into refs outside
of refs/notes/*

I think it's incredibly confusing that other DWIM's do not expand
"refs/*" into "refs/something/refs/*"

> The new rule of that function after the proposed patch says "It must
> come from either refs/notes or refs/ somewhere".  This does not make
> sense for the destination because it is too loose (and we didn't see
> any justification why loosening it is a good idea), and it does not
> make sense for the source because it still is too tight.  It should
> be able to take anything get_sha1() understands (including $THEIRS
> in the above example).

Agreed, we should use something more expansive for the non-write
operations. I still think the proposed change to expand_notes_refs
would be good... but that's because I find it incredibly confusing.
Apparently we disagree on that.

>
> In addition you might also want to allow additional DWIMs from X to
> refs/remote-notes/*/X as well as from X to refs/notes/X, but that is
> secondary and should be done as a follow-up "nice to have" change,
> because both "notes/remote-notes/origin/commits" and "notes/commits"
> would be understood by get_sha1() already, and it is questinable if
> it is a good idea to introduce special DWIMs that kick in only when
> the commands are talking about notes in the first place (an equally
> questionable alternative is to teach get_sha1() about refs/notes/*
> and refs/remote-notes/*/*, which will make the disambiguation noisy
> in the normal non-notes codepath---my knee-jerk reaction is to
> suggest not to go there, either).

The DWIM's I suggest are "foo" -> "refs/notes/foo", "notes/foo" ->
"refs/notes/foo", as these already work, in addition to "<origin>/foo"
-> "refs/remote-notes/<origin>/foo" and "<origin>/notes/foo" ->
"refs/remote-notes/<origin>/foo" Obviously only kicking in for notes
references.

>
> In any case, to get us closer to that endgame, change in the
> proposed patch does not help.  It tries to cover two different cases
> with a logic that is not a good match to either.  You need to have
> two separate helpers to interpret the source and the destination.
>

My patch does the opposite of your suggestion: Loosen expand_notes_ref
while keeping the same overall restrictions, which has the same result
if being considered a little backward.

> Calls expand_notes_ref() made on a command line argument that
> specifies the source (which I think is similar to what the other
> recent topic calls "read-only") should be made to calls to a more
> lenient version (and you can start with get_sha1() for that purpose
> without introducing your own DWIMs in the first step), while leaving
> calls to expand_notes_ref() for destination and the implementation
> of expand_notes_ref() itself unmolested, so that we can keep the
> safety in expands_notes_ref() that makes sure that the destination
> of a local operation is under refs/notes/*.

expand_notes_ref doesn't actually provide the safety net. You can
bypass this entirely by using the command line, at least for the
non-write operations. In addition, documentation says "non qualified
refs will be expanded" which usually means "refs/*" will be left
alone, but in this case "refs/*" outside of refs/notes will be
expanded.

Even with just this patch the only difference is an attempt to use
refs/foo/bad *won't* expand into "refs/notes/refs/foo/bar" which is an
expansion I find incredibly confusing to begin with.

Regards,
Jake

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Jacob Keller]

On Tue, Sep 22, 2015 at 7:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Calls expand_notes_ref() made on a command line argument that
> specifies the source (which I think is similar to what the other
> recent topic calls "read-only") should be made to calls to a more
> lenient version (and you can start with get_sha1() for that purpose
> without introducing your own DWIMs in the first step), while leaving
> calls to expand_notes_ref() for destination and the implementation
> of expand_notes_ref() itself unmolested, so that we can keep the
> safety in expands_notes_ref() that makes sure that the destination
> of a local operation is under refs/notes/*.

The other issue here is that expand_notes_ref is called on the --ref
argument waaay before the current code even decides if the operation
is "read" or "write". Thus we'd have to break this out and handle
things very differently.

I don't believe expand_notes_ref() is actually providing any safety.
That is all done by init_notes_check. Note that passing the
environment variable bypasses the expand_notes_ref entirely, though I
think we did use it as part of the refs configuration.

It seems like a lot more heavy lifting to change the entire flow of
how we decide when to expand "--ref" for "read" operations vs "write"
operations.

That is, git notes list.

It's easy to change behavior of git notes merge as it handles its
argument for where to merge from separately, but it's not so easy to
change git notes show...

Regards,
Jake

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Junio C Hamano]

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

> The other issue here is that expand_notes_ref is called on the --ref
> argument waaay before the current code even decides if the operation
> is "read" or "write". Thus we'd have to break this out and handle
> things very differently.

I think you hit the right nail here.  The handling of --ref argument
is what you need to adjust to the new world order.

And "safety" is a red herring.  Those who are used to run

	git log --notes=refs/heads/master

and relies on it to refer to refs/notes/refs/heads/master must
continue to be able to do so.  Changing expand_notes_ref() the
way the proposed patch does will break them, won't it?  "safety"
is not the only (or even the primary) requirement, but the
correctness must also be kept.

> It seems like a lot more heavy lifting to change the entire flow of
> how we decide when to expand "--ref" for "read" operations vs "write"
> operations.
>
> That is, git notes list.
>
> It's easy to change behavior of git notes merge as it handles its
> argument for where to merge from separately, but it's not so easy to
> change git notes show...

Yes, exactly.

I am _more than_ OK to see that read-only accesses to the notes tree
allowed anything under refs/ (like the proposed patch did) and also
a raw 40-hex object name in the endgame, but I really would not want
to see "we meant well and attempted to enhance 'notes merge' but we
ended up regressing the behaviour of unrelated operations all of a
sudden".

If you cannot do your change without breaking the existing users,
then you at least need a sound transition plan.  But I am not sure
yet if you have to break the world (yet).  Let me think aloud a bit
more.

There aren't all that many callers of expand_notes_ref().

My preference is to leave that function as-is, especially keep the
behaviour of the caller in revision.c that handles --show-notes=
(and --notes= that is its synonym) the same as before.

All the other callers are only reachable from the codepath that
originates from cmd_notes(), if I am reading the code correctly, and
that can be enhanced without breaking the existing users.  One
obvious way to do so would be to make "--ref" to keep the call to
expand_notes_ref(), and add another "--notes-rawref" or whatever
option that does not restrict it to "refs/notes", but there may be
other ways.

Re: [PATCH 1/2] notes: don't expand qualified refs in expand_notes_ref

[Jacob Keller]

On Tue, Sep 22, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> The other issue here is that expand_notes_ref is called on the --ref
>> argument waaay before the current code even decides if the operation
>> is "read" or "write". Thus we'd have to break this out and handle
>> things very differently.
>
> I think you hit the right nail here.  The handling of --ref argument
> is what you need to adjust to the new world order.
>
> And "safety" is a red herring.  Those who are used to run
>
>         git log --notes=refs/heads/master
>
> and relies on it to refer to refs/notes/refs/heads/master must
> continue to be able to do so.  Changing expand_notes_ref() the
> way the proposed patch does will break them, won't it?  "safety"
> is not the only (or even the primary) requirement, but the
> correctness must also be kept.

I think that's more confusing than helpful, but we really shouldn't
change behavior here. I think we need to update documentation to
clearly indicate the current behavior, as it was not obvious at least
to me. I can submit a patch for this at least.


>
>> It seems like a lot more heavy lifting to change the entire flow of
>> how we decide when to expand "--ref" for "read" operations vs "write"
>> operations.
>>
>> That is, git notes list.
>>
>> It's easy to change behavior of git notes merge as it handles its
>> argument for where to merge from separately, but it's not so easy to
>> change git notes show...
>
> Yes, exactly.
>
> I am _more than_ OK to see that read-only accesses to the notes tree
> allowed anything under refs/ (like the proposed patch did) and also
> a raw 40-hex object name in the endgame, but I really would not want
> to see "we meant well and attempted to enhance 'notes merge' but we
> ended up regressing the behaviour of unrelated operations all of a
> sudden".
>
> If you cannot do your change without breaking the existing users,
> then you at least need a sound transition plan.  But I am not sure
> yet if you have to break the world (yet).  Let me think aloud a bit
> more.
>
> There aren't all that many callers of expand_notes_ref().
>
> My preference is to leave that function as-is, especially keep the
> behaviour of the caller in revision.c that handles --show-notes=
> (and --notes= that is its synonym) the same as before.
>

Ok.

> All the other callers are only reachable from the codepath that
> originates from cmd_notes(), if I am reading the code correctly, and
> that can be enhanced without breaking the existing users.  One
> obvious way to do so would be to make "--ref" to keep the call to
> expand_notes_ref(), and add another "--notes-rawref" or whatever
> option that does not restrict it to "refs/notes", but there may be
> other ways.
>

I think the easiest way would be to have --ref check ahead of time
which commands are read or write, and perform the init_notes_check
code there instead of inside each command. I'll look at this again
when I have time in the next few days.

Regards,
Jake