threads / announce / 49877

[ANNOUNCE] Git v2.20.0-rc1

Subject: [ANNOUNCE] Git v2.20.0-rc1

## tl;dr

53 messages between Nov 21, 2018 and Dec 4, 2018.

replies: 52people: 8as markdown or json

Junio C Hamano· Nov 21, 2018, 15:20 UTC · lore

A release candidate Git v2.20.0-rc1 is now available for testing at the usual places. It is comprised of 915 non-merge commits since v2.19.0, contributed by 73 people, 24 of which are new faces.

The tarballs are found at:
    https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the 'v2.20.0-rc1' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.19.0 are as follows. Welcome to the Git development community!

  Aaron Lindsay, Alexander Pyhalov, Anton Serbulov, Brendan
  Forster, Carlo Marcelo Arenas Belón, Daniels Umanovskis, David
  Zych, Đoàn Trần Công Danh, Frederick Eaton, James Knight,
  Jann Horn, Joshua Watt, Loo Rong Jie, Lucas De Marchi, Matthew
  DeVore, Mihir Mehta, Nickolai Belakovski, Roger Strain, Sam
  McKelvie, Saulius Gurklys, Shulhan, Steven Fernandez, Strain,
  Roger L, and Tim Schumacher.

Returning contributors who helped this release are as follows. Thanks for your continued support.

  Ævar Arnfjörð Bjarmason, Alban Gruin, Andreas Gruenbacher,
  Andreas Heiduk, Antonio Ospite, Ben Peart, Brandon Williams,
  brian m. carlson, Christian Couder, Christian Hesse, Denton Liu,
  Derrick Stolee, Elijah Newren, Eric Sunshine, Jeff Hostetler,
  Jeff King, Johannes Schindelin, Johannes Sixt, Jonathan Nieder,
  Jonathan Tan, Josh Steadmon, Junio C Hamano, Karsten Blees,
  Luke Diamand, Martin Ågren, Max Kirillov, Michael Witten,
  Michał Górny, Nguyễn Thái Ngọc Duy, Noam Postavsky,
  Olga Telezhnaya, Phillip Wood, Pratik Karki, Rafael Ascensão,
  Ralf Thielow, Ramsay Jones, Rasmus Villemoes, René Scharfe,
  Sebastian Staudt, Stefan Beller, Stephen P. Smith, Steve Hoelzer,
  SZEDER Gábor, Tao Qingyun, Taylor Blau, Thomas Gummerer,
  Todd Zullinger, Torsten Bögershausen, and Uwe Kleine-König.
----------------------------------------------------------------

Git 2.20 Release Notes (draft) ==============================

