From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Wed, 27 Jul 2011 09:59:06 GMT
Subject: Re: [PATCH 17/18] revert: Don't implictly stomp pending sequencer operation
Message-ID: <CALkWK0mhAc4TtSzN5DW8OX91QBvosbbKsYfB5QPFy563qg7KRQ@mail.gmail.com>
In-Reply-To: <20110727051947.GL18470@elie>

Hi,

Jonathan Nieder writes:
> 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