On-branch topic description support?

I've been playing with this idea from time to time, but recently I started seeing a related discussion on the b4 front, so I thought I would throw it out and how people would think.

We made "git merge" not to silently commit the result, but open the editor to encourage the integrator to describe what the topic is about. We also made "git format-patch" prepare [PATCH 0/n] aka "cover letter" so that the author of the patch series can express what the overall topic is about. What the author should say in the cover letter very much overlaps what the integrator wants to have in the log message of the commit that merges the topic to the integration branch.

But there are two (and half) links from format-patch to that merge commit that are missing.

 - You cannot prepare the cover letter material while working on the
   topic---instead, you have to write one by editing the output from
   "format-patch --cover-letter";

 - "git am" at the receiving end discards the cover letter when
   queuing the e-mailed patches to a topic.

 - "git merge" cannot take advantage of the cover letter that was
   discarded when the topic was queued.

So, here is how I would imagine a slightly better world may work.

 * When you are almost finished with the initial draft of your
   topic, you'd write a cover letter, and record it as the log
   message of an empty commit at the tip of the topic.  As you go on
   polishing the topic with "rebase -i", the empty commit would be
   kept (if we currently lose an empty commit by default, we may
   need to teach "rebase -i" to special case an empty commit at the
   tip of the range to make this convenient to use), and you would
   keep it up to date as you update the topic.

 * "git format-patch" would notice that the topic has such an empty
   commit at the tip, and use the log message from it to
   pre-populate the cover letter.

 * "git am" would learn an option to save the cover letter [0/n] and
   create such an empty commit at the tip of the branch.

 * "git merge" would learn an option to recognize that the branch
   being merged has such an empty commit at the tip, and instead
   merge the parent of the tip of the branch into the integration
   branch, while using the log message of the discarded tip commit
   in the log message of the merge itself.

Yes, there is "git config branch.mytopic.description" that helps when pre-populating the cover letter, but that only helps at the origin, and it is not shared between your personal repositories. If you have the draft of the cover letter as part of the branch, you can push/fetch them around just like all the "real" commits you are working on.

Regardless of where the cover letter comes from, the changes to "am" and "merge" dreamed above in this message would be useful, and that is the primary reason why I am envisioning that right at the origin having the topic description as an empty commit at the tip would be the most convenient. It would match the shape of the history at the author side and at the side who runs "git am".

On 7/21/2022 7:40 AM, Junio C Hamano wrote:
 > I've been playing with this idea from time to time, but recently I
 > started seeing a related discussion on the b4 front, so I thought I
 > would throw it out and how people would think.
 >
 > We made "git merge" not to silently commit the result, but open the
 > editor to encourage the integrator to describe what the topic is
 > about.  We also made "git format-patch" prepare [PATCH 0/n] aka
 > "cover letter" so that the author of the patch series can express
 > what the overall topic is about.  What the author should say in the
 > cover letter very much overlaps what the integrator wants to have in
 > the log message of the commit that merges the topic to the
 > integration branch.
 >
 > But there are two (and half) links from format-patch to that merge
 > commit that are missing.
 >
 >  - You cannot prepare the cover letter material while working on the
 >    topic---instead, you have to write one by editing the output from
 >    "format-patch --cover-letter";

This bothered me for a long time. Across every "rebase -i", I could see a chance to modify the cover letter, but only right before sending the patch I got the chance to compose a summary of what I've done in the series, and that often makes me forget about things that should be in the cover letter. And I have to keep a draft somewhere to record the loose thoughts along the way.

 >  - "git am" at the receiving end discards the cover letter when
 >    queuing the e-mailed patches to a topic.
 >
 >  - "git merge" cannot take advantage of the cover letter that was
 >    discarded when the topic was queued.
 >
 > So, here is how I would imagine a slightly better world may work.
 >
 >  * When you are almost finished with the initial draft of your
 >    topic, you'd write a cover letter, and record it as the log
 >    message of an empty commit at the tip of the topic.  As you go on
 >    polishing the topic with "rebase -i", the empty commit would be
 >    kept (if we currently lose an empty commit by default, we may
 >    need to teach "rebase -i" to special case an empty commit at the
 >    tip of the range to make this convenient to use), and you would
 >    keep it up to date as you update the topic.
 >
 >  * "git format-patch" would notice that the topic has such an empty
 >    commit at the tip, and use the log message from it to
 >    pre-populate the cover letter.

It sounds pretty neat to have this on-the-go topic recorder :-)

-- Thanks, Shaoxuan

On Wed, Jul 20, 2022 at 5:08 PM Junio C Hamano <gitster@pobox.com> wrote:

No need for rebase changes here. By default, it preserves commits which start empty, and only drops commits which *become* empty.

(If one wants to drop commits that start empty, you can pass `--no-keep-empty`. If you want to keep commits that become empty, you can use either `--empty=keep` or `--empty=ask`.)

Seems mostly reasonable, but I'm curious about one thing:

The cover letter material often includes stuff that would not make sense for the merge message (e.g. "Rebased this version on master because of conflicts with ...", "Changes since v3", "I'm a little unsure of the logic in Patch 5", "Patch 3 might be controversial; take a look", etc.) Would there be some kind of syntax for specifying the part of the cover letter meant to be used in the merge commit message, or would we just start out with it pre-populating the commit message and expect the integrator to cull out the irrelevant parts?

Elijah Newren <newren@gmail.com> writes:

Yeah, I wanted to flesh out a bit more details on how this may work before writing more about it, but what I envisioned would go something along this line:

 * The authoring side would keep track of the description of the
   topic as a whole plus the description of inter-iteration changes,
   in the empty tip commit.  There may be some convention with a
   minimum mark-up (similar to how an innocuous three-dash line
   serves the "end of the message" marker in a patch e-mail) to
   separate the latter from the former.

 * "format-patch --cover-letter" would take the above, and add
   range-diff etc. that is easier to compute mechanically;

 * "am" may take the above, and keep the whole thing;

 * "merge" would keep the "main topic description" part and strip
   out everything after it, including the inter-iteration comparison
   and the range-diff, most likely by commenting the latter out when
   presenting it to the integrator.  So a lazy integrator can simply
   exit the editor to lose everything other than the topic
   description, and a more careful ones may refer to the supporting
   material to enhance/extend the topic description.

On Wed, Jul 20, 2022 at 10:25:19PM -0700, Elijah Newren wrote:

Maybe reuse the scissors line, but throw away stuff after the scissors? E.g.:

    Subject: [PATCH v2 0/3] Implement foo

    This implements foo that is needed to...
    ...
    ...
    ...

    Signed-off-by: De Veloper <dev@example.com>
    Link: https://example.com/foo

    -- >8 --

    Note: I'm not sure about....

    ---

    Changes in v2:
     - Rewrote using libfoo
     - ...

    ---
    Diffstat, range-diff, etc.

    ---
    base-commit: abcde
    change-id: abcde-foobarbaz

-K

On 21/07/2022 06:25, Elijah Newren wrote:

If one has a plan in place for a series in preparation, I'd want to prepare the outline of the cover letter first, and then later let it float to the tip of the series after a rebase, and incorporate (meld) additional notelets recorded as additional empty commits.

On thought is to marry this with the `rebase --autosquash` mechanism that will rearrange commits (when enabled) so that we could have a "branchtip! " subject line, similar to the `fixup! / squash!` subject lines to assist in that rearrangement  of the series for transmission.

I'm not yet sure how the "branchtip" method would work with Pull Requests and the idea of actually having a push/pull exchangeable branch description - there's been a very recent discussion on the Git-user list [1] which started from a slightly different premise, but maybe has some of the same issues as persistent Change-ID's [2].

-- Philip

[1] https://groups.google.com/g/git-users/c/5RftRV1c5Zw/m/cpem3KL1AAAJ Dan Rosen 18/07/2022 [2] https://lore.kernel.org/git/bdbe9b7c1123f70c0b4325d778af1df8fea2bb1b.camel@that.guru/

Philip Oakley <philipoakley@iee.email> writes:

Yeah, people have different preferences, and "rebase -i" is probably a key ingredient to make it work. As long as the convention to have topic description at a fixed known place on the topic branch, be it at the top or at the bottom, is followed in the end result, other tools like format-patch, am and merge can be written to follow and take advantage that known structure/topology of the topic branch.

I think what I described for "merge" would work well for pull requests (not the traditional e-mail kind of "please pull" we see on the kernel mailing list, but the GitHub kind). Merge the topic proper, excluding the tip "empty" commit that is there only to hold the description of the topic, into the history, and use the log message from the empty commit the procedure discards as the log message for the resulting merge.

Junio C Hamano <gitster@pobox.com> writes:

An issue that needs to be worked out at the Porcelain level is that this layout will make "git branch --[no-]merged master", which is an effective way to list what's in and what's left out of the 'master' integration branch, more or less useless. A topic branch with the cover letter commit at the tip may want to be merged to 'next' and then after cooking for a while merged down to 'master', and each time it gets merged, the merge has to leave the cover letter commit alone. In spirit, such a merge commit merges the topic "fully", but at the topological level, the topic is not (and will never be) merged fully, and "git branch --merged next" will not show.

I guess once the topic hits the oldest integration track that it meant to go, we could "pop" the cover letter commit out of the topic, and at that point it would appear to have been merged to 'master' as well as to 'next'. But it would be nice if we did not have to discard the cover letter commit only to please "git branch". IOW, it would be helpful to teach "git branch --[no-]merged" to compute the "right thing" in such a layout.

On July 21, 2022 11:06 AM, Junio C Hamano wrote:

layout will

what's in

letter

for a

to

the

never be)

go, we

would

be nice if

branch".

I might be missing something here, but could not a description follow similar semantics to how git describe works on tags? Anchor the description on the commit where the branch was spawned, then determine the appropriate description based on things like --first-parent. Again, I'm probably in the weeds. --Randall

On Thu, Jul 21, 2022 at 11:29:20AM -0400, rsbecker@nexbridge.com wrote:

I think the trouble is that "commit where the branch was spawned" is not reliable due to:

- rebasing the work on a different branch
- reordering commits (including the first commit in the series)

This is the main reason why I actually want to stick the empty commit with the cover letter not at the tip of the branch, but at the start of the series, so as to clearly demarcate where the series of commits we're interested in starts (at least while the author is working on them -- when it sent off upstream that commit can get removed/moved).

-K

On July 21, 2022 11:40 AM, Konstantin Ryabitsev wrote:

That process may be more suitable outside the git team where merge --squash is commonplace and the whole series goes into next, main, maint as a single commit as the empty commit will simply vanish. Although, with that said, for the git team's committer process, having a starting point commit in the history also has some obvious advantages, for example, starting and ending points in history for git bisect when things go terribly wrong. --R

On Wed, Jul 20 2022, Junio C Hamano wrote:

Isn't thu DAG topology of creating a merge that encompases the given range a more clear marker for this sort of thing?

	# Create a topic branch
	git checkout -b topic -t origin/master
	for f in A B C; do echo $f >$f && git add $f && git commit -m$f; done

	# I forget if there a way to make the porcelain do this, not this or --ff...
	git merge @{u}
        # ... that's better!
	git reset --hard $(echo My CL here | git commit-tree -p @{u} -p HEAD HEAD:)

To steal and amend a diagram from git-merge(1), we now have turned this:

              A---B---C topic
             / 
        X---Y
             \
              master

Into this:

              A---B---C---M topic
             / \         /
        X---Y   ---------
             \
              master

I vaguely recall having actually used this for something in the past.

I tried now with "git rebase --rebase-merges -i", and it supports it properly, i.e. I could re-arrange it so that it's:

              A---C---B---M topic
             / \         /
        X---Y   ---------
             \
              master

And it still gave me a merge commit whose parents were my branch tip, and my @{upstream}.

The advantage of doing it this way is that it's obvious from the topology what the lower bound of this "summary so far merge" is, with an empty commit on top it:

 A) isn't distinguishable from other uses of empty commits, but this
     merge structure is rather unusual, so it sets it apart.

 B) You don't know what the lower bound is if your @{upstream} ref
    moves.

    I could see it being useful to e.g. offer to update the CL if
    there's conflicts, or if upstream has integrated part of my commits.

On Thu, Jul 21, 2022 at 04:53:18PM +0200, Ævar Arnfjörð Bjarmason wrote:

This is clever, but it has a hard restriction that nothing happens to A:

- you can't move it so it's B---A---C---M
- you can't squash A+B

Unless all the tools are taught to properly modify the merge commit.

Or am I not reading this right?

-K

Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:

I think the drawing is about a two-commit topic that has B and C

Or the drawing is wrong and M's other parent should have been Y. IOW, "git merge-base M^1 M^2" is the bottom of the topic (and our ranges always exclude the bottom end, X..Y does not include X but does include Y). "git rev-list M^1...M^2" are the commits on the topic.

Then you can shuffle A--B--C in whatever way you like with "rebase -i" or "filter-branch", using "git merge-base M^1 M^2" as the base.

On Thu, Jul 21 2022, Junio C Hamano wrote:

Yes, those last two should be:

              A---B---C---M topic
             /           /
        X---Y------------
             \
              master

And:

              A---C---B---M topic
             /          /
        X---Y-----------
             \
              master

I.e. you could re-arrange it so that it's:

              C---B---A---M topic
             /          /
        X---Y-----------
             \
              master

Or whatever, sorry about that mix-up.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

If you mean your topic have 3 commits, then I think you want the first parent of M to be Y, not A, but I agree the above arrangement can also represent what I was discussing.

The thing is, it is unclear if such an artificial merge is an easy thing to maintain by individual contributors. "rebase -i" with some extra options people are not familiar with may be able to reproduce the topology without losing information after you accumulate more commits on top of M (e.g. you build D and E on top of M and now you want to make it A-B-C-D-E merged into Y with M that records the tree of E; you'd amend M's message to talk also about D and E), but "it is possible" is different from "people are comfortable doing".

If M did not have a backpointer to the other parent, and instead were an empty commit, then you recreated what I wrote in the message you are responding to.

On Thu, Jul 21 2022, Junio C Hamano wrote:

Yes, sorry. The diagram is incorrect, but not the commands I posted, i.e. one of the parents is "master".

Yes, it's magical, but so is an empty commit having a new "CL" meaning. I'd think both would require relevant tooling to be aware of them.

But this is worse in that "git rebase" will get rid of it by default.

Yes, that's the difference between the two.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

FWIW, I think I like this much better than Konstantin's "there is an empty commit at the bottom", for exactly the same reason why I like the original "empty commit at the tip", i.e. simply because we can strip away the "extra" commit that holds the topic description without having to change all the "real" commits.

In fact, I thought one of the newer "b4" subcommands that is used to accept a patch series with a cover letter creates exactly this sort of topology, when told to apply the topic to the tip of the integration branch? I do prefer to see a tool that it can use the same "format" of data for both input and output, and I think your "there is a redundant merge commit at the tip, with topic description" topology can be that input format, which would be recreated at the receiving end if the receiver happens to have the same base when applying it and the final integration branch was also at the same base.

On Thu, Jul 21, 2022 at 11:02:26AM -0700, Junio C Hamano wrote:

I'm happy to consider alternatives if I can have a reliable way of tracking "the series we're working on starts at this commit". I know that this is antithetical to git's design, but I also can't think of anything else that reliably survives rebases.

Yes, you're thinking "b4 shazam -M", which does a "merge --no-ff" with the cover letter as the template merge commit. I guess it does create this exact structure:

             C---B---A---M
             /          /
        X---Y-----------

I'll play with tip merge commits and see how well my "survives rebases" tests go.

-K

On Thu, Jul 21 2022, Konstantin Ryabitsev wrote:

Aside from any discussion of how such a "cover letter" thingy gets represented in the DAG, I wonder if rebase should be less aggressive about wiping away merge commits in general.

