Re: [GSoC][PATCH v3 1/3] sequencer: add advice for revert

["Martin Ågren" <martin.agren@gmail.com>]

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