* What's cooking in git.git (Sep 2023, #01; Fri, 1)
From: Junio C Hamano @ 2023-09-01 23:22 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).
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']
* ch/t6300-verify-commit-test-cleanup (2023-08-23) 2 commits
(merged to 'next' on 2023-08-24 at 15b842867e)
+ t/t6300: drop magic filtering
+ t/lib-gpg: forcibly run a trustdb update
Test clean-up.
source: <20230822150149.541ccb35@leda.eworm.net>
* ds/scalar-updates (2023-08-28) 3 commits
(merged to 'next' on 2023-08-28 at 093e6bcb9c)
+ scalar reconfigure: help users remove buggy repos
+ setup: add discover_git_directory_reason()
+ scalar: add --[no-]src option
Scalar updates.
source: <pull.1569.v3.git.1693230746.gitgitgadget@gmail.com>
* jc/ci-skip-same-commit (2023-08-25) 1 commit
(merged to 'next' on 2023-08-28 at e978717961)
+ ci: avoid building from the same commit in parallel
Tweak GitHub Actions CI so that pushing the same commit to multiple
branch tips at the same time will not waste building and testing
the same thing twice.
source: <1da763f3-60bf-a572-2c71-336b1fa5553d@gmx.de>
* jc/diff-exit-code-with-w-fixes (2023-08-21) 5 commits
(merged to 'next' on 2023-08-23 at 436a0aec3d)
+ diff: the -w option breaks --exit-code for --raw and other output modes
+ t4040: remove test that succeeded for a wrong reason
+ diff: teach "--stat -w --exit-code" to notice differences
+ diff: mode-only change should be noticed by "--patch -w --exit-code"
+ diff: move the fallback "--exit-code" code down
"git diff -w --exit-code" with various options did not work
correctly, which is being addressed.
source: <20230818235932.3253552-1-gitster@pobox.com>
* jc/mv-d-to-d-error-message-fix (2023-08-11) 1 commit
(merged to 'next' on 2023-08-21 at 2220d22d6f)
+ mv: fix error for moving directory to another
Typofix in an error message.
source: <xmqqjzu1njt0.fsf@gitster.g>
* jk/diff-result-code-cleanup (2023-08-21) 7 commits
(merged to 'next' on 2023-08-25 at 3b1280d42f)
+ diff: drop useless "status" parameter from diff_result_code()
+ diff: drop useless return values in git-diff helpers
+ diff: drop useless return from run_diff_{files,index} functions
+ diff: die when failing to read index in git-diff builtin
+ diff: show usage for unknown builtin_diff_files() options
+ diff-files: avoid negative exit value
+ diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()
"git diff --no-such-option" and other corner cases around the exit
status of the "diff" command has been corrected.
source: <20230821201358.GA2663749@coredump.intra.peff.net>
* ob/format-patch-description-file (2023-08-21) 1 commit
(merged to 'next' on 2023-08-25 at 89ea619311)
+ format-patch: add --description-file option
"git format-patch" learns a way to feed cover letter description,
that (1) can be used on detached HEAD where there is no branch
description available, and (2) also can override the branch
description if there is one.
source: <20230821170720.577820-1-oswald.buddenhagen@gmx.de>
* ob/sequencer-empty-hint-fix (2023-08-24) 1 commit
(merged to 'next' on 2023-08-24 at 626c52ad72)
+ sequencer: rectify empty hint in call of require_clean_work_tree()
The use of API between two calls to require_clean_work_tree() from
the sequencer code has been cleaned up for consistency.
source: <20230824150046.802008-1-oswald.buddenhagen@gmx.de>
* py/git-gui-updates (2023-08-24) 16 commits
(merged to 'next' on 2023-08-28 at df0b742344)
+ Merge https://github.com/prati0100/git-gui
+ Merge branch 'ml/cygwin-fixes'
+ git-gui - use mkshortcut on Cygwin
+ git-gui - use cygstart to browse on Cygwin
+ git-gui - remove obsolete Cygwin specific code
+ git gui Makefile - remove Cygwin modifications
+ Merge branch 'ab/makeflags'
+ Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
+ Merge branch 'js/windows-rce'
+ Work around Tcl's default `PATH` lookup
+ Move the `_which` function (almost) to the top
+ Move is_<platform> functions to the beginning
+ is_Cygwin: avoid `exec`ing anything
+ windows: ignore empty `PATH` elements
+ Merge branch 'vk/readme-typo'
+ git-gui: Fix a typo in README
Git GUI updates.
* sl/sparse-check-attr (2023-08-11) 3 commits
(merged to 'next' on 2023-08-21 at c202b15517)
+ check-attr: integrate with sparse-index
+ attr.c: read attributes in a sparse directory
+ t1092: add tests for 'git check-attr'
Teach "git check-attr" work better with sparse-index.
cf. <3b2a5b4b-ab8f-746b-6b69-8e8262b6390b@github.com>
source: <20230811142211.4547-1-cheskaqiqi@gmail.com>
* tb/commit-graph-verify-fix (2023-08-21) 4 commits
(merged to 'next' on 2023-08-23 at 2b4b74bb0d)
+ commit-graph: avoid repeated mixed generation number warnings
+ t/t5318-commit-graph.sh: test generation zero transitions during fsck
+ commit-graph: verify swapped zero/non-zero generation cases
+ commit-graph: introduce `commit_graph_generation_from_graph()`
The commit-graph verification code that detects mixture of zero and
non-zero generation numbers has been updated.
source: <cover.1692653671.git.me@ttaylorr.com>
* ts/unpacklimit-config-fix (2023-08-22) 1 commit
(merged to 'next' on 2023-08-23 at 4fabd9a697)
+ transfer.unpackLimit: fetch/receive.unpackLimit takes precedence
transfer.unpackLimit ought to be used as a fallback, but overrode
fetch.unpackLimit and receive.unpackLimit instead.
source: <xmqqpm3eh7f6.fsf@gitster.g>
--------------------------------------------------
[New Topics]
* dd/format-patch-rfc-updates (2023-08-31) 1 commit
(merged to 'next' on 2023-09-01 at ad87c89ee3)
+ format-patch: --rfc honors what --subject-prefix sets
"git format-patch --rfc --subject-prefix=<foo>" used to ignore the
"--subject-prefix" option and used "[RFC PATCH]"; now we will add
"RFC" prefix to whatever subject prefix is specified.
This is a backward compatible change that may deserve a note.
Will merge to 'master'.
source: <20230830064646.30904-1-sir@cmpwn.com>
* jk/test-lsan-denoise-output (2023-08-28) 1 commit
(merged to 'next' on 2023-08-29 at 9642d76264)
+ test-lib: ignore uninteresting LSan output
Tests with LSan from time to time seem to emit harmless message
that makes our tests unnecessarily flakey; we work it around by
filtering the uninteresting output.
Will merge to 'master'.
source: <20230828183735.GA3015072@coredump.intra.peff.net>
* jk/unused-post-2.42 (2023-08-29) 22 commits
(merged to 'next' on 2023-08-30 at ab0538e754)
+ update-ref: mark unused parameter in parser callbacks
+ gc: mark unused descriptors in scheduler callbacks
+ bundle-uri: mark unused parameters in callbacks
+ fetch: mark unused parameter in ref_transaction callback
+ credential: mark unused parameter in urlmatch callback
+ grep: mark unused parmaeters in pcre fallbacks
+ imap-send: mark unused parameters with NO_OPENSSL
+ worktree: mark unused parameters in noop repair callback
+ negotiator/noop: mark unused callback parameters
+ add-interactive: mark unused callback parameters
+ grep: mark unused parameter in output function
+ test-trace2: mark unused argv/argc parameters
+ trace2: mark unused config callback parameter
+ trace2: mark unused us_elapsed_absolute parameters
+ stash: mark unused parameter in diff callback
+ ls-tree: mark unused parameter in callback
+ commit-graph: mark unused data parameters in generation callbacks
+ worktree: mark unused parameters in each_ref_fn callback
+ pack-bitmap: mark unused parameters in show_object callback
+ ref-filter: mark unused parameters in parser callbacks
+ sequencer: mark repository argument as unused
+ sequencer: use repository parameter in short_commit_name()
Unused parameters to functions are marked as such, and/or removed,
in order to bring us closer to -Wunused-parameter clean.
Will merge to 'master'.
source: <20230829234305.GA226944@coredump.intra.peff.net>
* js/ci-san-skip-p4-and-svn-tests (2023-08-29) 1 commit
(merged to 'next' on 2023-08-29 at 9617f99668)
+ ci(linux-asan-ubsan): let's save some time
Flakey "git p4" tests, as well as "git svn" tests, are now skipped
in the (rather expensive) sanitizer CI job.
Will merge to 'master'.
source: <pull.1578.v2.git.1693342048633.gitgitgadget@gmail.com>
* tb/multi-cruft-pack (2023-08-29) 4 commits
(merged to 'next' on 2023-08-30 at 15f0b56ed0)
+ Documentation/gitformat-pack.txt: drop mixed version section
+ Documentation/gitformat-pack.txt: remove multi-cruft packs alternative
+ builtin/pack-objects.c: support `--max-pack-size` with `--cruft`
+ builtin/pack-objects.c: remove unnecessary strbuf_reset()
Use of --max-pack-size to allow multiple packfiles to be created is
now supported even when we are sending unreachable objects to cruft
packs.
Will merge to 'master'.
source: <cover.1693262936.git.me@ttaylorr.com>
* jk/ci-retire-allow-ref (2023-08-30) 2 commits
(merged to 'next' on 2023-08-31 at 5fe4861f16)
+ ci: deprecate ci/config/allow-ref script
+ ci: allow branch selection through "vars"
CI update.
Will merge to 'master'.
source: <20230830194919.GA1709446@coredump.intra.peff.net>
* ws/git-svn-retire-faketerm (2023-08-30) 1 commit
(merged to 'next' on 2023-08-31 at cb6e0d658d)
+ git-svn: drop FakeTerm hack
Code clean-up.
Will merge to 'master'.
source: <xmqqa5u888lz.fsf_-_@gitster.g>
* jk/unused-post-2.42-part2 (2023-08-31) 10 commits
- parse-options: mark unused parameters in noop callback
- interpret-trailers: mark unused "unset" parameters in option callbacks
- parse-options: add more BUG_ON() annotations
- merge: do not pass unused opt->value parameter
- parse-options: mark unused "opt" parameter in callbacks
- parse-options: prefer opt->value to globals in callbacks
- checkout-index: delay automatic setting of to_tempfile
- format-patch: use OPT_STRING_LIST for to/cc options
- merge: simplify parsing of "-n" option
- merge: make xopts a strvec
Unused parameters to functions are marked as such, and/or removed,
in order to bring us closer to -Wunused-parameter clean.
Will merge to 'next'?
source: <20230831211637.GA949188@coredump.intra.peff.net>
* jk/tree-name-and-depth-limit (2023-08-31) 10 commits
- lower core.maxTreeDepth default to 2048
- tree-diff: respect max_allowed_tree_depth
- list-objects: respect max_allowed_tree_depth
- read_tree(): respect max_allowed_tree_depth
- traverse_trees(): respect max_allowed_tree_depth
- add core.maxTreeDepth config
- fsck: detect very large tree pathnames
- tree-walk: rename "error" variable
- tree-walk: drop MAX_TRAVERSE_TREES macro
- tree-walk: reduce stack size for recursive functions
We now limit depth of the tree objects and maximum length of
pathnames recorded in tree objects.
source: <20230831061735.GA2702156@coredump.intra.peff.net>
* js/doc-unit-tests-with-cmake (2023-08-31) 4 commits
- 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 uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
source: <pull.1579.git.1693462532.gitgitgadget@gmail.com>
* ew/hash-with-openssl-evp (2023-08-31) 1 commit
- treewide: fix various bugs w/ OpenSSL 3+ EVP API
Fix-up new-ish code to support OpenSSL EVP API.
Will merge to 'next'?
source: <20230901020928.M610756@dcvr>
--------------------------------------------------
[Stalled]
* js/doc-unit-tests (2023-08-17) 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 js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Comments?
source: <cover.1692297001.git.steadmon@google.com>
* ak/pretty-decorate-more (2023-08-21) 8 commits
- decorate: use commit color for HEAD arrow
- pretty: add pointer and tag options to %(decorate)
- pretty: add %(decorate[:<options>]) format
- decorate: color each token separately
- decorate: avoid some unnecessary color overhead
- decorate: refactor format_decorations()
- pretty-formats: enclose options in angle brackets
- pretty-formats: define "literal formatting code"
"git log --format" has been taught the %(decorate) placeholder.
What's the status of this thing?
source: <20230820185009.20095-1-andy.koppe@gmail.com>
* cc/repack-sift-filtered-objects-to-separate-pack (2023-08-13) 8 commits
. gc: add `gc.repackFilterTo` config option
. repack: implement `--filter-to` for storing filtered out objects
. gc: add `gc.repackFilter` config option
. repack: add `--filter=<filter-spec>` option
. repack: refactor finding pack prefix
. repack: refactor finishing pack-objects command
. t/helper: add 'find-pack' test-tool
. pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
Kicked out of the 'seen', as it still seems to be failing tests.
cf. https://github.com/git/git/actions/runs/5850998716/job/15861158252#step:4:1822
source: <20230812000011.1227371-1-christian.couder@gmail.com>
* cc/git-replay (2023-06-03) 15 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: disallow revision specific options and pathspecs
- 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: don't simplify history
- 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
No reviews?
source: <20230602102533.876905-1-christian.couder@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>
--------------------------------------------------
[Cooking]
* rs/parse-options-help-text-is-optional (2023-08-28) 1 commit
(merged to 'next' on 2023-08-29 at 5466f7fcb2)
+ parse-options: allow omitting option help text
It may be tempting to leave the help text NULL for a command line
option that is either hidden or too obvious, but "git subcmd -h"
and "git subcmd --help-all" would have segfaulted if done so. Now
the help text is optional.
Will merge to 'master'.
source: <2b08dc43-621d-2170-c4a6-c2aac33a9a19@web.de>
* tb/path-filter-fix (2023-08-30) 15 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
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
- 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
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 more work?
cf. <20230830200218.GA5147@szeder.dev>
source: <cover.1693413637.git.jonathantanmy@google.com>
* js/config-parse (2023-08-23) 4 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 out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
Needs review.
source: <cover.1692827403.git.steadmon@google.com>
* tb/mark-more-tests-as-leak-free (2023-08-29) 3 commits
(merged to 'next' on 2023-08-29 at c1db288281)
+ leak tests: mark t5583-push-branches.sh as leak-free
+ leak tests: mark t3321-notes-stripspace.sh as leak-free
+ leak tests: mark a handful of tests as leak-free
Tests that are known to pass with LSan are now marked as such.
Will merge to 'master'.
source: <cover.1693263171.git.me@ttaylorr.com>
* jc/update-index-show-index-version (2023-08-18) 3 commits
- test-tool: retire "index-version"
- update-index: add --show-index-version
- update-index doc: v4 is OK with JGit and libgit2
"git update-index" learns "--show-index-version" to inspect
the index format version used by the on-disk index file.
Needs review.
source: <20230818233729.2766281-1-gitster@pobox.com>
* ob/revert-of-revert-is-reapply (2023-08-21) 2 commits
- git-revert.txt: add discussion
- sequencer: beautify subject of reverts of reverts
The default log message created by "git revert", when reverting a
commit that records a revert, has been tweaked.
Expecting a (hopefully small and final) reroll.
cf. <ZOZnNDd2pMX6M2Au@nand.local>
source: <20230809171531.2564807-1-oswald.buddenhagen@gmx.de>
* la/trailer-cleanups (2023-08-06) 5 commits
- trailer: rename *_DEFAULT enums to *_UNSPECIFIED
- trailer: teach find_patch_start about --no-divider
- trailer: split process_command_line_args into separate functions
- trailer: split process_input_file into separate pieces
- trailer: separate public from internal portion of trailer_iterator
Code clean-up.
Expecting a reroll.
cf. <owlyy1iifq0n.fsf@fine.c.googlers.com>
source: <pull.1563.git.1691211879.gitgitgadget@gmail.com>
* la/trailer-test-and-doc-updates (2023-08-10) 14 commits
- SQUASH???
- 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.
Expecting a reroll.
cf. <owlyh6p5fpi7.fsf@fine.c.googlers.com>
source: <pull.1564.v2.git.1691702283.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
(this branch uses jc/unresolve-removal.)
Code clean-up.
Not ready to be reviewed yet.
source: <20230731224409.4181277-1-gitster@pobox.com>
* pw/rebase-i-after-failure (2023-08-01) 7 commits
- rebase -i: fix adding failed command to the todo list
- rebase --continue: refuse to commit after failed command
- rebase: fix rewritten list for failed pick
- sequencer: factor out part of pick_commits()
- sequencer: use rebase_path_message()
- rebase -i: remove patch file after conflict resolution
- rebase -i: move unlink() calls
Various fixes to the behaviour of "rebase -i" when the command got
interrupted by conflicting changes.
Expecting a reroll.
The latter half of the series should be reviewed by those who have
more stake in the sequencer code than myself.
cf. <619e458b-218b-a790-dfb4-9200e201b513@gmail.com>
source: <pull.1492.v3.git.1690903412.gitgitgadget@gmail.com>
* jc/unresolve-removal (2023-07-31) 7 commits
- checkout: allow "checkout -m path" to unmerge removed paths
- checkout/restore: add basic tests for --merge
- checkout/restore: refuse unmerging paths unless checking out of the index
- update-index: remove stale fallback code for "--unresolve"
- update-index: use unmerge_index_entry() to support removal
- resolve-undo: allow resurrecting conflicted state that resolved to deletion
- update-index: do not read HEAD and MERGE_HEAD unconditionally
(this branch is used by jc/rerere-cleanup.)
"checkout --merge -- path" and "update-index --unresolve path" did
not resurrect conflicted state that was resolved to remove path,
but now they do.
Needs review.
source: <20230731224409.4181277-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]
* jt/path-filter-fix (2023-08-08) 8 commits
(merged to 'next' on 2023-08-09 at 59952b9ec9)
+ commit-graph: fix small leak with invalid changedPathsVersion
(merged to 'next' on 2023-08-03 at d99958c287)
+ 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
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.
Reverted out of 'next', superseded by the tb/path-filter-fix topic.
cf. <ZMqp6K2iXixWH/zT@nand.local>
source: <cover.1690912539.git.jonathantanmy@google.com>
source: <20230808192240.GA4091261@coredump.intra.peff.net>
* ab/tag-object-type-errors (2023-05-10) 4 commits
. tag: don't emit potentially incorrect "object is a X, not a Y"
. tag: don't misreport type of tagged objects in errors
. object tests: add test for unexpected objects in tags
. Merge branch 'jk/parse-object-type-mismatch' into ab/tag-object-type-errors
Hardening checks around mismatched object types when one of those
objects is a tag.
Stalled for too long.
source: <cover-v2-0.3-00000000000-20221230T011725Z-avarab@gmail.com>
* ob/send-email-interactive-failure (2023-08-29) 2 commits
. SQUASH???
. send-email: prompt-dependent exit codes
"git send-email" exits with non-zero status when end-user
interaction causes any prepared message not to be sent.
Breaks existing users without even an opt-out knob.
source: <20230821170720.577835-1-oswald.buddenhagen@gmx.de>
* ws/svn-with-new-readline (2023-08-11) 1 commit
. git-svn: avoid creating more than one than one Term::ReadLine object
Adjust to newer Term::ReadLine to prevent it from breaking
the interactive prompt code in git-svn.
Superseded by ws/git-svn-retire-faketerm
source: <20230810011831.1423208-1-wesleys@opperschaap.net>
^ permalink raw reply
* Re: [PATCH v5 0/8] pretty: add %(decorate[:<options>]) format
From: Andy Koppe @ 2023-09-01 21:33 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: glencbz, phillip.wood123
In-Reply-To: <xmqqcyz5cxy3.fsf@gitster.g>
On 29/08/2023 22:59, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
>
>> Apologies for sending another version so soon, but I realized that I
>> hadn't removed the use of a compound literal from the first commit where
>> I had added it, so it still appeared in the patches. The overall diff
>> for v5 is the same as for v4.
>
> Sorry, but I lost track. How does this latest round of the topic
> look to folks? Ready to go?
Unfortunately there haven't been any comments beyond v2, apart from your
request not to introduce compound literals, which I've addressed.
I haven't got any changes to this pending.
Regards,
Andy
^ permalink raw reply
* Re: [PATCH 07/15] commit-graph: new filter ver. that fixes murmur3
From: Jonathan Tan @ 2023-09-01 20:56 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Jonathan Tan, Taylor Blau, git, Junio C Hamano, Jeff King,
Derrick Stolee
In-Reply-To: <20230830200218.GA5147@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> I'd prefer to leave the test cases unchanged, and make the revision
> walking machinery look at Bloom filters even for root commits, because
> this is an annoying and recurring testing issue. I remember it
> annoyed me back then, when I wanted to reproduce a couple of bugs that
> I knew were there, but my initial test cases didn't fail because the
> Bloom filter of the root commit was ignored; Derrick overlooked this
> in b16a8277644, you overlooked it now, and none of the reviewers then
> and now caught it, either.
I agree that making the revwalk look at Bloom filters of root commits
is an urgent matter for the reasons you describe (it's something
easily missed that slows down future development and/or makes future
development error-prone), but so is moving to a correct murmur3
implementation, I think, and one shouldn't stop the other. There could
be an argument that because the revwalk doesn't look at root commit
Bloom filters, any development on a new Bloom filter hash version is
suspect, but I don't think that has to be completely true.
> > My original design (up to patch 7 in this patch set) defends against
> > this by taking the very first version detected and rejecting every
> > other version, and Taylor's subsequent design reads every version, but
> > annotates filters with its version. So I think we're covered.
>
> In the meantime I adapted the test cases from the above linked message
> to write commit-graph layers with different Bloom filter versions, and
> it does fail, because commits from the bottom commit-graph layer are
> omitted from the revision walk's output. And the test case doesn't
> even need a middle layer without modified path Bloom filters to "hide"
> the different version in the bottom layer. Merging the layers seems
> to work, though.
For what it's worth, your test case below (with test_expect_success
instead of test_expect_failure) passes with my original design. With the
full patch set, it does fail, but for what it's worth, I did spot this,
providing an incomplete solution [1] and then a complete one [2]. Your
test case passes if I also include the following:
diff --git a/bloom.c b/bloom.c
index ff131893cd..1bafd62a4e 100644
--- a/bloom.c
+++ b/bloom.c
@@ -344,6 +344,10 @@ struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c)
prepare_repo_settings(r);
hash_version = r->settings.commit_graph_changed_paths_version;
+ if (hash_version == -1) {
+ struct bloom_filter_settings *s = get_bloom_filter_settings(r);
+ hash_version = (s && s->hash_version == 2) ? 2 : 1;
+ }
if (!(hash_version == -1 || hash_version == filter->version))
return NULL; /* unusable filter */
[1] https://lore.kernel.org/git/20230824222051.2320003-1-jonathantanmy@google.com/
[2] https://lore.kernel.org/git/20230829220432.558674-1-jonathantanmy@google.com/
> Besides fixing this issue, I think that the interaction of different
> Bloom filter versions and split commit-graphs needs to be thoroughly
> covered with test cases and discussed in the commit messages before
> this series could be considered good for merging.
Regarding commit messages, I can see that in "commit-graph: new filter
ver. that fixes murmur3", I could add "(the first version read if there
are multiple versions of changed path filters in the repository)" after
"automatically determined from the version of the existing changed path
filters in the repository". Taylor's patches already inherently cover
multiple versions, I think, since Bloom filters are annotated with their
versions, individually.
Regarding tests, yes, we could add the one you provided.
If you (or anyone else) can spot anything else that should be added,
please let us know.
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Jeff King @ 2023-09-01 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <xmqq7cp9prux.fsf@gitster.g>
On Fri, Sep 01, 2023 at 01:21:10PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > But I think that is the wrong way to optimize it. We shouldn't be
> > storing any strings per-atom, but rather walking the parse tree to
> > produce a single output buffer. And the values should be cheap to fill
> > in, because we should parse the object as necessary up front. This is
> > more or less the way the pretty.c parser does it.
>
> I thought "as necessary" may be a bit tricky as populate_value()
> were taught to omit doing the whole get_object() thing when the
> values for used_atom[] are all computable without parsing the object
> at all, but it seems that over time the populate_value() callchain
> has degraded sufficiently to unconditionally call get_object() these
> days, so I agree that the arrangement does not have much optimization
> value, at least in the current code.
No, I think we still do that optimization. When parsing the format
string, the parser function for each atom sets fields in an object_info
struct to indicate what we're interested in. Then for each ref, we call
populate_value(). If that object_info doesn't need anything (we
byte-wise compare it to an empty dummy struct), then we return early,
before calling get_object().
And that optimization is very important to retain; it makes a format
like %(refname) an order of magnitude faster.
The optimization I was referring to is that if you have a format like:
%(contents:body) %(contents:body)
then we'll de-duplicate that to a single used_atom struct, and they'll
share the same v->s result string. That's much harder to do if you parse
into an abstract syntax tree, since the two occupy different parts of
the tree. But my contention is that it does not matter if you stop
allocating v->s in the first place, and just walk the tree to directly
output the result (either to a strbuf or directly to stdout).
-Peff
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Jeff King @ 2023-09-01 20:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <xmqqcyz1psnb.fsf@gitster.g>
On Fri, Sep 01, 2023 at 01:04:08PM -0700, Junio C Hamano wrote:
> In any case, it was very good that you noticed we do not sort
> numerically when sorting by size (I guess our sort by timestamp
> weren't affected only because we have been lucky?). Thanks for
> starting this topic.
I think the date code works as expected, and by design. It's just that
the code is a little confusing. :) Something like %(committerdate) gets
a used_atom with FIELD_TIME, and the "grab" function sets v->value to
the unix-epoch timestamp. And then the comparison function uses
v->value, since it's not FIELD_STR.
It's a little hard to follow the code that fills in v->value, but the
call stack is roughly grab_values() -> grab_person() -> grab_date().
-Peff
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Junio C Hamano @ 2023-09-01 20:21 UTC (permalink / raw)
To: Jeff King; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <20230901191639.GA1955435@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But I think that is the wrong way to optimize it. We shouldn't be
> storing any strings per-atom, but rather walking the parse tree to
> produce a single output buffer. And the values should be cheap to fill
> in, because we should parse the object as necessary up front. This is
> more or less the way the pretty.c parser does it.
I thought "as necessary" may be a bit tricky as populate_value()
were taught to omit doing the whole get_object() thing when the
values for used_atom[] are all computable without parsing the object
at all, but it seems that over time the populate_value() callchain
has degraded sufficiently to unconditionally call get_object() these
days, so I agree that the arrangement does not have much optimization
value, at least in the current code.
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Junio C Hamano @ 2023-09-01 20:04 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: Jeff King, git
In-Reply-To: <ZPI0e1XzZrDV2fJk@five231003>
Kousik Sanagavarapu <five231003@gmail.com> writes:
> What I also find weird is the fact that we assign a "cmp_type" to the
> whole atom. Like "contents" is FIELD_STR and "objectsize" is "FIELD_ULONG"
> in "valid_atom". This seems wrong because the options of the atoms should be
> the ones deciding the "cmp_type", no?
I do not quite get where your confusion comes from.
The use of valid_atom[] purely for catalogging things like
"contents", "refname", etc., before specialization, as opposed to
used_atom[] that lists the actual specialized form of the atoms used
in the format string. If you refer to "contents:body" and
"contents:size" in your format string, they become two entries in
used_atom[], both of which refer to the same atom_type obtained by
consulting the same entry in the valid_atom[] array.
The specialization between "contents:body" and "contents:size" must
be captured somewhere, and that happens by using two used_atom[]
entries. There will be one "struct atom_value" for each of these
placeholders, each of which refers to its own used_atom that knows
for which variant of "contents" it was created. Of course, these
two "struct atom_value" instances will have different content string
for the same ref (one stores the body part of the string, the other
stores the size of the contents).
> I wanted to leave the "cmp_type" field of the atom untouched because that
> would mess up this "global" setting of "contents" to be a "FIELD_STR" (or
> even "raw" for that matter).
We are not talking about futzing with valid_atom[] array.
Because the used_atom[] array is designed to be used to capture the
differences among "contents" vs "contents:body" vs "contents:size",
what types of entities the values that uses an entry in used_atom[]
array (i.e. an instance of "struct atom_value") should be decided
using the information stored there.
I agree that Peff's "the value for 'contents:size' we know is
numeric, so only store the numeric value in atom_value and let the
output logic handle that using cmp_type information" sound very
tempting. If we were to tackle it, however, I think it should be a
separate topic.
In any case, it was very good that you noticed we do not sort
numerically when sorting by size (I guess our sort by timestamp
weren't affected only because we have been lucky?). Thanks for
starting this topic.
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Jeff King @ 2023-09-01 19:16 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: Junio C Hamano, git
In-Reply-To: <ZPI0e1XzZrDV2fJk@five231003>
On Sat, Sep 02, 2023 at 12:29:07AM +0530, Kousik Sanagavarapu wrote:
> > > > I think they are covered implicitly by the "else" block of the
> > > > conditional that checks for FIELD_STR.
> > >
> > > Ah, OK. That needs to be future-proofed to force future developers
> > > who want to add different FIELD_FOO type to look at the comparison
> > > logic. If we want to do so, it should be done as a separate topic
> > > for cleaning-up the mess, not as part of this effort.
>
> What I also find weird is the fact that we assign a "cmp_type" to the
> whole atom. Like "contents" is FIELD_STR and "objectsize" is "FIELD_ULONG"
> in "valid_atom". This seems wrong because the options of the atoms should be
> the ones deciding the "cmp_type", no?
I think the data structure is a little confusing if you haven't worked
with it before. But basically each "atom" corresponds to a single "%()"
block in the format. So if you ran:
git for-each-ref --format="%(contents:size) %(contents:body)"
you'd have two atoms in the used_atom struct: one for the size and one
for the body.
IMHO the code would be a lot easier to work with if the atoms were
structured as a parse tree with child pointers (especially when you get
into things like "if" that have sub-expressions). I think one of the
reasons that used_atom is an array is to de-duplicate repeated mentions
(so if you formatted "%(foo) %(foo)" it would only have to store the
computed value once).
But I think that is the wrong way to optimize it. We shouldn't be
storing any strings per-atom, but rather walking the parse tree to
produce a single output buffer. And the values should be cheap to fill
in, because we should parse the object as necessary up front. This is
more or less the way the pretty.c parser does it.
But that is all quite a large tangent from what you're working on, and
would probably be a ground-up rewrite of the formatting code. You can
safely ignore my rant for the purposes of your patch. ;)
> I wanted to leave the "cmp_type" field of the atom untouched because that
> would mess up this "global" setting of "contents" to be a "FIELD_STR" (or
> even "raw" for that matter). Although that seems like a bad idea, after
> I've read Junio's and your comments.
Yeah, I agree that would be a problem if there were one global
"contents". But we are allocating a new atom struct on the fly for the
contents:size directive that we parse, and so on.
-Peff
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Kousik Sanagavarapu @ 2023-09-01 18:59 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20230901183206.GA1952051@coredump.intra.peff.net>
On Fri, Sep 01, 2023 at 02:32:06PM -0400, Jeff King wrote:
> On Fri, Sep 01, 2023 at 10:59:28AM -0700, Junio C Hamano wrote:
>
> > > Yeah, I had the same thought after reading the patch. Unfortunately the
> > > "type" is used only for comparison, not formatting. So you are still
> > > stuck setting both v->value and v->s in grab_sub_body_contents(). It
> > > feels like we could hoist that xstrfmt("%"PRIuMAX) to a higher level as
> > > a preparatory refactoring. But it's not that big a deal to work around
> > > it if that turns out to be hard.
> >
> > Setting of the .value member happens O(N) times for the number of
> > refs involved, which does not bother me. Do you mean "when we know
> > we are not sorting with size we should omit parsing the string into
> > the .value member"? If so, I think that would be nice to have.
>
> No, I wasn't worried about code efficiency, but rather programmer
> effort. IOW, I expected that the second hunk that I showed could look
> like this:
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 88b021dd1d..02b02d6813 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1886,7 +1886,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
> } else if (atom->u.contents.option == C_BODY_DEP)
> v->s = xmemdupz(bodypos, bodylen);
> else if (atom->u.contents.option == C_LENGTH)
> - v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
> + v->value = strlen(subpos);
> else if (atom->u.contents.option == C_BODY)
> v->s = xmemdupz(bodypos, nonsiglen);
> else if (atom->u.contents.option == C_SIG)
This looks very tempting, although too good to be true with the current
ref-filter I guess, as you explain below.
> rather than setting both "value" and "s", and that some higher level
> code would recognize "oh, this is FIELD_ULONG, so I'll format it rather
> than looking at v->s". But it seems that such code does not exist. :)
> All of the other spots that set v->value (e.g., objectsize), just set
> both.
This was also one of the reasons why I decided to set both v->value
and v->s, that is because "objectsize" was implemented in a similar
fashion. Although I left "cmp_type" field untouched for the reasons
below.
> > > I think they are covered implicitly by the "else" block of the
> > > conditional that checks for FIELD_STR.
> >
> > Ah, OK. That needs to be future-proofed to force future developers
> > who want to add different FIELD_FOO type to look at the comparison
> > logic. If we want to do so, it should be done as a separate topic
> > for cleaning-up the mess, not as part of this effort.
What I also find weird is the fact that we assign a "cmp_type" to the
whole atom. Like "contents" is FIELD_STR and "objectsize" is "FIELD_ULONG"
in "valid_atom". This seems wrong because the options of the atoms should be
the ones deciding the "cmp_type", no?
I wanted to leave the "cmp_type" field of the atom untouched because that
would mess up this "global" setting of "contents" to be a "FIELD_STR" (or
even "raw" for that matter). Although that seems like a bad idea, after
I've read Junio's and your comments.
Thanks
>
> Yes, agreed.
>
> -Peff
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Jeff King @ 2023-09-01 18:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <xmqqr0nhpyf3.fsf@gitster.g>
On Fri, Sep 01, 2023 at 10:59:28AM -0700, Junio C Hamano wrote:
> > Yeah, I had the same thought after reading the patch. Unfortunately the
> > "type" is used only for comparison, not formatting. So you are still
> > stuck setting both v->value and v->s in grab_sub_body_contents(). It
> > feels like we could hoist that xstrfmt("%"PRIuMAX) to a higher level as
> > a preparatory refactoring. But it's not that big a deal to work around
> > it if that turns out to be hard.
>
> Setting of the .value member happens O(N) times for the number of
> refs involved, which does not bother me. Do you mean "when we know
> we are not sorting with size we should omit parsing the string into
> the .value member"? If so, I think that would be nice to have.
No, I wasn't worried about code efficiency, but rather programmer
effort. IOW, I expected that the second hunk that I showed could look
like this:
diff --git a/ref-filter.c b/ref-filter.c
index 88b021dd1d..02b02d6813 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1886,7 +1886,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
} else if (atom->u.contents.option == C_BODY_DEP)
v->s = xmemdupz(bodypos, bodylen);
else if (atom->u.contents.option == C_LENGTH)
- v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
+ v->value = strlen(subpos);
else if (atom->u.contents.option == C_BODY)
v->s = xmemdupz(bodypos, nonsiglen);
else if (atom->u.contents.option == C_SIG)
rather than setting both "value" and "s", and that some higher level
code would recognize "oh, this is FIELD_ULONG, so I'll format it rather
than looking at v->s". But it seems that such code does not exist. :)
All of the other spots that set v->value (e.g., objectsize), just set
both.
The ref-filter code is a weird mix of almost-object-oriented bits, giant
switch statements, and assumptions about which fields are set when.
> > I think they are covered implicitly by the "else" block of the
> > conditional that checks for FIELD_STR.
>
> Ah, OK. That needs to be future-proofed to force future developers
> who want to add different FIELD_FOO type to look at the comparison
> logic. If we want to do so, it should be done as a separate topic
> for cleaning-up the mess, not as part of this effort.
Yes, agreed.
-Peff
^ permalink raw reply related
* [ANNOUNCE] Git Rev News edition 102
From: Christian Couder @ 2023-09-01 18:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen, Kaartic Sivaraam,
Taylor Blau, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Calvin Wan,
brian m. carlson, René Scharfe, nick, Jason Pyeron,
Theodore Ts'o, Štěpán Němec, lwn
Hi everyone,
The 101st edition of Git Rev News is now published:
https://git.github.io/rev_news/2023/08/31/edition-102/
Thanks a lot to Calvin Wan and Štěpán Němec who helped this month!
Enjoy,
Christian, Jakub, Markus and Kaartic.
PS: An issue for the next edition is already opened and contributions
are welcome:
https://github.com/git/git.github.io/issues/660
^ permalink raw reply
* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-09-01 18:10 UTC (permalink / raw)
To: Wesley; +Cc: phillip.wood, git
In-Reply-To: <a168fe69-f305-4280-b0e6-9406fbac796f@opperschaap.net>
Wesley <wesleys@opperschaap.net> writes:
> The quirk is this: --fork-point looks at the reflog and reflog is
> local. Meaning, having an remote upstream branch will make
> --fork-point a noop. Only where you have an upstream which is local
> and your reflog has seen dropped commits it does something.
Why do you lack reflog on your remote-tracking branches in the first
place?
The fork-point heuristics, as far as I understand it, was invented
exactly to protect you from your upstream repository rewinding and
rebuilding the branch you have been building on top of. The default
fetch refspec +refs/heads/*:refs/remotes/origin/* has the "force"
option "+" in front exactly because the fetching repository is
expected to keep the reflog for remote-tracking branches to help
recovering from such a rewind & rebuild.
Puzzled.
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Junio C Hamano @ 2023-09-01 17:59 UTC (permalink / raw)
To: Jeff King; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <20230901174540.GB1947546@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Sep 01, 2023 at 09:43:07AM -0700, Junio C Hamano wrote:
>
>> IOW, when you notice that you need to set, say, u.contents.option of
>> an atom to C_LENGTH, shouldn't you set cmp_type of the atom to
>> FIELD_ULONG, somewhere in contents_atom_parser() and friends, and
>> everything should naturally follow, no?
>
> Yeah, I had the same thought after reading the patch. Unfortunately the
> "type" is used only for comparison, not formatting. So you are still
> stuck setting both v->value and v->s in grab_sub_body_contents(). It
> feels like we could hoist that xstrfmt("%"PRIuMAX) to a higher level as
> a preparatory refactoring. But it's not that big a deal to work around
> it if that turns out to be hard.
Setting of the .value member happens O(N) times for the number of
refs involved, which does not bother me. Do you mean "when we know
we are not sorting with size we should omit parsing the string into
the .value member"? If so, I think that would be nice to have.
>> It seems that support for other cmp_types are incomplete in the
>> current code. There are FIELD_ULONG and FIELD_TIME defined, but
>> they do not appear to be used in any way, so the cmp_ref_sorting()
>> would need to be updated to make it actually pay attention to the
>> cmp_type and perform numeric comparison.
>
> I think they are covered implicitly by the "else" block of the
> conditional that checks for FIELD_STR.
Ah, OK. That needs to be future-proofed to force future developers
who want to add different FIELD_FOO type to look at the comparison
logic. If we want to do so, it should be done as a separate topic
for cleaning-up the mess, not as part of this effort.
> So just this is sufficient to make contents:size work:
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 88b021dd1d..02e3b6ba82 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -583,9 +583,10 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
> atom->u.contents.option = C_BARE;
> else if (!strcmp(arg, "body"))
> atom->u.contents.option = C_BODY;
> - else if (!strcmp(arg, "size"))
> + else if (!strcmp(arg, "size")) {
> + atom->type = FIELD_ULONG;
> atom->u.contents.option = C_LENGTH;
> - else if (!strcmp(arg, "signature"))
> + } else if (!strcmp(arg, "signature"))
> atom->u.contents.option = C_SIG;
> else if (!strcmp(arg, "subject"))
> atom->u.contents.option = C_SUB;
> @@ -1885,9 +1886,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
> v->s = strbuf_detach(&sb, NULL);
> } else if (atom->u.contents.option == C_BODY_DEP)
> v->s = xmemdupz(bodypos, bodylen);
> - else if (atom->u.contents.option == C_LENGTH)
> - v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
> - else if (atom->u.contents.option == C_BODY)
> + else if (atom->u.contents.option == C_LENGTH) {
> + v->value = strlen(subpos);
> + v->s = xstrfmt("%"PRIuMAX, v->value);
> + } else if (atom->u.contents.option == C_BODY)
> v->s = xmemdupz(bodypos, nonsiglen);
> else if (atom->u.contents.option == C_SIG)
> v->s = xmemdupz(sigpos, siglen);
Yup, exactly.
Thanks.
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Jeff King @ 2023-09-01 17:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kousik Sanagavarapu, git
In-Reply-To: <xmqqa5u5rgis.fsf@gitster.g>
On Fri, Sep 01, 2023 at 09:43:07AM -0700, Junio C Hamano wrote:
> IOW, when you notice that you need to set, say, u.contents.option of
> an atom to C_LENGTH, shouldn't you set cmp_type of the atom to
> FIELD_ULONG, somewhere in contents_atom_parser() and friends, and
> everything should naturally follow, no?
Yeah, I had the same thought after reading the patch. Unfortunately the
"type" is used only for comparison, not formatting. So you are still
stuck setting both v->value and v->s in grab_sub_body_contents(). It
feels like we could hoist that xstrfmt("%"PRIuMAX) to a higher level as
a preparatory refactoring. But it's not that big a deal to work around
it if that turns out to be hard.
> It seems that support for other cmp_types are incomplete in the
> current code. There are FIELD_ULONG and FIELD_TIME defined, but
> they do not appear to be used in any way, so the cmp_ref_sorting()
> would need to be updated to make it actually pay attention to the
> cmp_type and perform numeric comparison.
I think they are covered implicitly by the "else" block of the
conditional that checks for FIELD_STR.
So just this is sufficient to make contents:size work:
diff --git a/ref-filter.c b/ref-filter.c
index 88b021dd1d..02e3b6ba82 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -583,9 +583,10 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
atom->u.contents.option = C_BARE;
else if (!strcmp(arg, "body"))
atom->u.contents.option = C_BODY;
- else if (!strcmp(arg, "size"))
+ else if (!strcmp(arg, "size")) {
+ atom->type = FIELD_ULONG;
atom->u.contents.option = C_LENGTH;
- else if (!strcmp(arg, "signature"))
+ } else if (!strcmp(arg, "signature"))
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
@@ -1885,9 +1886,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
v->s = strbuf_detach(&sb, NULL);
} else if (atom->u.contents.option == C_BODY_DEP)
v->s = xmemdupz(bodypos, bodylen);
- else if (atom->u.contents.option == C_LENGTH)
- v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
- else if (atom->u.contents.option == C_BODY)
+ else if (atom->u.contents.option == C_LENGTH) {
+ v->value = strlen(subpos);
+ v->s = xstrfmt("%"PRIuMAX, v->value);
+ } else if (atom->u.contents.option == C_BODY)
v->s = xmemdupz(bodypos, nonsiglen);
else if (atom->u.contents.option == C_SIG)
v->s = xmemdupz(sigpos, siglen);
-Peff
^ permalink raw reply related
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Jeff King @ 2023-09-01 17:32 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <8624fc43-ab42-442b-a141-851fc35dd24f@gmail.com>
On Fri, Sep 01, 2023 at 02:24:59PM +0100, Phillip Wood wrote:
> On 30/08/2023 20:49, Jeff King wrote:
> > This is a more efficient way to do the same thing that
> > ci/config/allow-ref does (which didn't exist back then).
>
> I like the idea of a more efficient way to skip the ci for certain refs.
> I've got my allow-ref script set up to reject a bunch of refs and run the ci
> on everything else. It's not clear to me how to replicate that with the
> setup proposed here. Would it be possible to add a second variable that
> prevents the ci from being run if it contains ref being pushed?
Drat, I was hoping nobody was using it that way. :)
Yes, I think it would be possible to do something like:
if: |
(vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)) &&
!contains(vars.CI_BRANCHES_REJECT, github.ref_name)
It doesn't allow globbing, though. Do you need that?
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Wesley @ 2023-09-01 17:13 UTC (permalink / raw)
To: phillip.wood; +Cc: Junio C Hamano, git
In-Reply-To: <6127b570-5e9b-404f-9802-9135a1c9f31f@gmail.com>
Hello Phillip,
On 9/1/23 09:19, Phillip Wood wrote:
> Hi Wesley
>
> On 19/08/2023 21:34, Wesley Schwengle wrote:
>> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
>> 2021-09-16) was submitted there was a discussion on if the forkpoint
>> behaviour of `git rebase' was sane. In my experience this wasn't sane.
>> Git rebase doesn't work if you don't have an upstream branch configured
>> (or something that says `merge = refs/heads/master' in the git config).
>> The behaviour of `git rebase' was that if you supply an upstream on the
>> command line that it behaves as if `--no-forkpoint' was supplied and if
>> you don't supply an upstream, it behaves as if `--forkpoint' was
>> supplied. This can result in a loss of commits if you don't know that
>> and if you don't know about `git reflog' or have other copies of your
>> changes. This can be seen with the following reproduction path:
>>
>> mkdir reproduction
>> cd reproduction
>> git init .
>> echo "commit a" > file.txt
>> git add file.txt
>> git commit -m "First commit" file.txt
>> echo "commit b" >> file.txt
>> git commit -m "Second commit" file.txt
>>
>> git switch -c foo
>> echo "commit c" >> file.txt"
>> git commit -m "Third commit" file.txt
>> git branch --set-upstream-to=master
>>
>> git status
>> On branch foo
>> Your branch is ahead of 'master' by 1 commit.
>>
>> git switch master
>> git merge foo
>
> Here "git merge" fast-forwards I think, if instead it created a merge
> commit there would be no problem as the tip of branch "foo" would not
> end up in master's reflog.
If you do
git merge foo --no-ff
git reset --hard HEAD^
git switch foo
git rebase
You'll end up with just the commits that are in master. You'll lose all
commits from foo.
>> git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'
>
> For a reproduction recipe I think "git log --oneline" would suffice.
Will update, thanks.
>> [snip]
>
> Thanks for the detailed reproduction recipe, I think it would be helpful
> to summarize what's happening in the commit message, especially as it
> seems to depend on "git merge" fast-forwarding. Do you often merge a
> branch into it's upstream and then reset the upstream branch?
Tricky question. When I encountered this behavior I was working on an
epic/topic branch that I had locally. And I made a commit that I thought
should have been in another branch. I moved the commit to another branch
and than later on rebased it.
I didn't reply to Juno yet, but he refers to the discussion about
--fork-point and --root command line options. This discussion links to a
blogpost [*1*] where the same behavior is experienced.
The quirk is this: --fork-point looks at the reflog and reflog is local.
Meaning, having an remote upstream branch will make --fork-point a noop.
Only where you have an upstream which is local and your reflog has seen
dropped commits it does something. In all other cases (including
supplying the upstream) it behaves as if --no-fork-point was set. If you
do the same action in two different clones, you get a different result,
depending on what is in your reflog. I find this very tricky behavior
for a default. I've set it to false myself, to get a more consistent
behavior.
I usually have a remote upstream (gitlab/github) and work with that, so
the --fork-point behaviour isn't present because there is no reflog for
that, so it behaves as --no-fork-point.
Cheers,
Wesley
[1]: https://commaok.xyz/post/fork-point/
--
Wesley
Why not both?
^ permalink raw reply
* Re: [bug]fatal: fetch-pack: invalid index-pack output
From: Junio C Hamano @ 2023-09-01 16:53 UTC (permalink / raw)
To: Bagas Sanjaya; +Cc: mingli zhang, git, Jonathan Tan, brian m. carlson
In-Reply-To: <ZPHHuiFn3v06hhVe@debian.me>
Bagas Sanjaya <bagasdotme@gmail.com> writes:
> On Sun, Aug 14, 2022 at 05:09:13AM +0800, mingli zhang wrote:
>> ...
>> I use git 2.36.1 on MacOS Monterey 12.3.1
>> ...
>
> Related discussion is in [1] with the fix for OpenSSL users is in [2].
> Please test.
>
> [1]: https://lore.kernel.org/git/ZPCL11k38PXTkFga@debian.me/
> [2]: https://lore.kernel.org/git/20230901020928.M610756@dcvr/
I somehow think they are not related. The EVP thing appeared only
in Git 2.42 and the report says 2.36.1, which was released in May
2022 and predates it by more than a year, no?
^ permalink raw reply
* Re: [PATCH v4] format-patch: --rfc honors what --subject-prefix sets
From: Junio C Hamano @ 2023-09-01 16:48 UTC (permalink / raw)
To: Drew DeVault; +Cc: Jeff King, git
In-Reply-To: <CV7EK073OLB2.3Q4Y31O55ZY9P@taiga>
"Drew DeVault" <sir@cmpwn.com> writes:
> +1 to proposed changes
Thanks, let's merge that version to 'next' then.
^ permalink raw reply
* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-09-01 16:48 UTC (permalink / raw)
To: Phillip Wood; +Cc: Wesley Schwengle, git
In-Reply-To: <4ee8802b-0b54-4ed3-8ead-61e7d7628bce@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> It does seem strange, it looks like the variable was really added as a
> way to turn off the current default. If we do change the default to
> --no-fork-point when no upstream is given on the commandline then I
> think we should consider allowing "auto" for rebase.forkpoint with the
> some meaning as "true" and recommend that instead.
Perhaps. The current and existing users do not need to change
anything and 'true' should keep working fine, but given that we are
discouraging the use of fork-point heuristics, it is not clear if it
makes sense to entice new users with a new 'auto' synonym, so, I
dunno
Thanks.
.
^ permalink raw reply
* Re: [PATCH] ref-filter: sort numerically when ":size" is used
From: Junio C Hamano @ 2023-09-01 16:43 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git
In-Reply-To: <20230901142624.12063-1-five231003@gmail.com>
Kousik Sanagavarapu <five231003@gmail.com> writes:
> Atoms like "raw" and "contents" have a ":size" option which can be used
> to know the size of the data. Since these atoms have the cmp_type
> FIELD_STR, they are sorted alphabetically from 'a' to 'z' and '0' to
> '9'. Meaning, even when the ":size" option is used and what we
> ultimatlely have is numbers, we still sort alphabetically.
There are other cmp_types already defined, like ULONG and TIME. How
are they used and affect the comparison? Naively, :size sounds like
a good candidate to compare as ULONG, as it cannot be negative even
though 0 is a valid size.
I understand and agree with the motivation, but the implementation
looks puzzling.
> diff --git a/ref-filter.c b/ref-filter.c
> index 1bfaf20fbf..5d7bea5f23 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -932,7 +932,13 @@ struct atom_value {
> ssize_t s_size;
> int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
> struct strbuf *err);
> - uintmax_t value; /* used for sorting when not FIELD_STR */
> +
> + /*
> + * Used for sorting when not FIELD_STR or when FIELD_STR but the
> + * sort should be numeric and not alphabetic.
> + */
> + uintmax_t value;
This does not explain why we cannot make <anything>:size FIELD_ULONG
for <anything> that is of FIELD_STR type, though. IOW, why such a
strange "when FIELD_STR but the sort should be numeric" is needed?
If you have a <size>, shouldn't it always be numeric?
IOW, when you notice that you need to set, say, u.contents.option of
an atom to C_LENGTH, shouldn't you set cmp_type of the atom to
FIELD_ULONG, somewhere in contents_atom_parser() and friends, and
everything should naturally follow, no?
It seems that support for other cmp_types are incomplete in the
current code. There are FIELD_ULONG and FIELD_TIME defined, but
they do not appear to be used in any way, so the cmp_ref_sorting()
would need to be updated to make it actually pay attention to the
cmp_type and perform numeric comparison.
> @@ -1883,8 +1890,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
> v->s = strbuf_detach(&sb, NULL);
> } else if (atom->u.contents.option == C_BODY_DEP)
> v->s = xmemdupz(bodypos, bodylen);
> - else if (atom->u.contents.option == C_LENGTH)
> - v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
> + else if (atom->u.contents.option == C_LENGTH) {
> + v->value = (uintmax_t)strlen(subpos);
> + v->s = xstrfmt("%"PRIuMAX, v->value);
> + }
We should take a note that all of these v->value are *per* *item*
that will be sorted.
> @@ -2986,7 +2996,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
> cmp_detached_head = 1;
> } else if (s->sort_flags & REF_SORTING_VERSION) {
> cmp = versioncmp(va->s, vb->s);
> - } else if (cmp_type == FIELD_STR) {
> + } else if (cmp_type == FIELD_STR && !va->value && !vb->value) {
Two refs may point at an empty object with zero length, i.e. for
them, !v->value is true, and another ref may point at a non-empty
object. The two empty refs are compared with an algorithm different
from the algorithm used to compare the empty ref and the non-empty
ref. Isn't this broken as a comparison function to be given to
QSORT(), which must be transitive (e.g. if A < B and B < C, then it
should be guaranteed that A < C and you do not have to compare A and
C)?
IOW, the choice of the comparison algorithm should not depend on an
attribute (like value or s) that is specific to the item being
compared. Things like cmp_type that is defined at the used_atom
lefvel to make the sorting function stable, I would think.
^ permalink raw reply
* Re: Is there a way to get the "format-patch" formatted file name?
From: Vít Ondruch @ 2023-09-01 16:37 UTC (permalink / raw)
To: Junio C Hamano, Oswald Buddenhagen; +Cc: git
In-Reply-To: <xmqqwmx9ritx.fsf@gitster.g>
[-- Attachment #1.1: Type: text/plain, Size: 1731 bytes --]
Dne 01. 09. 23 v 17:53 Junio C Hamano napsal(a):
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> but i wouldn't be opposed to for example git-mailinfo learning to
>> understand the --pretty argument, if the implementation doesn't turn
>> out to be completely out of proportion.
> Excellent suggestion. I agree that 'mailinfo' would be the closest
> place we have for such a new feature. It's "info" output (i.e. what
> comes out to the standard output of the command) is designed to be
> extensible, and I vaguely recall that we indeed have added new
> field(s) during its lifetime with existing users already.
>
> We can just invent a new label (e.g. "Filesystem-safe-subject:"),
> pass the subject string to pretty.c:format_sanitized_subject() and
> emit the result next to the existing "Subject:" with that label, and
> we can even do so unconditionally without breaking anybody.
'mailinfo' is new to me. I am not really sure what is the proper use of
this tool, but it sound as a neat idea. I could certainly extract the
string from the 'mailinfo' output and it works just above the .patch
file, which is great.
>
>
> Having it in 'mailinfo' may still not be a good solution to the
> issue, given that Vit says
>
>>> But I typically don't have a Git.
> though.
Well, this should have been "I typically don't have a Git *repository*",
because I certainly have a Git. Otherwise I would not ask for this
functionality.
Just to explain, I would need to expand the sources, which I typically
don't do. I'd need initialize the repository and commit everything,
apply the patch, etc. In that case, I'd rater keep manually changing the
patch names :)
Vít
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply
* [PATCH v2 3/3] revision: comment `--no-standard-notes` as deprecated
From: Kristoffer Haugsbakk @ 2023-09-01 16:19 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1693584310.git.code@khaugsbakk.name>
`--standard-notes` used to be deprecated but is now (since 6a4fe606cf[1])
used internally. Its negation `--no-standard-notes`, however, is still
deprecated even for internal use.
Mark this option as such.
† 1: range-diff: treat notes like `log`, 2023-05-19
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
• Move comment: https://lore.kernel.org/git/xmqqbkhk724x.fsf@gitster.g/
• Tweak commit message so that it's clearer why we are only commenting
“no” as deprecated and not the other option as well
revision.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/revision.c b/revision.c
index 64aebc014b..44a04004a7 100644
--- a/revision.c
+++ b/revision.c
@@ -2501,6 +2501,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
&revs->show_notes);
revs->notes_opt.use_default_notes = -1;
} else if (!strcmp(arg, "--no-standard-notes")) {
+ /* Deprecated */
revs->notes_opt.use_default_notes = 0;
} else if (!strcmp(arg, "--oneline")) {
revs->verbose_header = 1;
--
2.41.0
^ permalink raw reply related
* [PATCH v2 2/3] doc: pretty-options: remove documentation for deprecated options
From: Kristoffer Haugsbakk @ 2023-09-01 16:19 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1693584310.git.code@khaugsbakk.name>
Remove documentation for options which were deprecated in
ab18b2c0df (log/pretty-options: Document --[no-]notes and deprecate old
notes options, 2011-03-30).
We now use `--standard-notes` internally for `range-diff` (see previous
commit).
Leave `--show-notes` as-is since we aren’t changing anything about it.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/pretty-options.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index dc685be363..8c982609c9 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -88,7 +88,6 @@ being displayed. Examples: "--notes=foo" will show only notes from
from "refs/notes/bar".
--show-notes[=<ref>]::
---[no-]standard-notes::
These options are deprecated. Use the above --notes/--no-notes
options instead.
endif::git-rev-list[]
--
2.41.0
^ permalink raw reply related
* [PATCH v2 1/3] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-01 16:19 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1693584310.git.code@khaugsbakk.name>
Currently, `range-diff` shows the default notes if no notes-related
arguments are given. This is also how `log` behaves. But unlike
`range-diff`, `log` does *not* show the default notes if
`--notes=<custom>` are given. In other words, this:
git log --notes=custom
is equivalent to this:
git log --no-notes --notes=custom
While:
git range-diff --notes=custom
acts like this:
git log --notes --notes-custom
This can’t be how the user expects `range-diff` to behave given that the
man page for `range diff` under `--[no-]notes[=<ref>]` says:
> This flag is passed to the git log program (see git-log(1)) that
> generates the patches.
This behavior also affects `format-patch` since it uses `range-diff` for
the cover letter. Unlike `log`, though, `format-patch` is not supposed
to show the default notes if no notes-related arguments are given.[1]
But this promise is broken when the range-diff happens to have something
to say about the changes to the default notes, since that will be shown
in the cover letter.
Remedy this by co-opting the `--standard-notes` option which has been
deprecated since ab18b2c0df[2] and which is currently only documented in
`pretty-options`.
† 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about
showing notes, 2010-01-20).
† 2: log/pretty-options: Document --[no-]notes and deprecate old notes
options, 2011-03-30
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
range-diff.c | 2 +-
revision.c | 7 +++++--
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
3 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index 2e86063491..f070e4a4ce 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--notes",
+ "--standard-notes",
NULL);
strvec_push(&cp.args, range);
if (other_arg)
diff --git a/revision.c b/revision.c
index 2f4c53ea20..64aebc014b 100644
--- a/revision.c
+++ b/revision.c
@@ -2495,8 +2495,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
disable_display_notes(&revs->notes_opt, &revs->show_notes);
revs->show_notes_given = 1;
} else if (!strcmp(arg, "--standard-notes")) {
- revs->show_notes_given = 1;
- revs->notes_opt.use_default_notes = 1;
+ disable_display_notes(&revs->notes_opt, &revs->show_notes);
+ revs->show_notes_given = 0;
+ enable_default_display_notes(&revs->notes_opt,
+ &revs->show_notes);
+ revs->notes_opt.use_default_notes = -1;
} else if (!strcmp(arg, "--no-standard-notes")) {
revs->notes_opt.use_default_notes = 0;
} else if (!strcmp(arg, "--oneline")) {
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index b5f4d6a653..b33afa1c6a 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -662,6 +662,20 @@ test_expect_success 'range-diff with multiple --notes' '
test_cmp expect actual
'
+# `range-diff` should act like `log` with regards to notes
+test_expect_success 'range-diff with --notes=custom does not show default notes' '
+ git notes add -m "topic note" topic &&
+ git notes add -m "unmodified note" unmodified &&
+ git notes --ref=custom add -m "topic note" topic &&
+ git notes --ref=custom add -m "unmodified note" unmodified &&
+ test_when_finished git notes remove topic unmodified &&
+ test_when_finished git notes --ref=custom remove topic unmodified &&
+ git range-diff --notes=custom main..topic main..unmodified \
+ >actual &&
+ ! grep "## Notes ##" actual &&
+ grep "## Notes (custom) ##" actual
+'
+
test_expect_success 'format-patch --range-diff does not compare notes by default' '
git notes add -m "topic note" topic &&
git notes add -m "unmodified note" unmodified &&
@@ -679,6 +693,20 @@ test_expect_success 'format-patch --range-diff does not compare notes by default
! grep "note" 0000-*
'
+test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '
+ git notes add -m "topic note" topic &&
+ git notes --ref=custom add -m "topic note (custom)" topic &&
+ git notes add -m "unmodified note" unmodified &&
+ git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
+ test_when_finished git notes remove topic unmodified &&
+ test_when_finished git notes --ref=custom remove topic unmodified &&
+ git format-patch --notes=custom --cover-letter --range-diff=$prev \
+ main..unmodified >actual &&
+ test_when_finished "rm 000?-*" &&
+ grep "## Notes (custom) ##" 0000-* &&
+ ! grep "## Notes ##" 0000-*
+'
+
test_expect_success 'format-patch --range-diff with --no-notes' '
git notes add -m "topic note" topic &&
git notes add -m "unmodified note" unmodified &&
--
2.41.0
^ permalink raw reply related
* [PATCH v2 0/3] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-01 16:18 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1686505920.git.code@khaugsbakk.name>
The following cover letter is identical to the previous one up to but not
including section “Changes since version 1”.
Cheers
🙛 🙙
Currently, `range-diff` shows the default notes if no notes-related
arguments are given. This is also how `log` behaves. But unlike
`range-diff`, `log` does *not* show the default notes if
`--notes=<custom>` are given.
These changes are supposed to make `format-range` behave like `log` with
regards to notes.
These changes also fixes an issue with notes being shared in the cover
letter via `range-diff`, and that’s really the main motivation for
making these changes.
§ How `log` works
`log` shows the default notes if no notes arguments are given. But if
you give it specific notes to show then it forgets about the default
notes. Further, there is the convenience `--notes` option which will
show the default notes again. These options are cumulative. For example:
git log --notes --notes=custom
Will show the default notes as well as the `custom` notes.
See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/
§ How `range-format` works
`range-format` passes `--notes` to `log`, which means that it does not
have the default behavior of `log` (forget the default logs if you say
e.g. `--notes=custom`). However, the man page says that (under
`--[no-]notes[=<ref>]`):
> This flag is passed to the git log program (see git-log(1)) that generates the patches.
This makes me (at least) think that `range-format` is supposed to work
just like `log` with regards to notes.
§ `format-patch` and the difference between showing and sharing
`format-patch` has a different default: it shows no notes. This makes
sense in my opinion since `format-patch` is meant to be used to share
changes with others, and you might be surprised if your notes (which
might have only been notes to yourself) are sent out in your emails
(keep in mind that notes refs are *not* pushed by default).
But the slightly faulty behavior of `range-diff` bleeds through to
`format-patch` since the latter calls the former; if you have default
notes they can be shared in the range-diff on the cover letter, even
though `format-patch` isn’t supposed to show them.
§ Code layout and approach to the problem
As I’ve said, my focus was on fixing `format-patch`, so I’ve considered
how `format-patch` calls `range-diff` which in turn calls `log`.
`format-patch` is a command which is defined in `builtin/log.c`. For
notes in particular it in fact has some explicit logic for handling
notes based on the value of `rev`. (There seems to be no issues with
this part of the code; only the code in `range-diff.c` which passes the
default argument to `log`.) It then calls
`range-diff.c:show_range_diff`. That function on `master` loads some
default arguments, among them `--notes`. It then eventually calls `log`
as a subprocess.
My change consists of co-opting the deprecated `--standard-notes` and
changing its behavior so that it can be used in
`range-diff.c:show_range_diff`.
Using a special switch/option was the only way I found in order to fix
this problem.
I could have also created a new option but I thought that doing a
smaller change initially would be better.
§ Changes since version 1
Patch 3: change comment placement based on feedback.
§ Rebase on upstream
Rebased on `master` (fc6bba66bc (Merge branch
'js/allow-t4000-to-be-indented-with-spaces', 2023-08-14)).
§ CI
https://github.com/LemmingAvalanche/git/actions/runs/5868463450
Kristoffer Haugsbakk (3):
range-diff: treat notes like `log`
doc: pretty-options: remove documentation for deprecated options
revision: comment `--no-standard-notes` as deprecated
Documentation/pretty-options.txt | 1 -
range-diff.c | 2 +-
revision.c | 8 ++++++--
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
4 files changed, 35 insertions(+), 4 deletions(-)
Range-diff against v1:
1: 6a4fe606cf = 1: e9a5910831 range-diff: treat notes like `log`
2: fbc1c47b92 = 2: f7308b7abf doc: pretty-options: remove documentation for deprecated options
3: 9141f5a86e ! 3: 80245bbb7e revision: comment `--no-standard-notes` as deprecated
@@ Metadata
## Commit message ##
revision: comment `--no-standard-notes` as deprecated
- We still use `--standard-notes` but this option has no use and is no
- longer documented anywhere.
+ `--standard-notes` used to be deprecated but is now (since 6a4fe606cf[1])
+ used internally. Its negation `--no-standard-notes`, however, is still
+ deprecated even for internal use.
+
+ Mark this option as such.
+
+ † 1: range-diff: treat notes like `log`, 2023-05-19
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
+
+ ## Notes (series) ##
+ • Move comment: https://lore.kernel.org/git/xmqqbkhk724x.fsf@gitster.g/
+ • Tweak commit message so that it's clearer why we are only commenting
+ “no” as deprecated and not the other option as well
+
## revision.c ##
@@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
- enable_default_display_notes(&revs->notes_opt,
&revs->show_notes);
revs->notes_opt.use_default_notes = -1;
-+ /* Deprecated */
} else if (!strcmp(arg, "--no-standard-notes")) {
++ /* Deprecated */
revs->notes_opt.use_default_notes = 0;
} else if (!strcmp(arg, "--oneline")) {
+ revs->verbose_header = 1;
--
2.41.0
^ 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