Re: [PATCH] read_gitfile_gently(): return non-repo path on error

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> I've tried to make the minimally-invasive fix here:
>
>   1. We only copy the string when we hit READ_GITFILE_ERR_NOT_A_REPO,
>      so other error codes don't have to worry about freeing it.
>
>   2. We'll turn read_gitfile_gently() into a wrapper which passes NULL
>      by default, leaving other callers unaffected.

Nice, probably.  I do not know what to feel about the first point,
though, as it burdens those who add new callers in the future more.

> The result is kind of gross. There's an extra layer of macro
> indirection, and the validity of the string is subtly tied to the
> NOT_A_REPO error. A cleaner solution might be an error struct that
> couples the code and the output string together, along with a function
> to free the error struct. But then all callers would have to be modified
> to call the free function. Alternatively, we could perhaps put a
> large-ish fixed-size buffer in the struct, though that means potential
> truncation and a larger stack footprint in each caller (even when they
> don't have see an error).

None of thoese are particularly appetizing ;-).

> So I've left that as possible work for the future, or maybe never. Some
> of this gross-ness was already there. For example, the only other caller
> of read_gitfile_error_die() is in submodule.c, and it also passes NULL
> for the "dir" parameter. But it does so only when the code is not
> NOT_A_REPO! So it is depending on the same subtle connection to avoid
> triggering the bug.

Yup.  I can agree with this.

> ---
> Two other points of interest.
>
> One, I'm not sure how useful printing the pointed-to directory is. We
> _could_ just say:
>
>   fatal: gitfile does not point to a valid repository: /path/to/.git
>
> which is enough for somebody to investigate themselves. That would
> certainly make the patch smaller.

Thanks.  While reading the main explanation, it was the first thing
that came to me.

The implementation and the test are as expected in patches from you
and matches the intent explained in the log message exactly.

Thanks, will queue.