[PATCH 80/83] run-command: make dup_devnull() non static

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 80/83] run-command: make dup_devnull() non static

Christian Couder-2
Signed-off-by: Christian Couder <[hidden email]>
---
 run-command.c | 2 +-
 run-command.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 8c7115a..29d2bda 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
 }
 
 #ifndef GIT_WINDOWS_NATIVE
-static inline void dup_devnull(int to)
+void dup_devnull(int to)
 {
  int fd = open("/dev/null", O_RDWR);
  if (fd < 0)
diff --git a/run-command.h b/run-command.h
index de1727e..3aa244a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -200,4 +200,10 @@ int run_processes_parallel(int n,
    task_finished_fn,
    void *pp_cb);
 
+/**
+ * Misc helper functions
+ */
+
+void dup_devnull(int to);
+
 #endif
--
2.8.1.300.g5fed0c0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 81/83] apply: roll back index in case of error

Christian Couder-2
Signed-off-by: Christian Couder <[hidden email]>
---
 apply.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index 86e0d20..7cee834 100644
--- a/apply.c
+++ b/apply.c
@@ -4718,8 +4718,11 @@ int apply_all_patches(struct apply_state *state,
 
  if (!strcmp(arg, "-")) {
  res = apply_patch(state, 0, "<stdin>", options);
- if (res < 0)
+ if (res < 0) {
+ if (state->lock_file)
+ rollback_lock_file(state->lock_file);
  return -1;
+ }
  errs |= res;
  read_stdin = 0;
  continue;
@@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,
  read_stdin = 0;
  set_default_whitespace_mode(state);
  res = apply_patch(state, fd, arg, options);
- if (res < 0)
+ if (res < 0) {
+ if (state->lock_file)
+ rollback_lock_file(state->lock_file);
  return -1;
+ }
  errs |= res;
  close(fd);
  }
  set_default_whitespace_mode(state);
  if (read_stdin) {
  res = apply_patch(state, 0, "<stdin>", options);
- if (res < 0)
+ if (res < 0) {
+ if (state->lock_file)
+ rollback_lock_file(state->lock_file);
  return -1;
+ }
  errs |= res;
  }
 
@@ -4757,11 +4766,14 @@ int apply_all_patches(struct apply_state *state,
    squelched),
  squelched);
  }
- if (state->ws_error_action == die_on_ws_error)
+ if (state->ws_error_action == die_on_ws_error) {
+ if (state->lock_file)
+ rollback_lock_file(state->lock_file);
  return error(Q_("%d line adds whitespace errors.",
  "%d lines add whitespace errors.",
  state->whitespace_error),
      state->whitespace_error);
+ }
  if (state->applied_after_fixing_ws && state->apply)
  warning("%d line%s applied after"
  " fixing whitespace errors.",
--
2.8.1.300.g5fed0c0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 82/83] environment: add set_index_file()

Christian Couder-2
In reply to this post by Christian Couder-2
Signed-off-by: Christian Couder <[hidden email]>
---
 cache.h       | 1 +
 environment.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/cache.h b/cache.h
index 2711048..7f36aa3 100644
--- a/cache.h
+++ b/cache.h
@@ -461,6 +461,7 @@ extern int is_inside_work_tree(void);
 extern const char *get_git_dir(void);
 extern const char *get_git_common_dir(void);
 extern char *get_object_directory(void);
+extern void set_index_file(char *index_file);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
diff --git a/environment.c b/environment.c
index 57acb2f..5a57822 100644
--- a/environment.c
+++ b/environment.c
@@ -290,6 +290,11 @@ int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1)
  return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 }
 
+void set_index_file(char *index_file)
+{
+ git_index_file = index_file;
+}
+
 char *get_index_file(void)
 {
  if (!git_index_file)
--
2.8.1.300.g5fed0c0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 83/83] builtin/am: use apply api in run_apply()

