What's cooking in git.git (Feb 2023, #01; Thu, 2)

What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Junio C Hamano]

Here are the topics that have been cooking in my tree.  Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a future
release).  Commits prefixed with '-' are only in 'seen', and aren't
considered "accepted" at all.  A topic without enough support may be
discarded after a long period of no activity.

Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors.  Some
repositories have only a subset of branches.

With maint, master, next, seen, todo:

	git://git.kernel.org/pub/scm/git/git.git/
	git://repo.or.cz/alt-git.git/
	https://kernel.googlesource.com/pub/scm/git/git/
	https://github.com/git/git/
	https://gitlab.com/git-vcs/git/

With all the integration branches and topics broken out:

	https://github.com/gitster/git/

Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):

	git://git.kernel.org/pub/scm/git/git-htmldocs.git/
	https://github.com/gitster/git-htmldocs.git/

Release tarballs are available at:

	https://www.kernel.org/pub/software/scm/git/

--------------------------------------------------
[New Topics]

* ab/imap-send-requires-curl (2023-02-02) 6 commits
 - imap-send: correctly report "host" when using "tunnel"
 - imap-send: remove old --no-curl codepath
 - imap-send: make --curl no-optional
 - imap-send: replace auto-probe libcurl with hard dependency
 - imap-send doc: the imap.sslVerify is used with imap.tunnel
 - imap-send: note "auth_method", not "host" on auth method failure

 Give a hard dependency on cURL library to build "git imap-send",
 and remove the code to interact with IMAP server without using cURL.

 Comments?
 source: <cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com>


* gc/index-format-doc (2023-02-01) 1 commit
 - docs: document zero bits in index "mode"

 Doc update.

 Will merge to 'next'.
 source: <20230201024041.29401-1-chooglen@google.com>


* jk/httpd-test-updates (2023-02-01) 4 commits
 - t/lib-httpd: increase ssl key size to 2048 bits
 - t/lib-httpd: drop SSLMutex config
 - t/lib-httpd: bump required apache version to 2.4
 - t/lib-httpd: bump required apache version to 2.2

 Test update.

 Will merge to 'next'.
 source: <Y9pOmR5fOfCHwYpF@coredump.intra.peff.net>


* rd/doc-default-date-format (2023-02-01) 1 commit
 - rev-list: clarify git-log default date format

 Update --date=default documentation.
 source: <20230201155712.86577-1-rafael@dulfer.be>


* ew/free-island-marks (2023-02-02) 1 commit
 - delta-islands: free island_marks and bitmaps

 "git pack-objects" learned to release delta-island bitmap data when
 it is done using it, saving peak heap memory usage.

 Will merge to 'next'.
 source: <20230202094217.M955476@dcvr>


* hj/remove-msys-support (2023-02-02) 2 commits
 - mingw: remove msysGit/MSYS1 support
 - mingw: remove duplicate `USE_NED_ALLOCATOR` directive

 Remove support for MSys, which now lags way behind MSys2.

 Will merge to 'next'.
 source: <pull.1433.v2.git.1675309898.gitgitgadget@gmail.com>


* sk/winansi-createthread-fix (2023-02-01) 1 commit
 - compat/winansi: check for errors of CreateThread() correctly

 Fix use of CreateThread() API call made early in the windows
 start-up code.

 Will merge to 'next'.
 source: <pull.1445.v4.git.git.1675290034144.gitgitgadget@gmail.com>

--------------------------------------------------
[Stalled]

* rj/branch-unborn-in-other-worktrees (2023-01-19) 3 commits
 - branch: rename orphan branches in any worktree
 - branch: description for orphan branch errors
 - avoid unnecessary worktrees traversing

 Error messages given when working on an unborn branch that is
 checked out in another worktree have been improvved.

 Expecting a reroll.
 cf. <527f7315-be7b-7ec0-04fc-d07da7d4fefa@gmail.com>
 source: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com>


* ja/worktree-orphan (2023-01-13) 4 commits
 - worktree add: add hint to direct users towards --orphan
 - worktree add: add --orphan flag
 - worktree add: refactor opt exclusion tests
 - worktree add: include -B in usage docs

 'git worktree add' learned how to create a worktree based on an
 orphaned branch with `--orphan`.

 Expecting a reroll.
 cf. <11be1b0e-ee38-119f-1d80-cb818946116b@dunelm.org.uk>
 source: <20230109173227.29264-1-jacobabel@nullpo.dev>


* ab/avoid-losing-exit-codes-in-tests (2022-12-20) 6 commits
 - tests: don't lose misc "git" exit codes
 - tests: don't lose "git" exit codes in "! ( git ... | grep )"
 - tests: don't lose exit status with "test <op> $(git ...)"
 - tests: don't lose exit status with "(cd ...; test <op> $(git ...))"
 - t/lib-patch-mode.sh: fix ignored exit codes
 - auto-crlf tests: don't lose exit code in loops and outside tests

 Test clean-up.

 Expecting a hopefully minor and final reroll.
 cf. <1182283a-4a78-3c99-e716-a8c3e58a5823@web.de>
 cf. <xmqqsfhb0vum.fsf@gitster.g>
 source: <cover-v4-0.6-00000000000-20221219T101240Z-avarab@gmail.com>


* tl/notes--blankline (2022-11-09) 5 commits
 - notes.c: introduce "--no-blank-line" option
 - notes.c: provide tips when target and append note are both empty
 - notes.c: drop unreachable code in 'append_edit()'
 - notes.c: cleanup for "designated init" and "char ptr init"
 - notes.c: cleanup 'strbuf_grow' call in 'append_edit'

 'git notes append' was taught '--[no-]blank-line' to conditionally
 add a LF between a new and existing note.

 Expecting a reroll.
 cf. <CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com>
 source: <cover.1667980450.git.dyroneteng@gmail.com>


* mc/switch-advice (2022-11-09) 1 commit
 - po: use `switch` over `checkout` in error message

 Use 'switch' instead of 'checkout' in an error message.

 Waiting for review response.
 source: <pull.1308.git.git.1668018620148.gitgitgadget@gmail.com>


* js/range-diff-mbox (2022-11-23) 1 commit
 - range-diff: support reading mbox files

 'git range-diff' gained support for reading either side from an .mbox
 file instead of a revision range.

 Waiting for review response.
 cf. <xmqqr0xupmnf.fsf@gitster.g>
 source: <pull.1420.v3.git.1669108102092.gitgitgadget@gmail.com>


* ab/tag-object-type-errors (2022-11-22) 5 commits
 - tag: don't emit potentially incorrect "object is a X, not a Y"
 - tag: don't misreport type of tagged objects in errors
 - object tests: add test for unexpected objects in tags
 - object-file.c: free the "t.tag" in check_tag()
 - Merge branch 'jk/parse-object-type-mismatch' into ab/tag-object-type-errors

 Hardening checks around mismatched object types when one of those
 objects is a tag.

 Expecting a reroll.
 cf. <xmqqzgb5jz5c.fsf@gitster.g>
 cf. <xmqqsfgxjugi.fsf@gitster.g>
 source: <cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com>


* ed/fsmonitor-inotify (2022-12-13) 6 commits
 - fsmonitor: update doc for Linux
 - fsmonitor: test updates
 - fsmonitor: enable fsmonitor for Linux
 - fsmonitor: implement filesystem change listener for Linux
 - fsmonitor: determine if filesystem is local or remote
 - fsmonitor: prepare to share code between Mac OS and Linux

 Bundled fsmonitor for Linux using inotify API.

 Needs review on the updated round.
 source: <pull.1352.v5.git.git.1670882286.gitgitgadget@gmail.com>


* jc/spell-id-in-both-caps-in-message-id (2022-12-17) 1 commit
 - e-mail workflow: Message-ID is spelled with ID in both capital letters

 Consistently spell "Message-ID" as such, not "Message-Id".

 Needs review.
 source: <xmqqsfhgnmqg.fsf@gitster.g>


* ad/test-record-count-when-harness-is-in-use (2022-12-25) 1 commit
 - test-lib: allow storing counts with test harnesses

 Allow summary results from tests to be written to t/test-results
 directory even when a test harness like 'prove' is in use.

 Needs review.
 source: <20221224225200.1027806-1-adam@dinwoodie.org>


* so/diff-merges-more (2022-12-18) 5 commits
 - diff-merges: improve --diff-merges documentation
 - diff-merges: issue warning on lone '-m' option
 - diff-merges: support list of values for --diff-merges
 - diff-merges: implement log.diffMerges-m-imply-p config
 - diff-merges: implement [no-]hide option and log.diffMergesHide config

 Assorted updates to "--diff-merges=X" option.

 May want to discard.  Breaking compatibility does not seem worth it.
 source: <20221217132955.108542-1-sorganov@gmail.com>

--------------------------------------------------
[Cooking]

