Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: error in topic-branches-2.svg #1982

Closed
wants to merge 1 commit into from
Closed

Conversation

iidear
Copy link

@iidear iidear commented Sep 13, 2024

Changes

  • Chapter 3.4 Figure 29. History after merging dumbidea and iss91v2. C14 should not be ahead of C11
  • topic-branches-2.png needs modification too, but I don't know how to generate the same quality png

Context

@rosshjb
Copy link

rosshjb commented Nov 24, 2024

I don't think so:

C14 should not be ahead of C11

IMO, there is nothing wrong with the original image. The figure 29 shows that the dumbidea and issq1v2 branches have been merged into the master branch. Following the diagram you suggested, the issq1v2 branch hasn't been merged into the master branch. Refer the following paragraph:

Now, let’s say you decide you like the second solution to your issue best (iss91v2); and you showed the dumbidea branch to your coworkers, and it turns out to be genius. You can throw away the original iss91 branch (losing commits C5 and C6) and merge in the other two.

@ben
Copy link
Member

ben commented Nov 25, 2024

The fix is incorrect because the merge of iss91v2 isn't a squash, it's meant to be a merge commit. That merge commit should have parents that are the tips of master (C13 after dumbidea is fast-forward-merged) and iss91v2 (C11), and that's exactly what's pictured.

I'll agree that the text and the diagram don't quite agree, but it's because of the order of the merges, not the presence of this arrow. We should probably make it more clear that dumbidea was merged first, so that people following along won't have to reconstruct that themselves.

@ben ben closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants