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

2014-03-19 Thread Junio C Hamano
Torsten Bögershausen  writes:

>> +int run_hook_with_custom_index(const char *index_file, const char *name, 
>> ...)
>> +{
>> +const char *hook_env[3] =  { NULL };
>> +char index[PATH_MAX];
> Sorry being late with the review.
>
> Recently some effort has been put to replace the
>  "PATH_MAX/snprintf() combo" with code using strbuf.
>
> So I think for new developed code it could make sense to avoid
> PATH_MAX from the start.

Yes but because this is a compatibility wrapper for an existing
function that does the string[PATH_MAX] thing already, it would be
clearer to have such a conversion as a separate step.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-03-19 Thread Torsten Bögershausen

On 03/18/2014 11:00 AM, 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 
---
  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/t7514-commit-patch.sh |  4 ++--
  9 files changed, 80 insertions(+), 32 deletions(-)


[]

index 3914d9c..75abc47 100644
--- a/run-command.c
+++ b/run-command.c
@@ -760,13 +760,11 @@ char *find_hook(const char *name)
return path;
  }

-int run_hook(const char *index_file, const char *name, ...)
+int run_hook_ve(const char *const *env, const char *name, va_list args)
  {
struct child_process hook;
struct argv_array argv = ARGV_ARRAY_INIT;
-   const char *p, *env[2];
-   char index[PATH_MAX];

(Please see below)

-   va_list args;
+   const char *p;
int ret;

p = find_hook(name);
@@ -775,23 +773,45 @@ int run_hook(const char *index_file, const char *name, 
...)

argv_array_push(&argv, p);

-   va_start(args, name);
while ((p = va_arg(args, const char *)))
argv_array_push(&argv, p);
-   va_end(args);

memset(&hook, 0, sizeof(hook));
hook.argv = argv.argv;
+   hook.env = env;
hook.no_stdin = 1;
hook.stdout_to_stderr = 1;
-   if (index_file) {
-   snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-   env[0] = index;
-   env[1] = NULL;
-   hook.env = env;
-   }

ret = run_command(&hook);
argv_array_clear(&argv);
return ret;
  }
+
+int run_hook_le(const char *const *env, const char *name, ...)
+{
+   va_list args;
+   int ret;
+
+   va_start(args, name);
+   ret = run_hook_ve(env, name, args);
+   va_end(args);
+
+   return ret;
+}
+
+int run_hook_with_custom_index(const char *index_file, const char *name, ...)
+{
+   const char *hook_env[3] =  { NULL };
+   char index[PATH_MAX];

Sorry being late with the review.

Recently some effort has been put to replace the
 "PATH_MAX/snprintf() combo" with code using strbuf.

So I think for new developed code it could make sense to avoid PATH_MAX 
from the start.

To my knowledge mkpathdup() from path.c can be used,
(or are there better ways ?)
and the whole function will look like below (not tested)


+   va_list args;
+   int ret;
+
+   snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+   hook_env[0] = index;
+
+   va_start(args, name);
+   ret = run_hook_ve(hook_env, name, args);
+   va_end(args);
+
+   return ret;
+}


int run_hook_with_custom_index(const char *index_file, const char *name, 
...)

{
const char *hook_env[3] =  { NULL };
char index = mkpathdup("GIT_INDEX_FILE=%s", index_file);
va_list args;
int ret;

hook_env[0] = index;

va_start(args, name);
ret = run_hook_ve(hook_env, name, args);
va_end(args);

free(index);
return ret;
}





diff --git a/run-command.h b/run-command.h
index 6b985af..88460f9 100644
--- a/run-command.h
+++ b/run-command.h
@@ -47,7 +47,11 @@ int run_command(struct child_process *);

  extern char *find_hook(const char *name);
  LAST_ARG_MUST_BE_NULL
-extern int run_hook(const char *index_file, const char *name, ...);
+extern int run_hook_le(const char *const *env, const char *name, ...);
+extern int run_hook_ve(const char *const *env, const char *name, va_list args);
+
+LAST_ARG_MUST_BE_NULL
+extern int run_hook_with_custom_index(const char *index_file, const char 
*name, ...);

  #define RUN_COMMAND_NO_STDIN 1
  #define RUN_GIT_CMD2  /*If this is to be git sub-command */
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 41dd37a..998a210 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -15,7 +15,7 @@ test_expect_success 'setup (initial)' '
git commit -m commit1
  '

-test_expect_failure 'edit hunk "commit -p -m message"' '
+test_expect_success 'edit hunk "commit -p -m message"' '
test_when_finished "rm -f editor_was_started" &&
rm -f editor_was_started &&
echo more >>file &&
@@ -23,7 +23,7 @@ test_expect_failure 'edit hunk "commit -p -m message"' '
test -r editor_was_started
  '

-test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
+test_expect_success 'edit hunk "commit --dry-run -p -m message"' '
test_when_finished "rm -f editor_was_started" &&
rm -f editor_was_started &&
echo more >>file &&



--
To unsubscribe from this list: send the line "unsubscribe

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

