Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs

3 messages, 2 authors, 2021-01-30 · open the first message on its own page

Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs

From: Junio C Hamano <hidden>
Date: 2021-01-30 18:57:39

"Kyle J. McKay" [off-list ref] writes:
The git command `git symbolic-ref <refname1> <refname2>` updates
<refname1> to be a "symbolic" pointer to <refname2>.  No matter
what future value <refname2> takes on, <refname1> will continue
to refer to that future value since it's "symbolic".
Correct.  While you are on the my-topic branch, HEAD (that's the
<refname1> in your description) points at refs/heads/my-topic
(that's the <refname2> in your description).

And when you create a new commit from this state, the logs of these
two refs will gain one entry each.  "git log HEAD@{now} would show
the recent progress of HEAD, "git log my-topic@{now}" would show the
recent progress of my-topic ("my-topic@"now}" can also be spelled as
"@{now}).

The <refname1> (HEAD) will keep referring to <refname2> (my-topic)
until you switch branches, and does not change even if <refname2>
points at a different commit, as it is "symbolic".
Since commit 523fa69c36744ae6 ("reflog: cleanse messages in the refs.c
layer", 2020-07-10, v2.29.0), the effect of using the aforementioned
"symbolic-ref" command on ref logs has changed in an unexpected way.
Please explain "in an unexpected way" here in the log message.  Not
every reader can read your mind and would expect the same as you do.

The said commit came as part of this topic, ...

https://lore.kernel.org/git/pull.669.v2.git.1594401593.gitgitgadget@gmail.com/

... so I've added the true author of it on the Cc: list.
Add a new set of tests to exercise and demonstrate this change
in preparation for correcting it (at which point the failing tests
will be flipped from `test_expect_failure` to `test_expect_success`).
We usually prefer not to do the two-step "expect_failure first and
then in a separate patch flip that to _success", as it makes it hard
to see the "fix" step (because the change in the test would be shown
only 3 lines before and after _failure->_success line, and what the
test is doing cannot be seen in the patch by itself).

Thanks.

Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs

From: Kyle J. McKay <hidden>
Date: 2021-01-30 23:12:42

On Jan 30, 2021, at 11:56, Junio C Hamano wrote:
The said commit came as part of this topic, ...

https://lore.kernel.org/git/pull.669.v2.git.1594401593.gitgitgadget@gmail.com/

... so I've added the true author of it on the Cc: list.
Out of curiosity, if Han-Wen Nienhuys is the true author of commit  
523fa69c36744ae6 why is it that you are both the committer and author  
of that commit in the commit's header?

The answer may also inform others trying to determine the best list of  
recipients for patches...

Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs

From: Kyle J. McKay <hidden>
Date: 2021-01-30 23:28:03

On Jan 30, 2021, at 11:56, Junio C Hamano wrote:
quoted
Add a new set of tests to exercise and demonstrate this change
in preparation for correcting it (at which point the failing tests
will be flipped from `test_expect_failure` to `test_expect_success`).
We usually prefer not to do the two-step "expect_failure first and
then in a separate patch flip that to _success", as it makes it hard
to see the "fix" step (because the change in the test would be shown
only 3 lines before and after _failure->_success line, and what the
test is doing cannot be seen in the patch by itself).
I'm having a bit of trouble parsing that into expectations.  A little  
help please.

Given the scenario that a bug is found that is not being caught by a  
test (irrespective of whether or not the outcome of this particular  
series discussion results in it being determined to be a "bug").

Further, if the fix is simple enough that it warrants only a single  
patch, what is the preferred order of patches then?

I would like the patch series to demonstrate:

1) that the test actually catches the bug (in case it comes back in  
the future)
2) the fix isolated (as much as possible) in a patch distinct from the  
test
3) that the test now passes, preferably with minimal changes to be  
sure that it hasn't accidentally been rendered ineffective

All the while, at no point after commits for (1), (2), or (3) is the  
test suite allowed to generate extra failures (that are not marked  
"expect failure").

patch 1/2 accomplishes (1)
patch 2/2 does both (2) and (3)

Are you suggesting that (1) just be omitted?  Or that it be modified  
so that it's an "expect success" patch?  But then (2) would break it  
and introduce a failing test into the test suite.  Should (2) then  
just flip the test from test_expect_success to test_expect_failure and  
then a separate commit flip it back to test_expect_success along with  
the minor change to the test itself to make it start passing again?   
In that case, there's always a risk that changing the test itself  
could accidentally make it no longer detect the problem anymore and  
just always succeed even if the problem comes back.

Without an initial expect_failure step (1), there's no commit in the  
repository that can demonstrate that the test actually catches the  
problem.

I understand that when new code is added, the tests come after, but it  
seems to me that when fixing a pre-existing problem, the test that  
demonstrates the problem should come first and be an expect failure  
since it is considered to be a problem.

What exactly is the order of test/fix changes that you're expecting to  
see here?

Thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help