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

From: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Date: Jan 13, 2024, 01:00 UTC
Message-ID: <CYD67AE9V26S.YEDQYOHZJV9M@gmail.com>
In-Reply-To: <xmqqbk9q6tcs.fsf@gitster.g>

On Sat Jan 13, 2024 at 4:40 AM IST, Junio C Hamano wrote:

Show 36 quoted lines

> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
> >  	test_must_fail git commit -m initial --long
> >  '
> >  
> > +test_expect_success 'fail to commit untracked file' '
> > +	echo content >baz &&
> > +	test_must_fail git commit -m "baz" baz
> > +'
> > +
> > +test_expect_success '--only also fail to commit untracked file' '
> > +	test_must_fail git commit --only -m "baz" baz
> > +'
> > +
> > +test_expect_success '--include also fail to commit untracked file' '
> > +	test_must_fail git commit --include -m "baz" baz
> > +'
>
> As the latter two depends on the first one's side effect of leaving
> an untracked 'baz' file in the working tree, I do not know if it is
> sensible to split these into three tests.  An obvious alternative is
> to have a single test
>
> 	test_expect_success 'pathspec that do not match any tracked path' '
> 		echo content >baz &&
> 		test_must_fail git commit -m baz baz &&
> 		test_must_fail git commit -o -m baz baz &&
> 		test_must_fail git commit -i -m baz baz
> 	'
>
> By the way, I do not think presence of 'baz' in the working tree
> matters in the failures from these tests all that much, as the
> reason they fail is because the pathspec does not match any tracked
> file, whose contents in the index to be updated before made into a
> commit.

Yes, that is true. However, as per your prior email in which you stated about --include:

    "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..."

If in such a discussion it is decided that -i should add to index and commit, then by not having 'baz' in the working tree, the -i test would still error out regardless of whether its behaviour is to [add to the index and commit] or [error out]. Therefore, by having 'baz' we can detect the change between [-i adds to index and commits] or [errors out].

Show 22 quoted lines

> Likewise.  An obvious thing to notice is that this cannot use the
> same "contents" text as before, even though it claims to be "same as
> above".  If the final contents of "file" and "baz" does not matter,
> but it matters more that these files have been changed, it often is
> a good idea to append to the file.  That way, you can ensure that
> you will be making them different, no matter what the initial
> condition was, i.e.,
>
> 	for opt in "" "-o" "--only"
> 	do
> 		test_expect_success 'skip over already added change' '
> 			echo more >>file &&
> 			echo more >>baz &&
> 			git add baz &&
> 			git commit $opt -m "file" file &&
>
> 			... ensure that changes to file are committed
> 			... and changes to baz is only in the index
> 		'
> 	done
>
> let's you test all three combinations.

Yeah, that is a more effective approach. I will change it and reroll quickly.

>
> Thanks.

Thank you for all the help!