* What's cooking in git.git (Oct 2023, #05; Thu, 12)
From: Junio C Hamano @ 2023-10-12 23:21 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
There are too many topics marked with "Needs review" label. I can
help reviewing some of them myself, but obviously it will not scale.
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* jm/git-status-submodule-states-docfix (2023-10-04) 1 commit
(merged to 'next' on 2023-10-04 at 520b7711a4)
+ git-status.txt: fix minor asciidoc format issue
Docfix.
source: <pull.1591.v3.git.1696386165616.gitgitgadget@gmail.com>
* js/ci-coverity (2023-10-05) 6 commits
(merged to 'next' on 2023-10-05 at 253788f0d1)
+ coverity: detect and report when the token or project is incorrect
+ coverity: allow running on macOS
+ coverity: support building on Windows
+ coverity: allow overriding the Coverity project
+ coverity: cache the Coverity Build Tool
+ ci: add a GitHub workflow to submit Coverity scans
GitHub CI workflow has learned to trigger Coverity check.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
* rs/parse-opt-ctx-cleanup (2023-10-03) 1 commit
(merged to 'next' on 2023-10-04 at d5d0a2ce3b)
+ parse-options: drop unused parse_opt_ctx_t member
Code clean-up.
source: <ebcaa9e1-d306-4c93-adec-3f35d7040531@web.de>
--------------------------------------------------
[New Topics]
* jc/fail-stash-to-store-non-stash (2023-10-11) 1 commit
- stash: be careful what we store
Feeding "git stash store" with a random commit that was not created
by "git stash create" now errors out.
Will merge to 'next'?
source: <xmqqbkd4lwj0.fsf_-_@gitster.g>
* jc/doc-unit-tests-fixup (2023-10-11) 1 commit
- SQUASH???
(this branch uses js/doc-unit-tests and js/doc-unit-tests-with-cmake.)
Quick fix for jc/doc-unit-tests topic to unbreak CI running on 'seen'.
source: <xmqqwmvskf8t.fsf@gitster.g>
* bc/racy-4gb-files (2023-10-12) 2 commits
- Prevent git from rehashing 4GiB files
- t: add a test helper to truncate files
The index file has room only for lower 32-bit of the file size in
the cached stat information, which means cached stat information
will have 0 in its sd_size member for a file whose size is multiple
of 4GiB. This is mistaken for a racily clean path. Avoid it by
storing a bogus sd_size value instead for such files.
Waiting for review response.
source: <20231012160930.330618-1-sandals@crustytoothpaste.net>
* ds/mailmap-entry-update (2023-10-12) 1 commit
(merged to 'next' on 2023-10-12 at 3de300ac62)
+ mailmap: change primary address for Derrick Stolee
Update mailmap entry for Derrick.
Will merge to 'master' immediately.
source: <pull.1592.git.1697131834003.gitgitgadget@gmail.com>
* jc/grep-f-relative-to-cwd (2023-10-12) 1 commit
- grep: -f <path> is relative to $cwd
"cd sub && git grep -f patterns" tried to read "patterns" file at
the top level of the working tree; it has been corrected to read
"sub/patterns" instead.
Needs review.
source: <xmqqedhzg37z.fsf@gitster.g>
--------------------------------------------------
[Stalled]
* pw/rebase-sigint (2023-09-07) 1 commit
- rebase -i: ignore signals when forking subprocesses
If the commit log editor or other external programs (spawned via
"exec" insn in the todo list) receive internactive signal during
"git rebase -i", it caused not just the spawned program but the
"Git" process that spawned them, which is often not what the end
user intended. "git" learned to ignore SIGINT and SIGQUIT while
waiting for these subprocesses.
Expecting a reroll.
cf. <12c956ea-330d-4441-937f-7885ab519e26@gmail.com>
source: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
* tk/cherry-pick-sequence-requires-clean-worktree (2023-06-01) 1 commit
- cherry-pick: refuse cherry-pick sequence if index is dirty
"git cherry-pick A" that replays a single commit stopped before
clobbering local modification, but "git cherry-pick A..B" did not,
which has been corrected.
Expecting a reroll.
cf. <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
source: <pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com>
* jc/diff-cached-fsmonitor-fix (2023-09-15) 3 commits
- diff-lib: fix check_removed() when fsmonitor is active
- Merge branch 'jc/fake-lstat' into jc/diff-cached-fsmonitor-fix
- Merge branch 'js/diff-cached-fsmonitor-fix' into jc/diff-cached-fsmonitor-fix
(this branch uses jc/fake-lstat.)
The optimization based on fsmonitor in the "diff --cached"
codepath is resurrected with the "fake-lstat" introduced earlier.
It is unknown if the optimization is worth resurrecting, but in case...
source: <xmqqr0n0h0tw.fsf@gitster.g>
--------------------------------------------------
[Cooking]
* tb/path-filter-fix (2023-10-10) 17 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
- commit-graph: ensure Bloom filters are read with consistent settings
- revision.c: consult Bloom filters for root commits
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Needs (hopefully final and quick) review.
source: <cover.1696969994.git.me@ttaylorr.com>
* ak/pretty-decorate-more-fix (2023-10-09) 1 commit
(merged to 'next' on 2023-10-12 at 3cbb4c2268)
+ pretty: fix ref filtering for %(decorate) formats
Unlike "git log --pretty=%D", "git log --pretty="%(decorate)" did
not auto-initialize the decoration subsystem, which has been
corrected.
Will merge to 'master'.
source: <20231008202307.1568477-1-andy.koppe@gmail.com>
* en/docfixes (2023-10-09) 25 commits
- documentation: add missing parenthesis
- documentation: add missing quotes
- documentation: add missing fullstops
- documentation: add some commas where they are helpful
- documentation: fix whitespace issues
- documentation: fix capitalization
- documentation: fix punctuation
- documentation: use clearer prepositions
- documentation: add missing hyphens
- documentation: remove unnecessary hyphens
- documentation: add missing article
- documentation: fix choice of article
- documentation: whitespace is already generally plural
- documentation: fix singular vs. plural
- documentation: fix verb vs. noun
- documentation: fix adjective vs. noun
- documentation: fix verb tense
- documentation: employ consistent verb tense for a list
- documentation: fix subject/verb agreement
- documentation: remove extraneous words
- documentation: add missing words
- documentation: fix apostrophe usage
- documentation: fix typos
- documentation: fix small error
- documentation: wording improvements
Documentation typo and grammo fixes.
Needs review.
source: <pull.1595.git.1696747527.gitgitgadget@gmail.com>
* kn/rev-list-missing-fix (2023-10-09) 3 commits
. rev-list: add commit object support in `--missing` option
. rev-list: move `show_commit()` to the bottom
. revision: rename bit to `do_not_die_on_missing_objects`
"git rev-list --missing" did not work for missing commit objects,
which has been corrected.
Needs review.
Seems to break CI job with extra environment settings.
cf. <xmqqil7etndo.fsf@gitster.g>
source: <20231009105528.17777-1-karthik.188@gmail.com>
* sn/cat-file-doc-update (2023-10-09) 1 commit
(merged to 'next' on 2023-10-10 at 6719ba7bd4)
+ doc/cat-file: make synopsis and description less confusing
"git cat-file" documentation updates.
Will merge to 'master'.
source: <20231009175604.2361700-1-stepnem@smrk.net>
* xz/commit-title-soft-limit-doc (2023-10-09) 1 commit
(merged to 'next' on 2023-10-10 at 0bf3d9ed51)
+ doc: correct the 50 characters soft limit (+)
Doc update.
Will merge to 'master'.
source: <pull.1585.v2.git.git.1696778367151.gitgitgadget@gmail.com>
* jk/chunk-bounds (2023-10-09) 20 commits
(merged to 'next' on 2023-10-10 at 21139603ce)
+ chunk-format: drop pair_chunk_unsafe()
+ commit-graph: detect out-of-order BIDX offsets
+ commit-graph: check bounds when accessing BIDX chunk
+ commit-graph: check bounds when accessing BDAT chunk
+ commit-graph: bounds-check generation overflow chunk
+ commit-graph: check size of generations chunk
+ commit-graph: bounds-check base graphs chunk
+ commit-graph: detect out-of-bounds extra-edges pointers
+ commit-graph: check size of commit data chunk
+ midx: check size of revindex chunk
+ midx: bounds-check large offset chunk
+ midx: check size of object offset chunk
+ midx: enforce chunk alignment on reading
+ midx: check size of pack names chunk
+ commit-graph: check consistency of fanout table
+ midx: check size of oid lookup chunk
+ commit-graph: check size of oid fanout chunk
+ midx: stop ignoring malformed oid fanout chunk
+ t: add library for munging chunk-format files
+ chunk-format: note that pair_chunk() is unsafe
The codepaths that read "chunk" formatted files have been corrected
to pay attention to the chunk size and notice broken files.
Will merge to 'master'.
source: <20231009205544.GA3281950@coredump.intra.peff.net>
* ps/rewritten-is-per-worktree-doc (2023-10-10) 1 commit
(merged to 'next' on 2023-10-12 at 501b960e8c)
+ doc/git-worktree: mention "refs/rewritten" as per-worktree refs
Doc update.
Will merge to 'master'.
source: <985ac850eb6e60ae76601acc8bfbcd56f99348b4.1696935657.git.ps@pks.im>
* jc/merge-ort-attr-index-fix (2023-10-09) 1 commit
(merged to 'next' on 2023-10-10 at b139b87502)
+ merge-ort: initialize repo in index state
Fix "git merge-tree" to stop segfaulting when the --attr-source
option is used.
Will merge to 'master'.
source: <pull.1583.v3.git.git.1696857660374.gitgitgadget@gmail.com>
* jk/decoration-and-other-leak-fixes (2023-10-05) 3 commits
(merged to 'next' on 2023-10-06 at 5fc05c94dc)
+ daemon: free listen_addr before returning
+ revision: clear decoration structs during release_revisions()
+ decorate: add clear_decoration() function
Leakfix.
Will merge to 'master'.
source: <20231005212802.GA982892@coredump.intra.peff.net>
* sn/typo-grammo-phraso-fixes (2023-10-05) 5 commits
- t/README: fix multi-prerequisite example
- doc/gitk: s/sticked/stuck/
- git-jump: admit to passing merge mode args to ls-files
- doc/diff-options: improve wording of the log.diffMerges mention
- doc: fix some typos, grammar and wording issues
Many typos, ungrammatical sentences and wrong phrasing have been
fixed.
Needs review.
source: <20231003082107.3002173-1-stepnem@smrk.net>
* so/diff-merges-dd (2023-10-09) 3 commits
- completion: complete '--dd'
- diff-merges: introduce '--dd' option
- diff-merges: improve --diff-merges documentation
"git log" and friends learned "--dd" that is a short-hand for
"--diff-merges=first-parent -p".
Will merge to 'next'?
source: <20231009160535.236523-1-sorganov@gmail.com>
* vd/loose-ref-iteration-optimization (2023-10-09) 4 commits
(merged to 'next' on 2023-10-12 at 99e2f83855)
+ files-backend.c: avoid stat in 'loose_fill_ref_dir'
+ dir.[ch]: add 'follow_symlink' arg to 'get_dtype'
+ dir.[ch]: expose 'get_dtype'
+ ref-cache.c: fix prefix matching in ref iteration
The code to iterate over loose references have been optimized to
reduce the number of lstat() system calls.
Will merge to 'master'.
source: <pull.1594.v2.git.1696888736.gitgitgadget@gmail.com>
* jc/update-list-references-to-lore (2023-10-06) 1 commit
- doc: update list archive reference to use lore.kernel.org
Doc update.
Needs review.
source: <xmqq7cnz741s.fsf@gitster.g>
* cc/git-replay (2023-10-10) 14 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: use standard revision ranges
- replay: make it a minimal server side command
- replay: remove HEAD related sanity check
- replay: remove progress and info output
- replay: add an important FIXME comment about gpg signing
- replay: change rev walking options
- replay: introduce pick_regular_commit()
- replay: die() instead of failing assert()
- replay: start using parse_options API
- replay: introduce new builtin
- t6429: remove switching aspects of fast-rebase
Introduce "git replay", a tool meant on the server side without
working tree to recreate a history.
Needs (hopefully final and quick) review.
source: <20231010123847.2777056-1-christian.couder@gmail.com>
* jk/commit-graph-leak-fixes (2023-10-03) 10 commits
(merged to 'next' on 2023-10-06 at 5d202ef8b9)
+ commit-graph: clear oidset after finishing write
+ commit-graph: free write-context base_graph_name during cleanup
+ commit-graph: free write-context entries before overwriting
+ commit-graph: free graph struct that was not added to chain
+ commit-graph: delay base_graph assignment in add_graph_to_chain()
+ commit-graph: free all elements of graph chain
+ commit-graph: move slab-clearing to close_commit_graph()
+ merge: free result of repo_get_merge_bases()
+ commit-reach: free temporary list in get_octopus_merge_bases()
+ t6700: mark test as leak-free
Leakfix.
Will merge to 'master'.
source: <20231003202504.GA7697@coredump.intra.peff.net>
* tb/repack-max-cruft-size (2023-10-09) 5 commits
(merged to 'next' on 2023-10-09 at 38f039e880)
+ repack: free existing_cruft array after use
(merged to 'next' on 2023-10-06 at b3ca6df3b9)
+ builtin/repack.c: avoid making cruft packs preferred
+ builtin/repack.c: implement support for `--max-cruft-size`
+ builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
+ t7700: split cruft-related tests to t7704
"git repack" learned "--max-cruft-size" to prevent cruft packs from
growing without bounds.
Will merge to 'master'.
source: <cover.1696293862.git.me@ttaylorr.com>
source: <20231007172031.GA1524950@coredump.intra.peff.net>
source: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
* ak/color-decorate-symbols (2023-10-03) 1 commit
- decorate: add color.decorate.symbols config option
A new config for coloring.
Needs review.
source: <20231003205442.22963-1-andy.koppe@gmail.com>
* jc/attr-tree-config (2023-10-11) 2 commits
- attr: add attr.tree for setting the treeish to read attributes from
- attr: read attributes from HEAD when bare repo
The attribute subsystem learned to honor `attr.tree` configuration
that specifies which tree to read the .gitattributes files from.
Will merge to 'next'?
source: <pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com>
* js/submodule-fix-misuse-of-path-and-name (2023-10-03) 6 commits
(merged to 'next' on 2023-10-06 at 1054b6e752)
+ t7420: test that we correctly handle renamed submodules
+ t7419: test that we correctly handle renamed submodules
+ t7419, t7420: use test_cmp_config instead of grepping .gitmodules
+ t7419: actually test the branch switching
+ submodule--helper: return error from set-url when modifying failed
+ submodule--helper: use submodule_from_path in set-{url,branch}
In .gitmodules files, submodules are keyed by their names, and the
path to the submodule whose name is $name is specified by the
submodule.$name.path variable. There were a few codepaths that
mixed the name and path up when consulting the submodule database,
which have been corrected. It took long for these bugs to be found
as the name of a submodule initially is the same as its path, and
the problem does not surface until it is moved to a different path,
which apparently happens very rarely.
Will merge to 'master'.
source: <0a0a157f88321d25fdb0be771a454b3410a449f3.camel@archlinux.org>
* ar/diff-index-merge-base-fix (2023-10-02) 1 commit
(merged to 'next' on 2023-10-06 at 0ff4dfc0e1)
+ diff: fix --merge-base with annotated tags
"git diff --merge-base X other args..." insisted that X must be a
commit and errored out when given an annotated tag that peels to a
commit, but we only need it to be a committish. This has been
corrected.
Will merge to 'master'.
source: <20231001151845.3621551-1-hi@alyssa.is>
* js/update-urls-in-doc-and-comment (2023-09-26) 4 commits
- doc: refer to internet archive
- doc: update links for andre-simon.de
- doc: update links to current pages
- doc: switch links to https
Stale URLs have been updated to their current counterparts (or
archive.org) and HTTP links are replaced with working HTTPS links.
Needs review.
source: <pull.1589.v2.git.1695553041.gitgitgadget@gmail.com>
* la/trailer-cleanups (2023-09-26) 4 commits
- trailer: only use trailer_block_* variables if trailers were found
- trailer: use offsets for trailer_start/trailer_end
- trailer: find the end of the log message
- commit: ignore_non_trailer computes number of bytes to ignore
Code clean-up.
Needs review.
source: <pull.1563.v4.git.1695709372.gitgitgadget@gmail.com>
* eb/hash-transition (2023-10-02) 30 commits
- t1016-compatObjectFormat: add tests to verify the conversion between objects
- t1006: test oid compatibility with cat-file
- t1006: rename sha1 to oid
- test-lib: compute the compatibility hash so tests may use it
- builtin/ls-tree: let the oid determine the output algorithm
- object-file: handle compat objects in check_object_signature
- tree-walk: init_tree_desc take an oid to get the hash algorithm
- builtin/cat-file: let the oid determine the output algorithm
- rev-parse: add an --output-object-format parameter
- repository: implement extensions.compatObjectFormat
- object-file: update object_info_extended to reencode objects
- object-file-convert: convert commits that embed signed tags
- object-file-convert: convert commit objects when writing
- object-file-convert: don't leak when converting tag objects
- object-file-convert: convert tag objects when writing
- object-file-convert: add a function to convert trees between algorithms
- object: factor out parse_mode out of fast-import and tree-walk into in object.h
- cache: add a function to read an OID of a specific algorithm
- tag: sign both hashes
- commit: export add_header_signature to support handling signatures on tags
- commit: convert mergetag before computing the signature of a commit
- commit: write commits for both hashes
- object-file: add a compat_oid_in parameter to write_object_file_flags
- object-file: update the loose object map when writing loose objects
- loose: compatibilty short name support
- loose: add a mapping between SHA-1 and SHA-256 for loose objects
- repository: add a compatibility hash algorithm
- object-names: support input of oids in any supported hash
- oid-array: teach oid-array to handle multiple kinds of oids
- object-file-convert: stubs for converting from one object format to another
Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.
Needs review.
source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
* jx/remote-archive-over-smart-http (2023-10-04) 4 commits
- archive: support remote archive from stateless transport
- transport-helper: call do_take_over() in connect_helper
- transport-helper: call do_take_over() in process_connect
- transport-helper: no connection restriction in connect_helper
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
Needs review.
source: <cover.1696432593.git.zhiyou.jx@alibaba-inc.com>
* jx/sideband-chomp-newline-fix (2023-10-04) 3 commits
- pkt-line: do not chomp newlines for sideband messages
- pkt-line: memorize sideband fragment in reader
- test-pkt-line: add option parser for unpack-sideband
Sideband demultiplexer fixes.
Needs review.
source: <cover.1696425168.git.zhiyou.jx@alibaba-inc.com>
* ty/merge-tree-strategy-options (2023-10-11) 2 commits
(merged to 'next' on 2023-10-12 at 9b873601df)
+ merge: introduce {copy|clear}_merge_options()
(merged to 'next' on 2023-09-29 at aa65b54416)
+ merge-tree: add -X strategy option
"git merge-tree" learned to take strategy backend specific options
via the "-X" option, like "git merge" does.
Will merge to 'master'.
source: <pull.1565.v6.git.1695522222723.gitgitgadget@gmail.com>
* js/config-parse (2023-09-21) 5 commits
- config-parse: split library out of config.[c|h]
- config.c: accept config_parse_options in git_config_from_stdin
- config: report config parse errors using cb
- config: split do_event() into start and flush operations
- config: split out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
Needs review.
source: <cover.1695330852.git.steadmon@google.com>
* jc/fake-lstat (2023-09-15) 1 commit
- cache: add fake_lstat()
(this branch is used by jc/diff-cached-fsmonitor-fix.)
A new helper to let us pretend that we called lstat() when we know
our cache_entry is up-to-date via fsmonitor.
Needs review.
source: <xmqqcyykig1l.fsf@gitster.g>
* rs/parse-options-value-int (2023-09-18) 2 commits
- parse-options: use and require int pointer for OPT_CMDMODE
- parse-options: add int value pointer to struct option
A bit of type safety for the "value" pointer used in the
parse-options API.
Not ready yet.
cf. <4014e490-c6c1-453d-b0ed-645220e3e614@web.de>
source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
* la/trailer-test-and-doc-updates (2023-09-07) 13 commits
(merged to 'next' on 2023-10-06 at 69fef35819)
+ trailer doc: <token> is a <key> or <keyAlias>, not both
+ trailer doc: separator within key suppresses default separator
+ trailer doc: emphasize the effect of configuration variables
+ trailer --unfold help: prefer "reformat" over "join"
+ trailer --parse docs: add explanation for its usefulness
+ trailer --only-input: prefer "configuration variables" over "rules"
+ trailer --parse help: expose aliased options
+ trailer --no-divider help: describe usual "---" meaning
+ trailer: trailer location is a place, not an action
+ trailer doc: narrow down scope of --where and related flags
+ trailer: add tests to check defaulting behavior with --no-* flags
+ trailer test description: this tests --where=after, not --where=before
+ trailer tests: make test cases self-contained
Test coverage for trailers has been improved.
Will merge to 'master'.
source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
* js/doc-unit-tests (2023-10-09) 3 commits
- ci: run unit tests in CI
- unit tests: add TAP unit test framework
- unit tests: add a project plan document
(this branch is used by jc/doc-unit-tests-fixup and js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Expecting a reroll to address CI breakage.
source: <cover.1696889529.git.steadmon@google.com>
* js/doc-unit-tests-with-cmake (2023-10-09) 7 commits
- cmake: handle also unit tests
- cmake: use test names instead of full paths
- cmake: fix typo in variable name
- artifacts-tar: when including `.dll` files, don't forget the unit-tests
- unit-tests: do show relative file paths
- unit-tests: do not mistake `.pdb` files for being executable
- cmake: also build unit tests
(this branch is used by jc/doc-unit-tests-fixup; uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
Needs review.
source: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
Code clean-up.
Not ready to be reviewed yet.
source: <20230824205456.1231371-1-gitster@pobox.com>
* rj/status-bisect-while-rebase (2023-08-01) 1 commit
- status: fix branch shown when not only bisecting
"git status" is taught to show both the branch being bisected and
being rebased when both are in effect at the same time.
Needs review.
cf. <xmqqtttia3vn.fsf@gitster.g>
source: <48745298-f12b-8efb-4e48-90d2c22a8349@gmail.com>
--------------------------------------------------
[Discarded]
* tb/ci-coverity (2023-09-21) 1 commit
. .github/workflows: add coverity action
GitHub CI workflow has learned to trigger Coverity check.
Superseded by the js/ci-coverity topic.
source: <b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com>
* cw/git-std-lib (2023-09-11) 7 commits
. SQUASH???
. git-std-lib: add test file to call git-std-lib.a functions
. git-std-lib: introduce git standard library
. parse: create new library for parsing strings and env values
. config: correct bad boolean env value error message
. wrapper: remove dependency to Git-specific internal file
. hex-ll: split out functionality from hex
Another libification effort.
Superseded by the cw/prelim-cleanup topic.
cf. <xmqqy1hfrk6p.fsf@gitster.g>
cf. <20230915183927.1597414-1-jonathantanmy@google.com>
source: <20230908174134.1026823-1-calvinwan@google.com>
* so/diff-merges-d (2023-09-11) 2 commits
- diff-merges: introduce '-d' option
- diff-merges: improve --diff-merges documentation
Superseded by the so/diff-merges-dd topic.
source: <20230909125446.142715-1-sorganov@gmail.com>
* kn/rev-list-ignore-missing-links (2023-09-20) 1 commit
. revision: add `--ignore-missing-links` user option
Surface the .ignore_missing_links bit that stops the revision
traversal from stopping and dying when encountering a missing
object to a new command line option of "git rev-list", so that the
objects that are required but are missing can be enumerated.
Superseded by kn/rev-list-missing-fix
source: <20230920104507.21664-1-karthik.188@gmail.com>
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-13 4:43 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: git
In-Reply-To: <ec91ff19cca3d881d4746208744663c650ebd250.camel@scientia.org>
On 2023-10-13 01:06, Christoph Anton Mitterer wrote:
> On Fri, 2023-10-13 at 00:36 +0200, Dragan Simic wrote:
>> It seems that "--redraw-on-quit" is a possible candidate for
>> replacing "-X" in the set of default options for less(1)
>
> *If* some changes were made to how git handles this, it might perhaps
> be worth to consider not to touch LESS at all, but only add the
> required settings via command line arguments (i.e. -F -R ...).
Actually, that would be wrong. If someone sets $LESS or $PAGER (or
$GIT_PAGER, more specifically), it's up to the utility that invokes the
pager internally not to override the user preferences configured through
these environment variables. That's how everyone can customize the
pager behavior.
> Or perhaps only remove options from it, if they're known to break the
> behaviour with git (like -+R might).
Again, not the way the whole thing with pagination works. If someone
sets their environment variables wrong, it's simply the way they want
it, and it isn't anyone else's business to attempt fixing it
automatically.
> I always feel configuration via env vars is a bit fragile:
> - especially when one has generic names like POSIXLY_CORRECT there's
> some chance that by exporting it to one program, where one wants the
> effect, another program started off by that also gets it
> unintentionally
> - generic terms may be used by multiple programs, causing problems
Well, fragile or not, that's the way it works. It has its downsides for
sure, but it's all about having each utility handle the environment
carefully and document it in its man page(s), so the users can also
carefully craft the values of their customized environment variables.
> Also, if one can set only one LESS var in the environment, not one for
> less "alone", one for less with git, etc. - that is unless for programs
> like bat/delta which have specific own env vars to set the pager.
$LESS can be seen as a global set of the common options for less(1),
which may include the coloring configuration or the enablement of
case-insensitive search, for example, while $MANPAGER, $GIT_PAGER and
$BAT_PAGER may contain utility-specific options for less(1). That's
actually very good, because it makes possible to avoid duplication of
the common options.
> So if I set e.g. LESS to something, than typically only to stuff from
> which I believe it works as expected for any possible users.
> E.g. -F might be such a case.
It's up to everyone to decide what are the common options for less(1)
that they want to set in $LESS.
> But if I do that, git won't touch LESS and set the required -R, so I
> have to do that manually for git, e.g. either via git_config or by
> defining an alias git='LESS=FRX git'.
You don't have to define an alias, there's $GIT_PAGER for that purpose,
as I already explained above.
Moreover, the whole idea of the various utilities touching the $LESS
variable internally is to provide sane defaults to the users that don't
configure $LESS, $PAGER, etc. on their own. Once the user starts to
provide their own environment variable(s), it's no longer up to the
utility to help the user by altering their environment configuration.
> But in both cases it would "break" again, should ever another option be
> needed and added by git to the default LESS (which is however only set
> when it's unset).
> And in case of an alias, there would be the additional problem, that
> it's typically not picked up in non-interactive shells.
As you can see in my replies, it isn't about taking care of the users
who provide their own environment configuration. It's all about
providing the set of sane defaults, for the users with no custom
configuration.
> Long story short, it might make sense for git, to (mostly) ignore LESS
> and rather invoke less with -F -R.
No, that would be wrong on multiple levels, as I already explained in
detail.
> The problem with that in turn would of course be that it doesn't
> automatically propagate down, if e.g. git's pager is set to detla and
> delta in turn runs less.
> However, that's IMO litte concern, since then it's delta's duty to set
> -R (if it think it needs to do so), which it actually does.
I don't know what delta is and how it actually paginates its outputs,
but it should follow the rules of the environment-based pager
configuration that I described in detail above.
^ permalink raw reply
* Re: [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Patrick Steinhardt @ 2023-10-13 5:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, git
In-Reply-To: <xmqqy1g7hl2y.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3888 bytes --]
On Thu, Oct 12, 2023 at 09:17:09AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Wouldn't this have the potential to significantly regress performance
> > for all those preexisting users of the `--missing` option? The commit
> > graph is quite an important optimization nowadays, and especially in
> > commands where we potentially walk a lot of commits (like we may do
> > here) it can result in queries that are orders of magnitudes faster.
>
> The test fails only when GIT_TEST_COMMIT_GRAPH is on, which updates
> the commit-graph every time a commit is made via "git commit" or
> "git merge".
>
> I'd suggest stepping back and think a bit.
>
> My assumption has been that the failing test emulates this scenario
> that can happen in real life:
>
> * The user creates a new commit.
>
> * A commit graph is written (not as part of GIT_TEST_COMMIT_GRAPH
> that is not realistic, but as part of "maintenance").
>
> * The repository loses some objects due to corruption.
>
> * Now, "--missing=print" is invoked so that the user can view what
> are missing. Or "--missing=allow-primisor" to ensure that the
> repository does not have missing objects other than the ones that
> the promisor will give us if we asked again.
>
> * But because the connectivity of these objects appear in the
> commit graph file, we fail to notice that these objects are
> missing, producing wrong results. If we disabled commit-graph
> while traversal (an earlier writing of it was perfectly OK), then
> "rev-list --missing" would have noticed and reported what the
> user wanted to know.
>
> In other words, the "optimization" you value is working to quickly
> produce a wrong result. Is it "significantly regress"ing if we
> disabled it to obtain the correct result?
It depends, in my opinion. If:
- Wrong results caused by the commit graph are only introduced with
this patch series due to the changed behaviour of `--missing`.
- We disable commit graphs proactively only because of the changed
behaviour of `--missing`.
Then yes, it does feel wrong to me to disable commit graphs and regress
performance for usecases that perviously worked both correct and fast.
> My assumption also has been that there is no point in running
> "rev-list --missing" if we know there is no repository corruption,
> and those who run "rev-list --missing" wants to know if the objects
> are really available, i.e. even if commit-graph that is out of sync
> with reality says it exists, if it is not in the object store, they
> would want to know that.
>
> If you can show me that it is not the case, then I may be pursuaded
> why producing a result that is out of sync with reality _quickly_,
> instead of taking time to produce a result that matches reality, is
> a worthy "optimization" to keep.
Note that I'm not saying that it's fine to return wrong results -- this
is of course a bug that needs to be addressed somehow. After all, things
working correctly should always trump things working fast. But until now
it felt more like we were going into the direction of disabling commit
graphs without checking whether there is an alternative solution that
allows us to get the best of both worlds, correctness and performance.
So what I'm looking for in this thread is a reason why we _can't_ have
that, or at least can't have it without unreasonable amounts of work. We
have helpers like `lookup_commit_in_graph()` that are designed to detect
stale commit graphs by double-checking whether a commit that has been
looked up via the commit graph actually exists in the repository. So I'm
wondering whether this could help us address the issue.
If there is a good reason why all of that is not possible then I'm happy
to carve in.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: -q option for git to suppress informational messages?
From: Michal Suchánek @ 2023-10-13 6:49 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
In-Reply-To: <CAJoAoZ=MrjjfH6Noganejey0bAaB=d+jH_rXAqbscPG8E0m3Pw@mail.gmail.com>
Hello,
On Thu, Oct 12, 2023 at 01:46:09PM -0700, Emily Shaffer wrote:
> On Thu, Oct 12, 2023 at 11:53 AM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > Hello,
> >
> > when using git in scripts I find that many git commands insist on
> > printing informational messages, and the only way to avoid this is to
> > sent the output to /dev/null.
>
> Michal, you might want to investigate further whether there is a
> plumbing-specific command that meets the needs you're concerned about
> instead. Those commands will not have additional human-facing output,
> and have a stronger guarantee around backwards-compatibility than the
> human-facing commands do.
>
> Commands which count as "plumbing" can be located in `git help git` in
> the full list of subcommands; if you're not sure what alternative to
> use, I think you can feel free to describe what you're trying to do
> here and get advice on which plumbing commands to use instead of
> porcelain ones.
I want to replicate the repository data between multiple machines.
Git is 'distributed' VCS but AFAICS it supports replicating repositories
very poorly.
In one direction there is git push which has convenient --mirror option
and also can be scripted to replicate only selected references. AFAICS
there is no lower level command that can be used. There are commands for
uploading data to the remote repository but only push updates
references, no -q option.
In the other direction there is git remote. Of course there is no
cross-reference from git push, cross-references are for losers that
don't know everything about the thing. There are lower level commands
that could be used instead I suppose. Commands for downloading data are
available as well as for updating local references, and for one fetch
does provide a -q option. Also unlike push git remote cannot operate on
commandline arguments alone, it requires configuration. Rewriting it
using lower level commands might bring the benefit of not requiring
that.
However, this whole suggestion somewhat sounds along the lines
- The shape of this chisel is not great, maybe it could be improved?
- The other side of the hammer has an edge, maybe you could do
without a chisel?
Thanks
Michal
^ permalink raw reply
* Re: [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Patrick Steinhardt @ 2023-10-13 8:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, git
In-Reply-To: <ZSjbYCXfSUtEIkAt@tanuki>
[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]
On Fri, Oct 13, 2023 at 07:53:36AM +0200, Patrick Steinhardt wrote:
> On Thu, Oct 12, 2023 at 09:17:09AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > My assumption also has been that there is no point in running
> > "rev-list --missing" if we know there is no repository corruption,
> > and those who run "rev-list --missing" wants to know if the objects
> > are really available, i.e. even if commit-graph that is out of sync
> > with reality says it exists, if it is not in the object store, they
> > would want to know that.
> >
> > If you can show me that it is not the case, then I may be pursuaded
> > why producing a result that is out of sync with reality _quickly_,
> > instead of taking time to produce a result that matches reality, is
> > a worthy "optimization" to keep.
>
> Note that I'm not saying that it's fine to return wrong results -- this
> is of course a bug that needs to be addressed somehow. After all, things
> working correctly should always trump things working fast. But until now
> it felt more like we were going into the direction of disabling commit
> graphs without checking whether there is an alternative solution that
> allows us to get the best of both worlds, correctness and performance.
>
> So what I'm looking for in this thread is a reason why we _can't_ have
> that, or at least can't have it without unreasonable amounts of work. We
> have helpers like `lookup_commit_in_graph()` that are designed to detect
> stale commit graphs by double-checking whether a commit that has been
> looked up via the commit graph actually exists in the repository. So I'm
> wondering whether this could help us address the issue.
>
> If there is a good reason why all of that is not possible then I'm happy
> to carve in.
I've had a quick look at this problem so that I can solidify my own
train of thought a bit. The issue is `repo_parse_commit_internal()`,
which calls `parse_commit_in_graph()` without verifying that the object
actually exists in the object database. It's the only callsite of that
function outside of "commit-graph.c", as all other external callers
would call `lookup_commit_in_graph()` which _does_ perform the object
existence check.
So I think that the proper way to address the regression would be a
patch similar to the following:
diff --git a/commit.c b/commit.c
index b3223478bc..109e9217e3 100644
--- a/commit.c
+++ b/commit.c
@@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,
return -1;
if (item->object.parsed)
return 0;
- if (use_commit_graph && parse_commit_in_graph(r, item))
+ if (use_commit_graph && parse_commit_in_graph(r, item)) {
+ if (!has_object(r, &item->object.oid, 0))
+ return quiet_on_missing ? -1 :
+ error(_("commit %s exists in commit-graph but not in the object database"),
+ oid_to_hex(&item->object.oid));
return 0;
+ }
if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
return quiet_on_missing ? -1 :
I wouldn't be surprised if there are other edge cases where this can
lead to buggy behaviour.
Also, this issue may not necessarily stem from repository corruption. It
could for example happen that commits are getting garbage collected
without the commit graph having been updated, whatever the reason may be
for this. In that case we would happily continue to return these commits
from the commit graph even though the underlying object has since been
deleted. The repository itself is not corrupt though, we merely look at
an out-of-date commit graph. And for what it's worth, I think that we
should always gracefully handle that case or otherwise the commit graph
becomes less useful overall.
I didn't dig much deeper yet. And while the above patch fixes some of
the test failures, it doesn't fix them all. If we agree that this is the
way to go then I'd be happy to turn this into a proper patch.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Sebastian Thiel @ 2023-10-13 9:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Josh Triplett, Kristoffer Haugsbakk
In-Reply-To: <xmqqttqvg4lw.fsf@gitster.g>
On 12 Oct 2023, at 18:58, Junio C Hamano wrote:
> I presume you picked '$' > exactly because of this reason?
Yes, and because I thought '$' seems a great fit to represent value.
> I do not think it will be the end of the world if we don't do so,
> but it would be really really nice if we at least explored a way (or
> two) to make a big enough hole in the syntax to not just add
> "precious", but leave room to later add other traits, without having
> to worry about breaking the backward compatibility again. A
> simplest and suboptimal way may be to declare that a path that
> begins with '$' now needs '\'-quoting (just like your proposal),
> reserve '$$' as the precious prefix, and '$' followed by any other
> byte reserved for future use, but there may be better ideas.
Even though I'd love to go with the unextensible option assuming it would last
another 15 years, I can see the appeal of making it extensible from the start.
In a world where '$' is a prefix, I'd also think that it's now possible to
specify exclusion using '$!path' for completeness, if '$$path' marks 'path'
precious.
But if there is now a prefix, I feel that it might as well be chosen so that it
is easier to remember and/or less likely to cause conflicts. I think it must
have been that reason for pathspecs to choose ':' as their prefix, and it seems
to be an equally good choice here.
This would give us the following, taking the Linux kernel as example:
.*
!this-file-is-hidden-and-tracked
:!new-syntax-for-negation-for-completeness
\!an-ignored-file-with-leading-!
\:an-ignored-file-with-leading-:-which-is-technically-breaking
:$.config
:x-invalid-as-:-needs-either-!-or-$-to-follow-it
Now ':$path' would make any path precious, which is `:$.config` in the example
above.
How does that 'feel'? Is the similarity to pathspecs without being pathspecs
an anti-feature maybe?
>> Thus, to make this work, projects that ship the `.gitignore` files would *have
>> to add patterns* that make certain files precious.
>
> Not really. They do not have to do anything if they are content
> with the current Git ecosystem. And users who have precious stuff
> can mark them in the.git/info/excludes no?
Yes, but only if they control all the ignore patterns in their global files. If
the repository decides to exclude a file they deem precious, now it won't be
precious anymore as their ':$make-this-precious' pattern is seen sequentially
after the pattern in the repository.
For instance, tooling-specific ignores are typically fully controlled by the
user, like '/.idea/', which could now easily be made precious with ':$/idea/'.
But as the Linux kernel repository ships with a '.gitignore' file that includes
the '.*' pattern, users won't be able to 'get ahead' of that pattern with their
':$.config' specification.
> The only case that is
> problematic is when the project says 'foo' is ignored and expendable
> but the user thinks otherwise. So to make this work, projects that
> ship the ".gitignore" files have to avoid adding patterns to ignore
> things that it may reasonably be expected for its users to mark
> precious.
Yes, I think my paragraph above is exactly that but with examples to practice
the new syntax-proposal.
>
>> Such opted-in projects would produce `.gitignore` files like these:
>>
>> .*
>> $.config
>
> I would understand if you ignored "*~" or "*.o", but why ignore ".*"?
I don't have an answer, the example is from the Linux Kernel repository was
added in 1e65174a33784 [1].
I am definitely getting excited about the progress the syntax is making :),
thanks for proposing it!
[ Reference ]
1. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1e65174a33784
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Phillip Wood @ 2023-10-13 10:06 UTC (permalink / raw)
To: Junio C Hamano, Sebastian Thiel; +Cc: git, Josh Triplett, Kristoffer Haugsbakk
In-Reply-To: <xmqqttqvg4lw.fsf@gitster.g>
On 12/10/2023 17:58, Junio C Hamano wrote:
> Sebastian Thiel <sebastian.thiel@icloud.com> writes:
Sebastian - thanks for raising this again, it would be really good to
get a solution for handling "ignored but not expendable" files
> I have been regretting that I did not make the quoting syntax not
> obviously extensible in f87f9497 (git-ls-files: --exclude mechanism
> updates., 2005-07-24), which technically was a breaking change (as a
> relative pathname that began with '!' were not special, but after
> the change, it became necessary to '\'-quote it). A relative
> pathname that begins with '$' would be now broken the same way, but
> hopefully the fallout would be minor. I presume you picked '$'
> exactly because of this reason?
>
> I do not think it will be the end of the world if we don't do so,
> but it would be really really nice if we at least explored a way (or
> two) to make a big enough hole in the syntax to not just add
> "precious", but leave room to later add other traits, without having
> to worry about breaking the backward compatibility again. A
> simplest and suboptimal way may be to declare that a path that
> begins with '$' now needs '\'-quoting (just like your proposal),
> reserve '$$' as the precious prefix, and '$' followed by any other
> byte reserved for future use, but there may be better ideas.
One thought I had was that we could abuse the comment syntax to annotate
paths something like
#(keep)
/my-precious-file
would prevent /my-precious-file from being deleted by git clean (and
hopefully unpack-trees()[1]). It means that older versions of git would
treat the file as ignored. If we ever want more than one annotation per
path we could separate them with commas
#(keep,something-else)
/my-file
Strictly speaking it is a backward incompatible change but I doubt there
are many people using comments like that. I also wondered about some
kind of suffix on the file
/my-precious-file #(keep)
but that means that older versions of git would not ignore the file.
Best Wishes
Phillip
[1] Of the cases listed in [2] it is "git checkout" and friends
overwriting ignored files that I worry about more. At least "git clean"
defaults to a dry-run and has in interactive mode to select what gets
deleted.
[2] https://lore.kernel.org/git/xmqqttqytnqb.fsf@gitster.g
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Oswald Buddenhagen @ 2023-10-13 11:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sebastian Thiel, git, Josh Triplett, Kristoffer Haugsbakk
In-Reply-To: <xmqqttqvg4lw.fsf@gitster.g>
On Thu, Oct 12, 2023 at 09:58:19AM -0700, Junio C Hamano wrote:
>I do not think it will be the end of the world if we don't do so,
>but it would be really really nice if we at least explored a way (or
>two) to make a big enough hole in the syntax to not just add
>"precious", but leave room to later add other traits, without having
>to worry about breaking the backward compatibility again.
>
that would invariably make the syntax more verbose, for dubious gain.
that the extension we're deliberating now (again) was coming (in some
form) was clear for quite a while, while i'm not aware of anything else
that would semantically fit gitignore (*). "other traits" sounds awfully
like scope creep, and would most likely fit gitattributes better.
anyway, such a hypothetical "breaking" change wouldn't have much impact,
because versioned files aren't affected by gitignore. and for the
misclassification to be actually harmful, the user would have to be
unable to notice or correct it.
(*) this got me thinking about things that would fit, and i came up with
a modification of the proposal: one might want to specify just *how*
precious a file is (which i guess would translate to how many times the
extra override option would have to be passed to git-clean). (**)
i guess a suitable syntax for that would be
2>.config
note that even though using the dollar sign to denote "precious" is kind
of intuitive, i'm not using it for two reasons: a) it's not "crazy"
enough to use it at not quite the beginning of a file name (note that
traditionally it isn't even special on windows), and b) the visual
separation of the prefix isn't as good as with the "arrow-like"
character.
(**) actually, one would probably want proper type tagging (e.g., config
files vs. autotools-generated files (which do not belong into a repo,
but do into a tar-ball)). that really does sound a lot like
gitattributes, only that the files aren't versioned.
regards
^ permalink raw reply
* [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
From: Patrick Steinhardt @ 2023-10-13 12:37 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano, Taylor Blau
In-Reply-To: <ZSkCGS3JPEQ71dOF@tanuki>
[-- Attachment #1: Type: text/plain, Size: 4246 bytes --]
Commit graphs can become stale and contain references to commits that do
not exist in the object database anymore. Theoretically, this can lead
to a scenario where we are able to successfully look up any such commit
via the commit graph even though such a lookup would fail if done via
the object database directly.
As the commit graph is mostly intended as a sort of cache to speed up
parsing of commits we do not want to have diverging behaviour in a
repository with and a repository without commit graphs, no matter
whether they are stale or not. As commits are otherwise immutable, the
only thing that we really need to care about is thus the presence or
absence of a commit.
To address potentially stale commit data that may exist in the graph,
our `lookup_commit_in_graph()` function will check for the commit's
existence in both the commit graph, but also in the object database. So
even if we were able to look up the commit's data in the graph, we would
still pretend as if the commit didn't exist if it is missing in the
object database.
We don't have the same safety net in `parse_commit_in_graph_one()`
though. This function is mostly used internally in "commit-graph.c"
itself to validate the commit graph, and this usage is fine. We do
expose its functionality via `parse_commit_in_graph()` though, which
gets called by `repo_parse_commit_internal()`, and that function is in
turn used in many places in our codebase.
For all I can see this function is never used to directly turn an object
ID into a commit object without additional safety checks before or after
this lookup. What it is being used for though is to walk history via the
parent chain of commits. So when commits in the parent chain of a graph
walk are missing it is possible that we wouldn't notice if that missing
commit was part of the commit graph. Thus, a query like `git rev-parse
HEAD~2` can succeed even if the intermittent commit is missing.
It's unclear whether there are additional ways in which such stale
commit graphs can lead to problems. In any case, it feels like this is a
bigger bug waiting to happen when we gain additional direct or indirect
callers of `repo_parse_commit_internal()`. So let's fix the inconsistent
behaviour by checking for object existence via the object database, as
well.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
commit.c | 7 ++++++-
t/t5318-commit-graph.sh | 23 +++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index b3223478bc..109e9217e3 100644
--- a/commit.c
+++ b/commit.c
@@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,
return -1;
if (item->object.parsed)
return 0;
- if (use_commit_graph && parse_commit_in_graph(r, item))
+ if (use_commit_graph && parse_commit_in_graph(r, item)) {
+ if (!has_object(r, &item->object.oid, 0))
+ return quiet_on_missing ? -1 :
+ error(_("commit %s exists in commit-graph but not in the object database"),
+ oid_to_hex(&item->object.oid));
return 0;
+ }
if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
return quiet_on_missing ? -1 :
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ba65f17dd9..25f8e9e2d3 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -821,4 +821,27 @@ test_expect_success 'overflow during generation version upgrade' '
)
'
+test_expect_success 'commit exists in commit-graph but not in object database' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+ git commit-graph write --reachable &&
+
+ # Corrupt the repository by deleting the intermittent commit
+ # object. Commands should notice that this object is absent and
+ # thus that the repository is corrupt even if the commit graph
+ # exists.
+ oid=$(git rev-parse B) &&
+ rm .git/objects/"$(test_oid_to_path "$oid")" &&
+
+ test_must_fail git rev-parse HEAD~2 2>error &&
+ grep "error: commit $oid exists in commit-graph but not in the object database" error
+ )
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: why does git set X in LESS env var?
From: Christoph Anton Mitterer @ 2023-10-13 13:45 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <6d673c1bdae41236e95e3a9fca853731@manjaro.org>
On Fri, 2023-10-13 at 06:43 +0200, Dragan Simic wrote:
> > *If* some changes were made to how git handles this, it might
> > perhaps
> > be worth to consider not to touch LESS at all, but only add the
> > required settings via command line arguments (i.e. -F -R ...).
>
> Actually, that would be wrong. If someone sets $LESS or $PAGER (or
> $GIT_PAGER, more specifically), it's up to the utility that invokes
> the
> pager internally not to override the user preferences configured
> through
> these environment variables. That's how everyone can customize the
> pager behavior.
Well, but if its clear that the output would otherwise be garbage (e.g.
because -R is missing).
In any case right now we have the situation that a user cannot just
easily set LESS in his environment, with a minimum set of options, and
git's use of less will continue flawlessly out of the box, as the -R
would be missing.
> > Or perhaps only remove options from it, if they're known to break
> > the
> > behaviour with git (like -+R might).
>
> Again, not the way the whole thing with pagination works. If someone
> sets their environment variables wrong, it's simply the way they want
> it, and it isn't anyone else's business to attempt fixing it
> automatically.
Well, I wouldn't agree with that.
LESS foremost a env var to configure less (surprise ^^).
If git (or anyone else) uses less internally, e.g. because they don't
want to implement their own pager, fine... but then they cannot just
blindly assume that LESS is set only for git's (or any other tool's
needs).
So I'd say the proper way is rather that any such tool makes sure, that
any options strictly required as set no matter what. Just as e.g. delta
does.
> Well, fragile or not, that's the way it works. It has its downsides
> for
> sure, but it's all about having each utility handle the environment
> carefully and document it in its man page(s), so the users can also
> carefully craft the values of their customized environment variables.
Sure, but from a user's view, the use of less (or anything else) within
git is conceptually completely opaque.
In less' manpage LESS isn't documented as "oh and you must make sure -R
is included or otherwise git will break"...
> $LESS can be seen as a global set of the common options for less(1),
o.O ... but, as I've described, one cannot really use it as that:
If I globally set e.g. LESS="F" because my desire is to make less
always exit as soon as the file fits on a screen, which I think is a
reasonable thing to do, git would no longer add "R" and output would
break.
> You don't have to define an alias, there's $GIT_PAGER for that
> purpose,
> as I already explained above.
Well, yes... and as I've said before, one could also solve it via
git_config... but the problem stays the same... as soon as someone
wants to use LESS as global less options just as you described it
yourself, git will no longer worker properly because of the missing -R.
And actually if one would use GIT_PAGER one would again defeat the
purpose of a allegedly global options LESS, because unless one does
something like GIT_PAGER="${LESS}R" it wouldn't see any changes made to
LESS.
> Moreover, the whole idea of the various utilities touching the $LESS
> variable internally is to provide sane defaults to the users that
> don't
> configure $LESS, $PAGER, etc. on their own.
Then I don't see what the big problem would be to just do it via a
command argument - if someone really has ever some reasons to remove --
RAW‐CONTROL‐CHARS from the command options when less is invoked via git
... then he could still go into git_config and set that manually.
But it would seem to me that the overall handling would be much more
what one expects, than when doing the same via LESS.
> I don't know what delta is and how it actually paginates its outputs,
> but it should follow the rules of the environment-based pager
> configuration that I described in detail above.
Well, AFAIU, it doesn't and for good reasons :-)
Anyway... I think all necessary things have been said and this thread
has grown far to large with only semi-related stuff... so thanks for
all the replies why git uses "-X".
Cheers,
Chris.
^ permalink raw reply
* Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Uwe Kleine-König @ 2023-10-13 14:14 UTC (permalink / raw)
To: git; +Cc: entwicklung, Michael Strawbridge, Luben Tuikov
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
Hello,
$ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
error: unable to extract a valid address from:
This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").
Bisection points at
a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
I didn't try to understand that change and fix the problem.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-13 15:00 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: git
In-Reply-To: <48ff9c2ac262cec32ab4681e8417413488278294.camel@scientia.org>
On 2023-10-13 15:45, Christoph Anton Mitterer wrote:
> On Fri, 2023-10-13 at 06:43 +0200, Dragan Simic wrote:
>> Actually, that would be wrong. If someone sets $LESS or $PAGER (or
>> $GIT_PAGER, more specifically), it's up to the utility that invokes
>> the pager internally not to override the user preferences configured
>> through these environment variables. That's how everyone can
>> customize
>> the pager behavior.
>
> Well, but if its clear that the output would otherwise be garbage (e.g.
> because -R is missing).
Well, it's the basic principle of "garbage in, garbage out". If there's
something wrong with the contents of the environment variables, that's
simply the way the user configured it, and it's only their job to get it
right.
> In any case right now we have the situation that a user cannot just
> easily set LESS in his environment, with a minimum set of options, and
> git's use of less will continue flawlessly out of the box, as the -R
> would be missing.
Let me repeat that it isn't the job of git or any other pager-enabled
utility to fix the user-defined environment. Otherwise, the user
actually wouldn't be able to make their choice freely.
>> Again, not the way the whole thing with pagination works. If someone
>> sets their environment variables wrong, it's simply the way they want
>> it, and it isn't anyone else's business to attempt fixing it
>> automatically.
>
> Well, I wouldn't agree with that.
> LESS foremost a env var to configure less (surprise ^^).
>
> If git (or anyone else) uses less internally, e.g. because they don't
> want to implement their own pager, fine... but then they cannot just
> blindly assume that LESS is set only for git's (or any other tool's
> needs).
You seem to be missing the presence of other enviroment variables,
namely $GIT_PAGER, which I already described in detail in my previous
reply. I'd appreciate if you'd read that description in detail, and
possibly test it a bit.
> So I'd say the proper way is rather that any such tool makes sure, that
> any options strictly required as set no matter what. Just as e.g. delta
> does.
Again, that's simply wrong and defeats the user's freedom of choice.
>> Well, fragile or not, that's the way it works. It has its downsides
>> for
>> sure, but it's all about having each utility handle the environment
>> carefully and document it in its man page(s), so the users can also
>> carefully craft the values of their customized environment variables.
>
> Sure, but from a user's view, the use of less (or anything else) within
> git is conceptually completely opaque.
Actually, it isn't, because there are $LESS, $PAGER and $GIT_PAGER
environment variables to customize the behavior.
> In less' manpage LESS isn't documented as "oh and you must make sure -R
> is included or otherwise git will break"...
Quite frankly, it would be silly to expect the less(1) man page to
mention something about git(1).
>> $LESS can be seen as a global set of the common options for less(1),
>
> o.O ... but, as I've described, one cannot really use it as that:
>
> If I globally set e.g. LESS="F" because my desire is to make less
> always exit as soon as the file fits on a screen, which I think is a
> reasonable thing to do, git would no longer add "R" and output would
> break.
Again, you seem not to understand well the distinction between the
global settings (i.e. $LESS and $PAGER) and the utility-specific
settings (e.g. $GIT_PAGER). Or you maybe simply refuse to understand
it, I don't know.
>> You don't have to define an alias, there's $GIT_PAGER for that
>> purpose, as I already explained above.
>
> Well, yes... and as I've said before, one could also solve it via
> git_config... but the problem stays the same... as soon as someone
> wants to use LESS as global less options just as you described it
> yourself, git will no longer worker properly because of the missing -R.
>
> And actually if one would use GIT_PAGER one would again defeat the
> purpose of a allegedly global options LESS, because unless one does
> something like GIT_PAGER="${LESS}R" it wouldn't see any changes made to
> LESS.
Let me clarify that the contents of $LESS is applied by less(1)
internally, so the final runtime configuration for less(1) is a sum of
the configurations made available through the $LESS and $PAGER (or
$GIT_PAGER) environment variables. It's a rather powerful approach, if
used properly.
>> Moreover, the whole idea of the various utilities touching the $LESS
>> variable internally is to provide sane defaults to the users that
>> don't
>> configure $LESS, $PAGER, etc. on their own.
>
> Then I don't see what the big problem would be to just do it via a
> command argument - if someone really has ever some reasons to remove --
> RAW‐CONTROL‐CHARS from the command options when less is invoked via git
> ... then he could still go into git_config and set that manually.
>
> But it would seem to me that the overall handling would be much more
> what one expects, than when doing the same via LESS.
Again, adding or modifying any command-line arguments by git itself
would defeat the purpose of the environment variables and prevent the
users from making their choice of the pagination configuration freely.
>> I don't know what delta is and how it actually paginates its outputs,
>> but it should follow the rules of the environment-based pager
>> configuration that I described in detail above.
>
> Well, AFAIU, it doesn't and for good reasons :-)
In that case, delta does it wrong, and I hope you understand why.
> Anyway... I think all necessary things have been said and this thread
> has grown far to large with only semi-related stuff... so thanks for
> all the replies why git uses "-X".
I hope all this was useful to you. It was useful to me. :)
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Kristoffer Haugsbakk @ 2023-10-13 15:07 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: entwicklung, Michael Strawbridge, Luben Tuikov, git
In-Reply-To: <20231013141437.ywrhw65xdapmev7d@pengutronix.de>
On Fri, Oct 13, 2023, at 16:14, Uwe Kleine-König wrote:
> $ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
> Use of uninitialized value $address in sprintf at
This looks the same problem as https://lore.kernel.org/git/ZQ1eGzqfyoeeTBUq@debian.me/
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v4 0/2] attr: add attr.tree config
From: John Cai @ 2023-10-13 15:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: John Cai via GitGitGadget, git, Jeff King, Jonathan Tan,
Eric Sunshine
In-Reply-To: <xmqqjzrskdzq.fsf@gitster.g>
Hi Junio,
On 11 Oct 2023, at 18:09, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06)
>> provided the ability to pass in a treeish as the attr source. When a
>> revision does not resolve to a valid tree is passed, Git will die. At
>> GitLab, we server repositories as bare repos and would like to always read
>> attributes from the default branch, so we'd like to pass in HEAD as the
>> treeish to read gitattributes from on every command. In this context we
>> would not want Git to die if HEAD is unborn, like in the case of empty
>> repositories.
>>
>> Instead of modifying the default behavior of --attr-source, create a new
>> config attr.tree with which an admin can configure a ref for all commands to
>> read gitattributes from. Also make the default tree to read from HEAD on
>> bare repositories.
>>
>> Changes since v2:
>>
>> * relax the restrictions around attr.tree so that if it does not resolve to
>> a valid treeish, ignore it.
>> * add a commit to default to HEAD in bare repositories
>>
>> Changes since v1:
>>
>> * Added a commit to add attr.tree config
>
> THis is v4 so there must be some changes since v3 that we are missing?
Oops I messed up the ordering of changes here. I'll fix in the (hopefully) final
re-roll
>
>> Range-diff vs v3:
>>
>> 1: cef206d47c7 ! 1: eaa27c47810 attr: read attributes from HEAD when bare repo
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>> +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
>> + test_when_finished rm -rf test bare_with_gitattribute &&
>> + git init test &&
>> -+ (
>> -+ cd test &&
>> -+ test_commit gitattributes .gitattributes "f/path test=val"
>> -+ ) &&
>> ++ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
>
> OK.
>
>> 2: dadb822da99 ! 2: 749d8a8082e attr: add attr.tree for setting the treeish to read attributes from
>> @@ Documentation/config.txt: other popular tools, and describe them in your documen
>>
>> ## Documentation/config/attr.txt (new) ##
>> @@
>> -+attr.tree:
>> -+ A <tree-ish> to read gitattributes from instead of the worktree. See
>> -+ linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,
>> -+ treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take
>> -+ precedence over attr.tree.
>> ++attr.tree::
>> ++ A reference to a tree in the repository from which to read attributes,
>> ++ instead of the `.gitattributes` file in the working tree. In a bare
>> ++ repository, this defaults to `HEAD:.gitattributes`. If the value does
>> ++ not resolve to a valid tree object, an empty tree is used instead.
>> ++ When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
>> ++ command line option are used, this configuration variable has no effect.
>
> OK.
>
>> -+ if (!default_attr_source_tree_object_name) {
>> ++ if (!default_attr_source_tree_object_name && git_attr_tree) {
>> + default_attr_source_tree_object_name = git_attr_tree;
>> + ignore_bad_attr_tree = 1;
>> + }
>
> Makes sense.
>
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>>
>> +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
>> +
>> ++test_expect_success '--attr-source is bad' '
>> ++ test_when_finished rm -rf empty &&
>> ++ git init empty &&
>> ++ (
>> ++ cd empty &&
>> ++ echo "$bad_attr_source_err" >expect_err &&
>> ++ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
>> ++ test_cmp expect_err err
>> ++ )
>> ++'
>
> OK. We fail when explicitly given a bad attr-source.
>
>> +test_expect_success 'attr.tree when HEAD is unborn' '
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> + cd empty &&
>> -+ echo $bad_attr_source_err >expect_err &&
>> + echo "f/path: test: unspecified" >expect &&
>> + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
>> + test_must_be_empty err &&
>
> But we silently ignore when given via a configuration variable.
>
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>> + git init empty &&
>> + (
>> + cd empty &&
>> -+ echo $bad_attr_source_err >expect_err &&
>> + echo "f/path: test: unspecified" >expect &&
>> + git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
>> + test_must_be_empty err &&
>
> Ditto. Is this any different from the above? Both points at an
> object that does not exist. If one were pointing at an object that
> does not exist (e.g., HEAD before the initial commit) and the other
> were pointing at an object that is not a tree-ish (e.g., a blob),
> then having two separate tests may make sense, but otherwise, I am
> not sure about the value proposition of the second test.
Yeah looking at it now, this test seems like it doesn't add much.
>
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
>> test_cmp expect actual
>> '
>>
>> -+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
>> ++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
>> + test_commit "val2" .gitattributes "f/path test=val2" &&
>> + git checkout attr-source &&
>> + echo "f/path: test: val1" >expect &&
>> -+ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
>> ++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
>> ++ check-attr test -- f/path >actual &&
>> + test_cmp expect actual &&
>> -+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
>> ++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
>> ++ check-attr test -- f/path >actual &&
>> + test_cmp expect actual
>> + )
>> +'
>
> Looking good.
>
> Thanks. Queued.
thanks!
John
^ permalink raw reply
* [PATCH 0/2] [Outreachy][PATCH v2] branch.c: adjust error messages to coding guidelines
From: Isoken June Ibizugbe via GitGitGadget @ 2023-10-13 15:33 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe
As per the CodingGuidelines document, it is recommended that a single-line
message provided to error messages such as die(), error() and warning(),
should start with a lowercase letter and should not end with a period.
Signed-off-by: Isoken June Ibizugbe isokenjune@gmail.com
Isoken June Ibizugbe (2):
branch.c: ammend error messages for die()
branch.c: adjust error messages to coding guidelines
builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 33 deletions(-)
base-commit: aab89be2eb6ca51eefeb8c8066f673f447058856
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1590%2FJunie06%2Famend-error-mesg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1590/Junie06/amend-error-mesg-v1
Pull-Request: https://github.com/git/git/pull/1590
--
gitgitgadget
^ permalink raw reply
* [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Isoken June Ibizugbe via GitGitGadget @ 2023-10-13 15:33 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe, Isoken June Ibizugbe
In-Reply-To: <pull.1590.git.git.1697211227.gitgitgadget@gmail.com>
From: Isoken June Ibizugbe <isokenjune@gmail.com>
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index a756543d644..e7ee9bd0f15 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
(head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
if (merged)
warning(_("deleting branch '%s' that has been merged to\n"
- " '%s', but not yet merged to HEAD."),
+ " '%s', but not yet merged to HEAD"),
name, reference_name);
else
warning(_("not deleting branch '%s' that is not yet merged to\n"
- " '%s', even though it is merged to HEAD."),
+ " '%s', even though it is merged to HEAD"),
name, reference_name);
}
free(reference_name_to_free);
@@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
{
struct commit *rev = lookup_commit_reference(the_repository, oid);
if (!force && !rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
+ error(_("couldn't look up commit object for '%s'"), refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
+ error(_("the branch '%s' is not fully merged.\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'"), branchname, branchname);
return -1;
}
return 0;
@@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "branch.%s", branchname);
if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
+ warning(_("update of config-file failed"));
strbuf_release(&buf);
}
@@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("Cannot delete branch '%s' "
+ error(_("cannot delete branch '%s' "
"used by worktree at '%s'"),
bname.buf, path);
ret = 1;
@@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
&oid, &flags);
if (!target) {
if (remote_branch) {
- error(_("remote-tracking branch '%s' not found."), bname.buf);
+ error(_("remote-tracking branch '%s' not found"), bname.buf);
} else {
char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
char *virtual_target = resolve_refdup(virtual_name,
@@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
"Did you forget --remote?"),
bname.buf);
else
- error(_("branch '%s' not found."), bname.buf);
+ error(_("branch '%s' not found"), bname.buf);
FREE_AND_NULL(virtual_target);
}
ret = 1;
@@ -630,17 +630,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (recovery) {
if (copy)
- warning(_("Created a copy of a misnamed branch '%s'"),
+ warning(_("created a copy of a misnamed branch '%s'"),
interpreted_oldname);
else
- warning(_("Renamed a misnamed branch '%s' away"),
+ warning(_("renamed a misnamed branch '%s' away"),
interpreted_oldname);
}
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated"), newname);
strbuf_release(&logmsg);
@@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
error((!argc || branch_checked_out(branch_ref.buf))
- ? _("No commit on branch '%s' yet.")
- : _("No branch named '%s'."),
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"),
branch_name);
else if (!edit_branch_description(branch_name))
ret = 0; /* happy */
@@ -893,7 +893,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
die(copy? _("cannot copy the current branch while not on any")
- : _("cannot rename the current branch while not on any."));
+ : _("cannot rename the current branch while not on any"));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
else if (argc == 2)
@@ -965,7 +965,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] branch.c: ammend error messages for die()
From: Isoken June Ibizugbe via GitGitGadget @ 2023-10-13 15:33 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe, Isoken June Ibizugbe
In-Reply-To: <pull.1590.git.git.1697211227.gitgitgadget@gmail.com>
From: Isoken June Ibizugbe <isokenjune@gmail.com>
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a7..a756543d644 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch %s is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch %s is being bisected at %s"),
target, wt->path);
}
}
@@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (ref_exists(oldref.buf))
recovery = 1;
else
- die(_("Invalid branch name: '%s'"), oldname);
+ die(_("invalid branch name: '%s'"), oldname);
}
for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
if (oldref_usage & IS_HEAD)
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -624,9 +624,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && !(oldref_usage & IS_ORPHAN) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
@@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated!"), newname);
strbuf_release(&logmsg);
strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldref);
strbuf_release(&newref);
strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
@@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -892,7 +892,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
+ die(copy? _("cannot copy the current branch while not on any")
: _("cannot rename the current branch while not on any."));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
@@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Isoken Ibizugbe @ 2023-10-13 15:40 UTC (permalink / raw)
Cc: git
In-Reply-To: <91e4ad3984147fcc277254a3f6836bf79f5c9550.1697211227.git.gitgitgadget@gmail.com>
On Fri, Oct 13, 2023 at 4:33 PM Isoken June Ibizugbe via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Isoken June Ibizugbe <isokenjune@gmail.com>
>
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
> builtin/branch.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index a756543d644..e7ee9bd0f15 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
> (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
> if (merged)
> warning(_("deleting branch '%s' that has been merged to\n"
> - " '%s', but not yet merged to HEAD."),
> + " '%s', but not yet merged to HEAD"),
> name, reference_name);
> else
> warning(_("not deleting branch '%s' that is not yet merged to\n"
> - " '%s', even though it is merged to HEAD."),
> + " '%s', even though it is merged to HEAD"),
> name, reference_name);
> }
> free(reference_name_to_free);
> @@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
> {
> struct commit *rev = lookup_commit_reference(the_repository, oid);
> if (!force && !rev) {
> - error(_("Couldn't look up commit object for '%s'"), refname);
> + error(_("couldn't look up commit object for '%s'"), refname);
> return -1;
> }
> if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
> - error(_("The branch '%s' is not fully merged.\n"
> + error(_("the branch '%s' is not fully merged.\n"
> "If you are sure you want to delete it, "
> - "run 'git branch -D %s'."), branchname, branchname);
> + "run 'git branch -D %s'"), branchname, branchname);
> return -1;
> }
> return 0;
> @@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
> struct strbuf buf = STRBUF_INIT;
> strbuf_addf(&buf, "branch.%s", branchname);
> if (git_config_rename_section(buf.buf, NULL) < 0)
> - warning(_("Update of config-file failed"));
> + warning(_("update of config-file failed"));
> strbuf_release(&buf);
> }
>
> @@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> if (kinds == FILTER_REFS_BRANCHES) {
> const char *path;
> if ((path = branch_checked_out(name))) {
> - error(_("Cannot delete branch '%s' "
> + error(_("cannot delete branch '%s' "
> "used by worktree at '%s'"),
> bname.buf, path);
> ret = 1;
> @@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> &oid, &flags);
> if (!target) {
> if (remote_branch) {
> - error(_("remote-tracking branch '%s' not found."), bname.buf);
> + error(_("remote-tracking branch '%s' not found"), bname.buf);
> } else {
> char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
> char *virtual_target = resolve_refdup(virtual_name,
> @@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> "Did you forget --remote?"),
> bname.buf);
> else
> - error(_("branch '%s' not found."), bname.buf);
> + error(_("branch '%s' not found"), bname.buf);
> FREE_AND_NULL(virtual_target);
> }
> ret = 1;
> @@ -630,17 +630,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
> if (recovery) {
> if (copy)
> - warning(_("Created a copy of a misnamed branch '%s'"),
> + warning(_("created a copy of a misnamed branch '%s'"),
> interpreted_oldname);
> else
> - warning(_("Renamed a misnamed branch '%s' away"),
> + warning(_("renamed a misnamed branch '%s' away"),
> interpreted_oldname);
> }
>
> if (!copy && (oldref_usage & IS_HEAD) &&
> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> logmsg.buf))
> - die(_("branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated"), newname);
>
> strbuf_release(&logmsg);
>
> @@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
> if (!ref_exists(branch_ref.buf))
> error((!argc || branch_checked_out(branch_ref.buf))
> - ? _("No commit on branch '%s' yet.")
> - : _("No branch named '%s'."),
> + ? _("no commit on branch '%s' yet")
> + : _("no branch named '%s'"),
> branch_name);
> else if (!edit_branch_description(branch_name))
> ret = 0; /* happy */
> @@ -893,7 +893,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> die(_("branch name required"));
> else if ((argc == 1) && filter.detached)
> die(copy? _("cannot copy the current branch while not on any")
> - : _("cannot rename the current branch while not on any."));
> + : _("cannot rename the current branch while not on any"));
> else if (argc == 1)
> copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> else if (argc == 2)
> @@ -965,7 +965,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
>
> if (track == BRANCH_TRACK_OVERRIDE)
> --
> gitgitgadget
I am sorry if I made any mistakes, It's my first time using
gitgitgadget. This commit was already sent before through git-send
email. The patch was intended to submit the revisions.
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Junio C Hamano @ 2023-10-13 16:39 UTC (permalink / raw)
To: Sebastian Thiel; +Cc: git, Josh Triplett, Kristoffer Haugsbakk
In-Reply-To: <9C4A2AFD-AAA2-4ABA-8A8B-2133FD870366@icloud.com>
Sebastian Thiel <sebastian.thiel@icloud.com> writes:
> But if there is now a prefix, I feel that it might as well be chosen so that it
> is easier to remember and/or less likely to cause conflicts.
Another criteria is that it is not very often used in real
pathnames, of course, and '!' and '$' are good ones.
Come to think of it, we might be able to retrofit '!' without too
much damage. Something like "!unignored" is now a deprecated but
still supported way to say "!!unignored", "!*precious" is new, and
"\!anything" is a pathname that begins with '!'.
> Yes, I think my paragraph above is exactly that but with examples to practice
> the new syntax-proposal.
OK.
^ permalink raw reply
* Re: [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Junio C Hamano @ 2023-10-13 17:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
In-Reply-To: <ZSkCGS3JPEQ71dOF@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> actually exists in the object database. It's the only callsite of that
> function outside of "commit-graph.c", as all other external callers
> would call `lookup_commit_in_graph()` which _does_ perform the object
> existence check.
>
> So I think that the proper way to address the regression would be a
> patch similar to the following:
>
> diff --git a/commit.c b/commit.c
> index b3223478bc..109e9217e3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,
> return -1;
> if (item->object.parsed)
> return 0;
> - if (use_commit_graph && parse_commit_in_graph(r, item))
> + if (use_commit_graph && parse_commit_in_graph(r, item)) {
> + if (!has_object(r, &item->object.oid, 0))
> + return quiet_on_missing ? -1 :
> + error(_("commit %s exists in commit-graph but not in the object database"),
> + oid_to_hex(&item->object.oid));
> return 0;
> + }
It is (overly) conservative, which I generally should find pleasing,
but as I said, for secondary information like commit-graph that is
derived from the primary source only to precompute for optimization,
our general attitude should be to trust it and let the optimization
kick in, unless the operation being performed primarily cares about
the case where the result from using and not using the secondary
source differs. An obvious example of such an operation is "fsck",
where we do care and want to notice when the underlying object graph
and what commit-graph records contradict with each other. And my
suggestion to disable commit-graph while running the "rev-list"
command with the "--missing" option is because that usage would fall
into the same category (please correct me if that is not the case) [*].
So for the purpose of "rev-list --missing", the above change does
take us closer to the answer we would get from the primary source,
but isn't it pessimising other users unnecessarily? Do we have a
rough idea (if we have a benchmark that would be even better) how
much this lack of has_object() check is contributing to the
performance benefit of using commit-graph? Majority of callers of
this code should not have to pay the additional overhead here, so
unless it is small enough, this would be pessimising the generic
code for everybody to please "rev-list --missing", which is why I am
worried about going in this direction.
^ permalink raw reply
* [PATCH v5 1/2] attr: read attributes from HEAD when bare repo
From: John Cai via GitGitGadget @ 2023-10-13 17:39 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai, John Cai
In-Reply-To: <pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
The motivation for 44451a2e5e (attr: teach "--attr-source=<tree>" global
option to "git" , 2023-05-06), was to make it possible to use
gitattributes with bare repositories.
To make it easier to read gitattributes in bare repositories however,
let's just make HEAD:.gitattributes the default. This is in line with
how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare
repositories, 2012-12-13).
Signed-off-by: John Cai <johncai86@gmail.com>
---
attr.c | 12 +++++++++++-
t/t0003-attributes.sh | 11 +++++++++++
t/t5001-archive-attr.sh | 2 +-
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 71c84fbcf86..bf2ea1626a6 100644
--- a/attr.c
+++ b/attr.c
@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate,
}
static const char *default_attr_source_tree_object_name;
+static int ignore_bad_attr_tree;
void set_git_attr_source(const char *tree_object_name)
{
@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name &&
+ startup_info->have_repository &&
+ is_bare_repository()) {
+ default_attr_source_tree_object_name = "HEAD";
+ ignore_bad_attr_tree = 1;
+ }
+
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
- if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))
+ if (repo_get_oid_treeish(the_repository,
+ default_attr_source_tree_object_name,
+ attr_source) && !ignore_bad_attr_tree)
die(_("bad --attr-source or GIT_ATTR_SOURCE"));
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..5665cdc079f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -342,6 +342,17 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+
+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
+ test_when_finished rm -rf test bare_with_gitattribute &&
+ git init test &&
+ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
+ git clone --bare test bare_with_gitattribute &&
+ echo "f/path: test: val" >expect &&
+ git -C bare_with_gitattribute check-attr test -- f/path >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 0ff47a239db..eaf959d8f63 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' '
'
test_expect_missing bare-worktree/ignored
-test_expect_exists bare-worktree/ignored-by-tree
+test_expect_missing bare-worktree/ignored-by-tree
test_expect_exists bare-worktree/ignored-by-worktree
test_expect_success 'export-subst' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 0/2] attr: add attr.tree config
From: John Cai via GitGitGadget @ 2023-10-13 17:39 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai
In-Reply-To: <pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com>
44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06)
provided the ability to pass in a treeish as the attr source. When a
revision does not resolve to a valid tree is passed, Git will die. At
GitLab, we server repositories as bare repos and would like to always read
attributes from the default branch, so we'd like to pass in HEAD as the
treeish to read gitattributes from on every command. In this context we
would not want Git to die if HEAD is unborn, like in the case of empty
repositories.
Instead of modifying the default behavior of --attr-source, create a new
config attr.tree with which an admin can configure a ref for all commands to
read gitattributes from. Also make the default tree to read from HEAD on
bare repositories.
Changes since v4:
* removed superfluous test
Changes since v3:
* clarified attr logic around ignoring errors if source set by attr.tree is
invalid
* refactored tests by using helpers
* modified test to check for precedence between --attr-source, attr.tree,
GIT_ATTR_SOURCE
Changes since v2:
* relax the restrictions around attr.tree so that if it does not resolve to
a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositories
* remove commit that adds attr.allowInvalidSource
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: read attributes from HEAD when bare repo
attr: add attr.tree for setting the treeish to read attributes from
Documentation/config.txt | 2 +
Documentation/config/attr.txt | 7 ++++
attr.c | 19 ++++++++-
attr.h | 2 +
config.c | 16 ++++++++
t/t0003-attributes.sh | 72 +++++++++++++++++++++++++++++++++++
t/t5001-archive-attr.sh | 2 +-
7 files changed, 118 insertions(+), 2 deletions(-)
create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v5
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v4:
1: eaa27c47810 = 1: eaa27c47810 attr: read attributes from HEAD when bare repo
2: 749d8a8082e ! 2: df4b3f53309 attr: add attr.tree for setting the treeish to read attributes from
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ )
+'
+
-+test_expect_success 'attr.tree points to non-existing ref' '
-+ test_when_finished rm -rf empty &&
-+ git init empty &&
-+ (
-+ cd empty &&
-+ echo "f/path: test: unspecified" >expect &&
-+ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
-+ test_must_be_empty err &&
-+ test_cmp expect actual
-+ )
-+'
-+
+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
--
gitgitgadget
^ permalink raw reply
* [PATCH v5 2/2] attr: add attr.tree for setting the treeish to read attributes from
From: John Cai via GitGitGadget @ 2023-10-13 17:39 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai, John Cai
In-Reply-To: <pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git",
2023-05-06) provided the ability to pass in a treeish as the attr
source. In the context of serving Git repositories as bare repos like we
do at GitLab however, it would be easier to point --attr-source to HEAD
for all commands by setting it once.
Add a new config attr.tree that allows this.
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/config.txt | 2 ++
Documentation/config/attr.txt | 7 ++++
attr.c | 7 ++++
attr.h | 2 ++
config.c | 16 +++++++++
t/t0003-attributes.sh | 61 +++++++++++++++++++++++++++++++++++
6 files changed, 95 insertions(+)
create mode 100644 Documentation/config/attr.txt
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.
include::config/advice.txt[]
+include::config/attr.txt[]
+
include::config/core.txt[]
include::config/add.txt[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..1a482d6af2b
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -0,0 +1,7 @@
+attr.tree::
+ A reference to a tree in the repository from which to read attributes,
+ instead of the `.gitattributes` file in the working tree. In a bare
+ repository, this defaults to `HEAD:.gitattributes`. If the value does
+ not resolve to a valid tree object, an empty tree is used instead.
+ When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
+ command line option are used, this configuration variable has no effect.
diff --git a/attr.c b/attr.c
index bf2ea1626a6..e62876dfd3e 100644
--- a/attr.c
+++ b/attr.c
@@ -24,6 +24,8 @@
#include "tree-walk.h"
#include "object-name.h"
+const char *git_attr_tree;
+
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
static const char git_attr__unknown[] = "(builtin)unknown";
@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name && git_attr_tree) {
+ default_attr_source_tree_object_name = git_attr_tree;
+ ignore_bad_attr_tree = 1;
+ }
+
if (!default_attr_source_tree_object_name &&
startup_info->have_repository &&
is_bare_repository()) {
diff --git a/attr.h b/attr.h
index 2b745df4054..127998ae013 100644
--- a/attr.h
+++ b/attr.h
@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);
/* Return whether the system gitattributes file is enabled and should be used. */
int git_attr_system_is_enabled(void);
+extern const char *git_attr_tree;
+
#endif /* ATTR_H */
diff --git a/config.c b/config.c
index 3846a37be97..fb6a2db1d9b 100644
--- a/config.c
+++ b/config.c
@@ -18,6 +18,7 @@
#include "repository.h"
#include "lockfile.h"
#include "mailmap.h"
+#include "attr.h"
#include "exec-cmd.h"
#include "strbuf.h"
#include "quote.h"
@@ -1904,6 +1905,18 @@ static int git_default_mailmap_config(const char *var, const char *value)
return 0;
}
+static int git_default_attr_config(const char *var, const char *value)
+{
+ if (!strcmp(var, "attr.tree"))
+ return git_config_string(&git_attr_tree, var, value);
+
+ /*
+ * Add other attribute related config variables here and to
+ * Documentation/config/attr.txt.
+ */
+ return 0;
+}
+
int git_default_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
@@ -1927,6 +1940,9 @@ int git_default_config(const char *var, const char *value,
if (starts_with(var, "mailmap."))
return git_default_mailmap_config(var, value);
+ if (starts_with(var, "attr."))
+ return git_default_attr_config(var, value);
+
if (starts_with(var, "advice.") || starts_with(var, "color.advice"))
return git_default_advice_config(var, value);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 5665cdc079f..ecf43ab5454 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -40,6 +40,10 @@ attr_check_source () {
test_cmp expect actual &&
test_must_be_empty err
+ git $git_opts -c "attr.tree=$source" check-attr test -- "$path" >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+
GIT_ATTR_SOURCE="$source" git $git_opts check-attr test -- "$path" >actual 2>err &&
test_cmp expect actual &&
test_must_be_empty err
@@ -342,6 +346,43 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
+test_expect_success '--attr-source is bad' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "$bad_attr_source_err" >expect_err &&
+ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
+ test_cmp expect_err err
+ )
+'
+
+test_expect_success 'attr.tree when HEAD is unborn' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "f/path test=val" >.gitattributes &&
+ echo "f/path: test: val" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+ )
+'
test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
test_when_finished rm -rf test bare_with_gitattribute &&
@@ -353,6 +394,26 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
test_cmp expect actual
'
+test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ git checkout -b attr-source &&
+ test_commit "val1" .gitattributes "f/path test=val1" &&
+ git checkout -b attr-tree &&
+ test_commit "val2" .gitattributes "f/path test=val2" &&
+ git checkout attr-source &&
+ echo "f/path: test: val1" >expect &&
+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
+ check-attr test -- f/path >actual &&
+ test_cmp expect actual &&
+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
+ check-attr test -- f/path >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
From: Junio C Hamano @ 2023-10-13 18:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Taylor Blau
In-Reply-To: <b0bf576c51a706367a758b8e30eca37edb9c2734.1697200576.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> @@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,
> return -1;
> if (item->object.parsed)
> return 0;
> - if (use_commit_graph && parse_commit_in_graph(r, item))
> + if (use_commit_graph && parse_commit_in_graph(r, item)) {
> + if (!has_object(r, &item->object.oid, 0))
> + return quiet_on_missing ? -1 :
> + error(_("commit %s exists in commit-graph but not in the object database"),
> + oid_to_hex(&item->object.oid));
> return 0;
> + }
Ever since this codepath was introduced by 177722b3 (commit:
integrate commit graph with commit parsing, 2018-04-10), we blindly
trusted what commit-graph file says. This change is a strict
improvement in the correctness department, but there are two things
that are a bit worrying.
One. The additional check should certainly be cheaper than a full
reading and parsing of an object, either from a loose object file or
from a pack entry. It may not hurt performance too much, but it
still would give us more confidence if we know by how much we are
pessimising good cases where the commit-graph does match reality.
Our stance on these secondary files that store precomputed values
for optimization purposes is in general to use them blindly unless
in exceptional cases where the operation values the correctness even
when the validity of these secondary files is dubious (e.g., "fsck"),
and doing this extra check regardless of the caller at this low level
of the callchain is a bit worrying.
Another is that by the time parse_commit_in_graph() returns true and
we realize that the answer we got is bogus by asking has_object(),
item->object.parsed has already been toggled on, so the caller now
has a commit object that claimed it was already parsed and does not
match reality. Hopefully the caller takes an early exit upon seeing
a failure from parse_commit_gently() and the .parsed bit does not
matter, but maybe I am missing a case where it does. I dunno.
Other than that, sounds very sensible and the code change is clean.
Will queue. Thanks.
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index ba65f17dd9..25f8e9e2d3 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -821,4 +821,27 @@ test_expect_success 'overflow during generation version upgrade' '
> )
> '
>
> +test_expect_success 'commit exists in commit-graph but not in object database' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + test_commit A &&
> + test_commit B &&
> + test_commit C &&
> + git commit-graph write --reachable &&
> +
> + # Corrupt the repository by deleting the intermittent commit
> + # object. Commands should notice that this object is absent and
> + # thus that the repository is corrupt even if the commit graph
> + # exists.
> + oid=$(git rev-parse B) &&
> + rm .git/objects/"$(test_oid_to_path "$oid")" &&
> +
> + test_must_fail git rev-parse HEAD~2 2>error &&
> + grep "error: commit $oid exists in commit-graph but not in the object database" error
> + )
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-13 18:25 UTC (permalink / raw)
To: Isoken June Ibizugbe via GitGitGadget, git; +Cc: Isoken June Ibizugbe
In-Reply-To: <91e4ad3984147fcc277254a3f6836bf79f5c9550.1697211227.git.gitgitgadget@gmail.com>
On 13-oct-2023 15:33:47, Isoken June Ibizugbe via GitGitGadget wrote:
> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> logmsg.buf))
> - die(_("branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated"), newname);
Thanks. This change is not explicitly suggested in the guidelines, but I think
it fits well in the spirit of this series.
> @@ -965,7 +965,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
You have mistakenly deleted this full stop in the previous [1/2] patch.
Therefore, if you stop doing so, you do not need to add it here.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox