On 03/12, Brandon Williams wrote:
> 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.
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
