Re: [PATCH v3 16/25] sequencer: support amending commits
Johannes Schindelin writes:
> On Mon, 17 Oct 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin writes:
>>
>> > This teaches the run_git_commit() function to take an argument that will
>> > allow us to implement "todo" commands that need to amend the commit
>> > messages ("fixup", "squash" and "reword").
>>
>> Likewise to 15/25, i.e. Good, though the growth by these two steps
>> starts to make me wonder if these three options should be crammed
>> into an unsigned "flags" bitword.
>
> After looking at the diff with the added complications of ORing and ANDing
> the flags, I'd much rather prefer to stay with the three flags being kept
> separately. It's not like we need to save bits, but we need to preserve
> readability as much as possible, I'd wager.
That's OK. I just wanted to make sure pros-and-cons have been
already considered.
The primary merit of using flags bitword is not to save bits; it is
done to limit the damage to the codebase when we need to add yet
another knob, by the way.
Re: [PATCH v3 16/25] sequencer: support amending commits
Hi Junio,
On Mon, 17 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > This teaches the run_git_commit() function to take an argument that will
> > allow us to implement "todo" commands that need to amend the commit
> > messages ("fixup", "squash" and "reword").
>
> Likewise to 15/25, i.e. Good, though the growth by these two steps
> starts to make me wonder if these three options should be crammed
> into an unsigned "flags" bitword.
After looking at the diff with the added complications of ORing and ANDing
the flags, I'd much rather prefer to stay with the three flags being kept
separately. It's not like we need to save bits, but we need to preserve
readability as much as possible, I'd wager.
> I see you have v4, so I'll ignore the remainder of this stale round
> and start reading that updated one instead.
Thanks,
Dscho
Re: [PATCH v3 16/25] sequencer: support amending commits
Johannes Schindelin writes:
> This teaches the run_git_commit() function to take an argument that will
> allow us to implement "todo" commands that need to amend the commit
> messages ("fixup", "squash" and "reword").
Likewise to 15/25, i.e. Good, though the growth by these two steps
starts to make me wonder if these three options should be crammed
into an unsigned "flags" bitword.
I see you have v4, so I'll ignore the remainder of this stale round
and start reading that updated one instead.
>
> Signed-off-by: Johannes Schindelin
> ---
> sequencer.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b621f4b..403a4f0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -481,7 +481,7 @@ static char **read_author_script(void)
> * author metadata.
> */
> static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> - int allow_empty, int edit)
> + int allow_empty, int edit, int amend)
> {
> char **env = NULL;
> struct argv_array array;
> @@ -510,6 +510,8 @@ static int run_git_commit(const char *defmsg, struct
> replay_opts *opts,
> argv_array_push(&array, "commit");
> argv_array_push(&array, "-n");
>
> + if (amend)
> + argv_array_push(&array, "--amend");
> if (opts->gpg_sign)
> argv_array_pushf(&array, "-S%s", opts->gpg_sign);
> if (opts->signoff)
> @@ -785,7 +787,7 @@ static int do_pick_commit(enum todo_command command,
> struct commit *commit,
> }
> if (!opts->no_commit)
> res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> - opts, allow, opts->edit);
> + opts, allow, opts->edit, 0);
>
> leave:
> free_message(commit, &msg);
