Jiang Xin <worldhello.net@xxxxxxxxx> writes:
> +static void report_v2(struct command *commands, const char *unpack_status)
> +{
> + struct command *cmd;
> + struct strbuf buf = STRBUF_INIT;
> + struct ref_push_report_options *options;
> +
> + packet_buf_write(&buf, "unpack %s\n",
> + unpack_status ? unpack_status : "ok");
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + int count = 0;
> +
> + if (!cmd->report.error_message)
> + packet_buf_write(&buf, "ok %s\n",
> + cmd->ref_name);
> + else
> + packet_buf_write(&buf, "ng %s %s\n",
> + cmd->ref_name,
> + cmd->report.error_message);
> + for (options = cmd->report.options; options; options = options->next) {
> + if (count++ > 0)
> + packet_buf_write(&buf, "ok %s\n", cmd->ref_name);
If there was an error, the first one gets "ng" but all others say it
was "ok"? That smells somewhat strange.
> + if (options->ref_name)
> + packet_buf_write(&buf, "option refname %s\n",
> + options->ref_name);
> + if (options->old_oid)
> + packet_buf_write(&buf, "option old-oid %s\n",
> + oid_to_hex(options->old_oid));
> + if (options->new_oid)
> + packet_buf_write(&buf, "option new-oid %s\n",
> + oid_to_hex(options->new_oid));
> + if (options->forced_update)
> + packet_buf_write(&buf, "option forced-update\n");
> + }
> + if (ref->report.error_message)
> + msg = ref->report.error_message;
> if (msg) {
> strbuf_addch(&buf, ' ');
> quote_two_c_style(&buf, "", msg, 0);
> }
> strbuf_addch(&buf, '\n');
>
> + for (options = ref->report.options; options; options = options->next) {
> + if (count++ > 0)
> + strbuf_addf(&buf, "ok %s\n", ref->name);
Would this one ever report a "ng" for an error?
> + if (options->ref_name)
> + strbuf_addf(&buf, "option refname %s\n",
> + options->ref_name);
> + if (options->old_oid)
> + strbuf_addf(&buf, "option old-oid %s\n",
> + oid_to_hex(options->old_oid));
> + if (options->new_oid)
> + strbuf_addf(&buf, "option new-oid %s\n",
> + oid_to_hex(options->new_oid));
> + if (options->forced_update)
> + strbuf_addstr(&buf, "option forced-update\n");
> + }
> write_or_die(1, buf.buf, buf.len);
> }
> strbuf_release(&buf);
> diff --git a/transport-helper.c b/transport-helper.c
> index defafbf4c1..0029ba18bd 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -729,6 +729,49 @@ static int push_update_ref_status(struct strbuf *buf,
> {
> char *refname, *msg;
> int status, forced = 0;
> + static struct ref *hint = NULL;
> + static int new_options = 1;
I hate to see more helper functions turned into pieces that are
impossible to reuse by gaining internal state like this. Does this
function have so many callers that makes it impractical to update
its signature to have the pointer to a "state" structure passed by
the caller? This is called only from push_update_refs_status(), so
you should be able to define a new structure that has the two fields
struct {
struct ref *hint;
int new_option;
};
initialize an instance of it before push_update_refs_status() enters
its endless loop, and pass a pointer to it to this helper function.
> + if (starts_with(buf->buf, "option ")) {
> + struct ref_push_report_options *options;
> + struct object_id old_oid, new_oid;
> + const char *key, *val;
> + char *p;
> +
> + if (!hint)
> + die(_("'option' without a matching 'ok/error' directive"));
> + options = hint->report.options;
> + while (options && options->next)
> + options = options->next;
> + if (new_options) {
> + if (!options) {
> + hint->report.options = xcalloc(1, sizeof(struct ref_push_report_options));
> + options = hint->report.options;
> + } else {
> + options->next = xcalloc(1, sizeof(struct ref_push_report_options));
> + options = options->next;
> + }
> + new_options = 0;
> + }
> -static int print_one_push_status(struct ref *ref, const char *dest, int count,
> - int porcelain, int summary_width)
> +static int _print_one_push_status(struct ref *ref, const char *dest, int count,
> + struct ref_push_report_options *options,
> + int porcelain, int summary_width)
Our naming convention avoids leading underscore (instead we use _1
suffix, when we cannot come up with a better name).
> +static int print_one_push_status(struct ref *ref, const char *dest, int count,
> + int porcelain, int summary_width)
> +{
> + struct ref_push_report_options *options;
> + int n = 0;
> +
> + if (!ref->report.options)
> + return _print_one_push_status(ref, dest, count,
> + NULL, porcelain, summary_width);
> +
> + for (options = ref->report.options; options; options = options->next)
This comment applies to all the patches in this series that adds the
same pattern, but
* "ref->report.options" may be a good name in that it is a (linked)
list of options;
* The variable that is used to iterate over the list points at one
of the elements on the list at any time, and is better called
'option' without 's' at the end.
> + _print_one_push_status(ref, dest, count + n++,
> + options, porcelain, summary_width);
> + return n;
> +}
> +
> static int measure_abbrev(const struct object_id *oid, int sofar)
> {
> char hex[GIT_MAX_HEXSZ + 1];
