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

2013/8/14 Junio C Hamano [off-list ref]

quoted

 /*
- * 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.

quoted

+     *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,

quoted

@@ -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.

quoted

@@ -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