| Message ID | 20200503091157.GA170902@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ref-filter: apply --ignore-case to all sorting keys | expand |
On 2020-05-03 05:11:57-0400, Jeff King <peff@peff.net> wrote: > +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' > + # name refs numerically to avoid case-insensitive filesystem conflicts > + nr=0 && > + for email in a A b B > + do > + for subject in a A b B > + do > + GIT_COMMITTER_EMAIL="$email@example.com" \ > + git tag -m "tag $subject" icase-$(printf %02d $nr) && > + nr=$((nr+1))|| The CodingGuidelines said we want to spell `$nr` instead of `nr` inside arithmetic expansion for dash older than 0.5.4 I'm not sure if we should go with just `$((nr+1))` or it's better to loosen our Guidelines. Since Debian Jessie (oldest supported Debian) ships 0.5.7. I don't know about other systems.
On Sun, May 03, 2020 at 06:44:02PM +0700, Danh Doan wrote: > On 2020-05-03 05:11:57-0400, Jeff King <peff@peff.net> wrote: > > +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' > > + # name refs numerically to avoid case-insensitive filesystem conflicts > > + nr=0 && > > + for email in a A b B > > + do > > + for subject in a A b B > > + do > > + GIT_COMMITTER_EMAIL="$email@example.com" \ > > + git tag -m "tag $subject" icase-$(printf %02d $nr) && > > + nr=$((nr+1))|| > > The CodingGuidelines said we want to spell `$nr` instead of `nr` > inside arithmetic expansion for dash older than 0.5.4 > > I'm not sure if we should go with just `$((nr+1))` or it's better to > loosen our Guidelines. Since Debian Jessie (oldest supported Debian) > ships 0.5.7. I don't know about other systems. Hmm, somehow I didn't know about that rule. We have many cases already in the test suite and elsewhere (try grepping for '$(([a-z]', which isn't exhaustive but turns up many examples). Maybe it's time to loosen the rule? I've actually seen style guides suggesting to never use "$" there for a few reasons: - it's slightly cleaner to read (this is the recommendation and rationale in Google's shell style guide) - it's less surprising if you somehow end up with a non-number in your variable: $ foo=bar $ bar=41 $ echo $((foo + 1)) dash: 8: Illegal number: bar $ echo $(($foo + 1)) 42 That's using dash. With bash, both produce the answer 42! Clearly this isn't something we should be doing either way, but I'd much rather see "illegal number" in some cases which would alert us that something confusing is going on. -Peff
Jeff King <peff@peff.net> writes: > Hmm, somehow I didn't know about that rule. We have many cases already > in the test suite and elsewhere (try grepping for '$(([a-z]', which > isn't exhaustive but turns up many examples). > > Maybe it's time to loosen the rule? Let's do that. It's time.
Jeff King <peff@peff.net> writes: > would sort the primary key (taggername) case-insensitively, but sort the > refname case-sensitively. We have two options here: > > - teach callers to set ignore_case on the whole list > > - replace the ref_sorting list with a struct that contains both the > list of sorting keys, as well as options that apply to _all_ > keys > > I went with the first one here, as it gives more flexibility if we later > want to let the users set the flag per-key (presumably through some > special syntax when defining the key; for now it's all or nothing > through --ignore-case). A good design decision I would fully support. > +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' > + # name refs numerically to avoid case-insensitive filesystem conflicts Very considerate. If I were writing these nested loops, I am sure I would have used "tag-$email-$subject" to be cute. Queued. Thanks. > + nr=0 && > + for email in a A b B > + do > + for subject in a A b B > + do > + GIT_COMMITTER_EMAIL="$email@example.com" \ > + git tag -m "tag $subject" icase-$(printf %02d $nr) && > + nr=$((nr+1))|| > + return 1 > + done > + done && > + git for-each-ref --ignore-case \ > + --format="%(taggeremail) %(subject) %(refname)" \ > + --sort=refname \ > + --sort=subject \ > + --sort=taggeremail \ > + refs/tags/icase-* >actual && > + cat >expect <<-\EOF && > + <a@example.com> tag a refs/tags/icase-00 > + <a@example.com> tag A refs/tags/icase-01 > + <A@example.com> tag a refs/tags/icase-04 > + <A@example.com> tag A refs/tags/icase-05 > + <a@example.com> tag b refs/tags/icase-02 > + <a@example.com> tag B refs/tags/icase-03 > + <A@example.com> tag b refs/tags/icase-06 > + <A@example.com> tag B refs/tags/icase-07 > + <b@example.com> tag a refs/tags/icase-08 > + <b@example.com> tag A refs/tags/icase-09 > + <B@example.com> tag a refs/tags/icase-12 > + <B@example.com> tag A refs/tags/icase-13 > + <b@example.com> tag b refs/tags/icase-10 > + <b@example.com> tag B refs/tags/icase-11 > + <B@example.com> tag b refs/tags/icase-14 > + <B@example.com> tag B refs/tags/icase-15 > + EOF > + test_cmp expect actual > +' > + > test_done
On Mon, May 04, 2020 at 02:00:12PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > would sort the primary key (taggername) case-insensitively, but sort the > > refname case-sensitively. We have two options here: > > > > - teach callers to set ignore_case on the whole list > > > > - replace the ref_sorting list with a struct that contains both the > > list of sorting keys, as well as options that apply to _all_ > > keys > > > > I went with the first one here, as it gives more flexibility if we later > > want to let the users set the flag per-key (presumably through some > > special syntax when defining the key; for now it's all or nothing > > through --ignore-case). > > A good design decision I would fully support. I admit I had second thoughts when dealing with the "oops, we have to choose ignore_case from the first one" part of the second patch. But I think it works OK in practice, and I did like having a less invasive diff. :) -Peff
On Sun, May 03, 2020 at 05:11:57AM -0400, Jeff King wrote: > All of the ref-filter users (for-each-ref, branch, and tag) take an > --ignore-case option which makes filtering and sorting case-insensitive. > However, this option was applied only to the first element of the > ref_sorting list. So: > > git for-each-ref --ignore-case --sort=refname > > would do what you expect, but: > > git for-each-ref --ignore-case --sort=refname --sort=taggername > > would sort the primary key (taggername) case-insensitively, but sort the > refname case-sensitively. We have two options here: > > - teach callers to set ignore_case on the whole list > > - replace the ref_sorting list with a struct that contains both the > list of sorting keys, as well as options that apply to _all_ > keys > > I went with the first one here, as it gives more flexibility if we later > want to let the users set the flag per-key (presumably through some > special syntax when defining the key; for now it's all or nothing > through --ignore-case). Makes sense, I think that this will provide us more flexibility in the future in case we want to have per-flag keys or some such. > The new test covers this by sorting on both tagger and subject > case-insensitively, which should compare "a" and "A" identically, but > still sort them before "b" and "B". We'll break ties by sorting on the > refname to give ourselves a stable output (this is actually supposed to > be done automatically, but there's another bug which will be fixed in > the next commit). Thanks for adding a test. > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/branch.c | 2 +- > builtin/for-each-ref.c | 2 +- > builtin/tag.c | 2 +- > ref-filter.c | 6 ++++++ > ref-filter.h | 2 ++ > t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 51 insertions(+), 3 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index d8297f80ff..86341cc835 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -739,7 +739,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > */ > if (!sorting) > sorting = ref_default_sorting(); > - sorting->ignore_case = icase; > + ref_sorting_icase_all(sorting, icase); > print_ref_list(&filter, sorting, &format); > print_columns(&output, colopts, NULL); > string_list_clear(&output, 0); > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > index 465153e853..57489e4eab 100644 > --- a/builtin/for-each-ref.c > +++ b/builtin/for-each-ref.c > @@ -70,7 +70,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) > > if (!sorting) > sorting = ref_default_sorting(); > - sorting->ignore_case = icase; > + ref_sorting_icase_all(sorting, icase); > filter.ignore_case = icase; > > filter.name_patterns = argv; > diff --git a/builtin/tag.c b/builtin/tag.c > index dd160b49c7..ff7610b5c8 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) > } > if (!sorting) > sorting = ref_default_sorting(); > - sorting->ignore_case = icase; > + ref_sorting_icase_all(sorting, icase); > filter.ignore_case = icase; > if (cmdmode == 'l') { > int ret; > diff --git a/ref-filter.c b/ref-filter.c > index 35776838f4..bdb3535ce5 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -2317,6 +2317,12 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting) > return 0; > } > > +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag) > +{ > + for (; sorting; sorting = sorting->next) > + sorting->ignore_case = !!flag; > +} > + > void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array) > { > QSORT_S(array->items, array->nr, compare_refs, sorting); > diff --git a/ref-filter.h b/ref-filter.h > index 64330e9601..8ecc33cdfa 100644 > --- a/ref-filter.h > +++ b/ref-filter.h > @@ -114,6 +114,8 @@ void ref_array_clear(struct ref_array *array); > int verify_ref_format(struct ref_format *format); > /* Sort the given ref_array as per the ref_sorting provided */ > void ref_array_sort(struct ref_sorting *sort, struct ref_array *array); > +/* Set the ignore_case flag for all elements of a sorting list */ > +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag); > /* Based on the given format and quote_style, fill the strbuf */ > int format_ref_array_item(struct ref_array_item *info, > const struct ref_format *format, > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index b3c1092338..c9caf26327 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -895,4 +895,44 @@ test_expect_success 'for-each-ref --ignore-case ignores case' ' > test_cmp expect actual > ' > > +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' > + # name refs numerically to avoid case-insensitive filesystem conflicts > + nr=0 && > + for email in a A b B > + do > + for subject in a A b B > + do > + GIT_COMMITTER_EMAIL="$email@example.com" \ > + git tag -m "tag $subject" icase-$(printf %02d $nr) && > + nr=$((nr+1))|| > + return 1 > + done > + done && > + git for-each-ref --ignore-case \ > + --format="%(taggeremail) %(subject) %(refname)" \ > + --sort=refname \ > + --sort=subject \ > + --sort=taggeremail \ > + refs/tags/icase-* >actual && > + cat >expect <<-\EOF && > + <a@example.com> tag a refs/tags/icase-00 > + <a@example.com> tag A refs/tags/icase-01 > + <A@example.com> tag a refs/tags/icase-04 > + <A@example.com> tag A refs/tags/icase-05 > + <a@example.com> tag b refs/tags/icase-02 > + <a@example.com> tag B refs/tags/icase-03 > + <A@example.com> tag b refs/tags/icase-06 > + <A@example.com> tag B refs/tags/icase-07 > + <b@example.com> tag a refs/tags/icase-08 > + <b@example.com> tag A refs/tags/icase-09 > + <B@example.com> tag a refs/tags/icase-12 > + <B@example.com> tag A refs/tags/icase-13 > + <b@example.com> tag b refs/tags/icase-10 > + <b@example.com> tag B refs/tags/icase-11 > + <B@example.com> tag b refs/tags/icase-14 > + <B@example.com> tag B refs/tags/icase-15 > + EOF > + test_cmp expect actual > +' > + > test_done > -- > 2.26.2.957.g6dc93e954a All looks very reasonable, so: Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor
diff --git a/builtin/branch.c b/builtin/branch.c index d8297f80ff..86341cc835 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -739,7 +739,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) */ if (!sorting) sorting = ref_default_sorting(); - sorting->ignore_case = icase; + ref_sorting_icase_all(sorting, icase); print_ref_list(&filter, sorting, &format); print_columns(&output, colopts, NULL); string_list_clear(&output, 0); diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 465153e853..57489e4eab 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -70,7 +70,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) if (!sorting) sorting = ref_default_sorting(); - sorting->ignore_case = icase; + ref_sorting_icase_all(sorting, icase); filter.ignore_case = icase; filter.name_patterns = argv; diff --git a/builtin/tag.c b/builtin/tag.c index dd160b49c7..ff7610b5c8 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) } if (!sorting) sorting = ref_default_sorting(); - sorting->ignore_case = icase; + ref_sorting_icase_all(sorting, icase); filter.ignore_case = icase; if (cmdmode == 'l') { int ret; diff --git a/ref-filter.c b/ref-filter.c index 35776838f4..bdb3535ce5 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2317,6 +2317,12 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting) return 0; } +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag) +{ + for (; sorting; sorting = sorting->next) + sorting->ignore_case = !!flag; +} + void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array) { QSORT_S(array->items, array->nr, compare_refs, sorting); diff --git a/ref-filter.h b/ref-filter.h index 64330e9601..8ecc33cdfa 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -114,6 +114,8 @@ void ref_array_clear(struct ref_array *array); int verify_ref_format(struct ref_format *format); /* Sort the given ref_array as per the ref_sorting provided */ void ref_array_sort(struct ref_sorting *sort, struct ref_array *array); +/* Set the ignore_case flag for all elements of a sorting list */ +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag); /* Based on the given format and quote_style, fill the strbuf */ int format_ref_array_item(struct ref_array_item *info, const struct ref_format *format, diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index b3c1092338..c9caf26327 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -895,4 +895,44 @@ test_expect_success 'for-each-ref --ignore-case ignores case' ' test_cmp expect actual ' +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' + # name refs numerically to avoid case-insensitive filesystem conflicts + nr=0 && + for email in a A b B + do + for subject in a A b B + do + GIT_COMMITTER_EMAIL="$email@example.com" \ + git tag -m "tag $subject" icase-$(printf %02d $nr) && + nr=$((nr+1))|| + return 1 + done + done && + git for-each-ref --ignore-case \ + --format="%(taggeremail) %(subject) %(refname)" \ + --sort=refname \ + --sort=subject \ + --sort=taggeremail \ + refs/tags/icase-* >actual && + cat >expect <<-\EOF && + <a@example.com> tag a refs/tags/icase-00 + <a@example.com> tag A refs/tags/icase-01 + <A@example.com> tag a refs/tags/icase-04 + <A@example.com> tag A refs/tags/icase-05 + <a@example.com> tag b refs/tags/icase-02 + <a@example.com> tag B refs/tags/icase-03 + <A@example.com> tag b refs/tags/icase-06 + <A@example.com> tag B refs/tags/icase-07 + <b@example.com> tag a refs/tags/icase-08 + <b@example.com> tag A refs/tags/icase-09 + <B@example.com> tag a refs/tags/icase-12 + <B@example.com> tag A refs/tags/icase-13 + <b@example.com> tag b refs/tags/icase-10 + <b@example.com> tag B refs/tags/icase-11 + <B@example.com> tag b refs/tags/icase-14 + <B@example.com> tag B refs/tags/icase-15 + EOF + test_cmp expect actual +' + test_done
All of the ref-filter users (for-each-ref, branch, and tag) take an --ignore-case option which makes filtering and sorting case-insensitive. However, this option was applied only to the first element of the ref_sorting list. So: git for-each-ref --ignore-case --sort=refname would do what you expect, but: git for-each-ref --ignore-case --sort=refname --sort=taggername would sort the primary key (taggername) case-insensitively, but sort the refname case-sensitively. We have two options here: - teach callers to set ignore_case on the whole list - replace the ref_sorting list with a struct that contains both the list of sorting keys, as well as options that apply to _all_ keys I went with the first one here, as it gives more flexibility if we later want to let the users set the flag per-key (presumably through some special syntax when defining the key; for now it's all or nothing through --ignore-case). The new test covers this by sorting on both tagger and subject case-insensitively, which should compare "a" and "A" identically, but still sort them before "b" and "B". We'll break ties by sorting on the refname to give ourselves a stable output (this is actually supposed to be done automatically, but there's another bug which will be fixed in the next commit). Signed-off-by: Jeff King <peff@peff.net> --- builtin/branch.c | 2 +- builtin/for-each-ref.c | 2 +- builtin/tag.c | 2 +- ref-filter.c | 6 ++++++ ref-filter.h | 2 ++ t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 51 insertions(+), 3 deletions(-)