Skip to content

My attempt at #1245#1247

Merged
pcottle merged 2 commits intomainfrom
attempt1245
Apr 26, 2025
Merged

My attempt at #1245#1247
pcottle merged 2 commits intomainfrom
attempt1245

Conversation

@pcottle
Copy link
Copy Markdown
Owner

@pcottle pcottle commented Apr 26, 2025

See #1246

@netlify
Copy link
Copy Markdown

netlify bot commented Apr 26, 2025

Deploy Preview for xenodochial-hugle-b9ec84 ready!

Name Link
🔨 Latest commit b76e4a4
🔍 Latest deploy log https://app.netlify.com/sites/xenodochial-hugle-b9ec84/deploys/680d1069f754f80008b138e8
😎 Deploy Preview https://deploy-preview-1247--xenodochial-hugle-b9ec84.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pcottle
Copy link
Copy Markdown
Owner Author

pcottle commented Apr 26, 2025

@cuinixam I think this works? Let me know what you think

@cuinixam
Copy link
Copy Markdown

@pcottle I've just tested it and it works. I like it! Thanks!
I was looking forward to finishing it myself today but you were much too fast for me 😀

@cuinixam
Copy link
Copy Markdown

Would you be so kind and explain why my change did not work.
My assumption was that for my use case, with rebasing a feature branch, because currentLocation argument is not changed, the else branch will be hit and currentLocation will be reset to base and then checked out.

    if (this.resolveID(currentLocation).get('type') == 'commit') {
      // we referenced a commit like git rebase C2 C1, so we have
      // to manually check out C1'
      this.checkout(base);
    } else {
      // now we just need to update the rebased branch is
      this.setTargetLocation(currentLocation, base);
      this.checkout(currentLocation);
    }

@pcottle
Copy link
Copy Markdown
Owner Author

pcottle commented Apr 26, 2025

@cuinixam I really appreciate you taking a stab at it! I wouldn't have done the change if you hadn't gotten it started 😄 it really helps, truly. Also if you want, you could submit a PR to add the animation delay if that feels necessary.

Your PR was quite close but I think the issue was this:

  • before, we had HEAD pointing at feature which then pointed to the actual commit E in your example. currentLocation in the method is actually HEAD (since we didn't specify a source, it just defaults to that)
  • we create all the new commits and then this line kicks in:

https://github.com/pcottle/learnGitBranching/blob/main/src/js/git/index.js#L2471-L2472

  • that line is a little deceptive, but inside the implementation, it's basically saying "go get the thing right before a commit (either the branch if its there, or HEAD if not) and go update it to this newly created commit. So it takes HEAD, follows it one away to the branch feature, and updates where feature points

So in your PR when this line runs:
this.checkout(this.getCommitFromRef(targetSource));

it finds the target source and then checks it out... aka it assigns HEAD to that location! Now we lost the reference to feature which we need to update.

This works great for the animation but later on we never update feature to point at the right commit.

So my code just found a way to hold onto that feature reference (if we need to) and update it later 😅

@pcottle pcottle merged commit 4dc4d0e into main Apr 26, 2025
5 checks passed
@pcottle
Copy link
Copy Markdown
Owner Author

pcottle commented Apr 26, 2025

(merged and pushing the site now)

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.

2 participants