Re: [PATCHv7 0/4] Push options

2016-07-14 Thread Junio C Hamano
Stefan Beller  writes:

> Jeff,
>
> here is the more idiomatic way.
>
> Thanks,
> Stefan

Looks good to me.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCHv7 0/4] Push options

2016-07-14 Thread Stefan Beller
Jeff,

here is the more idiomatic way.

Thanks,
Stefan

diff to v6:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9bb9afc..3c9360a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1499,11 +1499,8 @@ static struct command *read_head_info(struct sha1_array 
*shallow)
return commands;
 }
 
-static struct string_list *read_push_options(void)
+static void read_push_options(struct string_list *options)
 {
-   struct string_list *ret = xmalloc(sizeof(*ret));
-   string_list_init(ret, 1);
-
while (1) {
char *line;
int len;
@@ -1513,10 +1510,8 @@ static struct string_list *read_push_options(void)
if (!line)
break;
 
-   string_list_append(ret, line);
+   string_list_append(options, line);
}
-
-   return ret;
 }
 
 static const char *parse_pack_header(struct pack_header *hdr)
@@ -1804,10 +1799,10 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
 
if ((commands = read_head_info(&shallow)) != NULL) {
const char *unpack_status = NULL;
-   struct string_list *push_options = NULL;
+   struct string_list push_options = STRING_LIST_INIT_DUP;
 
if (use_push_options)
-   push_options = read_push_options();
+   read_push_options(&push_options);
 
prepare_shallow_info(&si, &shallow);
if (!si.nr_ours && !si.nr_theirs)
@@ -1817,18 +1812,16 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
update_shallow_info(commands, &si, &ref);
}
execute_commands(commands, unpack_status, &si,
-push_options);
+&push_options);
if (pack_lockfile)
unlink_or_warn(pack_lockfile);
if (report_status)
report(commands, unpack_status);
run_receive_hook(commands, "post-receive", 1,
-push_options);
+&push_options);
run_update_post_hook(commands);
-   if (push_options) {
-   string_list_clear(push_options, 0);
-   free(push_options);
-   }
+   if (push_options.nr)
+   string_list_clear(&push_options, 0);
if (auto_gc) {
const char *argv_gc_auto[] = {
"gc", "--auto", "--quiet", NULL,

v6 consisted of 2/4 only:

 Junio,
 please replace v5 2/4 with this patch (I only resend this single patch
 as the other 3 remain as is).

 This only changes read_push_options to not care at all about any
 limitations.
 
 Thanks,
 Stefan

# interdiff to v5:
# diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
# index 917ac18..9bb9afc 100644
# --- a/builtin/receive-pack.c
# +++ b/builtin/receive-pack.c
# @@ -1505,7 +1505,7 @@ static struct string_list *read_push_options(void)
#   string_list_init(ret, 1);
#  
#   while (1) {
# - char *line, *lf;
# + char *line;
#   int len;
#  
#   line = packet_read_line(0, &len);
# @@ -1513,30 +1513,6 @@ static struct string_list *read_push_options(void)
#   if (!line)
#   break;
#  
# - /*
# - * NEEDSWORK: expose the limitations to be configurable;
# - * Once the limit can be lifted, include a way for payloads
# - * larger than one pkt, e.g use last byte to indicate if
# - * the push option continues in the next packet or implement
# - * larger packets.
# - */
# - if (len > LARGE_PACKET_MAX - 1) {
# - /*
# -  * NEEDSWORK: The error message in die(..) is not
# -  * transmitted in call cases, so ideally all die(..)
# -  * calls are prefixed with rp_error and then we can
# -  * combine rp_error && die into one helper function.
# -  */
# - rp_error("protocol error: server configuration allows 
push "
# -  "options of size up to %d bytes",
# -  LARGE_PACKET_MAX - 1);
# - die("protocol error: push options too large");
# - }
# -
# - lf = strchr(line, '\n');
# - if (lf)
# - *lf = '\0';
# -
#   string_list_append(ret, line);
#   }

v5:

Jeff wrote:
> Junio wrote:
>> I think those extra knobs can come later.  If we are not going to
>> limit with max_options in the end, however, wouldn't it be more
>> natural for the initial iteration without any configuration not to
>> have