[PATCH] Remove restriction on notes ref base

[PATCH] Remove restriction on notes ref base

[Kenny Root]

Git notes were restricted to refs/notes/* in the command line utilities,
but setting things like GIT_NOTES_REF to something outside of that
structure would work.

This removes the restrictions on the git notes command line interface.

Signed-off-by: Kenny Root <kroot@google.com>
---
 builtin/notes.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 6d07aac..9acce7b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -343,11 +343,7 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
 	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
 		/* note that a refs/ prefix is implied in the
 		 * underlying for_each_glob_ref */
-		if (!prefixcmp(v, "refs/notes/"))
-			string_list_add_refs_by_glob(c->refs, v);
-		else
-			warning("Refusing to rewrite notes in %s"
-				" (outside of refs/notes/)", v);
+		string_list_add_refs_by_glob(c->refs, v);
 		return 0;
 	}
 
@@ -473,9 +469,6 @@ static struct notes_tree *init_notes_check(const char *subcommand)
 	init_notes(NULL, NULL, NULL, 0);
 	t = &default_notes_tree;
 
-	if (prefixcmp(t->ref, "refs/notes/"))
-		die("Refusing to %s notes in %s (outside of refs/notes/)",
-		    subcommand, t->ref);
 	return t;
 }
 
@@ -844,7 +837,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 
 	if (override_notes_ref) {
 		struct strbuf sb = STRBUF_INIT;
-		if (!prefixcmp(override_notes_ref, "refs/notes/"))
+		if (!prefixcmp(override_notes_ref, "refs/"))
 			/* we're happy */;
 		else if (!prefixcmp(override_notes_ref, "notes/"))
 			strbuf_addstr(&sb, "refs/");
-- 
1.7.3.1

Re: [PATCH] Remove restriction on notes ref base

[Jonathan Nieder]

(+cc: Johan, Thomas)

Kenny Root wrote:

> Git notes were restricted to refs/notes/* in the command line utilities,
> but setting things like GIT_NOTES_REF to something outside of that
> structure would work.
> 
> This removes the restrictions on the git notes command line interface.

Could you explain what those restrictions are (perhaps through an
example in the form of a patch to the t/ subdirectory)?

Cc-ing some people more knowledgeable about notes than I am; maybe
they can give more information about what this notes.rewriteref
protection and other check are about.

Jonathan

[patch follows for their convenience.]
> Signed-off-by: Kenny Root <kroot@google.com>
> ---
>  builtin/notes.c |   11 ++---------
>  1 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 6d07aac..9acce7b 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -343,11 +343,7 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
>  	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
>  		/* note that a refs/ prefix is implied in the
>  		 * underlying for_each_glob_ref */
> -		if (!prefixcmp(v, "refs/notes/"))
> -			string_list_add_refs_by_glob(c->refs, v);
> -		else
> -			warning("Refusing to rewrite notes in %s"
> -				" (outside of refs/notes/)", v);
> +		string_list_add_refs_by_glob(c->refs, v);
>  		return 0;
>  	}
>  
> @@ -473,9 +469,6 @@ static struct notes_tree *init_notes_check(const char *subcommand)
>  	init_notes(NULL, NULL, NULL, 0);
>  	t = &default_notes_tree;
>  
> -	if (prefixcmp(t->ref, "refs/notes/"))
> -		die("Refusing to %s notes in %s (outside of refs/notes/)",
> -		    subcommand, t->ref);
>  	return t;
>  }
>  
> @@ -844,7 +837,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
>  
>  	if (override_notes_ref) {
>  		struct strbuf sb = STRBUF_INIT;
> -		if (!prefixcmp(override_notes_ref, "refs/notes/"))
> +		if (!prefixcmp(override_notes_ref, "refs/"))
>  			/* we're happy */;
>  		else if (!prefixcmp(override_notes_ref, "notes/"))
>  			strbuf_addstr(&sb, "refs/");
> -- 

Re: [PATCH] Remove restriction on notes ref base

[Johan Herland]

On Tuesday 02 November 2010, Jonathan Nieder wrote:
> (+cc: Johan, Thomas)
> 
> Kenny Root wrote:
> > Git notes were restricted to refs/notes/* in the command line
> > utilities, but setting things like GIT_NOTES_REF to something outside
> > of that structure would work.
> > 
> > This removes the restrictions on the git notes command line interface.

Why do you need to set GIT_NOTES_REF to something outside refs/notes/ at 
all?

> Could you explain what those restrictions are (perhaps through an
> example in the form of a patch to the t/ subdirectory)?

Yes, please do. As it stands, I'm pretty sure the patch below breaks at 
least TCs t3301.4 and t3301.5.

> Cc-ing some people more knowledgeable about notes than I am; maybe
> they can give more information about what this notes.rewriteref
> protection and other check are about.

Well, I guess we originally decided to limit notes refs to within 
refs/notes/ in order to clearly separate notes from non-notes, and to 
prevent notes code from accidentally messing up non-notes refs.

Later, we have discovered that in order to work with remote notes refs, we 
probably have to lift this restriction, and allow (at least) notes refs 
somewhere within refs/remotes/, e.g. refs/remotes/*/notes/.

http://thread.gmane.org/gmane.comp.version-control.git/159469/focus=159703 
and its sub-thread contains more discussion on sharing notes with remote 
repos.

So, in conclusion, this patch is a move in the right direction, but in some 
respects it might be going a bit _too_ far: Maybe we should restrict notes 
to refs/notes/ and refs/remotes/*/notes/? ...or maybe I'm too paranoid, and 
allowing notes "everywhere" is ok. Then again, in other respects this patch 
isn't going far enough, since it doesn't deal with setting up the refspecs 
needed to make notes sharing easy and straighforward for end users.

> [patch follows for their convenience.]
> [...]
> > @@ -844,7 +837,7 @@ int cmd_notes(int argc, const char **argv, const
> > char *prefix)
> > 
> >  	if (override_notes_ref) {
> >  	
> >  		struct strbuf sb = STRBUF_INIT;
> > 
> > -		if (!prefixcmp(override_notes_ref, "refs/notes/"))
> > +		if (!prefixcmp(override_notes_ref, "refs/"))
> > 
> >  			/* we're happy */;
> >  		
> >  		else if (!prefixcmp(override_notes_ref, "notes/"))
> >  		
> >  			strbuf_addstr(&sb, "refs/");

FTR, this hunk will surely conflict with the introduction of 
expand_notes_ref() which is in patch 09/23 in the 'git notes merge' series 
in 'pu'.


...Johan

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

Re: [PATCH] Remove restriction on notes ref base

[Shawn Pearce]

On Tue, Nov 2, 2010 at 1:48 AM, Johan Herland <johan@herland.net> wrote:
> On Tuesday 02 November 2010, Jonathan Nieder wrote:
>> (+cc: Johan, Thomas)
>>
>> Kenny Root wrote:
>> > Git notes were restricted to refs/notes/* in the command line
>> > utilities, but setting things like GIT_NOTES_REF to something outside
>> > of that structure would work.
>> >
>> > This removes the restrictions on the git notes command line interface.
>
> Why do you need to set GIT_NOTES_REF to something outside refs/notes/ at
> all?

I wanted Kenny to create a notes branch called refs/meta/bad-commits,
and put that inside of one of our Gerrit Code Review server
repositories.  We want to extend Gerrit Code Review to check to see if
any commit in the incoming pack appears in the bad-commits with a
note.  If it does, it will reject the push.  This allows a repository
owner to ban certain commits from re-entering a repository once they
have done a filter-branch or rebase to rewrite a particular item out
of history.

I didn't want to use refs/notes/bad-commits because its not really an
annotation you would be looking at with git log.  But we do want to
have a log of who banned particular SHA-1s from entering the
repository, and being able to push that branch from a workstation to
the server is a convenient way to edit that list of banned SHA-1s.
During prototyping Kenny discovered you can't use `git note --ref
refs/meta/bad-commits`.  Which means a server administrator wouldn't
be able to edit the list directly in the repository.  Hence this
patch.

>> Cc-ing some people more knowledgeable about notes than I am; maybe
>> they can give more information about what this notes.rewriteref
>> protection and other check are about.
>
> Well, I guess we originally decided to limit notes refs to within
> refs/notes/ in order to clearly separate notes from non-notes, and to
> prevent notes code from accidentally messing up non-notes refs.

I guess I can see some logic in this.  But the documentation says
--ref accepts a qualified ref name, and then proceeds to assume that a
string that starts with "refs/" (like "refs/meta/bad-commits") should
actually be "refs/notes/refs/meta/bad-commits".  That's a bug in the
UI and/or documentation.  If we are given a string starting with
"refs/" and its documented as taking a qualified name, that's
qualified and should either be rejected, or should be taken as-is.

I think the docs are correct, and the code is buggy.  If the user
asked us to edit refs/meta/bad-commits, we should.  If the user asked
us to edit refs/heads/my-branch... well, they asked us to edit it.
:-)

A better safety measure might be to sniff the ref's contents and see
what it is.  If the top level directory has a number of non-note like
entries, we should abort editing the branch.  Its not common for users
to name their directories "02" and "fe".

-- 
Shawn.

Re: [PATCH] Remove restriction on notes ref base

[Jeff King]

On Tue, Nov 02, 2010 at 07:11:45AM -0700, Shawn O. Pearce wrote:

> I didn't want to use refs/notes/bad-commits because its not really an
> annotation you would be looking at with git log.  But we do want to
> have a log of who banned particular SHA-1s from entering the
> repository, and being able to push that branch from a workstation to
> the server is a convenient way to edit that list of banned SHA-1s.

FWIW, I use refs/notes/cache/textconv/* to store textconv caches, and
there is no problem. Unless somebody specifically configures
GIT_NOTES_REF or notes.displayRef to see them, log should never look at
them.

So I think you are being overly cautious. That being said:

> I think the docs are correct, and the code is buggy.  If the user
> asked us to edit refs/meta/bad-commits, we should.  If the user asked
> us to edit refs/heads/my-branch... well, they asked us to edit it.
> :-)

I agree. This part of git is unlike most other parts, which tend to DWYM
with a partial ref (by prepending refs/heads, refs/tags, etc), but still
accept a full ref if the user really feels like organizing their refs
differently.

> A better safety measure might be to sniff the ref's contents and see
> what it is.  If the top level directory has a number of non-note like
> entries, we should abort editing the branch.  Its not common for users
> to name their directories "02" and "fe".

Agreed.

-Peff

Re: [PATCH] Remove restriction on notes ref base

[Johan Herland]

On Tuesday 02 November 2010, Shawn Pearce wrote:
> On Tue, Nov 2, 2010 at 1:48 AM, Johan Herland wrote:
> > On Tuesday 02 November 2010, Jonathan Nieder wrote:
> >> (+cc: Johan, Thomas)
> >>
> >> Kenny Root wrote:
> >> > Git notes were restricted to refs/notes/* in the command line
> >> > utilities, but setting things like GIT_NOTES_REF to something
> >> > outside of that structure would work.
> >> >
> >> > This removes the restrictions on the git notes command line
> >> > interface.
> >
> > Why do you need to set GIT_NOTES_REF to something outside
> > refs/notes/ at all?
>
> I wanted Kenny to create a notes branch called refs/meta/bad-commits,
> and put that inside of one of our Gerrit Code Review server
> repositories.  We want to extend Gerrit Code Review to check to see
> if any commit in the incoming pack appears in the bad-commits with a
> note.  If it does, it will reject the push.  This allows a repository
> owner to ban certain commits from re-entering a repository once they
> have done a filter-branch or rebase to rewrite a particular item out
> of history.

Ah. Nice feature. Yet another use case for 'git notes' to add to my 
list... :)

> I didn't want to use refs/notes/bad-commits because its not really an
> annotation you would be looking at with git log. But we do want to
> have a log of who banned particular SHA-1s from entering the
> repository, and being able to push that branch from a workstation to
> the server is a convenient way to edit that list of banned SHA-1s.
> During prototyping Kenny discovered you can't use `git note --ref
> refs/meta/bad-commits`.  Which means a server administrator wouldn't
> be able to edit the list directly in the repository.  Hence this
> patch.

Ok, so you're using notes to annotate commits that should NOT exist in 
your repo. In other words (from the code's POV) you are annotating SHA1 
sums that (hopefully) don't exist as objects in your repo.

The notes code will happily do this (and I don't plan on changing this 
behaviour), but you should still know that this is more a side-effect 
of the current implementation than part of the design specification. 
Other notes implementations (jgit?) will need to keep this extra 
requirement in mind.

Also, if you run 'git notes prune' on your notes ref, it will (as 
specified) happily purge all notes to non-existing objects.

> >> Cc-ing some people more knowledgeable about notes than I am; maybe
> >> they can give more information about what this notes.rewriteref
> >> protection and other check are about.
> >
> > Well, I guess we originally decided to limit notes refs to within
> > refs/notes/ in order to clearly separate notes from non-notes, and
> > to prevent notes code from accidentally messing up non-notes refs.
>
> I guess I can see some logic in this.  But the documentation says
> --ref accepts a qualified ref name, and then proceeds to assume that
> a string that starts with "refs/" (like "refs/meta/bad-commits")
> should actually be "refs/notes/refs/meta/bad-commits".  That's a bug
> in the UI and/or documentation.  If we are given a string starting
> with "refs/" and its documented as taking a qualified name, that's
> qualified and should either be rejected, or should be taken as-is.
>
> I think the docs are correct, and the code is buggy.  If the user
> asked us to edit refs/meta/bad-commits, we should.  If the user asked
> us to edit refs/heads/my-branch... well, they asked us to edit it.
>
> :-)

Ok, we've probably been too conservative by trying to restrict where 
users put notes. As I said in the earlier email, we need to loosen this 
up in any case, in order to share notes with remote repos.

> A better safety measure might be to sniff the ref's contents and see
> what it is.  If the top level directory has a number of non-note like
> entries, we should abort editing the branch.  Its not common for
> users to name their directories "02" and "fe".

Good idea. We should probably add something like this to the bottom of 
notes.c:init_notes() (UNTESTED - not even compiled):

  unsigned int i, empty_tree = 1;

  for (i = 0; i < 16; i++) {
  	if (t->root[i]) {
  		empty_tree = 0;
  		break;
  	}
  }

  /* If no notes and >0 non-notes, this is probably not a notes tree */
  if (empty_tree && t->first_non_note)
  	die("'%s' doesn't look like a valid notes tree", t->ref);


Have fun! :)

