Re: [PATCH v2 05/27] upload-pack: factor out processing lines
- From
- Brandon Williams <bmwill@google.com>
- Date
- Jan 26, 2018, 21:33 UTC
- Message-ID
- <20180126213338.GB17576@google.com>
- In-Reply-To
- <CAGZ79kYbhR-y7WkhFgQ-YBkWaNXWuadDAGXaAzWDyJBzDSqc2w@mail.gmail.com>
On 01/26, Stefan Beller wrote:
Show 40 quoted lines
> On Thu, Jan 25, 2018 at 3:58 PM, Brandon Williams <bmwill@google.com> wrote:
> > 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 <bmwill@google.com>
> > ---
> > upload-pack.c | 113 ++++++++++++++++++++++++++++++++++++++--------------------
> > 1 file changed, 74 insertions(+), 39 deletions(-)
> >
> > diff --git a/upload-pack.c b/upload-pack.c
> > index 2ad73a98b..42d83d5b1 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 ", &arg)) {
>
> stylistic nit:
>
> You could invert the condition in each of the process_* functions
> to just have
>
> if (!skip_prefix...))
> return 0
>
> /* less indented code goes here */
>
> return 1;
>
> That way we have less indentation as well as easier code.
> (The reader doesn't need to keep in mind what the else
> part is about; it is a rather local decision to bail out instead
> of having the return at the end of the function.)I was trying to move the existing code into helper functions so rewriting them in transit may make it less reviewable?
Show 127 quoted lines
>
> > + 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)
> > + return 1;
> > + 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);
> > + }
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int process_deepen(const char *line, int *depth)
> > +{
> > + const char *arg;
> > + if (skip_prefix(line, "deepen ", &arg)) {
> > + char *end = NULL;
> > + *depth = (int) strtol(arg, &end, 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 ", &arg)) {
> > + char *end = NULL;
> > + *deepen_since = parse_timestamp(arg, &end, 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 ", &arg)) {
> > + char *ref = NULL;
> > + struct object_id oid;
> > + if (expand_ref(arg, strlen(arg), &oid, &ref) != 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 ", &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 (skip_prefix(line, "deepen ", &arg)) {
> > - char *end = NULL;
> > - depth = strtol(arg, &end, 0);
> > - if (!end || *end || depth <= 0)
> > - die("Invalid deepen: %s", line);
> > + if (process_deepen(line, &depth))
> > continue;
> > - }
> > - if (skip_prefix(line, "deepen-since ", &arg)) {
> > - char *end = NULL;
> > - deepen_since = parse_timestamp(arg, &end, 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, &deepen_since, &deepen_rev_list))
> > continue;
> > - }
> > - if (skip_prefix(line, "deepen-not ", &arg)) {
> > - char *ref = NULL;
> > - struct object_id oid;
> > - if (expand_ref(arg, strlen(arg), &oid, &ref) != 1)
> > - die("git upload-pack: ambiguous deepen-not: %s", line);
> > - string_list_append(&deepen_not, ref);
> > - free(ref);
> > - deepen_rev_list = 1;
> > + if (process_deepen_not(line, &deepen_not, &deepen_rev_list))
> > continue;
> > - }
> > +
> > if (!skip_prefix(line, "want ", &arg) ||
> > get_oid_hex(arg, &oid_buf))
> > die("git upload-pack: protocol error, "
> > --
> > 2.16.0.rc1.238.g530d649a79-goog
> >-- Brandon Williams