[PATCH] format-patch: introduce format.outputDirectory configuration

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

[PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov
We can pass -o/--output-directory to the format-patch command to
store patches not in the working directory. This patch introduces
format.outputDirectory configuration option for same purpose.

The case of usage of this configuration option can be convinience
to not pass everytime -o/--output-directory if an user has pattern
to store all patches in the /patches directory for example.

The format.outputDirectory has lower priority than command line
option, so if user will set format.outputDirectory and pass the
command line option, a result will be stored in a directory that
passed to command line option.

Signed-off-by: Alexander Kuleshov <[hidden email]>
---
 Documentation/config.txt |  4 ++++
 builtin/log.c            | 14 ++++++++++++--
 t/t4014-format-patch.sh  |  9 +++++++++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fd2036c..8f6f7ed 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1247,6 +1247,10 @@ format.coverLetter::
  format-patch is invoked, but in addition can be set to "auto", to
  generate a cover-letter only when there's more than one patch.
 
+format.outputDirectory::
+ Set a custom directory to store the resulting files instead of the
+ current working directory.
+
 filter.<driver>.clean::
  The command which is used to convert the content of a worktree
  file to a blob upon checkin.  See linkgit:gitattributes[5] for
diff --git a/builtin/log.c b/builtin/log.c
index dfb351e..22c1e46 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -687,6 +687,8 @@ enum {
  COVER_AUTO
 };
 
+static const char *config_output_directory = NULL;
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
  if (!strcmp(var, "format.headers")) {
@@ -757,6 +759,9 @@ static int git_format_config(const char *var, const char *value, void *cb)
  config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
  return 0;
  }
+ if (!strcmp(var, "format.outputdirectory")) {
+ return git_config_string(&config_output_directory, var, value);
+ }
 
  return git_log_config(var, value, cb);
 }
@@ -1006,7 +1011,8 @@ static const char *clean_message_id(const char *msg_id)
  return xmemdupz(a, z - a);
 }
 
-static const char *set_outdir(const char *prefix, const char *output_directory)
+static const char *set_outdir(const char *prefix, const char *output_directory,
+      const char *config_output_directory)
 {
  if (output_directory && is_absolute_path(output_directory))
  return output_directory;
@@ -1014,6 +1020,9 @@ static const char *set_outdir(const char *prefix, const char *output_directory)
  if (!prefix || !*prefix) {
  if (output_directory)
  return output_directory;
+
+ if (config_output_directory)
+ return config_output_directory;
  /* The user did not explicitly ask for "./" */
  outdir_offset = 2;
  return "./";
@@ -1368,7 +1377,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
  init_display_notes(&rev.notes_opt);
 
  if (!use_stdout)
- output_directory = set_outdir(prefix, output_directory);
+ output_directory = set_outdir(prefix, output_directory,
+      config_output_directory);
  else
  setup_pager();
 
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c39e500..a4b18b5 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -40,6 +40,15 @@ test_expect_success setup '
 
 '
 
+test_expect_success "format-patch format.outputDirectory option" '
+ git config format.outputDirectory "patches/" &&
+ git format-patch master..side &&
+ cnt=$(ls | wc -l) &&
+ echo $cnt &&
+ test $cnt = 3 &&
+ git config --unset format.outputDirectory
+'
+
 test_expect_success "format-patch --ignore-if-in-upstream" '
 
  git format-patch --stdout master..side >patch0 &&
--
2.4.0.383.gded6615.dirty

--
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] format-patch: introduce format.outputDirectory configuration

Junio C Hamano
Alexander Kuleshov <[hidden email]> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index fd2036c..8f6f7ed 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1247,6 +1247,10 @@ format.coverLetter::
>   format-patch is invoked, but in addition can be set to "auto", to
>   generate a cover-letter only when there's more than one patch.
>  
> +format.outputDirectory::
> + Set a custom directory to store the resulting files instead of the
> + current working directory.
> +

After you set this configuration variable, how would you override it
and get the default behaviour back from the command line for one
time invocation?  "-o ./"?  That needs to be documented somewhere.

Documentation/format-patch.txt must have description on -o; that
paragraph needs to mention this new configuration variable, and it
would be a good place to document the "-o ./" workaround.

> -static const char *set_outdir(const char *prefix, const char *output_directory)
> +static const char *set_outdir(const char *prefix, const char *output_directory,
> +      const char *config_output_directory)

This change looks ugly and unnecessary.  All the machinery after and
including the point set_outdir() is called, including reopen_stdout(),
work on output_directory variable and only that variable.

Wouldn't it work equally well to have

        if (!output_directory)
        output_directory = config_output_directory;

before a call to set_outdir() is made but after the configuration is
read (namely, soon after parse_options() returns), without making
any change to this 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] format-patch: introduce format.outputDirectory configuration

