| Message ID | 20190608191958.4593-4-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Teach cherry-pick/revert to skip commits | expand |
> Subject: [GSoC][PATCH 3/3] cherry-pick/revert: update hints "update hints" is very generic when read in 'git log --oneline', and doesn't give the reader much useful information. Maybe something like cherry-pick/revert: advice using --skip In the previous commit we introduced a --skip flag for cherry-pick and revert. Update the advice messages, to tell users about this less cumbersome way of skipping commits would work better? The rest of the patch looks good to me. On 06/09, Rohit Ashiwal wrote: > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > builtin/commit.c | 13 ++++++++----- > sequencer.c | 4 ++-- > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 1c9e8e2228..1f47c51bdc 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -60,15 +60,18 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\ > "\n"); > > static const char empty_cherry_pick_advice_single[] = > -N_("Otherwise, please use 'git reset'\n"); > +N_("Otherwise, please use 'git cherry-pick --skip'\n"); > > static const char empty_cherry_pick_advice_multi[] = > -N_("If you wish to skip this commit, use:\n" > +N_("and then use:\n" > "\n" > -" git reset\n" > +" git cherry-pick --continue\n" > "\n" > -"Then \"git cherry-pick --continue\" will resume cherry-picking\n" > -"the remaining commits.\n"); > +"to resume cherry-picking the remaining commits.\n" > +"If you wish to skip this commit, use:\n" > +"\n" > +" git cherry-pick --skip\n" > +"\n"); > > static const char *color_status_slots[] = { > [WT_STATUS_HEADER] = "header", > diff --git a/sequencer.c b/sequencer.c > index f586e677d3..e889427eef 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2654,8 +2654,8 @@ static int create_seq_dir(void) > { > if (file_exists(git_path_seq_dir())) { > error(_("a cherry-pick or revert is already in progress")); > - advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); > - advise(_("or \"git revert (--continue | --quit | --abort)\"")); > + advise(_("try \"git cherry-pick (--continue | --skip | --quit | --abort)\"")); > + advise(_("or \"git revert (--continue | --skip | --quit | --abort)\"")); > return -1; > } else if (mkdir(git_path_seq_dir(), 0777) < 0) > return error_errno(_("could not create sequencer directory '%s'"), > -- > 2.21.0 >
Hi Rohit On 08/06/2019 20:19, Rohit Ashiwal wrote: > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > builtin/commit.c | 13 ++++++++----- > sequencer.c | 4 ++-- > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 1c9e8e2228..1f47c51bdc 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -60,15 +60,18 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\ > "\n"); > > static const char empty_cherry_pick_advice_single[] = > -N_("Otherwise, please use 'git reset'\n"); > +N_("Otherwise, please use 'git cherry-pick --skip'\n"); > > static const char empty_cherry_pick_advice_multi[] = > -N_("If you wish to skip this commit, use:\n" > +N_("and then use:\n" > "\n" > -" git reset\n" > +" git cherry-pick --continue\n" > "\n" > -"Then \"git cherry-pick --continue\" will resume cherry-picking\n" > -"the remaining commits.\n"); > +"to resume cherry-picking the remaining commits.\n" > +"If you wish to skip this commit, use:\n" > +"\n" > +" git cherry-pick --skip\n" > +"\n"); > > static const char *color_status_slots[] = { > [WT_STATUS_HEADER] = "header", > diff --git a/sequencer.c b/sequencer.c > index f586e677d3..e889427eef 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2654,8 +2654,8 @@ static int create_seq_dir(void) > { > if (file_exists(git_path_seq_dir())) { > error(_("a cherry-pick or revert is already in progress")); > - advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); > - advise(_("or \"git revert (--continue | --quit | --abort)\"")); > + advise(_("try \"git cherry-pick (--continue | --skip | --quit | --abort)\"")); > + advise(_("or \"git revert (--continue | --skip | --quit | --abort)\"")); If the user has already committed the conflict resolution then we don't want to recommend --skip as there is nothing to skip. Best Wishes Phillip > return -1; > } else if (mkdir(git_path_seq_dir(), 0777) < 0) > return error_errno(_("could not create sequencer directory '%s'"), >
Hey Phillip On Sun, 9 Jun 2019 19:03:02 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Rohit > > On 08/06/2019 20:19, Rohit Ashiwal wrote: > > [...] > > @@ -2654,8 +2654,8 @@ static int create_seq_dir(void) > > { > > if (file_exists(git_path_seq_dir())) { > > error(_("a cherry-pick or revert is already in progress")); > > - advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); > > - advise(_("or \"git revert (--continue | --quit | --abort)\"")); > > + advise(_("try \"git cherry-pick (--continue | --skip | --quit | --abort)\"")); > > + advise(_("or \"git revert (--continue | --skip | --quit | --abort)\"")); > > If the user has already committed the conflict resolution then we don't > want to recommend --skip as there is nothing to skip. I think it is more about suggesting what are all the possibilities you can try and not about intelligently suggesting what you should do. ofc, we can not use `revert --<option>` while cherry-picking.( we should not be able to do so in ideal conditions, but the world does not work as we think it should). Still we are suggesting so here. Also, I think it is more reasonable to make "this" a part of patch which will cover "tailored" advice messages which is also a topic of discussion as I described here[1]. > Best Wishes > > Phillip Thanks Rohit [1]: https://public-inbox.org/git/20190609200038.GD28007@hank.intra.tgummerer.com/T/#mbb071f6e29c69f291ecd9c61c71b889774ff33b2
Hi Rohit On 10/06/2019 06:28, Rohit Ashiwal wrote: > Hey Phillip > > On Sun, 9 Jun 2019 19:03:02 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> Hi Rohit >> >> On 08/06/2019 20:19, Rohit Ashiwal wrote: >>> [...] >>> @@ -2654,8 +2654,8 @@ static int create_seq_dir(void) >>> { >>> if (file_exists(git_path_seq_dir())) { >>> error(_("a cherry-pick or revert is already in progress")); >>> - advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); >>> - advise(_("or \"git revert (--continue | --quit | --abort)\"")); >>> + advise(_("try \"git cherry-pick (--continue | --skip | --quit | --abort)\"")); >>> + advise(_("or \"git revert (--continue | --skip | --quit | --abort)\"")); >> >> If the user has already committed the conflict resolution then we don't >> want to recommend --skip as there is nothing to skip. > I think it is more about suggesting what are all the possibilities > you can try and not about intelligently suggesting what you should > do. Previously all the suggested options were viable, --skip is not applicable if the user has committed a conflict resolution. The idea of the advice is to help the user, suggesting options that wont work is not going to help them. > ofc, we can not use `revert --<option>` while cherry-picking.( As I suggested in patch 1 we should tailor the error message to the command. > we should not be able to do so in ideal conditions, but the world > does not work as we think it should). Still we are suggesting so > here. Yes because you have the power to easily make that change. It is normal to try and improve the code base when we make related changes. > Also, I think it is more reasonable to make "this" a part of patch > which will cover "tailored" advice messages which is also a topic > of discussion as I described here[1]. That might make sense, but it is a pretty self contained change as part of this patch. Best Wishes Phillip > >> Best Wishes >> >> Phillip > > Thanks > Rohit > > [1]: https://public-inbox.org/git/20190609200038.GD28007@hank.intra.tgummerer.com/T/#mbb071f6e29c69f291ecd9c61c71b889774ff33b2 >
Hi Phillip On 2019-06-10 10:40 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Rohit > > On 10/06/2019 06:28, Rohit Ashiwal wrote: >> Hey Phillip >> >> On Sun, 9 Jun 2019 19:03:02 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote: >>> >>> Hi Rohit >> [...] >> I think it is more about suggesting what are all the possibilities >> you can try and not about intelligently suggesting what you should >> do. > > Previously all the suggested options were viable, --skip is not > applicable if the user has committed a conflict resolution. The idea of > the advice is to help the user, suggesting options that wont work is not > going to help them. Now that I know what I should do, I'll make the change and submit a better patch. >> ofc, we can not use `revert --<option>` while cherry-picking.( > > As I suggested in patch 1 we should tailor the error message to the command. Yes, I'll tailor the messages based on which command was ran. >> we should not be able to do so in ideal conditions, but the world >> does not work as we think it should). Still we are suggesting so >> here. > > Yes because you have the power to easily make that change. It is normal > to try and improve the code base when we make related changes. :) >> Also, I think it is more reasonable to make "this" a part of patch >> which will cover "tailored" advice messages which is also a topic >> of discussion as I described here[1]. > > That might make sense, but it is a pretty self contained change as part > of this patch. Yes, this patch is the place where all changes should be made. Thanks Rohit
Hi Rohit On 10/06/2019 14:33, Rohit Ashiwal wrote: > Hi Phillip > > On 2019-06-10 10:40 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> Hi Rohit >> >> On 10/06/2019 06:28, Rohit Ashiwal wrote: >>> Hey Phillip >>> >>> On Sun, 9 Jun 2019 19:03:02 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote: >>>> >>>> Hi Rohit >>> [...] >>> I think it is more about suggesting what are all the possibilities >>> you can try and not about intelligently suggesting what you should >>> do. >> >> Previously all the suggested options were viable, --skip is not >> applicable if the user has committed a conflict resolution. The idea of >> the advice is to help the user, suggesting options that wont work is not >> going to help them. > > Now that I know what I should do, I'll make the change and submit a > better patch. That's great, thanks Phillip >>> ofc, we can not use `revert --<option>` while cherry-picking.( >> >> As I suggested in patch 1 we should tailor the error message to the command. > > Yes, I'll tailor the messages based on which command was ran. > >>> we should not be able to do so in ideal conditions, but the world >>> does not work as we think it should). Still we are suggesting so >>> here. >> >> Yes because you have the power to easily make that change. It is normal >> to try and improve the code base when we make related changes. > > :) > >>> Also, I think it is more reasonable to make "this" a part of patch >>> which will cover "tailored" advice messages which is also a topic >>> of discussion as I described here[1]. >> >> That might make sense, but it is a pretty self contained change as part >> of this patch. > > Yes, this patch is the place where all changes should be made. > > Thanks > Rohit >
diff --git a/builtin/commit.c b/builtin/commit.c index 1c9e8e2228..1f47c51bdc 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -60,15 +60,18 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\ "\n"); static const char empty_cherry_pick_advice_single[] = -N_("Otherwise, please use 'git reset'\n"); +N_("Otherwise, please use 'git cherry-pick --skip'\n"); static const char empty_cherry_pick_advice_multi[] = -N_("If you wish to skip this commit, use:\n" +N_("and then use:\n" "\n" -" git reset\n" +" git cherry-pick --continue\n" "\n" -"Then \"git cherry-pick --continue\" will resume cherry-picking\n" -"the remaining commits.\n"); +"to resume cherry-picking the remaining commits.\n" +"If you wish to skip this commit, use:\n" +"\n" +" git cherry-pick --skip\n" +"\n"); static const char *color_status_slots[] = { [WT_STATUS_HEADER] = "header", diff --git a/sequencer.c b/sequencer.c index f586e677d3..e889427eef 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2654,8 +2654,8 @@ static int create_seq_dir(void) { if (file_exists(git_path_seq_dir())) { error(_("a cherry-pick or revert is already in progress")); - advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); - advise(_("or \"git revert (--continue | --quit | --abort)\"")); + advise(_("try \"git cherry-pick (--continue | --skip | --quit | --abort)\"")); + advise(_("or \"git revert (--continue | --skip | --quit | --abort)\"")); return -1; } else if (mkdir(git_path_seq_dir(), 0777) < 0) return error_errno(_("could not create sequencer directory '%s'"),
Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- builtin/commit.c | 13 ++++++++----- sequencer.c | 4 ++-- 2 files changed, 10 insertions(+), 7 deletions(-)