Backward Compatibility Notes ----------------------------

 * "git branch -l <foo>" used to be a way to ask a reflog to be
   created while creating a new branch, but that is no longer the
   case.  It is a short-hand for "git branch --list <foo>" now.
 * "git push" into refs/tags/* hierarchy is rejected without getting
   forced, but "git fetch" (misguidedly) used the "fast forwarding"
   rule used for the refs/heads/* hierarchy; this has been corrected,
   which means some fetches of tags that did not fail with older
   version of Git will fail without "--force" with this version.
 * "git help -a" now gives verbose output (same as "git help -av").
   Those who want the old output may say "git help --no-verbose -a"..
 * "git cpn --help", when "cpn" is an alias to, say, "cherry-pick -n",
   reported only the alias expansion of "cpn" in earlier versions of
   Git.  It now runs "git cherry-pick --help" to show the manual page
   of the command, while sending the alias expansion to the standard
   error stream.
 * "git send-email" learned to grab address-looking string on any
   trailer whose name ends with "-by". This is a backward-incompatible
   change.  Adding "--suppress-cc=misc-by" on the command line, or
   setting sendemail.suppresscc configuration variable to "misc-by",
   can be used to disable this behaviour.

Updates since v2.19 -------------------

UI, Workflows & Features
 * Running "git clone" against a project that contain two files with
   pathnames that differ only in cases on a case insensitive
   filesystem would result in one of the files lost because the
   underlying filesystem is incapable of holding both at the same
   time.  An attempt is made to detect such a case and warn.
 * "git checkout -b newbranch [HEAD]" should not have to do as much as
   checking out a commit different from HEAD.  An attempt is made to
   optimize this special case.
 * "git rev-list --stdin </dev/null" used to be an error; it now shows
   no output without an error.  "git rev-list --stdin --default HEAD"
   still falls back to the given default when nothing is given on the
   standard input.
 * Lift code from GitHub to restrict delta computation so that an
   object that exists in one fork is not made into a delta against
   another object that does not appear in the same forked repository.
 * "git format-patch" learned new "--interdiff" and "--range-diff"
   options to explain the difference between this version and the
   previous attempt in the cover letter (or after the three-dashes as
   a comment).
 * "git mailinfo" used in "git am" learned to make a best-effort
   recovery of a patch corrupted by MUA that sends text/plain with
   format=flawed option.
   (merge 3aa4d81f88 rs/mailinfo-format-flowed later to maint).
 * The rules used by "git push" and "git fetch" to determine if a ref
   can or cannot be updated were inconsistent; specifically, fetching
   to update existing tags were allowed even though tags are supposed
   to be unmoving anchoring points.  "git fetch" was taught to forbid
   updates to existing tags without the "--force" option.
 * "git multi-pack-index" learned to detect corruption in the .midx
   file it uses, and this feature has been integrated into "git fsck".
 * Generation of (experimental) commit-graph files have so far been
   fairly silent, even though it takes noticeable amount of time in a
   meaningfully large repository.  The users will now see progress
   output.
 * The minimum version of Windows supported by Windows port of Git is
   now set to Vista.
 * The completion script (in contrib/) learned to complete a handful of
   options "git stash list" command takes.
 * The completion script (in contrib/) learned that "git fetch
   --multiple" only takes remote names as arguments and no refspecs.
 * "git status" learns to show progress bar when refreshing the index
   takes a long time.
   (merge ae9af12287 nd/status-refresh-progress later to maint).
 * "git help -a" and "git help -av" give different pieces of
   information, and generally the "verbose" version is more friendly
   to the new users.  "git help -a" by default now uses the more
   verbose output (with "--no-verbose", you can go back to the
   original).  Also "git help -av" now lists aliases and external
   commands, which it did not used to.
 * Unlike "grep", "git grep" by default recurses to the whole tree.
   The command learned "git grep --recursive" option, so that "git
   grep --no-recursive" can serve as a synonym to setting the
   max-depth to 0.
 * When pushing into a repository that borrows its objects from an
   alternate object store, "git receive-pack" that responds to the
   push request on the other side lists the tips of refs in the
   alternate to reduce the amount of objects transferred.  This
   sometimes is detrimental when the number of refs in the alternate
   is absurdly large, in which case the bandwidth saved in potentially
   fewer objects transferred is wasted in excessively large ref
   advertisement.  The alternate refs that are advertised are now
   configurable with a pair of configuration variables.
 * "git cmd --help" when "cmd" is aliased used to only say "cmd is
   aliased to ...".  Now it shows that to the standard error stream
   and runs "git $cmd --help" where $cmd is the first word of the
   alias expansion.
 * The documentation of "git gc" has been updated to mention that it
   is no longer limited to "pruning away crufts" but also updates
   ancillary files like commit-graph as a part of repository
   optimization.
 * "git p4 unshelve" improvements.
 * The logic to select the default user name and e-mail on Windows has
   been improved.
   (merge 501afcb8b0 js/mingw-default-ident later to maint).
 * The "rev-list --filter" feature learned to exclude all trees via
   "tree:0" filter.
 * "git send-email" learned to grab address-looking string on any
   trailer whose name ends with "-by"; --suppress-cc=misc-by on the
   command line, or setting sendemail.suppresscc configuration
   variable to "misc-by", can be used to disable this behaviour.
 * Developer builds now uses -Wunused-function compilation option.
 * One of our CI tests to run with "unusual/experimental/random"
   settings now also uses commit-graph and midx.
 * "git mergetool" learned to take the "--[no-]gui" option, just like
   "git difftool" does.
 * "git rebase -i" learned a new insn, 'break', that the user can
   insert in the to-do list.  Upon hitting it, the command returns
   control back to the user.
 * New "--pretty=format:" placeholders %GF and %GP that show the GPG
   key fingerprints have been invented.
 * On platforms with recent cURL library, http.sslBackend configuration
   variable can be used to choose a different SSL backend at runtime.
   The Windows port uses this mechanism to switch between OpenSSL and
   Secure Channel while talking over the HTTPS protocol.
 * "git send-email" learned to disable SMTP authentication via the
   "--smtp-auth=none" option, even when the smtp username is given
   (which turns the authentication on by default).
 * A fourth class of configuration files (in addition to the
   traditional "system wide", "per user in the $HOME directory" and
   "per repository in the $GIT_DIR/config") has been introduced so
   that different worktrees that share the same repository (hence the
   same $GIT_DIR/config file) can use different customization.
 * A pattern with '**' that does not have a slash on either side used
   to be an invalid one, but the code now treats such double-asterisks
   the same way as two normal asterisks that happen to be adjacent to
   each other.
   (merge e5bbe09e88 nd/wildmatch-double-asterisk later to maint).
 * The "--no-patch" option, which can be used to get a high-level
   overview without the actual line-by-line patch difference shown, of
   the "range-diff" command was earlier broken, which has been
   corrected.
 * The recently merged "rebase in C" has an escape hatch to use the
   scripted version when necessary, but it hasn't been documented,
   which has been corrected.
Performance, Internal Implementation, Development Support etc.
 * When there are too many packfiles in a repository (which is not
   recommended), looking up an object in these would require
   consulting many pack .idx files; a new mechanism to have a single
   file that consolidates all of these .idx files is introduced.
 * "git submodule update" is getting rewritten piece-by-piece into C.
 * The code for computing history reachability has been shuffled,
   obtained a bunch of new tests to cover them, and then being
   improved.
 * The unpack_trees() API used in checking out a branch and merging
   walks one or more trees along with the index.  When the cache-tree
   in the index tells us that we are walking a tree whose flattened
   contents is known (i.e. matches a span in the index), as linearly
   scanning a span in the index is much more efficient than having to
   open tree objects recursively and listing their entries, the walk
   can be optimized, which has been done.
 * When creating a thin pack, which allows objects to be made into a
   delta against another object that is not in the resulting pack but
   is known to be present on the receiving end, the code learned to
   take advantage of the reachability bitmap; this allows the server
   to send a delta against a base beyond the "boundary" commit.
 * spatch transformation to replace boolean uses of !hashcmp() to
   newly introduced oideq() is added, and applied, to regain
   performance lost due to support of multiple hash algorithms.
 * Fix a bug in which the same path could be registered under multiple
   worktree entries if the path was missing (for instance, was removed
   manually).  Also, as a convenience, expand the number of cases in
   which --force is applicable.
 * Split Documentation/config.txt for easier maintenance.
   (merge 6014363f0b nd/config-split later to maint).
 * Test helper binaries clean-up.
   (merge c9a1f4161f nd/test-tool later to maint).
 * Various tests have been updated to make it easier to swap the
   hash function used for object identification.
   (merge ae0c89d41b bc/hash-independent-tests later to maint).
 * Update fsck.skipList implementation and documentation.
   (merge 371a655074 ab/fsck-skiplist later to maint).
 * An alias that expands to another alias has so far been forbidden,
   but now it is allowed to create such an alias.
 * Various test scripts have been updated for style and also correct
   handling of exit status of various commands.
 * "gc --auto" ended up calling exit(-1) upon error, which has been
   corrected to use exit(1).  Also the error reporting behaviour when
   daemonized has been updated to exit with zero status when stopping
   due to a previously discovered error (which implies there is no
   point running gc to improve the situation); we used to exit with
   failure in such a case.
 * Various codepaths in the core-ish part learned to work on an
   arbitrary in-core index structure, not necessarily the default
   instance "the_index".
   (merge b3c7eef9b0 nd/the-index later to maint).
 * Code clean-up in the internal machinery used by "git status" and
   "git commit --dry-run".
   (merge 73ba5d78b4 ss/wt-status-committable later to maint).
 * Some environment variables that control the runtime options of Git
   used during tests are getting renamed for consistency.
   (merge 4231d1ba99 bp/rename-test-env-var later to maint).
 * A pair of new extensions to the index file have been introduced.
   They allow the index file to be read in parallel for performance.
 * The oidset API was built on top of the oidmap API which in turn is
   on the hashmap API.  Replace the implementation to build on top of
   the khash API and gain performance.
 * Over some transports, fetching objects with an exact commit object
   name can be done without first seeing the ref advertisements.  The
   code has been optimized to exploit this.
 * In a partial clone that will lazily be hydrated from the
   originating repository, we generally want to avoid "does this
   object exist (locally)?" on objects that we deliberately omitted
   when we created the clone.  The cache-tree codepath (which is used
   to write a tree object out of the index) however insisted that the
   object exists, even for paths that are outside of the partial
   checkout area.  The code has been updated to avoid such a check.
 * To help developers, an EditorConfig file that attempts to follow
   the project convention has been added.
   (merge b548d698a0 bc/editorconfig later to maint).
 * The result of coverage test can be combined with "git blame" to
   check the test coverage of code introduced recently with a new
   'coverage-diff' tool (in contrib/).
   (merge 783faedd65 ds/coverage-diff later to maint).
 * An experiment to fuzz test a few areas, hopefully we can gain more
   coverage to various areas.
 * More codepaths are moving away from hardcoded hash sizes.
 * The way the Windows port figures out the current directory has been
   improved.
 * The way DLLs are loaded on the Windows port has been improved.
 * Some tests have been reorganized and renamed; "ls t/" now gives a
   better overview of what is tested for these scripts than before.
 * "git rebase" and "git rebase -i" have been reimplemented in C.
 * Windows port learned to use nano-second resolution file timestamps.
 * The overly large Documentation/config.txt file have been split into
   million little pieces.  This potentially allows each individual piece
   included into the manual page of the command it affects more easily.
 * Replace three string-list instances used as look-up tables in "git
   fetch" with hashmaps.
 * Unify code to read the author-script used in "git am" and the
   commands that use the sequencer machinery, e.g. "git rebase -i".
 * In preparation to the day when we can deprecate and remove the
   "rebase -p", make sure we can skip and later remove tests for
   it.
 * The history traversal used to implement the tag-following has been
   optimized by introducing a new helper.
 * The helper function to refresh the cached stat information in the
   in-core index has learned to perform the lstat() part of the
   operation in parallel on multi-core platforms.
 * The code to traverse objects for reachability, used to decide what
   objects are unreferenced and expendable, have been taught to also
   consider per-worktree refs of other worktrees as starting points to
   prevent data loss.
 * "git add" needs to internally run "diff-files" equivalent, and the
   codepath learned the same optimization as "diff-files" has to run
   lstat(2) in parallel to find which paths have been updated in the
   working tree.
 * The procedure to install dependencies before testing at Travis CI
   is getting revamped for both simplicity and flexibility, taking
   advantage of the recent move to the vm-based environment.
 * The support for format-patch (and send-email) by the command-line
   completion script (in contrib/) has been simplified a bit.
 * The revision walker machinery learned to take advantage of the
   commit generation numbers stored in the commit-graph file.
 * The codebase has been cleaned up to reduce "#ifndef NO_PTHREADS".
 * The way -lcurl library gets linked has been simplified by taking
   advantage of the fact that we can just ask curl-config command how.
 * Various functions have been audited for "-Wunused-parameter" warnings
   and bugs in them got fixed.
 * A sanity check for start-up sequence has been added in the config
   API codepath.
 * The build procedure to link for fuzzing test has been made
   customizable with a new Makefile variable.
 * The way "git rebase" parses and forwards the command line options
   meant for underlying "git am" has been revamped, which fixed for
   options with parameters that were not passed correctly.
 * Our testing framework uses a special i18n "poisoned localization"
   feature to find messages that ought to stay constant but are
   incorrectly marked to be translated.  This feature has been made
   into a runtime option (it used to be a compile-time option).
 * "git push" used to check ambiguities between object-names and
   refnames while processing the list of refs' old and new values,
   which was unnecessary (as it knew that it is feeding raw object
   names).  This has been optimized out.
 * The xcurl_off_t() helper function is used to cast size_t to
   curl_off_t, but some compilers gave warnings against the code to
   ensure the casting is done without wraparound, when size_t is
   narrower than curl_off_t.  This warning has been squelched.
 * Code preparation to replace ulong vars with size_t vars where
   appropriate continues.
 * The "test installed Git" mode of our test suite has been updated to
   work better.
 * A coding convention around the Coccinelle semantic patches to have
   two classes to ease code migration process has been proposed and
   its support has been added to the Makefile.

Fixes since v2.19 -----------------

 * "git interpret-trailers" and its underlying machinery had a buggy
   code that attempted to ignore patch text after commit log message,
   which triggered in various codepaths that will always get the log
   message alone and never get such an input.
   (merge 66e83d9b41 jk/trailer-fixes later to maint).
 * Malformed or crafted data in packstream can make our code attempt
   to read or write past the allocated buffer and abort, instead of
   reporting an error, which has been fixed.
 * "git rebase -i" did not clear the state files correctly when a run
   of "squash/fixup" is aborted and then the user manually amended the
   commit instead, which has been corrected.
   (merge 10d2f35436 js/rebase-i-autosquash-fix later to maint).
 * When fsmonitor is in use, after operation on submodules updates
   .gitmodules, we lost track of the fact that we did so and relied on
   stale fsmonitor data.
   (merge 43f1180814 bp/mv-submodules-with-fsmonitor later to maint).
 * Fix for a long-standing bug that leaves the index file corrupt when
   it shrinks during a partial commit.
   (merge 6c003d6ffb jk/reopen-tempfile-truncate later to maint).
 * Further fix for O_APPEND emulation on Windows
   (merge eeaf7ddac7 js/mingw-o-append later to maint).
 * A corner case bugfix in "git rerere" code.
   (merge ad2bf0d9b4 en/rerere-multi-stage-1-fix later to maint).
 * "git add ':(attr:foo)'" is not supported and is supposed to be
   rejected while the command line arguments are parsed, but we fail
   to reject such a command line upfront.
   (merge 84d938b732 nd/attr-pathspec-fix later to maint).
 * Recent update broke the reachability algorithm when refs (e.g.
   tags) that point at objects that are not commit were involved,
   which has been fixed.
 * "git rebase" etc. in Git 2.19 fails to abort when given an empty
   commit log message as result of editing, which has been corrected.
   (merge a3ec9eaf38 en/sequencer-empty-edit-result-aborts later to maint).
 * The code to backfill objects in lazily cloned repository did not
   work correctly, which has been corrected.
   (merge e68302011c jt/lazy-object-fetch-fix later to maint).
 * Update error messages given by "git remote" and make them consistent.
   (merge 5025425dff ms/remote-error-message-update later to maint).
 * "git update-ref" learned to make both "--no-deref" and "--stdin"
   work at the same time.
   (merge d345e9fbe7 en/update-ref-no-deref-stdin later to maint).
 * Recently added "range-diff" had a corner-case bug to cause it
   segfault, which has been corrected.
   (merge e467a90c7a tg/range-diff-corner-case-fix later to maint).
 * The recently introduced commit-graph auxiliary data is incompatible
   with mechanisms such as replace & grafts that "breaks" immutable
   nature of the object reference relationship.  Disable optimizations
   based on its use (and updating existing commit-graph) when these
   incompatible features are in use in the repository.
   (merge 829a321569 ds/commit-graph-with-grafts later to maint).
 * The mailmap file update.
   (merge 255eb03edf jn/mailmap-update later to maint).
 * The code in "git status" sometimes hit an assertion failure.  This
   was caused by a structure that was reused without cleaning the data
   used for the first run, which has been corrected.
   (merge 3e73cc62c0 en/status-multiple-renames-to-the-same-target-fix later to maint).
 * "git fetch $repo $object" in a partial clone did not correctly
   fetch the asked-for object that is referenced by an object in
   promisor packfile, which has been fixed.
 * A corner-case bugfix.
   (merge c5cbb27cb5 sm/show-superproject-while-conflicted later to maint).
 * Various fixes to "diff --color-moved-ws".
 * A partial clone that is configured to lazily fetch missing objects
   will on-demand issue a "git fetch" request to the originating
   repository to fill not-yet-obtained objects.  The request has been
   optimized for requesting a tree object (and not the leaf blob
   objects contained in it) by telling the originating repository that
   no blobs are needed.
   (merge 4c7f9567ea jt/non-blob-lazy-fetch later to maint).
 * The codepath to support the experimental split-index mode had
   remaining "racily clean" issues fixed.
   (merge 4c490f3d32 sg/split-index-racefix later to maint).
 * "git log --graph" showing an octopus merge sometimes miscounted the
   number of display columns it is consuming to show the merge and its
   parent commits, which has been corrected.
   (merge 04005834ed np/log-graph-octopus-fix later to maint).
 * "git range-diff" did not work well when the compared ranges had
   changes in submodules and the "--submodule=log" was used.
 * The implementation of run_command() API on the UNIX platforms had a
   bug that caused a command not on $PATH to be found in the current
   directory.
   (merge f67b980771 jk/run-command-notdot later to maint).
 * A mutex used in "git pack-objects" were not correctly initialized
   and this caused "git repack" to dump core on Windows.
   (merge 34204c8166 js/pack-objects-mutex-init-fix later to maint).
 * Under certain circumstances, "git diff D:/a/b/c D:/a/b/d" on
   Windows would strip initial parts from the paths because they
   were not recognized as absolute, which has been corrected.
   (merge ffd04e92e2 js/diff-notice-has-drive-prefix later to maint).
 * The receive.denyCurrentBranch=updateInstead codepath kicked in even
   when the push should have been rejected due to other reasons, such
   as it does not fast-forward or the update-hook rejects it, which
   has been corrected.
   (merge b072a25fad jc/receive-deny-current-branch-fix later to maint).
 * The logic to determine the archive type "git archive" uses did not
   correctly kick in for "git archive --remote", which has been
   corrected.
 * "git repack" in a shallow clone did not correctly update the
   shallow points in the repository, leading to a repository that
   does not pass fsck.
   (merge 5dcfbf564c js/shallow-and-fetch-prune later to maint).
 * Some codepaths failed to form a proper URL when .gitmodules record
   the URL to a submodule repository as relative to the repository of
   superproject, which has been corrected.
   (merge e0a862fdaf sb/submodule-url-to-absolute later to maint).
 * "git fetch" over protocol v2 into a shallow repository failed to
   fetch full history behind a new tip of history that was diverged
   before the cut-off point of the history that was previously fetched
   shallowly.
 * The command line completion machinery (in contrib/) has been
   updated to allow the completion script to tweak the list of options
   that are reported by the parse-options machinery correctly.
   (merge 276b49ff34 nd/completion-negation later to maint).
 * Operations on promisor objects make sense in the context of only a
   small subset of the commands that internally use the revisions
   machinery, but the "--exclude-promisor-objects" option were taken
   and led to nonsense results by commands like "log", to which it
   didn't make much sense.  This has been corrected.
   (merge 669b1d2aae md/exclude-promisor-objects-fix later to maint).
 * The "container" mode of TravisCI is going away.  Our .travis.yml
   file is getting prepared for the transition.
   (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint).
 * Our test scripts can now take the '-V' option as a synonym for the
   '--verbose-log' option.
   (merge a5f52c6dab sg/test-verbose-log later to maint).
 * A regression in Git 2.12 era made "git fsck" fall into an infinite
   loop while processing truncated loose objects.
   (merge 18ad13e5b2 jk/detect-truncated-zlib-input later to maint).
 * "git ls-remote $there foo" was broken by recent update for the
   protocol v2 and stopped showing refs that match 'foo' that are not
   refs/{heads,tags}/foo, which has been fixed.
   (merge 6a139cdd74 jk/proto-v2-ref-prefix-fix later to maint).
 * Additional comment on a tricky piece of code to help developers.
   (merge 0afbe3e806 jk/stream-pack-non-delta-clarification later to maint).
 * A couple of tests used to leave the repository in a state that is
   deliberately corrupt, which have been corrected.
   (merge aa984dbe5e ab/pack-tests-cleanup later to maint).
 * The submodule support has been updated to read from the blob at
   HEAD:.gitmodules when the .gitmodules file is missing from the
   working tree.
   (merge 2b1257e463 ao/submodule-wo-gitmodules-checked-out later to maint).
 * "git fetch" was a bit loose in parsing responses from the other side
   when talking over the protocol v2.
 * "git rev-parse --exclude=* --branches --branches"  (i.e. first
   saying "add only things that do not match '*' out of all branches"
   and then adding all branches, without any exclusion this time")
   worked as expected, but "--exclude=* --all --all" did not work the
   same way, which has been fixed.
   (merge 5221048092 ag/rev-parse-all-exclude-fix later to maint).
 * "git send-email --transfer-encoding=..." in recent versions of Git
   sometimes produced an empty "Content-Transfer-Encoding:" header,
   which has been corrected.
   (merge 3c88e46f1a al/send-email-auto-cte-fixup later to maint).
 * The interface into "xdiff" library used to discover the offset and
   size of a generated patch hunk by first formatting it into the
   textual hunk header "@@ -n,m +k,l @@" and then parsing the numbers
   out.  A new interface has been introduced to allow callers a more
   direct access to them.
   (merge 5eade0746e jk/xdiff-interface later to maint).
 * Pathspec matching against a tree object were buggy when negative
   pathspec elements were involved, which has been fixed.
   (merge b7845cebc0 nd/tree-walk-path-exclusion later to maint).
 * "git merge" and "git pull" that merges into an unborn branch used
   to completely ignore "--verify-signatures", which has been
   corrected.
   (merge 01a31f3bca jk/verify-sig-merge-into-void later to maint).
 * "git rebase --autostash" did not correctly re-attach the HEAD at times.
 * "rev-parse --exclude=<pattern> --branches=<pattern>" etc. did not
   quite work, which has been corrected.
   (merge 9ab9b5df0e ra/rev-parse-exclude-glob later to maint).
 * When editing a patch in a "git add -i" session, a hunk could be
   made to no-op.  The "git apply" program used to reject a patch with
   such a no-op hunk to catch user mistakes, but it is now updated to
   explicitly allow a no-op hunk in an edited patch.
   (merge 22cb3835b9 js/apply-recount-allow-noop later to maint).
 * The URL to an MSDN page in a comment has been updated.
   (merge 2ef2ae2917 js/mingw-msdn-url later to maint).
 * "git ls-remote --sort=<thing>" can feed an object that is not yet
   available into the comparison machinery and segfault, which has
   been corrected to check such a request upfront and reject it.
 * When "git bundle" aborts due to an empty commit ranges
   (i.e. resulting in an empty pack), it left a file descriptor to an
   lockfile open, which resulted in leftover lockfile on Windows where
   you cannot remove a file with an open file descriptor.  This has
   been corrected.
   (merge 2c8ee1f53c jk/close-duped-fd-before-unlock-for-bundle later to maint).
 * "git format-patch --stat=<width>" can be used to specify the width
   used by the diffstat (shown in the cover letter).
   (merge 284aeb7e60 nd/format-patch-cover-letter-stat-width later to maint).
 * The way .git/index and .git/sharedindex* files were initially
   created gave these files different perm bits until they were
   adjusted for shared repository settings.  This was made consistent.
   (merge c9d6c78870 cc/shared-index-permbits later to maint).
 * Code cleanup, docfix, build fix, etc.
   (merge 96a7501aad ts/doc-build-manpage-xsl-quietly later to maint).
   (merge b9b07efdb2 tg/conflict-marker-size later to maint).
   (merge fa0aeea770 sg/doc-trace-appends later to maint).
   (merge d64324cb60 tb/void-check-attr later to maint).
   (merge c3b9bc94b9 en/double-semicolon-fix later to maint).
   (merge 79336116f5 sg/t3701-tighten-trace later to maint).
   (merge 801fa63a90 jk/dev-build-format-security later to maint).
   (merge 0597dd62ba sb/string-list-remove-unused later to maint).
   (merge db2d36fad8 bw/protocol-v2 later to maint).
   (merge 456d7cd3a9 sg/split-index-test later to maint).
   (merge 7b6057c852 tq/refs-internal-comment-fix later to maint).
   (merge 29e8dc50ad tg/t5551-with-curl-7.61.1 later to maint).
   (merge 55f6bce2c9 fe/doc-updates later to maint).
   (merge 7987d2232d jk/check-everything-connected-is-long-gone later to maint).
   (merge 4ba3c9be47 dz/credential-doc-url-matching-rules later to maint).
   (merge 4c399442f7 ma/commit-graph-docs later to maint).
   (merge fc0503b04e ma/t1400-undebug-test later to maint).
   (merge e56b53553a nd/packobjectshook-doc-fix later to maint).
   (merge c56170a0c4 ma/mailing-list-address-in-git-help later to maint).
   (merge 6e8fc70fce rs/sequencer-oidset-insert-avoids-dups later to maint).
   (merge ad0b8f9575 mw/doc-typofixes later to maint).
   (merge d9f079ad1a jc/how-to-document-api later to maint).
   (merge b1492bf315 ma/t7005-bash-workaround later to maint).
   (merge ac1f98a0df du/rev-parse-is-plumbing later to maint).
   (merge ca8ed443a5 mm/doc-no-dashed-git later to maint).
   (merge ce366a8144 du/get-tar-commit-id-is-plumbing later to maint).
   (merge 61018fe9e0 du/cherry-is-plumbing later to maint).
   (merge c7e5fe79b9 sb/strbuf-h-update later to maint).
   (merge 8d2008196b tq/branch-create-wo-branch-get later to maint).
   (merge 2e3c894f4b tq/branch-style-fix later to maint).
   (merge c5d844af9c sg/doc-show-branch-typofix later to maint).
   (merge 081d91618b ah/doc-updates later to maint).
   (merge b84c783882 jc/cocci-preincr later to maint).
   (merge 5e495f8122 uk/merge-subtree-doc-update later to maint).
   (merge aaaa881822 jk/uploadpack-packobjectshook-fix later to maint).
   (merge 3063477445 tb/char-may-be-unsigned later to maint).
   (merge 8c64bc9420 sg/test-rebase-editor-fix later to maint).
   (merge 71571cd7d6 ma/sequencer-do-reset-saner-loop-termination later to maint).
   (merge 9a4cb8781e cb/notes-freeing-always-null-fix later to maint).
----------------------------------------------------------------
Changes since v2.19.0 are as follows:
Aaron Lindsay (1):
      send-email: avoid empty transfer encoding header
Alban Gruin (21):
      sequencer: make three functions and an enum from sequencer.c public
      rebase -i: rewrite append_todo_help() in C
      editor: add a function to launch the sequence editor
      rebase -i: rewrite the edit-todo functionality in C
      sequencer: add a new function to silence a command, except if it fails
      rebase -i: rewrite setup_reflog_action() in C
      rebase -i: rewrite checkout_onto() in C
      sequencer: refactor append_todo_help() to write its message to a buffer
      sequencer: change the way skip_unnecessary_picks() returns its result
      t3404: todo list with commented-out commands only aborts
      rebase -i: rewrite complete_action() in C
      rebase -i: remove unused modes and functions
      rebase -i: implement the logic to initialize $revisions in C
      rebase -i: rewrite the rest of init_revisions_and_shortrevisions() in C
      rebase -i: rewrite write_basic_state() in C
      rebase -i: rewrite init_basic_state() in C
      rebase -i: implement the main part of interactive rebase as a builtin
      rebase--interactive2: rewrite the submodes of interactive rebase in C
      rebase -i: remove git-rebase--interactive.sh
      rebase -i: move rebase--helper modes to rebase--interactive
      p3400: replace calls to `git checkout -b' by `git checkout -B'
Alexander Pyhalov (1):
      t7005-editor: quote filename to fix whitespace-issue
Andreas Gruenbacher (1):
      rev-parse: clear --exclude list after 'git rev-parse --all'
Andreas Heiduk (6):
      doc: clarify boundaries of 'git worktree list --porcelain'
      doc: fix ASCII art tab spacing
      doc: fix inappropriate monospace formatting
      doc: fix descripion for 'git tag --format'
      doc: fix indentation of listing blocks in gitweb.conf.txt
      doc: fix formatting in git-update-ref
Anton Serbulov (1):
      mingw: fix getcwd when the parent directory cannot be queried
Antonio Ospite (10):
      submodule: add a print_config_from_gitmodules() helper
      submodule: factor out a config_set_in_gitmodules_file_gently function
      t7411: merge tests 5 and 6
      t7411: be nicer to future tests and really clean things up
      submodule--helper: add a new 'config' subcommand
      submodule: use the 'submodule--helper config' command
      t7506: clean up .gitmodules properly before setting up new scenario
      submodule: add a helper to check if it is safe to write to .gitmodules
      submodule: support reading .gitmodules when it's not in the working tree
      t/helper: add test-submodule-nested-repo-config
Ben Peart (19):
      checkout: optimize "git checkout -b <new_branch>"
      git-mv: allow submodules and fsmonitor to work together
      t/README: correct spelling of "uncommon"
      preload-index: use git_env_bool() not getenv() for customization
      fsmonitor: update GIT_TEST_FSMONITOR support
      read-cache: update TEST_GIT_INDEX_VERSION support
      preload-index: update GIT_FORCE_PRELOAD_TEST support
      read-cache: clean up casting and byte decoding
      eoie: add End of Index Entry (EOIE) extension
      config: add new index.threads config setting
      read-cache: load cache extensions on a worker thread
      ieot: add Index Entry Offset Table (IEOT) extension
      read-cache: load cache entries on worker threads
      reset: don't compute unstaged changes after reset when --quiet
      reset: add new reset.quiet config setting
      reset: warn when refresh_index() takes more than 2 seconds
      speed up refresh_index() by utilizing preload_index()
      add: speed up cmd_add() by utilizing read_cache_preload()
      refresh_index: remove unnecessary calls to preload_index()
Brandon Williams (1):
      config: document value 2 for protocol.version
Brendan Forster (1):
      http: add support for disabling SSL revocation checks in cURL
Carlo Marcelo Arenas Belón (8):
      unpack-trees: avoid dead store for struct progress
      multi-pack-index: avoid dead store for struct progress
      read-cache: use of memory after it is freed
      commit-slabs: move MAYBE_UNUSED out
      khash: silence -Wunused-function for delta-islands
      compat: make sure git_mmap is not expected to write
      sequencer: cleanup for gcc warning in non developer mode
      builtin/notes: remove unnecessary free
Christian Couder (3):
      pack-objects: refactor code into compute_layer_order()
      pack-objects: move tree_depth into 'struct packing_data'
      pack-objects: move 'layer' into 'struct packing_data'
Christian Hesse (2):
      subtree: add build targets 'man' and 'html'
      subtree: make install targets depend on build targets
Daniels Umanovskis (3):
      doc: move git-rev-parse from porcelain to plumbing
      doc: move git-get-tar-commit-id to plumbing
      doc: move git-cherry to plumbing
David Zych (1):
      doc: clarify gitcredentials path component matching
Denton Liu (3):
      mergetool: accept -g/--[no-]gui as arguments
      completion: support `git mergetool --[no-]gui`
      doc: document diff/merge.guitool config keys
Derrick Stolee (93):
      multi-pack-index: add design document
      multi-pack-index: add format details
      multi-pack-index: add builtin
      multi-pack-index: add 'write' verb
      midx: write header information to lockfile
      multi-pack-index: load into memory
      t5319: expand test data
      packfile: generalize pack directory list
      multi-pack-index: read packfile list
      multi-pack-index: write pack names in chunk
      midx: read pack names into array
      midx: sort and deduplicate objects from packfiles
      midx: write object ids in a chunk
      midx: write object id fanout chunk
      midx: write object offsets
      config: create core.multiPackIndex setting
      midx: read objects from multi-pack-index
      midx: use midx in abbreviation calculations
      midx: use existing midx when writing new one
      midx: use midx in approximate_object_count
      midx: prevent duplicate packfile loads
      packfile: skip loading index if in multi-pack-index
      midx: clear midx on repack
      commit-reach: move walk methods from commit.c
      commit.h: remove method declarations
      commit-reach: move ref_newer from remote.c
      commit-reach: move commit_contains from ref-filter
      upload-pack: make reachable() more generic
      upload-pack: refactor ok_to_give_up()
      upload-pack: generalize commit date cutoff
      commit-reach: move can_all_from_reach_with_flags
      test-reach: create new test tool for ref_newer
      test-reach: test in_merge_bases
      test-reach: test is_descendant_of
      test-reach: test get_merge_bases_many
      test-reach: test reduce_heads
      test-reach: test can_all_from_reach_with_flags
      test-reach: test commit_contains
      commit-reach: replace ref_newer logic
      commit-reach: make can_all_from_reach... linear
      commit-reach: use can_all_from_reach
      multi-pack-index: provide more helpful usage info
      multi-pack-index: store local property
      midx: mark bad packed objects
      midx: stop reporting garbage
      midx: fix bug that skips midx with alternates
      packfile: add all_packs list
      treewide: use get_all_packs
      midx: test a few commands that use get_all_packs
      pack-objects: consider packs in multi-pack-index
      commit-graph: update design document
      test-repository: properly init repo
      commit-graph: not compatible with replace objects
      commit-graph: not compatible with grafts
      commit-graph: not compatible with uninitialized repo
      commit-graph: close_commit_graph before shallow walk
      commit-graph: define GIT_TEST_COMMIT_GRAPH
      t3206-range-diff.sh: cover single-patch case
      t5318: use test_oid for HASH_LEN
      multi-pack-index: add 'verify' verb
      multi-pack-index: verify bad header
      multi-pack-index: verify corrupt chunk lookup table
      multi-pack-index: verify packname order
      multi-pack-index: verify missing pack
      multi-pack-index: verify oid fanout order
      multi-pack-index: verify oid lookup order
      multi-pack-index: fix 32-bit vs 64-bit size check
      multi-pack-index: verify object offsets
      multi-pack-index: report progress during 'verify'
      fsck: verify multi-pack-index
      commit-reach: properly peel tags
      commit-reach: fix memory and flag leaks
      commit-reach: cleanups in can_all_from_reach...
      commit-graph: clean up leaked memory during write
      commit-graph: reduce initial oid allocation
      midx: fix broken free() in close_midx()
      contrib: add coverage-diff script
      ci: add optional test variables
      commit-reach: fix first-parent heuristic
      midx: close multi-pack-index on repack
      multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX
      packfile: close multi-pack-index in close_all_packs
      prio-queue: add 'peek' operation
      test-reach: add run_three_modes method
      test-reach: add rev-list tests
      revision.c: begin refactoring --topo-order logic
      commit/revisions: bookkeeping before refactoring
      revision.c: generation-based topo-order algorithm
      t6012: make rev-list tests more interesting
      commit-reach: implement get_reachable_subset
      test-reach: test get_reachable_subset
      remote: make add_missing_tags() linear
      pack-objects: ignore ambiguous object warnings
Elijah Newren (14):
      Remove superfluous trailing semicolons
      t4200: demonstrate rerere segfault on specially crafted merge
      rerere: avoid buffer overrun
      update-ref: fix type of update_flags variable to match its usage
      update-ref: allow --no-deref with --stdin
      sequencer: fix --allow-empty-message behavior, make it smarter
      merge-recursive: set paths correctly when three-way merging content
      merge-recursive: avoid wrapper function when unnecessary and wasteful
      merge-recursive: remove final remaining caller of merge_file_one()
      merge-recursive: rename merge_file_1() and merge_content()
      commit: fix erroneous BUG, 'multiple renames on the same target? how?'
      merge-recursive: improve auto-merging messages with path collisions
      merge-recursive: avoid showing conflicts with merge branch before HEAD
      fsck: move fsck_head_link() to get_default_heads() to avoid some globals
Eric Sunshine (26):
      format-patch: allow additional generated content in make_cover_letter()
      format-patch: add --interdiff option to embed diff in cover letter
      format-patch: teach --interdiff to respect -v/--reroll-count
      interdiff: teach show_interdiff() to indent interdiff
      log-tree: show_log: make commentary block delimiting reusable
      format-patch: allow --interdiff to apply to a lone-patch
      range-diff: respect diff_option.file rather than assuming 'stdout'
      range-diff: publish default creation factor
      range-diff: relieve callers of low-level configuration burden
      format-patch: add --range-diff option to embed diff in cover letter
      format-patch: extend --range-diff to accept revision range
      format-patch: teach --range-diff to respect -v/--reroll-count
      format-patch: add --creation-factor tweak for --range-diff
      format-patch: allow --range-diff to apply to a lone-patch
      worktree: don't die() in library function find_worktree()
      worktree: move delete_git_dir() earlier in file for upcoming new callers
      worktree: generalize delete_git_dir() to reduce code duplication
      worktree: prepare for more checks of whether path can become worktree
      worktree: disallow adding same path multiple times
      worktree: teach 'add' to respect --force for registered but missing path
      worktree: teach 'move' to override lock when --force given twice
      worktree: teach 'remove' to override lock when --force given twice
      worktree: delete .git/worktrees if empty after 'remove'
      doc-diff: fix non-portable 'man' invocation
      doc-diff: add --clean mode to remove temporary working gunk
      doc/Makefile: drop doc-diff worktree and temporary files on "make clean"
Frederick Eaton (3):
      git-archimport.1: specify what kind of Arch we're talking about
      git-column.1: clarify initial description, provide examples
      git-describe.1: clarify that "human readable" is also git-readable
James Knight (1):
      build: link with curl-defined linker flags
Jann Horn (2):
      patch-delta: fix oob read
      patch-delta: consistently report corruption
Jeff Hostetler (2):
      t0051: test GIT_TRACE to a windows named pipe
      mingw: fix mingw_open_append to work with named pipes
Jeff King (97):
      branch: make "-l" a synonym for "--list"
      Add delta-islands.{c,h}
      pack-objects: add delta-islands support
      repack: add delta-islands support
      t5320: tests for delta islands
      t/perf: factor boilerplate out of test_perf
      t/perf: factor out percent calculations
      t/perf: add infrastructure for measuring sizes
      t/perf: add perf tests for fetches from a bitmapped server
      pack-bitmap: save "have" bitmap from walk
      pack-objects: reuse on-disk deltas for thin "have" objects
      SubmittingPatches: mention doc-diff
      rev-list: make empty --stdin not an error
      trailer: use size_t for string offsets
      trailer: use size_t for iterating trailer list
      trailer: pass process_trailer_opts to trailer_info_get()
      interpret-trailers: tighten check for "---" patch boundary
      interpret-trailers: allow suppressing "---" divider
      pretty, ref-filter: format %(trailers) with no_divider option
      sequencer: ignore "---" divider when parsing trailers
      append_signoff: use size_t for string offsets
      coccinelle: use <...> for function exclusion
      introduce hasheq() and oideq()
      convert "oidcmp() == 0" to oideq()
      convert "hashcmp() == 0" to hasheq()
      convert "oidcmp() != 0" to "!oideq()"
      convert "hashcmp() != 0" to "!hasheq()"
      convert hashmap comparison functions to oideq()
      read-cache: use oideq() in ce_compare functions
      show_dirstat: simplify same-content check
      doc-diff: always use oids inside worktree
      test-delta: read input into a heap buffer
      t5303: test some corrupt deltas
      patch-delta: handle truncated copy parameters
      t5303: use printf to generate delta bases
      doc/git-branch: remove obsolete "-l" references
      bitmap_has_sha1_in_uninteresting(): drop BUG check
      t5310: test delta reuse with bitmaps
      traverse_bitmap_commit_list(): don't free result
      pack-bitmap: drop "loaded" flag
      reopen_tempfile(): truncate opened file
      doc-diff: force worktree add
      config.mak.dev: add -Wformat-security
      pack-objects: handle island check for "external" delta base
      receive-pack: update comment with check_everything_connected
      submodule--helper: use "--" to signal end of clone options
      submodule-config: ban submodule urls that start with dash
      submodule-config: ban submodule paths that start with a dash
      fsck: detect submodule urls starting with dash
      fsck: detect submodule paths starting with dash
      more oideq/hasheq conversions
      transport: drop refnames from for_each_alternate_ref
      test-tool: show tool list on error
      config.mak.dev: enable -Wunused-function
      run-command: mark path lookup errors with ENOENT
      t5410: use longer path for sample script
      upload-pack: fix broken if/else chain in config callback
      t1450: check large blob in trailing-garbage test
      check_stream_sha1(): handle input underflow
      cat-file: handle streaming failures consistently
      ls-remote: do not send ref prefixes for patterns
      ls-remote: pass heads/tags prefixes to transport
      read_istream_pack_non_delta(): document input handling
      xdiff: provide a separate emit callback for hunks
      xdiff-interface: provide a separate consume callback for hunks
      rev-list: handle flags for --indexed-objects
      approxidate: handle pending number for "specials"
      pathspec: handle non-terminated strings with :(attr)
      diff: avoid generating unused hunk header lines
      diff: discard hunk headers for patch-ids earlier
      diff: use hunk callback for word-diff
      combine-diff: use an xdiff hunk callback
      diff: convert --check to use a hunk callback
      range-diff: use a hunk callback
      xdiff-interface: drop parse_hunk_header()
      apply: mark include/exclude options as NONEG
      am: handle --no-patch-format option
      ls-files: mark exclude options as NONEG
      pack-objects: mark index-version option as NONEG
      cat-file: mark batch options with NONEG
      status: mark --find-renames option with NONEG
      format-patch: mark "--no-numbered" option with NONEG
      show-branch: mark --reflog option as NONEG
      tag: mark "--message" option with NONEG
      cat-file: report an error on multiple --batch options
      apply: return -1 from option callback instead of calling exit(1)
      parse-options: drop OPT_DATE()
      assert NOARG/NONEG behavior of parse-options callbacks
      midx: double-check large object write loop
      merge: extract verify_merge_signature() helper
      merge: handle --verify-signatures for unborn branch
      pull: handle --verify-signatures for unborn branch
      approxidate: fix NULL dereference in date_time()
      bundle: dup() output descriptor closer to point-of-use
      pack-objects: fix tree_depth and layer invariants
      pack-objects: zero-initialize tree_depth/layer arrays
      pack-objects: fix off-by-one in delta-island tree-depth computation
Johannes Schindelin (62):
      rebase -i --autosquash: demonstrate a problem skipping the last squash
      rebase -i: be careful to wrap up fixup/squash chains
      compat/poll: prepare for targeting Windows Vista
      mingw: set _WIN32_WINNT explicitly for Git for Windows
      mingw: bump the minimum Windows version to Vista
      builtin rebase: prepare for builtin rebase -i
      rebase -i: clarify what happens on a failed `exec`
      rebase -i: introduce the 'break' command
      getpwuid(mingw): initialize the structure only once
      getpwuid(mingw): provide a better default for the user name
      mingw: use domain information for default email
      http: add support for selecting SSL backends at runtime
      pack-objects: fix typo 'detla' -> 'delta'
      pack-objects (mingw): demonstrate a segmentation fault with large deltas
      pack-objects (mingw): initialize `packing_data` mutex in the correct spot
      rebase (autostash): avoid duplicate call to state_dir_path()
      rebase (autostash): store the full OID in <state-dir>/autostash
      rebase (autostash): use an explicit OID to apply the stash
      mingw: factor out code to set stat() data
      rebase --autostash: demonstrate a problem with dirty submodules
      rebase --autostash: fix issue with dirty submodules
      mingw: load system libraries the recommended way
      mingw: ensure `getcwd()` reports the correct case
      repack: point out a bug handling stale shallow info
      shallow: offer to prune only non-existing entries
      repack -ad: prune the list of shallow commits
      http: when using Secure Channel, ignore sslCAInfo by default
      t7800: fix quoting
      mingw: reencode environment variables on the fly (UTF-16 <-> UTF-8)
      config: rename `dummy` parameter to `cb` in git_default_config()
      config: allow for platform-specific core.* config settings
      config: move Windows-specific config settings into compat/mingw.c
      mingw: unset PERL5LIB by default
      mingw: fix isatty() after dup2()
      t3404: decouple some test cases from outcomes of previous test cases
      t3418: decouple test cases from a previous `rebase -p` test case
      tests: optionally skip `git rebase -p` tests
      Windows: force-recompile git.res for differing architectures
      built-in rebase: demonstrate regression with --autostash
      built-in rebase --autostash: leave the current branch alone if possible
      Update .mailmap
      rebase -r: demonstrate bug with conflicting merges
      rebase -r: do not write MERGE_HEAD unless needed
      rebase -i: include MERGE_HEAD into files to clean up
      built-in rebase --skip/--abort: clean up stale .git/<name> files
      status: rebase and merge can be in progress at the same time
      apply --recount: allow "no-op hunks"
      rebase: consolidate clean-up code before leaving reset_head()
      rebase: prepare reset_head() for more flags
      built-in rebase: reinstate `checkout -q` behavior where appropriate
      tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
      tests: respect GIT_TEST_INSTALLED when initializing repositories
      t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
      mingw: use `CreateHardLink()` directly
      rebase: really just passthru the `git am` options
      rebase: validate -C<n> and --whitespace=<mode> parameters early
      config: report a bug if git_dir exists without commondir
      tests: do not require Git to be built when testing an installed Git
      tests: explicitly use `git.exe` on Windows
      mingw: replace an obsolete link with the superseding one
      legacy-rebase: backport -C<n> and --whitespace=<option> checks
      rebase: warn about the correct tree's OID
Johannes Sixt (3):
      diff: don't attempt to strip prefix from absolute Windows paths
      rebase -i: recognize short commands without arguments
      t3404-rebase-interactive: test abbreviated commands
Jonathan Nieder (9):
      gc: improve handling of errors reading gc.log
      gc: exit with status 128 on failure
      gc: do not return error for prior errors in daemonized mode
      commit-reach: correct accidental #include of C file
      mailmap: consistently normalize brian m. carlson's name
      git doc: direct bug reporters to mailing list archive
      eoie: default to not writing EOIE section
      ieot: default to not writing IEOT section
      index: make index.threads=true enable ieot and eoie
Jonathan Tan (15):
      fetch-object: unify fetch_object[s] functions
      fetch-object: set exact_oid when fetching
      connected: document connectivity in partial clones
      fetch: in partial clone, check presence of targets
      fetch-pack: avoid object flags if no_dependents
      fetch-pack: exclude blobs when lazy-fetching trees
      transport: allow skipping of ref listing
      transport: do not list refs if possible
      transport: list refs before fetch if necessary
      fetch: do not list refs if fetching only hashes
      cache-tree: skip some blob checks in partial clone
      upload-pack: make have_obj not global
      upload-pack: make want_obj not global
      upload-pack: clear flags before each v2 request
      fetch-pack: be more precise in parsing v2 response
Josh Steadmon (4):
      fuzz: add basic fuzz testing target.
      fuzz: add fuzz testing for packfile indices.
      archive: initialize archivers earlier
      Makefile: use FUZZ_CXXFLAGS for linking fuzzers
Joshua Watt (1):
      send-email: explicitly disable authentication
Junio C Hamano (34):
      Revert "doc/Makefile: drop doc-diff worktree and temporary files on "make clean""
      Initial batch post 2.19
      Second batch post 2.19
      Git 2.14.5
      Git 2.15.3
      Git 2.16.5
      Git 2.17.2
      Git 2.18.1
      Git 2.19.1
      t0000: do not get self-test disrupted by environment warnings
      CodingGuidelines: document the API in *.h files
      Declare that the next one will be named 2.20
      Third batch for 2.20
      rebase: fix typoes in error messages
      Fourth batch for 2.20
      Revert "subtree: make install targets depend on build targets"
      Fifth batch for 2.20
      receive: denyCurrentBranch=updateinstead should not blindly update
      cocci: simplify "if (++u > 1)" to "if (u++)"
      fsck: s/++i > 1/i++/
      http: give curl version warnings consistently
      Sixth batch for 2.20
      Seventh batch for 2.20
      fetch: replace string-list used as a look-up table with a hashmap
      rebase: apply cocci patch
      Eighth batch for 2.20
      Ninth batch for 2.20
      Makefile: ease dynamic-gettext-poison transition
      Tenth batch for 2.20
      Git 2.20-rc0
      RelNotes: name the release properly
      Prepare for 2.20-rc1
      Git 2.19.2
      Git 2.20-rc1
Karsten Blees (2):
      mingw: replace MSVCRT's fstat() with a Win32-based implementation
      mingw: implement nanosecond-precision file times
Loo Rong Jie (1):
      win32: replace pthread_cond_*() with much simpler code
Lucas De Marchi (1):
      range-diff: allow to diff files regardless of submodule config
Luke Diamand (3):
      git-p4: do not fail in verbose mode for missing 'fileSize' key
      git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved
      git-p4: fully support unshelving changelists
Martin Ågren (9):
      Doc: use `--type=bool` instead of `--bool`
      git-config.txt: fix 'see: above' note
      git-commit-graph.txt: fix bullet lists
      git-commit-graph.txt: typeset more in monospace
      git-commit-graph.txt: refer to "*commit*-graph file"
      Doc: refer to the "commit-graph file" with dash
      t1400: drop debug `echo` to actually execute `test`
      builtin/commit-graph.c: UNLEAK variables
      sequencer: break out of loop explicitly
Matthew DeVore (19):
      list-objects: store common func args in struct
      list-objects: refactor to process_tree_contents
      list-objects: always parse trees gently
      t/README: reformat Do, Don't, Keep in mind lists
      Documentation: add shell guidelines
      tests: standardize pipe placement
      t/*: fix ordering of expected/observed arguments
      tests: don't swallow Git errors upstream of pipes
      t9109: don't swallow Git errors upstream of pipes
      tests: order arguments to git-rev-list properly
      rev-list: handle missing tree objects properly
      revision: mark non-user-given objects instead
      list-objects-filter: use BUG rather than die
      list-objects-filter-options: do not over-strbuf_init
      list-objects-filter: implement filter tree:0
      filter-trees: code clean-up of tests
      list-objects: support for skipping tree traversal
      Documentation/git-log.txt: do not show --exclude-promisor-objects
      exclude-promisor-objects: declare when option is allowed
Max Kirillov (1):
      http-backend test: make empty CONTENT_LENGTH test more realistic
Michael Witten (3):
      docs: typo: s/go/to/
      docs: graph: remove unnecessary `graph_update()' call
      docs: typo: s/isimilar/similar/
Michał Górny (6):
      gpg-interface.c: detect and reject multiple signatures on commits
      gpg-interface.c: use flags to determine key/signer info presence
      gpg-interface.c: support getting key fingerprint via %GF format
      gpg-interface.c: obtain primary key fingerprint as well
      t/t7510-signed-commit.sh: Add %GP to custom format checks
      t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key
Mihir Mehta (1):
      doc: fix a typo and clarify a sentence
Nguyễn Thái Ngọc Duy (168):
      clone: report duplicate entries on case-insensitive filesystems
      trace.h: support nested performance tracing
      unpack-trees: add performance tracing
      unpack-trees: optimize walking same trees with cache-tree
      unpack-trees: reduce malloc in cache-tree walk
      unpack-trees: reuse (still valid) cache-tree from src_index
      unpack-trees: add missing cache invalidation
      cache-tree: verify valid cache-tree in the test suite
      Document update for nd/unpack-trees-with-cache-tree
      bisect.c: make show_list() build again
      t/helper: keep test-tool command list sorted
      t/helper: merge test-dump-untracked-cache into test-tool
      t/helper: merge test-pkt-line into test-tool
      t/helper: merge test-parse-options into test-tool
      t/helper: merge test-dump-fsmonitor into test-tool
      Makefile: add a hint about TEST_BUILTINS_OBJS
      config.txt: follow camelCase naming
      config.txt: move fetch part out to a separate file
      config.txt: move format part out to a separate file
      config.txt: move gitcvs part out to a separate file
      config.txt: move gui part out to a separate file
      config.txt: move pull part out to a separate file
      config.txt: move push part out to a separate file
      config.txt: move receive part out to a separate file
      config.txt: move sendemail part out to a separate file
      config.txt: move sequence.editor out of "core" part
      config.txt: move submodule part out to a separate file
      archive.c: remove implicit dependency the_repository
      status: show progress bar if refreshing the index takes too long
      add: do not accept pathspec magic 'attr'
      completion: support "git fetch --multiple"
      read-cache.c: remove 'const' from index_has_changes()
      diff.c: reduce implicit dependency on the_index
      combine-diff.c: remove implicit dependency on the_index
      blame.c: rename "repo" argument to "r"
      diff.c: remove the_index dependency in textconv() functions
      grep.c: remove implicit dependency on the_index
      diff.c: remove implicit dependency on the_index
      read-cache.c: remove implicit dependency on the_index
      diff-lib.c: remove implicit dependency on the_index
      ll-merge.c: remove implicit dependency on the_index
      merge-blobs.c: remove implicit dependency on the_index
      merge.c: remove implicit dependency on the_index
      patch-ids.c: remove implicit dependency on the_index
      sha1-file.c: remove implicit dependency on the_index
      rerere.c: remove implicit dependency on the_index
      userdiff.c: remove implicit dependency on the_index
      line-range.c: remove implicit dependency on the_index
      submodule.c: remove implicit dependency on the_index
      tree-diff.c: remove implicit dependency on the_index
      ws.c: remove implicit dependency on the_index
      revision.c: remove implicit dependency on the_index
      revision.c: reduce implicit dependency the_repository
      read-cache.c: optimize reading index format v4
      config.txt: correct the note about uploadpack.packObjectsHook
      help -a: improve and make --verbose default
      refs.c: indent with tabs, not spaces
      Add a place for (not) sharing stuff between worktrees
      submodule.c: remove some of the_repository references
      completion: fix __gitcomp_builtin no longer consider extra options
      t1300: extract and use test_cmp_config()
      worktree: add per-worktree config files
      refs: new ref types to make per-worktree refs visible to all worktrees
      revision.c: correct a parameter name
      revision.c: better error reporting on ref from different worktrees
      fsck: check HEAD and reflog from other worktrees
      reflog expire: cover reflog from all worktrees
      Update makefile in preparation for Documentation/config/*.txt
      config.txt: move advice.* to a separate file
      config.txt: move core.* to a separate file
      config.txt: move add.* to a separate file
      config.txt: move alias.* to a separate file
      config.txt: move am.* to a separate file
      config.txt: move apply.* to a separate file
      config.txt: move blame.* to a separate file
      config.txt: move branch.* to a separate file
      config.txt: move browser.* to a separate file
      config.txt: move checkout.* to a separate file
      config.txt: move clean.* to a separate file
      config.txt: move color.* to a separate file
      config.txt: move column.* to a separate file
      config.txt: move commit.* to a separate file
      config.txt: move credential.* to a separate file
      config.txt: move completion.* to a separate file
      config.txt: move diff-config.txt to config/
      config.txt: move difftool.* to a separate file
      config.txt: move fastimport.* to a separate file
      config.txt: move fetch-config.txt to config/
      config.txt: move filter.* to a separate file
      config.txt: move format-config.txt to config/
      config.txt: move fmt-merge-msg-config.txt to config/
      config.txt: move fsck.* to a separate file
      config.txt: move gc.* to a separate file
      config.txt: move gitcvs-config.txt to config/
      config.txt: move gitweb.* to a separate file
      config.txt: move grep.* to a separate file
      config.txt: move gpg.* to a separate file
      config.txt: move gui-config.txt to config/
      config.txt: move guitool.* to a separate file
      config.txt: move help.* to a separate file
      config.txt: move ssh.* to a separate file
      config.txt: move http.* to a separate file
      config.txt: move i18n.* to a separate file
      git-imap-send.txt: move imap.* to a separate file
      config.txt: move index.* to a separate file
      config.txt: move init.* to a separate file
      config.txt: move instaweb.* to a separate file
      config.txt: move interactive.* to a separate file
      config.txt: move log.* to a separate file
      config.txt: move mailinfo.* to a separate file
      config.txt: move mailmap.* to a separate file
      config.txt: move man.* to a separate file
      config.txt: move merge-config.txt to config/
      config.txt: move mergetool.* to a separate file
      config.txt: move notes.* to a separate file
      config.txt: move pack.* to a separate file
      config.txt: move pager.* to a separate file
      config.txt: move pretty.* to a separate file
      config.txt: move protocol.* to a separate file
      config.txt: move pull-config.txt to config/
      config.txt: move push-config.txt to config/
      config.txt: move rebase-config.txt to config/
      config.txt: move receive-config.txt to config/
      config.txt: move remote.* to a separate file
      config.txt: move remotes.* to a separate file
      config.txt: move repack.* to a separate file
      config.txt: move rerere.* to a separate file
      config.txt: move reset.* to a separate file
      config.txt: move sendemail-config.txt to config/
      config.txt: move sequencer.* to a separate file
      config.txt: move showBranch.* to a separate file
      config.txt: move splitIndex.* to a separate file
      config.txt: move status.* to a separate file
      config.txt: move stash.* to a separate file
      config.txt: move submodule.* to a separate file
      config.txt: move tag.* to a separate file
      config.txt: move transfer.* to a separate file
      config.txt: move uploadarchive.* to a separate file
      config.txt: move uploadpack.* to a separate file
      config.txt: move url.* to a separate file
      config.txt: move user.* to a separate file
      config.txt: move versionsort.* to a separate file
      config.txt: move web.* to a separate file
      config.txt: move worktree.* to a separate file
      config.txt: remove config/dummy.txt
      thread-utils: macros to unconditionally compile pthreads API
      wildmatch: change behavior of "foo**bar" in WM_PATHNAME mode
      git-worktree.txt: correct linkgit command name
      sequencer.c: remove a stray semicolon
      tree-walk.c: fix overoptimistic inclusion in :(exclude) matching
      run-command.h: include thread-utils.h instead of pthread.h
      send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c
      index-pack: remove #ifdef NO_PTHREADS
      name-hash.c: remove #ifdef NO_PTHREADS
      attr.c: remove #ifdef NO_PTHREADS
      grep: remove #ifdef NO_PTHREADS
      grep: clean up num_threads handling
      preload-index.c: remove #ifdef NO_PTHREADS
      pack-objects: remove #ifdef NO_PTHREADS
      read-cache.c: remove #ifdef NO_PTHREADS
      read-cache.c: reduce branching based on HAVE_THREADS
      read-cache.c: initialize copy_len to shut up gcc 8
      Clean up pthread_create() error handling
      completion: use __gitcomp_builtin for format-patch
      build: fix broken command-list.h generation with core.autocrlf
      format-patch: respect --stat in cover letter's diffstat
      doc: move extensions.worktreeConfig to the right place
      clone: fix colliding file detection on APFS
Nickolai Belakovski (2):
      worktree: update documentation for lock_reason and lock_reason_valid
      worktree: rename is_worktree_locked to worktree_lock_reason
Noam Postavsky (1):
      log: fix coloring of certain octopus merge shapes
Olga Telezhnaya (3):
      ref-filter: free memory from used_atom
      ls-remote: release memory instead of UNLEAK
      ref-filter: free item->value and item->value->s
Phillip Wood (11):
      diff: fix --color-moved-ws=allow-indentation-change
      diff --color-moved-ws: fix double free crash
      diff --color-moved-ws: fix out of bounds string access
      diff --color-moved-ws: fix a memory leak
      diff --color-moved-ws: fix another memory leak
      diff --color-moved: fix a memory leak
      am: don't die in read_author_script()
      am: improve author-script error reporting
      am: rename read_author_script()
      add read_author_script() to libgit
      sequencer: use read_author_script()
Pratik Karki (46):
      rebase: start implementing it as a builtin
      rebase: refactor common shell functions into their own file
      builtin/rebase: support running "git rebase <upstream>"
      builtin rebase: support --onto
      builtin rebase: support `git rebase --onto A...B`
      builtin rebase: handle the pre-rebase hook and --no-verify
      builtin rebase: support --quiet
      builtin rebase: support the `verbose` and `diffstat` options
      builtin rebase: require a clean worktree
      builtin rebase: try to fast forward when possible
      builtin rebase: support --force-rebase
      builtin rebase: start a new rebase only if none is in progress
      builtin rebase: only store fully-qualified refs in `options.head_name`
      builtin rebase: support `git rebase <upstream> <switch-to>`
      builtin rebase: support --continue
      builtin rebase: support --skip
      builtin rebase: support --abort
      builtin rebase: support --quit
      builtin rebase: support --edit-todo and --show-current-patch
      builtin rebase: actions require a rebase in progress
      builtin rebase: stop if `git am` is in progress
      builtin rebase: allow selecting the rebase "backend"
      builtin rebase: support --signoff
      builtin rebase: support --rerere-autoupdate
      builtin rebase: support --committer-date-is-author-date
      builtin rebase: support `ignore-whitespace` option
      builtin rebase: support `ignore-date` option
      builtin rebase: support `keep-empty` option
      builtin rebase: support `--autosquash`
      builtin rebase: support `--gpg-sign` option
      builtin rebase: support `-C` and `--whitespace=<type>`
      builtin rebase: support `--autostash` option
      builtin rebase: support `--exec`
      builtin rebase: support `--allow-empty-message` option
      builtin rebase: support --rebase-merges[=[no-]rebase-cousins]
      merge-base --fork-point: extract libified function
      builtin rebase: support `fork-point` option
      builtin rebase: add support for custom merge strategies
      builtin rebase: support --root
      builtin rebase: optionally auto-detect the upstream
      builtin rebase: optionally pass custom reflogs to reset_head()
      builtin rebase: fast-forward to onto if it is a proper descendant
      builtin rebase: show progress when connected to a terminal
      builtin rebase: use no-op editor when interactive is "implied"
      builtin rebase: error out on incompatible option/mode combinations
      rebase: default to using the builtin rebase
Rafael Ascensão (2):
      refs: show --exclude failure with --branches/tags/remotes=glob
      refs: fix some exclude patterns being ignored
Ralf Thielow (1):
      git-rebase.sh: fix typos in error messages
Ramsay Jones (12):
      Makefile: add a hdr-check target
      json-writer.h: add missing include (hdr-check)
      ewah/ewok_rlw.h: add missing include (hdr-check)
      refs/ref-cache.h: add missing declarations (hdr-check)
      refs/packed-backend.h: add missing declaration (hdr-check)
      refs/refs-internal.h: add missing declarations (hdr-check)
      midx.h: add missing forward declarations (hdr-check)
      delta-islands.h: add missing forward declarations (hdr-check)
      headers: normalize the spelling of some header guards
      fetch-object.h: add missing declaration (hdr-check)
      ewok_rlw.h: add missing 'inline' to function definition
      commit-reach.h: add missing declarations (hdr-check)
Rasmus Villemoes (6):
      help: redirect to aliased commands for "git cmd --help"
      git.c: handle_alias: prepend alias info when first argument is -h
      git-help.txt: document "git help cmd" vs "git cmd --help" for aliases
      Documentation/git-send-email.txt: style fixes
      send-email: only consider lines containing @ or <> for automatic Cc'ing
      send-email: also pick up cc addresses from -by trailers
René Scharfe (12):
      mailinfo: support format=flowed
      fsck: add a performance test for skipList
      fsck: use strbuf_getline() to read skiplist file
      fsck: use oidset instead of oid_array for skipList
      sequencer: use return value of oidset_insert()
      grep: add -r/--[no-]recursive
      fetch-pack: factor out is_unmatched_ref()
      fetch-pack: load tip_oids eagerly iff needed
      khash: factor out kh_release_*
      oidset: use khash
      oidset: uninline oidset_init()
      commit-reach: fix cast in compare_commits_by_gen()
Roger Strain (1):
      subtree: performance improvement for finding unexpected parent commits
SZEDER Gábor (17):
      t1404: increase core.packedRefsTimeout to avoid occasional test failure
      Documentation/git.txt: clarify that GIT_TRACE=/path appends
      t3701-add-interactive: tighten the check of trace output
      t1700-split-index: drop unnecessary 'grep'
      t0090: disable GIT_TEST_SPLIT_INDEX for the test checking split index
      t1700-split-index: document why FSMONITOR is disabled in this test script
      split-index: add tests to demonstrate the racy split index problem
      t1700-split-index: date back files to avoid racy situations
      split-index: count the number of deleted entries
      split-index: don't compare cached data of entries already marked for split index
      split-index: smudge and add racily clean cache entries to split index
      split-index: BUG() when cache entry refers to non-existing shared entry
      object_id.cocci: match only expressions of type 'struct object_id'
      test-lib: introduce the '-V' short option for '--verbose-log'
      travis-ci: install packages in 'ci/install-dependencies.sh'
      coccicheck: introduce 'pending' semantic patches
      ref-filter: don't look for objects when outside of a repository
Sam McKelvie (1):
      rev-parse: --show-superproject-working-tree should work during a merge
Saulius Gurklys (1):
      doc: fix small typo in git show-branch
Sebastian Staudt (1):
      travis-ci: no longer use containers
Shulhan (1):
      builtin/remote: quote remote name on error to display empty name
Stefan Beller (25):
      git-submodule.sh: align error reporting for update mode to use path
      git-submodule.sh: rename unused variables
      builtin/submodule--helper: factor out submodule updating
      builtin/submodule--helper: store update_clone information in a struct
      builtin/submodule--helper: factor out method to update a single submodule
      submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree
      submodule--helper: introduce new update-module-mode helper
      test_decode_color: understand FAINT and ITALIC
      t3206: add color test for range-diff --dual-color
      diff.c: simplify caller of emit_line_0
      diff.c: reorder arguments for emit_line_ws_markup
      diff.c: add set_sign to emit_line_0
      diff: use emit_line_0 once per line
      diff.c: omit check for line prefix in emit_line_0
      diff.c: rewrite emit_line_0 more understandably
      diff.c: add --output-indicator-{new, old, context}
      range-diff: make use of different output indicators
      range-diff: indent special lines as context
      refs.c: migrate internal ref iteration to pass thru repository argument
      refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
      string-list: remove unused function print_string_list
      strbuf.h: format according to coding guidelines
      diff.c: pass sign_index to emit_line_ws_markup
      submodule helper: convert relative URL to absolute URL if needed
      builtin/submodule--helper: remove debugging leftover tracing
Stephen P. Smith (10):
      wt-status.c: move has_unmerged earlier in the file
      wt-status: rename commitable to committable
      t7501: add test of "commit --dry-run --short"
      wt-status.c: set the committable flag in the collect phase
      roll wt_status_state into wt_status and populate in the collect phase
      t2000: rename and combine checkout clash tests
      t7509: cleanup description and filename
      t7502: rename commit test script to comply with naming convention
      t7500: rename commit tests script to comply with naming convention
      t7501: rename commit test to comply with naming convention
Steve Hoelzer (1):
      poll: use GetTickCount64() to avoid wrap-around issues
Steven Fernandez (1):
      git-completion.bash: add completion for stash list
Strain, Roger L (4):
      subtree: refactor split of a commit into standalone method
      subtree: make --ignore-joins pay attention to adds
      subtree: use commits before rejoins for splits
      subtree: improve decision on merges kept in split
Tao Qingyun (3):
      refs: docstring typo
      builtin/branch.c: remove useless branch_get
      branch: trivial style fix
Taylor Blau (4):
      transport.c: extract 'fill_alternate_refs_command'
      transport.c: introduce core.alternateRefsCommand
      transport.c: introduce core.alternateRefsPrefixes
      Documentation/config.txt: fix typo in core.alternateRefsCommand
Thomas Gummerer (17):
      rerere: unify error messages when read_cache fails
      rerere: lowercase error messages
      rerere: wrap paths in output in sq
      rerere: mark strings for translation
      rerere: add documentation for conflict normalization
      rerere: fix crash with files rerere can't handle
      rerere: only return whether a path has conflicts or not
      rerere: factor out handle_conflict function
      rerere: return strbuf from handle path
      rerere: teach rerere to handle nested conflicts
      rerere: recalculate conflict ID when unresolved conflict is committed
      rerere: mention caveat about unmatched conflict markers
      rerere: add note about files with existing conflict markers
      .gitattributes: add conflict-marker-size for relevant files
      linear-assignment: fix potential out of bounds memory access
      t5551: move setup code inside test_expect blocks
      t5551: compare sorted cookies files
Tim Schumacher (4):
      Documentation/Makefile: make manpage-base-url.xsl generation quieter
      alias: add support for aliases of an alias
      alias: show the call history when an alias is looping
      t0014: introduce an alias testing suite
Todd Zullinger (1):
      Documentation: build technical/multi-pack-index
Torsten Bögershausen (4):
      Make git_check_attr() a void function
      path.c: char is not (always) signed
      Upcast size_t variables to uintmax_t when printing
      remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)
Uwe Kleine-König (1):
      howto/using-merge-subtree: mention --allow-unrelated-histories
brian m. carlson (26):
      t: add test functions to translate hash-related values
      t0000: use hash translation table
      t0000: update tests for SHA-256
      t0002: abstract away SHA-1 specific constants
      t0064: make hash size independent
      t1006: make hash size independent
      t1400: switch hard-coded object ID to variable
      t1405: make hash size independent
      t1406: make hash-size independent
      t1407: make hash size independent
      editorconfig: provide editor settings for Git developers
      editorconfig: indicate settings should be kept in sync
      pack-bitmap-write: use GIT_MAX_RAWSZ for allocation
      builtin/repack: replace hard-coded constants
      builtin/mktree: remove hard-coded constant
      builtin/fetch-pack: remove constants with parse_oid_hex
      pack-revindex: express constants in terms of the_hash_algo
      packfile: express constants in terms of the_hash_algo
      refs/packed-backend: express constants using the_hash_algo
      upload-pack: express constants in terms of the_hash_algo
      transport: use parse_oid_hex instead of a constant
      tag: express constant in terms of the_hash_algo
      apply: replace hard-coded constants
      apply: rename new_sha1_prefix and old_sha1_prefix
      submodule: make zero-oid comparison hash function agnostic
      rerere: convert to use the_hash_algo
Ævar Arnfjörð Bjarmason (33):
      fetch: change "branch" to "reference" in --force -h output
      push tests: make use of unused $1 in test description
      push tests: use spaces in interpolated string
      fetch tests: add a test for clobbering tag behavior
      push doc: remove confusing mention of remote merger
      push doc: move mention of "tag <tag>" later in the prose
      push doc: correct lies about how push refspecs work
      fetch: document local ref updates with/without --force
      fetch: stop clobbering existing tags without --force
      fsck tests: setup of bogus commit object
      fsck tests: add a test for no skipList input
      fsck: document and test sorted skipList input
      fsck: document and test commented & empty line skipList input
      fsck: document that skipList input must be unabbreviated
      fsck: add a performance test
      fsck: support comments & empty lines in skipList
      commit-graph write: add progress output
      commit-graph verify: add progress output
      config doc: add missing list separator for checkout.optimizeNewBranch
      push doc: add spacing between two words
      fetch doc: correct grammar in --force docs
      gc: fix regression in 7b0f229222 impacting --quiet
      gc doc: mention the commit-graph in the intro
      pack-objects test: modernize style
      pack-objects tests: don't leave test .git corrupt at end
      index-pack tests: don't leave test repo dirty at end
      i18n: make GETTEXT_POISON a runtime option
      range-diff doc: add a section about output stability
      range-diff: fix regression in passing along diff options
      range-diff: make diff option behavior (e.g. --stat) consistent
      rebase doc: document rebase.useBuiltin
      tests: add a special setup where rebase.useBuiltin is off
      read-cache: make the split index obey umask settings
Đoàn Trần Công Danh (1):
      git-compat-util: prefer poll.h to sys/poll.h
Ævar Arnfjörð Bjarmason· Nov 22, 2018, 15:58 UTC · re: Junio C Hamano · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

On Wed, Nov 21 2018, Junio C Hamano wrote:
>  * The "--no-patch" option, which can be used to get a high-level
>    overview without the actual line-by-line patch difference shown, of
>    the "range-diff" command was earlier broken, which has been
>    corrected.

There's a regression related to this that I wanted to send a headsup for, but don't have time to fix today. Now range-diff in format-patch includes --stat output. See e.g. my https://public-inbox.org/git/20181122132823.9883-1-avarab@gmail.com/

Preliminary patch:
 builtin/log.c         | 3 +++
 t/t3206-range-diff.sh | 2 ++
 2 files changed, 5 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index 0fe6f9ba1e..fdaba480d2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}

 	if (rev->rdiff1) {
+		const int oldfmt = rev->diffopt.output_format;
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
+		rev->diffopt.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
 				rev->creation_factor, 1, &rev->diffopt);
+		rev->diffopt.output_format = oldfmt;
 	}
 }

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..2e913542f3 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -248,8 +248,10 @@ test_expect_success 'dual-coloring' '
 for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
+		cat actual &&
 		git format-patch --stdout --cover-letter --range-diff=$prev \
 			master..unmodified >actual &&
+		! grep "a => b" actual &&
 		grep "= 1: .* s/5/A" actual &&
 		grep "= 2: .* s/4/A" actual &&
 		grep "= 3: .* s/11/B" actual &&
Eric Sunshine· Nov 22, 2018, 19:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

On Thu, Nov 22, 2018 at 10:58 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> There's a regression related to this that I wanted to send a headsup
> for, but don't have time to fix today. Now range-diff in format-patch
> includes --stat output. See e.g. my
> https://public-inbox.org/git/20181122132823.9883-1-avarab@gmail.com/
Umf. Unfortunate fallout from [1].
Show 11 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1094,9 +1094,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>         if (rev->rdiff1) {
> +               const int oldfmt = rev->diffopt.output_format;
>                 fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> +               rev->diffopt.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>                 show_range_diff(rev->rdiff1, rev->rdiff2,
>                                 rev->creation_factor, 1, &rev->diffopt);
> +               rev->diffopt.output_format = oldfmt;
>         }
>  }
A few questions/observations:

Does this same "fix" need to be applied to the --interdiff case just above this --range-diff block?

Does the same "fix" need to be applied to the --interdiff and --range-diff cases in log-tree.c:show_log()?

Aside from fixing the broken --no-patches option[2], a goal of the series was to some day make --stat do something useful. Doesn't this "fix" go against that goal?

The way this change needs to be spread around at various locations is making it feel like a BandAid "fix" rather than a proper solution. It seems like it should be fixed at a different level, though I'm not sure yet if that level is higher (where the options get set) or lower (where they actually affect the operation).

Until we figure out the answers to these questions, I wonder if a more sensible short-term solution would be to back out [1] and just keep [2], which fixed the --no-patches regression.

Show 6 quoted lines
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> @@ -248,8 +248,10 @@ test_expect_success 'dual-coloring' '
>  for prev in topic master..topic
>  do
>         test_expect_success "format-patch --range-diff=$prev" '
> +               cat actual &&
Debugging session gunk?
>                 git format-patch --stdout --cover-letter --range-diff=$prev \
>                         master..unmodified >actual &&
> +               ! grep "a => b" actual &&
>                 grep "= 1: .* s/5/A" actual &&

[1]: https://public-inbox.org/git/20181113185558.23438-4-avarab@gmail.com/ [2]: https://public-inbox.org/git/20181113185558.23438-3-avarab@gmail.com/

Ævar Arnfjörð Bjarmason· Nov 22, 2018, 21:12 UTC · re: Eric Sunshine · lore

[PATCH 0/2] format-patch: pre-2.20 range-diff regression fix

[Dropping LKML & git-packagers from CC]
On Thu, Nov 22 2018, Eric Sunshine wrote:
Show 28 quoted lines
> On Thu, Nov 22, 2018 at 10:58 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> There's a regression related to this that I wanted to send a headsup
>> for, but don't have time to fix today. Now range-diff in format-patch
>> includes --stat output. See e.g. my
>> https://public-inbox.org/git/20181122132823.9883-1-avarab@gmail.com/
>
> Umf. Unfortunate fallout from [1].
>
>> diff --git a/builtin/log.c b/builtin/log.c
>> @@ -1094,9 +1094,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>>         if (rev->rdiff1) {
>> +               const int oldfmt = rev->diffopt.output_format;
>>                 fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> +               rev->diffopt.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>>                 show_range_diff(rev->rdiff1, rev->rdiff2,
>>                                 rev->creation_factor, 1, &rev->diffopt);
>> +               rev->diffopt.output_format = oldfmt;
>>         }
>>  }
>
> A few questions/observations:
>
> Does this same "fix" need to be applied to the --interdiff case just
> above this --range-diff block?
>
> Does the same "fix" need to be applied to the --interdiff and
> --range-diff cases in log-tree.c:show_log()?

No, that seems to do the right thing, but perhaps tests are lacking there too. I haven't looked.

> Aside from fixing the broken --no-patches option[2], a goal of the
> series was to some day make --stat do something useful. Doesn't this
> "fix" go against that goal?

The goal was to fix the regression introduced in 73a834e9e2 ("range-diff: relieve callers of low-level configuration burden", 2018-07-22). One aspect of having fixed that is we might in the future do stuff with --stat.

Show 9 quoted lines
> The way this change needs to be spread around at various locations is
> making it feel like a BandAid "fix" rather than a proper solution. It
> seems like it should be fixed at a different level, though I'm not
> sure yet if that level is higher (where the options get set) or lower
> (where they actually affect the operation).
>
> Until we figure out the answers to these questions, I wonder if a more
> sensible short-term solution would be to back out [1] and just keep
> [2], which fixed the --no-patches regression.

I think that patch leaves range-diff itself in a good state without any bugs, and it would be a mistake to revert it.

But this interaction with format-patch --range-diff is another matter. As noted in 2/2 I think in practice this series sweeps the current bugs under the rug.

But as also noted there I think re-using what we get from setup_revisions() in format-patch for the range-diff was a mistake, and is always going to lead to some confusion. It should be split up so we can supply those diff options independently.

> [...]
> [1]: https://public-inbox.org/git/20181113185558.23438-4-avarab@gmail.com/
> [2]: https://public-inbox.org/git/20181113185558.23438-3-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (2):
  format-patch: add a more exhaustive --range-diff test
  format-patch: don't include --stat with --range-diff output
 builtin/log.c         |  7 ++++++-
 t/t3206-range-diff.sh | 15 ++++++++++-----
 2 files changed, 16 insertions(+), 6 deletions(-)
-- 
2.20.0.rc0.387.gc7a69e6b6c
Ævar Arnfjörð Bjarmason· Nov 22, 2018, 21:12 UTC · re: Eric Sunshine · lore

[PATCH 1/2] format-patch: add a more exhaustive --range-diff test

Change the narrow test added in 31e2617a5f ("format-patch: add --range-diff option to embed diff in cover letter", 2018-07-22) to test the full output. This test would have spotted a regression in the output if it wasn't beating around the bush and tested the full output, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3206-range-diff.sh | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..0235c038be 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -249,11 +249,28 @@ for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
 		git format-patch --stdout --cover-letter --range-diff=$prev \
-			master..unmodified >actual &&
-		grep "= 1: .* s/5/A" actual &&
-		grep "= 2: .* s/4/A" actual &&
-		grep "= 3: .* s/11/B" actual &&
-		grep "= 4: .* s/12/B" actual
+			master..unmodified >actual.raw &&
+		sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
+		:1:  4de457d = 1:  35b9b25 s/5/A/
+		:     a => b | 0
+		:     1 file changed, 0 insertions(+), 0 deletions(-)
+		:    :
+		:2:  fccce22 = 2:  de345ab s/4/A/
+		:     a => b | 0
+		:     1 file changed, 0 insertions(+), 0 deletions(-)
+		:    :
+		:3:  147e64e = 3:  9af6654 s/11/B/
+		:     a => b | 0
+		:     1 file changed, 0 insertions(+), 0 deletions(-)
+		:    :
+		:4:  a63e992 = 4:  2901f77 s/12/B/
+		:     a => b | 0
+		:     1 file changed, 0 insertions(+), 0 deletions(-)
+		:    :
+		:-- :
+		EOF
+		sed -ne "/^1:/,/^--/p" <actual.raw >actual &&
+		test_cmp expect actual
 	'
 done
 
-- 
2.20.0.rc0.387.gc7a69e6b6c
Junio C Hamano· Nov 24, 2018, 04:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/2] format-patch: add a more exhaustive --range-diff test

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 5 quoted lines
> Change the narrow test added in 31e2617a5f ("format-patch: add
> --range-diff option to embed diff in cover letter", 2018-07-22) to
> test the full output. This test would have spotted a regression in the
> output if it wasn't beating around the bush and tested the full
> output, let's do that.

This looks wrong, given that we are declaring that showing the broken --stat in the output is wrong. It makes us to expect a known-wrong output from the command.

If the theme of the topic were "what we are giving by default is a sensible output when --stat is asked for, but it is rather too noisy and our default should be quieter---let's tone it down", then this patch in 1/2 and then a behaviour-change patch with updated expectation would make sense, but I do not think that is what you are aiming for with this series.

Perhaps squash this into the real "fix" to show what the desired output should look like with the same patch?

Show 42 quoted lines
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t3206-range-diff.sh | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index e497c1358f..0235c038be 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -249,11 +249,28 @@ for prev in topic master..topic
>  do
>  	test_expect_success "format-patch --range-diff=$prev" '
>  		git format-patch --stdout --cover-letter --range-diff=$prev \
> -			master..unmodified >actual &&
> -		grep "= 1: .* s/5/A" actual &&
> -		grep "= 2: .* s/4/A" actual &&
> -		grep "= 3: .* s/11/B" actual &&
> -		grep "= 4: .* s/12/B" actual
> +			master..unmodified >actual.raw &&
> +		sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
> +		:1:  4de457d = 1:  35b9b25 s/5/A/
> +		:     a => b | 0
> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
> +		:    :
> +		:2:  fccce22 = 2:  de345ab s/4/A/
> +		:     a => b | 0
> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
> +		:    :
> +		:3:  147e64e = 3:  9af6654 s/11/B/
> +		:     a => b | 0
> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
> +		:    :
> +		:4:  a63e992 = 4:  2901f77 s/12/B/
> +		:     a => b | 0
> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
> +		:    :
> +		:-- :
> +		EOF
> +		sed -ne "/^1:/,/^--/p" <actual.raw >actual &&
> +		test_cmp expect actual
>  	'
>  done
Ævar Arnfjörð Bjarmason· Nov 24, 2018, 11:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] format-patch: add a more exhaustive --range-diff test

On Sat, Nov 24 2018, Junio C Hamano wrote:
Show 21 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the narrow test added in 31e2617a5f ("format-patch: add
>> --range-diff option to embed diff in cover letter", 2018-07-22) to
>> test the full output. This test would have spotted a regression in the
>> output if it wasn't beating around the bush and tested the full
>> output, let's do that.
>
> This looks wrong, given that we are declaring that showing the
> broken --stat in the output is wrong.  It makes us to expect a
> known-wrong output from the command.
>
> If the theme of the topic were "what we are giving by default is a
> sensible output when --stat is asked for, but it is rather too noisy
> and our default should be quieter---let's tone it down", then this
> patch in 1/2 and then a behaviour-change patch with updated
> expectation would make sense, but I do not think that is what you
> are aiming for with this series.
>
> Perhaps squash this into the real "fix" to show what the desired
> output should look like with the same patch?
I see you did that in 'pu'. I don't mind, looks good to me.

FWIW I split this up for ease of review, i.e. showing what the output was before and what effect the code change had, but maybe that was overdoing it and it's simpler just to have this all in one go.

Show 42 quoted lines
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t3206-range-diff.sh | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> index e497c1358f..0235c038be 100755
>> --- a/t/t3206-range-diff.sh
>> +++ b/t/t3206-range-diff.sh
>> @@ -249,11 +249,28 @@ for prev in topic master..topic
>>  do
>>  	test_expect_success "format-patch --range-diff=$prev" '
>>  		git format-patch --stdout --cover-letter --range-diff=$prev \
>> -			master..unmodified >actual &&
>> -		grep "= 1: .* s/5/A" actual &&
>> -		grep "= 2: .* s/4/A" actual &&
>> -		grep "= 3: .* s/11/B" actual &&
>> -		grep "= 4: .* s/12/B" actual
>> +			master..unmodified >actual.raw &&
>> +		sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
>> +		:1:  4de457d = 1:  35b9b25 s/5/A/
>> +		:     a => b | 0
>> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
>> +		:    :
>> +		:2:  fccce22 = 2:  de345ab s/4/A/
>> +		:     a => b | 0
>> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
>> +		:    :
>> +		:3:  147e64e = 3:  9af6654 s/11/B/
>> +		:     a => b | 0
>> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
>> +		:    :
>> +		:4:  a63e992 = 4:  2901f77 s/12/B/
>> +		:     a => b | 0
>> +		:     1 file changed, 0 insertions(+), 0 deletions(-)
>> +		:    :
>> +		:-- :
>> +		EOF
>> +		sed -ne "/^1:/,/^--/p" <actual.raw >actual &&
>> +		test_cmp expect actual
>>  	'
>>  done
Ævar Arnfjörð Bjarmason· Nov 22, 2018, 21:12 UTC · re: Eric Sunshine · lore

[PATCH 2/2] format-patch: don't include --stat with --range-diff output

Fix a regression introduced in my a48e12ef7a ("range-diff: make diff option behavior (e.g. --stat) consistent", 2018-11-13). Since the format-patch setup code implicitly sets --stat --summary by default, we started emitting the --stat output in the cover letter's range-diff.

As noted in df569c3f31 ("range-diff doc: add a section about output stability", 2018-11-09) the --stat output is currently rather useless, and just adds noise.

Perhaps we should detect if --stat or --summary were implicitly passed to format-patch, and then pass them along, but I think fixing it this way is fine. If our --stat output ever becomes useful in range-diff we can revisit this.

There's still cases like e.g. --numstat triggering rather useless range-diff output, but I think it's OK to just handle the default case. Users are unlikely to produce a formatted patch with the likes of --numstat, or indeed any other custom diff option except -U<n> or maybe -W. If they need weirder combinations of options they can always manually produce the range-diff.

This whole situation comes about because we're assuming that when the user passes along e.g. -U10 that they want that some 10-line context for the range-diff as for the patches themselves. As noted in [1] I think it's worth re-visiting this and making -U10 just apply to the patches, and e.g. --range-diff-U10 to the range-diff. But that's left as a topic for another series less close to a rc2.

1. https://public-inbox.org/git/87d0ri7gbs.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/log.c         |  7 ++++++-
 t/t3206-range-diff.sh | 12 ------------
 2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 0fe6f9ba1e..7cd2db0be9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,13 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
+		struct diff_options opts;
+		memcpy(&opts, &rev->diffopt, sizeof(opts));
+		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
+
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &rev->diffopt);
+				rev->creation_factor, 1, &opts);
 	}
 }
 
@@ -1697,6 +1701,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (!use_patch_format &&
 		(!rev.diffopt.output_format ||
 		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
+		/* Needs to be mirrored in show_range_diff() invocation */
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
 	if (!rev.diffopt.stat_width)
 		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 0235c038be..90def330bd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -252,21 +252,9 @@ do
 			master..unmodified >actual.raw &&
 		sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
 		:1:  4de457d = 1:  35b9b25 s/5/A/
