Hi Taylor and Charvi
On 14/01/2021 08:12, Charvi Mendiratta wrote:
On Thu, 14 Jan 2021 at 00:13, Taylor Blau <me@xxxxxxxxxxxx> wrote:
On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Signed-off-by: Charvi Mendiratta <charvi077@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxxx/
