patch[OUTREACHY] t1002: modernize outdated conditional

From: 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  wilberfore <nsengiyumvawilberforce@gmail.com>
---
    [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
    christian.couder@gmail.com, Hariom verma hariom18599@gmail.com
    Signed-off-by: wilberforce nsengiyumvawilberforce@gmail.com

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

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

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

This name must match ...

... the name you sign-off your work with.

OK.

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  Wilberfore <nsengiyumvawilberforce@gmail.com>
---
    [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
    christian.couder@gmail.com, Hariom verma hariom18599@gmail.com
    Signed-off-by: wilberforce nsengiyumvawilberforce@gmail.com

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

Range-diff vs v1:

 1:  b2ed686bc94 ! 1:  8a9cd66d7d9 [OUTREACHY] t1002: modernize outdated conditional
     @@
       ## Metadata ##
     -Author: wilberforce <nsengiyumvawilberforce@gmail.com>
     +Author: Nsengiyumva Wilberforce <nsengiyumvawilberforce@gmail.com>
      
       ## Commit message ##
          [OUTREACHY] t1002: modernize outdated conditional
     @@ Commit message
      
          Cc: Christian Couder  <christian.couder@gmail.com>
          Cc: Hariom Verma <hariom18599@gmail.com>
     -    Signed-off-by: Nsengiyumva  wilberfore <nsengiyumvawilberforce@gmail.com>
     +    Signed-off-by: Nsengiyumva  Wilberfore <nsengiyumvawilberforce@gmail.com>
      
       ## 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(-)

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>
---
    [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
    christian.couder@gmail.com, Hariom verma hariom18599@gmail.com
    Signed-off-by: wilberforce nsengiyumvawilberforce@gmail.com

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  <christian.couder@gmail.com>
          Cc: Hariom Verma <hariom18599@gmail.com>
     -    Signed-off-by: Nsengiyumva  Wilberfore <nsengiyumvawilberforce@gmail.com>
     +    Signed-off-by: Nsengiyumva  Wilberforce <nsengiyumvawilberforce@gmail.com>
      
       ## 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(-)

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

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.

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.

On 10/14/2022 12:15 PM, Junio C Hamano 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.

Thanks, -Stolee

On Fri, Oct 14, 2022 at 12:35 PM Derrick Stolee <derrickstolee@github.com> wrote:

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?

On 10/14/2022 12:58 PM, Eric Sunshine wrote:

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

Derrick Stolee <derrickstolee@github.com> writes:

Good advice.

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

On Fri, Oct 14, 2022 at 3:06 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

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

On 14/10/2022 20:06, Junio C Hamano wrote:

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

Philip Oakley <philipoakley@iee.email> writes:

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.

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

Signed-off-by: Nsengiyumva  Wilberforce <nsengiyumvawilberforce@gmail.com>
---
    [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
    christian.couder@gmail.com, Hariom verma hariom18599@gmail.com
    Signed-off-by: wilberforce nsengiyumvawilberforce@gmail.com

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

Range-diff vs v3:

 1:  d019ce50dc9 ! 1:  c0109d947d4 [OUTREACHY] t1002: modernize outdated conditional
     @@ Commit message
      
              ! expression
      
     -    Cc: Christian Couder  <christian.couder@gmail.com>
     -    Cc: Hariom Verma <hariom18599@gmail.com>
          Signed-off-by: Nsengiyumva  Wilberforce <nsengiyumvawilberforce@gmail.com>
      
       ## t/t1002-read-tree-m-u-2way.sh ##

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