Re: [PATCH 1/1] Fix %(push:track) in ref-filter

From: Jeff King <peff@peff.net>
Date: Apr 16, 2019, 21:48 UTC
Message-ID: <20190416214842.GA21429@sigill.intra.peff.net>
In-Reply-To: <20190416123944.vtoremaitywtmkhj@mithrim>

On Tue, Apr 16, 2019 at 02:39:45PM +0200, Damien Robert wrote:

Show 9 quoted lines

> > Or perhaps it argues for just giving access to the more generic stat_*
> > function, and letting callers pass in a flag for push vs upstream (and
> > either leaving stat_tracking_info() as a wrapper, or just updating its
> > few callers).
> 
> So I went ahead with modifying `stat_tracking_info` to accept a 'for_push'
> flag, and updated the few callers. This means that `stat_compare_info` is
> only used by `stat_tracking_info` so I could reinline it, but I guess it
> could still be useful latter.

Reading this paragraph, my gut reaction was to say that it should stay as a single function. But actually looking at the code, I think it is a bit nicer to separate out "compare these two branches" from "figure out which branches to compare".

The name "compare_info" is a bit vague. Perhaps "stat_branch_pair" or something would be more descriptive.

Show 6 quoted lines

> > Also, since this is an internal helper function for the file, we should
> > mark it as static.
> 
> Yes. In fact in the first version of the patch I would call
> `stat_compare_info` directly in `ref_filter.c` so I needed to export it in
> `remote.h`, and then when I changed the patch I forgot to make it static.

Heh. I wondered if that might have been the reason.

Show 5 quoted lines

> > Thanks for working on this.
> 
> You are welcome. What's the standard way to acknowledge your help in
> the Foo-By: trailers? I did not put a Reviewed-By: because you reviewed the
> previous patch, not the current one :)

Right, Reviewed-by wouldn't be quite right. As Christian noted, Helped-by can be used for this (but I am also fine without credit; suggestions are a normal part of review).

Overall the patch looks good to me. I have a few extremely minor nits:

> Subject: [v2 PATCH 1/1] Fix %(push:track) in ref-filter

We'd usually say "area: do something" here, and it's nice to stay consistent so that reading --oneline output is easy. And it's nice if we can avoid vague terms like "fix". Maybe:

 ref-filter: use correct branch for %(push:track)

or something?

Show 5 quoted lines

> This bug was not detected in t/t6300-for-each-ref.sh because in the test
> for push:track, both the upstream and the push branches were behind by 1
> from the local branch. Change the test so that the upstream branch is
> behind by 1 while the push branch is ahead by 1. This allows us to test
> that %(push:track) refer to the correct branch.

s/refer/&s/

> This change the expected value of some following tests (by introducing
> new references), so update them too.

s/change/&s/

>  	if (abf != AHEAD_BEHIND_FULL)
> -		BUG("stat_tracking_info: invalid abf '%d'", abf);
> +		BUG("stat_compare_info: invalid abf '%d'", abf);

If we do the name change I mentioned above, don't forger this line. :)

> +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
> +		       const char **upstream_name, int for_push,
> +		       enum ahead_behind_flags abf)
> +{

Is it worth changing "upstream_name" since it sometimes is now not %(upstream)?

Show 6 quoted lines

> @@ -1977,7 +2003,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  	char *base;
>  	int upstream_is_gone = 0;
>  
> -	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, abf);
> +	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);

I was tempted to suggest doing this refactor as a separate patch, so that we'd see less noise in the diff. But in fact half of the callers we'd have to touch are ones that would be modified to use for_push anyway. So I think it makes sense to just keep it all together as a single unit.

Show 10 quoted lines

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> [...]
> @@ -594,6 +603,7 @@ $(git rev-parse refs/tags/bogo) <committer@example.com> refs/tags/bogo
>  $(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master
>  EOF
>  
> +
>  test_expect_success 'Verify sort with multiple keys' '
>  	git for-each-ref --format="%(objectname) %(taggeremail) %(refname)" --sort=objectname --sort=taggeremail \
>  		refs/tags/bogo refs/tags/master > actual &&

Leftover stray whitespace?

For any one of those nits I'd probably say it was not worth a re-roll (and the maintainer could adjust them when he picks up the patch). But there are just enough that it's probably worth making his life easier with a v3.

You can put my Reviewed-by on it, too. :)

-Peff