Re: [PATCH] notes: accept any ref for merge
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: accept any ref for merge
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 jo...@herland.net writes: On Fri, Sep 19, 2014 at 11:39 AM, Jeff King p...@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
Re: [PATCH] notes: accept any ref for merge
Johan Herland jo...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] notes: accept any ref for merge
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 scha...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: accept any ref for merge
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: accept any ref for merge
On Fri, Sep 19, 2014 at 11:39 AM, Jeff King p...@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, jo...@herland.net
www.herland.net
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: accept any ref for merge
Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: accept any ref for merge
Johan Herland jo...@herland.net writes: On Fri, Sep 19, 2014 at 11:39 AM, Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: accept any ref for merge
On Fri, Sep 19, 2014 at 8:22 PM, Junio C Hamano gits...@pobox.com wrote: Johan Herland jo...@herland.net writes: On Fri, Sep 19, 2014 at 11:39 AM, Jeff King p...@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,
