Skip to content

Git::Lib#ls_tree ignore other types fix #476

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Git::Lib#ls_tree ignore other types fix #476

wants to merge 2 commits into from
+110 −10

Conversation

stujapa
Copy link

@stujapa stujapa commented Jul 30, 2020

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

If git ls-tree includes items other than blobs and trees, Git::Lib.ls_tree
fails to either interpret or ignore the items.

Until first-class support is available, ignore types other than these two. No
mechanism for logging or otherwise recording this action is available, so, the
unknown types are ignored silently. (Debug logging elsewhere does include the
entire git ls-tree output.)

fixes #475
PHP-Proxy

PHP-Proxy

Error accessing resource: 404 - Not Found

@stujapa
Git::Lib#ls_tree ignore other types fix
391ce68 
If `git ls-tree` includes items other than blobs and trees, Git::Lib.ls_tree
fails to either interpret or ignore the items.

Until first-class support is available, ignore types other than these two.  No
mechanism for logging or otherwise recording this action is available, so, the
unknown types are ignored silently.  (Debug logging elsewhere does include the
entire `git ls-tree` output.)

Signed-off-by: Matthew Cary <98661+stujapa@users.noreply.github.com>
@stujapa stujapa force-pushed the lib-ls-tree-ignore-commits branch from 49d808e to 391ce68 Compare July 30, 2020 03:02
@stujapa
Copy link
Author

stujapa commented Jul 30, 2020

Hi @jcouball – could you please review? If I've missed something in CONTRIBUTING, please let me know. Thanks!

@stujapa
Copy link
Author

stujapa commented Aug 2, 2020

Hi @jcouball / @perlun / @tarcinil – could you please review?
Also, I'm sorry, travis-ci was triggered by this PR. However, I don't see how to connect the completed travis-ci run with the PR so that status is reported properly. Thanks!

@stujapa stujapa closed this Aug 6, 2020
@stujapa stujapa reopened this Aug 6, 2020
@stujapa
Copy link
Author

stujapa commented Aug 16, 2020

Hi @rvodden / @perlun / @tarcinil – please, could you review this PR?
travis-ci completed tests successfully. Please see https://travis-ci.org/github/ruby-git/ruby-git/builds/715390261 for details. I'm not sure why this and the previous test runs are not automatically linked.
Thanks!

@jcouball
Copy link
Member

jcouball commented Aug 16, 2020 •
edited
Loading

Thank you, @stujapa for this PR. Sorry I haven't gotten to it in a more timely manner.

In terms of functionality, I think this PR is fine.

Here is some general feedback about the tests that I wanted to run by you:

  1. What do you think about creating a test fixture specifically for this change? Reusing the default test fixture (test/files/working/dot_git) as you can see breaks a bunch of tests that don't have anything to do with the change you are making. To me this is tech debt that is piling up with each change like this one. The next PR might have to correct all the same tests plus the new ones introduced in this PR. Additionally, I think merge conflicts will be more of a problem if everyone is trying to change the same test fixture for their tests.
  2. What do you think about renaming your tests to indicate what is actually being tested? When I read the tests, I am not sure what is being tested. Based on the point of the PR, I'd expect there to be a test named test_ls_tree (that already existed) which tests basic functionality and a new test called test_ls_tree_ignores_unknown_types (or similar) to test the situation where there is an unknown item type. I think this would be more intention revealing.

This project's documentation could stand for some improvement. Right now, the most accessible and reliable documentation (other than the code) seems to be the README.md. Can you add an example for ls_tree and add the caveat that only blobs and trees are listed and other item types (such as submodules) dropped?

@jcouball
Copy link
Member

You can see an example of using a different fixture in tests/units/test_diff_non_default_encoding.rb for using the fixture tests/files/encoding

@jcouball
Copy link
Member

I re-ran the build and the status was updated. Not sure why it wasn't updating before.

@jcouball jcouball added the Bug label Aug 17, 2020
@jcouball jcouball added this to the 1.8.0 milestone Aug 17, 2020
@jcouball jcouball removed this from the 1.8.0 milestone Sep 5, 2020
@stale stale bot added the stale label Nov 7, 2020
@jcouball jcouball removed the stale label Mar 4, 2023
@ruby-git ruby-git deleted a comment from stale bot Mar 4, 2023
@jcouball
Copy link
Member

jcouball commented Mar 4, 2023

Thank you, @stujapa for this contribution.

I have fixed the issue in #643. Rather than ignoring commits returned by ls-files, that PR actually returns them with the other objects retuned by ls-files.

@jcouball jcouball closed this Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Git::Object::Tree#check_tree throws exception on a submodule, type 'commit'
2 participants
@stujapa @jcouball