{"thread":{"id":"49808","subject":"[PATCH 0/2] fix some exclude patterns being ignored","startedAt":"2018-11-12T13:26:37Z","lastAt":"2018-11-13T12:32:25Z","messageCount":5,"participants":["Rafael Ascensão","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"363011","messageId":"20181112132545.31092-1-rafa.almas@gmail.com","threadId":"49808","inReplyTo":null,"subject":"[PATCH 0/2] fix some exclude patterns being ignored","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2018-11-12T13:25:42Z","receivedAt":"2018-11-12T13:26:37Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"While trying to set up some aliases for my own use, I found out that\n--exclude with --branches behave differently depending if the latter\nuses globs.\n\nI tried to fix it making my 2nd contribution. :)\n\nCheers,\n\nRafael Ascensão (2):\n  refs: show --exclude failure with --branches/tags/remotes=glob\n  refs: fix some exclude patterns being ignored\n\n refs.c                   |  4 +++\n t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 61 insertions(+), 3 deletions(-)\n\n-- \n2.19.1\n\n"},{"id":"363012","messageId":"20181112132545.31092-2-rafa.almas@gmail.com","threadId":"49808","inReplyTo":"20181112132545.31092-1-rafa.almas@gmail.com","subject":"[PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2018-11-12T13:25:43Z","receivedAt":"2018-11-12T13:26:45Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"The documentation of `--exclude=` option from rev-list and rev-parse\nexplicitly states that exclude patterns *should not* start with 'refs/'\nwhen used with `--branches`, `--tags` or `--remotes`.\n\nHowever, following this advice results in refereces not being excluded\nif the next `--branches`, `--tags`, `--remotes` use the optional\ninclusive glob.\n\nDemonstrate this failure.\n\nSigned-off-by: Rafael Ascensão <rafa.almas@gmail.com>\n---\n t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 57 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex 0bf10d0686..8e2b136356 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -36,7 +36,13 @@ test_expect_success 'setup' '\n \tgit tag foo/bar master &&\n \tcommit master3 &&\n \tgit update-ref refs/remotes/foo/baz master &&\n-\tcommit master4\n+\tcommit master4 &&\n+\tgit update-ref refs/remotes/upstream/one subspace/one &&\n+\tgit update-ref refs/remotes/upstream/two subspace/two &&\n+\tgit update-ref refs/remotes/upstream/x subspace-x &&\n+\tgit tag qux/one subspace/one &&\n+\tgit tag qux/two subspace/two &&\n+\tgit tag qux/x subspace-x\n '\n \n test_expect_success 'rev-parse --glob=refs/heads/subspace/*' '\n@@ -141,6 +147,54 @@ test_expect_success 'rev-parse accumulates multiple --exclude' '\n \tcompare rev-parse \"--exclude=refs/remotes/* --exclude=refs/tags/* --all\" --branches\n '\n \n+test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '\n+\tcompare rev-parse \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n+'\n+\n+test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '\n+\tcompare rev-parse \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n+'\n+\n+test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '\n+\tcompare rev-parse \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n+'\n+\n+test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '\n+\tcompare rev-parse \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\"\n+'\n+\n+test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '\n+\tcompare rev-parse \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n+'\n+\n+test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '\n+\tcompare rev-parse \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n+'\n+\n+test_expect_failure 'rev-list --exclude=glob with --branches=glob' '\n+\tcompare rev-list \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n+'\n+\n+test_expect_failure 'rev-list --exclude=glob with --tags=glob' '\n+\tcompare rev-list \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n+'\n+\n+test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '\n+\tcompare rev-list \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n+'\n+\n+test_expect_failure 'rev-list --exclude=ref with --branches=glob' '\n+\tcompare rev-list \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\"\n+'\n+\n+test_expect_failure 'rev-list --exclude=ref with --tags=glob' '\n+\tcompare rev-list \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n+'\n+\n+test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '\n+\tcompare rev-list \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n+'\n+\n test_expect_success 'rev-list --glob=refs/heads/subspace/*' '\n \n \tcompare rev-list \"subspace/one subspace/two\" \"--glob=refs/heads/subspace/*\"\n@@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' '\n \n test_expect_success 'rev-list --tags' '\n \n-\tcompare rev-list \"foo/bar\" \"--tags\"\n+\tcompare rev-list \"foo/bar qux/x qux/two qux/one\" \"--tags\"\n \n '\n \n@@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' '\n \t  \"master other/three someref subspace-x subspace/one subspace/two\" \\\n \t  \"--glob=heads/*\" &&\n \tcompare shortlog foo/bar --tags=foo &&\n-\tcompare shortlog foo/bar --tags &&\n+\tcompare shortlog \"foo/bar qux/one qux/two qux/x\" --tags &&\n \tcompare shortlog foo/baz --remotes=foo\n \n '\n-- \n2.19.1\n\n"},{"id":"363013","messageId":"20181112132545.31092-3-rafa.almas@gmail.com","threadId":"49808","inReplyTo":"20181112132545.31092-1-rafa.almas@gmail.com","subject":"[PATCH 2/2] refs: fix some exclude patterns being ignored","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2018-11-12T13:25:44Z","receivedAt":"2018-11-12T13:26:48Z","isPatch":true,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"`--exclude` from rev-list and rev-parse fails to exclude references if\nthe next `--branches`, `--tags` or `--remotes` use the optional\ninclusive glob because those options are implemented as particular cases\nof `--glob=`, which itself requires that exclude patterns begin with\n'refs/'.\n\nBut it makes sense for `--branches=glob` and friends to be aware that\nexclusions patterns for them shouldn't be 'refs/<type>/' prefixed, the\nsame way exclude patterns for `--branches` and friends (without the\noptional glob) already are.\n\nLet's record in 'refs.c:struct ref_filter' which context the exclude\npattern is tied to, so refs.c:filter_refs() can decide if it should\nignore the prefix when trying to match.\n\nSigned-off-by: Rafael Ascensão <rafa.almas@gmail.com>\n---\n refs.c                   |  4 ++++\n t/t6018-rev-list-glob.sh | 24 ++++++++++++------------\n 2 files changed, 16 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex de81c7be7c..539f385f61 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -217,6 +217,7 @@ char *resolve_refdup(const char *refname, int resolve_flags,\n /* The argument to filter_refs */\n struct ref_filter {\n \tconst char *pattern;\n+\tconst char *prefix;\n \teach_ref_fn *fn;\n \tvoid *cb_data;\n };\n@@ -296,6 +297,8 @@ static int filter_refs(const char *refname, const struct object_id *oid,\n \n \tif (wildmatch(filter->pattern, refname, 0))\n \t\treturn 0;\n+\tif (filter->prefix)\n+\t\tskip_prefix(refname, filter->prefix, &refname);\n \treturn filter->fn(refname, oid, flags, filter->cb_data);\n }\n \n@@ -458,6 +461,7 @@ int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n \t}\n \n \tfilter.pattern = real_pattern.buf;\n+\tfilter.prefix = prefix;\n \tfilter.fn = fn;\n \tfilter.cb_data = cb_data;\n \tret = for_each_ref(filter_refs, &filter);\ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex 8e2b136356..7dc6cbdc42 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -147,51 +147,51 @@ test_expect_success 'rev-parse accumulates multiple --exclude' '\n \tcompare rev-parse \"--exclude=refs/remotes/* --exclude=refs/tags/* --all\" --branches\n '\n \n-test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '\n+test_expect_success 'rev-parse --exclude=glob with --branches=glob' '\n \tcompare rev-parse \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n '\n \n-test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '\n+test_expect_success 'rev-parse --exclude=glob with --tags=glob' '\n \tcompare rev-parse \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n '\n \n-test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '\n+test_expect_success 'rev-parse --exclude=glob with --remotes=glob' '\n \tcompare rev-parse \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n '\n \n-test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '\n+test_expect_success 'rev-parse --exclude=ref with --branches=glob' '\n \tcompare rev-parse \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\"\n '\n \n-test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '\n+test_expect_success 'rev-parse --exclude=ref with --tags=glob' '\n \tcompare rev-parse \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n '\n \n-test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '\n+test_expect_success 'rev-parse --exclude=ref with --remotes=glob' '\n \tcompare rev-parse \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n '\n \n-test_expect_failure 'rev-list --exclude=glob with --branches=glob' '\n+test_expect_success 'rev-list --exclude=glob with --branches=glob' '\n \tcompare rev-list \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n '\n \n-test_expect_failure 'rev-list --exclude=glob with --tags=glob' '\n+test_expect_success 'rev-list --exclude=glob with --tags=glob' '\n \tcompare rev-list \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n '\n \n-test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '\n+test_expect_success 'rev-list --exclude=glob with --remotes=glob' '\n \tcompare rev-list \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n '\n \n-test_expect_failure 'rev-list --exclude=ref with --branches=glob' '\n+test_expect_success 'rev-list --exclude=ref with --branches=glob' '\n \tcompare rev-list \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\"\n '\n \n-test_expect_failure 'rev-list --exclude=ref with --tags=glob' '\n+test_expect_success 'rev-list --exclude=ref with --tags=glob' '\n \tcompare rev-list \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n '\n \n-test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '\n+test_expect_success 'rev-list --exclude=ref with --remotes=glob' '\n \tcompare rev-list \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n '\n \n-- \n2.19.1\n\n"},{"id":"363131","messageId":"xmqqd0r9wm49.fsf@gitster-ct.c.googlers.com","threadId":"49808","inReplyTo":"20181112132545.31092-2-rafa.almas@gmail.com","subject":"Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T06:02:46Z","receivedAt":"2018-11-13T06:02:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Ascensão <rafa.almas@gmail.com> writes:\n\n> The documentation of `--exclude=` option from rev-list and rev-parse\n> explicitly states that exclude patterns *should not* start with 'refs/'\n> when used with `--branches`, `--tags` or `--remotes`.\n>\n> However, following this advice results in refereces not being excluded\n> if the next `--branches`, `--tags`, `--remotes` use the optional\n> inclusive glob.\n>\n> Demonstrate this failure.\n>\n> Signed-off-by: Rafael Ascensão <rafa.almas@gmail.com>\n> ---\n>  t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--\n>  1 file changed, 57 insertions(+), 3 deletions(-)\n\nFor a trivially small change/fix like this (i.e. the real fix in 2/2\nis just 4 lines), it is OK and even preferrable to make 1+2 a single\nstep, as applying t/ part only to try to see the breakage (or\n\"am\"ing everything and then \"diff | apply -R\" the part outside t/\nfor the same purpose) is easy enough.\n\nOften the patch 2 with your method ends up showing only the test\nset-up part in the context by changing _failure to _success, without\nshowing what end-user visible breakage the step fixed, which usually\ncomes near the end of the added test piece.  For this particular\ntest, s/_failure/_success/ shows everything in the verification\nphase, but the entire set-up for these tests cannot be seen while\nreviewing 2/2.  Unlike that, a single patch that gives tests that\nought to succeed would not force the readers to switch between\npatches 1 and 2 while reading the fix.\n\nOf course, the above would not apply for a more involved case where\nthe actual fix to the code needs to span multiple patches.\n\n> diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\n> index 0bf10d0686..8e2b136356 100755\n> --- a/t/t6018-rev-list-glob.sh\n> +++ b/t/t6018-rev-list-glob.sh\n> @@ -36,7 +36,13 @@ test_expect_success 'setup' '\n>  \tgit tag foo/bar master &&\n>  \tcommit master3 &&\n>  \tgit update-ref refs/remotes/foo/baz master &&\n> -\tcommit master4\n> +\tcommit master4 &&\n> +\tgit update-ref refs/remotes/upstream/one subspace/one &&\n> +\tgit update-ref refs/remotes/upstream/two subspace/two &&\n> +\tgit update-ref refs/remotes/upstream/x subspace-x &&\n> +\tgit tag qux/one subspace/one &&\n> +\tgit tag qux/two subspace/two &&\n> +\tgit tag qux/x subspace-x\n>  '\n\nLet me follow along.\n\nWe add three remote-tracking looking branches for 'upstream', and\nthree tags under refs/tags/qux/.\n\n\n> +test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '\n> +\tcompare rev-parse \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n> +'\n\nWe want to list all branches that begin with \"sub\", but we do not\nwant ones that begin with \"subspace-\".  subspace/{one,two} should\npass that criteria, while subspace-x, other/three, someref, and\nmaster should not.  Makes sense.\n\n> +\n> +test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '\n> +\tcompare rev-parse \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n> +'\n\nWe want all tags that begin with \"qux/\" but we do not want qux/\nfollowed by just a single letter.  qux/{one,two} are in, qux/x is\nout.  Makes sense.\n\n> +test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '\n> +\tcompare rev-parse \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n> +'\n\nSimilarly for refs/remotes/upstream/ hierarchy.\n\n> +test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '\n> +\tcompare rev-parse \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\n> +'\n\nThis is almost a repeat of the first new one.  As subspace-* in\nbranches only match subspace-x, this should give the same result as\nthat one.\n\n> +test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '\n> +\tcompare rev-parse \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n> +'\n\nLikewise.\n\n> +test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '\n> +\tcompare rev-parse \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n> +'\n\nLikewise.\n\n> +test_expect_failure 'rev-list --exclude=glob with --branches=glob' '\n> +\tcompare rev-list \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n> +'\n\nAnd then the same pattern continues with rev-list.\n\n> +test_expect_failure 'rev-list --exclude=glob with --tags=glob' '\n> +\tcompare rev-list \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n> +'\n> +\n> +test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '\n> +\tcompare rev-list \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n> +'\n> +\n> +test_expect_failure 'rev-list --exclude=ref with --branches=glob' '\n> +\tcompare rev-list \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\"\n> +'\n> +\n> +test_expect_failure 'rev-list --exclude=ref with --tags=glob' '\n> +\tcompare rev-list \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n> +'\n> +\n> +test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '\n> +\tcompare rev-list \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n> +'\n> +\n\nWith the ordering of these tests, it is fairly clear that you are\nexhaustively testing all the combinations \n\t\nfor command in rev-parse rev-list:\n\tfor exclude in glob ref:\n\t\tfor specifc in glob ref:\n\t\t\tfor kind in branches tags remotes:\n\t\t\t\tcompare $command exclude=$exclude --$kind=$specific\n\nwhich is very good.  No, I am not suggesting to write a shell loop\nto drive these tests; I am saying that the list of tests are in the\nsame order as such a nested loop would invoke compare, which makes\nit predictable for the readers who pay attention, and it is a good\nthing.\n\n\n>  test_expect_success 'rev-list --glob=refs/heads/subspace/*' '\n>  \n>  \tcompare rev-list \"subspace/one subspace/two\" \"--glob=refs/heads/subspace/*\"\n> @@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' '\n>  \n>  test_expect_success 'rev-list --tags' '\n>  \n> -\tcompare rev-list \"foo/bar\" \"--tags\"\n> +\tcompare rev-list \"foo/bar qux/x qux/two qux/one\" \"--tags\"\n\nOf course, you'd need to compensate for new stuff here ...\n\n>  \n>  '\n>  \n> @@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' '\n>  \t  \"master other/three someref subspace-x subspace/one subspace/two\" \\\n>  \t  \"--glob=heads/*\" &&\n>  \tcompare shortlog foo/bar --tags=foo &&\n> -\tcompare shortlog foo/bar --tags &&\n> +\tcompare shortlog \"foo/bar qux/one qux/two qux/x\" --tags &&\n\n... and here.\n\n>  \tcompare shortlog foo/baz --remotes=foo\n\nAll makes sense.  Will queue.\n\n"},{"id":"363145","messageId":"nycvar.QRO.7.76.6.1811131157060.39@tvgsbejvaqbjf.bet","threadId":"49808","inReplyTo":"xmqqd0r9wm49.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-13T12:31:58Z","receivedAt":"2018-11-13T12:32:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 13 Nov 2018, Junio C Hamano wrote:\n\n> Rafael Ascensão <rafa.almas@gmail.com> writes:\n> \n> > The documentation of `--exclude=` option from rev-list and rev-parse\n> > explicitly states that exclude patterns *should not* start with 'refs/'\n> > when used with `--branches`, `--tags` or `--remotes`.\n> >\n> > However, following this advice results in refereces not being excluded\n> > if the next `--branches`, `--tags`, `--remotes` use the optional\n> > inclusive glob.\n> >\n> > Demonstrate this failure.\n> >\n> > Signed-off-by: Rafael Ascensão <rafa.almas@gmail.com>\n> > ---\n> >  t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--\n> >  1 file changed, 57 insertions(+), 3 deletions(-)\n> \n> For a trivially small change/fix like this (i.e. the real fix in 2/2\n> is just 4 lines), it is OK and even preferrable to make 1+2 a single\n> step, as applying t/ part only to try to see the breakage (or\n> \"am\"ing everything and then \"diff | apply -R\" the part outside t/\n> for the same purpose) is easy enough.\n\nI wish you were not so adamant about this. I really consider it poor style\nto smoosh those together, and there is nothing easy about disentangling\nchanges that have been thrown into the same commit. Please stop saying\nthat this is easy. It is as easy as maintaining Linux kernel development\nusing .tar files and patches. It is possible, yes, and Linus Torvalds did\nit for years. It is also error-prone and the entire reason we have Git.\nAnd nobody wants to go back anymore to .tar files and patches. Likewise, I\ndo not want to read anybody recommending some semi-understandable\ndiff|apply-R dance when the alternative would be a simple cherry-pick. I\ndo not even want to read such a recommendation from you. I respect you a\nlot for what you do, and for your knowledge, but this is simply bad advice\nand I would wish you stopped giving it.\n\nBesides, we spent a decade trying to come up with clear-cut rules how to\norganize commits, and we ended up pretty quickly with recommending\nlogically-separate changes belonging to separate commits. A typo fix\nshould not be thrown in with a regression fix, they are two different\nthings. Likewise, demonstrating a bug is a different thing from fixing it.\n\nIf you need more arguments to make the case, here is another one: it is\nreflecting the reality a lot better if the regression test comes first,\nand then the fix. This is how Rafael did it, too, according to what he\nsaid on IRC. And reflecting this in the commit history is a good thing,\nnot a bad thing.\n\nIt goes further: obviously, Rafael had really good success with this\nstrategy, even figuring out part of the bug while trying to write the\nregression test.\n\nI, myself wrote a regression test yesterday that completely\nshort-circuited the bug hunt: originally, I thought the left-over\nMERGE_HEAD files in the rebase -r stemmed from mere conflicts during a\n`merge` command, and somehow `git commit` not cleaning it properly. But\nwhen I wrote that regression test and ran it, it failed to show a\nregression. So then I took my (rather lengthy: >200 todo commands)\nreal-world example, and condensed it into the regression test that you saw\nyesterday. I would estimate that this saved me about 1-3 hours of\ndebugging in vain.\n\nSo it is a very, very good idea to start with the regression test, and\nonly then analyze the bug.\n\nReading the commit history this way makes therefore not only sense, but\nalso sets a good example for new contributors to follow.\n\nFor these reasons, and many more, I implore you to stop suggesting to\nconflate the demonstration of a bug with the fix.\n\nInstead, we should be happy to see good practices in action and encourage\nmore of the same.\n\nThank you,\nDscho\n\n\n> Often the patch 2 with your method ends up showing only the test\n> set-up part in the context by changing _failure to _success, without\n> showing what end-user visible breakage the step fixed, which usually\n> comes near the end of the added test piece.  For this particular\n> test, s/_failure/_success/ shows everything in the verification\n> phase, but the entire set-up for these tests cannot be seen while\n> reviewing 2/2.  Unlike that, a single patch that gives tests that\n> ought to succeed would not force the readers to switch between\n> patches 1 and 2 while reading the fix.\n> \n> Of course, the above would not apply for a more involved case where\n> the actual fix to the code needs to span multiple patches.\n> \n> > diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\n> > index 0bf10d0686..8e2b136356 100755\n> > --- a/t/t6018-rev-list-glob.sh\n> > +++ b/t/t6018-rev-list-glob.sh\n> > @@ -36,7 +36,13 @@ test_expect_success 'setup' '\n> >  \tgit tag foo/bar master &&\n> >  \tcommit master3 &&\n> >  \tgit update-ref refs/remotes/foo/baz master &&\n> > -\tcommit master4\n> > +\tcommit master4 &&\n> > +\tgit update-ref refs/remotes/upstream/one subspace/one &&\n> > +\tgit update-ref refs/remotes/upstream/two subspace/two &&\n> > +\tgit update-ref refs/remotes/upstream/x subspace-x &&\n> > +\tgit tag qux/one subspace/one &&\n> > +\tgit tag qux/two subspace/two &&\n> > +\tgit tag qux/x subspace-x\n> >  '\n> \n> Let me follow along.\n> \n> We add three remote-tracking looking branches for 'upstream', and\n> three tags under refs/tags/qux/.\n> \n> \n> > +test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '\n> > +\tcompare rev-parse \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n> > +'\n> \n> We want to list all branches that begin with \"sub\", but we do not\n> want ones that begin with \"subspace-\".  subspace/{one,two} should\n> pass that criteria, while subspace-x, other/three, someref, and\n> master should not.  Makes sense.\n> \n> > +\n> > +test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '\n> > +\tcompare rev-parse \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n> > +'\n> \n> We want all tags that begin with \"qux/\" but we do not want qux/\n> followed by just a single letter.  qux/{one,two} are in, qux/x is\n> out.  Makes sense.\n> \n> > +test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '\n> > +\tcompare rev-parse \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n> > +'\n> \n> Similarly for refs/remotes/upstream/ hierarchy.\n> \n> > +test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '\n> > +\tcompare rev-parse \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\n> > +'\n> \n> This is almost a repeat of the first new one.  As subspace-* in\n> branches only match subspace-x, this should give the same result as\n> that one.\n> \n> > +test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '\n> > +\tcompare rev-parse \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n> > +'\n> \n> Likewise.\n> \n> > +test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '\n> > +\tcompare rev-parse \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n> > +'\n> \n> Likewise.\n> \n> > +test_expect_failure 'rev-list --exclude=glob with --branches=glob' '\n> > +\tcompare rev-list \"--exclude=subspace-* --branches=sub*\" \"subspace/one subspace/two\"\n> > +'\n> \n> And then the same pattern continues with rev-list.\n> \n> > +test_expect_failure 'rev-list --exclude=glob with --tags=glob' '\n> > +\tcompare rev-list \"--exclude=qux/? --tags=qux/*\" \"qux/one qux/two\"\n> > +'\n> > +\n> > +test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '\n> > +\tcompare rev-list \"--exclude=upstream/? --remotes=upstream/*\" \"upstream/one upstream/two\"\n> > +'\n> > +\n> > +test_expect_failure 'rev-list --exclude=ref with --branches=glob' '\n> > +\tcompare rev-list \"--exclude=subspace-x --branches=sub*\" \"subspace/one subspace/two\"\n> > +'\n> > +\n> > +test_expect_failure 'rev-list --exclude=ref with --tags=glob' '\n> > +\tcompare rev-list \"--exclude=qux/x --tags=qux/*\" \"qux/one qux/two\"\n> > +'\n> > +\n> > +test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '\n> > +\tcompare rev-list \"--exclude=upstream/x --remotes=upstream/*\" \"upstream/one upstream/two\"\n> > +'\n> > +\n> \n> With the ordering of these tests, it is fairly clear that you are\n> exhaustively testing all the combinations \n> \t\n> for command in rev-parse rev-list:\n> \tfor exclude in glob ref:\n> \t\tfor specifc in glob ref:\n> \t\t\tfor kind in branches tags remotes:\n> \t\t\t\tcompare $command exclude=$exclude --$kind=$specific\n> \n> which is very good.  No, I am not suggesting to write a shell loop\n> to drive these tests; I am saying that the list of tests are in the\n> same order as such a nested loop would invoke compare, which makes\n> it predictable for the readers who pay attention, and it is a good\n> thing.\n> \n> \n> >  test_expect_success 'rev-list --glob=refs/heads/subspace/*' '\n> >  \n> >  \tcompare rev-list \"subspace/one subspace/two\" \"--glob=refs/heads/subspace/*\"\n> > @@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' '\n> >  \n> >  test_expect_success 'rev-list --tags' '\n> >  \n> > -\tcompare rev-list \"foo/bar\" \"--tags\"\n> > +\tcompare rev-list \"foo/bar qux/x qux/two qux/one\" \"--tags\"\n> \n> Of course, you'd need to compensate for new stuff here ...\n> \n> >  \n> >  '\n> >  \n> > @@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' '\n> >  \t  \"master other/three someref subspace-x subspace/one subspace/two\" \\\n> >  \t  \"--glob=heads/*\" &&\n> >  \tcompare shortlog foo/bar --tags=foo &&\n> > -\tcompare shortlog foo/bar --tags &&\n> > +\tcompare shortlog \"foo/bar qux/one qux/two qux/x\" --tags &&\n> \n> ... and here.\n> \n> >  \tcompare shortlog foo/baz --remotes=foo\n> \n> All makes sense.  Will queue.\n> \n> "}]}