2014-03-18 Thread Benoit Pierre
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 
---
 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/t7514-commit-patch.sh |  4 ++--
 9 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..1b86d9c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,10 +53,10 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
  int changed)
 {
-   return run_hook(NULL, "post-checkout",
-   sha1_to_hex(old ? old->object.sha1 : null_sha1),
-   sha1_to_hex(new ? new->object.sha1 : null_sha1),
-   changed ? "1" : "0", NULL);
+   return run_hook_le(NULL, "post-checkout",
+  sha1_to_hex(old ? old->object.sha1 : null_sha1),
+  sha1_to_hex(new ? new->object.sha1 : null_sha1),
+  changed ? "1" : "0", NULL);
/* "new" can be NULL when checking out from the index before
   a commit exists. */
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..9b3c04d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -660,8 +660,8 @@ static int checkout(void)
commit_locked_index(lock_file))
die(_("unable to write new index file"));
 
-   err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
-   sha1_to_hex(sha1), "1", NULL);
+   err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
+  sha1_to_hex(sha1), "1", NULL);
 
if (!err && option_recursive)
err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
diff --git a/builtin/commit.c b/builtin/commit.c
index 3783bca..68a90b3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -610,7 +610,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
 
-   if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+   if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", 
NULL))
return 0;
 
if (squash_message) {
@@ -867,8 +867,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
return 0;
}
 
-   if (run_hook(index_file, "prepare-commit-msg",
-git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+   if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+   git_path(commit_editmsg), hook_arg1, hook_arg2, 
NULL))
return 0;
 
if (use_editor) {
@@ -884,7 +884,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
}
 
if (!no_verify &&
-   run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) 
{
+   run_commit_hook(use_editor, index_file, "commit-msg", 
git_path(commit_editmsg), NULL)) {
return 0;
}
 
@@ -1068,8 +1068,6 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
use_editor = 0;
if (0 <= edit_flag)
use_editor = edit_flag;
-   if (!use_editor)
-   setenv("GIT_EDITOR", ":", 1);
 
/* Sanity check options */
if (amend && !current_head)
@@ -1450,6 +1448,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
return finish_command(&proc);
 }
 
+int run_commit_hook(int editor_is_used, const char *index_file, const char 
*name, ...)
+{
+   const char *hook_env[3] =  { NULL };
+   char index[PATH_MAX];
+   va_list args;
+   int ret;
+
+   snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+   hook_env[0] = index;
+
+   /*
+* Let the hook know that no editor will be launched.
+*/
+   if (!editor_is_used)
+   hook_env[1] = "GIT_EDITOR=:";
+
+   va_start(args, name);
+   ret = run_hook_ve(hook_env, name, args);
+   va_end(args);
+
+   return ret;
+}
+
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
static struct wt_status s;
@@ -1674,7 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 "not exceeded, and then \"git reset HEAD\" to recover."));
 
rerere(0);
-   run_hook(get_index_file(), "post-commit", NULL);
+   run_commit_hook(use_editor, get_index_file(), "post-co

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

2014-03-15 Thread Benoit Pierre
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 
---
 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-patch.sh |  4 ++--
 9 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..1b86d9c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,10 +53,10 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
  int changed)
 {
-   return run_hook(NULL, "post-checkout",
-   sha1_to_hex(old ? old->object.sha1 : null_sha1),
-   sha1_to_hex(new ? new->object.sha1 : null_sha1),
-   changed ? "1" : "0", NULL);
+   return run_hook_le(NULL, "post-checkout",
+  sha1_to_hex(old ? old->object.sha1 : null_sha1),
+  sha1_to_hex(new ? new->object.sha1 : null_sha1),
+  changed ? "1" : "0", NULL);
/* "new" can be NULL when checking out from the index before
   a commit exists. */
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..9b3c04d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -660,8 +660,8 @@ static int checkout(void)
commit_locked_index(lock_file))
die(_("unable to write new index file"));
 
-   err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
-   sha1_to_hex(sha1), "1", NULL);
+   err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
+  sha1_to_hex(sha1), "1", NULL);
 
if (!err && option_recursive)
err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
diff --git a/builtin/commit.c b/builtin/commit.c
index 3783bca..68a90b3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -610,7 +610,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
 
-   if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+   if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", 
NULL))
return 0;
 
if (squash_message) {
@@ -867,8 +867,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
return 0;
}
 
-   if (run_hook(index_file, "prepare-commit-msg",
-git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+   if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+   git_path(commit_editmsg), hook_arg1, hook_arg2, 
NULL))
return 0;
 
if (use_editor) {
@@ -884,7 +884,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
}
 
if (!no_verify &&
-   run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) 
{
+   run_commit_hook(use_editor, index_file, "commit-msg", 
git_path(commit_editmsg), NULL)) {
return 0;
}
 
@@ -1068,8 +1068,6 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
use_editor = 0;
if (0 <= edit_flag)
use_editor = edit_flag;
-   if (!use_editor)
-   setenv("GIT_EDITOR", ":", 1);
 
/* Sanity check options */
if (amend && !current_head)
@@ -1450,6 +1448,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
return finish_command(&proc);
 }
 