It's really handy to e.g. fix something where you've got repeated merges of the "master" into your branch.

But if you've got some novel merge structure that's existing purely within your branch perhaps we should at least show some advice() before proceeding, or suggest --rebase-merges...

I haven't thought through all the edge cases with that, just food for thought.

On Thu, Jul 21, 2022 at 02:26:45PM -0400, Konstantin Ryabitsev wrote:

Hmm... well, it works great as long as you remember to always use --rebase-merges. The moment you forget to pass -r to "git rebase", your cover letter commit disappears, and I'm not sure this is going to work for what I am trying to do (which is to make the process of submitting patch series simpler). I know it's possible to configure git so that "git pull --rebase" preserves merges, but there doesn't appear to be a way to force "git rebase -i" to do the same without the -r flag. Also, "rebase -ir" looks really different when there is a tip merge commit, which will probably also be confusing to newbies just starting out with rebase workflows.

So, I'm not sure that at this time this is objectively "better" than keeping the cover letter in an empty commit at the start of the series. :-/

-K

Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:

Now you are discussing this on the git mailing list, you do not necessarily have to take the existing behaviour of Git as given.

For example, I do not think it is unreasonable to teach "git rebase [-i]" to special case a certain merge commit in the rebased range without any extra option, as long as the criteria to pick such a special "merge" is specific and narrow enough (a two-parent merge M whose tree matches that of one parent's tree (say M^1^{tree}), the other parent (say M^2) is an immediate ancestor of the bottommost commit of the range being rebased, or something like that). And the way you "special case" it does not have to be tied to the way the "-r" option handles it, either.

A possible design could go like this:

 * we recognize such a special merge commit;

 * we rebase the rest of the range, pretending as if that merge
   commit did not exist and instead its children are all direct
   child of one of its parent (say M^1), using the options given (so
   "-r" would affect how other merges in the rebased range is
   handled).

 * after everything is done, we create a new empty merge commit at
   the top, merging the bottom of the range and the tip of the range
   as its parents, using the log message from original M.  This can
   be done totally outside of the regular "rebase" machinery.

Such a change to existing behaviour is well within the scope of "On-branch topic description support", I would think.

On Thu, Jul 21, 2022 at 01:01:16PM -0700, Junio C Hamano wrote:

Indeed, I was mostly observing about how it works right now -- please don't take this as complaining. :)

I agree, this would be great. If this is in place, then it would certainly bring us closer to managing "proposed change" series completely in-git.

One thought that comes to mind -- perhaps it would be easier not to track the special commit, but designate the branch as a special "proposed changes" branch that could even "hide" the cover-letter merge commit (CLM) from most operations. It's still a simple two-parent merge commit, so it can be pushed/pulled with any existing remotes without requiring any changes on the server side, but operations like adding a regular new commit to the tip of the "proposed changes" branch would automatically move the merge commit so it's back at the tip. This way "not sure what you're trying to do" situations like this would be avoided:

  A--B--C--CLM--D--E
 /          /
Y-----------

-K

Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:

Sorry, you lost me. It is quite clear what the user is trying to do in this case. After preparing the initial cut of the topic that has three commits, A, B, and C, and writing about them in CLM, there were fix-ups and enhancements needed, which were implemented in D and E. The branch is waiting to be rebased into

   A--B--C--D--E--CLM'
  /               / 
 Y----------------

which gives you a chance to talk also about D and E in the updated iteration of CLM.

Whether the "cover letter material in an empty commit at the tip" is a single-parent commit or a needless merge, there will be fallouts, such as "git branch --[no-]merged" becoming much less useful. I am sure there will be many others once we start seriously exploring these approaches.

On Wed, Jul 20, 2022 at 04:40:52PM -0700, Junio C Hamano wrote:

Thanks for the idea.

The alternative would be "commit groups", which are named and annotated range of commits. The group description can contain cover letter content. This way, integrators can still have a description of merged commits without having an empty commit. Commit groups are particularly useful for rebase-then-merge workflow, where topic branches are rebased first before being merged into trunk.

Thanks.