Re: [PATCH v3] [OUTREACHY] t1002: modernize outdated conditional

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 14, 2022, 16:15 UTC
Message-ID: <xmqqv8om9yaz.fsf@gitster.g>
In-Reply-To: <pull.1362.v3.git.git.1665734502591.gitgitgadget@gmail.com>

"nsengaw4c via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

> From: Nsengiyumva Wilberforce <nsengiyumvawilberforce@gmail.com>
>
> Tests in this script use an unusual and hard to reason about
> conditional construct
>
>     if expression; then false; else :; fi
>
> Change them to use more idiomatic construct:
>
>     ! expression
>
> Cc: Christian Couder  <christian.couder@gmail.com>
> Cc: Hariom Verma <hariom18599@gmail.com>
> Signed-off-by: Nsengiyumva  Wilberforce <nsengiyumvawilberforce@gmail.com>

What are these C: lines for? I do not think the message I am responding to is Cc'ed to them. There may be a special incantation to tell GitGitGadget to Cc to certain folks, but adding Cc: to the log message trailer like this does not seem to be it---at least it appears that it did not work that way.

> ...
> -     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
> +     ! read_tree_u_must_succeed -m -u $treeH $treeM'

Looks good. For the purpose of microproject, I think this is a good place to stop, as it does not make anything worse and make the code prettier.

To those more experienced contributors who are watching from sidelines, and especially to our mentors, it may be worth taking a look at the implementation of the helper shell function used here, and think if it makes sense to expect a failure with a simple "!" prefix (or with the original long hand if/then/else/fi that has exactly the same issue).

read_tree_u_must_succeed () {
	git ls-files -s >pre-dry-run &&
	git diff-files -p >pre-dry-run-wt &&
	git read-tree -n "$@" &&
	git ls-files -s >post-dry-run &&
	git diff-files -p >post-dry-run-wt &&
	test_cmp pre-dry-run post-dry-run &&
	test_cmp pre-dry-run-wt post-dry-run-wt &&
	git read-tree "$@"
}

What if read-tree segfaults? This entire function will fail and the test that runs read_tree_u_must_succeed and negates its result would be a poor fit here.

Thanks.