Jeff King <p...@peff.net> writes:
> On Thu, May 21, 2015 at 02:14:29PM -0400, Jeff King wrote:
>
>> There is a related cleanup I resisted, which is that several call-sites
>> will call stat_tracking_info, then later look directly at
>> branch->merge[0]->dst without a check for NULL (fill_tracking_info is
>> such a site).
>>
>> This works because stat_tracking_info's return value tells us that we
>> did indeed find an upstream to compare with. But it feels a little leaky
>> to me. One solution is for stat_tracking_info to pass out the name of
>> thte upstream, making the caller side something like:
>
> So just for fun, here is what a whole patch, with the refactoring of the
> return value, would look like.
It looks much nicer and reads better, at least to me.
Nice.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index cc55ff2..8ecabd1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -425,25 +425,19 @@ static void fill_tracking_info(struct strbuf *stat,
> const char *branch_name,
> int ours, theirs;
> char *ref = NULL;
> struct branch *branch = branch_get(branch_name);
> + const char *upstream;
> struct strbuf fancy = STRBUF_INIT;
> int upstream_is_gone = 0;
> int added_decoration = 1;
>
> - switch (stat_tracking_info(branch, &ours, &theirs)) {
> - case 0:
> - /* no base */
> - return;
> - case -1:
> - /* with "gone" base */
> + if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
> + if (!upstream)
> + return;
> upstream_is_gone = 1;
> - break;
> - default:
> - /* with base */
> - break;
> }
>
> if (show_upstream_ref) {
> - ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
> + ref = shorten_unambiguous_ref(upstream, 0);
> if (want_color(branch_use_color))
> strbuf_addf(&fancy, "%s%s%s",
> branch_get_color(BRANCH_COLOR_UPSTREAM),
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 6847400..05dd23d 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -730,7 +730,7 @@ static void populate_value(struct refinfo *ref)
> char buf[40];
>
> if (stat_tracking_info(branch, &num_ours,
> - &num_theirs) != 1)
> + &num_theirs, NULL))
> continue;
>
> if (!num_ours && !num_theirs)
> @@ -753,7 +753,7 @@ static void populate_value(struct refinfo *ref)
> assert(branch);
>
> if (stat_tracking_info(branch, &num_ours,
> - &num_theirs) != 1)
> + &num_theirs, NULL))
> continue;
>
> if (!num_ours && !num_theirs)
> diff --git a/remote.c b/remote.c
> index be45a39..6765051 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2016,12 +2016,15 @@ int ref_newer(const unsigned char *new_sha1, const
> unsigned char *old_sha1)
>
> /*
> * Compare a branch with its upstream, and save their differences (number
> - * of commits) in *num_ours and *num_theirs.
> + * of commits) in *num_ours and *num_theirs. The name of the upstream branch
> + * (or NULL if no upstream is defined) is returned via *upstream_name, if it
> + * is not itself NULL.
> *
> - * Return 0 if branch has no upstream (no base), -1 if upstream is missing
> - * (with "gone" base), otherwise 1 (with base).
> + * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
> + * upstream defined, or ref does not exist), 0 otherwise.
> */
> -int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
> +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
> + const char **upstream_name)
> {
> unsigned char sha1[20];
> struct commit *ours, *theirs;
> @@ -2032,8 +2035,10 @@ int stat_tracking_info(struct branch *branch, int
> *num_ours, int *num_theirs)
>
> /* Cannot stat unless we are marked to build on top of somebody else. */
> base = branch_get_upstream(branch, NULL);
> + if (upstream_name)
> + *upstream_name = base;
> if (!base)
> - return 0;
> + return -1;
>
> /* Cannot stat if what we used to build on no longer exists */
> if (read_ref(base, sha1))
> @@ -2051,7 +2056,7 @@ int stat_tracking_info(struct branch *branch, int
> *num_ours, int *num_theirs)
> /* are we the same? */
> if (theirs == ours) {
> *num_theirs = *num_ours = 0;
> - return 1;
> + return 0;
> }
>
> /* Run "rev-list --left-right ours...theirs" internally... */
> @@ -2087,7 +2092,7 @@ int stat_tracking_info(struct branch *branch, int
> *num_ours, int *num_theirs)
> /* clear object flags smudged by the above traversal */
> clear_commit_marks(ours, ALL_REV_FLAGS);
> clear_commit_marks(theirs, ALL_REV_FLAGS);
> - return 1;
> + return 0;
> }
>
> /*
> @@ -2096,23 +2101,17 @@ int stat_tracking_info(struct branch *branch, int
> *num_ours, int *num_theirs)
> int format_tracking_info(struct branch *branch, struct strbuf *sb)
> {
> int ours, theirs;
> + const char *full_base;
> char *base;
> int upstream_is_gone = 0;
>
> - switch (stat_tracking_info(branch, &ours, &theirs)) {
> - case 0:
> - /* no base */
> - return 0;
> - case -1:
> - /* with "gone" base */
> + if (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {
> + if (!full_base)
> + return 0;
> upstream_is_gone = 1;
> - break;
> - default:
> - /* with base */
> - break;
> }
>
> - base = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
> + base = shorten_unambiguous_ref(full_base, 0);
> if (upstream_is_gone) {
> strbuf_addf(sb,
> _("Your branch is based on '%s', but the upstream is
> gone.\n"),
> diff --git a/remote.h b/remote.h
> index 3326f2b..312b7ca 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -249,7 +249,8 @@ enum match_refs_flags {
> };
>
> /* Reporting of tracking info */
> -int stat_tracking_info(struct branch *branch, int *num_ours, int
> *num_theirs);
> +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
> + const char **upstream_name);
> int format_tracking_info(struct branch *branch, struct strbuf *sb);
>
> struct ref *get_local_heads(void);
> diff --git a/wt-status.c b/wt-status.c
> index 38cb165..7c8ae57 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1532,21 +1532,15 @@ static void wt_shortstatus_print_tracking(struct
> wt_status *s)
>
> color_fprintf(s->fp, branch_color_local, "%s", branch_name);
>
> - switch (stat_tracking_info(branch, &num_ours, &num_theirs)) {
> - case 0:
> - /* no base */
> - fputc(s->null_termination ? '\0' : '\n', s->fp);
> - return;
> - case -1:
> - /* with "gone" base */
> + if (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {
> + if (!base) {
> + fputc(s->null_termination ? '\0' : '\n', s->fp);
> + return;
> + }
> +
> upstream_is_gone = 1;
> - break;
> - default:
> - /* with base */
> - break;
> }
>
> - base = branch->merge[0]->dst;
> base = shorten_unambiguous_ref(base, 0);
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
