| Message ID | 20240223100112.44127-1-karthik.188@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | for-each-ref: add '--include-root-refs' option | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > Changes from v4: > 1. Fixed erratic whitespace > 2. Remove braces from single line block > 3. Starting the comments with a capital and also adding more context. > 4. Removed a duplicate check. Does #4 refer to this removal? if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD) kind = FILTER_REFS_PSEUDOREFS; else if (!(kind & filter->kind)) return NULL; - if (!(kind & filter->kind)) - return NULL; - if (!filter_pattern_match(filter, refname)) return NULL; If filter->kind is MASK and kind is set to filter detached HEAD, we assign to and change the value of kind to FILTER_REFS_PSEUDOREFS, so it is unclear if the freestanding "if kind and filter->kind does not overlap, return NULL" was redundant or outright buggy. The hunk just stood out to me, but I haven't read other parts of the series yet. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> Changes from v4: >> 1. Fixed erratic whitespace >> 2. Remove braces from single line block >> 3. Starting the comments with a capital and also adding more context. >> 4. Removed a duplicate check. > > Does #4 refer to this removal? > > if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD) > kind = FILTER_REFS_PSEUDOREFS; > else if (!(kind & filter->kind)) > return NULL; > > - if (!(kind & filter->kind)) > - return NULL; > - > if (!filter_pattern_match(filter, refname)) > return NULL; > > > If filter->kind is MASK and kind is set to filter detached HEAD, we > assign to and change the value of kind to FILTER_REFS_PSEUDOREFS, > so it is unclear if the freestanding "if kind and filter->kind does > not overlap, return NULL" was redundant or outright buggy. Ah, of course this is simply redundant. The assignment to limit the kind to PSEUDOREFS happens only when filter->kind has all bits, and after assigning a different non-zero value to kind, (kind & filter->kind) will still overlap, so a freestanding "if no overlap between kind and filter-kind then return NULL" will not trigger for such a case. Thanks.
On Fri, Feb 23, 2024 at 11:01:07AM +0100, Karthik Nayak wrote: > This is the fifth version of my patch series to print root refs > in git-for-each-ref(1). > > With the introduction of the reftable backend, it becomes ever > so important to provide the necessary tooling for printing all refs > associated with a worktree. > > While regular refs stored within the "refs/" namespace are currently > supported by multiple commands like git-for-each-ref(1), > git-show-ref(1). Neither support printing root refs within the worktree. > > This patch series is a follow up to the RFC/discussion we had earlier on > the list [1]. > > The first 4 commits add the required functionality to ensure we can print > all refs (regular, pseudo, HEAD). The 5th commit modifies the > git-for-each-ref(1) command to add the "--include-root-refs" command which > will include HEAD and pseudorefs with regular "refs/" refs. > > [1]: https://lore.kernel.org/git/20231221170715.110565-1-karthik.188@gmail.com/#t > > Changes from v4: > 1. Fixed erratic whitespace > 2. Remove braces from single line block > 3. Starting the comments with a capital and also adding more context. > 4. Removed a duplicate check. > > Thanks for the reviews. > > Range diff against v4: The range-diff looks as expected, so this patch series looks good to me. Thanks! Patrick > 1: 98130a7ad7 ! 1: 6016042965 refs: introduce `is_pseudoref()` and `is_headref()` > @@ refs.c: static int is_pseudoref_syntax(const char *refname) > + > + if (ends_with(refname, "_HEAD")) { > + refs_resolve_ref_unsafe(refs, refname, > -+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > -+ &oid, NULL); > -+ return !is_null_oid(&oid); > ++ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > ++ &oid, NULL); > ++ return !is_null_oid(&oid); > + } > + > + for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++) > + if (!strcmp(refname, irregular_pseudorefs[i])) { > + refs_resolve_ref_unsafe(refs, refname, > -+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > -+ &oid, NULL); > ++ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > ++ &oid, NULL); > + return !is_null_oid(&oid); > + } > + > 2: 060ab08af5 = 2: acaa014841 refs: extract out `loose_fill_ref_dir_regular_file()` > 3: 40d2375ad9 = 3: f51c5bc307 refs: introduce `refs_for_each_include_root_refs()` > 4: b4b9435505 = 4: 7c004db6e7 ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR' > 5: ee99ac41ae ! 5: 53f6c0a6db for-each-ref: add new option to include root refs > @@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const > filter.name_patterns = argv; > } > > -+ if (include_root_refs) { > ++ if (include_root_refs) > + flags |= FILTER_REFS_ROOT_REFS; > -+ } > + > filter.match_as_path = 1; > - filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format); > @@ ref-filter.c: static int for_each_fullref_in_pattern(struct ref_filter *filter, > void *cb_data) > { > + if (filter->kind == FILTER_REFS_KIND_MASK) { > -+ /* in this case, we want to print all refs including root refs. */ > ++ /* In this case, we want to print all refs including root refs. */ > + return refs_for_each_include_root_refs(get_main_ref_store(the_repository), > + cb, cb_data); > + } > @@ ref-filter.c: static struct ref_array_item *apply_ref_filter(const char *refname > > /* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */ > kind = filter_ref_kind(filter, refname); > +- if (!(kind & filter->kind)) > + > + /* > -+ * When printing HEAD with all other refs, we want to apply the same formatting > -+ * rules as the other refs, so we simply ask it to be treated as a pseudoref. > ++ * Generally HEAD refs are printed with special description denoting a rebase, > ++ * detached state and so forth. This is useful when only printing the HEAD ref > ++ * But when it is being printed along with other pseudorefs, it makes sense to > ++ * keep the formatting consistent. So we mask the type to act like a pseudoref. > + */ > + if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD) > + kind = FILTER_REFS_PSEUDOREFS; > + else if (!(kind & filter->kind)) > -+ return NULL; > -+ > - if (!(kind & filter->kind)) > return NULL; > > + if (!filter_pattern_match(filter, refname)) > @@ ref-filter.c: static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref > ret = for_each_fullref_in("refs/tags/", fn, cb_data); > else if (filter->kind & FILTER_REFS_REGULAR) > > > Karthik Nayak (5): > refs: introduce `is_pseudoref()` and `is_headref()` > refs: extract out `loose_fill_ref_dir_regular_file()` > refs: introduce `refs_for_each_include_root_refs()` > ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR' > for-each-ref: add new option to include root refs > > Documentation/git-for-each-ref.txt | 5 +- > builtin/for-each-ref.c | 10 ++- > ref-filter.c | 30 ++++++- > ref-filter.h | 7 +- > refs.c | 48 +++++++++++ > refs.h | 9 ++ > refs/files-backend.c | 127 +++++++++++++++++++++-------- > refs/refs-internal.h | 6 ++ > refs/reftable-backend.c | 11 ++- > t/t6302-for-each-ref-filter.sh | 31 +++++++ > 10 files changed, 237 insertions(+), 47 deletions(-) > > -- > 2.43.GIT >
Patrick Steinhardt <ps@pks.im> writes: >> Changes from v4: >> 1. Fixed erratic whitespace >> 2. Remove braces from single line block >> 3. Starting the comments with a capital and also adding more context. >> 4. Removed a duplicate check. >> >> Thanks for the reviews. >> >> Range diff against v4: > > The range-diff looks as expected, so this patch series looks good to me. > Thanks! > > Patrick Thanks, let's mark the topic for 'next'.
This is the fifth version of my patch series to print root refs in git-for-each-ref(1). With the introduction of the reftable backend, it becomes ever so important to provide the necessary tooling for printing all refs associated with a worktree. While regular refs stored within the "refs/" namespace are currently supported by multiple commands like git-for-each-ref(1), git-show-ref(1). Neither support printing root refs within the worktree. This patch series is a follow up to the RFC/discussion we had earlier on the list [1]. The first 4 commits add the required functionality to ensure we can print all refs (regular, pseudo, HEAD). The 5th commit modifies the git-for-each-ref(1) command to add the "--include-root-refs" command which will include HEAD and pseudorefs with regular "refs/" refs. [1]: https://lore.kernel.org/git/20231221170715.110565-1-karthik.188@gmail.com/#t Changes from v4: 1. Fixed erratic whitespace 2. Remove braces from single line block 3. Starting the comments with a capital and also adding more context. 4. Removed a duplicate check. Thanks for the reviews. Range diff against v4: 1: 98130a7ad7 ! 1: 6016042965 refs: introduce `is_pseudoref()` and `is_headref()` @@ refs.c: static int is_pseudoref_syntax(const char *refname) + + if (ends_with(refname, "_HEAD")) { + refs_resolve_ref_unsafe(refs, refname, -+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, -+ &oid, NULL); -+ return !is_null_oid(&oid); ++ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, ++ &oid, NULL); ++ return !is_null_oid(&oid); + } + + for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++) + if (!strcmp(refname, irregular_pseudorefs[i])) { + refs_resolve_ref_unsafe(refs, refname, -+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, -+ &oid, NULL); ++ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, ++ &oid, NULL); + return !is_null_oid(&oid); + } + 2: 060ab08af5 = 2: acaa014841 refs: extract out `loose_fill_ref_dir_regular_file()` 3: 40d2375ad9 = 3: f51c5bc307 refs: introduce `refs_for_each_include_root_refs()` 4: b4b9435505 = 4: 7c004db6e7 ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR' 5: ee99ac41ae ! 5: 53f6c0a6db for-each-ref: add new option to include root refs @@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const filter.name_patterns = argv; } -+ if (include_root_refs) { ++ if (include_root_refs) + flags |= FILTER_REFS_ROOT_REFS; -+ } + filter.match_as_path = 1; - filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format); @@ ref-filter.c: static int for_each_fullref_in_pattern(struct ref_filter *filter, void *cb_data) { + if (filter->kind == FILTER_REFS_KIND_MASK) { -+ /* in this case, we want to print all refs including root refs. */ ++ /* In this case, we want to print all refs including root refs. */ + return refs_for_each_include_root_refs(get_main_ref_store(the_repository), + cb, cb_data); + } @@ ref-filter.c: static struct ref_array_item *apply_ref_filter(const char *refname /* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */ kind = filter_ref_kind(filter, refname); +- if (!(kind & filter->kind)) + + /* -+ * When printing HEAD with all other refs, we want to apply the same formatting -+ * rules as the other refs, so we simply ask it to be treated as a pseudoref. ++ * Generally HEAD refs are printed with special description denoting a rebase, ++ * detached state and so forth. This is useful when only printing the HEAD ref ++ * But when it is being printed along with other pseudorefs, it makes sense to ++ * keep the formatting consistent. So we mask the type to act like a pseudoref. + */ + if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD) + kind = FILTER_REFS_PSEUDOREFS; + else if (!(kind & filter->kind)) -+ return NULL; -+ - if (!(kind & filter->kind)) return NULL; + if (!filter_pattern_match(filter, refname)) @@ ref-filter.c: static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref ret = for_each_fullref_in("refs/tags/", fn, cb_data); else if (filter->kind & FILTER_REFS_REGULAR) Karthik Nayak (5): refs: introduce `is_pseudoref()` and `is_headref()` refs: extract out `loose_fill_ref_dir_regular_file()` refs: introduce `refs_for_each_include_root_refs()` ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR' for-each-ref: add new option to include root refs Documentation/git-for-each-ref.txt | 5 +- builtin/for-each-ref.c | 10 ++- ref-filter.c | 30 ++++++- ref-filter.h | 7 +- refs.c | 48 +++++++++++ refs.h | 9 ++ refs/files-backend.c | 127 +++++++++++++++++++++-------- refs/refs-internal.h | 6 ++ refs/reftable-backend.c | 11 ++- t/t6302-for-each-ref-filter.sh | 31 +++++++ 10 files changed, 237 insertions(+), 47 deletions(-)