[v3,OUTREACHY] t1002: modernize outdated conditional

Commit Message

Nsengiyumva Wilberforce Oct. 14, 2022, 8:01 a.m. UTC

From: Nsengiyumva Wilberforce <[e-mail]>

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  <[e-mail]>
Cc: Hariom Verma <[e-mail]>
Signed-off-by: Nsengiyumva  Wilberforce <[e-mail]>
---
    [OUTREACHY]cleaning t1002-read-tree-m-u-2way.sh
    
    This is an update in t1002-read-tree-m-u-2way.sh. all the tests that use
    the unusual construct: if read_tree_u_must_succeed -m -u $treeH $treeM;
    then false; else :; fi have been updated to ! read_tree_u_must_succeed
    -m -u $treeH $treeM "I am an outreachy applicant" CC: Christian Couder
    [e-mail], Hariom verma [e-mail]
    Signed-off-by: wilberforce [e-mail]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1362%2Fnsengiyumva-wilberforce%2Ft1002_usual_construct_updated-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1362/nsengiyumva-wilberforce/t1002_usual_construct_updated-v3
Pull-Request: https://github.com/git/git/pull/1362

Range-diff vs v2:

 1:  8a9cd66d7d9 ! 1:  d019ce50dc9 [OUTREACHY] t1002: modernize outdated conditional
     @@ Commit message
      
          Cc: Christian Couder  <[e-mail]>
          Cc: Hariom Verma <[e-mail]>
     -    Signed-off-by: Nsengiyumva  Wilberfore <[e-mail]>
     +    Signed-off-by: Nsengiyumva  Wilberforce <[e-mail]>
      
       ## t/t1002-read-tree-m-u-2way.sh ##
      @@ t/t1002-read-tree-m-u-2way.sh: test_expect_success \


 t/t1002-read-tree-m-u-2way.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)


base-commit: d420dda0576340909c3faff364cfbd1485f70376

Comments

Junio C Hamano Oct. 14, 2022, 4:15 p.m. UTC | #1

"nsengaw4c via GitGitGadget" <[e-mail]> writes:

> From: Nsengiyumva Wilberforce <[e-mail]>
>
> 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  <[e-mail]>
> Cc: Hariom Verma <[e-mail]>
> Signed-off-by: Nsengiyumva  Wilberforce <[e-mail]>

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.

Derrick Stolee Oct. 14, 2022, 4:21 p.m. UTC | #2

On 10/14/2022 12:15 PM, Junio C Hamano wrote:
> "nsengaw4c via GitGitGadget" <[e-mail]> writes:
> 
>> From: Nsengiyumva Wilberforce <[e-mail]>
>>
>> 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  <[e-mail]>
>> Cc: Hariom Verma <[e-mail]>
>> Signed-off-by: Nsengiyumva  Wilberforce <[e-mail]>
> 
> 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.

GitGitGadget will read the "cc:" lines from the end of the pull request
description, not the commit messages. I'm pretty sure they will be
ignored if there are other lines after them.

Thanks,
-Stolee

Eric Sunshine Oct. 14, 2022, 4:58 p.m. UTC | #3

On Fri, Oct 14, 2022 at 12:35 PM Derrick Stolee
<[e-mail]> wrote:
> On 10/14/2022 12:15 PM, Junio C Hamano wrote:
> > "nsengaw4c via GitGitGadget" <[e-mail]> writes:
> >> Cc: Christian Couder  <[e-mail]>
> >> Cc: Hariom Verma <[e-mail]>
> >
> > 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.
>
> GitGitGadget will read the "cc:" lines from the end of the pull request
> description, not the commit messages. I'm pretty sure they will be
> ignored if there are other lines after them.

For Wilberforce's edification for future submissions, presumably the
reason that the CC: in the pull-request's description didn't work is
because the CC: line wasn't the last line in the description? Does
there need to be a blank line before the CC: line? Is it okay to list
multiple people on the same CC: line as done in this case, or is that
also a problem?

Derrick Stolee Oct. 14, 2022, 5:54 p.m. UTC | #4

On 10/14/2022 12:58 PM, Eric Sunshine wrote:
> On Fri, Oct 14, 2022 at 12:35 PM Derrick Stolee
> <[e-mail]> wrote:
>> On 10/14/2022 12:15 PM, Junio C Hamano wrote:
>>> "nsengaw4c via GitGitGadget" <[e-mail]> writes:
>>>> Cc: Christian Couder  <[e-mail]>
>>>> Cc: Hariom Verma <[e-mail]>
>>>
>>> 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.
>>
>> GitGitGadget will read the "cc:" lines from the end of the pull request
>> description, not the commit messages. I'm pretty sure they will be
>> ignored if there are other lines after them.
> 
> For Wilberforce's edification for future submissions, presumably the
> reason that the CC: in the pull-request's description didn't work is
> because the CC: line wasn't the last line in the description? Does
> there need to be a blank line before the CC: line? Is it okay to list
> multiple people on the same CC: line as done in this case, or is that
> also a problem?

