| Message ID | 20210108092345.2178-2-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase -i: add options to fixup command | expand |
On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup > commands, there's no point in writing it for squash commands as it is > immediately deleted. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > --- > sequencer.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 8909a46770..f888a7ed3b 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r, > return error(_("could not read HEAD's commit message")); > > find_commit_subject(head_message, &body); > - if (write_message(body, strlen(body), > - rebase_path_fixup_msg(), 0)) { > - unuse_commit_buffer(head_commit, head_message); > - return error(_("cannot write '%s'"), > - rebase_path_fixup_msg()); > + if (command == TODO_FIXUP) { > + if (write_message(body, strlen(body), > + rebase_path_fixup_msg(), 0)) { > + unuse_commit_buffer(head_commit, head_message); > + return error(_("cannot write '%s'"), > + rebase_path_fixup_msg()); > + } I'm nit-picking here, but would this be clearer instead as: if (command == TODO_FIXUP && write_message(...) < 0) { unuse_commit_buffer(...); // ... } There are two changes there. One is two squash the two if-statements together, and the latter is to add a check that 'write_message()' returns an error. This explicit '< 0' checking was discussed recently in another thread[1], and I think makes the conditional here read more clearly. Thanks, Taylor [1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/
On Thu, 14 Jan 2021 at 00:13, Taylor Blau <me@ttaylorr.com> wrote: > > On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > > > The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup > > commands, there's no point in writing it for squash commands as it is > > immediately deleted. > > > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > > --- > > sequencer.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index 8909a46770..f888a7ed3b 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r, > > return error(_("could not read HEAD's commit message")); > > > > find_commit_subject(head_message, &body); > > - if (write_message(body, strlen(body), > > - rebase_path_fixup_msg(), 0)) { > > - unuse_commit_buffer(head_commit, head_message); > > - return error(_("cannot write '%s'"), > > - rebase_path_fixup_msg()); > > + if (command == TODO_FIXUP) { > > + if (write_message(body, strlen(body), > > + rebase_path_fixup_msg(), 0)) { > > + unuse_commit_buffer(head_commit, head_message); > > + return error(_("cannot write '%s'"), > > + rebase_path_fixup_msg()); > > + } > > I'm nit-picking here, but would this be clearer instead as: > > if (command == TODO_FIXUP && write_message(...) < 0) { > unuse_commit_buffer(...); > // ... > } > > There are two changes there. One is two squash the two if-statements > together, and the latter is to add a check that 'write_message()' > returns an error. This explicit '< 0' checking was discussed recently in > another thread[1], and I think makes the conditional here read more > clearly. > Okay, I got this and will change it. Thanks and Regards, Charvi > Thanks, > Taylor > > [1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/
Hi Taylor and Charvi On 14/01/2021 08:12, Charvi Mendiratta wrote: > On Thu, 14 Jan 2021 at 00:13, Taylor Blau <me@ttaylorr.com> wrote: >> >> On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote: >>> From: Phillip Wood <phillip.wood@dunelm.org.uk> >>> >>> The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup >>> commands, there's no point in writing it for squash commands as it is >>> immediately deleted. >>> >>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> >>> --- >>> sequencer.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/sequencer.c b/sequencer.c >>> index 8909a46770..f888a7ed3b 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r, >>> return error(_("could not read HEAD's commit message")); >>> >>> find_commit_subject(head_message, &body); >>> - if (write_message(body, strlen(body), >>> - rebase_path_fixup_msg(), 0)) { >>> - unuse_commit_buffer(head_commit, head_message); >>> - return error(_("cannot write '%s'"), >>> - rebase_path_fixup_msg()); >>> + if (command == TODO_FIXUP) { >>> + if (write_message(body, strlen(body), >>> + rebase_path_fixup_msg(), 0)) { >>> + unuse_commit_buffer(head_commit, head_message); >>> + return error(_("cannot write '%s'"), >>> + rebase_path_fixup_msg()); >>> + } >> >> I'm nit-picking here, but would this be clearer instead as: >> >> if (command == TODO_FIXUP && write_message(...) < 0) { >> unuse_commit_buffer(...); >> // ... >> } >> >> There are two changes there. One is two squash the two if-statements >> together, and the latter is to add a check that 'write_message()' >> returns an error. This explicit '< 0' checking was discussed recently in >> another thread[1], and I think makes the conditional here read more >> clearly. I don't feel that strongly but the addition of '< 0' feels like it is adding an unrelated change to this commit. It also leaves a code base where most callers of `write_message()` do not check the sign of the return value but a couple do (there appears to be one that checks the sign already and a couple that completely ignore the return value). If we want to standardize on always checking the sign of the return value of functions when checking for errors even when they never return a positive value then I think someone in favor of that change should propose a patch to the coding guidelines so it is clear what our policy is. When I see a '< 0`' check I tend to think the positive value has a non-error meaning. Best Wishes Phillip > Okay, I got this and will change it. > > Thanks and Regards, > Charvi > >> Thanks, >> Taylor >> >> [1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/
On Thu, 14 Jan 2021 at 16:16, Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Taylor and Charvi > > On 14/01/2021 08:12, Charvi Mendiratta wrote: > > On Thu, 14 Jan 2021 at 00:13, Taylor Blau <me@ttaylorr.com> wrote: > >> > >> On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote: > >>> From: Phillip Wood <phillip.wood@dunelm.org.uk> > >>> > >>> The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup > >>> commands, there's no point in writing it for squash commands as it is > >>> immediately deleted. > >>> > >>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > >>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > >>> --- > >>> sequencer.c | 12 +++++++----- > >>> 1 file changed, 7 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/sequencer.c b/sequencer.c > >>> index 8909a46770..f888a7ed3b 100644 > >>> --- a/sequencer.c > >>> +++ b/sequencer.c > >>> @@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r, > >>> return error(_("could not read HEAD's commit message")); > >>> > >>> find_commit_subject(head_message, &body); > >>> - if (write_message(body, strlen(body), > >>> - rebase_path_fixup_msg(), 0)) { > >>> - unuse_commit_buffer(head_commit, head_message); > >>> - return error(_("cannot write '%s'"), > >>> - rebase_path_fixup_msg()); > >>> + if (command == TODO_FIXUP) { > >>> + if (write_message(body, strlen(body), > >>> + rebase_path_fixup_msg(), 0)) { > >>> + unuse_commit_buffer(head_commit, head_message); > >>> + return error(_("cannot write '%s'"), > >>> + rebase_path_fixup_msg()); > >>> + } > >> > >> I'm nit-picking here, but would this be clearer instead as: > >> > >> if (command == TODO_FIXUP && write_message(...) < 0) { > >> unuse_commit_buffer(...); > >> // ... > >> } > >> > >> There are two changes there. One is two squash the two if-statements > >> together, and the latter is to add a check that 'write_message()' > >> returns an error. This explicit '< 0' checking was discussed recently in > >> another thread[1], and I think makes the conditional here read more > >> clearly. > > I don't feel that strongly but the addition of '< 0' feels like it is > adding an unrelated change to this commit. It also leaves a code base > where most callers of `write_message()` do not check the sign of the > return value but a couple do (there appears to be one that checks the > sign already and a couple that completely ignore the return value). If > we want to standardize on always checking the sign of the return value > of functions when checking for errors even when they never return a > positive value then I think someone in favor of that change should > propose a patch to the coding guidelines so it is clear what our policy > is. When I see a '< 0`' check I tend to think the positive value has a > non-error meaning. > Okay, I looked into the write_message(...) and agree that it does not return a positive value and only returns non-zero for error case and zero for success. So, for this patch maybe we can ignore checking '< 0' here and later add another patch to make this function follow the convention of "negative is error". Thanks and Regards, Charvi > Best Wishes > > Phillip > > > Okay, I got this and will change it. > > > > Thanks and Regards, > > Charvi > > > >> Thanks, > >> Taylor > >> > >> [1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/ >
Charvi Mendiratta <charvi077@gmail.com> writes: > Okay, I looked into the write_message(...) and agree that it does not return > a positive value and only returns non-zero for error case and zero for > success. So, for this patch maybe we can ignore checking '< 0' here and > later add another patch to make this function follow the convention of > "negative is error". Please don't. There is a higher cognitive cost to readers when you write if (write_message(...)) { The reader is forced to look at its implementation to see if it returns positive in a non-error situation. If you write it like so from the beginning if (write_message(...) < 0) { the reader can trust that the code follows "negative is an error" convention. One fewer thing readers have to worry about.
Hi Junio, On Fri, 15 Jan 2021 at 22:52, Junio C Hamano <gitster@pobox.com> wrote: > > Charvi Mendiratta <charvi077@gmail.com> writes: > > > Okay, I looked into the write_message(...) and agree that it does not return > > a positive value and only returns non-zero for error case and zero for > > success. So, for this patch maybe we can ignore checking '< 0' here and > > later add another patch to make this function follow the convention of > > "negative is error". > > Please don't. There is a higher cognitive cost to readers when you write > > if (write_message(...)) { > > The reader is forced to look at its implementation to see if it > returns positive in a non-error situation. > > If you write it like so from the beginning > > if (write_message(...) < 0) { > > the reader can trust that the code follows "negative is an error" > convention. One fewer thing readers have to worry about. I agree, earlier I was confused as there were many instances of write_message() without '< 0' in sequencer.c but considering the above I completely agree to follow the convention. Thanks and Regards, Charvi
diff --git a/sequencer.c b/sequencer.c index 8909a46770..f888a7ed3b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r, return error(_("could not read HEAD's commit message")); find_commit_subject(head_message, &body); - if (write_message(body, strlen(body), - rebase_path_fixup_msg(), 0)) { - unuse_commit_buffer(head_commit, head_message); - return error(_("cannot write '%s'"), - rebase_path_fixup_msg()); + if (command == TODO_FIXUP) { + if (write_message(body, strlen(body), + rebase_path_fixup_msg(), 0)) { + unuse_commit_buffer(head_commit, head_message); + return error(_("cannot write '%s'"), + rebase_path_fixup_msg()); + } } strbuf_addf(&buf, "%c ", comment_line_char);