Re: [PATCH v3 3/7] t3422: new testcases for checking when incompatible options passed

[Elijah Newren <newren@gmail.com>]

On Thu, Jun 21, 2018 at 1:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> +     git checkout B &&
>> +     # This is indented with HT SP HT.
>> +     echo "          foo();" >>foo &&
>
> I often wonder, whenever I see a need for a comment like this, if
> saying the same thing in code to make it visible is cleaner and less
> error prone way to do so, i.e. e.g.
>
>         echo "_ _foo();" | tr "_" "\011" >>foo &&

I can make that change.  Should I make the same change to
t4015-diff-whitespace.sh, where I copied that comment and line of code
from?  (In a different topic submission, of course.)

>> +# Rebase has lots of useful options like --whitepsace=fix, which are
>> +# actually all built in terms of flags to git-am.  Since neither
>> +# --merge nor --interactive (nor any options that imply those two) use
>> +# git-am, using them together will result in flags like --whitespace=fix
>> +# being ignored.  Make sure rebase warns the user and aborts instead.
>> +#
>> +
>> +test_run_rebase () {
>> +     opt=$1
>> +     shift
>> +     test_expect_failure "$opt incompatible with --merge" "
>> +             git checkout B^0 &&
>> +             test_must_fail git rebase $opt --merge A
>> +     "
>> +
>> +     test_expect_failure "$opt incompatible with --strategy=ours" "
>> +             git checkout B^0 &&
>> +             test_must_fail git rebase $opt --strategy=ours A
>> +     "
>> +
>> +     test_expect_failure "$opt incompatible with --strategy-option=ours" "
>> +             git checkout B^0 &&
>> +             test_must_fail git rebase $opt --strategy=ours A
>> +     "
>> +
>> +     test_expect_failure "$opt incompatible with --interactive" "
>> +             git checkout B^0 &&
>> +             test_must_fail git rebase $opt --interactive A
>> +     "
>> +
>> +     test_expect_failure "$opt incompatible with --exec" "
>> +             git checkout B^0 &&
>> +             test_must_fail git rebase $opt --exec 'true' A
>> +     "
>> +
>> +}
>> +
>> +test_run_rebase --whitespace=fix
>> +test_run_rebase --ignore-whitespace
>> +test_run_rebase --committer-date-is-author-date
>> +test_run_rebase -C4
>
> I happen to be from old school and "rebase" primarily means
> "format-patch piped to am" in my mind, so from that point of view,
> "test_run_rebase --OPT" that says "--OPT which is a valid option for
> the primary operating mode of rebase does not work with the other
> exotic modes of the command" is not all that bad, but I do not think
> that worldview holds for many people in general.  Perhaps calling it
> something like "test_rebase_am_only" makes the intent clearer?

Well, if that's what rebase means in your mind, then I'm sure you'll
have fun commenting on my follow-on series.  :-)

Anyway, sure, I'm happy to change the function name.