Enhancement request: git-push: Allow (configurable) default push-option

Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

There is a need to pass predefined push-option during "git push"
without need to specify it explicitly.

In another words we need to have a new "git config" variable to
specify string that will be automatically passed as "--push-option"
when pushing to remote.

Something like the following:

git config push.optionDefault AllowMultipleCommits

and then command
  git push
would silently run
  git push --push-option "AllowMultipleCommits"

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Stefan Beller]

On Tue, Oct 3, 2017 at 3:15 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
> There is a need to pass predefined push-option during "git push"
> without need to specify it explicitly.
>
> In another words we need to have a new "git config" variable to
> specify string that will be automatically passed as "--push-option"
> when pushing to remote.
>
> Something like the following:
>
> git config push.optionDefault AllowMultipleCommits
>
> and then command
>   git push
> would silently run
>   git push --push-option "AllowMultipleCommits"

We would need to
* design this feature (seems like you already have a good idea what you need)
* implement it (see builtin/push.c):
 - move "struct string_list push_options = STRING_LIST_INIT_DUP;"
  to be a file-static variable, such that we have access to it outside
of cmd_push.
 - In git_push_config in builtin/push.c that parses the config, we'd
need to check
  for "push.optionDefault" and add these to the push_options (I assume multiple
  are allowed)
* document it (Documentation/git-push.txt)
* add a test for it ? (t/t5545-push-options.sh)

Care to write a patch? Otherwise I'd mark it up as part of
#leftoverbits for now,
as it seems like a good starter project.

Thanks,
Stefan

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Hi Stefan,

I will look at it.

Thanks,
Marius


2017-10-03 18:53 GMT+02:00 Stefan Beller <sbeller@google.com>:
> On Tue, Oct 3, 2017 at 3:15 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
>> There is a need to pass predefined push-option during "git push"
>> without need to specify it explicitly.
>>
>> In another words we need to have a new "git config" variable to
>> specify string that will be automatically passed as "--push-option"
>> when pushing to remote.
>>
>> Something like the following:
>>
>> git config push.optionDefault AllowMultipleCommits
>>
>> and then command
>>   git push
>> would silently run
>>   git push --push-option "AllowMultipleCommits"
>
> We would need to
> * design this feature (seems like you already have a good idea what you need)
> * implement it (see builtin/push.c):
>  - move "struct string_list push_options = STRING_LIST_INIT_DUP;"
>   to be a file-static variable, such that we have access to it outside
> of cmd_push.
>  - In git_push_config in builtin/push.c that parses the config, we'd
> need to check
>   for "push.optionDefault" and add these to the push_options (I assume multiple
>   are allowed)
> * document it (Documentation/git-push.txt)
> * add a test for it ? (t/t5545-push-options.sh)
>
> Care to write a patch? Otherwise I'd mark it up as part of
> #leftoverbits for now,
> as it seems like a good starter project.
>
> Thanks,
> Stefan

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Including proposed patch...


Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
---
 Documentation/git-push.txt |  3 +++
 builtin/push.c             | 11 ++++++++++-
 t/t5545-push-options.sh    | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..133c42183 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
     Transmit the given string to the server, which passes them to
     the pre-receive as well as the post-receive hook. The given string
     must not contain a NUL or LF character.
+    Default push options can also be specified with configuration
+    variable `push.optiondefault`. String(s) specified here will always
+    be passed to the server without need to specify it using `--push-option`

 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..4dd5d6f0e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;

+static struct string_list push_options = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
     refspec_nr++;
@@ -467,6 +469,8 @@ static int git_push_config(const char *k, const
char *v, void *cb)
 {
     int *flags = cb;
     int status;
+    const struct string_list *default_push_options;
+    struct string_list_item *item;

     status = git_gpg_config(k, v, NULL);
     if (status)
@@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
char *v, void *cb)
         recurse_submodules = val;
     }

+    default_push_options = git_config_get_value_multi("push.optiondefault");
+    if (default_push_options)
+        for_each_string_list_item(item, default_push_options)
+            if (!string_list_has_string(&push_options, item->string))
+                string_list_append(&push_options, item->string);
+
     return git_default_config(k, v, NULL);
 }

@@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
     int push_cert = -1;
     int rc;
     const char *repo = NULL;    /* default repository */
-    struct string_list push_options = STRING_LIST_INIT_DUP;
     const struct string_list_item *item;

     struct option options[] = {
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..575f3dc38 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,54 @@ test_expect_success 'push options and submodules' '
     test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '

+test_expect_success 'default push option' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.optiondefault=default push up master
+    ) &&
+    test_refs master master &&
+    echo "default" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.optiondefault=default1 -c
push.optiondefault=default2 push up master
+    ) &&
+    test_refs master master &&
+    printf "default1\ndefault2\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'default and manual push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.optiondefault=default push --push-option=manual up master
+    ) &&
+    test_refs master master &&
+    printf "default\nmanual\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

-- 
2.14.1


2017-10-04 17:20 GMT+02:00 Marius Paliga <marius.paliga@gmail.com>:
> Hi Stefan,
>
> I will look at it.
>
> Thanks,
> Marius
>
>
> 2017-10-03 18:53 GMT+02:00 Stefan Beller <sbeller@google.com>:
>> On Tue, Oct 3, 2017 at 3:15 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
>>> There is a need to pass predefined push-option during "git push"
>>> without need to specify it explicitly.
>>>
>>> In another words we need to have a new "git config" variable to
>>> specify string that will be automatically passed as "--push-option"
>>> when pushing to remote.
>>>
>>> Something like the following:
>>>
>>> git config push.optionDefault AllowMultipleCommits
>>>
>>> and then command
>>>   git push
>>> would silently run
>>>   git push --push-option "AllowMultipleCommits"
>>
>> We would need to
>> * design this feature (seems like you already have a good idea what you need)
>> * implement it (see builtin/push.c):
>>  - move "struct string_list push_options = STRING_LIST_INIT_DUP;"
>>   to be a file-static variable, such that we have access to it outside
>> of cmd_push.
>>  - In git_push_config in builtin/push.c that parses the config, we'd
>> need to check
>>   for "push.optionDefault" and add these to the push_options (I assume multiple
>>   are allowed)
>> * document it (Documentation/git-push.txt)
>> * add a test for it ? (t/t5545-push-options.sh)
>>
>> Care to write a patch? Otherwise I'd mark it up as part of
>> #leftoverbits for now,
>> as it seems like a good starter project.
>>
>> Thanks,
>> Stefan

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Found one possible issue when looking for duplicates, we need to use
  "unsorted_string_list_has_string" instead of "string_list_has_string"

