Re: [PATCH 2/2] Fix for git-rev-list --merge-order B ^A (A,B share common base)

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 2/2] Fix for git-rev-list --merge-order B ^A (A,B share common base)

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:01

quoted
quoted
quoted
quoted
"JS" == Jon Seymour [off-list ref] writes:
I am puzzled about this part.

JS> The unit test changes in this patch remove use of the --show-breaks 
JS> flags from certain unit tests. The changed --merge-order behaviour 
JS> changed the annotation that --show-breaks prints for certain test cases. 
JS> The new behaviour is reasonable and irrelevant to the intent of the tests
JS> so that tests have been changed to eliminate the spurious behaviour.

If the behaviour of --show-breaks subtly changes, and if that
changed behaviour is something still acceptable, why not update
the test to show the new expected results since you are updating
the test anyway?

Showing that "subtle" change in the diff may draw people's
attention and would help you to verify that the behaviour change
is not something that would be unacceptable to them.

Also if you are changing t6001, could you also merge Mark
Allen's BSD portability fix while you are at it?

    Message-ID: [off-list ref]

Re: [PATCH 2/2] Fix for git-rev-list --merge-order B ^A (A,B share common base)

From: Jon Seymour <hidden>
Date: 2016-06-15 22:42:01

On 6/30/05, Junio C Hamano [off-list ref] wrote:
quoted
quoted
quoted
quoted
quoted
"JS" == Jon Seymour [off-list ref] writes:
I am puzzled about this part.

JS> The unit test changes in this patch remove use of the --show-breaks
JS> flags from certain unit tests. The changed --merge-order behaviour
JS> changed the annotation that --show-breaks prints for certain test cases.
JS> The new behaviour is reasonable and irrelevant to the intent of the tests
JS> so that tests have been changed to eliminate the spurious behaviour.

If the behaviour of --show-breaks subtly changes, and if that
changed behaviour is something still acceptable, why not update
the test to show the new expected results since you are updating
the test anyway?
I can do it this way, if you prefer. The issue was that the expected output was:

= l2
| l1 
| l0

but became:

^ l2
| l1
| l0

The annotation changes because there is no longer a single head in the
start list. There are now multiple heads, it just happens that one of
the heads is also a prune point.
Showing that "subtle" change in the diff may draw people's
attention and would help you to verify that the behaviour change
is not something that would be unacceptable to them.
Fair enough, I'll resubmit with a less drastic change to the test case.
Also if you are changing t6001, could you also merge Mark
Allen's BSD portability fix while you are at it?

    Message-ID: [off-list ref]
Ok.

jon.

Re: [PATCH 2/2] Fix for git-rev-list --merge-order B ^A (A,B share common base)

From: Jon Seymour <hidden>
Date: 2016-06-15 22:42:01

quoted
Also if you are changing t6001, could you also merge Mark
Allen's BSD portability fix while you are at it?

    Message-ID: [off-list ref]
Ok.
Sorry, forgot to do this. However, Mark's patch should still apply as
it is (or will do so with minimal edits), so I'll leave that to Linus'
discretion.

jon.
-- 
homepage: http://www.zeta.org.au/~jon/
blog: http://orwelliantremors.blogspot.com/
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help