{"thread":{"id":"59186","subject":"What's cooking in git.git (Feb 2023, #01; Thu, 2)","startedAt":"2023-02-03T04:02:55Z","lastAt":"2023-03-29T08:06:36Z","messageCount":30,"participants":["Junio C Hamano","Sergey Organov","Glen Choo","Victoria Dye"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"471417","messageId":"xmqqr0v7o0pp.fsf@gitster.g","threadId":"59186","inReplyTo":null,"subject":"What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-03T04:02:42Z","receivedAt":"2023-02-03T04:02:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here are the topics that have been cooking in my tree. Commits\nprefixed with '+' are in 'next' (being in 'next' is a sign that a\ntopic is stable enough to be used and are candidate to be in a future\nrelease). Commits prefixed with '-' are only in 'seen', and aren't\nconsidered \"accepted\" at all. A topic without enough support may be\ndiscarded after a long period of no activity.\n\nCopies of the source code to Git live in many repositories, and the\nfollowing is a list of the ones I push into or their mirrors. Some\nrepositories have only a subset of branches.\n\nWith maint, master, next, seen, todo:\n\n\tgit://git.kernel.org/pub/scm/git/git.git/\n\tgit://repo.or.cz/alt-git.git/\n\thttps://kernel.googlesource.com/pub/scm/git/git/\n\thttps://github.com/git/git/\n\thttps://gitlab.com/git-vcs/git/\n\nWith all the integration branches and topics broken out:\n\n\thttps://github.com/gitster/git/\n\nEven though the preformatted documentation in HTML and man format\nare not sources, they are published in these repositories for\nconvenience (replace \"htmldocs\" with \"manpages\" for the manual\npages):\n\n\tgit://git.kernel.org/pub/scm/git/git-htmldocs.git/\n\thttps://github.com/gitster/git-htmldocs.git/\n\nRelease tarballs are available at:\n\n\thttps://www.kernel.org/pub/software/scm/git/\n\n--------------------------------------------------\n[New Topics]\n\n* ab/imap-send-requires-curl (2023-02-02) 6 commits\n - imap-send: correctly report \"host\" when using \"tunnel\"\n - imap-send: remove old --no-curl codepath\n - imap-send: make --curl no-optional\n - imap-send: replace auto-probe libcurl with hard dependency\n - imap-send doc: the imap.sslVerify is used with imap.tunnel\n - imap-send: note \"auth_method\", not \"host\" on auth method failure\n\n Give a hard dependency on cURL library to build \"git imap-send\",\n and remove the code to interact with IMAP server without using cURL.\n\n Comments?\n source: \n\n\n* gc/index-format-doc (2023-02-01) 1 commit\n - docs: document zero bits in index \"mode\"\n\n Doc update.\n\n Will merge to 'next'.\n source: <20230201024041.29401-1-chooglen@google.com>\n\n\n* jk/httpd-test-updates (2023-02-01) 4 commits\n - t/lib-httpd: increase ssl key size to 2048 bits\n - t/lib-httpd: drop SSLMutex config\n - t/lib-httpd: bump required apache version to 2.4\n - t/lib-httpd: bump required apache version to 2.2\n\n Test update.\n\n Will merge to 'next'.\n source: \n\n\n* rd/doc-default-date-format (2023-02-01) 1 commit\n - rev-list: clarify git-log default date format\n\n Update --date=default documentation.\n source: <20230201155712.86577-1-rafael@dulfer.be>\n\n\n* ew/free-island-marks (2023-02-02) 1 commit\n - delta-islands: free island_marks and bitmaps\n\n \"git pack-objects\" learned to release delta-island bitmap data when\n it is done using it, saving peak heap memory usage.\n\n Will merge to 'next'.\n source: <20230202094217.M955476@dcvr>\n\n\n* hj/remove-msys-support (2023-02-02) 2 commits\n - mingw: remove msysGit/MSYS1 support\n - mingw: remove duplicate `USE_NED_ALLOCATOR` directive\n\n Remove support for MSys, which now lags way behind MSys2.\n\n Will merge to 'next'.\n source: \n\n\n* sk/winansi-createthread-fix (2023-02-01) 1 commit\n - compat/winansi: check for errors of CreateThread() correctly\n\n Fix use of CreateThread() API call made early in the windows\n start-up code.\n\n Will merge to 'next'.\n source: \n\n--------------------------------------------------\n[Stalled]\n\n* rj/branch-unborn-in-other-worktrees (2023-01-19) 3 commits\n - branch: rename orphan branches in any worktree\n - branch: description for orphan branch errors\n - avoid unnecessary worktrees traversing\n\n Error messages given when working on an unborn branch that is\n checked out in another worktree have been improvved.\n\n Expecting a reroll.\n cf. <527f7315-be7b-7ec0-04fc-d07da7d4fefa@gmail.com>\n source: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com>\n\n\n* ja/worktree-orphan (2023-01-13) 4 commits\n - worktree add: add hint to direct users towards --orphan\n - worktree add: add --orphan flag\n - worktree add: refactor opt exclusion tests\n - worktree add: include -B in usage docs\n\n 'git worktree add' learned how to create a worktree based on an\n orphaned branch with `--orphan`.\n\n Expecting a reroll.\n cf. <11be1b0e-ee38-119f-1d80-cb818946116b@dunelm.org.uk>\n source: <20230109173227.29264-1-jacobabel@nullpo.dev>\n\n\n* ab/avoid-losing-exit-codes-in-tests (2022-12-20) 6 commits\n - tests: don't lose misc \"git\" exit codes\n - tests: don't lose \"git\" exit codes in \"! ( git ... | grep )\"\n - tests: don't lose exit status with \"test $(git ...)\"\n - tests: don't lose exit status with \"(cd ...; test $(git ...))\"\n - t/lib-patch-mode.sh: fix ignored exit codes\n - auto-crlf tests: don't lose exit code in loops and outside tests\n\n Test clean-up.\n\n Expecting a hopefully minor and final reroll.\n cf. <1182283a-4a78-3c99-e716-a8c3e58a5823@web.de>\n cf. \n source: \n\n\n* tl/notes--blankline (2022-11-09) 5 commits\n - notes.c: introduce \"--no-blank-line\" option\n - notes.c: provide tips when target and append note are both empty\n - notes.c: drop unreachable code in 'append_edit()'\n - notes.c: cleanup for \"designated init\" and \"char ptr init\"\n - notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n\n 'git notes append' was taught '--[no-]blank-line' to conditionally\n add a LF between a new and existing note.\n\n Expecting a reroll.\n cf. \n source: \n\n\n* mc/switch-advice (2022-11-09) 1 commit\n - po: use `switch` over `checkout` in error message\n\n Use 'switch' instead of 'checkout' in an error message.\n\n Waiting for review response.\n source: \n\n\n* js/range-diff-mbox (2022-11-23) 1 commit\n - range-diff: support reading mbox files\n\n 'git range-diff' gained support for reading either side from an .mbox\n file instead of a revision range.\n\n Waiting for review response.\n cf. \n source: \n\n\n* ab/tag-object-type-errors (2022-11-22) 5 commits\n - tag: don't emit potentially incorrect \"object is a X, not a Y\"\n - tag: don't misreport type of tagged objects in errors\n - object tests: add test for unexpected objects in tags\n - object-file.c: free the \"t.tag\" in check_tag()\n - Merge branch 'jk/parse-object-type-mismatch' into ab/tag-object-type-errors\n\n Hardening checks around mismatched object types when one of those\n objects is a tag.\n\n Expecting a reroll.\n cf. \n cf. \n source: \n\n\n* ed/fsmonitor-inotify (2022-12-13) 6 commits\n - fsmonitor: update doc for Linux\n - fsmonitor: test updates\n - fsmonitor: enable fsmonitor for Linux\n - fsmonitor: implement filesystem change listener for Linux\n - fsmonitor: determine if filesystem is local or remote\n - fsmonitor: prepare to share code between Mac OS and Linux\n\n Bundled fsmonitor for Linux using inotify API.\n\n Needs review on the updated round.\n source: \n\n\n* jc/spell-id-in-both-caps-in-message-id (2022-12-17) 1 commit\n - e-mail workflow: Message-ID is spelled with ID in both capital letters\n\n Consistently spell \"Message-ID\" as such, not \"Message-Id\".\n\n Needs review.\n source: \n\n\n* ad/test-record-count-when-harness-is-in-use (2022-12-25) 1 commit\n - test-lib: allow storing counts with test harnesses\n\n Allow summary results from tests to be written to t/test-results\n directory even when a test harness like 'prove' is in use.\n\n Needs review.\n source: <20221224225200.1027806-1-adam@dinwoodie.org>\n\n\n* so/diff-merges-more (2022-12-18) 5 commits\n - diff-merges: improve --diff-merges documentation\n - diff-merges: issue warning on lone '-m' option\n - diff-merges: support list of values for --diff-merges\n - diff-merges: implement log.diffMerges-m-imply-p config\n - diff-merges: implement [no-]hide option and log.diffMergesHide config\n\n Assorted updates to \"--diff-merges=X\" option.\n\n May want to discard. Breaking compatibility does not seem worth it.\n source: <20221217132955.108542-1-sorganov@gmail.com>\n\n--------------------------------------------------\n[Cooking]\n\n* ab/config-multi-and-nonbool (2023-02-02) 9 commits\n - for-each-repo: with bad config, don't conflate and \n - config API: add \"string\" version of *_value_multi(), fix segfaults\n - config API users: test for *_get_value_multi() segfaults\n - for-each-repo: error on bad --config\n - config API: have *_multi() return an \"int\" and take a \"dest\"\n - versioncmp.c: refactor config reading next commit\n - config API: add and use a \"git_config_get()\" family of functions\n - config tests: add \"NULL\" tests for *_get_value_multi()\n - config tests: cover blind spots in git_die_config() tests\n\n Assorted config API updates.\n\n source: \n\n\n* ds/scalar-ignore-cron-error (2023-01-27) 3 commits\n (merged to 'next' on 2023-01-31 at 98d13ac3e7)\n + scalar: only warn when background maintenance fails\n + t921*: test scalar behavior starting maintenance\n + t: allow 'scalar' in test_must_fail\n\n Allow \"scalar\" to warn but continue when its periodic maintenance\n feature cannot be enabled.\n\n Will merge to 'master'.\n source: \n\n\n* mh/doc-credential-cache-only-in-core (2023-01-29) 1 commit\n (merged to 'next' on 2023-01-30 at 021b5227af)\n + Documentation: clarify that cache forgets credentials if the system restarts\n\n Documentation clarification.\n\n Will merge to 'master'.\n source: \n\n\n* ab/hook-api-with-stdin (2023-01-23) 5 commits\n - hook: support a --to-stdin= option for testing\n - sequencer: use the new hook API for the simpler \"post-rewrite\" call\n - hook API: support passing stdin to hooks, convert am's 'post-rewrite'\n - run-command: allow stdin for run_processes_parallel\n - run-command.c: remove dead assignment in while-loop\n\n Extend the run-hooks API to allow feeding data from the standard\n input when running the hook script(s).\n\n Expecting review responses.\n source: \n\n\n* as/ssh-signing-improve-key-missing-error (2023-01-25) 1 commit\n (merged to 'next' on 2023-01-25 at 140f2c2c60)\n + ssh signing: better error message when key not in agent\n\n Improve the error message given when private key is not loaded in\n the ssh agent in the codepath to sign with an ssh key.\n\n Will merge to 'master'.\n source: \n\n\n* en/rebase-incompatible-opts (2023-01-25) 10 commits\n (merged to 'next' on 2023-01-27 at 35a67cf2c6)\n + rebase: provide better error message for apply options vs. merge config\n + rebase: put rebase_options initialization in single place\n + rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n + rebase: clarify the OPT_CMDMODE incompatibilities\n + rebase: add coverage of other incompatible options\n + rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks\n + rebase: fix docs about incompatibilities with --root\n + rebase: remove --allow-empty-message from incompatible opts\n + rebase: flag --apply and --merge as incompatible\n + rebase: mark --update-refs as requiring the merge backend\n\n \"git rebase\" often ignored incompatible options instead of\n complaining, which has been corrected.\n\n Will merge to 'master'.\n Replaces en/rebase-update-refs-needs-merge-backend.\n source: \n\n\n* gm/request-pull-with-non-pgp-signed-tags (2023-01-25) 1 commit\n (merged to 'next' on 2023-01-30 at abc684d8df)\n + request-pull: filter out SSH/X.509 tag signatures\n\n Adjust \"git request-pull\" to strip embedded signature from signed\n tags to notice non-PGP signatures.\n\n Will merge to 'master'.\n source: <20230125234725.3918563-1-gwymor@tilde.club>\n\n\n* cb/grep-fallback-failing-jit (2023-01-31) 1 commit\n - grep: fall back to interpreter if JIT memory allocation fails\n\n In an environment where dynamically generated code is prohibited to\n run (e.g. SELinux), failure to JIT pcre patterns is expected. Fall\n back to interpreted execution in such a case.\n\n Will merge to 'next'?\n source: <20230131185611.520311-1-minipli@grsecurity.net>\n\n\n* cb/checkout-same-branch-twice (2023-01-20) 1 commit\n - checkout/switch: disallow checking out same branch in multiple worktrees\n\n \"git checkout -B $branch\" failed to protect against checking out\n a branch that is checked out elsewhere, unlike \"git branch -f\" did.\n\n Expecting a hopefully minor and final reroll.\n cf. <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk>\n source: <20230120113553.24655-1-carenas@gmail.com>\n\n\n* ab/sequencer-unleak (2023-01-18) 8 commits\n - commit.c: free() revs.commit in get_fork_point()\n - builtin/rebase.c: free() \"options.strategy_opts\"\n - sequencer.c: always free() the \"msgbuf\" in do_pick_commit()\n - builtin/rebase.c: fix \"options.onto_name\" leak\n - builtin/revert.c: move free-ing of \"revs\" to replay_opts_release()\n - rebase & sequencer API: fix get_replay_opts() leak in \"rebase\"\n - sequencer.c: split up sequencer_remove_state()\n - rebase: use \"cleanup\" pattern in do_interactive_rebase()\n\n Plug leaks in sequencer subsystem and its users.\n\n Expecting a hopefully minor and final reroll.\n cf. \n source: \n\n\n* jc/attr-doc-fix (2023-01-26) 1 commit\n (merged to 'next' on 2023-01-26 at cb327c4b5f)\n + attr: fix instructions on how to check attrs\n\n Comment fix.\n\n Will merge to 'master'.\n source: \n\n\n* rj/avoid-switching-to-already-used-branch (2023-01-22) 3 commits\n - switch: reject if the branch is already checked out elsewhere (test)\n - rebase: refuse to switch to a branch already checked out elsewhere (test)\n - branch: fix die_if_checked_out() when ignore_current_worktree\n\n A few subcommands have been taught to stop users from working on a\n branch that is being used in another worktree linked to the same\n repository.\n\n Expecting a hopefully minor and final reroll.\n cf. \n source: \n\n\n* rj/bisect-already-used-branch (2023-01-22) 1 commit\n - bisect: fix \"reset\" when branch is checked out elsewhere\n\n Allow \"git bisect reset [name]\" to check out the named branch (or\n the original one) even when the branch is already checked out in a\n different worktree linked to the same repository.\n\n Leaning negative. Why is it a good thing?\n cf. \n source: <1c36c334-9f10-3859-c92f-3d889e226769@gmail.com>\n\n\n* en/ls-files-doc-update (2023-01-13) 4 commits\n (merged to 'next' on 2023-01-27 at 20b9803add)\n + ls-files: guide folks to --exclude-standard over other --exclude* options\n + ls-files: clarify descriptions of status tags for -t\n + ls-files: clarify descriptions of file selection options\n + ls-files: add missing documentation for --resolve-undo option\n\n Doc update to ls-files.\n\n Will merge to 'master'.\n source: \n\n\n* ms/send-email-feed-header-to-validate-hook (2023-01-19) 2 commits\n - send-email: expose header information to git-send-email's sendemail-validate hook\n - send-email: refactor header generation functions\n\n \"git send-email\" learned to give the e-mail headers to the validate\n hook by passing an extra argument from the command line.\n\n Expecting a hopefully minor and final reroll.\n cf. \n source: <20230120012459.920932-1-michael.strawbridge@amd.com>\n\n\n* ds/bundle-uri-5 (2023-01-31) 11 commits\n - bundle-uri: test missing bundles with heuristic\n - bundle-uri: store fetch.bundleCreationToken\n - fetch: fetch from an external bundle URI\n - bundle-uri: drop bundle.flag from design doc\n - clone: set fetch.bundleURI if appropriate\n - bundle-uri: download in creationToken order\n - bundle-uri: parse bundle..creationToken values\n - bundle-uri: parse bundle.heuristic=creationToken\n - t5558: add tests for creationToken heuristic\n - bundle: verify using check_connected()\n - bundle: test unbundling with incomplete history\n\n The bundle-URI subsystem adds support for creation-token heuristics\n to help incremental fetches.\n\n Will merge to 'next'?\n source: \n\n\n* tc/cat-file-z-use-cquote (2023-01-16) 1 commit\n - cat-file: quote-format name in error when using -z\n\n \"cat-file\" in the batch mode that is fed NUL-terminated pathnames\n learned to cquote them in its error output (otherwise, a funny\n pathname with LF in it would break the lines in the output stream).\n\n Expecting a reroll.\n cf. <2a2a46f0-a9bc-06a6-72e1-28800518777c@dunelm.org.uk>\n source: <20230116190749.4141516-1-toon@iotcl.com>\n\n\n* cw/submodule-status-in-parallel (2023-01-17) 6 commits\n - submodule: call parallel code from serial status\n - diff-lib: parallelize run_diff_files for submodules\n - diff-lib: refactor match_stat_with_submodule\n - submodule: move status parsing into function\n - submodule: strbuf variable rename\n - run-command: add duplicate_output_fn to run_processes_parallel_opts\n\n \"git submodule status\" learned to run the comparison in submodule\n repositories in parallel.\n\n Expecting a reroll.\n cf. \n cf. \n source: <20230104215415.1083526-1-calvinwan@google.com>\n\n\n* ab/various-leak-fixes (2023-02-02) 19 commits\n - push: free_refs() the \"local_refs\" in set_refspecs()\n - push: refactor refspec_append_mapped() for subsequent leak-fix\n - receive-pack: free() the \"ref_name\" in \"struct command\"\n - grep API: plug memory leaks by freeing \"header_list\"\n - grep.c: refactor free_grep_patterns()\n - builtin/merge.c: free \"&buf\" on \"Your local changes...\" error\n - builtin/merge.c: use fixed strings, not \"strbuf\", fix leak\n - show-branch: free() allocated \"head\" before return\n - commit-graph: fix a parse_options_concat() leak\n - http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()\n - http-backend.c: fix \"dir\" and \"cmd_arg\" leaks in cmd_main()\n - worktree: fix a trivial leak in prune_worktrees()\n - repack: fix leaks on error with \"goto cleanup\"\n - name-rev: don't xstrdup() an already dup'd string\n - various: add missing clear_pathspec(), fix leaks\n - clone: use free() instead of UNLEAK()\n - commit-graph: use free_commit_graph() instead of UNLEAK()\n - bundle.c: don't leak the \"args\" in the \"struct child_process\"\n - tests: mark tests as passing with SANITIZE=leak\n\n Leak fixes.\n\n Comments?\n source: \n\n\n--------------------------------------------------\n[Discarded]\n\n* mc/credential-helper-auth-headers (2023-01-20) 12 commits\n (merged to 'next' on 2023-01-25 at cb95006bb2)\n + credential: add WWW-Authenticate header to cred requests\n + http: read HTTP WWW-Authenticate response headers\n + http: replace unsafe size_t multiplication with st_mult\n + test-http-server: add sending of arbitrary headers\n + test-http-server: add simple authentication\n + test-http-server: pass Git requests to http-backend\n + test-http-server: add HTTP request parsing\n + test-http-server: add HTTP error response function\n + test-http-server: add stub HTTP server test helper\n + daemon: rename some esoteric/laboured terminology\n + daemon: libify child process handling functions\n + daemon: libify socket setup and option functions\n\n Extending credential helper protocol.\n\n The test-only server is an eyesore.\n cf. \n cf. \n source: \n"},{"id":"471476","messageId":"871qn5pyez.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqqr0v7o0pp.fsf@gitster.g","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-02-04T09:33:56Z","receivedAt":"2023-02-04T09:34:08Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n\n> * so/diff-merges-more (2022-12-18) 5 commits\n> - diff-merges: improve --diff-merges documentation\n> - diff-merges: issue warning on lone '-m' option\n> - diff-merges: support list of values for --diff-merges\n> - diff-merges: implement log.diffMerges-m-imply-p config\n> - diff-merges: implement [no-]hide option and log.diffMergesHide config\n>\n> Assorted updates to \"--diff-merges=X\" option.\n>\n> May want to discard. Breaking compatibility does not seem worth it.\n> source: <20221217132955.108542-1-sorganov@gmail.com>\n\nHi Junio,\n\nThis does not break any compatibility, as far as me and I believe\nreviewers of these series are aware.\n\nPlease either merge this or explain what do you mean in more details.\n\nThanks,\n-- Sergey Organov\n"},{"id":"471573","messageId":"xmqqedr28wwb.fsf@gitster.g","threadId":"59186","inReplyTo":"871qn5pyez.fsf@osv.gnss.ru","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-06T18:35:32Z","receivedAt":"2023-02-06T18:35:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>\n>> * so/diff-merges-more (2022-12-18) 5 commits\n>> - diff-merges: improve --diff-merges documentation\n>> - diff-merges: issue warning on lone '-m' option\n>> - diff-merges: support list of values for --diff-merges\n>> - diff-merges: implement log.diffMerges-m-imply-p config\n>> - diff-merges: implement [no-]hide option and log.diffMergesHide config\n>>\n>> Assorted updates to \"--diff-merges=X\" option.\n>>\n>> May want to discard. Breaking compatibility does not seem worth it.\n>> source: <20221217132955.108542-1-sorganov@gmail.com>\n>\n> Hi Junio,\n>\n> This does not break any compatibility, as far as me and I believe\n> reviewers of these series are aware.\n\nThe last paragraphs in the review two months ago still describe what\nthis series does fairly accurately, I think.\n\n These patches do look like a good approach to solve the first point\n among the \"two problems\" in the previous round. Thanks for working\n on it.\n\n IIRC, the previous round (why is this round marked as v1, by the\n way?) was reviewed by some folks, so lets wait to hear from them\n how this round does better.\n\n Unfortunately, I do not think of any \"solution\" that would avoid\n breaking folks, if its end goal is to flip the default, either by\n hardcoding or with a configuration variable. IOW, the other one\n among the \"two problems\" in the previous round sounds unsolvable.\n We should question if it was really an \"issue\" worth \"resolving\",\n though.\n\n"},{"id":"471608","messageId":"87357ischs.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqqedr28wwb.fsf@gitster.g","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-02-06T21:35:59Z","receivedAt":"2023-02-06T21:36:06Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> Junio C Hamano writes:\n>>\n>>\n>>> * so/diff-merges-more (2022-12-18) 5 commits\n>>> - diff-merges: improve --diff-merges documentation\n>>> - diff-merges: issue warning on lone '-m' option\n>>> - diff-merges: support list of values for --diff-merges\n>>> - diff-merges: implement log.diffMerges-m-imply-p config\n>>> - diff-merges: implement [no-]hide option and log.diffMergesHide config\n>>>\n>>> Assorted updates to \"--diff-merges=X\" option.\n>>>\n>>> May want to discard. Breaking compatibility does not seem worth it.\n>>> source: <20221217132955.108542-1-sorganov@gmail.com>\n>>\n>> Hi Junio,\n>>\n>> This does not break any compatibility, as far as me and I believe\n>> reviewers of these series are aware.\n>\n> The last paragraphs in the review two months ago still describe what\n> this series does fairly accurately, I think.\n>\n> These patches do look like a good approach to solve the first point\n> among the \"two problems\" in the previous round. Thanks for working\n> on it.\n>\n> IIRC, the previous round (why is this round marked as v1, by the\n> way?) was reviewed by some folks, so lets wait to hear from them\n> how this round does better.\n>\n> Unfortunately, I do not think of any \"solution\" that would avoid\n> breaking folks, if its end goal is to flip the default, either by\n> hardcoding or with a configuration variable. IOW, the other one\n> among the \"two problems\" in the previous round sounds unsolvable.\n> We should question if it was really an \"issue\" worth \"resolving\",\n> though.\n\nWell, we may end up flipping or not flipping the default (even though\nI'd prefer we indeed do), the series are still valid either way, as they\nallow *me* (or anybody else who prefers more useful '-m' behavior) to\nflip the switch for myself, locally.\n\nAlso, the only patch that got some resistance from reviewers has been\nremoved from the series, so I don't see anything left that'd prevent\nthis from being merged.\n\nFrom my POV the only remotely questionable patch is:\n\n- diff-merges: issue warning on lone '-m' option\n\nand I hereby agree to remove it if it feels wrong to you.\n\nLet me state it cleanly: once these are accepted, I'll turn\nlog.diffMerges-m-imply-p on for myself, and will suggest it to others\nwho already asked about '-m' inconsistency, or will ask in the future.\n\nThanks,\n-- Sergey Organov\n"},{"id":"471668","messageId":"kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59186","inReplyTo":"871qn5pyez.fsf@osv.gnss.ru","subject":"so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-02-07T04:06:00Z","receivedAt":"2023-02-07T04:06:12Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Hi Sergey,\n\nSergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>\n>> * so/diff-merges-more (2022-12-18) 5 commits\n>> - diff-merges: improve --diff-merges documentation\n>> - diff-merges: issue warning on lone '-m' option\n>> - diff-merges: support list of values for --diff-merges\n>> - diff-merges: implement log.diffMerges-m-imply-p config\n>> - diff-merges: implement [no-]hide option and log.diffMergesHide config\n>>\n>> Assorted updates to \"--diff-merges=X\" option.\n>>\n>> May want to discard. Breaking compatibility does not seem worth it.\n>> source: <20221217132955.108542-1-sorganov@gmail.com>\n>\n> Hi Junio,\n>\n> This does not break any compatibility, as far as me and I believe\n> reviewers of these series are aware.\n\nMy 2 cents (which maybe lines up with what Junio meant) is that this\nseries doesn't break compatibility on its own, but IMO the approach\ndoesn't make sense unless we also intend to flip the default somewhere\ndown the line. \"log.diffMerges-m-imply-p\" is a really specific config\noption, and it needs to justify its addition with an outsized benefit.\n\nI don't think it meets that bar, the only use cases I can think of are:\n\n- Before we change the default, setting \"log.diffMerges-m-imply-p=true\"\n gives the 'nicer' behavior. But if the user had the permissions and\n knowledge to do so, they could just pass \"-p\" instead for correctness.\n\n If the goal is to reduce typing, then we could add a different CLI\n flag that would behave like \"-m -p\", or we could teach \"git diff\" to\n accept proper single-character flags. Either of these options would be\n more discoverable and cleaner.\n\n- After we change the default, setting \"log.diffMerges-m-imply-p=false\"\n gives the old behavior, which might be needed if the user is running\n scripts written in for an older version of Git. I would find this\n compelling, but as Junio mentioned [1], breaking compatibility doesn't\n seem worth it, so this point is moot. On the other hand, neither of\n the alternative approaches break compatibility, so they're superior in\n this regard too.\n\n- The only legitimate use case I think of is something like a script\n that uses \"-m\" assuming that it implied \"-p\", AND the user has no\n ability to modify the script. Then the user's only recourse is to set\n a config. But this is so exotic that I don't think this is a good\n enough reason to have the config.\n\nI wouldn't mind seeing a version of these patches without\n\"log.diffMerges-m-imply-p=false\" , but in its current form, I agree with\nJunio that this isn't worth it.\n\n[1] https://lore.kernel.org/git/xmqqbko1xv86.fsf@gitster.g/\n"},{"id":"471681","messageId":"87wn4tej2f.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-02-07T12:50:00Z","receivedAt":"2023-02-07T12:50:08Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> Hi Sergey,\n>\n> Sergey Organov writes:\n>\n>> Junio C Hamano writes:\n>>\n>>\n>>> * so/diff-merges-more (2022-12-18) 5 commits\n>>> - diff-merges: improve --diff-merges documentation\n>>> - diff-merges: issue warning on lone '-m' option\n>>> - diff-merges: support list of values for --diff-merges\n>>> - diff-merges: implement log.diffMerges-m-imply-p config\n>>> - diff-merges: implement [no-]hide option and log.diffMergesHide config\n>>>\n>>> Assorted updates to \"--diff-merges=X\" option.\n>>>\n>>> May want to discard. Breaking compatibility does not seem worth it.\n>>> source: <20221217132955.108542-1-sorganov@gmail.com>\n>>\n>> Hi Junio,\n>>\n>> This does not break any compatibility, as far as me and I believe\n>> reviewers of these series are aware.\n>\n> My 2 cents (which maybe lines up with what Junio meant) is that this\n> series doesn't break compatibility on its own, but IMO the approach\n> doesn't make sense unless we also intend to flip the default somewhere\n> down the line. \"log.diffMerges-m-imply-p\" is a really specific config\n> option, and it needs to justify its addition with an outsized benefit.\n>\n> I don't think it meets that bar, the only use cases I can think of are:\n>\n> - Before we change the default, setting \"log.diffMerges-m-imply-p=true\"\n> gives the 'nicer' behavior. But if the user had the permissions and\n> knowledge to do so, they could just pass \"-p\" instead for\n> correctness.\n\nExcept I can't configure this in centralized manner for multiple users\non our host machine?\n\n>\n> If the goal is to reduce typing, then we could add a different CLI\n> flag that would behave like \"-m -p\", or we could teach \"git diff\" to\n> accept proper single-character flags. Either of these options would be\n> more discoverable and cleaner.\n\nThe only drawback of this is that this leaves \"-m\" inconsistent with no\napparent reason.\n\nOTOH, teaching \"git log\" to use common options machinery to accept \"-pm\"\nlooks to be quite a project, and doesn't fix '-m' inconsistency anyway.\n\nThen, out of curiosity, what do you think was the reason to change\n\"--cc\" behavior to match that of \"-c\"? To save typing? To me, changing\n\"-m\" accordingly is an improvement to the overall feel of git interface,\nthe same way as changing \"--cc\" was.\n\nThat said, if we decide to go for new option, I'd suggest to add \"-d\",\nand probably obsolete \"-m\". This does not look to me as appealing as\nfinally fixing \"-m\" though.\n\n>\n> - After we change the default, setting \"log.diffMerges-m-imply-p=false\"\n> gives the old behavior, which might be needed if the user is running\n> scripts written in for an older version of Git. I would find this\n> compelling, but as Junio mentioned [1], breaking compatibility doesn't\n> seem worth it, so this point is moot. On the other hand, neither of\n> the alternative approaches break compatibility, so they're superior in\n> this regard too.\n\nThe only incompatibility found was obsolete use of \"--first-parent -m\",\nthat was the only use-case for \"-m\" for a long time, and this could\nprobably be detected and handled specially, though it sounds like a\nnasty kludge.\n\n>\n> - The only legitimate use case I think of is something like a script\n> that uses \"-m\" assuming that it implied \"-p\", AND the user has no\n> ability to modify the script. Then the user's only recourse is to set\n> a config. But this is so exotic that I don't think this is a good\n> enough reason to have the config.\n\nYes, let's just change \"-m\" behavior then, and let exotic scripts just\ndrop \"-m\", as it's not needed for a long time already in the combo\n\"--first-parent -m\" that currently reads \"--first-parent\".\n\n>\n> I wouldn't mind seeing a version of these patches without\n> \"log.diffMerges-m-imply-p=false\" , but in its current form, I agree with\n> Junio that this isn't worth it.\n>\n> [1] https://lore.kernel.org/git/xmqqbko1xv86.fsf@gitster.g/\n\nAll you say is understood and are valid arguments, but then we will\ncontinue to face pretty valid confusion of why \"-m\" behaves differently\nfrom \"-c/--cc/--remerge\". I personally don't care, provided I get a way\nto make it behave sanely, and that's what \"log.diffMerges-m-imply-p\"\npatch does.\n\nAs a kind of complaint, it was simple 1 patch from Junio once upon a\ntime to change \"--cc\" to a sane behavior of implying -p, and now it\ntakes rounds and rounds to do the same for -m. This is rather sad.\n\nFinally, event without \"log.diffMerges-m-imply-p\", the rest of the\nseries still makes sense, so if the conclusion is to remove it, let's\nstill merge the rest, please! Let me know, and I'll then remove the\npatch and will change documentation accordingly.\n\nThanks,\n-- Sergey Organov\n"},{"id":"471781","messageId":"4f48047d-50c2-5f59-82e0-dbafd3b380d3@github.com","threadId":"59186","inReplyTo":"xmqqr0v7o0pp.fsf@gitster.g","subject":"ds/bundle-uri-5 (was: Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-02-08T17:22:53Z","receivedAt":"2023-02-08T17:22:58Z","isPatch":false,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> * ds/bundle-uri-5 (2023-01-31) 11 commits\n> - bundle-uri: test missing bundles with heuristic\n> - bundle-uri: store fetch.bundleCreationToken\n> - fetch: fetch from an external bundle URI\n> - bundle-uri: drop bundle.flag from design doc\n> - clone: set fetch.bundleURI if appropriate\n> - bundle-uri: download in creationToken order\n> - bundle-uri: parse bundle..creationToken values\n> - bundle-uri: parse bundle.heuristic=creationToken\n> - t5558: add tests for creationToken heuristic\n> - bundle: verify using check_connected()\n> - bundle: test unbundling with incomplete history\n> \n> The bundle-URI subsystem adds support for creation-token heuristics\n> to help incremental fetches.\n> \n> Will merge to 'next'?\n> source: \n\nApologies for not responding sooner, but I reviewed the most recent version\nof this series and all of my previous feedback was addressed. Overall, I\nthink it looks good and is ready for merging to 'next'.\n\nThanks!\n-Victoria\n\n"},{"id":"472892","messageId":"87wn402u3n.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"87357ischs.fsf@osv.gnss.ru","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-01T18:40:28Z","receivedAt":"2023-03-01T18:40:54Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Hi, Junio!\n\nSergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>> Sergey Organov writes:\n>>\n>>> Junio C Hamano writes:\n>>>\n>>>\n>>>> * so/diff-merges-more (2022-12-18) 5 commits\n>>>> - diff-merges: improve --diff-merges documentation\n>>>> - diff-merges: issue warning on lone '-m' option\n>>>> - diff-merges: support list of values for --diff-merges\n>>>> - diff-merges: implement log.diffMerges-m-imply-p config\n>>>> - diff-merges: implement [no-]hide option and log.diffMergesHide config\n>>>>\n>>>> Assorted updates to \"--diff-merges=X\" option.\n>>>>\n>>>> May want to discard. Breaking compatibility does not seem worth it.\n>>>> source: <20221217132955.108542-1-sorganov@gmail.com>\n>>>\n>>> Hi Junio,\n>>>\n>>> This does not break any compatibility, as far as me and I believe\n>>> reviewers of these series are aware.\n>>\n>> The last paragraphs in the review two months ago still describe what\n>> this series does fairly accurately, I think.\n>>\n>> These patches do look like a good approach to solve the first point\n>> among the \"two problems\" in the previous round. Thanks for working\n>> on it.\n>>\n>> IIRC, the previous round (why is this round marked as v1, by the\n>> way?) was reviewed by some folks, so lets wait to hear from them\n>> how this round does better.\n>>\n>> Unfortunately, I do not think of any \"solution\" that would avoid\n>> breaking folks, if its end goal is to flip the default, either by\n>> hardcoding or with a configuration variable. IOW, the other one\n>> among the \"two problems\" in the previous round sounds unsolvable.\n>> We should question if it was really an \"issue\" worth \"resolving\",\n>> though.\n>\n> Well, we may end up flipping or not flipping the default (even though\n> I'd prefer we indeed do), the series are still valid either way, as they\n> allow *me* (or anybody else who prefers more useful '-m' behavior) to\n> flip the switch for myself, locally.\n>\n> Also, the only patch that got some resistance from reviewers has been\n> removed from the series, so I don't see anything left that'd prevent\n> this from being merged.\n>\n> From my POV the only remotely questionable patch is:\n>\n> - diff-merges: issue warning on lone '-m' option\n>\n> and I hereby agree to remove it if it feels wrong to you.\n>\n> Let me state it cleanly: once these are accepted, I'll turn\n> log.diffMerges-m-imply-p on for myself, and will suggest it to others\n> who already asked about '-m' inconsistency, or will ask in the future.\n\nThis is still marked as \"probably won't merge\" in the recent \"what's\ncooking\". Could it be merged, please?\n\nThanks,\n-- Sergey Organov\n"},{"id":"472904","messageId":"xmqq356om831.fsf@gitster.g","threadId":"59186","inReplyTo":"87wn402u3n.fsf@osv.gnss.ru","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-01T22:15:46Z","receivedAt":"2023-03-01T22:15:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n>>>>> May want to discard. Breaking compatibility does not seem worth it.\n>>>>> source: <20221217132955.108542-1-sorganov@gmail.com>\n>>>> ...\n> This is still marked as \"probably won't merge\" in the recent \"what's\n> cooking\". Could it be merged, please?\n\nI found the explanation given by Glen last time you mentioned the\ntopic much better written than anything I would write myself, and I\nhaven't seen any new input in the message I am responding to, so...\n\n https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nThanks.\n"},{"id":"472905","messageId":"878rgg2jnq.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqq356om831.fsf@gitster.g","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-01T22:26:01Z","receivedAt":"2023-03-01T22:26:16Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>>>>>> May want to discard. Breaking compatibility does not seem worth it.\n>>>>>> source: <20221217132955.108542-1-sorganov@gmail.com>\n>>>>> ...\n>> This is still marked as \"probably won't merge\" in the recent \"what's\n>> cooking\". Could it be merged, please?\n>\n> I found the explanation given by Glen last time you mentioned the\n> topic much better written than anything I would write myself, and I\n> haven't seen any new input in the message I am responding to, so...\n>\n> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nI believe I've addressed it with this:\n\nhttps://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/\n\nand seen no follow-up since then.\n\nThanks,\n-- Sergey Organov\n"},{"id":"472910","messageId":"kl6lk0003u41.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59186","inReplyTo":"878rgg2jnq.fsf@osv.gnss.ru","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-01T23:54:54Z","receivedAt":"2023-03-01T23:55:32Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Hi Sergey!\n\nSergey Organov writes:\n\n>>> This is still marked as \"probably won't merge\" in the recent \"what's\n>>> cooking\". Could it be merged, please?\n>>\n>> I found the explanation given by Glen last time you mentioned the\n>> topic much better written than anything I would write myself, and I\n>> haven't seen any new input in the message I am responding to, so...\n>>\n>> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/\n>\n> I believe I've addressed it with this:\n>\n> https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/\n>\n> and seen no follow-up since then.\n\nSorry for the lack of response, especially if it seemed like your\nconcerns were not listened to. I did try crafting a response when it was\nfirst written, but it didn't end up adding anything new and valuable to\nthe conversation, so I dropped it.\n\nI'll leave an updated response, but unfortunately, I am still of the\nopinion that we shouldn't merge this series.\n"},{"id":"472911","messageId":"kl6lh6v43s4g.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59186","inReplyTo":"87wn4tej2f.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-02T00:37:51Z","receivedAt":"2023-03-02T00:37:57Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Sergey Organov writes:\n\n>> If the goal is to reduce typing, then we could add a different CLI\n>> flag that would behave like \"-m -p\", or we could teach \"git diff\" to\n>> accept proper single-character flags. Either of these options would be\n>> more discoverable and cleaner.\n>\n> The only drawback of this is that this leaves \"-m\" inconsistent with no\n> apparent reason.\n> [...]\n> Then, out of curiosity, what do you think was the reason to change\n> \"--cc\" behavior to match that of \"-c\"? To save typing? To me, changing\n> \"-m\" accordingly is an improvement to the overall feel of git interface,\n> the same way as changing \"--cc\" was.\n\nFWIW, I agree that having an inconsistent \"-m\" is quite an eyesore, and\nthere isn't a particularly good reason for it.\n\nThe unfortunate reality of Hyrum's law is that some 'mistakes' or CLI\ncleanups are difficult or even downright impossible to fix. I'd consider\n\"-m on its own does nothing\" one of those.\n\n> All you say is understood and are valid arguments, but then we will\n> continue to face pretty valid confusion of why \"-m\" behaves differently\n> from \"-c/--cc/--remerge\". I personally don't care, provided I get a way\n> to make it behave sanely, and that's what \"log.diffMerges-m-imply-p\"\n> patch does.\n\nWe could say, e.g.:\n\n \"-m\" is inconsistent, but we can't change it for backwards\n compatibility reasons. Please do not use it, use \"-d\" instead, because\n that is more sensible.\n\n> As a kind of complaint, it was simple 1 patch from Junio once upon a\n> time to change \"--cc\" to a sane behavior of implying -p, and now it\n> takes rounds and rounds to do the same for -m. This is rather sad.\n\nIt is not because Junio has special privileges or anything of the sort,\nthough. My understanding is that it was just an accident that \"-m\"\n_happened_ to be misused by enough people that we deemed that breakage\nunacceptable, whereas \"--cc\" just _happened_ to be fixable.\n\nFWIW, I might have also been on your side when we rolled back the first\n\"-m\" implies \"-p\" change, but the consensus since then is that breaking\nbackwards compatibility just isn't worth it in this case, and it would\ntake a herculean effort to change that consensus. It might not be\ncompletely impossible though, more on that later...\n\n> Finally, event without \"log.diffMerges-m-imply-p\", the rest of the\n> series still makes sense, so if the conclusion is to remove it, let's\n> still merge the rest, please! Let me know, and I'll then remove the\n> patch and will change documentation accordingly.\n\nOops. Sorry, I missed this the first time I read this message. This\nalone should have warranted a response.\n\nI'm not convinced that the series makes sense without\n\"log.diffMerges-m-imply-p\":\n\n* The main patch is\n\n diff-merges: implement [no-]hide option and log.diffMergesHide config\n\n which gives us a way to redefine \"-m\" as \"--diff-merges=hide\n --diff-merges=on\". However, we haven't seen any compelling reasons to\n use --diff-merges=hide [1]. I'm also fairly convinced that if we go\n back in time, \"-m\" wouldn't have the semantics of 'do nothing unless\n -p is also given', and I don't think we should immortalize a mistake\n by giving it an explicit option.\n\n All the other patches in their current form are dependent on this\n patch going in.\n\n* diff-merges: support list of values for --diff-merges\n\n This only makes sense if we want to accept multiple values, which we\n don't without the main patch.\n\n* diff-merges: issue warning on lone '-m' option\n diff-merges: improve --diff-merges documentation\n\n These are docs and UX cleanups as a result of the main patch.\n\n_Maybe_ the number of lone \"-m\" users will shrink over time to the point\nwhere we could make the switch? We could prepare for that by warning\nthat lone \"-m\" is a no-op now, but might imply \"-p\" at some point in the\nfuture, and then after X amount of time, we might say that users have\nhad enough warning and actually change the default. I find this\nunlikely, but not impossible. However, the patches don't prepare us for\nthis path, and that would deserve new patches and a different discussion\nfrom what we've been having.\n\n1. https://lore.kernel.org/git/kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com/\n"},{"id":"472919","messageId":"875ybjrzg0.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"kl6lk0003u41.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-02T14:38:07Z","receivedAt":"2023-03-02T14:38:15Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> Hi Sergey!\n>\n> Sergey Organov writes:\n>\n>>>> This is still marked as \"probably won't merge\" in the recent \"what's\n>>>> cooking\". Could it be merged, please?\n>>>\n>>> I found the explanation given by Glen last time you mentioned the\n>>> topic much better written than anything I would write myself, and I\n>>> haven't seen any new input in the message I am responding to, so...\n>>>\n>>> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/\n>>\n>> I believe I've addressed it with this:\n>>\n>> https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/\n>>\n>> and seen no follow-up since then.\n>\n> Sorry for the lack of response, especially if it seemed like your\n> concerns were not listened to. I did try crafting a response when it was\n> first written, but it didn't end up adding anything new and valuable to\n> the conversation, so I dropped it.\n>\n> I'll leave an updated response, but unfortunately, I am still of the\n> opinion that we shouldn't merge this series.\n\nThe primary question here is: why don't allow me to get the behavior I\nneed, provided it has no impact on anybody else, unless they decide they\nneed it too? I've already performed \"you need it, -- do it yourself\"\ndance, along with documentation and tests, so may I be rewarded by\ngetting this feature finally in, please?\n\nThanks,\n-- Sergey Organov\n"},{"id":"472921","messageId":"xmqqr0u7ku3j.fsf@gitster.g","threadId":"59186","inReplyTo":"kl6lh6v43s4g.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-02T16:15:28Z","receivedAt":"2023-03-02T16:15:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n>> Finally, event without \"log.diffMerges-m-imply-p\", the rest of the\n>> series still makes sense, so if the conclusion is to remove it, let's\n>> still merge the rest, please! Let me know, and I'll then remove the\n>> patch and will change documentation accordingly.\n>\n> Oops. Sorry, I missed this the first time I read this message. This\n> alone should have warranted a response.\n\nHmph. I agreed with you that the last step to add the configuration\nwould not make sense unless we are planning to break users later,\nbut I have been under the impression that the remainder were OK\nclean-ups. Perhaps it is mostly because I read them so long ago and\nforgot the details X-<.\n\n> I'm not convinced that the series makes sense without\n> \"log.diffMerges-m-imply-p\":\n>\n> * The main patch is\n>\n> diff-merges: implement [no-]hide option and log.diffMergesHide config\n>\n> which gives us a way to redefine \"-m\" as \"--diff-merges=hide\n> --diff-merges=on\". However, we haven't seen any compelling reasons to\n> use --diff-merges=hide [1]. I'm also fairly convinced that if we go\n> back in time, \"-m\" wouldn't have the semantics of 'do nothing unless\n> -p is also given', and I don't think we should immortalize a mistake\n> by giving it an explicit option.\n>\n> All the other patches in their current form are dependent on this\n> patch going in.\n>\n> * diff-merges: support list of values for --diff-merges\n>\n> This only makes sense if we want to accept multiple values, which we\n> don't without the main patch.\n\nNow you mention it (and show example in the previous bullet point),\nI have to agree that we would not want this, not because we do not\nwant to have --diff-merges with multiple values, but because it\nintroduces an odd-man-out option that is not \"last one wins\" that is\nnot used anywhere else in the UI.\n\nNo response does not necessarily mean an agreement, but it indeed\nhelped in this case to be explicit.\n\nThanks for the clarification. \n"},{"id":"472924","messageId":"87wn3zqefx.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqqr0u7ku3j.fsf@gitster.g","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-02T16:57:06Z","receivedAt":"2023-03-02T16:57:14Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>>> Finally, event without \"log.diffMerges-m-imply-p\", the rest of the\n>>> series still makes sense, so if the conclusion is to remove it, let's\n>>> still merge the rest, please! Let me know, and I'll then remove the\n>>> patch and will change documentation accordingly.\n>>\n>> Oops. Sorry, I missed this the first time I read this message. This\n>> alone should have warranted a response.\n>\n> Hmph. I agreed with you that the last step to add the configuration\n> would not make sense unless we are planning to break users later,\n\nIt does make sense to me, and to anybody who does want -m to behave the\nsame sane way the rest of similar options behave. I've already got it\nyou don't care, but there are people here who do.\n\n> but I have been under the impression that the remainder were OK\n> clean-ups. Perhaps it is mostly because I read them so long ago and\n> forgot the details X-<.\n\nIt's not a cleanup, it's rather adding missed feature that allows to get\nprecise '-m' behavior with --diff-merges. To remind you, there was a\ndiscussion back then when --diff-merges= options were introduced, do\nthey need to immediately cause output of diffs for merge commits, or\nsuppress the output till '-p' is specified as well, like '--cc'\noriginally did, and like '-m' still does.\n\nThe conclusion at that time was that it makes sense to take immediate\naction as this allows to output diffs for merge commits only, a new\nopportunity that was not present before.\n\nHowever, by making such decision we lost ability to provide exact\nbehavior of -m using --diff-merges= set of options. This has been\npointed out later to me in the list, and felt obliged to finish\nimplementation by providing the feature.\n\n --diff-merges=hide\n\nis exactly that.\n\n>\n>> I'm not convinced that the series makes sense without\n>> \"log.diffMerges-m-imply-p\":\n>>\n>> * The main patch is\n>>\n>> diff-merges: implement [no-]hide option and log.diffMergesHide config\n>>\n>> which gives us a way to redefine \"-m\" as \"--diff-merges=hide\n>> --diff-merges=on\". However, we haven't seen any compelling reasons to\n>> use --diff-merges=hide [1].\n\nI think --diff-merges should be complete and be able to provide exact \"-m\"\nbehavior, rendering the latter pure shortcut.\n\nComplete orthogonal interfaces are good thing by themselves, and useful\napplications of them are often found later. That's common knowledge.\n\n>> I'm also fairly convinced that if we go\n>> back in time, \"-m\" wouldn't have the semantics of 'do nothing unless\n>> -p is also given', and I don't think we should immortalize a mistake\n>> by giving it an explicit option.\n\nYep, and I provided a config option that fixes this mistake. What's\nwrong about it? The complete orthogonal interface finalized by the\naforementioned feature allows me to achieve this goal easily.\n\n>>\n>> All the other patches in their current form are dependent on this\n>> patch going in.\n>>\n>> * diff-merges: support list of values for --diff-merges\n>>\n>> This only makes sense if we want to accept multiple values, which we\n>> don't without the main patch.\n>\n> Now you mention it (and show example in the previous bullet point),\n> I have to agree that we would not want this, not because we do not\n> want to have --diff-merges with multiple values, but because it\n> introduces an odd-man-out option that is not \"last one wins\" that is\n> not used anywhere else in the UI.\n\nIt's still the last one wins, and I believe I've carefully described it\nin the documentation of the options.\n\nThanks,\n-- Sergey Organov\n\n"},{"id":"473089","messageId":"kl6lzg8pcygl.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59186","inReplyTo":"87wn3zqefx.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-06T22:22:02Z","receivedAt":"2023-03-06T22:22:44Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Sergey Organov writes:\n\n>> but I have been under the impression that the remainder were OK\n>> clean-ups. Perhaps it is mostly because I read them so long ago and\n>> forgot the details X-<.\n>\n> It's not a cleanup, it's rather adding missed feature that allows to get\n> precise '-m' behavior with --diff-merges.\n> ...\n> However, by making such decision we lost ability to provide exact\n> behavior of -m using --diff-merges= set of options. This has been\n> pointed out later to me in the list, and felt obliged to finish\n> implementation by providing the feature.\n>\n> --diff-merges=hide\n>\n> is exactly that.\n\nThis is exactly the \"immortaliz[ing] a mistake\" that I mentioned\nupthread, by turning a UX wart (-m doesn't imply -p) that neither of us\nlikes into a feature. I'd be in favor of getting rid of the wart\naltogether (i.e. let's find a path that lets us make -m imply -p in the\nfuture), but not rebranding it as a feature.\n\n>>> I'm not convinced that the series makes sense without\n>>> \"log.diffMerges-m-imply-p\":\n>>>\n>>> * The main patch is\n>>>\n>>> diff-merges: implement [no-]hide option and log.diffMergesHide config\n>>>\n>>> which gives us a way to redefine \"-m\" as \"--diff-merges=hide\n>>> --diff-merges=on\". However, we haven't seen any compelling reasons to\n>>> use --diff-merges=hide [1].\n>\n> I think --diff-merges should be complete and be able to provide exact \"-m\"\n> behavior, rendering the latter pure shortcut.\n>\n> Complete orthogonal interfaces are good thing by themselves, and useful\n> applications of them are often found later. That's common knowledge.\n\nFor the user who wants it, yes, but these interfaces come with a\nmaintenance cost and a cost to the user who just wants the simpler\ninterface.\n\nI think that our goals here are actually the same: we both want Git to\nbe as simple and usable as possible. We disagree on whether this series\nmakes Git simpler or more complex (and I don't think either of us will\nchange our minds any time soon), but I think we can agree on an overall\ndirection. With that in mind, I'd be willing to discuss other approaches\nthat get us closer to \"-m implies -p\", if you have any.\n\n>>> I'm also fairly convinced that if we go\n>>> back in time, \"-m\" wouldn't have the semantics of 'do nothing unless\n>>> -p is also given', and I don't think we should immortalize a mistake\n>>> by giving it an explicit option.\n>\n> Yep, and I provided a config option that fixes this mistake. What's\n> wrong about it? The complete orthogonal interface finalized by the\n> aforementioned feature allows me to achieve this goal easily.\n\nAddressed above.\n\n"},{"id":"473115","messageId":"878rg8j2vg.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"kl6lzg8pcygl.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-07T10:02:27Z","receivedAt":"2023-03-07T10:02:37Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> Sergey Organov writes:\n>\n>>> but I have been under the impression that the remainder were OK\n>>> clean-ups. Perhaps it is mostly because I read them so long ago and\n>>> forgot the details X-<.\n>>\n>> It's not a cleanup, it's rather adding missed feature that allows to get\n>> precise '-m' behavior with --diff-merges.\n>> ...\n>> However, by making such decision we lost ability to provide exact\n>> behavior of -m using --diff-merges= set of options. This has been\n>> pointed out later to me in the list, and felt obliged to finish\n>> implementation by providing the feature.\n>>\n>> --diff-merges=hide\n>>\n>> is exactly that.\n>\n> This is exactly the \"immortaliz[ing] a mistake\" that I mentioned\n> upthread, by turning a UX wart (-m doesn't imply -p) that neither of us\n> likes into a feature. I'd be in favor of getting rid of the wart\n> altogether (i.e. let's find a path that lets us make -m imply -p in the\n> future), but not rebranding it as a feature.\n\nWe are walking in rounds here. Let me try to find at least some common\nground. Could we try to agree on *one* of these 2 mutually exclusive\nstatements, please:\n\n1. Current -m behavior is useful\n\n2. Current -m behavior is useless\n\nTo me it looks like Junio thinks 1 is the case, and you, Glen, think\nit's 2 that is the case, and you both somewhat oppose to my patch\nseries, believing you agree with each other, that in fact does not seem\nto be the case from my POV, effectively leaving me no way to adopt the\nseries so that they are actually accepted.\n\nThanks,\n-- Sergey Organov\n\n"},{"id":"473130","messageId":"xmqqzg8ozbuh.fsf@gitster.g","threadId":"59186","inReplyTo":"878rg8j2vg.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-07T17:54:14Z","receivedAt":"2023-03-07T18:00:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> We are walking in rounds here. Let me try to find at least some common\n> ground. Could we try to agree on *one* of these 2 mutually exclusive\n> statements, please:\n>\n> 1. Current -m behavior is useful\n>\n> 2. Current -m behavior is useless\n>\n> To me it looks like Junio thinks 1 is the case, and you, Glen, think\n> it's 2 that is the case, ...\n\nI agree you two are going around in circles and it may be time to\nagree to disagree and move on.\n\nI am not Glen but I do not think anybody is arguing \"-m\" that\ndoes not do anything by itself is useful. By definition, without\nhelp with \"-p\", it is a no-op, so by itself, it cannot be useful.\n\nThe reason why I am not enthused by your series is because changing\nbehaviour of \"-m\" can be harmful, because \"-m -p\" is not the first\nchoice people would make when they choose to view what a merge\ncommit did.\n\nBack when \"-m\" was invented (yes, \"-p\" came first before \"-m\"),\npeople were fairly content with \"git log -p\" that shows only\nindividual changes. After all, merges were to integrate what these\nindividual changes on each side of the merged history did. And you\nneed to be aware that \"-c\" and \"--cc\" did not exist, let alone\n\"--remerge-diff\", even as a concept. The philosophy of Git back\nthen stressed that these parts of Git were working as building\nblocks to feed necessary data to be used by tools people who were\nworking at higher, end-user facing layers, would write. And because\nwe ourselves have been mostly content with \"git log -p\" output that\nomits patches for merge commits so far, a quick approach to give\noutput for such tools to consume without losing information was what\nbecame \"-m -p\" output that shows pairwise diffs that such tools can\ncombine or do whatever they want to do. This choice was also\ninfluenced by the fact that we considered stronger than we do now\nthat all sides of a merge are equal---the idea that the first parent\nchain was somehow special came much leter with the \"--first-parent\"\noption.\n\nSo, given that background, \"-m\" that came as a modifier for what\nhappens (only) when \"-p\" was in use, is perfectly understandable.\nIn hindsight, it may appear to you that it should imply \"-p\", and\nthat it is an oversight that it does not imply it.\n\nBut it is more like how \"git log --histogram\" does not produce patch\noutput. The mental model that allows it is that \"--histogram\" is to\naffect the way how \"-p\" computes the diff---a purist in you may\nargue that it should also imply \"-p\", but in reality, nobody\ncomplained that \"--histogram\" does not imply \"-p\", probably because\nnobody sane would say \"--histogram\" when they do not mean to show\ndiff out of \"git log\" anyway.\n\nAs somebody who saw how various options were invented while they\nwere added to the system, I view \"-m\" as an option to decide what\n\"-p\" does when encountered a merge commit, and do not find it any\nmore odd that it is a no-op without \"-p\" or it does not imply \"-p\"\nthan \"--histogram\" vs \"-p\".\n\n"},{"id":"473229","messageId":"87jzzqzy20.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqqzg8ozbuh.fsf@gitster.g","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-08T22:19:03Z","receivedAt":"2023-03-08T22:19:13Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> We are walking in rounds here. Let me try to find at least some common\n>> ground. Could we try to agree on *one* of these 2 mutually exclusive\n>> statements, please:\n>>\n>> 1. Current -m behavior is useful\n>>\n>> 2. Current -m behavior is useless\n>>\n>> To me it looks like Junio thinks 1 is the case, and you, Glen, think\n>> it's 2 that is the case, ...\n>\n> I agree you two are going around in circles and it may be time to\n> agree to disagree and move on.\n\nI'm willing not only to disagree, but even to agree on any of these 2\noptions, but I can't (dis)agree on both, as they are mutually exclusive.\nYeah, it could be seen as my personal problem, but, well...\n\n> I am not Glen but I do not think anybody is arguing \"-m\" that does not\n> do anything by itself is useful.\n\nI'm sorry, I still can't quite understand your position.\n\nDo you think exact current behavior of \"-m\" is useful, or is it useless?\n\n> By definition, without help with \"-p\", it is a no-op, so by itself, it\n> cannot be useful.\n\nNo, it's not a no-op as I see it, it's just that its effect is hidden\ntill \"-p\" is specified as well, and that's the exact \"current behavior\"\nthat I'm asking about. Is it useful, or not?\n\nPlease let me remind you that at previous rounds there was the commit\nthat reverted my earlier \"Let -m imply -p\" patch, and that revert\ninsists lone \"-m\" is useful, and you accepted it, and pointed me to the\nexplanation there, telling me that there is pretty solid use-case for a\nlone \"-m\".\n\nI agreed with you at that time, and then followed with current patch\nseries created exactly based on that earlier agreement, that now somehow\nturns back to disagreement, leaving me sad and confused.\n\nProvided current -m behavior is indeed useful, in these series:\n\n1. Current -m behavior is now supported by --diff-merges (as presumably\n being useful.)\n\n2. Implying -p by -m is made an option (not to break those presumably\n valid uses.)\n\nAnd now these series are to be rejected on the ground that current \"-m\"\nbehavior is useless? Then let's revert the revert?\n\n>\n> The reason why I am not enthused by your series is because changing\n> behaviour of \"-m\" can be harmful, because \"-m -p\" is not the first\n> choice people would make when they choose to view what a merge\n> commit did.\n\nThat's your view on merge commits, but there is another view out there,\nwhere \"-m -p\" is exactly the first choice, provided \"-m\" is configured\nto give diff with respect to the first parent only. I want to have the\nsame convenience of saying just \"log -m\" as you already have with \"log\n-c\". What's wrong about it?\n\nI realize there is a large group here that looks at merge commits\ndifferently, and I respect their choice to show other kinds of\ninformation that could be derived from merges. What I can't understand\nis why another view is assumed to be a second-class citizen, sorry.\n\nPlease also note that I made behavior change an option, exactly not to\nharm those who doesn't need it, even though it's not clear how those who\ndon't use -m are to be affected anyway.\n\n>\n> Back when \"-m\" was invented (yes, \"-p\" came first before \"-m\"), people\n> were fairly content with \"git log -p\" that shows only individual\n> changes. After all, merges were to integrate what these individual\n> changes on each side of the merged history did. And you need to be\n> aware that \"-c\" and \"--cc\" did not exist, let alone \"--remerge-diff\",\n> even as a concept.\n\nYep, I'm aware of all that history now, as I went over it back and forth\na few times already, trying to figure out what't the actual ground\nbehind desire to *keep* \"-m\" different from the rest of similar options,\nbut still found none.\n\n[...]\n\n> So, given that background, \"-m\" that came as a modifier for what\n>happens (only) when \"-p\" was in use, is perfectly understandable. In\n>hindsight, it may appear to you that it should imply \"-p\", and that it\n>is an oversight that it does not imply it.\n\nMy point is that the story with -c, or --cc is roughly the same, and\nthat you yourself fixed --cc once upon a time to imply -p, so it's\nespecially confusing for me that you are now in opposition to similar\nchange to \"-m\".\n\n>\n> But it is more like how \"git log --histogram\" does not produce patch\n> output. The mental model that allows it is that \"--histogram\" is to\n> affect the way how \"-p\" computes the diff---a purist in you may\n> argue that it should also imply \"-p\", but in reality, nobody\n> complained that \"--histogram\" does not imply \"-p\", probably because\n> nobody sane would say \"--histogram\" when they do not mean to show\n> diff out of \"git log\" anyway.\n\nDo you then think that making -c/--cc imply \"-p\" was a mistake in the\nfirst place, and you try to hold me back from doing the same mistake?\n\n> As somebody who saw how various options were invented while they\n> were added to the system, I view \"-m\" as an option to decide what\n> \"-p\" does when encountered a merge commit, and do not find it any\n> more odd that it is a no-op without \"-p\" or it does not imply \"-p\"\n> than \"--histogram\" vs \"-p\".\n\nIt's fine to view \"-m\" this way, but how do you view \"-c\" then? If\ndifferently, then why?\n\nIn other words, in the above sentence, among \"--histogram -m -c\", you\neffectively group \"(--histogram -m) -c\" by leaving -c out of\nconsideration, whereas more natural grouping is rather \"--histogram (-m\n-c)\", that'd leave --histogram out of consideration.\n\nOverall, my point is that, provided we do care about consistency of Git\nUI, it's -m and -c that are to be treated the same, and it's much less\nrelevant if any of them matches treatment of --histogram.\n\nThanks,\n-- Sergey Organov\n"},{"id":"473235","messageId":"xmqqzg8mrgc8.fsf@gitster.g","threadId":"59186","inReplyTo":"87jzzqzy20.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-08T23:08:55Z","receivedAt":"2023-03-08T23:09:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> My point is that the story with -c, or --cc is roughly the same, and\n> that you yourself fixed --cc once upon a time to imply -p, so it's\n> especially confusing for me that you are now in opposition to similar\n> change to \"-m\".\n\nI think we all saw a good explanation for that already in the\nthread.\n\nThe mistake by \"--cc\" was fixed relatively quickly, but it is now\nway too late to change the behaviour of \"-m\" without hurting\nexisting users. I think I've wasted enough time on this in this\nthread already, so let's stop comparing --cc and -m now.\n\nThanks.\n"},{"id":"473283","messageId":"874jquxc67.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqqzg8mrgc8.fsf@gitster.g","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-09T13:54:40Z","receivedAt":"2023-03-09T13:56:16Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> My point is that the story with -c, or --cc is roughly the same, and\n>> that you yourself fixed --cc once upon a time to imply -p, so it's\n>> especially confusing for me that you are now in opposition to similar\n>> change to \"-m\".\n>\n> I think we all saw a good explanation for that already in the\n> thread.\n>\n> The mistake by \"--cc\" was fixed relatively quickly, but it is now\n> way too late to change the behaviour of \"-m\" without hurting\n> existing users. I think I've wasted enough time on this in this\n> thread already, so let's stop comparing --cc and -m now.\n\nThat's fine with me.\n\nI already agreed long ago that to be on the safe side we shouldn't\nsimply change -m nowadays, and addressed that concern by putting -m\nbehavior change under configuration option in the current series. So\nwhat's the reason of rejection?\n\nThanks,\n-- Sergey\n"},{"id":"473302","messageId":"kl6lh6uthlc3.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59186","inReplyTo":"874jquxc67.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-09T17:43:24Z","receivedAt":"2023-03-09T17:44:02Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Sergey Organov writes:\n\n> I already agreed long ago that to be on the safe side we shouldn't\n> simply change -m nowadays, and addressed that concern by putting -m\n> behavior change under configuration option in the current series. So\n> what's the reason of rejection?\n\nBefore we start talking in circles again, I think the responses\nelsewhere in the thread still accurately capture mine and Junio's views,\ne.g.:\n\n https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g\n https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/\n"},{"id":"473314","messageId":"878rg5vgvc.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"kl6lh6uthlc3.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-09T19:56:07Z","receivedAt":"2023-03-09T19:56:18Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> Sergey Organov writes:\n>\n>> I already agreed long ago that to be on the safe side we shouldn't\n>> simply change -m nowadays, and addressed that concern by putting -m\n>> behavior change under configuration option in the current series. So\n>> what's the reason of rejection?\n>\n> Before we start talking in circles again, I think the responses\n> elsewhere in the thread still accurately capture mine and Junio's\n> views, e.g.:\n>\n> https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g\n> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nThere were no any response from you to my recent very direct question\n(that Junio did somewhat address), but what I figured from your and\nJunio explanation so far looks like:\n\n1. The fact that -m does not imply -p is a mistake. There is no any\n reasons this exact behavior could be useful. As such, it does not\n make sense to support this exact behavior in --diff-merges. So the\n reject of --diff-merges=[no-]hide.\n\n2. This mistake is too dangerous to fix due to subtle compatibility\n problems, so we can't just fix -m behavior. Thus the reject of my\n earlier patch \"let -m imply -p\".\n\n3. Moving behavior change under option is not worth it, as nobody\n presumably needs this fixed -m behavior anyway (at least among 2\n persons that are actually opposing the changes). So the reject\n of \"add diffMerges-m-imply-p configuration option\" patch.\n\n4. Staring in the face inconsistency between -m and the rest\n of short diff-merge options is not significant enough to reconsider\n any of the above rejects.\n\nDid I outline your positions correctly and to the full extent? Anything\nelse I forgot?\n\nI want to make sure I got it exactly right, as I'd still like to tweak\nmy proposals, and I'd like to avoid the troubles I got into as much as\npossible.\n\nThanks,\n-- Sergey Organov\n"},{"id":"473383","messageId":"kl6la60kqp85.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59186","inReplyTo":"878rg5vgvc.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-10T21:19:06Z","receivedAt":"2023-03-10T21:19:25Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Sergey Organov writes:\n\n> Glen Choo writes:\n>\n>> Sergey Organov writes:\n>>\n>>> I already agreed long ago that to be on the safe side we shouldn't\n>>> simply change -m nowadays, and addressed that concern by putting -m\n>>> behavior change under configuration option in the current series. So\n>>> what's the reason of rejection?\n>>\n>> Before we start talking in circles again, I think the responses\n>> elsewhere in the thread still accurately capture mine and Junio's\n>> views, e.g.:\n>>\n>> https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g\n>> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/\n>\n> There were no any response from you to my recent very direct question\n> (that Junio did somewhat address), but what I figured from your and\n> Junio explanation so far looks like:\n\nYes, thank you for summarizing the discussion. I think the following\nsums up my position quite accurately, and I found this very helpful.\n\n> 1. The fact that -m does not imply -p is a mistake. There is no any\n> reasons this exact behavior could be useful. As such, it does not\n> make sense to support this exact behavior in --diff-merges. So the\n> reject of --diff-merges=[no-]hide.\n\nYes.\n\n> 2. This mistake is too dangerous to fix due to subtle compatibility\n> problems, so we can't just fix -m behavior. Thus the reject of my\n> earlier patch \"let -m imply -p\".\n\nYes, unfortunately. If we didn't have compatibility problems, \"-m imply\n-p\" would be very good IMO. I am not that hopeful, but perhaps there are\nways to get users to move off this compatibility problem. It would be\nsomething we would have to learn as a project though, it is not a\nproblem unique to this series.\n\n> 3. Moving behavior change under option is not worth it, as nobody\n> presumably needs this fixed -m behavior anyway (at least among 2\n> persons that are actually opposing the changes). So the reject\n> of \"add diffMerges-m-imply-p configuration option\" patch.\n\nYes, given how specific the option is (I think it would be the only\nconfig option that exists to change the behavior of a CLI option) I\ndon't think the option is worth it in light of alternatives like a new\n\"-d\" flag or supporting \"-mp\".\n\n> 4. Staring in the face inconsistency between -m and the rest\n> of short diff-merge options is not significant enough to reconsider\n> any of the above rejects.\n\nYes. Git carries many such inconsistencies.\n\n> Did I outline your positions correctly and to the full extent? Anything\n> else I forgot?\n\nI cannot think of anything else. Thank you.\n\n> I want to make sure I got it exactly right, as I'd still like to tweak\n> my proposals, and I'd like to avoid the troubles I got into as much as\n> possible.\n\nThanks. I hope that there is a good way forward for your proposals.\n\n>\n> Thanks,\n> -- Sergey Organov\n"},{"id":"473390","messageId":"xmqqjzzo46tv.fsf@gitster.g","threadId":"59186","inReplyTo":"878rg5vgvc.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-10T21:47:24Z","receivedAt":"2023-03-10T22:01:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> 1. The fact that -m does not imply -p is a mistake. There is no any\n> reasons this exact behavior could be useful. As such, it does not\n> make sense to support this exact behavior in --diff-merges. So the\n> reject of --diff-merges=[no-]hide.\n>\n> 2. This mistake is too dangerous to fix due to subtle compatibility\n> problems, so we can't just fix -m behavior. Thus the reject of my\n> earlier patch \"let -m imply -p\".\n>\n> 3. Moving behavior change under option is not worth it, as nobody\n> presumably needs this fixed -m behavior anyway (at least among 2\n> persons that are actually opposing the changes). So the reject\n> of \"add diffMerges-m-imply-p configuration option\" patch.\n>\n> 4. Staring in the face inconsistency between -m and the rest\n> of short diff-merge options is not significant enough to reconsider\n> any of the above rejects.\n\nI do not quite understand the last one (#4), and if I were to add my\nown 4., it would be that introducing --diff-merges={kind} may have\nbeen a mistake. It would have been fine and better to just let\nusers choose from whatever set of options we support, i.e. (-c,\n--cc, --remerge-diff, -m -p, -m --raw, ...).\n\nIOW, perhaps deprecate --diff-merges={kind} and eventually remove\nit, if we could. We've been fine without it and we'll be fine\nwithout it. Unfortunately it may be a bit too late for that,\nbut it certainly is much younger than \"-m\".\n\n"},{"id":"473632","messageId":"874jqj31gv.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqqjzzo46tv.fsf@gitster.g","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-17T14:18:56Z","receivedAt":"2023-03-17T14:19:24Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> 1. The fact that -m does not imply -p is a mistake. There is no any\n>> reasons this exact behavior could be useful. As such, it does not\n>> make sense to support this exact behavior in --diff-merges. So the\n>> reject of --diff-merges=[no-]hide.\n>>\n>> 2. This mistake is too dangerous to fix due to subtle compatibility\n>> problems, so we can't just fix -m behavior. Thus the reject of my\n>> earlier patch \"let -m imply -p\".\n>>\n>> 3. Moving behavior change under option is not worth it, as nobody\n>> presumably needs this fixed -m behavior anyway (at least among 2\n>> persons that are actually opposing the changes). So the reject\n>> of \"add diffMerges-m-imply-p configuration option\" patch.\n>>\n>> 4. Staring in the face inconsistency between -m and the rest\n>> of short diff-merge options is not significant enough to reconsider\n>> any of the above rejects.\n>\n> I do not quite understand the last one (#4),\n\nWell, -m does not imply -p, whereas the rest of diff-merges options\n(-c/--cc/--remerge-diff) do imply -p. This is what half of this\nlengthy discussion was about.\n\n> own 4., it would be that introducing --diff-merges={kind} may have\n> been a mistake. It would have been fine and better to just let\n> users choose from whatever set of options we support, i.e. (-c,\n> --cc, --remerge-diff, -m -p, -m --raw, ...).\n>\n> IOW, perhaps deprecate --diff-merges={kind} and eventually remove\n> it, if we could.\n\nWhy? Unlike -m vs -c they are at least self-consistent and besides allow\nto get the output that those short options do not.\n\n> We've been fine without it and we'll be fine without it. Unfortunately\n> it may be a bit too late for that, but it certainly is much younger\n> than \"-m\".\n\nI, for one, was never fine with what Git does to show diff for merges.\nThen, taking into account that introducing of --diff-merges was not\nmy idea in the first place, there should be at least two of us here\nwho were not fine.\n\nIt's fine with me that --cc is everything you need, but what I need is\nrather diff to the first parent, and I had a hope to finally get -m to\ndo it, aiming both for consistency and convenience. My 2 attempts\nperformed in different ways both failed, so, being tired of it likely\neven more than you do, I digress.\n\nThanks,\n-- Sergey Organov\n"},{"id":"473679","messageId":"xmqq8rfugbuy.fsf@gitster.g","threadId":"59186","inReplyTo":"874jqj31gv.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-18T00:08:21Z","receivedAt":"2023-03-18T00:08:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n>> I do not quite understand the last one (#4),\n>\n> Well, -m does not imply -p, whereas the rest of diff-merges options\n> (-c/--cc/--remerge-diff) do imply -p. This is what half of this\n> lengthy discussion was about.\n>\n>> own 4., it would be that introducing --diff-merges={kind} may have\n>> been a mistake. It would have been fine and better to just let\n>> users choose from whatever set of options we support, i.e. (-c,\n>> --cc, --remerge-diff, -m -p, -m --raw, ...).\n\n> It's fine with me that --cc is everything you need, but what I need is\n> rather diff to the first parent, ...\n\nI think \"show --first-parent\" should give that already. One problem\nwith \"-m implies -p\" is that it is unclear what should be done to\nthings like \"-m --raw\". Yes, we can declare an arbitrary choice\n(like \"-m implies -p unless --raw, --stat, etc. are given\") but that\nis just replacing an arbitrary rule (i.e. \"comparison with parents\nare not given for merges, unless things like --cc, --first-parent,\netc. that are specifically designed for showing merges are given;\nyou can give -m to force pairwise behaviour.\") with another one.\n\n\n\n"},{"id":"474177","messageId":"87y1nk6aa5.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"xmqq8rfugbuy.fsf@gitster.g","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-25T16:55:14Z","receivedAt":"2023-03-25T16:55:21Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>>> I do not quite understand the last one (#4),\n>>\n>> Well, -m does not imply -p, whereas the rest of diff-merges options\n>> (-c/--cc/--remerge-diff) do imply -p. This is what half of this\n>> lengthy discussion was about.\n>>\n>>> own 4., it would be that introducing --diff-merges={kind} may have\n>>> been a mistake. It would have been fine and better to just let\n>>> users choose from whatever set of options we support, i.e. (-c,\n>>> --cc, --remerge-diff, -m -p, -m --raw, ...).\n>\n>> It's fine with me that --cc is everything you need, but what I need is\n>> rather diff to the first parent, ...\n>\n> I think \"show --first-parent\" should give that already.\n\nWell, for \"git show\" even \"show -m\" does the right thing (once properly\nconfigured), as \"-p\" is implied by \"git show\".\n\nTaking \"git show\" into the picture brings yet another argument if favor\nof new \"-m\" behavior, as then \"git show -m\" and \"git log -m -n1\" will\nfinally start to produce the same result, that I'd find desirable.\n\nThat said,\n\n --diff-merges=first-parent\n\nthat could be shortened to\n\n --diff-merges=1\n\nis the universal answer that works out-of-the-box for any command the\nsame way, reliably, and then it's also\n\n -m -p\n\nif configured accordingly, that has been made available by previously\naccepted patches.\n\nThese series just did the last logical step: allowed it to be just\n\n -m\n\nif configured accordingly.\n\n> One problem with \"-m implies -p\" is that it is unclear what should be\n> done to things like \"-m --raw\".\n\nNothing specific is actually needed, as far as I'm aware, as implied -p\ndoesn't interfere with --raw. Please give particular example of a\nproblem if you foresee one.\n\nAs I see it, if there is indeed some problem with this, it should\nalready exist for -c, --cc, and --remerge-diff, and then probably needs\nto be fixed anyway. Moreover, it should also exist for \"git show\", as\nthe latter implies -p, and there is:\n\n -s, --no-patch\n Suppress diff output. Useful for commands like git show that show\n the patch by default, or to cancel the effect of --patch.\n\n[As a side-note, current behavior of implied -p, explicit -p, -s, and\n--raw with respect to each other that I figured by experiment looks\nsuspect to me. E.g., once explicit -p is given, and then canceled by -s,\nI can't get bare --raw output anymore]\n\n> Yes, we can declare an arbitrary choice (like \"-m implies -p unless\n> --raw, --stat, etc. are given\") but that is just replacing an\n> arbitrary rule [...] with another one.\n\nUh, this would be too cumbersome indeed, but fortunately it does not\nseem to be needed, see above.\n\nOverall, letting -m imply -p just makes things more consistent, even on\nthe problems side (if any), and I honestly still don't see'em.\n\nThanks,\n-- Sergey Organov\n"},{"id":"474396","messageId":"87r0t80zpg.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"87y1nk6aa5.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-29T07:43:55Z","receivedAt":"2023-03-29T07:44:08Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Sergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>> Sergey Organov writes:\n>>\n>>>> I do not quite understand the last one (#4),\n>>>\n>>> Well, -m does not imply -p, whereas the rest of diff-merges options\n>>> (-c/--cc/--remerge-diff) do imply -p. This is what half of this\n>>> lengthy discussion was about.\n>>>\n>>>> own 4., it would be that introducing --diff-merges={kind} may have\n>>>> been a mistake. It would have been fine and better to just let\n>>>> users choose from whatever set of options we support, i.e. (-c,\n>>>> --cc, --remerge-diff, -m -p, -m --raw, ...).\n>>\n>>> It's fine with me that --cc is everything you need, but what I need is\n>>> rather diff to the first parent, ...\n>>\n>> I think \"show --first-parent\" should give that already.\n>\n> Well, for \"git show\" even \"show -m\" does the right thing (once properly\n> configured), as \"-p\" is implied by \"git show\".\n>\n> Taking \"git show\" into the picture brings yet another argument if favor\n> of new \"-m\" behavior, as then \"git show -m\" and \"git log -m -n1\" will\n> finally start to produce the same result, that I'd find desirable.\n>\n> That said,\n>\n> --diff-merges=first-parent\n>\n> that could be shortened to\n>\n> --diff-merges=1\n>\n> is the universal answer that works out-of-the-box for any command the\n> same way, reliably, and then it's also\n>\n> -m -p\n>\n> if configured accordingly, that has been made available by previously\n> accepted patches.\n>\n> These series just did the last logical step: allowed it to be just\n>\n> -m\n>\n> if configured accordingly.\n>\n>> One problem with \"-m implies -p\" is that it is unclear what should be\n>> done to things like \"-m --raw\".\n>\n> Nothing specific is actually needed, as far as I'm aware, as implied -p\n> doesn't interfere with --raw. Please give particular example of a\n> problem if you foresee one.\n\nIn fact there is already a test for it that I added some time ago (as\nwell as for --stat), so a problem would have been caught. Please also\nnotice that it was an agreed goal for \"-m\" to finally imply \"-p\" at that\ntime:\n\ncommit 48229c193d2e6e728d3243bacea2f1e1490ced8a\nAuthor: Sergey Organov \nDate: Fri May 21 00:46:55 2021 +0300\n\nt4013: test \"git log -m --raw\"\n\nThis is to ensure we won't break different diff formats when we start\nto imply \"-p\" by \"-m\".\n\nSigned-off-by: Sergey Organov \nSigned-off-by: Junio C Hamano \n\nThanks,\n-- Sergey Organov\n"},{"id":"474397","messageId":"87mt3w0yo2.fsf@osv.gnss.ru","threadId":"59186","inReplyTo":"87y1nk6aa5.fsf@osv.gnss.ru","subject":"Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-03-29T08:06:21Z","receivedAt":"2023-03-29T08:06:36Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Sergey Organov writes:\n\n[...]\n\n> [As a side-note, current behavior of implied -p, explicit -p, -s, and\n> --raw with respect to each other that I figured by experiment looks\n> suspect to me. E.g., once explicit -p is given, and then canceled by -s,\n> I can't get bare --raw output anymore]\n\nSpecifically, this test unexpectedly fails:\n\nmodified t/t4013-diff-various.sh\n@@ -457,6 +457,16 @@ diff-tree --stat --compact-summary initial mode\n diff-tree -R --stat --compact-summary initial mode\n EOF\n \n+# This should succeed, but --patch --no-patch does something hidden\n+# that breaks --raw.\n+test_expect_success '--no-patch only cancels --patch' '\n+\tgit log --raw master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log --patch --no-patch --raw >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'log -m matches pure log' '\n \tgit log master >result &&\n \tprocess_diffs result >expected &&\n\n\nThanks,\n-- Sergey Organov\n"}]}