Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> writes:
>> +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.
--
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