From: Johan Herland <johan@herland.net>
Date: Thu, 28 Apr 2011 23:13:26 GMT
Subject: Re: [PATCHv5 7/7] Improve error handling when parsing dirstat parameters
Message-ID: <201104290113.26481.johan@herland.net>
In-Reply-To: <7vhb9i43sl.fsf@alter.siamese.dyndns.org>

(I've snipped the comments that I don't dispute or need to comment on)

On Thursday 28 April 2011, Junio C Hamano wrote:
> The error output shows "error:" followed by "warning:", which looked
> somewhat questionable.  Perhaps allow a pointer to a structure be passed
> in to describe the nature of a breakage to parse_dirstat_params()?

Not sure what you mean here. You want the caller to supply a string_list, to 
which parse_dirstat_params() appends error messages, and then the caller 
determines how to display those error messages to the user after 
parse_dirstat_params() has returned?

I'd rather go the simpler way, and simply turn the first "error:" into a 
"warning:". In other words, parse_dirstat_params() should only output 
"warning:", and then it's up to the caller whether to follow up with another 
"warning:" (in the diff.dirstat case), or a "fatal:" (in the --dirstat 
case).


Otherwise, I agree with all your comments, and the next re-roll will be 
updated accordingly.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net