[PATCH 0/2] cherry-pick: a fix and a new option

[PATCH 0/2] cherry-pick: a fix and a new option

[Felipe Contreras]

Hi,

It doesn't make sense for the user to be interrupted constantly.

Felipe Contreras (2):
  sequencer: trivial fix
  cherry-pick: add --skip-commits option

 Documentation/git-cherry-pick.txt   |  3 +++
 builtin/revert.c                    |  2 ++
 sequencer.c                         | 12 +++++++++---
 sequencer.h                         |  1 +
 t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++
 5 files changed, 28 insertions(+), 3 deletions(-)

-- 
1.8.3.rc3.312.g47657de

[PATCH 1/2] sequencer: trivial fix

[Felipe Contreras]

We should free objects before leaving.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sequencer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..7eeae2f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		rerere(opts->allow_rerere_auto);
 	} else {
 		int allow = allow_empty(opts, commit);
-		if (allow < 0)
-			return allow;
+		if (allow < 0) {
+			res = allow;
+			goto leave;
+		}
 		if (!opts->no_commit)
 			res = run_git_commit(defmsg, opts, allow);
 	}
 
+leave:
 	free_message(&msg);
 	free(defmsg);
 
-- 
1.8.3.rc3.312.g47657de

[PATCH 2/2] cherry-pick: add --skip-commits option

[Felipe Contreras]

Pretty much what it says on the tin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-cherry-pick.txt   |  3 +++
 builtin/revert.c                    |  2 ++
 sequencer.c                         |  5 ++++-
 sequencer.h                         |  1 +
 t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++
 5 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index c205d23..fccd936 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -129,6 +129,9 @@ effect to your index in a row.
 	redundant commits are ignored.  This option overrides that behavior and
 	creates an empty commit object.  Implies `--allow-empty`.
 
