Skip to content

config: properly delete or rename section containing multivars #6723

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

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Jan 22, 2024

Renaming a config section or deleting its content removes each entry after copying it in its new place if needed. However, since each entry may be part of a multivar, deletion must only remove the exact entry that has just been copied.

Fix #6722

Renaming a section or deleting its content removes each entry after
copying it in its new place if needed. However, since each entry
may be part of a multivar, deletion must only remove the exact entry
that has just been copied.
@samueltardieu samueltardieu changed the title config: properly rename section containing multivars config: properly delete or rename section containing multivars Jan 23, 2024
@ethomson
Copy link
Member

Thanks @samueltardieu - this looks great on first glance, but I'd like to spend a few more minutes to give this a deeper review. Great first PR, I appreciate this.

@samueltardieu
Copy link
Contributor Author

Hi @ethomson. Anything I need to do on my side?

We can just append the escaped regex to the value buffer, we don't need
create a new value buffer and then append _that_.
@ethomson
Copy link
Member

Hi @samueltardieu - sorry about the delay. This is a great fix. I took one more look and realized that we should be able to avoid the copy when constructing the escaped value. I think that we don't need to create the intermediate value str with the escaped regex — the git_str_escape_regex function should append to an existing buffer. So it should be safe to just putc the ^, then escape the regex into the value string.

I pushed up a commit to your branch to do this - would you mind 👀 just to make sure that I didn't make any logical errors?

Thanks again! 🙏

@samueltardieu
Copy link
Contributor Author

It is much better indeed.

@ethomson ethomson merged commit 9e4e930 into libgit2:main Feb 17, 2024
@ethomson
Copy link
Member

Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot delete or rename a branch whose config section contains multivar entries
2 participants