merge-commits policy (was: bug#8675: lisp_string_width and strings wider than INT_MAX)

merge-commits policy (was: bug#8675: lisp_string_width and strings wider than INT_MAX)

[Eli Zaretskii]

This issue came up as a spin-off of discussing the proposed patch for
bug #8675.  I moved the discussion here because it is related to the
procedures contributors are supposed to use when committing to the
master repo.

Here's the beginning, for those who don't read bug-gnu-emacs:

> > Date: Sun, 15 May 2011 22:33:03 -0700
> > From: Paul Eggert <eggert <at> cs.ucla.edu>
> > CC: 8675 <at> debbugs.gnu.org
> > 
> > On 05/15/11 22:30, Eli Zaretskii wrote:
> > >> Date: Sun, 15 May 2011 22:07:36 -0700
> > >> From: Paul Eggert <eggert <at> cs.ucla.edu>
> > >>
> > >> PATCH 3 depends on two obvious patches: PATCH 2 introduces a
> > >> helper
> > >> no-return function string_overflow, and PATCH 1 updates to the
> > >> latest
> > >> version of gnulib.
> > > 
> > > Thanks, but why do these patches come with unrelated changes in
> > > texinfo.tex?
> > 
> > Because that's part of PATCH 1, which updates to the latest version
> > of gnulib.  Gnulib contains texinfo.tex.  PATCH 1 was generated
> > entirely automatically by "make sync-from-gnulib".
> 
> When you merge, could you please make the texinfo.tex update a
> separate commit on the trunk, then?  No one will ever expect to find
> that file in a commit logged as "sync from gnulib", which will make
> forensics more difficult than it needs to (since your commits are
> always merge-commits).

And here's how Stefan replied:

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  8675@debbugs.gnu.org
> Date: Mon, 16 May 2011 13:48:33 -0300
> 
> > When you merge, could you please make the texinfo.tex update a
> > separate commit on the trunk, then?
> 
> Why?

I already explained the reasons in my original message (above),
i.e. that forensics is more difficult.

> > No one will ever expect to find that file in a commit logged as "sync
> > from gnulib",
> 
> I, for one, do.  texinfo.tex is not a file we maintain, so I fully
> expect to find its changes in some kind of "sync from outside" commit.

Well, then maybe replace "no one will ever" with "many people will
not".

However, I think even you will not expect to find that texinfo.tex was
updated in commits such as these two:

  revno: 103360 [merge]
  committer: Paul Eggert <eggert@cs.ucla.edu>
  branch nick: trunk
  timestamp: Sun 2011-02-20 00:48:52 -0800
  message:
    Merge: Import crypto/md5 and stdint modules from gnulib.
    --------------------------------------------------------
  revno: 103104 [merge]
  committer: Paul Eggert <eggert@cs.ucla.edu>
  branch nick: trunk
  timestamp: Thu 2011-02-03 11:30:24 -0800
  message:
    merge: allow C code to suppress warnings about ignored return values
  ------------------------------------------------------------

Yes, invoking "bzr log" with the -n0 or --include-merges switch would
have shown that the second one of these two mentions texinfo.tex in
its branch commit.  But these switches makes "bzr log" much slower.
Any bzr command that uses revision specs need special care (e.g.,
using the `before:' keyword) to ensure branch commits aren't missed.
And to find out which branch commit modified texinfo.tex as part of
revision 103360, you need "bzr diff", or other similar tricks.  It's
not rocket science, but it makes the job of finding the relevant
changes harder and more error prone.  Plus, it requires one to have
good control of relatively obscure options.

Thus my request.

More generally, I thought commits to mainline from local branches are
supposed to be one feature at a time.  That is, each changeset
committed to mainline is supposed to be self-contained and not mix
different features and unrelated bugfixes.  If you regard any "sync
from gnulib" as a single self-contained changeset, then at least it
should be committed to mainline separately from other changes.

admin/notes/commits has these instructions, which I thought was
supposed to be followed:

  (0) Each commit should correspond to a single change (whether spread
      over multiple files or not).  Do not mix different changes in the
      same commit (eg adding a feature in one file, fixing a bug in
      another should be two commits, not one).

  (1) Commit all changed files at once with a single log message (which
      in CVS will result in an identical log message for all committed
      files), not one-by-one.  This is pretty easy using vc-dir now.

  (2) Make the log message describe the entire changeset, perhaps
      including relevant changelog entiries (I often don't bother with
      the latter if it's a trivial sort of change).

If that's not the policy, then what _is_ the policy?

As an aside, it looks like the version of texinfo.tex we merge is from
ftp://tug.org/tex/.  But this message recently posted by Karl Berry,
the maintainer of that file:

  http://lists.gnu.org/archive/html/bug-texinfo/2011-04/msg00038.html

indicates that the right place is ftp://ftp.gnu.org/gnu/texinfo/.

Re: merge-commits policy

[Stefan Monnier]

>> I, for one, do.  texinfo.tex is not a file we maintain, so I fully
>> expect to find its changes in some kind of "sync from outside" commit.
> Well, then maybe replace "no one will ever" with "many people will not".

> However, I think even you will not expect to find that texinfo.tex was
> updated in commits such as these two:

>   revno: 103360 [merge]
>   committer: Paul Eggert <eggert@cs.ucla.edu>
>   branch nick: trunk
>   timestamp: Sun 2011-02-20 00:48:52 -0800
>   message:
>     Merge: Import crypto/md5 and stdint modules from gnulib.
>     --------------------------------------------------------
>   revno: 103104 [merge]
>   committer: Paul Eggert <eggert@cs.ucla.edu>
>   branch nick: trunk
>   timestamp: Thu 2011-02-03 11:30:24 -0800
>   message:
>     merge: allow C code to suppress warnings about ignored return values
>   ------------------------------------------------------------

The same holds for "merge from emacs-23" or any other merge for that matter.

> Yes, invoking "bzr log" with the -n0 or --include-merges switch would
> have shown that the second one of these two mentions texinfo.tex in
> its branch commit.

Exactly.  As would searching the ChangeLog rather than "bzr log".
Personally I would use C-x v l and/or C-x v g from the texinfo.tex buffer.

> different features and unrelated bugfixes.  If you regard any "sync
> from gnulib" as a single self-contained changeset, then at least it
> should be committed to mainline separately from other changes.

That would make sense, yes.  Although in the case of gnulib, most
"syncs" happen because we import yet another module, so it's OK to do
both "import foo module" and "sync" at the same time, but indeed, the
commit message should indicate that a sync with gnulib took place.


        Stefan

Re: merge-commits policy

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Tue, 17 May 2011 10:42:32 -0300
> 
> >   revno: 103104 [merge]
> >   committer: Paul Eggert <eggert@cs.ucla.edu>
> >   branch nick: trunk
> >   timestamp: Thu 2011-02-03 11:30:24 -0800
> >   message:
> >     merge: allow C code to suppress warnings about ignored return values
> >   ------------------------------------------------------------
> 
> The same holds for "merge from emacs-23" or any other merge for that matter.

Except that emacs-23 is a public branch which can be examined
directly, unlike private branches that cannot.

> > Yes, invoking "bzr log" with the -n0 or --include-merges switch would
> > have shown that the second one of these two mentions texinfo.tex in
> > its branch commit.
> 
> Exactly.

Exactly: harder.

> As would searching the ChangeLog rather than "bzr log".

Unfortunately, only some of these updates are reflected in
doc/misc/ChangeLog, and some aren't, so this is unreliable.

> Personally I would use C-x v l and/or C-x v g from the texinfo.tex buffer.

"C-x v l" takes ages for veteran files (because texinfo.tex was
"cvs mv"ed, it appears to have been born relatively recently, so it is
spared this problem).  To be efficient, you need to run "bzr log" for
a certain range of revisions.

"C-x v g" is fine only the first time.  Once you find the latest
revision that modified some line, and need to see the previous
revisions that modified the same line, you need to find the exact
revision number and use "-r before:", or else you will miss some of
the change history, e.g. if there were more than one change on the
branch before it was merged.

Like I said: possible, but harder.

> > different features and unrelated bugfixes.  If you regard any "sync
> > from gnulib" as a single self-contained changeset, then at least it
> > should be committed to mainline separately from other changes.
> 
> That would make sense, yes.  Although in the case of gnulib, most
> "syncs" happen because we import yet another module, so it's OK to do
> both "import foo module" and "sync" at the same time, but indeed, the
> commit message should indicate that a sync with gnulib took place.

Thanks, I hope this is acceptable.

Re: merge-commits policy

[Stefan Monnier]

>> As would searching the ChangeLog rather than "bzr log".
> Unfortunately, only some of these updates are reflected in
> doc/misc/ChangeLog, and some aren't, so this is unreliable.

That's a problem that needs fixing, then.

>> Personally I would use C-x v l and/or C-x v g from the texinfo.tex buffer.

> "C-x v l" takes ages for veteran files (because texinfo.tex was
> "cvs mv"ed, it appears to have been born relatively recently, so it is
> spared this problem).  To be efficient, you need to run "bzr log" for
> a certain range of revisions.

> "C-x v g" is fine only the first time.  Once you find the latest
> revision that modified some line, and need to see the previous
> revisions that modified the same line, you need to find the exact
> revision number and use "-r before:", or else you will miss some of
> the change history, e.g. if there were more than one change on the
> branch before it was merged.

> Like I said: possible, but harder.

Yup.  These are problems with Bazaar (and/or with vc-bzr.el).

>> > different features and unrelated bugfixes.  If you regard any "sync
>> > from gnulib" as a single self-contained changeset, then at least it
>> > should be committed to mainline separately from other changes.
>> That would make sense, yes.  Although in the case of gnulib, most
>> "syncs" happen because we import yet another module, so it's OK to do
>> both "import foo module" and "sync" at the same time, but indeed, the
>> commit message should indicate that a sync with gnulib took place.
> Thanks, I hope this is acceptable.

It is.


        Stefan