On Tue, Jun 11, 2019 at 05:37:16PM -0700, Matthew DeVore wrote:
> On Tue, Jun 11, 2019 at 02:48:51PM -0700, Junio C Hamano wrote:
> > Matthew DeVore <matvore@xxxxxxxxxxx> writes:
> >
> > >> It is brittle because callers are bound to forget doing "if
> > >> (!x->buf.buf) lazy_init(&x->buf)" at some point, and blindly use an
> > >> uninitialized x->buf. Making sure x->buf is always initialized
> > >
> > > A corallary proposition would be to make this particular strbuf a "struct
> > > strbuf *" rather than an inline strbuf. It should then be rather clear to users
> > > that it may be null.
> >
> > Would make it less likely for uses of an uninitialized strbuf to be
> > left undetected as errors? I guess so, and if that is the case it
> > would definitely be an improvement.
> >
> > But initializing the strbuf at the point where the enclosing
> > structure is initialized (or calloc()'ed) is also a vaiable option,
> > and between the two, I think that would be even more robust.
> >
> > There may be reasons why it is cumbersome to arrange it that way,
> > though (e.g. if the code does not introduce a "new_stuff()"
> > allocator that also initializes, and instead uses xcalloc() from
> > many places, initializing the enclosing structure properly might
> > take a preliminary clean-up step before the main part of the patch
> > series can begin).
Here is an alternate interdiff where I use a string_list rather than a strbuf
for the filter_spec. This is actually slightly shorter code than the earlier
interdiff using a struct filter_spec type. (not by much, maybe half-dozen lines)
I think this is my favorite approach so far.
diff --git a/builtin/clone.c b/builtin/clone.c
index 85b0d3155d..81e6010779 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1135,27 +1135,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (option_upload_pack)
transport_set_option(transport, TRANS_OPT_UPLOADPACK,
option_upload_pack);
if (server_options.nr)
transport->server_options = &server_options;
if (filter_options.choice) {
- struct strbuf expanded_filter_spec = STRBUF_INIT;
- expand_list_objects_filter_spec(&filter_options,
- &expanded_filter_spec);
+ const char *spec =
+ expand_list_objects_filter_spec(&filter_options);
transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
- expanded_filter_spec.buf);
+ spec);
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
- strbuf_release(&expanded_filter_spec);
}
if (transport->smart_options && !deepen && !filter_options.choice)
transport->smart_options->check_self_contained_and_connected = 1;
argv_array_push(&ref_prefixes, "HEAD");
refspec_ref_prefixes(&remote->fetch, &ref_prefixes);
if (option_branch)
expand_ref_prefix(&ref_prefixes, option_branch);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4ba63d5ac6..dee89e1a19 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1181,27 +1181,24 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
if (deepen && deepen_since)
set_option(transport, TRANS_OPT_DEEPEN_SINCE, deepen_since);
if (deepen && deepen_not.nr)
set_option(transport, TRANS_OPT_DEEPEN_NOT,
(const char *)&deepen_not);
if (deepen_relative)
set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
if (update_shallow)
set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
if (filter_options.choice) {
- struct strbuf expanded_filter_spec = STRBUF_INIT;
- expand_list_objects_filter_spec(&filter_options,
- &expanded_filter_spec);
- set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
- expanded_filter_spec.buf);
+ const char *spec =
+ expand_list_objects_filter_spec(&filter_options);
+ set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, spec);
set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
- strbuf_release(&expanded_filter_spec);
}
if (negotiation_tip.nr) {
if (transport->smart_options)
add_negotiation_tips(transport->smart_options);
else
warning("Ignoring --negotiation-tip because the protocol does not support it.");
}
return transport;
}
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7137f13a74..823e87c1c9 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -459,22 +459,24 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
show_progress = arg;
continue;
}
if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
parse_list_objects_filter(&filter_options, arg);
if (filter_options.choice && !revs.blob_objects)
die(_("object filtering requires --objects"));
if (filter_options.choice == LOFC_SPARSE_OID &&
!filter_options.sparse_oid_value)
- die(_("invalid sparse value '%s'"),
- filter_options.filter_spec.buf);
+ die(
+ _("invalid sparse value '%s'"),
+ list_objects_filter_spec(
+ &filter_options));
continue;
}
if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {
list_objects_filter_set_no_filter(&filter_options);
continue;
}
if (!strcmp(arg, "--filter-print-omitted")) {
arg_print_omitted = 1;
continue;
}
diff --git a/fetch-pack.c b/fetch-pack.c
index 1c10f54e78..72e13b0a1d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -332,26 +332,23 @@ static int find_common(struct fetch_negotiator *negotiator,
packet_buf_write(&req_buf, "deepen-since %"PRItime, max_age);
}
if (args->deepen_not) {
int i;
for (i = 0; i < args->deepen_not->nr; i++) {
struct string_list_item *s = args->deepen_not->items + i;
packet_buf_write(&req_buf, "deepen-not %s", s->string);
}
}
if (server_supports_filtering && args->filter_options.choice) {
- struct strbuf expanded_filter_spec = STRBUF_INIT;
- expand_list_objects_filter_spec(&args->filter_options,
- &expanded_filter_spec);
- packet_buf_write(&req_buf, "filter %s",
- expanded_filter_spec.buf);
- strbuf_release(&expanded_filter_spec);
+ const char *spec =
+ expand_list_objects_filter_spec(&args->filter_options);
+ packet_buf_write(&req_buf, "filter %s", spec);
}
packet_buf_flush(&req_buf);
state_len = req_buf.len;
if (args->deepen) {
const char *arg;
struct object_id oid;
send_request(args, fd[1], &req_buf);
while (packet_reader_read(&reader) == PACKET_READ_NORMAL) {
@@ -1092,21 +1089,21 @@ static int add_haves(struct fetch_negotiator *negotiator,
ret = 1;
}
/* Increase haves to send on next round */
*haves_to_send = next_flush(1, *haves_to_send);
return ret;
}
static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
- const struct fetch_pack_args *args,
+ struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
int *haves_to_send, int *in_vain,
int sideband_all)
{
int ret = 0;
struct strbuf req_buf = STRBUF_INIT;
if (server_supports_v2("fetch", 1))
packet_buf_write(&req_buf, "command=fetch");
if (server_supports_v2("agent", 0))
@@ -1133,27 +1130,24 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
/* Add shallow-info and deepen request */
if (server_supports_feature("fetch", "shallow", 0))
add_shallow_requests(&req_buf, args);
else if (is_repository_shallow(the_repository) || args->deepen)
die(_("Server does not support shallow requests"));
/* Add filter */
if (server_supports_feature("fetch", "filter", 0) &&
args->filter_options.choice) {
- struct strbuf expanded_filter_spec = STRBUF_INIT;
+ const char *spec =
+ expand_list_objects_filter_spec(&args->filter_options);
print_verbose(args, _("Server supports filter"));
- expand_list_objects_filter_spec(&args->filter_options,
- &expanded_filter_spec);
- packet_buf_write(&req_buf, "filter %s",
- expanded_filter_spec.buf);
- strbuf_release(&expanded_filter_spec);
+ packet_buf_write(&req_buf, "filter %s", spec);
} else if (args->filter_options.choice) {
warning("filtering not recognized by server, ignoring");
}
/* add wants */
add_wants(args->no_dependents, wants, &req_buf);
if (args->no_dependents) {
packet_buf_write(&req_buf, "done");
ret = 1;
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 9a5677c2c8..38729a7238 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -174,20 +174,29 @@ static int parse_combine_filter(
return result;
}
static int allow_unencoded(char ch)
{
if (ch <= ' ' || ch == '%' || ch == '+')
return 0;
return !strchr(RESERVED_NON_WS, ch);
}
+static void filter_spec_append_urlencode(
+ struct list_objects_filter_options *filter, const char *raw)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addstr_urlencode(&buf, raw, allow_unencoded);
+ trace_printf("Added to composite filter-spec: %s\n", buf.buf);
+ string_list_append(&filter->filter_spec, strbuf_detach(&buf, NULL));
+}
+
/*
* Changes filter_options into an equivalent LOFC_COMBINE filter options
* instance. Does not do anything if filter_options is already LOFC_COMBINE.
*/
static void transform_to_combine_type(
struct list_objects_filter_options *filter_options)
{
assert(filter_options->choice);
if (filter_options->choice == LOFC_COMBINE)
return;
@@ -195,64 +204,59 @@ static void transform_to_combine_type(
const int initial_sub_alloc = 2;
struct list_objects_filter_options *sub_array =
xcalloc(initial_sub_alloc, sizeof(*sub_array));
sub_array[0] = *filter_options;
memset(filter_options, 0, sizeof(*filter_options));
filter_options->sub = sub_array;
filter_options->sub_alloc = initial_sub_alloc;
}
filter_options->sub_nr = 1;
filter_options->choice = LOFC_COMBINE;
- strbuf_init(&filter_options->filter_spec, 0);
- strbuf_addstr(&filter_options->filter_spec, "combine:");
- strbuf_addstr_urlencode(&filter_options->filter_spec,
- filter_options->sub[0].filter_spec.buf,
- allow_unencoded);
+ string_list_append(&filter_options->filter_spec, xstrdup("combine:"));
+ filter_spec_append_urlencode(
+ filter_options,
+ list_objects_filter_spec(&filter_options->sub[0]));
/*
* We don't need the filter_spec strings for subfilter specs, only the
* top level.
*/
- strbuf_release(&filter_options->sub[0].filter_spec);
+ string_list_clear(&filter_options->sub[0].filter_spec, /*free_util=*/0);
}
void list_objects_filter_die_if_populated(
struct list_objects_filter_options *filter_options)
{
if (filter_options->choice)
die(_("multiple filter-specs cannot be combined"));
}
void parse_list_objects_filter(
struct list_objects_filter_options *filter_options,
const char *arg)
{
struct strbuf errbuf = STRBUF_INIT;
int parse_error;
if (!filter_options->choice) {
- strbuf_init(&filter_options->filter_spec, 0);
- strbuf_addstr(&filter_options->filter_spec, arg);
+ string_list_append(&filter_options->filter_spec, xstrdup(arg));
parse_error = gently_parse_list_objects_filter(
filter_options, arg, &errbuf);
} else {
/*
* Make filter_options an LOFC_COMBINE spec so we can trivially
* add subspecs to it.
*/
transform_to_combine_type(filter_options);
- strbuf_addstr(&filter_options->filter_spec, "+");
- strbuf_addstr_urlencode(&filter_options->filter_spec, arg,
- allow_unencoded);
- trace_printf("Generated composite filter-spec: %s\n",
- filter_options->filter_spec.buf);
+ string_list_append(&filter_options->filter_spec, xstrdup("+"));
+ filter_spec_append_urlencode(filter_options, arg);
ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
filter_options->sub_alloc);
parse_error = gently_parse_list_objects_filter(
&filter_options->sub[filter_options->sub_nr - 1], arg,
&errbuf);
}
if (parse_error)
die("%s", errbuf.buf);
}
@@ -262,54 +266,71 @@ int opt_parse_list_objects_filter(const struct option *opt,
{
struct list_objects_filter_options *filter_options = opt->value;
if (unset || !arg)
list_objects_filter_set_no_filter(filter_options);
else
parse_list_objects_filter(filter_options, arg);
return 0;
}
-void expand_list_objects_filter_spec(
- const struct list_objects_filter_options *filter,
- struct strbuf *expanded_spec)
+const char *list_objects_filter_spec(struct list_objects_filter_options *filter)
+{
+ if (!filter->filter_spec.nr)
+ BUG("no filter_spec available for this filter");
+ if (filter->filter_spec.nr != 1) {
+ struct strbuf concatted = STRBUF_INIT;
+ strbuf_add_separated_string_list(
+ &concatted, "", &filter->filter_spec);
+ string_list_clear(&filter->filter_spec, /*free_util=*/0);
+ string_list_append(
+ &filter->filter_spec, strbuf_detach(&concatted, NULL));
+ }
+
+ return filter->filter_spec.items[0].string;
+}
+
+const char *expand_list_objects_filter_spec(
+ struct list_objects_filter_options *filter)
{
- strbuf_init(expanded_spec, 0);
- if (filter->choice == LOFC_BLOB_LIMIT)
- strbuf_addf(expanded_spec, "blob:limit=%lu",
+ if (filter->choice == LOFC_BLOB_LIMIT) {
+ struct strbuf expanded_spec;
+ strbuf_addf(&expanded_spec, "blob:limit=%lu",
filter->blob_limit_value);
- else if (filter->choice == LOFC_TREE_DEPTH)
- strbuf_addf(expanded_spec, "tree:%lu",
- filter->tree_exclude_depth);
- else
- strbuf_addstr(expanded_spec, filter->filter_spec.buf);
+ string_list_clear(&filter->filter_spec, /*free_util=*/0);
+ string_list_append(
+ &filter->filter_spec,
+ strbuf_detach(&expanded_spec, NULL));
+ }
+
+ return list_objects_filter_spec(filter);
}
void list_objects_filter_release(
struct list_objects_filter_options *filter_options)
{
size_t sub;
if (!filter_options)
return;
- strbuf_release(&filter_options->filter_spec);
+ string_list_clear(&filter_options->filter_spec, /*free_util=*/0);
free(filter_options->sparse_oid_value);
free(filter_options->sparse_path_value);
for (sub = 0; sub < filter_options->sub_nr; sub++)
list_objects_filter_release(&filter_options->sub[sub]);
free(filter_options->sub);
memset(filter_options, 0, sizeof(*filter_options));
}
void partial_clone_register(
const char *remote,
- const struct list_objects_filter_options *filter_options)
+ struct list_objects_filter_options *filter_options)
{
/*
* Record the name of the partial clone remote in the
* config and in the global variable -- the latter is
* used throughout to indicate that partial clone is
* enabled and to expect missing objects.
*/
if (repository_format_partial_clone &&
*repository_format_partial_clone &&
strcmp(remote, repository_format_partial_clone))
@@ -318,35 +339,33 @@ void partial_clone_register(
git_config_set("core.repositoryformatversion", "1");
git_config_set("extensions.partialclone", remote);
repository_format_partial_clone = xstrdup(remote);
/*
* Record the initial filter-spec in the config as
* the default for subsequent fetches from this remote.
*/
core_partial_clone_filter_default =
- xstrdup(filter_options->filter_spec.buf);
+ xstrdup(expand_list_objects_filter_spec(filter_options));
git_config_set("core.partialclonefilter",
core_partial_clone_filter_default);
}
void partial_clone_get_default_filter_spec(
struct list_objects_filter_options *filter_options)
{
struct strbuf errbuf = STRBUF_INIT;
/*
* Parse default value, but silently ignore it if it is invalid.
*/
if (!core_partial_clone_filter_default)
return;
- if (!filter_options->filter_spec.buf)
- strbuf_init(&filter_options->filter_spec, 0);
- strbuf_addstr(&filter_options->filter_spec,
- core_partial_clone_filter_default);
+ string_list_append(&filter_options->filter_spec,
+ core_partial_clone_filter_default);
gently_parse_list_objects_filter(filter_options,
core_partial_clone_filter_default,
&errbuf);
strbuf_release(&errbuf);
}
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 2c0ce6383a..9b31048ada 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -1,15 +1,15 @@
#ifndef LIST_OBJECTS_FILTER_OPTIONS_H
#define LIST_OBJECTS_FILTER_OPTIONS_H
#include "parse-options.h"
-#include "strbuf.h"
+#include "string-list.h"
/*
* The list of defined filters for list-objects.
*/
enum list_objects_filter_choice {
LOFC_DISABLED = 0,
LOFC_BLOB_NONE,
LOFC_BLOB_LIMIT,
LOFC_TREE_DEPTH,
LOFC_SPARSE_OID,
@@ -18,22 +18,24 @@ enum list_objects_filter_choice {
LOFC__COUNT /* must be last */
};
struct list_objects_filter_options {
/*
* 'filter_spec' is the raw argument value given on the command line
* or protocol request. (The part after the "--keyword=".) For
* commands that launch filtering sub-processes, or for communication
* over the network, don't use this value; use the result of
* expand_list_objects_filter_spec() instead.
+ * To get the raw filter spec given by the user, use the result of
+ * list_objects_filter_spec().
*/
- struct strbuf filter_spec;
+ struct string_list filter_spec;
/*
* 'choice' is determined by parsing the filter-spec. This indicates
* the filtering algorithm to use.
*/
enum list_objects_filter_choice choice;
/*
* Choice is LOFC_DISABLED because "--no-filter" was requested.
*/
@@ -86,31 +88,33 @@ int opt_parse_list_objects_filter(const struct option *opt,
N_("object filtering"), 0, \
opt_parse_list_objects_filter }
/*
* Translates abbreviated numbers in the filter's filter_spec into their
* fully-expanded forms (e.g., "limit:blob=1k" becomes "limit:blob=1024").
*
* This form should be used instead of the raw filter_spec field when
* communicating with a remote process or subprocess.
*/
-void expand_list_objects_filter_spec(
- const struct list_objects_filter_options *filter,
- struct strbuf *expanded_spec);
+const char *expand_list_objects_filter_spec(
+ struct list_objects_filter_options *filter);
+
+const char *list_objects_filter_spec(
+ struct list_objects_filter_options *filter);
void list_objects_filter_release(
struct list_objects_filter_options *filter_options);
static inline void list_objects_filter_set_no_filter(
struct list_objects_filter_options *filter_options)
{
list_objects_filter_release(filter_options);
filter_options->no_filter = 1;
}
void partial_clone_register(
const char *remote,
- const struct list_objects_filter_options *filter_options);
+ struct list_objects_filter_options *filter_options);
void partial_clone_get_default_filter_spec(
struct list_objects_filter_options *filter_options);
#endif /* LIST_OBJECTS_FILTER_OPTIONS_H */
diff --git a/transport-helper.c b/transport-helper.c
index cec83bd663..d6313ef9f5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -675,27 +675,23 @@ static int fetch(struct transport *transport,
data->transport_options.check_self_contained_and_connected)
set_helper_option(transport, "check-connectivity", "true");
if (transport->cloning)
set_helper_option(transport, "cloning", "true");
if (data->transport_options.update_shallow)
set_helper_option(transport, "update-shallow", "true");
if (data->transport_options.filter_options.choice) {
- struct strbuf expanded_filter_spec = STRBUF_INIT;
- expand_list_objects_filter_spec(
- &data->transport_options.filter_options,
- &expanded_filter_spec);
- set_helper_option(transport, "filter",
- expanded_filter_spec.buf);
- strbuf_release(&expanded_filter_spec);
+ const char *spec = expand_list_objects_filter_spec(
+ &data->transport_options.filter_options);
+ set_helper_option(transport, "filter", spec);
}
if (data->transport_options.negotiation_tips)
warning("Ignoring --negotiation-tip because the protocol does not support it.");
if (data->fetch)
return fetch_with_fetch(transport, nr_heads, to_fetch);
if (data->import)
return fetch_with_import(transport, nr_heads, to_fetch);
diff --git a/upload-pack.c b/upload-pack.c
index ba8c3a1f8e..dda2ac6f44 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -133,32 +133,31 @@ static void create_pack_file(const struct object_array *have_obj,
argv_array_push(&pack_objects.args, "--stdout");
if (shallow_nr)
argv_array_push(&pack_objects.args, "--shallow");
if (!no_progress)
argv_array_push(&pack_objects.args, "--progress");
if (use_ofs_delta)
argv_array_push(&pack_objects.args, "--delta-base-offset");
if (use_include_tag)
argv_array_push(&pack_objects.args, "--include-tag");
- if (filter_options.filter_spec.len) {
- struct strbuf expanded_filter_spec = STRBUF_INIT;
- expand_list_objects_filter_spec(&filter_options,
- &expanded_filter_spec);
+ if (filter_options.choice) {
+ const char *spec =
+ expand_list_objects_filter_spec(&filter_options);
if (pack_objects.use_shell) {
struct strbuf buf = STRBUF_INIT;
- sq_quote_buf(&buf, expanded_filter_spec.buf);
+ sq_quote_buf(&buf, spec);
argv_array_pushf(&pack_objects.args, "--filter=%s", buf.buf);
strbuf_release(&buf);
} else {
argv_array_pushf(&pack_objects.args, "--filter=%s",
- expanded_filter_spec.buf);
+ spec);
}
}
pack_objects.in = -1;
pack_objects.out = -1;
pack_objects.err = -1;
if (start_command(&pack_objects))
die("git upload-pack: unable to fork git-pack-objects");
