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

Fix IndexError when empty string for credential.helper #1860

Merged
merged 9 commits into from
Nov 27, 2023

Conversation

SID262000
Copy link
Contributor

This PR addresses #1858

When credential.helper= we need to just strip off any whitespaces and return the empty string, as compared to the other scenario credential.helper=osykeychain or similar values where we return the first value only considering the fact that credential.helper is multivalued.

Copy link
Contributor

@Wauplin Wauplin 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 opening a PR @SID262000! I reviewed and left some comments. Two main things:

  • maybe a regex would be easier here to handle all tricky possibilities
  • we should still return multiple values => it is possible to configure multiple git credential helpers on a machine

return sorted( # Sort for nice printing
{ # Might have some duplicates
line.split("=")[-1].split()[0] for line in output.split("\n") if "credential.helper" in line
value
Copy link
Contributor

Choose a reason for hiding this comment

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

We must return a set of all git credential helpers configured on the machine. So here we would need to return a value for each line with "credential.helper=" in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for pointing it out.

Comment on lines 40 to 45
for line in output.split("\n"):
if "credential.helper" in line:
if line.split("=")[-1]:
value = line.split("=")[-1].split()[0]
else:
value = line.split("=")[-1].strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I think the parsing will break if line = credential.helper= since line.split("=")[-1] will evaluate to True but line.split("=")[-1].split()[0] will fail.

Would you like to try to fix this once and for all using a regex? I started something here that in theory should work for all the cases we want to handle (still has to be tested for real).

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll re-evaluate the parsing.

Regarding the usage of Regex,
It sounds more robust approach, many thanks for putting down the sample regex and sharing it with me. I'll test it for other potential use cases and incorporate the same in my ongoing PR

@SID262000
Copy link
Contributor Author

I modified the above Regex slightly from "^\s*credential\.helper\s*=\s*(\w+)\s*" to "^\s*credential\.helper\s*=\s*(\w*)\s*$" to capture empty strings as well.
Now the above change captures all the values for credential.helper= and fixes it all once and for all using Regex.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 25, 2023

The documentation is not available anymore as the PR was closed or merged.

@SID262000 SID262000 requested a review from Wauplin November 27, 2023 13:25
Comment on lines 41 to 46
for line in output.split("\n"):
l_cred_helper = sorted( # Sort for nice printing
# Might have some duplicates
re.findall(r"^\s*credential\.helper\s*=\s*(\w*)\s*$", line)
)
return l_cred_helper
Copy link
Contributor

Choose a reason for hiding this comment

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

Using re.findall is good. However, you don't need to run it on each line one by one. You can run on the whole output at once and use re.MULTILINE as flag. Here is how it looks like when you apply it only once:

Suggested change
for line in output.split("\n"):
l_cred_helper = sorted( # Sort for nice printing
# Might have some duplicates
re.findall(r"^\s*credential\.helper\s*=\s*(\w*)\s*$", line)
)
return l_cred_helper
return sorted( # Sort for nice printing
set( # Might have some duplicates
re.findall(
r"^\s*credential\.helper\s*=\s*(\w+)\s*$",
output,
flags=re.MULTILINE | re.IGNORECASE,
)
)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Wauplin for the feedback. I've incorporated the above changes as indicated.

Copy link
Contributor

@Wauplin Wauplin 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 making the changes @SID262000! It's good to merge 🔥

@Wauplin Wauplin merged commit 580d729 into huggingface:main Nov 27, 2023
11 of 14 checks passed
@SID262000 SID262000 deleted the fix/1858 branch November 27, 2023 14:58
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