From: Derrick Stolee Date: Wed, 31 Jan 2018 14:08:27 GMT Subject: Re: [PATCH v2 05/27] upload-pack: factor out processing lines Message-ID: <8df41c05-c1bf-b834-2815-0dfe6b1c7b09@gmail.com> In-Reply-To: <20180126213338.GB17576@google.com> On 1/26/2018 4:33 PM, Brandon Williams wrote: > On 01/26, Stefan Beller wrote: >> On Thu, Jan 25, 2018 at 3:58 PM, Brandon Williams wrote: >>> Factor out the logic for processing shallow, deepen, deepen_since, and >>> deepen_not lines into their own functions to simplify the >>> 'receive_needs()' function in addition to making it easier to reuse some >>> of this logic when implementing protocol_v2. >>> >>> Signed-off-by: Brandon Williams >>> --- >>> upload-pack.c | 113 ++++++++++++++++++++++++++++++++++++++-------------------- >>> 1 file changed, 74 insertions(+), 39 deletions(-) >>> >>> diff --git a/upload-pack.c b/upload-pack.c >>> index 2ad73a98b..42d83d5b1 100644 >>> --- a/upload-pack.c >>> +++ b/upload-pack.c >>> @@ -724,6 +724,75 @@ static void deepen_by_rev_list(int ac, const char **av, >>> packet_flush(1); >>> } >>> >>> +static int process_shallow(const char *line, struct object_array *shallows) >>> +{ >>> + const char *arg; >>> + if (skip_prefix(line, "shallow ", &arg)) { >> stylistic nit: >> >> You could invert the condition in each of the process_* functions >> to just have >> >> if (!skip_prefix...)) >> return 0 >> >> /* less indented code goes here */ >> >> return 1; >> >> That way we have less indentation as well as easier code. >> (The reader doesn't need to keep in mind what the else >> part is about; it is a rather local decision to bail out instead >> of having the return at the end of the function.) > I was trying to move the existing code into helper functions so > rewriting them in transit may make it less reviewable? I think the way you kept to the existing code as much as possible is good and easier to review. Perhaps a style pass after the patch lands is good for #leftoverbits. >> >>> + struct object_id oid; >>> + struct object *object; >>> + if (get_oid_hex(arg, &oid)) >>> + die("invalid shallow line: %s", line); >>> + object = parse_object(&oid); >>> + if (!object) >>> + return 1; >>> + if (object->type != OBJ_COMMIT) >>> + die("invalid shallow object %s", oid_to_hex(&oid)); >>> + if (!(object->flags & CLIENT_SHALLOW)) { >>> + object->flags |= CLIENT_SHALLOW; >>> + add_object_array(object, NULL, shallows); >>> + } >>> + return 1; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int process_deepen(const char *line, int *depth) >>> +{ >>> + const char *arg; >>> + if (skip_prefix(line, "deepen ", &arg)) { >>> + char *end = NULL; >>> + *depth = (int) strtol(arg, &end, 0); nit: space between (int) and strtol? >>> + if (!end || *end || *depth <= 0) >>> + die("Invalid deepen: %s", line); >>> + return 1; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int process_deepen_since(const char *line, timestamp_t *deepen_since, int *deepen_rev_list) >>> +{ >>> + const char *arg; >>> + if (skip_prefix(line, "deepen-since ", &arg)) { >>> + char *end = NULL; >>> + *deepen_since = parse_timestamp(arg, &end, 0); >>> + if (!end || *end || !deepen_since || >>> + /* revisions.c's max_age -1 is special */ >>> + *deepen_since == -1) >>> + die("Invalid deepen-since: %s", line); >>> + *deepen_rev_list = 1; >>> + return 1; >>> + } >>> + return 0; >>> +} >>> + >>> +static int process_deepen_not(const char *line, struct string_list *deepen_not, int *deepen_rev_list) >>> +{ >>> + const char *arg; >>> + if (skip_prefix(line, "deepen-not ", &arg)) { >>> + char *ref = NULL; >>> + struct object_id oid; >>> + if (expand_ref(arg, strlen(arg), &oid, &ref) != 1) >>> + die("git upload-pack: ambiguous deepen-not: %s", line); >>> + string_list_append(deepen_not, ref); >>> + free(ref); >>> + *deepen_rev_list = 1; >>> + return 1; >>> + } >>> + return 0; >>> +} >>> + >>> static void receive_needs(void) >>> { >>> struct object_array shallows = OBJECT_ARRAY_INIT; >>> @@ -745,49 +814,15 @@ static void receive_needs(void) >>> if (!line) >>> break; >>> >>> - if (skip_prefix(line, "shallow ", &arg)) { >>> - struct object_id oid; >>> - struct object *object; >>> - if (get_oid_hex(arg, &oid)) >>> - die("invalid shallow line: %s", line); >>> - object = parse_object(&oid); >>> - if (!object) >>> - continue; >>> - if (object->type != OBJ_COMMIT) >>> - die("invalid shallow object %s", oid_to_hex(&oid)); >>> - if (!(object->flags & CLIENT_SHALLOW)) { >>> - object->flags |= CLIENT_SHALLOW; >>> - add_object_array(object, NULL, &shallows); >>> - } >>> + if (process_shallow(line, &shallows)) >>> continue; >>> - } >>> - if (skip_prefix(line, "deepen ", &arg)) { >>> - char *end = NULL; >>> - depth = strtol(arg, &end, 0); >>> - if (!end || *end || depth <= 0) >>> - die("Invalid deepen: %s", line); >>> + if (process_deepen(line, &depth)) >>> continue; >>> - } >>> - if (skip_prefix(line, "deepen-since ", &arg)) { >>> - char *end = NULL; >>> - deepen_since = parse_timestamp(arg, &end, 0); >>> - if (!end || *end || !deepen_since || >>> - /* revisions.c's max_age -1 is special */ >>> - deepen_since == -1) >>> - die("Invalid deepen-since: %s", line); >>> - deepen_rev_list = 1; >>> + if (process_deepen_since(line, &deepen_since, &deepen_rev_list)) >>> continue; >>> - } >>> - if (skip_prefix(line, "deepen-not ", &arg)) { >>> - char *ref = NULL; >>> - struct object_id oid; >>> - if (expand_ref(arg, strlen(arg), &oid, &ref) != 1) >>> - die("git upload-pack: ambiguous deepen-not: %s", line); >>> - string_list_append(&deepen_not, ref); >>> - free(ref); >>> - deepen_rev_list = 1; >>> + if (process_deepen_not(line, &deepen_not, &deepen_rev_list)) >>> continue; >>> - } >>> + >>> if (!skip_prefix(line, "want ", &arg) || >>> get_oid_hex(arg, &oid_buf)) >>> die("git upload-pack: protocol error, " >>> -- >>> 2.16.0.rc1.238.g530d649a79-goog >>>