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

From: Jon Seymour <jon.seymour@gmail.com>
Date: Aug 23, 2010, 14:59 UTC
Message-ID: <AANLkTi=3=sumzcSFoCN6FUPQPzfQvfSim8KPRoXoT3tL@mail.gmail.com>
In-Reply-To: <7vy6by2vlf.fsf@alter.siamese.dyndns.org>

On Mon, Aug 23, 2010 at 7:20 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> 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.

Show 22 quoted lines

> 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.