| Message ID | 75eb2f6740eb5845afcb7d31956cc5b3e3957f97.1626939557.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ref-filter: add %(raw) and %(rest) atoms | expand |
On Thu, Jul 22 2021, ZheNing Hu via GitGitGadget wrote: > + } else if (atom_type == ATOM_REST) { > + if (ref->rest) > + v->s = xstrdup(ref->rest); > + else > + v->s = xstrdup(""); > + continue; > } else > continue; Another light reading nit, maybe more readable as: } else if (atom_type == ATOM_REST && ref->rest) { ... } else if (atom_type == ATOM_REST) { ... } continue; But we can't do that "continue" at the end because there's two cases where we don't continue, I wondered if elimanting that special case would make it easier, patch below. Probably not worth it & feel free to ignore: ref-filter.c | 63 +++++++++++++++++++++++++----------------------------------- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 81e77b13ad2..189244fed6f 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1890,18 +1890,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) name++; } - if (atom_type == ATOM_REFNAME) + if (atom_type == ATOM_REFNAME) { refname = get_refname(atom, ref); - else if (atom_type == ATOM_WORKTREEPATH) { - if (ref->kind == FILTER_REFS_BRANCHES) - v->s = get_worktree_path(atom, ref); + if (!deref) + v->s = xstrdup(refname); else - v->s = xstrdup(""); - continue; - } - else if (atom_type == ATOM_SYMREF) + v->s = xstrfmt("%s^{}", refname); + free((char *)refname); + } else if (atom_type == ATOM_WORKTREEPATH && + ref->kind == FILTER_REFS_BRANCHES) { + v->s = get_worktree_path(atom, ref); + } else if (atom_type == ATOM_WORKTREEPATH) { + v->s = xstrdup(""); + } else if (atom_type == ATOM_SYMREF) { refname = get_symref(atom, ref); - else if (atom_type == ATOM_UPSTREAM) { + if (!deref) + v->s = xstrdup(refname); + else + v->s = xstrfmt("%s^{}", refname); + free((char *)refname); + } else if (atom_type == ATOM_UPSTREAM) { const char *branch_name; /* only local branches may have an upstream */ if (!skip_prefix(ref->refname, "refs/heads/", @@ -1916,7 +1924,6 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) fill_remote_ref_details(atom, refname, branch, &v->s); else v->s = xstrdup(""); - continue; } else if (atom_type == ATOM_PUSH && atom->u.remote_ref.push) { const char *branch_name; v->s = xstrdup(""); @@ -1935,10 +1942,8 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) /* We will definitely re-init v->s on the next line. */ free((char *)v->s); fill_remote_ref_details(atom, refname, branch, &v->s); - continue; } else if (atom_type == ATOM_COLOR) { v->s = xstrdup(atom->u.color); - continue; } else if (atom_type == ATOM_FLAG) { char buf[256], *cp = buf; if (ref->flag & REF_ISSYMREF) @@ -1951,24 +1956,20 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) *cp = '\0'; v->s = xstrdup(buf + 1); } - continue; } else if (!deref && atom_type == ATOM_OBJECTNAME) { v->s = xstrdup(do_grab_oid("objectname", &ref->objectname, atom)); - continue; + } else if (atom_type == ATOM_HEAD && + atom->u.head && + !strcmp(ref->refname, atom->u.head)) { + v->s = xstrdup("*"); } else if (atom_type == ATOM_HEAD) { - if (atom->u.head && !strcmp(ref->refname, atom->u.head)) - v->s = xstrdup("*"); - else - v->s = xstrdup(" "); - continue; + v->s = xstrdup(" "); } else if (atom_type == ATOM_ALIGN) { v->handler = align_atom_handler; v->s = xstrdup(""); - continue; } else if (atom_type == ATOM_END) { v->handler = end_atom_handler; v->s = xstrdup(""); - continue; } else if (atom_type == ATOM_IF) { const char *s; if (skip_prefix(name, "if:", &s)) @@ -1976,29 +1977,17 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) else v->s = xstrdup(""); v->handler = if_atom_handler; - continue; } else if (atom_type == ATOM_THEN) { v->handler = then_atom_handler; v->s = xstrdup(""); - continue; } else if (atom_type == ATOM_ELSE) { v->handler = else_atom_handler; v->s = xstrdup(""); - continue; + } else if (atom_type == ATOM_REST && ref->rest) { + v->s = xstrdup(ref->rest); } else if (atom_type == ATOM_REST) { - if (ref->rest) - v->s = xstrdup(ref->rest); - else - v->s = xstrdup(""); - continue; - } else - continue; - - if (!deref) - v->s = xstrdup(refname); - else - v->s = xstrfmt("%s^{}", refname); - free((char *)refname); + v->s = xstrdup(""); + } } for (i = 0; i < used_atom_cnt; i++) {
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年7月22日周四 下午4:24写道： > > > On Thu, Jul 22 2021, ZheNing Hu via GitGitGadget wrote: > > > > + } else if (atom_type == ATOM_REST) { > > + if (ref->rest) > > + v->s = xstrdup(ref->rest); > > + else > > + v->s = xstrdup(""); > > + continue; > > } else > > continue; > > Another light reading nit, maybe more readable as: > > } else if (atom_type == ATOM_REST && ref->rest) { > ... > } else if (atom_type == ATOM_REST) { > ... > } > continue; > > But we can't do that "continue" at the end because there's two cases > where we don't continue, I wondered if elimanting that special case > would make it easier, patch below. Probably not worth it & feel free to > ignore: Yeah, the readability here is really bad, so many "continue" just to not execute part of the operation on refname. In fact, I have a similar patch (which will not be sent to the mailing list for the time being), it use switch and case to replace if and else. [1] > > ref-filter.c | 63 +++++++++++++++++++++++++----------------------------------- > 1 file changed, 26 insertions(+), 37 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 81e77b13ad2..189244fed6f 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1890,18 +1890,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) > name++; > } > > - if (atom_type == ATOM_REFNAME) > + if (atom_type == ATOM_REFNAME) { > refname = get_refname(atom, ref); > - else if (atom_type == ATOM_WORKTREEPATH) { > - if (ref->kind == FILTER_REFS_BRANCHES) > - v->s = get_worktree_path(atom, ref); > + if (!deref) > + v->s = xstrdup(refname); > else > - v->s = xstrdup(""); > - continue; > - } > - else if (atom_type == ATOM_SYMREF) > + v->s = xstrfmt("%s^{}", refname); > + free((char *)refname); > + } else if (atom_type == ATOM_WORKTREEPATH && > + ref->kind == FILTER_REFS_BRANCHES) { > + v->s = get_worktree_path(atom, ref); > + } else if (atom_type == ATOM_WORKTREEPATH) { > + v->s = xstrdup(""); > + } else if (atom_type == ATOM_SYMREF) { > refname = get_symref(atom, ref); > - else if (atom_type == ATOM_UPSTREAM) { > + if (!deref) > + v->s = xstrdup(refname); > + else > + v->s = xstrfmt("%s^{}", refname); > + free((char *)refname); > + } else if (atom_type == ATOM_UPSTREAM) { > const char *branch_name; > /* only local branches may have an upstream */ > if (!skip_prefix(ref->refname, "refs/heads/", > @@ -1916,7 +1924,6 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) > fill_remote_ref_details(atom, refname, branch, &v->s); > else > v->s = xstrdup(""); > - continue; > } else if (atom_type == ATOM_PUSH && atom->u.remote_ref.push) { > const char *branch_name; > v->s = xstrdup(""); > @@ -1935,10 +1942,8 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) > /* We will definitely re-init v->s on the next line. */ > free((char *)v->s); > fill_remote_ref_details(atom, refname, branch, &v->s); > - continue; > } else if (atom_type == ATOM_COLOR) { > v->s = xstrdup(atom->u.color); > - continue; > } else if (atom_type == ATOM_FLAG) { > char buf[256], *cp = buf; > if (ref->flag & REF_ISSYMREF) > @@ -1951,24 +1956,20 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) > *cp = '\0'; > v->s = xstrdup(buf + 1); > } > - continue; > } else if (!deref && atom_type == ATOM_OBJECTNAME) { > v->s = xstrdup(do_grab_oid("objectname", &ref->objectname, atom)); > - continue; > + } else if (atom_type == ATOM_HEAD && > + atom->u.head && > + !strcmp(ref->refname, atom->u.head)) { > + v->s = xstrdup("*"); > } else if (atom_type == ATOM_HEAD) { > - if (atom->u.head && !strcmp(ref->refname, atom->u.head)) > - v->s = xstrdup("*"); > - else > - v->s = xstrdup(" "); > - continue; > + v->s = xstrdup(" "); > } else if (atom_type == ATOM_ALIGN) { > v->handler = align_atom_handler; > v->s = xstrdup(""); > - continue; > } else if (atom_type == ATOM_END) { > v->handler = end_atom_handler; > v->s = xstrdup(""); > - continue; > } else if (atom_type == ATOM_IF) { > const char *s; > if (skip_prefix(name, "if:", &s)) > @@ -1976,29 +1977,17 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) > else > v->s = xstrdup(""); > v->handler = if_atom_handler; > - continue; > } else if (atom_type == ATOM_THEN) { > v->handler = then_atom_handler; > v->s = xstrdup(""); > - continue; > } else if (atom_type == ATOM_ELSE) { > v->handler = else_atom_handler; > v->s = xstrdup(""); > - continue; > + } else if (atom_type == ATOM_REST && ref->rest) { > + v->s = xstrdup(ref->rest); > } else if (atom_type == ATOM_REST) { > - if (ref->rest) > - v->s = xstrdup(ref->rest); > - else > - v->s = xstrdup(""); > - continue; > - } else > - continue; > - > - if (!deref) > - v->s = xstrdup(refname); > - else > - v->s = xstrfmt("%s^{}", refname); > - free((char *)refname); > + v->s = xstrdup(""); > + } > } > > for (i = 0; i < used_atom_cnt; i++) { [1]: https://github.com/adlternative/git/commit/bbc6ecba4c0f65e7328e571b0d38ff99f800dc09
On Thu, Jul 22, 2021 at 12:40 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: ZheNing Hu <adlternative@gmail.com> > > In order to let "cat-file --batch=%(rest)" use the ref-filter > interface, add %(rest) atom for ref-filter. Introduce the > reject_atom() to reject the atom %(rest) for "git for-each-ref", > "git branch", "git tag" and "git verify-tag". > To a reader intimately familiar with cat-file, %(rest) might be obvious.. but it would help the commit message read more easily if there was a short explanation of what %(rest) did. It is not obvious just from the name to me. Thanks, Jake
Jacob Keller <jacob.keller@gmail.com> 于2021年7月23日周五 下午5:09写道： > > On Thu, Jul 22, 2021 at 12:40 AM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: ZheNing Hu <adlternative@gmail.com> > > > > In order to let "cat-file --batch=%(rest)" use the ref-filter > > interface, add %(rest) atom for ref-filter. Introduce the > > reject_atom() to reject the atom %(rest) for "git for-each-ref", > > "git branch", "git tag" and "git verify-tag". > > > > To a reader intimately familiar with cat-file, %(rest) might be > obvious.. but it would help the commit message read more easily if > there was a short explanation of what %(rest) did. It is not obvious > just from the name to me. > You are right, I may subconsciously think that everyone knows the %(rest) in cat-file. I will change it. > Thanks, > Jake Thanks. -- ZheNing Hu
diff --git a/ref-filter.c b/ref-filter.c index c8e561a3687..af8216dcd5b 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -157,6 +157,7 @@ enum atom_type { ATOM_IF, ATOM_THEN, ATOM_ELSE, + ATOM_REST, }; /* @@ -559,6 +560,15 @@ static int if_atom_parser(struct ref_format *format, struct used_atom *atom, return 0; } +static int rest_atom_parser(struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + if (arg) + return strbuf_addf_ret(err, -1, _("%%(rest) does not take arguments")); + format->use_rest = 1; + return 0; +} + static int head_atom_parser(struct ref_format *format, struct used_atom *atom, const char *arg, struct strbuf *unused_err) { @@ -615,6 +625,7 @@ static struct { [ATOM_IF] = { "if", SOURCE_NONE, FIELD_STR, if_atom_parser }, [ATOM_THEN] = { "then", SOURCE_NONE }, [ATOM_ELSE] = { "else", SOURCE_NONE }, + [ATOM_REST] = { "rest", SOURCE_NONE, FIELD_STR, rest_atom_parser }, /* * Please update $__git_ref_fieldlist in git-completion.bash * when you add new atoms @@ -989,6 +1000,11 @@ static const char *find_next(const char *cp) return NULL; } +static int reject_atom(enum atom_type atom_type) +{ + return atom_type == ATOM_REST; +} + /* * Make sure the format string is well formed, and parse out * the used atoms. @@ -1009,6 +1025,8 @@ int verify_ref_format(struct ref_format *format) at = parse_ref_filter_atom(format, sp + 2, ep, &err); if (at < 0) die("%s", err.buf); + if (reject_atom(used_atom[at].atom_type)) + die(_("this command reject atom %%(%.*s)"), (int)(ep - sp - 2), sp + 2); if ((format->quote_style == QUOTE_PYTHON || format->quote_style == QUOTE_SHELL || @@ -1928,6 +1946,12 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) v->handler = else_atom_handler; v->s = xstrdup(""); continue; + } else if (atom_type == ATOM_REST) { + if (ref->rest) + v->s = xstrdup(ref->rest); + else + v->s = xstrdup(""); + continue; } else continue; @@ -2145,6 +2169,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname, FLEX_ALLOC_STR(ref, refname, refname); oidcpy(&ref->objectname, oid); + ref->rest = NULL; return ref; } diff --git a/ref-filter.h b/ref-filter.h index 74fb423fc89..c15dee8d6b9 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -38,6 +38,7 @@ struct ref_sorting { struct ref_array_item { struct object_id objectname; + const char *rest; int flag; unsigned int kind; const char *symref; @@ -76,14 +77,16 @@ struct ref_format { * verify_ref_format() afterwards to finalize. */ const char *format; + const char *rest; int quote_style; + int use_rest; int use_color; /* Internal state to ref-filter */ int need_color_reset_at_eol; }; -#define REF_FORMAT_INIT { NULL, 0, -1 } +#define REF_FORMAT_INIT { .use_color = -1 } /* Macros for checking --merged and --no-merged options */ #define _OPT_MERGED_NO_MERGED(option, filter, h) \ diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index 5325b9f67a0..6e94c6db7b5 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -340,6 +340,10 @@ test_expect_success 'git branch --format option' ' test_cmp expect actual ' +test_expect_success 'git branch with --format=%(rest) must fail' ' + test_must_fail git branch --format="%(rest)" >actual +' + test_expect_success 'worktree colors correct' ' cat >expect <<-EOF && * <GREEN>(HEAD detached from fromtag)<RESET> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 3d15d0a5360..0d2e062f791 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -1211,6 +1211,10 @@ test_expect_success 'basic atom: head contents:trailers' ' test_cmp expect actual.clean ' +test_expect_success 'basic atom: rest must fail' ' + test_must_fail git for-each-ref --format="%(rest)" refs/heads/main +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 2f72c5c6883..082be85dffc 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1998,6 +1998,10 @@ test_expect_success '--format should list tags as per format given' ' test_cmp expect actual ' +test_expect_success 'git tag -l with --format="%(rest)" must fail' ' + test_must_fail git tag -l --format="%(rest)" "v1*" +' + test_expect_success "set up color tests" ' echo "<RED>v1.0<RESET>" >expect.color && echo "v1.0" >expect.bare && diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index 3cefde9602b..10faa645157 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -194,6 +194,10 @@ test_expect_success GPG 'verifying tag with --format' ' test_cmp expect actual ' +test_expect_success GPG 'verifying tag with --format="%(rest)" must fail' ' + test_must_fail git verify-tag --format="%(rest)" "fourth-signed" +' + test_expect_success GPG 'verifying a forged tag with --format should fail silently' ' test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged && test_must_be_empty actual-forged