Re: [PATCH 4/7] push: introduce new push.default mode "simple"

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Junio C Hamano <gitster@pobox.com> writes:

>> -static void setup_push_upstream(struct remote *remote)
>> +NORETURN die_push_simple(struct branch *branch, struct remote *remote) {
>
> Not static?

fixed.

> Unless you change behaviour depending on NULL-ness of this variable
> later in this code (and I do not think you do---this is only for a
> message string as far as I can see), I'd prefer to see that ?: you have
> at the use site here instead, i.e.
>
> 	if (!short_up)
> 		short_up = branch->merge[0]->src;

applied.

> perhaps with s/short_up/dest_branch/ or something.

Hmm, not really. It's a candidate destination branch, but we'll use the
variable only if the push fails because there is another candidate.

I did s/short_up/short_upstream/ to make it clearer.

>> +	if (simple && strcmp(branch->refname, branch->merge[0]->src)) {
>> +		die_push_simple(branch, remote);
>> +	}
>
> Lose unnecessary {} pair, perhaps?

Yes, removed.

>> +	git --git-dir=repo1 log -1 --format="%h %s" "other-name" >expect-other-name &&
>> +	test_push_success current master &&
>> +	git --git-dir=repo1 log -1 --format="%h %s" "other-name" >actual-other-name &&
>> +	test_cmp expect-other-name actual-other-name
>
> Hrm.
>
> There is nothing wrong in the above part, but it shows taht it would be
> very nice if test_push_success helper also encapsulated the "make sure
> others did not change" logic.

The issue is that one has to define "others", and it is different for
current and upstream so we'd have to add arguments to specify that to
test_push_success. I'd rather keep the helpers API simple, and
special-case when needed as we did here.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/