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

From: Benoit Pierre <benoit.pierre@gmail.com>
Date: Mar 11, 2014, 17:56 UTC
Message-ID: <CA+SSzV04KCGyTkFTtSx_9sYAZyh=UzHOPV6tf9JT82w_DsSLHg@mail.gmail.com>
In-Reply-To: <20140310200756.GC24568@sigill.intra.peff.net>

On Mon, Mar 10, 2014 at 9:07 PM, Jeff King <peff@peff.net> wrote:

Show 24 quoted lines

> On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:
>
>> 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:

Show 5 quoted lines

> 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 <bonzini@gnu.org>
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 <gitster@pobox.com>

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?