Re: [PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.

On Mon, Aug 23, 2010 at 7:20 AM, Junio C Hamano [off-list ref] wrote:

Jon Seymour [off-list ref] writes:

quoted

With this change, git rev-parse master@{99999} reports:

  master@{99999}
  error: Log for 'master' only has 166 entries.
  fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

If we are going to say "fatal:" and die at the end, I think we probably do
not want to say a separate "error" message.  Instead of adding a boolean
parameter "fail silently or warn?", it may be better to make it a pointer
to a strbuf and have it filled with error details (or a machine readable
struct and make it responsibility of the caller to generate a message).

I agree that it is not ideal that we have an error message and a fatal
message in this case.

Ideally,  I think we should simply be reporting the log limit exceeded
condition as a fatal condition on its own, and not reporting the
ambiguous ref catch all at all in this case. We have already decided
that the argument is a reference at this point and as such it is not
really ambiguous, it just happens to be out of bounds.

Then die_verify_filename can become, e.g.:

static void NORETURN die_verify_filename(const char *prefix, const char *arg)
{
       unsigned char sha1[20];
       unsigned mode;
       struct strbuf e;

       /* learn in what way is it bad? */
       get_sha1_with_mode_1(arg, sha1, &mode, &e, prefix);

       /* ... or fall back the most general message. */
       die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
           "%s"
           "Use '--' to separate paths from revisions", arg, e.buf);
}

and we can later reuse the same mechanism to cover other kind of errors,
not just "the log does not have that many entries" error.  With your
"gently" approach it may not be easy to do that without adding more
parameters to all the functions in the codepath, no?

I agree that the gently approach used here doesn't generalise to other
cases. It seemed like the smallest change I could make that was
reasonably consistent with existing code. I am happy to look at a more
general solution.

I also wonder if this can simply become part of "struct object_context",
which is to pass extra information in addition to the SHA-1 (which the
original API returned) back to the caller.

I'll have a look at this. object_context seems like a reasonably
coherent structure at the moment - do we risk diluting that coherence
by attaching error reporting state to it?

Should we consider passing an error callback function (+ struct)
around so that the calling context can decide whether to directly
report errors at the point of detection or defer them until later?
Calling contexts that don't care about reporting immediately can do
so, calling contexts that do care can choose how to record errors into
the struct, as required.

jon.