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

[Jun Hao <achilles.hao@gmail.com>]

Jeff King <peff@peff.net> writes:

> 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.
>> 
>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>> ---
>>  builtin/checkout.c                |  8 +++----
>>  builtin/clone.c                   |  4 ++--
>>  builtin/commit.c                  | 35 ++++++++++++++++++++++++-------
>>  builtin/gc.c                      |  2 +-
>>  builtin/merge.c                   |  6 +++---
>>  commit.h                          |  3 +++
>>  run-command.c                     | 44 ++++++++++++++++++++++++++++-----------
>>  run-command.h                     |  6 +++++-
>>  t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
>>  9 files changed, 79 insertions(+), 31 deletions(-)
>
> 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.
>
> -Peff

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?

Thanks - Jun