[PATCH] notes: accept any ref for merge

[PATCH] notes: accept any ref for merge

[Scott Chacon]

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 5fe691d..78d58af 100644
--- a/notes.c
+++ b/notes.c
@@ -1293,7 +1293,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.0.0

Re: [PATCH] notes: accept any ref for merge

[Jeff King]

On Fri, Sep 19, 2014 at 09:39:45AM +0200, Scott Chacon wrote:

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

I think this change affects not just "git notes merge", but all of the
notes lookups (including just "git notes show"). However, I'd argue
that's a good thing, as it allows more flexibility in note storage. The
downside is that if you have a notes ref like
"refs/notes/refs/heads/master", you can no longer refer to it as
"refs/heads/master" (you have to use the fully qualified name to get the
note). But:

  1. This makes the notes resolution a lot more like regular ref
     resolution (i.e., we now allow fully qualified refs, and you can
     store remote notes outside of refs/notes if you want to).

  2. There are already a bunch of names that have the same problem. You
     cannot refer to "refs/notes/notes/foo" as "notes/foo", nor
     "refs/notes/refs/notes/foo" as "refs/notes/foo". Yes, these are
     silly names, so is the example above.

So it's backwards incompatible with the current behavior, but I think in
a good way.

> ---
>  notes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I think you need to adjust t3308 (and you should probably add a new test
exercising your case; this is exactly the sort of thing that it's easy
to accidentally regress later).

-Peff

Re: [PATCH] notes: accept any ref for merge

[Johan Herland]

On Fri, Sep 19, 2014 at 11:39 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Sep 19, 2014 at 09:39:45AM +0200, Scott Chacon wrote:
>> 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.
>
> I think this change affects not just "git notes merge", but all of the
> notes lookups (including just "git notes show"). However, I'd argue
> that's a good thing, as it allows more flexibility in note storage. The
> downside is that if you have a notes ref like
> "refs/notes/refs/heads/master", you can no longer refer to it as
> "refs/heads/master" (you have to use the fully qualified name to get the
> note). But:
>
>   1. This makes the notes resolution a lot more like regular ref
>      resolution (i.e., we now allow fully qualified refs, and you can
>      store remote notes outside of refs/notes if you want to).
>
>   2. There are already a bunch of names that have the same problem. You
>      cannot refer to "refs/notes/notes/foo" as "notes/foo", nor
>      "refs/notes/refs/notes/foo" as "refs/notes/foo". Yes, these are
>      silly names, so is the example above.
>
> So it's backwards incompatible with the current behavior, but I think in
> a good way.

FWIW, I agree with this analysis.

>> ---
>>  notes.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I think you need to adjust t3308 (and you should probably add a new test
> exercising your case; this is exactly the sort of thing that it's easy
> to accidentally regress later).

Agree here as well.

AFAICS, the only diff you'll need to make the test suite pass is this:

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

Additionally, I suggest adding another test demonstrating your use
case as well. Something like setting up a small scenario for notes
collaboration, and walking through the various steps:

 - Creating a couple of repos where notes are added/edited
 - Setting up config to allow pushing and/or fetching notes
 - Performing the push/fetch
 - Merging with the corresponding local notes ref

Have fun! :)

...Johan

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

Re: [PATCH] notes: accept any ref for merge

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Fri, Sep 19, 2014 at 11:39 AM, Jeff King <peff@peff.net> wrote:
>> On Fri, Sep 19, 2014 at 09:39:45AM +0200, Scott Chacon wrote:
>>> 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.
>>
>> I think this change affects not just "git notes merge", but all of the
>> notes lookups (including just "git notes show")....
> ...
> Additionally, I suggest adding another test demonstrating your use
> case as well. Something like setting up a small scenario for notes
> collaboration, and walking through the various steps:
>
>  - Creating a couple of repos where notes are added/edited
>  - Setting up config to allow pushing and/or fetching notes
>  - Performing the push/fetch
>  - Merging with the corresponding local notes ref

Is it our future direction to set up refs/remote-notes/<remote>/
namespace?  If so, let's not do it piecemeail in an unorganized
guerrilla fashion by starting with a stealth enabler with an
associated test.  We risk not following through and leave the
resulting user experience more puzzling if we go that way.

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.

The most important first step for that to happen is to make sure we
are on the same page on that future direction.  I personally think
refs/remote-notes/<remote> that runs parallel to the remote tracking
branch hierarchy refs/remotes/<remote> is a reasonable way to do
this, but my words are no way final.

