On Thu, Jul 9, 2015 at 4:48 AM, Mike Hommey <mh@xxxxxxxxxxxx> wrote:
> 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 committish.
>
> 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 committish 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.
>
> This has the side effect of enabling the use of committish as notes refs
> in commands allowing them, e.g. git log --notes=foo@{1}.
>
> Signed-off-by: Mike Hommey <mh@xxxxxxxxxxxx>
Reviewed-by: Johan Herland <johan@xxxxxxxxxxx>
...modulo some comments below.
> ---
> builtin/notes.c | 29 ++++++++++++++++-------------
> notes-cache.c | 11 ++++++-----
> notes-utils.c | 6 +++---
> notes.c | 11 +++++++----
> notes.h | 10 +++++++++-
> 5 files changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc..0fc6e7a 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -285,7 +285,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
> if (!c)
> return 0;
> } else {
> - init_notes(NULL, NULL, NULL, 0);
> + init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
> t = &default_notes_tree;
> }
>
> @@ -328,15 +328,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
> return ret;
> }
>
> -static struct notes_tree *init_notes_check(const char *subcommand)
> +static struct notes_tree *init_notes_check(const char *subcommand,
> + int flags)
> {
> struct notes_tree *t;
> - init_notes(NULL, NULL, NULL, 0);
> + const char *ref;
> + init_notes(NULL, NULL, NULL, flags);
> t = &default_notes_tree;
>
> - if (!starts_with(t->ref, "refs/notes/"))
> + ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;
Hmm, AFAICS from the code in notes.c, when NOTES_INIT_WRITABLE is set,
then t->ref == t->update_ref, which means the above line is redundant,
and t->ref will work just as well. That said, it's also bad to depend on
that fact from here, as the notes code might at some point change to
make t->ref != t->update_ref (e.g. if we want to allow a notes tree
which reads from one ref, but writes to another), so I guess the current
code is good.
> + if (!starts_with(ref, "refs/notes/"))
> die("Refusing to %s notes in %s (outside of refs/notes/)",
> - subcommand, t->ref);
> + subcommand, ref);
> return t;
> }
>
[...]
> diff --git a/notes.c b/notes.c
> index df08209..84f8a47 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -1007,13 +1007,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
> t->first_non_note = NULL;
> t->prev_non_note = NULL;
> t->ref = xstrdup_or_null(notes_ref);
> + t->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;
This is the only place in notes.c that references t->update_ref. Which
points to a latent design flaw in the notes API: struct notes_tree keeps
track of the notes ref (previously t->ref, but now also t->update_ref),
but it is actually never used by the core notes implementation (except
for display purposes in format_note() and an error message in
load_subtree()). I guess a better design would be to either
(a) move the ref out of the API entirely, leaving notes.h/.c to only
worry about reading and writing notes tree objects, and letting
the caller deal with resolving and updatiung notes refs and
commit objects.
(b) move the ref handling _into_ the API, basically moving
commit_notes() from notes-utils.h/.c into notes.h/.c
Of these, I believe (a) is better, escpecially if we also provide a
wrapper API (in notes-utils.h/.c?) around the inner/core API to deal
with the commit/ref details.
However, this is largely independent of your current effort, and
belongs in a different patch (series).
> t->combine_notes = combine_notes;
> t->initialized = 1;
> t->dirty = 0;
>
> if (flags & NOTES_INIT_EMPTY || !notes_ref ||
> - read_ref(notes_ref, object_sha1))
> + get_sha1_committish(notes_ref, object_sha1))
I believe this should be get_sha1_treeish() instead. The only place we use
the result (object_sha1) is when calling get_tree_entry() below, which
will peel to a tree object anyway, so there's no need to place the
stricter commitish requirement on the caller (in the read-only case).
As a bonus, you can also s/commitish/treeish/ in the commit subject line.
Also, I do not immediately remember whether we support notes refs that
point directly at tree objects (bypassing the commit object entirely),
but if we do, the get_sha1_committish() would break that use case...
> return;
> + if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1))
Drop the extra space after &&.
[....]
> diff --git a/notes.h b/notes.h
> index 2a3f923..aa9960c 100644
> --- a/notes.h
> +++ b/notes.h
> @@ -44,6 +44,7 @@ extern struct notes_tree {
> struct int_node *root;
> struct non_note *first_non_note, *prev_non_note;
> char *ref;
> + char *update_ref;
> combine_notes_fn combine_notes;
> int initialized;
> int dirty;
> @@ -72,6 +73,13 @@ const char *default_notes_ref(void);
> #define NOTES_INIT_EMPTY 1
>
> /*
> + * By default, the notes tree is only readable, and the notes ref can be
> + * any committish. The notes tree can however be made writable with this
s/commitish/treeish/
...Johan
--
Johan Herland, <johan@xxxxxxxxxxx>
www.herland.net
--
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