* ab/config-multi-and-nonbool (2023-02-02) 9 commits
 - for-each-repo: with bad config, don't conflate <path> and <cmd>
 - config API: add "string" version of *_value_multi(), fix segfaults
 - config API users: test for *_get_value_multi() segfaults
 - for-each-repo: error on bad --config
 - config API: have *_multi() return an "int" and take a "dest"
 - versioncmp.c: refactor config reading next commit
 - config API: add and use a "git_config_get()" family of functions
 - config tests: add "NULL" tests for *_get_value_multi()
 - config tests: cover blind spots in git_die_config() tests

 Assorted config API updates.

 source: <cover-v4-0.9-00000000000-20230202T131155Z-avarab@gmail.com>


* ds/scalar-ignore-cron-error (2023-01-27) 3 commits
  (merged to 'next' on 2023-01-31 at 98d13ac3e7)
 + scalar: only warn when background maintenance fails
 + t921*: test scalar behavior starting maintenance
 + t: allow 'scalar' in test_must_fail

 Allow "scalar" to warn but continue when its periodic maintenance
 feature cannot be enabled.

 Will merge to 'master'.
 source: <pull.1473.git.1674849963.gitgitgadget@gmail.com>


* mh/doc-credential-cache-only-in-core (2023-01-29) 1 commit
  (merged to 'next' on 2023-01-30 at 021b5227af)
 + Documentation: clarify that cache forgets credentials if the system restarts

 Documentation clarification.

 Will merge to 'master'.
 source: <pull.1447.v3.git.1674936815117.gitgitgadget@gmail.com>


* ab/hook-api-with-stdin (2023-01-23) 5 commits
 - hook: support a --to-stdin=<path> option for testing
 - sequencer: use the new hook API for the simpler "post-rewrite" call
 - hook API: support passing stdin to hooks, convert am's 'post-rewrite'
 - run-command: allow stdin for run_processes_parallel
 - run-command.c: remove dead assignment in while-loop

 Extend the run-hooks API to allow feeding data from the standard
 input when running the hook script(s).

 Expecting review responses.
 source: <cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com>


* as/ssh-signing-improve-key-missing-error (2023-01-25) 1 commit
  (merged to 'next' on 2023-01-25 at 140f2c2c60)
 + ssh signing: better error message when key not in agent

 Improve the error message given when private key is not loaded in
 the ssh agent in the codepath to sign with an ssh key.

 Will merge to 'master'.
 source: <pull.1270.v3.git.git.1674650450662.gitgitgadget@gmail.com>


* en/rebase-incompatible-opts (2023-01-25) 10 commits
  (merged to 'next' on 2023-01-27 at 35a67cf2c6)
 + rebase: provide better error message for apply options vs. merge config
 + rebase: put rebase_options initialization in single place
 + rebase: fix formatting of rebase --reapply-cherry-picks option in docs
 + rebase: clarify the OPT_CMDMODE incompatibilities
 + rebase: add coverage of other incompatible options
 + rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
 + rebase: fix docs about incompatibilities with --root
 + rebase: remove --allow-empty-message from incompatible opts
 + rebase: flag --apply and --merge as incompatible
 + rebase: mark --update-refs as requiring the merge backend

 "git rebase" often ignored incompatible options instead of
 complaining, which has been corrected.

 Will merge to 'master'.
 Replaces en/rebase-update-refs-needs-merge-backend.
 source: <pull.1466.v5.git.1674619434.gitgitgadget@gmail.com>


* gm/request-pull-with-non-pgp-signed-tags (2023-01-25) 1 commit
  (merged to 'next' on 2023-01-30 at abc684d8df)
 + request-pull: filter out SSH/X.509 tag signatures

 Adjust "git request-pull" to strip embedded signature from signed
 tags to notice non-PGP signatures.

 Will merge to 'master'.
 source: <20230125234725.3918563-1-gwymor@tilde.club>


* cb/grep-fallback-failing-jit (2023-01-31) 1 commit
 - grep: fall back to interpreter if JIT memory allocation fails

 In an environment where dynamically generated code is prohibited to
 run (e.g. SELinux), failure to JIT pcre patterns is expected.  Fall
 back to interpreted execution in such a case.

 Will merge to 'next'?
 source: <20230131185611.520311-1-minipli@grsecurity.net>


* cb/checkout-same-branch-twice (2023-01-20) 1 commit
 - checkout/switch: disallow checking out same branch in multiple worktrees

 "git checkout -B $branch" failed to protect against checking out
 a branch that is checked out elsewhere, unlike "git branch -f" did.

 Expecting a hopefully minor and final reroll.
 cf. <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk>
 source: <20230120113553.24655-1-carenas@gmail.com>


* ab/sequencer-unleak (2023-01-18) 8 commits
 - commit.c: free() revs.commit in get_fork_point()
 - builtin/rebase.c: free() "options.strategy_opts"
 - sequencer.c: always free() the "msgbuf" in do_pick_commit()
 - builtin/rebase.c: fix "options.onto_name" leak
 - builtin/revert.c: move free-ing of "revs" to replay_opts_release()
 - rebase & sequencer API: fix get_replay_opts() leak in "rebase"
 - sequencer.c: split up sequencer_remove_state()
 - rebase: use "cleanup" pattern in do_interactive_rebase()

 Plug leaks in sequencer subsystem and its users.

 Expecting a hopefully minor and final reroll.
 cf. <xmqqedry17r4.fsf@gitster.g>
 source: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>


* jc/attr-doc-fix (2023-01-26) 1 commit
  (merged to 'next' on 2023-01-26 at cb327c4b5f)
 + attr: fix instructions on how to check attrs

 Comment fix.

 Will merge to 'master'.
 source: <pull.1441.v3.git.git.1674768107941.gitgitgadget@gmail.com>


* rj/avoid-switching-to-already-used-branch (2023-01-22) 3 commits
 - switch: reject if the branch is already checked out elsewhere (test)
 - rebase: refuse to switch to a branch already checked out elsewhere (test)
 - branch: fix die_if_checked_out() when ignore_current_worktree

 A few subcommands have been taught to stop users from working on a
 branch that is being used in another worktree linked to the same
 repository.

 Expecting a hopefully minor and final reroll.
 cf. <d61a2393-64c8-da49-fe13-00bc4a52d5e3@gmail.com>
 source: <f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com>


* rj/bisect-already-used-branch (2023-01-22) 1 commit
 - bisect: fix "reset" when branch is checked out elsewhere

 Allow "git bisect reset [name]" to check out the named branch (or
 the original one) even when the branch is already checked out in a
 different worktree linked to the same repository.

 Leaning negative. Why is it a good thing?
 cf. <xmqqo7qqovp1.fsf@gitster.g>
 source: <1c36c334-9f10-3859-c92f-3d889e226769@gmail.com>


* en/ls-files-doc-update (2023-01-13) 4 commits
  (merged to 'next' on 2023-01-27 at 20b9803add)
 + ls-files: guide folks to --exclude-standard over other --exclude* options
 + ls-files: clarify descriptions of status tags for -t
 + ls-files: clarify descriptions of file selection options
 + ls-files: add missing documentation for --resolve-undo option

 Doc update to ls-files.

 Will merge to 'master'.
 source: <pull.1463.git.1673584914.gitgitgadget@gmail.com>


* ms/send-email-feed-header-to-validate-hook (2023-01-19) 2 commits
 - send-email: expose header information to git-send-email's sendemail-validate hook
 - send-email: refactor header generation functions

 "git send-email" learned to give the e-mail headers to the validate
 hook by passing an extra argument from the command line.

 Expecting a hopefully minor and final reroll.
 cf. <c1ba0a28-3c39-b313-2757-dceb02930334@amd.com>
 source: <20230120012459.920932-1-michael.strawbridge@amd.com>


* ds/bundle-uri-5 (2023-01-31) 11 commits
 - bundle-uri: test missing bundles with heuristic
 - bundle-uri: store fetch.bundleCreationToken
 - fetch: fetch from an external bundle URI
 - bundle-uri: drop bundle.flag from design doc
 - clone: set fetch.bundleURI if appropriate
 - bundle-uri: download in creationToken order
 - bundle-uri: parse bundle.<id>.creationToken values
 - bundle-uri: parse bundle.heuristic=creationToken
 - t5558: add tests for creationToken heuristic
 - bundle: verify using check_connected()
 - bundle: test unbundling with incomplete history

 The bundle-URI subsystem adds support for creation-token heuristics
 to help incremental fetches.

 Will merge to 'next'?
 source: <pull.1454.v3.git.1675171759.gitgitgadget@gmail.com>


* tc/cat-file-z-use-cquote (2023-01-16) 1 commit
 - cat-file: quote-format name in error when using -z

 "cat-file" in the batch mode that is fed NUL-terminated pathnames
 learned to cquote them in its error output (otherwise, a funny
 pathname with LF in it would break the lines in the output stream).

 Expecting a reroll.
 cf. <2a2a46f0-a9bc-06a6-72e1-28800518777c@dunelm.org.uk>
 source: <20230116190749.4141516-1-toon@iotcl.com>


