Message ID | pull.944.git.1619691880696.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [GSOC] ref-filter: add %(notes) format atom | expand |
Am 29.04.21 um 12:24 schrieb ZheNing Hu via GitGitGadget: > From: ZheNing Hu <adlternative@gmail.com> > > Note that `--pretty="%N"` can view notes related > to the commit. So add `%(notes)` to ref-filter > seem is a good choice. This atom `%(notes)` view > the notes associated with the ref, and support > dereference. I can't judge the usefulness of this feature because I don't use notes. > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > [GSOC] ref-filter: add %(notes) format atom > > An important step in the GSOC project Use ref-filter formats in git > cat-file is to integrate different format atoms into the ref-filter. > Olga and Hariom have also made a lot of efforts in this area. This is done to replace the custom format parser in git cat-file with ref-filter in order to reduce code duplication, right? > Currently, I noticed that there may be some format atoms in "pretty.c" > that have not been migrated to ref-filter, such as --pretty="%N", > --pretty="%(describe)". git cat-file doesn't support pretty format atoms, so I'm not sure I see the connection here. > So in this patch, I tried to migrate --pretty=%N to --format=%(notes). > > Hope this will be hopeful !!! > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-944%2Fadlternative%2Fformat-notes-atom-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-944/adlternative/format-notes-atom-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/944 > > Documentation/git-for-each-ref.txt | 4 ++++ > ref-filter.c | 31 ++++++++++++++++++++++++++++-- > t/t6300-for-each-ref.sh | 10 ++++++++++ > 3 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index 2ae2478de706..07f037a16e13 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -139,6 +139,9 @@ deltabase:: > given object, if it is stored as a delta. Otherwise it > expands to the null object name (all zeroes). > > +notes:: > + The notes associated with the ref. > + > upstream:: > The name of a local ref which can be considered ``upstream'' > from the displayed ref. Respects `:short`, `:lstrip` and > @@ -302,6 +305,7 @@ git for-each-ref --count=3 --sort='-*authordate' \ > Subject: %(*subject) > Date: %(*authordate) > Ref: %(*refname) > +Notes: %(*notes) > > %(*body) > ' 'refs/tags' > diff --git a/ref-filter.c b/ref-filter.c > index a0adb4551d87..42a5608a3056 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -23,6 +23,7 @@ > #include "worktree.h" > #include "hashmap.h" > #include "strvec.h" > +#include "run-command.h" > > static struct ref_msg { > const char *gone; > @@ -506,6 +507,7 @@ static struct { > { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser }, > { "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser }, > { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser }, > + { "notes", SOURCE_OTHER, FIELD_STR }, > { "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser }, > { "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser }, > { "numparent", SOURCE_OBJ, FIELD_ULONG }, > @@ -953,6 +955,24 @@ static int grab_oid(const char *name, const char *field, const struct object_id > return 0; > } > > +static int grab_notes(const struct object_id *oid, struct atom_value *v) > +{ > + struct child_process cmd = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + struct strbuf err = STRBUF_INIT; > + const char *args[] = { "notes", "show", NULL }; > + int ret; > + > + cmd.git_cmd = 1; > + strvec_pushv(&cmd.args, args); > + strvec_push(&cmd.args, oid_to_hex(oid)); > + ret = pipe_command(&cmd, NULL, 0, &out, 0, &err, 0); Nice prototype. Is it possible to avoid the overhead of calling git notes as an external process by calling load_display_notes() once at parse time and format_display_notes() for each item? Would it cause conflicts for code that uses ref-filter and handles notes already? > + strbuf_trim_trailing_newline(&out); > + v->s = strbuf_detach(&out, NULL); > + strbuf_release(&err); > + return ret; > +} > + > /* See grab_values */ > static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi) > { > @@ -975,8 +995,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_ > v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size); > } else if (!strcmp(name, "deltabase")) > v->s = xstrdup(oid_to_hex(&oi->delta_base_oid)); > - else if (deref) > - grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]); > + else if (deref) { > + if (!strcmp(name, "notes")) > + grab_notes(&oi->oid, v); > + else > + grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]); > + } > } > } > > @@ -1767,6 +1791,9 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) > continue; > } else if (!deref && grab_oid(name, "objectname", &ref->objectname, v, atom)) { > continue; > + } else if (!deref && !strcmp(name, "notes")) { > + grab_notes(&ref->objectname, v); > + continue; > } else if (!strcmp(name, "HEAD")) { > if (atom->u.head && !strcmp(ref->refname, atom->u.head)) > v->s = xstrdup("*");
René Scharfe <l.s.r@web.de> 于2021年5月2日周日 上午3:21写道: > > Am 29.04.21 um 12:24 schrieb ZheNing Hu via GitGitGadget: > > From: ZheNing Hu <adlternative@gmail.com> > > > > Note that `--pretty="%N"` can view notes related > > to the commit. So add `%(notes)` to ref-filter > > seem is a good choice. This atom `%(notes)` view > > the notes associated with the ref, and support > > dereference. > > I can't judge the usefulness of this feature because I don't use notes. > > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > --- > > [GSOC] ref-filter: add %(notes) format atom > > > > An important step in the GSOC project Use ref-filter formats in git > > cat-file is to integrate different format atoms into the ref-filter. > > Olga and Hariom have also made a lot of efforts in this area. > > This is done to replace the custom format parser in git cat-file with > ref-filter in order to reduce code duplication, right? > > > Currently, I noticed that there may be some format atoms in "pretty.c" > > that have not been migrated to ref-filter, such as --pretty="%N", > > --pretty="%(describe)". > > git cat-file doesn't support pretty format atoms, so I'm not sure I see > the connection here. > At present, `git cat-file --batch`, `git log --pretty`, and `git for-each-ref --format` lack consistency. The ambition of this project is to unify these different formats "%aN","%(authorname)" through an interface, maybe it's `ref-filter` or maybe a new one. It is estimated that many places in need to be replaced and rebuilt, and it not just to reduce code duplication. Note that `cat-file --batch` should be a superset of `ref-filter` and `--pretty`, because it supports all objects, while `ref-filter` only supports "commit" and "tag" two kinds of objects, and `--pretty` only supports "commit" one kind of object. So it may be reasonable to provide `%(notes)` to "commit","tag" objects in `cat-file --batch` in the future, now integrate them into ref-filter firstly. > > So in this patch, I tried to migrate --pretty=%N to --format=%(notes). > > > > Hope this will be hopeful !!! > > > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-944%2Fadlternative%2Fformat-notes-atom-v1 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-944/adlternative/format-notes-atom-v1 > > Pull-Request: https://github.com/gitgitgadget/git/pull/944 > > > > Documentation/git-for-each-ref.txt | 4 ++++ > > ref-filter.c | 31 ++++++++++++++++++++++++++++-- > > t/t6300-for-each-ref.sh | 10 ++++++++++ > > 3 files changed, 43 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > > index 2ae2478de706..07f037a16e13 100644 > > --- a/Documentation/git-for-each-ref.txt > > +++ b/Documentation/git-for-each-ref.txt > > @@ -139,6 +139,9 @@ deltabase:: > > given object, if it is stored as a delta. Otherwise it > > expands to the null object name (all zeroes). > > > > +notes:: > > + The notes associated with the ref. > > + > > upstream:: > > The name of a local ref which can be considered ``upstream'' > > from the displayed ref. Respects `:short`, `:lstrip` and > > @@ -302,6 +305,7 @@ git for-each-ref --count=3 --sort='-*authordate' \ > > Subject: %(*subject) > > Date: %(*authordate) > > Ref: %(*refname) > > +Notes: %(*notes) > > > > %(*body) > > ' 'refs/tags' > > diff --git a/ref-filter.c b/ref-filter.c > > index a0adb4551d87..42a5608a3056 100644 > > --- a/ref-filter.c > > +++ b/ref-filter.c > > @@ -23,6 +23,7 @@ > > #include "worktree.h" > > #include "hashmap.h" > > #include "strvec.h" > > +#include "run-command.h" > > > > static struct ref_msg { > > const char *gone; > > @@ -506,6 +507,7 @@ static struct { > > { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser }, > > { "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser }, > > { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser }, > > + { "notes", SOURCE_OTHER, FIELD_STR }, > > { "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser }, > > { "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser }, > > { "numparent", SOURCE_OBJ, FIELD_ULONG }, > > @@ -953,6 +955,24 @@ static int grab_oid(const char *name, const char *field, const struct object_id > > return 0; > > } > > > > +static int grab_notes(const struct object_id *oid, struct atom_value *v) > > +{ > > + struct child_process cmd = CHILD_PROCESS_INIT; > > + struct strbuf out = STRBUF_INIT; > > + struct strbuf err = STRBUF_INIT; > > + const char *args[] = { "notes", "show", NULL }; > > + int ret; > > + > > + cmd.git_cmd = 1; > > + strvec_pushv(&cmd.args, args); > > + strvec_push(&cmd.args, oid_to_hex(oid)); > > + ret = pipe_command(&cmd, NULL, 0, &out, 0, &err, 0); > > Nice prototype. Is it possible to avoid the overhead of calling git > notes as an external process by calling load_display_notes() once at > parse time and format_display_notes() for each item? Would it cause > conflicts for code that uses ref-filter and handles notes already? > This sounds feasible, I will try if it can work. Thanks! -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: > Note that `cat-file --batch` should be a superset of `ref-filter` and > `--pretty`, because > it supports all objects, while `ref-filter` only supports "commit" and > "tag" two kinds > of objects, and `--pretty` only supports "commit" one kind of object. What? A ref can point at any kind of objects, not necessarily commits and tags. %(objectname), %(objecttype), etc. obviously are applicable to any type of object. Another thing worth noting is that ref-filter needs to deal with traits that are not tied to any particular object, but to the ref itself, like who its upstream is and where it would be pushed to. > So it may be reasonable to provide `%(notes)` to "commit","tag" > objects in `cat-file --batch` > in the future, now integrate them into ref-filter firstly. And you can attach notes to objects of any type, not limited to commits and tags. >> > So in this patch, I tried to migrate --pretty=%N to --format=%(notes). What do you mean by "migrate"? Are you making both available? >> > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt >> > index 2ae2478de706..07f037a16e13 100644 >> > --- a/Documentation/git-for-each-ref.txt >> > +++ b/Documentation/git-for-each-ref.txt >> > @@ -139,6 +139,9 @@ deltabase:: >> > given object, if it is stored as a delta. Otherwise it >> > expands to the null object name (all zeroes). >> > >> > +notes:: >> > + The notes associated with the ref. "The notes associated with the object pointed at by the ref". A note is never associated with a ref. It can only associated with an object. If we are going to have %(notes), it may be natural to desire grabbing notes for the given object from specified notes ref, e.g. git for-each-ref --format="%(notes:amlog) %(subject)" zh/pretty-date-human may want to give the same kind of information as git show -s --format='%N %s' --notes=amlog zh/pretty-date-human The underlying notes machinery however may not be prepared to work with more than one notes tree at the same time, so git for-each-ref --format="%(notes) %(subject)" --notes=amlog zh/pretty-date-human might be a better syntax, as --format="%(notes:X) %(notes:Y)" may not be something you can easily support.
Junio C Hamano <gitster@pobox.com> 于2021年5月3日周一 上午10:57写道: > > ZheNing Hu <adlternative@gmail.com> writes: > > > Note that `cat-file --batch` should be a superset of `ref-filter` and > > `--pretty`, because > > it supports all objects, while `ref-filter` only supports "commit" and > > "tag" two kinds > > of objects, and `--pretty` only supports "commit" one kind of object. > > What? A ref can point at any kind of objects, not necessarily > commits and tags. %(objectname), %(objecttype), etc. obviously > are applicable to any type of object. > Thanks for correcting. I only saw tag and commit in the output of `git for-each-ref` before, and blob and tree are not processed in `grab_values` in `ref-filter.c`, which deepens my illusion. `git update-ref` can make a ref point to a blob object or tree object, although I don't know what these refs are used for... A reference to a commit object makes sense, but if it is a blob, are there any suitable scenarios? > Another thing worth noting is that ref-filter needs to deal with > traits that are not tied to any particular object, but to the ref > itself, like who its upstream is and where it would be pushed to. > Indeed need to be considered "%(push)" "%(remote)" will change "refname". > > So it may be reasonable to provide `%(notes)` to "commit","tag" > > objects in `cat-file --batch` > > in the future, now integrate them into ref-filter firstly. > > And you can attach notes to objects of any type, not limited to > commits and tags. > > >> > So in this patch, I tried to migrate --pretty=%N to --format=%(notes). > > What do you mean by "migrate"? Are you making both available? > Well, here are some incorrect statements, I just want to express that the `--format="%(notes)"` implemented in "ref-filter" has the same semantics as "--pretty=%N". > >> > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > >> > index 2ae2478de706..07f037a16e13 100644 > >> > --- a/Documentation/git-for-each-ref.txt > >> > +++ b/Documentation/git-for-each-ref.txt > >> > @@ -139,6 +139,9 @@ deltabase:: > >> > given object, if it is stored as a delta. Otherwise it > >> > expands to the null object name (all zeroes). > >> > > >> > +notes:: > >> > + The notes associated with the ref. > > "The notes associated with the object pointed at by the ref". > > A note is never associated with a ref. It can only associated with > an object. > Indeed. > If we are going to have %(notes), it may be natural to desire > grabbing notes for the given object from specified notes ref, e.g. > > git for-each-ref --format="%(notes:amlog) %(subject)" zh/pretty-date-human > > may want to give the same kind of information as > > git show -s --format='%N %s' --notes=amlog zh/pretty-date-human > > The underlying notes machinery however may not be prepared to work > with more than one notes tree at the same time, so > > git for-each-ref --format="%(notes) %(subject)" --notes=amlog zh/pretty-date-human > > might be a better syntax, as --format="%(notes:X) %(notes:Y)" may > not be something you can easily support. > Here is a "bad" idea that might support this feature: use another subprocess "git notes --refs=refs/notes/amlog" :) Thanks. -- ZheNing Hu
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 2ae2478de706..07f037a16e13 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -139,6 +139,9 @@ deltabase:: given object, if it is stored as a delta. Otherwise it expands to the null object name (all zeroes). +notes:: + The notes associated with the ref. + upstream:: The name of a local ref which can be considered ``upstream'' from the displayed ref. Respects `:short`, `:lstrip` and @@ -302,6 +305,7 @@ git for-each-ref --count=3 --sort='-*authordate' \ Subject: %(*subject) Date: %(*authordate) Ref: %(*refname) +Notes: %(*notes) %(*body) ' 'refs/tags' diff --git a/ref-filter.c b/ref-filter.c index a0adb4551d87..42a5608a3056 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -23,6 +23,7 @@ #include "worktree.h" #include "hashmap.h" #include "strvec.h" +#include "run-command.h" static struct ref_msg { const char *gone; @@ -506,6 +507,7 @@ static struct { { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser }, { "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser }, { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser }, + { "notes", SOURCE_OTHER, FIELD_STR }, { "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser }, { "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser }, { "numparent", SOURCE_OBJ, FIELD_ULONG }, @@ -953,6 +955,24 @@ static int grab_oid(const char *name, const char *field, const struct object_id return 0; } +static int grab_notes(const struct object_id *oid, struct atom_value *v) +{ + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + const char *args[] = { "notes", "show", NULL }; + int ret; + + cmd.git_cmd = 1; + strvec_pushv(&cmd.args, args); + strvec_push(&cmd.args, oid_to_hex(oid)); + ret = pipe_command(&cmd, NULL, 0, &out, 0, &err, 0); + strbuf_trim_trailing_newline(&out); + v->s = strbuf_detach(&out, NULL); + strbuf_release(&err); + return ret; +} + /* See grab_values */ static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi) { @@ -975,8 +995,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_ v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size); } else if (!strcmp(name, "deltabase")) v->s = xstrdup(oid_to_hex(&oi->delta_base_oid)); - else if (deref) - grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]); + else if (deref) { + if (!strcmp(name, "notes")) + grab_notes(&oi->oid, v); + else + grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]); + } } } @@ -1767,6 +1791,9 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) continue; } else if (!deref && grab_oid(name, "objectname", &ref->objectname, v, atom)) { continue; + } else if (!deref && !strcmp(name, "notes")) { + grab_notes(&ref->objectname, v); + continue; } else if (!strcmp(name, "HEAD")) { if (atom->u.head && !strcmp(ref->refname, atom->u.head)) v->s = xstrdup("*"); diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 9e0214076b4d..61cdbeb696ff 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -32,8 +32,10 @@ test_expect_success setup ' git add one && git commit -m "Initial" && git branch -M main && + git notes add -m "commit-notes" HEAD && setdate_and_increment && git tag -a -m "Tagging at $datestamp" testtag && + git notes add -m "tag-notes" testtag && git update-ref refs/remotes/origin/main main && git remote add origin nowhere && git config branch.main.remote origin && @@ -162,6 +164,7 @@ test_atom head contents:signature '' test_atom head contents 'Initial ' test_atom head HEAD '*' +test_atom head notes $(git notes show refs/heads/main) test_atom tag refname refs/tags/testtag test_atom tag refname:short testtag @@ -220,6 +223,8 @@ test_atom tag contents:signature '' test_atom tag contents 'Tagging at 1151968727 ' test_atom tag HEAD ' ' +test_atom tag notes $(git notes show refs/tags/testtag) +test_atom tag "*notes" $(git notes show refs/heads/main) test_expect_success 'Check invalid atoms names are errors' ' test_must_fail git for-each-ref --format="%(INVALID)" refs/heads @@ -380,6 +385,7 @@ test_expect_success 'exercise strftime with odd fields' ' cat >expected <<\EOF refs/heads/main +refs/notes/commits refs/remotes/origin/main refs/tags/testtag EOF @@ -393,6 +399,7 @@ test_expect_success 'Verify ascending sort' ' cat >expected <<\EOF refs/tags/testtag refs/remotes/origin/main +refs/notes/commits refs/heads/main EOF @@ -429,6 +436,7 @@ test_expect_success 'exercise glob patterns with prefixes' ' cat >expected <<\EOF 'refs/heads/main' +'refs/notes/commits' 'refs/remotes/origin/main' 'refs/tags/testtag' EOF @@ -450,6 +458,7 @@ test_expect_success 'Quoting style: python' ' cat >expected <<\EOF "refs/heads/main" +"refs/notes/commits" "refs/remotes/origin/main" "refs/tags/testtag" EOF @@ -509,6 +518,7 @@ test_expect_success 'Check for invalid refname format' ' test_expect_success 'set up color tests' ' cat >expected.color <<-EOF && $(git rev-parse --short refs/heads/main) <GREEN>main<RESET> + $(git rev-parse --short refs/notes/commits) <GREEN>notes/commits<RESET> $(git rev-parse --short refs/remotes/myfork/main) <GREEN>myfork/main<RESET> $(git rev-parse --short refs/remotes/origin/main) <GREEN>origin/main<RESET> $(git rev-parse --short refs/tags/testtag) <GREEN>testtag<RESET>