* 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 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Wesley @ 2023-09-02 1:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: phillip.wood, git
In-Reply-To: <xmqqledppxw3.fsf@gitster.g>
On 9/1/23 14:10, Junio C Hamano wrote:
> 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?
I do not know? I tested with a bare repo and two clones. And I also
tested it with just a remote upstream in another branch.
When in repo-1 I do the reset --hard HEAD^, and push the results, and
pull them in in repo-2 the behavior doesn't replicate. The git reflog
command doesn't show the reset.
However, if I delete the reflog entry for removal of the reset HEAD^,
git rebase exposes the fork-point behavior.
> 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.
I haven't force pushed anything btw, maybe that could explain things?
Cheers,
Wesley
--
Wesley
Why not both?
^ permalink raw reply
* Re: Git in Outreachy? (December, 2023)
From: Kaartic Sivaraam @ 2023-09-02 6:06 UTC (permalink / raw)
To: Christian Couder, Taylor Blau; +Cc: git, Hariom verma, Victoria Dye
In-Reply-To: <CAP8UFD2CGf8efBHquS=LJP8uQ5ivuDryqGz96PZ81oDtrNgNXw@mail.gmail.com>
On 30 August 2023 2:08:45 am IST, Christian Couder <christian.couder@gmail.com> wrote:
>On Tue, Aug 29, 2023 at 7:54 PM Taylor Blau <me@ttaylorr.com> wrote:
>>
>
>It will take some time before I can confirm whether or not GitLab is
>willing to sponsor one Outreachy intern.
>
>> If you (or others) are interested in mentoring but don't have a way to
>> pay Outreachy, we could certainly discuss allocating some of the
>> project's funds to support one or two students.
>
>I clicked on the "Community will participate" button, but this leads
>to a page saying "Outreachy internship funding is required" and where
>we have to list the sponsors we have, the amount of funds they are
>willing to sponsor, if the funding is secured and the funding decision
>date. I think I will fill this out saying that we have 3 (potential)
>sponsors: the Git project, GitHub and GitLab. I will not check the
>"funding is secured" checkbox and say that the decision date is
>October 5 (so just one day before the deadline for "Project list
>finalized"). This will leave time to find out if we can indeed sponsor
>at least one intern.
>
>Let me know if that works for both of you.
>
Great. Thanks for signing Git up for the program, Christian!
>By the way, Kaartic, do you still want to be an org admin? And Taylor
>are you Ok with Kaartic being an org admin?
>
>It seems that I received an email asking me to confirm Kaartic as an
>org admin, but I couldn't confirm then and later when clicked to the
>link to confirm it looked like it wasn't possible anymore to confirm
>that. So not sure what happened, but maybe we could try again to
>register you as an org admin.
>
I actually hit that button to become a community coordinator without confirming here about the need.
I would be able to help as a community coordinator for this round. I'm not sure about being able to mentor, yet.
That said, I believe both of you would do a great job as coordinators for the Git community. So, if you feel like you both could manage without help, that's totally fine. I'll check on the project list once it's available and explore the feasibility of mentoring :-)
Thanks,
Sivaraam
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v2 02/10] merge: simplify parsing of "-n" option
From: René Scharfe @ 2023-09-02 6:20 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
In-Reply-To: <20230831211716.GB949469@coredump.intra.peff.net>
Am 31.08.23 um 23:17 schrieb Jeff King:
> The "-n" option is implemented by an option callback, as it is really a
> "reverse bool". If given, it sets show_diffstat to 0. In theory, when
> negated, it would set the same flag to 1. But it's not possible to
> trigger that, since short options cannot be negated.
>
> So in practice this is really just a SET_INT to 0. Let's use that
> instead, which shortens the code.
>
> Note that negation here would do the wrong thing (as with any SET_INT
> with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof
> ourselves against somebody adding a long option name (which would make
> it possible to negate). But there's not much point:
>
> 1. Nobody is going to do that, because the negated form already
> exists, and is called "--stat" (which is defined separately so that
> "--no-stat" works).
>
> 2. If they did, the BUG() check added by 3284b93862 (parse-options:
> disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
> that check is smart enough to realize that our short-only option is
> OK).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/merge.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 53e9fe70d9..21363b7985 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
> return 0;
> }
>
> -static int option_parse_n(const struct option *opt,
> - const char *arg, int unset)
> -{
> - BUG_ON_OPT_ARG(arg);
> - show_diffstat = unset;
> - return 0;
> -}
Great -- the less custom callbacks, the better.
> -
> static struct option builtin_merge_options[] = {
> - OPT_CALLBACK_F('n', NULL, NULL, NULL,
> - N_("do not show a diffstat at the end of the merge"),
> - PARSE_OPT_NOARG, option_parse_n),
> + OPT_SET_INT('n', NULL, &show_diffstat,
> + N_("do not show a diffstat at the end of the merge"), 0),
> OPT_BOOL(0, "stat", &show_diffstat,
> N_("show a diffstat at the end of the merge")),
Makes it easier to see that we can replace the two complementary
definitions with a single one:
OPT_NEGBIT('n', "no-stat",
N_("do not show a diffstat at the end of the merge"), 1),
Which is a separate topic, of course. And if we did that, however, ...
> OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),
... we wouldn't be able to use OPT_ALIAS here anymore (which we arguably
should) because it doesn't allow referencing the negated (or
"positivated") form -- but that is yet another separate topic.
René
^ permalink raw reply
* Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile
From: René Scharfe @ 2023-09-02 6:20 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
In-Reply-To: <20230831212051.GD949469@coredump.intra.peff.net>
Am 31.08.23 um 23:20 schrieb Jeff King:
> @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> state.base_dir = "";
> state.base_dir_len = strlen(state.base_dir);
>
> + if (to_tempfile < 0)
> + to_tempfile = (checkout_stage == CHECKOUT_ALL);
> + if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
> + die("--stage=all and --no-temp are incompatible");
How about making this message translatable from the start and following
the convention from 12909b6b8a (i18n: turn "options are incompatible"
into "cannot be used together", 2022-01-05) to reuse the existing
translations?
René
^ permalink raw reply
* [PATCH v5] sequencer: beautify subject of reverts of reverts
From: Oswald Buddenhagen @ 2023-09-02 7:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <xmqqsf89e8wz.fsf@gitster.g>
Instead of generating a silly-looking `Revert "Revert "foo""`, make it
a more humane `Reapply "foo"`.
This is done for two reasons:
- To cover the actually common case of just a double revert.
- To encourage people to rewrite summaries of recursive reverts by
setting an example (a subsequent commit will also do this explicitly
in the documentation).
To achieve these goals, the mechanism does not need to be particularly
sophisticated. Therefore, more complicated alternatives which would
"compress more efficiently" have not been implemented.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v4:
- factor out verification of subject as per taylor's patch, with minor
modifications.
fwiw, it might make sense to put this into test-lib-functions.sh right
after test_commit_message(), then named test_commit_subject(). not
sure it would be worth it, given an equally generic implementation
would be kinda over-engineered, and the discoverability is kinda poor.
v3:
- capitulate at first sight of a pre-existing recursive reversion, as
handling the edge cases is a bottomless pit
- reworked commit message again
- moved test into existing file
- generalized docu change and factored it out
v2:
- add discussion to commit message
- add paragraph to docu
- add test
- use skip_prefix() instead of starts_with()
- catch pre-existing double reverts
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
sequencer.c | 11 +++++++++++
t/t3501-revert-cherry-pick.sh | 23 +++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index cc9821ece2..12ec158922 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r,
*/
if (command == TODO_REVERT) {
+ const char *orig_subject;
+
base = commit;
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
strbuf_addstr(&msgbuf,
"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
+ /*
+ * We don't touch pre-existing repeated reverts, because
+ * theoretically these can be nested arbitrarily deeply,
+ * thus requiring excessive complexity to deal with.
+ */
+ !starts_with(orig_subject, "Revert \"")) {
+ strbuf_addstr(&msgbuf, "Reapply \"");
+ strbuf_addstr(&msgbuf, orig_subject);
} else {
strbuf_addstr(&msgbuf, "Revert \"");
strbuf_addstr(&msgbuf, msg.subject);
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index e2ef619323..4158590322 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -176,6 +176,29 @@ test_expect_success 'advice from failed revert' '
test_cmp expected actual
'
+test_expect_subject () {
+ echo "$1" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'titles of fresh reverts' '
+ test_commit --no-tag A file1 &&
+ test_commit --no-tag B file1 &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Revert \"B\"" &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Reapply \"B\"" &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Revert \"Reapply \"B\"\""
+'
+
+test_expect_success 'title of legacy double revert' '
+ test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Revert \"Revert \"Revert \"B\"\"\""
+'
+
test_expect_success 'identification of reverted commit (default)' '
test_commit to-ident &&
test_when_finished "git reset --hard to-ident" &&
--
2.40.0.152.g15d061e6df
^ permalink raw reply related
* Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks
From: René Scharfe @ 2023-09-02 7:34 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
In-Reply-To: <20230831212107.GE949469@coredump.intra.peff.net>
Am 31.08.23 um 23:21 schrieb Jeff King:
> We have several parse-options callbacks that ignore their "opt"
> parameters entirely. This is a little unusual, as we'd normally put the
> result of the parsing into opt->value. In the case of these callbacks,
> though, they directly manipulate global variables instead (and in
> most cases the caller sets opt->value to NULL in the OPT_CALLBACK
> declaration).
>
> The immediate symptom we'd like to deal with is that the unused "opt"
> variables trigger -Wunused-parameter. But how to fix that is debatable.
> One option is to annotate them with UNUSED. But another is to have the
> caller pass in the appropriate variable via opt->value, and use it. That
> has the benefit of making the callbacks reusable (in theory at least),
> and makes it clear from the OPT_CALLBACK declaration which variables
> will be affected (doubly so for the cases in builtin/fast-export.c,
> where we do set opt->value, but it is completely ignored!).
Which allows turning global variables into local ones later, so unlocks
more cleanup potential.
> The slight downside is that we lose type safety, since they're now
> passing through void pointers.
>
> I went with the "just use them" approach here. The loss of type safety
> is unfortunate, but that is already an issue with most of the other
> callbacks. If we want to try to address that, we should do so more
> consistently (and this patch would prepare these callbacks for whatever
> we choose to do there).
Makes sense. The types used below look OK to me.
> Note that in the cases in builtin/fast-export.c, we are passing
> anonymous enums. We'll have to give them names so that we can declare
> the appropriate pointer type within the callbacks.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/checkout-index.c | 8 +++++---
> builtin/describe.c | 6 ++++--
> builtin/fast-export.c | 36 +++++++++++++++++++++---------------
> builtin/fetch.c | 4 ++--
> builtin/interpret-trailers.c | 12 ++++++------
> builtin/pack-objects.c | 21 ++++++++++++---------
> 6 files changed, 50 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 6687a495ff..6ef6ac4c2e 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -193,14 +193,16 @@ static const char * const builtin_checkout_index_usage[] = {
> static int option_parse_stage(const struct option *opt,
> const char *arg, int unset)
> {
> + int *stage = opt->value;
> +
> BUG_ON_OPT_NEG(unset);
>
> if (!strcmp(arg, "all")) {
> - checkout_stage = CHECKOUT_ALL;
> + *stage = CHECKOUT_ALL;
> } else {
> int ch = arg[0];
> if ('1' <= ch && ch <= '3')
> - checkout_stage = arg[0] - '0';
> + *stage = arg[0] - '0';
> else
> die(_("stage should be between 1 and 3 or all"));
> }
> @@ -238,7 +240,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> N_("write the content to temporary files")),
> OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
> N_("when creating files, prepend <string>")),
> - OPT_CALLBACK_F(0, "stage", NULL, "(1|2|3|all)",
> + OPT_CALLBACK_F(0, "stage", &checkout_stage, "(1|2|3|all)",
> N_("copy out the files from named stage"),
> PARSE_OPT_NONEG, option_parse_stage),
> OPT_END()
> diff --git a/builtin/describe.c b/builtin/describe.c
> index b28a4a1f82..718b5c3073 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
> static int option_parse_exact_match(const struct option *opt, const char *arg,
> int unset)
> {
> + int *val = opt->value;
> +
> BUG_ON_OPT_ARG(arg);
>
> - max_candidates = unset ? DEFAULT_CANDIDATES : 0;
> + *val = unset ? DEFAULT_CANDIDATES : 0;
> return 0;
> }
>
> @@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "long", &longformat, N_("always use long format")),
> OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
> OPT__ABBREV(&abbrev),
> - OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
> + OPT_CALLBACK_F(0, "exact-match", &max_candidates, NULL,
> N_("only output exact matches"),
> PARSE_OPT_NOARG, option_parse_exact_match),
> OPT_INTEGER(0, "candidates", &max_candidates,
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 56dc69fac1..70aff515ac 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {
> };
>
> static int progress;
> -static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
> -static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
> -static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
> +static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
> +static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
> +static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
> static int fake_missing_tagger;
> static int use_done_feature;
> static int no_data;
> @@ -53,16 +53,18 @@ static struct revision_sources revision_sources;
> static int parse_opt_signed_tag_mode(const struct option *opt,
> const char *arg, int unset)
> {
> + enum signed_tag_mode *val = opt->value;
> +
> if (unset || !strcmp(arg, "abort"))
> - signed_tag_mode = SIGNED_TAG_ABORT;
> + *val = SIGNED_TAG_ABORT;
> else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> - signed_tag_mode = VERBATIM;
> + *val = VERBATIM;
> else if (!strcmp(arg, "warn"))
> - signed_tag_mode = WARN;
> + *val = WARN;
> else if (!strcmp(arg, "warn-strip"))
> - signed_tag_mode = WARN_STRIP;
> + *val = WARN_STRIP;
> else if (!strcmp(arg, "strip"))
> - signed_tag_mode = STRIP;
> + *val = STRIP;
> else
> return error("Unknown signed-tags mode: %s", arg);
> return 0;
> @@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
> static int parse_opt_tag_of_filtered_mode(const struct option *opt,
> const char *arg, int unset)
> {
> + enum tag_of_filtered_mode *val = opt->value;
> +
> if (unset || !strcmp(arg, "abort"))
> - tag_of_filtered_mode = TAG_FILTERING_ABORT;
> + *val = TAG_FILTERING_ABORT;
> else if (!strcmp(arg, "drop"))
> - tag_of_filtered_mode = DROP;
> + *val = DROP;
> else if (!strcmp(arg, "rewrite"))
> - tag_of_filtered_mode = REWRITE;
> + *val = REWRITE;
> else
> return error("Unknown tag-of-filtered mode: %s", arg);
> return 0;
> @@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
> static int parse_opt_reencode_mode(const struct option *opt,
> const char *arg, int unset)
> {
> + enum reencode_mode *val = opt->value;
> +
> if (unset) {
> - reencode_mode = REENCODE_ABORT;
> + *val = REENCODE_ABORT;
> return 0;
> }
>
> switch (git_parse_maybe_bool(arg)) {
> case 0:
> - reencode_mode = REENCODE_NO;
> + *val = REENCODE_NO;
> break;
> case 1:
> - reencode_mode = REENCODE_YES;
> + *val = REENCODE_YES;
> break;
> default:
> if (!strcasecmp(arg, "abort"))
> - reencode_mode = REENCODE_ABORT;
> + *val = REENCODE_ABORT;
> else
> return error("Unknown reencoding mode: %s", arg);
> }
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8f93529505..fd134ba74d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -176,7 +176,7 @@ static int parse_refmap_arg(const struct option *opt, const char *arg, int unset
> * "git fetch --refmap='' origin foo"
> * can be used to tell the command not to store anywhere
> */
> - refspec_append(&refmap, arg);
> + refspec_append(opt->value, arg);
>
> return 0;
> }
> @@ -2204,7 +2204,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),
> OPT_BOOL(0, "update-shallow", &update_shallow,
> N_("accept refs that update .git/shallow")),
> - OPT_CALLBACK_F(0, "refmap", NULL, N_("refmap"),
> + OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
> N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
> OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
> OPT_IPVERSION(&family),
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index c5e8345265..6aadce6a1e 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;
> static int option_parse_where(const struct option *opt,
> const char *arg, int unset)
> {
> - return trailer_set_where(&where, arg);
> + return trailer_set_where(opt->value, arg);
> }
>
> static int option_parse_if_exists(const struct option *opt,
> const char *arg, int unset)
> {
> - return trailer_set_if_exists(&if_exists, arg);
> + return trailer_set_if_exists(opt->value, arg);
> }
>
> static int option_parse_if_missing(const struct option *opt,
> const char *arg, int unset)
> {
> - return trailer_set_if_missing(&if_missing, arg);
> + return trailer_set_if_missing(opt->value, arg);
> }
Not your fault, but these all silently exit if "arg" contains an
unrecognized value. Reporting the error would be better.
> static void new_trailers_clear(struct list_head *trailers)
> @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
> OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
>
> - OPT_CALLBACK(0, "where", NULL, N_("action"),
> + OPT_CALLBACK(0, "where", &where, N_("action"),
> N_("where to place the new trailer"), option_parse_where),
> - OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
> + OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
> N_("action if trailer already exists"), option_parse_if_exists),
> - OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
> + OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
> N_("action if trailer is missing"), option_parse_if_missing),
And I wonder if "action" should be replaced by "(after|before|end|start)",
"(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)" and
"(doNothing|add)", respectively. Gets a bit long in the middle, but would
be more helpful. #leftoverbits
> OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index d2a162d528..492372ee5d 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4120,29 +4120,32 @@ static void add_extra_kept_packs(const struct string_list *names)
> static int option_parse_quiet(const struct option *opt, const char *arg,
> int unset)
> {
> + int *val = opt->value;
> +
> BUG_ON_OPT_ARG(arg);
>
> if (!unset)
> - progress = 0;
> - else if (!progress)
> - progress = 1;
> + *val = 0;
> + else if (!*val)
> + *val = 1;
> return 0;
> }
>
> static int option_parse_index_version(const struct option *opt,
> const char *arg, int unset)
> {
> + struct pack_idx_option *popts = opt->value;
> char *c;
> const char *val = arg;
>
> BUG_ON_OPT_NEG(unset);
>
> - pack_idx_opts.version = strtoul(val, &c, 10);
> - if (pack_idx_opts.version > 2)
> + popts->version = strtoul(val, &c, 10);
> + if (popts->version > 2)
> die(_("unsupported index version %s"), val);
> if (*c == ',' && c[1])
> - pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
> - if (*c || pack_idx_opts.off32_limit & 0x80000000)
> + popts->off32_limit = strtoul(c+1, &c, 0);
> + if (*c || popts->off32_limit & 0x80000000)
> die(_("bad index version '%s'"), val);
> return 0;
> }
> @@ -4190,7 +4193,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> LIST_OBJECTS_FILTER_INIT;
>
> struct option pack_objects_options[] = {
> - OPT_CALLBACK_F('q', "quiet", NULL, NULL,
> + OPT_CALLBACK_F('q', "quiet", &progress, NULL,
> N_("do not show progress meter"),
> PARSE_OPT_NOARG, option_parse_quiet),
> OPT_SET_INT(0, "progress", &progress,
> @@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "all-progress-implied",
> &all_progress_implied,
> N_("similar to --all-progress when progress meter is shown")),
> - OPT_CALLBACK_F(0, "index-version", NULL, N_("<version>[,<offset>]"),
> + OPT_CALLBACK_F(0, "index-version", &pack_idx_opts, N_("<version>[,<offset>]"),
> N_("write the pack index file in the specified idx format version"),
> PARSE_OPT_NONEG, option_parse_index_version),
> OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
^ permalink raw reply
* [PATCH v2] ref-filter: sort numerically when ":size" is used
From: Kousik Sanagavarapu @ 2023-09-02 9:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu, Jeff King
In-Reply-To: <20230901142624.12063-1-five231003@gmail.com>
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.
For example, consider the the following case in a repo
refname contents:size raw:size
======= ============= ========
refs/heads/branch1 1130 1210
refs/heads/master 300 410
refs/tags/v1.0 140 260
Sorting with "--format="%(refname) %(contents:size) --sort=contents:size"
would give
refs/heads/branch1 1130
refs/tags/v1.0.0 140
refs/heads/master 300
which is an alphabetic sort, while what one might really expect is
refs/tags/v1.0.0 140
refs/heads/master 300
refs/heads/branch1 1130
which is a numeric sort (that is, a "$ sort -n file" as opposed to a
"$ sort file", where "file" contains only the "contents:size" or
"raw:size" info, each of which is on a newline).
Same is the case with "--sort=raw:size".
So, sort numerically whenever the sort is done with "contents:size" or
"raw:size" and do it the normal alphabetic way when "contents" or "raw"
are used with some other option (they are FIELD_STR anyways).
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
ref-filter.c | 21 +++++++++++++--------
t/t6300-for-each-ref.sh | 15 +++++++++++++--
2 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 1bfaf20fbf..9dbc4f71bd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -582,9 +582,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;
@@ -690,9 +691,10 @@ static int raw_atom_parser(struct ref_format *format UNUSED,
{
if (!arg)
atom->u.raw_data.option = RAW_BARE;
- else if (!strcmp(arg, "size"))
+ else if (!strcmp(arg, "size")) {
+ atom->type = FIELD_ULONG;
atom->u.raw_data.option = RAW_LENGTH;
- else
+ } else
return err_bad_arg(err, "raw", arg);
return 0;
}
@@ -1857,7 +1859,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
v->s = xmemdupz(buf, buf_size);
v->s_size = buf_size;
} else if (atom->u.raw_data.option == RAW_LENGTH) {
- v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);
+ v->value = buf_size;
+ v->s = xstrfmt("%"PRIuMAX, v->value);
}
continue;
}
@@ -1883,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);
@@ -2265,6 +2269,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
v->s_size = ATOM_SIZE_UNSPECIFIED;
v->handler = append_atom;
+ v->value = 0;
v->atom = atom;
if (*name == '*') {
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index aa3c7c03c4..7b943fd34c 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1017,16 +1017,16 @@ test_expect_success 'Verify sorts with raw' '
test_expect_success 'Verify sorts with raw:size' '
cat >expected <<-EOF &&
refs/myblobs/blob8
- refs/myblobs/first
refs/myblobs/blob7
- refs/heads/main
refs/myblobs/blob4
refs/myblobs/blob1
refs/myblobs/blob2
refs/myblobs/blob3
refs/myblobs/blob5
refs/myblobs/blob6
+ refs/myblobs/first
refs/mytrees/first
+ refs/heads/main
EOF
git for-each-ref --format="%(refname)" --sort=raw:size \
refs/heads/main refs/myblobs/ refs/mytrees/first >actual &&
@@ -1138,6 +1138,17 @@ test_expect_success 'for-each-ref --format compare with cat-file --batch' '
test_cmp expected actual
'
+test_expect_success 'verify sorts with contents:size' '
+ cat >expect <<-\EOF &&
+ refs/heads/main
+ refs/heads/newtag
+ refs/heads/ambiguous
+ EOF
+ git for-each-ref --format="%(refname)" \
+ --sort=contents:size refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'set up multiple-sort tags' '
for when in 100000 200000
do
--
2.42.0.101.g9b561e429b.dirty
^ permalink raw reply related
* Re: [PATCH v2] ref-filter: sort numerically when ":size" is used
From: Kousik Sanagavarapu @ 2023-09-02 9:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20230902090155.8978-1-five231003@gmail.com>
On Sat, Sep 02, 2023 at 02:30:39PM +0530, Kousik Sanagavarapu wrote:
> 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.
[...]
> So, sort numerically whenever the sort is done with "contents:size" or
> "raw:size" and do it the normal alphabetic way when "contents" or "raw"
> are used with some other option (they are FIELD_STR anyways).
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
Oops, forgot the range-diff.
Range-diff against v1:
1: 9b561e429b ! 1: 194dcb0b0d ref-filter: sort numerically when ":size" is used
@@ Commit message
"raw:size" and do it the normal alphabetic way when "contents" or "raw"
are used with some other option (they are FIELD_STR anyways).
+ Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
## ref-filter.c ##
-@@ ref-filter.c: 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;
-+
- struct used_atom *atom;
- };
-
+@@ ref-filter.c: 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;
+@@ ref-filter.c: static int raw_atom_parser(struct ref_format *format UNUSED,
+ {
+ if (!arg)
+ atom->u.raw_data.option = RAW_BARE;
+- else if (!strcmp(arg, "size"))
++ else if (!strcmp(arg, "size")) {
++ atom->type = FIELD_ULONG;
+ atom->u.raw_data.option = RAW_LENGTH;
+- else
++ } else
+ return err_bad_arg(err, "raw", arg);
+ return 0;
+ }
@@ ref-filter.c: static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
v->s = xmemdupz(buf, buf_size);
v->s_size = buf_size;
v->s = xmemdupz(buf, buf_size);
v->s_size = buf_size;
v->s_size = buf_size;
} else if (atom->u.raw_data.option == RAW_LENGTH) {
- v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);
-+ v->value = (uintmax_t)buf_size;
++ v->value = buf_size;
+ v->s = xstrfmt("%"PRIuMAX, v->value);
}
continue;
@@ ref-filter.c: static void grab_sub_body_contents(struct atom_value *val, int der
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 = (uintmax_t)strlen(subpos);
++ v->value = strlen(subpos);
+ v->s = xstrfmt("%"PRIuMAX, v->value);
-+ }
- else if (atom->u.contents.option == C_BODY)
++ } 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);
@@ ref-filter.c: static int populate_value(struct ref_array_item *ref, struct strbuf *err)
v->s_size = ATOM_SIZE_UNSPECIFIED;
@@ ref-filter.c: static int populate_value(struct ref_array_item *ref, struct strbu
v->atom = atom;
if (*name == '*') {
-@@ ref-filter.c: 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) {
- if (va->s_size < 0 && vb->s_size < 0) {
- int (*cmp_fn)(const char *, const char *);
- cmp_fn = s->sort_flags & REF_SORTING_ICASE
## t/t6300-for-each-ref.sh ##
@@ t/t6300-for-each-ref.sh: test_expect_success 'Verify sorts with raw' '
^ permalink raw reply
* Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks
From: René Scharfe @ 2023-09-02 10:12 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
In-Reply-To: <20230831212128.GF949469@coredump.intra.peff.net>
Am 31.08.23 um 23:21 schrieb Jeff King:
> The previous commit argued that parse-options callbacks should try to
> use opt->value rather than touching globals directly. In some cases,
> however, that's awkward to do. Some callbacks touch multiple variables,
> or may even just call into an abstracted function that does so.
>
> In some of these cases we _could_ convert them by stuffing the multiple
> variables into a single struct and passing the struct pointer through
> opt->value. But that may make other parts of the code less readable,
> as the struct relationship has to be mentioned everywhere.
Does that imply you'd be willing to use other methods? Let's find out
below. :)
>
> Let's just accept that these cases are special and leave them as-is. But
> we do need to mark their "opt" parameters to satisfy -Wunused-parameter.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/gc.c | 2 +-
> builtin/log.c | 10 ++++++----
> builtin/merge.c | 2 +-
> builtin/pack-objects.c | 6 +++---
> builtin/read-tree.c | 2 +-
> builtin/update-index.c | 4 ++--
> 6 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 369bd43fb2..b842349d86 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
> strbuf_release(&config_name);
> }
>
> -static int task_option_parse(const struct option *opt,
> +static int task_option_parse(const struct option *opt UNUSED,
Only the global variable "tasks" seems to be used in here if you don't
count the constant "TASK__COUNT", so you could pass it in. This could
also be converted to OPT_STRING_LIST with parsing and duplicate checking
done later.
And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
on error, but that's a different matter.
> const char *arg, int unset)
> {
> int i, num_selected = 0;
> diff --git a/builtin/log.c b/builtin/log.c
> index fb90d43717..3599063554 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
> static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
> static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
>
> -static int clear_decorations_callback(const struct option *opt,
> - const char *arg, int unset)
> +static int clear_decorations_callback(const struct option *opt UNUSED,
> + const char *arg, int unset)
> {
> string_list_clear(&decorate_refs_include, 0);
> string_list_clear(&decorate_refs_exclude, 0);
> use_default_decoration_filter = 0;
> return 0;
> }
>
Meta: Why do we get seven lines of context in an -U3 patch here? Did
you use --inter-hunk-context?
This patch would be better viewed with --function-context to see that
the callbacks change multiple variables or do other funky things. Only
doubles the line count.
> -static int decorate_callback(const struct option *opt, const char *arg, int unset)
> +static int decorate_callback(const struct option *opt UNUSED, const char *arg,
> + int unset)
> {
> if (unset)
> decoration_style = 0;
Sets decoration_style and decoration_given. Never sets decoration_given
to a negative value. This could be used to initialize decoration_style
to -1 and set decoration_given after parsing, then you could pass in
just decoration_style.
> @@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> -static int header_callback(const struct option *opt, const char *arg, int unset)
> +static int header_callback(const struct option *opt UNUSED, const char *arg,
Sorts strings into three string_lists based on prefix. Could become a
vanilla OPT_STRING_LIST with parsing and sorting done later, if it
wouldn't interact with to_callback() and cc_callback().
(--function-context is not enough to see that, it requires a look at the
definition of those other functions as well.)
> + int unset)
> {
> if (unset) {
> string_list_clear(&extra_hdr, 0);
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 21363b7985..0436986dab 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
> use_strategies[use_strategies_nr++] = s;
> }
>
> -static int option_parse_strategy(const struct option *opt,
> +static int option_parse_strategy(const struct option *opt UNUSED,
Could be an OPT_STRING_LIST and parsing done later. Except that
--no-strategy does nothing, which is weird.
> const char *name, int unset)
> {
> if (unset)
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 492372ee5d..91b4b7c177 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
> show_object(obj, name, data);
> }
>
> -static int option_parse_missing_action(const struct option *opt,
> +static int option_parse_missing_action(const struct option *opt UNUSED,
Sets the enum arg_missing_action, fetch_if_missing and fn_show_object.
Could be changed to set only the enum and then you could pass it in.
> const char *arg, int unset)
> {
> assert(arg);
> @@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
> return 0;
> }
>
> -static int option_parse_unpack_unreachable(const struct option *opt,
> +static int option_parse_unpack_unreachable(const struct option *opt UNUSED,
Sets both unpack_unreachable and unpack_unreachable_expiration. Perhaps
could set only unpack_unreachable_expiration and use TIME_MAX if no
argument is given, then set unpack_unreachable after parsing? Not sure.
> const char *arg, int unset)
> {
> if (unset) {
> @@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
> return 0;
> }
>
> -static int option_parse_cruft_expiration(const struct option *opt,
> +static int option_parse_cruft_expiration(const struct option *opt UNUSED,
Does the same as option_parse_unpack_unreachable(), just with two
different variables. And interacts with --cruft.
> const char *arg, int unset)
> {
> if (unset) {
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 1fec702a04..8196ca9dd8 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
> NULL
> };
>
> -static int index_output_cb(const struct option *opt, const char *arg,
> +static int index_output_cb(const struct option *opt UNUSED, const char *arg,
Calls set_alternate_index_output(). Could become an OPT_STRING_F if we
assume that the last value wins and earlier calls have no side effects.
> int unset)
> {
> BUG_ON_OPT_NEG(unset);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index aee3cb8cbd..59acae3336 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
> return 0;
> }
>
> -static int resolve_undo_clear_callback(const struct option *opt,
> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,
Affects the_index. This option (--clear-resolve-undo) is not mentioned
in Documentation/, by the way. Not sure if it interacts with other
callbacks, like the one below. Otherwise could be an OPT_BOOL and its
action done after parsing. Perhaps pass in &the_index?
> const char *arg, int unset)
> {
> BUG_ON_OPT_NEG(unset);
> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
> }
>
> static enum parse_opt_result cacheinfo_callback(
> - struct parse_opt_ctx_t *ctx, const struct option *opt,
> + struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
A low-level callback for a change. Affects the_index. Pass it in?
add_cacheinfo() would need to grow a parameter to receive it. It would
document that this option directly changes the_index.
> const char *arg, int unset)
> {
> struct object_id oid;
Phew, looks like that's stuff for a whole new series or two!
René
^ permalink raw reply
* Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback
From: René Scharfe @ 2023-09-02 11:37 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
In-Reply-To: <20230831212220.GJ949469@coredump.intra.peff.net>
Am 31.08.23 um 23:22 schrieb Jeff King:
> Unsurprisingly, the noop options callback doesn't bother to look at any
> of its parameters. Let's mark them so that -Wunused-parameter does not
> complain.
>
> Another option would be to drop the callback and have parse-options
> itself recognize OPT_NOOP_NOARG. But that seems like extra work for no
> real benefit.
I'm not sure about that -- we don't need to add special flags or option
types to drop parse_opt_noop_cb().
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> parse-options-cb.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index a24521dee0..bdc7fae497 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
> +int parse_opt_noop_cb(const struct option *opt UNUSED,
> + const char *arg UNUSED,
> + int unset UNUSED)
> {
> return 0;
> }
Your patch is simple and makes sense. The one below is more
complicated, but leaves the code in a slightly better state. I'd go
with that variant if I'd had to add OPT_NOOP_NOARG today, and I
slightly prefer it, but similar to you think that the benefit is low
(though non-zero). So I'm unsure if that's enough to be worth it or
just bikeshedding with some hindsight..
René
--- >8 ---
Subject: [PATCH] parse-options: let NULL callback be a noop
Allow OPTION_CALLBACK options to have a NULL callback function pointer
and do nothing in that case, yet still handle arguments as usual. Use
this to replace parse_opt_noop_cb().
We lose the ability to distinguish between a forgotten function pointer
and intentional noop, but that will be caught by a compiler warning
about an unused function in many cases and having an option do nothing
is a benign failure mode.
We also lose the ability to add a warning to the callback, but we
haven't done that since it was added by 6acec0380b (parseopt: add
OPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon. We can
add a callback back when we want to show a warning.
By letting go of features we don't need this patch simplifies the
parse-options API and gets rid of an exported empty function.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options-cb.c | 5 -----
parse-options.c | 7 +++----
parse-options.h | 2 --
3 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index a24521dee0..e79cd54ec2 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -227,11 +227,6 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
return 0;
}
-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
-{
- return 0;
-}
-
/**
* Recreates the command-line option in the strbuf.
*/
diff --git a/parse-options.c b/parse-options.c
index 76d2e76b49..5be8de93f5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -201,8 +201,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
}
if (opt->callback)
return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;
- else
+ if (opt->ll_callback)
return (*opt->ll_callback)(p, opt, p_arg, p_unset);
+ return 0;
}
case OPTION_INTEGER:
if (unset) {
@@ -494,9 +495,7 @@ static void parse_options_check(const struct option *opts)
optbug(opts, "should not accept an argument");
break;
case OPTION_CALLBACK:
- if (!opts->callback && !opts->ll_callback)
- optbug(opts, "OPTION_CALLBACK needs one callback");
- else if (opts->callback && opts->ll_callback)
+ if (opts->callback && opts->ll_callback)
optbug(opts, "OPTION_CALLBACK can't have two callbacks");
break;
case OPTION_LOWLEVEL_CALLBACK:
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..41bb47120d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -348,7 +348,6 @@ struct option {
.long_name = (l), \
.help = N_("no-op (backward compatibility)"), \
.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, \
- .callback = parse_opt_noop_cb, \
}
#define OPT_ALIAS(s, l, source_long_name) { \
@@ -490,7 +489,6 @@ int parse_opt_commit(const struct option *, const char *, int);
int parse_opt_tertiary(const struct option *, const char *, int);
int parse_opt_string_list(const struct option *, const char *, int);
int parse_opt_strvec(const struct option *, const char *, int);
-int parse_opt_noop_cb(const struct option *, const char *, int);
int parse_opt_passthru(const struct option *, const char *, int);
int parse_opt_passthru_argv(const struct option *, const char *, int);
/* value is enum branch_track* */
--
2.42.0
^ permalink raw reply related
* Hello world
From: ross nicholas Oneil thomas @ 2023-09-02 13:25 UTC (permalink / raw)
To: git@vger.kernel.org
Hello hello hello
Sent from my iPhone
Mr.R,Thomas
(559-816-2950
^ permalink raw reply
* [RFC] New configuration option "diff.statNameWidth"
From: Dragan Simic @ 2023-09-02 18:03 UTC (permalink / raw)
To: git
Hello,
I'd like to implement support for a new configuration option named
"diff.statNameWidth" and submit the patch, so I'd like to check first
would that patch be accepted and merged.
Similarly to the already existing configuration option
"diff.statGraphWidth",[1][2] "diff.statNameWidth" as a new configuration
option would limit the width of the filename part in generated diffstat
outputs, just like the command-line option "--stat-name-width=<n>"
already does.
Having "diff.statNameWidth" as a new configuration option would be quite
usable, and it would fit together nicely with the already existing
"diff.statGraphWidth" configuration option. For exaple, I have quite a
few aliases in my ~/.gitconfig that contain "--stat-name-width=<n>", and
having this new configuration option would allow those aliases to be
much shorter. Of course, it would benefit all other uses in which
diffstats are generated.
Please advise.
Links:
[1]
https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---statltwidthgtltname-widthgtltcountgt
[2]
https://git-scm.com/docs/git-config#Documentation/git-config.txt-diffstatGraphWidth
^ permalink raw reply
* [PATCH] name-rev: use OPT_HIDDEN_BOOL for --peel-tag
From: René Scharfe @ 2023-09-02 18:38 UTC (permalink / raw)
To: Git List
adfc1857bd (describe: fix --contains when a tag is given as input,
2013-07-18) added the option --peel-tag, defining it using a positional
struct option initializer and a comment indicating that it's intended to
be a hidden OPT_BOOL. 4741edd549 (Remove deprecated OPTION_BOOLEAN for
parsing arguments, 2013-08-03) added the macro OPT_HIDDEN_BOOL, which
allows to express this more succinctly. Use it.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/name-rev.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c706fa3720..2dd1807c4e 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -582,12 +582,8 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
OPT_BOOL(0, "always", &always,
N_("show abbreviated commit object as fallback")),
- {
- /* A Hidden OPT_BOOL */
- OPTION_SET_INT, 0, "peel-tag", &peel_tag, NULL,
- N_("dereference tags in the input (internal use)"),
- PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1,
- },
+ OPT_HIDDEN_BOOL(0, "peel-tag", &peel_tag,
+ N_("dereference tags in the input (internal use)")),
OPT_END(),
};
--
2.42.0
^ permalink raw reply related
* Re: [RFC] New configuration option "diff.statNameWidth"
From: Junio C Hamano @ 2023-09-02 18:47 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <eb8f524eca3975f086715ec32a8a1fbb@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> I'd like to implement support for a new configuration option named
> "diff.statNameWidth" and submit the patch, so I'd like to check first
> would that patch be accepted and merged.
In general, we do not give promises or estimates. The devil is in
the details and until we see the design we may not know if an
overall idea is good. Even when it is obviously a good idea, we
would not know the quality of the implementation until we see it.
- If something is worth adding, even if we do not accept it in the
upstream first, it will spread among users and developers, and
eventually we may realize the mistake of initially not taking it
and we may come begging to the contributor for upstreaming.
- On the other hand, a new thing that even the contributor
themselves are unsure if it is worth investing their work in, if
it is only to use it for themselves, is very unlikely to be of
interest to us or our users.
"If this will be accepted, I'll work on it" is a very counter
productive thing to say around here. It is easily (mis)taken as a
sign that it is the latter case. "This is a good idea, I believe in
it, and I'll work on it whether you initially show interest or not"
is what we want to see, and such a patch will not need a "check
first" letter.
In other words, make it so good that we would come to you, begging
;-).
Thanks.
^ permalink raw reply
* [PATCH] grep: use OPT_INTEGER_F for --max-depth
From: René Scharfe @ 2023-09-02 18:54 UTC (permalink / raw)
To: Git List
a91f453f64 (grep: Add --max-depth option., 2009-07-22) added the option
--max-depth, defining it using a positional struct option initializer of
type OPTION_INTEGER. It also sets defval to 1 for some reason, but that
value would only be used if the flag PARSE_OPT_OPTARG was given.
Use the macro OPT_INTEGER_F instead to standardize the definition and
specify only the necessary values. This also normalizes argh to N_("n")
as a side-effect, which is OK.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/grep.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 50e712a184..f5f5f6dbe1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -924,9 +924,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
N_("process binary files with textconv filters")),
OPT_SET_INT('r', "recursive", &opt.max_depth,
N_("search in subdirectories (default)"), -1),
- { OPTION_INTEGER, 0, "max-depth", &opt.max_depth, N_("depth"),
- N_("descend at most <depth> levels"), PARSE_OPT_NONEG,
- NULL, 1 },
+ OPT_INTEGER_F(0, "max-depth", &opt.max_depth,
+ N_("descend at most <n> levels"), PARSE_OPT_NONEG),
OPT_GROUP(""),
OPT_SET_INT('E', "extended-regexp", &opt.pattern_type_option,
N_("use extended POSIX regular expressions"),
--
2.42.0
^ permalink raw reply related
* Re: [RFC] New configuration option "diff.statNameWidth"
From: Dragan Simic @ 2023-09-02 18:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8r9ommyt.fsf@gitster.g>
On 2023-09-02 20:47, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> I'd like to implement support for a new configuration option named
>> "diff.statNameWidth" and submit the patch, so I'd like to check first
>> would that patch be accepted and merged.
>
> In general, we do not give promises or estimates. The devil is in
> the details and until we see the design we may not know if an
> overall idea is good. Even when it is obviously a good idea, we
> would not know the quality of the implementation until we see it.
Oh, totally! A good idea with a poor implementation is something that
simply can't be accepted and merged. No promises can be made until the
code is available for review.
> - If something is worth adding, even if we do not accept it in the
> upstream first, it will spread among users and developers, and
> eventually we may realize the mistake of initially not taking it
> and we may come begging to the contributor for upstreaming.
>
> - On the other hand, a new thing that even the contributor
> themselves are unsure if it is worth investing their work in, if
> it is only to use it for themselves, is very unlikely to be of
> interest to us or our users.
I agree, the mantra of open-source could be formulated as something like
"find an itch and scratch it" or "build it and they will come". In
other words, if someone finds something useful and worth investing their
time, perhaps other people will find it useful, too.
> "If this will be accepted, I'll work on it" is a very counter
> productive thing to say around here. It is easily (mis)taken as a
> sign that it is the latter case. "This is a good idea, I believe in
> it, and I'll work on it whether you initially show interest or not"
> is what we want to see, and such a patch will not need a "check
> first" letter.
>
> In other words, make it so good that we would come to you, begging
> ;-).
Thank you very much for your thoughts and the time required to write it
all down. I'll do my best to make my patch(es) irresistible. :)
^ permalink raw reply
* Re: [RFC] New configuration option "diff.statNameWidth"
From: Junio C Hamano @ 2023-09-02 22:16 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <72c114086590b9b15a3fdd9e0d6bd67e@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Thank you very much for your thoughts and the time required to write
> it all down. I'll do my best to make my patch(es) irresistible. :)
Heh, I have to send the same from time to time, so it didn't take
that much time. Having said all the above, once we start seeing the
actual patches and its sales pitch (aka proposed commit log
message), we do guide and help polishing the patch into a better
shape, so it will not be like the contributor has to work in the
dark without knowing what level of bar their contribution has to
pass.
Thanks.
^ permalink raw reply
* [PATCH v2] Emit warning when rebasing without a forkpoint
From: Wesley Schwengle @ 2023-09-02 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1qfiubg5.fsf@gitster.g>
This is the second version of the patch series.
Patch 1: Be able to use rebase.forkpoint and --root
Patch 2: Adding the warning + tests
Patch 3: Update documenation
I think I have covered most of your concerns and feedback in this second
version.
On 8/31/23 16:57, Junio C Hamano wrote:
> Wesley Schwengle <wesleys@opperschaap.net> writes:
>
> Here is my attempt to rewrite the above:
>
> When 'git rebase' is run without specifying <upstream> on the
> command line, the current default is to use the fork-point
> heuristics, but this is expected to change in a future version
> of Git, and you will have to explicitly give "--fork-point" from
> the command line if you keep using the fork-point mode. You can
> run "git config rebase.forkpoint false" to adopt the new default
> in advance and that will also squelch the message.
I agree. I'll change the text to your version.
> Note that the parsing of "rebase.forkpoint" is a bit peculiar in
> that
>
> - By leaving it unspecified, the .fork_point = -1 in
> REBASE_OPTIONS_INIT takes effect (which is unsurprising);
>
> - By setting it to false, .fork_point becomes 0; but
>
> - If you set the configuration variable to true, .fork_point
> becomes -1, not 1.
I changed this in patch 1.
> And this is very much deliberate if I understand it correctly [*1*].
> By the time we get to this part of the code (i.e. .fork_point is
> -1), the user may already have rebase.forkpoint set to true. IOW,
> setting it to 'true' is not a valid way to squelch this message.
So this works now with patch 2.
> Another worrysome thing about rebase.forkpoint is that it will be
> inevitable for folks to start complaining that it does not work the
> way other configuration variables do. Setting the variable to
> 'true' is not the same as passing '--fork-point=true' from the
> command line.
I think it is now with the current series.
> I actually think it would be a lot larger behaviour change with a
> huge potential to be received as a regression if we start making the
> variable to mean the same thing as passing '--fork-point=true'.
> People may like the current "if you are rebuilding your branch on
> its usual upstream, pay attention to the rebase and rewind of the
> upstream itself, but if you are giving an explicit upstream from the
> command line, the tool does not second guess you with the fork-point
> heuristics" behaviour and prefer to set it to true. We would be
> breaking them big time if suddenly the rebase.forkpoint=true they
> set previously starts triggering the fork-point heuristics when they
> run "git rebase upstream". So that needs to be kept in mind when/if
> we fix the "setting the variable, even to 'true', will squelch the
> warning".
I get what you are saying. My solution is to make the --fork-point or
--no-fork-point more explicit. People could use an alias for this?
It would mean a different approach to the problem and deprecating
rebase.forkpoint as a boolean value. It could become one of three values:
"true", "false" and "legacy". Where "legacy" can be "implicit" or "auto".
Although you had some ideas on "auto" already. I'm not sure on how I would call
it. "no-upstream"?
--
Wesley
Why not both?
^ permalink raw reply
* [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Wesley Schwengle @ 2023-09-02 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20230902221641.1399624-1-wesleys@opperschaap.net>
This patch adds a warning where it will indicate that `rebase.forkpoint'
must be set in the git configuration and/or that you can supply a
`--fork-point' or `--no-fork-point' command line option to your `git
rebase' invocation.
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).
You would than need to use `git rebase <upstream>' to rebase. If you
configure an upstream it would seem logical to be able to run `git
rebase' without arguments. However doing so would trigger a different
kind of behavior. `git rebase <upstream>' behaves as if
`--no-fork-point' was supplied and without it behaves as if
`--fork-point' was supplied. This behavior can result in a loss of
commits and can surprise users. The following reproduction path exposes
this behavior:
git init reproduction
cd reproduction
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
git reset --hard HEAD^
git switch foo
Switched to branch 'foo'
Your branch is ahead of 'master' by 1 commit.
git log --oneline
5f427e3 Third commit
03ad791 Second commit
411e6d4 First commit
git rebase
git status
On branch foo
Your branch is up to date with 'master'.
git log --oneline
03ad791 Second commit
411e6d4 First commit
Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
builtin/rebase.c | 16 +++++++++-
t/t3431-rebase-fork-point.sh | 62 ++++++++++++++++++++++++++++++++++++
2 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2108001600..ee7db9ba0c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1608,8 +1608,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
NULL);
if (!options.upstream_name)
error_on_missing_default_upstream();
- if (options.fork_point < 0)
+ if (options.fork_point < 0) {
+ warning(_(
+ "When \"git rebase\" is run without specifying <upstream> on the\n"
+ "command line, the current default is to use the fork-point\n"
+ "heuristics. This is expected to change in a future version\n"
+ "of Git, and you will have to explicitly give \"--fork-point\" from\n"
+ "the command line if you keep using the fork-point mode. You can\n"
+ "run \"git config rebase.forkpoint false\" to adopt the new default\n"
+ "in advance and that will also squelch the message.\n"
+ "\n"
+ "You can replace \"git config\" with \"git config --global\" to set a default\n"
+ "preference for all repositories. You can also pass --no-fork-point, --fork-point\n"
+ "on the command line to override the configured default per invocation.\n"
+ ));
options.fork_point = 1;
+ }
} else {
options.upstream_name = argv[0];
argc--;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 4bfc779bb8..908867ae0f 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -113,4 +113,66 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '
git rebase --root
'
+# The use of the diff -qw is because there is some kind of whitespace character
+# magic going on which probably has to do with the tabs. It only occurs when we
+# check STDERR
+test_expect_success 'rebase without rebase.forkpoint' '
+ git init rebase-forkpoint &&
+ cd rebase-forkpoint &&
+ git status >/tmp/foo &&
+ 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=main &&
+ git switch main &&
+ git merge foo &&
+ git reset --hard HEAD^ &&
+ git switch foo &&
+ commit=$(git log -n1 --format="%h") &&
+ git rebase >out 2>err &&
+ test_must_be_empty out &&
+ cat <<-\OEF > expect &&
+ warning: When "git rebase" is run without specifying <upstream> on the
+ command line, the current default is to use the fork-point
+ heuristics. This is expected to change in a future version
+ of Git, and you will have to explicitly give "--fork-point" from
+ the command line if you keep using the fork-point mode. You can
+ run "git config rebase.forkpoint false" to adopt the new default
+ in advance and that will also squelch the message.
+
+ You can replace "git config" with "git config --global" to set a default
+ preference for all repositories. You can also pass --no-fork-point, --fork-point
+ on the command line to override the configured default per invocation.
+
+ Successfully rebased and updated refs/heads/foo.
+ OEF
+ diff -qw expect err &&
+ git reset --hard $commit &&
+ git rebase --fork-point >out 2>err &&
+ test_must_be_empty out &&
+ echo "Successfully rebased and updated refs/heads/foo." > expect &&
+ diff -qw expect err &&
+ git reset --hard $commit &&
+ git rebase --no-fork-point >out 2>err &&
+ test_must_be_empty err &&
+ echo "Current branch foo is up to date." > expect &&
+ test_cmp out expect &&
+ git config --add rebase.forkpoint true &&
+ git rebase >out 2>err &&
+ test_must_be_empty out &&
+ echo "Successfully rebased and updated refs/heads/foo." > expect &&
+ diff -qw expect err &&
+ git reset --hard $commit &&
+ git config --replace-all rebase.forkpoint false &&
+ git rebase >out 2>err &&
+ test_must_be_empty err &&
+ echo "Current branch foo is up to date." > expect &&
+ test_cmp out expect
+'
+
test_done
--
2.42.0.103.g5622fd1409.dirty
^ permalink raw reply related
* [PATCH v2 1/3] rebase.c: Make a distiction between rebase.forkpoint and --fork-point arguments
From: Wesley Schwengle @ 2023-09-02 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20230902221641.1399624-1-wesleys@opperschaap.net>
When you call `git rebase --root' we are not interested in the
rebase.forkpoint configuration. The two options are not to be combined.
Because the implementation checks if the configured value for using a
forkpoint > 0 I've opted to give the configured forkpoint the value 2.
If the user supplies --fork-point on the command line this has a value
of 1. Now we can make a distinction between user input and the configured
value of rebase.forkpoint.
Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
builtin/rebase.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..2108001600 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -824,7 +824,7 @@ static int rebase_config(const char *var, const char *value,
}
if (!strcmp(var, "rebase.forkpoint")) {
- opts->fork_point = git_config_bool(var, value) ? -1 : 0;
+ opts->fork_point = git_config_bool(var, value) ? 2 : 0;
return 0;
}
@@ -1264,8 +1264,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.fork_point < 0)
options.fork_point = 0;
}
- if (options.root && options.fork_point > 0)
+ if (options.root && options.fork_point == 1) {
die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
+ } else if (options.root && options.fork_point > 1) {
+ options.fork_point = 0;
+ }
+
if (options.action != ACTION_NONE && !in_progress)
die(_("No rebase in progress?"));
--
2.42.0.103.g5622fd1409.dirty
^ permalink raw reply related
* [PATCH v2 3/3] git-rebase.txt: Add deprecation notice to the --fork-point options
From: Wesley Schwengle @ 2023-09-02 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20230902221641.1399624-1-wesleys@opperschaap.net>
Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
Documentation/git-rebase.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..e47b58bec2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -462,7 +462,9 @@ ends up being empty, the `<upstream>` will be used as a fallback.
+
If `<upstream>` or `--keep-base` is given on the command line, then
the default is `--no-fork-point`, otherwise the default is
-`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1]. This
+behaviour will be changed in an upcoming release of git. It is advised that you
+set `rebase.forkpoint` in your config or supply the command line switches.
+
If your branch was based on `<upstream>` but `<upstream>` was rewound and
your branch contains commits which were dropped, this option can be used
--
2.42.0.103.g5622fd1409.dirty
^ permalink raw reply related
* Re: [PATCH v2] ref-filter: sort numerically when ":size" is used
From: Junio C Hamano @ 2023-09-02 22:19 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Jeff King
In-Reply-To: <20230902090155.8978-1-five231003@gmail.com>
Kousik Sanagavarapu <five231003@gmail.com> writes:
> } else if (atom->u.raw_data.option == RAW_LENGTH) {
> - v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);
> + v->value = buf_size;
> + v->s = xstrfmt("%"PRIuMAX, v->value);
> }
> continue;
Interesting that typeof(.value) happens to be uintmax_t, keeping
this a safe change.
^ permalink raw reply
* Re: [PATCH v5] sequencer: beautify subject of reverts of reverts
From: Junio C Hamano @ 2023-09-02 22:24 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20230902072035.652549-1-oswald.buddenhagen@gmx.de>
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> ---
> v4:
> - factor out verification of subject as per taylor's patch, with minor
> modifications.
The change seems to make the test quite straight-forward to read.
Let's mark the topic for 'next'.
Thanks.
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index e2ef619323..4158590322 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -176,6 +176,29 @@ test_expect_success 'advice from failed revert' '
> test_cmp expected actual
> '
>
> +test_expect_subject () {
> + echo "$1" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +}
> +
> +test_expect_success 'titles of fresh reverts' '
> + test_commit --no-tag A file1 &&
> + test_commit --no-tag B file1 &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Revert \"B\"" &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Reapply \"B\"" &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Revert \"Reapply \"B\"\""
> +'
> +
> +test_expect_success 'title of legacy double revert' '
> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Revert \"Revert \"Revert \"B\"\"\""
> +'
> +
> test_expect_success 'identification of reverted commit (default)' '
> test_commit to-ident &&
> test_when_finished "git reset --hard to-ident" &&
^ permalink raw reply
* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-09-02 22:36 UTC (permalink / raw)
To: Wesley; +Cc: phillip.wood, git
In-Reply-To: <fa702b47-ae29-4299-9226-4920620b9fff@opperschaap.net>
Wesley <wesleys@opperschaap.net> writes:
> On 9/1/23 14:10, Junio C Hamano wrote:
>> 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?
>
> I do not know? I tested with a bare repo and two clones. And I also
> tested it with just a remote upstream in another branch.
IIRC, a non-bare repository (i.e. with working tree) should get
core.logallrefupdates set to true by default, so all your refs, not
just local and remote-tracking branches, should have records.
> I haven't force pushed anything btw, maybe that could explain things?
If your "remote" is never force-pushed, then the movements of refs
at the remote (which you will observe whenever you fetch from it)
will always fast-forward, and the remote-tracking branches in your
local repository that keeps track of the movement will also record
the fast-forwarding movement in the reflog. But then there is no
need for the fork-point heurisitics to trigger, and even if it
triggered the heuristics would not change the outcome, when rebasing
against such a remote branch, as their tip will always a decendant
of all commits that ever sat at the tip of that remote branch.
^ 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;
as well as URLs for NNTP newsgroup(s).