{"thread":{"id":"31287","subject":"[PATCH 1/2] t6300: test sort with multiple keys","startedAt":"2012-08-19T21:15:03Z","lastAt":"2012-08-21T21:33:57Z","messageCount":8,"participants":["Kacper Kornet","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"197301","messageId":"e5b3ab37553f384235f3cb14e42f7e2b56507bde.1345410836.git.draenog@pld-linux.org","threadId":"31287","inReplyTo":null,"subject":"[PATCH 1/2] t6300: test sort with multiple keys","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-08-19T21:15:03Z","receivedAt":"2012-08-19T21:15:03Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"Documentation of git-for-each-ref says that --sort= option can be\nused multiple times, in which case the last key becomes the primary key.\nHowever this functionality was never checked in test suite and is\ncurrently broken. This commit adds appropriate test in preparation for fix.\n\nSigned-off-by: Kacper Kornet \n---\n t/t6300-for-each-ref.sh | 33 ++++++++++++++++++++++++++++++++-\n 1 file changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 1721784..3d59bfc 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -242,7 +242,32 @@ test_expect_success 'Verify descending sort' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'Create branches to test sort with multiple keys' '\n+\tgit checkout -b Branch1 &&\n+\techo foo >> one &&\n+\tgit commit -a -m \"Branch1 commit\" &&\n+\tgit checkout -b Branch2 &&\n+\techo foo >> one &&\n+\tgit commit -a -m \"Branch2 commit\"\n+'\n+\n+test_atom refs/heads/Branch1 objectname 32fca05e9f638021a123a84226acf17756acc18b\n+test_atom refs/heads/Branch2 objectname 194a5b89ac661a114566ba4374bc06c2797539f3\n+\n cat >expected <<\\EOF\n+67a36f10722846e891fbada1ba48ed035de75581 commit\trefs/heads/master\n+194a5b89ac661a114566ba4374bc06c2797539f3 commit\trefs/heads/Branch2\n+32fca05e9f638021a123a84226acf17756acc18b commit\trefs/heads/Branch1\n+EOF\n+\n+test_expect_failure 'Verify sort with multiple keys' '\n+\tgit for-each-ref --sort=objectname --sort=committerdate refs/heads > actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+'refs/heads/Branch1'\n+'refs/heads/Branch2'\n 'refs/heads/master'\n 'refs/remotes/origin/master'\n 'refs/tags/testtag'\n@@ -264,6 +289,8 @@ test_expect_success 'Quoting style: python' '\n '\n \n cat >expected <<\\EOF\n+\"refs/heads/Branch1\"\n+\"refs/heads/Branch2\"\n \"refs/heads/master\"\n \"refs/remotes/origin/master\"\n \"refs/tags/testtag\"\n@@ -285,6 +312,8 @@ for i in \"--perl --shell\" \"-s --python\" \"--python --tcl\" \"--tcl --perl\"; do\n done\n \n cat >expected <<\\EOF\n+Branch1\n+Branch2\n master\n testtag\n EOF\n@@ -296,6 +325,8 @@ test_expect_success 'Check short refname format' '\n '\n \n cat >expected <expected <actual &&\n+\tgit for-each-ref --format=\"%(objectname:short)\" refs/heads/master >actual &&\n \ttest_cmp expected actual\n '\n \n-- \n1.7.12.rc3\n"},{"id":"197302","messageId":"2b3624458d79a1ec0b1437172437fbd78b3a0537.1345410836.git.draenog@pld-linux.org","threadId":"31287","inReplyTo":"e5b3ab37553f384235f3cb14e42f7e2b56507bde.1345410836.git.draenog@pld-linux.org","subject":"[PATCH 2/2] for-each-ref: Fix sort with multiple keys","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-08-19T21:15:44Z","receivedAt":"2012-08-19T21:15:44Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"The linked list describing sort options was not correctly set up in\nopt_parse_sort. In the result, contrary to the documentation. only the\nlast of multiple --sort options to git-for-each-ref was taken into\naccount. This commit fixes it.\n\nSigned-off-by: Kacper Kornet \n---\n builtin/for-each-ref.c | 4 +++-\n t/t6300-for-each-ref.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex b01d76a..0c5294e 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -962,7 +962,9 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset)\n \tif (!arg) /* should --no-sort void the list ? */\n \t\treturn -1;\n \n-\t*sort_tail = s = xcalloc(1, sizeof(*s));\n+\ts = xcalloc(1, sizeof(*s));\n+\ts->next = *sort_tail;\n+\t*sort_tail = s;\n \n \tif (*arg == '-') {\n \t\ts->reverse = 1;\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 3d59bfc..4c5d8ba 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -260,7 +260,7 @@ cat >expected <<\\EOF\n 32fca05e9f638021a123a84226acf17756acc18b commit\trefs/heads/Branch1\n EOF\n \n-test_expect_failure 'Verify sort with multiple keys' '\n+test_expect_success 'Verify sort with multiple keys' '\n \tgit for-each-ref --sort=objectname --sort=committerdate refs/heads > actual &&\n \ttest_cmp expected actual\n '\n-- \n1.7.12.rc3\n"},{"id":"197310","messageId":"7vk3wuo0sa.fsf@alter.siamese.dyndns.org","threadId":"31287","inReplyTo":"e5b3ab37553f384235f3cb14e42f7e2b56507bde.1345410836.git.draenog@pld-linux.org","subject":"Re: [PATCH 1/2] t6300: test sort with multiple keys","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T00:38:29Z","receivedAt":"2012-08-20T00:38:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kacper Kornet writes:\n\n> Documentation of git-for-each-ref says that --sort= option can be\n> used multiple times, in which case the last key becomes the primary key.\n> However this functionality was never checked in test suite and is\n> currently broken. This commit adds appropriate test in preparation for fix.\n>\n> Signed-off-by: Kacper Kornet \n> ---\n\nThanks.\n\n> +test_expect_success 'Create branches to test sort with multiple keys' '\n> +\tgit checkout -b Branch1 &&\n> +\techo foo >> one &&\n> +\tgit commit -a -m \"Branch1 commit\" &&\n> +\tgit checkout -b Branch2 &&\n> +\techo foo >> one &&\n> +\tgit commit -a -m \"Branch2 commit\"\n> +'\n> +\n> +test_atom refs/heads/Branch1 objectname 32fca05e9f638021a123a84226acf17756acc18b\n> +test_atom refs/heads/Branch2 objectname 194a5b89ac661a114566ba4374bc06c2797539f3\n\nDo these need to be \"Branch[12]\", not \"branch[12]\" for the code to\nexhibit the bug? If not, please don't be creative in names like\nthese. On case corrupting filesystems you may write Branch1 and\nthey may come back as branch1, but that is not what we are testing\nhere.\n\nAlso, style: redirection sticks to the target file, e.g.\n\n\techo foo >>one &&\n\n> +\n> cat >expected <<\\EOF\n> +67a36f10722846e891fbada1ba48ed035de75581 commit\trefs/heads/master\n> +194a5b89ac661a114566ba4374bc06c2797539f3 commit\trefs/heads/Branch2\n> +32fca05e9f638021a123a84226acf17756acc18b commit\trefs/heads/Branch1\n> +EOF\n> +\n> +test_expect_failure 'Verify sort with multiple keys' '\n> +\tgit for-each-ref --sort=objectname --sort=committerdate refs/heads > actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +cat >expected <<\\EOF\n> +'refs/heads/Branch1'\n> +'refs/heads/Branch2'\n> 'refs/heads/master'\n> 'refs/remotes/origin/master'\n> 'refs/tags/testtag'\n> @@ -264,6 +289,8 @@ test_expect_success 'Quoting style: python' '\n> '\n> \n> cat >expected <<\\EOF\n> +\"refs/heads/Branch1\"\n> +\"refs/heads/Branch2\"\n> \"refs/heads/master\"\n> \"refs/remotes/origin/master\"\n> \"refs/tags/testtag\"\n> @@ -285,6 +312,8 @@ for i in \"--perl --shell\" \"-s --python\" \"--python --tcl\" \"--tcl --perl\"; do\n> done\n> \n> cat >expected <<\\EOF\n> +Branch1\n> +Branch2\n> master\n> testtag\n> EOF\n> @@ -296,6 +325,8 @@ test_expect_success 'Check short refname format' '\n> '\n> \n> cat >expected < +\n> +\n> origin/master\n\nWhat are these blank line outputs?\n\n> EOF\n> \n> @@ -309,7 +340,7 @@ cat >expected < EOF\n> \n> test_expect_success 'Check short objectname format' '\n> -\tgit for-each-ref --format=\"%(objectname:short)\" refs/heads >actual &&\n> +\tgit for-each-ref --format=\"%(objectname:short)\" refs/heads/master >actual &&\n> \ttest_cmp expected actual\n> '\n\nAll in all, I have to wonder if you can limit the updates to other\nunrelated tests if you added a new test near the end. Also doesn't\nthe existing test already create enough refs to let you sort with\nmultiple keys and demonstrate the breakage already, without adding\nnew refs and objects?\n"},{"id":"197311","messageId":"7vfw7io0ny.fsf@alter.siamese.dyndns.org","threadId":"31287","inReplyTo":"2b3624458d79a1ec0b1437172437fbd78b3a0537.1345410836.git.draenog@pld-linux.org","subject":"Re: [PATCH 2/2] for-each-ref: Fix sort with multiple keys","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T00:41:05Z","receivedAt":"2012-08-20T00:41:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kacper Kornet writes:\n\n> The linked list describing sort options was not correctly set up in\n> opt_parse_sort. In the result, contrary to the documentation. only the\n> last of multiple --sort options to git-for-each-ref was taken into\n> account. This commit fixes it.\n>\n> Signed-off-by: Kacper Kornet \n> ---\n> builtin/for-each-ref.c | 4 +++-\n> t/t6300-for-each-ref.sh | 2 +-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index b01d76a..0c5294e 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -962,7 +962,9 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset)\n> \tif (!arg) /* should --no-sort void the list ? */\n> \t\treturn -1;\n> \n> -\t*sort_tail = s = xcalloc(1, sizeof(*s));\n> +\ts = xcalloc(1, sizeof(*s));\n> +\ts->next = *sort_tail;\n> +\t*sort_tail = s;\n\nThis fix looks correct. \n"},{"id":"197320","messageId":"20120820052429.GF1076@camk.edu.pl","threadId":"31287","inReplyTo":"7vk3wuo0sa.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t6300: test sort with multiple keys","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-08-20T05:24:29Z","receivedAt":"2012-08-20T05:24:29Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"On Sun, Aug 19, 2012 at 05:38:29PM -0700, Junio C Hamano wrote:\n> Kacper Kornet writes:\n\n> > Documentation of git-for-each-ref says that --sort= option can be\n> > used multiple times, in which case the last key becomes the primary key.\n> > However this functionality was never checked in test suite and is\n> > currently broken. This commit adds appropriate test in preparation for fix.\n\n> > Signed-off-by: Kacper Kornet \n> > ---\n\n> Thanks.\n\n> > +test_expect_success 'Create branches to test sort with multiple keys' '\n> > +\tgit checkout -b Branch1 &&\n> > +\techo foo >> one &&\n> > +\tgit commit -a -m \"Branch1 commit\" &&\n> > +\tgit checkout -b Branch2 &&\n> > +\techo foo >> one &&\n> > +\tgit commit -a -m \"Branch2 commit\"\n> > +'\n> > +\n> > +test_atom refs/heads/Branch1 objectname 32fca05e9f638021a123a84226acf17756acc18b\n> > +test_atom refs/heads/Branch2 objectname 194a5b89ac661a114566ba4374bc06c2797539f3\n\n> Do these need to be \"Branch[12]\", not \"branch[12]\" for the code to\n> exhibit the bug? If not, please don't be creative in names like\n> these. On case corrupting filesystems you may write Branch1 and\n> they may come back as branch1, but that is not what we are testing\n> here.\n\nBranches names can be lowercased. Only the commit messages should be\npreserved as they produce the test depends on the lexicographical order\nof created SHA1s.\n\n> > @@ -296,6 +325,8 @@ test_expect_success 'Check short refname format' '\n> > '\n\n> > cat >expected < > +\n> > +\n> > origin/master\n\n> What are these blank line outputs?\n\nThe upstreams of Branch1 and Branch2.\n\n> > EOF\n\n> > @@ -309,7 +340,7 @@ cat >expected < > EOF\n\n> > test_expect_success 'Check short objectname format' '\n> > -\tgit for-each-ref --format=\"%(objectname:short)\" refs/heads >actual &&\n> > +\tgit for-each-ref --format=\"%(objectname:short)\" refs/heads/master >actual &&\n> > \ttest_cmp expected actual\n> > '\n\n> All in all, I have to wonder if you can limit the updates to other\n> unrelated tests if you added a new test near the end. Also doesn't\n> the existing test already create enough refs to let you sort with\n> multiple keys and demonstrate the breakage already, without adding new\n> refs and objects?\n\nMy intention was to group all tests to sort in one place. But if the\npreferred place for a new one is at the end, then it is possible to find\nthe adequate refs among existing ones.\n\n-- \n Kacper Kornet\n"},{"id":"197515","messageId":"91678e1e50f23bdb2c3b2c5716f92d870a233e77.1345534654.git.draenog@pld-linux.org","threadId":"31287","inReplyTo":"7vk3wuo0sa.fsf@alter.siamese.dyndns.org","subject":"[PATCHv2 1/2] t6300: test sort with multiple keys","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-08-21T07:46:06Z","receivedAt":"2012-08-21T07:46:06Z","isPatch":false,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"Documentation of git-for-each-ref says that --sort= option can be\nused multiple times, in which case the last key becomes the primary key.\nHowever this functionality was never checked in test suite and is\ncurrently broken. This commit adds appropriate test in preparation for fix.\n\nSigned-off-by: Kacper Kornet \n---\n t/t6300-for-each-ref.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 1721784..a0d82d4 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -456,4 +456,14 @@ test_atom refs/tags/signed-long contents \"subject line\n body contents\n $sig\"\n \n+cat >expected <<\\EOF\n+408fe76d02a785a006c2e9c669b7be5589ede96d refs/tags/master\n+90b5ebede4899eda64893bc2a4c8f1d6fb6dfc40 refs/tags/bogo\n+EOF\n+\n+test_expect_failure 'Verify sort with multiple keys' '\n+\tgit for-each-ref --format=\"%(objectname) %(taggeremail) %(refname)\" --sort=objectname --sort=taggeremail \\\n+\t\trefs/tags/bogo refs/tags/master > actual &&\n+\ttest_cmp expected actual\n+'\n test_done\n-- \n1.7.12.rc3\n\n\n-- \n Kacper Kornet\n"},{"id":"197516","messageId":"fb5ad43b1c1ed627412ea6695875c66d6454cc0e.1345534654.git.draenog@pld-linux.org","threadId":"31287","inReplyTo":"91678e1e50f23bdb2c3b2c5716f92d870a233e77.1345534654.git.draenog@pld-linux.org","subject":"[PATCHv2 2/2] for-each-ref: Fix sort with multiple keys","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-08-21T07:47:26Z","receivedAt":"2012-08-21T07:47:26Z","isPatch":false,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"The linked list describing sort options was not correctly set up in\nopt_parse_sort. In the result, contrary to the documentation, only the\nlast of multiple --sort options to git-for-each-ref was taken into\naccount. This commit fixes it.\n\nSigned-off-by: Kacper Kornet \n---\n builtin/for-each-ref.c | 4 +++-\n t/t6300-for-each-ref.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex b01d76a..0c5294e 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -962,7 +962,9 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset)\n \tif (!arg) /* should --no-sort void the list ? */\n \t\treturn -1;\n \n-\t*sort_tail = s = xcalloc(1, sizeof(*s));\n+\ts = xcalloc(1, sizeof(*s));\n+\ts->next = *sort_tail;\n+\t*sort_tail = s;\n \n \tif (*arg == '-') {\n \t\ts->reverse = 1;\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex a0d82d4..752f5cb 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -461,7 +461,7 @@ cat >expected <<\\EOF\n 90b5ebede4899eda64893bc2a4c8f1d6fb6dfc40 refs/tags/bogo\n EOF\n \n-test_expect_failure 'Verify sort with multiple keys' '\n+test_expect_success 'Verify sort with multiple keys' '\n \tgit for-each-ref --format=\"%(objectname) %(taggeremail) %(refname)\" --sort=objectname --sort=taggeremail \\\n \t\trefs/tags/bogo refs/tags/master > actual &&\n \ttest_cmp expected actual\n-- \n1.7.12.rc3\n"},{"id":"197555","messageId":"7vobm4c4l6.fsf@alter.siamese.dyndns.org","threadId":"31287","inReplyTo":"91678e1e50f23bdb2c3b2c5716f92d870a233e77.1345534654.git.draenog@pld-linux.org","subject":"Re: [PATCHv2 1/2] t6300: test sort with multiple keys","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T21:33:57Z","receivedAt":"2012-08-21T21:33:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kacper Kornet writes:\n\n> Documentation of git-for-each-ref says that --sort= option can be\n> used multiple times, in which case the last key becomes the primary key.\n> However this functionality was never checked in test suite and is\n> currently broken. This commit adds appropriate test in preparation for fix.\n>\n> Signed-off-by: Kacper Kornet \n> ---\n> t/t6300-for-each-ref.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n\nMuch nicer and concise. It would have been even better if it didn't\nhave to depend on exact object names, but the existing tests already\ndepend on them, so it is not making things worse.\n\nThanks. Will queue.\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index 1721784..a0d82d4 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -456,4 +456,14 @@ test_atom refs/tags/signed-long contents \"subject line\n> body contents\n> $sig\"\n> \n> +cat >expected <<\\EOF\n> +408fe76d02a785a006c2e9c669b7be5589ede96d refs/tags/master\n> +90b5ebede4899eda64893bc2a4c8f1d6fb6dfc40 refs/tags/bogo\n> +EOF\n> +\n> +test_expect_failure 'Verify sort with multiple keys' '\n> +\tgit for-each-ref --format=\"%(objectname) %(taggeremail) %(refname)\" --sort=objectname --sort=taggeremail \\\n> +\t\trefs/tags/bogo refs/tags/master > actual &&\n> +\ttest_cmp expected actual\n> +'\n> test_done\n> -- \n> 1.7.12.rc3\n"}]}