Opened 9 years ago

Closed 9 years ago

#507 closed enhancement (complete)

Empty node processing: In-memory data source changes

Reported by: stephen Owned by: jinmei
Priority: medium Milestone: A-Team-Sprint-20110209
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 8.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description


Subtickets

Change History (7)

comment:1 follow-up: Changed 9 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to assigned

comment:2 in reply to: ↑ 1 Changed 9 years ago by jinmei

trac507 is ready for review.

While thinking about how to test this, I realized we actually didn't need
richer features implemented in #517 for the empty node handling. We only
need to have the "empty data OK" mode and the ability to exchange the
information on the "last comparison" in RBTree::find().

So I decided to complete this task rather than waiting for the completion
of #517. I started with incorporating necessary changes from #517
into the trac507 branch. The incorporated part has already been reviewed
in trac #517, so the review for this ticket can start from the next
commit: git diff 1ab958cdc6f1ebdbf88f863a4a15d6a5783035bc

Some part of the RBTreeNodeChain class haven't been merged to this branch
(that part hasn't been fully reviewed and is actually not necessary for
this task), but some comments for the unimplemented part was intentionally
left (with some notes that they are not yet implemented), assuming #517
will soon be merged anyway.

comment:3 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from assigned to reviewing

comment:4 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:5 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

The changes themself look good to me and I would say it could go to master. However, there's an issue not related to the code.

You mention you merged some changes from #517, but not merged everything. That is not true, at last from what is in the git history. You branched at some place from #517 and deleted some of the code that was there. Now, how does git do when you merge (by the usual algorithm)?

  • It looks for common ancestors of the merged branches.
  • Then it takes the parts that the branches don't have in common.
  • Each of that changes are put in some order onto the common ancestors (there are details about what is done when there were some merges in one branch, etc, but that's not interesting).

So, what will happen if we merge this into master now and #517 later. Git will discover, that the code was created in a common ancestor, so it is taken as a base to work on. Then both the changes from the rest of #517 (which don't contain the creation of code) and the deletion from this branch are applied to it. Which will result either in deletion of the code or bunch of conflicts.

I propose some solutions to this issue, and I'm willing to help with any of them, if you like:

  • Revert the commit that contains the deletes (1ab958cdc6f1ebdbf88f863a4a15d6a5783035bc) and wait for review of #517. In that case this branch will contain both delete and recreation of the code, which will negate each other (because git takes only the overall change, not single steps, if the history is linear) and therefore the code should stay there.
  • Rewrite the history of this branch, omitting the commit. Then the effect will be the same, the history cleaner, but warning about history rewriting below applies.
  • Do even more history rewriting ‒ create a new branch at some earlier point of trac517 that yet does not contain the code. Then if some code is missing, it can be cherry-picked (which will make the history contain the same commit at multiple places, but it will work) or the original trac517 reordered to include the correct bits of code at some point and add the others later (which might include not only reordering of commits, but also commit splitting and rebase of the rest of trac517 as well of rebase of #507). This might produce clean history in the end, but I would consider it both dangerous and overkill.
  • Merge this branch now and defer the solution until #517 is merged. Then, *before* actually merging it, revert 1ab958cdc6f1ebdbf88f863a4a15d6a5783035bc on master (which will lead to the same situation as the first one, but we don't have to wait ‒ on the other hand, we must not forget it). The whole thing can be done locally, before pushing, so it's safe.

Warning about rewriting: if you (or anybody) ever rewrites a branch that has been published already, it brings problems to anybody tracking the branch. If such person uses git pull on such branch, it will do bad things (merging the branch from before rewrite and after, including some commits twice in the history, and including all the commits left out). For that reason, git push requires -f if you rewrote a branch and want to push it and everybody should be notified about the need not to use pull. Rewriting locally is safe, though.

So, what of the solutions suit you? Or, do you have any other idea?

Thanks

comment:6 in reply to: ↑ 5 Changed 9 years ago by jinmei

Replying to vorner:

The changes themself look good to me and I would say it could go to master. However, there's an issue not related to the code.

You mention you merged some changes from #517, but not merged everything. That is not true, at last from what is in the git history. You branched at some place from #517 and deleted some of the code that was there. Now, how does git do when you merge (by the usual algorithm)?

Thanks for the careful check and suggestions. You were right. I
understood what I did, but the terminology wasn't really accurate.

I propose some solutions to this issue, and I'm willing to help with any of them, if you like:

  • Revert the commit that contains the deletes (1ab958cdc6f1ebdbf88f863a4a15d6a5783035bc) and wait for review of #517. In that case this branch will contain both delete and recreation of the code, which will negate each other (because git takes only the overall change, not single steps, if the history is linear) and therefore the code should stay there.

Now that #517 has been merged this seems to be the easiest choice. So
I did it and completed the merge.

I'm now closing the ticket.

comment:7 Changed 9 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.