Re: What's cooking in git.git (Oct 2019, #01; Thu, 3)

From: Elijah Newren <newren@gmail.com>
Date: Oct 5, 2019, 00:40 UTC
Message-ID: <CABPp-BE+D-GFJaucgCCsBA8E7nQAxc0vNE92HaCpOTQrUp=mPA@mail.gmail.com>
In-Reply-To: <972f4674-ed00-7113-24eb-f59f1b751690@gmail.com>

On Fri, Oct 4, 2019 at 4:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 42 quoted lines

>
> Hi Junio
>
> On 03/10/2019 06:04, Junio C Hamano wrote:
> > Here are the topics that have been cooking.  Commits prefixed with
> > '-' are only in 'pu' (proposed updates) while commits prefixed with
> > '+' are in 'next'.  The ones marked with '.' do not appear in any of
> > the integration branches, but I am still holding onto them.
> > [...]
> >
> >
> > * pw/rebase-i-show-HEAD-to-reword (2019-08-19) 3 commits
> >   - sequencer: simplify root commit creation
> >   - rebase -i: check for updated todo after squash and reword
> >   - rebase -i: always update HEAD before rewording
> >   (this branch is used by ra/rebase-i-more-options.)
> >
> >   "git rebase -i" showed a wrong HEAD while "reword" open the editor.
> >
> >   Will merge to 'next'.
>
> That's great, thanks
>
> >
> > * ra/rebase-i-more-options (2019-09-09) 6 commits
> >   - rebase: add --reset-author-date
> >   - rebase -i: support --ignore-date
> >   - sequencer: rename amend_author to author_to_rename
> >   - rebase -i: support --committer-date-is-author-date
> >   - sequencer: allow callers of read_author_script() to ignore fields
> >   - rebase -i: add --ignore-whitespace flag
> >   (this branch uses pw/rebase-i-show-HEAD-to-reword.)
> >
> >   "git rebase -i" learned a few options that are known by "git
> >   rebase" proper.
> >
> >   Is this ready for 'next'.
>
> Nearly, but not quite I think cf [1]. Also I'm still not convinced that
> having different behaviors for --ignore-whitespace depending on the
> backend is going to be helpful but maybe they are close enough not to
> matter too much in practice [2].

Sorry I should have chimed in sooner; I can speak to the second point. I would say that in practice it doesn't matter a lot; in most cases the two overlap. Both am's --ignore-whitespace and merge's -Xignore-space-change are buggy (in different ways) and should be fixed, but I'd consider them both to be buggy in edge cases. I recommended earlier this summer that Rohit submit the patches without first attempting to fix apply or xdiff, and kept in my TODO list that'd I'd go in and fix xdiff later if Rohit didn't have extra time for it. I did a little digging back then to find out the differences and suggested some text to use to explain them and to argue that they shouldn't block this feature:

""" am's --ignore-space-change (an alias for am's --ignore-whitespace; see git-apply's description of those two flags) not only share the same name with diff's --ignore-space-change and merge's -Xignore-space-change, but the similarity in naming appears to have been intentional with am's --ignore-space-change and merge's -Xignore-space-change being designed to have the same functionality (see e.g. the commit messages for f008cef4abb2 ("Merge branch 'jc/apply-ignore-whitespace'", 2014-06-03) and 4e5dd044c62f ("merge-recursive: options to ignore whitespace changes", 2010-08-26)). For the most part, these options do provide the same behavior. However, there are some edge cases where both apply's --ignore-space-change and merge's -Xignore-space-change fall short of optimal behavior, and in different ways. In particular, --ignore-space-change for apply will handle whitespace changes in the context region but not in the region the other side modified, and -Xignore-space-change will delete whitespace changes even when the other side had no changes (thus treating both sides as unmodified). Fixing these differences in edge cases is left for future work; this patch simply wires interactive rebase to also understand --ignore-whitespace by translating it to -Xignore-space-change. """

I've got another email with even more detail if folks need it.