-                       if (!string_list_has_string(&push_options,
item->string))
+                       if
(!unsorted_string_list_has_string(&push_options, item->string)) {

New (fixed) patch follows...


Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
---
 Documentation/git-push.txt |  3 +++
 builtin/push.c             | 11 ++++++++++-
 t/t5545-push-options.sh    | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..133c42183 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
     Transmit the given string to the server, which passes them to
     the pre-receive as well as the post-receive hook. The given string
     must not contain a NUL or LF character.
+    Default push options can also be specified with configuration
+    variable `push.optiondefault`. String(s) specified here will always
+    be passed to the server without need to specify it using `--push-option`

 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..ab458419a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;

+static struct string_list push_options = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
     refspec_nr++;
@@ -467,6 +469,8 @@ static int git_push_config(const char *k, const
char *v, void *cb)
 {
     int *flags = cb;
     int status;
+    const struct string_list *default_push_options;
+    struct string_list_item *item;

     status = git_gpg_config(k, v, NULL);
     if (status)
@@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
char *v, void *cb)
         recurse_submodules = val;
     }

+    default_push_options = git_config_get_value_multi("push.optiondefault");
+    if (default_push_options)
+        for_each_string_list_item(item, default_push_options)
+            if (!unsorted_string_list_has_string(&push_options, item->string))
+                string_list_append(&push_options, item->string);
+
     return git_default_config(k, v, NULL);
 }

@@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
     int push_cert = -1;
     int rc;
     const char *repo = NULL;    /* default repository */
-    struct string_list push_options = STRING_LIST_INIT_DUP;
     const struct string_list_item *item;

     struct option options[] = {
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..575f3dc38 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,54 @@ test_expect_success 'push options and submodules' '
     test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '

+test_expect_success 'default push option' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.optiondefault=default push up master
+    ) &&
+    test_refs master master &&
+    echo "default" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.optiondefault=default1 -c
push.optiondefault=default2 push up master
+    ) &&
+    test_refs master master &&
+    printf "default1\ndefault2\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'default and manual push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.optiondefault=default push --push-option=manual up master
+    ) &&
+    test_refs master master &&
+    printf "default\nmanual\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

-- 
2.14.1

2017-10-11 9:14 GMT+02:00 Marius Paliga <marius.paliga@gmail.com>:
> Including proposed patch...
>
>
> Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
> ---
>  Documentation/git-push.txt |  3 +++
>  builtin/push.c             | 11 ++++++++++-
>  t/t5545-push-options.sh    | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 3e76e99f3..133c42183 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -161,6 +161,9 @@ already exists on the remote side.
>      Transmit the given string to the server, which passes them to
>      the pre-receive as well as the post-receive hook. The given string
>      must not contain a NUL or LF character.
> +    Default push options can also be specified with configuration
> +    variable `push.optiondefault`. String(s) specified here will always
> +    be passed to the server without need to specify it using `--push-option`
>
>  --receive-pack=<git-receive-pack>::
>  --exec=<git-receive-pack>::
> diff --git a/builtin/push.c b/builtin/push.c
> index 2ac810422..4dd5d6f0e 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -32,6 +32,8 @@ static const char **refspec;
>  static int refspec_nr;
>  static int refspec_alloc;
>
> +static struct string_list push_options = STRING_LIST_INIT_DUP;
> +
>  static void add_refspec(const char *ref)
>  {
>      refspec_nr++;
> @@ -467,6 +469,8 @@ static int git_push_config(const char *k, const
> char *v, void *cb)
>  {
>      int *flags = cb;
>      int status;
> +    const struct string_list *default_push_options;
> +    struct string_list_item *item;
>
>      status = git_gpg_config(k, v, NULL);
>      if (status)
> @@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
> char *v, void *cb)
>          recurse_submodules = val;
>      }
>
> +    default_push_options = git_config_get_value_multi("push.optiondefault");
> +    if (default_push_options)
> +        for_each_string_list_item(item, default_push_options)
> +            if (!string_list_has_string(&push_options, item->string))
> +                string_list_append(&push_options, item->string);
> +
>      return git_default_config(k, v, NULL);
>  }
>
> @@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
> char *prefix)
>      int push_cert = -1;
>      int rc;
>      const char *repo = NULL;    /* default repository */
> -    struct string_list push_options = STRING_LIST_INIT_DUP;
>      const struct string_list_item *item;
>
>      struct option options[] = {
> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
> index 90a4b0d2f..575f3dc38 100755
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -140,6 +140,54 @@ test_expect_success 'push options and submodules' '
>      test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
>  '
>
> +test_expect_success 'default push option' '
> +    mk_repo_pair &&
> +    git -C upstream config receive.advertisePushOptions true &&
> +    (
> +        cd workbench &&
> +        test_commit one &&
> +        git push --mirror up &&
> +        test_commit two &&
> +        git -c push.optiondefault=default push up master
> +    ) &&
> +    test_refs master master &&
> +    echo "default" >expect &&
> +    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
> +    test_cmp expect upstream/.git/hooks/post-receive.push_options
> +'
> +
> +test_expect_success 'two default push options' '
> +    mk_repo_pair &&
> +    git -C upstream config receive.advertisePushOptions true &&
> +    (
> +        cd workbench &&
> +        test_commit one &&
> +        git push --mirror up &&
> +        test_commit two &&
> +        git -c push.optiondefault=default1 -c
> push.optiondefault=default2 push up master
> +    ) &&
> +    test_refs master master &&
> +    printf "default1\ndefault2\n" >expect &&
> +    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
> +    test_cmp expect upstream/.git/hooks/post-receive.push_options
> +'
> +
> +test_expect_success 'default and manual push options' '
> +    mk_repo_pair &&
> +    git -C upstream config receive.advertisePushOptions true &&
> +    (
> +        cd workbench &&
> +        test_commit one &&
> +        git push --mirror up &&
> +        test_commit two &&
> +        git -c push.optiondefault=default push --push-option=manual up master
> +    ) &&
> +    test_refs master master &&
> +    printf "default\nmanual\n" >expect &&
> +    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
> +    test_cmp expect upstream/.git/hooks/post-receive.push_options
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
>
> --
> 2.14.1
>
>
> 2017-10-04 17:20 GMT+02:00 Marius Paliga <marius.paliga@gmail.com>:
>> Hi Stefan,
>>
>> I will look at it.
>>
>> Thanks,
>> Marius
>>
>>
>> 2017-10-03 18:53 GMT+02:00 Stefan Beller <sbeller@google.com>:
>>> On Tue, Oct 3, 2017 at 3:15 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
>>>> There is a need to pass predefined push-option during "git push"
>>>> without need to specify it explicitly.
>>>>
>>>> In another words we need to have a new "git config" variable to
>>>> specify string that will be automatically passed as "--push-option"
>>>> when pushing to remote.
>>>>
>>>> Something like the following:
>>>>
>>>> git config push.optionDefault AllowMultipleCommits
>>>>
>>>> and then command
>>>>   git push
>>>> would silently run
>>>>   git push --push-option "AllowMultipleCommits"
>>>
>>> We would need to
>>> * design this feature (seems like you already have a good idea what you need)
>>> * implement it (see builtin/push.c):
>>>  - move "struct string_list push_options = STRING_LIST_INIT_DUP;"
>>>   to be a file-static variable, such that we have access to it outside
>>> of cmd_push.
>>>  - In git_push_config in builtin/push.c that parses the config, we'd
>>> need to check
>>>   for "push.optionDefault" and add these to the push_options (I assume multiple
>>>   are allowed)
>>> * document it (Documentation/git-push.txt)
>>> * add a test for it ? (t/t5545-push-options.sh)
>>>
>>> Care to write a patch? Otherwise I'd mark it up as part of
>>> #leftoverbits for now,
>>> as it seems like a good starter project.
>>>
>>> Thanks,
>>> Stefan

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Junio C Hamano]