-		:     a => b | 0
-		:     1 file changed, 0 insertions(+), 0 deletions(-)
-		:    :
 		:2:  fccce22 = 2:  de345ab s/4/A/
-		:     a => b | 0
-		:     1 file changed, 0 insertions(+), 0 deletions(-)
-		:    :
 		:3:  147e64e = 3:  9af6654 s/11/B/
-		:     a => b | 0
-		:     1 file changed, 0 insertions(+), 0 deletions(-)
-		:    :
 		:4:  a63e992 = 4:  2901f77 s/12/B/
-		:     a => b | 0
-		:     1 file changed, 0 insertions(+), 0 deletions(-)
-		:    :
 		:-- :
 		EOF
 		sed -ne "/^1:/,/^--/p" <actual.raw >actual &&
-- 
2.20.0.rc0.387.gc7a69e6b6c
Junio C Hamano· Nov 24, 2018, 02:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: don't include --stat with --range-diff output

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 9 quoted lines
>  	if (rev->rdiff1) {
> +		struct diff_options opts;
> +		memcpy(&opts, &rev->diffopt, sizeof(opts));
> +		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> +
>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>  		show_range_diff(rev->rdiff1, rev->rdiff2,
> -				rev->creation_factor, 1, &rev->diffopt);
> +				rev->creation_factor, 1, &opts);