* cw/submodule-status-in-parallel (2023-01-17) 6 commits
 - submodule: call parallel code from serial status
 - diff-lib: parallelize run_diff_files for submodules
 - diff-lib: refactor match_stat_with_submodule
 - submodule: move status parsing into function
 - submodule: strbuf variable rename
 - run-command: add duplicate_output_fn to run_processes_parallel_opts

 "git submodule status" learned to run the comparison in submodule
 repositories in parallel.

 Expecting a reroll.
 cf. <CAFySSZBiW7=ZTmXRaLzCoKUi0Jd=fzvW5PJ6=Ka0jKHoP2ddSw@mail.gmail.com>
 cf. <kl6lo7qlvg4h.fsf@chooglen-macbookpro.roam.corp.google.com>
 source: <20230104215415.1083526-1-calvinwan@google.com>


* ab/various-leak-fixes (2023-02-02) 19 commits
 - push: free_refs() the "local_refs" in set_refspecs()
 - push: refactor refspec_append_mapped() for subsequent leak-fix
 - receive-pack: free() the "ref_name" in "struct command"
 - grep API: plug memory leaks by freeing "header_list"
 - grep.c: refactor free_grep_patterns()
 - builtin/merge.c: free "&buf" on "Your local changes..." error
 - builtin/merge.c: use fixed strings, not "strbuf", fix leak
 - show-branch: free() allocated "head" before return
 - commit-graph: fix a parse_options_concat() leak
 - http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()
 - http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main()
 - worktree: fix a trivial leak in prune_worktrees()
 - repack: fix leaks on error with "goto cleanup"
 - name-rev: don't xstrdup() an already dup'd string
 - various: add missing clear_pathspec(), fix leaks
 - clone: use free() instead of UNLEAK()
 - commit-graph: use free_commit_graph() instead of UNLEAK()
 - bundle.c: don't leak the "args" in the "struct child_process"
 - tests: mark tests as passing with SANITIZE=leak

 Leak fixes.

 Comments?
 source: <cover-v6-00.19-00000000000-20230202T094704Z-avarab@gmail.com>


--------------------------------------------------
[Discarded]

* mc/credential-helper-auth-headers (2023-01-20) 12 commits
  (merged to 'next' on 2023-01-25 at cb95006bb2)
 + credential: add WWW-Authenticate header to cred requests
 + http: read HTTP WWW-Authenticate response headers
 + http: replace unsafe size_t multiplication with st_mult
 + test-http-server: add sending of arbitrary headers
 + test-http-server: add simple authentication
 + test-http-server: pass Git requests to http-backend
 + test-http-server: add HTTP request parsing
 + test-http-server: add HTTP error response function
 + test-http-server: add stub HTTP server test helper
 + daemon: rename some esoteric/laboured terminology
 + daemon: libify child process handling functions
 + daemon: libify socket setup and option functions

 Extending credential helper protocol.

 The test-only server is an eyesore.
 cf. <e57c1ca3-c21c-db41-a386-e5887f46055c@github.com>
 cf. <Y9JkMLueCwjkLHOr@coredump.intra.peff.net>
 source: <pull.1352.v7.git.1674252530.gitgitgadget@gmail.com>

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:


> * so/diff-merges-more (2022-12-18) 5 commits
>  - diff-merges: improve --diff-merges documentation
>  - diff-merges: issue warning on lone '-m' option
>  - diff-merges: support list of values for --diff-merges
>  - diff-merges: implement log.diffMerges-m-imply-p config
>  - diff-merges: implement [no-]hide option and log.diffMergesHide config
>
>  Assorted updates to "--diff-merges=X" option.
>
>  May want to discard.  Breaking compatibility does not seem worth it.
>  source: <20221217132955.108542-1-sorganov@gmail.com>

Hi Junio,

This does not break any compatibility, as far as me and I believe
reviewers of these series are aware.

Please either merge this or explain what do you mean in more details.

Thanks,
-- Sergey Organov

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>
>> * so/diff-merges-more (2022-12-18) 5 commits
>>  - diff-merges: improve --diff-merges documentation
>>  - diff-merges: issue warning on lone '-m' option
>>  - diff-merges: support list of values for --diff-merges
>>  - diff-merges: implement log.diffMerges-m-imply-p config
>>  - diff-merges: implement [no-]hide option and log.diffMergesHide config
>>
>>  Assorted updates to "--diff-merges=X" option.
>>
>>  May want to discard.  Breaking compatibility does not seem worth it.
>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>
> Hi Junio,
>
> This does not break any compatibility, as far as me and I believe
> reviewers of these series are aware.

The last paragraphs in the review two months ago still describe what
this series does fairly accurately, I think.

    These patches do look like a good approach to solve the first point
    among the "two problems" in the previous round. Thanks for working
    on it.

    IIRC, the previous round (why is this round marked as v1, by the
    way?) was reviewed by some folks, so lets wait to hear from them
    how this round does better.

    Unfortunately, I do not think of any "solution" that would avoid
    breaking folks, if its end goal is to flip the default, either by
    hardcoding or with a configuration variable.  IOW, the other one
    among the "two problems" in the previous round sounds unsolvable.
    We should question if it was really an "issue" worth "resolving",
    though.

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>
>>> * so/diff-merges-more (2022-12-18) 5 commits
>>>  - diff-merges: improve --diff-merges documentation
>>>  - diff-merges: issue warning on lone '-m' option
>>>  - diff-merges: support list of values for --diff-merges
>>>  - diff-merges: implement log.diffMerges-m-imply-p config
>>>  - diff-merges: implement [no-]hide option and log.diffMergesHide config
>>>
>>>  Assorted updates to "--diff-merges=X" option.
>>>
>>>  May want to discard.  Breaking compatibility does not seem worth it.
>>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>>
>> Hi Junio,
>>
>> This does not break any compatibility, as far as me and I believe
>> reviewers of these series are aware.
>
> The last paragraphs in the review two months ago still describe what
> this series does fairly accurately, I think.
>
>     These patches do look like a good approach to solve the first point
>     among the "two problems" in the previous round. Thanks for working
>     on it.
>
>     IIRC, the previous round (why is this round marked as v1, by the
>     way?) was reviewed by some folks, so lets wait to hear from them
>     how this round does better.
>
>     Unfortunately, I do not think of any "solution" that would avoid
>     breaking folks, if its end goal is to flip the default, either by
>     hardcoding or with a configuration variable.  IOW, the other one
>     among the "two problems" in the previous round sounds unsolvable.
>     We should question if it was really an "issue" worth "resolving",
>     though.

Well, we may end up flipping or not flipping the default (even though
I'd prefer we indeed do), the series are still valid either way, as they
allow *me* (or anybody else who prefers more useful '-m' behavior) to
flip the switch for myself, locally.

Also, the only patch that got some resistance from reviewers has been
removed from the series, so I don't see anything left that'd prevent
this from being merged.

From my POV the only remotely questionable patch is:

- diff-merges: issue warning on lone '-m' option

and I hereby agree to remove it if it feels wrong to you.

Let me state it cleanly: once these are accepted, I'll turn
log.diffMerges-m-imply-p on for myself, and will suggest it to others
who already asked about '-m' inconsistency, or will ask in the future.

Thanks,
-- Sergey Organov

so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Glen Choo]

Hi Sergey,

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>
>> * so/diff-merges-more (2022-12-18) 5 commits
>>  - diff-merges: improve --diff-merges documentation
>>  - diff-merges: issue warning on lone '-m' option
>>  - diff-merges: support list of values for --diff-merges
>>  - diff-merges: implement log.diffMerges-m-imply-p config
>>  - diff-merges: implement [no-]hide option and log.diffMergesHide config
>>
>>  Assorted updates to "--diff-merges=X" option.
>>
>>  May want to discard.  Breaking compatibility does not seem worth it.
>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>
> Hi Junio,
>
> This does not break any compatibility, as far as me and I believe
> reviewers of these series are aware.

My 2 cents (which maybe lines up with what Junio meant) is that this
series doesn't break compatibility on its own, but IMO the approach
doesn't make sense unless we also intend to flip the default somewhere
down the line. "log.diffMerges-m-imply-p" is a really specific config
option, and it needs to justify its addition with an outsized benefit.

I don't think it meets that bar, the only use cases I can think of are:

- Before we change the default, setting "log.diffMerges-m-imply-p=true"
  gives the 'nicer' behavior. But if the user had the permissions and
  knowledge to do so, they could just pass "-p" instead for correctness.

  If the goal is to reduce typing, then we could add a different CLI
  flag that would behave like "-m -p", or we could teach "git diff" to
  accept proper single-character flags. Either of these options would be
  more discoverable and cleaner.

- After we change the default, setting "log.diffMerges-m-imply-p=false"
  gives the old behavior, which might be needed if the user is running
  scripts written in for an older version of Git. I would find this
  compelling, but as Junio mentioned [1], breaking compatibility doesn't
  seem worth it, so this point is moot. On the other hand, neither of
  the alternative approaches break compatibility, so they're superior in
  this regard too.

- The only legitimate use case I think of is something like a script
  that uses "-m" assuming that it implied "-p", AND the user has no
  ability to modify the script. Then the user's only recourse is to set
  a config. But this is so exotic that I don't think this is a good
  enough reason to have the config.

I wouldn't mind seeing a version of these patches without
"log.diffMerges-m-imply-p=false" , but in its current form, I agree with
Junio that this isn't worth it.

