Skip to content

Fix commit tree with multiple parents #674

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

Closed
wants to merge 2 commits into from

Conversation

maths22
Copy link

@maths22 maths22 commented Oct 20, 2023

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

  • Ensure all commits include DCO sign-off.
  • Ensure that your contributions pass unit testing.
  • Ensure that your contributions contain documentation if applicable.

Description

This change fixes commit_tree to actually pass through multiple parents
correctly, while preserving backwards-compatibility by still handling
single commits passed into the parents field.

maths22 and others added 2 commits October 20, 2023 12:30
Signed-off-by: Jacob Burroughs <jburroughs@instructure.com>
@@ -1040,10 +1040,19 @@ def commit_tree(tree, opts = {})
t.write(opts[:message])
t.close

# Adapted from activesupport Array.wrap
parents = if opts[:parents].nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read up on Array.wrap but don't understand why you would choose that implementation instead of Kernel#Array. I am interested to hear why you would go this route instead of reducing this change to:

arr_opts += Array(opts[:parent]).map { |p| ['-p', p] }.flatten

I think calling #to_a if #to_ary doesn't exist would be the desired behavior.

@jcouball
Copy link
Member

@maths22 thank you for this contribution. After looking into the test_tree_ops.rb test case, I could see that these tests needed refactoring. This refactoring was done in #679 along with fixing the issue you submitted this fix for.

Apologies for taking so long.

@jcouball jcouball closed this Dec 26, 2023
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