Re: [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jan 17, 2020, 14:29 UTC
- Message-ID
- <551af3c3-4633-3dba-4999-1c1a63e5703a@gmail.com>
- In-Reply-To
- <pull.531.git.1579268705473.gitgitgadget@gmail.com>
On 1/17/2020 8:45 AM, Ben Curtis via GitGitGadget wrote:
> From: Ben Curtis <nospam@nowsci.com> > > In 116a408, the boolean `rebase_in_progress` was introduced by dscho to
In 116a408 (commit: give correct advice for empty commit during a rebase, 2019-10-22), ...
> handle instances when cherry-pick and rebase were occuring at the same time.
s/occuring/occurring
Show 5 quoted lines
> This created a situation where two independent booleans were being used > to define the state of git at a point in time. > > Under his recommendation to follow guidance from Junio, specifically > `https://public-inbox.org/git/xmqqr234i2q0.fsf@gitster-ct.c.googlers.com/`,
Use lore.kernel.org and use "[1]" like a citation.
[1] https://lore.kernel.org/git/xmqqr234i2q0.fsf@gitster-ct.c.googlers.com/
> it was decided that an `enum` that defines the state of git would be a > more effective path forward. > > Tasks completed:
Everything in the message is about what you did and why. It's good that you prefaced the "what" with so much "why", but now you can just describe the "what" using paragraphs. The "Tasks completed:" line is superfluous.
Show 53 quoted lines
> - Remove multiple booleans `rebase_in_progress` and `sequencer_in_use` and
> replaced with a single `pick_state` enum that determines if, when
> cherry-picking, we are in a rebase, multi-pick, or single-pick state
> - Converted double `if` statement to `if/else if` prioritizing `REBASE` to
> continue to disallow cherry pick in rebase.>
>
> Signed-off-by: Ben Curtis <nospam@nowsci.com>
> ---
> commit: replaced rebase/sequence booleans with single pick_state enum
>
> Addresses https://github.com/gitgitgadget/git/issues/426
>
> Previous discussions from @dscho and Junio led to the decision to merge
> two independent booleans into a single enum to track the state of git
> during a cherry-pick leading to this PR/patch.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-531%2FFmstrat%2Fjs%2Fadvise-rebase-skip-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-531/Fmstrat/js/advise-rebase-skip-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/531
>
> builtin/commit.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 2beae13620..84f7e69cb1 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -125,7 +125,11 @@ static enum commit_msg_cleanup_mode cleanup_mode;
> static const char *cleanup_arg;
>
> static enum commit_whence whence;
> -static int sequencer_in_use, rebase_in_progress;
> +static enum {
> + SINGLE_PICK,
> + MULTI_PICK,
> + REBASE
> +} pick_state;
> static int use_editor = 1, include_status = 1;
> static int have_option_m;
> static struct strbuf message = STRBUF_INIT;
> @@ -184,10 +188,12 @@ static void determine_whence(struct wt_status *s)
> whence = FROM_MERGE;
> else if (file_exists(git_path_cherry_pick_head(the_repository))) {
> whence = FROM_CHERRY_PICK;
> - if (file_exists(git_path_seq_dir()))
> - sequencer_in_use = 1;
> if (file_exists(git_path_rebase_merge_dir()))
> - rebase_in_progress = 1;
> + pick_state = REBASE;
> + else if (file_exists(git_path_seq_dir()))
> + pick_state = MULTI_PICK;
> + else
> + pick_state = SINGLE_PICK;Since before the "if"s were not exclusive, would rebase_in_progress = 1 also include sequencer_in_use = 1? That would explain why you needed to rearrange the cases here. (Based on later checks, it seems that these cases are indeed independent.)
> - if (rebase_in_progress && !sequencer_in_use) > + if (pick_state == REBASE)
This old error condition makes it appear that you _could_ be in the state where rebase_in_progress = 1 and sequencer_in_use = 0, showing that one does not imply the other.
Show 8 quoted lines
> - if (sequencer_in_use) > + if (pick_state == MULTI_PICK) > fputs(_(empty_cherry_pick_advice_multi), stderr); > - else if (rebase_in_progress) > + else if (pick_state == REBASE) > fputs(_(empty_rebase_advice), stderr); > else > fputs(_(empty_cherry_pick_advice_single), stderr);
Since we are using an enum, should we rearrange these cases into a switch (pick_state)?
Thanks, -Stolee