[1] https://lore.kernel.org/git/xmqqbko1xv86.fsf@gitster.g/

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Glen Choo <chooglen@google.com> writes:

> Hi Sergey,
>
> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>
>>> * so/diff-merges-more (2022-12-18) 5 commits
>>>  - diff-merges: improve --diff-merges documentation
>>>  - diff-merges: issue warning on lone '-m' option
>>>  - diff-merges: support list of values for --diff-merges
>>>  - diff-merges: implement log.diffMerges-m-imply-p config
>>>  - diff-merges: implement [no-]hide option and log.diffMergesHide config
>>>
>>>  Assorted updates to "--diff-merges=X" option.
>>>
>>>  May want to discard.  Breaking compatibility does not seem worth it.
>>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>>
>> Hi Junio,
>>
>> This does not break any compatibility, as far as me and I believe
>> reviewers of these series are aware.
>
> My 2 cents (which maybe lines up with what Junio meant) is that this
> series doesn't break compatibility on its own, but IMO the approach
> doesn't make sense unless we also intend to flip the default somewhere
> down the line. "log.diffMerges-m-imply-p" is a really specific config
> option, and it needs to justify its addition with an outsized benefit.
>
> I don't think it meets that bar, the only use cases I can think of are:
>
> - Before we change the default, setting "log.diffMerges-m-imply-p=true"
>   gives the 'nicer' behavior. But if the user had the permissions and
>   knowledge to do so, they could just pass "-p" instead for
>   correctness.

Except I can't configure this in centralized manner for multiple users
on our host machine?

>
>   If the goal is to reduce typing, then we could add a different CLI
>   flag that would behave like "-m -p", or we could teach "git diff" to
>   accept proper single-character flags. Either of these options would be
>   more discoverable and cleaner.

The only drawback of this is that this leaves "-m" inconsistent with no
apparent reason.

OTOH, teaching "git log" to use common options machinery to accept "-pm"
looks to be quite a project, and doesn't fix '-m' inconsistency anyway.

Then, out of curiosity, what do you think was the reason to change
"--cc" behavior to match that of "-c"? To save typing? To me, changing
"-m" accordingly is an improvement to the overall feel of git interface,
the same way as changing "--cc" was.

That said, if we decide to go for new option, I'd suggest to add "-d",
and probably obsolete "-m". This does not look to me as appealing as
finally fixing "-m" though.

>
> - After we change the default, setting "log.diffMerges-m-imply-p=false"
>   gives the old behavior, which might be needed if the user is running
>   scripts written in for an older version of Git. I would find this
>   compelling, but as Junio mentioned [1], breaking compatibility doesn't
>   seem worth it, so this point is moot. On the other hand, neither of
>   the alternative approaches break compatibility, so they're superior in
>   this regard too.

The only incompatibility found was obsolete use of "--first-parent -m",
that was the only use-case for "-m" for a long time, and this could
probably be detected and handled specially, though it sounds like a
nasty kludge.

>
> - The only legitimate use case I think of is something like a script
>   that uses "-m" assuming that it implied "-p", AND the user has no
>   ability to modify the script. Then the user's only recourse is to set
>   a config. But this is so exotic that I don't think this is a good
>   enough reason to have the config.

Yes, let's just change "-m" behavior then, and let exotic scripts just
drop "-m", as it's not needed for a long time already in the combo
"--first-parent -m" that currently reads "--first-parent".

>
> I wouldn't mind seeing a version of these patches without
> "log.diffMerges-m-imply-p=false" , but in its current form, I agree with
> Junio that this isn't worth it.
>
> [1] https://lore.kernel.org/git/xmqqbko1xv86.fsf@gitster.g/

All you say is understood and are valid arguments, but then we will
continue to face pretty valid confusion of why "-m" behaves differently
from "-c/--cc/--remerge". I personally don't care, provided I get a way
to make it behave sanely, and that's what "log.diffMerges-m-imply-p"
patch does.

As a kind of complaint, it was simple 1 patch from Junio once upon a
time to change "--cc" to a sane behavior of implying -p, and now it
takes rounds and rounds to do the same for -m. This is rather sad.

Finally, event without "log.diffMerges-m-imply-p", the rest of the
series still makes sense, so if the conclusion is to remove it, let's
still merge the rest, please! Let me know, and I'll then remove the
patch and will change documentation accordingly.

Thanks,
-- Sergey Organov

ds/bundle-uri-5 (was: Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Victoria Dye]

Junio C Hamano wrote:
> * ds/bundle-uri-5 (2023-01-31) 11 commits
>  - bundle-uri: test missing bundles with heuristic
>  - bundle-uri: store fetch.bundleCreationToken
>  - fetch: fetch from an external bundle URI
>  - bundle-uri: drop bundle.flag from design doc
>  - clone: set fetch.bundleURI if appropriate
>  - bundle-uri: download in creationToken order
>  - bundle-uri: parse bundle.<id>.creationToken values
>  - bundle-uri: parse bundle.heuristic=creationToken
>  - t5558: add tests for creationToken heuristic
>  - bundle: verify using check_connected()
>  - bundle: test unbundling with incomplete history
> 
>  The bundle-URI subsystem adds support for creation-token heuristics
>  to help incremental fetches.
> 
>  Will merge to 'next'?
>  source: <pull.1454.v3.git.1675171759.gitgitgadget@gmail.com>

Apologies for not responding sooner, but I reviewed the most recent version
of this series and all of my previous feedback was addressed. Overall, I
think it looks good and is ready for merging to 'next'.

Thanks!
-Victoria

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Sergey Organov]

Hi, Junio!

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>
>>>> * so/diff-merges-more (2022-12-18) 5 commits
>>>>  - diff-merges: improve --diff-merges documentation
>>>>  - diff-merges: issue warning on lone '-m' option
>>>>  - diff-merges: support list of values for --diff-merges
>>>>  - diff-merges: implement log.diffMerges-m-imply-p config
>>>>  - diff-merges: implement [no-]hide option and log.diffMergesHide config
>>>>
>>>>  Assorted updates to "--diff-merges=X" option.
>>>>
>>>>  May want to discard.  Breaking compatibility does not seem worth it.
>>>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>>>
>>> Hi Junio,
>>>
>>> This does not break any compatibility, as far as me and I believe
>>> reviewers of these series are aware.
>>
>> The last paragraphs in the review two months ago still describe what
>> this series does fairly accurately, I think.
>>
>>     These patches do look like a good approach to solve the first point
>>     among the "two problems" in the previous round. Thanks for working
>>     on it.
>>
>>     IIRC, the previous round (why is this round marked as v1, by the
>>     way?) was reviewed by some folks, so lets wait to hear from them
>>     how this round does better.
>>
>>     Unfortunately, I do not think of any "solution" that would avoid
>>     breaking folks, if its end goal is to flip the default, either by
>>     hardcoding or with a configuration variable.  IOW, the other one
>>     among the "two problems" in the previous round sounds unsolvable.
>>     We should question if it was really an "issue" worth "resolving",
>>     though.
>
> Well, we may end up flipping or not flipping the default (even though
> I'd prefer we indeed do), the series are still valid either way, as they
> allow *me* (or anybody else who prefers more useful '-m' behavior) to
> flip the switch for myself, locally.
>
> Also, the only patch that got some resistance from reviewers has been
> removed from the series, so I don't see anything left that'd prevent
> this from being merged.
>
> From my POV the only remotely questionable patch is:
>
> - diff-merges: issue warning on lone '-m' option
>
> and I hereby agree to remove it if it feels wrong to you.
>
> Let me state it cleanly: once these are accepted, I'll turn
> log.diffMerges-m-imply-p on for myself, and will suggest it to others
> who already asked about '-m' inconsistency, or will ask in the future.

This is still marked as "probably won't merge" in the recent "what's
cooking". Could it be merged, please?

Thanks,
-- Sergey Organov

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>>>>>  May want to discard.  Breaking compatibility does not seem worth it.
>>>>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>>>> ...
> This is still marked as "probably won't merge" in the recent "what's
> cooking". Could it be merged, please?

I found the explanation given by Glen last time you mentioned the
topic much better written than anything I would write myself, and I
haven't seen any new input in the message I am responding to, so...

  https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/

Thanks.

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>>>>>>  May want to discard.  Breaking compatibility does not seem worth it.
>>>>>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>>>>> ...
>> This is still marked as "probably won't merge" in the recent "what's
>> cooking". Could it be merged, please?
>
> I found the explanation given by Glen last time you mentioned the
> topic much better written than anything I would write myself, and I
> haven't seen any new input in the message I am responding to, so...
>
>   https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/

I believe I've addressed it with this:

https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/

and seen no follow-up since then.

Thanks,
-- Sergey Organov

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Glen Choo]

Hi Sergey!

Sergey Organov <sorganov@gmail.com> writes:

>>> This is still marked as "probably won't merge" in the recent "what's
>>> cooking". Could it be merged, please?
>>
>> I found the explanation given by Glen last time you mentioned the
>> topic much better written than anything I would write myself, and I
>> haven't seen any new input in the message I am responding to, so...
>>
>>   https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/
>
> I believe I've addressed it with this:
>
> https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/
>
> and seen no follow-up since then.

