Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line

Junio C Hamano <gitster@pobox.com> writes:

The only unknown here is the potential presence of "continue;" in ... and that can be addressed by writing

    for (p = *tail; p; p = *tail) {
       ...
    }

However, that only makes sense where ... is rather large and diverse and the assignment in question provides a unifying point. In this case, the loop is rather small and perfectly fits on one screen. It turns out that the assignment only serves for _obfuscating_ the various code paths. We have:

        while ((p = *tail) != NULL) {
                cmp = ((i >= q->nr)
                       ? -1 : strcmp(p->path, q->queue[i]->two->path));

                if (cmp < 0) {
                        /* p->path not in q->queue[]; drop it */
                        *tail = p->next;
                        free(p);
                        continue;
                }

                if (cmp > 0) {
                        /* q->queue[i] not in p->path; skip it */
                        i++;
                        continue;
                }

                hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
                p->parent[n].mode = q->queue[i]->one->mode;
                p->parent[n].status = q->queue[i]->status;

                tail = &p->next;
                i++;
        }

While we could instead have:
      p = curr;
      while (p) {
                cmp = ((i >= q->nr)
                       ? -1 : strcmp(p->path, q->queue[i]->two->path));

                if (cmp < 0) {
                        struct combine_diff_path *n = p->next;
                        /* p->path not in q->queue[]; drop it */
                        free(p);
                        p = *tail = n;
                        continue;
                }

                if (cmp > 0) {
                        /* q->queue[i] not in p->path; skip it */
                        i++;
                        continue;
                }

                hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
                p->parent[n].mode = q->queue[i]->one->mode;
                p->parent[n].status = q->queue[i]->status;

                p = *(tail = &p->next);
                i++;
        }

Of course, it only makes limited sense to recheck p after the second if, so it would be clearer to write

      p = curr;
      while (p) {
                cmp = ((i >= q->nr)
                       ? -1 : strcmp(p->path, q->queue[i]->two->path));

                if (cmp < 0) {
                        struct combine_diff_path *n = p->next;
                        /* p->path not in q->queue[]; drop it */
                        free(p);
                        p = *tail = n;
                        continue;
                }

                if (cmp > 0) {
                        /* q->queue[i] not in p->path; skip it */
                        i++;
                        continue;
                }

                hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
                p->parent[n].mode = q->queue[i]->one->mode;
                p->parent[n].status = q->queue[i]->status;

                p = *(tail = &p->next);
                i++;
        }

But that's sort of a red herring since the actual loop structure is hidden in conditions where it does not belong. (i >= q->nr) is a _terminal_ condition.

So it's more like

    p = curr;
    while (p) {
        if (i >= q->nr) {
            *tail = NULL;
            do {
                struct combine_diff_path *n = p->next;
                free(p);
                p = n;
            } while (p);
            break;
        }
        cmp = strcmp(p->path, q->queue[i]->two->path));
        if (cmp < 0) {
                struct combine_diff_path *n = p->next;
                /* p->path not in q->queue[]; drop it */
                free(p);
                p = *tail = n;
                continue;
        }
        if (cmp == 0) {
                hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
                p->parent[n].mode = q->queue[i]->one->mode;
                p->parent[n].status = q->queue[i]->status;

                p = *(tail = &p->next);
        }
        i++;
    }

Yes: it was just a matter of style instead of preventing _other_ code to
be rewritten in a clearer manner.

For a "don't look elsewhere" solution,

        while ((p = *tail) != NULL)

can _always_ be equivalently replaced with

        for (p = *tail; p; p = *tail)

and in this case already trivially improved with

        for (p = curr; p; p = *tail)

which meets your style prescription "avoid assignments in conditionals when you do not have to." but in this particular case, the "don't look elsewhere" solution was not called for. It unifies code paths that deserve to stay separate: we don't _want_ the assignment to take place for every path leading to the loop control. It makes it less clear to see what happens.