| Message ID | b5eb110958baa80b72a345b3c850f1dfceabf076.1659122979.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | log: create tighter default decoration filter | expand |
On Fri, Jul 29 2022, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> > > The color.decorate.<slot> config option added the 'grafted' slot in > 09c4ba410b0f (log-tree: allow to customize 'grafted' color, 2018-05-26) > but included no tests for this behavior. When modifying some logic > around decorations, this ref namespace was ignored and could have been > lost as a default namespace for 'git log' decorations by default. > > Add two tests to t4207 that check that the replaced objects are > correctly decorated. Use "black" as the color since it is distinct from > the other colors already in the test. The first test uses regular > replace-objects while the second creates a commit graft. > > Be sure to test both modes with GIT_REPLACE_REF_BASE unset and set to an > alternative base. > > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- > t/t4207-log-decoration-colors.sh | 59 ++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh > index 36ac6aff1e4..69f8ac602d6 100755 > --- a/t/t4207-log-decoration-colors.sh > +++ b/t/t4207-log-decoration-colors.sh > @@ -18,6 +18,7 @@ test_expect_success setup ' > git config color.decorate.tag "reverse bold yellow" && > git config color.decorate.stash magenta && > git config color.decorate.HEAD cyan && > + git config color.decorate.grafted black && > > c_reset="<RESET>" && > > @@ -27,6 +28,7 @@ test_expect_success setup ' > c_tag="<BOLD;REVERSE;YELLOW>" && > c_stash="<MAGENTA>" && > c_HEAD="<CYAN>" && > + c_grafted="<BLACK>" && > > test_commit A && > git clone . other && > @@ -63,4 +65,61 @@ test_expect_success 'Commit Decorations Colored Correctly' ' > test_cmp expected out > ' > > +cat >expected <<EOF > +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD ->\ > + ${c_reset}${c_branch}main${c_reset}${c_commit},\ > + ${c_reset}${c_tag}tag: D${c_reset}${c_commit})${c_reset} D > +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: C${c_reset}${c_commit},\ > + ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B > +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A > +EOF I see this is used by (one) existing test, but for new test code let's add such setup in the test_expect_success block. A small issue, but e.g. if the partition our trash directories is on fills up this sort of thing outside of test_expect_success hides the source of the "first" error. More generally (just eyeballing this v.s. the existing one) maybe we can combine them a bit & share some of these lines? > +test_expect_success 'test coloring with replace-objects' ' > + test_when_finished rm -rf .git/refs/replace* && > + test_commit C && > + test_commit D && > + > + git replace HEAD~1 HEAD~2 && > + git log --first-parent --abbrev=10 --decorate --oneline --color=always HEAD | This hides segfaults, abort() etc. in git log, let's use an intermediate file rather than git on the LHS of a pipe. > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | {10,10} in a regex is just {10}, no? > + test_decode_color >out && > + test_cmp expected out && > + git replace -d HEAD~1 && > + > + GIT_REPLACE_REF_BASE=refs/replace2/ git replace HEAD~1 HEAD~2 && > + GIT_REPLACE_REF_BASE=refs/replace2/ git log --first-parent --abbrev=10 \ > + --decorate --oneline --color=always HEAD | Ditto LHS. > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | Ditto RX, but at this point I see it's copy/pasted from the existing test in the file, might be worth starting by factoring the duplicate bits out into a function... > + test_decode_color >out && > + test_cmp expected out > +' > + > +cat >expected <<EOF > +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD ->\ > + ${c_reset}${c_branch}main${c_reset}${c_commit},\ > + ${c_reset}${c_tag}tag: D${c_reset}${c_commit},\ > + ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D > +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit},\ > + ${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B > +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A > +EOF > + > +test_expect_success 'test coloring with grafted commit' ' > + test_when_finished rm -rf .git/refs/replace* && > + > + git replace --graft HEAD HEAD~2 && > + git log --first-parent --abbrev=10 --decorate --oneline --color=always HEAD | > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | > + test_decode_color >out && > + test_cmp expected out && > + git replace -d HEAD && > + > + GIT_REPLACE_REF_BASE=refs/replace2/ git replace --graft HEAD HEAD~2 && > + GIT_REPLACE_REF_BASE=refs/replace2/ git log --first-parent --abbrev=10 \ > + --decorate --oneline --color=always HEAD | > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | > + test_decode_color >out && > + test_cmp expected out > +' > + > test_done ...as we had one of these, and now have 3x...
On Wed, Aug 3, 2022 at 2:32 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Fri, Jul 29 2022, Derrick Stolee via GitGitGadget wrote: > > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | > > {10,10} in a regex is just {10}, no? I'm more than a little surprised that this regex repeat-count notation works on macOS `sed` which, in the BSD tradition, is rather feature poor. Testing it, though, I find that it does work, even on my relatively old version of macOS. However, I'd still worry about other BSD `sed`s in the wild. Instead of using the repeat-count notation, we could model this after the way `OID_REGEX` is defined in t/test-lib.sh which, through automation, defines a highly portable regex. Alternatively, just use "$_x05$_x05", also from test-lib.sh, to define a regex matcher for ten hex digits.
On Wed, Aug 03 2022, Eric Sunshine wrote: > On Wed, Aug 3, 2022 at 2:32 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> On Fri, Jul 29 2022, Derrick Stolee via GitGitGadget wrote: >> > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | >> >> {10,10} in a regex is just {10}, no? > > I'm more than a little surprised that this regex repeat-count notation > works on macOS `sed` which, in the BSD tradition, is rather feature > poor. Testing it, though, I find that it does work, even on my > relatively old version of macOS. However, I'd still worry about other > BSD `sed`s in the wild. It seems you missed it but we already have this code tested "in the wild", i.e. the "new" code here is really just copy/pasting test setup from above. So maybe we want to change (and you have some good suggestions here), but it's already shown itself to be portable enough. We've had this "sed" command since v1.7.2, or 567102819ac (Add test for correct coloring of git log --decoration, 2010-06-29).
On Wed, Aug 3, 2022 at 9:05 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Wed, Aug 03 2022, Eric Sunshine wrote: > > On Wed, Aug 3, 2022 at 2:32 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> On Fri, Jul 29 2022, Derrick Stolee via GitGitGadget wrote: > >> > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | > >> > >> {10,10} in a regex is just {10}, no? > > > > I'm more than a little surprised that this regex repeat-count notation > > works on macOS `sed` which, in the BSD tradition, is rather feature > > poor. Testing it, though, I find that it does work, even on my > > relatively old version of macOS. However, I'd still worry about other > > BSD `sed`s in the wild. > > It seems you missed it but we already have this code tested "in the > wild", i.e. the "new" code here is really just copy/pasting test setup > from above. > > So maybe we want to change (and you have some good suggestions here), > but it's already shown itself to be portable enough. We've had this > "sed" command since v1.7.2, or 567102819ac (Add test for correct > coloring of git log --decoration, 2010-06-29). No, I didn't miss it; I saw your mention that it was copied from existing code in the script, but I assumed the existing code was recent -- hence my surprise. Since it's been around so long, I agree, it's hardly a cause for concern at this point (though, I still reflexively write the more portable notation as used by `OID_REGEX` and `_x05`, etc.).
Eric Sunshine <sunshine@sunshineco.com> writes: > it's hardly a cause for concern at this point (though, I still > reflexively write the more portable notation as used by `OID_REGEX` > and `_x05`, etc.). Yeah, use of predefined constants we have in our test framework releaves us from having to worry about the issue, which is why I too am in favor of using them.
On 8/4/2022 1:23 PM, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> it's hardly a cause for concern at this point (though, I still >> reflexively write the more portable notation as used by `OID_REGEX` >> and `_x05`, etc.). > > Yeah, use of predefined constants we have in our test framework > releaves us from having to worry about the issue, which is why > I too am in favor of using them. Thanks. Using $OID_REGEX (along with --no-abbrev) will work perfectly for this script. -Stolee
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh index 36ac6aff1e4..69f8ac602d6 100755 --- a/t/t4207-log-decoration-colors.sh +++ b/t/t4207-log-decoration-colors.sh @@ -18,6 +18,7 @@ test_expect_success setup ' git config color.decorate.tag "reverse bold yellow" && git config color.decorate.stash magenta && git config color.decorate.HEAD cyan && + git config color.decorate.grafted black && c_reset="<RESET>" && @@ -27,6 +28,7 @@ test_expect_success setup ' c_tag="<BOLD;REVERSE;YELLOW>" && c_stash="<MAGENTA>" && c_HEAD="<CYAN>" && + c_grafted="<BLACK>" && test_commit A && git clone . other && @@ -63,4 +65,61 @@ test_expect_success 'Commit Decorations Colored Correctly' ' test_cmp expected out ' +cat >expected <<EOF +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD ->\ + ${c_reset}${c_branch}main${c_reset}${c_commit},\ + ${c_reset}${c_tag}tag: D${c_reset}${c_commit})${c_reset} D +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: C${c_reset}${c_commit},\ + ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A +EOF + +test_expect_success 'test coloring with replace-objects' ' + test_when_finished rm -rf .git/refs/replace* && + test_commit C && + test_commit D && + + git replace HEAD~1 HEAD~2 && + git log --first-parent --abbrev=10 --decorate --oneline --color=always HEAD | + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | + test_decode_color >out && + test_cmp expected out && + git replace -d HEAD~1 && + + GIT_REPLACE_REF_BASE=refs/replace2/ git replace HEAD~1 HEAD~2 && + GIT_REPLACE_REF_BASE=refs/replace2/ git log --first-parent --abbrev=10 \ + --decorate --oneline --color=always HEAD | + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | + test_decode_color >out && + test_cmp expected out +' + +cat >expected <<EOF +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD ->\ + ${c_reset}${c_branch}main${c_reset}${c_commit},\ + ${c_reset}${c_tag}tag: D${c_reset}${c_commit},\ + ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit},\ + ${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A +EOF + +test_expect_success 'test coloring with grafted commit' ' + test_when_finished rm -rf .git/refs/replace* && + + git replace --graft HEAD HEAD~2 && + git log --first-parent --abbrev=10 --decorate --oneline --color=always HEAD | + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | + test_decode_color >out && + test_cmp expected out && + git replace -d HEAD && + + GIT_REPLACE_REF_BASE=refs/replace2/ git replace --graft HEAD HEAD~2 && + GIT_REPLACE_REF_BASE=refs/replace2/ git log --first-parent --abbrev=10 \ + --decorate --oneline --color=always HEAD | + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | + test_decode_color >out && + test_cmp expected out +' + test_done