* Enhancement request: git-push: Allow (configurable) default push-option @ 2017-10-03 10:15 Marius Paliga 2017-10-03 16:53 ` Stefan Beller 0 siblings, 1 reply; 24+ messages in thread From: Marius Paliga @ 2017-10-03 10:15 UTC (permalink / raw) To: git 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" ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-03 10:15 Enhancement request: git-push: Allow (configurable) default push-option Marius Paliga @ 2017-10-03 16:53 ` Stefan Beller 2017-10-04 15:20 ` Marius Paliga 0 siblings, 1 reply; 24+ messages in thread From: Stefan Beller @ 2017-10-03 16:53 UTC (permalink / raw) To: Marius Paliga; +Cc: git@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-03 16:53 ` Stefan Beller @ 2017-10-04 15:20 ` Marius Paliga 2017-10-11 7:14 ` Marius Paliga ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Marius Paliga @ 2017-10-04 15:20 UTC (permalink / raw) To: Stefan Beller; +Cc: git@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-04 15:20 ` Marius Paliga @ 2017-10-11 7:14 ` Marius Paliga 2017-10-11 9:18 ` Marius Paliga ` (2 more replies) 2017-10-11 20:25 ` Thais D. Braz 2017-10-11 20:40 ` Thais D. Braz 2 siblings, 3 replies; 24+ messages in thread From: Marius Paliga @ 2017-10-11 7:14 UTC (permalink / raw) To: Stefan Beller; +Cc: git@vger.kernel.org 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-11 7:14 ` Marius Paliga @ 2017-10-11 9:18 ` Marius Paliga 2017-10-11 20:52 ` Stefan Beller 2017-10-11 11:13 ` Junio C Hamano 2017-10-11 13:38 ` Junio C Hamano 2 siblings, 1 reply; 24+ messages in thread From: Marius Paliga @ 2017-10-11 9:18 UTC (permalink / raw) To: Stefan Beller; +Cc: git@vger.kernel.org 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-11 9:18 ` Marius Paliga @ 2017-10-11 20:52 ` Stefan Beller 0 siblings, 0 replies; 24+ messages in thread From: Stefan Beller @ 2017-10-11 20:52 UTC (permalink / raw) To: Marius Paliga; +Cc: git@vger.kernel.org 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 ... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-11 7:14 ` Marius Paliga 2017-10-11 9:18 ` Marius Paliga @ 2017-10-11 11:13 ` Junio C Hamano 2017-10-11 13:38 ` Junio C Hamano 2 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2017-10-11 11:13 UTC (permalink / raw) To: Marius Paliga; +Cc: Stefan Beller, git@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-11 7:14 ` Marius Paliga 2017-10-11 9:18 ` Marius Paliga 2017-10-11 11:13 ` Junio C Hamano @ 2017-10-11 13:38 ` Junio C Hamano 2017-10-12 14:59 ` Marius Paliga 2 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-10-11 13:38 UTC (permalink / raw) To: Marius Paliga; +Cc: Stefan Beller, git@vger.kernel.org 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 ;-) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-11 13:38 ` Junio C Hamano @ 2017-10-12 14:59 ` Marius Paliga 2017-10-12 16:32 ` Marius Paliga 2017-10-13 0:37 ` Junio C Hamano 0 siblings, 2 replies; 24+ messages in thread From: Marius Paliga @ 2017-10-12 14:59 UTC (permalink / raw) To: Junio C Hamano Cc: Stefan Beller, git@vger.kernel.org, christian.couder, peff, thais.dinizbraz 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 ;-) > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-12 14:59 ` Marius Paliga @ 2017-10-12 16:32 ` Marius Paliga 2017-10-12 16:51 ` Stefan Beller 2017-10-13 0:37 ` Junio C Hamano 1 sibling, 1 reply; 24+ messages in thread From: Marius Paliga @ 2017-10-12 16:32 UTC (permalink / raw) To: Junio C Hamano Cc: Stefan Beller, git@vger.kernel.org, christian.couder, peff, thais.dinizbraz 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 ;-) >> ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-12 16:32 ` Marius Paliga @ 2017-10-12 16:51 ` Stefan Beller 2017-10-13 1:39 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Stefan Beller @ 2017-10-12 16:51 UTC (permalink / raw) To: Marius Paliga Cc: Junio C Hamano, git@vger.kernel.org, Christian Couder, Jeff King, thais.dinizbraz 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-12 16:51 ` Stefan Beller @ 2017-10-13 1:39 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2017-10-13 1:39 UTC (permalink / raw) To: Stefan Beller Cc: Marius Paliga, git@vger.kernel.org, Christian Couder, Jeff King, thais.dinizbraz 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-12 14:59 ` Marius Paliga 2017-10-12 16:32 ` Marius Paliga @ 2017-10-13 0:37 ` Junio C Hamano 2017-10-13 8:45 ` Marius Paliga 1 sibling, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-10-13 0:37 UTC (permalink / raw) To: Marius Paliga Cc: Stefan Beller, git@vger.kernel.org, christian.couder, peff, thais.dinizbraz 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Enhancement request: git-push: Allow (configurable) default push-option 2017-10-13 0:37 ` Junio C Hamano @ 2017-10-13 8:45 ` Marius Paliga 0 siblings, 0 replies; 24+ messages in thread From: Marius Paliga @ 2017-10-13 8:45 UTC (permalink / raw) To: Junio C Hamano Cc: Stefan Beller, git@vger.kernel.org, Christian Couder, Jeff King, thais.dinizbraz 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. > ^ permalink raw reply related [flat|nested] 24+ messages in thread
* (no subject) 2017-10-04 15:20 ` Marius Paliga 2017-10-11 7:14 ` Marius Paliga @ 2017-10-11 20:25 ` Thais D. Braz 2017-10-11 20:25 ` [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option Thais D. Braz 2017-10-11 20:40 ` Thais D. Braz 2 siblings, 1 reply; 24+ messages in thread From: Thais D. Braz @ 2017-10-11 20:25 UTC (permalink / raw) To: marius.paliga; +Cc: git, sbeller I'll still try to make the test as suggested to complete this demand ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option 2017-10-11 20:25 ` Thais D. Braz @ 2017-10-11 20:25 ` Thais D. Braz 2017-10-12 1:21 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Thais D. Braz @ 2017-10-11 20:25 UTC (permalink / raw) To: marius.paliga; +Cc: git, sbeller, 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option 2017-10-11 20:25 ` [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option Thais D. Braz @ 2017-10-12 1:21 ` Junio C Hamano 2017-10-12 2:41 ` Christian Couder 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-10-12 1:21 UTC (permalink / raw) To: Thais D. Braz; +Cc: marius.paliga, git, sbeller "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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option 2017-10-12 1:21 ` Junio C Hamano @ 2017-10-12 2:41 ` Christian Couder 2017-10-12 3:26 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Christian Couder @ 2017-10-12 2:41 UTC (permalink / raw) To: Junio C Hamano, Thais D. Braz Cc: marius.paliga, git, Stefan Beller, Jeff King 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option 2017-10-12 2:41 ` Christian Couder @ 2017-10-12 3:26 ` Junio C Hamano 2017-10-17 3:47 ` [PATCH] patch reply Thais Diniz 2017-10-17 3:58 ` [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option thais braz 0 siblings, 2 replies; 24+ messages in thread From: Junio C Hamano @ 2017-10-12 3:26 UTC (permalink / raw) To: Christian Couder Cc: Thais D. Braz, marius.paliga, git, Stefan Beller, Jeff King 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] patch reply 2017-10-12 3:26 ` Junio C Hamano @ 2017-10-17 3:47 ` Thais Diniz 2017-10-17 4:01 ` Junio C Hamano 2017-10-17 3:58 ` [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option thais braz 1 sibling, 1 reply; 24+ messages in thread From: Thais Diniz @ 2017-10-17 3:47 UTC (permalink / raw) To: gitster Cc: christian.couder, git, marius.paliga, peff, sbeller, thais.dinizbraz 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] patch reply 2017-10-17 3:47 ` [PATCH] patch reply Thais Diniz @ 2017-10-17 4:01 ` Junio C Hamano 2017-10-17 7:15 ` Marius Paliga 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-10-17 4:01 UTC (permalink / raw) To: Thais Diniz; +Cc: christian.couder, git, marius.paliga, peff, sbeller 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] patch reply 2017-10-17 4:01 ` Junio C Hamano @ 2017-10-17 7:15 ` Marius Paliga 0 siblings, 0 replies; 24+ messages in thread From: Marius Paliga @ 2017-10-17 7:15 UTC (permalink / raw) To: Junio C Hamano Cc: Thais Diniz, Christian Couder, git, Jeff King, Stefan Beller 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option 2017-10-12 3:26 ` Junio C Hamano 2017-10-17 3:47 ` [PATCH] patch reply Thais Diniz @ 2017-10-17 3:58 ` thais braz 1 sibling, 0 replies; 24+ messages in thread From: thais braz @ 2017-10-17 3:58 UTC (permalink / raw) To: Junio C Hamano Cc: Christian Couder, marius.paliga, git, Stefan Beller, Jeff King 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option 2017-10-04 15:20 ` Marius Paliga 2017-10-11 7:14 ` Marius Paliga 2017-10-11 20:25 ` Thais D. Braz @ 2017-10-11 20:40 ` Thais D. Braz 2 siblings, 0 replies; 24+ messages in thread From: Thais D. Braz @ 2017-10-11 20:40 UTC (permalink / raw) To: marius.paliga; +Cc: git, sbeller --- 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-10-17 7:15 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-03 10:15 Enhancement request: git-push: Allow (configurable) default push-option Marius Paliga 2017-10-03 16:53 ` Stefan Beller 2017-10-04 15:20 ` Marius Paliga 2017-10-11 7:14 ` Marius Paliga 2017-10-11 9:18 ` Marius Paliga 2017-10-11 20:52 ` Stefan Beller 2017-10-11 11:13 ` Junio C Hamano 2017-10-11 13:38 ` Junio C Hamano 2017-10-12 14:59 ` Marius Paliga 2017-10-12 16:32 ` Marius Paliga 2017-10-12 16:51 ` Stefan Beller 2017-10-13 1:39 ` Junio C Hamano 2017-10-13 0:37 ` Junio C Hamano 2017-10-13 8:45 ` Marius Paliga 2017-10-11 20:25 ` Thais D. Braz 2017-10-11 20:25 ` [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option Thais D. Braz 2017-10-12 1:21 ` Junio C Hamano 2017-10-12 2:41 ` Christian Couder 2017-10-12 3:26 ` Junio C Hamano 2017-10-17 3:47 ` [PATCH] patch reply Thais Diniz 2017-10-17 4:01 ` Junio C Hamano 2017-10-17 7:15 ` Marius Paliga 2017-10-17 3:58 ` [PATCH][Outreachy] New git config variable to specify string that will be automatically passed as --push-option thais braz 2017-10-11 20:40 ` Thais D. Braz
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).