Re: [PATCH] fetch: print an error when declining to request an unadvertised object

Matt McCutchen <matt@mattmccutchen.net> writes:

The fact that we have the above two choices tells me that a two-step approach may be an appropriate approach.

The first step is to teach fetch_refs_via_pack() that it should not ignore the information returned in sought[]. It would add new code similar to what cmd_fetch_pack() uses to notice and report errors [*1*] to the function. It would be a sensible first step, but would not let the user know which of multiple causes of "not matched" we noticed.

By "a more specific error", I think you are envisioning that the current boolean "matched" is made into an enum that allows the caller to tell how each request did not match [*2*]. That can be the topic of the second patch and would have to touch filter_refs() [*3*], cmd_fetch_pack() and fetch_refs_via_pack().

I do not have strong preference myself offhand between stopping at the first step or completing both.

Even if you did only the first step, as long as the second step can be done without reverting what the first step did [*4*] by somebody who cares the "specific error" deeply enough, I am OK with that. Of course if you did both steps, that is fine by me as well ;-)

[Footnote]

*1* While I know that it is not right to die() in filter_refs(), and
    fetch_refs_via_pack() is a better place to notice errors, I do
    not offhand know if it is the right place to report errors, or a
    caller higher in the callchain may want the callee to be silent
    and wants to show its own error message (in which case the error
    may have to percolate upwards in the callchain).

*2* e.g. "was it a ref but they did not advertise?  Did it request
    an explicit object name and they did not allow it?"  We may want
    to support other "more specific" errors that can be detected in
    the future.

*3* The current code flips the sought[i]->matched bit on for matched
    ones (relying on the initial state of the bit being false), but
    it now needs to stuff different kind of "not matched" to the
    field to allow the caller to act on it.

*4* IOW, I am OK with an initial "small" improvement, but I'd want
    to make sure that such an initial step does not make future
    enhancements by others harder.