Re: [PATCH 17/18] revert: Don't implictly stomp pending sequencer operation
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Jul 27, 2011, 09:59 UTC
- Message-ID
- <CALkWK0mhAc4TtSzN5DW8OX91QBvosbbKsYfB5QPFy563qg7KRQ@mail.gmail.com>
- In-Reply-To
- <20110727051947.GL18470@elie>
Hi,
Jonathan Nieder writes:
Show 15 quoted lines
> Ramkumar Ramachandra wrote:
> [...]
>> + if (create_seq_dir() < 0) {
>> + advise(_("A cherry-pick or revert is in progress."));
>> + advise(_("Use --reset to forget about it"));
>> + return -1;
>> + }
>
> The usual formula is:
>
> error(... description of error ...)
> if (advice_foo_bar [i.e., if the user is not tired of the advice already]) {
> advise(... how to recover from error ...);
> advise(... more lines ...);
> }I think you're trying to say two things here: 1. Put the error() call in the caller. Why is this a good idea? The error is very specific to the create_seq_dir functionality, and has nothing to do with the caller. The advice on the other hand, can be quite caller-specific, which is why I put it in the caller in the first place. 2. Guard the advice using a variable. I have to invent a new configuration variable; can't that wait*?
Thanks.
* Ideally, I'd do these things immediately if I didn't fear picking a stupid variable name, and implementing it in a daft manner the first few times. More delays will result in me doing less post midterm work (the more interesting bits).
-- Ram