Skip to content

Fix ls-files for git 2.16 #350

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

Merged
merged 1 commit into from
Mar 28, 2018
Merged

Conversation

rafamanzo
Copy link
Contributor

Since GIT version 2.16.0 empty pathspecs as alias to "everything" has
been made illegal.

A more detailed study this change's impact has been made at:

#345 (comment)

Closes #345

@@ -396,6 +396,7 @@ def diff_index(treeish)
end

def ls_files(location=nil)
location ||= '.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do this in the method arguments?

def ls_files(location='.')
...
end

Copy link
Contributor Author

@rafamanzo rafamanzo Mar 7, 2018

Choose a reason for hiding this comment

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

Sorry for the delay, I got no notification of your review. Thanks in advance :)

I thought about doing this, but then on ls_remote it was handled this way, so I kept the standard of code that was already there.

Having 2 standards seemed a bad idea to me. Also modifying ls_remote which is not subject of this PR didn't feel right.

But if you think it is not a problem modifying just here, I can change it. What do you think is best?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct about maintaining the standards. I had not seen ls_remote for my various use cases.

👍

@@ -396,6 +396,7 @@ def diff_index(treeish)
end

def ls_files(location=nil)
location ||= '.'
Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct about maintaining the standards. I had not seen ls_remote for my various use cases.

👍

Since GIT version 2.16.0 empty pathspecs as alias to "everything" has
been made illegal.

A more detailed study this change's impact has been made at:

ruby-git#345 (comment)

Closes ruby-git#345
@rafamanzo rafamanzo force-pushed the fix_empty_refspec branch from 965ffee to 11e20a2 Compare March 8, 2018 01:00
@rafamanzo
Copy link
Contributor Author

Thanks @tarcinil I've rebased from master so Github can FF merge. Could you have another look?

Copy link
Contributor

@taquitos taquitos left a comment

Choose a reason for hiding this comment

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

Nice and simple fix, who should we ping to get this merged?

Copy link
Contributor

@rvodden rvodden left a comment

Choose a reason for hiding this comment

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

Looks good - thanks for the submission.

@rvodden rvodden merged commit 5507d98 into ruby-git:master Mar 28, 2018
neomilium added a commit to opus-codium/modulesync that referenced this pull request Dec 20, 2020
This is now fixed upstream since 2018:
ruby-git/ruby-git#350
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.

4 participants