2013/8/14 Junio C Hamano <gitster@xxxxxxxxx>
> > /*
> > - * 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.
>
> > + *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,
> > @@ -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.
> > @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html