Jeff King
On Thu, Jun 18, 2015 at 10:13:37AM -0700, Junio C Hamano wrote:

> > -static const char *set_outdir(const char *prefix, const char *output_directory)
> > +static const char *set_outdir(const char *prefix, const char *output_directory,
> > +      const char *config_output_directory)
>
> This change looks ugly and unnecessary.  All the machinery after and
> including the point set_outdir() is called, including reopen_stdout(),
> work on output_directory variable and only that variable.
>
> Wouldn't it work equally well to have
>
> if (!output_directory)
>         output_directory = config_output_directory;
>
> before a call to set_outdir() is made but after the configuration is
> read (namely, soon after parse_options() returns), without making
> any change to this function?

Don't we load the config before parsing options here? In that case, we
can use our usual strategy to just set output_directory (which is
already a static global) from the config callback, and everything Just
Works.

We do have to bump the definition of output_directory up above the
config callback, like so (while we are here, we might also want to
drop the unnecessary static initializers, which violate our style guide):

diff --git a/builtin/log.c b/builtin/log.c
index e67671e..77c06f7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -37,6 +37,10 @@ static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
+static FILE *realstdout = NULL;
+static const char *output_directory = NULL;
+static int outdir_offset;
+
 static const char * const builtin_log_usage[] = {
  N_("git log [<options>] [<revision-range>] [[--] <path>...]"),
  N_("git show [<options>] <object>..."),
@@ -752,14 +756,12 @@ static int git_format_config(const char *var, const char *value, void *cb)
  config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
  return 0;
  }
+ if (!strcmp(var, "format.outputdirectory"))
+ return git_config_string(&output_directory, var, value);
 
  return git_log_config(var, value, cb);
 }
 
-static FILE *realstdout = NULL;
-static const char *output_directory = NULL;
-static int outdir_offset;
-
 static int reopen_stdout(struct commit *commit, const char *subject,
  struct rev_info *rev, int quiet)
 {
--
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] format-patch: introduce format.outputDirectory configuration

Junio C Hamano
Jeff King <[hidden email]> writes:

>> This change looks ugly and unnecessary.  All the machinery after and
>> including the point set_outdir() is called, including reopen_stdout(),
>> work on output_directory variable and only that variable.
>>
>> Wouldn't it work equally well to have
>>
>> if (!output_directory)
>>         output_directory = config_output_directory;
>>
>> before a call to set_outdir() is made but after the configuration is
>> read (namely, soon after parse_options() returns), without making
>> any change to this function?
>
> Don't we load the config before parsing options here? In that case, we
> can use our usual strategy to just set output_directory (which is
> already a static global) from the config callback, and everything Just
> Works.
>
> We do have to bump the definition of output_directory up above the
> config callback, like so (while we are here, we might also want to
> drop the unnecessary static initializers, which violate our style guide):