+--skip-empty::
+	Instead of failing, skip commits that are or become empty.
+
 --strategy=<strategy>::
 	Use the given merge strategy.  Should only be used once.
 	See the MERGE STRATEGIES section in linkgit:git-merge[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index 0401fdb..0e5ce71 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -118,6 +118,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
@@ -127,6 +128,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOLEAN(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+			OPT_BOOLEAN(0, "skip-empty", &opts->skip_empty, N_("skip empty commits")),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
diff --git a/sequencer.c b/sequencer.c
index 7eeae2f..86e8e78 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -625,7 +625,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		print_advice(res == 1, opts);
 		rerere(opts->allow_rerere_auto);
 	} else {
-		int allow = allow_empty(opts, commit);
+		int allow;
+		if (opts->skip_empty && is_index_unchanged() == 1)
+			goto leave;
+		allow = allow_empty(opts, commit);
 		if (allow < 0) {
 			res = allow;
 			goto leave;
diff --git a/sequencer.h b/sequencer.h
index 1fc22dc..3b04844 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -34,6 +34,7 @@ struct replay_opts {
 	int allow_empty;
 	int allow_empty_message;
 	int keep_redundant_commits;
+	int skip_empty;
 
 	int mainline;
 
diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 19c99d7..3dc19c6 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -187,4 +187,17 @@ test_expect_success 'cherry-pick --stdin works' '
 	check_head_differs_from fourth
 '
 
+test_expect_success 'cherry-pick skip empty' '
+	git clean -fxd &&
+	git checkout -b empty fourth &&
+	git commit --allow-empty -m empty &&
+	test_commit ontop &&
+	git checkout -f master &&
+	git reset --hard fourth &&
+	git cherry-pick --skip-empty fourth..empty &&
+	echo ontop > expected &&
+	git log --format=%s fourth..HEAD > actual
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH 2/2] cherry-pick: add --skip-commits option

[Joachim Schmitz]

Felipe Contreras wrote:
> Pretty much what it says on the tin.

Only that it add --skip-empty and not --skip-commit ?!?
 
Bye, Jojo

Re: [PATCH 1/2] sequencer: trivial fix

[Neil Horman]

On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:
> We should free objects before leaving.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  sequencer.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index ab6f8a7..7eeae2f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		rerere(opts->allow_rerere_auto);
>  	} else {
>  		int allow = allow_empty(opts, commit);
> -		if (allow < 0)
> -			return allow;
> +		if (allow < 0) {
> +			res = allow;
> +			goto leave;
> +		}
>  		if (!opts->no_commit)
>  			res = run_git_commit(defmsg, opts, allow);
>  	}
>  
> +leave:
>  	free_message(&msg);
>  	free(defmsg);
>  
> -- 
> 1.8.3.rc3.312.g47657de
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH 2/2] cherry-pick: add --skip-commits option

[Neil Horman]

On Mon, May 27, 2013 at 11:52:19AM -0500, Felipe Contreras wrote:
> Pretty much what it says on the tin.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-cherry-pick.txt   |  3 +++
>  builtin/revert.c                    |  2 ++
>  sequencer.c                         |  5 ++++-
>  sequencer.h                         |  1 +
>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++
>  5 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index c205d23..fccd936 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -129,6 +129,9 @@ effect to your index in a row.
>  	redundant commits are ignored.  This option overrides that behavior and
>  	creates an empty commit object.  Implies `--allow-empty`.
>  
> +--skip-empty::
> +	Instead of failing, skip commits that are or become empty.
> +
>  --strategy=<strategy>::
>  	Use the given merge strategy.  Should only be used once.
>  	See the MERGE STRATEGIES section in linkgit:git-merge[1]
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 0401fdb..0e5ce71 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -118,6 +118,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  		OPT_END(),
>  		OPT_END(),
>  		OPT_END(),
> +		OPT_END(),
>  	};
>  
>  	if (opts->action == REPLAY_PICK) {
> @@ -127,6 +128,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
>  			OPT_BOOLEAN(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
>  			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
> +			OPT_BOOLEAN(0, "skip-empty", &opts->skip_empty, N_("skip empty commits")),
>  			OPT_END(),
>  		};
I like the idea, but this option seems a bit awkward to me.  At the very least
here, don't you now need to check for conflicts if --keep-redundant-commits and
skip-empty are both specified (as iirc git doens't see the difference between
empty commits and commits made empty by prior commits in the current history).
what if we merged the two options to an OPT_STRING, something like
--empty-commits=[keep|skip|ask].  The default currently is an impiled, since the
sequencer stops on an empty commit.

Neil

Re: [PATCH 1/2] sequencer: trivial fix

[Junio C Hamano]

Neil Horman <nhorman@tuxdriver.com> writes:

> On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:
>> We should free objects before leaving.
>> 
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  sequencer.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/sequencer.c b/sequencer.c
>> index ab6f8a7..7eeae2f 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>>  		rerere(opts->allow_rerere_auto);
>>  	} else {
>>  		int allow = allow_empty(opts, commit);
>> -		if (allow < 0)
>> -			return allow;
>> +		if (allow < 0) {
>> +			res = allow;
>> +			goto leave;
>> +		}
>>  		if (!opts->no_commit)
>>  			res = run_git_commit(defmsg, opts, allow);
>>  	}
>>  
>> +leave:
>>  	free_message(&msg);
>>  	free(defmsg);
>>  
>> -- 
>> 1.8.3.rc3.312.g47657de
>> 
>> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

This is better done without "goto" in general.

The other patch 2/2/ adds one more "we need to exit from the middle
of the flow" and makes it look handier to add an exit label here,
but it would be even better to express the logic of that patch as a
normal cascade of if/else if/..., which is small enough and we do
not need the "leave:" label.

It probably is better to fold this patch into the other one when it
is rerolled to correct the option name gotcha "on the tin".

Thanks.

Re: [PATCH 1/2] sequencer: trivial fix

[Felipe Contreras]

Junio C Hamano wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:
> >> We should free objects before leaving.
> >> 
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  sequencer.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/sequencer.c b/sequencer.c
> >> index ab6f8a7..7eeae2f 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> >>  		rerere(opts->allow_rerere_auto);
> >>  	} else {
> >>  		int allow = allow_empty(opts, commit);
> >> -		if (allow < 0)
> >> -			return allow;
> >> +		if (allow < 0) {
> >> +			res = allow;
> >> +			goto leave;
> >> +		}
> >>  		if (!opts->no_commit)
> >>  			res = run_git_commit(defmsg, opts, allow);
> >>  	}
> >>  
> >> +leave:
> >>  	free_message(&msg);
> >>  	free(defmsg);
> >>  
> >> -- 
> >> 1.8.3.rc3.312.g47657de
> >> 
> >> 
> > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> 
> This is better done without "goto" in general.
> 
> The other patch 2/2/ adds one more "we need to exit from the middle
> of the flow" and makes it look handier to add an exit label here,
> but it would be even better to express the logic of that patch as a
> normal cascade of if/else if/..., which is small enough and we do
> not need the "leave:" label.

Linux kernel developers would disagree. In C 'goto' is quite of then the only
sane option, and you can see 'goto' used in the Linux kernel all over the place
for that reason.

In this particular case it also makes perfect sense.

> It probably is better to fold this patch into the other one when it
> is rerolled to correct the option name gotcha "on the tin".

Why? This patch is standalone and fixes an issue that is independent of the
other patch. Why squash two patches that do *two* different things?

Anyway, I'll happily drop this patch if you want this memory leak to remain.
But then I'll do the same in the other patch.

This mantra of avodiing 'goto' is not helping anybody.

-- 
Felipe Contreras

Re: [PATCH 1/2] sequencer: trivial fix

[Joachim Schmitz]

Felipe Contreras wrote:
> Junio C Hamano wrote:
>> Neil Horman <nhorman@tuxdriver.com> writes:
>>
>>> On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:
>>>> We should free objects before leaving.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>> ---
>>>>  sequencer.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/sequencer.c b/sequencer.c
>>>> index ab6f8a7..7eeae2f 100644
>>>> --- a/sequencer.c
>>>> +++ b/sequencer.c
>>>> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit
>>>>  *commit, struct replay_opts *opts)
>>>>  rerere(opts->allow_rerere_auto); } else {
>>>>  int allow = allow_empty(opts, commit);
>>>> - if (allow < 0)
>>>> - return allow;
>>>> + if (allow < 0) {
>>>> + res = allow;
>>>> + goto leave;
>>>> + }
>>>>  if (!opts->no_commit)
>>>>  res = run_git_commit(defmsg, opts, allow);
>>>>  }
>>>>
>>>> +leave:
>>>>  free_message(&msg);
>>>>  free(defmsg);
>>>>
>>>> --
>>>> 1.8.3.rc3.312.g47657de
>>>>
>>>>
>>> Acked-by: Neil Horman <nhorman@tuxdriver.com>
>>
>> This is better done without "goto" in general.
>>
>> The other patch 2/2/ adds one more "we need to exit from the middle
>> of the flow" and makes it look handier to add an exit label here,
>> but it would be even better to express the logic of that patch as a
>> normal cascade of if/else if/..., which is small enough and we do
>> not need the "leave:" label.
>
> Linux kernel developers would disagree. In C 'goto' is quite of then
> the only sane option, and you can see 'goto' used in the Linux kernel
> all over the place for that reason.
>
> In this particular case it also makes perfect sense.
>
>> It probably is better to fold this patch into the other one when it
>> is rerolled to correct the option name gotcha "on the tin".
>
> Why? This patch is standalone and fixes an issue that is independent
> of the other patch. Why squash two patches that do *two* different
> things?
>
> Anyway, I'll happily drop this patch if you want this memory leak to
> remain. But then I'll do the same in the other patch.
>
> This mantra of avodiing 'goto' is not helping anybody.

adding 5 letters (to change the next "if" into an "else if") versus your 
addition of several lines and some 15 additional letters (ignoring the 
whitsspace)  is IMHO enough to see what is better?

bye, Jojo 

Re: [PATCH 1/2] sequencer: trivial fix

[Felipe Contreras]

On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz
<jojo@schmitz-digital.de> wrote:
> Felipe Contreras wrote:
>>
>> Junio C Hamano wrote:

>>> It probably is better to fold this patch into the other one when it
>>> is rerolled to correct the option name gotcha "on the tin".
>>
>>
>> Why? This patch is standalone and fixes an issue that is independent
>> of the other patch. Why squash two patches that do *two* different
>> things?
>>
>> Anyway, I'll happily drop this patch if you want this memory leak to
>> remain. But then I'll do the same in the other patch.
>>
>> This mantra of avodiing 'goto' is not helping anybody.
>
>
> adding 5 letters (to change the next "if" into an "else if") versus your
> addition of several lines and some 15 additional letters (ignoring the
> whitsspace)  is IMHO enough to see what is better?

This has nothing to do with what Junio said. Junio said it is better
to squash the two changes into one, which is not clearly better.

As for your suggestion, what happens the next time somebody needs to
add something else to this chunk of code? Another if, and then
another, and soon enough you end up with five levels of indentation,
and in some of those patches you have to change the indentation of
existing code.

If only there was much bigger and successful software project that had
hashed all these questions and came up with a code-style to last the
ages. Oh, but there is, it's called Linux, and the answer is to use
goto's.

If the code used a goto in the first place.. BAM:

--- a/sequencer.c
+++ b/sequencer.c
 <at>  <at>  -628,8 +628,10  <at>  <at>  static int
do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	}

 	allow = allow_empty(opts, commit);
-	if (allow < 0)
-		return allow;
+	if (allow < 0) {
+		res = allow;
+		goto leave;
+	}
 	if (!opts->no_commit)
 		res = run_git_commit(defmsg, opts, allow);

And every time you need to add more code you just do it, and stop
worrying about increasing indentation, or re-indenting.

Problem solved.

-- 
Felipe Contreras

RE: [PATCH 1/2] sequencer: trivial fix

[Joachim Schmitz]

> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Wednesday, May 29, 2013 12:52 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH 1/2] sequencer: trivial fix
> 
> On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz
> <jojo@schmitz-digital.de> wrote:
> > Felipe Contreras wrote:
> >>
> >> Junio C Hamano wrote:
> 
> >>> It probably is better to fold this patch into the other one when it
> >>> is rerolled to correct the option name gotcha "on the tin".
> >>
> >>
> >> Why? This patch is standalone and fixes an issue that is independent
> >> of the other patch. Why squash two patches that do *two* different
> >> things?
> >>
> >> Anyway, I'll happily drop this patch if you want this memory leak to
> >> remain. But then I'll do the same in the other patch.
> >>
> >> This mantra of avodiing 'goto' is not helping anybody.
> >
> >
> > adding 5 letters (to change the next "if" into an "else if") versus your
> > addition of several lines and some 15 additional letters (ignoring the
> > whitsspace)  is IMHO enough to see what is better?
> 
> This has nothing to do with what Junio said. 

Well, it has, but you had snipped it. But replied to the goto issue regardless

> This is better done without "goto" in general.

Bye, Jojo

Re: [PATCH 1/2] sequencer: trivial fix

[Felipe Contreras]

On Wed, May 29, 2013 at 6:13 AM, Joachim Schmitz
<jojo@schmitz-digital.de> wrote:
>> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
>> Sent: Wednesday, May 29, 2013 12:52 PM
>> To: Joachim Schmitz
>> Cc: git@vger.kernel.org
>> Subject: Re: [PATCH 1/2] sequencer: trivial fix
>>
>> On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz
>> <jojo@schmitz-digital.de> wrote:
>> > Felipe Contreras wrote:
>> >>
>> >> Junio C Hamano wrote:
>>
>> >>> It probably is better to fold this patch into the other one when it
>> >>> is rerolled to correct the option name gotcha "on the tin".
>> >>
>> >>
>> >> Why? This patch is standalone and fixes an issue that is independent
>> >> of the other patch. Why squash two patches that do *two* different
>> >> things?
>> >>
>> >> Anyway, I'll happily drop this patch if you want this memory leak to
>> >> remain. But then I'll do the same in the other patch.
>> >>
>> >> This mantra of avodiing 'goto' is not helping anybody.
>> >
>> >
>> > adding 5 letters (to change the next "if" into an "else if") versus your
>> > addition of several lines and some 15 additional letters (ignoring the
>> > whitsspace)  is IMHO enough to see what is better?
>>
>> This has nothing to do with what Junio said.
>
> Well, it has, but you had snipped it. But replied to the goto issue regardless

I didn't snip anything, this is a different context.

>> This is better done without "goto" in general.

He din't say:

__
It probably is better to fold this patch into the other one when it
is rerolled to correct the option name gotcha "on the tin", AND you
fix the goto issue.
__

You added that last part in your mind. Moreover, he didn't say goto
was an issue, he simply stated an opinion about some generality.

-- 
Felipe Contreras

RE: [PATCH 1/2] sequencer: trivial fix

[Joachim Schmitz]

> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Wednesday, May 29, 2013 1:24 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH 1/2] sequencer: trivial fix
> 
> On Wed, May 29, 2013 at 6:13 AM, Joachim Schmitz
> <jojo@schmitz-digital.de> wrote:
> >> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> >> Sent: Wednesday, May 29, 2013 12:52 PM
> >> To: Joachim Schmitz
> >> Cc: git@vger.kernel.org
> >> Subject: Re: [PATCH 1/2] sequencer: trivial fix
> >>
> >> On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz
> >> <jojo@schmitz-digital.de> wrote:
> >> > Felipe Contreras wrote:
> >> >>
> >> >> Junio C Hamano wrote:
> >>
> >> >>> It probably is better to fold this patch into the other one when it
> >> >>> is rerolled to correct the option name gotcha "on the tin".
> >> >>
> >> >>
> >> >> Why? This patch is standalone and fixes an issue that is independent
> >> >> of the other patch. Why squash two patches that do *two* different
> >> >> things?
> >> >>
> >> >> Anyway, I'll happily drop this patch if you want this memory leak to
> >> >> remain. But then I'll do the same in the other patch.
> >> >>
> >> >> This mantra of avodiing 'goto' is not helping anybody.
> >> >
> >> >
> >> > adding 5 letters (to change the next "if" into an "else if") versus your
> >> > addition of several lines and some 15 additional letters (ignoring the
> >> > whitsspace)  is IMHO enough to see what is better?
> >>
> >> This has nothing to do with what Junio said.
> >
> > Well, it has, but you had snipped it. But replied to the goto issue regardless
> 
> I didn't snip anything, this is a different context.

You did in your reply to me

> >> This is better done without "goto" in general.
> 
> He din't say:
> __
> It probably is better to fold this patch into the other one when it
> is rerolled to correct the option name gotcha "on the tin", AND you
> fix the goto issue.
> __
> 
> You added that last part in your mind. Moreover, he didn't say goto
> was an issue, he simply stated an opinion about some generality.

I added nothing in my mind, I just copy/paste that statement and was commenting on that and only that.
At least intended to.

Whenever anybody added more else branches, that's the time to possible switch to the goto style.

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.

Bye, Jojo

RE: [PATCH 1/2] sequencer: trivial fix

[Joachim Schmitz]

> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]
> Sent: Wednesday, May 29, 2013 1:30 PM
> To: 'Felipe Contreras'
> Cc: 'git@vger.kernel.org'
> Subject: RE: [PATCH 1/2] sequencer: trivial fix
<snip>
> 
> 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.

I take that back: your patches 'overlap' so the 2nd won't apply without the 1st

 Bye, Jojo

Re: [PATCH 1/2] sequencer: trivial fix

[Neil Horman]

