* What's cooking in git.git (Feb 2023, #01; Thu, 2) @ 2023-02-03 4:02 Junio C Hamano 2023-02-04 9:33 ` Sergey Organov 2023-02-08 17:22 ` ds/bundle-uri-5 (was: " Victoria Dye 0 siblings, 2 replies; 30+ messages in thread From: Junio C Hamano @ 2023-02-03 4:02 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. A topic without enough support may be discarded after a long period of no activity. 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/ -------------------------------------------------- [New Topics] * ab/imap-send-requires-curl (2023-02-02) 6 commits - imap-send: correctly report "host" when using "tunnel" - imap-send: remove old --no-curl codepath - imap-send: make --curl no-optional - imap-send: replace auto-probe libcurl with hard dependency - imap-send doc: the imap.sslVerify is used with imap.tunnel - imap-send: note "auth_method", not "host" on auth method failure Give a hard dependency on cURL library to build "git imap-send", and remove the code to interact with IMAP server without using cURL. Comments? source: <cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com> * gc/index-format-doc (2023-02-01) 1 commit - docs: document zero bits in index "mode" Doc update. Will merge to 'next'. source: <20230201024041.29401-1-chooglen@google.com> * jk/httpd-test-updates (2023-02-01) 4 commits - t/lib-httpd: increase ssl key size to 2048 bits - t/lib-httpd: drop SSLMutex config - t/lib-httpd: bump required apache version to 2.4 - t/lib-httpd: bump required apache version to 2.2 Test update. Will merge to 'next'. source: <Y9pOmR5fOfCHwYpF@coredump.intra.peff.net> * rd/doc-default-date-format (2023-02-01) 1 commit - rev-list: clarify git-log default date format Update --date=default documentation. source: <20230201155712.86577-1-rafael@dulfer.be> * ew/free-island-marks (2023-02-02) 1 commit - delta-islands: free island_marks and bitmaps "git pack-objects" learned to release delta-island bitmap data when it is done using it, saving peak heap memory usage. Will merge to 'next'. source: <20230202094217.M955476@dcvr> * hj/remove-msys-support (2023-02-02) 2 commits - mingw: remove msysGit/MSYS1 support - mingw: remove duplicate `USE_NED_ALLOCATOR` directive Remove support for MSys, which now lags way behind MSys2. Will merge to 'next'. source: <pull.1433.v2.git.1675309898.gitgitgadget@gmail.com> * sk/winansi-createthread-fix (2023-02-01) 1 commit - compat/winansi: check for errors of CreateThread() correctly Fix use of CreateThread() API call made early in the windows start-up code. Will merge to 'next'. source: <pull.1445.v4.git.git.1675290034144.gitgitgadget@gmail.com> -------------------------------------------------- [Stalled] * rj/branch-unborn-in-other-worktrees (2023-01-19) 3 commits - branch: rename orphan branches in any worktree - branch: description for orphan branch errors - avoid unnecessary worktrees traversing Error messages given when working on an unborn branch that is checked out in another worktree have been improvved. Expecting a reroll. cf. <527f7315-be7b-7ec0-04fc-d07da7d4fefa@gmail.com> source: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com> * ja/worktree-orphan (2023-01-13) 4 commits - worktree add: add hint to direct users towards --orphan - worktree add: add --orphan flag - worktree add: refactor opt exclusion tests - worktree add: include -B in usage docs 'git worktree add' learned how to create a worktree based on an orphaned branch with `--orphan`. Expecting a reroll. cf. <11be1b0e-ee38-119f-1d80-cb818946116b@dunelm.org.uk> source: <20230109173227.29264-1-jacobabel@nullpo.dev> * ab/avoid-losing-exit-codes-in-tests (2022-12-20) 6 commits - tests: don't lose misc "git" exit codes - tests: don't lose "git" exit codes in "! ( git ... | grep )" - tests: don't lose exit status with "test <op> $(git ...)" - tests: don't lose exit status with "(cd ...; test <op> $(git ...))" - t/lib-patch-mode.sh: fix ignored exit codes - auto-crlf tests: don't lose exit code in loops and outside tests Test clean-up. Expecting a hopefully minor and final reroll. cf. <1182283a-4a78-3c99-e716-a8c3e58a5823@web.de> cf. <xmqqsfhb0vum.fsf@gitster.g> source: <cover-v4-0.6-00000000000-20221219T101240Z-avarab@gmail.com> * tl/notes--blankline (2022-11-09) 5 commits - notes.c: introduce "--no-blank-line" option - notes.c: provide tips when target and append note are both empty - notes.c: drop unreachable code in 'append_edit()' - notes.c: cleanup for "designated init" and "char ptr init" - notes.c: cleanup 'strbuf_grow' call in 'append_edit' 'git notes append' was taught '--[no-]blank-line' to conditionally add a LF between a new and existing note. Expecting a reroll. cf. <CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com> source: <cover.1667980450.git.dyroneteng@gmail.com> * mc/switch-advice (2022-11-09) 1 commit - po: use `switch` over `checkout` in error message Use 'switch' instead of 'checkout' in an error message. Waiting for review response. source: <pull.1308.git.git.1668018620148.gitgitgadget@gmail.com> * js/range-diff-mbox (2022-11-23) 1 commit - range-diff: support reading mbox files 'git range-diff' gained support for reading either side from an .mbox file instead of a revision range. Waiting for review response. cf. <xmqqr0xupmnf.fsf@gitster.g> source: <pull.1420.v3.git.1669108102092.gitgitgadget@gmail.com> * ab/tag-object-type-errors (2022-11-22) 5 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 - object-file.c: free the "t.tag" in check_tag() - 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. Expecting a reroll. cf. <xmqqzgb5jz5c.fsf@gitster.g> cf. <xmqqsfgxjugi.fsf@gitster.g> source: <cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com> * ed/fsmonitor-inotify (2022-12-13) 6 commits - fsmonitor: update doc for Linux - fsmonitor: test updates - fsmonitor: enable fsmonitor for Linux - fsmonitor: implement filesystem change listener for Linux - fsmonitor: determine if filesystem is local or remote - fsmonitor: prepare to share code between Mac OS and Linux Bundled fsmonitor for Linux using inotify API. Needs review on the updated round. source: <pull.1352.v5.git.git.1670882286.gitgitgadget@gmail.com> * jc/spell-id-in-both-caps-in-message-id (2022-12-17) 1 commit - e-mail workflow: Message-ID is spelled with ID in both capital letters Consistently spell "Message-ID" as such, not "Message-Id". Needs review. source: <xmqqsfhgnmqg.fsf@gitster.g> * ad/test-record-count-when-harness-is-in-use (2022-12-25) 1 commit - test-lib: allow storing counts with test harnesses Allow summary results from tests to be written to t/test-results directory even when a test harness like 'prove' is in use. Needs review. source: <20221224225200.1027806-1-adam@dinwoodie.org> * so/diff-merges-more (2022-12-18) 5 commits - diff-merges: improve --diff-merges documentation - diff-merges: issue warning on lone '-m' option - diff-merges: support list of values for --diff-merges - diff-merges: implement log.diffMerges-m-imply-p config - diff-merges: implement [no-]hide option and log.diffMergesHide config Assorted updates to "--diff-merges=X" option. May want to discard. Breaking compatibility does not seem worth it. source: <20221217132955.108542-1-sorganov@gmail.com> -------------------------------------------------- [Cooking] * ab/config-multi-and-nonbool (2023-02-02) 9 commits - for-each-repo: with bad config, don't conflate <path> and <cmd> - config API: add "string" version of *_value_multi(), fix segfaults - config API users: test for *_get_value_multi() segfaults - for-each-repo: error on bad --config - config API: have *_multi() return an "int" and take a "dest" - versioncmp.c: refactor config reading next commit - config API: add and use a "git_config_get()" family of functions - config tests: add "NULL" tests for *_get_value_multi() - config tests: cover blind spots in git_die_config() tests Assorted config API updates. source: <cover-v4-0.9-00000000000-20230202T131155Z-avarab@gmail.com> * ds/scalar-ignore-cron-error (2023-01-27) 3 commits (merged to 'next' on 2023-01-31 at 98d13ac3e7) + scalar: only warn when background maintenance fails + t921*: test scalar behavior starting maintenance + t: allow 'scalar' in test_must_fail Allow "scalar" to warn but continue when its periodic maintenance feature cannot be enabled. Will merge to 'master'. source: <pull.1473.git.1674849963.gitgitgadget@gmail.com> * mh/doc-credential-cache-only-in-core (2023-01-29) 1 commit (merged to 'next' on 2023-01-30 at 021b5227af) + Documentation: clarify that cache forgets credentials if the system restarts Documentation clarification. Will merge to 'master'. source: <pull.1447.v3.git.1674936815117.gitgitgadget@gmail.com> * ab/hook-api-with-stdin (2023-01-23) 5 commits - hook: support a --to-stdin=<path> option for testing - sequencer: use the new hook API for the simpler "post-rewrite" call - hook API: support passing stdin to hooks, convert am's 'post-rewrite' - run-command: allow stdin for run_processes_parallel - run-command.c: remove dead assignment in while-loop Extend the run-hooks API to allow feeding data from the standard input when running the hook script(s). Expecting review responses. source: <cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com> * as/ssh-signing-improve-key-missing-error (2023-01-25) 1 commit (merged to 'next' on 2023-01-25 at 140f2c2c60) + ssh signing: better error message when key not in agent Improve the error message given when private key is not loaded in the ssh agent in the codepath to sign with an ssh key. Will merge to 'master'. source: <pull.1270.v3.git.git.1674650450662.gitgitgadget@gmail.com> * en/rebase-incompatible-opts (2023-01-25) 10 commits (merged to 'next' on 2023-01-27 at 35a67cf2c6) + rebase: provide better error message for apply options vs. merge config + rebase: put rebase_options initialization in single place + rebase: fix formatting of rebase --reapply-cherry-picks option in docs + rebase: clarify the OPT_CMDMODE incompatibilities + rebase: add coverage of other incompatible options + rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks + rebase: fix docs about incompatibilities with --root + rebase: remove --allow-empty-message from incompatible opts + rebase: flag --apply and --merge as incompatible + rebase: mark --update-refs as requiring the merge backend "git rebase" often ignored incompatible options instead of complaining, which has been corrected. Will merge to 'master'. Replaces en/rebase-update-refs-needs-merge-backend. source: <pull.1466.v5.git.1674619434.gitgitgadget@gmail.com> * gm/request-pull-with-non-pgp-signed-tags (2023-01-25) 1 commit (merged to 'next' on 2023-01-30 at abc684d8df) + request-pull: filter out SSH/X.509 tag signatures Adjust "git request-pull" to strip embedded signature from signed tags to notice non-PGP signatures. Will merge to 'master'. source: <20230125234725.3918563-1-gwymor@tilde.club> * cb/grep-fallback-failing-jit (2023-01-31) 1 commit - grep: fall back to interpreter if JIT memory allocation fails In an environment where dynamically generated code is prohibited to run (e.g. SELinux), failure to JIT pcre patterns is expected. Fall back to interpreted execution in such a case. Will merge to 'next'? source: <20230131185611.520311-1-minipli@grsecurity.net> * cb/checkout-same-branch-twice (2023-01-20) 1 commit - checkout/switch: disallow checking out same branch in multiple worktrees "git checkout -B $branch" failed to protect against checking out a branch that is checked out elsewhere, unlike "git branch -f" did. Expecting a hopefully minor and final reroll. cf. <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk> source: <20230120113553.24655-1-carenas@gmail.com> * ab/sequencer-unleak (2023-01-18) 8 commits - commit.c: free() revs.commit in get_fork_point() - builtin/rebase.c: free() "options.strategy_opts" - sequencer.c: always free() the "msgbuf" in do_pick_commit() - builtin/rebase.c: fix "options.onto_name" leak - builtin/revert.c: move free-ing of "revs" to replay_opts_release() - rebase & sequencer API: fix get_replay_opts() leak in "rebase" - sequencer.c: split up sequencer_remove_state() - rebase: use "cleanup" pattern in do_interactive_rebase() Plug leaks in sequencer subsystem and its users. Expecting a hopefully minor and final reroll. cf. <xmqqedry17r4.fsf@gitster.g> source: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com> * jc/attr-doc-fix (2023-01-26) 1 commit (merged to 'next' on 2023-01-26 at cb327c4b5f) + attr: fix instructions on how to check attrs Comment fix. Will merge to 'master'. source: <pull.1441.v3.git.git.1674768107941.gitgitgadget@gmail.com> * rj/avoid-switching-to-already-used-branch (2023-01-22) 3 commits - switch: reject if the branch is already checked out elsewhere (test) - rebase: refuse to switch to a branch already checked out elsewhere (test) - branch: fix die_if_checked_out() when ignore_current_worktree A few subcommands have been taught to stop users from working on a branch that is being used in another worktree linked to the same repository. Expecting a hopefully minor and final reroll. cf. <d61a2393-64c8-da49-fe13-00bc4a52d5e3@gmail.com> source: <f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com> * rj/bisect-already-used-branch (2023-01-22) 1 commit - bisect: fix "reset" when branch is checked out elsewhere Allow "git bisect reset [name]" to check out the named branch (or the original one) even when the branch is already checked out in a different worktree linked to the same repository. Leaning negative. Why is it a good thing? cf. <xmqqo7qqovp1.fsf@gitster.g> source: <1c36c334-9f10-3859-c92f-3d889e226769@gmail.com> * en/ls-files-doc-update (2023-01-13) 4 commits (merged to 'next' on 2023-01-27 at 20b9803add) + ls-files: guide folks to --exclude-standard over other --exclude* options + ls-files: clarify descriptions of status tags for -t + ls-files: clarify descriptions of file selection options + ls-files: add missing documentation for --resolve-undo option Doc update to ls-files. Will merge to 'master'. source: <pull.1463.git.1673584914.gitgitgadget@gmail.com> * ms/send-email-feed-header-to-validate-hook (2023-01-19) 2 commits - send-email: expose header information to git-send-email's sendemail-validate hook - send-email: refactor header generation functions "git send-email" learned to give the e-mail headers to the validate hook by passing an extra argument from the command line. Expecting a hopefully minor and final reroll. cf. <c1ba0a28-3c39-b313-2757-dceb02930334@amd.com> source: <20230120012459.920932-1-michael.strawbridge@amd.com> * ds/bundle-uri-5 (2023-01-31) 11 commits - bundle-uri: test missing bundles with heuristic - bundle-uri: store fetch.bundleCreationToken - fetch: fetch from an external bundle URI - bundle-uri: drop bundle.flag from design doc - clone: set fetch.bundleURI if appropriate - bundle-uri: download in creationToken order - bundle-uri: parse bundle.<id>.creationToken values - bundle-uri: parse bundle.heuristic=creationToken - t5558: add tests for creationToken heuristic - bundle: verify using check_connected() - bundle: test unbundling with incomplete history The bundle-URI subsystem adds support for creation-token heuristics to help incremental fetches. Will merge to 'next'? source: <pull.1454.v3.git.1675171759.gitgitgadget@gmail.com> * tc/cat-file-z-use-cquote (2023-01-16) 1 commit - cat-file: quote-format name in error when using -z "cat-file" in the batch mode that is fed NUL-terminated pathnames learned to cquote them in its error output (otherwise, a funny pathname with LF in it would break the lines in the output stream). Expecting a reroll. cf. <2a2a46f0-a9bc-06a6-72e1-28800518777c@dunelm.org.uk> source: <20230116190749.4141516-1-toon@iotcl.com> * cw/submodule-status-in-parallel (2023-01-17) 6 commits - submodule: call parallel code from serial status - diff-lib: parallelize run_diff_files for submodules - diff-lib: refactor match_stat_with_submodule - submodule: move status parsing into function - submodule: strbuf variable rename - run-command: add duplicate_output_fn to run_processes_parallel_opts "git submodule status" learned to run the comparison in submodule repositories in parallel. Expecting a reroll. cf. <CAFySSZBiW7=ZTmXRaLzCoKUi0Jd=fzvW5PJ6=Ka0jKHoP2ddSw@mail.gmail.com> cf. <kl6lo7qlvg4h.fsf@chooglen-macbookpro.roam.corp.google.com> source: <20230104215415.1083526-1-calvinwan@google.com> * ab/various-leak-fixes (2023-02-02) 19 commits - push: free_refs() the "local_refs" in set_refspecs() - push: refactor refspec_append_mapped() for subsequent leak-fix - receive-pack: free() the "ref_name" in "struct command" - grep API: plug memory leaks by freeing "header_list" - grep.c: refactor free_grep_patterns() - builtin/merge.c: free "&buf" on "Your local changes..." error - builtin/merge.c: use fixed strings, not "strbuf", fix leak - show-branch: free() allocated "head" before return - commit-graph: fix a parse_options_concat() leak - http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}() - http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main() - worktree: fix a trivial leak in prune_worktrees() - repack: fix leaks on error with "goto cleanup" - name-rev: don't xstrdup() an already dup'd string - various: add missing clear_pathspec(), fix leaks - clone: use free() instead of UNLEAK() - commit-graph: use free_commit_graph() instead of UNLEAK() - bundle.c: don't leak the "args" in the "struct child_process" - tests: mark tests as passing with SANITIZE=leak Leak fixes. Comments? source: <cover-v6-00.19-00000000000-20230202T094704Z-avarab@gmail.com> -------------------------------------------------- [Discarded] * mc/credential-helper-auth-headers (2023-01-20) 12 commits (merged to 'next' on 2023-01-25 at cb95006bb2) + credential: add WWW-Authenticate header to cred requests + http: read HTTP WWW-Authenticate response headers + http: replace unsafe size_t multiplication with st_mult + test-http-server: add sending of arbitrary headers + test-http-server: add simple authentication + test-http-server: pass Git requests to http-backend + test-http-server: add HTTP request parsing + test-http-server: add HTTP error response function + test-http-server: add stub HTTP server test helper + daemon: rename some esoteric/laboured terminology + daemon: libify child process handling functions + daemon: libify socket setup and option functions Extending credential helper protocol. The test-only server is an eyesore. cf. <e57c1ca3-c21c-db41-a386-e5887f46055c@github.com> cf. <Y9JkMLueCwjkLHOr@coredump.intra.peff.net> source: <pull.1352.v7.git.1674252530.gitgitgadget@gmail.com> ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-02-03 4:02 What's cooking in git.git (Feb 2023, #01; Thu, 2) Junio C Hamano @ 2023-02-04 9:33 ` Sergey Organov 2023-02-06 18:35 ` Junio C Hamano 2023-02-07 4:06 ` so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) Glen Choo 2023-02-08 17:22 ` ds/bundle-uri-5 (was: " Victoria Dye 1 sibling, 2 replies; 30+ messages in thread From: Sergey Organov @ 2023-02-04 9:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > * so/diff-merges-more (2022-12-18) 5 commits > - diff-merges: improve --diff-merges documentation > - diff-merges: issue warning on lone '-m' option > - diff-merges: support list of values for --diff-merges > - diff-merges: implement log.diffMerges-m-imply-p config > - diff-merges: implement [no-]hide option and log.diffMergesHide config > > Assorted updates to "--diff-merges=X" option. > > May want to discard. Breaking compatibility does not seem worth it. > source: <20221217132955.108542-1-sorganov@gmail.com> Hi Junio, This does not break any compatibility, as far as me and I believe reviewers of these series are aware. Please either merge this or explain what do you mean in more details. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-02-04 9:33 ` Sergey Organov @ 2023-02-06 18:35 ` Junio C Hamano 2023-02-06 21:35 ` Sergey Organov 2023-02-07 4:06 ` so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) Glen Choo 1 sibling, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-02-06 18:35 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > > >> * so/diff-merges-more (2022-12-18) 5 commits >> - diff-merges: improve --diff-merges documentation >> - diff-merges: issue warning on lone '-m' option >> - diff-merges: support list of values for --diff-merges >> - diff-merges: implement log.diffMerges-m-imply-p config >> - diff-merges: implement [no-]hide option and log.diffMergesHide config >> >> Assorted updates to "--diff-merges=X" option. >> >> May want to discard. Breaking compatibility does not seem worth it. >> source: <20221217132955.108542-1-sorganov@gmail.com> > > Hi Junio, > > This does not break any compatibility, as far as me and I believe > reviewers of these series are aware. The last paragraphs in the review two months ago still describe what this series does fairly accurately, I think. These patches do look like a good approach to solve the first point among the "two problems" in the previous round. Thanks for working on it. IIRC, the previous round (why is this round marked as v1, by the way?) was reviewed by some folks, so lets wait to hear from them how this round does better. Unfortunately, I do not think of any "solution" that would avoid breaking folks, if its end goal is to flip the default, either by hardcoding or with a configuration variable. IOW, the other one among the "two problems" in the previous round sounds unsolvable. We should question if it was really an "issue" worth "resolving", though. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-02-06 18:35 ` Junio C Hamano @ 2023-02-06 21:35 ` Sergey Organov 2023-03-01 18:40 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-02-06 21:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >> >>> * so/diff-merges-more (2022-12-18) 5 commits >>> - diff-merges: improve --diff-merges documentation >>> - diff-merges: issue warning on lone '-m' option >>> - diff-merges: support list of values for --diff-merges >>> - diff-merges: implement log.diffMerges-m-imply-p config >>> - diff-merges: implement [no-]hide option and log.diffMergesHide config >>> >>> Assorted updates to "--diff-merges=X" option. >>> >>> May want to discard. Breaking compatibility does not seem worth it. >>> source: <20221217132955.108542-1-sorganov@gmail.com> >> >> Hi Junio, >> >> This does not break any compatibility, as far as me and I believe >> reviewers of these series are aware. > > The last paragraphs in the review two months ago still describe what > this series does fairly accurately, I think. > > These patches do look like a good approach to solve the first point > among the "two problems" in the previous round. Thanks for working > on it. > > IIRC, the previous round (why is this round marked as v1, by the > way?) was reviewed by some folks, so lets wait to hear from them > how this round does better. > > Unfortunately, I do not think of any "solution" that would avoid > breaking folks, if its end goal is to flip the default, either by > hardcoding or with a configuration variable. IOW, the other one > among the "two problems" in the previous round sounds unsolvable. > We should question if it was really an "issue" worth "resolving", > though. Well, we may end up flipping or not flipping the default (even though I'd prefer we indeed do), the series are still valid either way, as they allow *me* (or anybody else who prefers more useful '-m' behavior) to flip the switch for myself, locally. Also, the only patch that got some resistance from reviewers has been removed from the series, so I don't see anything left that'd prevent this from being merged. From my POV the only remotely questionable patch is: - diff-merges: issue warning on lone '-m' option and I hereby agree to remove it if it feels wrong to you. Let me state it cleanly: once these are accepted, I'll turn log.diffMerges-m-imply-p on for myself, and will suggest it to others who already asked about '-m' inconsistency, or will ask in the future. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-02-06 21:35 ` Sergey Organov @ 2023-03-01 18:40 ` Sergey Organov 2023-03-01 22:15 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-03-01 18:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Hi, Junio! Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Junio C Hamano <gitster@pobox.com> writes: >>> >>> >>>> * so/diff-merges-more (2022-12-18) 5 commits >>>> - diff-merges: improve --diff-merges documentation >>>> - diff-merges: issue warning on lone '-m' option >>>> - diff-merges: support list of values for --diff-merges >>>> - diff-merges: implement log.diffMerges-m-imply-p config >>>> - diff-merges: implement [no-]hide option and log.diffMergesHide config >>>> >>>> Assorted updates to "--diff-merges=X" option. >>>> >>>> May want to discard. Breaking compatibility does not seem worth it. >>>> source: <20221217132955.108542-1-sorganov@gmail.com> >>> >>> Hi Junio, >>> >>> This does not break any compatibility, as far as me and I believe >>> reviewers of these series are aware. >> >> The last paragraphs in the review two months ago still describe what >> this series does fairly accurately, I think. >> >> These patches do look like a good approach to solve the first point >> among the "two problems" in the previous round. Thanks for working >> on it. >> >> IIRC, the previous round (why is this round marked as v1, by the >> way?) was reviewed by some folks, so lets wait to hear from them >> how this round does better. >> >> Unfortunately, I do not think of any "solution" that would avoid >> breaking folks, if its end goal is to flip the default, either by >> hardcoding or with a configuration variable. IOW, the other one >> among the "two problems" in the previous round sounds unsolvable. >> We should question if it was really an "issue" worth "resolving", >> though. > > Well, we may end up flipping or not flipping the default (even though > I'd prefer we indeed do), the series are still valid either way, as they > allow *me* (or anybody else who prefers more useful '-m' behavior) to > flip the switch for myself, locally. > > Also, the only patch that got some resistance from reviewers has been > removed from the series, so I don't see anything left that'd prevent > this from being merged. > > From my POV the only remotely questionable patch is: > > - diff-merges: issue warning on lone '-m' option > > and I hereby agree to remove it if it feels wrong to you. > > Let me state it cleanly: once these are accepted, I'll turn > log.diffMerges-m-imply-p on for myself, and will suggest it to others > who already asked about '-m' inconsistency, or will ask in the future. This is still marked as "probably won't merge" in the recent "what's cooking". Could it be merged, please? Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-03-01 18:40 ` Sergey Organov @ 2023-03-01 22:15 ` Junio C Hamano 2023-03-01 22:26 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-01 22:15 UTC (permalink / raw) To: Sergey Organov; +Cc: git, Glen Choo Sergey Organov <sorganov@gmail.com> writes: >>>>> May want to discard. Breaking compatibility does not seem worth it. >>>>> source: <20221217132955.108542-1-sorganov@gmail.com> >>>> ... > This is still marked as "probably won't merge" in the recent "what's > cooking". Could it be merged, please? I found the explanation given by Glen last time you mentioned the topic much better written than anything I would write myself, and I haven't seen any new input in the message I am responding to, so... https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/ Thanks. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-03-01 22:15 ` Junio C Hamano @ 2023-03-01 22:26 ` Sergey Organov 2023-03-01 23:54 ` Glen Choo 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-03-01 22:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Glen Choo Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>>>>> May want to discard. Breaking compatibility does not seem worth it. >>>>>> source: <20221217132955.108542-1-sorganov@gmail.com> >>>>> ... >> This is still marked as "probably won't merge" in the recent "what's >> cooking". Could it be merged, please? > > I found the explanation given by Glen last time you mentioned the > topic much better written than anything I would write myself, and I > haven't seen any new input in the message I am responding to, so... > > https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/ I believe I've addressed it with this: https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/ and seen no follow-up since then. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-03-01 22:26 ` Sergey Organov @ 2023-03-01 23:54 ` Glen Choo 2023-03-02 14:38 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Glen Choo @ 2023-03-01 23:54 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: git Hi Sergey! Sergey Organov <sorganov@gmail.com> writes: >>> This is still marked as "probably won't merge" in the recent "what's >>> cooking". Could it be merged, please? >> >> I found the explanation given by Glen last time you mentioned the >> topic much better written than anything I would write myself, and I >> haven't seen any new input in the message I am responding to, so... >> >> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/ > > I believe I've addressed it with this: > > https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/ > > and seen no follow-up since then. Sorry for the lack of response, especially if it seemed like your concerns were not listened to. I did try crafting a response when it was first written, but it didn't end up adding anything new and valuable to the conversation, so I dropped it. I'll leave an updated response, but unfortunately, I am still of the opinion that we shouldn't merge this series. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: What's cooking in git.git (Feb 2023, #01; Thu, 2) 2023-03-01 23:54 ` Glen Choo @ 2023-03-02 14:38 ` Sergey Organov 0 siblings, 0 replies; 30+ messages in thread From: Sergey Organov @ 2023-03-02 14:38 UTC (permalink / raw) To: Glen Choo; +Cc: Junio C Hamano, git Glen Choo <chooglen@google.com> writes: > Hi Sergey! > > Sergey Organov <sorganov@gmail.com> writes: > >>>> This is still marked as "probably won't merge" in the recent "what's >>>> cooking". Could it be merged, please? >>> >>> I found the explanation given by Glen last time you mentioned the >>> topic much better written than anything I would write myself, and I >>> haven't seen any new input in the message I am responding to, so... >>> >>> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/ >> >> I believe I've addressed it with this: >> >> https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/ >> >> and seen no follow-up since then. > > Sorry for the lack of response, especially if it seemed like your > concerns were not listened to. I did try crafting a response when it was > first written, but it didn't end up adding anything new and valuable to > the conversation, so I dropped it. > > I'll leave an updated response, but unfortunately, I am still of the > opinion that we shouldn't merge this series. The primary question here is: why don't allow me to get the behavior I need, provided it has no impact on anybody else, unless they decide they need it too? I've already performed "you need it, -- do it yourself" dance, along with documentation and tests, so may I be rewarded by getting this feature finally in, please? Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-02-04 9:33 ` Sergey Organov 2023-02-06 18:35 ` Junio C Hamano @ 2023-02-07 4:06 ` Glen Choo 2023-02-07 12:50 ` Sergey Organov 1 sibling, 1 reply; 30+ messages in thread From: Glen Choo @ 2023-02-07 4:06 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: git Hi Sergey, Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > > >> * so/diff-merges-more (2022-12-18) 5 commits >> - diff-merges: improve --diff-merges documentation >> - diff-merges: issue warning on lone '-m' option >> - diff-merges: support list of values for --diff-merges >> - diff-merges: implement log.diffMerges-m-imply-p config >> - diff-merges: implement [no-]hide option and log.diffMergesHide config >> >> Assorted updates to "--diff-merges=X" option. >> >> May want to discard. Breaking compatibility does not seem worth it. >> source: <20221217132955.108542-1-sorganov@gmail.com> > > Hi Junio, > > This does not break any compatibility, as far as me and I believe > reviewers of these series are aware. My 2 cents (which maybe lines up with what Junio meant) is that this series doesn't break compatibility on its own, but IMO the approach doesn't make sense unless we also intend to flip the default somewhere down the line. "log.diffMerges-m-imply-p" is a really specific config option, and it needs to justify its addition with an outsized benefit. I don't think it meets that bar, the only use cases I can think of are: - Before we change the default, setting "log.diffMerges-m-imply-p=true" gives the 'nicer' behavior. But if the user had the permissions and knowledge to do so, they could just pass "-p" instead for correctness. If the goal is to reduce typing, then we could add a different CLI flag that would behave like "-m -p", or we could teach "git diff" to accept proper single-character flags. Either of these options would be more discoverable and cleaner. - After we change the default, setting "log.diffMerges-m-imply-p=false" gives the old behavior, which might be needed if the user is running scripts written in for an older version of Git. I would find this compelling, but as Junio mentioned [1], breaking compatibility doesn't seem worth it, so this point is moot. On the other hand, neither of the alternative approaches break compatibility, so they're superior in this regard too. - The only legitimate use case I think of is something like a script that uses "-m" assuming that it implied "-p", AND the user has no ability to modify the script. Then the user's only recourse is to set a config. But this is so exotic that I don't think this is a good enough reason to have the config. I wouldn't mind seeing a version of these patches without "log.diffMerges-m-imply-p=false" , but in its current form, I agree with Junio that this isn't worth it. [1] https://lore.kernel.org/git/xmqqbko1xv86.fsf@gitster.g/ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-02-07 4:06 ` so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) Glen Choo @ 2023-02-07 12:50 ` Sergey Organov 2023-03-02 0:37 ` Glen Choo 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-02-07 12:50 UTC (permalink / raw) To: Glen Choo; +Cc: Junio C Hamano, git Glen Choo <chooglen@google.com> writes: > Hi Sergey, > > Sergey Organov <sorganov@gmail.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >> >>> * so/diff-merges-more (2022-12-18) 5 commits >>> - diff-merges: improve --diff-merges documentation >>> - diff-merges: issue warning on lone '-m' option >>> - diff-merges: support list of values for --diff-merges >>> - diff-merges: implement log.diffMerges-m-imply-p config >>> - diff-merges: implement [no-]hide option and log.diffMergesHide config >>> >>> Assorted updates to "--diff-merges=X" option. >>> >>> May want to discard. Breaking compatibility does not seem worth it. >>> source: <20221217132955.108542-1-sorganov@gmail.com> >> >> Hi Junio, >> >> This does not break any compatibility, as far as me and I believe >> reviewers of these series are aware. > > My 2 cents (which maybe lines up with what Junio meant) is that this > series doesn't break compatibility on its own, but IMO the approach > doesn't make sense unless we also intend to flip the default somewhere > down the line. "log.diffMerges-m-imply-p" is a really specific config > option, and it needs to justify its addition with an outsized benefit. > > I don't think it meets that bar, the only use cases I can think of are: > > - Before we change the default, setting "log.diffMerges-m-imply-p=true" > gives the 'nicer' behavior. But if the user had the permissions and > knowledge to do so, they could just pass "-p" instead for > correctness. Except I can't configure this in centralized manner for multiple users on our host machine? > > If the goal is to reduce typing, then we could add a different CLI > flag that would behave like "-m -p", or we could teach "git diff" to > accept proper single-character flags. Either of these options would be > more discoverable and cleaner. The only drawback of this is that this leaves "-m" inconsistent with no apparent reason. OTOH, teaching "git log" to use common options machinery to accept "-pm" looks to be quite a project, and doesn't fix '-m' inconsistency anyway. Then, out of curiosity, what do you think was the reason to change "--cc" behavior to match that of "-c"? To save typing? To me, changing "-m" accordingly is an improvement to the overall feel of git interface, the same way as changing "--cc" was. That said, if we decide to go for new option, I'd suggest to add "-d", and probably obsolete "-m". This does not look to me as appealing as finally fixing "-m" though. > > - After we change the default, setting "log.diffMerges-m-imply-p=false" > gives the old behavior, which might be needed if the user is running > scripts written in for an older version of Git. I would find this > compelling, but as Junio mentioned [1], breaking compatibility doesn't > seem worth it, so this point is moot. On the other hand, neither of > the alternative approaches break compatibility, so they're superior in > this regard too. The only incompatibility found was obsolete use of "--first-parent -m", that was the only use-case for "-m" for a long time, and this could probably be detected and handled specially, though it sounds like a nasty kludge. > > - The only legitimate use case I think of is something like a script > that uses "-m" assuming that it implied "-p", AND the user has no > ability to modify the script. Then the user's only recourse is to set > a config. But this is so exotic that I don't think this is a good > enough reason to have the config. Yes, let's just change "-m" behavior then, and let exotic scripts just drop "-m", as it's not needed for a long time already in the combo "--first-parent -m" that currently reads "--first-parent". > > I wouldn't mind seeing a version of these patches without > "log.diffMerges-m-imply-p=false" , but in its current form, I agree with > Junio that this isn't worth it. > > [1] https://lore.kernel.org/git/xmqqbko1xv86.fsf@gitster.g/ All you say is understood and are valid arguments, but then we will continue to face pretty valid confusion of why "-m" behaves differently from "-c/--cc/--remerge". I personally don't care, provided I get a way to make it behave sanely, and that's what "log.diffMerges-m-imply-p" patch does. As a kind of complaint, it was simple 1 patch from Junio once upon a time to change "--cc" to a sane behavior of implying -p, and now it takes rounds and rounds to do the same for -m. This is rather sad. Finally, event without "log.diffMerges-m-imply-p", the rest of the series still makes sense, so if the conclusion is to remove it, let's still merge the rest, please! Let me know, and I'll then remove the patch and will change documentation accordingly. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-02-07 12:50 ` Sergey Organov @ 2023-03-02 0:37 ` Glen Choo 2023-03-02 16:15 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Glen Choo @ 2023-03-02 0:37 UTC (permalink / raw) To: Sergey Organov; +Cc: Junio C Hamano, git Sergey Organov <sorganov@gmail.com> writes: >> If the goal is to reduce typing, then we could add a different CLI >> flag that would behave like "-m -p", or we could teach "git diff" to >> accept proper single-character flags. Either of these options would be >> more discoverable and cleaner. > > The only drawback of this is that this leaves "-m" inconsistent with no > apparent reason. > [...] > Then, out of curiosity, what do you think was the reason to change > "--cc" behavior to match that of "-c"? To save typing? To me, changing > "-m" accordingly is an improvement to the overall feel of git interface, > the same way as changing "--cc" was. FWIW, I agree that having an inconsistent "-m" is quite an eyesore, and there isn't a particularly good reason for it. The unfortunate reality of Hyrum's law is that some 'mistakes' or CLI cleanups are difficult or even downright impossible to fix. I'd consider "-m on its own does nothing" one of those. > All you say is understood and are valid arguments, but then we will > continue to face pretty valid confusion of why "-m" behaves differently > from "-c/--cc/--remerge". I personally don't care, provided I get a way > to make it behave sanely, and that's what "log.diffMerges-m-imply-p" > patch does. We could say, e.g.: "-m" is inconsistent, but we can't change it for backwards compatibility reasons. Please do not use it, use "-d" instead, because that is more sensible. > As a kind of complaint, it was simple 1 patch from Junio once upon a > time to change "--cc" to a sane behavior of implying -p, and now it > takes rounds and rounds to do the same for -m. This is rather sad. It is not because Junio has special privileges or anything of the sort, though. My understanding is that it was just an accident that "-m" _happened_ to be misused by enough people that we deemed that breakage unacceptable, whereas "--cc" just _happened_ to be fixable. FWIW, I might have also been on your side when we rolled back the first "-m" implies "-p" change, but the consensus since then is that breaking backwards compatibility just isn't worth it in this case, and it would take a herculean effort to change that consensus. It might not be completely impossible though, more on that later... > Finally, event without "log.diffMerges-m-imply-p", the rest of the > series still makes sense, so if the conclusion is to remove it, let's > still merge the rest, please! Let me know, and I'll then remove the > patch and will change documentation accordingly. Oops. Sorry, I missed this the first time I read this message. This alone should have warranted a response. I'm not convinced that the series makes sense without "log.diffMerges-m-imply-p": * The main patch is diff-merges: implement [no-]hide option and log.diffMergesHide config which gives us a way to redefine "-m" as "--diff-merges=hide --diff-merges=on". However, we haven't seen any compelling reasons to use --diff-merges=hide [1]. I'm also fairly convinced that if we go back in time, "-m" wouldn't have the semantics of 'do nothing unless -p is also given', and I don't think we should immortalize a mistake by giving it an explicit option. All the other patches in their current form are dependent on this patch going in. * diff-merges: support list of values for --diff-merges This only makes sense if we want to accept multiple values, which we don't without the main patch. * diff-merges: issue warning on lone '-m' option diff-merges: improve --diff-merges documentation These are docs and UX cleanups as a result of the main patch. _Maybe_ the number of lone "-m" users will shrink over time to the point where we could make the switch? We could prepare for that by warning that lone "-m" is a no-op now, but might imply "-p" at some point in the future, and then after X amount of time, we might say that users have had enough warning and actually change the default. I find this unlikely, but not impossible. However, the patches don't prepare us for this path, and that would deserve new patches and a different discussion from what we've been having. 1. https://lore.kernel.org/git/kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com/ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-02 0:37 ` Glen Choo @ 2023-03-02 16:15 ` Junio C Hamano 2023-03-02 16:57 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-02 16:15 UTC (permalink / raw) To: Glen Choo; +Cc: Sergey Organov, git Glen Choo <chooglen@google.com> writes: >> Finally, event without "log.diffMerges-m-imply-p", the rest of the >> series still makes sense, so if the conclusion is to remove it, let's >> still merge the rest, please! Let me know, and I'll then remove the >> patch and will change documentation accordingly. > > Oops. Sorry, I missed this the first time I read this message. This > alone should have warranted a response. Hmph. I agreed with you that the last step to add the configuration would not make sense unless we are planning to break users later, but I have been under the impression that the remainder were OK clean-ups. Perhaps it is mostly because I read them so long ago and forgot the details X-<. > I'm not convinced that the series makes sense without > "log.diffMerges-m-imply-p": > > * The main patch is > > diff-merges: implement [no-]hide option and log.diffMergesHide config > > which gives us a way to redefine "-m" as "--diff-merges=hide > --diff-merges=on". However, we haven't seen any compelling reasons to > use --diff-merges=hide [1]. I'm also fairly convinced that if we go > back in time, "-m" wouldn't have the semantics of 'do nothing unless > -p is also given', and I don't think we should immortalize a mistake > by giving it an explicit option. > > All the other patches in their current form are dependent on this > patch going in. > > * diff-merges: support list of values for --diff-merges > > This only makes sense if we want to accept multiple values, which we > don't without the main patch. Now you mention it (and show example in the previous bullet point), I have to agree that we would not want this, not because we do not want to have --diff-merges with multiple values, but because it introduces an odd-man-out option that is not "last one wins" that is not used anywhere else in the UI. No response does not necessarily mean an agreement, but it indeed helped in this case to be explicit. Thanks for the clarification. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-02 16:15 ` Junio C Hamano @ 2023-03-02 16:57 ` Sergey Organov 2023-03-06 22:22 ` Glen Choo 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-03-02 16:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >>> Finally, event without "log.diffMerges-m-imply-p", the rest of the >>> series still makes sense, so if the conclusion is to remove it, let's >>> still merge the rest, please! Let me know, and I'll then remove the >>> patch and will change documentation accordingly. >> >> Oops. Sorry, I missed this the first time I read this message. This >> alone should have warranted a response. > > Hmph. I agreed with you that the last step to add the configuration > would not make sense unless we are planning to break users later, It does make sense to me, and to anybody who does want -m to behave the same sane way the rest of similar options behave. I've already got it you don't care, but there are people here who do. > but I have been under the impression that the remainder were OK > clean-ups. Perhaps it is mostly because I read them so long ago and > forgot the details X-<. It's not a cleanup, it's rather adding missed feature that allows to get precise '-m' behavior with --diff-merges. To remind you, there was a discussion back then when --diff-merges= options were introduced, do they need to immediately cause output of diffs for merge commits, or suppress the output till '-p' is specified as well, like '--cc' originally did, and like '-m' still does. The conclusion at that time was that it makes sense to take immediate action as this allows to output diffs for merge commits only, a new opportunity that was not present before. However, by making such decision we lost ability to provide exact behavior of -m using --diff-merges= set of options. This has been pointed out later to me in the list, and felt obliged to finish implementation by providing the feature. --diff-merges=hide is exactly that. > >> I'm not convinced that the series makes sense without >> "log.diffMerges-m-imply-p": >> >> * The main patch is >> >> diff-merges: implement [no-]hide option and log.diffMergesHide config >> >> which gives us a way to redefine "-m" as "--diff-merges=hide >> --diff-merges=on". However, we haven't seen any compelling reasons to >> use --diff-merges=hide [1]. I think --diff-merges should be complete and be able to provide exact "-m" behavior, rendering the latter pure shortcut. Complete orthogonal interfaces are good thing by themselves, and useful applications of them are often found later. That's common knowledge. >> I'm also fairly convinced that if we go >> back in time, "-m" wouldn't have the semantics of 'do nothing unless >> -p is also given', and I don't think we should immortalize a mistake >> by giving it an explicit option. Yep, and I provided a config option that fixes this mistake. What's wrong about it? The complete orthogonal interface finalized by the aforementioned feature allows me to achieve this goal easily. >> >> All the other patches in their current form are dependent on this >> patch going in. >> >> * diff-merges: support list of values for --diff-merges >> >> This only makes sense if we want to accept multiple values, which we >> don't without the main patch. > > Now you mention it (and show example in the previous bullet point), > I have to agree that we would not want this, not because we do not > want to have --diff-merges with multiple values, but because it > introduces an odd-man-out option that is not "last one wins" that is > not used anywhere else in the UI. It's still the last one wins, and I believe I've carefully described it in the documentation of the options. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-02 16:57 ` Sergey Organov @ 2023-03-06 22:22 ` Glen Choo 2023-03-07 10:02 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Glen Choo @ 2023-03-06 22:22 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: git Sergey Organov <sorganov@gmail.com> writes: >> but I have been under the impression that the remainder were OK >> clean-ups. Perhaps it is mostly because I read them so long ago and >> forgot the details X-<. > > It's not a cleanup, it's rather adding missed feature that allows to get > precise '-m' behavior with --diff-merges. > ... > However, by making such decision we lost ability to provide exact > behavior of -m using --diff-merges= set of options. This has been > pointed out later to me in the list, and felt obliged to finish > implementation by providing the feature. > > --diff-merges=hide > > is exactly that. This is exactly the "immortaliz[ing] a mistake" that I mentioned upthread, by turning a UX wart (-m doesn't imply -p) that neither of us likes into a feature. I'd be in favor of getting rid of the wart altogether (i.e. let's find a path that lets us make -m imply -p in the future), but not rebranding it as a feature. >>> I'm not convinced that the series makes sense without >>> "log.diffMerges-m-imply-p": >>> >>> * The main patch is >>> >>> diff-merges: implement [no-]hide option and log.diffMergesHide config >>> >>> which gives us a way to redefine "-m" as "--diff-merges=hide >>> --diff-merges=on". However, we haven't seen any compelling reasons to >>> use --diff-merges=hide [1]. > > I think --diff-merges should be complete and be able to provide exact "-m" > behavior, rendering the latter pure shortcut. > > Complete orthogonal interfaces are good thing by themselves, and useful > applications of them are often found later. That's common knowledge. For the user who wants it, yes, but these interfaces come with a maintenance cost and a cost to the user who just wants the simpler interface. I think that our goals here are actually the same: we both want Git to be as simple and usable as possible. We disagree on whether this series makes Git simpler or more complex (and I don't think either of us will change our minds any time soon), but I think we can agree on an overall direction. With that in mind, I'd be willing to discuss other approaches that get us closer to "-m implies -p", if you have any. >>> I'm also fairly convinced that if we go >>> back in time, "-m" wouldn't have the semantics of 'do nothing unless >>> -p is also given', and I don't think we should immortalize a mistake >>> by giving it an explicit option. > > Yep, and I provided a config option that fixes this mistake. What's > wrong about it? The complete orthogonal interface finalized by the > aforementioned feature allows me to achieve this goal easily. Addressed above. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-06 22:22 ` Glen Choo @ 2023-03-07 10:02 ` Sergey Organov 2023-03-07 17:54 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-03-07 10:02 UTC (permalink / raw) To: Glen Choo; +Cc: Junio C Hamano, git Glen Choo <chooglen@google.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>> but I have been under the impression that the remainder were OK >>> clean-ups. Perhaps it is mostly because I read them so long ago and >>> forgot the details X-<. >> >> It's not a cleanup, it's rather adding missed feature that allows to get >> precise '-m' behavior with --diff-merges. >> ... >> However, by making such decision we lost ability to provide exact >> behavior of -m using --diff-merges= set of options. This has been >> pointed out later to me in the list, and felt obliged to finish >> implementation by providing the feature. >> >> --diff-merges=hide >> >> is exactly that. > > This is exactly the "immortaliz[ing] a mistake" that I mentioned > upthread, by turning a UX wart (-m doesn't imply -p) that neither of us > likes into a feature. I'd be in favor of getting rid of the wart > altogether (i.e. let's find a path that lets us make -m imply -p in the > future), but not rebranding it as a feature. We are walking in rounds here. Let me try to find at least some common ground. Could we try to agree on *one* of these 2 mutually exclusive statements, please: 1. Current -m behavior is useful 2. Current -m behavior is useless To me it looks like Junio thinks 1 is the case, and you, Glen, think it's 2 that is the case, and you both somewhat oppose to my patch series, believing you agree with each other, that in fact does not seem to be the case from my POV, effectively leaving me no way to adopt the series so that they are actually accepted. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-07 10:02 ` Sergey Organov @ 2023-03-07 17:54 ` Junio C Hamano 2023-03-08 22:19 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-07 17:54 UTC (permalink / raw) To: Sergey Organov; +Cc: Glen Choo, git Sergey Organov <sorganov@gmail.com> writes: > We are walking in rounds here. Let me try to find at least some common > ground. Could we try to agree on *one* of these 2 mutually exclusive > statements, please: > > 1. Current -m behavior is useful > > 2. Current -m behavior is useless > > To me it looks like Junio thinks 1 is the case, and you, Glen, think > it's 2 that is the case, ... I agree you two are going around in circles and it may be time to agree to disagree and move on. I am not Glen but I do not think anybody is arguing "-m" that does not do anything by itself is useful. By definition, without help with "-p", it is a no-op, so by itself, it cannot be useful. The reason why I am not enthused by your series is because changing behaviour of "-m" can be harmful, because "-m -p" is not the first choice people would make when they choose to view what a merge commit did. Back when "-m" was invented (yes, "-p" came first before "-m"), people were fairly content with "git log -p" that shows only individual changes. After all, merges were to integrate what these individual changes on each side of the merged history did. And you need to be aware that "-c" and "--cc" did not exist, let alone "--remerge-diff", even as a concept. The philosophy of Git back then stressed that these parts of Git were working as building blocks to feed necessary data to be used by tools people who were working at higher, end-user facing layers, would write. And because we ourselves have been mostly content with "git log -p" output that omits patches for merge commits so far, a quick approach to give output for such tools to consume without losing information was what became "-m -p" output that shows pairwise diffs that such tools can combine or do whatever they want to do. This choice was also influenced by the fact that we considered stronger than we do now that all sides of a merge are equal---the idea that the first parent chain was somehow special came much leter with the "--first-parent" option. So, given that background, "-m" that came as a modifier for what happens (only) when "-p" was in use, is perfectly understandable. In hindsight, it may appear to you that it should imply "-p", and that it is an oversight that it does not imply it. But it is more like how "git log --histogram" does not produce patch output. The mental model that allows it is that "--histogram" is to affect the way how "-p" computes the diff---a purist in you may argue that it should also imply "-p", but in reality, nobody complained that "--histogram" does not imply "-p", probably because nobody sane would say "--histogram" when they do not mean to show diff out of "git log" anyway. As somebody who saw how various options were invented while they were added to the system, I view "-m" as an option to decide what "-p" does when encountered a merge commit, and do not find it any more odd that it is a no-op without "-p" or it does not imply "-p" than "--histogram" vs "-p". ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-07 17:54 ` Junio C Hamano @ 2023-03-08 22:19 ` Sergey Organov 2023-03-08 23:08 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-03-08 22:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> We are walking in rounds here. Let me try to find at least some common >> ground. Could we try to agree on *one* of these 2 mutually exclusive >> statements, please: >> >> 1. Current -m behavior is useful >> >> 2. Current -m behavior is useless >> >> To me it looks like Junio thinks 1 is the case, and you, Glen, think >> it's 2 that is the case, ... > > I agree you two are going around in circles and it may be time to > agree to disagree and move on. I'm willing not only to disagree, but even to agree on any of these 2 options, but I can't (dis)agree on both, as they are mutually exclusive. Yeah, it could be seen as my personal problem, but, well... > I am not Glen but I do not think anybody is arguing "-m" that does not > do anything by itself is useful. I'm sorry, I still can't quite understand your position. Do you think exact current behavior of "-m" is useful, or is it useless? > By definition, without help with "-p", it is a no-op, so by itself, it > cannot be useful. No, it's not a no-op as I see it, it's just that its effect is hidden till "-p" is specified as well, and that's the exact "current behavior" that I'm asking about. Is it useful, or not? Please let me remind you that at previous rounds there was the commit that reverted my earlier "Let -m imply -p" patch, and that revert insists lone "-m" is useful, and you accepted it, and pointed me to the explanation there, telling me that there is pretty solid use-case for a lone "-m". I agreed with you at that time, and then followed with current patch series created exactly based on that earlier agreement, that now somehow turns back to disagreement, leaving me sad and confused. Provided current -m behavior is indeed useful, in these series: 1. Current -m behavior is now supported by --diff-merges (as presumably being useful.) 2. Implying -p by -m is made an option (not to break those presumably valid uses.) And now these series are to be rejected on the ground that current "-m" behavior is useless? Then let's revert the revert? > > The reason why I am not enthused by your series is because changing > behaviour of "-m" can be harmful, because "-m -p" is not the first > choice people would make when they choose to view what a merge > commit did. That's your view on merge commits, but there is another view out there, where "-m -p" is exactly the first choice, provided "-m" is configured to give diff with respect to the first parent only. I want to have the same convenience of saying just "log -m" as you already have with "log -c". What's wrong about it? I realize there is a large group here that looks at merge commits differently, and I respect their choice to show other kinds of information that could be derived from merges. What I can't understand is why another view is assumed to be a second-class citizen, sorry. Please also note that I made behavior change an option, exactly not to harm those who doesn't need it, even though it's not clear how those who don't use -m are to be affected anyway. > > Back when "-m" was invented (yes, "-p" came first before "-m"), people > were fairly content with "git log -p" that shows only individual > changes. After all, merges were to integrate what these individual > changes on each side of the merged history did. And you need to be > aware that "-c" and "--cc" did not exist, let alone "--remerge-diff", > even as a concept. Yep, I'm aware of all that history now, as I went over it back and forth a few times already, trying to figure out what't the actual ground behind desire to *keep* "-m" different from the rest of similar options, but still found none. [...] > So, given that background, "-m" that came as a modifier for what >happens (only) when "-p" was in use, is perfectly understandable. In >hindsight, it may appear to you that it should imply "-p", and that it >is an oversight that it does not imply it. My point is that the story with -c, or --cc is roughly the same, and that you yourself fixed --cc once upon a time to imply -p, so it's especially confusing for me that you are now in opposition to similar change to "-m". > > But it is more like how "git log --histogram" does not produce patch > output. The mental model that allows it is that "--histogram" is to > affect the way how "-p" computes the diff---a purist in you may > argue that it should also imply "-p", but in reality, nobody > complained that "--histogram" does not imply "-p", probably because > nobody sane would say "--histogram" when they do not mean to show > diff out of "git log" anyway. Do you then think that making -c/--cc imply "-p" was a mistake in the first place, and you try to hold me back from doing the same mistake? > As somebody who saw how various options were invented while they > were added to the system, I view "-m" as an option to decide what > "-p" does when encountered a merge commit, and do not find it any > more odd that it is a no-op without "-p" or it does not imply "-p" > than "--histogram" vs "-p". It's fine to view "-m" this way, but how do you view "-c" then? If differently, then why? In other words, in the above sentence, among "--histogram -m -c", you effectively group "(--histogram -m) -c" by leaving -c out of consideration, whereas more natural grouping is rather "--histogram (-m -c)", that'd leave --histogram out of consideration. Overall, my point is that, provided we do care about consistency of Git UI, it's -m and -c that are to be treated the same, and it's much less relevant if any of them matches treatment of --histogram. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-08 22:19 ` Sergey Organov @ 2023-03-08 23:08 ` Junio C Hamano 2023-03-09 13:54 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-08 23:08 UTC (permalink / raw) To: Sergey Organov; +Cc: Glen Choo, git Sergey Organov <sorganov@gmail.com> writes: > My point is that the story with -c, or --cc is roughly the same, and > that you yourself fixed --cc once upon a time to imply -p, so it's > especially confusing for me that you are now in opposition to similar > change to "-m". I think we all saw a good explanation for that already in the thread. The mistake by "--cc" was fixed relatively quickly, but it is now way too late to change the behaviour of "-m" without hurting existing users. I think I've wasted enough time on this in this thread already, so let's stop comparing --cc and -m now. Thanks. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-08 23:08 ` Junio C Hamano @ 2023-03-09 13:54 ` Sergey Organov 2023-03-09 17:43 ` Glen Choo 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-03-09 13:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> My point is that the story with -c, or --cc is roughly the same, and >> that you yourself fixed --cc once upon a time to imply -p, so it's >> especially confusing for me that you are now in opposition to similar >> change to "-m". > > I think we all saw a good explanation for that already in the > thread. > > The mistake by "--cc" was fixed relatively quickly, but it is now > way too late to change the behaviour of "-m" without hurting > existing users. I think I've wasted enough time on this in this > thread already, so let's stop comparing --cc and -m now. That's fine with me. I already agreed long ago that to be on the safe side we shouldn't simply change -m nowadays, and addressed that concern by putting -m behavior change under configuration option in the current series. So what's the reason of rejection? Thanks, -- Sergey ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-09 13:54 ` Sergey Organov @ 2023-03-09 17:43 ` Glen Choo 2023-03-09 19:56 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Glen Choo @ 2023-03-09 17:43 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > I already agreed long ago that to be on the safe side we shouldn't > simply change -m nowadays, and addressed that concern by putting -m > behavior change under configuration option in the current series. So > what's the reason of rejection? Before we start talking in circles again, I think the responses elsewhere in the thread still accurately capture mine and Junio's views, e.g.: https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-09 17:43 ` Glen Choo @ 2023-03-09 19:56 ` Sergey Organov 2023-03-10 21:19 ` Glen Choo 2023-03-10 21:47 ` Junio C Hamano 0 siblings, 2 replies; 30+ messages in thread From: Sergey Organov @ 2023-03-09 19:56 UTC (permalink / raw) To: Glen Choo; +Cc: Junio C Hamano, git Glen Choo <chooglen@google.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I already agreed long ago that to be on the safe side we shouldn't >> simply change -m nowadays, and addressed that concern by putting -m >> behavior change under configuration option in the current series. So >> what's the reason of rejection? > > Before we start talking in circles again, I think the responses > elsewhere in the thread still accurately capture mine and Junio's > views, e.g.: > > https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g > https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/ There were no any response from you to my recent very direct question (that Junio did somewhat address), but what I figured from your and Junio explanation so far looks like: 1. The fact that -m does not imply -p is a mistake. There is no any reasons this exact behavior could be useful. As such, it does not make sense to support this exact behavior in --diff-merges. So the reject of --diff-merges=[no-]hide. 2. This mistake is too dangerous to fix due to subtle compatibility problems, so we can't just fix -m behavior. Thus the reject of my earlier patch "let -m imply -p". 3. Moving behavior change under option is not worth it, as nobody presumably needs this fixed -m behavior anyway (at least among 2 persons that are actually opposing the changes). So the reject of "add diffMerges-m-imply-p configuration option" patch. 4. Staring in the face inconsistency between -m and the rest of short diff-merge options is not significant enough to reconsider any of the above rejects. Did I outline your positions correctly and to the full extent? Anything else I forgot? I want to make sure I got it exactly right, as I'd still like to tweak my proposals, and I'd like to avoid the troubles I got into as much as possible. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-09 19:56 ` Sergey Organov @ 2023-03-10 21:19 ` Glen Choo 2023-03-10 21:47 ` Junio C Hamano 1 sibling, 0 replies; 30+ messages in thread From: Glen Choo @ 2023-03-10 21:19 UTC (permalink / raw) To: Sergey Organov; +Cc: Junio C Hamano, git Sergey Organov <sorganov@gmail.com> writes: > Glen Choo <chooglen@google.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> I already agreed long ago that to be on the safe side we shouldn't >>> simply change -m nowadays, and addressed that concern by putting -m >>> behavior change under configuration option in the current series. So >>> what's the reason of rejection? >> >> Before we start talking in circles again, I think the responses >> elsewhere in the thread still accurately capture mine and Junio's >> views, e.g.: >> >> https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g >> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/ > > There were no any response from you to my recent very direct question > (that Junio did somewhat address), but what I figured from your and > Junio explanation so far looks like: Yes, thank you for summarizing the discussion. I think the following sums up my position quite accurately, and I found this very helpful. > 1. The fact that -m does not imply -p is a mistake. There is no any > reasons this exact behavior could be useful. As such, it does not > make sense to support this exact behavior in --diff-merges. So the > reject of --diff-merges=[no-]hide. Yes. > 2. This mistake is too dangerous to fix due to subtle compatibility > problems, so we can't just fix -m behavior. Thus the reject of my > earlier patch "let -m imply -p". Yes, unfortunately. If we didn't have compatibility problems, "-m imply -p" would be very good IMO. I am not that hopeful, but perhaps there are ways to get users to move off this compatibility problem. It would be something we would have to learn as a project though, it is not a problem unique to this series. > 3. Moving behavior change under option is not worth it, as nobody > presumably needs this fixed -m behavior anyway (at least among 2 > persons that are actually opposing the changes). So the reject > of "add diffMerges-m-imply-p configuration option" patch. Yes, given how specific the option is (I think it would be the only config option that exists to change the behavior of a CLI option) I don't think the option is worth it in light of alternatives like a new "-d" flag or supporting "-mp". > 4. Staring in the face inconsistency between -m and the rest > of short diff-merge options is not significant enough to reconsider > any of the above rejects. Yes. Git carries many such inconsistencies. > Did I outline your positions correctly and to the full extent? Anything > else I forgot? I cannot think of anything else. Thank you. > I want to make sure I got it exactly right, as I'd still like to tweak > my proposals, and I'd like to avoid the troubles I got into as much as > possible. Thanks. I hope that there is a good way forward for your proposals. > > Thanks, > -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-09 19:56 ` Sergey Organov 2023-03-10 21:19 ` Glen Choo @ 2023-03-10 21:47 ` Junio C Hamano 2023-03-17 14:18 ` Sergey Organov 1 sibling, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-10 21:47 UTC (permalink / raw) To: Sergey Organov; +Cc: Glen Choo, git Sergey Organov <sorganov@gmail.com> writes: > 1. The fact that -m does not imply -p is a mistake. There is no any > reasons this exact behavior could be useful. As such, it does not > make sense to support this exact behavior in --diff-merges. So the > reject of --diff-merges=[no-]hide. > > 2. This mistake is too dangerous to fix due to subtle compatibility > problems, so we can't just fix -m behavior. Thus the reject of my > earlier patch "let -m imply -p". > > 3. Moving behavior change under option is not worth it, as nobody > presumably needs this fixed -m behavior anyway (at least among 2 > persons that are actually opposing the changes). So the reject > of "add diffMerges-m-imply-p configuration option" patch. > > 4. Staring in the face inconsistency between -m and the rest > of short diff-merge options is not significant enough to reconsider > any of the above rejects. I do not quite understand the last one (#4), and if I were to add my own 4., it would be that introducing --diff-merges={kind} may have been a mistake. It would have been fine and better to just let users choose from whatever set of options we support, i.e. (-c, --cc, --remerge-diff, -m -p, -m --raw, ...). IOW, perhaps deprecate --diff-merges={kind} and eventually remove it, if we could. We've been fine without it and we'll be fine without it. Unfortunately it may be a bit too late for that, but it certainly is much younger than "-m". ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-10 21:47 ` Junio C Hamano @ 2023-03-17 14:18 ` Sergey Organov 2023-03-18 0:08 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Sergey Organov @ 2023-03-17 14:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> 1. The fact that -m does not imply -p is a mistake. There is no any >> reasons this exact behavior could be useful. As such, it does not >> make sense to support this exact behavior in --diff-merges. So the >> reject of --diff-merges=[no-]hide. >> >> 2. This mistake is too dangerous to fix due to subtle compatibility >> problems, so we can't just fix -m behavior. Thus the reject of my >> earlier patch "let -m imply -p". >> >> 3. Moving behavior change under option is not worth it, as nobody >> presumably needs this fixed -m behavior anyway (at least among 2 >> persons that are actually opposing the changes). So the reject >> of "add diffMerges-m-imply-p configuration option" patch. >> >> 4. Staring in the face inconsistency between -m and the rest >> of short diff-merge options is not significant enough to reconsider >> any of the above rejects. > > I do not quite understand the last one (#4), Well, -m does not imply -p, whereas the rest of diff-merges options (-c/--cc/--remerge-diff) do imply -p. This is what half of this lengthy discussion was about. > own 4., it would be that introducing --diff-merges={kind} may have > been a mistake. It would have been fine and better to just let > users choose from whatever set of options we support, i.e. (-c, > --cc, --remerge-diff, -m -p, -m --raw, ...). > > IOW, perhaps deprecate --diff-merges={kind} and eventually remove > it, if we could. Why? Unlike -m vs -c they are at least self-consistent and besides allow to get the output that those short options do not. > We've been fine without it and we'll be fine without it. Unfortunately > it may be a bit too late for that, but it certainly is much younger > than "-m". I, for one, was never fine with what Git does to show diff for merges. Then, taking into account that introducing of --diff-merges was not my idea in the first place, there should be at least two of us here who were not fine. It's fine with me that --cc is everything you need, but what I need is rather diff to the first parent, and I had a hope to finally get -m to do it, aiming both for consistency and convenience. My 2 attempts performed in different ways both failed, so, being tired of it likely even more than you do, I digress. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-17 14:18 ` Sergey Organov @ 2023-03-18 0:08 ` Junio C Hamano 2023-03-25 16:55 ` Sergey Organov 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2023-03-18 0:08 UTC (permalink / raw) To: Sergey Organov; +Cc: Glen Choo, git Sergey Organov <sorganov@gmail.com> writes: >> I do not quite understand the last one (#4), > > Well, -m does not imply -p, whereas the rest of diff-merges options > (-c/--cc/--remerge-diff) do imply -p. This is what half of this > lengthy discussion was about. > >> own 4., it would be that introducing --diff-merges={kind} may have >> been a mistake. It would have been fine and better to just let >> users choose from whatever set of options we support, i.e. (-c, >> --cc, --remerge-diff, -m -p, -m --raw, ...). > It's fine with me that --cc is everything you need, but what I need is > rather diff to the first parent, ... I think "show --first-parent" should give that already. One problem with "-m implies -p" is that it is unclear what should be done to things like "-m --raw". Yes, we can declare an arbitrary choice (like "-m implies -p unless --raw, --stat, etc. are given") but that is just replacing an arbitrary rule (i.e. "comparison with parents are not given for merges, unless things like --cc, --first-parent, etc. that are specifically designed for showing merges are given; you can give -m to force pairwise behaviour.") with another one. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-18 0:08 ` Junio C Hamano @ 2023-03-25 16:55 ` Sergey Organov 2023-03-29 7:43 ` Sergey Organov 2023-03-29 8:06 ` Sergey Organov 0 siblings, 2 replies; 30+ messages in thread From: Sergey Organov @ 2023-03-25 16:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>> I do not quite understand the last one (#4), >> >> Well, -m does not imply -p, whereas the rest of diff-merges options >> (-c/--cc/--remerge-diff) do imply -p. This is what half of this >> lengthy discussion was about. >> >>> own 4., it would be that introducing --diff-merges={kind} may have >>> been a mistake. It would have been fine and better to just let >>> users choose from whatever set of options we support, i.e. (-c, >>> --cc, --remerge-diff, -m -p, -m --raw, ...). > >> It's fine with me that --cc is everything you need, but what I need is >> rather diff to the first parent, ... > > I think "show --first-parent" should give that already. Well, for "git show" even "show -m" does the right thing (once properly configured), as "-p" is implied by "git show". Taking "git show" into the picture brings yet another argument if favor of new "-m" behavior, as then "git show -m" and "git log -m -n1" will finally start to produce the same result, that I'd find desirable. That said, --diff-merges=first-parent that could be shortened to --diff-merges=1 is the universal answer that works out-of-the-box for any command the same way, reliably, and then it's also -m -p if configured accordingly, that has been made available by previously accepted patches. These series just did the last logical step: allowed it to be just -m if configured accordingly. > One problem with "-m implies -p" is that it is unclear what should be > done to things like "-m --raw". Nothing specific is actually needed, as far as I'm aware, as implied -p doesn't interfere with --raw. Please give particular example of a problem if you foresee one. As I see it, if there is indeed some problem with this, it should already exist for -c, --cc, and --remerge-diff, and then probably needs to be fixed anyway. Moreover, it should also exist for "git show", as the latter implies -p, and there is: -s, --no-patch Suppress diff output. Useful for commands like git show that show the patch by default, or to cancel the effect of --patch. [As a side-note, current behavior of implied -p, explicit -p, -s, and --raw with respect to each other that I figured by experiment looks suspect to me. E.g., once explicit -p is given, and then canceled by -s, I can't get bare --raw output anymore] > Yes, we can declare an arbitrary choice (like "-m implies -p unless > --raw, --stat, etc. are given") but that is just replacing an > arbitrary rule [...] with another one. Uh, this would be too cumbersome indeed, but fortunately it does not seem to be needed, see above. Overall, letting -m imply -p just makes things more consistent, even on the problems side (if any), and I honestly still don't see'em. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-25 16:55 ` Sergey Organov @ 2023-03-29 7:43 ` Sergey Organov 2023-03-29 8:06 ` Sergey Organov 1 sibling, 0 replies; 30+ messages in thread From: Sergey Organov @ 2023-03-29 7:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>>> I do not quite understand the last one (#4), >>> >>> Well, -m does not imply -p, whereas the rest of diff-merges options >>> (-c/--cc/--remerge-diff) do imply -p. This is what half of this >>> lengthy discussion was about. >>> >>>> own 4., it would be that introducing --diff-merges={kind} may have >>>> been a mistake. It would have been fine and better to just let >>>> users choose from whatever set of options we support, i.e. (-c, >>>> --cc, --remerge-diff, -m -p, -m --raw, ...). >> >>> It's fine with me that --cc is everything you need, but what I need is >>> rather diff to the first parent, ... >> >> I think "show --first-parent" should give that already. > > Well, for "git show" even "show -m" does the right thing (once properly > configured), as "-p" is implied by "git show". > > Taking "git show" into the picture brings yet another argument if favor > of new "-m" behavior, as then "git show -m" and "git log -m -n1" will > finally start to produce the same result, that I'd find desirable. > > That said, > > --diff-merges=first-parent > > that could be shortened to > > --diff-merges=1 > > is the universal answer that works out-of-the-box for any command the > same way, reliably, and then it's also > > -m -p > > if configured accordingly, that has been made available by previously > accepted patches. > > These series just did the last logical step: allowed it to be just > > -m > > if configured accordingly. > >> One problem with "-m implies -p" is that it is unclear what should be >> done to things like "-m --raw". > > Nothing specific is actually needed, as far as I'm aware, as implied -p > doesn't interfere with --raw. Please give particular example of a > problem if you foresee one. In fact there is already a test for it that I added some time ago (as well as for --stat), so a problem would have been caught. Please also notice that it was an agreed goal for "-m" to finally imply "-p" at that time: commit 48229c193d2e6e728d3243bacea2f1e1490ced8a Author: Sergey Organov <sorganov@gmail.com> Date: Fri May 21 00:46:55 2021 +0300 t4013: test "git log -m --raw" This is to ensure we won't break different diff formats when we start to imply "-p" by "-m". Signed-off-by: Sergey Organov <sorganov@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-03-25 16:55 ` Sergey Organov 2023-03-29 7:43 ` Sergey Organov @ 2023-03-29 8:06 ` Sergey Organov 1 sibling, 0 replies; 30+ messages in thread From: Sergey Organov @ 2023-03-29 8:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git Sergey Organov <sorganov@gmail.com> writes: [...] > [As a side-note, current behavior of implied -p, explicit -p, -s, and > --raw with respect to each other that I figured by experiment looks > suspect to me. E.g., once explicit -p is given, and then canceled by -s, > I can't get bare --raw output anymore] Specifically, this test unexpectedly fails: modified t/t4013-diff-various.sh @@ -457,6 +457,16 @@ diff-tree --stat --compact-summary initial mode diff-tree -R --stat --compact-summary initial mode EOF +# This should succeed, but --patch --no-patch does something hidden +# that breaks --raw. +test_expect_success '--no-patch only cancels --patch' ' + git log --raw master >result && + process_diffs result >expected && + git log --patch --no-patch --raw >result && + process_diffs result >actual && + test_cmp expected actual +' + test_expect_success 'log -m matches pure log' ' git log master >result && process_diffs result >expected && Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 30+ messages in thread
* ds/bundle-uri-5 (was: Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) 2023-02-03 4:02 What's cooking in git.git (Feb 2023, #01; Thu, 2) Junio C Hamano 2023-02-04 9:33 ` Sergey Organov @ 2023-02-08 17:22 ` Victoria Dye 1 sibling, 0 replies; 30+ messages in thread From: Victoria Dye @ 2023-02-08 17:22 UTC (permalink / raw) To: Junio C Hamano, git Junio C Hamano wrote: > * ds/bundle-uri-5 (2023-01-31) 11 commits > - bundle-uri: test missing bundles with heuristic > - bundle-uri: store fetch.bundleCreationToken > - fetch: fetch from an external bundle URI > - bundle-uri: drop bundle.flag from design doc > - clone: set fetch.bundleURI if appropriate > - bundle-uri: download in creationToken order > - bundle-uri: parse bundle.<id>.creationToken values > - bundle-uri: parse bundle.heuristic=creationToken > - t5558: add tests for creationToken heuristic > - bundle: verify using check_connected() > - bundle: test unbundling with incomplete history > > The bundle-URI subsystem adds support for creation-token heuristics > to help incremental fetches. > > Will merge to 'next'? > source: <pull.1454.v3.git.1675171759.gitgitgadget@gmail.com> Apologies for not responding sooner, but I reviewed the most recent version of this series and all of my previous feedback was addressed. Overall, I think it looks good and is ready for merging to 'next'. Thanks! -Victoria ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-03-29 8:06 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-03 4:02 What's cooking in git.git (Feb 2023, #01; Thu, 2) Junio C Hamano 2023-02-04 9:33 ` Sergey Organov 2023-02-06 18:35 ` Junio C Hamano 2023-02-06 21:35 ` Sergey Organov 2023-03-01 18:40 ` Sergey Organov 2023-03-01 22:15 ` Junio C Hamano 2023-03-01 22:26 ` Sergey Organov 2023-03-01 23:54 ` Glen Choo 2023-03-02 14:38 ` Sergey Organov 2023-02-07 4:06 ` so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)) Glen Choo 2023-02-07 12:50 ` Sergey Organov 2023-03-02 0:37 ` Glen Choo 2023-03-02 16:15 ` Junio C Hamano 2023-03-02 16:57 ` Sergey Organov 2023-03-06 22:22 ` Glen Choo 2023-03-07 10:02 ` Sergey Organov 2023-03-07 17:54 ` Junio C Hamano 2023-03-08 22:19 ` Sergey Organov 2023-03-08 23:08 ` Junio C Hamano 2023-03-09 13:54 ` Sergey Organov 2023-03-09 17:43 ` Glen Choo 2023-03-09 19:56 ` Sergey Organov 2023-03-10 21:19 ` Glen Choo 2023-03-10 21:47 ` Junio C Hamano 2023-03-17 14:18 ` Sergey Organov 2023-03-18 0:08 ` Junio C Hamano 2023-03-25 16:55 ` Sergey Organov 2023-03-29 7:43 ` Sergey Organov 2023-03-29 8:06 ` Sergey Organov 2023-02-08 17:22 ` ds/bundle-uri-5 (was: " Victoria Dye
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).