Sorry for the lack of response, especially if it seemed like your
concerns were not listened to. I did try crafting a response when it was
first written, but it didn't end up adding anything new and valuable to
the conversation, so I dropped it.

I'll leave an updated response, but unfortunately, I am still of the
opinion that we shouldn't merge this series.

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Glen Choo]

Sergey Organov <sorganov@gmail.com> writes:

>>   If the goal is to reduce typing, then we could add a different CLI
>>   flag that would behave like "-m -p", or we could teach "git diff" to
>>   accept proper single-character flags. Either of these options would be
>>   more discoverable and cleaner.
>
> The only drawback of this is that this leaves "-m" inconsistent with no
> apparent reason.
> [...]
> Then, out of curiosity, what do you think was the reason to change
> "--cc" behavior to match that of "-c"? To save typing? To me, changing
> "-m" accordingly is an improvement to the overall feel of git interface,
> the same way as changing "--cc" was.

FWIW, I agree that having an inconsistent "-m" is quite an eyesore, and
there isn't a particularly good reason for it.

The unfortunate reality of Hyrum's law is that some 'mistakes' or CLI
cleanups are difficult or even downright impossible to fix. I'd consider
"-m on its own does nothing" one of those.

> All you say is understood and are valid arguments, but then we will
> continue to face pretty valid confusion of why "-m" behaves differently
> from "-c/--cc/--remerge". I personally don't care, provided I get a way
> to make it behave sanely, and that's what "log.diffMerges-m-imply-p"
> patch does.

We could say, e.g.:

  "-m" is inconsistent, but we can't change it for backwards
  compatibility reasons. Please do not use it, use "-d" instead, because
  that is more sensible.

> As a kind of complaint, it was simple 1 patch from Junio once upon a
> time to change "--cc" to a sane behavior of implying -p, and now it
> takes rounds and rounds to do the same for -m. This is rather sad.

It is not because Junio has special privileges or anything of the sort,
though. My understanding is that it was just an accident that "-m"
_happened_ to be misused by enough people that we deemed that breakage
unacceptable, whereas "--cc" just _happened_ to be fixable.

FWIW, I might have also been on your side when we rolled back the first
"-m" implies "-p" change, but the consensus since then is that breaking
backwards compatibility just isn't worth it in this case, and it would
take a herculean effort to change that consensus. It might not be
completely impossible though, more on that later...

> Finally, event without "log.diffMerges-m-imply-p", the rest of the
> series still makes sense, so if the conclusion is to remove it, let's
> still merge the rest, please! Let me know, and I'll then remove the
> patch and will change documentation accordingly.

Oops. Sorry, I missed this the first time I read this message. This
alone should have warranted a response.

I'm not convinced that the series makes sense without
"log.diffMerges-m-imply-p":

* The main patch is

    diff-merges: implement [no-]hide option and log.diffMergesHide config

  which gives us a way to redefine "-m" as "--diff-merges=hide
  --diff-merges=on". However, we haven't seen any compelling reasons to
  use --diff-merges=hide [1]. I'm also fairly convinced that if we go
  back in time, "-m" wouldn't have the semantics of 'do nothing unless
  -p is also given', and I don't think we should immortalize a mistake
  by giving it an explicit option.

  All the other patches in their current form are dependent on this
  patch going in.

* diff-merges: support list of values for --diff-merges

  This only makes sense if we want to accept multiple values, which we
  don't without the main patch.

* diff-merges: issue warning on lone '-m' option
  diff-merges: improve --diff-merges documentation

  These are docs and UX cleanups as a result of the main patch.

_Maybe_ the number of lone "-m" users will shrink over time to the point
where we could make the switch? We could prepare for that by warning
that lone "-m" is a no-op now, but might imply "-p" at some point in the
future, and then after X amount of time, we might say that users have
had enough warning and actually change the default. I find this
unlikely, but not impossible. However, the patches don't prepare us for
this path, and that would deserve new patches and a different discussion
from what we've been having.

1. https://lore.kernel.org/git/kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com/

Re: What's cooking in git.git (Feb 2023, #01; Thu, 2)

[Sergey Organov]

Glen Choo <chooglen@google.com> writes:

> Hi Sergey!
>
> Sergey Organov <sorganov@gmail.com> writes:
>
>>>> This is still marked as "probably won't merge" in the recent "what's
>>>> cooking". Could it be merged, please?
>>>
>>> I found the explanation given by Glen last time you mentioned the
>>> topic much better written than anything I would write myself, and I
>>> haven't seen any new input in the message I am responding to, so...
>>>
>>>   https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/
>>
>> I believe I've addressed it with this:
>>
>> https://lore.kernel.org/git/87wn4tej2f.fsf@osv.gnss.ru/
>>
>> and seen no follow-up since then.
>
> Sorry for the lack of response, especially if it seemed like your
> concerns were not listened to. I did try crafting a response when it was
> first written, but it didn't end up adding anything new and valuable to
> the conversation, so I dropped it.
>
> I'll leave an updated response, but unfortunately, I am still of the
> opinion that we shouldn't merge this series.

The primary question here is: why don't allow me to get the behavior I
need, provided it has no impact on anybody else, unless they decide they
need it too? I've already performed "you need it, -- do it yourself"
dance, along with documentation and tests, so may I be rewarded by
getting this feature finally in, please?

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

>> Finally, event without "log.diffMerges-m-imply-p", the rest of the
>> series still makes sense, so if the conclusion is to remove it, let's
>> still merge the rest, please! Let me know, and I'll then remove the
>> patch and will change documentation accordingly.
>
> Oops. Sorry, I missed this the first time I read this message. This
> alone should have warranted a response.

Hmph.  I agreed with you that the last step to add the configuration
would not make sense unless we are planning to break users later,
but I have been under the impression that the remainder were OK
clean-ups.  Perhaps it is mostly because I read them so long ago and
forgot the details X-<.

> I'm not convinced that the series makes sense without
> "log.diffMerges-m-imply-p":
>
> * The main patch is
>
>     diff-merges: implement [no-]hide option and log.diffMergesHide config
>
>   which gives us a way to redefine "-m" as "--diff-merges=hide
>   --diff-merges=on". However, we haven't seen any compelling reasons to
>   use --diff-merges=hide [1]. I'm also fairly convinced that if we go
>   back in time, "-m" wouldn't have the semantics of 'do nothing unless
>   -p is also given', and I don't think we should immortalize a mistake
>   by giving it an explicit option.
>
>   All the other patches in their current form are dependent on this
>   patch going in.
>
> * diff-merges: support list of values for --diff-merges
>
>   This only makes sense if we want to accept multiple values, which we
>   don't without the main patch.

Now you mention it (and show example in the previous bullet point),
I have to agree that we would not want this, not because we do not
want to have --diff-merges with multiple values, but because it
introduces an odd-man-out option that is not "last one wins" that is
not used anywhere else in the UI.

No response does not necessarily mean an agreement, but it indeed
helped in this case to be explicit.

Thanks for the clarification.  

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Glen Choo <chooglen@google.com> writes:
>
>>> Finally, event without "log.diffMerges-m-imply-p", the rest of the
>>> series still makes sense, so if the conclusion is to remove it, let's
>>> still merge the rest, please! Let me know, and I'll then remove the
>>> patch and will change documentation accordingly.
>>
>> Oops. Sorry, I missed this the first time I read this message. This
>> alone should have warranted a response.
>
> Hmph.  I agreed with you that the last step to add the configuration
> would not make sense unless we are planning to break users later,

It does make sense to me, and to anybody who does want -m to behave the
same sane way the rest of similar options behave. I've already got it
you don't care, but there are people here who do.

> but I have been under the impression that the remainder were OK
> clean-ups.  Perhaps it is mostly because I read them so long ago and
> forgot the details X-<.

It's not a cleanup, it's rather adding missed feature that allows to get
precise '-m' behavior with --diff-merges. To remind you, there was a
discussion back then when --diff-merges= options were introduced, do
they need to immediately cause output of diffs for merge commits, or
suppress the output till '-p' is specified as well, like '--cc'
originally did, and like '-m' still does.

The conclusion at that time was that it makes sense to take immediate
action as this allows to output diffs for merge commits only, a new
opportunity that was not present before.

However, by making such decision we lost ability to provide exact
behavior of -m using --diff-merges= set of options. This has been
pointed out later to me in the list, and felt obliged to finish
implementation by providing the feature.

  --diff-merges=hide

is exactly that.

>
>> I'm not convinced that the series makes sense without
>> "log.diffMerges-m-imply-p":
>>
>> * The main patch is
>>
>>     diff-merges: implement [no-]hide option and log.diffMergesHide config
>>
>>   which gives us a way to redefine "-m" as "--diff-merges=hide
>>   --diff-merges=on". However, we haven't seen any compelling reasons to
>>   use --diff-merges=hide [1].

I think --diff-merges should be complete and be able to provide exact "-m"
behavior, rendering the latter pure shortcut.

