| Message ID | 20210407225608.14611-8-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git log: configurable default format for merge diffs | expand |
On Thu, Apr 08 2021, Sergey Organov wrote: > There were 3 completion tests failures due to introduction of > log.diffMerges configuration variable that affected the result of > completion of log.d. Fixed them accordingly. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > t/t9902-completion.sh | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 04ce884ef5ac..4d732d6d4f81 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' ' > test_completion "git config log.d" <<-\EOF > log.date Z > log.decorate Z > + log.diffMerges Z > EOF > ' > > @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' ' > test_completion "git -c log.d" <<-\EOF > log.date=Z > log.decorate=Z > + log.diffMerges=Z > EOF > ' > > @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' ' > test_completion "git clone --config=log.d" <<-\EOF > log.date=Z > log.decorate=Z > + log.diffMerges=Z > EOF > ' Commits should be made in such a way as to not break the build/tests partway through a series, which it seems is happening until this fixup. Having read this far most of what you have in this 9 patch series could/should be squashed into something much smaller, e.g. tests being added for code added in previous steps, let's add the tests along with the code since this isn't such a large change.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Apr 08 2021, Sergey Organov wrote: > >> There were 3 completion tests failures due to introduction of >> log.diffMerges configuration variable that affected the result of >> completion of log.d. Fixed them accordingly. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> t/t9902-completion.sh | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> index 04ce884ef5ac..4d732d6d4f81 100755 >> --- a/t/t9902-completion.sh >> +++ b/t/t9902-completion.sh >> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' ' >> test_completion "git config log.d" <<-\EOF >> log.date Z >> log.decorate Z >> + log.diffMerges Z >> EOF >> ' >> >> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' ' >> test_completion "git -c log.d" <<-\EOF >> log.date=Z >> log.decorate=Z >> + log.diffMerges=Z >> EOF >> ' >> >> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' ' >> test_completion "git clone --config=log.d" <<-\EOF >> log.date=Z >> log.decorate=Z >> + log.diffMerges=Z >> EOF >> ' > > Commits should be made in such a way as to not break the build/tests > partway through a series, which it seems is happening until this > fixup. Yep. Could these tests be somehow written in a more robust manner, to be protected against future additions of configuration variables that are unrelated to the features being tested? If so, I'd prefer to fix them as a prerequisite to the series rather than adding fixes to unrelated existing tests into my patches. > Having read this far most of what you have in this 9 patch series > could/should be squashed into something much smaller, e.g. tests being > added for code added in previous steps, let's add the tests along with > the code since this isn't such a large change. In general, I try to make commits as small as possible, but if you prefer tests to be included with the code in the same commit, – that's fine with me too. Will meld new tests into code commits for the next re-roll. Thanks! -- Sergey Organov
On Thu, Apr 08 2021, Sergey Organov wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Thu, Apr 08 2021, Sergey Organov wrote: >> >>> There were 3 completion tests failures due to introduction of >>> log.diffMerges configuration variable that affected the result of >>> completion of log.d. Fixed them accordingly. >>> >>> Signed-off-by: Sergey Organov <sorganov@gmail.com> >>> --- >>> t/t9902-completion.sh | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >>> index 04ce884ef5ac..4d732d6d4f81 100755 >>> --- a/t/t9902-completion.sh >>> +++ b/t/t9902-completion.sh >>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' ' >>> test_completion "git config log.d" <<-\EOF >>> log.date Z >>> log.decorate Z >>> + log.diffMerges Z >>> EOF >>> ' >>> >>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' ' >>> test_completion "git -c log.d" <<-\EOF >>> log.date=Z >>> log.decorate=Z >>> + log.diffMerges=Z >>> EOF >>> ' >>> >>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' ' >>> test_completion "git clone --config=log.d" <<-\EOF >>> log.date=Z >>> log.decorate=Z >>> + log.diffMerges=Z >>> EOF >>> ' >> >> Commits should be made in such a way as to not break the build/tests >> partway through a series, which it seems is happening until this >> fixup. > > Yep. > > Could these tests be somehow written in a more robust manner, to be > protected against future additions of configuration variables that are > unrelated to the features being tested? If so, I'd prefer to fix them as > a prerequisite to the series rather than adding fixes to unrelated > existing tests into my patches. Hrm? I mean if you have a commit fixing up failing tests in an earlier commit then that change should in one way or the other be made as part of that earlier change. Yes we can skip the tests or something in the meantime, which we do sometimes as part of some really large changes, but these can just be squashed, no? >> Having read this far most of what you have in this 9 patch series >> could/should be squashed into something much smaller, e.g. tests being >> added for code added in previous steps, let's add the tests along with >> the code since this isn't such a large change. > > In general, I try to make commits as small as possible, but if you > prefer tests to be included with the code in the same commit, – that's > fine with me too. > > Will meld new tests into code commits for the next re-roll. I'm probably the last person to give advice on this list about not overly splitting up ones commits :) Having said that, some sage advice: It's really helpful to split commits into discrete understandable pieces when it aids in reviewing/understanding the code. But something like say your 8/9 is IMNSHO a step to far, you're just adding a feature earlier and then docs for it later. That doesn't help to review or understand the change, now you just need to look in two places for what's one logical change. Ditto for e.g. the 5/9 here. That's just a test for a feature added earlier. So let's add it to the commit where we add that feature. There *are* cases where it helps to split up these changes, but they're things like adding tests for existing behavior before changing something, as an aid to demonstrate what the behavior was before & after. In those cases it's a lot better to split the commits, because nobody wants to waste time discerning what's a test for existing v.s. new behavior.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Apr 08 2021, Sergey Organov wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> On Thu, Apr 08 2021, Sergey Organov wrote: >>> >>>> There were 3 completion tests failures due to introduction of >>>> log.diffMerges configuration variable that affected the result of >>>> completion of log.d. Fixed them accordingly. >>>> >>>> Signed-off-by: Sergey Organov <sorganov@gmail.com> >>>> --- >>>> t/t9902-completion.sh | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >>>> index 04ce884ef5ac..4d732d6d4f81 100755 >>>> --- a/t/t9902-completion.sh >>>> +++ b/t/t9902-completion.sh >>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' ' >>>> test_completion "git config log.d" <<-\EOF >>>> log.date Z >>>> log.decorate Z >>>> + log.diffMerges Z >>>> EOF >>>> ' >>>> >>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' ' >>>> test_completion "git -c log.d" <<-\EOF >>>> log.date=Z >>>> log.decorate=Z >>>> + log.diffMerges=Z >>>> EOF >>>> ' >>>> >>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' ' >>>> test_completion "git clone --config=log.d" <<-\EOF >>>> log.date=Z >>>> log.decorate=Z >>>> + log.diffMerges=Z >>>> EOF >>>> ' >>> >>> Commits should be made in such a way as to not break the build/tests >>> partway through a series, which it seems is happening until this >>> fixup. >> >> Yep. >> >> Could these tests be somehow written in a more robust manner, to be >> protected against future additions of configuration variables that are >> unrelated to the features being tested? If so, I'd prefer to fix them as >> a prerequisite to the series rather than adding fixes to unrelated >> existing tests into my patches. > > Hrm? I mean if you have a commit fixing up failing tests in an earlier > commit then that change should in one way or the other be made as part > of that earlier change. > > Yes we can skip the tests or something in the meantime, which we do > sometimes as part of some really large changes, but these can just be > squashed, no? I mean I don't want this change at all. I didn't change completion mechanism, so completion tests should not suddenly fail because of my changes. I did entirely unrelated change and noticed the breakage only by accident, as tests even don't fail unless you *install* git, not only make it. So, for example, just "make test" doesn't fail, while "make install; make test" will. It looks like something is wrong here, a bug or misfeature, or even two, and if it's fixed before these series, I won't need this in my series at all. Besides, that's yet another reason *not* to squash this change into an otherwise unrelated commit. -- Sergey Organov
On Thu, Apr 08, 2021 at 11:26:38PM +0300, Sergey Organov wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > On Thu, Apr 08 2021, Sergey Organov wrote: > > > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> > >>> On Thu, Apr 08 2021, Sergey Organov wrote: > >>> > >>>> There were 3 completion tests failures due to introduction of > >>>> log.diffMerges configuration variable that affected the result of > >>>> completion of log.d. Fixed them accordingly. > >>>> > >>>> Signed-off-by: Sergey Organov <sorganov@gmail.com> > >>>> --- > >>>> t/t9902-completion.sh | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > >>>> index 04ce884ef5ac..4d732d6d4f81 100755 > >>>> --- a/t/t9902-completion.sh > >>>> +++ b/t/t9902-completion.sh > >>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' ' > >>>> test_completion "git config log.d" <<-\EOF > >>>> log.date Z > >>>> log.decorate Z > >>>> + log.diffMerges Z > >>>> EOF > >>>> ' > >>>> > >>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' ' > >>>> test_completion "git -c log.d" <<-\EOF > >>>> log.date=Z > >>>> log.decorate=Z > >>>> + log.diffMerges=Z > >>>> EOF > >>>> ' > >>>> > >>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' ' > >>>> test_completion "git clone --config=log.d" <<-\EOF > >>>> log.date=Z > >>>> log.decorate=Z > >>>> + log.diffMerges=Z > >>>> EOF > >>>> ' > >>> > >>> Commits should be made in such a way as to not break the build/tests > >>> partway through a series, which it seems is happening until this > >>> fixup. Well, actually no: it _starts_ to break with this patch, because 'log.diffMerges' is not documented yet, and it will pass again with the last patch in the series that adds that missing piece of documentation. > >> Yep. > >> > >> Could these tests be somehow written in a more robust manner, to be > >> protected against future additions of configuration variables that are > >> unrelated to the features being tested? If so, I'd prefer to fix them as > >> a prerequisite to the series rather than adding fixes to unrelated > >> existing tests into my patches. > > > > Hrm? I mean if you have a commit fixing up failing tests in an earlier > > commit then that change should in one way or the other be made as part > > of that earlier change. > > > > Yes we can skip the tests or something in the meantime, which we do > > sometimes as part of some really large changes, but these can just be > > squashed, no? > > I mean I don't want this change at all. You'll definitely need this change, though. > I didn't change completion mechanism, so completion tests should not > suddenly fail because of my changes. We auto-generate the list of supported configuration variables from the documentation and use that list in our Bash completion script to list possible configuration variables for 'git config <TAB>' and 'git -c <TAB>'. And we want to make sure that this feature works as intended, so we have a couple of tests that try to complete real config variable sections and names. You are just unlucky to introduce a new configuraton variable that happenes to start with the same prefix that is used in some of those tests. > I did entirely unrelated change and > noticed the breakage only by accident, as tests even don't fail unless > you *install* git, not only make it. So, for example, just "make test" > doesn't fail, while "make install; make test" will. It might be related to a bug in the build process that doesn't update that auto-generated list of supported configuration variables after e.g. 'Documentation/config/log.txt' was modified; see a proposed fix at: https://public-inbox.org/git/20210408212915.3060286-1-szeder.dev@gmail.com/ > It looks like something is wrong here, a bug or misfeature, or even two, > and if it's fixed before these series, I won't need this in my series at > all. Besides, that's yet another reason *not* to squash this change into > an otherwise unrelated commit. The introduction of the new configuration variable, its documentation and this test update should all go into a single patch. The whole test suite must pass for every single commit.
SZEDER Gábor <szeder.dev@gmail.com> writes: > On Thu, Apr 08, 2021 at 11:26:38PM +0300, Sergey Organov wrote: [...] > >> It looks like something is wrong here, a bug or misfeature, or even two, >> and if it's fixed before these series, I won't need this in my series at >> all. Besides, that's yet another reason *not* to squash this change into >> an otherwise unrelated commit. > > The introduction of the new configuration variable, its documentation > and this test update should all go into a single patch. The whole > test suite must pass for every single commit. OK, fine, thanks for clarification! -- Sergey Organov
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 04ce884ef5ac..4d732d6d4f81 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' ' test_completion "git config log.d" <<-\EOF log.date Z log.decorate Z + log.diffMerges Z EOF ' @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' ' test_completion "git -c log.d" <<-\EOF log.date=Z log.decorate=Z + log.diffMerges=Z EOF ' @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' ' test_completion "git clone --config=log.d" <<-\EOF log.date=Z log.decorate=Z + log.diffMerges=Z EOF '
There were 3 completion tests failures due to introduction of log.diffMerges configuration variable that affected the result of completion of log.d. Fixed them accordingly. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- t/t9902-completion.sh | 3 +++ 1 file changed, 3 insertions(+)