Re: [RFC/PATCH v2] pull: add --set-upstream

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2016, 19:06 UTC
Message-ID: <xmqqoa7ew319.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <vpqvb1mgvn5.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 9 quoted lines

>> +test_config_unchanged () {
>> +	git config --list --local >original
>> +	"$@"
>> +	git config --list --local >modified
>> +	test_cmp original modified
>> +}
>
> The test passes if "$@" fails. You should &&-chain the lines here to
> catch things like crashes or unexpected "exit 1" in git.

That is true, but allowing "$@" failure may be deliberate. After all, "git pull -u there that", when pulling that from there fails, may not want to update the upstream tracking information.

But I am unhappy with a more serious problem with the tests in this patch. They assume that "-u" option will forever be the only thing that is allowed to modify the configuration during "git pull -u". It should never make such an assumption.

The only thing these additional tests later in the patch (ommitted) want to check, if I understand them correctly, is that when -u is used on a ref that shouldn't be tracked from the given remote then remote.<that remote>.merge etc. are not updated. Make a list of the configuration variables the feature cares about, and check them and ignore changes to any other variable. Somebody else's feature that will be added to "git pull" may have legitimate reason to update configuration variables that are not releated to this feature, and you shouldn't be writing your test for your feature in such a way to forbid such a new feature by others from being added.