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

> On 03/01, Junio C Hamano wrote:
> > 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.
> 
> 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.