On Fri, 2020-01-17 at 09:29 -0500, Derrick Stolee wrote: > On 1/17/2020 8:45 AM, Ben Curtis via GitGitGadget wrote: > > From: Ben Curtis <nospam@xxxxxxxxxx> > > > > 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 > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx/` > > , > > Use lore.kernel.org and use "[1]" like a citation. > > [1] > https://lore.kernel.org/git/xmqqr234i2q0.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/ > > > 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. > > > - 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@xxxxxxxxxx> > > --- > > 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. > > Sure thing! I will revise the commit as described. And thanks for the feedback, just diving into `git` development so this is my first time through and this is very helpful. > > 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.) > While the above two `if` statements were not exclusive, their use in the below `if` statements did appear to be (at first). The line right above the if statement just below this comment is: else if (whence == FROM_CHERRY_PICK) { Since we are always in a cherry pick state, and the new code prioritizes checking on a rebase first, I had thought this would work out. However given the below I can see how the single-pick state could still crop up. I will update the commit with REBASE_SINGLE and REBASE_MULTI states to eliminate that without adding redundancy. > > - 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. > > > - 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)? > Yes, that would be cleaner, I will shift to a switch. > Thanks, > -Stolee > Thanks! Ben