Marius Paliga <marius.paliga@gmail.com> writes:

> +    default_push_options = git_config_get_value_multi("push.optiondefault");
> +    if (default_push_options)
> +        for_each_string_list_item(item, default_push_options)
> +            if (!string_list_has_string(&push_options, item->string))
> +                string_list_append(&push_options, item->string);

One thing that is often overlooked is how to allow users to override
a multi-value configuration variable that gets some values from
lower priority configuration files (e.g. ~/.gitconfig) with
repository specific settings in .git/config, and the way we
typically do so is to define "When a variable definition with an
empty string is given, it is a signal to clear everything
accumulated so far."  E.g. if your ~/.gitconfig has

	[push]
		defaultPushOption = foo
		defaultPushOption = bar

and then you write in your .git/config something like

	[push]
		defaultPushOption =
		defaultPushOption = baz

The configuration mechanism reads from lower priority files and then
proceeds to read higher priority files, so the parser would read them
in this order:

	push.defaultPushOption = foo
	push.defaultPushOption = bar
	push.defaultPushOption = 
	push.defaultPushOption = baz

and then it would build a list ('foo'), then ('foo', 'bar'), and
clears it upon seeing an empty, and compute the final result as
('baz').

You may want to do something like that in this code.

        

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Junio C Hamano]

Marius Paliga <marius.paliga@gmail.com> writes:

> @@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
> char *v, void *cb)
>          recurse_submodules = val;
>      }
>
> +    default_push_options = git_config_get_value_multi("push.optiondefault");
> +    if (default_push_options)
> +        for_each_string_list_item(item, default_push_options)
> +            if (!string_list_has_string(&push_options, item->string))
> +                string_list_append(&push_options, item->string);
> +
>      return git_default_config(k, v, NULL);
>  }

Sorry for not catching this earlier, but git_config_get_value* call
inside git_push_config() is just wrong.

There are two styles of configuration parsing.  The original (and
still perfectly valid) way is to call git_config() with a callback
function like git_push_config().  Under this style, the config files
are read from lower-priority to higher-priority ones, and the
callback function is called once for each entry found, with <key, value>
pair and the callback specific opaque data.  One way to add the
parsing of a new variable like push.optiondefault is to add

	if (!strcmp(k, "push.optiondefault") {
		... handle one "[push] optiondefault" entry here ...
		return 0;
	}

to the function.

An alternate way is to use git_config_get_* functions _outside_
callback of git_config().  This is a newer invention.  Your call to
git_config_get_value_multi() will scan all configuration files and
returns _all_  entries for the given variable at once.

When there is already a callback style parser, in general, it is
cleaner to simply piggy-back on it, instead of reading variables
independently using git_config_get_* functions.  When there isn't a
callback style parser, using either style is OK.  It also is OK to
switch to git_config_get_* altogether, rewriting the callback style
parser, but I do not think it is warranted in this case, which adds
just one variable.

In any case, with the above code, you'll end up calling the
git_config_get_* function and grabbing all the values for
push.optiondefault for each and every configuration variable
definition (count "git config -l | wc -l" to estimate how many times
it will be called).  Which is probably not what you wanted to do.

Also, watch out for how a configuration variable defined like below
is reported to either of the above two styles:

	[push]	optiondefault

 - To a git_config() callback function like git_push_config(), such
   an entry is called with k=="push.optiondefault", v==NULL.

 - git_config_get_value_multi() would return a string-list element
   with the string set to NULL to signal that one value is NULL
   (i.e. it is different from "[push] optiondefault = ").

I suspect that with your code, we'd hit

	if (strchr(item->string, '\n'))

and end up dereferencing NULL right there.

> @@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
> char *prefix)
>      int push_cert = -1;
>      int rc;
>      const char *repo = NULL;    /* default repository */
> -    struct string_list push_options = STRING_LIST_INIT_DUP;
>      const struct string_list_item *item;
>
>      struct option options[] = {

Also, I suspect that this code does not allow the command line
option to override the default set in the configuration file.
OPT_STRING_LIST() appends to the &push_options string list without
first clearing it, and you are pre-populating the list while reading
the configuration, so the values taken from the command line will
only add to them.

The right way to do this would probably be:

 - Do not muck with push_options in cmd_push().

 - Prepare another string list, push_options_from_config, that is
   file-scope global.

 - In git_push_config(), do not call get_multi; instead react to a
   call with k=="push.optionsdefault" and

   - reject if "v" is NULL, with "return config_error_nonbool(k);"

   - otherwise, append "v" to the "from-config" string list--do not
     attempt to dedup or sort.

   - if "v" is an empty string, clear the "from-config" list.

 - After parse_options() returns to cmd_push(), see if push_options
   is empty.  If it is, you did not get any command line option, so
   override it with what you collected in the "from-config" string
   list.  Otherwise, do not even look at "from-config" string list.

By the way, I really hate "push.optiondefault" as the variable
name.  The "default" part is obvious and there is no need to say it,
as the configuration variables are there to give the default to what
we would normally give from the command line.  Rather, you should
say for which option (there are many options "git push" takes) this
variable gives the default.  Perhaps "push.pushOption" is a much
better name; I am sure people can come up with even better ones,
though ;-)

