Re: [PATCH v5 1/2] branch: not report invalid tracking branch

From: Jiang Xin <worldhello.net@gmail.com>
Date: Aug 15, 2013, 02:14 UTC
Message-ID: <CANYiYbHC1oXjMy9tb7vRJmq2LMzk0yXwjm_kMoM-U7cqvrjiZA@mail.gmail.com>
In-Reply-To: <7vfvucuwm7.fsf@alter.siamese.dyndns.org>

2013/8/14 Junio C Hamano <gitster@pobox.com>

Show 22 quoted lines

> >  /*
> > - * Return true if there is anything to report, otherwise false.
> > + * Return false if cannot stat a tracking branch (not exist or invalid),
> > + * otherwise true.
> >   */
> >  int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
> >  {
> > @@ -1740,18 +1741,12 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
> >       const char *rev_argv[10], *base;
> >       int rev_argc;
> >
> > -     /*
> > -      * Nothing to report unless we are marked to build on top of
> > -      * somebody else.
> > -      */
> > +     /* False unless we are marked to build on top of somebody else. */
>
> Aren't these saying the same thing?  I'd rather see the comment say
> "nothing/something to report", instead of "false/true".  The latter
> can be read from the value returned in the code, and writing that in
> the comment is redundant.  The former tells the reader what that
> "false" _means_, which is the whole point of adding a comment.

Maybe "Cannot stat unless ..." is better than "Nothing to report unless ...", because this patch change the meaning of returns of stat_tracking_info(). And I have already updated the comments for this function.

Show 15 quoted lines

>
> > +     *num_theirs = 0;
> > +     *num_ours = 0;
> > +
> >       /* are we the same? */
> >       if (theirs == ours)
> > -             return 0;
> > +             return 1;
>
> Shouldn't these zero assignments belong to this condition?  I.e.
>
>         if (theirs == ours) {
>                 *num_theirs = *num_ours = 0;
>                 return 1;
>         }

I will refactor like this,

Show 9 quoted lines

> > @@ -1786,8 +1784,6 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
> >       prepare_revision_walk(&revs);
> >
> >       /* ... and count the commits on each side. */
> > -     *num_ours = 0;
> > -     *num_theirs = 0;
> >       while (1) {
> >               struct commit *c = get_revision(&revs);
> >               if (!c)

and these two variables(*num_ours and *num_theirs) have to be initialized here again.

Show 19 quoted lines

> > @@ -1815,6 +1811,10 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
> >       if (!stat_tracking_info(branch, &num_ours, &num_theirs))
> >               return 0;
> >
> > +     /* Nothing to report if neither side has changes. */
> > +     if (!num_ours && !num_theirs)
> > +             return 0;
>
> As far as I can tell, all callers of stat_tracking_info() pass
> non-NULL pointers to these two parameters, with or without your
> patch.  Can this ever trigger?
>
> The changes you made to builtin/branch.c seems to expect that
> returned *num_ours and *num_theirs could both be 0, so it does not
> look like the above is a typo of
>
>         if (!*num_ours && !*num_theirs)
>                 return 0;
>

It's really easy to make people puzzled, since these two hunks in this patch both have two similar variables: num_ours and num_theirs. But they are different.

In previous hunk, num_ours and num_theres are from stat_tracking_info(), and they are pointers.

    int stat_tracking_info(struct branch *branch,
                           int *num_ours,
                           int *num_theirs)

But in this hunk, num_ours and num_theres are defined as integers in funciton format_tracking_info().

    int format_tracking_info(struct branch *branch, struct strbuf *sb)
    {
        int num_ours, num_theirs;

To make it clear, I should change the variables name to ours and theirs just like function fill_tracking_info() in builtin/branch.c.

-- 
Jiang Xin