You would also need to remove the "oh you gave me -o twice?" check,
and change the semantics to "later -o overrides an earlier one",
wouldn't you?  Otherwise you would never be able to override what
you read from the config, I am afraid.
--
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] format-patch: introduce format.outputDirectory configuration

Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> Jeff King <[hidden email]> writes:
>
>>> This change looks ugly and unnecessary.  All the machinery after and
>>> including the point set_outdir() is called, including reopen_stdout(),
>>> work on output_directory variable and only that variable.
>>>
>>> Wouldn't it work equally well to have
>>>
>>> if (!output_directory)
>>>         output_directory = config_output_directory;
>>>
>>> before a call to set_outdir() is made but after the configuration is
>>> read (namely, soon after parse_options() returns), without making
>>> any change to this function?
>>
>> Don't we load the config before parsing options here? In that case, we
>> can use our usual strategy to just set output_directory (which is
>> already a static global) from the config callback, and everything Just
>> Works.
>>
>> We do have to bump the definition of output_directory up above the
>> config callback, like so (while we are here, we might also want to
>> drop the unnecessary static initializers, which violate our style guide):
>
> You would also need to remove the "oh you gave me -o twice?" check,
> and change the semantics to "later -o overrides an earlier one",
> wouldn't you?  Otherwise you would never be able to override what
> you read from the config, I am afraid.

By the way, I actually think "later -o overrides an earlier one" is
a good change by itself, regardless of this new configuration.
--
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] format-patch: introduce format.outputDirectory configuration

Jeff King
On Thu, Jun 18, 2015 at 01:06:54PM -0700, Junio C Hamano wrote:

> >> Don't we load the config before parsing options here? In that case, we
> >> can use our usual strategy to just set output_directory (which is
> >> already a static global) from the config callback, and everything Just
> >> Works.
> >>
> >> We do have to bump the definition of output_directory up above the
> >> config callback, like so (while we are here, we might also want to
> >> drop the unnecessary static initializers, which violate our style guide):
> >
> > You would also need to remove the "oh you gave me -o twice?" check,
> > and change the semantics to "later -o overrides an earlier one",
> > wouldn't you?  Otherwise you would never be able to override what
> > you read from the config, I am afraid.
>
> By the way, I actually think "later -o overrides an earlier one" is
> a good change by itself, regardless of this new configuration.

Ah, I didn't realize we did that. Yeah, I think we should switch to
"later overrides earlier". There is no need for "-o" to behave
completely differently than all of our other options.

-Peff
--
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] format-patch: introduce format.outputDirectory configuration

Jeff King
On Thu, Jun 18, 2015 at 04:13:23PM -0400, Jeff King wrote:

> > > You would also need to remove the "oh you gave me -o twice?" check,
> > > and change the semantics to "later -o overrides an earlier one",
> > > wouldn't you?  Otherwise you would never be able to override what
> > > you read from the config, I am afraid.
> >
> > By the way, I actually think "later -o overrides an earlier one" is
> > a good change by itself, regardless of this new configuration.
>
> Ah, I didn't realize we did that. Yeah, I think we should switch to
> "later overrides earlier". There is no need for "-o" to behave
> completely differently than all of our other options.

Much worse, though, is that we also have to interact with "--stdout". We
currently treat "--stdout -o foo" as an error; you need a separate
config_output_directory to continue to handle that (and allow "--stdout"
to override the config).

If I were designing from scratch, I would consider making "-o -" output
to stdout, and letting it override a previous "-o" (or vice versa). We
could still do that (and make "--stdout" an alias for that), but I don't
know if it is worth the trouble (it does change the behavior for anybody
who wanted a directory called "-", but IMHO it is more likely to save
somebody a headache than create one).

-Peff
--
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] format-patch: introduce format.outputDirectory configuration

Junio C Hamano
Jeff King <[hidden email]> writes:

> Much worse, though, is that we also have to interact with "--stdout". We
> currently treat "--stdout -o foo" as an error; you need a separate
> config_output_directory to continue to handle that (and allow "--stdout"
> to override the config).
>
> If I were designing from scratch, I would consider making "-o -" output
> to stdout, and letting it override a previous "-o" (or vice versa). We
> could still do that (and make "--stdout" an alias for that), but I don't
> know if it is worth the trouble (it does change the behavior for anybody
> who wanted a directory called "-", but IMHO it is more likely to save
> somebody a headache than create one).

