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

[Brandon Williams]

On 03/12, Brandon Williams wrote:
> On 03/01, Junio C Hamano wrote:
> > Brandon Williams  writes:
> > 
> > > 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.
> > 
> > These little functions that still require their incoming data to
> > begin with fixed prefixes feels a bit strange way to refactor the
> > logic for later reuse (when I imagine "reuse", the first use case
> > that comes to my mind is "this data source our new code reads from
> > gives the same data as the old 'shallow' packet used to give, but in
> > a different syntax"---so I'd restructure the code in such a way that
> > the caller figures out the syntax part and the called helper just
> > groks the "information contents" unwrapped from the surface syntax;
> > the syntax may be different in the new codepath but once unwrapped,
> > the "information contents" to be processed would not be different
> > hence we can reuse the helper).
> > 
> > IOW, I would have expected the caller to be not like this:
> > 
> > > - if (skip_prefix(line, "shallow ", )) {
> > > - struct object_id oid;
> > > - struct object *object;
> > > - if (get_oid_hex(arg, ))
> > > - die("invalid shallow line: %s", line);
> > > - object = parse_object();
> > > - if (!object)
> > > - continue;
> > > - if (object->type != OBJ_COMMIT)
> > > - die("invalid shallow object %s", 
> > > oid_to_hex());
> > > - if (!(object->flags & CLIENT_SHALLOW)) {
> > > - object->flags |= CLIENT_SHALLOW;
> > > - add_object_array(object, NULL, );
> > > - }
> > > + if (process_shallow(line, ))
> > >   continue;
> > > + if (process_deepen(line, ))
> > >   continue;
> > ...
> > 
> > but more like
> > 
> > if (skip_prefix(line, "shallow ", ) {
> > process_shallow(arg, );
> > continue;
> > }
> > if (skip_prefix(line, "deepen ", ) {
> > process_deepen(arg, );
> > continue;
> > }
> > ...
> > 
> > I need to defer the final judgment until I see how they are used,
> > though.  It's not too big a deal either way---it just felt "not
> > quite right" to me.
> 
> This is actually a really good point (and maybe the same point stefan
> was trying to make on an old revision of this series).  I think it makes
> much more sense to refactor the code to have a structure like you've
> outlined.  I'll fix this for the next version.

And then I started writing the code and now I don't know which I
prefer.  The issue is that its for processing a line which has some well
defined structure and moving the check for "shallow " away from the rest
of the code which does the processing makes it a little less clear how
that shallow line is to be defined.

-- 
Brandon Williams

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

[Brandon Williams]

On 03/01, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > 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.
> 
> These little functions that still require their incoming data to
> begin with fixed prefixes feels a bit strange way to refactor the
> logic for later reuse (when I imagine "reuse", the first use case
> that comes to my mind is "this data source our new code reads from
> gives the same data as the old 'shallow' packet used to give, but in
> a different syntax"---so I'd restructure the code in such a way that
> the caller figures out the syntax part and the called helper just
> groks the "information contents" unwrapped from the surface syntax;
> the syntax may be different in the new codepath but once unwrapped,
> the "information contents" to be processed would not be different
> hence we can reuse the helper).
> 
> IOW, I would have expected the caller to be not like this:
> 
> > -   if (skip_prefix(line, "shallow ", )) {
> > -   struct object_id oid;
> > -   struct object *object;
> > -   if (get_oid_hex(arg, ))
> > -   die("invalid shallow line: %s", line);
> > -   object = parse_object();
> > -   if (!object)
> > -   continue;
> > -   if (object->type != OBJ_COMMIT)
> > -   die("invalid shallow object %s", 
> > oid_to_hex());
> > -   if (!(object->flags & CLIENT_SHALLOW)) {
> > -   object->flags |= CLIENT_SHALLOW;
> > -   add_object_array(object, NULL, );
> > -   }
> > +   if (process_shallow(line, ))
> > continue;
> > +   if (process_deepen(line, ))
> > continue;
>   ...
> 
> but more like
> 
>   if (skip_prefix(line, "shallow ", ) {
>   process_shallow(arg, );
>   continue;
>   }
>   if (skip_prefix(line, "deepen ", ) {
>   process_deepen(arg, );
>   continue;
>   }
>   ...
> 
> I need to defer the final judgment until I see how they are used,
> though.  It's not too big a deal either way---it just felt "not
> quite right" to me.

This is actually a really good point (and maybe the same point stefan
was trying to make on an old revision of this series).  I think it makes
much more sense to refactor the code to have a structure like you've
outlined.  I'll fix this for the next version.

-- 
Brandon Williams

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

[Junio C Hamano]

Brandon Williams  writes:

> 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.

These little functions that still require their incoming data to
begin with fixed prefixes feels a bit strange way to refactor the
logic for later reuse (when I imagine "reuse", the first use case
that comes to my mind is "this data source our new code reads from
gives the same data as the old 'shallow' packet used to give, but in
a different syntax"---so I'd restructure the code in such a way that
the caller figures out the syntax part and the called helper just
groks the "information contents" unwrapped from the surface syntax;
the syntax may be different in the new codepath but once unwrapped,
the "information contents" to be processed would not be different
hence we can reuse the helper).

IOW, I would have expected the caller to be not like this:

> - if (skip_prefix(line, "shallow ", )) {
> - struct object_id oid;
> - struct object *object;
> - if (get_oid_hex(arg, ))
> - die("invalid shallow line: %s", line);
> - object = parse_object();
> - if (!object)
> - continue;
> - if (object->type != OBJ_COMMIT)
> - die("invalid shallow object %s", 
> oid_to_hex());
> - if (!(object->flags & CLIENT_SHALLOW)) {
> - object->flags |= CLIENT_SHALLOW;
> - add_object_array(object, NULL, );
> - }
> + if (process_shallow(line, ))
>   continue;
> + if (process_deepen(line, ))
>   continue;
...

but more like

if (skip_prefix(line, "shallow ", ) {
process_shallow(arg, );
continue;
}
if (skip_prefix(line, "deepen ", ) {
process_deepen(arg, );
continue;
}
...

I need to defer the final judgment until I see how they are used,
though.  It's not too big a deal either way---it just felt "not
quite right" to me.

[PATCH v4 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 ", )) {
+   struct object_id oid;
+   struct object *object;
+   if (get_oid_hex(arg, ))
+   die("invalid shallow line: %s", line);
+   object = parse_object();
+   if (!object)
+   return 1;
+   if (object->type != OBJ_COMMIT)
+   die("invalid shallow object %s", oid_to_hex());
+   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 ", )) {
+   char *end = NULL;
+   *depth = (int)strtol(arg, , 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 ", )) {
+   char *end = NULL;
+   *deepen_since = parse_timestamp(arg, , 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 ", )) {
+   char *ref = NULL;
+   struct object_id oid;
+   if (expand_ref(arg, strlen(arg), , ) != 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 ", )) {
-   struct object_id oid;
-   struct object *object;
-   if (get_oid_hex(arg, ))
-   die("invalid shallow line: %s", line);
-   object = parse_object();
-   if (!object)
-   continue;
-   if (object->type != OBJ_COMMIT)
-   die("invalid shallow object %s", 
oid_to_hex());
-   if (!(object->flags & CLIENT_SHALLOW)) {
-   object->flags |= CLIENT_SHALLOW;
-   add_object_array(object, NULL, );
-   }
+   if (process_shallow(line, ))
continue;
-   }
-   if (skip_prefix(line, "deepen ", )) {
-   char *end = NULL;
-   depth = strtol(arg, , 0);
-   if (!end || *end || depth <= 0)
-   die("Invalid deepen: %s", line);
+   if (process_deepen(line, ))
continue;
-   }
-   if (skip_prefix(line, "deepen-since ", )) {
-   char *end = NULL;
-   deepen_since = parse_timestamp(arg, , 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, _since, _rev_list))
continue;
-