Keep original author with git merge --squash?

I frequently find myself using `git merge --squash` to combine a series of commits by the same author into one.

(For example, I fetch my project's GitHub PRs into my repo. Frequently, a PR consists of the original PR (with a good description) followed by a few follow-ups based on feedback from me. While I'd prefer that the submitter amended their original commit instead of making it my job, this is rare. And I don't feel that it's valuable to my project's git history to contain all the intermediate stages of code review --- it's usually just one commit.)

So `git merge --squash origin/pr/1234` is a really convenient command here... except for one thing: it sets the author as me. I always have to manually find the author line and make sure to pass it to --author (perhaps with --amend).

What would people think of a flag (or a config value) that means "if all merged commits are by the same author, use that author for the resulting commit instead of the default author"?

(I'm not sure if this should be a flag to --squash or to commit. Maybe `git merge --squash`; `git commit --use-squashed-author`? Seems like it should be not too hard to implement; SQUASH_MSG is pretty parseable. Or just a config value.)

--dave

On Wed, Feb 11, 2015 at 09:21:04AM -0800, David Glasser wrote:

It sounds like "git commit -c" is close to what you want, which will pull the author and commit message from a particular commit. But I don't think there is a convenient way to name the commit in your case (it is likely to be the first commit on the branch you are squash-merging, but there isn't a shorthand for that).

I assume you are already munging in your editor the template provided by "git commit" after the squash? What would be really nice, IMHO, is if there was a way to set the author during that edit (e.g., by moving one of the "Author:" lines to the top of the file). That would cover your use case, I think, and would also be useful in general.

-Peff

On 02/12/2015 10:28 AM, Jeff King wrote:

If only Git supported options on todo list lines [1], this could be implemented via a "--use-author" option:

    pick --use-author 1234556 blah
    squash 84392ab etc
    fixup 49106a5 another

Happily, this would work with fixup, too, without forcing the user to go into the editor. Also, it wouldn't require metadata to be read in-band from the commit message.

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/255410

On Thu, Feb 12, 2015 at 12:35:48PM +0100, Michael Haggerty wrote:

Yes, that would be nice, but I don't think David is using a sequencer todo list here at all. It's just:

  git merge --squash pr/100
  git commit

-Peff

On Thu, Feb 12, 2015 at 4:12 AM, Jeff King <peff@peff.net> wrote:

That's correct. I certainly know how to do

  git checkout pr/100
  git rebase -i master  # set things to squash, etc
  git checkout master
  git merge pr/100  # or cherry-pick or whatever

And that's how I always used to do it. But `merge --squash` is so much more convenient... except for the minor wrinkle of needing an extra manual step to give the original author their credit.

--dave

Jeff King <peff@peff.net> writes:

I thought David was primarily interested in the case where a branch authored by a single person, so specifying the tip of the branch being "merged" would be sufficient, no?

On Thu, Feb 12, 2015 at 12:18 PM, Junio C Hamano <gitster@pobox.com> wrote:

Well, using -c appears to override SQUASH_MSG entirely; it replaces the message as well as the author. Often I do want to make my own message based on all the messages provided by the submitter. (And typically the branch's tip is the least useful message anyway: it's usually something like "respond to code review".)

--dave

David Glasser <glasser@davidglasser.net> writes:

I wonder if there is a bug in the workflow. Isn't the contributor forcing more work to the integrator that way and making it a responsibility of the integrator to squash multiple commits into one, instead of asking a cleaned-up branch to be merged in the first place? It is a key to keep your project scalable to push as much work out of the integrator's plate to the contributors' plates.

But that is an unrelated tangent. Among the ideas floated in the thread, I tend to like something like what Peff mentioned earlier.

I don't know if the exact solution Peff mentioned is good [*1*], but being able to do an equivalent of setting --author="<author>" and "--date=<date>" from the command line inside the log message editor would be a huge win.

[Footnote]

*1* You would see

	gostak: do not distim doshes unconditionally

	Usually it is a good idea to let gostak distim doshes,
        but it should cause the program segfault when there is
        no doshes to be distimmed.  Detect this case and error
        out.

	Signed-off-by: David Glasser <glasser@davidglasser.net>

        # Please enter the commit message ...
        #
        # Author:    David Glasser <glasser@davidglasser.net>
        # Date:      Thu, 12 Feb 2015 04:28:24 -0500
        # ...

when you start "git commit --amend" or "git merge --squash", and the proposal, as I understand it, is to allow you to do this:

        From: David Glasser <glasser@davidglasser.net>
        Date: Thu, 12 Feb 2015 04:28:24 -0500

	gostak: do not distim doshes unconditionally

	Usually it is a good idea to let gostak distim doshes,
        but it should cause the program segfault when there is
        no doshes to be distimmed.  Detect this case and error
        out.

	Signed-off-by: David Glasser <glasser@davidglasser.net>

        # Please enter the commit message ...
        #
        # ...

which is the same format of the _body_ of the message "git am" would take. The reason I am not so sure about such a change is that having to move and edit things would be error prone. Some may forget to do s/Author:/From:/ and we would end up having to support both. Some existing commit may validly have lines that begin with these tokens ("git am" does not have such a problem because it is not the underlying "git commit" or "git commit-tree" that interprets these in-body headers). Some users may forget to leave a blank line between the in-body headers and the subject line.

I suspect that it _might_ work better if we always look for the pair of "# Author: " and "# Date:" lines (taking the commentchar setting in to account, of course) immediately at the beginning of the trailing comment block, and if exists, use that to override the authorship identity. If the user does not do anything when running "git commit --amend", the resulting commit will get the original authorship. If the user strips all the lines in the trailing comment block, we would do the same thing as we have always done and retain the original authorship.

I dunno.

On Thu, Feb 12, 2015 at 1:23 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yes, I agree that the GitHub pull request workflow has some major disadvantages. It does a good job of enabling unsophisticated contributors who aren't git experts to propose changes to a codebase, but not a very good job of encouraging them to do the work of massaging what may be a series of changes as the patch undergoes code review into a single patch (or well-structured series of patches). At this point in time, the tradeoff of requiring a little more integrator work makes sense for my project. I'd rather spend my limited "educate potential contributors" energy on ensuring that they include actual reproductions with their bug reports than on getting them to do administrative git work that isn't hard for me.

So to be concrete: What I'm proposing (and I'm excited to implement it!) is the following:

When running "git commit" and:
- You've fallen into the case where the message was read from SQUASH_MSG
- You haven't used another method of specifying the author (--author,
  -C, -c, --amend)
- You have not specified --reset-author
- You have set the "commit.useSquashAuthor" option
- Before invoking prepare-commit-msg, all of the `Author:` lines found
  in SQUASH_MSG have the same value

then that author is used, as if it were specified with --author. (And this will show up, commented-out, at the bottom of COMMIT_EDITMSG.)

If you have the config option set but you don't want this logic to take place for a particular post-squash merge, just specify --reset-author.

I think this makes git more internally consistent, since it makes `merge --squash` act more like the squash rebase action. (Personally I'd be happy if this behavior was the default, but since it is not backwards-compatible I've included a config option in my proposal.)

(It is my understanding, based on reading the code, that the format of SQUASH_MSG is not user-configurable, and that scanning for Author: lines in it is straightforward.)

--dave

On Thu, Feb 12, 2015 at 2:16 PM, David Glasser <glasser@davidglasser.net> wrote:

OK, and to be very specific: I'm just proposing "literally the same text written after Author"; using mailmap to detect that multiple different Author lines are actually the same author could be a potential later improvement.

David Glasser <glasser@davidglasser.net> writes:

I actually was hoping that this would extend to cases other than "git merge --squash".

When running "git commit" and:

 - You didn't use a more explicit method of specifying the
   authorship identity (--author, --date, -C, -c --amend,
   --reset-author options, or environment variables GIT_AUTHOR_*);

 - You have commit.useAuthorFromEditorComment variable;

 - You have "# Author: " line that are identical in the result of
   the editor,

then use that author. That would allow "git commit --amend" to update a misspelled author name, for example.

Is that a bit too liberal?  Would it invite mistakes?

On Thu, Feb 12, 2015 at 02:34:43PM -0800, Junio C Hamano wrote:

I like this direction in general.

What happens if there is no "Author:" line in the output? Do we do the equivalent "--reset-author"? That seems slightly error-prone to me. It is not uncommon for me to delete to the end file in my editor to drop cruft (e.g., in an interactive rebase with a "squash" command, I very often drop the final commit message, and it is simpler to just delete to the end of file than to delete to the top of the comment block).

I wonder if we should have some markers in the commented-out section that indicate it even exists, like:

  # --- Lines in this section affect the commit authorship ---
  # Author: ...
  # ---

Of course that is nice when you are editing an existing Author line, but not so much when you have to remember to type that line yourself (because you are adding an author attribution when there was not one before).

So probably a saner thing is that a missing "Author:" line does nothing, and using "Author: " (with no text) does a reset.

Also, on the topic of "merge --squash". I never use it myself, but having experimented with it due to this thread, I found the template it sticks into COMMIT_EDITMSG to be horribly unfriendly for munging. For example, with two simple commits, I get:

    Squashed commit of the following:
    
    commit 6821a8ac920ed00675e4aec10dcef705211105cd
    Author: Jeff King <peff@peff.net>
    Date:   Thu Feb 12 17:39:28 2015 -0500
    
        commit subject 2
    
        commit body 2
    
    commit b0840bb4bbfe00b6ed8c7c4d483f11d126fa2d69
    Author: Jeff King <peff@peff.net>
    Date:   Thu Feb 12 17:39:28 2015 -0500
    
        commit subject 1
    
        commit body 1

I guess that is helpful if you want to keep a complete log of what got squashed, but I doubt that is the common case (if you did, then doing a real merge would probably be in order). But to munge that into a usable single commit message, I have to:

  1. Drop the header fields. We could mark these with "#" instead (which
     would also make the "# Author: " proposal here work.

  2. Reindent all of the actual message lines!

  3. Probably reorder the commit messages, since they are
     reverse-chronological here.

I would find something like:

    # commit b0840bb4bbfe00b6ed8c7c4d483f11d126fa2d69
    # Author: Jeff King <peff@peff.net>
    # Date:   Thu Feb 12 17:39:28 2015 -0500
    
    commit subject 1
    
    commit body 1

    # ... and then the second commit ...

much more friendly, and closer to what interactive rebase's squash does. It also raises a question for the proposal in this thread: if there are multiple "Author:" lines, which one do we take? The first, or the last? I think in the proposed chronological-order format I just showed, it would make sense to take the first.

-Peff

Jeff King <peff@peff.net> writes:

I've been assuming that we would do what the current code does. "git commit --amend" for example internally remembers who the original author was and uses that, without paying any attention to the result from the editor. If there is no "Author:", that would not change.

And I do not think we need to be able to say "Oops, I forgot to pass the --reset-author option from the command line", personally, so...

yes and

no (I do not think it is wrong per-se, but I do not think such a good idea).

I think it should show exactly the same thing as "rebase -i" squash insn would give you. People already know how to munge that, right?

I was siding with David's "pay attention to in-buffer Author: only when all of them agree". When squash-merging a branch with two or more authors, we would attribute the authorship silently and automatically to you if you do not do anything special otherwise.

Possible alternatives when multiple "Author:"s do not agree are:

 - use you who is playing the integrator;

 - use the tip;

 - use the one that most often appears; or

 - error out and ask the user to leave only one (or zero--if you
   want to take the authorship) by re-attempting "git commit".

On Thu, Feb 12, 2015 at 03:32:37PM -0800, Junio C Hamano wrote:

Fair enough. It is probably a minority use case, and one that is likely to cause confusion.

Yeah, that was exactly what I expected to see (but didn't). They should probably have the same format, though we may want to enhance both to contain more information (like author names).

That's probably reasonable. I was thinking more of a case where you made some fixups on top of somebody else's branch, and then used "git rebase -i" to squash them together. But I think we already use the authorship for the root of the squash in that case.

This case collapses nicely if we make a slight tweak to your proposed behavior (or maybe this is what you meant). If there are multiple authors listed, we behave as if none was listed. That would leave the authorship as it behaves today (with the author of the first commit) if you do nothing, or you can override it by dropping all but one.

-Peff

Jeff King <peff@peff.net> writes:

I actually was (and am still) wondering that "silently ignore all of them if there are multiple ones that contradict with each other" is a bad idea, and that was why the last item on the "possible alternatives" list was to error out and ask clarification.

On Fri, Feb 13, 2015 at 11:30:53AM -0800, Junio C Hamano wrote:

Normally I like "error out and ask the user" as an approach to avoiding mistakes, but I can think of two bad side effects:

  1. If we pre-populate the "# Author:" lines in "git merge --squash",
     then if I run "git commit" on the result and don't explicitly take
     an action to clean up those comment fields, I get an error.  That's
     kind of annoying.

  2. Dumping the user out of "git commit" with an error isn't very
     elegant. They may have put significant work into writing the commit
     message. It's saved there in COMMIT_EDITMSG, but what is the easy
     path to them repeating their action where they left off?

It seems like the potential for confusion comes from the same place as my complaint (1) above: the implicit-ness of the "# Author:" lines (git writes them, assumes you've looked at and manipulated them to your liking, and then reads them back in).

What if there was a step required of the user to say "really, I want to use this one"? Like converting s/Author/Set-Author/, or taking away the "#" comment character (though that has its own confusions, as you noted earlier).

-Peff

On Thu, Feb 12, 2015 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:

That does sound pretty good. And for the specific case of squash, what I would do would be (a) take one of the existing "Author: " lines and turn it into an "# Author:" line and (b) do the ordinary editing that I do to combine the existing messages?

(Maybe my proposed "detect that they're all the same" code would run at `merge --squash` time, producing a trailing "# Author:" line if all of the produced "Author: " lines were identical?)

--dave

On Thu, Feb 12, 2015 at 4:17 PM, David Glasser <glasser@davidglasser.net> wrote:

Or, well, as Peff suggested, maybe SQUASH_MSG should look exactly like the message that rebase's squash gives you, which in fact is a lot better: no random indent, "# Author", etc. So making your suggested change to parse "# Author" in `git commit`, plus a second change to make SQUASH_MSG look like rebase's squash, would achieve my goals.