{"thread":{"id":"10785","subject":"[PATCH] for-each-ref: fix setup of option-parsing for --sort","startedAt":"2007-11-10T16:47:54Z","lastAt":"2007-11-11T19:19:04Z","messageCount":3,"participants":["Lars Hjemli","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"59223","messageId":"1194713274-31200-1-git-send-email-hjemli@gmail.com","threadId":"10785","inReplyTo":null,"subject":"[PATCH] for-each-ref: fix setup of option-parsing for --sort","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2007-11-10T16:47:54Z","receivedAt":"2007-11-10T16:47:54Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"The option value for --sort is already a pointer to a pointer to struct\nref_sort, so just use it.\n\nSigned-off-by: Lars Hjemli \n---\n\nOn Nov 10, 2007 5:25 PM, Johannes Schindelin wrote:\n> Could you add a test for that too, please?\n\nIs this ok?\n\n\n builtin-for-each-ref.c | 2 +-\n t/t6300-for-each-ref.sh | 22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c\nindex da8c794..e909e66 100644\n--- a/builtin-for-each-ref.c\n+++ b/builtin-for-each-ref.c\n@@ -847,7 +847,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, \"show only matched refs\"),\n \t\tOPT_STRING( 0 , \"format\", &format, \"format\", \"format to use for the output\"),\n-\t\tOPT_CALLBACK(0 , \"sort\", &sort_tail, \"key\",\n+\t\tOPT_CALLBACK(0 , \"sort\", sort_tail, \"key\",\n \t\t \"field name to sort on\", &opt_parse_sort),\n \t\tOPT_END(),\n \t};\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex d0809eb..c722635 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -148,4 +148,26 @@ test_expect_success 'Check format \"rfc2822\" date fields output' '\n \tgit diff expected actual\n '\n \n+cat >expected <<\\EOF\n+refs/heads/master\n+refs/tags/testtag\n+EOF\n+\n+test_expect_success 'Verify ascending sort' '\n+\tgit-for-each-ref --format=\"%(refname)\" --sort=refname >actual &&\n+\tgit diff expected actual\n+'\n+\n+\n+cat >expected <<\\EOF\n+refs/tags/testtag\n+refs/heads/master\n+EOF\n+\n+test_expect_success 'Verify descending sort' '\n+\tgit-for-each-ref --format=\"%(refname)\" --sort=-refname >actual &&\n+\tgit diff expected actual\n+'\n+\n+\n test_done\n-- \n1.5.3.5.623.g0a1d\n"},{"id":"59231","messageId":"7v7ikqlyfo.fsf@gitster.siamese.dyndns.org","threadId":"10785","inReplyTo":"1194713274-31200-1-git-send-email-hjemli@gmail.com","subject":"Re: [PATCH] for-each-ref: fix setup of option-parsing for --sort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-10T18:58:51Z","receivedAt":"2007-11-10T18:58:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Hjemli writes:\n\n> The option value for --sort is already a pointer to a pointer to struct\n> ref_sort, so just use it.\n>\n> Signed-off-by: Lars Hjemli \n> ---\n>\n> On Nov 10, 2007 5:25 PM, Johannes Schindelin wrote:\n>> Could you add a test for that too, please?\n>\n> Is this ok?\n>\n\nTesting \"for that\" would be kind of hard and semi pointless,\nisn't it?\n\nIf it's mismatch of the expected number of times a pointer is\ndereferenced between the caller and the callee, I'd imagine that\nit will read from and write to random place in memory and would\nlead to unpredictable behaviour. If you are lucky you would not\nget expected results but if you are unlucky who knows what would\nhappen.\n\nBut the new test makes sure --sort takes intended effect, which\nis a good thing.\n\nThanks.\n\n> builtin-for-each-ref.c | 2 +-\n> t/t6300-for-each-ref.sh | 22 ++++++++++++++++++++++\n> 2 files changed, 23 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c\n> index da8c794..e909e66 100644\n> --- a/builtin-for-each-ref.c\n> +++ b/builtin-for-each-ref.c\n> @@ -847,7 +847,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\tOPT_GROUP(\"\"),\n> \t\tOPT_INTEGER( 0 , \"count\", &maxcount, \"show only matched refs\"),\n> \t\tOPT_STRING( 0 , \"format\", &format, \"format\", \"format to use for the output\"),\n> -\t\tOPT_CALLBACK(0 , \"sort\", &sort_tail, \"key\",\n> +\t\tOPT_CALLBACK(0 , \"sort\", sort_tail, \"key\",\n> \t\t \"field name to sort on\", &opt_parse_sort),\n> \t\tOPT_END(),\n> \t};\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index d0809eb..c722635 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -148,4 +148,26 @@ test_expect_success 'Check format \"rfc2822\" date fields output' '\n> \tgit diff expected actual\n> '\n> \n> +cat >expected <<\\EOF\n> +refs/heads/master\n> +refs/tags/testtag\n> +EOF\n> +\n> +test_expect_success 'Verify ascending sort' '\n> +\tgit-for-each-ref --format=\"%(refname)\" --sort=refname >actual &&\n> +\tgit diff expected actual\n> +'\n> +\n> +\n> +cat >expected <<\\EOF\n> +refs/tags/testtag\n> +refs/heads/master\n> +EOF\n> +\n> +test_expect_success 'Verify descending sort' '\n> +\tgit-for-each-ref --format=\"%(refname)\" --sort=-refname >actual &&\n> +\tgit diff expected actual\n> +'\n> +\n> +\n> test_done\n> -- \n> 1.5.3.5.623.g0a1d\n"},{"id":"59358","messageId":"Pine.LNX.4.64.0711111918470.4362@racer.site","threadId":"10785","inReplyTo":"1194713274-31200-1-git-send-email-hjemli@gmail.com","subject":"Re: [PATCH] for-each-ref: fix setup of option-parsing for --sort","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-11T19:19:04Z","receivedAt":"2007-11-11T19:19:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 10 Nov 2007, Lars Hjemli wrote:\n\n> The option value for --sort is already a pointer to a pointer to struct\n> ref_sort, so just use it.\n> \n> Signed-off-by: Lars Hjemli \n> ---\n> \n> On Nov 10, 2007 5:25 PM, Johannes Schindelin wrote:\n> > Could you add a test for that too, please?\n> \n> Is this ok?\n\nThat's exactly what I had in mind.\n\nThank you,\nDscho\n"}]}