Moving etc/NEWS around causing release branch merge to fail again

Moving etc/NEWS around causing release branch merge to fail again

[Stefan Kangas]

Merging the release branch is failing due to our practice of moving
etc/NEWS around again.  Here's what I see:

    $ ./admin/automerge -r -n 1 -b -t -p
    Resetting...
    Pulling...
    Merging...
    merged ok
    automerge: etc/NEWS has been modified

I'll see if I can find the time and energy to investigate this today,
but otherwise it might be a couple of days until I'll merge emacs-29 to
master again.  If anyone knows what's going on, please fix it or even
better tell me which steps to take.

Re: Moving etc/NEWS around causing release branch merge to fail again

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Thu, 8 Dec 2022 07:49:48 -0800
> 
> Merging the release branch is failing due to our practice of moving
> etc/NEWS around again.  Here's what I see:
> 
>     $ ./admin/automerge -r -n 1 -b -t -p
>     Resetting...
>     Pulling...
>     Merging...
>     merged ok
>     automerge: etc/NEWS has been modified
> 
> I'll see if I can find the time and energy to investigate this today,
> but otherwise it might be a couple of days until I'll merge emacs-29 to
> master again.  If anyone knows what's going on, please fix it or even
> better tell me which steps to take.

I did the merge.

I don't use admin/automerge, I use admin/gitmerge.el in an interactive
session.  It asked me whether to try resolving conflicts in NEWS; I
answered YES.  Then it said it finished successfully.  I compared
NEWS.29 on master with NEWS on the release branch, and saw that the
entry for external-completion-table was lost; so I added it manually
to NEWS.29.

Maybe the way gitmerge.el attempts to fix the conflicts in NEWS is
unreliable, and we should always do that manually?  OTOH, fixing such
probl;ems with NEWS is easy: in case there are too many diffs, just
copy NEWS from the branch to NEWS.29 on master, and commit that.

Do we edit NEWS.29 on master, ever?  If we do, we should refrain from
doing that.

Re: Moving etc/NEWS around causing release branch merge to fail again

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't use admin/automerge, I use admin/gitmerge.el in an interactive
> session.  It asked me whether to try resolving conflicts in NEWS; I
> answered YES.  Then it said it finished successfully.  I compared
> NEWS.29 on master with NEWS on the release branch, and saw that the
> entry for external-completion-table was lost; so I added it manually
> to NEWS.29.

Thanks, I'll try a manual merge next time.

> Maybe the way gitmerge.el attempts to fix the conflicts in NEWS is
> unreliable, and we should always do that manually?

I think it mostly either works, or it is clear how to fix it.  But it
was the first time I saw this particular error.

> OTOH, fixing such probl;ems with NEWS is easy: in case there are too
> many diffs, just copy NEWS from the branch to NEWS.29 on master, and
> commit that.

Yup.  Perhaps we could even make automerge detect that, and do it for
us if needed (or at least let us know).  I'll put it on my TODO.

> Do we edit NEWS.29 on master, ever?  If we do, we should refrain from
> doing that.

I've had to backport NEWS.28 changes from master to emacs-28 on at least
one occasion.  It is indeed better to avoid that.

Re: Moving etc/NEWS around causing release branch merge to fail again

[Stefan Kangas]

Stefan Kangas <stefankangas@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> I don't use admin/automerge, I use admin/gitmerge.el in an interactive
>> session.  It asked me whether to try resolving conflicts in NEWS; I
>> answered YES.  Then it said it finished successfully.  I compared
>> NEWS.29 on master with NEWS on the release branch, and saw that the
>> entry for external-completion-table was lost; so I added it manually
>> to NEWS.29.
>
> Thanks, I'll try a manual merge next time.

