Jean-Noël Avila <avila.jn@xxxxxxxxx> writes:
>> -const char *read_gitfile_gently(const char *path, int *return_error_code)
>> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
>> {
>> const int max_file_size = 1 << 20; /* 1MB */
>> int error_code = 0;
>> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>> struct stat st;
>> int fd;
>> ssize_t len;
>> - static struct strbuf realpath = STRBUF_INIT;
>> + static struct strbuf shared = STRBUF_INIT;
>> + if (!result_buf) {
>> + result_buf = &shared;
>> + }
>>
>
> Question about general style: is it accepted practice to override a
> parameter of a function?
We do not forbid it. We have a rule against enclosing a single
statement block inside {braces}, though ;-).
> I would have written:
If it matters to know what the caller-supplied value of the
parameter was, then we would probably write that way. If it does
not, then the above is perfectly fine. Even with the above, if a
later code really wanted to, it can compare the pointers to find out
if the caller was uninterested in the result (i.e., passed NULL),
but at that point, we may be better off to (re)write it your way.
>> - strbuf_realpath(&realpath, dir, 1);
>> - path = realpath.buf;
>> + strbuf_realpath(result_buf, dir, 1);
>> + path = result_buf->buf;
>> + // TODO is this valid?
>> + if (!path) die(_("Unexpected null from realpath '%s'"), dir);
>
> In fact, this is not a null path, but an empty path (null is not part of
> the string).
> By the way, shouldn't this be an internal bug instead of a message to
> the user?
Unless the strbuf instance the result_buf pointer points at is
corrupt, its .buf member should *NEVER* be NULL. Testing for NULL
is meaningless, unless you are manually futzing with the members of
strbuf (you shouldn't).
Thanks for carefully reading.
