From c18eecbe5c44be4c23f978a3f1c74b802d26c277 Mon Sep 17 00:00:00 2001 From: Elijah Conners Date: Wed, 14 Sep 2022 20:37:34 -0700 Subject: [PATCH 01/17] reftable: use a pointer for pq_entry param The speed of the merged_iter_pqueue_add() can be improved by using a pointer to the pq_entry struct, which is 96 bytes. Since the pq_entry param is worked directly on the stack and does not currently have a pointer to it, the merged_iter_pqueue_add() function is slightly slower. References to pq_entry in reftable have typically included pointers, such as both of the params for pq_less(). Since we are working with pointers in the pq_entry param, as keenly pointed out, the pq_entry param has also been made into a const since the contents of the pq_entry param are copied and not manipulated. Signed-off-by: Elijah Conners Signed-off-by: Junio C Hamano --- reftable/merged.c | 4 ++-- reftable/pq.c | 4 ++-- reftable/pq.h | 2 +- reftable/pq_test.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/reftable/merged.c b/reftable/merged.c index 2a6efa110d..5ded470c08 100644 --- a/reftable/merged.c +++ b/reftable/merged.c @@ -36,7 +36,7 @@ static int merged_iter_init(struct merged_iter *mi) .rec = rec, .index = i, }; - merged_iter_pqueue_add(&mi->pq, e); + merged_iter_pqueue_add(&mi->pq, &e); } } @@ -71,7 +71,7 @@ static int merged_iter_advance_nonnull_subiter(struct merged_iter *mi, return 0; } - merged_iter_pqueue_add(&mi->pq, e); + merged_iter_pqueue_add(&mi->pq, &e); return 0; } diff --git a/reftable/pq.c b/reftable/pq.c index 96ca6dd37b..dcefeb793a 100644 --- a/reftable/pq.c +++ b/reftable/pq.c @@ -71,7 +71,7 @@ struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq) return e; } -void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, struct pq_entry e) +void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry *e) { int i = 0; @@ -81,7 +81,7 @@ void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, struct pq_entry e) pq->cap * sizeof(struct pq_entry)); } - pq->heap[pq->len++] = e; + pq->heap[pq->len++] = *e; i = pq->len - 1; while (i > 0) { int j = (i - 1) / 2; diff --git a/reftable/pq.h b/reftable/pq.h index 56fc1b6d87..e85bac9b52 100644 --- a/reftable/pq.h +++ b/reftable/pq.h @@ -26,7 +26,7 @@ struct pq_entry merged_iter_pqueue_top(struct merged_iter_pqueue pq); int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq); void merged_iter_pqueue_check(struct merged_iter_pqueue pq); struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq); -void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, struct pq_entry e); +void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry *e); void merged_iter_pqueue_release(struct merged_iter_pqueue *pq); int pq_less(struct pq_entry *a, struct pq_entry *b); diff --git a/reftable/pq_test.c b/reftable/pq_test.c index 7de5e886f3..011b5c7502 100644 --- a/reftable/pq_test.c +++ b/reftable/pq_test.c @@ -46,7 +46,7 @@ static void test_pq(void) .u.ref = { .refname = names[i], } } }; - merged_iter_pqueue_add(&pq, e); + merged_iter_pqueue_add(&pq, &e); merged_iter_pqueue_check(pq); i = (i * 7) % N; } while (i != 1); From 086eaab8da7da69907cfc1461d912aea2827406b Mon Sep 17 00:00:00 2001 From: Todd Zullinger Date: Fri, 16 Sep 2022 02:23:02 -0400 Subject: [PATCH 02/17] docs: fix a few recently broken links Some links were broken in the recent move of various technical docs c0f6dd49f1 (Merge branch 'ab/tech-docs-to-help', 2022-08-14). Fix them. Signed-off-by: Todd Zullinger Signed-off-by: Junio C Hamano --- Documentation/gitprotocol-capabilities.txt | 4 ++-- Documentation/gitprotocol-v2.txt | 4 ++-- Documentation/technical/bundle-uri.txt | 3 +-- Documentation/user-manual.txt | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/Documentation/gitprotocol-capabilities.txt b/Documentation/gitprotocol-capabilities.txt index c6dcc7d565..0fb5ea0c1c 100644 --- a/Documentation/gitprotocol-capabilities.txt +++ b/Documentation/gitprotocol-capabilities.txt @@ -388,8 +388,8 @@ the server as well. Session IDs should be unique to a given process. They must fit within a packet-line, and must not contain non-printable or whitespace characters. The current implementation uses trace2 session IDs (see -link:api-trace2.html[api-trace2] for details), but this may change and users of -the session ID should not rely on this fact. +link:technical/api-trace2.html[api-trace2] for details), but this may change +and users of the session ID should not rely on this fact. GIT --- diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt index c9c0f9160b..59bf41cefb 100644 --- a/Documentation/gitprotocol-v2.txt +++ b/Documentation/gitprotocol-v2.txt @@ -544,8 +544,8 @@ the server as well. Session IDs should be unique to a given process. They must fit within a packet-line, and must not contain non-printable or whitespace characters. The current implementation uses trace2 session IDs (see -link:api-trace2.html[api-trace2] for details), but this may change and users of -the session ID should not rely on this fact. +link:technical/api-trace2.html[api-trace2] for details), but this may change +and users of the session ID should not rely on this fact. object-info ~~~~~~~~~~~ diff --git a/Documentation/technical/bundle-uri.txt b/Documentation/technical/bundle-uri.txt index c25c42378a..85c6a7fc7c 100644 --- a/Documentation/technical/bundle-uri.txt +++ b/Documentation/technical/bundle-uri.txt @@ -3,8 +3,7 @@ Bundle URIs Git bundles are files that store a pack-file along with some extra metadata, including a set of refs and a (possibly empty) set of necessary commits. See -linkgit:git-bundle[1] and link:bundle-format.txt[the bundle format] for more -information. +linkgit:git-bundle[1] and linkgit:gitformat-bundle[5] for more information. Bundle URIs are locations where Git can download one or more bundles in order to bootstrap the object database in advance of fetching the remaining diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt index ca9decdd95..dc9c6a663a 100644 --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -3133,7 +3133,7 @@ those "loose" objects. You can save space and make Git faster by moving these loose objects in to a "pack file", which stores a group of objects in an efficient compressed format; the details of how pack files are formatted can be -found in link:gitformat-pack[5]. +found in linkgit:gitformat-pack[5]. To put the loose objects into a pack, just run git repack: From 4945f046c7f5ef6e84f06a5f4abb1bbd18c1eb85 Mon Sep 17 00:00:00 2001 From: Todd Zullinger Date: Fri, 16 Sep 2022 02:23:03 -0400 Subject: [PATCH 03/17] api docs: link to html version of api-trace2 In f6d25d7878 (api docs: document that BUG() emits a trace2 error event, 2021-04-13), a link to the plain text version of api-trace2 was added in `technical/api-error-handling.txt`. All of our other `link:`s point to the html versions. Do the same here. Signed-off-by: Todd Zullinger Signed-off-by: Junio C Hamano --- Documentation/technical/api-error-handling.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt index 70bf1d3e52..665c4960b4 100644 --- a/Documentation/technical/api-error-handling.txt +++ b/Documentation/technical/api-error-handling.txt @@ -46,7 +46,7 @@ parse-options.c. returns -1 after reporting the situation to the caller. These reports will be logged via the trace2 facility. See the "error" -event in link:api-trace2.txt[trace2 API]. +event in link:api-trace2.html[trace2 API]. Customizable error handlers --------------------------- From 225e815ef238d6033c7f78160274b96de7b197f9 Mon Sep 17 00:00:00 2001 From: Fangyi Zhou Date: Fri, 16 Sep 2022 13:05:29 +0000 Subject: [PATCH 04/17] help: fix doubled words in explanation for developer interfaces Signed-off-by: Fangyi Zhou Signed-off-by: Junio C Hamano --- help.c | 2 +- t/t0012-help.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index ec670d5f68..d04542d826 100644 --- a/help.c +++ b/help.c @@ -39,7 +39,7 @@ static struct category_description main_categories[] = { { CAT_synchingrepositories, N_("Low-level Commands / Syncing Repositories") }, { CAT_purehelpers, N_("Low-level Commands / Internal Helpers") }, { CAT_userinterfaces, N_("User-facing repository, command and file interfaces") }, - { CAT_developerinterfaces, N_("Developer-facing file file formats, protocols and interfaces") }, + { CAT_developerinterfaces, N_("Developer-facing file formats, protocols and other interfaces") }, { 0, NULL } }; diff --git a/t/t0012-help.sh b/t/t0012-help.sh index 4ed2f242eb..dbfc5c8267 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -231,7 +231,7 @@ test_expect_success "'git help -a' section spacing" ' User-facing repository, command and file interfaces - Developer-facing file file formats, protocols and interfaces + Developer-facing file formats, protocols and other interfaces EOF test_cmp expect actual ' From cb98e1d50a7a4a84b76f72dad694d49d2276eef3 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Sat, 17 Sep 2022 18:16:55 +0000 Subject: [PATCH 05/17] diagnose.c: refactor to safely use 'd_type' Refactor usage of the 'd_type' property of 'struct dirent' in 'diagnose.c' to instead utilize the compatibility macro 'DTYPE()'. On systems where 'd_type' is not present in 'struct dirent', this macro will always return 'DT_UNKNOWN'. In that case, instead fall back on using the 'stat.st_mode' to determine whether the dirent points to a dir, file, or link. Additionally, add a test to 't0092-diagnose.sh' to verify that files (e.g., loose objects) are counted properly. Note that the new function 'get_dtype()' is based on 'resolve_dtype()' in 'dir.c' (which itself was refactored from a prior 'get_dtype()' in ad6f2157f9 (dir: restructure in a way to avoid passing around a struct dirent, 2020-01-16)), but differs in that it is meant for use on arbitrary files, such as those inside the '.git' dir. Because of this, it does not search the index for a matching entry to derive the 'd_type'. Reported-by: Randall S. Becker Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- diagnose.c | 70 ++++++++++++++++++++++++++++++++++++--------- t/t0092-diagnose.sh | 12 ++++++++ 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/diagnose.c b/diagnose.c index beb0a8741b..8f26569896 100644 --- a/diagnose.c +++ b/diagnose.c @@ -66,17 +66,53 @@ static int dir_file_stats(struct object_directory *object_dir, void *data) return 0; } -static int count_files(char *path) +/* + * Get the d_type of a dirent. If the d_type is unknown, derive it from + * stat.st_mode. + * + * Note that 'path' is assumed to have a trailing slash. It is also modified + * in-place during the execution of the function, but is then reverted to its + * original value before returning. + */ +static unsigned char get_dtype(struct dirent *e, struct strbuf *path) { - DIR *dir = opendir(path); + struct stat st; + unsigned char dtype = DTYPE(e); + size_t base_path_len; + + if (dtype != DT_UNKNOWN) + return dtype; + + /* d_type unknown in dirent, try to fall back on lstat results */ + base_path_len = path->len; + strbuf_addstr(path, e->d_name); + if (lstat(path->buf, &st)) + goto cleanup; + + /* determine d_type from st_mode */ + if (S_ISREG(st.st_mode)) + dtype = DT_REG; + else if (S_ISDIR(st.st_mode)) + dtype = DT_DIR; + else if (S_ISLNK(st.st_mode)) + dtype = DT_LNK; + +cleanup: + strbuf_setlen(path, base_path_len); + return dtype; +} + +static int count_files(struct strbuf *path) +{ + DIR *dir = opendir(path->buf); struct dirent *e; int count = 0; if (!dir) return 0; - while ((e = readdir(dir)) != NULL) - if (!is_dot_or_dotdot(e->d_name) && e->d_type == DT_REG) + while ((e = readdir_skip_dot_and_dotdot(dir)) != NULL) + if (get_dtype(e, path) == DT_REG) count++; closedir(dir); @@ -104,13 +140,13 @@ static void loose_objs_stats(struct strbuf *buf, const char *path) strbuf_addch(&count_path, '/'); base_path_len = count_path.len; - while ((e = readdir(dir)) != NULL) - if (!is_dot_or_dotdot(e->d_name) && - e->d_type == DT_DIR && strlen(e->d_name) == 2 && + while ((e = readdir_skip_dot_and_dotdot(dir)) != NULL) + if (get_dtype(e, &count_path) == DT_DIR && + strlen(e->d_name) == 2 && !hex_to_bytes(&c, e->d_name, 1)) { strbuf_setlen(&count_path, base_path_len); - strbuf_addstr(&count_path, e->d_name); - total += (count = count_files(count_path.buf)); + strbuf_addf(&count_path, "%s/", e->d_name); + total += (count = count_files(&count_path)); strbuf_addf(buf, "%s : %7d files\n", e->d_name, count); } @@ -144,22 +180,28 @@ static int add_directory_to_archiver(struct strvec *archiver_args, len = buf.len; strvec_pushf(archiver_args, "--prefix=%s", buf.buf); - while (!res && (e = readdir(dir))) { - if (!strcmp(".", e->d_name) || !strcmp("..", e->d_name)) - continue; + while (!res && (e = readdir_skip_dot_and_dotdot(dir))) { + struct strbuf abspath = STRBUF_INIT; + unsigned char dtype; + + strbuf_add_absolute_path(&abspath, at_root ? "." : path); + strbuf_addch(&abspath, '/'); + dtype = get_dtype(e, &abspath); strbuf_setlen(&buf, len); strbuf_addstr(&buf, e->d_name); - if (e->d_type == DT_REG) + if (dtype == DT_REG) strvec_pushf(archiver_args, "--add-file=%s", buf.buf); - else if (e->d_type != DT_DIR) + else if (dtype != DT_DIR) warning(_("skipping '%s', which is neither file nor " "directory"), buf.buf); else if (recurse && add_directory_to_archiver(archiver_args, buf.buf, recurse) < 0) res = -1; + + strbuf_release(&abspath); } closedir(dir); diff --git a/t/t0092-diagnose.sh b/t/t0092-diagnose.sh index fca9b58489..133e5747d6 100755 --- a/t/t0092-diagnose.sh +++ b/t/t0092-diagnose.sh @@ -28,12 +28,23 @@ test_expect_success UNZIP 'creates diagnostics zip archive' ' ! "$GIT_UNZIP" -l "$zip_path" | grep ".git/" ' +test_expect_success UNZIP 'counts loose objects' ' + test_commit A && + + # After committing, should have non-zero loose objects + git diagnose -o test-count -s 1 >out && + zip_path=test-count/git-diagnostics-1.zip && + "$GIT_UNZIP" -p "$zip_path" objects-local.txt >out && + grep "^Total: [1-9][0-9]* loose objects" out +' + test_expect_success UNZIP '--mode=stats excludes .git dir contents' ' test_when_finished rm -rf report && git diagnose -o report -s test --mode=stats >out && # Includes pack quantity/size info + zip_path=report/git-diagnostics-test.zip && "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && grep ".git/objects" out && @@ -47,6 +58,7 @@ test_expect_success UNZIP '--mode=all includes .git dir contents' ' git diagnose -o report -s test --mode=all >out && # Includes pack quantity/size info + zip_path=report/git-diagnostics-test.zip && "$GIT_UNZIP" -p "$zip_path" packs-local.txt >out && grep ".git/objects" out && From 12f1ae53243d3ff06a956da1846dde6f32498342 Mon Sep 17 00:00:00 2001 From: Miaoqian Lin Date: Mon, 19 Sep 2022 18:14:40 +0400 Subject: [PATCH 06/17] commit-graph: Fix missing closedir in expire_commit_graphs The function calls opendir() but missing the corresponding closedir() before exit the function. Add missing closedir() to fix it. Signed-off-by: Miaoqian Lin Reviewed-by: Derrick Stolee Signed-off-by: Junio C Hamano --- commit-graph.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/commit-graph.c b/commit-graph.c index 2b52818731..0d44cd0fa4 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2269,6 +2269,8 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx) } out: + if(dir) + closedir(dir); strbuf_release(&path); } From e01b851923d43cbd3c5b7055f689cc18283591b9 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Mon, 19 Sep 2022 19:12:46 +0000 Subject: [PATCH 07/17] Documentation: add ReviewingGuidelines Add a reviewing guidelines document including advice and common terminology used in Git mailing list reviews. The document is included in the 'TECH_DOCS' list in order to include it in Git's published documentation. Helped-by: Johannes Schindelin Helped-by: Derrick Stolee Helped-by: Junio C Hamano Helped-by: Josh Steadmon Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/Makefile | 1 + Documentation/ReviewingGuidelines.txt | 162 ++++++++++++++++++++++++++ 2 files changed, 163 insertions(+) create mode 100644 Documentation/ReviewingGuidelines.txt diff --git a/Documentation/Makefile b/Documentation/Makefile index 4f801f4e4c..d4c389324e 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -90,6 +90,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt))) SP_ARTICLES += $(API_DOCS) +TECH_DOCS += ReviewingGuidelines TECH_DOCS += MyFirstContribution TECH_DOCS += MyFirstObjectWalk TECH_DOCS += SubmittingPatches diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt new file mode 100644 index 0000000000..0e323d5477 --- /dev/null +++ b/Documentation/ReviewingGuidelines.txt @@ -0,0 +1,162 @@ +Reviewing Patches in the Git Project +==================================== + +Introduction +------------ +The Git development community is a widely distributed, diverse, ever-changing +group of individuals. Asynchronous communication via the Git mailing list poses +unique challenges when reviewing or discussing patches. This document contains +some guiding principles and helpful tools you can use to make your reviews both +more efficient for yourself and more effective for other contributors. + +Note that none of the recommendations here are binding or in any way a +requirement of participation in the Git community. They are provided as a +resource to supplement your skills as a contributor. + +Principles +---------- + +Selecting patch(es) to review +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +If you are looking for a patch series in need of review, start by checking +latest "What's cooking in git.git" email +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's +cooking" emails & replies can be found using the query `s:"What's cooking"` on +the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive]; +alternatively, you can find the contents of the "What's cooking" email tracked +in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with "Needs +review" and those in the "[New Topics]" section are typically those that would +benefit the most from additional review. + +Patches can also be searched manually in the mailing list archive using a query +like `s:"PATCH" -s:"Re:"`. You can browse these results for topics relevant to +your expertise or interest. + +If you've already contributed to Git, you may also be CC'd in another +contributor's patch series. These are topics where the author feels that your +attention is warranted. This may be because their patch changes something you +wrote previously (making you a good judge of whether the new approach does or +doesn't work), or because you have the expertise to provide an exceptionally +helpful review. There is no requirement to review these patches but, in the +spirit of open source collaboration, you should strongly consider doing so. + +Reviewing patches +~~~~~~~~~~~~~~~~~ +While every contributor takes their own approach to reviewing patches, here are +some general pieces of advice to make your reviews as clear and helpful as +possible. The advice is broken into two rough categories: high-level reviewing +guidance, and concrete tips for interacting with patches on the mailing list. + +==== High-level guidance +- Remember to review the content of commit messages for correctness and clarity, + in addition to the code change in the patch's diff. The commit message of a + patch should accurately and fully explain the code change being made in the + diff. + +- Reviewing test coverage is an important - but easy to overlook - component of + reviews. A patch's changes may be covered by existing tests, or new tests may + be introduced to exercise new behavior. Checking out a patch or series locally + allows you to manually mutate lines of new & existing tests to verify expected + pass/fail behavior. You can use this information to verify proper coverage or + to suggest additional tests the author could add. + +- When providing a recommendation, be as clear as possible about whether you + consider it "blocking" (the code would be broken or otherwise made worse if an + issue isn't fixed) or "non-blocking" (the patch could be made better by taking + the recommendation, but acceptance of the series does not require it). + Non-blocking recommendations can be particularly ambiguous when they are + related to - but outside the scope of - a series ("nice-to-have"s), or when + they represent only stylistic differences between the author and reviewer. + +- When commenting on an issue, try to include suggestions for how the author + could fix it. This not only helps the author to understand and fix the issue, + it also deepens and improves your understanding of the topic. + +- Reviews do not need to exclusively point out problems. Feel free to "think out + loud" in your review: describe how you read & understood a complex section of + a patch, ask a question about something that confused you, point out something + you found exceptionally well-written, etc. In particular, uplifting feedback + goes a long way towards encouraging contributors to participate more actively + in the Git community. + +==== Performing your review +- Provide your review comments per-patch in a plaintext "Reply-All" email to the + relevant patch. Comments should be made inline, immediately below the relevant + section(s). + +- You may find that the limited context provided in the patch diff is sometimes + insufficient for a thorough review. In such cases, you can review patches in + your local tree by either applying patches with linkgit:git-am[1] or checking + out the associated branch from https://github.com/gitster/git once the series + is tracked there. + +- Large, complicated patch diffs are sometimes unavoidable, such as when they + refactor existing code. If you find such a patch difficult to parse, try + reviewing the diff produced with the `--color-moved` and/or + `--ignore-space-change` options. + +- If a patch is long, you are encouraged to delete parts of it that are + unrelated to your review from the email reply. Make sure to leave enough + context for readers to understand your comments! + +- If you cannot complete a full review of a series all at once, consider letting + the author know (on- or off-list) if/when you plan to review the rest of the + series. + +Completing a review +~~~~~~~~~~~~~~~~~~~ +Once each patch of a series is reviewed, the author (and/or other contributors) +may discuss the review(s). This may result in no changes being applied, or the +author will send a new version of their patch(es). + +After a series is rerolled in response to your or others' review, make sure to +re-review the updates. If you are happy with the state of the patch series, +explicitly indicate your approval (typically with a reply to the latest +version's cover letter). Optionally, you can let the author know that they can +add a "Reviewed-by: " trailer if they resubmit the reviewed patch verbatim +in a later iteration of the series. + +Finally, subsequent "What's cooking" emails may explicitly ask whether a +reviewed topic is ready for merging to the `next` branch (typically phrased +"Will merge to \'next\'?"). You can help the maintainer and author by responding +with a short description of the state of your (and others', if applicable) +review, including the links to the relevant thread(s). + +Terminology +----------- +nit: :: + Denotes a small issue that should be fixed, such as a typographical error + or mis-alignment of conditions in an `if()` statement. + +aside: :: +optional: :: +non-blocking: :: + Indicates to the reader that the following comment should not block the + acceptance of the patch or series. These are typically recommendations + related to code organization & style, or musings about topics related to + the patch in question, but beyond its scope. + +s///:: + Shorthand for "you wrote , but I think you meant ," usually + for misspellings or other typographical errors. The syntax is a reference + to "substitute" command commonly found in Unix tools such as `ed`, `sed`, + `vim`, and `perl`. + +cover letter:: + The "Patch 0" of a multi-patch series. This email describes the + high-level intent and structure of the patch series to readers on the + Git mailing list. It is also where the changelog notes and range-diff of + subsequent versions are provided by the author. ++ +On single-patch submissions, cover letter content is typically not sent as a +separate email. Instead, it is inserted between the end of the patch's commit +message (after the `---`) and the beginning of the diff. + +#leftoverbits:: + Used by either an author or a reviewer to describe features or suggested + changes that are out-of-scope of a given patch or series, but are relevant + to the topic for the sake of discussion. + +See Also +-------- +link:MyFirstContribution.html[MyFirstContribution] From 89c8048855e7193988a7991ad01af0c6d8cf9226 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 20 Sep 2022 00:19:54 +0000 Subject: [PATCH 08/17] diagnose: add to command-list.txt Add 'git diagnose' as an "ancilliaryinterrogator" (like 'git bugreport') to 'command-list.txt' in order to have it show up in 'git help -a' and avoid the "no link" warning message from the 'check-docs' Makefile target. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- command-list.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/command-list.txt b/command-list.txt index 93f94e42ab..bb2e0a9214 100644 --- a/command-list.txt +++ b/command-list.txt @@ -91,6 +91,7 @@ git-cvsimport foreignscminterface git-cvsserver foreignscminterface git-daemon synchingrepositories git-describe mainporcelain +git-diagnose ancillaryinterrogators git-diff mainporcelain info git-diff-files plumbinginterrogators git-diff-index plumbinginterrogators From 9b1dc1c9d879368b6cfccb183f5fc19facb1d9df Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 20 Sep 2022 00:19:55 +0000 Subject: [PATCH 09/17] version: fix builtin linking & documentation Like most builtins, 'version' is documented in a corresponding 'Documentation/git-version.txt' and can be invoked with 'git version'. However, the 'check-docs' Makefile target showed that it was "removed but documented: git-version." This was cause by the fact that it is not built as a standalone 'git-version' executable, therefore appearing "removed" to 'check-docs'. Without a precedent for documented builtins that aren't built into an executable *or* any clear reason why a standalone 'git-version' shouldn't exist, the 'check-docs' error appears to correctly identify an issue. To correct that mismatch, add 'git-version' to the 'BUILT_INS' list in the root Makefile (indicating that the 'cmd_version()' function appears in a file that is *not* 'builtin/version.c'). Additionally, to avoid the "no link" message in 'check-docs', list 'git-version' as an "ancilliaryinterrogator" (like 'git help') in 'command-list.txt'. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- .gitignore | 1 + Makefile | 1 + command-list.txt | 1 + 3 files changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 3d1b880101..b3dcafcb33 100644 --- a/.gitignore +++ b/.gitignore @@ -181,6 +181,7 @@ /git-verify-commit /git-verify-pack /git-verify-tag +/git-version /git-web--browse /git-whatchanged /git-worktree diff --git a/Makefile b/Makefile index 35a07f80c5..cac3452edb 100644 --- a/Makefile +++ b/Makefile @@ -818,6 +818,7 @@ BUILT_INS += git-show$X BUILT_INS += git-stage$X BUILT_INS += git-status$X BUILT_INS += git-switch$X +BUILT_INS += git-version$X BUILT_INS += git-whatchanged$X # what 'all' will build but not install in gitexecdir diff --git a/command-list.txt b/command-list.txt index bb2e0a9214..54b2a50f5f 100644 --- a/command-list.txt +++ b/command-list.txt @@ -199,6 +199,7 @@ git-var plumbinginterrogators git-verify-commit ancillaryinterrogators git-verify-pack plumbinginterrogators git-verify-tag ancillaryinterrogators +git-version ancillaryinterrogators git-whatchanged ancillaryinterrogators complete git-worktree mainporcelain git-write-tree plumbingmanipulators From 72991ff558585490aa4284c0b8ca1f13e86f0f18 Mon Sep 17 00:00:00 2001 From: Jacob Stopak Date: Mon, 19 Sep 2022 19:45:56 -0700 Subject: [PATCH 10/17] Documentation: clean up a few misspelled word typos Used GNU "aspell check " to review various documentation files with the default aspell dictionary. Ignored false-positives between american and british english. Signed-off-by: Jacob Stopak Reviewed-by: Eric Sunshine Signed-off-by: Junio C Hamano --- Documentation/MyFirstContribution.txt | 2 +- Documentation/MyFirstObjectWalk.txt | 2 +- Documentation/git.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt index 1da15d9ad4..1a4be8ee0a 100644 --- a/Documentation/MyFirstContribution.txt +++ b/Documentation/MyFirstContribution.txt @@ -1160,7 +1160,7 @@ all named like `v2-000n-my-commit-subject.patch`. `-v2` will also format your patches by prefixing them with "[PATCH v2]" instead of "[PATCH]", and your range-diff will be prefaced with "Range-diff against v1". -Afer you run this command, `format-patch` will output the patches to the `psuh/` +After you run this command, `format-patch` will output the patches to the `psuh/` directory, alongside the v1 patches. Using a single directory makes it easy to refer to the old v1 patches while proofreading the v2 patches, but you will need to be careful to send out only the v2 patches. We will use a pattern like diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt index 8d9e85566e..eee513e86f 100644 --- a/Documentation/MyFirstObjectWalk.txt +++ b/Documentation/MyFirstObjectWalk.txt @@ -534,7 +534,7 @@ the arguments to `traverse_commit_list()`. - `void *show_data`: A context buffer which is passed in turn to `show_commit` and `show_object`. -In addition, `traverse_commit_list_filtered()` has an additional paramter: +In addition, `traverse_commit_list_filtered()` has an additional parameter: - `struct oidset *omitted`: A linked-list of object IDs which the provided filter caused to be omitted. diff --git a/Documentation/git.txt b/Documentation/git.txt index 0ef7f5e4ec..0c15ef3a8e 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -344,7 +344,7 @@ Repository, command and file interfaces This documentation discusses repository and command interfaces which users are expected to interact with directly. See `--user-formats` in -linkgit:git-help[1] for more details on the critera. +linkgit:git-help[1] for more details on the criteria. include::cmds-userinterfaces.txt[] From bbb0c357b81d86dfd0b843cabe6c8fe29ced9ebd Mon Sep 17 00:00:00 2001 From: Jacob Stopak Date: Mon, 19 Sep 2022 19:45:57 -0700 Subject: [PATCH 11/17] Documentation: clean up various typos in technical docs Used GNU "aspell check " to review various technical documentation files with the default aspell dictionary. Ignored false-positives between american and british english. Signed-off-by: Jacob Stopak Reviewed-by: Eric Sunshine Signed-off-by: Junio C Hamano --- Documentation/technical/api-parse-options.txt | 2 +- Documentation/technical/bundle-uri.txt | 6 +++--- Documentation/technical/commit-graph.txt | 4 ++-- Documentation/technical/remembering-renames.txt | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index c2a5e42914..61fa6ee167 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -60,7 +60,7 @@ Subcommands are special in a couple of ways: * All arguments following the subcommand are considered to be arguments of the subcommand, and, conversely, arguments meant for the subcommand may - not preceed the subcommand. + not precede the subcommand. Therefore, if the options array contains at least one subcommand and `parse_options()` encounters the first dashless argument, it will either: diff --git a/Documentation/technical/bundle-uri.txt b/Documentation/technical/bundle-uri.txt index c25c42378a..8939655fc0 100644 --- a/Documentation/technical/bundle-uri.txt +++ b/Documentation/technical/bundle-uri.txt @@ -290,7 +290,7 @@ expect that the process will end when all prerequisite commit OIDs in a thin bundle are already in the object database. When using the `creationToken` heuristic, the client can avoid downloading -any bundles if their creation tokenss are not larger than the stored +any bundles if their creation tokens are not larger than the stored creation token. After fetching new bundles, Git updates this local creation token. @@ -319,7 +319,7 @@ Here are a few example error conditions: Git's other HTTP protocols in terms of handling specific 400-level errors. -* The server reports any other failure reponse. +* The server reports any other failure response. * The client receives data that is not parsable as a bundle or bundle list. @@ -447,7 +447,7 @@ created every hour, and then once a day those "hourly" bundles could be merged into a "daily" bundle. The daily bundles are merged into the oldest bundle after 30 days. -It is recommened that this bundle strategy is repeated with the `blob:none` +It is recommended that this bundle strategy is repeated with the `blob:none` filter if clients of this repository are expecting to use blobless partial clones. This list of blobless bundles stays in the same list as the full bundles, but uses the `bundle..filter` key to separate the two groups. diff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt index f05e7bda1a..90c9760c23 100644 --- a/Documentation/technical/commit-graph.txt +++ b/Documentation/technical/commit-graph.txt @@ -40,7 +40,7 @@ Values 1-4 satisfy the requirements of parse_commit_gently(). There are two definitions of generation number: 1. Corrected committer dates (generation number v2) -2. Topological levels (generation nummber v1) +2. Topological levels (generation number v1) Define "corrected committer date" of a commit recursively as follows: @@ -48,7 +48,7 @@ Define "corrected committer date" of a commit recursively as follows: equal to its committer date. * A commit with at least one parent has corrected committer date equal to - the maximum of its commiter date and one more than the largest corrected + the maximum of its committer date and one more than the largest corrected committer date among its parents. * As a special case, a root commit with timestamp zero has corrected commit diff --git a/Documentation/technical/remembering-renames.txt b/Documentation/technical/remembering-renames.txt index af091a7556..1e34d91390 100644 --- a/Documentation/technical/remembering-renames.txt +++ b/Documentation/technical/remembering-renames.txt @@ -407,7 +407,7 @@ considered to be "irrelevant". See for example the following commits: no longer relevant", 2021-03-13) Relevance is always determined by what the _other_ side of history has -done, in terms of modifing a file that our side renamed, or adding a +done, in terms of modifying a file that our side renamed, or adding a file to a directory which our side renamed. This means that a path that is "irrelevant" when picking the first commit of a series in a rebase or cherry-pick, may suddenly become "relevant" when picking the From 8b74492135481fe0fdf4b2e023c55d4146a6d209 Mon Sep 17 00:00:00 2001 From: Alex Henrie Date: Mon, 19 Sep 2022 23:07:25 -0600 Subject: [PATCH 12/17] gc: don't translate literal commands The command you type is still "git maintenance" even in other languages. Signed-off-by: Alex Henrie Signed-off-by: Junio C Hamano --- builtin/gc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 84549888f5..01ab0716ee 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1466,7 +1466,7 @@ static char *get_maintpath(void) } static char const * const builtin_maintenance_register_usage[] = { - N_("git maintenance register"), + "git maintenance register", NULL }; @@ -1524,7 +1524,7 @@ done: } static char const * const builtin_maintenance_unregister_usage[] = { - N_("git maintenance unregister"), + "git maintenance unregister", NULL }; @@ -2540,7 +2540,7 @@ static int maintenance_start(int argc, const char **argv, const char *prefix) } static const char *const builtin_maintenance_stop_usage[] = { - N_("git maintenance stop"), + "git maintenance stop", NULL }; From d11b875197c7b8150136f94788330567dc5902d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Tue, 20 Sep 2022 22:16:19 +0200 Subject: [PATCH 13/17] t/Makefile: remove 'test-results' on 'make clean' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 't/test-results' directory and its contents are by-products of the test process, so 'make clean' should remove them, but, alas, this has been broken since fee65b194d (t/Makefile: don't remove test-results in "clean-except-prove-cache", 2022-07-28). The 'clean' target in 't/Makefile' was not directly responsible for removing the 'test-results' directory, but relied on its dependency 'clean-except-prove-cache' to do that [1]. ee65b194d broke this, because it only removed the 'rm -r test-results' command from the 'clean-except-prove-cache' target instead of moving it to the 'clean' target, resulting in stray 't/test-results' directories. Add that missing cleanup command to 't/Makefile', and to all sub-Makefiles touched by that commit as well. [1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs, 2012-05-02) Signed-off-by: SZEDER Gábor Signed-off-by: Junio C Hamano --- contrib/subtree/t/Makefile | 1 + t/Makefile | 1 + 2 files changed, 2 insertions(+) diff --git a/contrib/subtree/t/Makefile b/contrib/subtree/t/Makefile index 3d278bb0ed..4655e0987b 100644 --- a/contrib/subtree/t/Makefile +++ b/contrib/subtree/t/Makefile @@ -51,6 +51,7 @@ clean-except-prove-cache: $(RM) -r valgrind/bin clean: clean-except-prove-cache + $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' $(RM) .prove test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax diff --git a/t/Makefile b/t/Makefile index 3db48c0cb6..882782a519 100644 --- a/t/Makefile +++ b/t/Makefile @@ -73,6 +73,7 @@ clean-except-prove-cache: clean-chainlint $(RM) -r valgrind/bin clean: clean-except-prove-cache + $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' $(RM) .prove clean-chainlint: From d956fa8082e1f8fb0fb26493113c1b98fee19fe2 Mon Sep 17 00:00:00 2001 From: Alex Henrie Date: Mon, 19 Sep 2022 23:06:32 -0600 Subject: [PATCH 14/17] builtin/diagnose.c: don't translate the two mode values These strings are not translatable in the diagnose_options array in diagnose.c. Don't translate them in builtin/diagnose.c either. Signed-off-by: Alex Henrie Signed-off-by: Junio C Hamano --- builtin/diagnose.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/diagnose.c b/builtin/diagnose.c index cd260c2015..576e0e8e38 100644 --- a/builtin/diagnose.c +++ b/builtin/diagnose.c @@ -22,7 +22,7 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix) N_("specify a destination for the diagnostics archive")), OPT_STRING('s', "suffix", &option_suffix, N_("format"), N_("specify a strftime format suffix for the filename")), - OPT_CALLBACK_F(0, "mode", &mode, N_("(stats|all)"), + OPT_CALLBACK_F(0, "mode", &mode, "(stats|all)", N_("specify the content of the diagnostic archive"), PARSE_OPT_NONEG, option_parse_diagnose), OPT_END() From 370d3a06a3ecb6950e10158c77ac32eb01ae0a88 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 21 Sep 2022 13:50:47 -0700 Subject: [PATCH 15/17] Final batch before -rc1 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.38.0.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/RelNotes/2.38.0.txt b/Documentation/RelNotes/2.38.0.txt index 5d9bd8c295..d16d19dcdd 100644 --- a/Documentation/RelNotes/2.38.0.txt +++ b/Documentation/RelNotes/2.38.0.txt @@ -390,6 +390,10 @@ Fixes since v2.37 been corrected. (merge 49ca2fba39 jk/proto-v2-ref-prefix-fix later to maint). + * A result from opendir() was leaking in the commit-graph expiration + codepath, which has been plugged. + (merge 12f1ae5324 ml/commit-graph-expire-dir-leak-fix later to maint). + * Other code cleanup, docfix, build fix, etc. (merge 77b9e85c0f vd/fix-perf-tests later to maint). (merge 0682bc43f5 jk/test-crontab-fixes later to maint). From 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 21 Sep 2022 15:26:39 -0700 Subject: [PATCH 16/17] Git 2.38-rc1 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.38.0.txt | 4 ++++ GIT-VERSION-GEN | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/RelNotes/2.38.0.txt b/Documentation/RelNotes/2.38.0.txt index d16d19dcdd..870581fc57 100644 --- a/Documentation/RelNotes/2.38.0.txt +++ b/Documentation/RelNotes/2.38.0.txt @@ -394,6 +394,10 @@ Fixes since v2.37 codepath, which has been plugged. (merge 12f1ae5324 ml/commit-graph-expire-dir-leak-fix later to maint). + * Just like we have coding guidelines, we now have guidelines for + reviewers. + (merge e01b851923 vd/doc-reviewing-guidelines later to maint). + * Other code cleanup, docfix, build fix, etc. (merge 77b9e85c0f vd/fix-perf-tests later to maint). (merge 0682bc43f5 jk/test-crontab-fixes later to maint). diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index ecd94fd3f2..a6d1044e8d 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.38.0-rc0 +DEF_VER=v2.38.0-rc1 LF=' ' From 4eaed7c2f2aed1ef510ff97e7e91fa0cbcbef65d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 22 Sep 2022 05:35:33 -0400 Subject: [PATCH 17/17] list-objects-filter: initialize sub-filter structs Since commit c54980ab83 (list-objects-filter: convert filter_spec to a strbuf, 2022-09-11), building with SANITIZE=undefined triggers an error in t5616. The problem is that we end up with a strbuf that has been zero-initialized instead of via STRBUF_INIT. Feeding that strbuf to strbuf_addbuf() in list_objects_filter_copy() means we will call memcpy like: memcpy(some_actual_buffer, NULL, 0); This works on most systems because we're copying zero bytes, but it is technically undefined behavior to ever pass NULL to memcpy. Even though c54980ab83 is where the bug manifests, that is only because we switched away from a string_list, which is OK with being zero-initialized (though it may cause other problems by not duplicating the strings, it happened to be OK in this instance). The actual bug is caused by the commit before that, 2a01bdedf8 (list-objects-filter: add and use initializers, 2022-09-11). There we consistently initialize the top-level filter structs, but we forgot the dynamically allocated ones we stick in filter_options->sub when creating combined filters. Note that we need to fix two spots here: where we parse a "combine:" filter, but also where we transform from a single-filter into a combined one after seeing multiple "--filter" options. In the second spot, we'll do some minor refactoring to avoid repeating our very-long array index. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- list-objects-filter-options.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index d46ce4acc4..5339660238 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -143,6 +143,7 @@ static int parse_combine_subfilter( ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, filter_options->sub_alloc); + list_objects_filter_init(&filter_options->sub[new_index]); decoded = url_percent_decode(subspec->buf); @@ -263,6 +264,8 @@ void parse_list_objects_filter( parse_error = gently_parse_list_objects_filter( filter_options, arg, &errbuf); } else { + struct list_objects_filter_options *sub; + /* * Make filter_options an LOFC_COMBINE spec so we can trivially * add subspecs to it. @@ -273,10 +276,11 @@ void parse_list_objects_filter( filter_spec_append_urlencode(filter_options, arg); ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, filter_options->sub_alloc); + sub = &filter_options->sub[filter_options->sub_nr - 1]; - parse_error = gently_parse_list_objects_filter( - &filter_options->sub[filter_options->sub_nr - 1], arg, - &errbuf); + list_objects_filter_init(sub); + parse_error = gently_parse_list_objects_filter(sub, arg, + &errbuf); } if (parse_error) die("%s", errbuf.buf);