git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johan Herland <johan@herland.net>
Date
Jul 10, 2015, 01:03 UTC
Message-ID
<CALKQrgd_eB2ZUwUz54vW9OFOuZshvGGe9dx+suvd0=WA0OmOFQ@mail.gmail.com>
In-Reply-To
<1436410125-4957-1-git-send-email-mh@glandium.org>
On Thu, Jul 9, 2015 at 4:48 AM, Mike Hommey <mh@glandium.org> wrote:
Show 16 quoted lines
> 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.
Show 37 quoted lines
> ---
>  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.

Show 7 quoted lines
> +       if (!starts_with(ref, "refs/notes/"))
>                 die("Refusing to %s notes in %s (outside of refs/notes/)",
> -                   subcommand, t->ref);
> +                   subcommand, ref);
>         return t;
>  }
>
[...]
Show 9 quoted lines
> 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).

Show 7 quoted lines
>         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 &&.
[....]
Show 18 quoted lines
> 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
Previous: Mike HommeyNext: Mike Hommey
Message 8 of 22 in “notes: Use get_sha1_committish instead of read_ref in init_notes()”
  1. notes: Use get_sha1_committish instead of read_ref in init_notes()Mike Hommey, Jun 17, 2015
  2. Junio C HamanoJun 17, 2015
  3. Mike HommeyJun 17, 2015
  4. Junio C HamanoJun 17, 2015
  5. Johan HerlandJun 17, 2015
  6. notes: Allow committish expressions as notes refMike Hommey, Jul 8, 2015
  7. notes: Allow committish expressions as notes refMike Hommey, Jul 9, 2015
  8. Johan HerlandJul 10, 2015
  9. notes: Allow treeish expressions as notes refMike Hommey, Jul 10, 2015
  10. Johan HerlandJul 10, 2015
  11. Mike HommeyJul 10, 2015
  12. notes: Allow treeish expressions as notes refMike Hommey, Jul 10, 2015
  13. Junio C HamanoJul 13, 2015
  14. Mike HommeyJul 13, 2015
  15. Junio C HamanoJul 13, 2015
  16. Mike HommeyOct 8, 2015
  17. notes: allow treeish expressions as notes refMike Hommey, Oct 8, 2015
  18. Junio C HamanoOct 8, 2015
  19. Junio C HamanoJul 13, 2015
  20. Jeff KingJun 17, 2015
  21. Mike HommeyJun 17, 2015
  22. Jeff KingJun 17, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.