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

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Date: Jun 14, 2019, 03:44 UTC
Message-ID: <20190614034457.19183-1-rohit.ashiwal265@gmail.com>
In-Reply-To: <CAN0heSp_XDZTviwGDpOuXhuh+8k-BA7Ld6z9gSHA9My+16BnbQ@mail.gmail.com>

Hi Martin

On 2019-06-13 19:21 UTC Martin Ågren <martin.agren@gmail.com> wrote:

Show 6 quoted lines

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

Show 8 quoted lines

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

Show 6 quoted lines

> 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