Christian Couder-2
In reply to this post by Christian Couder-2
This replaces run_apply() implementation with a new one that
uses the apply api that has been previously prepared in
apply.c and apply.h.

This shoud improve performance a lot in certain cases.

As the previous implementation was creating a new `git apply`
process to apply each patch, it could be slow on systems like
Windows where it is costly to create new processes.

Also the new `git apply` process had to read the index from
disk, and when the process was done the calling process
discarded its own index and read back from disk the new
index that had been created by the `git apply` process.

This could be very inefficient with big repositories that
have big index files, especially when the system decided
that it was a good idea to run the `git apply` processes on
a different processor core.

Also eliminating index reads enables further performance
improvements by using:

`git update-index --split-index`

Signed-off-by: Christian Couder <[hidden email]>
---
 builtin/am.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 85 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d003939..85a77d7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -28,6 +28,7 @@
 #include "rerere.h"
 #include "prompt.h"
 #include "mailinfo.h"
+#include "apply.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -1522,39 +1523,105 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
  */
 static int run_apply(const struct am_state *state, const char *index_file)
 {
- struct child_process cp = CHILD_PROCESS_INIT;
-
- cp.git_cmd = 1;
-
- if (index_file)
- argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", index_file);
+ struct argv_array apply_paths = ARGV_ARRAY_INIT;
+ struct argv_array apply_opts = ARGV_ARRAY_INIT;
+ struct apply_state apply_state;
+ int save_stdout_fd, save_stderr_fd;
+ int res, opts_left;
+ char *save_index_file;
+
+ struct option am_apply_options[] = {
+ { OPTION_CALLBACK, 0, "whitespace", &apply_state, N_("action"),
+ N_("detect new or modified lines that have whitespace errors"),
+ 0, apply_option_parse_whitespace },
+ { OPTION_CALLBACK, 0, "ignore-space-change", &apply_state, NULL,
+ N_("ignore changes in whitespace when finding context"),
+ PARSE_OPT_NOARG, apply_option_parse_space_change },
+ { OPTION_CALLBACK, 0, "ignore-whitespace", &apply_state, NULL,
+ N_("ignore changes in whitespace when finding context"),
+ PARSE_OPT_NOARG, apply_option_parse_space_change },
+ { OPTION_CALLBACK, 0, "directory", &apply_state, N_("root"),
+ N_("prepend <root> to all filenames"),
+ 0, apply_option_parse_directory },
+ { OPTION_CALLBACK, 0, "exclude", &apply_state, N_("path"),
+ N_("don't apply changes matching the given path"),
+ 0, apply_option_parse_exclude },
+ { OPTION_CALLBACK, 0, "include", &apply_state, N_("path"),
+ N_("apply changes matching the given path"),
+ 0, apply_option_parse_include },
+ OPT_INTEGER('C', NULL, &apply_state.p_context,
+ N_("ensure at least <n> lines of context match")),
+ { OPTION_CALLBACK, 'p', NULL, &apply_state, N_("num"),
+ N_("remove <num> leading slashes from traditional diff paths"),
+ 0, apply_option_parse_p },
+ OPT_BOOL(0, "reject", &apply_state.apply_with_reject,
+ N_("leave the rejected hunks in corresponding *.rej files")),
+ OPT_END()
+ };
 
  /*
  * If we are allowed to fall back on 3-way merge, don't give false
  * errors during the initial attempt.
  */
+
  if (state->threeway && !index_file) {
- cp.no_stdout = 1;
- cp.no_stderr = 1;
+ save_stdout_fd = dup(1);
+ dup_devnull(1);
+ save_stderr_fd = dup(2);
+ dup_devnull(2);
  }
 
- argv_array_push(&cp.args, "apply");
+ if (index_file) {
+ save_index_file = get_index_file();
+ set_index_file((char *)index_file);
+ }
 
- argv_array_pushv(&cp.args, state->git_apply_opts.argv);
+ if (init_apply_state(&apply_state, NULL))
+ die("init_apply_state() failed");
+
+ argv_array_push(&apply_opts, "apply");
+ argv_array_pushv(&apply_opts, state->git_apply_opts.argv);
+
+ opts_left = parse_options(apply_opts.argc, apply_opts.argv,
+  NULL, am_apply_options, NULL, 0);
+
+ if (opts_left != 0)
+ die("unknown option passed thru to git apply");
 
  if (index_file)
- argv_array_push(&cp.args, "--cached");
+ apply_state.cached = 1;
  else
- argv_array_push(&cp.args, "--index");
+ apply_state.check_index = 1;
 
- argv_array_push(&cp.args, am_path(state, "patch"));
+ if (check_apply_state(&apply_state, 0))
+ die("check_apply_state() failed");
 
- if (run_command(&cp))
- return -1;
+ argv_array_push(&apply_paths, am_path(state, "patch"));
 
- /* Reload index as git-apply will have modified it. */
- discard_cache();
- read_cache_from(index_file ? index_file : get_index_file());
+ res = apply_all_patches(&apply_state, apply_paths.argc, apply_paths.argv, 0);
+
+ /* Restore stdout and stderr */
+ if (state->threeway && !index_file) {
+ dup2(save_stdout_fd, 1);
+ close(save_stdout_fd);
+ dup2(save_stderr_fd, 2);
+ close(save_stderr_fd);
+ }
+
+ if (index_file)
+ set_index_file(save_index_file);
+
+ argv_array_clear(&apply_paths);
+ argv_array_clear(&apply_opts);
+
+ if (res)
+ return res;
+
+ if (index_file) {
+ /* Reload index as apply_all_patches() will have modified it. */
+ discard_cache();
+ read_cache_from(index_file);
+ }
 
  return 0;
 }
