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.

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

Fix IndexError when empty string for credential.helper #1860

Merged
merged 9 commits into from
Nov 27, 2023
Merged

Fix IndexError when empty string for credential.helper #1860

Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d40c46e
Fix IndexError while empty string for credential.helper
SID262000 Nov 24, 2023
5c48472
finally fix robustness?
Wauplin Nov 24, 2023
9c6b0c6
Add Regex to capture all values for credential.helper including empty…
SID262000 Nov 25, 2023
9ec981a
Merge branch 'huggingface:main' into fix/1858
SID262000 Nov 25, 2023
0fe2b90
fix typo (#1864)
pkking Nov 27, 2023
76e05f0
Allow create inference endpoint from docker image (#1861)
Wauplin Nov 27, 2023
d3ac4d0
Merge branch 'huggingface:main' into fix/1858
SID262000 Nov 27, 2023
f98edd5
Fix mypy errors
SID262000 Nov 27, 2023
7ccd97a
Re-factor Regex using MULTILINE flag
SID262000 Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/huggingface_hub/utils/_git_credential.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
"""Contains utilities to manage Git credentials."""
import re
import subprocess
from typing import List, Optional

Expand All @@ -37,11 +38,12 @@ def list_credential_helpers(folder: Optional[str] = None) -> List[str]:
# NOTE: If user has set an helper for a custom URL, it will not we caught here.
# Example: `credential.https://huggingface.co.helper=store`
# See: https://github.com/huggingface/huggingface_hub/pull/1138#discussion_r1013324508
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
}
)
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.

except subprocess.CalledProcessError as exc:
raise EnvironmentError(exc.stderr)

Expand Down