Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

On Mon, Mar 10, 2014 at 9:07 PM, Jeff King [off-list ref] wrote:

On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:

quoted

Don't change git environment: move the GIT_EDITOR=":" override to the
hook command subprocess, like it's already done for GIT_INDEX_FILE.
[...]

This is a lot of change, and in some ways I think it is making things
better overall. However, the simplest fix for this is basically to move
the setting of GIT_EDITOR down to after we prepare the index.

Jun Hao (cc'd) has been preparing a series for this based on the
Bloomberg git hackday a few weeks ago, but it hasn't been sent to the
list yet.

Commits are here:

  https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing

if you care to look. I'm not sure which solution is technically
superior, but it's worth considering both.

I regret not encouraging Jun to post to the list sooner, as we might
have avoided some duplicated effort. But that's a sunk cost, and we
should pick up whichever is the best for the project.

Replying here instead of to Jun Hao message (I'm not subscribed to the
mailing list, so I did not received it):

Jun Hao wrote:

I like the idea that the environment setting should be done in one
place. Just not sure run_hook is the right place tho. If user doesn't have
any hook setup, will this kick in?
One more question, will this work for dry run? Or dry run doesn't matter
in this case?

According to the original commit, the change to GIT_EDITOR is only
here for hooks:

commit 406400ce4f69e79b544dd3539a71b85d99331820
Author: Paolo Bonzini [off-list ref]
Date:   Tue Feb 5 11:01:45 2008 +0100

    git-commit: set GIT_EDITOR=: if editor will not be launched

    This is a preparatory patch that provides a simple way for the future
    prepare-commit-msg hook to discover if the editor will be launched.

    Signed-off-by: Junio C Hamano [off-list ref]

So there is really no reason to set it earlier, and not just in the
hook subprocess environment.

Regarding dry run: the bug is present, and my patch fix it too (but is
missing a test for this).

As to which patch is better: it's really not for me to decide! It's a
question for the maintainer(s), Jun Hao patch is sure much smaller and
simpler, mine is more involved and I believe cleaner in the long term:
there is no risk of another part of the commit process to be impacted
by the change of environment. Also note that my patch changes the
merge builtin too: GIT_EDITOR will not be overriden if an editor will
be launched (when used with --edit).

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?