On 03/01, Junio C Hamano wrote:
> Brandon Williams <bmwill@xxxxxxxxxx> 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.
--
Brandon Williams
