(This addresses some smaller specific questions from Kyle, and is not an
attempt to take focus away from the main direction of this thread)
On Jan 6, 2015 1:27 PM, "Kyle J. McKay" <mackyle@xxxxxxxxx> wrote:
> On Jan 6, 2015, at 02:20, Junio C Hamano wrote:
[...]
> At the point the dropped line runs, core.notesRef has been set to
> refs/notes/y which does not exist.
Merging into an empty or non-existing notes ref is not in itself an error.
> All of the tests in the 'fail to merge various non-note-trees' section
> fail with one of these errors:
>
> 1) Failed to resolve remote notes ref '<ref-being-tested>'
>
> 2) Cannot merge empty notes ref (<ref-being-tested>) into empty notes
> ref (refs/notes/y)
>
> 3) error: object 6c99d48c... is a tree, not a commit
>
> The 3rd error comes from the "git notes merge x:" attempt.
Yes. We need an actual commit object which can become the second patent of
the notes merge.
> So despite the name of the test, the actual tree contents do not seem to
> be examined.
Correct. The test name should probably be improved. I have a bad habit of
using the term "notes tree" to refer to generic collection of notes, sometimes
meaning the notes ref, sometimes the commit referenced by a notes ref, and
sometimes the actual tree object referenced by the notes commit (or directly
referenced by the notes ref). In this case I guess a better name for the test
would be: "fail to merge various things that are not notes commits"
> When the notes ref checking is relaxed to leave refs/heads/master alone
> rather than turning it into refs/notes/refs/heads/master, the previous
> error (#2 in this case) goes away and since refs/notes/y does not exist,
> it is simply updated to the value of refs/heads/master without any checks.
This is the expected behaviour of merging an otherwise valid (notes) ref.
> Of course that refs/heads/master tree doesn't look like a notes tree.
>
> And if we do this:
>
> git update-ref refs/notes/refs/heads/master master
(loading the gun and pointing it at your foot...)
> Then "git notes merge refs/heads/master" also succeeds without
> complaining that the refs/notes/refs/heads/master tree does not look
> like a notes tree and we didn't need to relax the refs/notes restriction
> and, as you point out, the name of the test seems to imply that would be
> rejected.
In this case, the test name is clearly misleading. As discussed elsewhere
in the thread, we cannot absolutely enforce a distinction between a "notes"
tree object (i.e. a git tree that contain ONLY notes entries) and a
"non-notes" tree object. There is only a strong convention/expectation that
a notes tree object contains only (or at least mostly) SHA1-named entries
(possibly with fanout).
That said, if you stick to the porcelain, you should not be able to create
a notes ref referencing a tree object that contains anything but notes.
> Interestingly, if we then attempt to merge refs/notes/x into this
> non-notes refs/notes/y tree, it also succeeds and even keeps the things
> that do not look like notes. The reverse merge (y into x) succeeds as
> well, but the non-notes stuff in y is not merged in in that case.
The general policy for handling non-notes in notes code, is to ignore them
when querying, and preserve them when manipulating (t/t3304-notes-mixed.sh
has examples). In this case, we preserve the non-notes from the primary
parent (the notes tree from which the merge was started), and we ignore
any non-notes coming from subsequent parents. I can't remember if this
behaviour was explicitly desired, or if it's just an accident of
implementation. However, I will argue that it doesn't make sense for
notes-merge to attempt any meaningful merge of non-notes. That is simply
outside its scope.
>> Arguably, not being able to save notes tree anywhere outside of
>> refs/notes/ hierarchy may be too high a price to pay in order to
>> prevent refs/heads/master from being considered (hence to avoid such
>> end-user mistakes), but at the same time, losing this safetly may
>> also be too high a price to pay in order to allow people to store
>> their notes in somewhere outside e.g. refs/remote-notes/origin/foo.
>> "Somewhere outside" does not mean "Including other hierarchies like
>> refs/heads and refs/tags that have long established meaning".
>
> If we relax the refs/notes restriction, putting a notes ref in
> refs/heads/<whatever> doesn't necessarily seem like that's a terrible
> thing as long as it's really a notes tree if used with the notes
> machinery. AIUI, the refs/heads convention only requires the ref to
> point to the tip of a commit chain which all of the refs under
> refs/notes satisfy.
At some point I remember having a discussion on whether we should
support notes refs pointing directly to a tree objects (to implement
"history-less" notes - sometimes the notes history itself is
uninteresting, if you're only interested in the current state of the
notes tree). However, I can't immediately find any tests exercising
this functionality, so it's likely unsupported (the convention here is
instead to use a commit object, but make sure to prepare the next notes
commit with zero parents; see notes-cache.{h,c} for an example of this.
> The refs/heads convention AIUI does not impose any requirement about
> the contents of the tree(s) those commits in the chain refer to.
> But at the same time I can't think of any particular reason I'd want
> to store notes refs in there either.
>
>> Although I am not fundamentally against allowing to store notes
>> outside refs/notes/, it is different from "anywhere is fine".
>> Can't we do this widening in a less damaging way?
>
> Without arbitrarily restricting where notes can be stored it seems to
> me the only option would be to have the notes machinery actually
> inspect the tree of any existing notes ref it's passed. That would
> also catch the case where
> "git update-ref refs/notes/refs/heads/master master" was run as well.
> It also seems like a good check to have in place to help catch user
> errors.
>
> I'm not all that familiar with the notes code, perhaps there's already
> a function that does the tree check to make sure the tree actually
> looks like a notes tree that can easily be called? Maybe Johan has
> some thoughts on this?
Nope, no such check. However, it's worth considering giving the user a
warning (maybe even - optionally - an error) when non-notes are found
in a notes tree. Search for non_note in notes.c to find relevant code.
Have fun! :)
...Johan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html