From: Junio C Hamano <gitster@pobox.com>
To: "nsengaw4c via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, nsengaw4c <nsengiyumvawilberforce@gmail.com>,
Christian Couder <christian.couder@gmail.com>,
Hariom Verma <hariom18599@gmail.com>
Subject: Re: [PATCH v3] [OUTREACHY] t1002: modernize outdated conditional
Date: Fri, 14 Oct 2022 09:15:16 -0700 [thread overview]
Message-ID: <xmqqv8om9yaz.fsf@gitster.g> (raw)
In-Reply-To: <pull.1362.v3.git.git.1665734502591.gitgitgadget@gmail.com> (nsengaw4c via GitGitGadget's message of "Fri, 14 Oct 2022 08:01:42 +0000")
"nsengaw4c via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
next prev parent reply other threads:[~2022-10-14 16:15 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-14 2:06 [PATCH] [OUTREACHY] t1002: modernize outdated conditional nsengaw4c via GitGitGadget
2022-10-14 5:05 ` Junio C Hamano
2022-10-14 7:47 ` [PATCH v2] " nsengaw4c via GitGitGadget
2022-10-14 8:01 ` [PATCH v3] " nsengaw4c via GitGitGadget
2022-10-14 16:15 ` Junio C Hamano [this message]
2022-10-14 16:21 ` Derrick Stolee
2022-10-14 16:58 ` Eric Sunshine
2022-10-14 17:54 ` Derrick Stolee
2022-10-14 18:57 ` Junio C Hamano
2022-10-14 19:06 ` Junio C Hamano
2022-10-14 19:14 ` Eric Sunshine
2022-10-14 20:41 ` Junio C Hamano
2022-10-14 20:19 ` Philip Oakley
2022-10-14 20:42 ` Junio C Hamano
2022-10-14 18:28 ` [PATCH v4] " nsengaw4c via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqv8om9yaz.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=hariom18599@gmail.com \
--cc=nsengiyumvawilberforce@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).