Re: lily-git: amending patches

[Carl Sorensen]

On 12/31/09 12:46 AM, "Graham Percival" <address@hidden> wrote:

> We either need one more button for lily-git, or one less.
> 
> Taking a completely hypthetical example, suppose a new doc
> contributors writes a nice addition for the docs, but only uses one
> space after a period (the heretic!).  I ask for an updated patch.  Our
> fearless contributor edits the file accordingly, then presses "2.
> local commit" and "3. generate patches"  (or whatever the buttons
> are).

I previously had an "Amend previous commit" button, but took it out because
it called the editor (and we don't want that happening).

I now found the proper git command to use to make the Amend button work,
so I've added it.

> 
> He now has two patches; one of which is just fixing the mistakes in
> the previous one.
> 
> 
> I see two possibilities:
> - "one less button": eliminate the "2. local commit"; just make button
> 3 produce a patch including _any_ difference between the source tree
> and origin.

I don't like this idea; I think that creating a patch should be a separate
step.

> - "one more button": add a button for "un-commit", which takes the
> patch out of the git history, but leaves the files modified.  The
> contributor can then edit the file and do a "local commit" like
> normal.
> This would require a prompt for which local commit to un-commit.  I
> think we can assume that contributors can recognize the correct patch
> to un-commit; they'll generally only have one or two.  And if they
> *do* get confused, it'll be a good lesson to write better changelog
> message.  :)

I took an intermediate approach.  The extra button isn't "un-commit", it's
"wrap the current changes into the previous commit".  There isn't as much
flexibility as the "un-commit" button, but it should handle the hypothetical
situation quite well.

User sends a patch.  You send back "Make a couple of changes."  User makes
the changes, then clicks "Amend previous commit", followed by "Make patch".
And now there is just one patch to be applied.

I think this works.

Thanks,

Carl