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

Ignore `path=` lines in git credential fill output. #7

Closed
wants to merge 3 commits into from

Conversation

@JornVernee
Copy link
Member

JornVernee commented Jun 26, 2019

Hi,

I'm trying out skara, but when running git fork https://github.com/openjdk/panama git-panama I'm getting the following exception:

Exception in thread "main" java.io.IOException: 'git credential' returned unexpected line: path=
        at org.openjdk.skara.cli/org.openjdk.skara.cli.GitCredentials.fill(GitCredentials.java:101)
        at org.openjdk.skara.cli/org.openjdk.skara.cli.GitFork.main(GitFork.java:130)
        at org.openjdk.skara.cli/org.openjdk.skara.cli.GitSkara.main(GitSkara.java:130)

Looks like path= is not handled yet?

This PR adds some handling that ignores lines starting with path= for now, but maybe any unknown line should be ignored instead of throwing an exception, what do you think?

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Approvers

  • Erik Helin (ehelin - Reviewer)
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 26, 2019

Hi JornVernee, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user JornVernee as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@bridgekeeper bridgekeeper bot added the oca label Jun 26, 2019
@openjdk openjdk bot added the cli label Jun 26, 2019
@JornVernee
Copy link
Member Author

JornVernee commented Jun 26, 2019

@bridgekeeper bridgekeeper bot removed the oca label Jun 26, 2019
@openjdk openjdk bot added the rfr label Jun 26, 2019
@mlbridge
Copy link

mlbridge bot commented Jun 26, 2019

Webrevs

@JornVernee
Copy link
Member Author

JornVernee commented Jun 26, 2019

webrev is a 404 for me...

Edit: Okay, now it's working :)

@edvbld
Copy link
Member

edvbld commented Jun 26, 2019

Thanks @JornVernee for your contribution! 🎉

The patch looks good, have you tried it with a credential with a path? I'm thinking that if the user has a path set for a credential then we should probably pass that path back to git credential approve in GitCredentials::approve. Won't we otherwise set the credential for the entire host, or am I missing something? Looking at the man page it seems like we should pass the path back to git credential approve, but I haven't had time to try this myself yet.

@JornVernee
Copy link
Member Author

JornVernee commented Jun 26, 2019

Yeah, that's a good point. I hadn't looked into it that much. I figured since there was no handling for path it was safe to ignore.

I usually use TortoiseGit, and I'm not really familiar with the credential CLI. Trying to run git credential fill manually from the command line seems to hang forever. I'll keep trying.


Nvm, didn't realize it was waiting for input from stdin (should have read the man page more carefully ;/)

@JornVernee
Copy link
Member Author

JornVernee commented Jun 26, 2019

Okay, I've tested with a credential with a path as well (had to set credential.useHttpPath=true in git config for this), and it seems to be working. I can use git fork to clone a repo like that. I also see the credential (including the path) appearing in wincred.

@edvbld
edvbld approved these changes Jun 27, 2019
Copy link
Member

edvbld left a comment

I tried the patch locally as well without a path and that case seems to still work fine, so 👍 from me, great work!

@openjdk openjdk bot removed the rfr label Jun 27, 2019
@openjdk
Copy link

openjdk bot commented Jun 27, 2019

This PR has been reviewed by Erik Helin (ehelin - Reviewer) - changes are approved.

@openjdk
Copy link

openjdk bot commented Jun 27, 2019

@JornVernee This change can now be integrated. The commit message will be:

Ignore `path=` lines in git credential fill output.

Reviewed-by: ehelin
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 3 commits pushed to the master branch:

  • 48427ff: 24: Allow lazy evaluation of the fullName field in HostUserDetails
  • c4cc930: 5: Remove pipeline support from process
  • 561d715: 19: Allow GitHub bots to use a PAT for authentication as well

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@edvbld) but any other Committer may sponsor as well.

  • To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@openjdk openjdk bot added the ready label Jun 27, 2019
@JornVernee
Copy link
Member Author

JornVernee commented Jun 27, 2019

/integrate

@openjdk
Copy link

openjdk bot commented Jun 27, 2019

@JornVernee
Your change (at version 113c93d) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Jun 27, 2019
@edvbld
Copy link
Member

edvbld commented Jun 27, 2019

/sponsor

@openjdk openjdk bot closed this Jun 27, 2019
@openjdk
Copy link

openjdk bot commented Jun 27, 2019

@edvbld @JornVernee The following commits have been pushed to master since your change was applied:

Your commit was automatically rebased without conflicts.
Pushed as commit 99c8223.

@JornVernee JornVernee deleted the JornVernee:path_bug branch Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.