--
2.8.1.300.g5fed0c0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 83/83] builtin/am: use apply api in run_apply()

Johannes Schindelin
Hi Chris,

On Sun, 24 Apr 2016, Christian Couder wrote:

> [...]
>   /*
>   * If we are allowed to fall back on 3-way merge, don't give false
>   * errors during the initial attempt.
>   */
> +
>   if (state->threeway && !index_file) {
> - cp.no_stdout = 1;
> - cp.no_stderr = 1;
> + save_stdout_fd = dup(1);
> + dup_devnull(1);
> + save_stderr_fd = dup(2);
> + dup_devnull(2);

I wonder. It should be possible to teach the apply function to be quiet by
default, yes? That would be more elegant than dup()ing back and forth.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Johannes Schindelin
In reply to this post by Christian Couder-2
Hi Chris,

On Sun, 24 Apr 2016, Christian Couder wrote:

> diff --git a/run-command.c b/run-command.c
> index 8c7115a..29d2bda 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
>  }
>  
>  #ifndef GIT_WINDOWS_NATIVE
> -static inline void dup_devnull(int to)
> +void dup_devnull(int to)
>  {

The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 81/83] apply: roll back index in case of error

Johannes Schindelin
In reply to this post by Christian Couder-2
Hi Chris,

On Sun, 24 Apr 2016, Christian Couder wrote:

> @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,
>   read_stdin = 0;
>   set_default_whitespace_mode(state);
>   res = apply_patch(state, fd, arg, options);
> - if (res < 0)
> + if (res < 0) {
> + if (state->lock_file)
> + rollback_lock_file(state->lock_file);
>   return -1;
> + }
>   errs |= res;
>   close(fd);

In case of error, this leaves fd open, which in the end will prevent the
"patch" file, and hence the "rebase-apply/" directory from being removed
on Windows. This triggered a failure of t4014 here (and possibly more, but
it took me quite a while to track this down, what with builtin/am.c's
am_destroy() not bothering at all to check the return value of
remove_dir_recursively(), resulting in the error to be caught only much,
much later).

