git ubuntu merge finish may generate an invalid debian/changelog

[Steve Langasek]

https://wiki.ubuntu.com/UbuntuDevelopment/Merging/GitWorkflow#Git_workflow_for_merging documents that one of the steps for 'git ubuntu merge finish' is 'git reconstruct-changelog new/debian'. I'm unable to find where this command is supposed to come from (both 'git reconstruct-changelog' and 'git ubuntu reconstruct-changelog' are unknown here), but the result here is a changelog that is invalid:

commit 87ac7edb0949744d0e027f2fe3b0489e193d8508
Author: Steve Langasek <email address hidden>
Date: Wed Oct 25 20:51:35 2017 -0700

    reconstruct-changelog

diff --git a/debian/changelog b/debian/changelog
index 35e886c..457bc96 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+memcached (1.5.0-1ubuntu1) bionic; urgency=medium
+
+ * Merge with Debian unstable. Remaining changes:
+added patch to show distribution on version
+Force alignment on in configure.ac
+disable unreliable test.
+
+ -- Steve Langasek <email address hidden> Wed, 25 Oct 2017 20:51:35 -0700
+
 memcached (1.5.0-1) unstable; urgency=medium

   * New upstream release (Closes: #853544, #869479)

It appears that git-ubuntu makes assumptions about the formatting of commit messages that are not spelled out anywhere and are not otherwise consistent with git conventions.

[Christian Ehrhardt  (paelzer)]

Hi Steve,
I just yesterday - hours before your bug - changed the wiki wording slightly.

"This effectively wraps all of the following steps. From git those can be run manually instead, if desired - in the snap not all subcommands might be exposed."
...
"While doing the former the Changelog is constructed out of the commit messages you had in your branch. Please check debian/changelog if they are sufficient for an upload."

There are a set of bugs for "better generated changelog" already - I know of bug 1705214 and bug 1709785

I think Nish shall decide if we ever want to expose merge-changelog, reconstruct-changelog steps again (e.g. by implementing bug 1721844). If so we can update the doc for that, if not we fully remove those parts.

[Steve Langasek (vorlon)]

On Thu, Oct 26, 2017 at 06:01:48AM -0000, ChristianEhrhardt wrote:
> Hi Steve,
> I just yesterday - hours before your bug - changed the wiki wording slightly.

> "This effectively wraps all of the following steps. From git those can be run manually instead, if desired - in the snap not all subcommands might be exposed."
> ...
> "While doing the former the Changelog is constructed out of the commit messages you had in your branch. Please check debian/changelog if they are sufficient for an upload."

Yes, I read those words before I filed this bug. Documentation does not
excuse the tool generating and committing a changelog that fails
dpkg-parsechangelog.

> There are a set of bugs for "better generated changelog" already - I
> know of bug 1705214 and bug 1709785

Yes, I saw those and this bug doesn't appear to be a duplicate of either.

[Nish Aravamudan (nacc)]

On Wed, Oct 25, 2017 at 8:57 PM, Steve Langasek
<email address hidden> wrote:
> Public bug reported:
>
> https://wiki.ubuntu.com/UbuntuDevelopment/Merging/GitWorkflow#Git_workflow_for_merging
> documents that one of the steps for 'git ubuntu merge finish' is 'git
> reconstruct-changelog new/debian'. I'm unable to find where this
> command is supposed to come from (both 'git reconstruct-changelog' and
> 'git ubuntu reconstruct-changelog' are unknown here),

Right, we are fixing that in the snap (they will be exposed as
git-ubuntu.git-{reconstruct-changelog,merge-changelogs}. See the bug
Christian referred to.

> but the result
> here is a changelog that is invalid:
>
> commit 87ac7edb0949744d0e027f2fe3b0489e193d8508
> Author: Steve Langasek <email address hidden>
> Date: Wed Oct 25 20:51:35 2017 -0700
>
> reconstruct-changelog
>
> diff --git a/debian/changelog b/debian/changelog
> index 35e886c..457bc96 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,3 +1,12 @@
> +memcached (1.5.0-1ubuntu1) bionic; urgency=medium
> +
> + * Merge with Debian unstable. Remaining changes:
> +added patch to show distribution on version
> +Force alignment on in configure.ac
> +disable unreliable test.
> +
> + -- Steve Langasek <email address hidden> Wed, 25 Oct 2017 20:51:35 -0700
> +
> memcached (1.5.0-1) unstable; urgency=medium
>
> * New upstream release (Closes: #853544, #869479)
>
> It appears that git-ubuntu makes assumptions about the formatting of
> commit messages that are not spelled out anywhere and are not otherwise
> consistent with git conventions.

Yes, we know this is a rough spot. Currently, we are sensitive to formatting.

I believe what you are suggesting is we should try to do the
reconstruct step, and if it fails to be parseable, then it should spit
an error instead of committing at all. I can see the logic behind such
a check, and with the latest code this is easier to do without
affecting the current working tree.

Thank you for this suggestion.

At the same time, we have not yet decided on how to make this less
painful in general, without building a syntax tree of sorts.

We, could, as a a first pass, parse every git commit message for
leading fields indicating indentation, '*', '+', '-' and just trust
those are correct.

For instance, in your case, it's not generically possible to know what
the nesting of each line is (afaict). Perhaps we can make an
assumption in the merge case (note that this is why I dropped
git-reconstruct-changelog from the snap originally, as the command
does not need to be run on its own any longer).

Finally, while it's not well written or explicit enough:

"This replays the commit messages for all commits since new/debian on
the current branch into debian/changelog in a new entry."

Thank you again for this bug report!

[Nish Aravamudan (nacc)]

Also, an FYI, I just found a corner-case bug in the edge snap (and
fixed it) for the merge finish step that was breaking our changelog
reconstruction.