+int run_commit_hook(int editor_is_used, const char *index_file, const char 
*name, ...)
+{
+   const char *hook_env[3] =  { NULL };
+   char index[PATH_MAX];
+   va_list args;
+   int ret;
+
+   snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+   hook_env[0] = index;
+
+   /*
+* Let the hook know that no editor will be launched.
+*/
+   if (!editor_is_used)
+   hook_env[1] = "GIT_EDITOR=:";
+
+   va_start(args, name);
+   ret = run_hook_ve(hook_env, name, args);
+   va_end(args);
+
+   return ret;
+}
+
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
static struct wt_status s;
@@ -1674,7 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 "not exceeded, and then \"git reset HEAD\" to recover."));
 
rerere(0);
-   run_hook(get_index_file(), "post-commit", NULL);
+   run_commit_hook(use_editor, get_index_file(), "post-co

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

2014-03-11 Thread Junio C Hamano
Benoit Pierre  writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5df3837..da423b2 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -53,10 +53,10 @@ struct checkout_opts {
>  static int post_checkout_hook(struct commit *old, struct commit *new,
> int changed)
>  {
> - return run_hook(NULL, "post-checkout",
> - sha1_to_hex(old ? old->object.sha1 : null_sha1),
> - sha1_to_hex(new ? new->object.sha1 : null_sha1),
> - changed ? "1" : "0", NULL);
> +return run_hook_le(NULL, "post-checkout",
> +sha1_to_hex(old ? old->object.sha1 : null_sha1),
> +sha1_to_hex(new ? new->object.sha1 : null_sha1),
> +changed ? "1" : "0", NULL);

Funny indentation.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-03-11 Thread Jeff King
On Tue, Mar 11, 2014 at 06:56:02PM +0100, Benoit Pierre wrote:

> 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.

Ah, you're right. I was thinking that our invocation of launch_editor
also respected it. It does, but we never get there at all because
use_editor is set to 0. So yeah, it really is just needed for the hook.

Your patch, even though it is a bigger change, keeps the setting to the
minimal area, which is cleaner and more maintainable in the long run.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-03-11 Thread Benoit Pierre
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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-03-10 Thread Jun Hao
Jeff King  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 
>> ---
>>  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






--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-03-10 Thread Jeff King
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 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-03-10 Thread Benoit Pierre
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 
---
 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(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..da423b2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,10 +53,10 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
  int changed)
 {
-   return run_hook(NULL, "post-checkout",
-   sha1_to_hex(old ? old->object.sha1 : null_sha1),
-   sha1_to_hex(new ? new->object.sha1 : null_sha1),
-   changed ? "1" : "0", NULL);
+return run_hook_le(NULL, "post-checkout",
+  sha1_to_hex(old ? old->object.sha1 : null_sha1),
+  sha1_to_hex(new ? new->object.sha1 : null_sha1),
+  changed ? "1" : "0", NULL);
/* "new" can be NULL when checking out from the index before
   a commit exists. */
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..9b3c04d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -660,8 +660,8 @@ static int checkout(void)
commit_locked_index(lock_file))
die(_("unable to write new index file"));
 
-   err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
-   sha1_to_hex(sha1), "1", NULL);
+   err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
+  sha1_to_hex(sha1), "1", NULL);
 
if (!err && option_recursive)
err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..baf1fc0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -612,7 +612,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
 
-   if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+   if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", 
NULL))
return 0;
 
if (squash_message) {
@@ -866,8 +866,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
return 0;
}
 
-   if (run_hook(index_file, "prepare-commit-msg",
-git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+   if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+   git_path(commit_editmsg), hook_arg1, hook_arg2, 
NULL))
return 0;
 
if (use_editor) {
@@ -883,7 +883,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
}
 
if (!no_verify &&
-   run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) 
{
+   run_commit_hook(use_editor, index_file, "commit-msg", 
git_path(commit_editmsg), NULL)) {
return 0;
}
 
@@ -1067,8 +1067,6 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
use_editor = 0;
if (0 <= edit_flag)
use_editor = edit_flag;
-   if (!use_editor)
-   setenv("GIT_EDITOR", ":", 1);
 
/* Sanity check options */
if (amend && !current_head)
@@ -1445,6 +1443,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
return finish_command(&proc);
 }
 
+int run_commit_hook(int editor_is_used, const char *index_file, const char 
*name, ...)
+{
+   const char *hook_env[3] =  { NULL };
+   char index[PATH_MAX];
+   va_list args;
+   int ret;
+
+   snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+   hook_env[0] = index;
+
+   /*
+* Let the hook know that no editor will be launched.
+*/
+   if (!editor_is_used)
+   hook_env[1] = "GIT_EDITOR=:";
+
+   va_start(args, name);
+   ret = run_hook_ve(hook_env, name, args);
+   va_end(args);
+
+   return ret;
+}
+
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
static struct wt_status s;
@@ -1669,7 +1690,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 "not exceeded, and then \"git reset HEAD\" to recover."));
 
rerere(0);
-   run_hook(get_index_file(), "post-commit", NULL);