I am not quite convinced if this shallow copy is a safe thing to do. Quite honestly at this late in the release cycle, as a band-aid, I'd rather see a simpler revert than a change like this that we have to worry about what happens if/when show_range_diff() _thinks_ it is done with the opts and ends up discarding resources (e.g. "FILE *") that are shared with rev->diffopt that would still have to be used later.

Junio C Hamano· Nov 24, 2018, 04:17 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] format-patch: don't include --stat with --range-diff output

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>>  	if (rev->rdiff1) {
>> +		struct diff_options opts;
>> +		memcpy(&opts, &rev->diffopt, sizeof(opts));
>> +		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> +
>>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>>  		show_range_diff(rev->rdiff1, rev->rdiff2,
>> -				rev->creation_factor, 1, &rev->diffopt);
>> +				rev->creation_factor, 1, &opts);
>
> I am not quite convinced if this shallow copy is a safe thing to do.
> Quite honestly at this late in the release cycle, as a band-aid, I'd
> rather see a simpler revert than a change like this that we have to
> worry about what happens if/when show_range_diff() _thinks_ it is
> done with the opts and ends up discarding resources (e.g. "FILE *")
> that are shared with rev->diffopt that would still have to be used
> later.

Well, let's take it anyway as-is, at least for today, as I notice show_range_diff() itself does another shallow copy, so we are not making anything dramatically worse.

Thanks.
Ævar Arnfjörð Bjarmason· Nov 28, 2018, 20:18 UTC · re: Junio C Hamano · lore

[PATCH 2/2] format-patch: allow for independent diff & range-diff options

Change the semantics of the "--range-diff" option so that the regular diff options can be provided separately for the range-diff and the patch. This allows for supplying e.g. --range-diff-U0 and -U1 to "format-patch" to provide different context for the range-diff and the patch. This wasn't possible before.

Ever since the "--range-diff" option was added in 31e2617a5f ("format-patch: add --range-diff option to embed diff in cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff machinery has been the one we get from "format-patch"'s own setup_revisions().

This sort of thing is unique among the log-like commands in builtin/log.c, no command than format-patch will embed the output of another log-like command. Since the "rev->diffopt" is reused we need to munge it before we pass it to show_range_diff(). See 43dafc4172 ("format-patch: don't include --stat with --range-diff output", 2018-11-22) for a related regression fix which is being mostly reverted here.

Implementation notes: 1) We're not bothering with the full teardown around die() and will leak memory, but it's too much boilerplate to do all the frees with/without the die() and not worth it. 2) We call repo_init_revisions() for "rd_rev" even though we could get away with a shallow copy like the code we're replacing (and which show_range_diff() itself does). This is to make this code more easily understood.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-format-patch.txt | 10 ++++++-
 builtin/log.c                      | 42 +++++++++++++++++++++++-------
 t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index aba4c5febe..6c048f415f 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -24,7 +24,8 @@ SYNOPSIS
 		   [--to=<email>] [--cc=<email>]
 		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
 		   [--interdiff=<previous>]
-		   [--range-diff=<previous> [--creation-factor=<percent>]]
+		   [--range-diff=<previous> [--creation-factor=<percent>]
+		      [--range-diff<common diff option>]]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -257,6 +258,13 @@ feeding the result to `git send-email`.
 	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
 	for details.
 
+--range-diff<common diff option>::
+	Other options prefixed with `--range-diff` are stripped of
+	that prefix and passed as-is to the diff machinery used to
+	generate the range-diff, e.g. `--range-diff-U0` and
+	`--range-diff--no-color`. This allows for adjusting the format
+	of the range-diff independently from the patch itself.
+
 --notes[=<ref>]::
 	Append the notes (see linkgit:git-notes[1]) for the commit
 	after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 02d88fa233..7658e56ecc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
 	fprintf(rev->diffopt.file, "\n");
 }
 
-static void make_cover_letter(struct rev_info *rev, int use_stdout,
+static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
+			      int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
@@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
-		struct diff_options opts;
-		memcpy(&opts, &rev->diffopt, sizeof(opts));
-		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
-
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &opts);
+				rev->creation_factor, 1, &rd_rev->diffopt);
 	}
 }
 
@@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct commit *commit;
 	struct commit **list = NULL;
 	struct rev_info rev;
+	struct rev_info rd_rev;
 	struct setup_revision_opt s_r_opt;
 	int nr = 0, total, i;
 	int use_stdout = 0;
@@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	init_log_defaults();
 	git_config(git_format_config, NULL);
 	repo_init_revisions(the_repository, &rev, prefix);
+	repo_init_revisions(the_repository, &rd_rev, prefix);
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.expand_tabs_in_log_default = 0;
 	rev.verbose_header = 1;
@@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.preserve_subject = keep_subject;
 
 	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
-	if (argc > 1)
-		die(_("unrecognized argument: %s"), argv[1]);
+	if (argc > 1) {
+		struct argv_array args = ARGV_ARRAY_INIT;
+		const char *prefix = "--range-diff";
+		int have_prefix = 0;
+
+		for (i = 0; i < argc; i++) {
+			struct strbuf sb = STRBUF_INIT;
+			char *str;
+
+			strbuf_addstr(&sb, argv[i]);
+			if (starts_with(argv[i], prefix)) {
+				have_prefix = 1;
+				strbuf_remove(&sb, 0, strlen(prefix));
+			}
+			str = strbuf_detach(&sb, NULL);
+			strbuf_release(&sb);
+
+			argv_array_push(&args, str);
+		}
+
+		if (!have_prefix)
+			die(_("unrecognized argument: %s"), argv[1]);
+		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
+		if (argc > 1)
+			die(_("unrecognized argument: %s"), argv[1]);
+	}
 
 	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
 		die(_("--name-only does not make sense"));