Could you please review all open()/close() and fopen()/fclose() calls in
your patch series, to make sure that there are no mistakes? A passing test
suite does not really make me confident here, as our code coverage is not
quite 100%.

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 81/83] apply: roll back index in case of error

Johannes Schindelin
Hi,

apparently this mail never made it to the list???

On Mon, 25 Apr 2016, Johannes Schindelin wrote:

> Hi Chris,
>
> On Sun, 24 Apr 2016, Christian Couder wrote:
>
> > @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,
> >   read_stdin = 0;
> >   set_default_whitespace_mode(state);
> >   res = apply_patch(state, fd, arg, options);
> > - if (res < 0)
> > + if (res < 0) {
> > + if (state->lock_file)
> > + rollback_lock_file(state->lock_file);
> >   return -1;
> > + }
> >   errs |= res;
> >   close(fd);
>
> In case of error, this leaves fd open, which in the end will prevent the
> "patch" file, and hence the "rebase-apply/" directory from being removed
> on Windows. This triggered a failure of t4014 here (and possibly more, but
> it took me quite a while to track this down, what with builtin/am.c's
> am_destroy() not bothering at all to check the return value of
> remove_dir_recursively(), resulting in the error to be caught only much,
> much later).
>
> Could you please review all open()/close() and fopen()/fclose() calls in
> your patch series, to make sure that there are no mistakes? A passing test
> suite does not really make me confident here, as our code coverage is not
> quite 100%.
>
> Thanks,
> Dscho
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 81/83] apply: roll back index in case of error

Eric Sunshine
On Mon, May 2, 2016 at 3:07 AM, Johannes Schindelin
<[hidden email]> wrote:
> apparently this mail never made it to the list???

It made it to the list[1].

[1]: http://git.661346.n2.nabble.com/PATCH-80-83-run-command-make-dup-devnull-non-static-tp7654127p7654233.html

> On Mon, 25 Apr 2016, Johannes Schindelin wrote:
>
>> Hi Chris,
>>
>> On Sun, 24 Apr 2016, Christian Couder wrote:
>>
>> > @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,
>> >             read_stdin = 0;
>> >             set_default_whitespace_mode(state);
>> >             res = apply_patch(state, fd, arg, options);
>> > -           if (res < 0)
>> > +           if (res < 0) {
>> > +                   if (state->lock_file)
>> > +                           rollback_lock_file(state->lock_file);
>> >                     return -1;
>> > +           }
>> >             errs |= res;
>> >             close(fd);
>>
>> In case of error, this leaves fd open, which in the end will prevent the
>> "patch" file, and hence the "rebase-apply/" directory from being removed
>> on Windows. This triggered a failure of t4014 here (and possibly more, but
>> it took me quite a while to track this down, what with builtin/am.c's
>> am_destroy() not bothering at all to check the return value of
>> remove_dir_recursively(), resulting in the error to be caught only much,
>> much later).
>>
>> Could you please review all open()/close() and fopen()/fclose() calls in
>> your patch series, to make sure that there are no mistakes? A passing test
>> suite does not really make me confident here, as our code coverage is not
>> quite 100%.
>>
>> Thanks,
>> Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 81/83] apply: roll back index in case of error

Christian Couder-2
In reply to this post by Johannes Schindelin
Hi Dscho,

On Mon, Apr 25, 2016 at 6:06 PM, Johannes Schindelin
<[hidden email]> wrote:

> Hi Chris,
>
> On Sun, 24 Apr 2016, Christian Couder wrote:
>
>> @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,
>>               read_stdin = 0;
>>               set_default_whitespace_mode(state);
>>               res = apply_patch(state, fd, arg, options);
>> -             if (res < 0)
>> +             if (res < 0) {
>> +                     if (state->lock_file)
>> +                             rollback_lock_file(state->lock_file);
>>                       return -1;
>> +             }
>>               errs |= res;
>>               close(fd);
>
> In case of error, this leaves fd open, which in the end will prevent the
> "patch" file, and hence the "rebase-apply/" directory from being removed
> on Windows. This triggered a failure of t4014 here (and possibly more, but
> it took me quite a while to track this down, what with builtin/am.c's
> am_destroy() not bothering at all to check the return value of
> remove_dir_recursively(), resulting in the error to be caught only much,
> much later).

Sorry about that and thanks for tracking down the source of the test failure.
I fixed this by moving the "close(fd)" call just after the "apply_patch()" call.

> Could you please review all open()/close() and fopen()/fclose() calls in
> your patch series, to make sure that there are no mistakes? A passing test
> suite does not really make me confident here, as our code coverage is not
> quite 100%.

Ok, I will have another look at the 2 other places where there are
open()/close() or fopen()/fclose() calls.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 82/83] environment: add set_index_file()

Junio C Hamano
In reply to this post by Christian Couder-2
Christian Couder <[hidden email]> writes:

> +void set_index_file(char *index_file)
> +{
> + git_index_file = index_file;
> +}

What's the rationale for this change, and more importantly, the
ownership rule for the string?  When you call this function, does
the caller still own that piece of memory?  When you call this
twice, where does the old value go and who is responsible for
taking care of not leaking it?

Cannot guess any of the above with no proposed log message (that
comment applies most of your patches in this series).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 82/83] environment: add set_index_file()

Christian Couder-2
On Tue, May 3, 2016 at 5:36 PM, Junio C Hamano <[hidden email]> wrote:

> Christian Couder <[hidden email]> writes:
>
>> +void set_index_file(char *index_file)
>> +{
>> +     git_index_file = index_file;
>> +}
>
> What's the rationale for this change, and more importantly, the
> ownership rule for the string?  When you call this function, does
> the caller still own that piece of memory?  When you call this
> twice, where does the old value go and who is responsible for
> taking care of not leaking it?
>
> Cannot guess any of the above with no proposed log message (that
> comment applies most of your patches in this series).

Yeah, I agree that I could have been more verbose on this one, and
some other ones too.

The reason for this is that run_apply() in builtin/am.c has a "const
char *index_file" argument.
The current version of run_apply() does:

        if (index_file)
                argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s",
index_file);

to pass that parameter to the `git apply` process that it launches.

I couldn't find a good way other than doing:

    if (index_file) {
        save_index_file = get_index_file();
        set_index_file((char *)index_file);
    }

    /* Call libified apply functions */
    ...

    if (index_file)
        set_index_file(save_index_file);

to do the equivalent of what was done previously.

So I guess I could have written something like the following in the
commit message of the commit that introduces set_index_file():

    Introduce set_index_file() to be able to temporarily change the index file.

    It should be used like:

        /* Save current index file */
        old_index_file = get_index_file();
        set_index_file((char *)tmp_index_file);

        /* Do stuff that will use tmp_index_file as the index file */
        ...

        /* When finished reset the index file */
        set_index_file(old_index_file);

Maybe I could also add a comment just before the function.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Christian Couder-2
In reply to this post by Johannes Schindelin
On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
<[hidden email]> wrote:

> Hi Chris,
>
> On Sun, 24 Apr 2016, Christian Couder wrote:
>
>> diff --git a/run-command.c b/run-command.c
>> index 8c7115a..29d2bda 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
>>  }
>>
>>  #ifndef GIT_WINDOWS_NATIVE
>> -static inline void dup_devnull(int to)
>> +void dup_devnull(int to)
>>  {
>
> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.

Yeah, but I must say that I don't know what I should do about this.
Do you have a suggestion? Should I try to implement the same function
for Windows?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 83/83] builtin/am: use apply api in run_apply()

Christian Couder-2
In reply to this post by Johannes Schindelin
Hi Dscho,

