From: Benoit Pierre Date: Tue, 11 Mar 2014 17:56:02 GMT Subject: Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m" Message-ID: In-Reply-To: <20140310200756.GC24568@sigill.intra.peff.net> On Mon, Mar 10, 2014 at 9:07 PM, Jeff King wrote: > 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: > 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 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 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?