{"thread":{"id":"37325","subject":"[PATCH 1/2] blame.c: Add translation to warning about failed revision walk","startedAt":"2014-08-10T21:33:25Z","lastAt":"2014-08-12T16:57:58Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"247566","messageId":"1407706406-30455-1-git-send-email-stefanbeller@gmail.com","threadId":"37325","inReplyTo":null,"subject":"[PATCH 1/2] blame.c: Add translation to warning about failed revision walk","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-10T21:33:25Z","receivedAt":"2014-08-10T21:33:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"blame belonging to the group of\nancillaryinterrogators and not to plumbinginterrogators\nshould have localized error messages?\n\nSigned-off-by: Stefan Beller <stefanbeller@gmail.com>\n---\n builtin/blame.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 17d30d0..ca4ba6f 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2700,7 +2700,7 @@ parse_done:\n \t * uninteresting.\n \t */\n \tif (prepare_revision_walk(&revs))\n-\t\tdie(\"revision walk setup failed\");\n+\t\tdie(_(\"revision walk setup failed\"));\n \n \tif (is_null_sha1(sb.final->object.sha1)) {\n \t\to = sb.final->util;\n-- \n2.1.0.rc2\n"},{"id":"247567","messageId":"1407706406-30455-2-git-send-email-stefanbeller@gmail.com","threadId":"37325","inReplyTo":"1407706406-30455-1-git-send-email-stefanbeller@gmail.com","subject":"[PATCH 2/2] prepare_revision_walk: Check for return value in all places","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-10T21:33:26Z","receivedAt":"2014-08-10T21:33:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Even the documentation tells us:\n\tYou should check if it\n\treturns any error (non-zero return code) and if it does not, you can\n\tstart using get_revision() to do the iteration.\n\nIn preparation for this commit, I grepped all occurrences of\nprepare_revision_walk and added error messages, when there were none.\n\nSigned-off-by: Stefan Beller <stefanbeller@gmail.com>\n---\n builtin/branch.c | 4 +++-\n builtin/commit.c | 3 ++-\n remote.c         | 3 ++-\n 3 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0591b22..ced422b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -653,7 +653,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \t\tadd_pending_object(&ref_list.revs,\n \t\t\t\t   (struct object *) filter, \"\");\n \t\tref_list.revs.limited = 1;\n-\t\tprepare_revision_walk(&ref_list.revs);\n+\n+\t\tif (prepare_revision_walk(&ref_list.revs))\n+\t\t\tdie(_(\"revision walk setup failed\"));\n \t\tif (verbose)\n \t\t\tref_list.maxwidth = calc_maxwidth(&ref_list);\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7867768..bb84e1d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1055,7 +1055,8 @@ static const char *find_author_by_nickname(const char *name)\n \trevs.mailmap = &mailmap;\n \tread_mailmap(revs.mailmap, NULL);\n \n-\tprepare_revision_walk(&revs);\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(\"revision walk setup failed\");\n \tcommit = get_revision(&revs);\n \tif (commit) {\n \t\tstruct pretty_print_context ctx = {0};\ndiff --git a/remote.c b/remote.c\nindex 894db09..112e4d5 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1893,7 +1893,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \n \tinit_revisions(&revs, NULL);\n \tsetup_revisions(rev_argc, rev_argv, &revs, NULL);\n-\tprepare_revision_walk(&revs);\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(\"revision walk setup failed\");\n \n \t/* ... and count the commits on each side. */\n \t*num_ours = 0;\n-- \n2.1.0.rc2\n"},{"id":"247590","messageId":"xmqq1tsm7si5.fsf@gitster.dls.corp.google.com","threadId":"37325","inReplyTo":"1407706406-30455-2-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH 2/2] prepare_revision_walk: Check for return value in all places","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-11T19:09:38Z","receivedAt":"2014-08-11T19:09:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@gmail.com> writes:\n\n> Even the documentation tells us:\n> \tYou should check if it\n> \treturns any error (non-zero return code) and if it does not, you can\n> \tstart using get_revision() to do the iteration.\n>\n> In preparation for this commit, I grepped all occurrences of\n> prepare_revision_walk and added error messages, when there were none.\n\nThanks.  One niggling thing is that I do not seem to find a way for\nthe function to actually return an error without dying in it, but\nthese are independently good changes.\n\n>\n> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>\n> ---\n>  builtin/branch.c | 4 +++-\n>  builtin/commit.c | 3 ++-\n>  remote.c         | 3 ++-\n>  3 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 0591b22..ced422b 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -653,7 +653,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n>  \t\tadd_pending_object(&ref_list.revs,\n>  \t\t\t\t   (struct object *) filter, \"\");\n>  \t\tref_list.revs.limited = 1;\n> -\t\tprepare_revision_walk(&ref_list.revs);\n> +\n> +\t\tif (prepare_revision_walk(&ref_list.revs))\n> +\t\t\tdie(_(\"revision walk setup failed\"));\n>  \t\tif (verbose)\n>  \t\t\tref_list.maxwidth = calc_maxwidth(&ref_list);\n>  \t}\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 7867768..bb84e1d 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1055,7 +1055,8 @@ static const char *find_author_by_nickname(const char *name)\n>  \trevs.mailmap = &mailmap;\n>  \tread_mailmap(revs.mailmap, NULL);\n>  \n> -\tprepare_revision_walk(&revs);\n> +\tif (prepare_revision_walk(&revs))\n> +\t\tdie(\"revision walk setup failed\");\n>  \tcommit = get_revision(&revs);\n>  \tif (commit) {\n>  \t\tstruct pretty_print_context ctx = {0};\n> diff --git a/remote.c b/remote.c\n> index 894db09..112e4d5 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1893,7 +1893,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n>  \n>  \tinit_revisions(&revs, NULL);\n>  \tsetup_revisions(rev_argc, rev_argv, &revs, NULL);\n> -\tprepare_revision_walk(&revs);\n> +\tif (prepare_revision_walk(&revs))\n> +\t\tdie(\"revision walk setup failed\");\n>  \n>  \t/* ... and count the commits on each side. */\n>  \t*num_ours = 0;\n"},{"id":"247591","messageId":"53E9184A.8030504@gmail.com","threadId":"37325","inReplyTo":"xmqq1tsm7si5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] prepare_revision_walk: Check for return value in all places","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-11T19:23:54Z","receivedAt":"2014-08-11T19:23:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 11.08.2014 21:09, Junio C Hamano wrote:\n> Stefan Beller <stefanbeller@gmail.com> writes:\n> \n>> Even the documentation tells us:\n>> \tYou should check if it\n>> \treturns any error (non-zero return code) and if it does not, you can\n>> \tstart using get_revision() to do the iteration.\n>>\n>> In preparation for this commit, I grepped all occurrences of\n>> prepare_revision_walk and added error messages, when there were none.\n> \n> Thanks.  One niggling thing is that I do not seem to find a way for\n> the function to actually return an error without dying in it, but\n> these are independently good changes.\n> \n>>\n>> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>\n>> ---\n>>  builtin/branch.c | 4 +++-\n>>  builtin/commit.c | 3 ++-\n>>  remote.c         | 3 ++-\n>>  3 files changed, 7 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/branch.c b/builtin/branch.c\n>> index 0591b22..ced422b 100644\n>> --- a/builtin/branch.c\n>> +++ b/builtin/branch.c\n>> @@ -653,7 +653,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n>>  \t\tadd_pending_object(&ref_list.revs,\n>>  \t\t\t\t   (struct object *) filter, \"\");\n>>  \t\tref_list.revs.limited = 1;\n>> -\t\tprepare_revision_walk(&ref_list.revs);\n>> +\n>> +\t\tif (prepare_revision_walk(&ref_list.revs))\n>> +\t\t\tdie(_(\"revision walk setup failed\"));\n>>  \t\tif (verbose)\n>>  \t\t\tref_list.maxwidth = calc_maxwidth(&ref_list);\n>>  \t}\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 7867768..bb84e1d 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1055,7 +1055,8 @@ static const char *find_author_by_nickname(const char *name)\n>>  \trevs.mailmap = &mailmap;\n>>  \tread_mailmap(revs.mailmap, NULL);\n>>  \n>> -\tprepare_revision_walk(&revs);\n>> +\tif (prepare_revision_walk(&revs))\n>> +\t\tdie(\"revision walk setup failed\");\n\nJust reviewed the patch myself and here in commit.c we should have a\nlocalized version, right?\nShould I resend the patch with the localisation or could you just amend\nthat?\n\n>>  \tcommit = get_revision(&revs);\n>>  \tif (commit) {\n>>  \t\tstruct pretty_print_context ctx = {0};\n>> diff --git a/remote.c b/remote.c\n>> index 894db09..112e4d5 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1893,7 +1893,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n>>  \n>>  \tinit_revisions(&revs, NULL);\n>>  \tsetup_revisions(rev_argc, rev_argv, &revs, NULL);\n>> -\tprepare_revision_walk(&revs);\n>> +\tif (prepare_revision_walk(&revs))\n>> +\t\tdie(\"revision walk setup failed\");\n>>  \n>>  \t/* ... and count the commits on each side. */\n>>  \t*num_ours = 0;\n"},{"id":"247614","messageId":"xmqqppg563xl.fsf@gitster.dls.corp.google.com","threadId":"37325","inReplyTo":"1407706406-30455-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH 1/2] blame.c: Add translation to warning about failed revision walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-12T16:57:58Z","receivedAt":"2014-08-12T16:57:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@gmail.com> writes:\n\n> blame belonging to the group of\n> ancillaryinterrogators and not to plumbinginterrogators\n> should have localized error messages?\n\nUnless running under --porcelain option to be driven by scripts, we\nexpect that we are talking to a human user, so using \"_(msg)\" is very\nmuch appropriate for that case.\n\nA possibly problematic script might do something like this:\n\n\tgit blame --porcelain \"$1\" 2>&1 |\n        awk \"$awkScript\"\n\nand the $awkScript may check the input lines that do not match the\nexpected pattern the output lines from the command follow and act on\nthem, though.  _(msg) is unwelcome to such a script [*1*].\n\nI suspect the above problem is likely to be theoretical.  People\nwould be more sloppy and write this instead:\n\n\tgit blame --porcelain \"$1\" |\n        awk \"$awkScript\"\n\nand let the problem pass unnoticed, affecting the later parts of\ntheir processing ;-).  And \"_(msg)\", not \"msg\", would help.\n\n\n[Footnote]\n\n*1* ... and with possible interleaving of output that came to the\nstandard output and the standard error, such parsing by $awkScript\nwould not be a reliable way to do this anyway.  A truly careful one\nhas to be written along the lines of:\n\n\tgit blame --porcelain \"$1\" >\"$tmp\" &&\n        awk \"$awkScript\" <\"$tmp\"\n\nanyway.\n\n> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>\n> ---\n>  builtin/blame.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 17d30d0..ca4ba6f 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2700,7 +2700,7 @@ parse_done:\n>  \t * uninteresting.\n>  \t */\n>  \tif (prepare_revision_walk(&revs))\n> -\t\tdie(\"revision walk setup failed\");\n> +\t\tdie(_(\"revision walk setup failed\"));\n>  \n>  \tif (is_null_sha1(sb.final->object.sha1)) {\n>  \t\to = sb.final->util;\n"}]}