On Fri, Sep 19, 2014 at 11:39 AM, Jeff King <[email protected]> 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, <[email protected]>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
