Message ID | pull.531.git.1579268705473.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | commit: replace rebase/sequence booleans with single pick_state enum | expand |
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 > 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. > - 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. > - 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
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@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 > > > 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. > > > - 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. > > 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
Hi Ben On 17/01/2020 13:45, Ben Curtis via GitGitGadget wrote: > From: Ben Curtis <nospam@nowsci.com> > > In 116a408, That commit is no longer in pu, it has been replaced by 430b75f720 ("commit: give correct advice for empty commit during a rebase", 2019-12-06). There is now a preparatory commit 8d57f75749 ("commit: use enum value for multiple cherry-picks", 2019-12-06) which replaces the booleans with an enum. I need to reroll the series (pw/advise-rebase-skip) that contains them, if you've got any comments please let me know. Best Wishes Phillip the boolean `rebase_in_progress` was introduced by dscho to > handle instances when cherry-pick and rebase were occuring at the same time. > 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/`, > it was decided that an `enum` that defines the state of git would be a > more effective path forward. > > Tasks completed: > - 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; > } > else > whence = FROM_COMMIT; > @@ -459,7 +465,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix > if (whence == FROM_MERGE) > die(_("cannot do a partial commit during a merge.")); > else if (whence == FROM_CHERRY_PICK) { > - if (rebase_in_progress && !sequencer_in_use) > + if (pick_state == REBASE) > die(_("cannot do a partial commit during a rebase.")); > die(_("cannot do a partial commit during a cherry-pick.")); > } > @@ -958,9 +964,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > fputs(_(empty_amend_advice), stderr); > else if (whence == FROM_CHERRY_PICK) { > fputs(_(empty_cherry_pick_advice), stderr); > - 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); > @@ -1167,7 +1173,7 @@ static int parse_and_validate_options(int argc, const char *argv[], > if (whence == FROM_MERGE) > die(_("You are in the middle of a merge -- cannot amend.")); > else if (whence == FROM_CHERRY_PICK) { > - if (rebase_in_progress && !sequencer_in_use) > + if (pick_state == REBASE) > die(_("You are in the middle of a rebase -- cannot amend.")); > die(_("You are in the middle of a cherry-pick -- cannot amend.")); > } > @@ -1643,7 +1649,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > if (!reflog_msg) > reflog_msg = (whence != FROM_CHERRY_PICK) > ? "commit" > - : rebase_in_progress && !sequencer_in_use > + : pick_state == REBASE > ? "commit (rebase)" > : "commit (cherry-pick)"; > commit_list_insert(current_head, &parents); > > base-commit: 116a408b6ffcb2496ebf10dfce1364a42e8f0b32 >
On Fri, 2020-01-17 at 20:01 +0000, Phillip Wood wrote: > Hi Ben > > On 17/01/2020 13:45, Ben Curtis via GitGitGadget wrote: > > From: Ben Curtis <nospam@nowsci.com> > > > > In 116a408, > > That commit is no longer in pu, it has been replaced by 430b75f720 > ("commit: give correct advice for empty commit during a rebase", > 2019-12-06). There is now a preparatory commit 8d57f75749 ("commit: > use > enum value for multiple cherry-picks", 2019-12-06) which replaces > the > booleans with an enum. I need to reroll the series > (pw/advise-rebase-skip) that contains them, if you've got any > comments > please let me know. > > Best Wishes > > Phillip > Hi Phillip, Thank you for the feedback, I assume that means my patch is no longer required? Also, is there a formal issue assignment method with `git`? I hopped on this particular issue on GitGitGadget to get my feet wet here but was not sure if there was a separate maintained list to track overlap like the above. Thanks! Ben
Hi Ben [Cc'ing dscho as it relates to issue management on gitgitgadget] On 18/01/2020 16:34, Ben Curtis wrote: > On Fri, 2020-01-17 at 20:01 +0000, Phillip Wood wrote: >> Hi Ben >> >> On 17/01/2020 13:45, Ben Curtis via GitGitGadget wrote: >>> From: Ben Curtis <nospam@nowsci.com> >>> >>> In 116a408, >> >> That commit is no longer in pu, it has been replaced by 430b75f720 >> ("commit: give correct advice for empty commit during a rebase", >> 2019-12-06). There is now a preparatory commit 8d57f75749 ("commit: >> use >> enum value for multiple cherry-picks", 2019-12-06) which replaces >> the >> booleans with an enum. I need to reroll the series >> (pw/advise-rebase-skip) that contains them, if you've got any >> comments >> please let me know. >> >> Best Wishes >> >> Phillip >> > > Hi Phillip, > > Thank you for the feedback, I assume that means my patch is no longer > required? Unfortunately yes > Also, is there a formal issue assignment method with `git`? I hopped on > this particular issue on GitGitGadget to get my feet wet here but was > not sure if there was a separate maintained list to track overlap like > the above. Unfortunately there is no formal issue management. There is the mailing list which is where patches are picked up but it does not provide any issue management. In practice when an issue is reported on the list there is either a fix posted relatively quickly or someone notes it down somewhere and may work on it later. There is a convention of adding #leftoverbits to an email in the hope that someone will search for that and find things but I've never seen someone reference that when submitting a new patch and if the fix comes in a different email thread then there's no way to see that the issue has been fixed. There is gitgitgadet's list of issues but not everyone uses it (and it's only triaged by a small subset of people so there's no guarantee that a feature requested there will be accepted once it gets submitted to the mailing list). If someone posts directly to the mailing list then they probably wont see that there is an issue open there. Further confusion is provided by https://bugs.chromium.org/p/git/issues/list which has a different list of issues. The best thing would be to check the history of the 'pu' branch before starting work on an issue to see if it has already been fixed. I'm really sorry that you've had a bad experience, the idea of the gitgitgadget issue tracker is to make it easier for new contributors, not to waste their time. I hope it wont put you off making another contribution. Best Wishes Phillip > Thanks! > Ben >
Hi Phillip,
On Mon, 20 Jan 2020, Phillip Wood wrote:
> [Cc'ing dscho as it relates to issue management on gitgitgadget]
Duplicating efforts is sadly a thing we cannot prevent, not even should we
agree on a single issue tracker (we won't).
Ciao,
Dscho
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; } else whence = FROM_COMMIT; @@ -459,7 +465,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (whence == FROM_MERGE) die(_("cannot do a partial commit during a merge.")); else if (whence == FROM_CHERRY_PICK) { - if (rebase_in_progress && !sequencer_in_use) + if (pick_state == REBASE) die(_("cannot do a partial commit during a rebase.")); die(_("cannot do a partial commit during a cherry-pick.")); } @@ -958,9 +964,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix, fputs(_(empty_amend_advice), stderr); else if (whence == FROM_CHERRY_PICK) { fputs(_(empty_cherry_pick_advice), stderr); - 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); @@ -1167,7 +1173,7 @@ static int parse_and_validate_options(int argc, const char *argv[], if (whence == FROM_MERGE) die(_("You are in the middle of a merge -- cannot amend.")); else if (whence == FROM_CHERRY_PICK) { - if (rebase_in_progress && !sequencer_in_use) + if (pick_state == REBASE) die(_("You are in the middle of a rebase -- cannot amend.")); die(_("You are in the middle of a cherry-pick -- cannot amend.")); } @@ -1643,7 +1649,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!reflog_msg) reflog_msg = (whence != FROM_CHERRY_PICK) ? "commit" - : rebase_in_progress && !sequencer_in_use + : pick_state == REBASE ? "commit (rebase)" : "commit (cherry-pick)"; commit_list_insert(current_head, &parents);