(no subject)

[Thais D. Braz]

I'll still try to make the test as suggested to complete this demand

[PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option

[Thais D. Braz]

---
 Documentation/git-push.txt | 3 +++
 builtin/push.c             | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..e1036feaf 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
 	Transmit the given string to the server, which passes them to
 	the pre-receive as well as the post-receive hook. The given string
 	must not contain a NUL or LF character.
+	Can be configured using "git config push.optionDefault <option-string>".
+	After configured git push will always be executed silently
+	with --push-options <options configured>.
 
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..ae3efafce 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -467,11 +467,18 @@ static int git_push_config(const char *k, const char *v, void *cb)
 {
 	int *flags = cb;
 	int status;
+	struct string_list push_options = STRING_LIST_INIT_DUP;
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
 
+	const struct string_list *optionsDefault = git_config_get_value_multi("push.optionDefault");
+	for (int i = 0; i < optionsDefault->nr; i++) {
+		string_list_insert(&push_options, optionsDefault->items[i].string);
+	}
+
+
 	if (!strcmp(k, "push.followtags")) {
 		if (git_config_bool(k, v))
 			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
@@ -515,7 +522,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int push_cert = -1;
 	int rc;
 	const char *repo = NULL;	/* default repository */
-	struct string_list push_options = STRING_LIST_INIT_DUP;
+	static struct string_list push_options = STRING_LIST_INIT_DUP;
 	const struct string_list_item *item;
 
 	struct option options[] = {
-- 
2.15.0.rc0.39.g2f0e14e.dirty

[PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option

[Thais D. Braz]

---
 Documentation/git-push.txt | 3 +++
 builtin/push.c             | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..e1036feaf 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
 	Transmit the given string to the server, which passes them to
 	the pre-receive as well as the post-receive hook. The given string
 	must not contain a NUL or LF character.
+	Can be configured using "git config push.optionDefault <option-string>".
+	After configured git push will always be executed silently
+	with --push-options <options configured>.
 
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..ae3efafce 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -467,11 +467,18 @@ static int git_push_config(const char *k, const char *v, void *cb)
 {
 	int *flags = cb;
 	int status;
+	struct string_list push_options = STRING_LIST_INIT_DUP;
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
 
+	const struct string_list *optionsDefault = git_config_get_value_multi("push.optionDefault");
+	for (int i = 0; i < optionsDefault->nr; i++) {
+		string_list_insert(&push_options, optionsDefault->items[i].string);
+	}
+
+
 	if (!strcmp(k, "push.followtags")) {
 		if (git_config_bool(k, v))
 			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
@@ -515,7 +522,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int push_cert = -1;
 	int rc;
 	const char *repo = NULL;	/* default repository */
-	struct string_list push_options = STRING_LIST_INIT_DUP;
+	static struct string_list push_options = STRING_LIST_INIT_DUP;
 	const struct string_list_item *item;
 
 	struct option options[] = {
-- 
2.15.0.rc0.39.g2f0e14e.dirty

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Stefan Beller]

On Wed, Oct 11, 2017 at 2:18 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
> Found one possible issue when looking for duplicates, we need to use
>   "unsorted_string_list_has_string" instead of "string_list_has_string"
>
> -                       if (!string_list_has_string(&push_options,
> item->string))
> +                       if
> (!unsorted_string_list_has_string(&push_options, item->string)) {
>
> New (fixed) patch follows...
>
>
> Signed-off-by: Marius Paliga <marius.paliga@gmail.com>

Yay, thanks for working on this!

Junio gave good advice to the patch itself (the code), another thing
is the commit message, which follows the formalities with the sign off,
but the content is not addressing the target audience.

The current commit message is written as an improvement compared
to the previous patch and for readers who are reviewing the patch
right now.

Commit messages are read by people later in time, also they do not
care about the different iterations of the patch, as only the final iteration
matters.

I think for the commit message you can borrow from the very first email
you sent to the list, maybe something like:

    builtin/push.c: add push.pushOption config

    Currently push options need to be given explicitly, via
    the command line as "git push --push-option".

    Some code review systems [which?] need specific push options
    nearly all the time, so the UX of Git would be enhanced if push
    options could be configured instead of given each time on the\
    command line.

    Add the config option push.pushOption, which is a multi
    string option, containing push options that are sent by default.

    When push options are set in the system wide config
    (/etc/gitconfig), they can be unset(?) later in the more specific
    repository config by setting the string to the empty string.

    Add tests and documentation as well.

    Signed-off-by ...

Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option

[Junio C Hamano]

"Thais D. Braz" <thais.dinizbraz@gmail.com> writes:

> ---
>  Documentation/git-push.txt | 3 +++
>  builtin/push.c             | 9 ++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)

Can somebody explain what is going on?  

I am guessing that Thais and marius are different people (judging by
the fact that one CC's a message to the other).  Are you two
collaborating on this change, or something?

It puzzles me to see almost identical change sent to the list
without much explanation from multiple parties, with no apparent
inter-developer coordination.

Thanks.

Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option

[Christian Couder]

On Thu, Oct 12, 2017 at 3:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Thais D. Braz" <thais.dinizbraz@gmail.com> writes:
>
>> ---
>>  Documentation/git-push.txt | 3 +++
>>  builtin/push.c             | 9 ++++++++-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> Can somebody explain what is going on?
>
> I am guessing that Thais and marius are different people (judging by
> the fact that one CC's a message to the other).  Are you two
> collaborating on this change, or something?

I guess that Thais decided to work on this, because we ask Outreachy
applicants to search for #leftoverbits mentions in the mailing list
archive to find small tasks they could work on.

In this case it looks like Marius sent a patch a few hours before
Thais also sent one.

Thais, I am sorry, but as Marius sent a patch first, I think it is
better if you search for another different small task to work on.
Also please keep Peff and me in cc.

Thanks.

Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> Can somebody explain what is going on?
>>
>> I am guessing that Thais and marius are different people (judging by
>> the fact that one CC's a message to the other).  Are you two
>> collaborating on this change, or something?
>
> I guess that Thais decided to work on this, because we ask Outreachy
> applicants to search for #leftoverbits mentions in the mailing list
> archive to find small tasks they could work on.
>
> In this case it looks like Marius sent a patch a few hours before
> Thais also sent one.

... after seeing Marius's already working on it, I think.

> Thais, I am sorry, but as Marius sent a patch first, I think it is
> better if you search for another different small task to work on.

In general, I do not mind seeing people working together well, and
it is one of the more important skills necessary in the open source
community.  I however tend to agree with you that this is a bit too
small a topic for multiple people to be working on.

> Also please keep Peff and me in cc.

Yup, that is always a good idea.

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Thank you for your coments and explanation.

Just one thing:

>  - After parse_options() returns to cmd_push(), see if push_options
>    is empty.  If it is, you did not get any command line option, so
>    override it with what you collected in the "from-config" string
>    list.  Otherwise, do not even look at "from-config" string list.

The idea is that there are default push options (read from config) that are
always sent to the server and you can add (not overwrite) additional by
specifying "--push-option".
So I would rather concatenate both lists - from command line and from-config.

> By the way, I really hate "push.optiondefault" as the variable
> name.  The "default" part is obvious and there is no need to say it,
> as the configuration variables are there to give the default to what
> we would normally give from the command line.  Rather, you should
> say for which option (there are many options "git push" takes) this
> variable gives the default.  Perhaps "push.pushOption" is a much
> better name; I am sure people can come up with even better ones,
> though ;-)

In the light of the above the "default" may be correct, but I don't
have a problem
with any name.

Marius


2017-10-11 15:38 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Marius Paliga <marius.paliga@gmail.com> writes:
>
>> @@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
>> char *v, void *cb)
>>          recurse_submodules = val;
>>      }
>>
>> +    default_push_options = git_config_get_value_multi("push.optiondefault");
>> +    if (default_push_options)
>> +        for_each_string_list_item(item, default_push_options)
>> +            if (!string_list_has_string(&push_options, item->string))
>> +                string_list_append(&push_options, item->string);
>> +
>>      return git_default_config(k, v, NULL);
>>  }
>
> Sorry for not catching this earlier, but git_config_get_value* call
> inside git_push_config() is just wrong.
>
> There are two styles of configuration parsing.  The original (and
> still perfectly valid) way is to call git_config() with a callback
> function like git_push_config().  Under this style, the config files
> are read from lower-priority to higher-priority ones, and the
> callback function is called once for each entry found, with <key, value>
> pair and the callback specific opaque data.  One way to add the
> parsing of a new variable like push.optiondefault is to add
>
>         if (!strcmp(k, "push.optiondefault") {
>                 ... handle one "[push] optiondefault" entry here ...
>                 return 0;
>         }
>
> to the function.
>
> An alternate way is to use git_config_get_* functions _outside_
> callback of git_config().  This is a newer invention.  Your call to
> git_config_get_value_multi() will scan all configuration files and
> returns _all_  entries for the given variable at once.
>
> When there is already a callback style parser, in general, it is
> cleaner to simply piggy-back on it, instead of reading variables
> independently using git_config_get_* functions.  When there isn't a
> callback style parser, using either style is OK.  It also is OK to
> switch to git_config_get_* altogether, rewriting the callback style
> parser, but I do not think it is warranted in this case, which adds
> just one variable.
>
> In any case, with the above code, you'll end up calling the
> git_config_get_* function and grabbing all the values for
> push.optiondefault for each and every configuration variable
> definition (count "git config -l | wc -l" to estimate how many times
> it will be called).  Which is probably not what you wanted to do.
>
> Also, watch out for how a configuration variable defined like below
> is reported to either of the above two styles:
>
>         [push]  optiondefault
>
>  - To a git_config() callback function like git_push_config(), such
>    an entry is called with k=="push.optiondefault", v==NULL.
>
>  - git_config_get_value_multi() would return a string-list element
>    with the string set to NULL to signal that one value is NULL
>    (i.e. it is different from "[push] optiondefault = ").
>
> I suspect that with your code, we'd hit
>
>         if (strchr(item->string, '\n'))
>
> and end up dereferencing NULL right there.
>
>> @@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
>> char *prefix)
>>      int push_cert = -1;
>>      int rc;
>>      const char *repo = NULL;    /* default repository */
>> -    struct string_list push_options = STRING_LIST_INIT_DUP;
>>      const struct string_list_item *item;
>>
>>      struct option options[] = {
>
> Also, I suspect that this code does not allow the command line
> option to override the default set in the configuration file.
> OPT_STRING_LIST() appends to the &push_options string list without
> first clearing it, and you are pre-populating the list while reading
> the configuration, so the values taken from the command line will
> only add to them.
>
> The right way to do this would probably be:
>
>  - Do not muck with push_options in cmd_push().
>
>  - Prepare another string list, push_options_from_config, that is
>    file-scope global.
>
>  - In git_push_config(), do not call get_multi; instead react to a
>    call with k=="push.optionsdefault" and
>
>    - reject if "v" is NULL, with "return config_error_nonbool(k);"
>
>    - otherwise, append "v" to the "from-config" string list--do not
>      attempt to dedup or sort.
>
>    - if "v" is an empty string, clear the "from-config" list.
>
>  - After parse_options() returns to cmd_push(), see if push_options
>    is empty.  If it is, you did not get any command line option, so
>    override it with what you collected in the "from-config" string
>    list.  Otherwise, do not even look at "from-config" string list.
>
> By the way, I really hate "push.optiondefault" as the variable
> name.  The "default" part is obvious and there is no need to say it,
> as the configuration variables are there to give the default to what
> we would normally give from the command line.  Rather, you should
> say for which option (there are many options "git push" takes) this
> variable gives the default.  Perhaps "push.pushOption" is a much
> better name; I am sure people can come up with even better ones,
> though ;-)
>

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Subject: [PATCH] Added support for new configuration parameter push.pushOption

builtin/push.c: add push.pushOption config

Currently push options need to be given explicitly, via
the command line as "git push --push-option".

The UX of Git would be enhanced if push options could be
configured instead of given each time on the command line.

Add the config option push.pushOption, which is a multi
string option, containing push options that are sent by default.

When push options are set in the system wide config
(/etc/gitconfig), they can be unset later in the more specific
repository config by setting the string to the empty string.

Add tests and documentation as well.

Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
---
 Documentation/git-push.txt |  3 +++
 builtin/push.c             | 11 ++++++++++-
 t/t5545-push-options.sh    | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..da9b17624 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
     Transmit the given string to the server, which passes them to
     the pre-receive as well as the post-receive hook. The given string
     must not contain a NUL or LF character.
+    Default push options can also be specified with configuration
+    variable `push.pushOption`. String(s) specified here will always
+    be passed to the server without need to specify it using `--push-option`

 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..f761fe4ba 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;

+static struct string_list push_options = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
     refspec_nr++;
@@ -503,6 +505,14 @@ static int git_push_config(const char *k, const
char *v, void *cb)
         int val = git_config_bool(k, v) ?
             RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
         recurse_submodules = val;
+    } else if (!strcmp(k, "push.pushoption")) {
+        if (v == NULL)
+            return config_error_nonbool(k);
+        else
+            if (v[0] == '\0')
+                string_list_clear(&push_options, 0);
+            else
+                string_list_append(&push_options, v);
     }

     return git_default_config(k, v, NULL);
@@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
     int push_cert = -1;
     int rc;
     const char *repo = NULL;    /* default repository */
-    struct string_list push_options = STRING_LIST_INIT_DUP;
     const struct string_list_item *item;

     struct option options[] = {
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..2cf9f7968 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,54 @@ test_expect_success 'push options and submodules' '
     test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '

+test_expect_success 'default push option' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default push up master
+    ) &&
+    test_refs master master &&
+    echo "default" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default1 -c push.pushOption=default2
push up master
+    ) &&
+    test_refs master master &&
+    printf "default1\ndefault2\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'default and manual push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default push --push-option=manual up master
+    ) &&
+    test_refs master master &&
+    printf "default\nmanual\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

-- 
2.14.1


2017-10-12 16:59 GMT+02:00 Marius Paliga <marius.paliga@gmail.com>:
> Thank you for your coments and explanation.
>
> Just one thing:
>
>>  - After parse_options() returns to cmd_push(), see if push_options
>>    is empty.  If it is, you did not get any command line option, so
>>    override it with what you collected in the "from-config" string
>>    list.  Otherwise, do not even look at "from-config" string list.
>
> The idea is that there are default push options (read from config) that are
> always sent to the server and you can add (not overwrite) additional by
> specifying "--push-option".
> So I would rather concatenate both lists - from command line and from-config.
>
>> By the way, I really hate "push.optiondefault" as the variable
>> name.  The "default" part is obvious and there is no need to say it,
>> as the configuration variables are there to give the default to what
>> we would normally give from the command line.  Rather, you should
>> say for which option (there are many options "git push" takes) this
>> variable gives the default.  Perhaps "push.pushOption" is a much
>> better name; I am sure people can come up with even better ones,
>> though ;-)
>
> In the light of the above the "default" may be correct, but I don't
> have a problem
> with any name.
>
> Marius
>
>
> 2017-10-11 15:38 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>> Marius Paliga <marius.paliga@gmail.com> writes:
>>
>>> @@ -505,6 +509,12 @@ static int git_push_config(const char *k, const
>>> char *v, void *cb)
>>>          recurse_submodules = val;
>>>      }
>>>
>>> +    default_push_options = git_config_get_value_multi("push.optiondefault");
>>> +    if (default_push_options)
>>> +        for_each_string_list_item(item, default_push_options)
>>> +            if (!string_list_has_string(&push_options, item->string))
>>> +                string_list_append(&push_options, item->string);
>>> +
>>>      return git_default_config(k, v, NULL);
>>>  }
>>
>> Sorry for not catching this earlier, but git_config_get_value* call
>> inside git_push_config() is just wrong.
>>
>> There are two styles of configuration parsing.  The original (and
>> still perfectly valid) way is to call git_config() with a callback
>> function like git_push_config().  Under this style, the config files
>> are read from lower-priority to higher-priority ones, and the
>> callback function is called once for each entry found, with <key, value>
>> pair and the callback specific opaque data.  One way to add the
>> parsing of a new variable like push.optiondefault is to add
>>
>>         if (!strcmp(k, "push.optiondefault") {
>>                 ... handle one "[push] optiondefault" entry here ...
>>                 return 0;
>>         }
>>
>> to the function.
>>
>> An alternate way is to use git_config_get_* functions _outside_
>> callback of git_config().  This is a newer invention.  Your call to
>> git_config_get_value_multi() will scan all configuration files and
>> returns _all_  entries for the given variable at once.
>>
>> When there is already a callback style parser, in general, it is
>> cleaner to simply piggy-back on it, instead of reading variables
>> independently using git_config_get_* functions.  When there isn't a
>> callback style parser, using either style is OK.  It also is OK to
>> switch to git_config_get_* altogether, rewriting the callback style
>> parser, but I do not think it is warranted in this case, which adds
>> just one variable.
>>
>> In any case, with the above code, you'll end up calling the
>> git_config_get_* function and grabbing all the values for
>> push.optiondefault for each and every configuration variable
>> definition (count "git config -l | wc -l" to estimate how many times
>> it will be called).  Which is probably not what you wanted to do.
>>
>> Also, watch out for how a configuration variable defined like below
>> is reported to either of the above two styles:
>>
>>         [push]  optiondefault
>>
>>  - To a git_config() callback function like git_push_config(), such
>>    an entry is called with k=="push.optiondefault", v==NULL.
>>
>>  - git_config_get_value_multi() would return a string-list element
>>    with the string set to NULL to signal that one value is NULL
>>    (i.e. it is different from "[push] optiondefault = ").
>>
>> I suspect that with your code, we'd hit
>>
>>         if (strchr(item->string, '\n'))
>>
>> and end up dereferencing NULL right there.
>>
>>> @@ -515,7 +525,6 @@ int cmd_push(int argc, const char **argv, const
>>> char *prefix)
>>>      int push_cert = -1;
>>>      int rc;
>>>      const char *repo = NULL;    /* default repository */
>>> -    struct string_list push_options = STRING_LIST_INIT_DUP;
>>>      const struct string_list_item *item;
>>>
>>>      struct option options[] = {
>>
>> Also, I suspect that this code does not allow the command line
>> option to override the default set in the configuration file.
>> OPT_STRING_LIST() appends to the &push_options string list without
>> first clearing it, and you are pre-populating the list while reading
>> the configuration, so the values taken from the command line will
>> only add to them.
>>
>> The right way to do this would probably be:
>>
>>  - Do not muck with push_options in cmd_push().
>>
>>  - Prepare another string list, push_options_from_config, that is
>>    file-scope global.
>>
>>  - In git_push_config(), do not call get_multi; instead react to a
>>    call with k=="push.optionsdefault" and
>>
>>    - reject if "v" is NULL, with "return config_error_nonbool(k);"
>>
>>    - otherwise, append "v" to the "from-config" string list--do not
>>      attempt to dedup or sort.
>>
>>    - if "v" is an empty string, clear the "from-config" list.
>>
>>  - After parse_options() returns to cmd_push(), see if push_options
>>    is empty.  If it is, you did not get any command line option, so
>>    override it with what you collected in the "from-config" string
>>    list.  Otherwise, do not even look at "from-config" string list.
>>
>> By the way, I really hate "push.optiondefault" as the variable
>> name.  The "default" part is obvious and there is no need to say it,
>> as the configuration variables are there to give the default to what
>> we would normally give from the command line.  Rather, you should
>> say for which option (there are many options "git push" takes) this
>> variable gives the default.  Perhaps "push.pushOption" is a much
>> better name; I am sure people can come up with even better ones,
>> though ;-)
>>

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Stefan Beller]

