Skip to contentSkip to conversation

t1002: modernize outdated conditional

PATCH Superseded nsengaw4c via GitGitGadget 5 participants 15 messages Oct 14, 2022

lore
Share & tools
lore

nsengaw4c via GitGitGadget

author
#0 lore raw

PATCH 1/1 t1002: modernize outdated conditional

From: wilberforce <[email protected]>

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 <[email protected]>
Cc Hariom Verma <[email protected]>
Signed-off-by Nsengiyumva wilberfore <[email protected]>

---
[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 [email protected], Hariom verma [email protected]

Signed-off-by wilberforce [email protected]

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(-)
t/t1002-read-tree-m-u-2way.sh +8 -8
index bd5313caec9..cdc077ce12d 100755
@@ -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 \

base-commit: d420dda0576340909c3faff364cfbd1485f70376

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 1 line · "nsengaw4c via GitGitGadget" <[email protected]> writes:

From: wilberforce <[email protected]>

This name must match ...

Quoted 5 lines

Tests in this script use an unusual and hard to reason about conditional construct

if expression; then false; else :; fi

Quoted 7 lines

Change them to use more idiomatic construct:

! expression

Cc Christian Couder <[email protected]>
Cc Hariom Verma <[email protected]>
Signed-off-by Nsengiyumva wilberfore <[email protected]>

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

Review

Quoted 2 lines
(quoted diff) +1 -1
-     if read_tree_u_must_succeed -m -u $treeH $treeM; then false; else :; fi'
+     ! read_tree_u_must_succeed -m -u $treeH $treeM'

OK.

author
#2 lore raw

PATCH v2 1/1 t1002: modernize outdated conditional

In reply to nsengaw4c via GitGitGadget

From: Nsengiyumva Wilberforce <[email protected]>

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 <[email protected]>
Cc Hariom Verma <[email protected]>
Signed-off-by Nsengiyumva Wilberfore <[email protected]>

---
[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 [email protected], Hariom verma [email protected]

Signed-off-by wilberforce [email protected]

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 <[email protected]>
+Author: Nsengiyumva Wilberforce <[email protected]>

## Commit message ##
[OUTREACHY] t1002: modernize outdated conditional
@@ Commit message

Cc: Christian Couder <[email protected]>
Cc: Hariom Verma <[email protected]>
- Signed-off-by: Nsengiyumva wilberfore <[email protected]>
+ Signed-off-by: Nsengiyumva Wilberfore <[email protected]>

## 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(-)
t/t1002-read-tree-m-u-2way.sh +8 -8
index bd5313caec9..cdc077ce12d 100755
@@ -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 \

base-commit: d420dda0576340909c3faff364cfbd1485f70376

Signature · 1 line
gitgitgadget
author
#3 lore raw

PATCH v3 1/1 t1002: modernize outdated conditional

From: Nsengiyumva Wilberforce <[email protected]>

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 <[email protected]>
Cc Hariom Verma <[email protected]>
Signed-off-by Nsengiyumva Wilberforce <[email protected]>

---
[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 [email protected], Hariom verma [email protected]

Signed-off-by wilberforce [email protected]

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 <[email protected]>
Cc: Hariom Verma <[email protected]>
- Signed-off-by: Nsengiyumva Wilberfore <[email protected]>
+ Signed-off-by: Nsengiyumva Wilberforce <[email protected]>

## 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(-)
t/t1002-read-tree-m-u-2way.sh +8 -8
index bd5313caec9..cdc077ce12d 100755
@@ -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 \

base-commit: d420dda0576340909c3faff364cfbd1485f70376

Signature · 1 line
gitgitgadget
#4 lore raw
Quoted 7 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:

From: Nsengiyumva Wilberforce <[email protected]>

Tests in this script use an unusual and hard to reason about conditional construct

if expression; then false; else :; fi

Quoted 7 lines

Change them to use more idiomatic construct:

! expression

Cc Christian Couder <[email protected]>
Cc Hariom Verma <[email protected]>
Signed-off-by Nsengiyumva Wilberforce <[email protected]>

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.

Review

Quoted 3 lines

...

(quoted diff) +1 -1
-     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.

#5 lore raw
Quoted 17 lines · Junio C Hamano
Quoted 7 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:

From: Nsengiyumva Wilberforce <[email protected]>

Tests in this script use an unusual and hard to reason about conditional construct

if expression; then false; else :; fi

Quoted 7 lines

Change them to use more idiomatic construct:

! expression

Cc Christian Couder <[email protected]>
Cc Hariom Verma <[email protected]>
Signed-off-by Nsengiyumva Wilberforce <[email protected]>
Quoted 5 lines

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

#6 lore raw
Quoted 11 lines · Derrick Stolee
Quoted 9 lines · Junio C Hamano
Quoted 2 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:
Cc Christian Couder <[email protected]>
Cc Hariom Verma <[email protected]>

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.

Quoted 3 lines

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?

#7 lore raw
Quoted 17 lines · Eric Sunshine
Quoted 11 lines · Derrick Stolee
Quoted 9 lines · Junio C Hamano
Quoted 2 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:
Cc Christian Couder <[email protected]>
Cc Hariom Verma <[email protected]>

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.

Quoted 3 lines

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.

Quoted 6 lines

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

#8 lore raw
Quoted 10 lines · Derrick Stolee

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.

Quoted 2 lines

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.

#9 lore raw
In reply to Eric Sunshine
Quoted 17 lines · Eric Sunshine
Quoted 11 lines · Derrick Stolee
Quoted 9 lines · Junio C Hamano
Quoted 2 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:
Cc Christian Couder <[email protected]>
Cc Hariom Verma <[email protected]>

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

#10 lore raw
Quoted 14 lines · Junio C Hamano
Quoted 6 lines · Eric Sunshine
Quoted 3 lines · Derrick Stolee

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.

Quoted 6 lines

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?

Quoted 5 lines

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.

#11 lore raw
Quoted 5 lines · Eric Sunshine

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

#12 lore raw
In reply to Junio C Hamano
Quoted 22 lines · Junio C Hamano
Quoted 14 lines · Eric Sunshine
Quoted 4 lines · Derrick Stolee
Quoted 3 lines · Junio C Hamano
Quoted 2 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:
Cc Christian Couder <[email protected]>
Cc Hariom Verma <[email protected]>
Quoted 8 lines
Quoted 5 lines

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.

Quoted 6 lines

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?

Quoted 6 lines

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

#13 lore raw
Quoted 5 lines · Philip Oakley

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.

author
#14 lore raw

PATCH v4 1/1 t1002: modernize outdated conditional

In reply to nsengaw4c via GitGitGadget

From: Nsengiyumva Wilberforce <[email protected]>

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 <[email protected]>

---
[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 [email protected], Hariom verma [email protected]

Signed-off-by wilberforce [email protected]

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 <[email protected]>
- Cc: Hariom Verma <[email protected]>

Signed-off-by Nsengiyumva Wilberforce <[email protected]>

## t/t1002-read-tree-m-u-2way.sh ##

 t/t1002-read-tree-m-u-2way.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
t/t1002-read-tree-m-u-2way.sh +8 -8
index bd5313caec9..cdc077ce12d 100755
@@ -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 \

base-commit: d420dda0576340909c3faff364cfbd1485f70376

Signature · 1 line
gitgitgadget