Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

From: Jeff King <peff@peff.net>
Date: Jul 13, 2014, 17:33 UTC
Message-ID: <20140713173356.GA8406@sigill.intra.peff.net>
In-Reply-To: <xmqqfvi518xo.fsf@gitster.dls.corp.google.com>

On Sun, Jul 13, 2014 at 10:10:27AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> Made parse_sort_string take a "var" parameter, and if given will only warn
> >> about invalid parameter, instead of error.
> >
> > This seems unnecessarily ugly since it's hard-coding specialized
> > knowledge of the callers' error-reporting requirements into what
> > should be a generalized parsing function. If you instead make
> > parse_sort_string() responsible only for attempting to parse the
> > value, but leave error-reporting to the callers, then this ugliness
> > goes away. See below.
> 
> Yup, you are absolutely right.  Thanks for catching my silly.

I do not know if it is that silly. The reason we push the error reporting down into reusable library functions, even though it is less flexible or causes us to have "quiet" flags and such, is that the library function knows more about the specific error.

In this case we are just saying "your sort specification is not valid", so it is not adding much value, and returning "-1" provides enough information for the caller to say that. But would we eventually want to diagnose errors more specifically? For example, in "foo:refname", we could complain that "foo" is not a valid sort function. And in "version:bar", we could complain that "bar" is not a valid sorting atom.

We can encode these error types into an enum, but it is often much easier to report them at the time of discovery (e.g., because you have the half-parsed string available that says "foo" or "bar"). This is a general problem throughout a lot of our code. In higher level languages you might throw an exception with the error message and let the caller decide how to report it. I wonder if it is too gross to do something like:

  push_error_handler(collect_errors);

  /* all calls to error() in will push error text onto a stack */
  do_something();

  pop_error_handler();

  /* traverse the list, calling warning() on each, and clear the stack */
  print_errors_as_warnings();

One can imagine print_errors_as_errors, which would be useful when a caller is not sure whether a full operation will succeed (e.g., you try X, then Y, and only when both fail do you report an error). Or a caller which does not call any print_error_* at all (i.e., replacing the "quiet" flag that many library functions take).

I realize that I am reinventing the error-reporting wheel on a sleepy Sunday afternoon without having thought about it much, so there is probably some gotcha or case that makes this ugly, or perhaps it just ends up verbose in practice. But one can dream.

-Peff