On Thu, Oct 12, 2017 at 9:32 AM, Marius Paliga <marius.paliga@gmail.com> wrote:
> Subject: [PATCH] Added support for new configuration parameter push.pushOption
>
> builtin/push.c: add push.pushOption config
>
> Currently push options need to be given explicitly, via
> the command line as "git push --push-option".
>
> The UX of Git would be enhanced if push options could be
> configured instead of given each time on the command line.
>
> Add the config option push.pushOption, which is a multi
> string option, containing push options that are sent by default.
>
> When push options are set in the system wide config
> (/etc/gitconfig), they can be unset later in the more specific
> repository config by setting the string to the empty string.

Now that I review this patch, this is nice information and can
remain in the commit message, but it would be more useful
in the Documentation as that is where the users look.
(Another thing regarding the documentation: Maybe we want
to update Documentation/config.txt as well, that contains all
configuration)

> @@ -503,6 +505,14 @@ static int git_push_config(const char *k, const
> char *v, void *cb)
>          int val = git_config_bool(k, v) ?
>              RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
>          recurse_submodules = val;
> +    } else if (!strcmp(k, "push.pushoption")) {
> +        if (v == NULL)
> +            return config_error_nonbool(k);
> +        else
> +            if (v[0] == '\0')
> +                string_list_clear(&push_options, 0);

Junio,
do we have variables that behave similarly to this?

(I just wondered if the `v == NULL` could be lumped in
to here, resetting the string list)

>
> +test_expect_success 'default push option' '
...
> +'
> +
> +test_expect_success 'two default push options' '
...
> +'
> +
> +test_expect_success 'default and manual push options' '
...
> +'

Thanks for adding thorough tests!
Do we also need tests for the reset of the list?

Thanks,
Stefan

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Junio C Hamano]

Marius Paliga <marius.paliga@gmail.com> writes:

> Thank you for your coments and explanation.
>
> Just one thing:
>
>>  - After parse_options() returns to cmd_push(), see if push_options
>>    is empty.  If it is, you did not get any command line option, so
>>    override it with what you collected in the "from-config" string
>>    list.  Otherwise, do not even look at "from-config" string list.
>
> The idea is that there are default push options (read from config) that are
> always sent to the server and you can add (not overwrite) additional by
> specifying "--push-option".

I can imagine that sometimes giving a base from a configuration and
then adding more for specific invocation may be useful.  

But I do not think of any --command-line-option and config.variable
pair whose configured value cannot be overriden by the command line
option; we should strive to avoid making --push-option a special
case that the users need to aware of, and more importantly, users
other than you who expect the more usual "command line overrides"
behaviour should get that.

Your "I wanted to accumulate, so I made so and made it impossible to
override" won't fly as a justification.  The default should be
"command line overrides", and if you need a way to allow command
line to add without overiding, that should be added as an optional
feature.

	[alias]
		mypush = push --push-option=foo --push-option=bar

may give you a set of push-options that are always in effect (they
are not even "by default") and cannot be overriden.

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Junio,
> do we have variables that behave similarly to this?

If you scan Documentation/config.txt, you'll find http.extraHeader
as an example' but I do not recall offhand if that was the original
that introduced the convention, or it merely followed precedence set
by other variables.