I agree with "later -o should override an earlier one", but I do not
necessarily agree with "'-o -' should be --stdout", for a simple
reason that "-o foo" is not "--stdout >foo".

Perhaps something like this to replace builtin/ part of Alexander's
patch?

 builtin/log.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index e67671e..e022d62 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -682,6 +682,8 @@ enum {
  COVER_AUTO
 };
 
+static const char *config_output_directory;
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
  if (!strcmp(var, "format.headers")) {
@@ -752,6 +754,9 @@ static int git_format_config(const char *var, const char *value, void *cb)
  config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
  return 0;
  }
+ if (!strcmp(var, "format.outputdirectory")) {
+ return git_config_string(&config_output_directory, var, value);
+ }
 
  return git_log_config(var, value, cb);
 }
@@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
  die (_("--subject-prefix and -k are mutually exclusive."));
  rev.preserve_subject = keep_subject;
 
+ if (!output_directory && !use_stdout)
+ output_directory = config_output_directory;
+
  argc = setup_revisions(argc, argv, &rev, &s_r_opt);
  if (argc > 1)
  die (_("unrecognized argument: %s"), argv[1]);
--
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] format-patch: introduce format.outputDirectory configuration

Jeff King
On Thu, Jun 18, 2015 at 02:46:36PM -0700, Junio C Hamano wrote:

> > If I were designing from scratch, I would consider making "-o -" output
> > to stdout, and letting it override a previous "-o" (or vice versa). We
> > could still do that (and make "--stdout" an alias for that), but I don't
> > know if it is worth the trouble (it does change the behavior for anybody
> > who wanted a directory called "-", but IMHO it is more likely to save
> > somebody a headache than create one).
>
> I agree with "later -o should override an earlier one", but I do not
> necessarily agree with "'-o -' should be --stdout", for a simple
> reason that "-o foo" is not "--stdout >foo".

Good point. At any rate, that was all in my "designing from scratch"
hypothetical, so it is doubly not worth considering.

> Perhaps something like this to replace builtin/ part of Alexander's
> patch?
> [...]
> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   die (_("--subject-prefix and -k are mutually exclusive."));
>   rev.preserve_subject = keep_subject;
>  
> + if (!output_directory && !use_stdout)
> + output_directory = config_output_directory;
> +

Yeah, I think that is the sanest way to do it given the constraints.

-Peff

--
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] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov
Hello Jeff and Junio,

Thank you for feedback and help. I think also I need to add yet another test
which tests case when configuration option is set and -o passed.

I'll make changes and resend the patch.

Thank you.


2015-06-19 10:14 GMT+06:00 Jeff King <[hidden email]>:

> On Thu, Jun 18, 2015 at 02:46:36PM -0700, Junio C Hamano wrote:
>
>> > If I were designing from scratch, I would consider making "-o -" output
>> > to stdout, and letting it override a previous "-o" (or vice versa). We
>> > could still do that (and make "--stdout" an alias for that), but I don't
>> > know if it is worth the trouble (it does change the behavior for anybody
>> > who wanted a directory called "-", but IMHO it is more likely to save
>> > somebody a headache than create one).
>>
>> I agree with "later -o should override an earlier one", but I do not
>> necessarily agree with "'-o -' should be --stdout", for a simple
>> reason that "-o foo" is not "--stdout >foo".
>
> Good point. At any rate, that was all in my "designing from scratch"
> hypothetical, so it is doubly not worth considering.
>
>> Perhaps something like this to replace builtin/ part of Alexander's
>> patch?
>> [...]
>> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>               die (_("--subject-prefix and -k are mutually exclusive."));
>>       rev.preserve_subject = keep_subject;
>>
>> +     if (!output_directory && !use_stdout)
>> +             output_directory = config_output_directory;
>> +
>
> Yeah, I think that is the sanest way to do it given the constraints.
>
> -Peff
>
--
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] format-patch: introduce format.outputDirectory configuration

