Re: [PATCH v3 05/35] upload-pack: factor out processing lines

[Brandon Williams]

On 02/22, Stefan Beller wrote:
> On Tue, Feb 6, 2018 at 5:12 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 
> 
> Reviewed-by: Stefan Beller 
> for the stated purpose of just refactoring existing code for better reuse 
> later.
> 
> I do have a few comments on the code in general,
> which might be out of scope for this series.

Yeah you mentioned some comments in a previous round based on style
preference.  I'm going to refrain from changing the style of this patch
since it is a matter of preference.

> 
> A close review would have been fastest if we had some sort of
> https://public-inbox.org/git/20171025224620.27657-1-sbel...@google.com/
> which I might revive soon for this purpose. (it showed that I would need it)
> 
> 
> > +   *depth = (int)strtol(arg, &end, 0);
> 
> strtol is not used quite correctly here IMHO, as we do not
> inspect errno for ERANGE
> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: [PATCH v3 05/35] upload-pack: factor out processing lines

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 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 

Reviewed-by: Stefan Beller 
for the stated purpose of just refactoring existing code for better reuse later.

I do have a few comments on the code in general,
which might be out of scope for this series.

A close review would have been fastest if we had some sort of
https://public-inbox.org/git/20171025224620.27657-1-sbel...@google.com/
which I might revive soon for this purpose. (it showed that I would need it)


> +   *depth = (int)strtol(arg, &end, 0);

strtol is not used quite correctly here IMHO, as we do not
inspect errno for ERANGE

Thanks,
Stefan

[PATCH v3 05/35] upload-pack: factor out processing lines

[Brandon Williams]

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..1e8a9e1ca 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;
+   if