Re: Enhancement request: git-push: Allow (configurable) default push-option

[Marius Paliga]

Updated patch and added tests:
(feel free to modify Documentation)

---

Subject: [PATCH] Added support for new configuration parameter push.pushOption

builtin/push.c: add push.pushOption config

Currently push options need to be given explicitly, via
the command line as "git push --push-option".

The UX of Git would be enhanced if push options could be
configured instead of given each time on the command line.

Add the config option push.pushOption, which is a multi
string option, containing push options that are sent by default.

When push options are set in the system wide config
(/etc/gitconfig), they can be unset later in the more specific
repository config by setting the string to the empty string.

Add tests and documentation as well.

Signed-off-by: Marius Paliga <marius.paliga@gmail.com>
---
 Documentation/git-push.txt |  3 ++
 builtin/push.c             | 12 ++++++++
 t/t5545-push-options.sh    | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3e76e99f3..da9b17624 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -161,6 +161,9 @@ already exists on the remote side.
     Transmit the given string to the server, which passes them to
     the pre-receive as well as the post-receive hook. The given string
     must not contain a NUL or LF character.
+    Default push options can also be specified with configuration
+    variable `push.pushOption`. String(s) specified here will always
+    be passed to the server without need to specify it using `--push-option`

 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
diff --git a/builtin/push.c b/builtin/push.c
index 2ac810422..10e520c8f 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -32,6 +32,8 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;

+static struct string_list push_options_from_config = STRING_LIST_INIT_DUP;
+
 static void add_refspec(const char *ref)
 {
     refspec_nr++;
@@ -503,6 +505,14 @@ static int git_push_config(const char *k, const
char *v, void *cb)
         int val = git_config_bool(k, v) ?
             RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
         recurse_submodules = val;
+    } else if (!strcmp(k, "push.pushoption")) {
+        if (v == NULL)
+            return config_error_nonbool(k);
+        else
+            if (v[0] == '\0')
+                string_list_clear(&push_options_from_config, 0);
+            else
+                string_list_append(&push_options_from_config, v);
     }

     return git_default_config(k, v, NULL);
