git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Marius Paliga <marius.paliga@gmail.com>
Cc: Stefan Beller <sbeller@google.com>,
	"git\@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: Enhancement request: git-push: Allow (configurable) default push-option
Date: Wed, 11 Oct 2017 22:38:21 +0900
Message-ID: <xmqqh8v5ls8i.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAK7vU=22W9mYdSnw_LP2uWYyKZuTzF0JgTVWCX+nMhUnLjQ_Cw@mail.gmail.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 ;-)


  parent reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-03 10:15 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 [this message]
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

Reply instructions:

You may reply publically 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=xmqqh8v5ls8i.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=marius.paliga@gmail.com \
    --cc=sbeller@google.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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox