Re: [PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts

[Johannes Schindelin]

Hi Junio,

On Thu, 20 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > The return value of do_recursive_merge() may be positive (indicating merge
> > conflicts), or 0 (indicating success). It also may be negative, indicating
> > a fatal error that requires us to abort.
> >
> > Now, if the return value indicates that there are merge conflicts, we
> > should not try to commit those changes, of course.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index cbc3742..9ffc090 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, 
> > struct commit *commit,
> > res = allow;
> > goto leave;
> > }
> > -   if (!opts->no_commit)
> > +   if (!res && !opts->no_commit)
> > res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> >  opts, allow, opts->edit, 0, 0);
> 
> This by itself looks more like a bugfix than preparation for later
> steps.  The only reason why it is not a bugfix is because there is
> nothing in this function that makes res a non-zero value and reach
> this if statement at this step.  We would have been caught by an 
> "if (res) { ... rerere(); goto leave; }" or 
> "if (allow < 0) { res = allow; goto leave; }" 
> that appear before this part of the code.
> 
> So while it is not wrong per-se, I think this should be part of an
> actual change that makes it possible for the control flow to reach
> here with non-zero res.

It looks like it is no longer needed (I *think* that it was made obsolete
by the change where I now "goto fast_forward_edit" only in case there were
no errors).

In any case, the patch's gone now,
Dscho

Re: [PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts

[Junio C Hamano]

Johannes Schindelin  writes:

> The return value of do_recursive_merge() may be positive (indicating merge
> conflicts), or 0 (indicating success). It also may be negative, indicating
> a fatal error that requires us to abort.
>
> Now, if the return value indicates that there are merge conflicts, we
> should not try to commit those changes, of course.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index cbc3742..9ffc090 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   res = allow;
>   goto leave;
>   }
> - if (!opts->no_commit)
> + if (!res && !opts->no_commit)
>   res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
>opts, allow, opts->edit, 0, 0);

This by itself looks more like a bugfix than preparation for later
steps.  The only reason why it is not a bugfix is because there is
nothing in this function that makes res a non-zero value and reach
this if statement at this step.  We would have been caught by an 
"if (res) { ... rerere(); goto leave; }" or 
"if (allow < 0) { res = allow; goto leave; }" 
that appear before this part of the code.

So while it is not wrong per-se, I think this should be part of an
actual change that makes it possible for the control flow to reach
here with non-zero res.

[PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts

[Johannes Schindelin]

The return value of do_recursive_merge() may be positive (indicating merge
conflicts), or 0 (indicating success). It also may be negative, indicating
a fatal error that requires us to abort.

Now, if the return value indicates that there are merge conflicts, we
should not try to commit those changes, of course.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index cbc3742..9ffc090 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
res = allow;
goto leave;
}
-   if (!opts->no_commit)
+   if (!res && !opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
 opts, allow, opts->edit, 0, 0);
 
-- 
2.10.1.513.g00ef6dd