On Mon, Apr 25, 2016 at 5:03 PM, Johannes Schindelin
<[hidden email]> wrote:

> Hi Chris,
>
> On Sun, 24 Apr 2016, Christian Couder wrote:
>
>> [...]
>>       /*
>>        * If we are allowed to fall back on 3-way merge, don't give false
>>        * errors during the initial attempt.
>>        */
>> +
>>       if (state->threeway && !index_file) {
>> -             cp.no_stdout = 1;
>> -             cp.no_stderr = 1;
>> +             save_stdout_fd = dup(1);
>> +             dup_devnull(1);
>> +             save_stderr_fd = dup(2);
>> +             dup_devnull(2);
>
> I wonder. It should be possible to teach the apply function to be quiet by
> default, yes? That would be more elegant than dup()ing back and forth.

Yes, it could be possible, but it could mean many changes not only in
the apply functions, but in possibly many other places as well. I
didn't check, but for example if an apply function calls a function
from another part of git and this function uses error(...) in case of
error, I would have to change this function too.

I could also introduce a hack like a global variable that would tell
error() to shut up, but I am not sure that would be more elegant.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Johannes Sixt-3
In reply to this post by Christian Couder-2
Am 05.05.2016 um 11:50 schrieb Christian Couder:

> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
> <[hidden email]> wrote:
>> Hi Chris,
>>
>> On Sun, 24 Apr 2016, Christian Couder wrote:
>>
>>> diff --git a/run-command.c b/run-command.c
>>> index 8c7115a..29d2bda 100644
>>> --- a/run-command.c
>>> +++ b/run-command.c
>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
>>>   }
>>>
>>>   #ifndef GIT_WINDOWS_NATIVE
>>> -static inline void dup_devnull(int to)
>>> +void dup_devnull(int to)
>>>   {
>>
>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.
>
> Yeah, but I must say that I don't know what I should do about this.
> Do you have a suggestion? Should I try to implement the same function
> for Windows?

No, just remove the #ifndef brackets. There is already code in
compat/mingw.c that treats the file name "/dev/null" specially.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Christian Couder-2
On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <[hidden email]> wrote:

> Am 05.05.2016 um 11:50 schrieb Christian Couder:
>>
>> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
>> <[hidden email]> wrote:
>>>
>>> Hi Chris,
>>>
>>> On Sun, 24 Apr 2016, Christian Couder wrote:
>>>
>>>> diff --git a/run-command.c b/run-command.c
>>>> index 8c7115a..29d2bda 100644
>>>> --- a/run-command.c
>>>> +++ b/run-command.c
>>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
>>>>   }
>>>>
>>>>   #ifndef GIT_WINDOWS_NATIVE
>>>> -static inline void dup_devnull(int to)
>>>> +void dup_devnull(int to)
>>>>   {
>>>
>>>
>>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.
>>
>>
>> Yeah, but I must say that I don't know what I should do about this.
>> Do you have a suggestion? Should I try to implement the same function
>> for Windows?
>
> No, just remove the #ifndef brackets. There is already code in
> compat/mingw.c that treats the file name "/dev/null" specially.

Ok, I will do that in the same patch though the "#ifndef
GIT_WINDOWS_NATIVE" was already there before.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Johannes Schindelin
Hi Chris,

On Fri, 6 May 2016, Christian Couder wrote:

> On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <[hidden email]> wrote:
> > Am 05.05.2016 um 11:50 schrieb Christian Couder:
> >>
> >> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
> >> <[hidden email]> wrote:
> >>>
> >>> Hi Chris,
> >>>
> >>> On Sun, 24 Apr 2016, Christian Couder wrote:
> >>>
> >>>> diff --git a/run-command.c b/run-command.c
> >>>> index 8c7115a..29d2bda 100644
> >>>> --- a/run-command.c
> >>>> +++ b/run-command.c
> >>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
> >>>>   }
> >>>>
> >>>>   #ifndef GIT_WINDOWS_NATIVE
> >>>> -static inline void dup_devnull(int to)
> >>>> +void dup_devnull(int to)
> >>>>   {
> >>>
> >>>
> >>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.
> >>
> >>
> >> Yeah, but I must say that I don't know what I should do about this.
> >> Do you have a suggestion? Should I try to implement the same function
> >> for Windows?

No, you should change the code that requires that ugly dup()ing so that it
can be configured to shut up.

> > No, just remove the #ifndef brackets. There is already code in
> > compat/mingw.c that treats the file name "/dev/null" specially.
>
> Ok, I will do that in the same patch though the "#ifndef
> GIT_WINDOWS_NATIVE" was already there before.

The idea was that compat/mingw.c is *really* only for the MINGW version,
not for the MSVC version.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Christian Couder-2
Hi Dscho,

On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin
<[hidden email]> wrote:

> Hi Chris,
>
> On Fri, 6 May 2016, Christian Couder wrote:
>
>> On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <[hidden email]> wrote:
>> > Am 05.05.2016 um 11:50 schrieb Christian Couder:
>> >>
>> >> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
>> >> <[hidden email]> wrote:
>> >>>
>> >>> Hi Chris,
>> >>>
>> >>> On Sun, 24 Apr 2016, Christian Couder wrote:
>> >>>
>> >>>> diff --git a/run-command.c b/run-command.c
>> >>>> index 8c7115a..29d2bda 100644
>> >>>> --- a/run-command.c
>> >>>> +++ b/run-command.c
>> >>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
>> >>>>   }
>> >>>>
>> >>>>   #ifndef GIT_WINDOWS_NATIVE
>> >>>> -static inline void dup_devnull(int to)
>> >>>> +void dup_devnull(int to)
>> >>>>   {
>> >>>
>> >>>
>> >>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.
>> >>
>> >>
>> >> Yeah, but I must say that I don't know what I should do about this.
>> >> Do you have a suggestion? Should I try to implement the same function
>> >> for Windows?
>
> No, you should change the code that requires that ugly dup()ing so that it
> can be configured to shut up.

After taking a look, it looks like a routine that does nothing could
be passed to set_error_routine() and that could do part of the trick.

This part might not be too ugly, but it would anyway be more complex,
less close to what the code is doing now and more error prone, as one
also need to make sure that for example no warning() or
fprintf(stderr, ...) are called and nothing is printed on stdout.

By the way I took a look and there are 11 calls to fprintf(stderr,
...) and 10 calls to warning() in different places in builtin/apply.c.
There might also be such calls in functions outside builtin/apply.c
that are called by the functions in builtin/apply.c.

So I'd much rather keep doing what I am doing now. If you or someone
else want to contribute patches on top of the series to do it in
another way, maybe they might be integrated at the same time by Junio,
so that the whole thing would appear in the same release and there
would be no feature discrepancy between Windows and the other
platforms, and you wouldn't need to implement anything special for
Windows.

But anyway, even though I don't know much about Windows, I think if
you have some code already in compat/mingw.c to handle redirections,
it might be easier and safer overall to just implement the
redirections in Windows.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Johannes Schindelin
Hi Chris,

On Sat, 7 May 2016, Christian Couder wrote:

> On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin
> <[hidden email]> wrote:
> >
> > On Fri, 6 May 2016, Christian Couder wrote:
> >
> >> On Thu, May 5, 2016 at 10:07 PM, Johannes Sixt <[hidden email]> wrote:
> >> > Am 05.05.2016 um 11:50 schrieb Christian Couder:
> >> >>
> >> >> On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
> >> >> <[hidden email]> wrote:
> >> >>>
> >> >>> Hi Chris,
> >> >>>
> >> >>> On Sun, 24 Apr 2016, Christian Couder wrote:
> >> >>>
> >> >>>> diff --git a/run-command.c b/run-command.c
> >> >>>> index 8c7115a..29d2bda 100644
> >> >>>> --- a/run-command.c
> >> >>>> +++ b/run-command.c
> >> >>>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
> >> >>>>   }
> >> >>>>
> >> >>>>   #ifndef GIT_WINDOWS_NATIVE
> >> >>>> -static inline void dup_devnull(int to)
> >> >>>> +void dup_devnull(int to)
> >> >>>>   {
> >> >>>
> >> >>>
> >> >>> The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.
> >> >>
> >> >>
> >> >> Yeah, but I must say that I don't know what I should do about this.
> >> >> Do you have a suggestion? Should I try to implement the same function
> >> >> for Windows?
> >
> > No, you should change the code that requires that ugly dup()ing so that it
> > can be configured to shut up.
>
> After taking a look, it looks like a routine that does nothing could
> be passed to set_error_routine() and that could do part of the trick.
>
> This part might not be too ugly, but it would anyway be more complex,
> less close to what the code is doing now and more error prone, as one
> also need to make sure that for example no warning() or
> fprintf(stderr, ...) are called and nothing is printed on stdout.

I am afraid that you *have* to do that, though, if you truly want to
libify the code.

Of course you can go with really ugly workarounds instead. Something like
a global flag that die() and error() and warning() respect. It would
incur some technical debt, but it would make your life easier in the short
run.

Both the real solution and the workaround would be better than the current
version of the patches that dup() back and forth, just to avoid addressing
the real problem.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 80/83] run-command: make dup_devnull() non static

