Jiang Xin <worldhello.net@xxxxxxxxx> writes:
> Command "git branch -vv" will report tracking branches, but invalid
> tracking branches are also reported. This is because the function
> stat_tracking_info() can not distinguish whether the upstream branch
> does not exist, or nothing is changed between one branch and its
> upstream.
>
> This patch changes the return value of function stat_tracking_info().
> Only returns false when there is no tracking branch or the tracking
> branch is invalid, otherwise true. If the caller does not like to
> report tracking info when nothing changed between the branch and its
> upstream, simply checks if num_theirs and num_ours are both 0.
>
> Signed-off-by: Jiang Xin <worldhello.net@xxxxxxxxx>
> ---
> builtin/branch.c | 18 +++++-------------
> remote.c | 24 ++++++++++++------------
> wt-status.c | 13 +++++++++++--
> 3 files changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0836890..359e75d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -424,19 +424,8 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
> struct branch *branch = branch_get(branch_name);
> struct strbuf fancy = STRBUF_INIT;
>
> - if (!stat_tracking_info(branch, &ours, &theirs)) {
> - if (branch && branch->merge && branch->merge[0]->dst &&
> - show_upstream_ref) {
> - ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
> - if (want_color(branch_use_color))
> - strbuf_addf(stat, "[%s%s%s] ",
> - branch_get_color(BRANCH_COLOR_UPSTREAM),
> - ref, branch_get_color(BRANCH_COLOR_RESET));
> - else
> - strbuf_addf(stat, "[%s] ", ref);
> - }
> + if (!stat_tracking_info(branch, &ours, &theirs))
> return;
> - }
>
> if (show_upstream_ref) {
> ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
> @@ -448,7 +437,10 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
> strbuf_addstr(&fancy, ref);
> }
>
> - if (!ours) {
> + if (!ours && !theirs) {
> + if (ref)
> + strbuf_addf(stat, _("[%s]"), fancy.buf);
Mental note: the updated stat_tracking_info() says "something to
report exists" when the current branch and its @{u} are the same,
and we report the branch here.
> + } else if (!ours) {
> if (ref)
> strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
This is not a fault of your change, but the logic to show the
upstream ref is unnecessarily unclear due to variable cascading.
The master switch "show_upstream_ref" is what is used to ask this to
fill the name of the upstream ref, and "ref" is made non-NULL only
when "show_upstream_ref" is true. Also "fancy" would be non-empty
only in that case, so this "if (ref)" could be any one of these:
if (show_upstream_ref)
if (ref)
if (fancy.len)
It is disturbing that the middle one is used. If you take the
stance that the code should reflect the logic, the first one
(i.e. "we were asked to show upstream ref, so we show it here")
makes the most sense. If you take the stance that the code should
be safe with the low-level implementation, the last one (i.e. "we
are showing what is in fancy, if we have prepared something to show
in that buffer") makes sense. The middle one is the least logical
thing to base the decision on.
> diff --git a/remote.c b/remote.c
> index 2433467..c747936 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1729,7 +1729,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
> }
>
> /*
> - * 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.
> if (!branch ||
> !branch->merge || !branch->merge[0] || !branch->merge[0]->dst)
> return 0;
>
> - /*
> - * If what we used to build on no longer exists, there is
> - * nothing to report.
> - */
> + /* False if what we used to build on no longer exists */
> base = branch->merge[0]->dst;
> if (read_ref(base, sha1))
> return 0;
Likewise.
> + *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;
}
> @@ -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)
> @@ -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;
even though the comment "neither side has changes" seems to indicate
that was what was meant.
I am puzzled by this incoherence between the callers and the callee.
> diff --git a/wt-status.c b/wt-status.c
> index ff4b324..0c6a3a5 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1380,15 +1380,24 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
> branch = branch_get(s->branch + 11);
> if (s->is_initial)
> color_fprintf(s->fp, header_color, _("Initial commit on "));
> +
> + color_fprintf(s->fp, branch_color_local, "%s", branch_name);
> +
> + /*
> + * Not report tracking info if no tracking branch found
> + * or no difference found.
> + */
> if (!stat_tracking_info(branch, &num_ours, &num_theirs)) {
> - color_fprintf(s->fp, branch_color_local, "%s", branch_name);
> + fputc(s->null_termination ? '\0' : '\n', s->fp);
> + return;
> + }
> + if (!num_ours && !num_theirs) {
> fputc(s->null_termination ? '\0' : '\n', s->fp);
> return;
> }
>
> base = branch->merge[0]->dst;
> base = shorten_unambiguous_ref(base, 0);
> - color_fprintf(s->fp, branch_color_local, "%s", branch_name);
> color_fprintf(s->fp, header_color, "...");
> color_fprintf(s->fp, branch_color_remote, "%s", base);
--
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