...Johan

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

Re: [PATCH] Remove restriction on notes ref base

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> I didn't want to use refs/notes/bad-commits because its not really an
> annotation you would be looking at with git log.

Why not?  Within a repository with bad commits, you would want an option
to have them applied to bad commits you still have, no?

I am still torn with this patch, and I say "still" for a reason.  Even
though notes are implemented as commits, they are not commits on a part of
normal histories, and restriction of them within refs/notes hierarchy at
least is a safety measure (it is easy to loosen a restriction later, but
it is hard to let people loose first and then later restrict).  While a
purist in me says that GIT_NOTES_REF and command line option _should_
enforce the same restriction, keeping the more obscure GIT_NOTES_REF
interface a bit looser gives us an escape hatch.

As to remote interface, refs/remotes/$remotes/ hierarchy corresponds to
the local refs/heads/ interface, so I do not think we will change the
default mapping we document (and have "clone" prepare) to place notes
obtained from elsewhere in refs/remotes/ hierarchy (we do not do that for
tags neither), so I think Johan's point is an independent issue.

Re: [PATCH] Remove restriction on notes ref base

[Johan Herland]

On Tuesday 02 November 2010, Junio C Hamano wrote:
> As to remote interface, refs/remotes/$remotes/ hierarchy corresponds to
> the local refs/heads/ interface, so I do not think we will change the
> default mapping we document (and have "clone" prepare) to place notes
> obtained from elsewhere in refs/remotes/ hierarchy (we do not do that for
> tags neither), so I think Johan's point is an independent issue.

I assume that means you'd rather have remote-tracking notes refs live under 
refs/notes. So, to avoid collisions with local notes refs (and remote-
tracking notes refs from _other_ remote), we probably want to store them 
under refs/notes/remotes/$remote/. I have no problem with that.

Although I'm starting to wonder whether our remote -> local refspec mappings 
are getting too varied (i.e. confusing). Currently we have:

  Remote repo    ->   Local repo
  ------------------------------------------------
  refs/heads/*        refs/remotes/$remote/*
  refs/tags/*         refs/tags/*

...and soon we may also have:
  refs/notes/*        refs/notes/remotes/$remote/*

Of these, the first is specified in the config, the second is 
implicit/magic, and the third would be specified in the config.


...Johan

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

RE: [PATCH] Remove restriction on notes ref base

[Chris Forbes]

Hi

> Although I'm starting to wonder whether our remote -> local refspec
mappings 
> are getting too varied (i.e. confusing). Currently we have:

>  Remote repo    ->   Local repo
>  ------------------------------------------------
>  refs/heads/*        refs/remotes/$remote/*
>  refs/tags/*         refs/tags/*

What's the rationale for the implicit/magic behaviour for tags? It
causes plenty of confusion for new users, etc.

-- Chris Forbes

Re: [PATCH] Remove restriction on notes ref base

[Jonathan Nieder]

Johan Herland wrote:

> Although I'm starting to wonder whether our remote -> local refspec mappings 
> are getting too varied (i.e. confusing). Currently we have:
> 
>   Remote repo    ->   Local repo
>   ------------------------------------------------
>   refs/heads/*        refs/remotes/$remote/*
>   refs/tags/*         refs/tags/*
> 
> ...and soon we may also have:
>   refs/notes/*        refs/notes/remotes/$remote/*

How about

    refs/notes/*        refs/notes/$remote/*

?

Plus side: shorter magic prefix to remember,
           --notes-ref=charon/full would work with the
           current --notes-ref dwimery.
Downside: potential for name conflicts?

Re: [PATCH] Remove restriction on notes ref base

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> How about
>
>     refs/notes/*        refs/notes/$remote/*
>
> ?

I was actually thinking more along the lines of "not keeping track of
remote state at all".  We don't do that for tags either.

Re: [PATCH] Remove restriction on notes ref base

[Sverre Rabbelier]

Heya,

On Wed, Nov 3, 2010 at 17:17, Junio C Hamano <gitster@pobox.com> wrote:
> I was actually thinking more along the lines of "not keeping track of
> remote state at all".  We don't do that for tags either.

I would rather see us go the other way (and make the tags refspec put
tags under refs/tags/remotes/.../). I can understand not scoping tags
(since they're supposed to be immutable, and are usually global), but
I don't think the same holds for notes. Notes _are_ versioned, and
it's expected that users will collaborate.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Remove restriction on notes ref base

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> How about
>>
>>     refs/notes/*        refs/notes/$remote/*
>>
>> ?
>
> I was actually thinking more along the lines of "not keeping track of
> remote state at all".  We don't do that for tags either.

What would be the behavior when a remote and local notes ref have the
same name?

Tags are supposed to be universal and never change, so that doesn't
come up.  With branches and notes trees, one can impose a requirement
that the remote and local refs never have the same name (the "no
separate remotes" layout) but I think that runs into problems from
both ends:

When the remote and local notes are written by the same person, it
seems like busy work to ask that person to give different names for
the same branch at different installations.

In a distributed world where many developers might not even know
about each other's repositories, name conflicts on development
branches would seem to be inevitable.

 home> ... hack hack hack ...
 home> git notes add some-commit;	# hmm, noticed something
 home> ... hack hack hack ...

 Time to go to work...

 work> ... hack hack hack ...
 work> git notes add another-commit
 work> ... hack hack hack ...
 work> git fetch home;			# grabbing notes from home.
 work> git notes merge home/commits
 work> git push;			# publish.

Still, as long as the default can be overridden (even if only on a
per-remote basis), I wouldn't mind.  Maybe this way of using notes
would be unusual and the no-separate-remotes layout supports some
other use case better.

Re: [PATCH] Remove restriction on notes ref base

[Johan Herland]

On Wednesday 03 November 2010, Sverre Rabbelier wrote:
> On Wed, Nov 3, 2010 at 17:17, Junio C Hamano <gitster@pobox.com> wrote:
> > I was actually thinking more along the lines of "not keeping track of
> > remote state at all".  We don't do that for tags either.
> 
> I would rather see us go the other way (and make the tags refspec put
> tags under refs/tags/remotes/.../). I can understand not scoping tags
> (since they're supposed to be immutable, and are usually global), but
> I don't think the same holds for notes. Notes _are_ versioned, and
> it's expected that users will collaborate.

Agreed. I don't see how you can easily share and manipulate notes between 
repos _without_ keeping the remote state separate from the local state.


I'm probably gonna be flamed for this, but I'd like to go even further, and 
- for a future major version of Git - reconsider Git's default refspecs. 
Currently we have:

  Remote repo    ->   Local repo
  ------------------------------------------------
  refs/heads/*        refs/remotes/$remote/*
  refs/tags/*         refs/tags/*
  refs/notes/*        ???

Of these, the first is specified in the config, the second is 
implicit/magic, and the third would be specified in the config.

I'd probably suggest a more straightforward (and hopefully less confusing) 
setup like this:

  Remote repo    ->   Local repo
  ------------------------------------------------
  refs/heads/*        refs/remotes/$remote/heads/*
  refs/tags/*         refs/remotes/$remote/tags/*
  refs/notes/*        refs/remotes/$remote/notes/*

...and these would all be set in the config, i.e. no implicit/magic 
refspecs.

Now, there would obviously need to be some accompanying changes:

We would, for example, extend the ref disambiguation of <name> (as 
documented in the "SPECIFYING REVISIONS" section of git-rev-parse(1)), so 
that in the cases where <name> is of the form "<foo>/<bar>" AND <foo> is an 
existing remote, we also check for the following refs (after none of the 
existing checks have returned a match):

  7. refs/remotes/<foo>/tags/<bar>
  8. refs/remotes/<foo>/heads/<bar>

We would also need similar disambiguation rules for notes refs, e.g.:

  1. $GIT_DIR/<name>
  2. refs/notes/<name>
  3. refs/remotes/<foo>/notes/<bar> (when <name> is of the form <foo>/<bar>)

With these rules, we could use "origin/master", "origin/v1.2.3" and 
"origin/bugnotes" to refer to "refs/remotes/origin/heads/master", 
"refs/remotes/origin/tags/v1.2.3" and "refs/remotes/origin/notes/bugnotes" 
respectively.

As a bonus, we'd get better handling of conflicting tag names: If e.g. 
remotes "alice" and "bob" each have a tag "xyzzy" pointing to different 
objects, you could reference and compare both tags (using "alice/xyzzy" and 
"bob/xyzzy", respectively), and optionally set your own local tag 
("refs/tags/xyzzy") to match either of them.


...Johan (scrambles for a flame retardant suit)


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

Re: [PATCH] Remove restriction on notes ref base

[Sverre Rabbelier]

Heya,

On Thu, Nov 4, 2010 at 01:49, Johan Herland <johan@herland.net> wrote:
> ...Johan (scrambles for a flame retardant suit)

I for one, have thought about this design, and it's probably how I
will represent refs for hg remotes (for git-remote-hg). E.g.:

* refs/remotes/<some-hg-remote>/heads
* refs/remotes/<some-hg-remote>/tags
* refs/remotes/<some-hg-remote>/branches

So, no retardant suit needed as far as I'm concerned :)

-- 
Cheers,

Sverre Rabbelier

Tag refspecs (was Re: [PATCH] Remove restriction on notes ref base)

[Marc Branchaud]

On 10-11-03 08:49 PM, Johan Herland wrote:
> On Wednesday 03 November 2010, Sverre Rabbelier wrote:
>> On Wed, Nov 3, 2010 at 17:17, Junio C Hamano <gitster@pobox.com> wrote:
>>> I was actually thinking more along the lines of "not keeping track of
>>> remote state at all".  We don't do that for tags either.
>>
>> I would rather see us go the other way (and make the tags refspec put
>> tags under refs/tags/remotes/.../). I can understand not scoping tags
>> (since they're supposed to be immutable, and are usually global), but
>> I don't think the same holds for notes. Notes _are_ versioned, and
>> it's expected that users will collaborate.
> 
> Agreed. I don't see how you can easily share and manipulate notes between 
> repos _without_ keeping the remote state separate from the local state.
> 
> 
> I'm probably gonna be flamed for this, but I'd like to go even further, and 
> - for a future major version of Git - reconsider Git's default refspecs. 
> Currently we have:
> 
>   Remote repo    ->   Local repo
>   ------------------------------------------------
>   refs/heads/*        refs/remotes/$remote/*
>   refs/tags/*         refs/tags/*
>   refs/notes/*        ???
> 
> Of these, the first is specified in the config, the second is 
> implicit/magic, and the third would be specified in the config.
> 
> I'd probably suggest a more straightforward (and hopefully less confusing) 
> setup like this:
> 
>   Remote repo    ->   Local repo
>   ------------------------------------------------
>   refs/heads/*        refs/remotes/$remote/heads/*
>   refs/tags/*         refs/remotes/$remote/tags/*
>   refs/notes/*        refs/remotes/$remote/notes/*
> 
> ...and these would all be set in the config, i.e. no implicit/magic 
> refspecs.

I'll second this proposal, at least as far as tags go.  I can offer two
reasons to support this.


First, I think the assumption that tags are immutable is too strong.  In our
repo, we try to keep our topic branches mergeable into both the "master" and
"maintenance-of-the-latest-release" branches.

This means the topic branches need to be based at the point where the
maintenance and master branches diverged.  Making this rule easy to follow is
best accomplished with a tag, e.g. "topic-base", but that tag will move when
we create a new maintenance branch for a new release.  With the current tag
semantics, when that happens everyone has to delete their local topic-base
tags and get the new one from the common/shared repo.  People who forget to
do this end up basing their topics on outdated code, with predictable results.

It would be much easier to be able to just use an "origin/topic-base" tag
instead, one that that tracks the topic-base tag in the origin repo.


Second, I agree with Johan that the current semantics are confusing.  I'm
basically the git guru here at work, so it falls to me to teach people how
git works and how to use it.  I find that once people wrap their minds around
the concept of remote and local branches, they get tripped up by the way tags
work.

IMHO it would be more intuitive if tags used the same local/remote semantics
as branches.

		M.

Re: [PATCH] Remove restriction on notes ref base

[Jeff King]

On Thu, Nov 04, 2010 at 01:49:47AM +0100, Johan Herland wrote:

> I'd probably suggest a more straightforward (and hopefully less confusing) 
> setup like this:
> 
>   Remote repo    ->   Local repo
>   ------------------------------------------------
>   refs/heads/*        refs/remotes/$remote/heads/*
>   refs/tags/*         refs/remotes/$remote/tags/*
>   refs/notes/*        refs/remotes/$remote/notes/*
> 
> ...and these would all be set in the config, i.e. no implicit/magic 
> refspecs.

I have often considered that something like that would be simpler, too.
But just specifying

  fetch = refs/tags/*:refs/remotes/$remote/tags/*

would pull down _all_ tags from the remote. Right now we only pull down
tags for things that we are actually fetching (i.e., auto-follow).

Now you could argue that auto-follow is not worth the effort. It is
somewhat confusing, and I can't think of a time when it ever actually
reduced the set of objects I was fetching (as opposed to just fetching
all tags). But maybe others have use cases where it matters.

> We would, for example, extend the ref disambiguation of <name> (as 
> documented in the "SPECIFYING REVISIONS" section of git-rev-parse(1)), so 
> that in the cases where <name> is of the form "<foo>/<bar>" AND <foo> is an 
> existing remote, we also check for the following refs (after none of the 
> existing checks have returned a match):
> 
>   7. refs/remotes/<foo>/tags/<bar>
>   8. refs/remotes/<foo>/heads/<bar>

This codifies that refs for remote $foo are in refs/remotes/$foo, which
is something we have avoided so far. For example, when finding the
"upstream" branch, we have the name of the remote and the merge branch,
look up the fetch refspecs in the config, and then figure out where that
branch would be fetched to. Which of course turns out as you say (as
remotes/$remote_name/$branch) in the default config, but we don't
restrict people to that.

-Peff

Re: Tag refspecs (was Re: [PATCH] Remove restriction on notes ref base)

[Johan Herland]

On Thursday 04 November 2010, Marc Branchaud wrote:
> On 10-11-03 08:49 PM, Johan Herland wrote:
> > I'd probably suggest a more straightforward (and hopefully less
> > confusing)
> > 
> > setup like this:
> >   Remote repo    ->   Local repo
> >   ------------------------------------------------
> >   refs/heads/*        refs/remotes/$remote/heads/*
> >   refs/tags/*         refs/remotes/$remote/tags/*
> >   refs/notes/*        refs/remotes/$remote/notes/*
> > 
> > ...and these would all be set in the config, i.e. no implicit/magic
> > refspecs.
> 
> I'll second this proposal, at least as far as tags go.  I can offer two
> reasons to support this.
> 
> 
> First, I think the assumption that tags are immutable is too strong.  In
> our repo, we try to keep our topic branches mergeable into both the
> "master" and "maintenance-of-the-latest-release" branches.
> 
> This means the topic branches need to be based at the point where the
> maintenance and master branches diverged.  Making this rule easy to
> follow is best accomplished with a tag, e.g. "topic-base", but that tag
> will move when we create a new maintenance branch for a new release. 
> With the current tag semantics, when that happens everyone has to delete
> their local topic-base tags and get the new one from the common/shared
> repo.  People who forget to do this end up basing their topics on
> outdated code, with predictable results.
> 
> It would be much easier to be able to just use an "origin/topic-base" tag
> instead, one that that tracks the topic-base tag in the origin repo.

Actually, this is not a valid reason. To me, it sounds like your "topic-
base" tag _really_ should be a "topic_base" _branch_. The branch is 
initialized to the merge-base between "master" and "maintenance-of-the-
latest-release" branches, and when you create a new maintenance branch, the 
"topic-base" branch is fast-forwarded along the master branch until it 
reaches the merge-base with the new maintenance branch. (Remember that 
branches really are nothing but named pointers into the commit graph, and as 
long as the "topic-base" branch stays within the history of the "master" 
branch, it does not constitute a proper "branch", i.e. a fork in the commit 
history.)

IMHO, tags should still be very much immutable.

However, when working with remotes, you may get into a situation where the 
same tag name is independently set in different repos pointing at different 
commits. When these repos fetch from eachother, the tag name collision will 
be ignored, and each repo will keep their local version of the tag. Git 
will, AFAIK, not alert you to the tag name collision, and even after you 
identify the collision, it is not always easy to resolve it [1]. However, if 
we namespace remote tags (like we already do with remote branches), it is 
much easier to review which commits each remote has associated with a given 
tag name, and it's also much simpler to resolve which remote's tag you want 
to use in your own repo. (e.g. "git tag xyzzy refs/remotes/foo/tags/xyzzy").

> Second, I agree with Johan that the current semantics are confusing.  I'm
> basically the git guru here at work, so it falls to me to teach people
> how git works and how to use it.  I find that once people wrap their
> minds around the concept of remote and local branches, they get tripped
> up by the way tags work.
> 
> IMHO it would be more intuitive if tags used the same local/remote
> semantics as branches.

I am in much the same situation at my $dayjob, and I can only agree.


...Johan


[1]: Resolving tag collisions currently involves removing the tag from one 
repo and re-fetching from the other, or writing a custom refspec to rename 
the tag when fetching.


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

Re: [PATCH] Remove restriction on notes ref base

[Johan Herland]

On Thursday 04 November 2010, Jeff King wrote:
> On Thu, Nov 04, 2010 at 01:49:47AM +0100, Johan Herland wrote:
> > I'd probably suggest a more straightforward (and hopefully less
> > confusing)
> > 
> > setup like this:
> >   Remote repo    ->   Local repo
> >   ------------------------------------------------
> >   refs/heads/*        refs/remotes/$remote/heads/*
> >   refs/tags/*         refs/remotes/$remote/tags/*
> >   refs/notes/*        refs/remotes/$remote/notes/*
> > 
> > ...and these would all be set in the config, i.e. no implicit/magic
> > refspecs.
> 
> I have often considered that something like that would be simpler, too.
> But just specifying
> 
>   fetch = refs/tags/*:refs/remotes/$remote/tags/*
> 
> would pull down _all_ tags from the remote. Right now we only pull down
> tags for things that we are actually fetching (i.e., auto-follow).
> 
> Now you could argue that auto-follow is not worth the effort. It is
> somewhat confusing, and I can't think of a time when it ever actually
> reduced the set of objects I was fetching (as opposed to just fetching
> all tags). But maybe others have use cases where it matters.

Actually, I don't have anything against auto-follow per se, and we could 
easily enough extend the refspec to specify auto-follow behaviour: There is 
currently the "+" prefix for specifying a forced (non-fast-forward) update. 
We could for example add a "~" prefix for specifying that the refs matched 
by this refspec should only be fetched if they are part of the history 
fetched by other refspecs. We could then reproduce the current auto-follow 
behaviour with the following pair of refspecs:

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

or in my above proposal:

  +refs/heads/*:refs/remotes/$remote/heads/*
  ~+refs/tags/*:refs/remotes/$remote/tags/*
  +refs/notes/*:refs/remotes/$remote/notes/*

> > We would, for example, extend the ref disambiguation of <name> (as
> > documented in the "SPECIFYING REVISIONS" section of git-rev-parse(1)),
> > so that in the cases where <name> is of the form "<foo>/<bar>" AND
> > <foo> is an existing remote, we also check for the following refs
> > (after none of the
> > 
> > existing checks have returned a match):
> >   7. refs/remotes/<foo>/tags/<bar>
> >   8. refs/remotes/<foo>/heads/<bar>
> 
> This codifies that refs for remote $foo are in refs/remotes/$foo, which
> is something we have avoided so far. For example, when finding the
> "upstream" branch, we have the name of the remote and the merge branch,
> look up the fetch refspecs in the config, and then figure out where that
> branch would be fetched to. Which of course turns out as you say (as
> remotes/$remote_name/$branch) in the default config, but we don't
> restrict people to that.

We can do the same for "foo/bar" as well, although things become slightly 
more fiddly:

When "foo" exists as a remote, look up its fetch refspec(s), and determine 
possible mappings for name "bar". I.e. given the following (non-default) 
refspecs for remote "foo":

  +refs/heads/*:refs/remotes/spam/heads/*
  +refs/tags/*:refs/remotes/eggs/tags/*
  +refs/notes/*:refs/remotes/bacon/notes/*

we know that the intersection between the left side of these refspec and the 
ref disambiguation rules consists of "refs/tags/bar" and "refs/heads/bar" 
(in that order). We can then map these to the right side of the refspec to 
get "refs/remotes/eggs/tags/bar" and "refs/remotes/spam/heads/bar" (in that 
order). We would then use the first match from these alternatives.


...Johan

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

Re: [PATCH] Remove restriction on notes ref base

[Jeff King]

On Fri, Nov 05, 2010 at 02:29:15AM +0100, Johan Herland wrote:

> Actually, I don't have anything against auto-follow per se, and we could 
> easily enough extend the refspec to specify auto-follow behaviour: There is 

Yup, agreed. That would be sufficient to capture the current behavior
(and IMHO makes it much more transparent to the user).

> > This codifies that refs for remote $foo are in refs/remotes/$foo, which
> > is something we have avoided so far. For example, when finding the
> > "upstream" branch, we have the name of the remote and the merge branch,
> > look up the fetch refspecs in the config, and then figure out where that
> > branch would be fetched to. Which of course turns out as you say (as
> > remotes/$remote_name/$branch) in the default config, but we don't
> > restrict people to that.
> 
> We can do the same for "foo/bar" as well, although things become slightly 
> more fiddly:

Yeah, I don't think the problem is unsurmountable. But I do think it is
worth doing the more complex behavior, as it keeps things "right" with
respect to arbitrary config.

> When "foo" exists as a remote, look up its fetch refspec(s), and determine 
> possible mappings for name "bar". I.e. given the following (non-default) 
> refspecs for remote "foo":
> 
>   +refs/heads/*:refs/remotes/spam/heads/*
>   +refs/tags/*:refs/remotes/eggs/tags/*
>   +refs/notes/*:refs/remotes/bacon/notes/*
> 
> we know that the intersection between the left side of these refspec and the 
> ref disambiguation rules consists of "refs/tags/bar" and "refs/heads/bar" 
> (in that order). We can then map these to the right side of the refspec to 
> get "refs/remotes/eggs/tags/bar" and "refs/remotes/spam/heads/bar" (in that 
> order). We would then use the first match from these alternatives.

Yeah, that makes sense. If the remote side is storing tags somewhere
besides "refs/tags" you wouldn't find them, but that is probably a good
thing.

-Peff

Re: Tag refspecs (was Re: [PATCH] Remove restriction on notes ref base)

[Marc Branchaud]

On 10-11-04 09:02 PM, Johan Herland wrote:
> On Thursday 04 November 2010, Marc Branchaud wrote:
>> On 10-11-03 08:49 PM, Johan Herland wrote:
>>> I'd probably suggest a more straightforward (and hopefully less
>>> confusing)
>>>
>>> setup like this:
>>>   Remote repo    ->   Local repo
>>>   ------------------------------------------------
>>>   refs/heads/*        refs/remotes/$remote/heads/*
>>>   refs/tags/*         refs/remotes/$remote/tags/*
>>>   refs/notes/*        refs/remotes/$remote/notes/*
>>>
>>> ...and these would all be set in the config, i.e. no implicit/magic
>>> refspecs.
>>
>> I'll second this proposal, at least as far as tags go.  I can offer two
>> reasons to support this.
>>
>>
>> First, I think the assumption that tags are immutable is too strong.  In
>> our repo, we try to keep our topic branches mergeable into both the
>> "master" and "maintenance-of-the-latest-release" branches.
>>
>> This means the topic branches need to be based at the point where the
>> maintenance and master branches diverged.  Making this rule easy to
>> follow is best accomplished with a tag, e.g. "topic-base", but that tag
>> will move when we create a new maintenance branch for a new release. 
>> With the current tag semantics, when that happens everyone has to delete
>> their local topic-base tags and get the new one from the common/shared
>> repo.  People who forget to do this end up basing their topics on
>> outdated code, with predictable results.
>>
>> It would be much easier to be able to just use an "origin/topic-base" tag
>> instead, one that that tracks the topic-base tag in the origin repo.
> 
> Actually, this is not a valid reason. To me, it sounds like your "topic-
> base" tag _really_ should be a "topic_base" _branch_. The branch is 
> initialized to the merge-base between "master" and "maintenance-of-the-
> latest-release" branches, and when you create a new maintenance branch, the 
> "topic-base" branch is fast-forwarded along the master branch until it 
> reaches the merge-base with the new maintenance branch. (Remember that 
> branches really are nothing but named pointers into the commit graph, and as 
> long as the "topic-base" branch stays within the history of the "master" 
> branch, it does not constitute a proper "branch", i.e. a fork in the commit 
> history.)

I understand how a branch can, with careful management, mechanically achieve
the behaviour I want.

But the problem with that approach is that it breaks as soon as someone
commits to the topic-base branch (an easy mistake to make, especially for
rookie users).  Then that person would base later topics on the wrong commit,
and worse, if they pushed their "topic-base" branch to our shared repo then
everyone would be affected.

A hook in the shared repo could prevent the pushing of the topic-base branch,
but that that's just a complicated, half-baked way of making a branch behave
like a tag.  A tag doesn't need the careful management that a branch would.


Anyway, regardless of whether or not the scenario I described is valid, I
still wonder if tag immutability makes sense when remote tags live in the
repo's namespace.  If a repo first sets tag "foo" to point to commit 'A' then
later changes "foo" to point to 'B', shouldn't my clone's "repo/foo" tag
update automatically to reflect that?  I see little point in keeping my
"repo/foo" tag pointing at commit 'A'.

		M.