From c858c6442b53fc9b7e79923546668fe38fe2c58d Mon Sep 17 00:00:00 2001 From: Scott Chacon Date: Fri, 25 Apr 2025 19:33:08 +0000 Subject: [PATCH 01/17] bundle-uri: copy all bundle references ino the refs/bundle space When downloading bundles via the bundle-uri functionality, we only copy the references from refs/heads into the refs/bundle space. I'm not sure why this refspec is hardcoded to be so limited, but it makes the ref negotiation on the subsequent fetch suboptimal, since it won't use objects that are referenced outside of the current heads of the bundled repository. This change to copy everything in refs/ in the bundle to refs/bundles/ significantly helps the subsequent fetch, since nearly all the references are now included in the negotiation. The update to the bundle-uri unbundling refspec puts all the heads from a bundle file into refs/bundle/heads instead of directly into refs/bundle/ so the tests also need to be updated to look in the new heirarchy. Signed-off-by: Scott Chacon Signed-off-by: Junio C Hamano --- Documentation/technical/bundle-uri.adoc | 14 +- bundle-uri.c | 2 +- t/t5558-clone-bundle-uri.sh | 172 ++++++++++++------------ 3 files changed, 94 insertions(+), 94 deletions(-) diff --git a/Documentation/technical/bundle-uri.adoc b/Documentation/technical/bundle-uri.adoc index 91d3a13e3276fc..12283fa9ed52f8 100644 --- a/Documentation/technical/bundle-uri.adoc +++ b/Documentation/technical/bundle-uri.adoc @@ -232,13 +232,13 @@ will interact with bundle URIs according to the following flow: are present in the client repository. If some are missing, then the client delays unbundling until other bundles have been unbundled, making those OIDs present. When all required OIDs are present, the - client unbundles that data using a refspec. The default refspec is - `+refs/heads/*:refs/bundles/*`, but this can be configured. These refs - are stored so that later `git fetch` negotiations can communicate each - bundled ref as a `have`, reducing the size of the fetch over the Git - protocol. To allow pruning refs from this ref namespace, Git may - introduce a numbered namespace (such as `refs/bundles//*`) such that - stale bundle refs can be deleted. + client unbundles that data using a refspec. The refspec used is + `+refs/*:refs/bundles/*`. These refs are stored so that later + `git fetch` negotiations can communicate each bundled ref as a `have`, + reducing the size of the fetch over the Git protocol. To allow pruning + refs from this ref namespace, Git may introduce a numbered namespace + (such as `refs/bundles//*`) such that stale bundle refs can be + deleted. 3. If the file is instead a bundle list, then the client inspects the `bundle.mode` to see if the list is of the `all` or `any` form. diff --git a/bundle-uri.c b/bundle-uri.c index 744257c49c1328..3371d56f4ce148 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -403,7 +403,7 @@ static int unbundle_from_file(struct repository *r, const char *file) const char *branch_name; int has_old; - if (!skip_prefix(refname->string, "refs/heads/", &branch_name)) + if (!skip_prefix(refname->string, "refs/", &branch_name)) continue; strbuf_setlen(&bundle_ref, bundle_prefix_len); diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh index 3816ed5058d901..33a7009e9a2f02 100755 --- a/t/t5558-clone-bundle-uri.sh +++ b/t/t5558-clone-bundle-uri.sh @@ -58,7 +58,7 @@ test_expect_success 'create bundle' ' test_expect_success 'clone with path bundle' ' git clone --bundle-uri="clone-from/B.bundle" \ clone-from clone-path && - git -C clone-path rev-parse refs/bundles/topic >actual && + git -C clone-path rev-parse refs/bundles/heads/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual ' @@ -68,9 +68,9 @@ test_expect_success 'clone with bundle that has bad header' ' git clone --bundle-uri="clone-from/bad-header.bundle" \ clone-from clone-bad-header 2>err && commit_b=$(git -C clone-from rev-parse B) && - test_grep "trying to write ref '\''refs/bundles/topic'\'' with nonexistent object $commit_b" err && + test_grep "trying to write ref '\''refs/bundles/heads/topic'\'' with nonexistent object $commit_b" err && git -C clone-bad-header for-each-ref --format="%(refname)" >refs && - test_grep ! "refs/bundles/" refs + test_grep ! "refs/bundles/heads/" refs ' test_expect_success 'clone with bundle that has bad object' ' @@ -78,8 +78,8 @@ test_expect_success 'clone with bundle that has bad object' ' git clone --bundle-uri="clone-from/bad-object.bundle" \ clone-from clone-bad-object-no-fsck && git -C clone-bad-object-no-fsck for-each-ref --format="%(refname)" >refs && - grep "refs/bundles/" refs >actual && - test_write_lines refs/bundles/bad >expect && + grep "refs/bundles/heads/" refs >actual && + test_write_lines refs/bundles/heads/bad >expect && test_cmp expect actual && # Unbundle fails with fsckObjects set true, but clone can still proceed. @@ -87,14 +87,14 @@ test_expect_success 'clone with bundle that has bad object' ' clone-from clone-bad-object-fsck 2>err && test_grep "missingEmail" err && git -C clone-bad-object-fsck for-each-ref --format="%(refname)" >refs && - test_grep ! "refs/bundles/" refs + test_grep ! "refs/bundles/heads/" refs ' test_expect_success 'clone with path bundle and non-default hash' ' test_when_finished "rm -rf clone-path-non-default-hash" && GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="clone-from/B.bundle" \ clone-from clone-path-non-default-hash && - git -C clone-path-non-default-hash rev-parse refs/bundles/topic >actual && + git -C clone-path-non-default-hash rev-parse refs/bundles/heads/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual ' @@ -102,7 +102,7 @@ test_expect_success 'clone with path bundle and non-default hash' ' test_expect_success 'clone with file:// bundle' ' git clone --bundle-uri="file://$(pwd)/clone-from/B.bundle" \ clone-from clone-file && - git -C clone-file rev-parse refs/bundles/topic >actual && + git -C clone-file rev-parse refs/bundles/heads/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual ' @@ -173,12 +173,12 @@ test_expect_success 'clone bundle list (file, no heuristic)' ' git -C clone-list-file cat-file --batch-check refs && - grep "refs/bundles/" refs >actual && + grep "refs/bundles/heads/" refs >actual && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/merge - refs/bundles/right + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/merge + refs/bundles/heads/right EOF test_cmp expect actual ' @@ -220,10 +220,10 @@ test_expect_success 'clone bundle list (file, all mode, some failures)' ' git -C clone-all-some cat-file --batch-check refs && - grep "refs/bundles/" refs >actual && + grep "refs/bundles/heads/" refs >actual && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left + refs/bundles/heads/base + refs/bundles/heads/left EOF test_cmp expect actual ' @@ -253,7 +253,7 @@ test_expect_success 'clone bundle list (file, all mode, all failures)' ' git -C clone-all-fail cat-file --batch-check refs && - ! grep "refs/bundles/" refs + ! grep "refs/bundles/heads/" refs ' test_expect_success 'clone bundle list (file, any mode)' ' @@ -282,9 +282,9 @@ test_expect_success 'clone bundle list (file, any mode)' ' git -C clone-any-file cat-file --batch-check refs && - grep "refs/bundles/" refs >actual && + grep "refs/bundles/heads/" refs >actual && cat >expect <<-\EOF && - refs/bundles/base + refs/bundles/heads/base EOF test_cmp expect actual ' @@ -313,7 +313,7 @@ test_expect_success 'clone bundle list (file, any mode, all failures)' ' git -C clone-any-fail cat-file --batch-check refs && - ! grep "refs/bundles/" refs + ! grep "refs/bundles/heads/" refs ' test_expect_success 'negotiation: bundle with part of wanted commits' ' @@ -322,10 +322,10 @@ test_expect_success 'negotiation: bundle with part of wanted commits' ' git clone --no-local --bundle-uri="clone-from/A.bundle" \ clone-from nego-bundle-part && git -C nego-bundle-part for-each-ref --format="%(refname)" >refs && - grep "refs/bundles/" refs >actual && - test_write_lines refs/bundles/topic >expect && + grep "refs/bundles/heads/" refs >actual && + test_write_lines refs/bundles/heads/topic >expect && test_cmp expect actual && - # Ensure that refs/bundles/topic are sent as "have". + # Ensure that refs/bundles/heads/topic are sent as "have". tip=$(git -C clone-from rev-parse A) && test_grep "clone> have $tip" trace-packet.txt ' @@ -337,8 +337,8 @@ test_expect_success 'negotiation: bundle with all wanted commits' ' --bundle-uri="clone-from/B.bundle" \ clone-from nego-bundle-all && git -C nego-bundle-all for-each-ref --format="%(refname)" >refs && - grep "refs/bundles/" refs >actual && - test_write_lines refs/bundles/topic >expect && + grep "refs/bundles/heads/" refs >actual && + test_write_lines refs/bundles/heads/topic >expect && test_cmp expect actual && # We already have all needed commits so no "want" needed. test_grep ! "clone> want " trace-packet.txt @@ -363,13 +363,13 @@ test_expect_success 'negotiation: bundle list (no heuristic)' ' clone-from nego-bundle-list-no-heuristic && git -C nego-bundle-list-no-heuristic for-each-ref --format="%(refname)" >refs && - grep "refs/bundles/" refs >actual && + grep "refs/bundles/heads/" refs >actual && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left + refs/bundles/heads/base + refs/bundles/heads/left EOF test_cmp expect actual && - tip=$(git -C nego-bundle-list-no-heuristic rev-parse refs/bundles/left) && + tip=$(git -C nego-bundle-list-no-heuristic rev-parse refs/bundles/heads/left) && test_grep "clone> have $tip" trace-packet.txt ' @@ -395,13 +395,13 @@ test_expect_success 'negotiation: bundle list (creationToken)' ' clone-from nego-bundle-list-heuristic && git -C nego-bundle-list-heuristic for-each-ref --format="%(refname)" >refs && - grep "refs/bundles/" refs >actual && + grep "refs/bundles/heads/" refs >actual && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left + refs/bundles/heads/base + refs/bundles/heads/left EOF test_cmp expect actual && - tip=$(git -C nego-bundle-list-heuristic rev-parse refs/bundles/left) && + tip=$(git -C nego-bundle-list-heuristic rev-parse refs/bundles/heads/left) && test_grep "clone> have $tip" trace-packet.txt ' @@ -428,10 +428,10 @@ test_expect_success 'negotiation: bundle list with all wanted commits' ' clone-from nego-bundle-list-all && git -C nego-bundle-list-all for-each-ref --format="%(refname)" >refs && - grep "refs/bundles/" refs >actual && + grep "refs/bundles/heads/" refs >actual && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left + refs/bundles/heads/base + refs/bundles/heads/left EOF test_cmp expect actual && # We already have all needed commits so no "want" needed. @@ -465,7 +465,7 @@ test_expect_success 'clone HTTP bundle' ' git clone --bundle-uri="$HTTPD_URL/B.bundle" \ "$HTTPD_URL/smart/fetch.git" clone-http && - git -C clone-http rev-parse refs/bundles/topic >actual && + git -C clone-http rev-parse refs/bundles/heads/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual && @@ -476,7 +476,7 @@ test_expect_success 'clone HTTP bundle with non-default hash' ' test_when_finished "rm -rf clone-http-non-default-hash" && GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="$HTTPD_URL/B.bundle" \ "$HTTPD_URL/smart/fetch.git" clone-http-non-default-hash && - git -C clone-http-non-default-hash rev-parse refs/bundles/topic >actual && + git -C clone-http-non-default-hash rev-parse refs/bundles/heads/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual ' @@ -553,12 +553,12 @@ test_expect_success 'clone bundle list (HTTP, any mode)' ' git -C clone-any-http cat-file --batch-check refs && - grep "refs/bundles/" refs >actual && + grep "refs/bundles/heads/" refs >actual && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/merge - refs/bundles/right + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/merge + refs/bundles/heads/right EOF test_cmp expect actual ' @@ -641,9 +641,9 @@ test_expect_success 'clone incomplete bundle list (http, creationToken)' ' test_cmp expect actual && # We now have only one bundle ref. - git -C clone-token-http for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C clone-token-http for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-\EOF && - refs/bundles/base + refs/bundles/heads/base EOF test_cmp expect refs && @@ -679,13 +679,13 @@ test_expect_success 'clone incomplete bundle list (http, creationToken)' ' test_cmp expect actual && # We now have all bundle refs. - git -C clone-token-http for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C clone-token-http for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/merge - refs/bundles/right + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/merge + refs/bundles/heads/right EOF test_cmp expect refs ' @@ -721,9 +721,9 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' ' test_cmp expect actual && # only received base ref from bundle-1 - git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-\EOF && - refs/bundles/base + refs/bundles/heads/base EOF test_cmp expect refs && @@ -749,10 +749,10 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' ' test_cmp expect actual && # received left from bundle-2 - git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left + refs/bundles/heads/base + refs/bundles/heads/left EOF test_cmp expect refs && @@ -795,12 +795,12 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' ' # received merge ref from bundle-4, but right is missing # because we did not download bundle-3. - git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-\EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/merge + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/merge EOF test_cmp expect refs && @@ -862,7 +862,7 @@ test_expect_success 'creationToken heuristic with failed downloads (clone)' ' test_cmp expect actual && # All bundles failed to unbundle - git -C download-1 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C download-1 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && test_must_be_empty refs && # Case 2: middle bundle does not exist, only two bundles can unbundle @@ -909,10 +909,10 @@ test_expect_success 'creationToken heuristic with failed downloads (clone)' ' test_cmp expect actual && # bundle-1 and bundle-3 could unbundle, but bundle-4 could not - git -C download-2 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C download-2 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-EOF && - refs/bundles/base - refs/bundles/right + refs/bundles/heads/base + refs/bundles/heads/right EOF test_cmp expect refs && @@ -961,11 +961,11 @@ test_expect_success 'creationToken heuristic with failed downloads (clone)' ' test_cmp expect actual && # fake.bundle did not unbundle, but the others did. - git -C download-3 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C download-3 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/right + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/right EOF test_cmp expect refs ' @@ -1083,15 +1083,15 @@ test_expect_success 'creationToken heuristic with failed downloads (fetch)' ' test_cmp expect actual && # Check which bundles have unbundled by refs - git -C fetch-1 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C fetch-1 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/lefter - refs/bundles/merge - refs/bundles/right - refs/bundles/righter - refs/bundles/top + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/lefter + refs/bundles/heads/merge + refs/bundles/heads/right + refs/bundles/heads/righter + refs/bundles/heads/top EOF test_cmp expect refs && @@ -1144,12 +1144,12 @@ test_expect_success 'creationToken heuristic with failed downloads (fetch)' ' test_cmp expect actual && # Check which bundles have unbundled by refs - git -C fetch-2 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C fetch-2 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/merge - refs/bundles/right + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/merge + refs/bundles/heads/right EOF test_cmp expect refs && @@ -1204,13 +1204,13 @@ test_expect_success 'creationToken heuristic with failed downloads (fetch)' ' test_cmp expect actual && # Check which bundles have unbundled by refs - git -C fetch-3 for-each-ref --format="%(refname)" "refs/bundles/*" >refs && + git -C fetch-3 for-each-ref --format="%(refname)" "refs/bundles/heads/*" >refs && cat >expect <<-EOF && - refs/bundles/base - refs/bundles/left - refs/bundles/lefter - refs/bundles/right - refs/bundles/righter + refs/bundles/heads/base + refs/bundles/heads/left + refs/bundles/heads/lefter + refs/bundles/heads/right + refs/bundles/heads/righter EOF test_cmp expect refs ' From 435b076ceb6e42c2c4c66422c036a02982b36bd4 Mon Sep 17 00:00:00 2001 From: Scott Chacon Date: Fri, 25 Apr 2025 19:33:09 +0000 Subject: [PATCH 02/17] bundle-uri: add test for bundle-uri clones with tags The change to the bundle-uri unbundling refspec now includes tags, so this adds a very, very simple test to make sure that tags in a bundle are properly added to the cloned repository and will be included in ref negotiation with the subsequent fetch. Signed-off-by: Scott Chacon Signed-off-by: Junio C Hamano --- t/t5558-clone-bundle-uri.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh index 33a7009e9a2f02..9b211a626bd76d 100755 --- a/t/t5558-clone-bundle-uri.sh +++ b/t/t5558-clone-bundle-uri.sh @@ -107,6 +107,36 @@ test_expect_success 'clone with file:// bundle' ' test_cmp expect actual ' +test_expect_success 'create bundle with tags' ' + git init clone-from-tags && + ( + cd clone-from-tags && + git checkout -b base && + git checkout -b topic && + + test_commit A && + git tag tag-A && + git checkout -b base && + git branch -d topic && + test_commit B && + + git bundle create ALL.bundle --all && + git bundle verify ALL.bundle + ) +' + +test_expect_success 'clone with tags bundle' ' + git clone --bundle-uri="clone-from-tags/ALL.bundle" \ + clone-from-tags clone-tags-path && + + git -C clone-from-tags for-each-ref --format="%(refname:lstrip=1)" \ + >expect && + git -C clone-tags-path for-each-ref --format="%(refname:lstrip=2)" \ + refs/bundles >actual && + + test_cmp expect actual +' + # To get interesting tests for bundle lists, we need to construct a # somewhat-interesting commit history. # From 8adee0c0b06f7d1347b4e26a635e0ef20be217f4 Mon Sep 17 00:00:00 2001 From: Aditya Garg Date: Thu, 8 May 2025 17:14:27 +0000 Subject: [PATCH 03/17] send-mail: improve checks for valid_fqdn The current implementation of a valid Fully Qualified Domain Name is not that strict. It just checks whether it has a dot (.) and if using macOS, it should not end with .local. As per RFC1035[1], from what I understood, the following checks need to be done: - The domain must contain atleast one dot - Each label (separated by dots) must be 1-63 characters long - Labels must start and end with an alphanumeric character - Labels can contain alphanumeric characters and hyphens Here are some examples of valid and invalid labels: 'example.com', # Valid 'sub.example.com', # Valid 'my-domain.org', # Valid 'localhost', # Invalid (no dot) 'MacBook..', # Invalid (double dots) '-example.com', # Invalid (starts with a hyphen) 'example-.com', # Invalid (ends with a hyphen) 'example..com', # Invalid (double dots) 'example', # Invalid (no TLD) 'example.local', # Invalid on macOS 'valid-domain.co.uk', # Valid '123.example.com', # Valid 'example.com.', # Invalid (trailing dot) 'toolonglabeltoolonglabeltoolonglabeltoolonglabeltoolonglabeltoolonglabel.com', # Invalid (label > 63 chars) Due to current implementation, I was not able to send emails from Ubuntu. Upon debugging, I found that the SMTP domain being passed to Outlook's servers was not valid. Net::SMTP=GLOB(0x5db4351225f8)>>> EHLO MacBook.. Net::SMTP=GLOB(0x5db4351225f8)<<< 501 5.5.4 Invalid domain name Net::SMTP=GLOB(0x5db4351225f8)>>> HELO MacBook.. Notice that an invalid domain name "MacBook.." is sent by git-send-email. We have a fallback code that checks output from Net::Domain::domainname() or asking domain method of an Net::SMTP instance to detect a misconfigured hostname and replace it with fallback "localhost.localdomain", but the valid_fqdn apparently is failing to say "MacBook.." is not a valid fqdn. With this patch, the rule used in valid_fqdn is tightened, the beginning part of the SMTP exchange looked like this: Net::SMTP=GLOB(0x58c8af71e930)>>> EHLO localhost.localdomain Net::SMTP=GLOB(0x58c8af71e930)<<< 250-PN4P287CA0064.outlook.office365.com Hello [1]: https://datatracker.ietf.org/doc/html/rfc1035 Signed-off-by: Aditya Garg Signed-off-by: Junio C Hamano --- git-send-email.perl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/git-send-email.perl b/git-send-email.perl index 1f613fa979df45..caffee5dc392ff 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1354,7 +1354,9 @@ sub process_address_list { sub valid_fqdn { my $domain = shift; - return defined $domain && !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./; + my $subdomain = '(?!-)[A-Za-z0-9-]{1,63}(? Date: Thu, 8 May 2025 17:14:28 +0000 Subject: [PATCH 04/17] docs: improve send-email documentation OAuth2.0 is a new authentication method that is being used by many email providers, including Outlook and Gmail. Recently, the Authen::SASL perl module has been updated to support OAuth2.0 authentication, thus making the git-send-email script be able to use this authentication method as well. So lets improve the documentation to reflect this change. I also had a hard time finding a reliable OAuth2.0 access token generator for Outlook and Gmail. So I added a link to the such generators which I developed myself after seaching through lots of code and API documentation to make things easier for others. Signed-off-by: Aditya Garg Signed-off-by: Junio C Hamano --- Documentation/git-send-email.adoc | 67 +++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 8 deletions(-) diff --git a/Documentation/git-send-email.adoc b/Documentation/git-send-email.adoc index 7f223db42dd313..6fa7f096893868 100644 --- a/Documentation/git-send-email.adoc +++ b/Documentation/git-send-email.adoc @@ -496,12 +496,12 @@ include::includes/cmd-config-section-all.adoc[] include::config/sendemail.adoc[] -EXAMPLES --------- -Use gmail as the smtp server +EXAMPLES OF SMTP SERVERS +------------------------ +Use Gmail as the SMTP Server ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -To use 'git send-email' to send your patches through the GMail SMTP server, -edit ~/.gitconfig to specify your account settings: +To use `git send-email` to send your patches through the Gmail SMTP server, +edit `~/.gitconfig` to specify your account settings: ---- [sendemail] @@ -515,6 +515,41 @@ If you have multi-factor authentication set up on your Gmail account, you can generate an app-specific password for use with 'git send-email'. Visit https://security.google.com/settings/security/apppasswords to create it. +You can also use OAuth2.0 authentication with Gmail. `OAUTHBEARER` and +`XOAUTH2` are common methods used for this type of authentication. Gmail +supports both of them. As an example, if you want to use `OAUTHBEARER`, edit +your `~/.gitconfig` file and add `smtpAuth = OAUTHBEARER` to your account +settings: + +---- +[sendemail] + smtpEncryption = tls + smtpServer = smtp.gmail.com + smtpUser = yourname@gmail.com + smtpServerPort = 587 + smtpAuth = OAUTHBEARER +---- + +Use Microsoft Outlook as the SMTP Server +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Unlike Gmail, Microsoft Outlook no longer supports app-specific passwords. +Therefore, OAuth2.0 authentication must be used for Outlook. Also, it only +supports `XOAUTH2` authentication method. + +Edit `~/.gitconfig` to specify your account settings for Outlook and use its +SMTP server with `git send-email`: + +---- +[sendemail] + smtpEncryption = tls + smtpServer = smtp.office365.com + smtpUser = yourname@outlook.com + smtpServerPort = 587 + smtpAuth = XOAUTH2 +---- + +SENDING PATCHES +--------------- Once your commits are ready to be sent to the mailing list, run the following commands: @@ -523,9 +558,25 @@ following commands: $ git send-email outgoing/* The first time you run it, you will be prompted for your credentials. Enter the -app-specific or your regular password as appropriate. If you have credential -helper configured (see linkgit:git-credential[1]), the password will be saved in -the credential store so you won't have to type it the next time. +app-specific or your regular password as appropriate. + +If you have a credential helper configured (see linkgit:git-credential[1]), the +password will be saved in the credential store so you won't have to type it the +next time. + +If you are using OAuth2.0 authentication, you need to use an access token in +place of a password when prompted. Various OAuth2.0 token generators are +available online. Community maintained credential helpers for Gmail and Outlook +are also available: + + - https://github.com/AdityaGarg8/git-credential-email[git-credential-gmail] + (cross platform, dedicated helper for authenticating Gmail accounts) + + - https://github.com/AdityaGarg8/git-credential-email[git-credential-outlook] + (cross platform, dedicated helper for authenticating Microsoft Outlook accounts) + +You can also see linkgit:gitcredentials[7] for more OAuth based authentication +helpers. Note: the following core Perl modules that may be installed with your distribution of Perl are required: From ba998f61072943aa8205bfaf966412ecc9cb7af9 Mon Sep 17 00:00:00 2001 From: Aditya Garg Date: Thu, 8 May 2025 17:14:29 +0000 Subject: [PATCH 05/17] docs: add credential helper for outlook and gmail in OAuth list of helpers This commit adds the `git-credential-outlook` and `git-credential-gmail` helpers to the list of OAuth helpers. Signed-off-by: Aditya Garg Signed-off-by: Junio C Hamano --- Documentation/gitcredentials.adoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/gitcredentials.adoc b/Documentation/gitcredentials.adoc index 3337bb475de4c5..b49923db026fe2 100644 --- a/Documentation/gitcredentials.adoc +++ b/Documentation/gitcredentials.adoc @@ -133,6 +133,10 @@ Popular helpers with OAuth support include: - https://github.com/hickford/git-credential-oauth[git-credential-oauth] (cross platform, included in many Linux distributions) + - https://github.com/AdityaGarg8/git-credential-email[git-credential-gmail] (cross platform, dedicated helper to authenticate Gmail accounts for linkgit:git-send-email[1]) + + - https://github.com/AdityaGarg8/git-credential-email[git-credential-outlook] (cross platform, dedicated helper to authenticate Microsoft Outlook accounts for linkgit:git-send-email[1]) + CREDENTIAL CONTEXTS ------------------- From 74727214639d7d8635f667111f4fd6a3295a18bb Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 9 May 2025 16:22:26 +0000 Subject: [PATCH 06/17] sequencer: move reflog message functions In the next commit these functions will be called from pick_one_commit() so move them above that function to avoid a forward declaration. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 66 ++++++++++++++++++++++++++--------------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/sequencer.c b/sequencer.c index 407ee4e90fea68..78db77c36ce91b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2235,6 +2235,39 @@ static void refer_to_commit(struct replay_opts *opts, } } +static const char *sequencer_reflog_action(struct replay_opts *opts) +{ + if (!opts->reflog_action) { + opts->reflog_action = getenv(GIT_REFLOG_ACTION); + opts->reflog_action = + xstrdup(opts->reflog_action ? opts->reflog_action + : action_name(opts)); + } + + return opts->reflog_action; +} + +__attribute__((format (printf, 3, 4))) +static const char *reflog_message(struct replay_opts *opts, + const char *sub_action, const char *fmt, ...) +{ + va_list ap; + static struct strbuf buf = STRBUF_INIT; + + va_start(ap, fmt); + strbuf_reset(&buf); + strbuf_addstr(&buf, sequencer_reflog_action(opts)); + if (sub_action) + strbuf_addf(&buf, " (%s)", sub_action); + if (fmt) { + strbuf_addstr(&buf, ": "); + strbuf_vaddf(&buf, fmt, ap); + } + va_end(ap); + + return buf.buf; +} + static int do_pick_commit(struct repository *r, struct todo_item *item, struct replay_opts *opts, @@ -3922,39 +3955,6 @@ static int do_label(struct repository *r, const char *name, int len) return ret; } -static const char *sequencer_reflog_action(struct replay_opts *opts) -{ - if (!opts->reflog_action) { - opts->reflog_action = getenv(GIT_REFLOG_ACTION); - opts->reflog_action = - xstrdup(opts->reflog_action ? opts->reflog_action - : action_name(opts)); - } - - return opts->reflog_action; -} - -__attribute__((format (printf, 3, 4))) -static const char *reflog_message(struct replay_opts *opts, - const char *sub_action, const char *fmt, ...) -{ - va_list ap; - static struct strbuf buf = STRBUF_INIT; - - va_start(ap, fmt); - strbuf_reset(&buf); - strbuf_addstr(&buf, sequencer_reflog_action(opts)); - if (sub_action) - strbuf_addf(&buf, " (%s)", sub_action); - if (fmt) { - strbuf_addstr(&buf, ": "); - strbuf_vaddf(&buf, fmt, ap); - } - va_end(ap); - - return buf.buf; -} - static struct commit *lookup_label(struct repository *r, const char *label, int len, struct strbuf *buf) { From 5dbaec628d6dfbdc4db9ac528d2b77cc4286d70a Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 9 May 2025 16:22:27 +0000 Subject: [PATCH 07/17] sequencer: rework reflog message handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It has been reported that "git rebase --rebase-merges" can create corrupted reflog entries like e9c962f2ea0 HEAD@{8}: �: Merged in (pull request #4441) This is due to a use-after-free bug that happens because reflog_message() uses a static `struct strbuf` and is not called to update the current reflog message stored in `ctx->reflog_message` when creating the merge. This means `ctx->reflog_message` points to a stale reflog message that has been freed by subsequent call to reflog_message() by a command such as `reset` that used the return value directly rather than storing the result in `ctx->reflog_message`. Fix this by creating the reflog message nearer to where the commit is created and storing it in a local variable which is passed as an additional parameter to run_git_commit() rather than storing the message in `struct replay_ctx`. This makes it harder to forget to call `reflog_message()` before creating a commit and using a variable with a narrower scope means that a stale value cannot carried across a from one iteration of the loop to the next which should prevent any similar use-after-free bugs in the future. A existing test is modified to demonstrate that merges are now created with the correct reflog message. Reported-by: Kristoffer Haugsbakk Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 50 +++++++++++++++++++--------------------- t/t3430-rebase-merges.sh | 11 ++++++++- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/sequencer.c b/sequencer.c index 78db77c36ce91b..c4b3cb245ce4af 100644 --- a/sequencer.c +++ b/sequencer.c @@ -224,11 +224,6 @@ struct replay_ctx { * current chain. */ struct strbuf current_fixups; - /* - * Stores the reflog message that will be used when creating a - * commit. Points to a static buffer and should not be free()'d. - */ - const char *reflog_message; /* * The number of completed fixup and squash commands in the * current chain. @@ -1133,10 +1128,10 @@ static int run_command_silent_on_success(struct child_process *cmd) * author metadata. */ static int run_git_commit(const char *defmsg, + const char *reflog_action, struct replay_opts *opts, unsigned int flags) { - struct replay_ctx *ctx = opts->ctx; struct child_process cmd = CHILD_PROCESS_INIT; if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG)) @@ -1154,7 +1149,7 @@ static int run_git_commit(const char *defmsg, gpg_opt, gpg_opt); } - strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", ctx->reflog_message); + strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", reflog_action); if (opts->committer_date_is_author_date) strvec_pushf(&cmd.env, "GIT_COMMITTER_DATE=%s", @@ -1538,10 +1533,10 @@ static int parse_head(struct repository *r, struct commit **head) */ static int try_to_commit(struct repository *r, struct strbuf *msg, const char *author, + const char *reflog_action, struct replay_opts *opts, unsigned int flags, struct object_id *oid) { - struct replay_ctx *ctx = opts->ctx; struct object_id tree; struct commit *current_head = NULL; struct commit_list *parents = NULL; @@ -1703,7 +1698,7 @@ static int try_to_commit(struct repository *r, goto out; } - if (update_head_with_reflog(current_head, oid, ctx->reflog_message, + if (update_head_with_reflog(current_head, oid, reflog_action, msg, &err)) { res = error("%s", err.buf); goto out; @@ -1734,6 +1729,7 @@ static int write_rebase_head(struct object_id *oid) static int do_commit(struct repository *r, const char *msg_file, const char *author, + const char *reflog_action, struct replay_opts *opts, unsigned int flags, struct object_id *oid) { @@ -1749,7 +1745,7 @@ static int do_commit(struct repository *r, msg_file); res = try_to_commit(r, msg_file ? &sb : NULL, - author, opts, flags, &oid); + author, reflog_action, opts, flags, &oid); strbuf_release(&sb); if (!res) { refs_delete_ref(get_main_ref_store(r), "", @@ -1765,7 +1761,7 @@ static int do_commit(struct repository *r, if (is_rebase_i(opts) && oid) if (write_rebase_head(oid)) return -1; - return run_git_commit(msg_file, opts, flags); + return run_git_commit(msg_file, reflog_action, opts, flags); } return res; @@ -2278,13 +2274,19 @@ static int do_pick_commit(struct repository *r, const char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r); struct object_id head; struct commit *base, *next, *parent; - const char *base_label, *next_label; + const char *base_label, *next_label, *reflog_action; char *author = NULL; struct commit_message msg = { NULL, NULL, NULL, NULL }; int res, unborn = 0, reword = 0, allow, drop_commit; enum todo_command command = item->command; struct commit *commit = item->commit; + if (is_rebase_i(opts)) + reflog_action = reflog_message( + opts, command_to_string(item->command), NULL); + else + reflog_action = sequencer_reflog_action(opts); + if (opts->no_commit) { /* * We do not intend to commit immediately. We just want to @@ -2536,14 +2538,15 @@ static int do_pick_commit(struct repository *r, } /* else allow == 0 and there's nothing special to do */ if (!opts->no_commit && !drop_commit) { if (author || command == TODO_REVERT || (flags & AMEND_MSG)) - res = do_commit(r, msg_file, author, opts, flags, + res = do_commit(r, msg_file, author, reflog_action, + opts, flags, commit? &commit->object.oid : NULL); else res = error(_("unable to parse commit author")); *check_todo = !!(flags & EDIT_MSG); if (!res && reword) { fast_forward_edit: - res = run_git_commit(NULL, opts, EDIT_MSG | + res = run_git_commit(NULL, reflog_action, opts, EDIT_MSG | VERIFY_MSG | AMEND_MSG | (flags & ALLOW_EMPTY)); *check_todo = 1; @@ -4092,6 +4095,7 @@ static int do_merge(struct repository *r, int merge_arg_len, oneline_offset, can_fast_forward, ret, k; static struct lock_file lock; const char *p; + const char *reflog_action = reflog_message(opts, "merge", NULL); if (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) { ret = -1; @@ -4370,14 +4374,15 @@ static int do_merge(struct repository *r, * value (a negative one would indicate that the `merge` * command needs to be rescheduled). */ - ret = !!run_git_commit(git_path_merge_msg(r), opts, - run_commit_flags); + ret = !!run_git_commit(git_path_merge_msg(r), reflog_action, + opts, run_commit_flags); if (!ret && flags & TODO_EDIT_MERGE_MSG) { fast_forward_edit: *check_todo = 1; run_commit_flags |= AMEND_MSG | EDIT_MSG | VERIFY_MSG; - ret = !!run_git_commit(NULL, opts, run_commit_flags); + ret = !!run_git_commit(NULL, reflog_action, opts, + run_commit_flags); } @@ -4892,13 +4897,9 @@ static int pick_one_commit(struct repository *r, struct replay_opts *opts, int *check_todo, int* reschedule) { - struct replay_ctx *ctx = opts->ctx; int res; struct todo_item *item = todo_list->items + todo_list->current; const char *arg = todo_item_get_arg(todo_list, item); - if (is_rebase_i(opts)) - ctx->reflog_message = reflog_message( - opts, command_to_string(item->command), NULL); res = do_pick_commit(r, item, opts, is_final_fixup(todo_list), check_todo); @@ -4957,7 +4958,6 @@ static int pick_commits(struct repository *r, struct replay_ctx *ctx = opts->ctx; int res = 0, reschedule = 0; - ctx->reflog_message = sequencer_reflog_action(opts); if (opts->allow_ff) assert(!(opts->signoff || opts->no_commit || opts->record_origin || should_edit(opts) || @@ -5218,6 +5218,7 @@ static int commit_staged_changes(struct repository *r, unsigned int flags = ALLOW_EMPTY | EDIT_MSG; unsigned int final_fixup = 0, is_clean; struct strbuf rev = STRBUF_INIT; + const char *reflog_action = reflog_message(opts, "continue", NULL); int ret; if (has_unstaged_changes(r, 1)) { @@ -5380,7 +5381,7 @@ static int commit_staged_changes(struct repository *r, } if (run_git_commit(final_fixup ? NULL : rebase_path_message(), - opts, flags)) { + reflog_action, opts, flags)) { ret = error(_("could not commit staged changes.")); goto out; } @@ -5412,7 +5413,6 @@ static int commit_staged_changes(struct repository *r, int sequencer_continue(struct repository *r, struct replay_opts *opts) { - struct replay_ctx *ctx = opts->ctx; struct todo_list todo_list = TODO_LIST_INIT; int res; @@ -5433,7 +5433,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) unlink(rebase_path_dropped()); } - ctx->reflog_message = reflog_message(opts, "continue", NULL); if (commit_staged_changes(r, opts, &todo_list)) { res = -1; goto release_todo_list; @@ -5485,7 +5484,6 @@ static int single_pick(struct repository *r, TODO_PICK : TODO_REVERT; item.commit = cmit; - opts->ctx->reflog_message = sequencer_reflog_action(opts); return do_pick_commit(r, &item, opts, 0, &check_todo); } diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 2593711fecdc9f..b7da7c364e200c 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -86,7 +86,7 @@ test_expect_success 'create completely different structure' ' test_config sequence.editor \""$PWD"/replace-editor.sh\" && test_tick && git rebase -i -r A main && - test_cmp_graph <<-\EOF + test_cmp_graph <<-\EOF && * Merge the topic branch '\''onebranch'\'' |\ | * D @@ -99,6 +99,15 @@ test_expect_success 'create completely different structure' ' |/ * A EOF + + head="$(git show-ref --verify -s --abbrev HEAD)" && + cat >expect <<-EOF && + $head HEAD@{0}: rebase (finish): returning to refs/heads/main + $head HEAD@{1}: rebase (merge): Merge the topic branch ${SQ}onebranch${SQ} + EOF + + git reflog -n2 HEAD >actual && + test_cmp expect actual ' test_expect_success 'generate correct todo list' ' From 880146aefe0e60e330409a916a0c1b4ac21388c6 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 9 May 2025 21:12:02 +0000 Subject: [PATCH 08/17] Makefile: avoid constant rebuilds with compilation database Many contributors to software use a Language Server Protocol implementation to allow their editor to learn structural information about the code they write and provide additional features, such as jumping to the declaration or definition of a function or type. In C, the usual implementation is clangd, which requires compiling with clang. Because C and C++ projects lack a standard file system layout and build system, unlike languages such as Rust and Go, clangd requires a compilation database to be generated by the clang compiler in order to pass the proper compilation flags and discover all of the files necessary to make the LSP work. This is done by setting GENERATE_COMPILATION_DATABASE to "yes". However, when that's enabled and the user runs "make" a second time, all of the files are re-compiled, which is inconvenient for contributors to Git, since it makes small changes or rebases recompile the entirety of the codebase. This happens because the directory holding the compilation database is updated anytime an object is built, so its modification date will always be newer than the first object built. To solve this, use the same trick we do just above for the .depend directory and filter the compilation database directory out if it already exists, which avoids making it a target to be built. Signed-off-by: brian m. carlson Helped-by: Philippe Blain Signed-off-by: Junio C Hamano --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 97e8385b6643b9..ee36e700e8862c 100644 --- a/Makefile +++ b/Makefile @@ -2757,7 +2757,7 @@ endif compdb_dir = compile_commands ifeq ($(GENERATE_COMPILATION_DATABASE),yes) -missing_compdb_dir = $(compdb_dir) +missing_compdb_dir = $(filter-out $(wildcard $(compdb_dir)), $(compdb_dir)) $(missing_compdb_dir): @mkdir -p $@ From c8e752eaeff299012b582507fed078a7cecbb7a3 Mon Sep 17 00:00:00 2001 From: Lidong Yan <502024330056@smail.nju.edu.cn> Date: Mon, 12 May 2025 12:49:03 +0000 Subject: [PATCH 09/17] reftable/writer: fix memory leak when `padded_write()` fails In reftable/writer.c:padded_write(), if w->writer failed, zeroed allocated in `reftable_calloc` will leak. w->writer could be `reftable_write_data` in reftable/stack.c, and could fail due to some write error. Simply add reftable_free(zeroed) will solve this problem. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> Signed-off-by: Junio C Hamano --- reftable/writer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/reftable/writer.c b/reftable/writer.c index f3ab1035d61d96..d71a56e8fc4f27 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -57,8 +57,10 @@ static int padded_write(struct reftable_writer *w, uint8_t *data, size_t len, return -1; n = w->write(w->write_arg, zeroed, w->pending_padding); - if (n < 0) + if (n < 0) { + reftable_free(zeroed); return n; + } w->pending_padding = 0; reftable_free(zeroed); From 91db6c735dd1da215ae5e12506139f0aba5e426b Mon Sep 17 00:00:00 2001 From: Lidong Yan <502024330056@smail.nju.edu.cn> Date: Mon, 12 May 2025 12:49:04 +0000 Subject: [PATCH 10/17] reftable/writer: fix memory leak when `writer_index_hash()` fails In reftable/writer.c:writer_index_hash(), if `reftable_buf_add` failed, key allocated by `reftable_malloc` will not be insert into `obj_index_tree` thus leaks. Simple add reftable_free(key) will solve this problem. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> Signed-off-by: Junio C Hamano --- reftable/writer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/reftable/writer.c b/reftable/writer.c index d71a56e8fc4f27..5de684b36ff257 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -258,8 +258,10 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has reftable_buf_reset(&key->hash); err = reftable_buf_add(&key->hash, hash->buf, hash->len); - if (err < 0) + if (err < 0) { + reftable_free(key); return err; + } tree_insert(&w->obj_index_tree, key, &obj_index_tree_node_compare); } else { From bac220e154994c3d68089d6860de9f78ce5a01f9 Mon Sep 17 00:00:00 2001 From: Rodrigo Carvalho Date: Sat, 10 May 2025 20:09:09 -0300 Subject: [PATCH 11/17] t1001: replace 'test -f' with 'test_path_is_file' 'test_path_is_file' is a modern path checking method in Git's development. Replace the basic shell command 'test -f' with this approach. Signed-off-by: Rodrigo Carvalho Signed-off-by: Junio C Hamano --- t/t1001-read-tree-m-2way.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh index 88c524f6558eed..59b8e56058e017 100755 --- a/t/t1001-read-tree-m-2way.sh +++ b/t/t1001-read-tree-m-2way.sh @@ -363,7 +363,7 @@ test_expect_success 'a/b (untracked) vs a case setup.' ' test_expect_success 'a/b (untracked) vs a, plus c/d case test.' ' read_tree_u_must_fail -u -m "$treeH" "$treeM" && git ls-files --stage && - test -f a/b + test_path_is_file a/b ' test_expect_success 'read-tree supports the super-prefix' ' From e5dd0a05ed392bc0c2dde84a1ee1d6eaeaac357f Mon Sep 17 00:00:00 2001 From: Lidong Yan <502024330056@smail.nju.edu.cn> Date: Mon, 12 May 2025 02:07:27 +0000 Subject: [PATCH 12/17] builtin/am: fix memory leak in `split_mail_stgit_series` In builtin/am.c:split_mail_stgit_series, if `fopen` failed, `series_dir_buf` allocated by `xstrdup` will leak. Add `free` in `!fp` if branch will prevent the leak. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> Signed-off-by: Junio C Hamano --- builtin/am.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index d1990d7edcbe37..bb36b42aa1deb5 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -848,8 +848,10 @@ static int split_mail_stgit_series(struct am_state *state, const char **paths, series_dir = dirname(series_dir_buf); fp = fopen(*paths, "r"); - if (!fp) + if (!fp) { + free(series_dir_buf); return error_errno(_("could not open '%s' for reading"), *paths); + } while (!strbuf_getline_lf(&sb, fp)) { if (*sb.buf == '#') From 1970333644fad127c68046697b9b86fd8d7f28c2 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 12 May 2025 17:15:56 +0200 Subject: [PATCH 13/17] reftable: fix perf regression when reading blocks of unwanted type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In fd888311fbc (reftable/table: move reading block into block reader, 2025-04-07), we have refactored how reftable blocks are read so that most of the logic is contained in the "block.c" subsystem itself. Most importantly, the whole logic to read the data itself is now contained in that subsystem. This change caused a significant performance regression though when reading blocks that aren't of the specific type one is searching for: Benchmark 1: update-ref: create 100k refs (revision = fd888311fbc~) Time (mean ± σ): 2.171 s ± 0.028 s [User: 1.189 s, System: 0.977 s] Range (min … max): 2.117 s … 2.206 s 10 runs Benchmark 2: update-ref: create 100k refs (revision = fd888311fbc) Time (mean ± σ): 3.418 s ± 0.030 s [User: 2.371 s, System: 1.037 s] Range (min … max): 3.377 s … 3.473 s 10 runs Summary update-ref: create 100k refs (revision = fd888311fbc~) ran 1.57 ± 0.02 times faster than update-ref: create 100k refs (revision = fd888311fbc) The root caute of the performance regression is that we changed when exactly blocks of an uninteresting type are being discarded. Previous to the refactoring in the mentioned commit we'd load the block data, read its type, notice that it's not the wanted type and discard the block. After the commit though we don't discard the block immediately, but we fully decode it only to realize that it's not the desired type. We then discard the block again, but have already performed a bunch of pointless work. Fix the regression by making `reftable_block_init()` return early in case the block is not of the desired type. This fixes the performance hit: Benchmark 1: update-ref: create 100k refs (revision = HEAD~) Time (mean ± σ): 2.712 s ± 0.018 s [User: 1.990 s, System: 0.716 s] Range (min … max): 2.682 s … 2.741 s 10 runs Benchmark 2: update-ref: create 100k refs (revision = HEAD) Time (mean ± σ): 1.670 s ± 0.012 s [User: 0.991 s, System: 0.676 s] Range (min … max): 1.652 s … 1.693 s 10 runs Summary update-ref: create 100k refs (revision = HEAD) ran 1.62 ± 0.02 times faster than update-ref: create 100k refs (revision = HEAD~) Note that the baseline performance is lower than in the original due to a couple of unrelated performance improvements that have landed since the original commit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/block.c | 7 ++++++- reftable/reftable-block.h | 3 ++- reftable/table.c | 11 +---------- t/unit-tests/t-reftable-block.c | 15 ++++++++++----- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/reftable/block.c b/reftable/block.c index 795815b4762991..31569cba593abc 100644 --- a/reftable/block.c +++ b/reftable/block.c @@ -226,7 +226,8 @@ static int read_block(struct reftable_block_source *source, int reftable_block_init(struct reftable_block *block, struct reftable_block_source *source, uint32_t offset, uint32_t header_size, - uint32_t table_block_size, uint32_t hash_size) + uint32_t table_block_size, uint32_t hash_size, + uint8_t want_type) { uint32_t guess_block_size = table_block_size ? table_block_size : DEFAULT_BLOCK_SIZE; @@ -246,6 +247,10 @@ int reftable_block_init(struct reftable_block *block, err = REFTABLE_FORMAT_ERROR; goto done; } + if (want_type != REFTABLE_BLOCK_TYPE_ANY && block_type != want_type) { + err = 1; + goto done; + } block_size = reftable_get_be24(block->block_data.data + header_size + 1); if (block_size > guess_block_size) { diff --git a/reftable/reftable-block.h b/reftable/reftable-block.h index 04c3b518c87460..0b05a8f7e376bc 100644 --- a/reftable/reftable-block.h +++ b/reftable/reftable-block.h @@ -56,7 +56,8 @@ struct reftable_block { int reftable_block_init(struct reftable_block *b, struct reftable_block_source *source, uint32_t offset, uint32_t header_size, - uint32_t table_block_size, uint32_t hash_size); + uint32_t table_block_size, uint32_t hash_size, + uint8_t want_type); /* Release resources allocated by the block. */ void reftable_block_release(struct reftable_block *b); diff --git a/reftable/table.c b/reftable/table.c index ee831276158c93..56362df0eda5a6 100644 --- a/reftable/table.c +++ b/reftable/table.c @@ -173,16 +173,7 @@ int table_init_block(struct reftable_table *t, struct reftable_block *block, return 1; err = reftable_block_init(block, &t->source, next_off, header_off, - t->block_size, hash_size(t->hash_id)); - if (err < 0) - goto done; - - if (want_typ != REFTABLE_BLOCK_TYPE_ANY && block->block_type != want_typ) { - err = 1; - goto done; - } - -done: + t->block_size, hash_size(t->hash_id), want_typ); if (err) reftable_block_release(block); return err; diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c index 7dbd93601c7696..52f1dae1c9576d 100644 --- a/t/unit-tests/t-reftable-block.c +++ b/t/unit-tests/t-reftable-block.c @@ -64,7 +64,8 @@ static void t_ref_block_read_write(void) block_writer_release(&bw); block_source_from_buf(&source ,&block_data); - reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1); + reftable_block_init(&block, &source, 0, header_off, block_size, + REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF); block_iter_init(&it, &block); @@ -153,7 +154,8 @@ static void t_log_block_read_write(void) block_writer_release(&bw); block_source_from_buf(&source, &block_data); - reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1); + reftable_block_init(&block, &source, 0, header_off, block_size, + REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_LOG); block_iter_init(&it, &block); @@ -245,7 +247,8 @@ static void t_obj_block_read_write(void) block_writer_release(&bw); block_source_from_buf(&source, &block_data); - reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1); + reftable_block_init(&block, &source, 0, header_off, block_size, + REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_OBJ); block_iter_init(&it, &block); @@ -329,7 +332,8 @@ static void t_index_block_read_write(void) block_writer_release(&bw); block_source_from_buf(&source, &block_data); - reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1); + reftable_block_init(&block, &source, 0, header_off, block_size, + REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_INDEX); block_iter_init(&it, &block); @@ -411,7 +415,8 @@ static void t_block_iterator(void) check_int(err, >, 0); block_source_from_buf(&source, &data); - reftable_block_init(&block, &source, 0, 0, data.len, REFTABLE_HASH_SIZE_SHA1); + reftable_block_init(&block, &source, 0, 0, data.len, + REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF); err = reftable_block_init_iterator(&block, &it); check_int(err, ==, 0); From 274464683462d04363d2107822b0f9d2d5a27623 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 May 2025 14:50:28 -0400 Subject: [PATCH 14/17] oidmap: rename oidmap_free() to oidmap_clear() This function does not free the oidmap struct itself; it just drops all items from the map (using hashmap_clear_() internally). It should be called oidmap_clear(), per CodingGuidelines. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rev-list.c | 2 +- list-objects-filter.c | 2 +- object-store.c | 2 +- oidmap.c | 2 +- oidmap.h | 5 +++-- sequencer.c | 4 ++-- t/unit-tests/u-oidmap.c | 2 +- 7 files changed, 10 insertions(+), 9 deletions(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index c4cd4ed5c81570..0984b607bf052d 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -924,7 +924,7 @@ int cmd_rev_list(int argc, free((void *)entry->path); } - oidmap_free(&missing_objects, true); + oidmap_clear(&missing_objects, true); } stop_progress(&progress); diff --git a/list-objects-filter.c b/list-objects-filter.c index 7765761b3c6e2a..78b397bc194849 100644 --- a/list-objects-filter.c +++ b/list-objects-filter.c @@ -244,7 +244,7 @@ static void filter_trees_free(void *filter_data) { struct filter_trees_depth_data *d = filter_data; if (!d) return; - oidmap_free(&d->seen_at_depth, 1); + oidmap_clear(&d->seen_at_depth, 1); free(d); } diff --git a/object-store.c b/object-store.c index 6ab50d25d3eb4f..bc24e8082904d5 100644 --- a/object-store.c +++ b/object-store.c @@ -1017,7 +1017,7 @@ void raw_object_store_clear(struct raw_object_store *o) { FREE_AND_NULL(o->alternate_db); - oidmap_free(o->replace_map, 1); + oidmap_clear(o->replace_map, 1); FREE_AND_NULL(o->replace_map); pthread_mutex_destroy(&o->replace_mutex); diff --git a/oidmap.c b/oidmap.c index 8b1bc4dec9496e..508d6c7dec17a7 100644 --- a/oidmap.c +++ b/oidmap.c @@ -22,7 +22,7 @@ void oidmap_init(struct oidmap *map, size_t initial_size) hashmap_init(&map->map, oidmap_neq, NULL, initial_size); } -void oidmap_free(struct oidmap *map, int free_entries) +void oidmap_clear(struct oidmap *map, int free_entries) { if (!map) return; diff --git a/oidmap.h b/oidmap.h index fad412827af688..603ae1adbcc058 100644 --- a/oidmap.h +++ b/oidmap.h @@ -36,12 +36,13 @@ struct oidmap { void oidmap_init(struct oidmap *map, size_t initial_size); /* - * Frees an oidmap structure and allocated memory. + * Clear an oidmap, freeing any allocated memory. The map is empty and + * can be reused without another explicit init. * * If `free_entries` is true, each oidmap_entry in the map is freed as well * using stdlibs free(). */ -void oidmap_free(struct oidmap *map, int free_entries); +void oidmap_clear(struct oidmap *map, int free_entries); /* * Returns the oidmap entry for the specified oid, or NULL if not found. diff --git a/sequencer.c b/sequencer.c index b5c4043757e948..7fa24db14303b0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -6053,8 +6053,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, oidset_clear(&interesting); oidset_clear(&child_seen); oidset_clear(&shown); - oidmap_free(&commit2todo, 1); - oidmap_free(&state.commit2label, 1); + oidmap_clear(&commit2todo, 1); + oidmap_clear(&state.commit2label, 1); hashmap_clear_and_free(&state.labels, struct labels_entry, entry); strbuf_release(&state.buf); diff --git a/t/unit-tests/u-oidmap.c b/t/unit-tests/u-oidmap.c index dc805b7e3cb424..b23af449f6452e 100644 --- a/t/unit-tests/u-oidmap.c +++ b/t/unit-tests/u-oidmap.c @@ -35,7 +35,7 @@ void test_oidmap__initialize(void) void test_oidmap__cleanup(void) { - oidmap_free(&map, 1); + oidmap_clear(&map, 1); } void test_oidmap__replace(void) From 596184786c1b1998573df4c130eadb1668d8c304 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 May 2025 14:51:30 -0400 Subject: [PATCH 15/17] oidmap: add size function Callers which want to know how many items are in an oidmap have to look at the underlying hashmap struct, leaking an implementation detail. Let's provide a type-appropriate wrapper and use it. Note in the call from lookup_replace_object(), the caller was actually looking at the hashmap's tablesize parameter (the allocated size of the table) rather than hashmap_get_size(), the number of items in the table. This probably should have been checking the number of items all along, but the two are functionally equivalent here since we only add to the map and never remove anything. Thus if there was any allocation, it was because there is at least one item. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 2 +- oidmap.h | 4 ++++ replace-object.h | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 6394752b0b0868..1a74e1e1baffbe 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -222,7 +222,7 @@ static int commit_graph_compatible(struct repository *r) if (replace_refs_enabled(r)) { prepare_replace_object(r); - if (hashmap_get_size(&r->objects->replace_map->map)) + if (oidmap_get_size(r->objects->replace_map)) return 0; } diff --git a/oidmap.h b/oidmap.h index 603ae1adbcc058..67fb32290f1c49 100644 --- a/oidmap.h +++ b/oidmap.h @@ -67,6 +67,10 @@ void *oidmap_put(struct oidmap *map, void *entry); */ void *oidmap_remove(struct oidmap *map, const struct object_id *key); +static inline unsigned int oidmap_get_size(struct oidmap *map) +{ + return hashmap_get_size(&map->map); +} struct oidmap_iter { struct hashmap_iter h_iter; diff --git a/replace-object.h b/replace-object.h index ba478eb30c47a6..4226376534b22e 100644 --- a/replace-object.h +++ b/replace-object.h @@ -47,7 +47,7 @@ static inline const struct object_id *lookup_replace_object(struct repository *r { if (!replace_refs_enabled(r) || (r->objects->replace_map_initialized && - r->objects->replace_map->map.tablesize == 0)) + oidmap_get_size(r->objects->replace_map) == 0)) return oid; return do_lookup_replace_object(r, oid); } From 4b63963f5d729cb9eb997c8912b7d500ffc53297 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 May 2025 14:52:33 -0400 Subject: [PATCH 16/17] raw_object_store: drop extra pointer to replace_map We store the replacement data in an oidmap, which is itself a pointer in the raw_object_store struct. But there's no need for an extra pointer indirection here. It is always allocated and initialized along with the containing struct, and we never check it for NULL-ness. Let's embed the map directly in the struct, which is simpler and avoids extra pointer chasing. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 2 +- object-store.c | 3 +-- object-store.h | 3 ++- replace-object.c | 8 +++----- replace-object.h | 2 +- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 1a74e1e1baffbe..4a6e34f8a0a070 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -222,7 +222,7 @@ static int commit_graph_compatible(struct repository *r) if (replace_refs_enabled(r)) { prepare_replace_object(r); - if (oidmap_get_size(r->objects->replace_map)) + if (oidmap_get_size(&r->objects->replace_map)) return 0; } diff --git a/object-store.c b/object-store.c index bc24e8082904d5..911bc7ff5ffdf6 100644 --- a/object-store.c +++ b/object-store.c @@ -1017,8 +1017,7 @@ void raw_object_store_clear(struct raw_object_store *o) { FREE_AND_NULL(o->alternate_db); - oidmap_clear(o->replace_map, 1); - FREE_AND_NULL(o->replace_map); + oidmap_clear(&o->replace_map, 1); pthread_mutex_destroy(&o->replace_mutex); free_commit_graph(o->commit_graph); diff --git a/object-store.h b/object-store.h index 46961dc954257b..9f6f27c016a8ca 100644 --- a/object-store.h +++ b/object-store.h @@ -5,6 +5,7 @@ #include "object.h" #include "list.h" #include "oidset.h" +#include "oidmap.h" #include "thread-utils.h" struct oidmap; @@ -176,7 +177,7 @@ struct raw_object_store { * Objects that should be substituted by other objects * (see git-replace(1)). */ - struct oidmap *replace_map; + struct oidmap replace_map; unsigned replace_map_initialized : 1; pthread_mutex_t replace_mutex; /* protect object replace functions */ diff --git a/replace-object.c b/replace-object.c index 7b8a09b5cb4959..f8c5f68837f12e 100644 --- a/replace-object.c +++ b/replace-object.c @@ -31,7 +31,7 @@ static int register_replace_ref(const char *refname, oidcpy(&repl_obj->replacement, oid); /* Register new object */ - if (oidmap_put(r->objects->replace_map, repl_obj)) + if (oidmap_put(&r->objects->replace_map, repl_obj)) die(_("duplicate replace ref: %s"), refname); return 0; @@ -48,9 +48,7 @@ void prepare_replace_object(struct repository *r) return; } - r->objects->replace_map = - xmalloc(sizeof(*r->objects->replace_map)); - oidmap_init(r->objects->replace_map, 0); + oidmap_init(&r->objects->replace_map, 0); refs_for_each_replace_ref(get_main_ref_store(r), register_replace_ref, r); @@ -80,7 +78,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r, /* Try to recursively replace the object */ while (depth-- > 0) { struct replace_object *repl_obj = - oidmap_get(r->objects->replace_map, cur); + oidmap_get(&r->objects->replace_map, cur); if (!repl_obj) return cur; cur = &repl_obj->replacement; diff --git a/replace-object.h b/replace-object.h index 4226376534b22e..3052e96a6203fd 100644 --- a/replace-object.h +++ b/replace-object.h @@ -47,7 +47,7 @@ static inline const struct object_id *lookup_replace_object(struct repository *r { if (!replace_refs_enabled(r) || (r->objects->replace_map_initialized && - oidmap_get_size(r->objects->replace_map) == 0)) + oidmap_get_size(&r->objects->replace_map) == 0)) return oid; return do_lookup_replace_object(r, oid); } From 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 19 May 2025 15:32:53 -0700 Subject: [PATCH 17/17] The sixteenth batch Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.50.0.adoc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Documentation/RelNotes/2.50.0.adoc b/Documentation/RelNotes/2.50.0.adoc index 02fa875823da8d..bf73de114eb147 100644 --- a/Documentation/RelNotes/2.50.0.adoc +++ b/Documentation/RelNotes/2.50.0.adoc @@ -65,6 +65,13 @@ UI, Workflows & Features * Make repository clean-up tasks "gc" can do available to "git maintenance" front-end. + * Bundle-URI feature did not use refs recorded in the bundle other + than normal branches as anchoring points to optimize the follow-up + fetch during "git clone"; now it is told to utilize all. + + * The `send-email` documentation has been updated with OAuth2.0 + related examples. + Performance, Internal Implementation, Development Support etc. -------------------------------------------------------------- @@ -148,6 +155,8 @@ Performance, Internal Implementation, Development Support etc. * Further code clean-up in the object-store layer. + * Build performance fix. + Fixes since v2.49 ----------------- @@ -294,6 +303,9 @@ Fixes since v2.49 derived systems use different errno, which has been worked around. (merge f47bcc3413 cf/wrapper-bsd-eloop later to maint). + * Use-after-free fix in the sequencer. + (merge 5dbaec628d pw/sequencer-reflog-use-after-free later to maint). + * Other code cleanup, docfix, build fix, etc. (merge 227c4f33a0 ja/doc-block-delimiter-markup-fix later to maint). (merge 2bfd3b3685 ab/decorate-code-cleanup later to maint). @@ -321,3 +333,6 @@ Fixes since v2.49 (merge 42cf4ac552 ps/ci-resurrect-p4-on-github later to maint). (merge 104add8368 js/diff-codeql-false-positive-workaround later to maint). (merge f62977b93c en/get-tree-entry-doc later to maint). + (merge e5dd0a05ed ly/am-split-stgit-leakfix later to maint). + (merge bac220e154 rc/t1001-test-path-is-file later to maint). + (merge 91db6c735d ly/reftable-writer-leakfix later to maint).