Assuming that this is we all agree to go in that direction, let's
make a list of things to be done to codify it, and do them.  For a
starter, I think these are needed, perhaps?

 - This patch (or an enhancement to keep some safety)

 - Documentation updates to "git notes"

 - Documentation updates to Documentation/gitrepository-layout.txt

 - Update to "git clone" and "git remote add" to add a fetch refspec
   refs/notes:refs/remote-refs/<remote>/*

 - New tests you suggest

Re: [PATCH] notes: accept any ref for merge

[Johan Herland]

On Fri, Sep 19, 2014 at 8:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> On Fri, Sep 19, 2014 at 11:39 AM, Jeff King <peff@peff.net> wrote:
>>> On Fri, Sep 19, 2014 at 09:39:45AM +0200, Scott Chacon wrote:
>>>> 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.
>>>
>>> I think this change affects not just "git notes merge", but all of the
>>> notes lookups (including just "git notes show")....
>> ...
>> Additionally, I suggest adding another test demonstrating your use
>> case as well. Something like setting up a small scenario for notes
>> collaboration, and walking through the various steps:
>>
>>  - Creating a couple of repos where notes are added/edited
>>  - Setting up config to allow pushing and/or fetching notes
>>  - Performing the push/fetch
>>  - Merging with the corresponding local notes ref
>
> Is it our future direction to set up refs/remote-notes/<remote>/
> namespace?  If so, let's not do it piecemeail in an unorganized
> guerrilla fashion by starting with a stealth enabler with an
> associated test.  We risk not following through and leave the
> resulting user experience more puzzling if we go that way.
>
> 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.
>
> The most important first step for that to happen is to make sure we
> are on the same page on that future direction.  I personally think
> refs/remote-notes/<remote> that runs parallel to the remote tracking
> branch hierarchy refs/remotes/<remote> is a reasonable way to do
> this, but my words are no way final.

This has been discussed several times in the past, and - as I have
argued before - I believe Git would benefit from a more thorough
revamp of the ref namespace, one that would allow a straightforward
naming of _any_ kind of remote-tracking ref (heads, tags, notes,
whatever). The scheme I have proposed would map refs/<kind>/<name>
from a remote repo to a remote-tracking
refs/remotes/<remote>/<kind>/<name> in the local repo.

Having said that, I have clearly failed to find the time and
motivation to follow through on this topic, and although there was
some support for the idea, nobody else has stepped up to tackle it.
Unfortunately, that has left "git notes" in a sorry state when it
comes to sharing and collaboration. This has to stop. Fixing notes
sharing is much more important than whatever lofty ideas I might
have about how things should "ideally" be organized.

Therefore, you can count me in support of organizing remote-tracking
notes refs under refs/remote-notes/<remote>/<name>. In case of a more
thorough redesign of the ref namespace at some point in the future,
we will have to deal with a lot of "legacy" anyway, and adding
refs/remote-notes/<remote> will not considerably increase that
burden.

> Assuming that this is we all agree to go in that direction, let's
> make a list of things to be done to codify it, and do them.  For a
> starter, I think these are needed, perhaps?
>
>  - This patch (or an enhancement to keep some safety)
>
>  - Documentation updates to "git notes"
>
>  - Documentation updates to Documentation/gitrepository-layout.txt
>
>  - Update to "git clone" and "git remote add" to add a fetch refspec
>    refs/notes:refs/remote-refs/<remote>/*
>
>  - New tests you suggest

Sounds good to me. At least that would get us to the point where a
simple "git fetch" will also fetch notes updates, and you can then
choose to "git notes merge" them into your corresponding local notes
refs.

In addition to that we might want to consider streamlining things
further by having a single command (like "git pull") that performs
both fetching and merging. A complication here is that - unlike the
branch realm where HEAD points to our "current" branch - there is
not really a concept of a "current" notes ref, which could specify
_which_ remote-notes ref to merge and/or the parameters of that
merge. However, (as usual) I'm getting ahead of myself here. The
points you list above go more than halfway to making notes sharing
straightforward, and are in any case necessary prerequisites for
whatever might follow.


...Johan

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

Re: [PATCH] notes: accept any ref for merge

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> Assuming that this is we all agree to go in that direction, let's
>> make a list of things to be done to codify it, and do them.  For a
>> starter, I think these are needed, perhaps?
>> ...
> Sounds good to me. At least that would ...
> ...
> In addition to that we might want to consider ...

Yes, I specifically meant my list as "a starter", not wanting to
make an exhaustive list myself.

Re: [PATCH] notes: accept any ref for merge

[Kyle J. McKay]

I see this patch has not been picked up.

I would like to lobby for inclusion of this patch.

On Sep 19, 2014, at 11:22, Junio C Hamano wrote:

> Johan Herland <johan@herland.net> writes:
>
>> On Fri, Sep 19, 2014 at 11:39 AM, Jeff King <peff@peff.net> wrote:
>>> On Fri, Sep 19, 2014 at 09:39:45AM +0200, Scott Chacon wrote:
>>>> 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.
>>>
>>> I think this change affects not just "git notes merge", but all of  
>>> the
>>> notes lookups (including just "git notes show")....
>> ...
>
> Is it our future direction to set up refs/remote-notes/<remote>/
> namespace?

When cloning (without --mirror) Git now sets up a fetch spec like:

   +refs/heads/*:refs/remotes/origin/*

It's unfortunate that it does not preserve the notion of "heads" and  
instead set it up like this:

   +refs/heads/*:refs/remotes/origin/heads/*

In which case it would make more sense to then simply clone notes like  
so:

   +refs/notes/*:refs/remotes/origin/notes/*

That would also clear the way to always fetching all remote tags into  
refs/remotes/<remote>/tags/* by default as well even if the local refs/ 
tags/* do not end up being updated.

It seems clumsy to me to use a new remotes-notes ref namespace.  What  
happens if Git grows the ability to store some kind of (bug) tracking  
ticket in refs/tickets in the future?  Does Git then use refs/remote- 
tickets/<remote> to store the remote refs rather than simply refs/ 
remotes/<remote>/tickets?

There are a number of applications that create refs outside of refs/ 
heads/* and refs/tags/*.  GitHub uses refs/pull/*, should Git have a  
refs/remote-pull/<remote>/* namespace and for Gerrit refs/remote- 
changes/<remote>/* and also refs/remote-cache-automerge/<remote>/*  
(for refs/cache-automerge/*)?

> If so, let's not do it piecemeail in an unorganized
> guerrilla fashion by starting with a stealth enabler
>
> 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/*).

Already that's not true if you use an ugly symbolic-ref workaround,  
but that requires polluting your ref namespace.

With all the other Git "restrictions" they are almost always strong  
guidance, not brutally enforced.

Take, for example, the "restriction" that HEAD should be either  
detached or a symbolic ref to refs/heads/<something>.

It's not absolutely enforced.  If you really want to, you can use git  
symbolic-ref and set HEAD to somewhere else (even under refs/taga) --  
and it mostly works -- `git branch` gets upset but you can commit new  
changes, view the log, etc.

How about the "guidance" that pushing does not update tags even if the  
change would be a fast-forward?  Again, not enforced, use the -f  
option or add an explicit refspec to the appropriate remote config.

What about the restriction that `git config --get user.name` cannot  
end in "."?  (It gets magically stripped off.)  That's a toughie, but  
if you really, really, really want to you can always `git cat-file  
commit HEAD > temp`, add the trailing dot and then git update-ref HEAD  
$(git hash-object -t commit -w temp)`.  Not recommended but possible.

So anyway, my point is that arbitrarily forcefully restricting the  
operation of the various notes commands to refs/notes/* does not seem  
consistent with the way everything else works.

> The most important first step for that to happen is to make sure we
> are on the same page on that future direction.  I personally think
> refs/remote-notes/<remote> that runs parallel to the remote tracking
> branch hierarchy refs/remotes/<remote> is a reasonable way to do
> this, but my words are no way final.

I'd prefer refs/remotes/<remote>/notes for the reasons stated above.   
Having a refs/remote-notes/<remote>/* hierarchy opens the door to a  
proliferation of refs/remote-<whatever>/<remote>/* items in the refs  
namespace in the future.

So in the vein of providing guidance to the user but, in the end,  
allowing the user to remain in control, I have gussied up Johan's  
suggested fix for the failing notes test into the following patch.

--Kyle

-- 8< --
Subject: [PATCH] t/t3308-notes-merge.sh: succeed with relaxed notes refs

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

Re: [PATCH] notes: accept any ref for merge

[Jeff King]

On Sat, Nov 22, 2014 at 10:04:57AM -0800, Kyle J. McKay 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.

The original patch discussion sort of fizzled, and your response here
largely slipped through the cracks. I am not sure everyone even
remembers the exact patch under discussion. Maybe a better way to
re-kickstart the discussion is to repost the patch along with a synopsis
of the previous discussion and your arguments about moving it forward.

-Peff

Re: [PATCH] notes: accept any ref for merge

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think this change affects not just "git notes merge", but all of the
> notes lookups (including just "git notes show"). However, I'd argue
> that's a good thing, as it allows more flexibility in note storage. The
> downside is that if you have a notes ref like
> "refs/notes/refs/heads/master", you can no longer refer to it as
> "refs/heads/master" (you have to use the fully qualified name to get the
> note). But:
>
>   1. This makes the notes resolution a lot more like regular ref
>      resolution (i.e., we now allow fully qualified refs, and you can
>      store remote notes outside of refs/notes if you want to).
>
>   2. There are already a bunch of names that have the same problem. You
>      cannot refer to "refs/notes/notes/foo" as "notes/foo", nor
>      "refs/notes/refs/notes/foo" as "refs/notes/foo". Yes, these are
>      silly names, so is the example above.
>
> So it's backwards incompatible with the current behavior, but I think in
> a good way.

Yup, I agree with the analysis.

>> ---
>>  notes.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I think you need to adjust t3308 (and you should probably add a new test
> exercising your case; this is exactly the sort of thing that it's easy
> to accidentally regress later).
>
> -Peff