Looking at the PR (https://github.com/git/git/pull/1362) it seems
there was no "cc:" lines in the PR description (until GitGitGadget
added them due to our replies).

Nsengiyumva: you'll want to be careful to edit your pull request
description on GitHub before running the "/submit" chatop. Your
current description has a paste of your commit message followed by
the contributing template. The pull request description becomes
your cover letter (in the case of multiple patches) or a commentary
section after the commit message (in this case of a single patch).

The description is a good place to say things like "I started
working on this because of a mailing list thread..." or "I'm not
sure if I've tested everything enough".

The "cc:" lines should _not_ be in the commit message at all, which
is what's visible in the patch.

Thanks,
-Stolee

Junio C Hamano Oct. 14, 2022, 6:57 p.m. UTC | #5

Derrick Stolee <[e-mail]> writes:

> Nsengiyumva: you'll want to be careful to edit your pull request
> description on GitHub before running the "/submit" chatop. Your
> current description has a paste of your commit message followed by
> the contributing template. The pull request description becomes
> your cover letter (in the case of multiple patches) or a commentary
> section after the commit message (in this case of a single patch).
>
> The description is a good place to say things like "I started
> working on this because of a mailing list thread..." or "I'm not
> sure if I've tested everything enough".

Good advice.

> The "cc:" lines should _not_ be in the commit message at all, which
> is what's visible in the patch.

I agree that it would probably be better without CC: in the trailer
in this case.  Some projects seem to use the CC: in the trailer to
signal that these people have been notified, without implying
anything about their reaction (i.e. when the author cannot use
reviewed-by or acked-by).  We are not that large a community, so I
personally do not see a need to use such a trailer around here.  But
that is only a local convention in this project.

Thanks.

Junio C Hamano Oct. 14, 2022, 7:06 p.m. UTC | #6

Eric Sunshine <[e-mail]> writes:

> On Fri, Oct 14, 2022 at 12:35 PM Derrick Stolee
> <[e-mail]> wrote:
>> On 10/14/2022 12:15 PM, Junio C Hamano wrote:
>> > "nsengaw4c via GitGitGadget" <[e-mail]> writes:
>> >> Cc: Christian Couder  <[e-mail]>
>> >> Cc: Hariom Verma <[e-mail]>
>> >
>> > 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.
>>
>> GitGitGadget will read the "cc:" lines from the end of the pull request
>> description, not the commit messages. I'm pretty sure they will be
>> ignored if there are other lines after them.
>
> For Wilberforce's edification for future submissions, presumably the
> reason that the CC: in the pull-request's description didn't work is
> because the CC: line wasn't the last line in the description? Does
> there need to be a blank line before the CC: line? Is it okay to list
> multiple people on the same CC: line as done in this case, or is that
> also a problem?

Ah, now I can see why the round v4 is CC'ed to you and Derrick on
the list.  The pull-request text (visible in GitHub UI in the top
most box of https://github.com/git/git/pull/1362) ends with two
lines of cc: that list you two.  The one named Christian and Hariom
were not at the end and was ignored by GGG, it seems.

Eric Sunshine Oct. 14, 2022, 7:14 p.m. UTC | #7

On Fri, Oct 14, 2022 at 3:06 PM Junio C Hamano <[e-mail]> wrote:
> Eric Sunshine <[e-mail]> writes:
> > On Fri, Oct 14, 2022 at 12:35 PM Derrick Stolee
> > <[e-mail]> wrote:
> >> GitGitGadget will read the "cc:" lines from the end of the pull request
> >> description, not the commit messages. I'm pretty sure they will be
> >> ignored if there are other lines after them.
> >
> > For Wilberforce's edification for future submissions, presumably the
> > reason that the CC: in the pull-request's description didn't work is
> > because the CC: line wasn't the last line in the description? Does
> > there need to be a blank line before the CC: line? Is it okay to list
> > multiple people on the same CC: line as done in this case, or is that
> > also a problem?
>
> Ah, now I can see why the round v4 is CC'ed to you and Derrick on
> the list.  The pull-request text (visible in GitHub UI in the top
> most box of https://github.com/git/git/pull/1362) ends with two
> lines of cc: that list you two.  The one named Christian and Hariom
> were not at the end and was ignored by GGG, it seems.

Yes, the CC: line mentioning Christian and Hariom was not at the end
of the description, which is likely why GitGitGadget didn't pick it
up. (Presumably Stolee overlooked that line when responding to my
question.) However, clarification about whether or not there needs to
be a blank line before the CC: line would be nice (I presume the blank
line is needed), but also whether or not GitGitGadget correctly deals
with multiple people mentioned on the same CC: line or if they each
need to occupy a single CC: line.

Philip Oakley Oct. 14, 2022, 8:19 p.m. UTC | #8

On 14/10/2022 20:06, Junio C Hamano wrote:
> Eric Sunshine <[e-mail]> writes:
>
>> On Fri, Oct 14, 2022 at 12:35 PM Derrick Stolee
>> <[e-mail]> wrote:
>>> On 10/14/2022 12:15 PM, Junio C Hamano wrote:
>>>> "nsengaw4c via GitGitGadget" <[e-mail]> writes:
>>>>> Cc: Christian Couder  <[e-mail]>
>>>>> Cc: Hariom Verma <[e-mail]>
>>>> 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.
>>> GitGitGadget will read the "cc:" lines from the end of the pull request
>>> description, not the commit messages. I'm pretty sure they will be
>>> ignored if there are other lines after them.
>> For Wilberforce's edification for future submissions, presumably the
>> reason that the CC: in the pull-request's description didn't work is
>> because the CC: line wasn't the last line in the description? Does
>> there need to be a blank line before the CC: line? Is it okay to list
>> multiple people on the same CC: line as done in this case, or is that
>> also a problem?
> Ah, now I can see why the round v4 is CC'ed to you and Derrick on
> the list.  The pull-request text (visible in GitHub UI in the top
> most box of https://github.com/git/git/pull/1362) ends with two
> lines of cc: that list you two.  The one named Christian and Hariom
> were not at the end and was ignored by GGG, it seems.
>
I just want to throw in that because GitHub takes the PR & comitts
verbatim, but Git itself works via email, you can add description
portions to commits, and I believe the PR part, by add a line containing
just three dashes `---` followed by the additional descriptive note text
which won't be used when `am` (apply mailbox) is used.

I've certainly used that technique when sending patches. See the "Bonus
Chapter: One-Patch Changes" in MyFirstContribution.txt
--
Philip

Junio C Hamano Oct. 14, 2022, 8:41 p.m. UTC | #9

Eric Sunshine <[e-mail]> writes:

> ... However, clarification about whether or not there needs to
> be a blank line before the CC: line would be nice (I presume the blank
> line is needed), but also whether or not GitGitGadget correctly deals
> with multiple people mentioned on the same CC: line or if they each
> need to occupy a single CC: line.

Indeed it is very good to have such a documentation that tells us
all these things.  Is the "Welcome to GGG" comment it adds to first
time users a good place to have this kind of information (I am
guessing not, as more advanced features may become needed after you
used the tool several times)?

Junio C Hamano Oct. 14, 2022, 8:42 p.m. UTC | #10

Philip Oakley <[e-mail]> writes:

> I just want to throw in that because GitHub takes the PR & comitts
> verbatim, but Git itself works via email, you can add description
> portions to commits, and I believe the PR part, by add a line containing
> just three dashes `---` followed by the additional descriptive note text
> which won't be used when `am` (apply mailbox) is used.

Yup, if you are absolutely sure you won't interact with others in
any way other than e-mailed patches, it is a great trick to use.

Patch

diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
index bd5313caec9..cdc077ce12d 100755
--- a/t/t1002-read-tree-m-u-2way.sh
+++ b/t/t1002-read-tree-m-u-2way.sh
@@ -154,7 +154,7 @@  test_expect_success \
      read_tree_u_must_succeed --reset -u $treeH &&
      echo frotz frotz >frotz &&
      git update-index --add frotz &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 test_expect_success \
     '9 - conflicting addition.' \
@@ -163,7 +163,7 @@  test_expect_success \
      echo frotz frotz >frotz &&
      git update-index --add frotz &&
      echo frotz >frotz &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 test_expect_success \
     '10 - path removed.' \
@@ -186,7 +186,7 @@  test_expect_success \
      echo rezrov >rezrov &&
      git update-index --add rezrov &&
      echo rezrov rezrov >rezrov &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 test_expect_success \
     '12 - unmatching local changes being removed.' \
@@ -194,7 +194,7 @@  test_expect_success \
      read_tree_u_must_succeed --reset -u $treeH &&
      echo rezrov rezrov >rezrov &&
      git update-index --add rezrov &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 test_expect_success \
     '13 - unmatching local changes being removed.' \
@@ -203,7 +203,7 @@  test_expect_success \
      echo rezrov rezrov >rezrov &&
      git update-index --add rezrov &&
      echo rezrov >rezrov &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 cat >expected <<EOF
 -100644 X 0	nitfol
@@ -251,7 +251,7 @@  test_expect_success \
      read_tree_u_must_succeed --reset -u $treeH &&
      echo bozbar bozbar >bozbar &&
      git update-index --add bozbar &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 test_expect_success \
     '17 - conflicting local change.' \
@@ -260,7 +260,7 @@  test_expect_success \
      echo bozbar bozbar >bozbar &&
      git update-index --add bozbar &&
      echo bozbar bozbar bozbar >bozbar &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 test_expect_success \
     '18 - local change already having a good result.' \
@@ -316,7 +316,7 @@  test_expect_success \
      echo bozbar >bozbar &&
      git update-index --add bozbar &&
      echo gnusto gnusto >bozbar &&
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'
 
 # Also make sure we did not break DF vs DF/DF case.
 test_expect_success \