Complete orthogonal interfaces are good thing by themselves, and useful
applications of them are often found later. That's common knowledge.

>> I'm also fairly convinced that if we go
>>   back in time, "-m" wouldn't have the semantics of 'do nothing unless
>>   -p is also given', and I don't think we should immortalize a mistake
>>   by giving it an explicit option.

Yep, and I provided a config option that fixes this mistake. What's
wrong about it? The complete orthogonal interface finalized by the
aforementioned feature allows me to achieve this goal easily.

>>
>>   All the other patches in their current form are dependent on this
>>   patch going in.
>>
>> * diff-merges: support list of values for --diff-merges
>>
>>   This only makes sense if we want to accept multiple values, which we
>>   don't without the main patch.
>
> Now you mention it (and show example in the previous bullet point),
> I have to agree that we would not want this, not because we do not
> want to have --diff-merges with multiple values, but because it
> introduces an odd-man-out option that is not "last one wins" that is
> not used anywhere else in the UI.

It's still the last one wins, and I believe I've carefully described it
in the documentation of the options.

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Glen Choo]

Sergey Organov <sorganov@gmail.com> writes:

>> but I have been under the impression that the remainder were OK
>> clean-ups.  Perhaps it is mostly because I read them so long ago and
>> forgot the details X-<.
>
> It's not a cleanup, it's rather adding missed feature that allows to get
> precise '-m' behavior with --diff-merges.
> ...
> However, by making such decision we lost ability to provide exact
> behavior of -m using --diff-merges= set of options. This has been
> pointed out later to me in the list, and felt obliged to finish
> implementation by providing the feature.
>
>   --diff-merges=hide
>
> is exactly that.

This is exactly the "immortaliz[ing] a mistake" that I mentioned
upthread, by turning a UX wart (-m doesn't imply -p) that neither of us
likes into a feature. I'd be in favor of getting rid of the wart
altogether (i.e. let's find a path that lets us make -m imply -p in the
future), but not rebranding it as a feature.

>>> I'm not convinced that the series makes sense without
>>> "log.diffMerges-m-imply-p":
>>>
>>> * The main patch is
>>>
>>>     diff-merges: implement [no-]hide option and log.diffMergesHide config
>>>
>>>   which gives us a way to redefine "-m" as "--diff-merges=hide
>>>   --diff-merges=on". However, we haven't seen any compelling reasons to
>>>   use --diff-merges=hide [1].
>
> I think --diff-merges should be complete and be able to provide exact "-m"
> behavior, rendering the latter pure shortcut.
>
> Complete orthogonal interfaces are good thing by themselves, and useful
> applications of them are often found later. That's common knowledge.

For the user who wants it, yes, but these interfaces come with a
maintenance cost and a cost to the user who just wants the simpler
interface.

I think that our goals here are actually the same: we both want Git to
be as simple and usable as possible. We disagree on whether this series
makes Git simpler or more complex (and I don't think either of us will
change our minds any time soon), but I think we can agree on an overall
direction. With that in mind, I'd be willing to discuss other approaches
that get us closer to "-m implies -p", if you have any.

>>> I'm also fairly convinced that if we go
>>>   back in time, "-m" wouldn't have the semantics of 'do nothing unless
>>>   -p is also given', and I don't think we should immortalize a mistake
>>>   by giving it an explicit option.
>
> Yep, and I provided a config option that fixes this mistake. What's
> wrong about it? The complete orthogonal interface finalized by the
> aforementioned feature allows me to achieve this goal easily.

Addressed above.

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Glen Choo <chooglen@google.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> but I have been under the impression that the remainder were OK
>>> clean-ups.  Perhaps it is mostly because I read them so long ago and
>>> forgot the details X-<.
>>
>> It's not a cleanup, it's rather adding missed feature that allows to get
>> precise '-m' behavior with --diff-merges.
>> ...
>> However, by making such decision we lost ability to provide exact
>> behavior of -m using --diff-merges= set of options. This has been
>> pointed out later to me in the list, and felt obliged to finish
>> implementation by providing the feature.
>>
>>   --diff-merges=hide
>>
>> is exactly that.
>
> This is exactly the "immortaliz[ing] a mistake" that I mentioned
> upthread, by turning a UX wart (-m doesn't imply -p) that neither of us
> likes into a feature. I'd be in favor of getting rid of the wart
> altogether (i.e. let's find a path that lets us make -m imply -p in the
> future), but not rebranding it as a feature.

We are walking in rounds here. Let me try to find at least some common
ground. Could we try to agree on *one* of these 2 mutually exclusive
statements, please:

1. Current -m behavior is useful

2. Current -m behavior is useless

To me it looks like Junio thinks 1 is the case, and you, Glen, think
it's 2 that is the case, and you both somewhat oppose to my patch
series, believing you agree with each other, that in fact does not seem
to be the case from my POV, effectively leaving me no way to adopt the
series so that they are actually accepted.

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> We are walking in rounds here. Let me try to find at least some common
> ground. Could we try to agree on *one* of these 2 mutually exclusive
> statements, please:
>
> 1. Current -m behavior is useful
>
> 2. Current -m behavior is useless
>
> To me it looks like Junio thinks 1 is the case, and you, Glen, think
> it's 2 that is the case, ...

I agree you two are going around in circles and it may be time to
agree to disagree and move on.

I am not Glen but I do not think anybody is arguing "-m" that
does not do anything by itself is useful.  By definition, without
help with "-p", it is a no-op, so by itself, it cannot be useful.

The reason why I am not enthused by your series is because changing
behaviour of "-m" can be harmful, because "-m -p" is not the first
choice people would make when they choose to view what a merge
commit did.

Back when "-m" was invented (yes, "-p" came first before "-m"),
people were fairly content with "git log -p" that shows only
individual changes.  After all, merges were to integrate what these
individual changes on each side of the merged history did.  And you
need to be aware that "-c" and "--cc" did not exist, let alone
"--remerge-diff", even as a concept.  The philosophy of Git back
then stressed that these parts of Git were working as building
blocks to feed necessary data to be used by tools people who were
working at higher, end-user facing layers, would write.  And because
we ourselves have been mostly content with "git log -p" output that
omits patches for merge commits so far, a quick approach to give
output for such tools to consume without losing information was what
became "-m -p" output that shows pairwise diffs that such tools can
combine or do whatever they want to do.  This choice was also
influenced by the fact that we considered stronger than we do now
that all sides of a merge are equal---the idea that the first parent
chain was somehow special came much leter with the "--first-parent"
option.

So, given that background, "-m" that came as a modifier for what
happens (only) when "-p" was in use, is perfectly understandable.
In hindsight, it may appear to you that it should imply "-p", and
that it is an oversight that it does not imply it.

But it is more like how "git log --histogram" does not produce patch
output.  The mental model that allows it is that "--histogram" is to
affect the way how "-p" computes the diff---a purist in you may
argue that it should also imply "-p", but in reality, nobody
complained that "--histogram" does not imply "-p", probably because
nobody sane would say "--histogram" when they do not mean to show
diff out of "git log" anyway.

As somebody who saw how various options were invented while they
were added to the system, I view "-m" as an option to decide what
"-p" does when encountered a merge commit, and do not find it any
more odd that it is a no-op without "-p" or it does not imply "-p"
than "--histogram" vs "-p".

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> We are walking in rounds here. Let me try to find at least some common
>> ground. Could we try to agree on *one* of these 2 mutually exclusive
>> statements, please:
>>
>> 1. Current -m behavior is useful
>>
>> 2. Current -m behavior is useless
>>
>> To me it looks like Junio thinks 1 is the case, and you, Glen, think
>> it's 2 that is the case, ...
>
> I agree you two are going around in circles and it may be time to
> agree to disagree and move on.

I'm willing not only to disagree, but even to agree on any of these 2
options, but I can't (dis)agree on both, as they are mutually exclusive.
Yeah, it could be seen as my personal problem, but, well...

> I am not Glen but I do not think anybody is arguing "-m" that does not
> do anything by itself is useful.

I'm sorry, I still can't quite understand your position.

Do you think exact current behavior of "-m" is useful, or is it useless?

> By definition, without help with "-p", it is a no-op, so by itself, it
> cannot be useful.

No, it's not a no-op as I see it, it's just that its effect is hidden
till "-p" is specified as well, and that's the exact "current behavior"
that I'm asking about. Is it useful, or not?

Please let me remind you that at previous rounds there was the commit
that reverted my earlier "Let -m imply -p" patch, and that revert
insists lone "-m" is useful, and you accepted it, and pointed me to the
explanation there, telling me that there is pretty solid use-case for a
lone "-m".

I agreed with you at that time, and then followed with current patch
series created exactly based on that earlier agreement, that now somehow
turns back to disagreement, leaving me sad and confused.

Provided current -m behavior is indeed useful, in these series:

1. Current -m behavior is now supported by --diff-merges (as presumably
   being useful.)

2. Implying -p by -m is made an option (not to break those presumably
   valid uses.)

And now these series are to be rejected on the ground that current "-m"
behavior is useless? Then let's revert the revert?

>
> The reason why I am not enthused by your series is because changing
> behaviour of "-m" can be harmful, because "-m -p" is not the first
> choice people would make when they choose to view what a merge
> commit did.

That's your view on merge commits, but there is another view out there,
where "-m -p" is exactly the first choice, provided "-m" is configured
to give diff with respect to the first parent only. I want to have the
same convenience of saying just "log -m" as you already have with "log
-c". What's wrong about it?

I realize there is a large group here that looks at merge commits
differently, and I respect their choice to show other kinds of
information that could be derived from merges. What I can't understand
is why another view is assumed to be a second-class citizen, sorry.

Please also note that I made behavior change an option, exactly not to
harm those who doesn't need it, even though it's not clear how those who
don't use -m are to be affected anyway.

>
> Back when "-m" was invented (yes, "-p" came first before "-m"), people
> were fairly content with "git log -p" that shows only individual
> changes. After all, merges were to integrate what these individual
> changes on each side of the merged history did. And you need to be
> aware that "-c" and "--cc" did not exist, let alone "--remerge-diff",
> even as a concept.

Yep, I'm aware of all that history now, as I went over it back and forth
a few times already, trying to figure out what't the actual ground
behind desire to *keep* "-m" different from the rest of similar options,
but still found none.

[...]

> So, given that background, "-m" that came as a modifier for what
>happens (only) when "-p" was in use, is perfectly understandable. In
>hindsight, it may appear to you that it should imply "-p", and that it
>is an oversight that it does not imply it.

My point is that the story with -c, or --cc is roughly the same, and
that you yourself fixed --cc once upon a time to imply -p, so it's
especially confusing for me that you are now in opposition to similar
change to "-m".

>
> But it is more like how "git log --histogram" does not produce patch
> output.  The mental model that allows it is that "--histogram" is to
> affect the way how "-p" computes the diff---a purist in you may
> argue that it should also imply "-p", but in reality, nobody
> complained that "--histogram" does not imply "-p", probably because
> nobody sane would say "--histogram" when they do not mean to show
> diff out of "git log" anyway.

Do you then think that making -c/--cc imply "-p" was a mistake in the
first place, and you try to hold me back from doing the same mistake?

> As somebody who saw how various options were invented while they
> were added to the system, I view "-m" as an option to decide what
> "-p" does when encountered a merge commit, and do not find it any
> more odd that it is a no-op without "-p" or it does not imply "-p"
> than "--histogram" vs "-p".

It's fine to view "-m" this way, but how do you view "-c" then? If
differently, then why?

In other words, in the above sentence, among "--histogram -m -c", you
effectively group "(--histogram -m) -c" by leaving -c out of
consideration, whereas more natural grouping is rather "--histogram (-m
-c)", that'd leave --histogram out of consideration.

Overall, my point is that, provided we do care about consistency of Git
UI, it's -m and -c that are to be treated the same, and it's much less
relevant if any of them matches treatment of --histogram.

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> My point is that the story with -c, or --cc is roughly the same, and
> that you yourself fixed --cc once upon a time to imply -p, so it's
> especially confusing for me that you are now in opposition to similar
> change to "-m".

I think we all saw a good explanation for that already in the
thread.

The mistake by "--cc" was fixed relatively quickly, but it is now
way too late to change the behaviour of "-m" without hurting
existing users.  I think I've wasted enough time on this in this
thread already, so let's stop comparing --cc and -m now.

Thanks.

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> My point is that the story with -c, or --cc is roughly the same, and
>> that you yourself fixed --cc once upon a time to imply -p, so it's
>> especially confusing for me that you are now in opposition to similar
>> change to "-m".
>
> I think we all saw a good explanation for that already in the
> thread.
>
> The mistake by "--cc" was fixed relatively quickly, but it is now
> way too late to change the behaviour of "-m" without hurting
> existing users.  I think I've wasted enough time on this in this
> thread already, so let's stop comparing --cc and -m now.

That's fine with me.

I already agreed long ago that to be on the safe side we shouldn't
simply change -m nowadays, and addressed that concern by putting -m
behavior change under configuration option in the current series. So
what's the reason of rejection?

Thanks,
-- Sergey

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Glen Choo]