On Tue, May 28, 2013 at 09:32:59PM -0500, Felipe Contreras wrote:
> Junio C Hamano wrote:
> > Neil Horman <nhorman@tuxdriver.com> writes:
> > 
> > > On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:
> > >> We should free objects before leaving.
> > >> 
> > >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > >> ---
> > >>  sequencer.c | 7 +++++--
> > >>  1 file changed, 5 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/sequencer.c b/sequencer.c
> > >> index ab6f8a7..7eeae2f 100644
> > >> --- a/sequencer.c
> > >> +++ b/sequencer.c
> > >> @@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> > >>  		rerere(opts->allow_rerere_auto);
> > >>  	} else {
> > >>  		int allow = allow_empty(opts, commit);
> > >> -		if (allow < 0)
> > >> -			return allow;
> > >> +		if (allow < 0) {
> > >> +			res = allow;
> > >> +			goto leave;
> > >> +		}
> > >>  		if (!opts->no_commit)
> > >>  			res = run_git_commit(defmsg, opts, allow);
> > >>  	}
> > >>  
> > >> +leave:
> > >>  	free_message(&msg);
> > >>  	free(defmsg);
> > >>  
> > >> -- 
> > >> 1.8.3.rc3.312.g47657de
> > >> 
> > >> 
> > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > This is better done without "goto" in general.
> > 
> > The other patch 2/2/ adds one more "we need to exit from the middle
> > of the flow" and makes it look handier to add an exit label here,
> > but it would be even better to express the logic of that patch as a
> > normal cascade of if/else if/..., which is small enough and we do
> > not need the "leave:" label.
> 
> Linux kernel developers would disagree. In C 'goto' is quite of then the only
> sane option, and you can see 'goto' used in the Linux kernel all over the place
> for that reason.
> 
> In this particular case it also makes perfect sense.
> 
I agree with Felipe here.  Setting asside coding practice in other projects,
while its nice to follow coding convention in a project, a jump label just makes
more sense here.  To not use it either requires you to duplicate the free
statements (undesireable), or to change the sense of theif clause here and nest
your if statements (makes for ugly reading).

> > It probably is better to fold this patch into the other one when it
> > is rerolled to correct the option name gotcha "on the tin".
> 
> Why? This patch is standalone and fixes an issue that is independent of the
> other patch. Why squash two patches that do *two* different things?
> 
I agree here as well.  This fixes a bug that has nothing to do with the other
patch, save for it being in the same C file.  Fix them separately.

> Anyway, I'll happily drop this patch if you want this memory leak to remain.
> But then I'll do the same in the other patch.
> 
> This mantra of avodiing 'goto' is not helping anybody.
> 
> -- 
> Felipe Contreras
> 

Re: [PATCH 1/2] sequencer: trivial fix

[Duy Nguyen]

On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> We should free objects before leaving.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Micronit: perhaps you should move the "free obejcts before leaving"
(in do_pick_commit) to the subject instead of "trivial fix", which
adds no value to the patch.
--
Duy

Re: [PATCH 1/2] sequencer: trivial fix

[Felipe Contreras]

On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> We should free objects before leaving.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> Micronit: perhaps you should move the "free obejcts before leaving"
> (in do_pick_commit) to the subject instead of "trivial fix", which
> adds no value to the patch.

Perhaps. I prefer it this way because it's really a trivial fix not
really worth much time thinking about it. So when somebody is browsing
the history they can happily skip this one. The time save by not
reading I think adds more value than any succinct description that
would force each and every patch-reviewer/history-reader to read it.

-- 
Felipe Contreras

Re: [PATCH 1/2] sequencer: trivial fix

[Duy Nguyen]

On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> We should free objects before leaving.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> Micronit: perhaps you should move the "free obejcts before leaving"
>> (in do_pick_commit) to the subject instead of "trivial fix", which
>> adds no value to the patch.
>
> Perhaps. I prefer it this way because it's really a trivial fix not
> really worth much time thinking about it. So when somebody is browsing
> the history they can happily skip this one. The time save by not
> reading I think adds more value than any succinct description that
> would force each and every patch-reviewer/history-reader to read it.

Some time from now, assume a ridiculus case when this function grows
more complex and somebody wonders what the "leave" label is for, "git
log --oneline -Slabel:" showing "trivial fix" would not help much.
--
Duy

Re: [PATCH 1/2] sequencer: trivial fix

[Felipe Contreras]

On Wed, May 29, 2013 at 8:42 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras
>>> <felipe.contreras@gmail.com> wrote:
>>>> We should free objects before leaving.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>
>>> Micronit: perhaps you should move the "free obejcts before leaving"
>>> (in do_pick_commit) to the subject instead of "trivial fix", which
>>> adds no value to the patch.
>>
>> Perhaps. I prefer it this way because it's really a trivial fix not
>> really worth much time thinking about it. So when somebody is browsing
>> the history they can happily skip this one. The time save by not
>> reading I think adds more value than any succinct description that
>> would force each and every patch-reviewer/history-reader to read it.
>
> Some time from now, assume a ridiculus case when this function grows
> more complex and somebody wonders what the "leave" label is for, "git
> log --oneline -Slabel:" showing "trivial fix" would not help much.

