git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
@ 2020-01-17 13:45 Ben Curtis via GitGitGadget
  2020-01-17 14:29 ` Derrick Stolee
  2020-01-17 20:01 ` Phillip Wood
  0 siblings, 2 replies; 7+ messages in thread
From: Ben Curtis via GitGitGadget @ 2020-01-17 13:45 UTC (permalink / raw)
  To: git; +Cc: Ben Curtis, Ben Curtis

From: Ben Curtis <nospam@nowsci.com>

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/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
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
  2020-01-17 13:45 [PATCH] commit: replace rebase/sequence booleans with single pick_state enum Ben Curtis via GitGitGadget
@ 2020-01-17 14:29 ` Derrick Stolee
  2020-01-17 15:46   ` Ben Curtis
  2020-01-17 20:01 ` Phillip Wood
  1 sibling, 1 reply; 7+ messages in thread
From: Derrick Stolee @ 2020-01-17 14:29 UTC (permalink / raw)
  To: Ben Curtis via GitGitGadget, git; +Cc: Ben Curtis

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
  2020-01-17 14:29 ` Derrick Stolee
@ 2020-01-17 15:46   ` Ben Curtis
  0 siblings, 0 replies; 7+ messages in thread
From: Ben Curtis @ 2020-01-17 15:46 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee, Ben Curtis via GitGitGadget

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
  2020-01-17 13:45 [PATCH] commit: replace rebase/sequence booleans with single pick_state enum Ben Curtis via GitGitGadget
  2020-01-17 14:29 ` Derrick Stolee
@ 2020-01-17 20:01 ` Phillip Wood
  2020-01-18 16:34   ` Ben Curtis
  1 sibling, 1 reply; 7+ messages in thread
From: Phillip Wood @ 2020-01-17 20:01 UTC (permalink / raw)
  To: Ben Curtis via GitGitGadget, git; +Cc: Ben Curtis, Derrick Stolee

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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
  2020-01-17 20:01 ` Phillip Wood
@ 2020-01-18 16:34   ` Ben Curtis
  2020-01-20 17:09     ` Phillip Wood
  0 siblings, 1 reply; 7+ messages in thread
From: Ben Curtis @ 2020-01-18 16:34 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee, phillip.wood, Ben Curtis via GitGitGadget

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
  2020-01-18 16:34   ` Ben Curtis
@ 2020-01-20 17:09     ` Phillip Wood
  2020-01-20 21:11       ` Johannes Schindelin
  0 siblings, 1 reply; 7+ messages in thread
From: Phillip Wood @ 2020-01-20 17:09 UTC (permalink / raw)
  To: Ben Curtis, git
  Cc: Derrick Stolee, phillip.wood, Ben Curtis via GitGitGadget,
	Johannes Schindelin

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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] commit: replace rebase/sequence booleans with single pick_state enum
  2020-01-20 17:09     ` Phillip Wood
@ 2020-01-20 21:11       ` Johannes Schindelin
  0 siblings, 0 replies; 7+ messages in thread
From: Johannes Schindelin @ 2020-01-20 21:11 UTC (permalink / raw)
  To: phillip.wood; +Cc: Ben Curtis, git, Derrick Stolee, Ben Curtis via GitGitGadget

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-01-20 21:12 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-17 13:45 [PATCH] commit: replace rebase/sequence booleans with single pick_state enum Ben Curtis via GitGitGadget
2020-01-17 14:29 ` Derrick Stolee
2020-01-17 15:46   ` Ben Curtis
2020-01-17 20:01 ` Phillip Wood
2020-01-18 16:34   ` Ben Curtis
2020-01-20 17:09     ` Phillip Wood
2020-01-20 21:11       ` Johannes Schindelin

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).