| Message ID | 16fec87766f97d46a337f5c514f1aec0668546ec.1739723830.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | merge-tree --stdin: flush stdout | expand |
On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > real_merge() only ever returns "0" or "1" as it dies if the merge status > is less than zero. Therefore the check for "result < 0" is redundant and > the result variable is not needed. Indeed, the only return statement in real_merge(), occurring on the last line of the function, is even: return !result.clean; /* result.clean < 0 handled above */ However, it might be worth adding to the commit message some comments about o->use_stdin here. When o->use_stdin is true, that the program exit status is 0 for both successful merges and conflicts but the conflict status for each individual commit is printed as part of the output. As such, the return status isn't used in those cases and real_merge() might as well be a void function. However, when o->use_stdin is false, the exit status from real_merge is used, which is why that callsite (not visibile in this patch since it is unmodified) still pays attention to real_merge()'s return status. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > builtin/merge-tree.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c > index 57f4340faba..3c73482f2b0 100644 > --- a/builtin/merge-tree.c > +++ b/builtin/merge-tree.c > @@ -601,7 +601,6 @@ int cmd_merge_tree(int argc, > line_termination = '\0'; > while (strbuf_getline_lf(&buf, stdin) != EOF) { > struct strbuf **split; > - int result; > const char *input_merge_base = NULL; > > split = strbuf_split(&buf, ' '); > @@ -618,16 +617,14 @@ int cmd_merge_tree(int argc, > if (input_merge_base && split[2] && split[3] && !split[4]) { > strbuf_rtrim(split[2]); > strbuf_rtrim(split[3]); > - result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix); > + real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix); > } else if (!input_merge_base && !split[2]) { > - result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix); > + real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix); > } else { > die(_("malformed input line: '%s'."), buf.buf); > } > maybe_flush_or_die(stdout, "stdout"); > > - if (result < 0) > - die(_("merging cannot continue; got unclean result of %d"), result); > strbuf_list_free(split); > } > strbuf_release(&buf); > -- > gitgitgadget Looks good.
Hi Elijah On 17/02/2025 20:15, Elijah Newren wrote: > On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> real_merge() only ever returns "0" or "1" as it dies if the merge status >> is less than zero. Therefore the check for "result < 0" is redundant and >> the result variable is not needed. > > Indeed, the only return statement in real_merge(), occurring on the > last line of the function, is even: > return !result.clean; /* result.clean < 0 handled above */ > > However, it might be worth adding to the commit message some comments > about o->use_stdin here. When o->use_stdin is true, that the program > exit status is 0 for both successful merges and conflicts but the > conflict status for each individual commit is printed as part of the > output. As such, the return status isn't used in those cases and > real_merge() might as well be a void function. However, when > o->use_stdin is false, the exit status from real_merge is used, which > is why that callsite (not visibile in this patch since it is > unmodified) still pays attention to real_merge()'s return status. That's a good suggestion - I'll re-roll Thanks Phillip >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> builtin/merge-tree.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c >> index 57f4340faba..3c73482f2b0 100644 >> --- a/builtin/merge-tree.c >> +++ b/builtin/merge-tree.c >> @@ -601,7 +601,6 @@ int cmd_merge_tree(int argc, >> line_termination = '\0'; >> while (strbuf_getline_lf(&buf, stdin) != EOF) { >> struct strbuf **split; >> - int result; >> const char *input_merge_base = NULL; >> >> split = strbuf_split(&buf, ' '); >> @@ -618,16 +617,14 @@ int cmd_merge_tree(int argc, >> if (input_merge_base && split[2] && split[3] && !split[4]) { >> strbuf_rtrim(split[2]); >> strbuf_rtrim(split[3]); >> - result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix); >> + real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix); >> } else if (!input_merge_base && !split[2]) { >> - result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix); >> + real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix); >> } else { >> die(_("malformed input line: '%s'."), buf.buf); >> } >> maybe_flush_or_die(stdout, "stdout"); >> >> - if (result < 0) >> - die(_("merging cannot continue; got unclean result of %d"), result); >> strbuf_list_free(split); >> } >> strbuf_release(&buf); >> -- >> gitgitgadget > > Looks good. >
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 57f4340faba..3c73482f2b0 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -601,7 +601,6 @@ int cmd_merge_tree(int argc, line_termination = '\0'; while (strbuf_getline_lf(&buf, stdin) != EOF) { struct strbuf **split; - int result; const char *input_merge_base = NULL; split = strbuf_split(&buf, ' '); @@ -618,16 +617,14 @@ int cmd_merge_tree(int argc, if (input_merge_base && split[2] && split[3] && !split[4]) { strbuf_rtrim(split[2]); strbuf_rtrim(split[3]); - result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix); + real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix); } else if (!input_merge_base && !split[2]) { - result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix); + real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix); } else { die(_("malformed input line: '%s'."), buf.buf); } maybe_flush_or_die(stdout, "stdout"); - if (result < 0) - die(_("merging cannot continue; got unclean result of %d"), result); strbuf_list_free(split); } strbuf_release(&buf);