Mike Hommey <mh@xxxxxxxxxxxx> writes:
> init_notes() is the main 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 treeish.
>
> However, some operations that require updating the notes tree require a
> strict ref name, because they wouldn't be able to update e.g. foo@{1}.
>
> So we allow treeish expressions to be used in the case the notes tree is
> going to be used without write "permissions", and to distinguish whether
> the notes tree is intended to be used for reads only, or will be updated,
> a flag is added.
It is unfair to call the current check arbitrary, though. From the
point of view of the person who views notes as a database, it is
entirely a sensible thing to treat it as an read/write data store.
> This has the side effect of enabling the use of treeish as notes refs in
> commands allowing them, e.g. git log --notes=foo@{1}.
I do not think it is a "side effect". It's the primary benefit this
change brings in.
I'd flip the attitude around and sell this as "an enhancement",
perhaps like this:
notes: allow treeish expressions as notes ref
init_notes() is the main point of entry to the notes API. It ensures
that the input can be used as ref, because it needs a ref to update
to store notes tree after modifying it.
There however are many use cases where notes tree is only read, e.g.
"git log --notes=...". Any notes-shaped treeish could be used for
such purpose, but it is not allowed due to existing restriction.
Allow treeish expressions to be used in the case the notes tree is
going to be used without write "permissions". Add a flag to
distinguish whether the notes tree is intended to be used read-only,
or will be updated.
With this change, operations that use notes read-only can be fed any
notes-shaped tree-ish can be used, e.g. git log --notes=notes@{1}.
Signed-off-by: Mike Hommey <mh@xxxxxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
One thing I found puzzling is that the "arbitrary restriction" does
not seem to do anything visibly "restricting".
I taught my copy of "git am" to add the message-ID of the original
patch to the resulting commit, and that is kept in "amlog" notes.
Without this patch, I am getting this:
$ git log --notes=amlog -1 --pretty=short
commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
Author: Mike Hommey <mh@xxxxxxxxxxxx>
notes: allow treeish expressions as notes ref
Notes (amlog):
Message-Id: <1436517551-12172-1-git-send-email-mh@xxxxxxxxxxxx>
The above is very much expected. Now without your patch, I get
this:
$ git log --notes='amlog@{1}' -1 --pretty=short
commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
Author: Mike Hommey <mh@xxxxxxxxxxxx>
notes: allow treeish expressions as notes ref
It seems to be ignoring what is not a refname without complaining.
However, that does not seem to be the whole story. Look:
$ git log --notes='amlog@{1' -1 --pretty=short
warning: notes ref refs/notes/amlog@{1 is invalid
commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
Author: Mike Hommey <mh@xxxxxxxxxxxx>
notes: allow treeish expressions as notes ref
$ git log --notes='amlog@{5000}' -1 --pretty=short
fatal: Log for 'refs/notes/amlog' only has 3072 entries.
So sometimes we silently ignore, sometimes we warn, and sometimes
we refuse to work. The above are all without your patch applied.
With your patch, the only change I see is this:
$ git log --notes=amlog@{0} -1 --pretty=short
commit 7a3744f95d4d21b27470e5bacaed8c434150ecac
Author: Mike Hommey <mh@xxxxxxxxxxxx>
notes: allow treeish expressions as notes ref
Notes (amlog@{0}):
Message-Id: <1436517551-12172-1-git-send-email-mh@xxxxxxxxxxxx>
We read from a tree-ish ;-) Whee!
What is interesting is to think what should happen when amlog@{1}
is given. The version with your patch gives the same output as we
saw earlier, because amlog@{1} tree exists, but does not know about
your patch (yet), so does not add "Notes" section, which makes sense
by itself.
But we cannot tell if amlog@{1} was somehow malformed or it was OK
and there was no notes on the commit.
We probably should do a few more things:
- Make sure that we show "there is no such tree-ish, no way to look
up any note to any commit from there" and "I understood the tree
you gave me, but there is no note for that commit" differently.
- Decide if we want to "fail" the operation when the notes tree
given by the user is not even a tree-ish or just "warn" and keep
going. And do so consistently.
Thanks.
--
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