| Message ID | 20190613040504.32317-2-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Teach cherry-pick/revert to skip commits | expand |
On 13/06/2019 05:05, Rohit Ashiwal wrote: > In the case of merge conflicts, while performing a revert, we are > currently advised to use `git cherry-pick --<sequencer-options>` > of which --continue is incompatible for continuing the revert. > Introduce a separate advice message for `git revert`. Also change > the signature of `create_seq_dir` to handle which advice to display > selectively. > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > changes: > - Following Junio's advice[1], I've changed how we're handling error and > advice fundamentally. > > [1]: https://public-inbox.org/git/20190608191958.4593-1-rohit.ashiwal265@gmail.com/T/#mf8eefb068b7246c7c2a02cc8f7120a1a903a1eba > > sequencer.c | 34 ++++++++++++++++++++++++++++------ > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index f88a97fb10..918cb5d761 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2650,15 +2650,37 @@ static int walk_revs_populate_todo(struct todo_list *todo_list, > return 0; > } > > -static int create_seq_dir(void) > +static int create_seq_dir(struct repository *r) > { > - 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)\"")); > + enum replay_action action; > + const char *in_progress_advice; > + const char *in_progress_error = NULL; > + > + if (!sequencer_get_last_command(r, &action)) { > + switch (action) { > + case REPLAY_REVERT: > + in_progress_error = _("revert is already in progress"); > + in_progress_advice = > + _("try \"git revert (--continue | --abort | --quit)\""); > + break; > + case REPLAY_PICK: > + in_progress_error = _("cherry-pick is already in progress"); > + in_progress_advice = > + _("try \"git cherry-pick (--continue | --abort | --quit)\""); > + break; > + default: > + BUG(_("the control must not reach here")); This does not need to be translated as BUG() messages are not really for users. Everything else looks fine to be now Best Wishes Phillip > + } > + } > + if (in_progress_error) { > + error("%s", in_progress_error); > + advise("%s", in_progress_advice); > return -1; > - } else if (mkdir(git_path_seq_dir(), 0777) < 0) > + } > + if (mkdir(git_path_seq_dir(), 0777) < 0) > return error_errno(_("could not create sequencer directory '%s'"), > git_path_seq_dir()); > + > return 0; > } > > @@ -4237,7 +4259,7 @@ int sequencer_pick_revisions(struct repository *r, > */ > > if (walk_revs_populate_todo(&todo_list, opts) || > - create_seq_dir() < 0) > + create_seq_dir(r) < 0) > return -1; > if (get_oid("HEAD", &oid) && (opts->action == REPLAY_REVERT)) > return error(_("can't revert as initial commit")); >
Hi Rohit, On Thu, 13 Jun 2019 at 19:46, Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 13/06/2019 05:05, Rohit Ashiwal wrote: > > -static int create_seq_dir(void) > > +static int create_seq_dir(struct repository *r) > > { > > - 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)\"")); > > + enum replay_action action; > > + const char *in_progress_advice; > > + const char *in_progress_error = NULL; The assigning vs not assigning is a bit inconsistent, but that's a very minor nit, and not why I started replying. Only noticed it just now. :-) > > + if (!sequencer_get_last_command(r, &action)) { > > + switch (action) { > > + case REPLAY_REVERT: > > + in_progress_error = _("revert is already in progress"); > > + in_progress_advice = > > + _("try \"git revert (--continue | --abort | --quit)\""); > > + break; > > + case REPLAY_PICK: > > + in_progress_error = _("cherry-pick is already in progress"); > > + in_progress_advice = > > + _("try \"git cherry-pick (--continue | --abort | --quit)\""); > > + break; > > + default: > > + BUG(_("the control must not reach here")); > > This does not need to be translated as BUG() messages are not really for > users. Everything else looks fine to be now I agree 100% with Phillip, but I'll also note that "the control must not reach here" doesn't tell me anything that BUG() doesn't already. That is, the point of BUG() is to document that, indeed, we shouldn't get here and to alert if we do anyway. An obvious alternative would be BUG("action is neither revert nor pick"); but that doesn't say much more than the code already says quite clearly, plus it risks getting outdated. I'd probably settle on something like BUG("unexpected action in create_seq_dir"); which should give us a good clue even if all we have is this message (so no file, no line number), but I am sure there are other good choices here. :-) Thanks Rohit for your work on this. I'm impressed by how you've polished this series. Martin
Martin Ågren <martin.agren@gmail.com> writes: > ... > I agree 100% with Phillip, but I'll also note that "the control must not > reach here" doesn't tell me anything that BUG() doesn't already. That > is,... Thanks, all of you involved in this topic, for excellent mentorship.
Hi Phillip On 2019-06-13 17:45 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: > >> + break; >> + default: >> + BUG(_("the control must not reach here")); > > This does not need to be translated as BUG() messages are not really for > users. Everything else looks fine to be now I know this is not intended for the users, but seeing a message like this a user might think to report it to the team with the steps for reproducing the BUG. Yes, I agree this needs a little bit of re-wording. > Best Wishes > > Phillip Thanks Rohit
Hi Martin On 2019-06-13 19:21 UTC Martin Ågren <martin.agren@gmail.com> wrote: > > > > + const char *in_progress_advice; > > > + const char *in_progress_error = NULL; > > The assigning vs not assigning is a bit inconsistent, but that's a very > minor nit, and not why I started replying. Only noticed it just now. :-) Let's make both NULL then. > I agree 100% with Phillip, but I'll also note that "the control must not > reach here" doesn't tell me anything that BUG() doesn't already. That > is, the point of BUG() is to document that, indeed, we shouldn't get > here and to alert if we do anyway. Agreed. > An obvious alternative would be > > BUG("action is neither revert nor pick"); > > but that doesn't say much more than the code already says quite clearly, > plus it risks getting outdated. I'd probably settle on something like > > BUG("unexpected action in create_seq_dir"); Yeah, now that I know more about what BUG is truly intended for, I think you are right here. We should change it. > which should give us a good clue even if all we have is this message (so > no file, no line number), but I am sure there are other good choices > here. :-) > > Thanks Rohit for your work on this. I'm impressed by how you've polished > this series. Thank you very much Martin. Appreciations like these help us developers keep going and working even harder. Thanks Rohit
diff --git a/sequencer.c b/sequencer.c index f88a97fb10..918cb5d761 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2650,15 +2650,37 @@ static int walk_revs_populate_todo(struct todo_list *todo_list, return 0; } -static int create_seq_dir(void) +static int create_seq_dir(struct repository *r) { - 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)\"")); + enum replay_action action; + const char *in_progress_advice; + const char *in_progress_error = NULL; + + if (!sequencer_get_last_command(r, &action)) { + switch (action) { + case REPLAY_REVERT: + in_progress_error = _("revert is already in progress"); + in_progress_advice = + _("try \"git revert (--continue | --abort | --quit)\""); + break; + case REPLAY_PICK: + in_progress_error = _("cherry-pick is already in progress"); + in_progress_advice = + _("try \"git cherry-pick (--continue | --abort | --quit)\""); + break; + default: + BUG(_("the control must not reach here")); + } + } + if (in_progress_error) { + error("%s", in_progress_error); + advise("%s", in_progress_advice); return -1; - } else if (mkdir(git_path_seq_dir(), 0777) < 0) + } + if (mkdir(git_path_seq_dir(), 0777) < 0) return error_errno(_("could not create sequencer directory '%s'"), git_path_seq_dir()); + return 0; } @@ -4237,7 +4259,7 @@ int sequencer_pick_revisions(struct repository *r, */ if (walk_revs_populate_todo(&todo_list, opts) || - create_seq_dir() < 0) + create_seq_dir(r) < 0) return -1; if (get_oid("HEAD", &oid) && (opts->action == REPLAY_REVERT)) return error(_("can't revert as initial commit"));
In the case of merge conflicts, while performing a revert, we are currently advised to use `git cherry-pick --<sequencer-options>` of which --continue is incompatible for continuing the revert. Introduce a separate advice message for `git revert`. Also change the signature of `create_seq_dir` to handle which advice to display selectively. Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- changes: - Following Junio's advice[1], I've changed how we're handling error and advice fundamentally. [1]: https://public-inbox.org/git/20190608191958.4593-1-rohit.ashiwal265@gmail.com/T/#mf8eefb068b7246c7c2a02cc8f7120a1a903a1eba sequencer.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)