Skip to main content

Browse the archive

swh:1:snp:6df5a50b8107b6bbe1e51d0239d816a7503c536a
sort by:
Revision Author Date Message Commit Date
9274dea Ævar Arnfjörð Bjarmason 07 September 2022, 08:27:05 UTC docs: add CONFIGURATION sections that fuzzy map to built-ins Add a CONFIGURATION section to the documentation of various built-ins, for those cases where the relevant config/NAME.txt doesn't map only to one git-NAME.txt. In particular: * config/blame.txt: used by git-{blame,annotate}.txt. Since the git-annotate(1) documentation refers to git-blame(1) don't add a "CONFIGURATION" section to git-annotate(1), only to git-blame(1). * config/checkout.txt: maps to both git-checkout.txt and git-switch.txt (but nothing else). * config/init.txt: should be included in git-init(1) and git-clone(1). * config/column.txt: We should ideally mention the relevant subset of this in git-{branch,clean,status,tag}.txt, but let's punt on it for now. We will when we eventually split these sort of files into e.g. config/column.txt and config/column/{branch,clean,status,tag}.txt, with the former including the latter set. Things that are being left out, and why: * config/{remote,remotes,credential}.txt: Configuration that affects how we talk to remote repositories is harder to untangle. We'll need to include some of this in git-{fetch,remote,push,ls-remote}.txt etc., but some of those only use a small subset of these options. Let's leave this for now. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:07 UTC
16f6b0d Ævar Arnfjörð Bjarmason 07 September 2022, 08:27:04 UTC docs: add CONFIGURATION sections that map to a built-in Add a CONFIGURATION section to the documentation of various built-ins, for those cases where the relevant config/NAME.txt describes configuration that is only used by the relevant built-in documented in git-NAME.txt. Subsequent commits will handle more complex cases. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:06 UTC
00c8053 Ævar Arnfjörð Bjarmason 07 September 2022, 08:27:03 UTC log docs: de-duplicate configuration sections Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:06 UTC
2a9dfdf Ævar Arnfjörð Bjarmason 07 September 2022, 08:27:02 UTC difftool docs: de-duplicate configuration sections Include the "config/difftool.txt" file in "git-difftool.txt", and move the relevant part of git-difftool(1) configuration from "config/diff.txt" to config/difftool.txt". Doing this is slightly odd, as we usually discuss configuration in alphabetical order, but by doing it we're able to include the full set of configuration used by git-difftool(1) (and only that configuration) in its own documentation. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:06 UTC
5bd277e Ævar Arnfjörð Bjarmason 07 September 2022, 08:27:01 UTC notes docs: de-duplicate and combine configuration sections Combine the various "notes" configuration sections spread across Documentation/config/notes.txt and Documentation/git-notes.txt to live in the former, and to be included in the latter. We'll now forward link from "git notes" to the "CONFIGURATION" section below, rather than to "git-config(1)" when discussing configuration variables that are (also) discussed in that section. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:06 UTC
416fed2 Ævar Arnfjörð Bjarmason 07 September 2022, 08:27:00 UTC apply docs: de-duplicate configuration sections The wording is not identical to Documentation/config/apply.txt, but that version is better. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:05 UTC
bac1d52 Ævar Arnfjörð Bjarmason 07 September 2022, 08:26:59 UTC send-email docs: de-duplicate configuration sections De-duplicate the discussion of "send-email" configuration, such that the "git-config(1)" manual page becomes the source of truth, and "git-send-email(1)" includes the relevant part. Most commands that suffered from such duplication had diverging text discussing the same variables, but in this case some config was also only discussed in one or the other. This is mostly a move-only change, the exception is a minor rewording of changing wording like "see above" to "see linkgit:git-config[1]", as well as a clarification about the big section of command-line option tweaking config being discussed in git-send-email(1)'s main docs. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:05 UTC
a2811dd Ævar Arnfjörð Bjarmason 07 September 2022, 08:26:58 UTC grep docs: de-duplicate configuration sections Include the "config/grep.txt" file in "git-grep.txt", instead of repeating an almost identical description of the "grep" configuration variables in two places. There is no loss of information here that isn't shown in the addition to "grep.txt". This change was made by copying the contents of "git-grep.txt"'s version over the "grep.txt" version. Aside from the change "grep.txt" being made here the two were identical. This documentation started being copy/pasted around in b22520a37c8 (grep: allow -E and -n to be turned on by default via configuration, 2011-03-30). After that in e.g. 6453f7b3486 (grep: add grep.fullName config variable, 2014-03-17) they started drifting apart, with only grep.fullName being described in the command documentation. In 434e6e753fe (config.txt: move grep.* to a separate file, 2018-10-27) we gained the include, but didn't do this next step, let's do it now. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:05 UTC
18d89fe Ævar Arnfjörð Bjarmason 07 September 2022, 08:26:57 UTC docs: add and use include template for config/* includes In b6a8d09f6d8 (gc docs: include the "gc.*" section from "config" in "gc", 2019-04-07) the "git gc" documentation was made to include the config/gc.txt in its "CONFIGURATION" section. We do that in several other places, but "git gc" was the only one with a blurb above the include to orient the reader. We don't want readers to carefully scrutinize "git-config(1)" and "git-gc(1)" looking for discrepancies, instead we should tell them that the latter includes a part of the former. This change formalizes that wording in two new templates to be included, one for the "git gc" case where the entire section is included from "git-config(1)", and another for when the inclusion of "git-config(1)" follows discussion unique to that documentation. In order to use that re-arrange the order of those being discussed in the "git-merge(1)" documentation. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:46:05 UTC
b27ccae Jeff King 06 September 2022, 21:04:35 UTC rev-list: disable commit graph with --verify-objects Since the point of --verify-objects is to actually load and checksum the bytes of each object, optimizing out reads using the commit graph runs contrary to our goal. The most targeted way to implement this would be for the revision traversal code to check revs->verify_objects and avoid using the commit graph. But it's difficult to be sure we've hit all of the correct spots. For instance, I started this patch by writing the first of the included test cases, where the corrupted commit is directly on rev-list's command line. And that is easy to fix by teaching get_reference() to check revs->verify_objects before calling lookup_commit_in_graph(). But that doesn't cover the second test case: when we traverse to a corrupted commit, we'd parse the parent in process_parents(). So we'd need to check there, too. And it keeps going. In handle_commit() we sometimes parses commits, too, though I couldn't figure out a way to trigger it that did not already parse via get_reference() or tag peeling. And try_to_simplify_commit() has its own parse call, and so on. So it seems like the safest thing is to just disable the commit graph for the whole process when we see the --verify-objects option. We can do that either in builtin/rev-list.c, where we use the option, or in revision.c, where we parse it. There are some subtleties: - putting it in rev-list.c is less surprising in some ways, because there we know we are just doing a single traversal. In a command which does multiple traversals in a single process, it's rather unexpected to globally disable the commit graph. - putting it in revision.c is less surprising in some ways, because the caller does not have to remember to disable the graph themselves. But this is already tricky! The verify_objects flag in rev_info doesn't do anything by itself. The caller has to provide an object callback which does the right thing. - for that reason, in practice nobody but rev-list uses this option in the first place. So the distinction is probably not important either way. Arguably it should just be an option of rev-list, and not the general revision machinery; right now you can run "git log --verify-objects", but it does not actually do anything useful. - checking for a parsed revs.verify_objects flag in rev-list.c is too late. By that time we've already passed the arguments to setup_revisions(), which will have parsed the commits using the graph. So this commit disables the graph as soon as we see the option in revision.c. That's a pretty broad hammer, but it does what we want, and in practice nobody but rev-list is using this flag anyway. The tests cover both the "tip" and "parent" cases. Obviously our hammer hits them both in this case, but it's good to check both in case somebody later tries the more focused approach. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:44:30 UTC
d604529 Jeff King 06 September 2022, 21:02:13 UTC lookup_commit_in_graph(): use prepare_commit_graph() to check for graph We exit early from lookup_commit_in_graph() if the commit_graph pointer is NULL, under the assumption that we don't have a graph to look at. But the graph pointer is lazy-loaded; if no other code happens to have called prepare_commit_graph(), we'll incorrectly assume that one isn't available at all. This has a pretty small performance impact in practice, because the fallback will generally be to call parse_object() instead. That ends up in parse_commit_buffer(), which loads the graph data itself. So the first commit we see won't use the graph, but subsequent ones will. Since using the graph is just an optimization there's generally no user-visible difference, but if you instrument rev-list like so: diff --git a/revision.c b/revision.c index ee702e498a..63c488ffb6 100644 --- a/revision.c +++ b/revision.c @@ -381,6 +381,9 @@ static struct object *get_reference(struct rev_info *revs, const char *name, * parsing commit data from disk. */ commit = lookup_commit_in_graph(revs->repo, oid); + warning("%s %s in commit graph", + commit ? "found" : "did not find", + name); if (commit) object = &commit->object; else and run (in git.git): git commit-graph write --reachable git rev-list origin/master origin/next >/dev/null you'll see that we fail to find the first one: warning: did not find origin/master in commit graph warning: found origin/next in commit graph After this patch, you'll see that we find both: warning: found origin/master in commit graph warning: found origin/next in commit graph Even though the performance implication is small here, there are two important reasons to do this: - it's downright confusing if you are hunting a bug triggered by the use of the commit graph. It may or may not trigger depending on the number and ordering of tips you ask for. - prepare_commit_graph() has other policy logic, too. In particular, if we've loaded a commit graph and then disabled the graph via disable_commit_graph(), that should take precedence. I'm not sure if this can trigger bad behavior in practice. The only caller there is upload-pack's deepen_by_rev_list(), which should be avoiding the commit graph for its traversal tips, but probably wasn't before this patch. Whether you could come up with a case where that mattered is unclear. Still, this is obviously the right thing to be doing. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 07 September 2022, 16:44:28 UTC
79f2338 Junio C Hamano 05 September 2022, 20:54:46 UTC The eighteenth batch Signed-off-by: Junio C Hamano <gitster@pobox.com> 06 September 2022, 01:33:41 UTC
27fb520 Junio C Hamano 06 September 2022, 01:33:41 UTC Merge branch 'jk/test-crontab-fixes' Test helper fix. * jk/test-crontab-fixes: test-crontab: minor memory and error handling fixes 06 September 2022, 01:33:41 UTC
fcbc874 Junio C Hamano 06 September 2022, 01:33:41 UTC Merge branch 'en/test-without-test-create-repo' Test clean-up. * en/test-without-test-create-repo: t64xx: convert 'test_create_repo' to 'git init' 06 September 2022, 01:33:41 UTC
56785a3 Junio C Hamano 06 September 2022, 01:33:41 UTC Merge branch 'bc/gc-crontab-fix' FreeBSD portability fix for "git maintenance" that spawns "crontab" to schedule tasks. * bc/gc-crontab-fix: gc: use temporary file for editing crontab 06 September 2022, 01:33:41 UTC
2d88021 Junio C Hamano 06 September 2022, 01:33:40 UTC Merge branch 'es/t4301-sed-portability-fix' Test clean-up. * es/t4301-sed-portability-fix: t4301: emit blank line in more idiomatic fashion t4301: fix broken &&-chains and add missing loop termination t4301: account for behavior differences between sed implementations 06 September 2022, 01:33:40 UTC
5784d20 Junio C Hamano 06 September 2022, 01:33:40 UTC Merge branch 'rs/test-mergesort' Optimization of a test-helper command. * rs/test-mergesort: test-mergesort: use mem_pool for sort input test-mergesort: read sort input all at once 06 September 2022, 01:33:40 UTC
b5d2e99 Junio C Hamano 06 September 2022, 01:33:40 UTC Merge branch 'rs/tempfile-cleanup-race-fix' The clean-up of temporary files created via mks_tempfile_dt() was racy and attempted to unlink() the leading directory when signals are involved, which has been corrected. * rs/tempfile-cleanup-race-fix: tempfile: avoid directory cleanup race 06 September 2022, 01:33:40 UTC
3fe0121 Junio C Hamano 06 September 2022, 01:33:39 UTC Merge branch 'ac/bitmap-lookup-table' The pack bitmap file gained a bitmap-lookup table to speed up locating the necessary bitmap for a given commit. * ac/bitmap-lookup-table: pack-bitmap-write: drop unused pack_idx_entry parameters bitmap-lookup-table: add performance tests for lookup table pack-bitmap: prepare to read lookup table extension pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests pack-bitmap-write.c: write lookup table extension bitmap: move `get commit positions` code to `bitmap_writer_finish` Documentation/technical: describe bitmap lookup table extension 06 September 2022, 01:33:39 UTC
cf98b69 Junio C Hamano 06 September 2022, 01:33:39 UTC Merge branch 'tb/midx-with-changing-preferred-pack-fix' Multi-pack index got corrupted when preferred pack changed from one pack to another in a certain way, which has been corrected. * tb/midx-with-changing-preferred-pack-fix: midx.c: avoid adding preferred objects twice midx.c: include preferred pack correctly with existing MIDX midx.c: extract `midx_fanout_add_pack_fanout()` midx.c: extract `midx_fanout_add_midx_fanout()` midx.c: extract `struct midx_fanout` t/lib-bitmap.sh: avoid silencing stderr t5326: demonstrate potential bitmap corruption 06 September 2022, 01:33:39 UTC
9eb7a73 Victoria Dye 02 September 2022, 15:56:50 UTC Documentation/technical: include Scalar technical doc Include 'Documentation/technical/scalar.txt' alongside the other HTML technical docs when installing them. Now that the document is intended as a widely-accessible reference, remove the internal work-in-progress roadmap from the document. Those details should no longer be needed to guide Scalar's development and, if they were left, they could fall out-of-date and be misleading to readers. Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:56 UTC
ba1b117 Victoria Dye 02 September 2022, 15:56:49 UTC t/perf: add 'GIT_PERF_USE_SCALAR' run option Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf config 'useScalar') to register a repository created with any of: * test_perf_fresh_repo * test_perf_default_repo * test_perf_large_repo as a Scalar enlistment. This is intended to allow a developer to test the impact of Scalar on already-defined performance scenarios. Suggested-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:56 UTC
e280923 Victoria Dye 02 September 2022, 15:56:48 UTC t/perf: add Scalar performance tests Create 'p9210-scalar.sh' for testing Scalar performance and comparing performance of Git operations in Scalar registrations and standard repositories. Example results: Test this tree ------------------------------------------------------------------------ 9210.2: scalar clone 14.82(18.00+3.63) 9210.3: git clone 26.15(36.67+6.90) 9210.4: git status (scalar) 0.04(0.01+0.01) 9210.5: git status (non-scalar) 0.10(0.02+0.11) 9210.6: test_commit --append --no-tag A (scalar) 0.08(0.02+0.03) 9210.7: test_commit --append --no-tag A (non-scalar) 0.13(0.03+0.11) Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:56 UTC
14b4e7e Victoria Dye 02 September 2022, 15:56:47 UTC scalar-clone: add test coverage Create a new test file ('t9211-scalar-clone.sh') to exercise the options and behavior of the 'scalar clone' command. Each test clones to a unique target location and cleans up the cloned repo only when the test passes. This ensures that failed tests' artifacts are captured in CI artifacts for further debugging. Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:56 UTC
cc75e55 Victoria Dye 02 September 2022, 15:56:46 UTC scalar: add to 'git help -a' command list Add 'scalar' as a 'mainporcelain' command in the Git command list. Update the regex in 'cmd-list.perl' used to match the first line of command documentation to find 'scalar(1)'. Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:56 UTC
951759d Johannes Schindelin 02 September 2022, 15:56:45 UTC scalar: implement the `help` subcommand It is merely handing off to `git help scalar`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:56 UTC
dd9603e Johannes Schindelin 02 September 2022, 15:56:44 UTC git help: special-case `scalar` With this commit, `git help scalar` will open the appropriate manual or HTML page (instead of looking for `gitscalar`). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:56 UTC
7b5c93c Victoria Dye 02 September 2022, 15:56:43 UTC scalar: include in standard Git build & installation Move 'scalar' out of 'contrib/' and into the root of the Git tree. The goal of this change is to build 'scalar' as part of the standard Git build & install processes. This patch includes both the physical move of Scalar's files out of 'contrib/' ('scalar.c', 'scalar.txt', and 't9xxx-scalar.sh'), and the changes to the build definitions in 'Makefile' and 'CMakelists.txt' to accommodate the new program. At a high level, Scalar is built so that: - there is a 'scalar-objs' target (similar to those created in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)) for debugging purposes. - it appears in the root of the install directory (rather than the gitexecdir). - it is included in the 'bin-wrappers/' directory for use in tests. - it receives a platform-specific executable suffix (e.g., '.exe'), if applicable. - 'scalar.txt' is installed as 'man1' documentation. - the 'clean' target removes the 'scalar' executable. Additionally, update the root level '.gitignore' file to ignore the Scalar executable. Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:55 UTC
b6faef3 Victoria Dye 02 September 2022, 15:56:42 UTC scalar: fix command documentation section header Rename the last section header in 'contrib/scalar/scalar.txt' from "Scalar" to "GIT". The linting rules of the 'documentation' CI build enforce the existence of a "GIT" section in command documentation. Although 'scalar.txt' is not yet checked, it will be in a future patch. Here, changing the header name is more appropriate than making a Scalar-specific exception to the linting rule. The existing "Scalar" section contains only a link back to the main Git documentation, essentially the same as the "GIT" section in builtin documentation. Changing the section name further clarifies the Scalar-Git association and maintains consistency with the rest of Git. Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 17:02:55 UTC
037f8ea Victoria Dye 01 September 2022, 21:02:33 UTC unpack-trees: fix sparse directory recursion check Ensure 'is_sparse_directory_entry()' receives a valid 'name_entry *' if one exists in the list of tree(s) being unpacked in 'unpack_callback()'. Currently, 'is_sparse_directory_entry()' is called with the first 'name_entry' in the 'names' list of entries on 'unpack_callback()'. However, this entry may be empty even when other elements of 'names' are not (such as when switching from an orphan branch back to a "normal" branch). As a result, 'is_sparse_directory_entry()' could incorrectly indicate that a sparse directory is *not* actually sparse because the name of the index entry does not match the (empty) 'name_entry' path. Fix the issue by using the existing 'name_entry *p' value in 'unpack_callback()', which points to the first non-empty entry in 'names'. Because 'p' is 'const', also update 'is_sparse_directory_entry()'s 'name_entry *' argument to be 'const'. Finally, add a regression test case. Reported-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:43:09 UTC
67360b7 Elijah Newren 02 September 2022, 03:53:30 UTC diff: fix filtering of merge commits under --remerge-diff Commit 95433eeed9 ("diff: add ability to insert additional headers for paths", 2022-02-02) introduced the possibility of additional headers. Because there could be conflicts with no content differences (e.g. a modify/delete conflict resolved in favor of taking the modified file as-is), that commit also modified the diff_queue_is_empty() and diff_flush_patch() logic to ensure these headers were included even if there was no associated content diff. However, the added logic was a bit inconsistent between these two functions. diff_queue_is_empty() overlooked the fact that the additional headers strmap could be non-NULL and empty, which would cause it to display commits that should have been filtered out. Fix the diff_queue_is_empty() logic to also account for additional_path_headers being empty. Reported-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:22:25 UTC
71a146d Elijah Newren 02 September 2022, 03:53:29 UTC diff: fix filtering of additional headers under --remerge-diff Commit 95433eeed9 ("diff: add ability to insert additional headers for paths", 2022-02-02) introduced the possibility of additional headers. Because there could be conflicts with no content differences (e.g. a modify/delete conflict resolved in favor of taking the modified file as-is), that commit also modified the diff_queue_is_empty() and diff_flush_patch() logic to ensure these headers were included even if there was no associated content diff. However, when the pickaxe is active, we really only want the remerge conflict headers to be shown when there is an associated content diff. Adjust the logic in these two functions accordingly. This also removes the TEST_PASSES_SANITIZE_LEAK=true declaration from t4069, as there is apparently some kind of memory leak with the pickaxe code. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:22:25 UTC
9b08091 Elijah Newren 02 September 2022, 03:53:28 UTC diff: have submodule_format logic avoid additional diff headers Commit 95433eeed9 ("diff: add ability to insert additional headers for paths", 2022-02-02) introduced the possibility of additional headers, created in create_filepairs_for_header_only_notifications(). These are represented by inserting additional pairs in diff_queued_diff which always have a mode of 0 and a null_oid. When these were added, one code path was noted to assume that at least one of the diff_filespecs in the pair were valid, and that codepath was corrected. The submodule_format handling is another codepath with the same issue; it would operate on these additional headers and attempt to display them as submodule changes. Prevent that by explicitly checking for "phoney" filepairs (i.e. filepairs with both modes being 0). Reported-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:22:25 UTC
fe4c750 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:24 UTC submodule--helper: fix a configure_added_submodule() leak Fix config API a memory leak added in a452128a36c (submodule--helper: introduce add-config subcommand, 2021-08-06) by using the *_tmp() variant of git_config_get_string(). In this case we're only checking whether the (repo|git)_config_get_string() call is telling us that the "submodule.active" key exists. As with the preceding commit we'll find many other such patterns in the codebase if we go fishing. E.g. "git gc" leaks in the code added in 61f7a383d3b (maintenance: use 'incremental' strategy by default, 2020-10-15). Similar code in "git gc" added in b08ff1fee00 (maintenance: add --schedule option and config, 2020-09-11) doesn't leak, but we could avoid the malloc() & free() in that case. A coccinelle rule to find those would find and fix some leaks, and cases where we're doing needless malloc() + free()'s but only care about the key existence, or are copying the (repo|git)_config_get_string() return value right away. But as with the preceding commit let's punt on all of that for now, and just narrowly fix this specific case in submodule--helper. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
4c4d3e7 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:23 UTC submodule--helper: free rest of "displaypath" in "struct update_data" Fix a leak in code added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24), we clobber the "displaypath" member of the passed-in "struct update_data" both so that die() messages in this update_submodule() function itself can use it, and for the run_update_procedure() called within this function. Fix a leak in code added in 51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24). We'd always clobber the old "displaypath" member of the previously passed-in "struct update_data". A better fix for this would be to remove the "displaypath" member from the "struct update_data" entirely. Along with "oid", "suboid", "just_cloned" and "sm_path" it's managing members that mainly need to be passed between 1-3 stack frames of functions adjacent to this code. But doing so would be a much larger change (I have it locally, and fully untangling that in an incremental way is a 10 patch journey). So let's go for this much more isolated fix suggested by Glen. We FREE_AND_NULL() the "update_data->displaypath", the "AND_NULL()" part of that is needed due to the later "free(ud->displaypath)" in "update_data_release()" introduced in the preceding commit Moving ensure_core_worktree() out of update_submodule() may not be strictly required, but in doing so we are left with the exact same ordering as before, making this a smaller functional change. Helped-by: Glen Choo <chooglen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
d40c42e Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:22 UTC submodule--helper: free some "displaypath" in "struct update_data" Make the update_data_release() function free "displaypath" member when appropriate. The "displaypath" member is always ours, the "const" on the "char *" was wrong to begin with. This leaves a leak of "displaypath" in update_submodule(), which as we'll see in subsequent commits is harder to deal with than this trivial fix. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
25b6a95 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:21 UTC submodule--helper: fix a memory leak in print_status() Fix a leak in print_status(), the compute_rev_name() function implemented in this file will return a strbuf_detach()'d value, or NULL. This leak has existed since this code was added in a9f8a37584a (submodule: port submodule subcommand 'status' from shell to C, 2017-10-06), but in 0b5e2ea7cf3 (submodule--helper: don't print null in 'submodule status', 2018-04-18) we added a "const" intermediate variable for the return value, that "const" should be removed. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
623bd7d Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:20 UTC submodule--helper: fix a leak in module_add() Fix a leak in module_path(), since a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10), we've been freeing add_data.sm_path, but in this case we clobbered it, and didn't free the value we clobbered. This makes test 28 of "t/t7400-submodule-basic.sh" ("submodule add in subdirectory") pass when we're compiled with SANITIZE=leak.. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
4e83605 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:19 UTC submodule--helper: fix obscure leak in module_add() Fix an obscure leak in module_add(), if the "git add" command we were piping to failed we'd fail to strbuf_release(&sb). This fixes a leak introduced in a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10). In fixing it move to a "goto cleanup" pattern, and since we need to introduce a "ret" variable to do that let's also get rid of the intermediate "exit_code" variable. The initialization to "-1" in a6226fd772b has always been redundant, we'd only use the "exit_code" value after assigning the return value of pipe_command() to it. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
4c81ee9 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:18 UTC submodule--helper: fix "reference" leak Fix leaks in the "reference" variable declared in add_submodule() and module_clone(). In preceding commits this variable was refactored out of the "struct module_clone_data", but the leak has been with us since 31224cbdc72 (clone: recursive and reference option triggers submodule alternates, 2016-08-17) and 8c8195e9c3e (submodule--helper: introduce add-clone subcommand, 2021-07-10). Those commits added an xstrdup()'d member of the STRING_LIST_INIT_NODUP'd "struct string_list". We need to free() those, but not the ones we get from argv, let's make use of the "util" member, if it has a pointer it's the pointer we'll need to free, otherwise it'll be NULL (i.e. from argv). Note that the free() of the "util" member is needed in both module_clone() and add_submodule(). The module_clone() function itself doesn't populate the "util" pointer as add_submodule() does, but module_clone() is upstream of the add_possible_reference_from_superproject() caller we're modifying here, which does do that. This does preclude the use of the "util" pointer for any other reasons for now, but that's OK. If we ever need to use it for something else we could turn it into a small "struct" with an optional "to_free" member, and switch to using string_list_clear_func(). Alternatively we could have another "struct string_list to_free" which would keep a copy of the strings we've dup'd to free(). But for now this is perfectly adequate. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
ae3ef94 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:17 UTC submodule--helper: fix a memory leak in get_default_remote_submodule() Fix a memory leak in the get_default_remote_submodule() function added in a77c3fcb5ec (submodule--helper: get remote names from any repository, 2022-03-04), we need to repo_clear() the submodule we initialize. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
17af0a8 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:16 UTC submodule--helper: fix a leak with repo_clear() Call repo_clear() in ensure_core_worktree() to free the "struct repository". Fixes a leak that's been here since 74d4731da1f (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:13 UTC
980416e Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:15 UTC submodule--helper: fix "sm_path" and other "module_cb_list" leaks Fix leaks in "struct module_cb_list" and the "struct module_cb" which it contains, these fix leaks in e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13). The "sm_path" should always have been a "char *", not a "const char *", we always create it with xstrdup(). We can't mark any tests passing passing with SANITIZE=leak using "TEST_PASSES_SANITIZE_LEAK=true" as a result of this change, but "t7401-submodule-summary.sh" gets closer to passing as a result of this change. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
61adac6 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:14 UTC submodule--helper: fix "errmsg_str" memory leak Fix a memory leak introduced in e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13), we sometimes append to the "errmsg", and need to free the "struct strbuf". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
87a6834 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:13 UTC submodule--helper: add and use *_release() functions Add release functions for "struct module_list", "struct submodule_update_clone" and "struct update_data". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
0a4d315 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:12 UTC submodule--helper: don't leak {run,capture}_command() cp.dir argument Fix a memory leak in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) and 3c3558f0953 (submodule--helper: run update using child process struct, 2022-03-15) by not allocating memory in the first place. The "dir" member of "struct child_process" will not be modified by that API, and it's declared to be "const char *". So let's not needlessly duplicate these strings. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
4b9d124 Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:11 UTC submodule--helper: "struct pathspec" memory leak in module_update() The module_update() function calls module_list_compute() twice, which in turn will reset the "struct pathspec" passed to it. Let's instead track two of them, and clear them both. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
8fb201d Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:10 UTC submodule--helper: fix most "struct pathspec" memory leaks Call clear_pathspec() at the end of various functions that work with and allocate a "struct pathspec". In some cases the zero-initialization here isn't strictly needed, but as we're moving to a "goto cleanup" pattern let's make sure that it's safe to call clear_pathspec(), we don't want the data to be uninitialized. E.g. for module_foreach() we can see from looking at module_list_compute() that if it returns non-zero that the "pathspec" will always have been initialized. But relying on that both assumes knowledge about parse_pathspec(), and would set up a fragile pattern going forward. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
d76260e Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:09 UTC submodule--helper: fix trivial get_default_remote_submodule() leak Fix a leak in code added in 1012a5cbc3f (submodule--helper run-update-procedure: learn --remote, 2022-03-04), we need to free() the xstrdup()'d string. This gets e.g. t/t7419-submodule-set-branch.sh closer to passing under SANITIZE=leak. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
e77b3da Ævar Arnfjörð Bjarmason 31 August 2022, 23:14:08 UTC submodule--helper: fix a leak in "clone_submodule" Fix a memory leak of the "clone_data_path" variable that we copy or derive from the "struct module_clone_data" in clone_submodule(). This code was refactored in preceding commits, but the leak has been with us since f8eaa0ba98b (submodule--helper, module_clone: always operate on absolute paths, 2016-03-31). For the "else" case we don't need to xstrdup() the "clone_data->path", and we don't need to free our own "clone_data_path". We can therefore assign the "clone_data->path" to our own "clone_data_path" right away, and only override it (and remember to free it!) if we need to xstrfmt() a replacement. In the case of the module_clone() caller it's from "argv", and doesn't need to be free'd, and in the case of the add_submodule() caller we get a pointer to "sm_path", which doesn't need to be directly free'd either. Fixing this leak makes several tests pass, so let's mark them as passing with TEST_PASSES_SANITIZE_LEAK=true. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:18:12 UTC
5647d74 Junio C Hamano 02 September 2022, 16:17:17 UTC Merge branch 'ab/submodule-helper-prep' into ab/submodule-helper-leakfix * ab/submodule-helper-prep: (33 commits) submodule--helper: fix bad config API usage submodule--helper: libify even more "die" paths for module_update() submodule--helper: libify more "die" paths for module_update() submodule--helper: check repo{_submodule,}_init() return values submodule--helper: libify "must_die_on_failure" code paths (for die) submodule--helper update: don't override 'checkout' exit code submodule--helper: libify "must_die_on_failure" code paths submodule--helper: libify determine_submodule_update_strategy() submodule--helper: don't exit() on failure, return submodule--helper: use "code" in run_update_command() submodule API: don't handle SM_..{UNSPECIFIED,COMMAND} in to_string() submodule--helper: don't call submodule_strategy_to_string() in BUG() submodule--helper: add missing braces to "else" arm submodule--helper: return "ret", not "1" from update_submodule() submodule--helper: rename "int res" to "int ret" submodule--helper: don't redundantly check "else if (res)" submodule--helper: refactor "errmsg_str" to be a "struct strbuf" submodule--helper: add "const" to passed "struct update_data" submodule--helper: add "const" to copy of "update_data" submodule--helper: add "const" to passed "module_clone_data" ... 02 September 2022, 16:17:17 UTC
d4a492f Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:15 UTC submodule--helper: fix bad config API usage Fix bad config API usage added in a452128a36c (submodule--helper: introduce add-config subcommand, 2021-08-06). After git_config_get_string() returns successfully we know the "char **dest" will be non-NULL. A coccinelle patch that transforms this turns up a couple of other such issues, one in fetch-pack.c, and another in upload-pack.c: @@ identifier F =~ "^(repo|git)_config_get_string(_tmp)?$"; identifier V; @@ !F(..., &V) - && (V) But let's focus narrowly on submodule--helper for now, we can fix those some other time. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:25 UTC
86e16ed Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:14 UTC submodule--helper: libify even more "die" paths for module_update() As noted in a preceding commit the get_default_remote_submodule() and remote_submodule_branch() functions would invoke die(), and thus leave update_submodule() only partially lib-ified. We've addressed the former of those in a preceding commit, let's now address the latter. In addition to lib-ifying the function this fixes a potential (but obscure) segfault introduced by a logic error in 1012a5cbc3f (submodule--helper run-update-procedure: learn --remote, 2022-03-04): We were assuming that remote_submodule_branch() would always return non-NULL, but if the submodule_from_path() call in that function fails we'll return NULL. See its introduction in 92bbe7ccf1f (submodule--helper: add remote-branch helper, 2016-08-03). I.e. we'd previously have segfaulted in the xstrfmt() call in update_submodule() seen in the context. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:25 UTC
f5373de Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:13 UTC submodule--helper: libify more "die" paths for module_update() As noted in a preceding commit the get_default_remote_submodule() and remote_submodule_branch() functions would invoke die(), and thus leave update_submodule() only partially lib-ified. Let's address the former of those cases. Change the functions to return an int exit code (non-zero on failure), while leaving the get_default_remote() function for the callers that still want the die() semantics. This change addresses 1/2 of the "die" issue in these two lines in update_submodule(): char *remote_name = get_default_remote_submodule(update_data->sm_path); const char *branch = remote_submodule_branch(update_data->sm_path); We can safely remove the "!default_remote" case from sync_submodule(), because our get_default_remote_submodule() function now returns a die_message() on failure, so we can have it and other callers check if the exit code should be non-zero instead. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:25 UTC
1e8697b Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:12 UTC submodule--helper: check repo{_submodule,}_init() return values Fix code added in ce125d431aa (submodule: extract path to submodule gitdir func, 2021-09-15) and a77c3fcb5ec (submodule--helper: get remote names from any repository, 2022-03-04) which failed to check the return values of repo_init() and repo_submodule_init(). If we failed to initialize the repository or submodule we could segfault when trying to access the invalid repository structs. Let's also check that these were the only such logic errors in the codebase by making use of the "warn_unused_result" attribute. This is valid as of GCC 3.4.0 (and clang will catch it via its faking of __GNUC__ ). As the comment being added to git-compat-util.h we're piggy-backing on the LAST_ARG_MUST_BE_NULL version check out of lazyness. See 9fe3edc47f1 (Add the LAST_ARG_MUST_BE_NULL macro, 2013-07-18) for its addition. The marginal benefit of covering gcc 3.4.0..4.0.0 is near-zero (or zero) at this point. It mostly matters that we catch this somewhere. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
ac35015 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:11 UTC submodule--helper: libify "must_die_on_failure" code paths (for die) Continue the libification of codepaths that previously relied on "must_die_on_failure". In these cases we've always been early aborting by calling die(), but as we know that these codepaths will properly handle return codes of 128 to mean an early abort let's have them use die_message() instead. This still isn't a complete migration away from die() for these codepaths, in particular this code in update_submodule() will still call die() in some cases: char *remote_name = get_default_remote_submodule(update_data->sm_path); const char *branch = remote_submodule_branch(update_data->sm_path); But as that code is used by other callers than the "update" code let's leave converting it for a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
a03c01d Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:10 UTC submodule--helper update: don't override 'checkout' exit code When "git submodule update" runs it might call "checkout", "merge", "rebase", or a custom command. Ever since run_update_command() was added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) we'd either exit immediately if the "submodule.<name>.update" method failed, or in the case of "checkout" continue trying to update other submodules. This code used to use the magical "2" return code, but in 55b3f12cb54 (submodule update: use die_message(), 2022-03-15) it was made to exit(128), which in preceding commits has been changed to return that 128 code to the top-level. Let's "libify" this code even more by not having it arbitrarily override the return code. In practice this doesn't change anything as the code "git checkout" would return on any normal failure is "1", but we'll now in principle properly abort the operation if "git checkout" were to exit with 128. It would make sense to follow-up this change with a change to allow the "submodule.<name>.update = !..." (SM_UPDATE_COMMAND) method the same liberties as "checkout", and perhaps to do the same with a failed "merge" or "rebase". But let's leave that for now. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
d905d44 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:09 UTC submodule--helper: libify "must_die_on_failure" code paths In preceding commits the codepaths around update_submodules() were changed from using exit() or die() to ferrying up a "must_die_on_failure" in the cases where we'd exit(), and in most cases where we'd die(). We needed to do this this to ensure that we'd early exit or otherwise abort the update_submodules() processing before it was completed. Now that those preceding changes have shown that we've converted those paths, we can remove the remaining "ret == 128" special-cases, leaving the only such special-case in update_submodules(). I.e. we now know after having gone through the various codepaths that we were only returning 128 if we meant to early abort. In update_submodules() we'll for now set any non-zero non-128 exit codes to "1", but will start ferrying up the exit code as-is in a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
484f915 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:08 UTC submodule--helper: libify determine_submodule_update_strategy() Libify the determine_submodule_update_strategy() by having it invoke die_message() rather than die(), and returning the code die_message() returns on failure. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
2cb9294 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:07 UTC submodule--helper: don't exit() on failure, return Change code downstream of module_update() to short-circuit and return to the top-level on failure, rather than calling exit(). To do so we need to diligently check whether we "must_die_on_failure", which is a pattern started in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24), but which hadn't been completed to the point where we could avoid calling exit() here. This introduces no functional changes, but makes it easier to both call these routines as a library in the future, and to eventually avoid leaking memory. This and similar control flow in submodule--helper.c could be made simpler by properly "libifying" it, i.e. to have it consistently return -1 on failures, and to early return on any non-success. But let's leave that larger project for now, and (mostly) emulate what were doing with the "exit(128)" before this change. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
6870cdc Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:06 UTC submodule--helper: use "code" in run_update_command() Apply some DRY principles in run_update_command() and don't have two "switch" statements over "ud->update_strategy.type" determine the same thing. First we were setting "must_die_on_failure = 1" in all cases except "SM_UPDATE_CHECKOUT" (and we'd BUG(...) out on the rest). This code was added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24). Then we'd duplicate same "switch" logic when we were using the "must_die_on_failure" variable. Let's instead have the "case" branches in that inner "switch" determine whether or not the "update must continue" by picking an exit code. This also mostly avoids hardcoding the "128" exit code, instead we can make use of the return value of the die_message() function, which we've been calling here since 55b3f12cb54 (submodule update: use die_message(), 2022-03-15). We're still hardcoding it to determine if we "exit()", but subsequent commit(s) will address that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
b9dd63f Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:05 UTC submodule API: don't handle SM_..{UNSPECIFIED,COMMAND} in to_string() Change the submodule_strategy_to_string() function added in 3604242f080 (submodule: port init from shell to C, 2016-04-15) to really return a "const char *". In the "SM_UPDATE_COMMAND" case it would return a strbuf_detach(). Furthermore, this function would return NULL on SM_UPDATE_UNSPECIFIED, so it wasn't safe to xstrdup() its return value in the general case, or to use it in a sprintf() format as the code removed in the preceding commit did. But its callers would never call it with either SM_UPDATE_UNSPECIFIED or SM_UPDATE_COMMAND. Let's have its behavior reflect how its only user expects it to behave, and BUG() out on the rest. By doing this we can also stop needlessly xstrdup()-ing and free()-ing the memory for the config we're setting. We can instead always use constant strings. We can also use the *_tmp() variant of git_config_get_string(). Let's also rename this submodule_strategy_to_string() function to submodule_update_type_to_string(). Now that it's only tasked with returning a string version of the "enum submodule_update_type type". Before it would look at the "command" field in "struct submodule_update_strategy". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
08c2e77 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:04 UTC submodule--helper: don't call submodule_strategy_to_string() in BUG() Don't call submodule_strategy_to_string() in a BUG() message. These calls added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) don't need the extra information submodule_strategy_to_string() gives us, as we'll never reach the SM_UPDATE_COMMAND case here. That case is the only one where we'd get any information beyond the straightforward number-to-string mapping. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
96a9073 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:03 UTC submodule--helper: add missing braces to "else" arm Add missing braces to an "else" arm in init_submodule(), this stylistic change makes this code conform to the CodingGuidelines, and makes a subsequent commit smaller. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
0b917a9 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:02 UTC submodule--helper: return "ret", not "1" from update_submodule() Amend the update_submodule() function to return the failing "ret" on error, instead of overriding it with "1". This code was added in b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), and this change ends up not making a difference as this function is only called in update_submodules(). If we return non-zero here we'll always in turn return "1" in module_update(). But if we didn't do that and returned any other non-zero exit code in update_submodules() we'd fail the test that's being amended here. We're still testing the status quo here. This change makes subsequent refactoring of update_submodule() easier, as we'll no longer need to worry about clobbering the "ret" we get from the run_command(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
addda28 Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:01 UTC submodule--helper: rename "int res" to "int ret" Rename the "res" variable added in b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) to "ret", which is the convention in the rest of this file. Eventual follow-up commits will change the code in update_submodule() to a "goto cleanup" pattern, let's have the post image look consistent with the rest. For update_submodules() let's also use a "ret" for consistency, that use was also added in b3c5f5cb048. We'll be modifying that codepath in subsequent commits. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
b0bff0b Ævar Arnfjörð Bjarmason 31 August 2022, 23:18:00 UTC submodule--helper: don't redundantly check "else if (res)" The "res" variable must be true at this point in update_submodule(), as just a few lines above this we've unconditionally: if (!res) return 0; So we don't need to guard the "return 1" with an "else if (res)", we can return unconditionally at this point. See b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) for the initial introduction of this code, this check of "res" has always been redundant. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
9d02f94 Glen Choo 31 August 2022, 23:17:59 UTC submodule--helper: refactor "errmsg_str" to be a "struct strbuf" Refactor code added in e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13) so that "errmsg" and "errmsg_str" are folded into one. The distinction between the empty string and NULL is something that's tested for by e.g. "t/t7401-submodule-summary.sh". This is in preparation for fixing a memory leak the "struct strbuf" in the pre-image. Let's also pass a "const char *" to print_submodule_summary(), as it should not be modifying the "errmsg". Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:24 UTC
a253be6 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:58 UTC submodule--helper: add "const" to passed "struct update_data" Add a "const" to the "struct update_data" passed to run_update_procedure(), which it in turn passes along (peeled) to is_tip_reachable() and fetch_in_submodule()). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
1da635b Glen Choo 31 August 2022, 23:17:57 UTC submodule--helper: add "const" to copy of "update_data" Add a "const" to the copy of "struct update_data" that's tracked by the "struct submodule_update_clone", as it neither owns nor modifies it. Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
6fac5b2 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:56 UTC submodule--helper: add "const" to passed "module_clone_data" Add "const" to the "struct module_clone_data" that we pass to clone_submodule(), which makes the ownership clear, and stops us from clobbering the "clone_data->path". We still need to add to the "reference" member, which is a "struct string_list". Let's do this by having clone_submodule() create its own, and copy the contents over, allowing us to pass it as a separate parameter. This new "struct string_list" still leaks memory, just as the "struct module_clone_data" did before. let's not fix that for now, to fix that we'll need to add some "goto cleanup" to the relevant code. That will eventually be done in follow-up commits, this change makes it easier to fix the memory leak. The scope of the new "reference" variable in add_submodule() could be narrowed to the "else" block, but as we'll eventually free it with a "goto cleanup" let's declare it at the start of the function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
9bdf527 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:55 UTC submodule--helper: move "sb" in clone_submodule() to its own scope Refactor the only remaining use of a "struct strbuf sb" in clone_submodule() to live in its own scope. This makes the code clearer by limiting its lifetime. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
21496b4 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:54 UTC submodule--helper: use xstrfmt() in clone_submodule() Use xstrfmt() in clone_submodule() instead of a "struct strbuf" in two cases where we weren't getting anything out of using the "struct strbuf". This changes code that was was added along with other uses of "struct strbuf" in this function in ee8838d1577 (submodule: rewrite `module_clone` shell function in C, 2015-09-08). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
4b82d75 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:53 UTC submodule--helper: replace memset() with { 0 }-initialization Use the less verbose { 0 }-initialization syntax rather than memset() in builtin/submodule--helper.c, this doesn't make a difference in terms of behavior, but as we're about to modify adjacent code makes this more consistent, and lets us avoid worrying about when the memset() happens v.s. a "goto cleanup". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
0b83b2b Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:52 UTC submodule--helper style: add \n\n after variable declarations Since the preceding commit fixed style issues with \n\n among the declared variables let's fix the minor stylistic issues with those variables not being consistently followed by a \n\n. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
e2d5c88 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:51 UTC submodule--helper style: don't separate declared variables with \n\n The usual style in the codebase is to separate declared variables with a single newline, not two, let's adjust this code to conform to that. This makes the eventual addition of various "int ret" variables more consistent. In doing this the comment added in 2964d6e5e1e (submodule: port subcommand 'set-branch' from shell to C, 2020-06-02) might become ambiguous to some, although it should be clear what it's referring to, let's move it above the 'OPT_NOOP_NOARG('q', "quiet")' to make that clearer. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
96a28a9 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:50 UTC submodule--helper: move "resolve-relative-url-test" to a test-tool As its name suggests the "resolve-relative-url-test" has never been used outside of the test suite, see 63e95beb085 (submodule: port resolve_relative_url from shell to C, 2016-04-15) for its original addition. Perhaps it would make sense to drop this code entirely, as we feel that we've got enough indirect test coverage, but let's leave that question to a possible follow-up change. For now let's keep the test coverage this gives us. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
85321a3 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:49 UTC submodule--helper: move "check-name" to a test-tool Move the "check-name" helper to a test-tool, since a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10) it has only been used by this test, not git-submodule.sh. As noted with its introduction in 0383bbb9015 (submodule-config: verify submodule names as paths, 2018-04-30) the intent of t7450-bad-git-dotfiles.sh has always been to unit test the check_submodule_name() function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
9fb2a97 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:48 UTC submodule--helper: move "is-active" to a test-tool Create a new "test-tool submodule" and move the "is-active" subcommand over to it. It was added in 5c2bd8b77ae (submodule--helper: add is-active subcommand, 2017-03-16), since a452128a36c (submodule--helper: introduce add-config subcommand, 2021-08-06) it hasn't been used by git-submodule.sh. Since we're creating a command dispatch similar to test-tool.c itself let's split out the "struct test_cmd" into a new test-tool-utils.h, which both this new code and test-tool.c itself can use. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
255a1ae Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:47 UTC test-tool submodule-config: remove unused "--url" handling No test has used this "--url" parameter since the test code that made use of it was removed in 32bc548329d (submodule-config: remove support for overlaying repository config, 2017-08-03). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
3195547 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:46 UTC submodule--helper: remove unused "list" helper Remove the "submodule--helper list" sub-command, which hasn't been used by git-submodule.sh since 2964d6e5e1e (submodule: port subcommand 'set-branch' from shell to C, 2020-06-02). There was a test added in 2b56bb7a87a (submodule helper list: respect correct path prefix, 2016-02-24) which relied on it, but the right thing to do here is to delete that test as well. That test was regression testing the "list" subcommand itself. We're not getting anything useful from the "list | cut -f2" invocation that we couldn't get from "foreach 'echo $sm_path'". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
76d63dd Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:45 UTC submodule--helper: remove unused "name" helper The "name" helper has not been used since e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:23 UTC
59378e3 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:44 UTC submodule tests: test for "add <repository> <abs-path>" Add a missing test for ""add <repository> <path>" where "<path>" is an absolute path. This tests code added in [1] and later turned into an "else" branch in clone_submodule() in [2] that's never been tested. This needs to be skipped on WINDOWS because all of $PWD, $(pwd) and the "$(pwd -P)" we get via "$submodurl" would fail in CI with e.g.: fatal: could not create directory 'D:/a/git/git/t/trash directory.t7400-submodule-basic/.git/modules/D:/a/git/git/t/trash directory.t7400-submodule-basic/add-abs' I.e. we can't handle these sorts of paths in this context on that platform. I'm not sure where we run into the edges of "$PWD" behavior on Windows (see [1] for a previous loose end on the topic), but for the purposes of this test it's sufficient that we test this on other platforms. 1. ee8838d1577 (submodule: rewrite `module_clone` shell function in C, 2015-09-08) 2. f8eaa0ba98b (submodule--helper, module_clone: always operate on absolute paths, 2016-03-31) 1. https://lore.kernel.org/git/220630.86edz6c75c.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:22 UTC
89bc7b5 Ævar Arnfjörð Bjarmason 31 August 2022, 23:17:43 UTC submodule tests: test usage behavior Test what exit code and output we emit on "git submodule -h", how we handle "--" when no subcommand is specified, and how the top-level "--recursive" option is handled. For "-h" this doesn't make sense, but let's test for it so that any subsequent eventual behavior change will become clear. For "--" this follows up on 68cabbfda36 (submodule: document default behavior, 2019-02-15) and tests that "status" doesn't support the "--" delimiter. There's no intrinsically good reason not to support that. We behave this way due to edge cases in git-submodule.sh's implementation, but as with "-h" let's assert our current long-standing behavior for now. For "--recursive" the exclusion of it from the top-level appears to have been an omission in 15fc56a8536 (git submodule foreach: Add --recursive to recurse into nested submodules, 2009-08-19), there doesn't seem to be a reason not to support it alongside "--quiet" and "--cached", but let's likewise assert our existing behavior for now. I.e. as long as "status" is optional it would make sense to support all of its options when it's omitted, but we only do that with "--quiet" and "--cached", and curiously omit "--recursive". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 02 September 2022, 16:16:22 UTC
be1a02a Junio C Hamano 01 September 2022, 20:07:08 UTC The seventeenth batch Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 20:40:19 UTC
624a936 Junio C Hamano 01 September 2022, 20:40:19 UTC Merge branch 'en/merge-multi-strategies' The code that implements multi-strategy support in "git merge" has been clean-up a bit. * en/merge-multi-strategies: merge: small code readability improvement merge: cleanup confusing logic for handling successful merges 01 September 2022, 20:40:19 UTC
014a9ea Junio C Hamano 01 September 2022, 20:40:18 UTC Merge branch 'en/t4301-more-merge-tree-tests' More tests to protect the current behaviour of "merge-tree" before it gets further updated. * en/t4301-more-merge-tree-tests: t4301: add more interesting merge-tree testcases 01 September 2022, 20:40:19 UTC
3a47790 Junio C Hamano 01 September 2022, 20:40:18 UTC Merge branch 'en/merge-unstash-only-on-clean-merge' The auto-stashed local changes created by "git merge --autostash" was mixed into a conflicted state left in the working tree, which has been corrected. * en/merge-unstash-only-on-clean-merge: merge: only apply autostash when appropriate 01 September 2022, 20:40:18 UTC
d528044 Junio C Hamano 01 September 2022, 20:40:18 UTC Merge branch 'sg/parse-options-subcommand' Introduce the "subcommand" mode to parse-options API and update the command line parser of Git commands with subcommands. * sg/parse-options-subcommand: (23 commits) remote: run "remote rm" argv through parse_options() maintenance: add parse-options boilerplate for subcommands pass subcommand "prefix" arguments to parse_options() builtin/worktree.c: let parse-options parse subcommands builtin/stash.c: let parse-options parse subcommands builtin/sparse-checkout.c: let parse-options parse subcommands builtin/remote.c: let parse-options parse subcommands builtin/reflog.c: let parse-options parse subcommands builtin/notes.c: let parse-options parse subcommands builtin/multi-pack-index.c: let parse-options parse subcommands builtin/hook.c: let parse-options parse subcommands builtin/gc.c: let parse-options parse 'git maintenance's subcommands builtin/commit-graph.c: let parse-options parse subcommands builtin/bundle.c: let parse-options parse subcommands parse-options: add support for parsing subcommands parse-options: drop leading space from '--git-completion-helper' output parse-options: clarify the limitations of PARSE_OPT_NODASH parse-options: PARSE_OPT_KEEP_UNKNOWN only applies to --options api-parse-options.txt: fix description of OPT_CMDMODE t0040-parse-options: test parse_options() with various 'parse_opt_flags' ... 01 September 2022, 20:40:18 UTC
68ef042 Junio C Hamano 01 September 2022, 20:40:17 UTC Merge branch 'ds/bundle-uri-clone' Implement "git clone --bundle-uri". * ds/bundle-uri-clone: clone: warn on failure to repo_init() clone: --bundle-uri cannot be combined with --depth bundle-uri: add support for http(s):// and file:// clone: add --bundle-uri option bundle-uri: create basic file-copy logic remote-curl: add 'get' capability 01 September 2022, 20:40:17 UTC
9ff7eb8 Ævar Arnfjörð Bjarmason 25 August 2022, 17:09:49 UTC git-compat-util.h: use "deprecated" for UNUSED variables As noted in the preceding commit our "UNUSED" macro was no longer protecting against actual use of the "unused" variables, which it was previously doing by renaming the variable. Let's instead use the "deprecated" attribute to accomplish that goal. As [1] rightly notes this has the drawback that compiling with "-Wno-deprecated-declarations" will silence any such uses. I think the trade-off is worth it as: * We can consider that a feature, as e.g. backporting certain patches might use a now "unused" parameter, and the person doing that might want to silence it with DEVOPTS=no-error. * This way we play nicely with coccinelle, and any other dumb(er) parser of C (such as syntax highlighters). * Not every single compilation of git needs to catch "used but declared unused" parameters. It's sufficient that the default "make DEVELOPER=1" will do so, and that the "static-analysis" CI job will catch it. 1. https://lore.kernel.org/git/YwCtkwjWdJVHHZV0@coredump.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:49:49 UTC
5cf88fd Ævar Arnfjörð Bjarmason 25 August 2022, 17:09:48 UTC git-compat-util.h: use "UNUSED", not "UNUSED(var)" As reported in [1] the "UNUSED(var)" macro introduced in 2174b8c75de (Merge branch 'jk/unused-annotation' into next, 2022-08-24) breaks coccinelle's parsing of our sources in files where it occurs. Let's instead partially go with the approach suggested in [2] of making this not take an argument. As noted in [1] "coccinelle" will ignore such tokens in argument lists that it doesn't know about, and it's less of a surprise to syntax highlighters. This undoes the "help us notice when a parameter marked as unused is actually use" part of 9b240347543 (git-compat-util: add UNUSED macro, 2022-08-19), a subsequent commit will further tweak the macro to implement a replacement for that functionality. 1. https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ 2. https://lore.kernel.org/git/220819.868rnk54ju.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:49:48 UTC
fb41727 Eric Sunshine 01 September 2022, 00:29:56 UTC t: retire unused chainlint.sed Retire chainlint.sed since it has been replaced by a more accurate and functional &&-chain "linter", thus is no longer used. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
69b9924 Eric Sunshine 01 September 2022, 00:29:55 UTC t/Makefile: teach `make test` and `make prove` to run chainlint.pl Unlike chainlint.sed which "lints" a single test body at a time, thus is invoked once per test, chainlint.pl can check all test bodies in all test scripts with a single invocation. As such, it is akin to other bulk "linters" run by the Makefile, such as `test-lint-shell-syntax`, `test-lint-duplicates`, etc. Therefore, teach `make test` and `make prove` to invoke chainlint.pl along with the other bulk linters. Also, since the single chainlint.pl invocation by `make test` or `make prove` has already checked all tests in all scripts, instruct the individual test scripts not to run chainlint.pl on themselves unnecessarily. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
23a14f3 Eric Sunshine 01 September 2022, 00:29:54 UTC test-lib: replace chainlint.sed with chainlint.pl By automatically invoking chainlint.sed upon each test it runs, `test_run_` in test-lib.sh ensures that broken &&-chains will be detected early as tests are modified or new are tests created since it is typical to run a test script manually (i.e. `./t1234-test-script.sh`) during test development. Now that the implementation of chainlint.pl is complete, modify test-lib.sh to invoke it automatically instead of chainlint.sed each time a test script is run. This change reduces the number of "linter" invocations from 26800+ (once per test run) down to 1050+ (once per test script), however, a subsequent change will drop the number of invocations to 1 per `make test`, thus fully realizing the benefit of the new linter. Note that the "magic exit code 117" &&-chain checker added by bb79af9d09 (t/test-lib: introduce --chain-lint option, 2015-03-20) which is built into t/test-lib.sh is retained since it has near zero-cost and (theoretically) may catch a broken &&-chain not caught by chainlint.pl. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
9fd9112 Eric Sunshine 01 September 2022, 00:29:53 UTC test-lib: retire "lint harder" optimization hack `test_run_` in test-lib.sh "lints" the body of a test by sending it down a `sed chainlint.sed | grep` pipeline; this happens once for each test run by a test script. Although this pipeline may seem relatively cheap in isolation, it can become expensive when invoked 26800+ times by `make test`, once for each test run, despite the existence of only 16500+ test definitions across all tests scripts. This difference in the number of tests defined in the scripts (16500+) and the number of tests actually run by `make test` (26800+) is explained by the fact that some test scripts run a very large number of small tests, all driven by a series of functions/loops which fill in the test bodies. This means that certain test definitions are being linted repeatedly (tens or hundreds of times) unnecessarily. To avoid such unnecessary work, 2d86a96220 (t: avoid sed-based chain-linting in some expensive cases, 2021-05-13) added an optimization hack which allows individual scripts to manually suppress the unnecessary repeated linting of the same test definition. However, unlike chainlint.sed which checks a test body as the test is run, chainlint.pl checks each test definition just once, no matter how many times the test is run, thus the sort of optimization hack introduced by 2d86a96220 is no longer needed and can be retired. Therefore, revert 2d86a96220. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
5606652 Eric Sunshine 01 September 2022, 00:29:52 UTC t/chainlint: add more chainlint.pl self-tests During the development of chainlint.pl, numerous new self-tests were created to verify correct functioning beyond the checks already represented by the existing self-tests. The new checks fall into several categories: * behavior of the lexical analyzer for complex cases, such as line splicing, token pasting, entering and exiting string contexts inside and outside of test script bodies; for instance: test_expect_success 'title' ' x=$(echo "something" | sed -e '\''s/\\/\\\\/g'\'' -e '\''s/[[/.*^$]/\\&/g'\'' ' * behavior of the parser for all compound grammatical constructs, such as `if...fi`, `case...esac`, `while...done`, `{...}`, etc., and for other legal shell grammatical constructs not covered by existing chainlint.sed self-tests, as well as complex cases, such as: OUT=$( ((large_git 1>&3) | :) 3>&1 ) && * detection of problems, such as &&-chain breakage, from top-level to any depth since the existing self-tests do not cover any top-level context and only cover subshells one level deep due to limitations of chainlint.sed * address blind spots in chainlint.sed (such as not detecting a broken &&-chain on a one-line for-loop in a subshell[1]) which chainlint.pl correctly detects * real-world cases which tripped up chainlint.pl during its development [1]: https://lore.kernel.org/git/dce35a47012fecc6edc11c68e91dbb485c5bc36f.1661663880.git.gitgitgadget@gmail.com/ Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
ae0c55a Eric Sunshine 01 September 2022, 00:29:51 UTC chainlint.pl: allow `|| echo` to signal failure upstream of a pipe The use of `|| return` (or `|| exit`) to signal failure within a loop isn't effective when the loop is upstream of a pipe since the pipe swallows all upstream exit codes and returns only the exit code of the final command in the pipeline. To work around this limitation, tests may adopt an alternative strategy of signaling failure by emitting text which would never be emitted in the non-failing case. For instance: while condition do command1 && command2 || echo "impossible text" done | sort >actual && Such usage indicates deliberate thought about failure cases by the test author, thus flagging them as missing `|| return` (or `|| exit`) is not helpful. Therefore, take this case into consideration when checking for explicit loop termination. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
fd4094c Eric Sunshine 01 September 2022, 00:29:50 UTC chainlint.pl: complain about loops lacking explicit failure handling Shell `for` and `while` loops do not terminate automatically just because a command fails within the loop body. Instead, the loop continues to iterate and eventually returns the exit status of the final command of the final iteration, which may not be the command which failed, thus it is possible for failures to go undetected. Consequently, it is important for test authors to explicitly handle failure within the loop body by terminating the loop manually upon failure. This can be done by returning a non-zero exit code from within the loop body (i.e. `|| return 1`) or exiting (i.e. `|| exit 1`) if the loop is within a subshell, or by manually checking `$?` and taking some appropriate action. Therefore, add logic to detect and complain about loops which lack explicit `return` or `exit`, or `$?` check. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
832c68b Eric Sunshine 01 September 2022, 00:29:49 UTC chainlint.pl: don't flag broken &&-chain if failure indicated explicitly There are quite a few tests which print an error messages and then explicitly signal failure with `false`, `return 1`, or `exit 1` as the final command in an `if` branch. In these cases, the tests don't bother maintaining the &&-chain between `echo` and the explicit "test failed" indicator. Since such constructs are manually signaling failure, their &&-chain breakage is legitimate and safe -- both for the command immediately preceding `false`, `return`, or `exit`, as well as for all preceding commands in the `if` branch. Therefore, stop flagging &&-chain breakage in these sorts of cases. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 01 September 2022, 17:07:41 UTC
back to top