Christian Couder-2
Hi Dscho,

On Sat, May 7, 2016 at 2:13 PM, Johannes Schindelin
<[hidden email]> wrote:

> Hi Chris,
>
> On Sat, 7 May 2016, Christian Couder wrote:
>
>> On Fri, May 6, 2016 at 5:34 PM, Johannes Schindelin
>> <[hidden email]> wrote:
>> >
>> > No, you should change the code that requires that ugly dup()ing so that it
>> > can be configured to shut up.
>>
>> After taking a look, it looks like a routine that does nothing could
>> be passed to set_error_routine() and that could do part of the trick.
>>
>> This part might not be too ugly, but it would anyway be more complex,
>> less close to what the code is doing now and more error prone, as one
>> also need to make sure that for example no warning() or
>> fprintf(stderr, ...) are called and nothing is printed on stdout.
>
> I am afraid that you *have* to do that, though, if you truly want to
> libify the code.
>
> Of course you can go with really ugly workarounds instead. Something like
> a global flag that die() and error() and warning() respect. It would
> incur some technical debt, but it would make your life easier in the short
> run.
>
> Both the real solution and the workaround would be better than the current
> version of the patches that dup() back and forth, just to avoid addressing
> the real problem.

The code that is now in master in builtin/am.c does:

    if (state->threeway && !index_file) {
        cp.no_stdout = 1;
        cp.no_stderr = 1;
    }

and in run-command.c there is already:

        if (cmd->no_stdout)
            dup_devnull(1);
        [...]
        if (cmd->no_stderr)
            dup_devnull(2);

for Linux and the following for Windows:

    if (cmd->no_stderr)
        fherr = open("/dev/null", O_RDWR);
        [...]
    if (cmd->no_stdout)
        fhout = open("/dev/null", O_RDWR);

so the current code is already using dup_devnull() for Linux that you
don't want me to use, and it looks like there is already a simple way
to do that on Windows.

So what's the problem? Isn't it just that you don't want a
dup_devnull() for Windows that would be a few lines long?

You keep saying that what is done in this patch is "ugly" or that
there is a "real problem", but frankly I don't see why. Could you
explain exactly why?
Because the more I look at it, the more it looks to me like the
solution that is the simplest (even for Windows), the safest and the
closest to what the current code is doing.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
12