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

[Jeff King <peff@peff.net>]

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

> 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