{"thread":{"id":"25897","subject":"[PATCH] commit: Remove backward goto in read_craft_line()","startedAt":"2010-12-01T19:15:59Z","lastAt":"2010-12-01T21:40:55Z","messageCount":8,"participants":["Ralf Thielow","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"156986","messageId":"1291230959-3894-1-git-send-email-ralf.thielow@googlemail.com","threadId":"25897","inReplyTo":null,"subject":"[PATCH] commit: Remove backward goto in read_craft_line()","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-12-01T19:15:59Z","receivedAt":"2010-12-01T19:15:59Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"In case of bad graft data which is determine on many\nplaces we go back to the first place of detection,\nso move it to the end of the function.\n\nSigned-off-by: Ralf Thielow <ralf.thielow@googlemail.com>\n---\n commit.c |   13 +++++++------\n 1 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 0094ec1..d86159a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -137,12 +137,8 @@ struct commit_graft *read_graft_line(char *buf, int len)\n \t\tbuf[--len] = '\\0';\n \tif (buf[0] == '#' || buf[0] == '\\0')\n \t\treturn NULL;\n-\tif ((len + 1) % 41) {\n-\tbad_graft_data:\n-\t\terror(\"bad graft data: %s\", buf);\n-\t\tfree(graft);\n-\t\treturn NULL;\n-\t}\n+\tif ((len + 1) % 41) \n+\t\tgoto bad_graft_data;\n \ti = (len + 1) / 41 - 1;\n \tgraft = xmalloc(sizeof(*graft) + 20 * i);\n \tgraft->nr_parent = i;\n@@ -155,6 +151,11 @@ struct commit_graft *read_graft_line(char *buf, int len)\n \t\t\tgoto bad_graft_data;\n \t}\n \treturn graft;\n+\n+bad_graft_data:\n+\terror(\"bad graft data: %s\", buf);\n+\tfree(graft);\n+\treturn NULL;\n }\n \n static int read_graft_file(const char *graft_file)\n-- \n1.7.1\n"},{"id":"156995","messageId":"20101201194441.GB27347@burratino","threadId":"25897","inReplyTo":"1291230959-3894-1-git-send-email-ralf.thielow@googlemail.com","subject":"Re: [PATCH] commit: Remove backward goto in read_craft_line()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T19:44:42Z","receivedAt":"2010-12-01T19:44:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ralf Thielow wrote:\n\n> In case of bad graft data which is determine on many\n> places we go back to the first place of detection,\n> so move it to the end of the function.\n\nI like it.  Three tiny nits.\n\nPerhaps the log message could be clarified like so:\n\n bad graft data is noticed in several places in read_graft_line and\n in each case we go back to the first site of detection.  Move the\n error handling to the end of the function for better readability.\n\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -137,12 +137,8 @@ struct commit_graft *read_graft_line(char *buf, int len)\n>  \t\tbuf[--len] = '\\0';\n>  \tif (buf[0] == '#' || buf[0] == '\\0')\n>  \t\treturn NULL;\n> -\tif ((len + 1) % 41) {\n> -\tbad_graft_data:\n> -\t\terror(\"bad graft data: %s\", buf);\n> -\t\tfree(graft);\n> -\t\treturn NULL;\n> -\t}\n> +\tif ((len + 1) % 41) \n> +\t\tgoto bad_graft_data;\n\nTrailing whitespace.  (You can avoid this in the future with\n\"git diff --check\".)\n\n[...]\n> @@ -155,6 +151,11 @@ struct commit_graft *read_graft_line(char *buf, int len)\n>  \t\t\tgoto bad_graft_data;\n>  \t}\n>  \treturn graft;\n> +\n> +bad_graft_data:\n> +\terror(\"bad graft data: %s\", buf);\n\nA space before the \"bad_graft_data:\" label would improve future\ndiff --show-c-function output.\n\nExcept as noted above,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n---\ndiff --git a/commit.c b/commit.c\nindex d86159a..d5144f6 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -137,7 +137,7 @@ struct commit_graft *read_graft_line(char *buf, int len)\n \t\tbuf[--len] = '\\0';\n \tif (buf[0] == '#' || buf[0] == '\\0')\n \t\treturn NULL;\n-\tif ((len + 1) % 41) \n+\tif ((len + 1) % 41)\n \t\tgoto bad_graft_data;\n \ti = (len + 1) / 41 - 1;\n \tgraft = xmalloc(sizeof(*graft) + 20 * i);\n@@ -152,7 +152,7 @@ struct commit_graft *read_graft_line(char *buf, int len)\n \t}\n \treturn graft;\n \n-bad_graft_data:\n+ bad_graft_data:\n \terror(\"bad graft data: %s\", buf);\n \tfree(graft);\n \treturn NULL;\n"},{"id":"157011","messageId":"7vaakputxs.fsf@alter.siamese.dyndns.org","threadId":"25897","inReplyTo":"20101201194441.GB27347@burratino","subject":"Re: [PATCH] commit: Remove backward goto in read_craft_line()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-01T20:19:59Z","receivedAt":"2010-12-01T20:19:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> @@ -155,6 +151,11 @@ struct commit_graft *read_graft_line(char *buf, int len)\n>>  \t\t\tgoto bad_graft_data;\n>>  \t}\n>>  \treturn graft;\n>> +\n>> +bad_graft_data:\n>> +\terror(\"bad graft data: %s\", buf);\n>\n> A space before the \"bad_graft_data:\" label would improve future\n> diff --show-c-function output.\n\nHmm, I actually do not think we encourage that (nor we should).\n\n    $ git grep -e '^ [a-z0-9]*:' -- '*.c' | wc -l\n    23\n    $ git grep -e '^[a-z0-9]*:' -- '*.c' | wc -l\n    42\n\nIf \"--show-c-function\" output is the problem, perhaps we should know a bit\nbetter about what C function header looks like?\n"},{"id":"157015","messageId":"20101201203132.GB27845@burratino","threadId":"25897","inReplyTo":"7vaakputxs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: Remove backward goto in read_craft_line()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T20:31:32Z","receivedAt":"2010-12-01T20:31:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> A space before the \"bad_graft_data:\" label would improve future\n>> diff --show-c-function output.\n>\n> Hmm, I actually do not think we encourage that (nor we should).\n> \n>     $ git grep -e '^ [a-z0-9]*:' -- '*.c' | wc -l\n>     23\n>     $ git grep -e '^[a-z0-9]*:' -- '*.c' | wc -l\n>     42\n> \n> If \"--show-c-function\" output is the problem, perhaps we should know a bit\n> better about what C function header looks like?\n\nThanks for checking.  Yes, I think so.\n\n\t$ git grep --show-function strbuf_release -- http.c\n\thttp.c=static int http_request(const char *url, void *result, int target, int options)\n\thttp.c: strbuf_release(&buf);\n\thttp.c=cleanup:\n\thttp.c: strbuf_release(&tmpfile);\n\thttp.c=int http_fetch_ref(const char *base, struct ref *ref)\n\thttp.c: strbuf_release(&buffer);\n\nThe following gives me some joy.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\ndiff --git a/.gitattributes b/.gitattributes\nindex 5e98806..5888a53 100644\n--- a/.gitattributes\n+++ b/.gitattributes\n@@ -1,3 +1,4 @@\n * whitespace=!indent,trail,space\n *.[ch] whitespace=indent,trail,space\n+*.[ch] diff=cpp\n *.sh whitespace=indent,trail,space\n"},{"id":"157016","messageId":"AANLkTinLuF74UKaTMNX84FJt+PNoKkkOr3LaXDSCFqdz@mail.gmail.com","threadId":"25897","inReplyTo":"20101201203132.GB27845@burratino","subject":"Re: [PATCH] commit: Remove backward goto in read_craft_line()","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-12-01T20:44:42Z","receivedAt":"2010-12-01T20:44:42Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"> If \"--show-c-function\" output is the problem, perhaps we should know a bit\n> better about what C function header looks like?\n\nIn fact the \"--show-c-function\" output is the problem. But I think that\na change can't be rejected because of another issue.\nThe style of placing \"goto\"-statements, which leave a function to the\nend of that is used in many other projects. And I think\nit's very usefull.\n\n2010/12/1 Jonathan Nieder <jrnieder@gmail.com>:\n> Junio C Hamano wrote:\n>> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>>> A space before the \"bad_graft_data:\" label would improve future\n>>> diff --show-c-function output.\n>>\n>> Hmm, I actually do not think we encourage that (nor we should).\n>>\n>>     $ git grep -e '^ [a-z0-9]*:' -- '*.c' | wc -l\n>>     23\n>>     $ git grep -e '^[a-z0-9]*:' -- '*.c' | wc -l\n>>     42\n>>\n>> If \"--show-c-function\" output is the problem, perhaps we should know a bit\n>> better about what C function header looks like?\n>\n> Thanks for checking.  Yes, I think so.\n>\n>        $ git grep --show-function strbuf_release -- http.c\n>        http.c=static int http_request(const char *url, void *result, int target, int options)\n>        http.c: strbuf_release(&buf);\n>        http.c=cleanup:\n>        http.c: strbuf_release(&tmpfile);\n>        http.c=int http_fetch_ref(const char *base, struct ref *ref)\n>        http.c: strbuf_release(&buffer);\n>\n> The following gives me some joy.\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> diff --git a/.gitattributes b/.gitattributes\n> index 5e98806..5888a53 100644\n> --- a/.gitattributes\n> +++ b/.gitattributes\n> @@ -1,3 +1,4 @@\n>  * whitespace=!indent,trail,space\n>  *.[ch] whitespace=indent,trail,space\n> +*.[ch] diff=cpp\n>  *.sh whitespace=indent,trail,space\n>\n"},{"id":"157023","messageId":"20101201210700.GC27845@burratino","threadId":"25897","inReplyTo":"AANLkTinLuF74UKaTMNX84FJt+PNoKkkOr3LaXDSCFqdz@mail.gmail.com","subject":"Trivial patches (Re: [PATCH] commit: Remove backward goto in read_craft_line())","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-01T21:07:00Z","receivedAt":"2010-12-01T21:07:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Ralf,\n\nRalf Thielow wrote:\n\n> In fact the \"--show-c-function\" output is the problem. But I think that\n> a change can't be rejected because of another issue.\n> The style of placing \"goto\"-statements, which leave a function to the\n> end of that is used in many other projects. And I think\n> it's very usefull.\n\nThinking more about your patch reminds me of something that can be\nconfusing to new contributors.  Sometimes it is hard for a trivial\npatch to be accepted than one which makes a larger change, requires\nmore time reviewing it, and has more potential for catastrophic\nbreakage.\n\nWhy is that?  Resistance to trivial changes is in my opinion a good\nthing and I'd like to emphasize that now, since my feeling about\nthis patch is borderline.[1]\n\nIn this case, the patch is changing code like this:\n\n\tdo something\n\tif (error) {\n\terror_case:\n\t\treport error;\n\t\tfree resources;\n\t\treturn -1;\n\t}\n\tdo something else\n\tif (error)\n\t\tgoto error_case;\n\tif (other error)\n\t\tgoto error_case;\n\treturn result;\n\ninto the more usual simulated exception handling:\n\n\tdo something\n\tif (error)\n\t\tgoto error_case;\n\tdo something else\n\tif (error)\n\t\tgoto error_case;\n\t...\n\treturn result;\nerror_case:\n\treport error;\n\tfree resources;\n\treturn -1;\n\nThe latter is not really much clearer than the former, just less\nunusual.  Changing the code means patches are less likely to apply\nto both the before and after.  Changing the code requires time to\nreview it and to explain why this kind of change is okay in this\ncase but in other cases it wouldn't be.\n\nSo it would be much easier to like this if the change fixed a\nnoticeable problem (in user-visible behavior, maintainability,\nor clarity).\n\nSorry for the mixed message.  Partly I really _wanted_ to like\nthis because it is in better taste than some of the trivial patches\nthe git list has received before.  As far as applying this one, I\nsuppose I would not mind either way.\n\nAnyway, I hope that makes the underlying principles a bit clearer.\n\nThanks for a useful example.\nJonathan\n\n[1] Linux has a \"maintainer of trivial\" that takes care of such\npatches while minimizing the damage to bystanders.  Traditionally\nthere were some pretty rigid guidelines for what patches in this\narea were accepted, which was probably a good idea.\nftp://ftp.kernel.org/pub/linux/kernel/people/rusty/trivial/template-index.html\n"},{"id":"157026","messageId":"7vwrnttcm3.fsf@alter.siamese.dyndns.org","threadId":"25897","inReplyTo":"AANLkTinLuF74UKaTMNX84FJt+PNoKkkOr3LaXDSCFqdz@mail.gmail.com","subject":"Re: [PATCH] commit: Remove backward goto in read_craft_line()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-01T21:19:32Z","receivedAt":"2010-12-01T21:19:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralf Thielow <ralf.thielow@googlemail.com> writes:\n\n>> If \"--show-c-function\" output is the problem, perhaps we should know a bit\n>> better about what C function header looks like?\n>\n> In fact the \"--show-c-function\" output is the problem. But I think that\n> a change can't be rejected because of another issue.\n\nWell, I never said anything about rejection nor acceptance.\n\n> The style of placing \"goto\"-statements, which leave a function to the\n> end of that is used in many other projects. And I think\n> it's very usefull.\n\nIt is a good discipline to follow in general to place an exceptional case\nat the end if you jump out of the general flow.  But because the affected\nfunction was so small, its readability doesn't benefit very much from the\ndiscipline (in other words, you knew about a good discipline, but applied\nit to a wrong function).  The patch was small and obviously correct, so it\nwill not hurt, but it will not make the world greatly a better place,\neither.\n\nIOW, it was a \"Meh\" topic for me.\n\nIt was more important to discuss Jonathan's \"leave SP at the beginning of\na goto label to please --show-c-function\" from the maintainer's point of\nview, as people may remember it as a rule and start sending patches that\nfollow it, which I will need to deal with in the future.  I do not think\nthat one is a good rule.\n\nNow that we have dealt with that more important business of letting people\nknow that protecting goto labels with a leading SP is _not_ the rule ;-),\nI am happy with this discussion.\n\nAnd often I forget about the original issue when a discussion reaches this\nstage of happiness.  So thanks for reminding me.\n\nAs I said, even though I do not think the particular function you touched\nbadly needs the discipline applied, it would not hurt, and it obviously is\nthe right thing to do (iow, if the function were written from day one in a\nway your patch reorganized it, we would never accept a patch to change it\ninto today's shape of jumping backwards).  For one thing, it would remove\nan example from the codebase people can point at to make excuses when\nresponding to a review of their patch that adds a backward goto to a much\nlarger function.\n\nWill queue after massaging the log message somewhat.  Thanks.\n"},{"id":"157027","messageId":"AANLkTikoh01_gQAZ3Js4cTfdRioKrR6GHAPDBvUOZm8P@mail.gmail.com","threadId":"25897","inReplyTo":"7vwrnttcm3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: Remove backward goto in read_craft_line()","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-12-01T21:40:55Z","receivedAt":"2010-12-01T21:40:55Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"I remember about a peoble (not on this project) who sad that\nalso a little change like this should includes in the sources because\nit's make the code better without fixing a bug or add a new feature.\nBut in my experience these patches are not very popular.\nBut sure, I understand your reasons, Junio.\nThe statement from above is not even true.\n\n2010/12/1 Junio C Hamano <gitster@pobox.com>:\n> Ralf Thielow <ralf.thielow@googlemail.com> writes:\n>\n>>> If \"--show-c-function\" output is the problem, perhaps we should know a bit\n>>> better about what C function header looks like?\n>>\n>> In fact the \"--show-c-function\" output is the problem. But I think that\n>> a change can't be rejected because of another issue.\n>\n> Well, I never said anything about rejection nor acceptance.\n>\n>> The style of placing \"goto\"-statements, which leave a function to the\n>> end of that is used in many other projects. And I think\n>> it's very usefull.\n>\n> It is a good discipline to follow in general to place an exceptional case\n> at the end if you jump out of the general flow.  But because the affected\n> function was so small, its readability doesn't benefit very much from the\n> discipline (in other words, you knew about a good discipline, but applied\n> it to a wrong function).  The patch was small and obviously correct, so it\n> will not hurt, but it will not make the world greatly a better place,\n> either.\n>\n> IOW, it was a \"Meh\" topic for me.\n>\n> It was more important to discuss Jonathan's \"leave SP at the beginning of\n> a goto label to please --show-c-function\" from the maintainer's point of\n> view, as people may remember it as a rule and start sending patches that\n> follow it, which I will need to deal with in the future.  I do not think\n> that one is a good rule.\n>\n> Now that we have dealt with that more important business of letting people\n> know that protecting goto labels with a leading SP is _not_ the rule ;-),\n> I am happy with this discussion.\n>\n> And often I forget about the original issue when a discussion reaches this\n> stage of happiness.  So thanks for reminding me.\n>\n> As I said, even though I do not think the particular function you touched\n> badly needs the discipline applied, it would not hurt, and it obviously is\n> the right thing to do (iow, if the function were written from day one in a\n> way your patch reorganized it, we would never accept a patch to change it\n> into today's shape of jumping backwards).  For one thing, it would remove\n> an example from the codebase people can point at to make excuses when\n> responding to a review of their patch that adds a backward goto to a much\n> larger function.\n>\n> Will queue after massaging the log message somewhat.  Thanks.\n>\n"}]}