* [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() @ 2015-06-17 1:15 Mike Hommey 2015-06-17 3:17 ` Junio C Hamano 2015-06-17 3:22 ` [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() Jeff King 0 siblings, 2 replies; 22+ messages in thread From: Mike Hommey @ 2015-06-17 1:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Johan Herland 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. 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}, although I haven't checked whether that's the case for all of them. Signed-off-by: Mike Hommey <mh@glandium.org> --- My motivation for this change is to allow to use init_notes from a third-party helper that links to libgit.a with a commit that is not pointed at directly by a ref. The side effect of making git log --notes=foo@{1} work is nice IMHO. As noted, though, I haven't checked if all code paths using init_notes don't prefilter the ref, but I assume they don't. notes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes.c b/notes.c index df08209..402e4ce 100644 --- a/notes.c +++ b/notes.c @@ -1012,7 +1012,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, t->dirty = 0; if (flags & NOTES_INIT_EMPTY || !notes_ref || - read_ref(notes_ref, object_sha1)) + get_sha1_committish(notes_ref, object_sha1)) return; if (get_tree_entry(object_sha1, "", sha1, &mode)) die("Failed to read notes tree referenced by %s (%s)", -- 2.4.3.2.gf0a024e.dirty ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() 2015-06-17 1:15 [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() Mike Hommey @ 2015-06-17 3:17 ` Junio C Hamano 2015-06-17 9:40 ` Mike Hommey 2015-06-17 3:22 ` [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() Jeff King 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2015-06-17 3:17 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Johan Herland Mike Hommey <mh@glandium.org> writes: > 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". ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() 2015-06-17 3:17 ` Junio C Hamano @ 2015-06-17 9:40 ` Mike Hommey 2015-06-17 15:18 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Mike Hommey @ 2015-06-17 9:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johan Herland On Tue, Jun 16, 2015 at 08:17:03PM -0700, Junio C Hamano wrote: > Mike Hommey <mh@glandium.org> writes: > > > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() 2015-06-17 9:40 ` Mike Hommey @ 2015-06-17 15:18 ` Junio C Hamano 2015-06-17 16:35 ` Johan Herland 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2015-06-17 15:18 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Johan Herland Mike Hommey <mh@glandium.org> writes: > I'm tempted to make init_notes itself do the check, based on the value > it is given for a "read_only" argument. Yeah, that would be one sensible way to go after making sure that everything goes thru this interface. > On the other hand, some commands > do their ref resolving themselves already. Again, as long as they do not bypass the "read-only" safety you are suggesting to add to init_notes(), that is OK. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() 2015-06-17 15:18 ` Junio C Hamano @ 2015-06-17 16:35 ` Johan Herland 2015-07-08 10:21 ` [PATCH v2 - RFH] notes: Allow committish expressions as notes ref Mike Hommey 0 siblings, 1 reply; 22+ messages in thread From: Johan Herland @ 2015-06-17 16:35 UTC (permalink / raw) To: Mike Hommey; +Cc: Junio C Hamano, Git mailing list On Wed, Jun 17, 2015 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote: > Mike Hommey <mh@glandium.org> writes: > > I'm tempted to make init_notes itself do the check, based on the value > > it is given for a "read_only" argument. > > Yeah, that would be one sensible way to go after making sure that > everything goes thru this interface. Agreed. Furthermore, consider adding the read_only flag (or however you choose to encode it internally) to struct notes_tree, so that the API functions that _manipulate_ notes trees can immediately bail out when used on a read-only tree (i.e. we want them to fail as early as possible). > > On the other hand, some commands > > do their ref resolving themselves already. > > Again, as long as they do not bypass the "read-only" safety you are > suggesting to add to init_notes(), that is OK. Agreed. An alternative to adding a simple read_only flag argument is to modify the const char *notes_ref argument into two separate arguments: const char *notes_treeish, and const char *update_ref, where the latter should be NULL for read-only trees. That said, currently the logic for actually updating notes ref lives outside the notes.h API (see commit_notes() in notes-utils.h/c), so there might be room for more consolidation/refactoring here... ...Johan -- Johan Herland, <johan@herland.net> www.herland.net ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 - RFH] notes: Allow committish expressions as notes ref 2015-06-17 16:35 ` Johan Herland @ 2015-07-08 10:21 ` Mike Hommey 2015-07-09 2:48 ` [PATCH v3] " Mike Hommey 0 siblings, 1 reply; 22+ messages in thread From: Mike Hommey @ 2015-07-08 10:21 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Johan Herland 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> --- The die() message in init_notes is left to be desired. But I don't have a better idea at the moment. Note that die()ing in an API is not exactly nice, but since init_notes was already die()ing, I figured I wasn't making things worse than they already are. Further refactoring feels like scope bloat. Now, for the RFH, this actually doesn't entirely work, because *creating* a new notes ref fails on that die() I'm adding. So the question is, how do I distinguish between read_ref failing because the ref is not a proper ref, and read_ref failing because the ref doesn't exist? (I figured I'd ask instead of looking it up myself because it's the end of the day here, and by the time I look at it again, there should be an answer from someone who knows the code already :) ) builtin/notes.c | 27 +++++++++++++++------------ notes-cache.c | 7 ++++--- notes-utils.c | 4 ++-- notes.c | 9 +++++++-- notes.h | 8 ++++++++ 5 files changed, 36 insertions(+), 19 deletions(-) diff --git a/builtin/notes.c b/builtin/notes.c index 63f95fc..0712178 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -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; + if (!starts_with(ref, "refs/notes/")) die("Refusing to %s notes in %s (outside of refs/notes/)", - subcommand, t->ref); + subcommand, ref); return t; } @@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_list_usage, options); } - t = init_notes_check("list"); + t = init_notes_check("list", 0); if (argc) { if (get_sha1(argv[0], object)) die(_("Failed to resolve '%s' as a valid ref."), argv[0]); @@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("add"); + t = init_notes_check("add", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("copy"); + t = init_notes_check("copy", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check(argv[0]); + t = init_notes_check(argv[0], NOTES_INIT_WRITABLE); note = get_note(t, object); prepare_note_data(object, &d, edit ? note : NULL); @@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("show"); + t = init_notes_check("show", 0); note = get_note(t, object); if (!note) @@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix) } } - t = init_notes_check("merge"); + t = init_notes_check("merge", NOTES_INIT_WRITABLE); strbuf_addf(&msg, "notes: Merged notes from %s into %s", remote_ref.buf, default_notes_ref()); @@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_notes_remove_usage, 0); - t = init_notes_check("remove"); + t = init_notes_check("remove", NOTES_INIT_WRITABLE); if (!argc && !from_stdin) { retval = remove_one_note(t, "HEAD", flag); @@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_prune_usage, options); } - t = init_notes_check("prune"); + t = init_notes_check("prune", NOTES_INIT_WRITABLE); prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) | (show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) ); diff --git a/notes-cache.c b/notes-cache.c index c4e9bb7..3ee8d2c 100644 --- a/notes-cache.c +++ b/notes-cache.c @@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c) unsigned char tree_sha1[20]; unsigned char commit_sha1[20]; - if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref) + if (!c || !c->tree.initialized || !c->tree.update_ref || + !*c->tree.update_ref) return -1; if (!c->tree.dirty) return 0; @@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c) if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL, commit_sha1, NULL, NULL) < 0) return -1; - if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL, - 0, UPDATE_REFS_QUIET_ON_ERR) < 0) + if (update_ref("update notes cache", c->tree.update_ref, commit_sha1, + NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0) return -1; return 0; diff --git a/notes-utils.c b/notes-utils.c index ccbf073..937e1f0 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg) if (!t) t = &default_notes_tree; - if (!t->initialized || !t->ref || !*t->ref) + if (!t->initialized || !t->update_ref || !*t->update_ref) die(_("Cannot commit uninitialized/unreferenced notes tree")); if (!t->dirty) return; /* don't have to commit an unchanged tree */ @@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg) create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1); strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */ - update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, + update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); strbuf_release(&buf); diff --git a/notes.c b/notes.c index df08209..8c6019d 100644 --- a/notes.c +++ b/notes.c @@ -1007,12 +1007,17 @@ 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; t->combine_notes = combine_notes; t->initialized = 1; t->dirty = 0; - if (flags & NOTES_INIT_EMPTY || !notes_ref || - read_ref(notes_ref, object_sha1)) + if (flags & NOTES_INIT_EMPTY || !notes_ref) + return; + if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1)) + die("Invalid ref %s", notes_ref);; + if (!(flags & NOTES_INIT_WRITABLE) && + get_sha1_committish(notes_ref, object_sha1)) return; if (get_tree_entry(object_sha1, "", sha1, &mode)) die("Failed to read notes tree referenced by %s (%s)", diff --git a/notes.h b/notes.h index 2a3f923..9b2b96c 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 + * flag, in which case only strict ref names can be used. + */ +#define NOTES_INIT_WRITABLE 2 + +/* * Initialize the given notes_tree with the notes tree structure at the given * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment * variable is used, and if that is missing, the default notes ref is used -- 2.4.3.2.gf0a024e.dirty ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3] notes: Allow committish expressions as notes ref 2015-07-08 10:21 ` [PATCH v2 - RFH] notes: Allow committish expressions as notes ref Mike Hommey @ 2015-07-09 2:48 ` Mike Hommey 2015-07-10 1:03 ` Johan Herland 0 siblings, 1 reply; 22+ messages in thread From: Mike Hommey @ 2015-07-09 2:48 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Johan Herland 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> --- 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; + if (!starts_with(ref, "refs/notes/")) die("Refusing to %s notes in %s (outside of refs/notes/)", - subcommand, t->ref); + subcommand, ref); return t; } @@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_list_usage, options); } - t = init_notes_check("list"); + t = init_notes_check("list", 0); if (argc) { if (get_sha1(argv[0], object)) die(_("Failed to resolve '%s' as a valid ref."), argv[0]); @@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("add"); + t = init_notes_check("add", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("copy"); + t = init_notes_check("copy", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check(argv[0]); + t = init_notes_check(argv[0], NOTES_INIT_WRITABLE); note = get_note(t, object); prepare_note_data(object, &d, edit ? note : NULL); @@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("show"); + t = init_notes_check("show", 0); note = get_note(t, object); if (!note) @@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix) } } - t = init_notes_check("merge"); + t = init_notes_check("merge", NOTES_INIT_WRITABLE); strbuf_addf(&msg, "notes: Merged notes from %s into %s", remote_ref.buf, default_notes_ref()); @@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_notes_remove_usage, 0); - t = init_notes_check("remove"); + t = init_notes_check("remove", NOTES_INIT_WRITABLE); if (!argc && !from_stdin) { retval = remove_one_note(t, "HEAD", flag); @@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_prune_usage, options); } - t = init_notes_check("prune"); + t = init_notes_check("prune", NOTES_INIT_WRITABLE); prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) | (show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) ); diff --git a/notes-cache.c b/notes-cache.c index c4e9bb7..5dfc5cb 100644 --- a/notes-cache.c +++ b/notes-cache.c @@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name, const char *validity) { struct strbuf ref = STRBUF_INIT; - int flags = 0; + int flags = NOTES_INIT_WRITABLE; memset(c, 0, sizeof(*c)); c->validity = xstrdup(validity); strbuf_addf(&ref, "refs/notes/%s", name); if (!notes_cache_match_validity(ref.buf, validity)) - flags = NOTES_INIT_EMPTY; + flags |= NOTES_INIT_EMPTY; init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags); strbuf_release(&ref); } @@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c) unsigned char tree_sha1[20]; unsigned char commit_sha1[20]; - if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref) + if (!c || !c->tree.initialized || !c->tree.update_ref || + !*c->tree.update_ref) return -1; if (!c->tree.dirty) return 0; @@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c) if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL, commit_sha1, NULL, NULL) < 0) return -1; - if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL, - 0, UPDATE_REFS_QUIET_ON_ERR) < 0) + if (update_ref("update notes cache", c->tree.update_ref, commit_sha1, + NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0) return -1; return 0; diff --git a/notes-utils.c b/notes-utils.c index ccbf073..f65edba 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg) if (!t) t = &default_notes_tree; - if (!t->initialized || !t->ref || !*t->ref) + if (!t->initialized || !t->update_ref || !*t->update_ref) die(_("Cannot commit uninitialized/unreferenced notes tree")); if (!t->dirty) return; /* don't have to commit an unchanged tree */ @@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg) create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1); strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */ - update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, + update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); strbuf_release(&buf); @@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd) free(c); return NULL; } - c->trees = load_notes_trees(c->refs); + c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE); string_list_clear(c->refs, 0); free(c->refs); return c; 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; 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)) return; + if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1)) + die("Cannot use notes ref %s", notes_ref); if (get_tree_entry(object_sha1, "", sha1, &mode)) die("Failed to read notes tree referenced by %s (%s)", notes_ref, sha1_to_hex(object_sha1)); @@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, load_subtree(t, &root_tree, t->root, 0); } -struct notes_tree **load_notes_trees(struct string_list *refs) +struct notes_tree **load_notes_trees(struct string_list *refs, int flags) { struct string_list_item *item; int counter = 0; @@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs) trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *)); for_each_string_list_item(item, refs) { struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree)); - init_notes(t, item->string, combine_notes_ignore, 0); + init_notes(t, item->string, combine_notes_ignore, flags); trees[counter++] = t; } trees[counter] = NULL; @@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt) item->string); } - display_notes_trees = load_notes_trees(&display_notes_refs); + display_notes_trees = load_notes_trees(&display_notes_refs, 0); string_list_clear(&display_notes_refs, 0); } 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 + * flag, in which case only strict ref names can be used. + */ +#define NOTES_INIT_WRITABLE 2 + +/* * Initialize the given notes_tree with the notes tree structure at the given * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment * variable is used, and if that is missing, the default notes ref is used @@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1, * Load the notes tree from each ref listed in 'refs'. The output is * an array of notes_tree*, terminated by a NULL. */ -struct notes_tree **load_notes_trees(struct string_list *refs); +struct notes_tree **load_notes_trees(struct string_list *refs, int flags); /* * Add all refs that match 'glob' to the 'list'. -- 2.4.3.2.gf0a024e.dirty ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3] notes: Allow committish expressions as notes ref 2015-07-09 2:48 ` [PATCH v3] " Mike Hommey @ 2015-07-10 1:03 ` Johan Herland 2015-07-10 1:28 ` [PATCH v4] notes: Allow treeish " Mike Hommey 0 siblings, 1 reply; 22+ messages in thread From: Johan Herland @ 2015-07-10 1:03 UTC (permalink / raw) To: Mike Hommey; +Cc: Git mailing list, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4] notes: Allow treeish expressions as notes ref 2015-07-10 1:03 ` Johan Herland @ 2015-07-10 1:28 ` Mike Hommey 2015-07-10 7:16 ` Johan Herland 0 siblings, 1 reply; 22+ messages in thread From: Mike Hommey @ 2015-07-10 1:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johan Herland 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. 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}. Signed-off-by: Mike Hommey <mh@glandium.org> Reviewed-by: Johan Herland <johan@herland.net> --- 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; + if (!starts_with(ref, "refs/notes/")) die("Refusing to %s notes in %s (outside of refs/notes/)", - subcommand, t->ref); + subcommand, ref); return t; } @@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_list_usage, options); } - t = init_notes_check("list"); + t = init_notes_check("list", 0); if (argc) { if (get_sha1(argv[0], object)) die(_("Failed to resolve '%s' as a valid ref."), argv[0]); @@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("add"); + t = init_notes_check("add", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("copy"); + t = init_notes_check("copy", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check(argv[0]); + t = init_notes_check(argv[0], NOTES_INIT_WRITABLE); note = get_note(t, object); prepare_note_data(object, &d, edit ? note : NULL); @@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("show"); + t = init_notes_check("show", 0); note = get_note(t, object); if (!note) @@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix) } } - t = init_notes_check("merge"); + t = init_notes_check("merge", NOTES_INIT_WRITABLE); strbuf_addf(&msg, "notes: Merged notes from %s into %s", remote_ref.buf, default_notes_ref()); @@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_notes_remove_usage, 0); - t = init_notes_check("remove"); + t = init_notes_check("remove", NOTES_INIT_WRITABLE); if (!argc && !from_stdin) { retval = remove_one_note(t, "HEAD", flag); @@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_prune_usage, options); } - t = init_notes_check("prune"); + t = init_notes_check("prune", NOTES_INIT_WRITABLE); prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) | (show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) ); diff --git a/notes-cache.c b/notes-cache.c index c4e9bb7..5dfc5cb 100644 --- a/notes-cache.c +++ b/notes-cache.c @@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name, const char *validity) { struct strbuf ref = STRBUF_INIT; - int flags = 0; + int flags = NOTES_INIT_WRITABLE; memset(c, 0, sizeof(*c)); c->validity = xstrdup(validity); strbuf_addf(&ref, "refs/notes/%s", name); if (!notes_cache_match_validity(ref.buf, validity)) - flags = NOTES_INIT_EMPTY; + flags |= NOTES_INIT_EMPTY; init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags); strbuf_release(&ref); } @@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c) unsigned char tree_sha1[20]; unsigned char commit_sha1[20]; - if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref) + if (!c || !c->tree.initialized || !c->tree.update_ref || + !*c->tree.update_ref) return -1; if (!c->tree.dirty) return 0; @@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c) if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL, commit_sha1, NULL, NULL) < 0) return -1; - if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL, - 0, UPDATE_REFS_QUIET_ON_ERR) < 0) + if (update_ref("update notes cache", c->tree.update_ref, commit_sha1, + NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0) return -1; return 0; diff --git a/notes-utils.c b/notes-utils.c index ccbf073..f65edba 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg) if (!t) t = &default_notes_tree; - if (!t->initialized || !t->ref || !*t->ref) + if (!t->initialized || !t->update_ref || !*t->update_ref) die(_("Cannot commit uninitialized/unreferenced notes tree")); if (!t->dirty) return; /* don't have to commit an unchanged tree */ @@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg) create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1); strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */ - update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, + update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); strbuf_release(&buf); @@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd) free(c); return NULL; } - c->trees = load_notes_trees(c->refs); + c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE); string_list_clear(c->refs, 0); free(c->refs); return c; diff --git a/notes.c b/notes.c index df08209..dc95ebb 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; 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_treeish(notes_ref, object_sha1)) return; + if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1)) + die("Cannot use notes ref %s", notes_ref); if (get_tree_entry(object_sha1, "", sha1, &mode)) die("Failed to read notes tree referenced by %s (%s)", notes_ref, sha1_to_hex(object_sha1)); @@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, load_subtree(t, &root_tree, t->root, 0); } -struct notes_tree **load_notes_trees(struct string_list *refs) +struct notes_tree **load_notes_trees(struct string_list *refs, int flags) { struct string_list_item *item; int counter = 0; @@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs) trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *)); for_each_string_list_item(item, refs) { struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree)); - init_notes(t, item->string, combine_notes_ignore, 0); + init_notes(t, item->string, combine_notes_ignore, flags); trees[counter++] = t; } trees[counter] = NULL; @@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt) item->string); } - display_notes_trees = load_notes_trees(&display_notes_refs); + display_notes_trees = load_notes_trees(&display_notes_refs, 0); string_list_clear(&display_notes_refs, 0); } diff --git a/notes.h b/notes.h index 2a3f923..e5d67fd 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 treeish. The notes tree can however be made writable with this flag, + * in which case only strict ref names can be used. + */ +#define NOTES_INIT_WRITABLE 2 + +/* * Initialize the given notes_tree with the notes tree structure at the given * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment * variable is used, and if that is missing, the default notes ref is used @@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1, * Load the notes tree from each ref listed in 'refs'. The output is * an array of notes_tree*, terminated by a NULL. */ -struct notes_tree **load_notes_trees(struct string_list *refs); +struct notes_tree **load_notes_trees(struct string_list *refs, int flags); /* * Add all refs that match 'glob' to the 'list'. -- 2.4.3.3.g2dde64c ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4] notes: Allow treeish expressions as notes ref 2015-07-10 1:28 ` [PATCH v4] notes: Allow treeish " Mike Hommey @ 2015-07-10 7:16 ` Johan Herland 2015-07-10 7:22 ` Mike Hommey 2015-07-10 8:39 ` [PATCH] " Mike Hommey 0 siblings, 2 replies; 22+ messages in thread From: Johan Herland @ 2015-07-10 7:16 UTC (permalink / raw) To: Mike Hommey; +Cc: Junio C Hamano, Git mailing list On Fri, Jul 10, 2015 at 3:28 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 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. > > 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}. Looks good. However, on a second pass I noticed that the patch comes with no tests. I'd like at least a couple of tests thrown in there to verify correctness; e.g. reading notes from refs/notes/commits^{tree} shall succeed, and trying to write notes to refs/notes/commits^{tree} shall fail. ...Johan -- Johan Herland, <johan@herland.net> www.herland.net ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4] notes: Allow treeish expressions as notes ref 2015-07-10 7:16 ` Johan Herland @ 2015-07-10 7:22 ` Mike Hommey 2015-07-10 8:39 ` [PATCH] " Mike Hommey 1 sibling, 0 replies; 22+ messages in thread From: Mike Hommey @ 2015-07-10 7:22 UTC (permalink / raw) To: Johan Herland; +Cc: Junio C Hamano, Git mailing list On Fri, Jul 10, 2015 at 09:16:16AM +0200, Johan Herland wrote: > On Fri, Jul 10, 2015 at 3:28 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 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. > > > > 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}. > > Looks good. However, on a second pass I noticed that the patch comes > with no tests. I'd like at least a couple of tests thrown in there to verify > correctness; e.g. reading notes from refs/notes/commits^{tree} shall > succeed, and trying to write notes to refs/notes/commits^{tree} shall fail. Fair enough. Will update. Mike ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] notes: Allow treeish expressions as notes ref 2015-07-10 7:16 ` Johan Herland 2015-07-10 7:22 ` Mike Hommey @ 2015-07-10 8:39 ` Mike Hommey 2015-07-13 16:35 ` Junio C Hamano 2015-07-13 21:19 ` Junio C Hamano 1 sibling, 2 replies; 22+ messages in thread From: Mike Hommey @ 2015-07-10 8:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Johan Herland 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. 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}. Signed-off-by: Mike Hommey <mh@glandium.org> --- builtin/notes.c | 29 ++++++++++++++++------------- notes-cache.c | 11 ++++++----- notes-utils.c | 6 +++--- notes.c | 11 +++++++---- notes.h | 10 +++++++++- t/t3301-notes.sh | 10 ++++++++++ 6 files changed, 51 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; + if (!starts_with(ref, "refs/notes/")) die("Refusing to %s notes in %s (outside of refs/notes/)", - subcommand, t->ref); + subcommand, ref); return t; } @@ -359,7 +362,7 @@ static int list(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_list_usage, options); } - t = init_notes_check("list"); + t = init_notes_check("list", 0); if (argc) { if (get_sha1(argv[0], object)) die(_("Failed to resolve '%s' as a valid ref."), argv[0]); @@ -419,7 +422,7 @@ static int add(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("add"); + t = init_notes_check("add", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -510,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("copy"); + t = init_notes_check("copy", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -588,7 +591,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check(argv[0]); + t = init_notes_check(argv[0], NOTES_INIT_WRITABLE); note = get_note(t, object); prepare_note_data(object, &d, edit ? note : NULL); @@ -651,7 +654,7 @@ static int show(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("show"); + t = init_notes_check("show", 0); note = get_note(t, object); if (!note) @@ -812,7 +815,7 @@ static int merge(int argc, const char **argv, const char *prefix) } } - t = init_notes_check("merge"); + t = init_notes_check("merge", NOTES_INIT_WRITABLE); strbuf_addf(&msg, "notes: Merged notes from %s into %s", remote_ref.buf, default_notes_ref()); @@ -878,7 +881,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_notes_remove_usage, 0); - t = init_notes_check("remove"); + t = init_notes_check("remove", NOTES_INIT_WRITABLE); if (!argc && !from_stdin) { retval = remove_one_note(t, "HEAD", flag); @@ -920,7 +923,7 @@ static int prune(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_prune_usage, options); } - t = init_notes_check("prune"); + t = init_notes_check("prune", NOTES_INIT_WRITABLE); prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) | (show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) ); diff --git a/notes-cache.c b/notes-cache.c index c4e9bb7..5dfc5cb 100644 --- a/notes-cache.c +++ b/notes-cache.c @@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name, const char *validity) { struct strbuf ref = STRBUF_INIT; - int flags = 0; + int flags = NOTES_INIT_WRITABLE; memset(c, 0, sizeof(*c)); c->validity = xstrdup(validity); strbuf_addf(&ref, "refs/notes/%s", name); if (!notes_cache_match_validity(ref.buf, validity)) - flags = NOTES_INIT_EMPTY; + flags |= NOTES_INIT_EMPTY; init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags); strbuf_release(&ref); } @@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c) unsigned char tree_sha1[20]; unsigned char commit_sha1[20]; - if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref) + if (!c || !c->tree.initialized || !c->tree.update_ref || + !*c->tree.update_ref) return -1; if (!c->tree.dirty) return 0; @@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c) if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL, commit_sha1, NULL, NULL) < 0) return -1; - if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL, - 0, UPDATE_REFS_QUIET_ON_ERR) < 0) + if (update_ref("update notes cache", c->tree.update_ref, commit_sha1, + NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0) return -1; return 0; diff --git a/notes-utils.c b/notes-utils.c index ccbf073..f65edba 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg) if (!t) t = &default_notes_tree; - if (!t->initialized || !t->ref || !*t->ref) + if (!t->initialized || !t->update_ref || !*t->update_ref) die(_("Cannot commit uninitialized/unreferenced notes tree")); if (!t->dirty) return; /* don't have to commit an unchanged tree */ @@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg) create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1); strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */ - update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, + update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); strbuf_release(&buf); @@ -130,7 +130,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd) free(c); return NULL; } - c->trees = load_notes_trees(c->refs); + c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE); string_list_clear(c->refs, 0); free(c->refs); return c; diff --git a/notes.c b/notes.c index 2be4d7f..25ed506 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; 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_treeish(notes_ref, object_sha1)) return; + if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1)) + die("Cannot use notes ref %s", notes_ref); if (get_tree_entry(object_sha1, "", sha1, &mode)) die("Failed to read notes tree referenced by %s (%s)", notes_ref, sha1_to_hex(object_sha1)); @@ -1023,7 +1026,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, load_subtree(t, &root_tree, t->root, 0); } -struct notes_tree **load_notes_trees(struct string_list *refs) +struct notes_tree **load_notes_trees(struct string_list *refs, int flags) { struct string_list_item *item; int counter = 0; @@ -1031,7 +1034,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs) trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *)); for_each_string_list_item(item, refs) { struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree)); - init_notes(t, item->string, combine_notes_ignore, 0); + init_notes(t, item->string, combine_notes_ignore, flags); trees[counter++] = t; } trees[counter] = NULL; @@ -1067,7 +1070,7 @@ void init_display_notes(struct display_notes_opt *opt) item->string); } - display_notes_trees = load_notes_trees(&display_notes_refs); + display_notes_trees = load_notes_trees(&display_notes_refs, 0); string_list_clear(&display_notes_refs, 0); } diff --git a/notes.h b/notes.h index 2a3f923..e5d67fd 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 treeish. The notes tree can however be made writable with this flag, + * in which case only strict ref names can be used. + */ +#define NOTES_INIT_WRITABLE 2 + +/* * Initialize the given notes_tree with the notes tree structure at the given * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment * variable is used, and if that is missing, the default notes ref is used @@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1, * Load the notes tree from each ref listed in 'refs'. The output is * an array of notes_tree*, terminated by a NULL. */ -struct notes_tree **load_notes_trees(struct string_list *refs); +struct notes_tree **load_notes_trees(struct string_list *refs, int flags); /* * Add all refs that match 'glob' to the 'list'. diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 8cffd35..06b4584 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -83,6 +83,16 @@ test_expect_success 'edit existing notes' ' test_must_fail git notes show HEAD^ ' +test_expect_success 'show notes from treeish' ' + test "b3" = "$(git notes --ref commits^{tree} show)" && + test "b4" = "$(git notes --ref commits@{1} show)" +' + +test_expect_success 'cannot edit notes from non-ref' ' + test_must_fail git notes --ref commits^{tree} edit && + test_must_fail git notes --ref commits@{1} edit +' + test_expect_success 'cannot "git notes add -m" where notes already exists' ' test_must_fail git notes add -m "b2" && test_path_is_missing .git/NOTES_EDITMSG && -- 2.4.3.3.g2dde64c.dirty ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Allow treeish expressions as notes ref 2015-07-10 8:39 ` [PATCH] " Mike Hommey @ 2015-07-13 16:35 ` Junio C Hamano 2015-07-13 20:53 ` Mike Hommey 2015-10-08 2:50 ` Mike Hommey 2015-07-13 21:19 ` Junio C Hamano 1 sibling, 2 replies; 22+ messages in thread From: Junio C Hamano @ 2015-07-13 16:35 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Johan Herland Mike Hommey <mh@glandium.org> 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@glandium.org> Signed-off-by: Junio C Hamano <gitster@pobox.com> 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@glandium.org> notes: allow treeish expressions as notes ref Notes (amlog): Message-Id: <1436517551-12172-1-git-send-email-mh@glandium.org> 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@glandium.org> 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@glandium.org> 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@glandium.org> notes: allow treeish expressions as notes ref Notes (amlog@{0}): Message-Id: <1436517551-12172-1-git-send-email-mh@glandium.org> 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Allow treeish expressions as notes ref 2015-07-13 16:35 ` Junio C Hamano @ 2015-07-13 20:53 ` Mike Hommey 2015-07-13 21:15 ` Junio C Hamano 2015-10-08 2:50 ` Mike Hommey 1 sibling, 1 reply; 22+ messages in thread From: Mike Hommey @ 2015-07-13 20:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johan Herland On Mon, Jul 13, 2015 at 09:35:40AM -0700, Junio C Hamano wrote: > Mike Hommey <mh@glandium.org> 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. My original motivation for this change was for init_notes() to take a committish so that I can feed it with one in a C program that uses the notes API. So from that perspective, that command lines can now use them is a side effect. > 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@glandium.org> > Signed-off-by: Junio C Hamano <gitster@pobox.com> WFM. > 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. How would you reconcile that with the usual "there are only a couple commits with a note in the hundreds you make me display"? > - 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. Is this something you want to be figured before merging this patch? Mike ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Allow treeish expressions as notes ref 2015-07-13 20:53 ` Mike Hommey @ 2015-07-13 21:15 ` Junio C Hamano 0 siblings, 0 replies; 22+ messages in thread From: Junio C Hamano @ 2015-07-13 21:15 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Johan Herland Mike Hommey <mh@glandium.org> writes: >> - 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. > > How would you reconcile that with the usual "there are only a couple > commits with a note in the hundreds you make me display"? I am talking about the difference between "a tree exists (which may lack notes for a given commit)" and "a tree does not even exist in the first place". This patch removed "a tree exists but that is not a ref so we silently ignore", but I do not know if that change alone covers everything---do you? >> - 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. > > Is this something you want to be figured before merging this patch? Depends on the definition of 'merging'. I queued this one on 'pu', and have no intention to merge it down to 'master' by the end of this month; in the meantime either incremental or replacement refinement can certainly address that inconsistency I'd hope ;-) Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Allow treeish expressions as notes ref 2015-07-13 16:35 ` Junio C Hamano 2015-07-13 20:53 ` Mike Hommey @ 2015-10-08 2:50 ` Mike Hommey 2015-10-08 2:54 ` [PATCH v6] notes: allow " Mike Hommey 2015-10-08 19:03 ` [PATCH] notes: Allow " Junio C Hamano 1 sibling, 2 replies; 22+ messages in thread From: Mike Hommey @ 2015-10-08 2:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johan Herland On Mon, Jul 13, 2015 at 09:35:40AM -0700, Junio C Hamano wrote: > 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. After refreshing the patch against current "next", it appears that there is such a distinction: $ ./git-log --notes=fdsfgsfdg HEAD^! --pretty=short warning: notes ref refs/notes/fdsfgsfdg is invalid commit e5b68b2e879608d881c2e3600ce84962fcdefc88 Author: Mike Hommey <mh@glandium.org> notes: allow treeish expressions as notes ref $ ./git-log --notes=foo HEAD^! --pretty=short commit e5b68b2e879608d881c2e3600ce84962fcdefc88 Author: Mike Hommey <mh@glandium.org> notes: allow treeish expressions as notes ref (I do have a refs/notes/foo ref) > - 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. Currently, it warns and doesn't fail. Cheers, Mike ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6] notes: allow treeish expressions as notes ref 2015-10-08 2:50 ` Mike Hommey @ 2015-10-08 2:54 ` Mike Hommey 2015-10-08 19:03 ` [PATCH] notes: Allow " Junio C Hamano 1 sibling, 0 replies; 22+ messages in thread From: Mike Hommey @ 2015-10-08 2:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johan Herland, Jacob Keller 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@glandium.org> --- I'm really not satisfied with the documentation change, but I don't have a better idea :(. It didn't feel like Documentation/git-notes.txt needed an update. Documentation/pretty-options.txt | 8 ++++---- builtin/notes.c | 29 ++++++++++++++++------------- notes-cache.c | 11 ++++++----- notes-utils.c | 6 +++--- notes.c | 11 +++++++---- notes.h | 10 +++++++++- t/t3301-notes.sh | 10 ++++++++++ 7 files changed, 55 insertions(+), 30 deletions(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index 4b659ac..54b88b6 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -43,7 +43,7 @@ people using 80-column terminals. commit may be copied to the output. ifndef::git-rev-list[] ---notes[=<ref>]:: +--notes[=<treeish>]:: Show the notes (see linkgit:git-notes[1]) that annotate the commit, when showing the commit log message. This is the default for `git log`, `git show` and `git whatchanged` commands when @@ -54,8 +54,8 @@ By default, the notes shown are from the notes refs listed in the 'core.notesRef' and 'notes.displayRef' variables (or corresponding environment overrides). See linkgit:git-config[1] for more details. + -With an optional '<ref>' argument, show this notes ref instead of the -default notes ref(s). The ref specifies the full refname when it begins +With an optional '<treeish>' argument, use the treeish to find the notes +to display. The treeish can specify the full refname when it begins with `refs/notes/`; when it begins with `notes/`, `refs/` and otherwise `refs/notes/` is prefixed to form a full name of the ref. + @@ -71,7 +71,7 @@ being displayed. Examples: "--notes=foo" will show only notes from "--notes --notes=foo --no-notes --notes=bar" will only show notes from "refs/notes/bar". ---show-notes[=<ref>]:: +--show-notes[=<treeish>]:: --[no-]standard-notes:: These options are deprecated. Use the above --notes/--no-notes options instead. diff --git a/builtin/notes.c b/builtin/notes.c index 3608c64..6371d53 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -286,7 +286,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; } @@ -329,15 +329,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; + if (!starts_with(ref, "refs/notes/")) die("Refusing to %s notes in %s (outside of refs/notes/)", - subcommand, t->ref); + subcommand, ref); return t; } @@ -360,7 +363,7 @@ static int list(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_list_usage, options); } - t = init_notes_check("list"); + t = init_notes_check("list", 0); if (argc) { if (get_sha1(argv[0], object)) die(_("Failed to resolve '%s' as a valid ref."), argv[0]); @@ -420,7 +423,7 @@ static int add(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("add"); + t = init_notes_check("add", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -511,7 +514,7 @@ static int copy(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("copy"); + t = init_notes_check("copy", NOTES_INIT_WRITABLE); note = get_note(t, object); if (note) { @@ -589,7 +592,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check(argv[0]); + t = init_notes_check(argv[0], NOTES_INIT_WRITABLE); note = get_note(t, object); prepare_note_data(object, &d, edit ? note : NULL); @@ -652,7 +655,7 @@ static int show(int argc, const char **argv, const char *prefix) if (get_sha1(object_ref, object)) die(_("Failed to resolve '%s' as a valid ref."), object_ref); - t = init_notes_check("show"); + t = init_notes_check("show", 0); note = get_note(t, object); if (!note) @@ -809,7 +812,7 @@ static int merge(int argc, const char **argv, const char *prefix) expand_notes_ref(&remote_ref); o.remote_ref = remote_ref.buf; - t = init_notes_check("merge"); + t = init_notes_check("merge", NOTES_INIT_WRITABLE); if (strategy) { if (parse_notes_merge_strategy(strategy, &o.strategy)) { @@ -901,7 +904,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_notes_remove_usage, 0); - t = init_notes_check("remove"); + t = init_notes_check("remove", NOTES_INIT_WRITABLE); if (!argc && !from_stdin) { retval = remove_one_note(t, "HEAD", flag); @@ -943,7 +946,7 @@ static int prune(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_prune_usage, options); } - t = init_notes_check("prune"); + t = init_notes_check("prune", NOTES_INIT_WRITABLE); prune_notes(t, (verbose ? NOTES_PRUNE_VERBOSE : 0) | (show_only ? NOTES_PRUNE_VERBOSE|NOTES_PRUNE_DRYRUN : 0) ); diff --git a/notes-cache.c b/notes-cache.c index c4e9bb7..5dfc5cb 100644 --- a/notes-cache.c +++ b/notes-cache.c @@ -32,14 +32,14 @@ void notes_cache_init(struct notes_cache *c, const char *name, const char *validity) { struct strbuf ref = STRBUF_INIT; - int flags = 0; + int flags = NOTES_INIT_WRITABLE; memset(c, 0, sizeof(*c)); c->validity = xstrdup(validity); strbuf_addf(&ref, "refs/notes/%s", name); if (!notes_cache_match_validity(ref.buf, validity)) - flags = NOTES_INIT_EMPTY; + flags |= NOTES_INIT_EMPTY; init_notes(&c->tree, ref.buf, combine_notes_overwrite, flags); strbuf_release(&ref); } @@ -49,7 +49,8 @@ int notes_cache_write(struct notes_cache *c) unsigned char tree_sha1[20]; unsigned char commit_sha1[20]; - if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref) + if (!c || !c->tree.initialized || !c->tree.update_ref || + !*c->tree.update_ref) return -1; if (!c->tree.dirty) return 0; @@ -59,8 +60,8 @@ int notes_cache_write(struct notes_cache *c) if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL, commit_sha1, NULL, NULL) < 0) return -1; - if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL, - 0, UPDATE_REFS_QUIET_ON_ERR) < 0) + if (update_ref("update notes cache", c->tree.update_ref, commit_sha1, + NULL, 0, UPDATE_REFS_QUIET_ON_ERR) < 0) return -1; return 0; diff --git a/notes-utils.c b/notes-utils.c index 299e34b..24a3361 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -37,7 +37,7 @@ void commit_notes(struct notes_tree *t, const char *msg) if (!t) t = &default_notes_tree; - if (!t->initialized || !t->ref || !*t->ref) + if (!t->initialized || !t->update_ref || !*t->update_ref) die(_("Cannot commit uninitialized/unreferenced notes tree")); if (!t->dirty) return; /* don't have to commit an unchanged tree */ @@ -48,7 +48,7 @@ void commit_notes(struct notes_tree *t, const char *msg) create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1); strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */ - update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, + update_ref(buf.buf, t->update_ref, commit_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); strbuf_release(&buf); @@ -148,7 +148,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd) free(c); return NULL; } - c->trees = load_notes_trees(c->refs); + c->trees = load_notes_trees(c->refs, NOTES_INIT_WRITABLE); string_list_clear(c->refs, 0); free(c->refs); return c; diff --git a/notes.c b/notes.c index eacd2a6..6ef347c 100644 --- a/notes.c +++ b/notes.c @@ -1008,13 +1008,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; 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_treeish(notes_ref, object_sha1)) return; + if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, object_sha1)) + die("Cannot use notes ref %s", notes_ref); if (get_tree_entry(object_sha1, "", sha1, &mode)) die("Failed to read notes tree referenced by %s (%s)", notes_ref, sha1_to_hex(object_sha1)); @@ -1024,7 +1027,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, load_subtree(t, &root_tree, t->root, 0); } -struct notes_tree **load_notes_trees(struct string_list *refs) +struct notes_tree **load_notes_trees(struct string_list *refs, int flags) { struct string_list_item *item; int counter = 0; @@ -1032,7 +1035,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs) trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *)); for_each_string_list_item(item, refs) { struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree)); - init_notes(t, item->string, combine_notes_ignore, 0); + init_notes(t, item->string, combine_notes_ignore, flags); trees[counter++] = t; } trees[counter] = NULL; @@ -1068,7 +1071,7 @@ void init_display_notes(struct display_notes_opt *opt) item->string); } - display_notes_trees = load_notes_trees(&display_notes_refs); + display_notes_trees = load_notes_trees(&display_notes_refs, 0); string_list_clear(&display_notes_refs, 0); } diff --git a/notes.h b/notes.h index 2a3f923..e5d67fd 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 treeish. The notes tree can however be made writable with this flag, + * in which case only strict ref names can be used. + */ +#define NOTES_INIT_WRITABLE 2 + +/* * Initialize the given notes_tree with the notes tree structure at the given * ref. If given ref is NULL, the value of the $GIT_NOTES_REF environment * variable is used, and if that is missing, the default notes ref is used @@ -276,7 +284,7 @@ void format_display_notes(const unsigned char *object_sha1, * Load the notes tree from each ref listed in 'refs'. The output is * an array of notes_tree*, terminated by a NULL. */ -struct notes_tree **load_notes_trees(struct string_list *refs); +struct notes_tree **load_notes_trees(struct string_list *refs, int flags); /* * Add all refs that match 'glob' to the 'list'. diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index cd70274..2d200fd 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -83,6 +83,16 @@ test_expect_success 'edit existing notes' ' test_must_fail git notes show HEAD^ ' +test_expect_success 'show notes from treeish' ' + test "b3" = "$(git notes --ref commits^{tree} show)" && + test "b4" = "$(git notes --ref commits@{1} show)" +' + +test_expect_success 'cannot edit notes from non-ref' ' + test_must_fail git notes --ref commits^{tree} edit && + test_must_fail git notes --ref commits@{1} edit +' + test_expect_success 'cannot "git notes add -m" where notes already exists' ' test_must_fail git notes add -m "b2" && test_path_is_missing .git/NOTES_EDITMSG && -- 2.6.1.146.ge5b68b2 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Allow treeish expressions as notes ref 2015-10-08 2:50 ` Mike Hommey 2015-10-08 2:54 ` [PATCH v6] notes: allow " Mike Hommey @ 2015-10-08 19:03 ` Junio C Hamano 1 sibling, 0 replies; 22+ messages in thread From: Junio C Hamano @ 2015-10-08 19:03 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Johan Herland Mike Hommey <mh@glandium.org> writes: > After refreshing the patch against current "next", it appears that > there is such a distinction: > > $ ./git-log --notes=fdsfgsfdg HEAD^! --pretty=short > warning: notes ref refs/notes/fdsfgsfdg is invalid > commit e5b68b2e879608d881c2e3600ce84962fcdefc88 > Author: Mike Hommey <mh@glandium.org> > > notes: allow treeish expressions as notes ref > > $ ./git-log --notes=foo HEAD^! --pretty=short > commit e5b68b2e879608d881c2e3600ce84962fcdefc88 > Author: Mike Hommey <mh@glandium.org> > > notes: allow treeish expressions as notes ref Good; thanks for checking. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Allow treeish expressions as notes ref 2015-07-10 8:39 ` [PATCH] " Mike Hommey 2015-07-13 16:35 ` Junio C Hamano @ 2015-07-13 21:19 ` Junio C Hamano 1 sibling, 0 replies; 22+ messages in thread From: Junio C Hamano @ 2015-07-13 21:19 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Johan Herland Mike Hommey <mh@glandium.org> 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. > > 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}. > > Signed-off-by: Mike Hommey <mh@glandium.org> > --- > builtin/notes.c | 29 ++++++++++++++++------------- > notes-cache.c | 11 ++++++----- > notes-utils.c | 6 +++--- > notes.c | 11 +++++++---- > notes.h | 10 +++++++++- > t/t3301-notes.sh | 10 ++++++++++ > 6 files changed, 51 insertions(+), 26 deletions(-) At least Documentation/pretty-options.txt needs to be updated, as it explicitly requests you to feed a ref, but you'd want to tell the users that you loosened it. I suspect Documentation/git-notes.txt may also need adjustment as that involves writing side, but I didn't look very carefully. Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() 2015-06-17 1:15 [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() Mike Hommey 2015-06-17 3:17 ` Junio C Hamano @ 2015-06-17 3:22 ` Jeff King 2015-06-17 9:02 ` Mike Hommey 1 sibling, 1 reply; 22+ messages in thread From: Jeff King @ 2015-06-17 3:22 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Junio C Hamano, Johan Herland On Wed, Jun 17, 2015 at 10:15:31AM +0900, Mike Hommey wrote: > 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. > > 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}, although > I haven't checked whether that's the case for all of them. What about expand_notes_ref? We call that on the argument to "--notes". I guess it is OK to expand "foo@{1}" into "refs/notes/foo@{1}", but what about other more arcane syntaxes, like ":/"? In a sense that is weirdly broken already: $ git log --notes=:/foo >/dev/null warning: notes ref refs/notes/:/foo is invalid but I wonder if we should be making expand_notes_ref a little more careful as part of the same topic. -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() 2015-06-17 3:22 ` [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() Jeff King @ 2015-06-17 9:02 ` Mike Hommey 2015-06-17 16:35 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Mike Hommey @ 2015-06-17 9:02 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Johan Herland On Tue, Jun 16, 2015 at 11:22:31PM -0400, Jeff King wrote: > On Wed, Jun 17, 2015 at 10:15:31AM +0900, Mike Hommey wrote: > > > 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. > > > > 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}, although > > I haven't checked whether that's the case for all of them. > > What about expand_notes_ref? We call that on the argument to "--notes". > I guess it is OK to expand "foo@{1}" into "refs/notes/foo@{1}", but what > about other more arcane syntaxes, like ":/"? > > In a sense that is weirdly broken already: > > $ git log --notes=:/foo >/dev/null > warning: notes ref refs/notes/:/foo is invalid > > but I wonder if we should be making expand_notes_ref a little more > careful as part of the same topic. Interestingly, now that I look, there's also this: https://github.com/git/git/blob/master/notes-cache.c#L40 that doesn't use expand_notes_ref, but it's apparently only used in userdiff_get_textconv, and I'm not sure why. Mike ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() 2015-06-17 9:02 ` Mike Hommey @ 2015-06-17 16:35 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2015-06-17 16:35 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Junio C Hamano, Johan Herland On Wed, Jun 17, 2015 at 06:02:46PM +0900, Mike Hommey wrote: > > In a sense that is weirdly broken already: > > > > $ git log --notes=:/foo >/dev/null > > warning: notes ref refs/notes/:/foo is invalid > > > > but I wonder if we should be making expand_notes_ref a little more > > careful as part of the same topic. > > Interestingly, now that I look, there's also this: > https://github.com/git/git/blob/master/notes-cache.c#L40 > > that doesn't use expand_notes_ref, but it's apparently only used in > userdiff_get_textconv, and I'm not sure why. notes-cache.c predates expand_notes_ref. I don't think it's a big deal. The implementation of notes-cache is an internal detail (the user interacts with it by setting diff.*.cacheTextConv, and then git handles the cache). -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2015-10-08 19:03 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-17 1:15 [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() Mike Hommey 2015-06-17 3:17 ` Junio C Hamano 2015-06-17 9:40 ` Mike Hommey 2015-06-17 15:18 ` Junio C Hamano 2015-06-17 16:35 ` Johan Herland 2015-07-08 10:21 ` [PATCH v2 - RFH] notes: Allow committish expressions as notes ref Mike Hommey 2015-07-09 2:48 ` [PATCH v3] " Mike Hommey 2015-07-10 1:03 ` Johan Herland 2015-07-10 1:28 ` [PATCH v4] notes: Allow treeish " Mike Hommey 2015-07-10 7:16 ` Johan Herland 2015-07-10 7:22 ` Mike Hommey 2015-07-10 8:39 ` [PATCH] " Mike Hommey 2015-07-13 16:35 ` Junio C Hamano 2015-07-13 20:53 ` Mike Hommey 2015-07-13 21:15 ` Junio C Hamano 2015-10-08 2:50 ` Mike Hommey 2015-10-08 2:54 ` [PATCH v6] notes: allow " Mike Hommey 2015-10-08 19:03 ` [PATCH] notes: Allow " Junio C Hamano 2015-07-13 21:19 ` Junio C Hamano 2015-06-17 3:22 ` [PATCH] notes: Use get_sha1_committish instead of read_ref in init_notes() Jeff King 2015-06-17 9:02 ` Mike Hommey 2015-06-17 16:35 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).