It happened again.  I spent an hour trying to fix the merge this time,
and doing it manually with `M-x gitmerge' got me nowhere.  NEWS was
still being modified, and it didn't prompt me about NEWS.  I ended up
just fixing up NEWS manually after the merge, which is what I'll do from
here on out.

BTW, when doing manual merges, can everyone please try to make sure that
etc/NEWS is *not* modified, or fix it manually.  Merges with `M-x
gitmerge' are prone to such errors, as there are no automatic checks in
place for modifying NEWS, in the case where there are no git merge
conflicts.  See the FIXME in admin/gitmerge.el.

(Such a mistake was made in commit 6d6ca47aba7b, where the NEWS entry
added in 8f49137c9bf was merged into the etc/NEWS file on master.
I've fixed that in commit f1840cf12fda.)

Re: Moving etc/NEWS around causing release branch merge to fail again

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Tue, 13 Dec 2022 15:34:09 -0800
> Cc: emacs-devel@gnu.org
> 
> Stefan Kangas <stefankangas@gmail.com> writes:
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> >> I don't use admin/automerge, I use admin/gitmerge.el in an interactive
> >> session.  It asked me whether to try resolving conflicts in NEWS; I
> >> answered YES.  Then it said it finished successfully.  I compared
> >> NEWS.29 on master with NEWS on the release branch, and saw that the
> >> entry for external-completion-table was lost; so I added it manually
> >> to NEWS.29.
> >
> > Thanks, I'll try a manual merge next time.
> 
> It happened again.

What happened, exactly?  Please tell more about what you saw.

> I spent an hour trying to fix the merge this time,
> and doing it manually with `M-x gitmerge' got me nowhere.  NEWS was
> still being modified, and it didn't prompt me about NEWS.  I ended up
> just fixing up NEWS manually after the merge, which is what I'll do from
> here on out.

Did you succeed in understanding the reason(s)?

> BTW, when doing manual merges, can everyone please try to make sure that
> etc/NEWS is *not* modified, or fix it manually.

You mean, the merge to NEWS.29 on master?  Which NEWS should not be
modified, and what do you mean by "manual merges"?

Re: Moving etc/NEWS around causing release branch merge to fail again

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> What happened, exactly?  Please tell more about what you saw.

I saw the automatic merge fail with this error message:

    etc/NEWS has been modified

> Did you succeed in understanding the reason(s)?

For some reason, git succeeds in merging NEWS without any conflicts, and
then admin/automerge croaks because of it (see below).  Perhaps it is
related to the merge errors fixed in f1840cf12fda7f?  I don't know.

If it happens again, I hope to understand more about what is going on.
Should anyone want to dig in, just say this on master:

    git reset --hard 8036739c1bb6fa^ # warning: hard reset
    ./admin/automerge -r -n1

Fix the conflicts in typescript-ts-mode.el and lisp/treesit.el, and
then:

    ./admin/automerge -n1

>> BTW, when doing manual merges, can everyone please try to make sure that
>> etc/NEWS is *not* modified, or fix it manually.
>
> You mean, the merge to NEWS.29 on master?  Which NEWS should not be
> modified, and what do you mean by "manual merges"?

When I say manual merges, I mean those carried out using something like:

    emacs -Q -l admin/gitmerge.el -eval '(gitmerge "emacs-29")'

After a merge from the release branch (emacs-NN) to master, the etc/NEWS
file should never be modified.  Only the etc/NEWS.NN file should have
been modified.  Otherwise, it needs to be fixed manually.

See also these lines in admin/automerge:

    ## FIXME it would be better to trap this in gitmerge.
    ## NEWS should never be modified, only eg NEWS.26.
    git diff --stat --cached origin/master | grep -q "etc/NEWS " && \
        die "etc/NEWS has been modified"

Re: Moving etc/NEWS around causing release branch merge to fail again

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Tue, 13 Dec 2022 21:32:41 -0800
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What happened, exactly?  Please tell more about what you saw.
> 
> I saw the automatic merge fail with this error message:
> 
>     etc/NEWS has been modified

I looked at what gitmerge.el does for NEWS, and I admit that I'm
confused.  AFAIU, it does this:

  . use "git diff" to write the diffs for branch/NEWS to a patchfile
  . use "patch" to patch master/NEWS.29 using patchfile
  . delete patchfile
  . "git add master/NEWS.29"

So far so good, but then it also does this:

  . "git reset -- NEWS"
  . "git checkout NEWS"

Why does it do that?  AFAIU, these two commands _can_ produce changes
in master/NEWS.

> After a merge from the release branch (emacs-NN) to master, the etc/NEWS
> file should never be modified.  Only the etc/NEWS.NN file should have
> been modified.  Otherwise, it needs to be fixed manually.
> 
> See also these lines in admin/automerge:
> 
>     ## FIXME it would be better to trap this in gitmerge.
>     ## NEWS should never be modified, only eg NEWS.26.
>     git diff --stat --cached origin/master | grep -q "etc/NEWS " && \
>         die "etc/NEWS has been modified"

Right.

Re: Moving etc/NEWS around causing release branch merge to fail again

[Yuan Fu]

> On Dec 13, 2022, at 3:34 PM, Stefan Kangas <stefankangas@gmail.com> wrote:
> 
> Stefan Kangas <stefankangas@gmail.com> writes:
> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>>> I don't use admin/automerge, I use admin/gitmerge.el in an interactive
>>> session.  It asked me whether to try resolving conflicts in NEWS; I
>>> answered YES.  Then it said it finished successfully.  I compared
>>> NEWS.29 on master with NEWS on the release branch, and saw that the
>>> entry for external-completion-table was lost; so I added it manually
>>> to NEWS.29.
>> 
>> Thanks, I'll try a manual merge next time.
> 
> It happened again.  I spent an hour trying to fix the merge this time,
> and doing it manually with `M-x gitmerge' got me nowhere.  NEWS was
> still being modified, and it didn't prompt me about NEWS.  I ended up
> just fixing up NEWS manually after the merge, which is what I'll do from
> here on out.
> 
> BTW, when doing manual merges, can everyone please try to make sure that
> etc/NEWS is *not* modified, or fix it manually.  Merges with `M-x
> gitmerge' are prone to such errors, as there are no automatic checks in
> place for modifying NEWS, in the case where there are no git merge
> conflicts.  See the FIXME in admin/gitmerge.el.
> 
> (Such a mistake was made in commit 6d6ca47aba7b, where the NEWS entry
> added in 8f49137c9bf was merged into the etc/NEWS file on master.
> I've fixed that in commit f1840cf12fda.)

Do I need to do anything special when editing the NEWS file on emacs-29? Doesn’t seem like it but just want make sure.

Yuan

Re: Moving etc/NEWS around causing release branch merge to fail again

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Sat, 31 Dec 2022 14:42:35 -0800
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  emacs-devel@gnu.org
> 
> Do I need to do anything special when editing the NEWS file on emacs-29? Doesn’t seem like it but just want make sure.

No, editing NEWS on the emacs-29 branch doesn't need anything special.
The problems happen on master, not on the branch.  On master, we
should never edit the NEWS.NN files from past versions, and especially
not the one from the current release branch, on which we routinely
make changes to the corresponding NEWS.

Re: Moving etc/NEWS around causing release branch merge to fail again

[Yuan Fu]

> On Dec 31, 2022, at 10:30 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Sat, 31 Dec 2022 14:42:35 -0800
>> Cc: Eli Zaretskii <eliz@gnu.org>,
>> emacs-devel@gnu.org
>> 
>> Do I need to do anything special when editing the NEWS file on emacs-29? Doesn’t seem like it but just want make sure.
> 
> No, editing NEWS on the emacs-29 branch doesn't need anything special.
> The problems happen on master, not on the branch.  On master, we
> should never edit the NEWS.NN files from past versions, and especially
> not the one from the current release branch, on which we routinely
> make changes to the corresponding NEWS.

Cool, thanks.

Yuan