Remi Galan Alfonso
In reply to this post by Alexander Kuleshov
Alexander Kuleshov <[hidden email]> writes:
> +test_expect_success "format-patch format.outputDirectory option" '
> + git config format.outputDirectory "patches/" &&
> + git format-patch master..side &&
> + cnt=$(ls | wc -l) &&
> + echo $cnt &&
> + test $cnt = 3 &&
> + git config --unset format.outputDirectory
> +'

You should probably do:
> + test_config format.outputDirectory "patches/" &&

instead of:
> + git config format.outputDirectory "patches/" &&
> [...]
> + git config --unset format.outputDirectory

This way there shouldn't be any problem with the
tests following yours if your test fails in the middle.

Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov
Hello,

Yes, thank you for advice.

2015-06-19 17:34 GMT+06:00 Remi Galan Alfonso
<[hidden email]>:

> Alexander Kuleshov <[hidden email]> writes:
>> +test_expect_success "format-patch format.outputDirectory option" '
>> + git config format.outputDirectory "patches/" &&
>> + git format-patch master..side &&
>> + cnt=$(ls | wc -l) &&
>> + echo $cnt &&
>> + test $cnt = 3 &&
>> + git config --unset format.outputDirectory
>> +'
>
> You should probably do:
>> + test_config format.outputDirectory "patches/" &&
>
> instead of:
>> + git config format.outputDirectory "patches/" &&
>> [...]
>> + git config --unset format.outputDirectory
>
> This way there shouldn't be any problem with the
> tests following yours if your test fails in the middle.
>
> Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov
In reply to this post by Junio C Hamano
2015-06-19 3:46 GMT+06:00 Junio C Hamano <[hidden email]>:

> I agree with "later -o should override an earlier one", but I do not
> necessarily agree with "'-o -' should be --stdout", for a simple
> reason that "-o foo" is not "--stdout >foo".
>
> Perhaps something like this to replace builtin/ part of Alexander's
> patch?
>
> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>                 die (_("--subject-prefix and -k are mutually exclusive."));
>         rev.preserve_subject = keep_subject;
>
> +       if (!output_directory && !use_stdout)
> +               output_directory = config_output_directory;
> +
>

But there is following condition above:

 if (!use_stdout)
      output_directory = set_outdir(prefix, output_directory);

After which output_directory will be "./" everytime and

>
> +       if (!output_directory && !use_stdout)
> +               output_directory = config_output_directory;
> +
>

will not work here. What if we remove if (output_directory) {}....
and update it as:

    if (!use_stdout) {
        if (!config_output_directory && !output_directory)
            output_directory = set_outdir(prefix, output_directory);
        else if (config_output_directory)
            output_directory = config_output_directory;

        if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
            die_errno(_("Could not create directory '%s'"),
                  output_directory);
    }
    else
        setup_pager();

?
--
To unsubscribe from this list: send the line "unsubscribe git" in
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Junio C Hamano
Alexander Kuleshov <[hidden email]> writes:

> 2015-06-19 3:46 GMT+06:00 Junio C Hamano <[hidden email]>:
>> I agree with "later -o should override an earlier one", but I do not
>> necessarily agree with "'-o -' should be --stdout", for a simple
>> reason that "-o foo" is not "--stdout >foo".
>>
>> Perhaps something like this to replace builtin/ part of Alexander's
>> patch?
>>
>> @@ -1337,6 +1342,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>                 die (_("--subject-prefix and -k are mutually exclusive."));
>>         rev.preserve_subject = keep_subject;
>>
>> +       if (!output_directory && !use_stdout)
>> +               output_directory = config_output_directory;
>> +
>>
>
> But there is following condition above:
>
>  if (!use_stdout)
>       output_directory = set_outdir(prefix, output_directory);
>
> After which output_directory will be "./" everytime and
>
>>
>> +       if (!output_directory && !use_stdout)
>> +               output_directory = config_output_directory;
>> +
>>
>
> will not work here.

