Re: [PATCH 2/2] daemon: report permission denied error to clients

Jeff King [off-list ref] writes:

On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:

I like the intent. This actually does leak a little more information
than the existing --informative-errors, as before you couldn't tell the
difference between "not found" and "not exported".

I personally think this is going a bit too far, even for "informative"
option, by allowing to fish for possible list of usernames. It would make
it a tougher sell to later default to "informative", I am afraid.

Suppose you are setting up your own repository (either on your own box or
on a box with a separate administrator), and youare wondering why your
attempted access failed. You know /pub/repo/sito.git exists (you created
it after all) and you get "no such repository: /repo/sito.git" when you
ran:

    $ git clone git://host/repo/sito.git/

If you have another repository in /pub/repo/ that does already work, and
if you know /pub/repo/sito.git/ is fine locally (e.g. you can see local
command like "git log" works fine there), then even if you see "not found"
you would know to compare what the difference between these two are.

If there is no other repositories in /pub/repo/ or you are setting up many
repositories on this same box for the first time, wouldn't it be plausible
that you _are_ the administrator of the box and have access to the daemon
log to diagnose the problem more easily anyway?

I can see how this is "leaking a little more information", but I am not
convinced that leak is helping legit users more than helping unwanted
snoopers.

The new calling conventions for this function seem a little weird.  I
would expect either "return negative, and set errno" for usual library
code, or possibly "return negative error value". But "return -1, or a
positive error code" seems unusual to me.

One of:

  errno = EACCESS;
  return -1;

or

  return -EACCESS;

would be more idiomatic, I think.

Yes, the former would probably be easier to handle.