Re: [PATCH v3] notes: Allow committish expressions as notes ref

[Johan Herland <johan@herland.net>]

On Thu, Jul 9, 2015 at 4:48 AM, Mike Hommey <mh@glandium.org> 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@glandium.org>

Reviewed-by: Johan Herland <johan@herland.net>

...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@herland.net>
www.herland.net