Re: [PATCH 1/4] rebase -i: add ack action

From: Michael S. Tsirkin <mst@redhat.com>
Date: Apr 11, 2016, 11:24 UTC
Message-ID: <20160411141428-mutt-send-email-mst@redhat.com>
In-Reply-To: <alpine.DEB.2.20.1604111239100.2967@virtualbox>

On Mon, Apr 11, 2016 at 01:02:07PM +0200, Johannes Schindelin wrote:

Show 31 quoted lines

> Hi Michael,
> 
> On Sun, 10 Apr 2016, Michael S. Tsirkin wrote:
> 
> > This implements a new ack! action for git rebase -i
> > It is essentially a middle ground between fixup! and squash!:
> > - commits are squashed silently without editor being started
> > - commit logs are concatenated (with action line being discarded)
> > - because of the above, empty commits aren't discarded,
> >   their log is also included.
> > 
> > I am using it as follows:
> > 	git am -s < mailbox #creates first commit
> > 	hack ...
> > 	get mail with Ack
> > 	git commit --allow-empty -m `cat <<-EOF
> > 	ack! first
> > 
> > 	Acked-by: maintainer
> > 	EOF`
> > 	repeat cycle
> > 	git rebase --autosquash -i origin/master
> > 	before public branch push
> > 
> > The "cat" command above is actually a script that
> > parses the Ack mail to create the empty commit -
> > to be submitted separately.
> 
> This looks awfully complicated, still, and not very generic.
> 
> How about making it easier to use, and much, much more generic, like this?

I can look at using a different syntax but the below does not support the workflow I described, which is a standard email based one: get email, handle it.

Show 9 quoted lines

> 1. introducing an `--add-footer` flag to `git commit` that you could use
> like this:
> 
> 	git commit --amend --add-footer "Acked-by: Bugs Bunny"
> 2. introducing an `--exec-after` flag to `git commit` that would be a new
> sibling of `--fixup` and `--squash` and would work like this:
> 
> 	git commit --exec-after HEAD~5 \
> 		'git commit --amend --add-footer "Acked-by: Bugs Bunny"'

But it wouldn't address my use-case where I get an ack by email. If I have to dig up the relevant commit(s) by hand anyway, then what was the point?

Show 8 quoted lines

> 
> (it should imply `--allow-empty`, of course, and probably even fail if
> anything was staged for commit at that point.) The commit message would
> then look something like
> 
> 	exec-after! Fix broken breakage
> 
> 	git commit --amend --add-footer "Acked-by: Bugs Bunny"

So if I happen to fetch a branch from someone and rebase it, stuff gets auto-executed on my local system? That looks scary.

> This way would obviously benefit a lot more users.

It might benefit others who have the commit handy but it does not look like it helps email based workflow.

Show 13 quoted lines

> For example, you could
> easily say (and alias)
> 
> 	git commit --amend --add-footer 'Reviewed-by: Arrested Developer"
> 
> i.e. support all kind of use cases where developers need to slap on
> footers in a quick & easy way.
>
> And the --exec-after option would obviously have *a lot* more use cases
> than just squashing in ACKs.
> 
> Ciao,
> Johannes

So far I only see examples of adding footers. If that's all we can think up, why code in all this genericity? All these small scripts scattered around just make things hard to use, and add security issues.

-- 
MSR