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

> Brandon Williams <bmwill@google.com> 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 ", &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 (process_deepen(line, &depth))
> >  			continue;
> 		...
> 
> but more like
> 
> 		if (skip_prefix(line, "shallow ", &arg) {
> 			process_shallow(arg, &shallows);
> 			continue;
> 		}
> 		if (skip_prefix(line, "deepen ", &arg) {
> 			process_deepen(arg, &depth);
> 			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.