Sergey Organov <sorganov@gmail.com> writes:

> I already agreed long ago that to be on the safe side we shouldn't
> simply change -m nowadays, and addressed that concern by putting -m
> behavior change under configuration option in the current series. So
> what's the reason of rejection?

Before we start talking in circles again, I think the responses
elsewhere in the thread still accurately capture mine and Junio's views,
e.g.:

  https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g
  https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Glen Choo <chooglen@google.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I already agreed long ago that to be on the safe side we shouldn't
>> simply change -m nowadays, and addressed that concern by putting -m
>> behavior change under configuration option in the current series. So
>> what's the reason of rejection?
>
> Before we start talking in circles again, I think the responses
> elsewhere in the thread still accurately capture mine and Junio's
> views, e.g.:
>
>   https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g
> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/

There were no any response from you to my recent very direct question
(that Junio did somewhat address), but what I figured from your and
Junio explanation so far looks like:

1. The fact that -m does not imply -p is a mistake. There is no any
   reasons this exact behavior could be useful. As such, it does not
   make sense to support this exact behavior in --diff-merges. So the
   reject of --diff-merges=[no-]hide.

2. This mistake is too dangerous to fix due to subtle compatibility
   problems, so we can't just fix -m behavior. Thus the reject of my
   earlier patch "let -m imply -p".

3. Moving behavior change under option is not worth it, as nobody
   presumably needs this fixed -m behavior anyway (at least among 2
   persons that are actually opposing the changes). So the reject
   of "add diffMerges-m-imply-p configuration option" patch.

4. Staring in the face inconsistency between -m and the rest
   of short diff-merge options is not significant enough to reconsider
   any of the above rejects.

Did I outline your positions correctly and to the full extent? Anything
else I forgot?

I want to make sure I got it exactly right, as I'd still like to tweak
my proposals, and I'd like to avoid the troubles I got into as much as
possible.

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Glen Choo]

Sergey Organov <sorganov@gmail.com> writes:

> Glen Choo <chooglen@google.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> I already agreed long ago that to be on the safe side we shouldn't
>>> simply change -m nowadays, and addressed that concern by putting -m
>>> behavior change under configuration option in the current series. So
>>> what's the reason of rejection?
>>
>> Before we start talking in circles again, I think the responses
>> elsewhere in the thread still accurately capture mine and Junio's
>> views, e.g.:
>>
>>   https://lore.kernel.org/git/xmqqedr28wwb.fsf@gitster.g
>> https://lore.kernel.org/git/kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com/
>
> There were no any response from you to my recent very direct question
> (that Junio did somewhat address), but what I figured from your and
> Junio explanation so far looks like:

Yes, thank you for summarizing the discussion. I think the following
sums up my position quite accurately, and I found this very helpful.

> 1. The fact that -m does not imply -p is a mistake. There is no any
>    reasons this exact behavior could be useful. As such, it does not
>    make sense to support this exact behavior in --diff-merges. So the
>    reject of --diff-merges=[no-]hide.

Yes.

> 2. This mistake is too dangerous to fix due to subtle compatibility
>    problems, so we can't just fix -m behavior. Thus the reject of my
>    earlier patch "let -m imply -p".

Yes, unfortunately. If we didn't have compatibility problems, "-m imply
-p" would be very good IMO. I am not that hopeful, but perhaps there are
ways to get users to move off this compatibility problem. It would be
something we would have to learn as a project though, it is not a
problem unique to this series.

> 3. Moving behavior change under option is not worth it, as nobody
>    presumably needs this fixed -m behavior anyway (at least among 2
>    persons that are actually opposing the changes). So the reject
>    of "add diffMerges-m-imply-p configuration option" patch.

Yes, given how specific the option is (I think it would be the only
config option that exists to change the behavior of a CLI option) I
don't think the option is worth it in light of alternatives like a new
"-d" flag or supporting "-mp".

> 4. Staring in the face inconsistency between -m and the rest
>    of short diff-merge options is not significant enough to reconsider
>    any of the above rejects.

Yes. Git carries many such inconsistencies.

> Did I outline your positions correctly and to the full extent? Anything
> else I forgot?

I cannot think of anything else. Thank you.

> I want to make sure I got it exactly right, as I'd still like to tweak
> my proposals, and I'd like to avoid the troubles I got into as much as
> possible.

Thanks. I hope that there is a good way forward for your proposals.

>
> Thanks,
> -- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> 1. The fact that -m does not imply -p is a mistake. There is no any
>    reasons this exact behavior could be useful. As such, it does not
>    make sense to support this exact behavior in --diff-merges. So the
>    reject of --diff-merges=[no-]hide.
>
> 2. This mistake is too dangerous to fix due to subtle compatibility
>    problems, so we can't just fix -m behavior. Thus the reject of my
>    earlier patch "let -m imply -p".
>
> 3. Moving behavior change under option is not worth it, as nobody
>    presumably needs this fixed -m behavior anyway (at least among 2
>    persons that are actually opposing the changes). So the reject
>    of "add diffMerges-m-imply-p configuration option" patch.
>
> 4. Staring in the face inconsistency between -m and the rest
>    of short diff-merge options is not significant enough to reconsider
>    any of the above rejects.

