From: Marius Paliga <marius.paliga@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Stefan Beller <sbeller@google.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
christian.couder@gmail.com, peff@peff.net,
thais.dinizbraz@gmail.com
Subject: Re: Enhancement request: git-push: Allow (configurable) default push-option
Date: Thu, 12 Oct 2017 18:32:09 +0200 [thread overview]
Message-ID: <CAK7vU=0A_nf2bAgV6dQyiesLJ3HHs5guyfNSNTwzYzgS2+YeWg@mail.gmail.com> (raw)
In-Reply-To: <CAK7vU=0wbGsFCXmwmCc-XX9K07UF_OZ7tFa4_GVb-H7fxakssg@mail.gmail.com>
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 ;-)
>>
next prev parent reply other threads:[~2017-10-12 16:32 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAK7vU=0A_nf2bAgV6dQyiesLJ3HHs5guyfNSNTwzYzgS2+YeWg@mail.gmail.com' \
--to=marius.paliga@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=sbeller@google.com \
--cc=thais.dinizbraz@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).