Re: [PATCH v5] contrib/subtree: fix "subtree split" skipped-merge bug

From: David Ware <davidw@realtimegenomics.com>
Date: Jan 14, 2016, 20:45 UTC
Message-ID: <CAET=KiWjVr5h8nfU2DfUHGvzc7Tq7LoDWym7zXPq1Nvf+xHCCg@mail.gmail.com>
In-Reply-To: <87bn8o97mh.fsf@waller.obbligato.org>

On Thu, Jan 14, 2016 at 4:12 PM, David A. Greene <greened@obbligato.org> wrote:

Show 5 quoted lines

> David Ware <davidw@realtimegenomics.com> writes:
>> The commit was made against v2.6.3, when I try to apply the patch
>> against master it fails.
>
> Any ideas why?

"git am" (a command I have never used before) Fails like so

Applying: contrib/subtree: fix "subtree split" skipped-merge bug
error: patch failed: contrib/subtree/t/t7900-subtree.sh:468
error: contrib/subtree/t/t7900-subtree.sh: patch does not apply

It doesn't even put any files into a conflict state. I guess it's because of the hefty test refactoring you mentioned.

Show 14 quoted lines

>
>> However I can verify the test passes for me when applied against
>> v2.6.3, and it also passed if I merge my patched copy of v2.6.3 into
>> master.
>
> I don't think the subtree split code has changed at all in that period
> and the logs bear that out.  So there must be some change in
> v2.6.3..master that confounds your patch.
>
> Re-checking the patch submission guidelines, it looks like bugfixes
> should be based against maint.  I did that and the test still fails with
> your changes.  It seems like we ought to rebase to maint and continue
> our investigation there.
>

Hmm, the patch fails to apply for me there also. Same issue with contrib/subtree/t/t7900-subtree.sh

I haven't worked with mailed patches at all before, so it is possible I'm not using the correct workflow (I just saved the raw email I received for the patch as txt and fed it to 'git am'). Cherrypicking the commit onto maint works fine though, and the test passes for me in this situation.

Show 9 quoted lines

>> The process I'm using to run the tests is a little strange though, it
>> seems I have to make git, then make contrib/subtree, then cp
>> git-subtree to the root before running the Makefile on the tests.  Let
>> me know if there's a less strange process for running the subtree
>> tests.
>
> I actually have an update that makes this easier but I haven't submitted
> it yet.  But yes, you've got the current process right.
>

That will be nice.

Show 16 quoted lines

> Ok.  Your patch applied cleanly to maint and maint has the latest
> version of the test file.  It should be just a matter of following what
> the other tests do.  I'm more than happy to guide you through it.
>
>>>> +             git branch noop_branch &&
>>> [...]
>>>> +             git checkout noop_branch &&
>>>> +             echo moreText >anotherText.txt &&
>>>> +             git add . &&
>>>> +             git commit -m "irrelevant" &&
>>>
>>> This is unfortunate naming.  Why is the branch a no-op and why is the
>>> commit irrelevant?  Does the test test the same thing without them?  I
>>> not they should have different names.  If so, why are these needed in
>>> the test?
>>>

As noted above I can't get the patch to apply cleanly to maint for me, but I suppose it doesn't matter since I'm about to mail in a new version created against maint. I've rewritten the test to use the repo/commit creation methods, and renamed that branch. I've also added the comments you requested, and changed the push to an ancestor check. I'll be submitting the new version of the patch shortly.

Cheers, Dave Ware