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

Add vcsh locate to locate repo holding a file #268

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

madduck
Copy link
Contributor

@madduck madduck commented Nov 2, 2019

This adds the locate subcommand, which takes an existing file and
returns the repository that's tracking the file.

Signed-off-by: martin f. krafft [email protected]

This adds the `locate` subcommand, which takes an existing file and
returns the repository that's tracking the file.

Signed-off-by: martin f. krafft <[email protected]>
@madduck
Copy link
Contributor Author

madduck commented Nov 2, 2019

Looks like the testing framework is broken.

Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to contribute this. I've wanted some function like this many times!

What happens if more than one repo tracks a file? (I have several instances of this for various reasons.)

Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Besides the other couple comments, I'm wonding if this isn't an inefficient way to get the job done. I know git ls-files supports glob matching. That might be faster that listing everything and grepping. Did you check that?

Lastly does this cover the case of things behind spares checkouts?

vcsh Outdated Show resolved Hide resolved
fatal "'$VCSH_COMMAND_PARAMETER' does not exist" 1
fi
for VCSH_REPO_NAME in $(list); do
if get_files "$VCSH_COMMAND_PARAMETER" | @GREP@ -q .; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

This grep feels very superfluous. Did you look into what the return being given are and if they might suffice for this logic?

Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I resolved the merge conflicts with the default branch, but this still needs some confirmation on a couple points.

  1. How it handles multiple positives.
  2. Whether there is a simpler way to code it up, see L538 comment and earlier concern about globbing/iterating everything.

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