[PATCH v2 0/2] Accept any notes ref

[PATCH v2 0/2] Accept any notes ref

[Kyle J. McKay]

This is a resend of the original patch by Scott Chacon combined with the
suggested patch by Johan Herland with a summary of the discussion so far
in the hope to restart discussion about including this change.

The original thread can be found at [1].

The history so far:

The Patch 1/2 was originally submitted to allow using refs outside of
the refs/notes/ namespace as notes refs for the purpose of merging notes.
However, that patch actually just relaxes the restriction on notes refs so
that a fully-qualified ref will be used as-is in the notes code and affects
more than just notes merging.

The concern was then raised that this is a "stealth-enabler" patch and that
even if notes refs are allowed outside of refs/notes/ then they should still
be restricted to a some subset (to be determined) of the the refs/ hierarchy
and not just allowed anywhere.

But, I feel that the rest of Git does not restrict the user in this way -- if
the user really wants to do something that is "not recommended" there is
almost always a mechanism and/or option to leave the user in control and allow
it despite any recommendation(s) to the contrary.

So I am resending this summary with attached patches (both of which are
very trivial) to allow using any ref for notes as long as it's fully qualified
(i.e. starts with "refs/").  Patch 1/2 does that and patch 2/2 fixes the test
that breaks because of it.

In the hope of restarting discussion towards enabling use of refs outside of
refs/notes/ with the notes machinery I conclude by including Peff's final
reply to the original thread which I think contains the most compelling
aregument for inclusion:

On Dec 4, 2014, at 02:26, Jeff King wrote:

> On Sat, Nov 22, 2014 at 10:04:57AM -0800, Kyle J. McKay wrote:
>
>> On Sep 19, 2014, at 11:22, Junio C Hamano wrote:
>>
>>> By "stealth enabler" I mean the removal of refs/notes/ restriction
>>> that was originally done as a safety measure to avoid mistakes of
>>> storing notes outside.  The refs/remote-notes/ future direction
>>> declares that it is no longer a mistake to store notes outside
>>> refs/notes/, but that does not necessarily have to mean that
>>> anywhere under refs/ is fine.  It may make more sense to be explicit
>>> with the code touched here to allow traditional refs/notes/ and the
>>> new hierarchy only.  That way, we will still keep the "avoid
>>> mistakes" safety and enable the new layout at the same time.
>>
>> This is the part where I want to lobby for inclusion of this change.
>> I do not believe it is consistent with existing Git practice to
>> enforce restrictions like this (you can only display/edit/etc. notes
>> under refs/notes/*).
>
> Yeah, this is the compelling part to me. There is literally no way to
> utilize the notes codes for any ref outside of refs/notes currently.
> We don't know if refs/remote-notes is the future, or refs/remotes/
> origin/notes, or something else. But we can't even experiment with it in a
> meaningful way because the plumbing layer is so restrictive.
>
> The notes feature has stagnated. Not many people use it because it  
> requires so much magic to set up and share notes. I think it makes sense to  
> remove a safety feature that is making it harder to experiment with. If the  
> worst case is that people start actually _using_ notes and we get  
> proliferation of places that people are sticking them in the refs hierarchy,
> that is vastly preferable IMHO to the current state, in which few people use
> them and there is little support for sharing them at all.

-Kyle

[1] http://thread.gmane.org/gmane.comp.version-control.git/257281

Kyle J. McKay (1):
  t/t3308-notes-merge.sh: succeed with relaxed notes refs

Scott Chacon (1):
  notes: accept any ref

 notes.c                | 2 +-
 t/t3308-notes-merge.sh | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

-- 
2.1.4

[PATCH v2 1/2] notes: accept any ref

[Kyle J. McKay]

From: Scott Chacon <schacon@gmail.com>

Currently if you try to merge notes, the notes code ensures that the
reference is under the 'refs/notes' namespace. In order to do any sort
of collaborative workflow, this doesn't work well as you can't easily
have local notes refs seperate from remote notes refs.

This patch changes the expand_notes_ref function to check for simply a
leading refs/ instead of refs/notes to check if we're being passed an
expanded notes reference. This would allow us to set up
refs/remotes-notes or otherwise keep mergeable notes references outside
of what would be contained in the notes push refspec.

Signed-off-by: Scott Chacon <schacon@gmail.com>
---
 notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notes.c b/notes.c
index c763a21e..7165a33f 100644
--- a/notes.c
+++ b/notes.c
@@ -1292,7 +1292,7 @@ int copy_note(struct notes_tree *t,
 
 void expand_notes_ref(struct strbuf *sb)
 {
-	if (starts_with(sb->buf, "refs/notes/"))
+	if (starts_with(sb->buf, "refs/"))
 		return; /* we're happy */
 	else if (starts_with(sb->buf, "notes/"))
 		strbuf_insert(sb, 0, "refs/", 5);
-- 
2.1.4

[PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

With the recent change to allow notes refs to be located in
the refs hierarchy in locations other than refs/notes/ the
'git notes merge refs/heads/master' test started succeeding.

Previously refs/heads/master would have been expanded to
a non-existing, ref refs/notes/refs/heads/master, and the
merge would have failed (as expected).

Now, however, since refs/heads/master exists and the new,
more relaxed notes refs rules leave it unchanged, the merge
succeeds.  This has a follow-on effect which makes the
next two tests fail as well.

The refs/heads/master ref could just be replaced with
another ref name that does not exist such as refs/heads/xmaster,
but there are already several tests using non-existant refs
so instead just remove the refs/heads/master line.

Suggested-by: Johan Herland <johan@herland.net>
Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
 t/t3308-notes-merge.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b49..f0feb64b 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.1.4

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> Now, however, since refs/heads/master exists and the new,
> more relaxed notes refs rules leave it unchanged, the merge
> succeeds. ...
> ...
> diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
> index 24d82b49..f0feb64b 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

The test title reads "fail to merge non-note trees", and I am
assuming that the tree-ish refs/heads/master (aka 'master' branch)
represents does not look anything like a typical note tree where
pathnames are 40-hex with fan-out.

The fact that "git notes merge refs/heads/master" fails is a very
good prevention of end-user mistakes, and this removal of test
demonstrates that we are dropping a valuable safety.

Arguably, not being able to save notes tree anywhere outside of
refs/notes/ hierarchy may be too high a price to pay in order to
prevent refs/heads/master from being considered (hence to avoid such
end-user mistakes), but at the same time, losing this safetly may
also be too high a price to pay in order to allow people to store
their notes in somewhere outside e.g. refs/remote-notes/origin/foo.
"Somewhere outside" does not mean "Including other hierarchies like
refs/heads and refs/tags that have long established meaning".

Although I am not fundamentally against allowing to store notes
outside refs/notes/, it is different from "anywhere is fine".
Can't we do this widening in a less damaging way?

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 02:20, Junio C Hamano wrote:

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> Now, however, since refs/heads/master exists and the new,
>> more relaxed notes refs rules leave it unchanged, the merge
>> succeeds. ...
>> ...
>> diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
>> index 24d82b49..f0feb64b 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
>
> The test title reads "fail to merge non-note trees", and I am
> assuming that the tree-ish refs/heads/master (aka 'master' branch)
> represents does not look anything like a typical note tree where
> pathnames are 40-hex with fan-out.

In fact it looks like this:

100644 blob 2a5d0158a25a97e8ebf4158d9187acb124da50ea	1st.t
100644 blob 3f514b8c0d8e9345bda64de1664eb43d7d38d12a	2nd.t
100644 blob e5404b81e697da4f0c99aac167b5e63bcce4b78b	3rd.t
100644 blob c950fbad52232390031696035ad79c670ee3bd7b	4th.t
100644 blob ba96e617c4d2741ac7693ca7eb20f9dddf4754f6	5th.t

so you are correct.

> The fact that "git notes merge refs/heads/master" fails is a very
> good prevention of end-user mistakes, and this removal of test
> demonstrates that we are dropping a valuable safety.

At the point the dropped line runs, core.notesRef has been set to refs/ 
notes/y which does not exist.

All of the tests in the 'fail to merge various non-note-trees' section  
fail with one of these errors:

   1) Failed to resolve remote notes ref '<ref-being-tested>'

   2) Cannot merge empty notes ref (<ref-being-tested>) into empty  
notes ref (refs/notes/y)

   3) error: object 6c99d48c9905deea5d59d723468862362918626a is a  
tree, not a commit

The 3rd error comes from the "git notes merge x:" attempt.

So despite the name of the test, the actual tree contents do not seem  
to be examined.