Fortunately that's not the main use-case, and for that single instance
that probably will never happen, I think it's not too much to ask to
this hypothetical developer to remove the --oneline, or copy-paste the
SHA-1 and take a peek. He would probably need to do that anyway.

-- 
Felipe Contreras

Re: [PATCH 1/2] sequencer: trivial fix

[Duy Nguyen]

On Wed, May 29, 2013 at 8:46 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, May 29, 2013 at 8:42 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras
>>>> <felipe.contreras@gmail.com> wrote:
>>>>> We should free objects before leaving.
>>>>>
>>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>>
>>>> Micronit: perhaps you should move the "free obejcts before leaving"
>>>> (in do_pick_commit) to the subject instead of "trivial fix", which
>>>> adds no value to the patch.
>>>
>>> Perhaps. I prefer it this way because it's really a trivial fix not
>>> really worth much time thinking about it. So when somebody is browsing
>>> the history they can happily skip this one. The time save by not
>>> reading I think adds more value than any succinct description that
>>> would force each and every patch-reviewer/history-reader to read it.
>>
>> Some time from now, assume a ridiculus case when this function grows
>> more complex and somebody wonders what the "leave" label is for, "git
>> log --oneline -Slabel:" showing "trivial fix" would not help much.
>
> Fortunately that's not the main use-case, and for that single instance
> that probably will never happen, I think it's not too much to ask to
> this hypothetical developer to remove the --oneline, or copy-paste the
> SHA-1 and take a peek. He would probably need to do that anyway.

And the "time saving by not reading" is also hypothetical. But I won't
continue this discussion.
--
Duy

Re: [PATCH 1/2] sequencer: trivial fix

[Felipe Contreras]

On Wed, May 29, 2013 at 8:54 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, May 29, 2013 at 8:46 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Wed, May 29, 2013 at 8:42 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Wed, May 29, 2013 at 8:34 PM, Felipe Contreras
>>> <felipe.contreras@gmail.com> wrote:
>>>> On Wed, May 29, 2013 at 8:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>>> On Mon, May 27, 2013 at 11:52 PM, Felipe Contreras
>>>>> <felipe.contreras@gmail.com> wrote:
>>>>>> We should free objects before leaving.
>>>>>>
>>>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>>>
>>>>> Micronit: perhaps you should move the "free obejcts before leaving"
>>>>> (in do_pick_commit) to the subject instead of "trivial fix", which
>>>>> adds no value to the patch.
>>>>
>>>> Perhaps. I prefer it this way because it's really a trivial fix not
>>>> really worth much time thinking about it. So when somebody is browsing
>>>> the history they can happily skip this one. The time save by not
>>>> reading I think adds more value than any succinct description that
>>>> would force each and every patch-reviewer/history-reader to read it.
>>>
>>> Some time from now, assume a ridiculus case when this function grows
>>> more complex and somebody wonders what the "leave" label is for, "git
>>> log --oneline -Slabel:" showing "trivial fix" would not help much.
>>
>> Fortunately that's not the main use-case, and for that single instance
>> that probably will never happen, I think it's not too much to ask to
>> this hypothetical developer to remove the --oneline, or copy-paste the
>> SHA-1 and take a peek. He would probably need to do that anyway.
>
> And the "time saving by not reading" is also hypothetical. But I won't
> continue this discussion.

Is it? How much time does it take to read "trivial fix"? Half a
second? How much time does it take copy-paste the SHA-1 of a --oneline
log? Five seconds? So to break even we need ten readers that would
only browse the history per each person that goes beyond the summary.
To be safe let's do +- 100% and make it twenty readers.

I think it's safe to assume there will be more than 20 readers
skipping this commit without much though, perhaps a 100 or even more,
and how many would need to take a closer look? I'd say 0, 1 might be
possible, but to err on the side of caution let's say 2, hell, let's
be generous and make it 3. We are still safe well beyond profit.

But we have already wasted many more seconds than any of those guys
would, so does it really matter?

-- 
Felipe Contreras