commit: replace rebase/sequence booleans with single pick_state enum

PATCH In review Ben Curtis via GitGitGadget 5 participants 7 messages Jan 17, 2020 – Jan 20, 2020

lore

Share & tools

lore

Ben Curtis via GitGitGadget

author

2020-01-17 13:45 UTC #0 lore raw

PATCH 1/1 commit: replace rebase/sequence booleans with single pick_state enum

From: Ben Curtis <[email protected]>

In 116a408, 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/[email protected]/, 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 <[email protected]>

---
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(-)

builtin/commit.c +15 -9

index 2beae13620..84f7e69cb1 100644
@@ -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

Signature · 1 line

gitgitgadget

Derrick Stolee

2020-01-17 14:29 UTC #1 lore raw

Quoted 3 lines · Ben Curtis via GitGitGadget

From: Ben Curtis <[email protected]>
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), ...

Quoted 1 line

handle instances when cherry-pick and rebase were occuring at the same time.

s/occuring/occurring

Quoted 5 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/[email protected]/,

Use lore.kernel.org and use "[1]" like a citation.

[1] https://lore.kernel.org/git/[email protected]/

Quoted 4 lines

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.

Quoted 43 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 <[email protected]>
---
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(-)
builtin/commit.c +5 -1
index 2beae13620..84f7e69cb1 100644
@@ -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))) {

builtin/commit.c static void determine_whence(struct wt_status *s) · line 196

Quoted 10 lines

builtin/commit.c +5 -3

 		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.)

builtin/commit.c static void determine_whence(struct wt_status *s) · line 197

Quoted 2 lines

builtin/commit.c +1 -1

-			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.

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 972

Quoted 8 lines

builtin/commit.c +2 -2

-			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

Ben Curtis

2020-01-17 15:55 UTC #2 lore raw

Quoted 34 lines · Derrick Stolee

Quoted 4 lines · Ben Curtis via GitGitGadget
From: Ben Curtis <[email protected]>
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), ...
Quoted 2 lines
handle instances when cherry-pick and rebase were occuring at the same time.
s/occuring/occurring
Quoted 9 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/[email protected]/`
,
Use lore.kernel.org and use "[1]" like a citation.
[1]
https://lore.kernel.org/git/[email protected]/
Quoted 4 lines
it was decided that an enum that defines the state of git would be a
more effective path forward.

Quoted 31 lines

Quoted 1 line
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.
Quoted 22 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 <[email protected]>
---
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.

Quoted 41 lines

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(-)

builtin/commit.c +5 -1

index 2beae13620..84f7e69cb1 100644
@@ -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;

builtin/commit.c +4 -3

@@ -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

Quoted 9 lines

builtin/commit.c static void determine_whence(struct wt_status *s) · line 196
Quoted 1 line
builtin/commit.c +1 -0
+			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.

Quoted 9 lines

builtin/commit.c static void determine_whence(struct wt_status · line 199
Quoted 2 lines
builtin/commit.c +1 -1
-			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.

Quoted 14 lines

Quoted 10 lines
builtin/commit.c +1 -1
-			if (sequencer_in_use)
+			if (pick_state == MULTI_PICK)
 				fputs(_(empty_cherry_pick_advice_multi)
, stderr);
builtin/commit.c +1 -1
-			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.

Quoted 3 lines

Thanks,
-Stolee

Thanks!
Ben

Phillip Wood

2020-01-17 20:01 UTC #3 lore raw

In reply to Ben Curtis via GitGitGadget

Hi Ben

Quoted 3 lines · Ben Curtis via GitGitGadget

From: Ben Curtis <[email protected]>
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

Quoted 108 lines

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.

Signed-off-by Ben Curtis <[email protected]>

---
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.

  builtin/commit.c | 24 +++++++++++++++---------
  1 file changed, 15 insertions(+), 9 deletions(-)

builtin/commit.c +15 -9

index 2beae13620..84f7e69cb1 100644
@@ -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

Ben Curtis

2020-01-18 16:34 UTC #4 lore raw

Quoted 18 lines · Phillip Wood

Hi Ben
Quoted 3 lines · Ben Curtis via GitGitGadget
From: Ben Curtis <[email protected]>
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.

Quoted 4 lines

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

Phillip Wood

2020-01-20 17:09 UTC #5 lore raw

Hi Ben

[Cc'ing dscho as it relates to issue management on gitgitgadget]

Quoted 21 lines · Ben Curtis

Quoted 20 lines · Phillip Wood
Hi Ben
Quoted 3 lines · Ben Curtis via GitGitGadget
From: Ben Curtis <[email protected]>
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

Quoted 7 lines

Quoted 2 lines
Phillip
Hi Phillip,
Thank you for the feedback, I assume that means my patch is no longer required?

Unfortunately yes

Quoted 4 lines

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

Quoted 3 lines

Thanks!
Ben

Johannes Schindelin

2020-01-20 21:12 UTC #6 lore raw

Hi Phillip,

Quoted 1 line · Phillip Wood

[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