NotificationsYou must be signed in to change notification settings
Fork2.5k
Star10k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ethomson merged 2 commits intolibgit2:mainfromsamueltardieu:config-rename-multivar

Feb 17, 2024

Merged

config: properly delete or rename section containing multivars#6723

ethomson merged 2 commits intolibgit2:mainfromsamueltardieu:config-rename-multivar

Feb 17, 2024

Conversation

Copy link

Contributor

samueltardieu commentedJan 22, 2024•
edited
Loading

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

config: properly rename section containing multivars

7f1d520

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

samueltardieu changed the title~~config: properly rename section containing multivars~~config: properly delete or rename section containing multivars

Jan 23, 2024

Copy link

Member

ethomson commentedJan 26, 2024

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.

Copy link

ContributorAuthor

samueltardieu commentedFeb 8, 2024

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

config: avoid unnecessary copy in rename_section

8422a63

We can just append the escaped regex to the value buffer, we don't needcreate a new value buffer and then append _that_.

Copy link

Member

ethomson commentedFeb 17, 2024

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 intermediatevalue str with the escaped regex — thegit_str_escape_regex function shouldappend 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! 🙏