Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes()

On Tue, Jun 16, 2015 at 08:17:03PM -0700, Junio C Hamano wrote:

Mike Hommey [off-list ref] writes:

quoted

init_notes() is essentially the only point of entry to the notes API.
It is an arbitrary restriction that all it allows as input is a strict
ref name, when callers may want to give an arbitrary committish.

While it may be a good idea to allow reading from any note-shaped
tree-ish, not just what is at the tip of a ref, I suspect that the
use of read_ref() is not an arbitrary restriction, but is an
effective way to achieve safety against callers that update notes.

That is, you can feed refs/notes/commit@{4.days.ago} to the
machinery and show you notes from 4 days ago, but you cannot update
that as if it were a ref.

Hence, if you are loosening the safety at init_notes() site, you
would at least need to add a similar safety in the write codepath, I
would think.  

One thing you would need to be careful about is that you would give
users a crappy experience, if an operation reads notes, does its own
thing, and then tries to write updated notes (think: rebase that
transplants notes from original to rewritten commits), and you fail
the operation only at the very last phase of updating.  In order to
prevent that, "in the write codepath" above has to be "reject any
non-ref note, e.g. --notes=refs/notes/commit@{4.days.ago} upfront,
if the operation will write updated notes".

Looking around in the code, I'm not sure how to address this.
Considering the APIs, it would seem logical to have individual callers
care about filtering the refs themselves, on the other hand, there's
room for shooting oneself in the foot when adding new code paths needing
writes.

I'm tempted to make init_notes itself do the check, based on the value
it is given for a "read_only" argument. On the other hand, some commands
do their ref resolving themselves already. For example, notes_merge uses
read_ref_full and check_refname_format itself (not necessarily in the
right order, btw). OTOH, I'm not even sure it ends up using init_notes
at all.

Mike