Message ID | 20181023191758.15138-1-agruenba@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Clear --exclude list after 'git rev-parse --all' | expand |
Andreas Gruenbacher <agruenba@redhat.com> writes: > Commit [1] added the --exclude option to revision.c. The --all, > --branches, --tags, --remotes, and --glob options clear the exclude > list. Shortly therafter, commit [2] added the same to 'git rev-parse', > but without clearing the exclude list for the --all option. Fix that. > > [1] e7b432c52 ("revision: introduce --exclude=<glob> to tame wildcards", 2013-08-30) > [2] 9dc01bf06 ("rev-parse: introduce --exclude=<glob> to tame wildcards", 2013-11-01) > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > builtin/rev-parse.c | 1 + > 1 file changed, 1 insertion(+) All other glob options do show_reference with for_each_ref_in() and then calls clear_ref_exclusion(), and logically the patch makes sense. What is the "problem" this patch fixes, though? Is it easy to add a new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or whatever that clears exclusion list without this patch) works correctly but "--all" misbehaves without this change? Thanks. > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c > index 0f09bbbf6..c71e3b104 100644 > --- a/builtin/rev-parse.c > +++ b/builtin/rev-parse.c > @@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) > } > if (!strcmp(arg, "--all")) { > for_each_ref(show_reference, NULL); > + clear_ref_exclusion(&ref_excludes); > continue; > } > if (skip_prefix(arg, "--disambiguate=", &arg)) {
On Wed, 24 Oct 2018 at 06:35, Junio C Hamano <gitster@pobox.com> wrote: > Andreas Gruenbacher <agruenba@redhat.com> writes: > > > Commit [1] added the --exclude option to revision.c. The --all, > > --branches, --tags, --remotes, and --glob options clear the exclude > > list. Shortly therafter, commit [2] added the same to 'git rev-parse', > > but without clearing the exclude list for the --all option. Fix that. > > > > [1] e7b432c52 ("revision: introduce --exclude=<glob> to tame wildcards", 2013-08-30) > > [2] 9dc01bf06 ("rev-parse: introduce --exclude=<glob> to tame wildcards", 2013-11-01) > > > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > > --- > > builtin/rev-parse.c | 1 + > > 1 file changed, 1 insertion(+) > > All other glob options do show_reference with for_each_ref_in() and > then calls clear_ref_exclusion(), and logically the patch makes > sense. > > What is the "problem" this patch fixes, though? Is it easy to add a > new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or > whatever that clears exclusion list without this patch) works > correctly but "--all" misbehaves without this change? The test suite doesn't cover clearing the exclusion list for any of those rev-parse options and I also didn't write such a test case. I ran into this inconsistency during code review. Andreas
Andreas Gruenbacher <agruenba@redhat.com> writes: >> All other glob options do show_reference with for_each_ref_in() and >> then calls clear_ref_exclusion(), and logically the patch makes >> sense. >> >> What is the "problem" this patch fixes, though? Is it easy to add a >> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or >> whatever that clears exclusion list without this patch) works >> correctly but "--all" misbehaves without this change? > > The test suite doesn't cover clearing the exclusion list for any of > those rev-parse options and I also didn't write such a test case. I > ran into this inconsistency during code review. That is why I asked what "problem" this patch fixes. Without answering that question, it is unclear if the patch is completing missing coverage for "--all", or it is cargo culting an useless clearing done for "--glob" and friends to code for "--all" that did not do the same useless clearing. IOW, there are two ways to address the "inconsistency", and the proposed log message (nor your answer above) does not make a convincing argument why adding the same code to the "--all" side is the right way to achieve consistency---rather than removing the call to clear from the existing ones. Thanks.
On Wed, 24 Oct 2018 at 11:24, Junio C Hamano <gitster@pobox.com> wrote: > Andreas Gruenbacher <agruenba@redhat.com> writes: > >> All other glob options do show_reference with for_each_ref_in() and > >> then calls clear_ref_exclusion(), and logically the patch makes > >> sense. > >> > >> What is the "problem" this patch fixes, though? Is it easy to add a > >> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or > >> whatever that clears exclusion list without this patch) works > >> correctly but "--all" misbehaves without this change? > > > > The test suite doesn't cover clearing the exclusion list for any of > > those rev-parse options and I also didn't write such a test case. I > > ran into this inconsistency during code review. > > That is why I asked what "problem" this patch fixes. Without > answering that question, it is unclear if the patch is completing > missing coverage for "--all", or it is cargo culting an useless > clearing done for "--glob" and friends to code for "--all" that did > not do the same useless clearing. This is how the --exclude option is described: --exclude=<glob-pattern> Do not include refs matching <glob-pattern> that the next --all, --branches, --tags, --remotes, or --glob would otherwise consider. Repetitions of this option accumulate exclusion patterns up to the next --all, --branches, --tags, --remotes, or --glob option (other options or arguments do not clear accumulated patterns). I'm assuming that some thought has gone into making the options behave in this particular way. The implementation in revision.c follows this pattern, but the implementation in builtin/rev-parse.c only almost does. Andreas
On Wed, Oct 24, 2018 at 11:49:06AM +0200, Andreas Gruenbacher wrote: > > That is why I asked what "problem" this patch fixes. Without > > answering that question, it is unclear if the patch is completing > > missing coverage for "--all", or it is cargo culting an useless > > clearing done for "--glob" and friends to code for "--all" that did > > not do the same useless clearing. > > This is how the --exclude option is described: > > --exclude=<glob-pattern> > Do not include refs matching <glob-pattern> that the next > --all, --branches, > --tags, --remotes, or --glob would otherwise consider. > Repetitions of this > option accumulate exclusion patterns up to the next --all, > --branches, --tags, > --remotes, or --glob option (other options or arguments do not clear > accumulated patterns). > > I'm assuming that some thought has gone into making the options behave > in this particular way. The implementation in revision.c follows this > pattern, but the implementation in builtin/rev-parse.c only almost > does. Yeah. I think this is just a bug in 9dc01bf063 (rev-parse: introduce --exclude=<glob> to tame wildcards, 2013-11-01), in that it's handling of --all differed from e7b432c521 (revision: introduce --exclude=<glob> to tame wildcards, 2013-08-30). The clearing is very much intentional and documented, and in general rev-parse should behave the same as rev-list. An easy test is: $ git rev-list --no-walk --exclude='*' --all --all 3289ca716320457af5d2dd550b716282ad22da11 ...a bunch of other tip commits... $ git rev-parse --exclude='*' --all --all [no output, but it should print those same tip commits] -Peff
Jeff King <peff@peff.net> writes: > An easy test is: > > $ git rev-list --no-walk --exclude='*' --all --all > 3289ca716320457af5d2dd550b716282ad22da11 > ...a bunch of other tip commits... > > $ git rev-parse --exclude='*' --all --all > [no output, but it should print those same tip commits] I actually was hoping to see a test that contrasts "--all" (which lacks the alleged "clear exclude" bug) with another option that does have the "clear exclude", both used with rev-parse, i.e. $ git rev-parse --exclude='*' --glob='*' --glob='*' ... all the ref tips ... $ git rev-parse --exclude='*' --all --all ... ought to be equivalent, but is empty due to the bug ... would have been a good demonstration that shows what bug we are fixing d(and would have been a good test to accompany the patch.
On Fri, Oct 26, 2018 at 08:43:37AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > An easy test is: > > > > $ git rev-list --no-walk --exclude='*' --all --all > > 3289ca716320457af5d2dd550b716282ad22da11 > > ...a bunch of other tip commits... > > > > $ git rev-parse --exclude='*' --all --all > > [no output, but it should print those same tip commits] > > I actually was hoping to see a test that contrasts "--all" (which > lacks the alleged "clear exclude" bug) with another option that does > have the "clear exclude", both used with rev-parse, i.e. > > $ git rev-parse --exclude='*' --glob='*' --glob='*' > ... all the ref tips ... > $ git rev-parse --exclude='*' --all --all > ... ought to be equivalent, but is empty due to the bug ... > > would have been a good demonstration that shows what bug we are > fixing d(and would have been a good test to accompany the patch. Yeah, I agree that would be fine, too. I think there are two dimensions in which to look at the problem, like so: rev-list rev-parse -------- --------- --glob clears clears --all clears does not clear Testing either the row or the column (or both) works for me. :) -Peff
Jeff King <peff@peff.net> writes: >> I actually was hoping to see a test that contrasts "--all" (which >> lacks the alleged "clear exclude" bug) with another option that does >> have the "clear exclude", both used with rev-parse, i.e. >> >> $ git rev-parse --exclude='*' --glob='*' --glob='*' >> ... all the ref tips ... >> $ git rev-parse --exclude='*' --all --all >> ... ought to be equivalent, but is empty due to the bug ... >> >> would have been a good demonstration that shows what bug we are >> fixing d(and would have been a good test to accompany the patch. > > Yeah, I agree that would be fine, too. I think there are two dimensions > in which to look at the problem, like so: > > rev-list rev-parse > -------- --------- > --glob clears clears > --all clears does not clear > > Testing either the row or the column (or both) works for me. :) OK, so let's not leave this loose end untied. This may be good enough to squash in. builtin/rev-parse.c | 1 - t/t6018-rev-list-glob.sh | 12 ++++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index f4847d3008..a1e680b5e9 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -760,7 +760,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) } if (!strcmp(arg, "--all")) { for_each_ref(show_reference, NULL); - clear_ref_exclusion(&ref_excludes); continue; } if (skip_prefix(arg, "--disambiguate=", &arg)) { diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh index d3453c583c..b28075b65d 100755 --- a/t/t6018-rev-list-glob.sh +++ b/t/t6018-rev-list-glob.sh @@ -141,6 +141,18 @@ test_expect_success 'rev-parse accumulates multiple --exclude' ' compare rev-parse "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches ' +test_expect_success 'rev-parse --branches clears --exclude' ' + compare rev-parse "--exclude=* --branches --branches" "--branches" +' + +test_expect_success 'rev-parse --tags clears --exclude' ' + compare rev-parse "--exclude=* --tags --tags" "--tags" +' + +test_expect_success 'rev-parse --all clears --exclude' ' + compare rev-parse "--exclude=* --all --all" "--all" +' + test_expect_success 'rev-list --glob=refs/heads/subspace/*' ' compare rev-list "subspace/one subspace/two" "--glob=refs/heads/subspace/*"
On Sat, Oct 27, 2018 at 04:12:40PM +0900, Junio C Hamano wrote: > > Yeah, I agree that would be fine, too. I think there are two dimensions > > in which to look at the problem, like so: > > > > rev-list rev-parse > > -------- --------- > > --glob clears clears > > --all clears does not clear > > > > Testing either the row or the column (or both) works for me. :) > > OK, so let's not leave this loose end untied. This may be good > enough to squash in. > [...] > +test_expect_success 'rev-parse --branches clears --exclude' ' > + compare rev-parse "--exclude=* --branches --branches" "--branches" > +' > + > +test_expect_success 'rev-parse --tags clears --exclude' ' > + compare rev-parse "--exclude=* --tags --tags" "--tags" > +' > + > +test_expect_success 'rev-parse --all clears --exclude' ' > + compare rev-parse "--exclude=* --all --all" "--all" > +' Yes, this looks good to me. In theory a more intricate test might catch other kinds of bugs (e.g., a more limited exclude and making sure it was applied correctly in each place), but I don't think it's really worth the effort. -Peff
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 0f09bbbf6..c71e3b104 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) } if (!strcmp(arg, "--all")) { for_each_ref(show_reference, NULL); + clear_ref_exclusion(&ref_excludes); continue; } if (skip_prefix(arg, "--disambiguate=", &arg)) {
Commit [1] added the --exclude option to revision.c. The --all, --branches, --tags, --remotes, and --glob options clear the exclude list. Shortly therafter, commit [2] added the same to 'git rev-parse', but without clearing the exclude list for the --all option. Fix that. [1] e7b432c52 ("revision: introduce --exclude=<glob> to tame wildcards", 2013-08-30) [2] 9dc01bf06 ("rev-parse: introduce --exclude=<glob> to tame wildcards", 2013-11-01) Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- builtin/rev-parse.c | 1 + 1 file changed, 1 insertion(+)