I do not quite understand the last one (#4), and if I were to add my
own 4., it would be that introducing --diff-merges={kind} may have
been a mistake.  It would have been fine and better to just let
users choose from whatever set of options we support, i.e. (-c,
--cc, --remerge-diff, -m -p, -m --raw, ...).

IOW, perhaps deprecate --diff-merges={kind} and eventually remove
it, if we could.  We've been fine without it and we'll be fine
without it.  Unfortunately it may be a bit too late for that,
but it certainly is much younger than "-m".

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> 1. The fact that -m does not imply -p is a mistake. There is no any
>>    reasons this exact behavior could be useful. As such, it does not
>>    make sense to support this exact behavior in --diff-merges. So the
>>    reject of --diff-merges=[no-]hide.
>>
>> 2. This mistake is too dangerous to fix due to subtle compatibility
>>    problems, so we can't just fix -m behavior. Thus the reject of my
>>    earlier patch "let -m imply -p".
>>
>> 3. Moving behavior change under option is not worth it, as nobody
>>    presumably needs this fixed -m behavior anyway (at least among 2
>>    persons that are actually opposing the changes). So the reject
>>    of "add diffMerges-m-imply-p configuration option" patch.
>>
>> 4. Staring in the face inconsistency between -m and the rest
>>    of short diff-merge options is not significant enough to reconsider
>>    any of the above rejects.
>
> I do not quite understand the last one (#4),

Well, -m does not imply -p, whereas the rest of diff-merges options
(-c/--cc/--remerge-diff) do imply -p. This is what half of this
lengthy discussion was about.

> own 4., it would be that introducing --diff-merges={kind} may have
> been a mistake.  It would have been fine and better to just let
> users choose from whatever set of options we support, i.e. (-c,
> --cc, --remerge-diff, -m -p, -m --raw, ...).
>
> IOW, perhaps deprecate --diff-merges={kind} and eventually remove
> it, if we could.

Why? Unlike -m vs -c they are at least self-consistent and besides allow
to get the output that those short options do not.

> We've been fine without it and we'll be fine without it. Unfortunately
> it may be a bit too late for that, but it certainly is much younger
> than "-m".

I, for one, was never fine with what Git does to show diff for merges.
Then, taking into account that introducing of --diff-merges was not
my idea in the first place, there should be at least two of us here
who were not fine.

It's fine with me that --cc is everything you need, but what I need is
rather diff to the first parent, and I had a hope to finally get -m to
do it, aiming both for consistency and convenience. My 2 attempts
performed in different ways both failed, so, being tired of it likely
even more than you do, I digress.

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> I do not quite understand the last one (#4),
>
> Well, -m does not imply -p, whereas the rest of diff-merges options
> (-c/--cc/--remerge-diff) do imply -p. This is what half of this
> lengthy discussion was about.
>
>> own 4., it would be that introducing --diff-merges={kind} may have
>> been a mistake.  It would have been fine and better to just let
>> users choose from whatever set of options we support, i.e. (-c,
>> --cc, --remerge-diff, -m -p, -m --raw, ...).

> It's fine with me that --cc is everything you need, but what I need is
> rather diff to the first parent, ...

I think "show --first-parent" should give that already.  One problem
with "-m implies -p" is that it is unclear what should be done to
things like "-m --raw".  Yes, we can declare an arbitrary choice
(like "-m implies -p unless --raw, --stat, etc. are given") but that
is just replacing an arbitrary rule (i.e. "comparison with parents
are not given for merges, unless things like --cc, --first-parent,
etc. that are specifically designed for showing merges are given;
you can give -m to force pairwise behaviour.") with another one.

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> I do not quite understand the last one (#4),
>>
>> Well, -m does not imply -p, whereas the rest of diff-merges options
>> (-c/--cc/--remerge-diff) do imply -p. This is what half of this
>> lengthy discussion was about.
>>
>>> own 4., it would be that introducing --diff-merges={kind} may have
>>> been a mistake.  It would have been fine and better to just let
>>> users choose from whatever set of options we support, i.e. (-c,
>>> --cc, --remerge-diff, -m -p, -m --raw, ...).
>
>> It's fine with me that --cc is everything you need, but what I need is
>> rather diff to the first parent, ...
>
> I think "show --first-parent" should give that already.

Well, for "git show" even "show -m" does the right thing (once properly
configured), as "-p" is implied by "git show".

Taking "git show" into the picture brings yet another argument if favor
of new "-m" behavior, as then "git show -m" and "git log -m -n1" will
finally start to produce the same result, that I'd find desirable.

That said,

  --diff-merges=first-parent

that could be shortened to

  --diff-merges=1

is the universal answer that works out-of-the-box for any command the
same way, reliably, and then it's also

  -m -p

if configured accordingly, that has been made available by previously
accepted patches.

These series just did the last logical step: allowed it to be just

  -m

if configured accordingly.

> One problem with "-m implies -p" is that it is unclear what should be
> done to things like "-m --raw".

Nothing specific is actually needed, as far as I'm aware, as implied -p
doesn't interfere with --raw. Please give particular example of a
problem if you foresee one.

As I see it, if there is indeed some problem with this, it should
already exist for -c, --cc, and --remerge-diff, and then probably needs
to be fixed anyway. Moreover, it should also exist for "git show", as
the latter implies -p, and there is:

       -s, --no-patch
           Suppress diff output. Useful for commands like git show that show
           the patch by default, or to cancel the effect of --patch.

[As a side-note, current behavior of implied -p, explicit -p, -s, and
--raw with respect to each other that I figured by experiment looks
suspect to me. E.g., once explicit -p is given, and then canceled by -s,
I can't get bare --raw output anymore]

> Yes, we can declare an arbitrary choice (like "-m implies -p unless
> --raw, --stat, etc. are given") but that is just replacing an
> arbitrary rule [...] with another one.

Uh, this would be too cumbersome indeed, but fortunately it does not
seem to be needed, see above.

Overall, letting -m imply -p just makes things more consistent, even on
the problems side (if any), and I honestly still don't see'em.

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>>> I do not quite understand the last one (#4),
>>>
>>> Well, -m does not imply -p, whereas the rest of diff-merges options
>>> (-c/--cc/--remerge-diff) do imply -p. This is what half of this
>>> lengthy discussion was about.
>>>
>>>> own 4., it would be that introducing --diff-merges={kind} may have
>>>> been a mistake.  It would have been fine and better to just let
>>>> users choose from whatever set of options we support, i.e. (-c,
>>>> --cc, --remerge-diff, -m -p, -m --raw, ...).
>>
>>> It's fine with me that --cc is everything you need, but what I need is
>>> rather diff to the first parent, ...
>>
>> I think "show --first-parent" should give that already.
>
> Well, for "git show" even "show -m" does the right thing (once properly
> configured), as "-p" is implied by "git show".
>
> Taking "git show" into the picture brings yet another argument if favor
> of new "-m" behavior, as then "git show -m" and "git log -m -n1" will
> finally start to produce the same result, that I'd find desirable.
>
> That said,
>
>   --diff-merges=first-parent
>
> that could be shortened to
>
>   --diff-merges=1
>
> is the universal answer that works out-of-the-box for any command the
> same way, reliably, and then it's also
>
>   -m -p
>
> if configured accordingly, that has been made available by previously
> accepted patches.
>
> These series just did the last logical step: allowed it to be just
>
>   -m
>
> if configured accordingly.
>
>> One problem with "-m implies -p" is that it is unclear what should be
>> done to things like "-m --raw".
>
> Nothing specific is actually needed, as far as I'm aware, as implied -p
> doesn't interfere with --raw. Please give particular example of a
> problem if you foresee one.

In fact there is already a test for it that I added some time ago (as
well as for --stat), so a problem would have been caught. Please also
notice that it was an agreed goal for "-m" to finally imply "-p" at that
time:

commit 48229c193d2e6e728d3243bacea2f1e1490ced8a
Author: Sergey Organov <sorganov@gmail.com>
Date:   Fri May 21 00:46:55 2021 +0300

t4013: test "git log -m --raw"

This is to ensure we won't break different diff formats when we start
to imply "-p" by "-m".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks,
-- Sergey Organov

Re: so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

[...]

> [As a side-note, current behavior of implied -p, explicit -p, -s, and
> --raw with respect to each other that I figured by experiment looks
> suspect to me. E.g., once explicit -p is given, and then canceled by -s,
> I can't get bare --raw output anymore]

Specifically, this test unexpectedly fails:

modified   t/t4013-diff-various.sh
@@ -457,6 +457,16 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+# This should succeed, but --patch --no-patch does something hidden
+# that breaks --raw.
+test_expect_success '--no-patch only cancels --patch' '
+	git log --raw master >result &&
+	process_diffs result >expected &&
+	git log --patch --no-patch --raw >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -m matches pure log' '
 	git log master >result &&
 	process_diffs result >expected &&


Thanks,
-- Sergey Organov