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

> 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?

>
> > +	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?