@@ -562,6 +572,8 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
     packet_trace_identity("push");
     git_config(git_push_config, &flags);
     argc = parse_options(argc, argv, prefix, options, push_usage, 0);
+    if (!push_options.nr)
+        push_options = push_options_from_config;
     set_push_cert_flags(&flags, push_cert);

     if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL |
TRANSPORT_PUSH_MIRROR))))
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 90a4b0d2f..463783789 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '
     test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '

+test_expect_success 'default push option' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default push up master
+    ) &&
+    test_refs master master &&
+    echo "default" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'two default push options' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default1 -c push.pushOption=default2
push up master
+    ) &&
+    test_refs master master &&
+    printf "default1\ndefault2\n" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'push option from command line overrides
from-config push option' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default push --push-option=manual up master
+    ) &&
+    test_refs master master &&
+    echo "manual" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'empty value of push.pushOption in config clears
the list' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        git -c push.pushOption=default1 -c push.pushOption= -c
push.pushOption=default2 push up master
+    ) &&
+    test_refs master master &&
+    echo "default2" >expect &&
+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+    test_cmp expect upstream/.git/hooks/post-receive.push_options
+'
+
+test_expect_success 'invalid push option in config' '
+    mk_repo_pair &&
+    git -C upstream config receive.advertisePushOptions true &&
+    (
+        cd workbench &&
+        test_commit one &&
+        git push --mirror up &&
+        test_commit two &&
+        test_must_fail git -c push.pushOption push up master
+    ) &&
+    test_refs master HEAD@{1}
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

-- 
2.14.1


2017-10-13 2:37 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Marius Paliga <marius.paliga@gmail.com> writes:
>
>> Thank you for your coments and explanation.
>>
>> Just one thing:
>>
>>>  - After parse_options() returns to cmd_push(), see if push_options
>>>    is empty.  If it is, you did not get any command line option, so
>>>    override it with what you collected in the "from-config" string
>>>    list.  Otherwise, do not even look at "from-config" string list.
>>
>> The idea is that there are default push options (read from config) that are
>> always sent to the server and you can add (not overwrite) additional by
>> specifying "--push-option".
>
> I can imagine that sometimes giving a base from a configuration and
> then adding more for specific invocation may be useful.
>
> But I do not think of any --command-line-option and config.variable
> pair whose configured value cannot be overriden by the command line
> option; we should strive to avoid making --push-option a special
> case that the users need to aware of, and more importantly, users
> other than you who expect the more usual "command line overrides"
> behaviour should get that.
>
> Your "I wanted to accumulate, so I made so and made it impossible to
> override" won't fly as a justification.  The default should be
> "command line overrides", and if you need a way to allow command
> line to add without overiding, that should be added as an optional
> feature.
>
>         [alias]
>                 mypush = push --push-option=foo --push-option=bar
>
> may give you a set of push-options that are always in effect (they
> are not even "by default") and cannot be overriden.
>

[PATCH] patch reply

[Thais Diniz]

From: Thais Diniz Braz <thais.dinizbraz@gmail.com>

---
 emailReply | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 emailReply

diff --git a/emailReply b/emailReply
new file mode 100644
index 000000000..2d591b55b
--- /dev/null
+++ b/emailReply
@@ -0,0 +1,4 @@
+Just to clarify I did not see Marius patch.
+Did see Marius' comment saying he would look it in the leftoverbits list,
+but since i didn't see any patch i thought i could work on it and did so based on Stephan's comment 
+(which i suppose Mario also did and that is why the code resulted to be similar).
-- 
2.15.0.rc0.39.g2f0e14e.dirty

Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option

[thais braz]

Just to clarify I did not see Marius patch.
Did see Marius' comment saying he would look it in the leftoverbits list,
but since i didn't see any patch i thought i could work on it and did
so based on Stephan's comment
(which i suppose Mario also did and that is why the code resulted to
be similar).

And sorry send this email as patch. Didn't know how to use git
send-email just as reply

On Thu, Oct 12, 2017 at 12:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>>> Can somebody explain what is going on?
>>>
>>> I am guessing that Thais and marius are different people (judging by
>>> the fact that one CC's a message to the other).  Are you two
>>> collaborating on this change, or something?
>>
>> I guess that Thais decided to work on this, because we ask Outreachy
>> applicants to search for #leftoverbits mentions in the mailing list
>> archive to find small tasks they could work on.
>>
>> In this case it looks like Marius sent a patch a few hours before
>> Thais also sent one.
>
> ... after seeing Marius's already working on it, I think.
>
>> Thais, I am sorry, but as Marius sent a patch first, I think it is
>> better if you search for another different small task to work on.
>
> In general, I do not mind seeing people working together well, and
> it is one of the more important skills necessary in the open source
> community.  I however tend to agree with you that this is a bit too
> small a topic for multiple people to be working on.
>
>> Also please keep Peff and me in cc.
>
> Yup, that is always a good idea.
>



-- 
Atenciosamente Thais Diniz Braz

Re: [PATCH] patch reply

[Junio C Hamano]

Thais Diniz <thais.dinizbraz@gmail.com> writes:

> +Just to clarify I did not see Marius patch.
> +Did see Marius' comment saying he would look it in the leftoverbits list,
> +but since i didn't see any patch i thought i could work on it and did so based on Stephan's comment 
> +(which i suppose Mario also did and that is why the code resulted to be similar).

In any case, both versions share exactly the same "don't call
get_multi() to grab the same configuration values from inside the
callback of git_config()" issue, so whoever works on it to complete
the topic, it needs further work.

Re: [PATCH] patch reply

[Marius Paliga]

I just sent a patch to a new thread which is not what I wanted.
However I already sent the same patch a few days ago:
https://public-inbox.org/git/CAK7vU=2ePR3jQsgu=RxSMrxytAAHqxC0SFrN5YozLzQzP2ZT2A@mail.gmail.com/


2017-10-17 6:01 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Thais Diniz <thais.dinizbraz@gmail.com> writes:
>
>> +Just to clarify I did not see Marius patch.
>> +Did see Marius' comment saying he would look it in the leftoverbits list,
>> +but since i didn't see any patch i thought i could work on it and did so based on Stephan's comment
>> +(which i suppose Mario also did and that is why the code resulted to be similar).
>
> In any case, both versions share exactly the same "don't call
> get_multi() to grab the same configuration values from inside the
> callback of git_config()" issue, so whoever works on it to complete
> the topic, it needs further work.