Re: [PATCH v3 1/2] t7501: add tests for --include and --only

[Junio C Hamano <gitster@pobox.com>]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> -# FIXME: Test the various index usages, -i and -o, test reflog,
> +# fixme: test the various index usages, test reflog,
>  # signoff

Why the sudden downcasing?  If this were to turn it to "TODO:"
(110+) or "NEEDSWORK:" (110+) from less often used "FIXME:" (40-),
such a change might be defensible, but there is only one instance
of downcased "fixme:", so I am curious how this happened.

> +test_expect_success '--include fails with untracked files' '
> +	echo content >baz &&
> +	test_must_fail git commit --include -m "initial" baz
> +'

OK, this is because "--allow-empty" is not passed.  This should fail
without -i/-o (which should be the same as passing -o), with -i, or
with -o.

Calling this commit "initial" is highly misleading.  There are bunch
of commits already, but path "baz" has never been used.

> +test_expect_success '--include with staged changes' '
> +	echo newcontent >baz &&
> +	echo newcontent >file &&
> +	git add file &&
> +	git commit --include -m "file baz" baz  &&

I suspect that you found a bug whose behaviour we do not want to
carve into stone with this test.  When this test begins, because the
previous step failed to create the initial commit, we are creating
the root commit, without --allow-empty, with contents in the index
for path "file".  At this point

    $ git commit -m "file baz" baz

(or with "-o", which is the same thing) does error out with

    error: pathspec 'baz' did not match any file(s) known to git

because the "only" thing is to take the changes between HEAD and the
index and limit them further to those paths that match "baz", but
there is no path that matches "baz".

This command

    $ git commit -m "file baz" -i baz

should either error out the same way, or behave more or less[*] like

    $ git add baz && git commit -m "file baz"

and record changes to both "file" and "baz".

    Side note: The "more or less" is we should do "git rm baz"
               instead, if you removed the path.

But it seems that the current code simply ignores the unmatching
pathspec "baz" that is on the command line, hence recording only the
change to the contents of "file".

> +	git diff --name-only >remaining &&
> +	test_must_be_empty remaining &&
> +	git diff --name-only --staged >remaining &&
> +	test_must_be_empty remaining

These two tests to see if the working tree is clean and if the index
is clean are not wrong per-se, but is insufficient.  Judging from
the commit message you used, I think you expected this commit to
contain both changes to 'file' and 'baz'.  That needs to be also
checked with something like "git diff --name-only HEAD^ HEAD".

Now which behaviour between "error out because there is no path in
the index that matches pathspec 'baz'" and "add baz to the index and
commit that addition, together with what is already in the index" we
would want to take is probably open for discussion.  Such a discussion
may decide that the current behaviour is fine.  Until then...

> +test_expect_success '--only fails with untracked files' '
> +	echo content >newfile &&
> +	test_must_fail git commit --only -m "newfile" newfile
> +'

And this should fail the same way without "-o".  Don't we have such
a test that we can just sneak "with -o the same thing happens" test
next to it?

> +test_expect_success '--only excludes staged changes' '
> +	echo content >file &&
> +	echo content >baz &&
> +	git add baz &&
> +	git commit --only -m "file" file &&

This should behave exactly the same way without "-o".

> +	git diff --name-only >actual &&
> +	test_must_be_empty actual &&
> +	git diff --name-only --staged >actual &&
> +	test_grep "baz" actual
> +'
> +
> +test_expect_success '--include and --only do not mix' '
> +	test_when_finished "git reset --hard" &&
> +	echo new >file &&
> +	echo new >baz &&
> +	test_must_fail git commit --include --only -m "bar" bar baz

OK.  If you grep for 'cannot be used together' in t/ directory,
there are many tests that make sure how an invocation like this
should fail, i.e. with an error message that mentions incompatible
options.  Don't we want to do the same?

Thanks.