When the notes ref checking is relaxed to leave refs/heads/master  
alone rather than turning it into refs/notes/refs/heads/master, the  
previous error (#2 in this case) goes away and since refs/notes/y does  
not exist, it is simply updated to the value of refs/heads/master  
without any checks.  Of course that refs/heads/master tree doesn't  
look like a notes tree.

And if we do this:

   git update-ref refs/notes/refs/heads/master master

Then "git notes merge refs/heads/master" also succeeds without  
complaining that the refs/notes/refs/heads/master tree does not look  
like a notes tree and we didn't need to relax the refs/notes  
restriction and, as you point out, the name of the test seems to imply  
that would be rejected.

Interestingly, if we then attempt to merge refs/notes/x into this non- 
notes refs/notes/y tree, it also succeeds and even keeps the things  
that do not look like notes.  The reverse merge (y into x) succeeds as  
well, but the non-notes stuff in y is not merged in in that case.

> Arguably, not being able to save notes tree anywhere outside of
> refs/notes/ hierarchy may be too high a price to pay in order to
> prevent refs/heads/master from being considered (hence to avoid such
> end-user mistakes), but at the same time, losing this safetly may
> also be too high a price to pay in order to allow people to store
> their notes in somewhere outside e.g. refs/remote-notes/origin/foo.
> "Somewhere outside" does not mean "Including other hierarchies like
> refs/heads and refs/tags that have long established meaning".

If we relax the refs/notes restriction, putting a notes ref in refs/ 
heads/<whatever> doesn't necessarily seem like that's a terrible thing  
as long as it's really a notes tree if used with the notes machinery.   
AIUI, the refs/heads convention only requires the ref to point to the  
tip of a commit chain which all of the refs under refs/notes satisfy.   
The refs/heads convention AIUI does not impose any requirement about  
the contents of the tree(s) those commits in the chain refer to.  But  
at the same time I can't think of any particular reason I'd want to  
store notes refs in there either.

> Although I am not fundamentally against allowing to store notes
> outside refs/notes/, it is different from "anywhere is fine".
> Can't we do this widening in a less damaging way?

Without arbitrarily restricting where notes can be stored it seems to  
me the only option would be to have the notes machinery actually  
inspect the tree of any existing notes ref it's passed.  That would  
also catch the case where "git update-ref refs/notes/refs/heads/master  
master" was run as well.  It also seems like a good check to have in  
place to help catch user errors.

I'm not all that familiar with the notes code, perhaps there's already  
a function that does the tree check to make sure the tree actually  
looks like a notes tree that can easily be called?  Maybe Johan has  
some thoughts on this?

-Kyle

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> So despite the name of the test, the actual tree contents do not seem
> to be examined.

Yes, but the thing is, thanks to refs/notes restriction, there is no
need to do such examination [*1*].

Note that it is an entirely different matter when you deliberately
violate the expectation using plumbing (e.g. update-ref).  Users of
plumbing commands are expected to know what they are doing, so the
level of safety we need to give them can be much lower than Porcelain
commands such as 'git notes'.  

But when you stick to Porcelain commands, it is very hard to mix
refs/notes/* with refs/heads/* and refs/remotes/* by mistake.  You
have to really work on it by doing something unusual to have a non
commit in refs/heads/*, a commit in refs/notes/*, or a regular
non-note commit in refs/notes/*.

Once you lift the existing restriction, that easy safety goes away,
so the burden of giving a reasonable safety in some other way falls
on the one who is dropping refs/notes restriction.

That is exactly what I meant by that the existing safety pays price
of not being able to store notes outside refs/notes, which may be
too high a price to pay.

>> Although I am not fundamentally against allowing to store notes
>> outside refs/notes/, it is different from "anywhere is fine".
>> Can't we do this widening in a less damaging way?
>
> Without arbitrarily restricting where notes can be stored it seems to
> me the only option would be to have the notes machinery actually
> inspect the tree of any existing notes ref it's passed.

As I said earlier (assuming you read footnotes before you read a new
paragraph), the ship has already sailed.

Obvious two sensible ways forward are to do a blacklist (i.e. allow
anywhere except for known non-notes namespaces like refs/heads/) or
do a whitelist (i.e. allow refs/<some-known-space> in addition to
refs/notes) of the namespace, and the latter is vastly preferrable
than the former, because you can practically *never* crawl back a
namespace once you give it to the general public, even if you later
find it a grave error to open it too widely and need a more
controlled access [*2*].  And the name of the game for a software
that can be relied for a long haul is to avoid painting ourselves in
a corner that we cannot get out of.

If we add refs/remote-notes/* to the whitelist now, and if later it
turns out to be not so ideal and we would prefer another layout for
remotes, e.g. refs/remotesNew/*/{heads,notes,tags}/, we can add
refs/remotesNew/*/notes/ to the whitelist _without_ breaking those
who have already started using refs/remote-notes/*.  That is why
I said whitelist is preferrable than blacklist.


[Footnote]

*1* I actually do not think a tree examination would help very much
    here.  IIRC, somebody decided long time ago that it would be a
    good idea to be able to store a path that is not a fanned-out
    40-hex in a notes tree and 'git notes merge' would accept such a
    notes-tree.  Although I doubt that the resulting notes-tree
    produced by 'notes merge' is carefully designed one (as opposed
    to whatever the implementation happens to do) with sensible
    semantics, people may already be relying on it.

*2* The above 'notes-tree can store non fanned-out 40-hex' is a good
    example why you need to start strict and loosen only when it
    becomes necessary.  Despite that even Git itself does not use
    that "facility" to do anything useful AFAIK, only because we
    started with a loose variant that allows arbitrary garbage, we
    cannot retroactively tighten the definition of what a notes-tree
    should look like without risking to break practice of other
    people.

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 10:25, Junio C Hamano wrote:

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> So despite the name of the test, the actual tree contents do not seem
>> to be examined.
>
> Yes, but the thing is, thanks to refs/notes restriction, there is no
> need to do such examination [*1*].
>
> Note that it is an entirely different matter when you deliberately
> violate the expectation using plumbing (e.g. update-ref).  Users of
> plumbing commands are expected to know what they are doing, so the
> level of safety we need to give them can be much lower than Porcelain
> commands such as 'git notes'.
>
> But when you stick to Porcelain commands, it is very hard to mix
> refs/notes/* with refs/heads/* and refs/remotes/* by mistake.  You
> have to really work on it by doing something unusual to have a non
> commit in refs/heads/*, a commit in refs/notes/*, or a regular
> non-note commit in refs/notes/*.

Perhaps that is the crux of the issue.  There is no git notes-plumbing  
command where the git notes command continues to apply restrictions  
but the vaporware notes-plumbing command allows anything just like  
update-ref does.

I think there are two issues here:

1) There's no simple way to store remote notes refs outside the refs/ 
notes namespace in such a way that they can be used with git notes  
commands.

2) People who want to experiment with using git notes storage as a  
basis for building some new feature are forced to store their refs  
under the refs/notes namespace even if that does not make sense for  
the feature when what's stored in the notes tree is not intended to  
provide any content that is directly meaningful to the user.

> That is exactly what I meant by that the existing safety pays price
> of not being able to store notes outside refs/notes, which may be
> too high a price to pay.
>
>>> Although I am not fundamentally against allowing to store notes
>>> outside refs/notes/, it is different from "anywhere is fine".
>>> Can't we do this widening in a less damaging way?
>>
>> Without arbitrarily restricting where notes can be stored it seems to
>> me the only option would be to have the notes machinery actually
>> inspect the tree of any existing notes ref it's passed.
>
> As I said earlier (assuming you read footnotes before you read a new
> paragraph), the ship has already sailed.

Hmpf.  So the only possible safety check is refname-based.  But, as  
you say, it's no use crying over spilled milk [3].

> Obvious two sensible ways forward are to do a blacklist (i.e. allow
> anywhere except for known non-notes namespaces like refs/heads/) or
> do a whitelist (i.e. allow refs/<some-known-space> in addition to
> refs/notes) of the namespace, and the latter is vastly preferrable
> than the former, because you can practically *never* crawl back a
> namespace once you give it to the general public, even if you later
> find it a grave error to open it too widely and need a more
> controlled access [*2*].  And the name of the game for a software
> that can be relied for a long haul is to avoid painting ourselves in
> a corner that we cannot get out of.
>
> If we add refs/remote-notes/* to the whitelist now, and if later it
> turns out to be not so ideal and we would prefer another layout for
> remotes, e.g. refs/remotesNew/*/{heads,notes,tags}/, we can add
> refs/remotesNew/*/notes/ to the whitelist _without_ breaking those
> who have already started using refs/remote-notes/*.  That is why
> I said whitelist is preferrable than blacklist.

A whitelist solves issue (1) but is no help for issue (2) unless some  
additional additional part of the refs namespace were to be also  
whitelisted.  Perhaps something like refs/x-<anything>/... in the same  
vein as the various IETF standards for experimental names.

> [Footnote]
>
> *1* I actually do not think a tree examination would help very much
>    here.  IIRC, somebody decided long time ago that it would be a
>    good idea to be able to store a path that is not a fanned-out
>    40-hex in a notes tree and 'git notes merge' would accept such a
>    notes-tree.  Although I doubt that the resulting notes-tree
>    produced by 'notes merge' is carefully designed one (as opposed
>    to whatever the implementation happens to do) with sensible
>    semantics, people may already be relying on it.
>
> *2* The above 'notes-tree can store non fanned-out 40-hex' is a good
>    example why you need to start strict and loosen only when it
>    becomes necessary.  Despite that even Git itself does not use
>    that "facility" to do anything useful AFAIK, only because we
>    started with a loose variant that allows arbitrary garbage, we
>    cannot retroactively tighten the definition of what a notes-tree
>    should look like without risking to break practice of other
>    people.


[3] http://cheezburger.com/6423972864

-Kyle

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> A whitelist solves issue (1) but is no help for issue (2) unless some
> additional additional part of the refs namespace were to be also
> whitelisted.  Perhaps something like refs/x-<anything>/... in the same
> vein as the various IETF standards for experimental names.

Your (2) is about people who are _experimenting_, no?

Why can't they use refs/notes/x-<anything>/* while doing so, and
when that matures and proves useful to wider public we can make it
more official by whitelisting refs/<that-thing> in addition to
refs/notes/, and the problem is solved, no?

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Johan Herland]

On Wed, Jan 7, 2015 at 12:29 AM, Kyle J. McKay <mackyle@gmail.com> wrote:
> On Jan 6, 2015, at 10:25, Junio C Hamano wrote:
>> "Kyle J. McKay" <mackyle@gmail.com> writes:
>>> So despite the name of the test, the actual tree contents do not seem
>>> to be examined.
>>
>> Yes, but the thing is, thanks to refs/notes restriction, there is no
>> need to do such examination [*1*].
>>
>> Note that it is an entirely different matter when you deliberately
>> violate the expectation using plumbing (e.g. update-ref).  Users of
>> plumbing commands are expected to know what they are doing, so the
>> level of safety we need to give them can be much lower than Porcelain
>> commands such as 'git notes'.
>>
>> But when you stick to Porcelain commands, it is very hard to mix
>> refs/notes/* with refs/heads/* and refs/remotes/* by mistake.  You
>> have to really work on it by doing something unusual to have a non
>> commit in refs/heads/*, a commit in refs/notes/*, or a regular
>> non-note commit in refs/notes/*.

Just injecting my own (probably faulty) memory here: I agree with Junio's
analysis (including his footnote *1*). We figured that it was not possible
to outright forbid non-notes in notes trees (a sufficiently determined user
can always use plumbing commands to create arbitrarily non-well-formed
"notes trees"), so instead we implemented the notes plumbing
(notes.{h,c}) to deal with non-notes somewhat sanely instead (ignoring
them in notes queries, and preserving them across notes manipulations).

As a consequence, the safety features were pushed up into the porcelain
layer (builtin/notes.{h,c} and builtin/notes-merge.{h,c}), where they apply
to ref names instead of actual notes tree contents.

> Perhaps that is the crux of the issue.  There is no git notes-plumbing
> command where the git notes command continues to apply restrictions but the
> vaporware notes-plumbing command allows anything just like update-ref does.

Good observation. From my POV, as people start creating tools that use
notes in a more structured manner (than as free-form plain-text appendages
to commit messages), the sharp and pointy bits (and bugs) of the interface
come into view. This applies to the safety features being discussed in this
thread, but also IMHO to the other things being currently discussed/fixed
in the notes code:

 - adding empty notes
 - adding notes to non-commits in fast-import
 - suboptimal fanout when mixing fast-import
   and git-notes manipulations of notes trees

So far I've approached some of these issues by making/suggesting fairly
modest fixes/extensions to the notes porcelain, to make it more friendly
to these users. I haven't considered that we might at some point be
better off splitting out a separate plumbing command for manipulating
notes. However, I'm not sure we're there, yet. The problems raised so
far do not IMHO warrant the creation of a whole new plumbing command.
Instead, we can still keep fixing and improving 'git notes'.

> I think there are two issues here:
>
> 1) There's no simple way to store remote notes refs outside the refs/notes
> namespace in such a way that they can be used with git notes commands.
>
> 2) People who want to experiment with using git notes storage as a basis for
> building some new feature are forced to store their refs under the
> refs/notes namespace even if that does not make sense for the feature when
> what's stored in the notes tree is not intended to provide any content that
> is directly meaningful to the user.

Agreed.

>> That is exactly what I meant by that the existing safety pays price
>> of not being able to store notes outside refs/notes, which may be
>> too high a price to pay.
>>
>>>> Although I am not fundamentally against allowing to store notes
>>>> outside refs/notes/, it is different from "anywhere is fine".
>>>> Can't we do this widening in a less damaging way?
>>>
>>>
>>> Without arbitrarily restricting where notes can be stored it seems to
>>> me the only option would be to have the notes machinery actually
>>> inspect the tree of any existing notes ref it's passed.
>>
>>
>> As I said earlier (assuming you read footnotes before you read a new
>> paragraph), the ship has already sailed.
>
>
> Hmpf.  So the only possible safety check is refname-based.  But, as you say,
> it's no use crying over spilled milk [3].
>
>> Obvious two sensible ways forward are to do a blacklist (i.e. allow
>> anywhere except for known non-notes namespaces like refs/heads/) or
>> do a whitelist (i.e. allow refs/<some-known-space> in addition to
>> refs/notes) of the namespace, and the latter is vastly preferrable
>> than the former, because you can practically *never* crawl back a
>> namespace once you give it to the general public, even if you later
>> find it a grave error to open it too widely and need a more
>> controlled access [*2*].  And the name of the game for a software
>> that can be relied for a long haul is to avoid painting ourselves in
>> a corner that we cannot get out of.
>>
>> If we add refs/remote-notes/* to the whitelist now, and if later it
>> turns out to be not so ideal and we would prefer another layout for
>> remotes, e.g. refs/remotesNew/*/{heads,notes,tags}/, we can add
>> refs/remotesNew/*/notes/ to the whitelist _without_ breaking those
>> who have already started using refs/remote-notes/*.  That is why
>> I said whitelist is preferrable than blacklist.

I agree that a whitelist is preferable to a blacklist.

> A whitelist solves issue (1) but is no help for issue (2) unless some
> additional additional part of the refs namespace were to be also
> whitelisted.  Perhaps something like refs/x-<anything>/... in the same vein
> as the various IETF standards for experimental names.

Alternatively (or additionally), for issue (2), we could add a
--disable-ref-safety option to 'git notes', to explicitly disable the
safety checks for "experimental" use.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Johan Herland]

(This addresses some smaller specific questions from Kyle, and is not an
attempt to take focus away from the main direction of this thread)

On Jan 6, 2015 1:27 PM, "Kyle J. McKay" <mackyle@gmail.com> wrote:
> On Jan 6, 2015, at 02:20, Junio C Hamano wrote:

[...]

> At the point the dropped line runs, core.notesRef has been set to
> refs/notes/y which does not exist.

Merging into an empty or non-existing notes ref is not in itself an error.

> All of the tests in the 'fail to merge various non-note-trees' section
> fail with one of these errors:
>
>   1) Failed to resolve remote notes ref '<ref-being-tested>'
>
>   2) Cannot merge empty notes ref (<ref-being-tested>) into empty notes
>      ref (refs/notes/y)
>
>   3) error: object 6c99d48c... is a tree, not a commit
>
> The 3rd error comes from the "git notes merge x:" attempt.

Yes. We need an actual commit object which can become the second patent of
the notes merge.

> So despite the name of the test, the actual tree contents do not seem to
> be examined.

Correct. The test name should probably be improved. I have a bad habit of
using the term "notes tree" to refer to generic collection of notes, sometimes
meaning the notes ref, sometimes the commit referenced by a notes ref, and
sometimes the actual tree object referenced by the notes commit (or directly
referenced by the notes ref). In this case I guess a better name for the test
would be: "fail to merge various things that are not notes commits"

> When the notes ref checking is relaxed to leave refs/heads/master alone
> rather than turning it into refs/notes/refs/heads/master, the previous
> error (#2 in this case) goes away and since refs/notes/y does not exist,
> it is simply updated to the value of refs/heads/master without any checks.

This is the expected behaviour of merging an otherwise valid (notes) ref.

> Of course that refs/heads/master tree doesn't look like a notes tree.
>
> And if we do this:
>
>   git update-ref refs/notes/refs/heads/master master

(loading the gun and pointing it at your foot...)

> Then "git notes merge refs/heads/master" also succeeds without
> complaining that the refs/notes/refs/heads/master tree does not look
> like a notes tree and we didn't need to relax the refs/notes restriction
> and, as you point out, the name of the test seems to imply that would be
> rejected.

In this case, the test name is clearly misleading. As discussed elsewhere
in the thread, we cannot absolutely enforce a distinction between a "notes"
tree object (i.e. a git tree that contain ONLY notes entries) and a
"non-notes" tree object. There is only a strong convention/expectation that
a notes tree object contains only (or at least mostly) SHA1-named entries
(possibly with fanout).

That said, if you stick to the porcelain, you should not be able to create
a notes ref referencing a tree object that contains anything but notes.

> Interestingly, if we then attempt to merge refs/notes/x into this
> non-notes refs/notes/y tree, it also succeeds and even keeps the things
> that do not look like notes.  The reverse merge (y into x) succeeds as
> well, but the non-notes stuff in y is not merged in in that case.

The general policy for handling non-notes in notes code, is to ignore them
when querying, and preserve them when manipulating (t/t3304-notes-mixed.sh
has examples). In this case, we preserve the non-notes from the primary
parent (the notes tree from which the merge was started), and we ignore
any non-notes coming from subsequent parents. I can't remember if this
behaviour was explicitly desired, or if it's just an accident of
implementation. However, I will argue that it doesn't make sense for
notes-merge to attempt any meaningful merge of non-notes. That is simply
outside its scope.

>> Arguably, not being able to save notes tree anywhere outside of
>> refs/notes/ hierarchy may be too high a price to pay in order to
>> prevent refs/heads/master from being considered (hence to avoid such
>> end-user mistakes), but at the same time, losing this safetly may
>> also be too high a price to pay in order to allow people to store
>> their notes in somewhere outside e.g. refs/remote-notes/origin/foo.
>> "Somewhere outside" does not mean "Including other hierarchies like
>> refs/heads and refs/tags that have long established meaning".
>
> If we relax the refs/notes restriction, putting a notes ref in
> refs/heads/<whatever> doesn't necessarily seem like that's a terrible
> thing as long as it's really a notes tree if used with the notes
> machinery.  AIUI, the refs/heads convention only requires the ref to
> point to the tip of a commit chain which all of the refs under
> refs/notes satisfy.

At some point I remember having a discussion on whether we should
support notes refs pointing directly to a tree objects (to implement
"history-less" notes - sometimes the notes history itself is
uninteresting, if you're only interested in the current state of the
notes tree). However, I can't immediately find any tests exercising
this functionality, so it's likely unsupported (the convention here is
instead to use a commit object, but make sure to prepare the next notes
commit with zero parents; see notes-cache.{h,c} for an example of this.

> The refs/heads convention AIUI does not impose any requirement about
> the contents of the tree(s) those commits in the chain refer to.
> But at the same time I can't think of any particular reason I'd want
> to store notes refs in there either.
>
>> Although I am not fundamentally against allowing to store notes
>> outside refs/notes/, it is different from "anywhere is fine".
>> Can't we do this widening in a less damaging way?
>
> Without arbitrarily restricting where notes can be stored it seems to
> me the only option would be to have the notes machinery actually
> inspect the tree of any existing notes ref it's passed.  That would
> also catch the case where
> "git update-ref refs/notes/refs/heads/master master" was run as well.
> It also seems like a good check to have in place to help catch user
> errors.
>
> I'm not all that familiar with the notes code, perhaps there's already
> a function that does the tree check to make sure the tree actually
> looks like a notes tree that can easily be called?  Maybe Johan has
> some thoughts on this?

Nope, no such check. However, it's worth considering giving the user a
warning (maybe even - optionally - an error) when non-notes are found
in a notes tree. Search for non_note in notes.c to find relevant code.


Have fun! :)

...Johan

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Jeff King]

On Tue, Jan 06, 2015 at 02:20:33AM -0800, Junio C Hamano wrote:

> The fact that "git notes merge refs/heads/master" fails is a very
> good prevention of end-user mistakes, and this removal of test
> demonstrates that we are dropping a valuable safety.

Is it really that valuable? If it were:

  git notes merge master

I could see somebody running that accidentally. But we are talking about
somebody who is already fully-qualifying a ref (and anything unqualified
continues to get looked up under refs/notes). Do people really go to the
length of qualifying the ref and then get confused or upset that git did
exactly what they asked it to do?

I'm worried that the end-user safety here is really a strawman, and we
are making this more complicated than it needs to be.

-Peff

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 15:54, Junio C Hamano wrote:

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> A whitelist solves issue (1) but is no help for issue (2) unless some
>> additional additional part of the refs namespace were to be also
>> whitelisted.  Perhaps something like refs/x-<anything>/... in the  
>> same
>> vein as the various IETF standards for experimental names.
>
> Your (2) is about people who are _experimenting_, no?
>
> Why can't they use refs/notes/x-<anything>/* while doing so, and
> when that matures and proves useful to wider public we can make it
> more official by whitelisting refs/<that-thing> in addition to
> refs/notes/, and the problem is solved, no?

I supposed if that's the best that can be done.  The problem is then  
that if you set notes.displayRef to refs/notes/* to see all relevant  
notes, don't you end up seeing these experimental, not-meant-for-end- 
user-consumption notes if they are somewhere under refs/notes/ ?  My  
glob specs are a little fuzzy, maybe it's refs/notes/** that gets  
everything under it?

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Kyle J. McKay]

On Jan 6, 2015, at 16:28, Johan Herland wrote:

> On Wed, Jan 7, 2015 at 12:29 AM, Kyle J. McKay <mackyle@gmail.com>  
> wrote:
>> Perhaps that is the crux of the issue.  There is no git notes- 
>> plumbing
>> command where the git notes command continues to apply restrictions  
>> but the
>> vaporware notes-plumbing command allows anything just like update- 
>> ref does.
>
> Good observation. From my POV, as people start creating tools that use
> notes in a more structured manner (than as free-form plain-text  
> appendages
> to commit messages), the sharp and pointy bits (and bugs) of the  
> interface
> come into view. This applies to the safety features being discussed  
> in this
> thread, but also IMHO to the other things being currently discussed/ 
> fixed
> in the notes code:
> [...]
> I haven't considered that we might at some point be
> better off splitting out a separate plumbing command for manipulating
> notes. However, I'm not sure we're there, yet. The problems raised so
> far do not IMHO warrant the creation of a whole new plumbing command.
> Instead, we can still keep fixing and improving 'git notes'.
>
>> 1) There's no simple way to store remote notes refs outside the  
>> refs/notes
>> namespace in such a way that they can be used with git notes  
>> commands.
>>
>> 2) People who want to experiment with using git notes storage as a  
>> basis for
>> building some new feature are forced to store their refs under the
>> refs/notes namespace even if that does not make sense for the  
>> feature when
>> what's stored in the notes tree is not intended to provide any  
>> content that
>> is directly meaningful to the user.
>
>> A whitelist solves issue (1) but is no help for issue (2) unless some
>> additional additional part of the refs namespace were to be also
>> whitelisted.  Perhaps something like refs/x-<anything>/... in the  
>> same vein
>> as the various IETF standards for experimental names.
>
> Alternatively (or additionally), for issue (2), we could add a
> --disable-ref-safety option to 'git notes', to explicitly disable the
> safety checks for "experimental" use.

I like that.  Sort of a poor-man's git notes-plumbing option.  And  
there is precedence for this sort of thing as recently (v2.2.0) git  
hash-object learned a "--literally" option to bypass checks it would  
otherwise normally perform.

-Kyle

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jan 06, 2015 at 02:20:33AM -0800, Junio C Hamano wrote:
>
>> The fact that "git notes merge refs/heads/master" fails is a very
>> good prevention of end-user mistakes, and this removal of test
>> demonstrates that we are dropping a valuable safety.
>
> Is it really that valuable? If it were:
>
>   git notes merge master
>
> I could see somebody running that accidentally.
> ...
> But we are talking about
> somebody who is already fully-qualifying a ref (and anything unqualified
> continues to get looked up under refs/notes).

That (specifically 'merge') is not my real worry.  It's the other
way around, actually.

Because expand_notes_ref() makes sure that any given notes ref is
prefixed appropriately to start with refs/notes/,

    git notes --ref=refs/heads/master add ...blah...
    git notes --ref=refs/tag/v1.0 add ...blah...

would be a sensible way when somebody wants to keep a forest of
notes refs, one per real ref.  Wouldn't they have already been
trained to spell "refs/heads/master" when they want to refer to
refs/notes/refs/heads/master because of this?

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Alternatively (or additionally), for issue (2), we could add a
> --disable-ref-safety option to 'git notes', to explicitly disable the
> safety checks for "experimental" use.

I actually would rather prefer to see a proper plumbing use
supported, either by a new "git notes-plumb" subcommand or "git
notes --<some option>" that gives the scriptors a stable interface
to the low-level machinery without enforcing any Policy that belongs
to the Porcelain layer.  At the very least, the plumbing should:

 - disable anything that introduces potential ambiguity, e.g. DWIMs
   done by expand_notes_ref(), but not limited to it.

 - lift any restrictions based on policy, e.g. "where can notes
   trees live" (again, but not limited to this).

Re: [PATCH v2 2/2] t/t3308-notes-merge.sh: succeed with relaxed notes refs

[Jeff King]

On Wed, Jan 07, 2015 at 08:03:58AM -0800, Junio C Hamano wrote:

> > But we are talking about
> > somebody who is already fully-qualifying a ref (and anything unqualified
> > continues to get looked up under refs/notes).
> 
> That (specifically 'merge') is not my real worry.  It's the other
> way around, actually.
> 
> Because expand_notes_ref() makes sure that any given notes ref is
> prefixed appropriately to start with refs/notes/,
> 
>     git notes --ref=refs/heads/master add ...blah...
>     git notes --ref=refs/tag/v1.0 add ...blah...
> 
> would be a sensible way when somebody wants to keep a forest of
> notes refs, one per real ref.  Wouldn't they have already been
> trained to spell "refs/heads/master" when they want to refer to
> refs/notes/refs/heads/master because of this?

Thanks, that is a more interesting case, and I agree that moving to
allowing fully-qualified refs would technically be a regression.  I'm
still slightly doubtful that this is something people do in practice,
but I guess we have no way to know for sure.

-Peff