I thought I made that "if we did not see '-o dir' on the command
line, initialize output_directory to what we read from the config"
before we make a call to set_outdir().

What I am missing?  

Puzzled...  FWIW, IIRC, the patch you are responding to passed the
test you added.

--
To unsubscribe from this list: send the line "unsubscribe git" in
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov
> I thought I made that "if we did not see '-o dir' on the command
> line, initialize output_directory to what we read from the config"
> before we make a call to set_outdir().
>
> What I am missing?
>
> Puzzled...  FWIW, IIRC, the patch you are responding to passed the
> test you added.

Ok, Now we have:

if (!use_stdout)
        output_directory = set_outdir(prefix, output_directory);
else
        setup_pager();

and

if (output_directory) {
    // test that we did not pass use_stdout and mkdir than
}

If we didn't pass --stdout and -o the set_outdir will be called
and there is

static const char *set_outdir(const char *prefix, const char *output_directory)
{
    //printf("is_absoulte_path %d\n", is_absolute_path(output_directory));
    if (output_directory && is_absolute_path(output_directory))
        return output_directory;

    if (!prefix || !*prefix) {
        if (output_directory)
            return output_directory;
        return "./";
    }
....
}

So it returns "./", output_directory will not be null. After this

>> +       if (!output_directory && !use_stdout)
>> +               output_directory = config_output_directory;

clause will not be executed never. Or I've missed something?

Thank you.
--
To unsubscribe from this list: send the line "unsubscribe git" in
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov
Ah, you mean to put this check before. Just tested it and
many tests are broken. Will look on it now

2015-06-19 23:19 GMT+06:00 Alexander Kuleshov <[hidden email]>:

>> I thought I made that "if we did not see '-o dir' on the command
>> line, initialize output_directory to what we read from the config"
>> before we make a call to set_outdir().
>>
>> What I am missing?
>>
>> Puzzled...  FWIW, IIRC, the patch you are responding to passed the
>> test you added.
>
> Ok, Now we have:
>
> if (!use_stdout)
>         output_directory = set_outdir(prefix, output_directory);
> else
>         setup_pager();
>
> and
>
> if (output_directory) {
>     // test that we did not pass use_stdout and mkdir than
> }
>
> If we didn't pass --stdout and -o the set_outdir will be called
> and there is
>
> static const char *set_outdir(const char *prefix, const char *output_directory)
> {
>     //printf("is_absoulte_path %d\n", is_absolute_path(output_directory));
>     if (output_directory && is_absolute_path(output_directory))
>         return output_directory;
>
>     if (!prefix || !*prefix) {
>         if (output_directory)
>             return output_directory;
>         return "./";
>     }
> ....
> }
>
> So it returns "./", output_directory will not be null. After this
>
>>> +       if (!output_directory && !use_stdout)
>>> +               output_directory = config_output_directory;
>
> clause will not be executed never. Or I've missed something?
>
> Thank you.
--
To unsubscribe from this list: send the line "unsubscribe git" in
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Alexander Kuleshov
Sorry for the noise guys, was my fault.

Junio, now all is working and I'm going to send v2.
How to send it better in one patch or separate patches
for the documentation, tests and etc..?

Thank you.
--
To unsubscribe from this list: send the line "unsubscribe git" in
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] format-patch: introduce format.outputDirectory configuration

Junio C Hamano
In reply to this post by Alexander Kuleshov
Alexander Kuleshov <[hidden email]> writes:

> Ah, you mean to put this check before.

I am fuzzy what you mean "before" (or "after"); the "how about doing
it this way instead?" patch we are discussing is to replace the
change you did in your original, so if you apply it you would know
what addition goes to where ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in