| Message ID | e6a2628ce57668aa17101e73edaead0ef34d8a8c.1709590037.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | advise about ref syntax rules | expand |
Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > Also: > > • Remove a now-irrelevant comment about test placement and switch back > to `main` post-test. > • Prefer indented literal heredocs (`-\EOF`) except for a block which > says that this is intentional > • Move a `git config` command into the test and mark it as `setup` since > the next test depends on it > Especially the change to use "-\EOF" to make them align better caused too many tests to be touched, but overall the result may have become much easier to follow. Good job. > -mv .git/config .git/config-saved > - > test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' ' > + test_when_finished mv .git/config-saved .git/config && > + mv .git/config .git/config-saved && > git branch -m q q2 && > git branch -m q2 q > ' > > -mv .git/config-saved .git/config The above is a truly valuable clean-up. But I am not really sure if the paritcular condition is worth testing in the first place these days. No configuration file means we cannot even read the repository format version, and working under such a condition is quite a bad promise that we would rather not to having to keep. But that is an entirely different topic from what this patch is doing. > -git config branch.s/s.dummy Hello > - > -test_expect_success 'git branch -m s/s s should work when s/t is deleted' ' > +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' ' > + git config branch.s/s.dummy Hello && > git branch --create-reflog s/s && > git reflog exists refs/heads/s/s && > git branch --create-reflog s/t && I do not know if the change of the title is warranted. It is doing its own test, not just setup. It may be merely donw for the side effect of making the step unskippable, but still .... > -# Keep this test last, as it changes the current branch Yes, removal of this line is really appreciated ;-) > -cat >expect <<EOF > -$HEAD refs/heads/g/h/i@{0}: branch: Created from main > -EOF > test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' ' > + test_when_finished git checkout main && > GIT_COMMITTER_DATE="2005-05-26 23:30" \ > git checkout -b g/h/i -l main && > test_ref_exists refs/heads/g/h/i && > + cat >expect <<-EOF && > + $HEAD refs/heads/g/h/i@{0}: branch: Created from main > + EOF > git reflog show --no-abbrev-commit refs/heads/g/h/i >actual && > test_cmp expect actual > ' Thanks.
On Tue, Mar 5, 2024, at 02:25, Junio C Hamano wrote: > Especially the change to use "-\EOF" to make them align better > caused too many tests to be touched, but overall the result may have > become much easier to follow. Good job. I reckon that this can be worth doing now as long as no other topics in `next` or `seen` happen to touch the same code. What do you think? I can evict hunks if they happen to overlap with other in-flight topics. >> -mv .git/config .git/config-saved >> - >> test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' ' >> + test_when_finished mv .git/config-saved .git/config && >> + mv .git/config .git/config-saved && >> git branch -m q q2 && >> git branch -m q2 q >> ' >> >> -mv .git/config-saved .git/config > > The above is a truly valuable clean-up. > > But I am not really sure if the paritcular condition is worth > testing in the first place these days. No configuration file means > we cannot even read the repository format version, and working under > such a condition is quite a bad promise that we would rather not to > having to keep. But that is an entirely different topic from what > this patch is doing. Okay. I could undo this change and remove the test in its own commit? > >> -git config branch.s/s.dummy Hello >> - >> -test_expect_success 'git branch -m s/s s should work when s/t is deleted' ' >> +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' ' >> + git config branch.s/s.dummy Hello && >> git branch --create-reflog s/s && >> git reflog exists refs/heads/s/s && >> git branch --create-reflog s/t && > > I do not know if the change of the title is warranted. It is doing > its own test, not just setup. It may be merely donw for the side > effect of making the step unskippable, but still .... Sure, I’ll remove `(setup)`. The test name suggests that the test depends on the previous one in any case.
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: >>> -mv .git/config .git/config-saved >>> - >>> test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' ' >>> + test_when_finished mv .git/config-saved .git/config && >>> + mv .git/config .git/config-saved && >>> git branch -m q q2 && >>> git branch -m q2 q >>> ' >>> >>> -mv .git/config-saved .git/config >> >> The above is a truly valuable clean-up. >> >> But I am not really sure if the paritcular condition is worth >> testing in the first place these days. No configuration file means >> we cannot even read the repository format version, and working under >> such a condition is quite a bad promise that we would rather not to >> having to keep. But that is an entirely different topic from what >> this patch is doing. > > Okay. I could undo this change and remove the test in its own commit? No, please keep it. I think removing it is totally outside the scope of this series. We do preliminary clean-up in various areas, so that the last step can do the advise thing for "git branch". In the context of the series, removing this test does not fit anywhere. It is not a clean-up like any other preliminary steps. Thanks.
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index de7d3014e4f..273a57a72d8 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -75,13 +75,13 @@ test_expect_success 'git branch HEAD should fail' ' test_must_fail git branch HEAD ' -cat >expect <<EOF -$HEAD refs/heads/d/e/f@{0}: branch: Created from main -EOF test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' ' GIT_COMMITTER_DATE="2005-05-26 23:30" \ git -c core.logallrefupdates=false branch --create-reflog d/e/f && test_ref_exists refs/heads/d/e/f && + cat >expect <<-EOF && + $HEAD refs/heads/d/e/f@{0}: branch: Created from main + EOF git reflog show --no-abbrev-commit refs/heads/d/e/f >actual && test_cmp expect actual ' @@ -440,10 +440,10 @@ test_expect_success 'git branch --list -v with --abbrev' ' test_expect_success 'git branch --column' ' COLUMNS=81 git branch --column=column >actual && - cat >expect <<\EOF && - a/b/c bam foo l * main n o/p r - abc bar j/k m/m mb o/o q topic -EOF + cat >expect <<-\EOF && + a/b/c bam foo l * main n o/p r + abc bar j/k m/m mb o/o q topic + EOF test_cmp expect actual ' @@ -453,25 +453,25 @@ test_expect_success 'git branch --column with an extremely long branch name' ' test_when_finished "git branch -d $long" && git branch $long && COLUMNS=80 git branch --column=column >actual && - cat >expect <<EOF && - a/b/c - abc - bam - bar - foo - j/k - l - m/m -* main - mb - n - o/o - o/p - q - r - topic - $long -EOF + cat >expect <<-EOF && + a/b/c + abc + bam + bar + foo + j/k + l + m/m + * main + mb + n + o/o + o/p + q + r + topic + $long + EOF test_cmp expect actual ' @@ -481,10 +481,10 @@ test_expect_success 'git branch with column.*' ' COLUMNS=80 git branch >actual && git config --unset column.branch && git config --unset column.ui && - cat >expect <<\EOF && - a/b/c bam foo l * main n o/p r - abc bar j/k m/m mb o/o q topic -EOF + cat >expect <<-\EOF && + a/b/c bam foo l * main n o/p r + abc bar j/k m/m mb o/o q topic + EOF test_cmp expect actual ' @@ -496,39 +496,36 @@ test_expect_success 'git branch -v with column.ui ignored' ' git config column.ui column && COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual && git config --unset column.ui && - cat >expect <<\EOF && - a/b/c - abc - bam - bar - foo - j/k - l - m/m -* main - mb - n - o/o - o/p - q - r - topic -EOF + cat >expect <<-\EOF && + a/b/c + abc + bam + bar + foo + j/k + l + m/m + * main + mb + n + o/o + o/p + q + r + topic + EOF test_cmp expect actual ' -mv .git/config .git/config-saved - test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' ' + test_when_finished mv .git/config-saved .git/config && + mv .git/config .git/config-saved && git branch -m q q2 && git branch -m q2 q ' -mv .git/config-saved .git/config - -git config branch.s/s.dummy Hello - -test_expect_success 'git branch -m s/s s should work when s/t is deleted' ' +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' ' + git config branch.s/s.dummy Hello && git branch --create-reflog s/s && git reflog exists refs/heads/s/s && git branch --create-reflog s/t && @@ -1141,14 +1138,14 @@ test_expect_success '--set-upstream-to notices an error to set branch as own ups test_cmp expect actual " -# Keep this test last, as it changes the current branch -cat >expect <<EOF -$HEAD refs/heads/g/h/i@{0}: branch: Created from main -EOF test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' ' + test_when_finished git checkout main && GIT_COMMITTER_DATE="2005-05-26 23:30" \ git checkout -b g/h/i -l main && test_ref_exists refs/heads/g/h/i && + cat >expect <<-EOF && + $HEAD refs/heads/g/h/i@{0}: branch: Created from main + EOF git reflog show --no-abbrev-commit refs/heads/g/h/i >actual && test_cmp expect actual '
Some tests use a preliminary heredoc for `expect` or have setup and teardown commands before and after, respectively. It is however preferred to keep all the logic in the test itself. Let’s move these into the tests. Also: • Remove a now-irrelevant comment about test placement and switch back to `main` post-test. • Prefer indented literal heredocs (`-\EOF`) except for a block which says that this is intentional • Move a `git config` command into the test and mark it as `setup` since the next test depends on it Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- t/t3200-branch.sh | 115 ++++++++++++++++++++++------------------------ 1 file changed, 56 insertions(+), 59 deletions(-)