Re: [PATCH v2 05/27] upload-pack: factor out processing lines
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); - } +
Re: [PATCH v2 05/27] upload-pack: factor out processing lines
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? > > > + 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); > > + 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_
Re: [PATCH v2 05/27] upload-pack: factor out processing lines
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.) > + 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); > + 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; > -
[PATCH v2 05/27] upload-pack: factor out processing lines
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)) { + 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); + 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; + i