From: Jiang Xin <worldhello.net@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>,
Git List <git@vger.kernel.org>
Subject: Re: [PATCH v5 1/2] branch: not report invalid tracking branch
Date: Thu, 15 Aug 2013 10:14:40 +0800 [thread overview]
Message-ID: <CANYiYbHC1oXjMy9tb7vRJmq2LMzk0yXwjm_kMoM-U7cqvrjiZA@mail.gmail.com> (raw)
In-Reply-To: <7vfvucuwm7.fsf@alter.siamese.dyndns.org>
2013/8/14 Junio C Hamano <gitster@pobox.com>
> > /*
> > - * 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
next prev parent reply other threads:[~2013-08-15 2:14 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-07 15:42 [RFC] status: show tracking branch even no difference Jiang Xin
2013-08-07 15:50 ` Matthieu Moy
2013-08-07 16:03 ` Jiang Xin
2013-08-08 5:40 ` [PATCH v2] status: always show tracking branch even no change Jiang Xin
2013-08-08 14:49 ` [PATCH v3] " Jiang Xin
2013-08-08 14:49 ` Jiang Xin
2013-08-09 21:18 ` Junio C Hamano
2013-08-10 15:05 ` Jiang Xin
2013-08-12 6:15 ` Junio C Hamano
2013-08-13 4:49 ` Jiang Xin
2013-08-13 4:53 ` [PATCH v5 1/2] branch: not report invalid tracking branch Jiang Xin
2013-08-14 15:21 ` Junio C Hamano
2013-08-15 2:14 ` Jiang Xin [this message]
2013-08-14 15:38 ` Junio C Hamano
2013-08-15 18:11 ` [PATCH v6 1/3] " Jiang Xin
2013-08-15 18:11 ` [PATCH v6 2/3] branch: report invalid tracking branch as broken Jiang Xin
2013-08-15 18:38 ` Junio C Hamano
2013-08-15 22:54 ` Junio C Hamano
2013-08-16 2:29 ` [PATCH v7 0/3] some enhancements for reporting branch tracking info Jiang Xin
2013-08-18 19:51 ` Junio C Hamano
2013-08-19 0:38 ` Jiang Xin
2013-08-26 7:02 ` [PATCH v8 0/2] " Jiang Xin
2013-08-26 7:21 ` Junio C Hamano
2013-08-26 7:02 ` [PATCH v8 1/2] branch: report invalid tracking branch as gone Jiang Xin
2013-08-26 7:02 ` [PATCH v8 2/2] status: always show tracking branch even no change Jiang Xin
2013-08-26 7:47 ` Jeremy Rosen
2013-08-26 8:04 ` Jiang Xin
2013-08-26 16:08 ` Junio C Hamano
[not found] ` <cover.1376620130.git.worldhello.net@gmail.com>
2013-08-16 2:29 ` [PATCH v7 1/3] branch: not report invalid tracking branch Jiang Xin
2013-08-16 2:29 ` [PATCH v7 2/3] branch: mark missing tracking branch as gone Jiang Xin
2013-08-21 7:37 ` Matthieu Moy
2013-08-22 0:00 ` Jiang Xin
2013-08-16 2:29 ` [PATCH v7 3/3] status: always show tracking branch even no change Jiang Xin
2013-08-15 18:11 ` [PATCH v6 " Jiang Xin
2013-08-15 22:56 ` Junio C Hamano
2013-08-13 4:53 ` [PATCH v5 2/2] " Jiang Xin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANYiYbHC1oXjMy9tb7vRJmq2LMzk0yXwjm_kMoM-U7cqvrjiZA@mail.gmail.com \
--to=worldhello.net@gmail.com \
--cc=Matthieu.Moy@grenoble-inp.fr \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).