{"thread":{"id":"33951","subject":"[PATCH 0/2] cherry-pick: a fix and a new option","startedAt":"2013-05-27T16:52:17Z","lastAt":"2013-05-29T14:10:44Z","messageCount":21,"participants":["Felipe Contreras","Joachim Schmitz","Neil Horman","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"218622","messageId":"1369673539-28692-1-git-send-email-felipe.contreras@gmail.com","threadId":"33951","inReplyTo":null,"subject":"[PATCH 0/2] cherry-pick: a fix and a new option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-27T16:52:17Z","receivedAt":"2013-05-27T16:52:17Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nIt doesn't make sense for the user to be interrupted constantly.\n\nFelipe Contreras (2):\n sequencer: trivial fix\n cherry-pick: add --skip-commits option\n\n Documentation/git-cherry-pick.txt | 3 +++\n builtin/revert.c | 2 ++\n sequencer.c | 12 +++++++++---\n sequencer.h | 1 +\n t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n 5 files changed, 28 insertions(+), 3 deletions(-)\n\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218623","messageId":"1369673539-28692-2-git-send-email-felipe.contreras@gmail.com","threadId":"33951","inReplyTo":"1369673539-28692-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/2] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-27T16:52:18Z","receivedAt":"2013-05-27T16:52:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We should free objects before leaving.\n\nSigned-off-by: Felipe Contreras \n---\n sequencer.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ab6f8a7..7eeae2f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\trerere(opts->allow_rerere_auto);\n \t} else {\n \t\tint allow = allow_empty(opts, commit);\n-\t\tif (allow < 0)\n-\t\t\treturn allow;\n+\t\tif (allow < 0) {\n+\t\t\tres = allow;\n+\t\t\tgoto leave;\n+\t\t}\n \t\tif (!opts->no_commit)\n \t\t\tres = run_git_commit(defmsg, opts, allow);\n \t}\n \n+leave:\n \tfree_message(&msg);\n \tfree(defmsg);\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218624","messageId":"1369673539-28692-3-git-send-email-felipe.contreras@gmail.com","threadId":"33951","inReplyTo":"1369673539-28692-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/2] cherry-pick: add --skip-commits option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-27T16:52:19Z","receivedAt":"2013-05-27T16:52:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Pretty much what it says on the tin.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/git-cherry-pick.txt | 3 +++\n builtin/revert.c | 2 ++\n sequencer.c | 5 ++++-\n sequencer.h | 1 +\n t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n 5 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex c205d23..fccd936 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -129,6 +129,9 @@ effect to your index in a row.\n \tredundant commits are ignored. This option overrides that behavior and\n \tcreates an empty commit object. Implies `--allow-empty`.\n \n+--skip-empty::\n+\tInstead of failing, skip commits that are or become empty.\n+\n --strategy=::\n \tUse the given merge strategy. Should only be used once.\n \tSee the MERGE STRATEGIES section in linkgit:git-merge[1]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0401fdb..0e5ce71 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -118,6 +118,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n+\t\tOPT_END(),\n \t};\n \n \tif (opts->action == REPLAY_PICK) {\n@@ -127,6 +128,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOLEAN(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n \t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n+\t\t\tOPT_BOOLEAN(0, \"skip-empty\", &opts->skip_empty, N_(\"skip empty commits\")),\n \t\t\tOPT_END(),\n \t\t};\n \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\ndiff --git a/sequencer.c b/sequencer.c\nindex 7eeae2f..86e8e78 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -625,7 +625,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tprint_advice(res == 1, opts);\n \t\trerere(opts->allow_rerere_auto);\n \t} else {\n-\t\tint allow = allow_empty(opts, commit);\n+\t\tint allow;\n+\t\tif (opts->skip_empty && is_index_unchanged() == 1)\n+\t\t\tgoto leave;\n+\t\tallow = allow_empty(opts, commit);\n \t\tif (allow < 0) {\n \t\t\tres = allow;\n \t\t\tgoto leave;\ndiff --git a/sequencer.h b/sequencer.h\nindex 1fc22dc..3b04844 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -34,6 +34,7 @@ struct replay_opts {\n \tint allow_empty;\n \tint allow_empty_message;\n \tint keep_redundant_commits;\n+\tint skip_empty;\n \n \tint mainline;\n \ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex 19c99d7..3dc19c6 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -187,4 +187,17 @@ test_expect_success 'cherry-pick --stdin works' '\n \tcheck_head_differs_from fourth\n '\n \n+test_expect_success 'cherry-pick skip empty' '\n+\tgit clean -fxd &&\n+\tgit checkout -b empty fourth &&\n+\tgit commit --allow-empty -m empty &&\n+\ttest_commit ontop &&\n+\tgit checkout -f master &&\n+\tgit reset --hard fourth &&\n+\tgit cherry-pick --skip-empty fourth..empty &&\n+\techo ontop > expected &&\n+\tgit log --format=%s fourth..HEAD > actual\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218639","messageId":"ko20u9$nbv$1@ger.gmane.org","threadId":"33951","inReplyTo":"1369673539-28692-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/2] cherry-pick: add --skip-commits option","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-05-28T10:29:25Z","receivedAt":"2013-05-28T10:29:25Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Felipe Contreras wrote:\n> Pretty much what it says on the tin.\n\nOnly that it add --skip-empty and not --skip-commit ?!?\n \nBye, Jojo\n"},{"id":"218641","messageId":"20130528110014.GA1264@hmsreliant.think-freely.org","threadId":"33951","inReplyTo":"1369673539-28692-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2013-05-28T11:00:14Z","receivedAt":"2013-05-28T11:00:14Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:\n> We should free objects before leaving.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> sequencer.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index ab6f8a7..7eeae2f 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n> \t\trerere(opts->allow_rerere_auto);\n> \t} else {\n> \t\tint allow = allow_empty(opts, commit);\n> -\t\tif (allow < 0)\n> -\t\t\treturn allow;\n> +\t\tif (allow < 0) {\n> +\t\t\tres = allow;\n> +\t\t\tgoto leave;\n> +\t\t}\n> \t\tif (!opts->no_commit)\n> \t\t\tres = run_git_commit(defmsg, opts, allow);\n> \t}\n> \n> +leave:\n> \tfree_message(&msg);\n> \tfree(defmsg);\n> \n> -- \n> 1.8.3.rc3.312.g47657de\n> \n> \nAcked-by: Neil Horman \n"},{"id":"218642","messageId":"20130528110626.GB1264@hmsreliant.think-freely.org","threadId":"33951","inReplyTo":"1369673539-28692-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/2] cherry-pick: add --skip-commits option","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2013-05-28T11:06:26Z","receivedAt":"2013-05-28T11:06:26Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Mon, May 27, 2013 at 11:52:19AM -0500, Felipe Contreras wrote:\n> Pretty much what it says on the tin.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> Documentation/git-cherry-pick.txt | 3 +++\n> builtin/revert.c | 2 ++\n> sequencer.c | 5 ++++-\n> sequencer.h | 1 +\n> t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++\n> 5 files changed, 23 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n> index c205d23..fccd936 100644\n> --- a/Documentation/git-cherry-pick.txt\n> +++ b/Documentation/git-cherry-pick.txt\n> @@ -129,6 +129,9 @@ effect to your index in a row.\n> \tredundant commits are ignored. This option overrides that behavior and\n> \tcreates an empty commit object. Implies `--allow-empty`.\n> \n> +--skip-empty::\n> +\tInstead of failing, skip commits that are or become empty.\n> +\n> --strategy=::\n> \tUse the given merge strategy. Should only be used once.\n> \tSee the MERGE STRATEGIES section in linkgit:git-merge[1]\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 0401fdb..0e5ce71 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -118,6 +118,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> \t\tOPT_END(),\n> \t\tOPT_END(),\n> \t\tOPT_END(),\n> +\t\tOPT_END(),\n> \t};\n> \n> \tif (opts->action == REPLAY_PICK) {\n> @@ -127,6 +128,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n> \t\t\tOPT_BOOLEAN(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n> \t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n> +\t\t\tOPT_BOOLEAN(0, \"skip-empty\", &opts->skip_empty, N_(\"skip empty commits\")),\n> \t\t\tOPT_END(),\n> \t\t};\nI like the idea, but this option seems a bit awkward to me. At the very least\nhere, don't you now need to check for conflicts if --keep-redundant-commits and\nskip-empty are both specified (as iirc git doens't see the difference between\nempty commits and commits made empty by prior commits in the current history).\nwhat if we merged the two options to an OPT_STRING, something like\n--empty-commits=[keep|skip|ask]. The default currently is an impiled, since the\nsequencer stops on an empty commit.\n\nNeil\n"},{"id":"218676","messageId":"7vobbv2fze.fsf@alter.siamese.dyndns.org","threadId":"33951","inReplyTo":"20130528110014.GA1264@hmsreliant.think-freely.org","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-28T17:04:21Z","receivedAt":"2013-05-28T17:04:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neil Horman writes:\n\n> On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:\n>> We should free objects before leaving.\n>> \n>> Signed-off-by: Felipe Contreras \n>> ---\n>> sequencer.c | 7 +++++--\n>> 1 file changed, 5 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/sequencer.c b/sequencer.c\n>> index ab6f8a7..7eeae2f 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n>> \t\trerere(opts->allow_rerere_auto);\n>> \t} else {\n>> \t\tint allow = allow_empty(opts, commit);\n>> -\t\tif (allow < 0)\n>> -\t\t\treturn allow;\n>> +\t\tif (allow < 0) {\n>> +\t\t\tres = allow;\n>> +\t\t\tgoto leave;\n>> +\t\t}\n>> \t\tif (!opts->no_commit)\n>> \t\t\tres = run_git_commit(defmsg, opts, allow);\n>> \t}\n>> \n>> +leave:\n>> \tfree_message(&msg);\n>> \tfree(defmsg);\n>> \n>> -- \n>> 1.8.3.rc3.312.g47657de\n>> \n>> \n> Acked-by: Neil Horman \n\nThis is better done without \"goto\" in general.\n\nThe other patch 2/2/ adds one more \"we need to exit from the middle\nof the flow\" and makes it look handier to add an exit label here,\nbut it would be even better to express the logic of that patch as a\nnormal cascade of if/else if/..., which is small enough and we do\nnot need the \"leave:\" label.\n\nIt probably is better to fold this patch into the other one when it\nis rerolled to correct the option name gotcha \"on the tin\".\n\nThanks.\n"},{"id":"218712","messageId":"51a568db9c9b8_807b33e18996fa@nysa.mail","threadId":"33951","inReplyTo":"7vobbv2fze.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T02:32:59Z","receivedAt":"2013-05-29T02:32:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Neil Horman writes:\n> \n> > On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:\n> >> We should free objects before leaving.\n> >> \n> >> Signed-off-by: Felipe Contreras \n> >> ---\n> >> sequencer.c | 7 +++++--\n> >> 1 file changed, 5 insertions(+), 2 deletions(-)\n> >> \n> >> diff --git a/sequencer.c b/sequencer.c\n> >> index ab6f8a7..7eeae2f 100644\n> >> --- a/sequencer.c\n> >> +++ b/sequencer.c\n> >> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n> >> \t\trerere(opts->allow_rerere_auto);\n> >> \t} else {\n> >> \t\tint allow = allow_empty(opts, commit);\n> >> -\t\tif (allow < 0)\n> >> -\t\t\treturn allow;\n> >> +\t\tif (allow < 0) {\n> >> +\t\t\tres = allow;\n> >> +\t\t\tgoto leave;\n> >> +\t\t}\n> >> \t\tif (!opts->no_commit)\n> >> \t\t\tres = run_git_commit(defmsg, opts, allow);\n> >> \t}\n> >> \n> >> +leave:\n> >> \tfree_message(&msg);\n> >> \tfree(defmsg);\n> >> \n> >> -- \n> >> 1.8.3.rc3.312.g47657de\n> >> \n> >> \n> > Acked-by: Neil Horman \n> \n> This is better done without \"goto\" in general.\n> \n> The other patch 2/2/ adds one more \"we need to exit from the middle\n> of the flow\" and makes it look handier to add an exit label here,\n> but it would be even better to express the logic of that patch as a\n> normal cascade of if/else if/..., which is small enough and we do\n> not need the \"leave:\" label.\n\nLinux kernel developers would disagree. In C 'goto' is quite of then the only\nsane option, and you can see 'goto' used in the Linux kernel all over the place\nfor that reason.\n\nIn this particular case it also makes perfect sense.\n\n> It probably is better to fold this patch into the other one when it\n> is rerolled to correct the option name gotcha \"on the tin\".\n\nWhy? This patch is standalone and fixes an issue that is independent of the\nother patch. Why squash two patches that do *two* different things?\n\nAnyway, I'll happily drop this patch if you want this memory leak to remain.\nBut then I'll do the same in the other patch.\n\nThis mantra of avodiing 'goto' is not helping anybody.\n\n-- \nFelipe Contreras\n"},{"id":"218793","messageId":"ko4jf7$e4d$1@ger.gmane.org","threadId":"33951","inReplyTo":"51a568db9c9b8_807b33e18996fa@nysa.mail","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-05-29T09:58:03Z","receivedAt":"2013-05-29T09:58:03Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Felipe Contreras wrote:\n> Junio C Hamano wrote:\n>> Neil Horman writes:\n>>\n>>> On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:\n>>>> We should free objects before leaving.\n>>>>\n>>>> Signed-off-by: Felipe Contreras \n>>>> ---\n>>>> sequencer.c | 7 +++++--\n>>>> 1 file changed, 5 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/sequencer.c b/sequencer.c\n>>>> index ab6f8a7..7eeae2f 100644\n>>>> --- a/sequencer.c\n>>>> +++ b/sequencer.c\n>>>> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit\n>>>> *commit, struct replay_opts *opts)\n>>>> rerere(opts->allow_rerere_auto); } else {\n>>>> int allow = allow_empty(opts, commit);\n>>>> - if (allow < 0)\n>>>> - return allow;\n>>>> + if (allow < 0) {\n>>>> + res = allow;\n>>>> + goto leave;\n>>>> + }\n>>>> if (!opts->no_commit)\n>>>> res = run_git_commit(defmsg, opts, allow);\n>>>> }\n>>>>\n>>>> +leave:\n>>>> free_message(&msg);\n>>>> free(defmsg);\n>>>>\n>>>> --\n>>>> 1.8.3.rc3.312.g47657de\n>>>>\n>>>>\n>>> Acked-by: Neil Horman \n>>\n>> This is better done without \"goto\" in general.\n>>\n>> The other patch 2/2/ adds one more \"we need to exit from the middle\n>> of the flow\" and makes it look handier to add an exit label here,\n>> but it would be even better to express the logic of that patch as a\n>> normal cascade of if/else if/..., which is small enough and we do\n>> not need the \"leave:\" label.\n>\n> Linux kernel developers would disagree. In C 'goto' is quite of then\n> the only sane option, and you can see 'goto' used in the Linux kernel\n> all over the place for that reason.\n>\n> In this particular case it also makes perfect sense.\n>\n>> It probably is better to fold this patch into the other one when it\n>> is rerolled to correct the option name gotcha \"on the tin\".\n>\n> Why? This patch is standalone and fixes an issue that is independent\n> of the other patch. Why squash two patches that do *two* different\n> things?\n>\n> Anyway, I'll happily drop this patch if you want this memory leak to\n> remain. But then I'll do the same in the other patch.\n>\n> This mantra of avodiing 'goto' is not helping anybody.\n\nadding 5 letters (to change the next \"if\" into an \"else if\") versus your \naddition of several lines and some 15 additional letters (ignoring the \nwhitsspace) is IMHO enough to see what is better?\n\nbye, Jojo \n"},{"id":"218795","messageId":"CAMP44s0vARKGsn2noBEAxSVHD1bkU9pR7nPCvFJwp5epwidkQw@mail.gmail.com","threadId":"33951","inReplyTo":"ko4jf7$e4d$1@ger.gmane.org","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T10:51:53Z","receivedAt":"2013-05-29T10:51:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz\n wrote:\n> Felipe Contreras wrote:\n>>\n>> Junio C Hamano wrote:\n\n>>> It probably is better to fold this patch into the other one when it\n>>> is rerolled to correct the option name gotcha \"on the tin\".\n>>\n>>\n>> Why? This patch is standalone and fixes an issue that is independent\n>> of the other patch. Why squash two patches that do *two* different\n>> things?\n>>\n>> Anyway, I'll happily drop this patch if you want this memory leak to\n>> remain. But then I'll do the same in the other patch.\n>>\n>> This mantra of avodiing 'goto' is not helping anybody.\n>\n>\n> adding 5 letters (to change the next \"if\" into an \"else if\") versus your\n> addition of several lines and some 15 additional letters (ignoring the\n> whitsspace) is IMHO enough to see what is better?\n\nThis has nothing to do with what Junio said. Junio said it is better\nto squash the two changes into one, which is not clearly better.\n\nAs for your suggestion, what happens the next time somebody needs to\nadd something else to this chunk of code? Another if, and then\nanother, and soon enough you end up with five levels of indentation,\nand in some of those patches you have to change the indentation of\nexisting code.\n\nIf only there was much bigger and successful software project that had\nhashed all these questions and came up with a code-style to last the\nages. Oh, but there is, it's called Linux, and the answer is to use\ngoto's.\n\nIf the code used a goto in the first place.. BAM:\n\n--- a/sequencer.c\n+++ b/sequencer.c\n -628,8 +628,10 static int\ndo_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t}\n\n \tallow = allow_empty(opts, commit);\n-\tif (allow < 0)\n-\t\treturn allow;\n+\tif (allow < 0) {\n+\t\tres = allow;\n+\t\tgoto leave;\n+\t}\n \tif (!opts->no_commit)\n \t\tres = run_git_commit(defmsg, opts, allow);\n\nAnd every time you need to add more code you just do it, and stop\nworrying about increasing indentation, or re-indenting.\n\nProblem solved.\n\n-- \nFelipe Contreras\n"},{"id":"218797","messageId":"001601ce5c5d$89974830$9cc5d890$@schmitz-digital.de","threadId":"33951","inReplyTo":"CAMP44s0vARKGsn2noBEAxSVHD1bkU9pR7nPCvFJwp5epwidkQw@mail.gmail.com","subject":"RE: [PATCH 1/2] sequencer: trivial fix","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-05-29T11:13:24Z","receivedAt":"2013-05-29T11:13:24Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]\n> Sent: Wednesday, May 29, 2013 12:52 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH 1/2] sequencer: trivial fix\n> \n> On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz\n> wrote:\n> > Felipe Contreras wrote:\n> >>\n> >> Junio C Hamano wrote:\n> \n> >>> It probably is better to fold this patch into the other one when it\n> >>> is rerolled to correct the option name gotcha \"on the tin\".\n> >>\n> >>\n> >> Why? This patch is standalone and fixes an issue that is independent\n> >> of the other patch. Why squash two patches that do *two* different\n> >> things?\n> >>\n> >> Anyway, I'll happily drop this patch if you want this memory leak to\n> >> remain. But then I'll do the same in the other patch.\n> >>\n> >> This mantra of avodiing 'goto' is not helping anybody.\n> >\n> >\n> > adding 5 letters (to change the next \"if\" into an \"else if\") versus your\n> > addition of several lines and some 15 additional letters (ignoring the\n> > whitsspace) is IMHO enough to see what is better?\n> \n> This has nothing to do with what Junio said. \n\nWell, it has, but you had snipped it. But replied to the goto issue regardless\n\n> This is better done without \"goto\" in general.\n\nBye, Jojo\n"},{"id":"218800","messageId":"CAMP44s0U65oxCVy3EwQxF+4ZgRc31z29mwwdO=4x--oFVTFW+g@mail.gmail.com","threadId":"33951","inReplyTo":"001601ce5c5d$89974830$9cc5d890$@schmitz-digital.de","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T11:23:48Z","receivedAt":"2013-05-29T11:23:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 6:13 AM, Joachim Schmitz\n wrote:\n>> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]\n>> Sent: Wednesday, May 29, 2013 12:52 PM\n>> To: Joachim Schmitz\n>> Cc: git@vger.kernel.org\n>> Subject: Re: [PATCH 1/2] sequencer: trivial fix\n>>\n>> On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz\n>> wrote:\n>> > Felipe Contreras wrote:\n>> >>\n>> >> Junio C Hamano wrote:\n>>\n>> >>> It probably is better to fold this patch into the other one when it\n>> >>> is rerolled to correct the option name gotcha \"on the tin\".\n>> >>\n>> >>\n>> >> Why? This patch is standalone and fixes an issue that is independent\n>> >> of the other patch. Why squash two patches that do *two* different\n>> >> things?\n>> >>\n>> >> Anyway, I'll happily drop this patch if you want this memory leak to\n>> >> remain. But then I'll do the same in the other patch.\n>> >>\n>> >> This mantra of avodiing 'goto' is not helping anybody.\n>> >\n>> >\n>> > adding 5 letters (to change the next \"if\" into an \"else if\") versus your\n>> > addition of several lines and some 15 additional letters (ignoring the\n>> > whitsspace) is IMHO enough to see what is better?\n>>\n>> This has nothing to do with what Junio said.\n>\n> Well, it has, but you had snipped it. But replied to the goto issue regardless\n\nI didn't snip anything, this is a different context.\n\n>> This is better done without \"goto\" in general.\n\nHe din't say:\n\n__\nIt probably is better to fold this patch into the other one when it\nis rerolled to correct the option name gotcha \"on the tin\", AND you\nfix the goto issue.\n__\n\nYou added that last part in your mind. Moreover, he didn't say goto\nwas an issue, he simply stated an opinion about some generality.\n\n-- \nFelipe Contreras\n"},{"id":"218802","messageId":"001c01ce5c5f$d581b8a0$808529e0$@schmitz-digital.de","threadId":"33951","inReplyTo":"CAMP44s0U65oxCVy3EwQxF+4ZgRc31z29mwwdO=4x--oFVTFW+g@mail.gmail.com","subject":"RE: [PATCH 1/2] sequencer: trivial fix","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-05-29T11:29:50Z","receivedAt":"2013-05-29T11:29:50Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]\n> Sent: Wednesday, May 29, 2013 1:24 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH 1/2] sequencer: trivial fix\n> \n> On Wed, May 29, 2013 at 6:13 AM, Joachim Schmitz\n> wrote:\n> >> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]\n> >> Sent: Wednesday, May 29, 2013 12:52 PM\n> >> To: Joachim Schmitz\n> >> Cc: git@vger.kernel.org\n> >> Subject: Re: [PATCH 1/2] sequencer: trivial fix\n> >>\n> >> On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz\n> >> wrote:\n> >> > Felipe Contreras wrote:\n> >> >>\n> >> >> Junio C Hamano wrote:\n> >>\n> >> >>> It probably is better to fold this patch into the other one when it\n> >> >>> is rerolled to correct the option name gotcha \"on the tin\".\n> >> >>\n> >> >>\n> >> >> Why? This patch is standalone and fixes an issue that is independent\n> >> >> of the other patch. Why squash two patches that do *two* different\n> >> >> things?\n> >> >>\n> >> >> Anyway, I'll happily drop this patch if you want this memory leak to\n> >> >> remain. But then I'll do the same in the other patch.\n> >> >>\n> >> >> This mantra of avodiing 'goto' is not helping anybody.\n> >> >\n> >> >\n> >> > adding 5 letters (to change the next \"if\" into an \"else if\") versus your\n> >> > addition of several lines and some 15 additional letters (ignoring the\n> >> > whitsspace) is IMHO enough to see what is better?\n> >>\n> >> This has nothing to do with what Junio said.\n> >\n> > Well, it has, but you had snipped it. But replied to the goto issue regardless\n> \n> I didn't snip anything, this is a different context.\n\nYou did in your reply to me\n\n> >> This is better done without \"goto\" in general.\n> \n> He din't say:\n> __\n> It probably is better to fold this patch into the other one when it\n> is rerolled to correct the option name gotcha \"on the tin\", AND you\n> fix the goto issue.\n> __\n> \n> You added that last part in your mind. Moreover, he didn't say goto\n> was an issue, he simply stated an opinion about some generality.\n\nI added nothing in my mind, I just copy/paste that statement and was commenting on that and only that.\nAt least intended to.\n\nWhenever anybody added more else branches, that's the time to possible switch to the goto style.\n\nAnd for the record: I agree with you that these 2 things should rather not be in a single patch as they are completely unrelated.\n\nBye, Jojo\n"},{"id":"218804","messageId":"001d01ce5c60$f20b5d90$d62218b0$@schmitz-digital.de","threadId":"33951","inReplyTo":"CAMP44s0U65oxCVy3EwQxF+4ZgRc31z29mwwdO=4x--oFVTFW+g@mail.gmail.com","subject":"RE: [PATCH 1/2] sequencer: trivial fix","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-05-29T11:37:47Z","receivedAt":"2013-05-29T11:37:47Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]\n> Sent: Wednesday, May 29, 2013 1:30 PM\n> To: 'Felipe Contreras'\n> Cc: 'git@vger.kernel.org'\n> Subject: RE: [PATCH 1/2] sequencer: trivial fix\n\n> \n> And for the record: I agree with you that these 2 things should rather not be in a single patch as they are completely unrelated.\n\nI take that back: your patches 'overlap' so the 2nd won't apply without the 1st\n\n Bye, Jojo\n"},{"id":"218820","messageId":"20130529131346.GA28198@hmsreliant.think-freely.org","threadId":"33951","inReplyTo":"51a568db9c9b8_807b33e18996fa@nysa.mail","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2013-05-29T13:13:46Z","receivedAt":"2013-05-29T13:13:46Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Tue, May 28, 2013 at 09:32:59PM -0500, Felipe Contreras wrote:\n> Junio C Hamano wrote:\n> > Neil Horman writes:\n> > \n> > > On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:\n> > >> We should free objects before leaving.\n> > >> \n> > >> Signed-off-by: Felipe Contreras \n> > >> ---\n> > >> sequencer.c | 7 +++++--\n> > >> 1 file changed, 5 insertions(+), 2 deletions(-)\n> > >> \n> > >> diff --git a/sequencer.c b/sequencer.c\n> > >> index ab6f8a7..7eeae2f 100644\n> > >> --- a/sequencer.c\n> > >> +++ b/sequencer.c\n> > >> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n> > >> \t\trerere(opts->allow_rerere_auto);\n> > >> \t} else {\n> > >> \t\tint allow = allow_empty(opts, commit);\n> > >> -\t\tif (allow < 0)\n> > >> -\t\t\treturn allow;\n> > >> +\t\tif (allow < 0) {\n> > >> +\t\t\tres = allow;\n> > >> +\t\t\tgoto leave;\n> > >> +\t\t}\n> > >> \t\tif (!opts->no_commit)\n> > >> \t\t\tres = run_git_commit(defmsg, opts, allow);\n> > >> \t}\n> > >> \n> > >> +leave:\n> > >> \tfree_message(&msg);\n> > >> \tfree(defmsg);\n> > >> \n> > >> -- \n> > >> 1.8.3.rc3.312.g47657de\n> > >> \n> > >> \n> > > Acked-by: Neil Horman \n> > \n> > This is better done without \"goto\" in general.\n> > \n> > The other patch 2/2/ adds one more \"we need to exit from the middle\n> > of the flow\" and makes it look handier to add an exit label here,\n> > but it would be even better to express the logic of that patch as a\n> > normal cascade of if/else if/..., which is small enough and we do\n> > not need the \"leave:\" label.\n> \n> Linux kernel developers would disagree. In C 'goto' is quite of then the only\n> sane option, and you can see 'goto' used in the Linux kernel all over the place\n> for that reason.\n> \n> In this particular case it also makes perfect sense.\n> \nI agree with Felipe here. Setting asside coding practice in other projects,\nwhile its nice to follow coding convention in a project, a jump label just makes\nmore sense here. To not use it either requires you to duplicate the free\nstatements (undesireable), or to change the sense of theif clause here and nest\nyour if statements (makes for ugly reading).\n\n> > It probably is better to fold this patch into the other one when it\n> > is rerolled to correct the option name gotcha \"on the tin\".\n> \n> Why? This patch is standalone and fixes an issue that is independent of the\n> other patch. Why squash two patches that do *two* different things?\n> \nI agree here as well. This fixes a bug that has nothing to do with the other\npatch, save for it being in the same C file. Fix them separately.\n\n> Anyway, I'll happily drop this patch if you want this memory leak to remain.\n> But then I'll do the same in the other patch.\n> \n> This mantra of avodiing 'goto' is not helping anybody.\n> \n> -- \n> Felipe Contreras\n> \n"},{"id":"218825","messageId":"CACsJy8DTKZgVM7TSBUKJq2pspkR1jH-fyG6BHr1YYz3N+Ov3XA@mail.gmail.com","threadId":"33951","inReplyTo":"1369673539-28692-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-29T13:25:42Z","receivedAt":"2013-05-29T13:25:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras\n wrote:\n> We should free objects before leaving.\n>\n> Signed-off-by: Felipe Contreras \n\nMicronit: perhaps you should move the \"free obejcts before leaving\"\n(in do_pick_commit) to the subject instead of \"trivial fix\", which\nadds no value to the patch.\n--\nDuy\n"},{"id":"218830","messageId":"CAMP44s0cpAkAUzo0nS55yv+6=cCBsBhgNiYxpEd8Hzk=3mfNhw@mail.gmail.com","threadId":"33951","inReplyTo":"CACsJy8DTKZgVM7TSBUKJq2pspkR1jH-fyG6BHr1YYz3N+Ov3XA@mail.gmail.com","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T13:34:37Z","receivedAt":"2013-05-29T13:34:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen wrote:\n> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras\n> wrote:\n>> We should free objects before leaving.\n>>\n>> Signed-off-by: Felipe Contreras \n>\n> Micronit: perhaps you should move the \"free obejcts before leaving\"\n> (in do_pick_commit) to the subject instead of \"trivial fix\", which\n> adds no value to the patch.\n\nPerhaps. I prefer it this way because it's really a trivial fix not\nreally worth much time thinking about it. So when somebody is browsing\nthe history they can happily skip this one. The time save by not\nreading I think adds more value than any succinct description that\nwould force each and every patch-reviewer/history-reader to read it.\n\n-- \nFelipe Contreras\n"},{"id":"218833","messageId":"CACsJy8Do-djwjVP1YvGnvsbdiWjE47KTK4pZZ3Qdnubvd-r3Lw@mail.gmail.com","threadId":"33951","inReplyTo":"CAMP44s0cpAkAUzo0nS55yv+6=cCBsBhgNiYxpEd8Hzk=3mfNhw@mail.gmail.com","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-29T13:42:18Z","receivedAt":"2013-05-29T13:42:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras\n wrote:\n> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen wrote:\n>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras\n>> wrote:\n>>> We should free objects before leaving.\n>>>\n>>> Signed-off-by: Felipe Contreras \n>>\n>> Micronit: perhaps you should move the \"free obejcts before leaving\"\n>> (in do_pick_commit) to the subject instead of \"trivial fix\", which\n>> adds no value to the patch.\n>\n> Perhaps. I prefer it this way because it's really a trivial fix not\n> really worth much time thinking about it. So when somebody is browsing\n> the history they can happily skip this one. The time save by not\n> reading I think adds more value than any succinct description that\n> would force each and every patch-reviewer/history-reader to read it.\n\nSome time from now, assume a ridiculus case when this function grows\nmore complex and somebody wonders what the \"leave\" label is for, \"git\nlog --oneline -Slabel:\" showing \"trivial fix\" would not help much.\n--\nDuy\n"},{"id":"218836","messageId":"CAMP44s0vaPUmvS8OzZ_4UR5XmKK-fyU4nE47Djjr215zkgpRFg@mail.gmail.com","threadId":"33951","inReplyTo":"CACsJy8Do-djwjVP1YvGnvsbdiWjE47KTK4pZZ3Qdnubvd-r3Lw@mail.gmail.com","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T13:46:50Z","receivedAt":"2013-05-29T13:46:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 8:42 AM, Duy Nguyen wrote:\n> On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras\n> wrote:\n>> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen wrote:\n>>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras\n>>> wrote:\n>>>> We should free objects before leaving.\n>>>>\n>>>> Signed-off-by: Felipe Contreras \n>>>\n>>> Micronit: perhaps you should move the \"free obejcts before leaving\"\n>>> (in do_pick_commit) to the subject instead of \"trivial fix\", which\n>>> adds no value to the patch.\n>>\n>> Perhaps. I prefer it this way because it's really a trivial fix not\n>> really worth much time thinking about it. So when somebody is browsing\n>> the history they can happily skip this one. The time save by not\n>> reading I think adds more value than any succinct description that\n>> would force each and every patch-reviewer/history-reader to read it.\n>\n> Some time from now, assume a ridiculus case when this function grows\n> more complex and somebody wonders what the \"leave\" label is for, \"git\n> log --oneline -Slabel:\" showing \"trivial fix\" would not help much.\n\nFortunately that's not the main use-case, and for that single instance\nthat probably will never happen, I think it's not too much to ask to\nthis hypothetical developer to remove the --oneline, or copy-paste the\nSHA-1 and take a peek. He would probably need to do that anyway.\n\n-- \nFelipe Contreras\n"},{"id":"218841","messageId":"CACsJy8Br1gigKL2GHQxC-9X8nizXzWcBysVuCHCGh1F3DiGp8Q@mail.gmail.com","threadId":"33951","inReplyTo":"CAMP44s0vaPUmvS8OzZ_4UR5XmKK-fyU4nE47Djjr215zkgpRFg@mail.gmail.com","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-29T13:54:23Z","receivedAt":"2013-05-29T13:54:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 29, 2013 at 8:46 PM, Felipe Contreras\n wrote:\n> On Wed, May 29, 2013 at 8:42 AM, Duy Nguyen wrote:\n>> On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras\n>> wrote:\n>>> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen wrote:\n>>>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras\n>>>> wrote:\n>>>>> We should free objects before leaving.\n>>>>>\n>>>>> Signed-off-by: Felipe Contreras \n>>>>\n>>>> Micronit: perhaps you should move the \"free obejcts before leaving\"\n>>>> (in do_pick_commit) to the subject instead of \"trivial fix\", which\n>>>> adds no value to the patch.\n>>>\n>>> Perhaps. I prefer it this way because it's really a trivial fix not\n>>> really worth much time thinking about it. So when somebody is browsing\n>>> the history they can happily skip this one. The time save by not\n>>> reading I think adds more value than any succinct description that\n>>> would force each and every patch-reviewer/history-reader to read it.\n>>\n>> Some time from now, assume a ridiculus case when this function grows\n>> more complex and somebody wonders what the \"leave\" label is for, \"git\n>> log --oneline -Slabel:\" showing \"trivial fix\" would not help much.\n>\n> Fortunately that's not the main use-case, and for that single instance\n> that probably will never happen, I think it's not too much to ask to\n> this hypothetical developer to remove the --oneline, or copy-paste the\n> SHA-1 and take a peek. He would probably need to do that anyway.\n\nAnd the \"time saving by not reading\" is also hypothetical. But I won't\ncontinue this discussion.\n--\nDuy\n"},{"id":"218843","messageId":"CAMP44s03Gig4j5RD+7FnjWppHZ0rsJ0mvFrrVbpYoRmyU=xJWA@mail.gmail.com","threadId":"33951","inReplyTo":"CACsJy8Br1gigKL2GHQxC-9X8nizXzWcBysVuCHCGh1F3DiGp8Q@mail.gmail.com","subject":"Re: [PATCH 1/2] sequencer: trivial fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T14:10:44Z","receivedAt":"2013-05-29T14:10:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 8:54 AM, Duy Nguyen wrote:\n> On Wed, May 29, 2013 at 8:46 PM, Felipe Contreras\n> wrote:\n>> On Wed, May 29, 2013 at 8:42 AM, Duy Nguyen wrote:\n>>> On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras\n>>> wrote:\n>>>> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen wrote:\n>>>>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras\n>>>>> wrote:\n>>>>>> We should free objects before leaving.\n>>>>>>\n>>>>>> Signed-off-by: Felipe Contreras \n>>>>>\n>>>>> Micronit: perhaps you should move the \"free obejcts before leaving\"\n>>>>> (in do_pick_commit) to the subject instead of \"trivial fix\", which\n>>>>> adds no value to the patch.\n>>>>\n>>>> Perhaps. I prefer it this way because it's really a trivial fix not\n>>>> really worth much time thinking about it. So when somebody is browsing\n>>>> the history they can happily skip this one. The time save by not\n>>>> reading I think adds more value than any succinct description that\n>>>> would force each and every patch-reviewer/history-reader to read it.\n>>>\n>>> Some time from now, assume a ridiculus case when this function grows\n>>> more complex and somebody wonders what the \"leave\" label is for, \"git\n>>> log --oneline -Slabel:\" showing \"trivial fix\" would not help much.\n>>\n>> Fortunately that's not the main use-case, and for that single instance\n>> that probably will never happen, I think it's not too much to ask to\n>> this hypothetical developer to remove the --oneline, or copy-paste the\n>> SHA-1 and take a peek. He would probably need to do that anyway.\n>\n> And the \"time saving by not reading\" is also hypothetical. But I won't\n> continue this discussion.\n\nIs it? How much time does it take to read \"trivial fix\"? Half a\nsecond? How much time does it take copy-paste the SHA-1 of a --oneline\nlog? Five seconds? So to break even we need ten readers that would\nonly browse the history per each person that goes beyond the summary.\nTo be safe let's do +- 100% and make it twenty readers.\n\nI think it's safe to assume there will be more than 20 readers\nskipping this commit without much though, perhaps a 100 or even more,\nand how many would need to take a closer look? I'd say 0, 1 might be\npossible, but to err on the side of caution let's say 2, hell, let's\nbe generous and make it 3. We are still safe well beyond profit.\n\nBut we have already wasted many more seconds than any of those guys\nwould, so does it really matter?\n\n-- \nFelipe Contreras\n"}]}