Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov [off-list ref] wrote:

[...]
Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.
[...]

A few small comments below; with the possible exception of one,
probably none worth a re-roll...

Unsigned format conversion '%lu' doesn't seem correct for ssize_t.

+       }
+
+       if (req_len <= 0) {
+               *out = NULL;
+               return 0;
+       }
+
+       buf = xmalloc(req_len);
+       cnt = read_in_full(fd, buf, req_len);
+       if (cnt < 0) {
+               free(buf);
+               return -1;
+       } else {
+               *out = buf;
+               return cnt;
+       }

This could have been written:

    if (cnt < 0) {
        free(buf);
        return -1;
    }
    *out = buf;
    return cnt;

but not worth a re-roll.

+}
+
+static ssize_t env_content_length(void)

The caller of this function doesn't care how the content length is
being determined -- whether it comes from an environment variable or
is computed some other way; it cares only about the result. Having
"env" in the name ties it to checking only the environment. A more
generic name, such as get_content_length(), would help to decouple the
API from the implementation.

Nevertheless, not worth a re-roll.

+{
+       ssize_t val = -1;
+       const char *str = getenv("CONTENT_LENGTH");
+
+       if (str && !git_parse_ssize_t(str, &val))

git_parse_ssize_t() does the right thing even when 'str' is NULL, so
this condition could be simplified (but not worth a re-roll and may
not improve clarity).

+               die("failed to parse CONTENT_LENGTH: %s", str);
+       return val;
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+       ssize_t req_len = env_content_length();

Grabbing and parsing the value from the environment variable is
effectively a one-liner, so env_content_length() could be dropped
altogether, and instead (taking advantage of git_parse_ssize_t()'s
proper NULL-handling):

    if (!git_parse_ssize_t(getenv(...), &req_len))
        die(...);

Not worth a re-roll.

+       if (req_len < 0)
+               return read_request_eof(fd, out);
+       else
+               return read_request_fixed_len(fd, req_len, out);
+}