@@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (!use_patch_format &&
 		(!rev.diffopt.output_format ||
 		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
-		/* Needs to be mirrored in show_range_diff() invocation */
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
 	if (!rev.diffopt.stat_width)
 		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
@@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
-		make_cover_letter(&rev, use_stdout,
+		make_cover_letter(&rev, &rd_rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index bc5facc1cd..6916103888 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
 		--range-diff=topic~..topic changed~..changed >actual.raw &&
 	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
 	sed -e "s|:$||" >expect <<-\EOF &&
+	1:  a63e992 ! 1:  d966c5c s/12/B/
+	    @@ -8,7 +8,7 @@
+	     @@
+	      9
+	      10
+	    - B
+	    + BB
+	     -12
+	     +B
+	      13
+	-- :
+	EOF
+	test_cmp expect actual.range-diff &&
+	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
+	--- a/file
+	+++ b/file
+	@@ -12 +12 @@ BB
+	-12
+	+B
+	-- :
+	EOF
+	test_cmp expect actual.diff &&
+
+	# -U0 & --range-diff-U0
+	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
+		--range-diff=topic~..topic changed~..changed >actual.raw &&
+	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
 	1:  a63e992 ! 1:  d966c5c s/12/B/
 	    @@ -11 +11 @@
 	    - B
@@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
 	test_cmp expect actual.diff
 '
 
+test_expect_success 'format-patch option parsing with --range-diff-*' '
+	test_must_fail git format-patch --stdout --unknown \
+		master..unmodified 2>stderr &&
+	test_i18ngrep "unrecognized argument: --unknown" stderr &&
+	test_must_fail git format-patch --stdout --range-diff-unknown \
+		master..unmodified 2>stderr &&
+	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
+	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
+		master..unmodified 2>stderr &&
+	test_i18ngrep "unrecognized argument: --unknown" stderr
+'
+
 test_done
-- 
2.20.0.rc1.387.gf8505762e3
Junio C Hamano· Nov 29, 2018, 02:59 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> +		      [--range-diff<common diff option>]]

Let's make sure a random string thrown at this mechanism will properly get noticed and diagnosed.

Show 10 quoted lines
> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
>  	for details.
>  
> +--range-diff<common diff option>::
> +	Other options prefixed with `--range-diff` are stripped of
> +	that prefix and passed as-is to the diff machinery used to
> +	generate the range-diff, e.g. `--range-diff-U0` and
> +	`--range-diff--no-color`. This allows for adjusting the format
> +	of the range-diff independently from the patch itself.

Taking anything is of course the most general, but I am afraid if this backfires if there are some options that do not make sense to be different between the invocations of range-diff and format-patch.

Show 9 quoted lines
> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	rev.preserve_subject = keep_subject;
>  
>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> -	if (argc > 1)
> -		die(_("unrecognized argument: %s"), argv[1]);
> +	if (argc > 1) {
> +		struct argv_array args = ARGV_ARRAY_INIT;
> +		const char *prefix = "--range-diff";

Please call that anything but "prefix" that hides the parameter to the function.

	const char *range_diff_opt = "--range-diff";
might work OK, or it might not.  Let's read on.
Show 17 quoted lines
> +		int have_prefix = 0;
> +
> +		for (i = 0; i < argc; i++) {
> +			struct strbuf sb = STRBUF_INIT;
> +			char *str;
> +
> +			strbuf_addstr(&sb, argv[i]);
> +			if (starts_with(argv[i], prefix)) {
> +				have_prefix = 1;
> +				strbuf_remove(&sb, 0, strlen(prefix));
> +			}
> +			str = strbuf_detach(&sb, NULL);
> +			strbuf_release(&sb);
> +
> +			argv_array_push(&args, str);
> +		}
> +
Is the body of the loop essentially this?
			char *passopt = argv[i];
			if (!skip_prefix(passopt, range_diff_opt, &passopt))
				saw_range_diff_opt = 1;
			argv_array_push(&args, xstrdup(passopt));

We only use that "prefix" thing once, so we may not even need the variable.

> +		if (!have_prefix)
> +			die(_("unrecognized argument: %s"), argv[1]);

So we take normal options and check the leftover args; if there is no --range-diff<whatever> among the leftover bits, we pretend that we stumbled while reading the first such leftover arg.

> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> +		if (argc > 1)
> +			die(_("unrecognized argument: %s"), argv[1]);
> +	}

Otherwise, we pass all the leftover bits, which is a random mixture but guaranteed to have at least one meant for range-diff, to another setup_revisions(). If it leaves a leftover arg, then that is diagnosed here, so we'd be OK (iow, this is not a new "attack vector" to inject random string to command line parser).

One minor glitch I can see is "format-patch --range-diffSilly" would report "unrecognised arg: Silly". As we are pretending to be and reporting errors as format-patch, it would be better if we report that --range-diffSilly was what we did not understand.

> Junio: I know it's late, but unless Eric has objections to this UI
> change I'd really like to have this in 2.20 since this is a change to
> a new command-line UI that's newly added in 2.20.

Quite honestly, I'd rather document "driving range-diff from format-patch is experimental and does silly things when given non-standard options in this release" and not touch the code at this late stage in the game. Would it be less intrusive a change to *not* support the --range-diff<whatever> option, still use rd_rev that is separate from the main rev, and use a reasonable hardcoded default settings when preparing rd_rev?

Johannes Schindelin· Nov 29, 2018, 10:07 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

Hi Ævar,
On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> Change the semantics of the "--range-diff" option so that the regular
> diff options can be provided separately for the range-diff and the
> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> "format-patch" to provide different context for the range-diff and the
> patch. This wasn't possible before.

I really, really dislike the `--range-diff-<random-thing>`. We have precedent for passing optional arguments that are passed to some other command, so a much more logical and consistent convention would be to use `--range-diff[=<diff-option>..]`, allowing all of the diff options that you might want to pass to the outer diff in one go rather than having a lengthy string of `--range-diff-this` and `--range-diff-that` options.

I only had time to skim the patch, and I have to wonder why you pass around full-blown `rev_info` structs for range diff (and with that really awful name `rd_rev`) rather than just the `diff_options` that you *actually* care about?

Ciao, Dscho

Show 216 quoted lines
> 
> Ever since the "--range-diff" option was added in
> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> machinery has been the one we get from "format-patch"'s own
> setup_revisions().
> 
> This sort of thing is unique among the log-like commands in
> builtin/log.c, no command than format-patch will embed the output of
> another log-like command. Since the "rev->diffopt" is reused we need
> to munge it before we pass it to show_range_diff(). See
> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> output", 2018-11-22) for a related regression fix which is being
> mostly reverted here.
> 
> Implementation notes: 1) We're not bothering with the full teardown
> around die() and will leak memory, but it's too much boilerplate to do
> all the frees with/without the die() and not worth it. 2) We call
> repo_init_revisions() for "rd_rev" even though we could get away with
> a shallow copy like the code we're replacing (and which
> show_range_diff() itself does). This is to make this code more easily
> understood.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/git-format-patch.txt | 10 ++++++-
>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index aba4c5febe..6c048f415f 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -24,7 +24,8 @@ SYNOPSIS
>  		   [--to=<email>] [--cc=<email>]
>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>  		   [--interdiff=<previous>]
> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
> +		   [--range-diff=<previous> [--creation-factor=<percent>]
> +		      [--range-diff<common diff option>]]
>  		   [--progress]
>  		   [<common diff options>]
>  		   [ <since> | <revision range> ]
> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
>  	for details.
>  
> +--range-diff<common diff option>::
> +	Other options prefixed with `--range-diff` are stripped of
> +	that prefix and passed as-is to the diff machinery used to
> +	generate the range-diff, e.g. `--range-diff-U0` and
> +	`--range-diff--no-color`. This allows for adjusting the format
> +	of the range-diff independently from the patch itself.
> +
>  --notes[=<ref>]::
>  	Append the notes (see linkgit:git-notes[1]) for the commit
>  	after the three-dash line.
> diff --git a/builtin/log.c b/builtin/log.c
> index 02d88fa233..7658e56ecc 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>  	fprintf(rev->diffopt.file, "\n");
>  }
>  
> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> +			      int use_stdout,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
>  			      const char *branch_name,
> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	}
>  
>  	if (rev->rdiff1) {
> -		struct diff_options opts;
> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> -
>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>  		show_range_diff(rev->rdiff1, rev->rdiff2,
> -				rev->creation_factor, 1, &opts);
> +				rev->creation_factor, 1, &rd_rev->diffopt);
>  	}
>  }
>  
> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	struct commit *commit;
>  	struct commit **list = NULL;
>  	struct rev_info rev;
> +	struct rev_info rd_rev;
>  	struct setup_revision_opt s_r_opt;
>  	int nr = 0, total, i;
>  	int use_stdout = 0;
> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	init_log_defaults();
>  	git_config(git_format_config, NULL);
>  	repo_init_revisions(the_repository, &rev, prefix);
> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>  	rev.commit_format = CMIT_FMT_EMAIL;
>  	rev.expand_tabs_in_log_default = 0;
>  	rev.verbose_header = 1;
> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	rev.preserve_subject = keep_subject;
>  
>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> -	if (argc > 1)
> -		die(_("unrecognized argument: %s"), argv[1]);
> +	if (argc > 1) {
> +		struct argv_array args = ARGV_ARRAY_INIT;
> +		const char *prefix = "--range-diff";
> +		int have_prefix = 0;
> +
> +		for (i = 0; i < argc; i++) {
> +			struct strbuf sb = STRBUF_INIT;
> +			char *str;
> +
> +			strbuf_addstr(&sb, argv[i]);
> +			if (starts_with(argv[i], prefix)) {
> +				have_prefix = 1;
> +				strbuf_remove(&sb, 0, strlen(prefix));
> +			}
> +			str = strbuf_detach(&sb, NULL);
> +			strbuf_release(&sb);
> +
> +			argv_array_push(&args, str);
> +		}
> +
> +		if (!have_prefix)
> +			die(_("unrecognized argument: %s"), argv[1]);
> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> +		if (argc > 1)
> +			die(_("unrecognized argument: %s"), argv[1]);
> +	}
>  
>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>  		die(_("--name-only does not make sense"));
> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (!use_patch_format &&
>  		(!rev.diffopt.output_format ||
>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> -		/* Needs to be mirrored in show_range_diff() invocation */
>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>  	if (!rev.diffopt.stat_width)
>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (cover_letter) {
>  		if (thread)
>  			gen_message_id(&rev, "cover");
> -		make_cover_letter(&rev, use_stdout,
> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>  				  origin, nr, list, branch_name, quiet);
>  		print_bases(&bases, rev.diffopt.file);
>  		print_signature(rev.diffopt.file);
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index bc5facc1cd..6916103888 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>  	sed -e "s|:$||" >expect <<-\EOF &&
> +	1:  a63e992 ! 1:  d966c5c s/12/B/
> +	    @@ -8,7 +8,7 @@
> +	     @@
> +	      9
> +	      10
> +	    - B
> +	    + BB
> +	     -12
> +	     +B
> +	      13
> +	-- :
> +	EOF
> +	test_cmp expect actual.range-diff &&
> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
> +	sed -e "s|:$||" >expect <<-\EOF &&
> +	--- a/file
> +	+++ b/file
> +	@@ -12 +12 @@ BB
> +	-12
> +	+B
> +	-- :
> +	EOF
> +	test_cmp expect actual.diff &&
> +
> +	# -U0 & --range-diff-U0
> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> +	sed -e "s|:$||" >expect <<-\EOF &&
>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>  	    @@ -11 +11 @@
>  	    - B
> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>  	test_cmp expect actual.diff
>  '
>  
> +test_expect_success 'format-patch option parsing with --range-diff-*' '
> +	test_must_fail git format-patch --stdout --unknown \
> +		master..unmodified 2>stderr &&
> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
> +	test_must_fail git format-patch --stdout --range-diff-unknown \
> +		master..unmodified 2>stderr &&
> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
> +		master..unmodified 2>stderr &&
> +	test_i18ngrep "unrecognized argument: --unknown" stderr
> +'
> +
>  test_done
> -- 
> 2.20.0.rc1.387.gf8505762e3
> 
> 
Ævar Arnfjörð Bjarmason· Nov 29, 2018, 10:30 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

On Thu, Nov 29 2018, Johannes Schindelin wrote:
Show 16 quoted lines
> Hi Ævar,
>
> On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the semantics of the "--range-diff" option so that the regular
>> diff options can be provided separately for the range-diff and the
>> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
>> "format-patch" to provide different context for the range-diff and the
>> patch. This wasn't possible before.
>
> I really, really dislike the `--range-diff-<random-thing>`. We have
> precedent for passing optional arguments that are passed to some other
> command, so a much more logical and consistent convention would be to use
> `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> you might want to pass to the outer diff in one go rather than having a
> lengthy string of `--range-diff-this` and `--range-diff-that` options.
Where do we pass those sorts of arguments?
Reasons I did it this way:
 a) Passing it as one option will require the user to double-quote those
    options that take quoted arguments (e.g. --word-diff-regex), which I
    thought sucked more than the prefix. On the implementation side we
    couldn't leave the parsing of the command-line to the shell anymore.
 b) I think people will want to tweak this very rarely, much more rarely
    than e.g. -U10 in format-patch itself, so having something long-ish
    doesn't sound bad.
> I only had time to skim the patch, and I have to wonder why you pass
> around full-blown `rev_info` structs for range diff (and with that really
> awful name `rd_rev`) rather than just the `diff_options` that you
> *actually* care about?

Because setup_revisions() which does all the command-line parsing needs a rev_info, so even if we only need the diffopt in the end we need to initiate the whole thing.

Suggestions for a better varibale name most welcome.
Show 219 quoted lines
> Ciao,
> Dscho
>
>>
>> Ever since the "--range-diff" option was added in
>> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
>> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
>> machinery has been the one we get from "format-patch"'s own
>> setup_revisions().
>>
>> This sort of thing is unique among the log-like commands in
>> builtin/log.c, no command than format-patch will embed the output of
>> another log-like command. Since the "rev->diffopt" is reused we need
>> to munge it before we pass it to show_range_diff(). See
>> 43dafc4172 ("format-patch: don't include --stat with --range-diff
>> output", 2018-11-22) for a related regression fix which is being
>> mostly reverted here.
>>
>> Implementation notes: 1) We're not bothering with the full teardown
>> around die() and will leak memory, but it's too much boilerplate to do
>> all the frees with/without the die() and not worth it. 2) We call
>> repo_init_revisions() for "rd_rev" even though we could get away with
>> a shallow copy like the code we're replacing (and which
>> show_range_diff() itself does). This is to make this code more easily
>> understood.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  Documentation/git-format-patch.txt | 10 ++++++-
>>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>>  3 files changed, 82 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> index aba4c5febe..6c048f415f 100644
>> --- a/Documentation/git-format-patch.txt
>> +++ b/Documentation/git-format-patch.txt
>> @@ -24,7 +24,8 @@ SYNOPSIS
>>  		   [--to=<email>] [--cc=<email>]
>>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>>  		   [--interdiff=<previous>]
>> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
>> +		   [--range-diff=<previous> [--creation-factor=<percent>]
>> +		      [--range-diff<common diff option>]]
>>  		   [--progress]
>>  		   [<common diff options>]
>>  		   [ <since> | <revision range> ]
>> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
>>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
>>  	for details.
>>
>> +--range-diff<common diff option>::
>> +	Other options prefixed with `--range-diff` are stripped of
>> +	that prefix and passed as-is to the diff machinery used to
>> +	generate the range-diff, e.g. `--range-diff-U0` and
>> +	`--range-diff--no-color`. This allows for adjusting the format
>> +	of the range-diff independently from the patch itself.
>> +
>>  --notes[=<ref>]::
>>  	Append the notes (see linkgit:git-notes[1]) for the commit
>>  	after the three-dash line.
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 02d88fa233..7658e56ecc 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>>  	fprintf(rev->diffopt.file, "\n");
>>  }
>>
>> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
>> +			      int use_stdout,
>>  			      struct commit *origin,
>>  			      int nr, struct commit **list,
>>  			      const char *branch_name,
>> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>>  	}
>>
>>  	if (rev->rdiff1) {
>> -		struct diff_options opts;
>> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
>> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> -
>>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>>  		show_range_diff(rev->rdiff1, rev->rdiff2,
>> -				rev->creation_factor, 1, &opts);
>> +				rev->creation_factor, 1, &rd_rev->diffopt);
>>  	}
>>  }
>>
>> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	struct commit *commit;
>>  	struct commit **list = NULL;
>>  	struct rev_info rev;
>> +	struct rev_info rd_rev;
>>  	struct setup_revision_opt s_r_opt;
>>  	int nr = 0, total, i;
>>  	int use_stdout = 0;
>> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	init_log_defaults();
>>  	git_config(git_format_config, NULL);
>>  	repo_init_revisions(the_repository, &rev, prefix);
>> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>>  	rev.commit_format = CMIT_FMT_EMAIL;
>>  	rev.expand_tabs_in_log_default = 0;
>>  	rev.verbose_header = 1;
>> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	rev.preserve_subject = keep_subject;
>>
>>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
>> -	if (argc > 1)
>> -		die(_("unrecognized argument: %s"), argv[1]);
>> +	if (argc > 1) {
>> +		struct argv_array args = ARGV_ARRAY_INIT;
>> +		const char *prefix = "--range-diff";
>> +		int have_prefix = 0;
>> +
>> +		for (i = 0; i < argc; i++) {
>> +			struct strbuf sb = STRBUF_INIT;
>> +			char *str;
>> +
>> +			strbuf_addstr(&sb, argv[i]);
>> +			if (starts_with(argv[i], prefix)) {
>> +				have_prefix = 1;
>> +				strbuf_remove(&sb, 0, strlen(prefix));
>> +			}
>> +			str = strbuf_detach(&sb, NULL);
>> +			strbuf_release(&sb);
>> +
>> +			argv_array_push(&args, str);
>> +		}
>> +
>> +		if (!have_prefix)
>> +			die(_("unrecognized argument: %s"), argv[1]);
>> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
>> +		if (argc > 1)
>> +			die(_("unrecognized argument: %s"), argv[1]);
>> +	}
>>
>>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>>  		die(_("--name-only does not make sense"));
>> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	if (!use_patch_format &&
>>  		(!rev.diffopt.output_format ||
>>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
>> -		/* Needs to be mirrored in show_range_diff() invocation */
>>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>>  	if (!rev.diffopt.stat_width)
>>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
>> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	if (cover_letter) {
>>  		if (thread)
>>  			gen_message_id(&rev, "cover");
>> -		make_cover_letter(&rev, use_stdout,
>> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>>  				  origin, nr, list, branch_name, quiet);
>>  		print_bases(&bases, rev.diffopt.file);
>>  		print_signature(rev.diffopt.file);
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> index bc5facc1cd..6916103888 100755
>> --- a/t/t3206-range-diff.sh
>> +++ b/t/t3206-range-diff.sh
>> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>>  	sed -e "s|:$||" >expect <<-\EOF &&
>> +	1:  a63e992 ! 1:  d966c5c s/12/B/
>> +	    @@ -8,7 +8,7 @@
>> +	     @@
>> +	      9
>> +	      10
>> +	    - B
>> +	    + BB
>> +	     -12
>> +	     +B
>> +	      13
>> +	-- :
>> +	EOF
>> +	test_cmp expect actual.range-diff &&
>> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
>> +	sed -e "s|:$||" >expect <<-\EOF &&
>> +	--- a/file
>> +	+++ b/file
>> +	@@ -12 +12 @@ BB
>> +	-12
>> +	+B
>> +	-- :
>> +	EOF
>> +	test_cmp expect actual.diff &&
>> +
>> +	# -U0 & --range-diff-U0
>> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
>> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> +	sed -e "s|:$||" >expect <<-\EOF &&
>>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>>  	    @@ -11 +11 @@
>>  	    - B
>> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>>  	test_cmp expect actual.diff
>>  '
>>
>> +test_expect_success 'format-patch option parsing with --range-diff-*' '
>> +	test_must_fail git format-patch --stdout --unknown \
>> +		master..unmodified 2>stderr &&
>> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
>> +	test_must_fail git format-patch --stdout --range-diff-unknown \
>> +		master..unmodified 2>stderr &&
>> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
>> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
>> +		master..unmodified 2>stderr &&
>> +	test_i18ngrep "unrecognized argument: --unknown" stderr
>> +'
>> +
>>  test_done
>> --
>> 2.20.0.rc1.387.gf8505762e3
>>
>>
Johannes Schindelin· Nov 29, 2018, 12:12 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

Hi Ævar,
On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 29 quoted lines
> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> 
> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> Change the semantics of the "--range-diff" option so that the regular
> >> diff options can be provided separately for the range-diff and the
> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> "format-patch" to provide different context for the range-diff and the
> >> patch. This wasn't possible before.
> >
> > I really, really dislike the `--range-diff-<random-thing>`. We have
> > precedent for passing optional arguments that are passed to some other
> > command, so a much more logical and consistent convention would be to use
> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> > you might want to pass to the outer diff in one go rather than having a
> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
> 
> Where do we pass those sorts of arguments?
> 
> Reasons I did it this way:
> 
>  a) Passing it as one option will require the user to double-quote those
>     options that take quoted arguments (e.g. --word-diff-regex), which I
>     thought sucked more than the prefix. On the implementation side we
>     couldn't leave the parsing of the command-line to the shell anymore.
> 
>  b) I think people will want to tweak this very rarely, much more rarely
>     than e.g. -U10 in format-patch itself, so having something long-ish
>     doesn't sound bad.

Hmm. I still don't like it. It sets a precedent, and we simply do not do it that way in other circumstances (most obvious would be the -X merge options). The more divergent user interfaces for the same sort of thing are, the more brain cycles you force users to spend on navigating said interfaces.

Show 10 quoted lines
> > I only had time to skim the patch, and I have to wonder why you pass
> > around full-blown `rev_info` structs for range diff (and with that really
> > awful name `rd_rev`) rather than just the `diff_options` that you
> > *actually* care about?
> 
> Because setup_revisions() which does all the command-line parsing needs
> a rev_info, so even if we only need the diffopt in the end we need to
> initiate the whole thing.
> 
> Suggestions for a better varibale name most welcome.
`range_diff_revs`

And you do not need to pass around the whole thing. You can easily pass `&range_diff_revs.diffopt`.

Don't pass around what you do not need to pass around.

Ciao, Dscho

Show 221 quoted lines
> 
> > Ciao,
> > Dscho
> >
> >>
> >> Ever since the "--range-diff" option was added in
> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> >> machinery has been the one we get from "format-patch"'s own
> >> setup_revisions().
> >>
> >> This sort of thing is unique among the log-like commands in
> >> builtin/log.c, no command than format-patch will embed the output of
> >> another log-like command. Since the "rev->diffopt" is reused we need
> >> to munge it before we pass it to show_range_diff(). See
> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> >> output", 2018-11-22) for a related regression fix which is being
> >> mostly reverted here.
> >>
> >> Implementation notes: 1) We're not bothering with the full teardown
> >> around die() and will leak memory, but it's too much boilerplate to do
> >> all the frees with/without the die() and not worth it. 2) We call
> >> repo_init_revisions() for "rd_rev" even though we could get away with
> >> a shallow copy like the code we're replacing (and which
> >> show_range_diff() itself does). This is to make this code more easily
> >> understood.
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  Documentation/git-format-patch.txt | 10 ++++++-
> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
> >>  3 files changed, 82 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> >> index aba4c5febe..6c048f415f 100644
> >> --- a/Documentation/git-format-patch.txt
> >> +++ b/Documentation/git-format-patch.txt
> >> @@ -24,7 +24,8 @@ SYNOPSIS
> >>  		   [--to=<email>] [--cc=<email>]
> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> >>  		   [--interdiff=<previous>]
> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
> >> +		      [--range-diff<common diff option>]]
> >>  		   [--progress]
> >>  		   [<common diff options>]
> >>  		   [ <since> | <revision range> ]
> >> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
> >>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
> >>  	for details.
> >>
> >> +--range-diff<common diff option>::
> >> +	Other options prefixed with `--range-diff` are stripped of
> >> +	that prefix and passed as-is to the diff machinery used to
> >> +	generate the range-diff, e.g. `--range-diff-U0` and
> >> +	`--range-diff--no-color`. This allows for adjusting the format
> >> +	of the range-diff independently from the patch itself.
> >> +
> >>  --notes[=<ref>]::
> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
> >>  	after the three-dash line.
> >> diff --git a/builtin/log.c b/builtin/log.c
> >> index 02d88fa233..7658e56ecc 100644
> >> --- a/builtin/log.c
> >> +++ b/builtin/log.c
> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
> >>  	fprintf(rev->diffopt.file, "\n");
> >>  }
> >>
> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> >> +			      int use_stdout,
> >>  			      struct commit *origin,
> >>  			      int nr, struct commit **list,
> >>  			      const char *branch_name,
> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >>  	}
> >>
> >>  	if (rev->rdiff1) {
> >> -		struct diff_options opts;
> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> >> -
> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
> >> -				rev->creation_factor, 1, &opts);
> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
> >>  	}
> >>  }
> >>
> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	struct commit *commit;
> >>  	struct commit **list = NULL;
> >>  	struct rev_info rev;
> >> +	struct rev_info rd_rev;
> >>  	struct setup_revision_opt s_r_opt;
> >>  	int nr = 0, total, i;
> >>  	int use_stdout = 0;
> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	init_log_defaults();
> >>  	git_config(git_format_config, NULL);
> >>  	repo_init_revisions(the_repository, &rev, prefix);
> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
> >>  	rev.commit_format = CMIT_FMT_EMAIL;
> >>  	rev.expand_tabs_in_log_default = 0;
> >>  	rev.verbose_header = 1;
> >> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	rev.preserve_subject = keep_subject;
> >>
> >>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> >> -	if (argc > 1)
> >> -		die(_("unrecognized argument: %s"), argv[1]);
> >> +	if (argc > 1) {
> >> +		struct argv_array args = ARGV_ARRAY_INIT;
> >> +		const char *prefix = "--range-diff";
> >> +		int have_prefix = 0;
> >> +
> >> +		for (i = 0; i < argc; i++) {
> >> +			struct strbuf sb = STRBUF_INIT;
> >> +			char *str;
> >> +
> >> +			strbuf_addstr(&sb, argv[i]);
> >> +			if (starts_with(argv[i], prefix)) {
> >> +				have_prefix = 1;
> >> +				strbuf_remove(&sb, 0, strlen(prefix));
> >> +			}
> >> +			str = strbuf_detach(&sb, NULL);
> >> +			strbuf_release(&sb);
> >> +
> >> +			argv_array_push(&args, str);
> >> +		}
> >> +
> >> +		if (!have_prefix)
> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> >> +		if (argc > 1)
> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> +	}
> >>
> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
> >>  		die(_("--name-only does not make sense"));
> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	if (!use_patch_format &&
> >>  		(!rev.diffopt.output_format ||
> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> >> -		/* Needs to be mirrored in show_range_diff() invocation */
> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
> >>  	if (!rev.diffopt.stat_width)
> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >>  	if (cover_letter) {
> >>  		if (thread)
> >>  			gen_message_id(&rev, "cover");
> >> -		make_cover_letter(&rev, use_stdout,
> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
> >>  				  origin, nr, list, branch_name, quiet);
> >>  		print_bases(&bases, rev.diffopt.file);
> >>  		print_signature(rev.diffopt.file);
> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> >> index bc5facc1cd..6916103888 100755
> >> --- a/t/t3206-range-diff.sh
> >> +++ b/t/t3206-range-diff.sh
> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >>  	sed -e "s|:$||" >expect <<-\EOF &&
> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
> >> +	    @@ -8,7 +8,7 @@
> >> +	     @@
> >> +	      9
> >> +	      10
> >> +	    - B
> >> +	    + BB
> >> +	     -12
> >> +	     +B
> >> +	      13
> >> +	-- :
> >> +	EOF
> >> +	test_cmp expect actual.range-diff &&
> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >> +	--- a/file
> >> +	+++ b/file
> >> +	@@ -12 +12 @@ BB
> >> +	-12
> >> +	+B
> >> +	-- :
> >> +	EOF
> >> +	test_cmp expect actual.diff &&
> >> +
> >> +	# -U0 & --range-diff-U0
> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
> >>  	    @@ -11 +11 @@
> >>  	    - B
> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
> >>  	test_cmp expect actual.diff
> >>  '
> >>
> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
> >> +	test_must_fail git format-patch --stdout --unknown \
> >> +		master..unmodified 2>stderr &&
> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
> >> +		master..unmodified 2>stderr &&
> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
> >> +		master..unmodified 2>stderr &&
> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
> >> +'
> >> +
> >>  test_done
> >> --
> >> 2.20.0.rc1.387.gf8505762e3
> >>
> >>
> 
Ævar Arnfjörð Bjarmason· Nov 29, 2018, 14:35 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

On Thu, Nov 29 2018, Johannes Schindelin wrote:
Show 39 quoted lines
> Hi Ævar,
>
> On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>>
>> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> Change the semantics of the "--range-diff" option so that the regular
>> >> diff options can be provided separately for the range-diff and the
>> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
>> >> "format-patch" to provide different context for the range-diff and the
>> >> patch. This wasn't possible before.
>> >
>> > I really, really dislike the `--range-diff-<random-thing>`. We have
>> > precedent for passing optional arguments that are passed to some other
>> > command, so a much more logical and consistent convention would be to use
>> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
>> > you might want to pass to the outer diff in one go rather than having a
>> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
>>
>> Where do we pass those sorts of arguments?
>>
>> Reasons I did it this way:
>>
>>  a) Passing it as one option will require the user to double-quote those
>>     options that take quoted arguments (e.g. --word-diff-regex), which I
>>     thought sucked more than the prefix. On the implementation side we
>>     couldn't leave the parsing of the command-line to the shell anymore.
>>
>>  b) I think people will want to tweak this very rarely, much more rarely
>>     than e.g. -U10 in format-patch itself, so having something long-ish
>>     doesn't sound bad.
>
> Hmm. I still don't like it. It sets a precedent, and we simply do not do
> it that way in other circumstances (most obvious would be the -X merge
> options). The more divergent user interfaces for the same sort of thing
> are, the more brain cycles you force users to spend on navigating said
> interfaces.

Yeah it sucks, I just think it sucks less than the alternative :) I.e. I'm not picky about --range-diff-* prefix the name, but I think doing our own shell parsing would be nasty.

Show 17 quoted lines
>> > I only had time to skim the patch, and I have to wonder why you pass
>> > around full-blown `rev_info` structs for range diff (and with that really
>> > awful name `rd_rev`) rather than just the `diff_options` that you
>> > *actually* care about?
>>
>> Because setup_revisions() which does all the command-line parsing needs
>> a rev_info, so even if we only need the diffopt in the end we need to
>> initiate the whole thing.
>>
>> Suggestions for a better varibale name most welcome.
>
> `range_diff_revs`
>
> And you do not need to pass around the whole thing. You can easily pass
> `&range_diff_revs.diffopt`.
>
> Don't pass around what you do not need to pass around.

Ah, you mean internally in log.c, yes that makes sense. I thought you meant just pass diffopt to setup_revisions() (which needs the containing struct). Willdo.

Show 224 quoted lines
> Ciao,
> Dscho
>
>>
>> > Ciao,
>> > Dscho
>> >
>> >>
>> >> Ever since the "--range-diff" option was added in
>> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
>> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
>> >> machinery has been the one we get from "format-patch"'s own
>> >> setup_revisions().
>> >>
>> >> This sort of thing is unique among the log-like commands in
>> >> builtin/log.c, no command than format-patch will embed the output of
>> >> another log-like command. Since the "rev->diffopt" is reused we need
>> >> to munge it before we pass it to show_range_diff(). See
>> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
>> >> output", 2018-11-22) for a related regression fix which is being
>> >> mostly reverted here.
>> >>
>> >> Implementation notes: 1) We're not bothering with the full teardown
>> >> around die() and will leak memory, but it's too much boilerplate to do
>> >> all the frees with/without the die() and not worth it. 2) We call
>> >> repo_init_revisions() for "rd_rev" even though we could get away with
>> >> a shallow copy like the code we're replacing (and which
>> >> show_range_diff() itself does). This is to make this code more easily
>> >> understood.
>> >>
>> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >> ---
>> >>  Documentation/git-format-patch.txt | 10 ++++++-
>> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>> >>  3 files changed, 82 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> >> index aba4c5febe..6c048f415f 100644
>> >> --- a/Documentation/git-format-patch.txt
>> >> +++ b/Documentation/git-format-patch.txt
>> >> @@ -24,7 +24,8 @@ SYNOPSIS
>> >>  		   [--to=<email>] [--cc=<email>]
>> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>> >>  		   [--interdiff=<previous>]
>> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
>> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
>> >> +		      [--range-diff<common diff option>]]
>> >>  		   [--progress]
>> >>  		   [<common diff options>]
>> >>  		   [ <since> | <revision range> ]
>> >> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
>> >>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
>> >>  	for details.
>> >>
>> >> +--range-diff<common diff option>::
>> >> +	Other options prefixed with `--range-diff` are stripped of
>> >> +	that prefix and passed as-is to the diff machinery used to
>> >> +	generate the range-diff, e.g. `--range-diff-U0` and
>> >> +	`--range-diff--no-color`. This allows for adjusting the format
>> >> +	of the range-diff independently from the patch itself.
>> >> +
>> >>  --notes[=<ref>]::
>> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
>> >>  	after the three-dash line.
>> >> diff --git a/builtin/log.c b/builtin/log.c
>> >> index 02d88fa233..7658e56ecc 100644
>> >> --- a/builtin/log.c
>> >> +++ b/builtin/log.c
>> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>> >>  	fprintf(rev->diffopt.file, "\n");
>> >>  }
>> >>
>> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
>> >> +			      int use_stdout,
>> >>  			      struct commit *origin,
>> >>  			      int nr, struct commit **list,
>> >>  			      const char *branch_name,
>> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >>  	}
>> >>
>> >>  	if (rev->rdiff1) {
>> >> -		struct diff_options opts;
>> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
>> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> >> -
>> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
>> >> -				rev->creation_factor, 1, &opts);
>> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
>> >>  	}
>> >>  }
>> >>
>> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	struct commit *commit;
>> >>  	struct commit **list = NULL;
>> >>  	struct rev_info rev;
>> >> +	struct rev_info rd_rev;
>> >>  	struct setup_revision_opt s_r_opt;
>> >>  	int nr = 0, total, i;
>> >>  	int use_stdout = 0;
>> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	init_log_defaults();
>> >>  	git_config(git_format_config, NULL);
>> >>  	repo_init_revisions(the_repository, &rev, prefix);
>> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>> >>  	rev.commit_format = CMIT_FMT_EMAIL;
>> >>  	rev.expand_tabs_in_log_default = 0;
>> >>  	rev.verbose_header = 1;
>> >> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	rev.preserve_subject = keep_subject;
>> >>
>> >>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
>> >> -	if (argc > 1)
>> >> -		die(_("unrecognized argument: %s"), argv[1]);
>> >> +	if (argc > 1) {
>> >> +		struct argv_array args = ARGV_ARRAY_INIT;
>> >> +		const char *prefix = "--range-diff";
>> >> +		int have_prefix = 0;
>> >> +
>> >> +		for (i = 0; i < argc; i++) {
>> >> +			struct strbuf sb = STRBUF_INIT;
>> >> +			char *str;
>> >> +
>> >> +			strbuf_addstr(&sb, argv[i]);
>> >> +			if (starts_with(argv[i], prefix)) {
>> >> +				have_prefix = 1;
>> >> +				strbuf_remove(&sb, 0, strlen(prefix));
>> >> +			}
>> >> +			str = strbuf_detach(&sb, NULL);
>> >> +			strbuf_release(&sb);
>> >> +
>> >> +			argv_array_push(&args, str);
>> >> +		}
>> >> +
>> >> +		if (!have_prefix)
>> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
>> >> +		if (argc > 1)
>> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> +	}
>> >>
>> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>> >>  		die(_("--name-only does not make sense"));
>> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	if (!use_patch_format &&
>> >>  		(!rev.diffopt.output_format ||
>> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
>> >> -		/* Needs to be mirrored in show_range_diff() invocation */
>> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>> >>  	if (!rev.diffopt.stat_width)
>> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
>> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >>  	if (cover_letter) {
>> >>  		if (thread)
>> >>  			gen_message_id(&rev, "cover");
>> >> -		make_cover_letter(&rev, use_stdout,
>> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>> >>  				  origin, nr, list, branch_name, quiet);
>> >>  		print_bases(&bases, rev.diffopt.file);
>> >>  		print_signature(rev.diffopt.file);
>> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> >> index bc5facc1cd..6916103888 100755
>> >> --- a/t/t3206-range-diff.sh
>> >> +++ b/t/t3206-range-diff.sh
>> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >>  	sed -e "s|:$||" >expect <<-\EOF &&
>> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >> +	    @@ -8,7 +8,7 @@
>> >> +	     @@
>> >> +	      9
>> >> +	      10
>> >> +	    - B
>> >> +	    + BB
>> >> +	     -12
>> >> +	     +B
>> >> +	      13
>> >> +	-- :
>> >> +	EOF
>> >> +	test_cmp expect actual.range-diff &&
>> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
>> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >> +	--- a/file
>> >> +	+++ b/file
>> >> +	@@ -12 +12 @@ BB
>> >> +	-12
>> >> +	+B
>> >> +	-- :
>> >> +	EOF
>> >> +	test_cmp expect actual.diff &&
>> >> +
>> >> +	# -U0 & --range-diff-U0
>> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
>> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >>  	    @@ -11 +11 @@
>> >>  	    - B
>> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>> >>  	test_cmp expect actual.diff
>> >>  '
>> >>
>> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
>> >> +	test_must_fail git format-patch --stdout --unknown \
>> >> +		master..unmodified 2>stderr &&
>> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
>> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
>> >> +		master..unmodified 2>stderr &&
>> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
>> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
>> >> +		master..unmodified 2>stderr &&
>> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
>> >> +'
>> >> +
>> >>  test_done
>> >> --
>> >> 2.20.0.rc1.387.gf8505762e3
>> >>
>> >>
>>
Johannes Schindelin· Nov 29, 2018, 15:41 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

Hi Ævar,
On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 43 quoted lines
> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> 
> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> >>
> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >> >
> >> >> Change the semantics of the "--range-diff" option so that the regular
> >> >> diff options can be provided separately for the range-diff and the
> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> >> "format-patch" to provide different context for the range-diff and the
> >> >> patch. This wasn't possible before.
> >> >
> >> > I really, really dislike the `--range-diff-<random-thing>`. We have
> >> > precedent for passing optional arguments that are passed to some other
> >> > command, so a much more logical and consistent convention would be to use
> >> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> >> > you might want to pass to the outer diff in one go rather than having a
> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
> >>
> >> Where do we pass those sorts of arguments?
> >>
> >> Reasons I did it this way:
> >>
> >>  a) Passing it as one option will require the user to double-quote those
> >>     options that take quoted arguments (e.g. --word-diff-regex), which I
> >>     thought sucked more than the prefix. On the implementation side we
> >>     couldn't leave the parsing of the command-line to the shell anymore.
> >>
> >>  b) I think people will want to tweak this very rarely, much more rarely
> >>     than e.g. -U10 in format-patch itself, so having something long-ish
> >>     doesn't sound bad.
> >
> > Hmm. I still don't like it. It sets a precedent, and we simply do not do
> > it that way in other circumstances (most obvious would be the -X merge
> > options). The more divergent user interfaces for the same sort of thing
> > are, the more brain cycles you force users to spend on navigating said
> > interfaces.
> 
> Yeah it sucks, I just think it sucks less than the alternative :)
> I.e. I'm not picky about --range-diff-* prefix the name, but I think
> doing our own shell parsing would be nasty.
What prevents you from using `sq_dequote_to_argv()`?
Show 21 quoted lines
> >> > I only had time to skim the patch, and I have to wonder why you pass
> >> > around full-blown `rev_info` structs for range diff (and with that really
> >> > awful name `rd_rev`) rather than just the `diff_options` that you
> >> > *actually* care about?
> >>
> >> Because setup_revisions() which does all the command-line parsing needs
> >> a rev_info, so even if we only need the diffopt in the end we need to
> >> initiate the whole thing.
> >>
> >> Suggestions for a better varibale name most welcome.
> >
> > `range_diff_revs`
> >
> > And you do not need to pass around the whole thing. You can easily pass
> > `&range_diff_revs.diffopt`.
> >
> > Don't pass around what you do not need to pass around.
> 
> Ah, you mean internally in log.c, yes that makes sense. I thought you
> meant just pass diffopt to setup_revisions() (which needs the containing
> struct). Willdo.

Thanks, Dscho

Show 226 quoted lines
> 
> > Ciao,
> > Dscho
> >
> >>
> >> > Ciao,
> >> > Dscho
> >> >
> >> >>
> >> >> Ever since the "--range-diff" option was added in
> >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> >> >> machinery has been the one we get from "format-patch"'s own
> >> >> setup_revisions().
> >> >>
> >> >> This sort of thing is unique among the log-like commands in
> >> >> builtin/log.c, no command than format-patch will embed the output of
> >> >> another log-like command. Since the "rev->diffopt" is reused we need
> >> >> to munge it before we pass it to show_range_diff(). See
> >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> >> >> output", 2018-11-22) for a related regression fix which is being
> >> >> mostly reverted here.
> >> >>
> >> >> Implementation notes: 1) We're not bothering with the full teardown
> >> >> around die() and will leak memory, but it's too much boilerplate to do
> >> >> all the frees with/without the die() and not worth it. 2) We call
> >> >> repo_init_revisions() for "rd_rev" even though we could get away with
> >> >> a shallow copy like the code we're replacing (and which
> >> >> show_range_diff() itself does). This is to make this code more easily
> >> >> understood.
> >> >>
> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> >> ---
> >> >>  Documentation/git-format-patch.txt | 10 ++++++-
> >> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
> >> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
> >> >>  3 files changed, 82 insertions(+), 11 deletions(-)
> >> >>
> >> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> >> >> index aba4c5febe..6c048f415f 100644
> >> >> --- a/Documentation/git-format-patch.txt
> >> >> +++ b/Documentation/git-format-patch.txt
> >> >> @@ -24,7 +24,8 @@ SYNOPSIS
> >> >>  		   [--to=<email>] [--cc=<email>]
> >> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> >> >>  		   [--interdiff=<previous>]
> >> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
> >> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
> >> >> +		      [--range-diff<common diff option>]]
> >> >>  		   [--progress]
> >> >>  		   [<common diff options>]
> >> >>  		   [ <since> | <revision range> ]
> >> >> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
> >> >>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
> >> >>  	for details.
> >> >>
> >> >> +--range-diff<common diff option>::
> >> >> +	Other options prefixed with `--range-diff` are stripped of
> >> >> +	that prefix and passed as-is to the diff machinery used to
> >> >> +	generate the range-diff, e.g. `--range-diff-U0` and
> >> >> +	`--range-diff--no-color`. This allows for adjusting the format
> >> >> +	of the range-diff independently from the patch itself.
> >> >> +
> >> >>  --notes[=<ref>]::
> >> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
> >> >>  	after the three-dash line.
> >> >> diff --git a/builtin/log.c b/builtin/log.c
> >> >> index 02d88fa233..7658e56ecc 100644
> >> >> --- a/builtin/log.c
> >> >> +++ b/builtin/log.c
> >> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
> >> >>  	fprintf(rev->diffopt.file, "\n");
> >> >>  }
> >> >>
> >> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> >> >> +			      int use_stdout,
> >> >>  			      struct commit *origin,
> >> >>  			      int nr, struct commit **list,
> >> >>  			      const char *branch_name,
> >> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> >> >>  	}
> >> >>
> >> >>  	if (rev->rdiff1) {
> >> >> -		struct diff_options opts;
> >> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
> >> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> >> >> -
> >> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> >> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
> >> >> -				rev->creation_factor, 1, &opts);
> >> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
> >> >>  	}
> >> >>  }
> >> >>
> >> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	struct commit *commit;
> >> >>  	struct commit **list = NULL;
> >> >>  	struct rev_info rev;
> >> >> +	struct rev_info rd_rev;
> >> >>  	struct setup_revision_opt s_r_opt;
> >> >>  	int nr = 0, total, i;
> >> >>  	int use_stdout = 0;
> >> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	init_log_defaults();
> >> >>  	git_config(git_format_config, NULL);
> >> >>  	repo_init_revisions(the_repository, &rev, prefix);
> >> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
> >> >>  	rev.commit_format = CMIT_FMT_EMAIL;
> >> >>  	rev.expand_tabs_in_log_default = 0;
> >> >>  	rev.verbose_header = 1;
> >> >> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	rev.preserve_subject = keep_subject;
> >> >>
> >> >>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> >> >> -	if (argc > 1)
> >> >> -		die(_("unrecognized argument: %s"), argv[1]);
> >> >> +	if (argc > 1) {
> >> >> +		struct argv_array args = ARGV_ARRAY_INIT;
> >> >> +		const char *prefix = "--range-diff";
> >> >> +		int have_prefix = 0;
> >> >> +
> >> >> +		for (i = 0; i < argc; i++) {
> >> >> +			struct strbuf sb = STRBUF_INIT;
> >> >> +			char *str;
> >> >> +
> >> >> +			strbuf_addstr(&sb, argv[i]);
> >> >> +			if (starts_with(argv[i], prefix)) {
> >> >> +				have_prefix = 1;
> >> >> +				strbuf_remove(&sb, 0, strlen(prefix));
> >> >> +			}
> >> >> +			str = strbuf_detach(&sb, NULL);
> >> >> +			strbuf_release(&sb);
> >> >> +
> >> >> +			argv_array_push(&args, str);
> >> >> +		}
> >> >> +
> >> >> +		if (!have_prefix)
> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> >> >> +		if (argc > 1)
> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
> >> >> +	}
> >> >>
> >> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
> >> >>  		die(_("--name-only does not make sense"));
> >> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	if (!use_patch_format &&
> >> >>  		(!rev.diffopt.output_format ||
> >> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> >> >> -		/* Needs to be mirrored in show_range_diff() invocation */
> >> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
> >> >>  	if (!rev.diffopt.stat_width)
> >> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
> >> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >> >>  	if (cover_letter) {
> >> >>  		if (thread)
> >> >>  			gen_message_id(&rev, "cover");
> >> >> -		make_cover_letter(&rev, use_stdout,
> >> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
> >> >>  				  origin, nr, list, branch_name, quiet);
> >> >>  		print_bases(&bases, rev.diffopt.file);
> >> >>  		print_signature(rev.diffopt.file);
> >> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> >> >> index bc5facc1cd..6916103888 100755
> >> >> --- a/t/t3206-range-diff.sh
> >> >> +++ b/t/t3206-range-diff.sh
> >> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
> >> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >> >>  	sed -e "s|:$||" >expect <<-\EOF &&
> >> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
> >> >> +	    @@ -8,7 +8,7 @@
> >> >> +	     @@
> >> >> +	      9
> >> >> +	      10
> >> >> +	    - B
> >> >> +	    + BB
> >> >> +	     -12
> >> >> +	     +B
> >> >> +	      13
> >> >> +	-- :
> >> >> +	EOF
> >> >> +	test_cmp expect actual.range-diff &&
> >> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >> >> +	--- a/file
> >> >> +	+++ b/file
> >> >> +	@@ -12 +12 @@ BB
> >> >> +	-12
> >> >> +	+B
> >> >> +	-- :
> >> >> +	EOF
> >> >> +	test_cmp expect actual.diff &&
> >> >> +
> >> >> +	# -U0 & --range-diff-U0
> >> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
> >> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
> >> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
> >> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
> >> >>  	    @@ -11 +11 @@
> >> >>  	    - B
> >> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
> >> >>  	test_cmp expect actual.diff
> >> >>  '
> >> >>
> >> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
> >> >> +	test_must_fail git format-patch --stdout --unknown \
> >> >> +		master..unmodified 2>stderr &&
> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
> >> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
> >> >> +		master..unmodified 2>stderr &&
> >> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
> >> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
> >> >> +		master..unmodified 2>stderr &&
> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
> >> >> +'
> >> >> +
> >> >>  test_done
> >> >> --
> >> >> 2.20.0.rc1.387.gf8505762e3
> >> >>
> >> >>
> >>
> 
Ævar Arnfjörð Bjarmason· Nov 29, 2018, 16:03 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

On Thu, Nov 29 2018, Johannes Schindelin wrote:
Show 49 quoted lines
> Hi Ævar,
>
> On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>>
>> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>> >>
>> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>> >> >
>> >> >> Change the semantics of the "--range-diff" option so that the regular
>> >> >> diff options can be provided separately for the range-diff and the
>> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
>> >> >> "format-patch" to provide different context for the range-diff and the
>> >> >> patch. This wasn't possible before.
>> >> >
>> >> > I really, really dislike the `--range-diff-<random-thing>`. We have
>> >> > precedent for passing optional arguments that are passed to some other
>> >> > command, so a much more logical and consistent convention would be to use
>> >> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
>> >> > you might want to pass to the outer diff in one go rather than having a
>> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
>> >>
>> >> Where do we pass those sorts of arguments?
>> >>
>> >> Reasons I did it this way:
>> >>
>> >>  a) Passing it as one option will require the user to double-quote those
>> >>     options that take quoted arguments (e.g. --word-diff-regex), which I
>> >>     thought sucked more than the prefix. On the implementation side we
>> >>     couldn't leave the parsing of the command-line to the shell anymore.
>> >>
>> >>  b) I think people will want to tweak this very rarely, much more rarely
>> >>     than e.g. -U10 in format-patch itself, so having something long-ish
>> >>     doesn't sound bad.
>> >
>> > Hmm. I still don't like it. It sets a precedent, and we simply do not do
>> > it that way in other circumstances (most obvious would be the -X merge
>> > options). The more divergent user interfaces for the same sort of thing
>> > are, the more brain cycles you force users to spend on navigating said
>> > interfaces.
>>
>> Yeah it sucks, I just think it sucks less than the alternative :)
>> I.e. I'm not picky about --range-diff-* prefix the name, but I think
>> doing our own shell parsing would be nasty.
>
> What prevents you from using `sq_dequote_to_argv()`?

I mean not just nasty in terms of implementation, yeah we could do it, but also a nasty UX for things like --word-diff-regex. I.e. instead of:

    --range-diff-word-diff-regex='[0-9"]'
You need:
    --range-diff-opts="--word-diff-regex='[0-9\"]'"

Now admittedly that in itself isn't very painful *in this case*, but in terms of precedent I really dislike that option, i.e. git having some mode where I need to work to escape input to pass to another command.

Not saying that this --range-diff-* thing is what we should go for, but surely we can find some way to do deal with this that doesn't involve the user needing to escape stuff like this.

It also has other downstream effects in the UI, e.g. it's presumably easy to teach the bash completion that a --foo=XYZ option is also called --some-prefix--foo=XYZ and to enable completion for that, less so for making it smart enough to complete "--some-prefix-opts="--foo=<TAB>".

Show 251 quoted lines
>> >> > I only had time to skim the patch, and I have to wonder why you pass
>> >> > around full-blown `rev_info` structs for range diff (and with that really
>> >> > awful name `rd_rev`) rather than just the `diff_options` that you
>> >> > *actually* care about?
>> >>
>> >> Because setup_revisions() which does all the command-line parsing needs
>> >> a rev_info, so even if we only need the diffopt in the end we need to
>> >> initiate the whole thing.
>> >>
>> >> Suggestions for a better varibale name most welcome.
>> >
>> > `range_diff_revs`
>> >
>> > And you do not need to pass around the whole thing. You can easily pass
>> > `&range_diff_revs.diffopt`.
>> >
>> > Don't pass around what you do not need to pass around.
>>
>> Ah, you mean internally in log.c, yes that makes sense. I thought you
>> meant just pass diffopt to setup_revisions() (which needs the containing
>> struct). Willdo.
>
> Thanks,
> Dscho
>
>>
>> > Ciao,
>> > Dscho
>> >
>> >>
>> >> > Ciao,
>> >> > Dscho
>> >> >
>> >> >>
>> >> >> Ever since the "--range-diff" option was added in
>> >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
>> >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
>> >> >> machinery has been the one we get from "format-patch"'s own
>> >> >> setup_revisions().
>> >> >>
>> >> >> This sort of thing is unique among the log-like commands in
>> >> >> builtin/log.c, no command than format-patch will embed the output of
>> >> >> another log-like command. Since the "rev->diffopt" is reused we need
>> >> >> to munge it before we pass it to show_range_diff(). See
>> >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
>> >> >> output", 2018-11-22) for a related regression fix which is being
>> >> >> mostly reverted here.
>> >> >>
>> >> >> Implementation notes: 1) We're not bothering with the full teardown
>> >> >> around die() and will leak memory, but it's too much boilerplate to do
>> >> >> all the frees with/without the die() and not worth it. 2) We call
>> >> >> repo_init_revisions() for "rd_rev" even though we could get away with
>> >> >> a shallow copy like the code we're replacing (and which
>> >> >> show_range_diff() itself does). This is to make this code more easily
>> >> >> understood.
>> >> >>
>> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >> >> ---
>> >> >>  Documentation/git-format-patch.txt | 10 ++++++-
>> >> >>  builtin/log.c                      | 42 +++++++++++++++++++++++-------
>> >> >>  t/t3206-range-diff.sh              | 41 +++++++++++++++++++++++++++++
>> >> >>  3 files changed, 82 insertions(+), 11 deletions(-)
>> >> >>
>> >> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> >> >> index aba4c5febe..6c048f415f 100644
>> >> >> --- a/Documentation/git-format-patch.txt
>> >> >> +++ b/Documentation/git-format-patch.txt
>> >> >> @@ -24,7 +24,8 @@ SYNOPSIS
>> >> >>  		   [--to=<email>] [--cc=<email>]
>> >> >>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>> >> >>  		   [--interdiff=<previous>]
>> >> >> -		   [--range-diff=<previous> [--creation-factor=<percent>]]
>> >> >> +		   [--range-diff=<previous> [--creation-factor=<percent>]
>> >> >> +		      [--range-diff<common diff option>]]
>> >> >>  		   [--progress]
>> >> >>  		   [<common diff options>]
>> >> >>  		   [ <since> | <revision range> ]
>> >> >> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
>> >> >>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
>> >> >>  	for details.
>> >> >>
>> >> >> +--range-diff<common diff option>::
>> >> >> +	Other options prefixed with `--range-diff` are stripped of
>> >> >> +	that prefix and passed as-is to the diff machinery used to
>> >> >> +	generate the range-diff, e.g. `--range-diff-U0` and
>> >> >> +	`--range-diff--no-color`. This allows for adjusting the format
>> >> >> +	of the range-diff independently from the patch itself.
>> >> >> +
>> >> >>  --notes[=<ref>]::
>> >> >>  	Append the notes (see linkgit:git-notes[1]) for the commit
>> >> >>  	after the three-dash line.
>> >> >> diff --git a/builtin/log.c b/builtin/log.c
>> >> >> index 02d88fa233..7658e56ecc 100644
>> >> >> --- a/builtin/log.c
>> >> >> +++ b/builtin/log.c
>> >> >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
>> >> >>  	fprintf(rev->diffopt.file, "\n");
>> >> >>  }
>> >> >>
>> >> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >> >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
>> >> >> +			      int use_stdout,
>> >> >>  			      struct commit *origin,
>> >> >>  			      int nr, struct commit **list,
>> >> >>  			      const char *branch_name,
>> >> >> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> >> >>  	}
>> >> >>
>> >> >>  	if (rev->rdiff1) {
>> >> >> -		struct diff_options opts;
>> >> >> -		memcpy(&opts, &rev->diffopt, sizeof(opts));
>> >> >> -		opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>> >> >> -
>> >> >>  		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> >> >>  		show_range_diff(rev->rdiff1, rev->rdiff2,
>> >> >> -				rev->creation_factor, 1, &opts);
>> >> >> +				rev->creation_factor, 1, &rd_rev->diffopt);
>> >> >>  	}
>> >> >>  }
>> >> >>
>> >> >> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	struct commit *commit;
>> >> >>  	struct commit **list = NULL;
>> >> >>  	struct rev_info rev;
>> >> >> +	struct rev_info rd_rev;
>> >> >>  	struct setup_revision_opt s_r_opt;
>> >> >>  	int nr = 0, total, i;
>> >> >>  	int use_stdout = 0;
>> >> >> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	init_log_defaults();
>> >> >>  	git_config(git_format_config, NULL);
>> >> >>  	repo_init_revisions(the_repository, &rev, prefix);
>> >> >> +	repo_init_revisions(the_repository, &rd_rev, prefix);
>> >> >>  	rev.commit_format = CMIT_FMT_EMAIL;
>> >> >>  	rev.expand_tabs_in_log_default = 0;
>> >> >>  	rev.verbose_header = 1;
>> >> >> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	rev.preserve_subject = keep_subject;
>> >> >>
>> >> >>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
>> >> >> -	if (argc > 1)
>> >> >> -		die(_("unrecognized argument: %s"), argv[1]);
>> >> >> +	if (argc > 1) {
>> >> >> +		struct argv_array args = ARGV_ARRAY_INIT;
>> >> >> +		const char *prefix = "--range-diff";
>> >> >> +		int have_prefix = 0;
>> >> >> +
>> >> >> +		for (i = 0; i < argc; i++) {
>> >> >> +			struct strbuf sb = STRBUF_INIT;
>> >> >> +			char *str;
>> >> >> +
>> >> >> +			strbuf_addstr(&sb, argv[i]);
>> >> >> +			if (starts_with(argv[i], prefix)) {
>> >> >> +				have_prefix = 1;
>> >> >> +				strbuf_remove(&sb, 0, strlen(prefix));
>> >> >> +			}
>> >> >> +			str = strbuf_detach(&sb, NULL);
>> >> >> +			strbuf_release(&sb);
>> >> >> +
>> >> >> +			argv_array_push(&args, str);
>> >> >> +		}
>> >> >> +
>> >> >> +		if (!have_prefix)
>> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> >> +		argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
>> >> >> +		if (argc > 1)
>> >> >> +			die(_("unrecognized argument: %s"), argv[1]);
>> >> >> +	}
>> >> >>
>> >> >>  	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
>> >> >>  		die(_("--name-only does not make sense"));
>> >> >> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	if (!use_patch_format &&
>> >> >>  		(!rev.diffopt.output_format ||
>> >> >>  		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
>> >> >> -		/* Needs to be mirrored in show_range_diff() invocation */
>> >> >>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
>> >> >>  	if (!rev.diffopt.stat_width)
>> >> >>  		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
>> >> >> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>> >> >>  	if (cover_letter) {
>> >> >>  		if (thread)
>> >> >>  			gen_message_id(&rev, "cover");
>> >> >> -		make_cover_letter(&rev, use_stdout,
>> >> >> +		make_cover_letter(&rev, &rd_rev, use_stdout,
>> >> >>  				  origin, nr, list, branch_name, quiet);
>> >> >>  		print_bases(&bases, rev.diffopt.file);
>> >> >>  		print_signature(rev.diffopt.file);
>> >> >> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> >> >> index bc5facc1cd..6916103888 100755
>> >> >> --- a/t/t3206-range-diff.sh
>> >> >> +++ b/t/t3206-range-diff.sh
>> >> >> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
>> >> >>  		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >> >>  	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >> >>  	sed -e "s|:$||" >expect <<-\EOF &&
>> >> >> +	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >> >> +	    @@ -8,7 +8,7 @@
>> >> >> +	     @@
>> >> >> +	      9
>> >> >> +	      10
>> >> >> +	    - B
>> >> >> +	    + BB
>> >> >> +	     -12
>> >> >> +	     +B
>> >> >> +	      13
>> >> >> +	-- :
>> >> >> +	EOF
>> >> >> +	test_cmp expect actual.range-diff &&
>> >> >> +	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
>> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >> >> +	--- a/file
>> >> >> +	+++ b/file
>> >> >> +	@@ -12 +12 @@ BB
>> >> >> +	-12
>> >> >> +	+B
>> >> >> +	-- :
>> >> >> +	EOF
>> >> >> +	test_cmp expect actual.diff &&
>> >> >> +
>> >> >> +	# -U0 & --range-diff-U0
>> >> >> +	git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
>> >> >> +		--range-diff=topic~..topic changed~..changed >actual.raw &&
>> >> >> +	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
>> >> >> +	sed -e "s|:$||" >expect <<-\EOF &&
>> >> >>  	1:  a63e992 ! 1:  d966c5c s/12/B/
>> >> >>  	    @@ -11 +11 @@
>> >> >>  	    - B
>> >> >> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
>> >> >>  	test_cmp expect actual.diff
>> >> >>  '
>> >> >>
>> >> >> +test_expect_success 'format-patch option parsing with --range-diff-*' '
>> >> >> +	test_must_fail git format-patch --stdout --unknown \
>> >> >> +		master..unmodified 2>stderr &&
>> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr &&
>> >> >> +	test_must_fail git format-patch --stdout --range-diff-unknown \
>> >> >> +		master..unmodified 2>stderr &&
>> >> >> +	test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
>> >> >> +	test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
>> >> >> +		master..unmodified 2>stderr &&
>> >> >> +	test_i18ngrep "unrecognized argument: --unknown" stderr
>> >> >> +'
>> >> >> +
>> >> >>  test_done
>> >> >> --
>> >> >> 2.20.0.rc1.387.gf8505762e3
>> >> >>
>> >> >>
>> >>
>>
Johannes Schindelin· Nov 29, 2018, 19:03 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

Hi Ævar,
On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 58 quoted lines
> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> 
> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> >>
> >> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >> >
> >> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> >> >>
> >> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >> >> >
> >> >> >> Change the semantics of the "--range-diff" option so that the regular
> >> >> >> diff options can be provided separately for the range-diff and the
> >> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> >> >> "format-patch" to provide different context for the range-diff and the
> >> >> >> patch. This wasn't possible before.
> >> >> >
> >> >> > I really, really dislike the `--range-diff-<random-thing>`. We have
> >> >> > precedent for passing optional arguments that are passed to some other
> >> >> > command, so a much more logical and consistent convention would be to use
> >> >> > `--range-diff[=<diff-option>..]`, allowing all of the diff options that
> >> >> > you might want to pass to the outer diff in one go rather than having a
> >> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
> >> >>
> >> >> Where do we pass those sorts of arguments?
> >> >>
> >> >> Reasons I did it this way:
> >> >>
> >> >>  a) Passing it as one option will require the user to double-quote those
> >> >>     options that take quoted arguments (e.g. --word-diff-regex), which I
> >> >>     thought sucked more than the prefix. On the implementation side we
> >> >>     couldn't leave the parsing of the command-line to the shell anymore.
> >> >>
> >> >>  b) I think people will want to tweak this very rarely, much more rarely
> >> >>     than e.g. -U10 in format-patch itself, so having something long-ish
> >> >>     doesn't sound bad.
> >> >
> >> > Hmm. I still don't like it. It sets a precedent, and we simply do not do
> >> > it that way in other circumstances (most obvious would be the -X merge
> >> > options). The more divergent user interfaces for the same sort of thing
> >> > are, the more brain cycles you force users to spend on navigating said
> >> > interfaces.
> >>
> >> Yeah it sucks, I just think it sucks less than the alternative :)
> >> I.e. I'm not picky about --range-diff-* prefix the name, but I think
> >> doing our own shell parsing would be nasty.
> >
> > What prevents you from using `sq_dequote_to_argv()`?
> 
> I mean not just nasty in terms of implementation, yeah we could do it,
> but also a nasty UX for things like --word-diff-regex. I.e. instead of:
> 
>     --range-diff-word-diff-regex='[0-9"]'
> 
> You need:
> 
>     --range-diff-opts="--word-diff-regex='[0-9\"]'"

Really? I think that would not work. It would pass the single quotes as part of the regex to the diff machinery.

Or maybe not. But the extra quotes do not strike me as necessary, as there is no shell script involved (thank deity!) after `git range-diff` parsed the options.

Show 12 quoted lines
> Now admittedly that in itself isn't very painful *in this case*, but in
> terms of precedent I really dislike that option, i.e. git having some
> mode where I need to work to escape input to pass to another command.
> 
> Not saying that this --range-diff-* thing is what we should go for, but
> surely we can find some way to do deal with this that doesn't involve
> the user needing to escape stuff like this.
> 
> It also has other downstream effects in the UI, e.g. it's presumably
> easy to teach the bash completion that a --foo=XYZ option is also called
> --some-prefix--foo=XYZ and to enable completion for that, less so for
> making it smart enough to complete "--some-prefix-opts="--foo=<TAB>".
These are all good points, and need proper discussion.

Sadly, all that time needed for a proper discussion is not left before v2.20.0 is supposed to come out.

Quite honestly, I think what we will have to do is to describe in the documentation of `format-patch`'s `--range-diff` option that the exact user interface how to pass diff options down to `range-diff` is in flux and not final.

That way, we can give your design the proper treatment, and work together on making a user interface we all can be happy with.

Ciao, Dscho

Junio C Hamano· Nov 30, 2018, 02:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 14 quoted lines
>> What prevents you from using `sq_dequote_to_argv()`?
>
> I mean not just nasty in terms of implementation, yeah we could do it,
> but also a nasty UX for things like --word-diff-regex. I.e. instead of:
>
>     --range-diff-word-diff-regex='[0-9"]'
>
> You need:
>
>     --range-diff-opts="--word-diff-regex='[0-9\"]'"
>
> Now admittedly that in itself isn't very painful *in this case*, but in
> terms of precedent I really dislike that option, i.e. git having some
> mode where I need to work to escape input to pass to another command.

In addition, sq_dequote are meant to be used on quoted string we internally produce; I do not think we want to promise that it is safe to use on a random string that comes from end users.

In any case, I tend to agree with the conclusion in the downthread by Dscho that we should just clearly mark that invocations of the "format-patch --range-diff" command with additional diff options is an experimental feature that may not do anything sensible in the upcoming release, and declare that the UI to pass diff options to affect only the range-diff part may later be invented. IOW, I am coming a bit stronger than Dscho's suggestion in that we should not even pretend that we aimed to make the options used for range-diff customizable when driven from format-patch in the upcoming release, or aimed to make --range-diff option compatible with other diff options given to the format-patch command.

I had to delay -rc2 to see these last minute tweaks come to some reasonable place to stop at, and I do not think we want to delay the final any longer or destablizing it further by piling last minute undercooked changes on top.

Junio C Hamano· Nov 30, 2018, 04:27 UTC · re: Junio C Hamano · lore

[PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> In any case, I tend to agree with the conclusion in the downthread
> by Dscho that we should just clearly mark that invocations of the
> "format-patch --range-diff" command with additional diff options is
> an experimental feature that may not do anything sensible in the
> upcoming release, and declare that the UI to pass diff options to
> affect only the range-diff part may later be invented.  IOW, I am
> coming a bit stronger than Dscho's suggestion in that we should not
> even pretend that we aimed to make the options used for range-diff
> customizable when driven from format-patch in the upcoming release,
> or aimed to make --range-diff option compatible with other diff
> options given to the format-patch command.
>
> I had to delay -rc2 to see these last minute tweaks come to some
> reasonable place to stop at, and I do not think we want to delay the
> final any longer or destablizing it further by piling last minute
> undercooked changes on top.

So how about doing this on top of 'master' instead? As this leaks *no* information wrt how range-diff machinery should behave from the format-patch side by not passing any diffopt, as long as the new code I added to show_range_diff() comes up with a reasonable default diffopts (for which I really would appreciate extra sets of eyes to make sure), this change by definition cannot be wrong (famous last words).

-- >8 --
Subject: format-patch: do not let its diff-options affect --range-diff

Stop leaking how the primary output of format-patch is customized to the range-diff machinery and instead let the latter use its own "reasonable default", in order to correct the breakage introduced by a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18) on the 'master' front. "git format-patch --range-diff..." without any weird diff option started to include the "range-diff --stat" output, which is rather useless right now, that made the whole thing unusable and this is probably the least disruptive way to whip the codebase into a shippable shape.

We may want to later make the range-diff driven by format-patch more configurable, but that would have to wait until we have a good design.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt | 5 +++++
 builtin/log.c                      | 2 +-
 log-tree.c                         | 2 +-
 range-diff.c                       | 6 +++++-
 range-diff.h                       | 5 +++++
 5 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index aba4c5febe..27304428a1 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -250,6 +250,11 @@ feeding the result to `git send-email`.
 	feature/v2`), or a revision range if the two versions of the series are
 	disjoint (for example `git format-patch --cover-letter
 	--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
++
+Note that diff options passed to the command affect how the primary
+product of `format-patch` is generated, and they are not passed to
+the underlying `range-diff` machinery used to generate the cover-letter
+material (this may change in the future).
 
 --creation-factor=<percent>::
 	Used with `--range-diff`, tweak the heuristic which matches up commits
diff --git a/builtin/log.c b/builtin/log.c
index 0fe6f9ba1e..5ac18e2848 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (rev->rdiff1) {
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &rev->diffopt);
+				rev->creation_factor, 1, NULL);
 	}
 }
 
diff --git a/log-tree.c b/log-tree.c
index 7a83e99250..b243779a0b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -762,7 +762,7 @@ void show_log(struct rev_info *opt)
 		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, &opt->diffopt);
+				opt->creation_factor, 1, NULL);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
diff --git a/range-diff.c b/range-diff.c
index 767af8c5bb..8e52a85c19 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -460,7 +460,11 @@ int show_range_diff(const char *range1, const char *range2,
 		struct diff_options opts;
 		struct strbuf indent = STRBUF_INIT;
 
-		memcpy(&opts, diffopt, sizeof(opts));
+		if (diffopt)
+			memcpy(&opts, diffopt, sizeof(opts));
+		else
+			repo_diff_setup(the_repository, &opts);
+
 		if (!opts.output_format)
 			opts.output_format = DIFF_FORMAT_PATCH;
 		opts.flags.suppress_diff_headers = 1;
diff --git a/range-diff.h b/range-diff.h
index 190593f0c7..08a50b6e98 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -5,6 +5,11 @@
 
 #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
 
+/*
+ * Compare series of commmits in RANGE1 and RANGE2, and emit to the
+ * standard output.  NULL can be passed to DIFFOPT to use the built-in
+ * default.
+ */
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
 		    struct diff_options *diffopt);
Junio C Hamano· Nov 30, 2018, 08:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
>> I had to delay -rc2 to see these last minute tweaks come to some
>> reasonable place to stop at, and I do not think we want to delay the
>> final any longer or destablizing it further by piling last minute
>> undercooked changes on top.
>
> So how about doing this on top of 'master' instead?  As this leaks
> *no* information wrt how range-diff machinery should behave from the
> format-patch side by not passing any diffopt, as long as the new
> code I added to show_range_diff() comes up with a reasonable default
> diffopts (for which I really would appreciate extra sets of eyes to
> make sure), this change by definition cannot be wrong (famous last
> words).

As listed in today's "What's cooking" report, I've merged this to 'next' in today's pushout and planning to have it in the -rc2. I am not married to this exact implementation, and I'd welcome to have an even simpler and less disruptive solution if exists, but I am hoping that this is a good-enough interim measure for the upcoming release, until we decide what to do with the customizability of range-diff driven by format-patch.

In addition to this, I am planning the "rebase --stat" and "reflog that does not say 'rebase -i' but 'rebase'" fixes merged to 'master' before cutting -rc2.

Ævar Arnfjörð Bjarmason· Nov 30, 2018, 09:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)

On Fri, Nov 30 2018, Junio C Hamano wrote:
Show 26 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> I had to delay -rc2 to see these last minute tweaks come to some
>>> reasonable place to stop at, and I do not think we want to delay the
>>> final any longer or destablizing it further by piling last minute
>>> undercooked changes on top.
>>
>> So how about doing this on top of 'master' instead?  As this leaks
>> *no* information wrt how range-diff machinery should behave from the
>> format-patch side by not passing any diffopt, as long as the new
>> code I added to show_range_diff() comes up with a reasonable default
>> diffopts (for which I really would appreciate extra sets of eyes to
>> make sure), this change by definition cannot be wrong (famous last
>> words).
>
> As listed in today's "What's cooking" report, I've merged this to
> 'next' in today's pushout and planning to have it in the -rc2.  I am
> not married to this exact implementation, and I'd welcome to have an
> even simpler and less disruptive solution if exists, but I am hoping
> that this is a good-enough interim measure for the upcoming release,
> until we decide what to do with the customizability of range-diff
> driven by format-patch.
>
> In addition to this, I am planning the "rebase --stat" and "reflog
> that does not say 'rebase -i' but 'rebase'" fixes merged to 'master'
> before cutting -rc2.
Thanks a lot, yeah having this wait looks good to me.
Johannes Schindelin· Nov 30, 2018, 12:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)

Hi Junio,
On Fri, 30 Nov 2018, Junio C Hamano wrote:
Show 26 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> I had to delay -rc2 to see these last minute tweaks come to some
> >> reasonable place to stop at, and I do not think we want to delay the
> >> final any longer or destablizing it further by piling last minute
> >> undercooked changes on top.
> >
> > So how about doing this on top of 'master' instead?  As this leaks
> > *no* information wrt how range-diff machinery should behave from the
> > format-patch side by not passing any diffopt, as long as the new
> > code I added to show_range_diff() comes up with a reasonable default
> > diffopts (for which I really would appreciate extra sets of eyes to
> > make sure), this change by definition cannot be wrong (famous last
> > words).
> 
> As listed in today's "What's cooking" report, I've merged this to
> 'next' in today's pushout and planning to have it in the -rc2.  I am
> not married to this exact implementation, and I'd welcome to have an
> even simpler and less disruptive solution if exists, but I am hoping
> that this is a good-enough interim measure for the upcoming release,
> until we decide what to do with the customizability of range-diff
> driven by format-patch.
> 
> In addition to this, I am planning the "rebase --stat" and "reflog
> that does not say 'rebase -i' but 'rebase'" fixes merged to 'master'
> before cutting -rc2.

Thank you for integrating them. That way, we have an -rc2 with no issues in the built-in rebase/rebase -i that we know of.

Ciao, Dscho

Eric Sunshine· Nov 30, 2018, 09:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)

On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> > In any case, I tend to agree with the conclusion in the downthread
> > by Dscho that we should just clearly mark that invocations of the
> > "format-patch --range-diff" command with additional diff options is
> > an experimental feature that may not do anything sensible in the
> > upcoming release, and declare that the UI to pass diff options to
> > affect only the range-diff part may later be invented.  IOW, I am
> > coming a bit stronger than Dscho's suggestion in that we should not
> > even pretend that we aimed to make the options used for range-diff
> > customizable when driven from format-patch in the upcoming release,
> > or aimed to make --range-diff option compatible with other diff
> > options given to the format-patch command.

I agree that this way forward makes sense. It's clear that I overlooked how there could be unexpected interactions from passing git-format-patch's own diff_options to show_range_diff(), so taking time to think it through without the pressure of a looming release is preferable to rushing out some "fixes".

Show 7 quoted lines
> So how about doing this on top of 'master' instead?  As this leaks
> *no* information wrt how range-diff machinery should behave from the
> format-patch side by not passing any diffopt, as long as the new
> code I added to show_range_diff() comes up with a reasonable default
> diffopts (for which I really would appreciate extra sets of eyes to
> make sure), this change by definition cannot be wrong (famous last
> words).

I, myself, was going to suggest this approach of leaking none of the git-format-patch's options into range_diff(), so I think it is a good one. Later, we can selectively pass certain _sensible_ options into show_range_diff() once we figure out the correct UI (for instance, --range-diff-opts=nopatch,creation-factor:60).

A couple comments on the patch itself...
Show 7 quoted lines
> diff --git a/range-diff.c b/range-diff.c
> @@ -460,7 +460,11 @@ int show_range_diff(const char *range1, const char *range2,
> -               memcpy(&opts, diffopt, sizeof(opts));
> +               if (diffopt)
> +                       memcpy(&opts, diffopt, sizeof(opts));
> +               else
> +                       repo_diff_setup(the_repository, &opts);

The first attempt at adding --range-diff to git-format-patch invoked the git-range-diff command, so no diff_options were passed at all. After Dscho libified the range-diff machinery in one of his major re-rolls, I took advantage of that to avoid the subprocess invocation. Another benefit of calling show_range_diff() directly is that when "git format-patch --stdout --range-diff=..." is sent to the terminal, the range-diff gets colored output for free. I'm pleased to see that that still works after this change.

Show 7 quoted lines
> diff --git a/range-diff.h b/range-diff.h
> @@ -5,6 +5,11 @@
> +/*
> + * Compare series of commmits in RANGE1 and RANGE2, and emit to the
> + * standard output.  NULL can be passed to DIFFOPT to use the built-in
> + * default.
> + */

It is more correct to say that the range-diff is emitted to diffopt->file (which may be stdout).

Thanks for working on this.
Martin Ågren· Dec 3, 2018, 13:27 UTC · re: Eric Sunshine · lore

Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)

On Fri, 30 Nov 2018 at 10:32, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 10 quoted lines
>
> On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> > So how about doing this on top of 'master' instead?  As this leaks
> > *no* information wrt how range-diff machinery should behave from the
> > format-patch side by not passing any diffopt, as long as the new
> > code I added to show_range_diff() comes up with a reasonable default
> > diffopts (for which I really would appreciate extra sets of eyes to
> > make sure), this change by definition cannot be wrong (famous last
> > words).
> Another benefit of calling show_range_diff() directly is that when
> "git format-patch --stdout --range-diff=..." is sent to the terminal,
> the range-diff gets colored output for free. I'm pleased to see that
> that still works after this change.

(If the patch below makes any sense to you and you know more about this diff/color thing, the fourth bullet in the log message below might interest you.)

Show 10 quoted lines
> > diff --git a/range-diff.h b/range-diff.h
> > @@ -5,6 +5,11 @@
> > +/*
> > + * Compare series of commmits in RANGE1 and RANGE2, and emit to the
> > + * standard output.  NULL can be passed to DIFFOPT to use the built-in
> > + * default.
> > + */
>
> It is more correct to say that the range-diff is emitted to
> diffopt->file (which may be stdout).

This seems to be an important remark. There's a pretty bad regression here since when `diffopt` is NULL, we've lost our original, intended `diffopt->file` and the range-diff ends up on stdout.

Here's my whitespace-damaged WIP. I would be able to pick this up again in about 6h, but anyone is more than welcome to pick this up and run with it in the meantime.

This is not a corner of the code that I'm particularly familiar with...
-->8--
Subject: [PATCH] range-diff: always pass at least minimal diff options
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit d8981c3f88 ("format-patch: do not let its diff-options affect --range-diff", 2018-11-30) taught `show_range_diff()` to accept a NULL-pointer as an indication that it should use its own "reasonable default". That fixed a regression from a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced a regression of its own.

In particular, it means we forget the `file` member of the diff options, so rather than placing a range-diff in the cover-letter, we write it to stdout. In order to fix this, rewrite the two callers adjusted by d8981c3f88 to create a "dummy" set of diff options where they only fill in which file to use.

A couple of remarks about this commit:
  * No tests. The change in builtin/log.c has been tested manually, the
    one in log-tree.c not at all, other than by running existing tests.
  * I have not convinced myself 100% that there aren't other things that
    are just as important as `file` to pass down.
  * `show_range_diff()` can still take NULL, although that is now dead
    code. If something like this here commit is deemed the proper fix
    for this, that code path could also go, either as part of this
    commit, or separately, once we've cut 2.20.
  * The range-diff is written colored regardless of destination, i.e.,
    when written to a file, it contains garbage.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/log.c | 12 +++++++++++-
 log-tree.c    | 12 +++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..0609e41ae5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,19 @@ static void make_cover_letter(struct rev_info
*rev, int use_stdout,
     }

     if (rev->rdiff1) {
+        /**
+         * (At least for now) we only want to pass down
+         * the file handle where we want the range-diff
+         * to appear. Avoid any other diff options until
+         * we know how we want to handle them.
+         */
+        struct diff_options opts;
+        diff_setup(&opts);
+        opts.file = rev->diffopt.file;
+        diff_setup_done(&opts);
         fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
         show_range_diff(rev->rdiff1, rev->rdiff2,
-                rev->creation_factor, 1, NULL);
+                rev->creation_factor, 1, &opts);
     }
 }

diff --git a/log-tree.c b/log-tree.c
index b243779a0b..bc355a4e91 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,24 @@ void show_log(struct rev_info *opt)

     if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
         struct diff_queue_struct dq;
+        struct diff_options opts;

         memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
         DIFF_QUEUE_CLEAR(&diff_queued_diff);

         next_commentary_block(opt, NULL);
         fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+        /**
+         * (At least for now) we only want to pass down
+         * the file handle where we want the range-diff
+         * to appear. Avoid any other diff options until
+         * we know how we want to handle them.
+        */
+        diff_setup(&opts);
+        opts.file = opt->diffopt.file;
+        diff_setup_done(&opts);
         show_range_diff(opt->rdiff1, opt->rdiff2,
-                opt->creation_factor, 1, NULL);
+                opt->creation_factor, 1, &opts);

         memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
     }
-- 
2.20.0.rc2
Martin Ågren· Dec 3, 2018, 20:07 UTC · re: Martin Ågren · lore

[PATCH v2] range-diff: always pass at least minimal diff options

Commit d8981c3f88 ("format-patch: do not let its diff-options affect --range-diff", 2018-11-30) taught `show_range_diff()` to accept a NULL-pointer as an indication that it should use its own "reasonable default". That fixed a regression from a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced a regression of its own.

In particular, it means we forget the `file` member of the diff options, so rather than placing a range-diff in the cover-letter, we write it to stdout. In order to fix this, rewrite the two callers adjusted by d8981c3f88 to instead create a "dummy" set of diff options where they only fill in which file to use.

Plus, turn off coloring to make sure we don't write any color codes. Maybe we could do `opts.use_color = opts.file != stdout`, but for now, I'd much rather always write uncolored output than write color codes where there shouldn't be any.

Modify and extend the existing tests to try and verify that the right contents end up in the right place.

Don't revert `show_range_diff()`, i.e., let it keep accepting NULL. Rather than removing what is dead code and figuring out it isn't actually dead and we've broken 2.20, just leave it for now.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Here's another attempt at fixing this recent regression.
 t/t3206-range-diff.sh | 20 +++++++++++++-------
 builtin/log.c         | 13 ++++++++++++-
 log-tree.c            | 13 ++++++++++++-
 3 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..048feaf6dd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' '
 for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
-		git format-patch --stdout --cover-letter --range-diff=$prev \
+		git format-patch --cover-letter --range-diff=$prev \
 			master..unmodified >actual &&
-		grep "= 1: .* s/5/A" actual &&
-		grep "= 2: .* s/4/A" actual &&
-		grep "= 3: .* s/11/B" actual &&
-		grep "= 4: .* s/12/B" actual
+		test_when_finished "rm 000?-*" &&
+		test_line_count = 5 actual &&
+		test_i18ngrep "^Range-diff:$" 0000-* &&
+		grep "= 1: .* s/5/A" 0000-* &&
+		grep "= 2: .* s/4/A" 0000-* &&
+		grep "= 3: .* s/11/B" 0000-* &&
+		grep "= 4: .* s/12/B" 0000-*
 	'
 done
 
 test_expect_success 'format-patch --range-diff as commentary' '
-	git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
-	test_i18ngrep "^Range-diff:$" actual
+	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
+	test_when_finished "rm 0001-*" &&
+	test_line_count = 1 actual &&
+	test_i18ngrep "^Range-diff:$" 0001-* &&
+	grep "> 1: .* new message" 0001-*
 '
 
 test_done
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..e42487b46d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,20 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
+		/*
+		 * (At least for now) we only want to pass down
+		 * the file handle where we want the range-diff
+		 * to appear. Avoid any other diff options until
+		 * we know how we want to handle them.
+		 */
+		struct diff_options opts;
+		diff_setup(&opts);
+		opts.file = rev->diffopt.file;
+		opts.use_color = 0;
+		diff_setup_done(&opts);
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, NULL);
+				rev->creation_factor, 1, &opts);
 	}
 }
 
diff --git a/log-tree.c b/log-tree.c
index b243779a0b..fd79a3ec37 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,25 @@ void show_log(struct rev_info *opt)
 
 	if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
 		struct diff_queue_struct dq;
+		struct diff_options opts;
 
 		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
 		DIFF_QUEUE_CLEAR(&diff_queued_diff);
 
 		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+		/*
+		 * (At least for now) we only want to pass down
+		 * the file handle where we want the range-diff
+		 * to appear. Avoid any other diff options until
+		 * we know how we want to handle them.
+		 */
+		diff_setup(&opts);
+		opts.file = opt->diffopt.file;
+		opts.use_color = 0;
+		diff_setup_done(&opts);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, NULL);
+				opt->creation_factor, 1, &opts);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
-- 
2.20.0.rc2.1.gfcc5f94f1e
Eric Sunshine· Dec 3, 2018, 21:21 UTC · re: Martin Ågren · lore

[PATCH v3] range-diff: always pass at least minimal diff options

From: Martin Ågren <martin.agren@gmail.com>

Commit d8981c3f88 ("format-patch: do not let its diff-options affect --range-diff", 2018-11-30) taught `show_range_diff()` to accept a NULL-pointer as an indication that it should use its own "reasonable default". That fixed a regression from a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced a regression of its own.

In particular, it means we forget the `file` member of the diff options, so rather than placing a range-diff in the cover-letter, we write it to stdout. In order to fix this, rewrite the two callers adjusted by d8981c3f88 to instead create a "dummy" set of diff options where they only fill in the fields we absolutely require, such as output file and color.

Modify and extend the existing tests to try and verify that the right contents end up in the right place.

Don't revert `show_range_diff()`, i.e., let it keep accepting NULL. Rather than removing what is dead code and figuring out it isn't actually dead and we've broken 2.20, just leave it for now.

[es: retain diff coloring when going to stdout]
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

This is a re-roll of Martin's v2[1]. The only difference from v2 is that it retains coloring when emitting to the terminal (plus an in-code comment was simplified).

The regression introduced by d8981c3f88, in which the range-diff only ever gets emitted to the terminal, and never to the cover letter or commentary section of a standalone patch, makes the --range-diff option rather useless, so this fix probably ought to be fast-tracked. An alternative would be to rip out all the recent "--range-diff"-related changes and go with the --range-diff implementation which has been in use for a few months, even if it is not perfect.

[1]: https://public-inbox.org/git/20181203200734.527341-1-martin.agren@gmail.com/
builtin/log.c         | 11 ++++++++++-
 log-tree.c            | 11 ++++++++++-
 t/t3206-range-diff.sh | 20 +++++++++++++-------
 3 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..e8e51068bd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,18 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
+		/*
+		 * Pass minimum required diff-options to range-diff; others
+		 * can be added later if deemed desirable.
+		 */
+		struct diff_options opts;
+		diff_setup(&opts);
+		opts.file = rev->diffopt.file;
+		opts.use_color = rev->diffopt.use_color;
+		diff_setup_done(&opts);
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, NULL);
+				rev->creation_factor, 1, &opts);
 	}
 }
 
diff --git a/log-tree.c b/log-tree.c
index b243779a0b..10680c139e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,23 @@ void show_log(struct rev_info *opt)
 
 	if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
 		struct diff_queue_struct dq;
+		struct diff_options opts;
 
 		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
 		DIFF_QUEUE_CLEAR(&diff_queued_diff);
 
 		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+		/*
+		 * Pass minimum required diff-options to range-diff; others
+		 * can be added later if deemed desirable.
+		 */
+		diff_setup(&opts);
+		opts.file = opt->diffopt.file;
+		opts.use_color = opt->diffopt.use_color;
+		diff_setup_done(&opts);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, NULL);
+				opt->creation_factor, 1, &opts);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..048feaf6dd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' '
 for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
-		git format-patch --stdout --cover-letter --range-diff=$prev \
+		git format-patch --cover-letter --range-diff=$prev \
 			master..unmodified >actual &&
-		grep "= 1: .* s/5/A" actual &&
-		grep "= 2: .* s/4/A" actual &&
-		grep "= 3: .* s/11/B" actual &&
-		grep "= 4: .* s/12/B" actual
+		test_when_finished "rm 000?-*" &&
+		test_line_count = 5 actual &&
+		test_i18ngrep "^Range-diff:$" 0000-* &&
+		grep "= 1: .* s/5/A" 0000-* &&
+		grep "= 2: .* s/4/A" 0000-* &&
+		grep "= 3: .* s/11/B" 0000-* &&
+		grep "= 4: .* s/12/B" 0000-*
 	'
 done
 
 test_expect_success 'format-patch --range-diff as commentary' '
-	git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
-	test_i18ngrep "^Range-diff:$" actual
+	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
+	test_when_finished "rm 0001-*" &&
+	test_line_count = 1 actual &&
+	test_i18ngrep "^Range-diff:$" 0001-* &&
+	grep "> 1: .* new message" 0001-*
 '
 
 test_done
-- 
2.20.0.rc2.403.gdbc3b29805
Junio C Hamano· Dec 4, 2018, 01:35 UTC · re: Eric Sunshine · lore

Re: [PATCH v3] range-diff: always pass at least minimal diff options

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 8 quoted lines
> This is a re-roll of Martin's v2[1]. The only difference from v2 is that
> it retains coloring when emitting to the terminal (plus an in-code
> comment was simplified).
>
> The regression introduced by d8981c3f88, in which the range-diff only
> ever gets emitted to the terminal, and never to the cover letter or
> commentary section of a standalone patch, makes the --range-diff option
> rather useless, so this fix probably ought to be fast-tracked.

Yup. Thanks. The only thing that makes me wonder is why any of the existing tests (among which I think I saw range-diff driven from format-patch) did not catch this rather obvious glitch.

Show 11 quoted lines
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index e497c1358f..048feaf6dd 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' '
>  for prev in topic master..topic
>  do
>  	test_expect_success "format-patch --range-diff=$prev" '
> -		git format-patch --stdout --cover-letter --range-diff=$prev \
> +		git format-patch --cover-letter --range-diff=$prev \
>  			master..unmodified >actual &&

Ah, of course. Then the "actual" file gets the names of the output files; we expect to see 5 of them.

But now we have lost all the range-diff tests run under "--stdout", so next time some other change regresses only that codepath, we will not notice (which is fine for now but would want to be addressed before the end of the year around which time we certainly will all forget).

Thanks.
Show 25 quoted lines
> -		grep "= 1: .* s/5/A" actual &&
> -		grep "= 2: .* s/4/A" actual &&
> -		grep "= 3: .* s/11/B" actual &&
> -		grep "= 4: .* s/12/B" actual
> +		test_when_finished "rm 000?-*" &&
> +		test_line_count = 5 actual &&
> +		test_i18ngrep "^Range-diff:$" 0000-* &&
> +		grep "= 1: .* s/5/A" 0000-* &&
> +		grep "= 2: .* s/4/A" 0000-* &&
> +		grep "= 3: .* s/11/B" 0000-* &&
> +		grep "= 4: .* s/12/B" 0000-*
>  	'
>  done
>  
>  test_expect_success 'format-patch --range-diff as commentary' '
> -	git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
> -	test_i18ngrep "^Range-diff:$" actual
> +	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
> +	test_when_finished "rm 0001-*" &&
> +	test_line_count = 1 actual &&
> +	test_i18ngrep "^Range-diff:$" 0001-* &&
> +	grep "> 1: .* new message" 0001-*
>  '
>  
>  test_done
Martin Ågren· Dec 4, 2018, 05:40 UTC · re: Eric Sunshine · lore

Re: [PATCH v3] range-diff: always pass at least minimal diff options

On Mon, 3 Dec 2018 at 22:21, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 9 quoted lines
> [es: retain diff coloring when going to stdout]
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> This is a re-roll of Martin's v2[1]. The only difference from v2 is that
> it retains coloring when emitting to the terminal (plus an in-code
> comment was simplified).
Thank you so much for this.
Show 5 quoted lines
>         if (rev->rdiff1) {
> +               /*
> +                * Pass minimum required diff-options to range-diff; others
> +                * can be added later if deemed desirable.
> +                */
Agreed.
> +               struct diff_options opts;
> +               diff_setup(&opts);
> +               opts.file = rev->diffopt.file;
> +               opts.use_color = rev->diffopt.use_color;
Ah, s/0/rev->diffopt.use_color/, well that's obvious.
Thanks!
Martin
Eric Sunshine· Nov 30, 2018, 09:58 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

On Thu, Nov 29, 2018 at 11:03 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 16 quoted lines
> I mean not just nasty in terms of implementation, yeah we could do it,
> but also a nasty UX for things like --word-diff-regex. I.e. instead of:
>
>     --range-diff-word-diff-regex='[0-9"]'
>
> You need:
>
>     --range-diff-opts="--word-diff-regex='[0-9\"]'"
>
> Now admittedly that in itself isn't very painful *in this case*, but in
> terms of precedent I really dislike that option, i.e. git having some
> mode where I need to work to escape input to pass to another command.
>
> Not saying that this --range-diff-* thing is what we should go for, but
> surely we can find some way to do deal with this that doesn't involve
> the user needing to escape stuff like this.

I should mention that it was never the intention that git-format-patch's --range-diff option would allow passing _all_ possible options to git-range-diff, but only those most likely to be tweaked by the user (such as --creation-factor). It was understood from the start (and stated by me either in the cover letter to that series or in discussion) that the user always has the escape hatch of running git-range-diff manually and copy/pasting its output into the cover letter.

So, I'm not convinced that we need this sort of flexibility in git-format-patch's --range-diff option, but we certainly can add convenience options (such as I did with --creation-factor) as people complain that their favorite option is missing. For a UI, I think we already have sufficient precedent for "--range-diff-opts=nopatch,creation-factor:60" as a possibility.

Ævar Arnfjörð Bjarmason· Nov 28, 2018, 20:18 UTC · re: Junio C Hamano · lore

[PATCH 0/2] format-patch: fix root cause of recent regression

As noted in 2/2 this fixes the root cause of the bug I plastered over in https://public-inbox.org/git/20181122211248.24546-3-avarab@gmail.com/ (that patch is sitting in 'next').

1/2 is a test for existing behavior, to make it more easily understood what's being changed.

Junio: I know it's late, but unless Eric has objections to this UI
change I'd really like to have this in 2.20 since this is a change to
a new command-line UI that's newly added in 2.20.

As noted in 2/2 the current implementation is inherently limited, you can't tweak diff options for the range-diff in the cover-letter and the patch independently, now you can, and the implementation is much less nasty now that we're not having to share diffopts across two different modes of operation.

Ævar Arnfjörð Bjarmason (2):
  format-patch: add test for --range-diff diff output
  format-patch: allow for independent diff & range-diff options
 Documentation/git-format-patch.txt |  10 ++-
 builtin/log.c                      |  42 +++++++++---
 t/t3206-range-diff.sh              | 101 +++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 11 deletions(-)
-- 
2.20.0.rc1.387.gf8505762e3
Ævar Arnfjörð Bjarmason· Nov 28, 2018, 20:18 UTC · re: Junio C Hamano · lore

[PATCH 1/2] format-patch: add test for --range-diff diff output

As noted in 43dafc4172 ("format-patch: don't include --stat with --range-diff output", 2018-11-22) the diff options provided on the command-line currently affect both the range-diff and the patch output, but there was no test for checking this with output where we'd show a patch diff. Let's add one.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3206-range-diff.sh | 60 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 90def330bd..bc5facc1cd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -267,4 +267,64 @@ test_expect_success 'format-patch --range-diff as commentary' '
 	test_i18ngrep "^Range-diff:$" actual
 '
 
+test_expect_success 'format-patch with <common diff option>' '
+	# No diff options
+	git format-patch --cover-letter --stdout --range-diff=topic~..topic \
+		changed~..changed >actual.raw &&
+	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
+	1:  a63e992 ! 1:  d966c5c s/12/B/
+	    @@ -8,7 +8,7 @@
+	     @@
+	      9
+	      10
+	    - B
+	    + BB
+	     -12
+	     +B
+	      13
+	-- :
+	EOF
+	test_cmp expect actual.range-diff &&
+	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
+	--- a/file
+	+++ b/file
+	@@ -9,7 +9,7 @@ A
+	 9
+	 10
+	 BB
+	-12
+	+B
+	 13
+	 14
+	 15
+	-- :
+	EOF
+	test_cmp expect actual.diff &&
+
+	# -U0
+	git format-patch --cover-letter --stdout -U0 \
+		--range-diff=topic~..topic changed~..changed >actual.raw &&
+	sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
+	1:  a63e992 ! 1:  d966c5c s/12/B/
+	    @@ -11 +11 @@
+	    - B
+	    + BB
+	-- :
+	EOF
+	test_cmp expect actual.range-diff &&
+	sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
+	sed -e "s|:$||" >expect <<-\EOF &&
+	--- a/file
+	+++ b/file
+	@@ -12 +12 @@ BB
+	-12
+	+B
+	-- :
+	EOF
+	test_cmp expect actual.diff
+'
+
 test_done
-- 
2.20.0.rc1.387.gf8505762e3
Ævar Arnfjörð Bjarmason· Nov 24, 2018, 20:54 UTC · re: Junio C Hamano · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

On Wed, Nov 21 2018, Junio C Hamano wrote:
>  * "git rebase" and "git rebase -i" have been reimplemented in C.

Here's another regression in the C version (and rc1), note: the sha1collisiondetection is just a stand in for "some repo":

    (
        rm -rf /tmp/repo &&
        git init /tmp/repo &&
        cd /tmp/repo &&
        for c in 1 2
        do
            touch $c &&
            git add $c &&
            git commit -m"add $c"
        done &&
        git remote add origin https://github.com/cr-marcstevens/sha1collisiondetection.git &&
        git fetch &&
        git branch --set-upstream-to origin/master &&
        git rebase -i
    )

The C version will die with "fatal: unable to read tree 0000000000000000000000000000000000000000". Running this with rebase.useBuiltin=false does the right thing and rebases as of the merge base of the two (which here is the root of the history).

I wasn't trying to stress test rebase. I was just wanting to rebase a history I was about to force-push after cleaning it up, hardly an obscure use-case. So [repeat last transmission in https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/ ]

Junio C Hamano· Nov 25, 2018, 01:00 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 7 quoted lines
>>  * "git rebase" and "git rebase -i" have been reimplemented in C.
>
> Here's another regression in the C version (and rc1),...
> I wasn't trying to stress test rebase. I was just wanting to rebase a
> history I was about to force-push after cleaning it up, hardly an
> obscure use-case. So [repeat last transmission in
> https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/ ]
which, to those who are reading from sidelines:
    Given that we're still finding regressions bugs in the rebase-in-C
    version should we be considering reverting 5541bd5b8f ("rebase: default
    to using the builtin rebase", 2018-08-08)?
    I love the feature, but fear that the current list of known regressions
    serve as a canary for a larger list which we'd discover if we held off
    for another major release (and would re-enable rebase.useBuiltin=true in
    master right after 2.20 is out the door).

I am fine with the proposed flip, but I'll have to see the extent of damage this late in the game so that I won't miss anything. In addition to the one-liner below, we'd need to update the quoted release notes entry, and possibly adjust some tests (even though the "reimplementation" ought to be bug-to-bug compatible, it may not be).

diff --git b/builtin/rebase.c a/builtin/rebase.c
index 9dc8475cd3..60e357c735 100644
--- b/builtin/rebase.c
+++ a/builtin/rebase.c
@@ -54,7 +54,7 @@ static int use_builtin_rebase(void)
 	cp.git_cmd = 1;
 	if (capture_command(&cp, &out, 6)) {
 		strbuf_release(&out);
-		return 1;
+		return 0;
 	}
 
 	strbuf_trim(&out);
Junio C Hamano· Nov 26, 2018, 06:10 UTC · re: Junio C Hamano · lore

[PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>  * "git rebase" and "git rebase -i" have been reimplemented in C.
>>
>> Here's another regression in the C version (and rc1),...
>> I wasn't trying to stress test rebase. I was just wanting to rebase a
>> history I was about to force-push after cleaning it up, hardly an
>> obscure use-case. So [repeat last transmission in
>> https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/ ]
>
> which, to those who are reading from sidelines:
>
>     Given that we're still finding regressions bugs in the rebase-in-C
>     version should we be considering reverting 5541bd5b8f ("rebase: default
>     to using the builtin rebase", 2018-08-08)?
>
>     I love the feature, but fear that the current list of known regressions
>     serve as a canary for a larger list which we'd discover if we held off
>     for another major release (and would re-enable rebase.useBuiltin=true in
>     master right after 2.20 is out the door).
>
> I am fine with the proposed flip, but I'll have to see the extent of
> damage this late in the game so that I won't miss anything.  In
> addition to the one-liner below, we'd need to update the quoted
> release notes entry, and possibly adjust some tests (even though the
> "reimplementation" ought to be bug-to-bug compatible, it may not be).

So, in a more concrete form, what you want to see is something like this in -rc2 and later?

-- >8 --
Subject: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature

It turns out to be a bit too early to unleash the reimplementation to the general public. Let's rewrite some documentation and make it an opt-in feature.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config/rebase.txt | 16 ++++++----------
 builtin/rebase.c                |  2 +-
 t/README                        |  4 ++--
 3 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f079bf6b7e..af12623151 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,16 +1,12 @@
 rebase.useBuiltin::
-	Set to `false` to use the legacy shellscript implementation of
-	linkgit:git-rebase[1]. Is `true` by default, which means use
-	the built-in rewrite of it in C.
+	Set to `true` to use the experimental reimplementation of
+	linkgit:git-rebase[1] in C.  Defaults to `false`.
 +
 The C rewrite is first included with Git version 2.20. This option
-serves an an escape hatch to re-enable the legacy version in case any
-bugs are found in the rewrite. This option and the shellscript version
-of linkgit:git-rebase[1] will be removed in some future release.
-+
-If you find some reason to set this option to `false` other than
-one-off testing you should report the behavior difference as a bug in
-git.
+allows early adopters to opt into the experimental version to find
+bugs in the rewritten version. This option and the shellscript version
+of linkgit:git-rebase[1] will be removed in some future release once
+the reimplementation becomes more stable.
 
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..19ad97b177 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -59,7 +59,7 @@ static int use_builtin_rebase(void)
 	cp.git_cmd = 1;
 	if (capture_command(&cp, &out, 6)) {
 		strbuf_release(&out);
-		return 1;
+		return 0;
 	}
 
 	strbuf_trim(&out);
diff --git a/t/README b/t/README
index 28711cc508..7e925e5fea 100644
--- a/t/README
+++ b/t/README
@@ -345,8 +345,8 @@ for the index version specified.  Can be set to any valid version
 GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
 by overriding the minimum number of cache entries required per thread.
 
-GIT_TEST_REBASE_USE_BUILTIN=<boolean>, when false, disables the
-builtin version of git-rebase. See 'rebase.useBuiltin' in
+GIT_TEST_REBASE_USE_BUILTIN=<boolean>, when true, forces the use of
+builtin version of git-rebase in the test. See 'rebase.useBuiltin' in
 git-config(1).
 
 GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading
-- 
2.20.0-rc1
Jonathan Nieder· Nov 28, 2018, 04:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Hi,
Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 8 quoted lines
>>> Given that we're still finding regressions bugs in the rebase-in-C
>>> version should we be considering reverting 5541bd5b8f ("rebase: default
>>> to using the builtin rebase", 2018-08-08)?
>>>
>>> I love the feature, but fear that the current list of known regressions
>>> serve as a canary for a larger list which we'd discover if we held off
>>> for another major release (and would re-enable rebase.useBuiltin=true in
>>> master right after 2.20 is out the door).
[...]
Show 16 quoted lines
> So, in a more concrete form, what you want to see is something like
> this in -rc2 and later?
>
> -- >8 --
> Subject: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature
>
> It turns out to be a bit too early to unleash the reimplementation
> to the general public.  Let's rewrite some documentation and make it
> an opt-in feature.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/config/rebase.txt | 16 ++++++----------
>  builtin/rebase.c                |  2 +-
>  t/README                        |  4 ++--
>  3 files changed, 9 insertions(+), 13 deletions(-)

I thought I should weigh in on how this would affect Debian's and Google's deployments.

First of all, I've looked over the revert patch carefully and it is well written and does what it says on the tin.

At https://bugs.debian.org/914695 is a report of a test regression in an outside project that is very likely to have been triggered by the new faster rebase code. The issue has not been triaged, so I don't know yet whether it's a problem in rebase-in-c or a manifestation of a bug in the test.

That said, Google has been running with the new rebase since ~1 month ago when it became the default, with no issues reported by users. As a result, I am confident that it can cope with what most users of "next" throw at it, which means that if we are to find more issues to polish it better, it will need all the exposure it can get.

In the Google deployment, we will keep using rebase-in-c even if it gets disabled by default, in order to help with that.

From the Debian point of view, it's only a matter of time before rebase-in-c becomes the default: even if it's not the default in 2.20, it would presumably be so in 2.21 or 2.22. That means the community's attention when resolving security and reliability bugs would be on the rebase-in-c implementation. As a result, the Debian package will most likely enable rebase-in-c by default even if upstream disables it, in order to increase the package's shelf life (i.e. to ease the maintenance burden of supporting whichever version of the package ends up in the next Debian stable).

So with either hat on, it doesn't matter whether you apply this patch upstream.

Having two pretty different deployments end up with the same conclusion leads me to suspect that it's best for upstream not to apply the revert patch, unless either

  (a) we have a concrete regression to address and then try again, or
  (b) we have a test or other plan to follow before trying again.

Thanks and hope that helps, Jonathan

Johannes Schindelin· Nov 28, 2018, 09:23 UTC · re: Jonathan Nieder · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Hi Jonathan,
On Tue, 27 Nov 2018, Jonathan Nieder wrote:
> At https://bugs.debian.org/914695 is a report of a test regression in
> an outside project that is very likely to have been triggered by the
> new faster rebase code.

From looking through that log.gz (without having a clue where the test code lives, so I cannot say what it is supposed to do, and also: this is the first time I hear about dgit...), it would appear that this must be a regression in the reflog messages produced by `git rebase`.

> The issue has not been triaged, so I don't know yet whether it's a
> problem in rebase-in-c or a manifestation of a bug in the test.
It ends thusly:
-- snip --
[...]
+ git reflog
+ egrep 'debrebase new-upstream.*checkout'
+ test 1 = 0
+ t-report-failure
+ set +x
TEST FAILED
-- snap --

Which makes me think that the reflog we produce in *some* code path that originally called `git checkout` differs from the scripted rebase's generated reflog.

Show 5 quoted lines
> That said, Google has been running with the new rebase since ~1 month
> ago when it became the default, with no issues reported by users.  As a
> result, I am confident that it can cope with what most users of "next"
> throw at it, which means that if we are to find more issues to polish it
> better, it will need all the exposure it can get.

Right. And there are a few weeks before the holidays, which should give me time to fix whatever bugs are discovered (I only half mind being the only one who fixes these bugs).

Show 22 quoted lines
> In the Google deployment, we will keep using rebase-in-c even if it
> gets disabled by default, in order to help with that.
> 
> From the Debian point of view, it's only a matter of time before
> rebase-in-c becomes the default: even if it's not the default in 2.20,
> it would presumably be so in 2.21 or 2.22.  That means the community's
> attention when resolving security and reliability bugs would be on the
> rebase-in-c implementation.  As a result, the Debian package will most
> likely enable rebase-in-c by default even if upstream disables it, in
> order to increase the package's shelf life (i.e. to ease the
> maintenance burden of supporting whichever version of the package ends
> up in the next Debian stable).
> 
> So with either hat on, it doesn't matter whether you apply this patch
> upstream.
> 
> Having two pretty different deployments end up with the same
> conclusion leads me to suspect that it's best for upstream not to
> apply the revert patch, unless either
> 
>   (a) we have a concrete regression to address and then try again, or
>   (b) we have a test or other plan to follow before trying again.

In this instance, I am more a fan of the "let's move fast and break things, then move even faster fixing them" approach.

Besides, the bug that Ævar discovered was a bug already in the scripted rebase, but hidden by yet another bug (the missing error checking).

I get the pretty firm impression that the common code paths are now pretty robust, and only lesser-exercised features may expose a bug (or regression, as in the case of the reflogs, where one could argue that the exact reflog message is not something we promise not to fiddle with).

Ciao, Dscho

Ævar Arnfjörð Bjarmason· Nov 28, 2018, 12:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

On Wed, Nov 28 2018, Johannes Schindelin wrote:
Show 75 quoted lines
> Hi Jonathan,
>
> On Tue, 27 Nov 2018, Jonathan Nieder wrote:
>
>> At https://bugs.debian.org/914695 is a report of a test regression in
>> an outside project that is very likely to have been triggered by the
>> new faster rebase code.
>
> From looking through that log.gz (without having a clue where the test
> code lives, so I cannot say what it is supposed to do, and also: this is
> the first time I hear about dgit...), it would appear that this must be a
> regression in the reflog messages produced by `git rebase`.
>
>> The issue has not been triaged, so I don't know yet whether it's a
>> problem in rebase-in-c or a manifestation of a bug in the test.
>
> It ends thusly:
>
> -- snip --
> [...]
> + git reflog
> + egrep 'debrebase new-upstream.*checkout'
> + test 1 = 0
> + t-report-failure
> + set +x
> TEST FAILED
> -- snap --
>
> Which makes me think that the reflog we produce in *some* code path that
> originally called `git checkout` differs from the scripted rebase's
> generated reflog.
>
>> That said, Google has been running with the new rebase since ~1 month
>> ago when it became the default, with no issues reported by users.  As a
>> result, I am confident that it can cope with what most users of "next"
>> throw at it, which means that if we are to find more issues to polish it
>> better, it will need all the exposure it can get.
>
> Right. And there are a few weeks before the holidays, which should give me
> time to fix whatever bugs are discovered (I only half mind being the only
> one who fixes these bugs).
>
>> In the Google deployment, we will keep using rebase-in-c even if it
>> gets disabled by default, in order to help with that.
>>
>> From the Debian point of view, it's only a matter of time before
>> rebase-in-c becomes the default: even if it's not the default in 2.20,
>> it would presumably be so in 2.21 or 2.22.  That means the community's
>> attention when resolving security and reliability bugs would be on the
>> rebase-in-c implementation.  As a result, the Debian package will most
>> likely enable rebase-in-c by default even if upstream disables it, in
>> order to increase the package's shelf life (i.e. to ease the
>> maintenance burden of supporting whichever version of the package ends
>> up in the next Debian stable).
>>
>> So with either hat on, it doesn't matter whether you apply this patch
>> upstream.
>>
>> Having two pretty different deployments end up with the same
>> conclusion leads me to suspect that it's best for upstream not to
>> apply the revert patch, unless either
>>
>>   (a) we have a concrete regression to address and then try again, or
>>   (b) we have a test or other plan to follow before trying again.
>
> In this instance, I am more a fan of the "let's move fast and break
> things, then move even faster fixing them" approach.
>
> Besides, the bug that Ævar discovered was a bug already in the scripted
> rebase, but hidden by yet another bug (the missing error checking).
>
> I get the pretty firm impression that the common code paths are now pretty
> robust, and only lesser-exercised features may expose a bug (or
> regression, as in the case of the reflogs, where one could argue that the
> exact reflog message is not something we promise not to fiddle with).

Since I raised this 'should we hold off?' I thought I'd chime in and say that I'm fine with going along with what you suggest and having the builtin as the default in the final. IOW not merge jc/postpone-rebase-in-c down.

Junio C Hamano· Nov 29, 2018, 04:58 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Since I raised this 'should we hold off?' I thought I'd chime in and say
> that I'm fine with going along with what you suggest and having the
> builtin as the default in the final. IOW not merge
> jc/postpone-rebase-in-c down.
OK.
Johannes Schindelin· Nov 29, 2018, 14:17 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Hi Jonathan,

if you could pry more information (or better information) out of that bug reporter, that would be nice. Apparently my email address is blacklisted by his mail provider, so he is unlikely to have received my previous mail (nor will he receive this one, I am sure).

Thanks, Dscho

On Wed, 28 Nov 2018, Johannes Schindelin wrote:
Show 78 quoted lines
> Hi Jonathan,
> 
> On Tue, 27 Nov 2018, Jonathan Nieder wrote:
> 
> > At https://bugs.debian.org/914695 is a report of a test regression in
> > an outside project that is very likely to have been triggered by the
> > new faster rebase code.
> 
> From looking through that log.gz (without having a clue where the test
> code lives, so I cannot say what it is supposed to do, and also: this is
> the first time I hear about dgit...), it would appear that this must be a
> regression in the reflog messages produced by `git rebase`.
> 
> > The issue has not been triaged, so I don't know yet whether it's a
> > problem in rebase-in-c or a manifestation of a bug in the test.
> 
> It ends thusly:
> 
> -- snip --
> [...]
> + git reflog
> + egrep 'debrebase new-upstream.*checkout'
> + test 1 = 0
> + t-report-failure
> + set +x
> TEST FAILED
> -- snap --
> 
> Which makes me think that the reflog we produce in *some* code path that
> originally called `git checkout` differs from the scripted rebase's
> generated reflog.
> 
> > That said, Google has been running with the new rebase since ~1 month
> > ago when it became the default, with no issues reported by users.  As a
> > result, I am confident that it can cope with what most users of "next"
> > throw at it, which means that if we are to find more issues to polish it
> > better, it will need all the exposure it can get.
> 
> Right. And there are a few weeks before the holidays, which should give me
> time to fix whatever bugs are discovered (I only half mind being the only
> one who fixes these bugs).
> 
> > In the Google deployment, we will keep using rebase-in-c even if it
> > gets disabled by default, in order to help with that.
> > 
> > From the Debian point of view, it's only a matter of time before
> > rebase-in-c becomes the default: even if it's not the default in 2.20,
> > it would presumably be so in 2.21 or 2.22.  That means the community's
> > attention when resolving security and reliability bugs would be on the
> > rebase-in-c implementation.  As a result, the Debian package will most
> > likely enable rebase-in-c by default even if upstream disables it, in
> > order to increase the package's shelf life (i.e. to ease the
> > maintenance burden of supporting whichever version of the package ends
> > up in the next Debian stable).
> > 
> > So with either hat on, it doesn't matter whether you apply this patch
> > upstream.
> > 
> > Having two pretty different deployments end up with the same
> > conclusion leads me to suspect that it's best for upstream not to
> > apply the revert patch, unless either
> > 
> >   (a) we have a concrete regression to address and then try again, or
> >   (b) we have a test or other plan to follow before trying again.
> 
> In this instance, I am more a fan of the "let's move fast and break
> things, then move even faster fixing them" approach.
> 
> Besides, the bug that Ævar discovered was a bug already in the scripted
> rebase, but hidden by yet another bug (the missing error checking).
> 
> I get the pretty firm impression that the common code paths are now pretty
> robust, and only lesser-exercised features may expose a bug (or
> regression, as in the case of the reflogs, where one could argue that the
> exact reflog message is not something we promise not to fiddle with).
> 
> Ciao,
> Dscho
Ian Jackson· Nov 29, 2018, 14:30 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> if you could pry more information (or better information) out of that bug
> reporter, that would be nice. Apparently my email address is blacklisted
> by his mail provider, so he is unlikely to have received my previous mail
> (nor will he receive this one, I am sure).

(I did receive this mail. Sorry for the inconvenience, which sadly is inevitable occasionally in the modern email world. FTR in future feel free to send the bounce to postmaster@chiark and I will make a you-shaped hole in my spamfilter. Also with Debian bugs you can launder your messages by, eg, emailing 914695-submitter@bugs.)

> > > At https://bugs.debian.org/914695 is a report of a test regression in
> > > an outside project that is very likely to have been triggered by the
> > > new faster rebase code.
As I wrote in the bug report last night:
 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914695#15
 I have investigated and the bug seems to be that git-rebase --onto now
 fails to honour GIT_REFLOG_ACTION for the initial checkout.
 In a successful run with older git I get a reflog like this:
   4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
   4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
   cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
   0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
   29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
   85e0c46 HEAD@{5}: debrebase: launder for new upstream
 With a newer git I get this:
   6d3fb91 HEAD@{0}: rebase finished: returning to refs/heads/master
   6d3fb91 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
   86c0721 HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
   50ba56c HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
   8272825 HEAD@{4}: rebase: checkout 8272825bb4ff6eba89afa936e32b6460f963a36a
   c78db55 HEAD@{5}: debrebase: launder for new upstream
 This breaks the test because my test suite is checking that I set
 GIT_REFLOG_ACTION appropriately.
 If you want I can provide a minimal test case but this should suffice
 to see the bug I hope...

Regards Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
Johannes Schindelin· Nov 29, 2018, 15:39 UTC · re: Ian Jackson · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Hi Ian,
On Thu, 29 Nov 2018, Ian Jackson wrote:
Show 11 quoted lines
> Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> > if you could pry more information (or better information) out of that bug
> > reporter, that would be nice. Apparently my email address is blacklisted
> > by his mail provider, so he is unlikely to have received my previous mail
> > (nor will he receive this one, I am sure).
> 
> (I did receive this mail.  Sorry for the inconvenience, which sadly is
> inevitable occasionally in the modern email world.  FTR in future feel
> free to send the bounce to postmaster@chiark and I will make a
> you-shaped hole in my spamfilter.  Also with Debian bugs you can
> launder your messages by, eg, emailing 914695-submitter@bugs.)

Right. I myself have plenty of email-related problems that seem to crop up this year in particular.

Show 34 quoted lines
> > > > At https://bugs.debian.org/914695 is a report of a test regression in
> > > > an outside project that is very likely to have been triggered by the
> > > > new faster rebase code.
> 
> As I wrote in the bug report last night:
> 
>  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914695#15
> 
>  I have investigated and the bug seems to be that git-rebase --onto now
>  fails to honour GIT_REFLOG_ACTION for the initial checkout.
> 
>  In a successful run with older git I get a reflog like this:
> 
>    4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
>    4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
>    cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
>    0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
>    29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
>    85e0c46 HEAD@{5}: debrebase: launder for new upstream
> 
>  With a newer git I get this:
> 
>    6d3fb91 HEAD@{0}: rebase finished: returning to refs/heads/master
>    6d3fb91 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
>    86c0721 HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
>    50ba56c HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
>    8272825 HEAD@{4}: rebase: checkout 8272825bb4ff6eba89afa936e32b6460f963a36a
>    c78db55 HEAD@{5}: debrebase: launder for new upstream
> 
>  This breaks the test because my test suite is checking that I set
>  GIT_REFLOG_ACTION appropriately.
> 
>  If you want I can provide a minimal test case but this should suffice
>  to see the bug I hope...
This should be plenty for me to get going. Thank you!

Ciao, Johannes

Show 10 quoted lines
> 
> Regards
> Ian.
> 
> -- 
> Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.
> 
> If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
> a private address which bypasses my fierce spamfilter.
> 
Ian Jackson· Nov 29, 2018, 15:50 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
Show 8 quoted lines
> >  In a successful run with older git I get a reflog like this:
> > 
> >    4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
> >    4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
> >    cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
> >    0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
> >    29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
> >    85e0c46 HEAD@{5}: debrebase: launder for new upstream
...
Show 7 quoted lines
> >  This breaks the test because my test suite is checking that I set
> >  GIT_REFLOG_ACTION appropriately.
> > 
> >  If you want I can provide a minimal test case but this should suffice
> >  to see the bug I hope...
> 
> This should be plenty for me to get going. Thank you!
Happy hunting.

While you're looking at this, I observe that the fact that the `rebase finished' message also does not honour GIT_REFLOG_ACTION appears to be a pre-existing bug.

(In general one often can't rely on GIT_REFLOG_ACTION still being set because the rebase might have been interrupted and restarted, which I think is why my test case looks for it in the initial `checkout' message.)

Regards, Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
Johannes Schindelin· Nov 29, 2018, 16:14 UTC · re: Ian Jackson · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Hi Ian,
On Thu, 29 Nov 2018, Ian Jackson wrote:
Show 19 quoted lines
> Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> > >  In a successful run with older git I get a reflog like this:
> > > 
> > >    4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
> > >    4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
> > >    cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
> > >    0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
> > >    29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
> > >    85e0c46 HEAD@{5}: debrebase: launder for new upstream
> ...
> > >  This breaks the test because my test suite is checking that I set
> > >  GIT_REFLOG_ACTION appropriately.
> > > 
> > >  If you want I can provide a minimal test case but this should suffice
> > >  to see the bug I hope...
> > 
> > This should be plenty for me to get going. Thank you!
> 
> Happy hunting.

I'll have to take a (lengthy) dinner break now, but this is what I have so far: a regression test that verifies the breakage (see the `fix-reflog-action` branch at https://github.com/dscho/git). I'll continue after dinner and am confident that this bug will be fixed within the next four hours.

> While you're looking at this, I observe that the fact that the `rebase
> finished' message also does not honour GIT_REFLOG_ACTION appears to be
> a pre-existing bug.

I noticed that, too, but at this point I am only fixing regressions. We can try to fix this long-standing bug in the v2.20 cycle.

Ciao, Johannes

Show 14 quoted lines
> (In general one often can't rely on GIT_REFLOG_ACTION still being set
> because the rebase might have been interrupted and restarted, which I
> think is why my test case looks for it in the initial `checkout'
> message.)
> 
> Regards,
> Ian.
> 
> -- 
> Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.
> 
> If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
> a private address which bypasses my fierce spamfilter.
> 
Ian Jackson· Nov 29, 2018, 16:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
Show 5 quoted lines
> I'll have to take a (lengthy) dinner break now, but this is what I have so
> far: a regression test that verifies the breakage (see the
> `fix-reflog-action` branch at https://github.com/dscho/git). I'll continue
> after dinner and am confident that this bug will be fixed within the next
> four hours.
That seems super speedy to me!

When you have a fix I will leave it up to the Debian git maintainers to decide whether they want to cherry pick your fix into their package, or await an updated upstream branch with rc, or what.

Show 7 quoted lines
> [Ian:]
> > While you're looking at this, I observe that the fact that the `rebase
> > finished' message also does not honour GIT_REFLOG_ACTION appears to be
> > a pre-existing bug.
> 
> I noticed that, too, but at this point I am only fixing regressions. We
> can try to fix this long-standing bug in the v2.20 cycle.
Right.

Thanks, Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
Johannes Schindelin· Nov 26, 2018, 22:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

Hi Ævar,
On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 27 quoted lines
> On Wed, Nov 21 2018, Junio C Hamano wrote:
> 
> >  * "git rebase" and "git rebase -i" have been reimplemented in C.
> 
> Here's another regression in the C version (and rc1), note: the
> sha1collisiondetection is just a stand in for "some repo":
> 
>     (
>         rm -rf /tmp/repo &&
>         git init /tmp/repo &&
>         cd /tmp/repo &&
>         for c in 1 2
>         do
>             touch $c &&
>             git add $c &&
>             git commit -m"add $c"
>         done &&
>         git remote add origin https://github.com/cr-marcstevens/sha1collisiondetection.git &&
>         git fetch &&
>         git branch --set-upstream-to origin/master &&
>         git rebase -i
>     )
> 
> The C version will die with "fatal: unable to read tree
> 0000000000000000000000000000000000000000". Running this with
> rebase.useBuiltin=false does the right thing and rebases as of the merge
> base of the two (which here is the root of the history).
Sorry, this bug does not reproduce here:

$ git rebase -i Successfully rebased and updated refs/heads/master.

> I wasn't trying to stress test rebase. I was just wanting to rebase a
> history I was about to force-push after cleaning it up, hardly an
> obscure use-case. So [repeat last transmission in
> https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/ ]

Maybe you can give me the full details so that I can verify that this is indeed a bug in the builtin C and not just a regression caused by some random branches being merged together?

In short: please provide me with the exact URL and branch of your git.git fork to test. Then please make sure to specify the precise revision of the sha1collisiondetection/master rev, just in case that it matters.

Ideally, you would reduce the problem to a proper test case, say, for t3412 (it seems that you try to rebase onto an unrelated history, so it is *vaguely* related to "rebase-root").

Ciao, Dscho

Johannes Schindelin· Nov 26, 2018, 23:47 UTC · re: Johannes Schindelin · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

Hi Ævar,
On Mon, 26 Nov 2018, Johannes Schindelin wrote:
Show 51 quoted lines
> On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Wed, Nov 21 2018, Junio C Hamano wrote:
> > 
> > >  * "git rebase" and "git rebase -i" have been reimplemented in C.
> > 
> > Here's another regression in the C version (and rc1), note: the
> > sha1collisiondetection is just a stand in for "some repo":
> > 
> >     (
> >         rm -rf /tmp/repo &&
> >         git init /tmp/repo &&
> >         cd /tmp/repo &&
> >         for c in 1 2
> >         do
> >             touch $c &&
> >             git add $c &&
> >             git commit -m"add $c"
> >         done &&
> >         git remote add origin https://github.com/cr-marcstevens/sha1collisiondetection.git &&
> >         git fetch &&
> >         git branch --set-upstream-to origin/master &&
> >         git rebase -i
> >     )
> > 
> > The C version will die with "fatal: unable to read tree
> > 0000000000000000000000000000000000000000". Running this with
> > rebase.useBuiltin=false does the right thing and rebases as of the merge
> > base of the two (which here is the root of the history).
> 
> Sorry, this bug does not reproduce here:
> 
> $ git rebase -i
> Successfully rebased and updated refs/heads/master.
> 
> > I wasn't trying to stress test rebase. I was just wanting to rebase a
> > history I was about to force-push after cleaning it up, hardly an
> > obscure use-case. So [repeat last transmission in
> > https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/ ]
> 
> Maybe you can give me the full details so that I can verify that this is
> indeed a bug in the builtin C and not just a regression caused by some
> random branches being merged together?
> 
> In short: please provide me with the exact URL and branch of your git.git
> fork to test. Then please make sure to specify the precise revision of the
> sha1collisiondetection/master rev, just in case that it matters.
> 
> Ideally, you would reduce the problem to a proper test case, say, for
> t3412 (it seems that you try to rebase onto an unrelated history, so it is
> *vaguely* related to "rebase-root").

So I was getting spooked enough by your half-complete bug report that I did more digging (it is really quite a bit frustrating to have so little hard evidence to go on, a wild goose chase is definitely not what I was looking forward to after a day of fighting other fires, but you know, built-in rebase is dear to me).

The error message you copied clearly comes from tree-walk.c, from `fill_tree_descriptor()` (the other "unable to read tree" messages enclose the hash in parentheses).

There are exactly 3 calls to said function in the built-in rebase/rebase -i in the current `master`, a1598010f775 (Merge branch 'nd/per-worktree-ref-iteration', 2018-11-26):

$ git grep fill_tree_descriptor -- builtin/rebase*.c sequencer.[ch] rebase-interactive.[ch] builtin/rebase.c: if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) { builtin/rebase.c: if (!fill_tree_descriptor(&desc[nr++], oid)) { sequencer.c: if (!fill_tree_descriptor(&desc, &oid)) {

The last one of these is in `do_reset()`, i.e. handling a `reset` command which you did not ask for, as you passed `-i` to `git rebase`, not `-ir`.

The first two *both* are in `reset_head()`. The first of them uses `head_oid`, which is read directly via `get_oid("HEAD", &head_oid)`, so if this is all zeroes for you, then it's not rebase's fault.

The second one uses the parameter `oid` passed into `reset_head()`. The only calls to that function that do not pass `NULL` as `oid` (which would trigger `oid` to be replaced by `&head_oid`, i.e again not all zeroes unless your setup is broken) are:

- in the `--abort` code path
- in the `--autostash` code path
- in the fast-forwarding code path
- just after the "First, rewinding head" message in the *non*-interactive
  rebase
None of these apply to your script snippet.

Under the assumption that you might have forgotten to talk about rebase.autostash=true and some dirty file, I tried to augment the script snippet accordingly, but the built-in rebase as of current `master` still works for me, plus: reading the autostash code path, it is hard to imagine that the `lookup_commit_reference()` would return a pointer to a commit object whose oid is all zeroes.

In short, even a thorough study of the code (keeping in mind the few tidbits of information provided by you) leaves me really wondering which code you run, because it sure does not look like current `master` to me.

And if it is not `master`, then I have to ask why you keep suggesting to turn off the built-in rebase by default in `master`.

Ciao, Johannes

P.S.: Maybe you have a hook you forgot to mention?
Junio C Hamano· Nov 28, 2018, 04:07 UTC · re: Johannes Schindelin · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> ...
> In short, even a thorough study of the code (keeping in mind the few
> tidbits of information provided by you) leaves me really wondering which
> code you run, because it sure does not look like current `master` to me.
>
> And if it is not `master`, then I have to ask why you keep suggesting to
> turn off the built-in rebase by default in `master`.
>
> Ciao,
> Johannes
>
> P.S.: Maybe you have a hook you forgot to mention?

Any response? Or can I retract jc/postpone-rebase-in-c that was prepared as a reaction to this?

Johannes Schindelin· Nov 28, 2018, 09:30 UTC · re: Junio C Hamano · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

Hi Junio,
On Wed, 28 Nov 2018, Junio C Hamano wrote:
Show 17 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > ...
> > In short, even a thorough study of the code (keeping in mind the few
> > tidbits of information provided by you) leaves me really wondering which
> > code you run, because it sure does not look like current `master` to me.
> >
> > And if it is not `master`, then I have to ask why you keep suggesting to
> > turn off the built-in rebase by default in `master`.
> >
> > Ciao,
> > Johannes
> >
> > P.S.: Maybe you have a hook you forgot to mention?
> 
> Any response?  Or can I retract jc/postpone-rebase-in-c that was
> prepared as a reaction to this?

I worked with Ævar via IRC and we figured out the root cause and I submitted https://public-inbox.org/git/pull.88.git.gitgitgadget@gmail.com/ to fix it.

Given that this is a really obscure edge case (`git rebase --stat -v -i` onto an unrelated commit history, if you take away one of these, the bug does not trigger), and that it was only discovered to be a bug *because* of the built-in rebase (the scripted version had the same bug, but simply forgot to do proper error checking), I would not think that the reported bug is a strong argument in favor of turning off the built-in rebase by defauly.

In other words, after understanding the bug I am even more confident than before that the built-in rebase is actually in a pretty good shape.

I do not expect any major regressions, and if any happen: we do have that escape hatch for corner cases while I fix those bugs.

Ciao, Dscho

Junio C Hamano· Nov 26, 2018, 07:35 UTC · re: Junio C Hamano · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

Unless I hear otherwise in the next 24 hours, I am planning to merge the following topics to 'master' before cutting -rc2. Please stop me on any of these topics.

 - jc/postpone-rebase-in-c
   This may be the most controversial.  It demotes the C
   reimplementation of "git rebase" to an experimental opt-in
   feature that can only be enabled by setting rebase.useBuiltIn
   configuration that defaults to false.
   cf. <xmqq36roz7ve.fsf_-_@gitster-ct.c.googlers.com>
 - ab/format-patch-rangediff-not-stat
   The "--rangediff" option recently added to "format-patch"
   interspersed a bogus and useless "--stat" information by mistake,
   which is being corrected.
   cf. <xmqqa7lz43d8.fsf@gitster-ct.c.googlers.com>

The following three topics I do not need help in deciding; they are all good and will be merged before -rc2.

 - jk/t5562-perl-path-fix
   This is to invoke a perl scriptlet with "$PERL_PATH" in one of
   the new tests, instead of relying on (an incorrect) she-bang line
   in the script file.
 - tb/clone-case-smashing-warning-test
   This enables the test to see the behaviour of "git clone" after
   cloning a project that has paths that are different only in case
   on MINGW (earlier it wanted CASE_INSENSITIVE_FS prerequisite and
   in addition not on MINGW).
 - nd/per-worktree-ref-iteration
   This fixes a "return function_that_returns_void(...)" in a
   function that returns void.
Thanks.
Elijah Newren· Nov 26, 2018, 15:41 UTC · re: Junio C Hamano · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

On Sun, Nov 25, 2018 at 11:37 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> Unless I hear otherwise in the next 24 hours, I am planning to
> merge the following topics to 'master' before cutting -rc2.  Please
> stop me on any of these topics.
>
>  - jc/postpone-rebase-in-c
>
>    This may be the most controversial.  It demotes the C
>    reimplementation of "git rebase" to an experimental opt-in
>    feature that can only be enabled by setting rebase.useBuiltIn
>    configuration that defaults to false.
>
>    cf. <xmqq36roz7ve.fsf_-_@gitster-ct.c.googlers.com>

If we don't set rebase.useBuiltin to false, then there is also a minor regression in the error message printed by the built-in rebase we may want to try to address. I have a patch for it at <20181122044841.20993-2-newren@gmail.com>, but I don't know if that patch is acceptable as-is this close to a release since that'd not give translators much time to update.

Junio C Hamano· Nov 27, 2018, 00:40 UTC · re: Elijah Newren · lore

Re: [ANNOUNCE] Git v2.20.0-rc1

Elijah Newren <newren@gmail.com> writes:
Show 6 quoted lines
> If we don't set rebase.useBuiltin to false, then there is also a minor
> regression in the error message printed by the built-in rebase we may
> want to try to address.  I have a patch for it at
> <20181122044841.20993-2-newren@gmail.com>, but I don't know if that
> patch is acceptable as-is this close to a release since that'd not
> give translators much time to update.

For this particular one, I'd rather ship "rebase in C" with known message glitch, with or without the "mark it experimental and make it opt-in" last-time change. Of course, turning it off by default would let us worry about these message glitches even less, but at this point, I'd be worried more about bugs that can affect the actual operation and outcome recorded in the resulting history.

← back to recent threads