{"thread":{"id":"37826","subject":"[PATCH] use child_process_init() to initialize struct child_process variables","startedAt":"2014-10-28T20:52:34Z","lastAt":"2014-11-10T07:14:10Z","messageCount":25,"participants":["René Scharfe","mike.gorchak.qnx@gmail.com","Jeff King","Junio C Hamano","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251157","messageId":"54500212.7040603@web.de","threadId":"37826","inReplyTo":null,"subject":"[PATCH] use child_process_init() to initialize struct child_process variables","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-28T20:52:34Z","receivedAt":"2014-10-28T20:52:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call child_process_init() instead of zeroing the memory of variables of\ntype struct child_process by hand before use because the former is both\nclearer and shorter.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n bundle.c           | 2 +-\n column.c           | 2 +-\n trailer.c          | 2 +-\n transport-helper.c | 2 +-\n 4 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex fa67057..c846092 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \twrite_or_die(bundle_fd, \"\\n\", 1);\n \n \t/* write pack */\n-\tmemset(&rls, 0, sizeof(rls));\n+\tchild_process_init(&rls);\n \targv_array_pushl(&rls.args,\n \t\t\t \"pack-objects\", \"--all-progress-implied\",\n \t\t\t \"--stdout\", \"--thin\", \"--delta-base-offset\",\ndiff --git a/column.c b/column.c\nindex 8082a94..786abe6 100644\n--- a/column.c\n+++ b/column.c\n@@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts)\n \tif (fd_out != -1)\n \t\treturn -1;\n \n-\tmemset(&column_process, 0, sizeof(column_process));\n+\tchild_process_init(&column_process);\n \targv = &column_process.args;\n \n \targv_array_push(argv, \"column\");\ndiff --git a/trailer.c b/trailer.c\nindex 8514566..7ff036c 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)\n \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n \n \targv[0] = cmd.buf;\n-\tmemset(&cp, 0, sizeof(cp));\n+\tchild_process_init(&cp);\n \tcp.argv = argv;\n \tcp.env = local_repo_env;\n \tcp.no_stdin = 1;\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6cd9dd1..0224687 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -414,7 +414,7 @@ static int get_exporter(struct transport *transport,\n \tstruct child_process *helper = get_helper(transport);\n \tint i;\n \n-\tmemset(fastexport, 0, sizeof(*fastexport));\n+\tchild_process_init(fastexport);\n \n \t/* we need to duplicate helper->in because we want to use it after\n \t * fastexport is done with it. */\n-- \n2.1.2\n"},{"id":"251165","messageId":"20141028215856.6643859.60752.16778@gmail.com","threadId":"37826","inReplyTo":"54500212.7040603@web.de","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"","fromEmail":"mike.gorchak.qnx@gmail.com","sentAt":"2014-10-28T21:58:56Z","receivedAt":"2014-10-28T21:58:56Z","isPatch":true,"sender":{"key":"mike.gorchak.qnx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1337711?v=4"},"body":"\n\nSent from my BlackBerry 10 smartphone on the Rogers network.\n  Original Message  \nFrom: René Scharfe\nSent: Tuesday, October 28, 2014 16:59\nTo: Git Mailing List\nCc: Junio C Hamano\nSubject: [PATCH] use child_process_init() to initialize struct child_process variables\n\nCall child_process_init() instead of zeroing the memory of variables of\ntype struct child_process by hand before use because the former is both\nclearer and shorter.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nbundle.c | 2 +-\ncolumn.c | 2 +-\ntrailer.c | 2 +-\ntransport-helper.c | 2 +-\n4 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex fa67057..c846092 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,\nwrite_or_die(bundle_fd, \"\\n\", 1);\n\n/* write pack */\n-\tmemset(&rls, 0, sizeof(rls));\n+\tchild_process_init(&rls);\nargv_array_pushl(&rls.args,\n\"pack-objects\", \"--all-progress-implied\",\n\"--stdout\", \"--thin\", \"--delta-base-offset\",\ndiff --git a/column.c b/column.c\nindex 8082a94..786abe6 100644\n--- a/column.c\n+++ b/column.c\n@@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts)\nif (fd_out != -1)\nreturn -1;\n\n-\tmemset(&column_process, 0, sizeof(column_process));\n+\tchild_process_init(&column_process);\nargv = &column_process.args;\n\nargv_array_push(argv, \"column\");\ndiff --git a/trailer.c b/trailer.c\nindex 8514566..7ff036c 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)\nstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n\nargv[0] = cmd.buf;\n-\tmemset(&cp, 0, sizeof(cp));\n+\tchild_process_init(&cp);\ncp.argv = argv;\ncp.env = local_repo_env;\ncp.no_stdin = 1;\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6cd9dd1..0224687 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -414,7 +414,7 @@ static int get_exporter(struct transport *transport,\nstruct child_process *helper = get_helper(transport);\nint i;\n\n-\tmemset(fastexport, 0, sizeof(*fastexport));\n+\tchild_process_init(fastexport);\n\n/* we need to duplicate helper->in because we want to use it after\n* fastexport is done with it. */\n-- \n2.1.2\n\n"},{"id":"251180","messageId":"20141029172109.GA32234@peff.net","threadId":"37826","inReplyTo":"54500212.7040603@web.de","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-29T17:21:09Z","receivedAt":"2014-10-29T17:21:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:\n\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \twrite_or_die(bundle_fd, \"\\n\", 1);\n>  \n>  \t/* write pack */\n> -\tmemset(&rls, 0, sizeof(rls));\n> +\tchild_process_init(&rls);\n>  \targv_array_pushl(&rls.args,\n>  \t\t\t \"pack-objects\", \"--all-progress-implied\",\n>  \t\t\t \"--stdout\", \"--thin\", \"--delta-base-offset\",\n\nI wondered if this one could use CHILD_PROCESS_INIT in the declaration\ninstead. And indeed, we _do_ use CHILD_PROCESS_INIT there, but we use\nthe same variable twice for two different child processes in the same\nfunction. Besides variable reuse being slightly confusing, the name\n\"rls\" (which presumably stands for \"rev-list\" for the first child) means\nnothing here, where we are calling \"pack-objects\". Maybe it would be\ncleaner to introduce a second variable?\n\nI also suspect the function would be a lot more readable broken into two\nsub-functions (reading from rev-list and writing to pack-objects), but I\ndid not look closely enough to see whether there were any complicating\nfactors.\n\n> diff --git a/column.c b/column.c\n> index 8082a94..786abe6 100644\n> --- a/column.c\n> +++ b/column.c\n> @@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts)\n>  \tif (fd_out != -1)\n>  \t\treturn -1;\n>  \n> -\tmemset(&column_process, 0, sizeof(column_process));\n> +\tchild_process_init(&column_process);\n>  \targv = &column_process.args;\n>  \n>  \targv_array_push(argv, \"column\");\n\nThis one uses a static child_process which needs to be reinitialized on\neach run of the function. So it definitely needs child_process_init.\n\n> diff --git a/trailer.c b/trailer.c\n> index 8514566..7ff036c 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)\n>  \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n>  \n>  \targv[0] = cmd.buf;\n> -\tmemset(&cp, 0, sizeof(cp));\n> +\tchild_process_init(&cp);\n>  \tcp.argv = argv;\n>  \tcp.env = local_repo_env;\n>  \tcp.no_stdin = 1;\n\nI think this one can use CHILD_PROCESS_INIT in the declaration. I guess\nit is debatable whether that is actually preferable, but I tend to think\nit is cleaner and less error-prone.\n\n-Peff\n"},{"id":"251187","messageId":"xmqqlhnyy9e2.fsf@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"20141029172109.GA32234@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-29T19:16:05Z","receivedAt":"2014-10-29T19:16:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:\n>\n>> --- a/bundle.c\n>> +++ b/bundle.c\n>> @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n>>  \twrite_or_die(bundle_fd, \"\\n\", 1);\n>>  \n>>  \t/* write pack */\n>> -\tmemset(&rls, 0, sizeof(rls));\n>> +\tchild_process_init(&rls);\n>>  \targv_array_pushl(&rls.args,\n>>  \t\t\t \"pack-objects\", \"--all-progress-implied\",\n>>  \t\t\t \"--stdout\", \"--thin\", \"--delta-base-offset\",\n>\n> I wondered if this one could use CHILD_PROCESS_INIT in the declaration\n> instead. And indeed, we _do_ use CHILD_PROCESS_INIT there, but we use\n> the same variable twice for two different child processes in the same\n> function. Besides variable reuse being slightly confusing, the name\n> \"rls\" (which presumably stands for \"rev-list\" for the first child) means\n> nothing here, where we are calling \"pack-objects\". Maybe it would be\n> cleaner to introduce a second variable?\n\nIt has been this way since day one at b1daf300 (Replace\nfork_with_pipe in bundle with run_command, 2007-03-12); I agree that\ntwo variables might make things less confusing.\n\n> I also suspect the function would be a lot more readable broken into two\n> sub-functions (reading from rev-list and writing to pack-objects), but I\n> did not look closely enough to see whether there were any complicating\n> factors.\n\nProbably three helper functions:\n\n - The first is to find tops and bottoms (this translates fuzzy\n   specifications such as \"--since 30.days\" into a more concrete\n   revision range \"^A ^B ... Z\" to establish bundle prerequisites),\n   which is done by running a \"rev-list --boundary\".\n\n - The second is to show refs, while paying attention to things like\n   \"--10 maint master\" which may result in the tip of 'maint' not\n   being shown at all.  I am not sure if this part can/should take\n   advantage of revs.cmdline, though.\n\n - The last is to create the actual pack data.\n\nI agree with your analysis on the change in column.c and trailer.c\n\nThanks.\n"},{"id":"251226","messageId":"xmqqlhnxwhw4.fsf@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"xmqqlhnyy9e2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-30T18:07:39Z","receivedAt":"2014-10-30T18:07:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Probably three helper functions:\n>\n>  - The first is to find tops and bottoms (this translates fuzzy\n>    specifications such as \"--since 30.days\" into a more concrete\n>    revision range \"^A ^B ... Z\" to establish bundle prerequisites),\n>    which is done by running a \"rev-list --boundary\".\n>\n>  - The second is to show refs, while paying attention to things like\n>    \"--10 maint master\" which may result in the tip of 'maint' not\n>    being shown at all.  I am not sure if this part can/should take\n>    advantage of revs.cmdline, though.\n>\n>  - The last is to create the actual pack data.\n>\n> I agree with your analysis on the change in column.c and trailer.c\n>\n> Thanks.\n\nSo here are a few patches on top of René's change.  This is the\nthird point in the above list.\n\n-- >8 --\nSubject: [PATCH] bundle: split out a helper function to create a pack data\n\nThe create_bundle() function, while it does one single logical thing\nand tries to do it well, that single logical thing takes a rather\nlarge implementation.\n\nLet's start separating what it does into smaller steps to make it\neasier what is going on.  This is a first step to separate out the\nactual pack-data generation, after the earlier part of the function\nfigures out which part of the history to place in the bundle.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n bundle.c | 64 +++++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 37 insertions(+), 27 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex c846092..9c87532 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -235,6 +235,41 @@ out:\n \treturn result;\n }\n \n+static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs)\n+{\n+\tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n+\tint i;\n+\n+\targv_array_pushl(&pack_objects.args,\n+\t\t\t \"pack-objects\", \"--all-progress-implied\",\n+\t\t\t \"--stdout\", \"--thin\", \"--delta-base-offset\",\n+\t\t\t NULL);\n+\tpack_objects.in = -1;\n+\tpack_objects.out = bundle_fd;\n+\tpack_objects.git_cmd = 1;\n+\tif (start_command(&pack_objects))\n+\t\treturn error(_(\"Could not spawn pack-objects\"));\n+\n+\t/*\n+\t * start_command closed bundle_fd if it was > 1\n+\t * so set the lock fd to -1 so commit_lock_file()\n+\t * won't fail trying to close it.\n+\t */\n+\tlock->fd = -1;\n+\n+\tfor (i = 0; i < revs->pending.nr; i++) {\n+\t\tstruct object *object = revs->pending.objects[i].item;\n+\t\tif (object->flags & UNINTERESTING)\n+\t\t\twrite_or_die(pack_objects.in, \"^\", 1);\n+\t\twrite_or_die(pack_objects.in, sha1_to_hex(object->sha1), 40);\n+\t\twrite_or_die(pack_objects.in, \"\\n\", 1);\n+\t}\n+\tclose(pack_objects.in);\n+\tif (finish_command(&pack_objects))\n+\t\treturn error(_(\"pack-objects died\"));\n+\treturn 0;\n+}\n+\n int create_bundle(struct bundle_header *header, const char *path,\n \t\t  int argc, const char **argv)\n {\n@@ -381,34 +416,9 @@ int create_bundle(struct bundle_header *header, const char *path,\n \twrite_or_die(bundle_fd, \"\\n\", 1);\n \n \t/* write pack */\n-\tchild_process_init(&rls);\n-\targv_array_pushl(&rls.args,\n-\t\t\t \"pack-objects\", \"--all-progress-implied\",\n-\t\t\t \"--stdout\", \"--thin\", \"--delta-base-offset\",\n-\t\t\t NULL);\n-\trls.in = -1;\n-\trls.out = bundle_fd;\n-\trls.git_cmd = 1;\n-\tif (start_command(&rls))\n-\t\treturn error(_(\"Could not spawn pack-objects\"));\n-\n-\t/*\n-\t * start_command closed bundle_fd if it was > 1\n-\t * so set the lock fd to -1 so commit_lock_file()\n-\t * won't fail trying to close it.\n-\t */\n-\tlock.fd = -1;\n+\tif (write_pack_data(bundle_fd, &lock, &revs))\n+\t\treturn -1;\n \n-\tfor (i = 0; i < revs.pending.nr; i++) {\n-\t\tstruct object *object = revs.pending.objects[i].item;\n-\t\tif (object->flags & UNINTERESTING)\n-\t\t\twrite_or_die(rls.in, \"^\", 1);\n-\t\twrite_or_die(rls.in, sha1_to_hex(object->sha1), 40);\n-\t\twrite_or_die(rls.in, \"\\n\", 1);\n-\t}\n-\tclose(rls.in);\n-\tif (finish_command(&rls))\n-\t\treturn error(_(\"pack-objects died\"));\n \tif (!bundle_to_stdout) {\n \t\tif (commit_lock_file(&lock))\n \t\t\tdie_errno(_(\"cannot create '%s'\"), path);\n-- \n2.1.3-612-g493e79e\n"},{"id":"251227","messageId":"xmqqh9ylwhv2.fsf_-_@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"xmqqlhnyy9e2.fsf@gitster.dls.corp.google.com","subject":"[PATCH] bundle: split out a helper function to compute and write prerequisites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-30T18:08:17Z","receivedAt":"2014-10-30T18:08:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The new helper compute_and_write_prerequistes() is ugly, but it\ncannot be avoided.  Ideally we should avoid a function that computes\nand does I/O at the same time, but the prerequisites lines in the\noutput needs the human readable title only to help the recipient of\nthe bundle.  The code copies them straight from the rev-list output\nand immediately discards as no other internal computation needs that\ninformation.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * And this is to address the first one in the three-bullet list.\n\n bundle.c | 59 +++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 35 insertions(+), 24 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 9c87532..0ca8737 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -270,33 +270,15 @@ static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_inf\n \treturn 0;\n }\n \n-int create_bundle(struct bundle_header *header, const char *path,\n-\t\t  int argc, const char **argv)\n+static int compute_and_write_prerequistes(int bundle_fd,\n+\t\t\t\t\t  struct rev_info *revs,\n+\t\t\t\t\t  int argc, const char **argv)\n {\n-\tstatic struct lock_file lock;\n-\tint bundle_fd = -1;\n-\tint bundle_to_stdout;\n-\tint i, ref_count = 0;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct rev_info revs;\n \tstruct child_process rls = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tFILE *rls_fout;\n+\tint i;\n \n-\tbundle_to_stdout = !strcmp(path, \"-\");\n-\tif (bundle_to_stdout)\n-\t\tbundle_fd = 1;\n-\telse\n-\t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n-\t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n-\n-\t/* write signature */\n-\twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n-\n-\t/* init revs to list objects for pack-objects later */\n-\tsave_commit_buffer = 0;\n-\tinit_revisions(&revs, NULL);\n-\n-\t/* write prerequisites */\n \targv_array_pushl(&rls.args,\n \t\t\t \"rev-list\", \"--boundary\", \"--pretty=oneline\",\n \t\t\t NULL);\n@@ -314,7 +296,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\n-\t\t\t\tadd_pending_object(&revs, object, buf.buf);\n+\t\t\t\tadd_pending_object(revs, object, buf.buf);\n \t\t\t}\n \t\t} else if (!get_sha1_hex(buf.buf, sha1)) {\n \t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n@@ -325,6 +307,35 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tfclose(rls_fout);\n \tif (finish_command(&rls))\n \t\treturn error(_(\"rev-list died\"));\n+\treturn 0;\n+}\n+\n+int create_bundle(struct bundle_header *header, const char *path,\n+\t\t  int argc, const char **argv)\n+{\n+\tstatic struct lock_file lock;\n+\tint bundle_fd = -1;\n+\tint bundle_to_stdout;\n+\tint i, ref_count = 0;\n+\tstruct rev_info revs;\n+\n+\tbundle_to_stdout = !strcmp(path, \"-\");\n+\tif (bundle_to_stdout)\n+\t\tbundle_fd = 1;\n+\telse\n+\t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n+\t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n+\n+\t/* write signature */\n+\twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n+\n+\t/* init revs to list objects for pack-objects later */\n+\tsave_commit_buffer = 0;\n+\tinit_revisions(&revs, NULL);\n+\n+\t/* write prerequisites */\n+\tif (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))\n+\t\treturn -1;\n \n \t/* write references */\n \targc = setup_revisions(argc, argv, &revs, NULL);\n-- \n2.1.3-612-g493e79e\n"},{"id":"251242","messageId":"20141030212551.GA26030@peff.net","threadId":"37826","inReplyTo":"xmqqlhnxwhw4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-30T21:25:51Z","receivedAt":"2014-10-30T21:25:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 30, 2014 at 11:07:39AM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: [PATCH] bundle: split out a helper function to create a pack data\n\ns/a pack data/pack data/\n\n> The create_bundle() function, while it does one single logical thing\n> and tries to do it well, that single logical thing takes a rather\n> large implementation.\n\nI had minor trouble parsing this. I think it might be more clearly said\nas just:\n\n  The create_bundle() function, while it does one single logical thing,\n  takes a rather large implementation to do so.\n\n> Let's start separating what it does into smaller steps to make it\n> easier what is going on.  This is a first step to separate out the\n\ns/easier/& to see/\n\n>  bundle.c | 64 +++++++++++++++++++++++++++++++++++++---------------------------\n>  1 file changed, 37 insertions(+), 27 deletions(-)\n\nThe patch itself looked OK to me.\n\n-Peff\n"},{"id":"251243","messageId":"20141030212641.GB26030@peff.net","threadId":"37826","inReplyTo":"xmqqh9ylwhv2.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] bundle: split out a helper function to compute and write prerequisites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-30T21:26:42Z","receivedAt":"2014-10-30T21:26:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 30, 2014 at 11:08:17AM -0700, Junio C Hamano wrote:\n\n> The new helper compute_and_write_prerequistes() is ugly, but it\n\ns/quistes/quisites/\n\nThe same typo is in the function name in the code.\n\n-Peff\n"},{"id":"251244","messageId":"20141030213523.GA21017@peff.net","threadId":"37826","inReplyTo":"xmqqlhnyy9e2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-30T21:35:24Z","receivedAt":"2014-10-30T21:35:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 29, 2014 at 12:16:05PM -0700, Junio C Hamano wrote:\n\n> Probably three helper functions:\n> \n>  - The first is to find tops and bottoms (this translates fuzzy\n>    specifications such as \"--since 30.days\" into a more concrete\n>    revision range \"^A ^B ... Z\" to establish bundle prerequisites),\n>    which is done by running a \"rev-list --boundary\".\n> \n>  - The second is to show refs, while paying attention to things like\n>    \"--10 maint master\" which may result in the tip of 'maint' not\n>    being shown at all.  I am not sure if this part can/should take\n>    advantage of revs.cmdline, though.\n> \n>  - The last is to create the actual pack data.\n> \n> I agree with your analysis on the change in column.c and trailer.c\n\nI was not planning to work on this, but since you did the first and\nthird bullet points, I think it makes sense to start the second one with\nthis cleanup:\n\n-- >8 --\nSubject: bundle: split out ref writing from bundle_create\n\nThe bundle_create() function has a number of logical steps:\nprocess the input, write the refs, and write the packfile.\nRecent commits split the first and third into separate\nsub-functions. It's worth splitting the middle step out,\ntoo, if only because it makes the progression of the steps\nmore obvious.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously this should be dropped if somebody is actively working on the\nrevs.cmdline thing you mentioned, as it would conflict horribly. But I\nthink it is a nice step for somebody working on it later, because the\nrevs.cmdline changes should be isolated to write_bundle_refs.\n\n bundle.c | 97 ++++++++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 58 insertions(+), 39 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 0ca8737..ca4803b 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -310,43 +310,22 @@ static int compute_and_write_prerequistes(int bundle_fd,\n \treturn 0;\n }\n \n-int create_bundle(struct bundle_header *header, const char *path,\n-\t\t  int argc, const char **argv)\n+/*\n+ * Write out bundle refs based on the tips already\n+ * parsed into revs.pending. As a side effect, may\n+ * manipulate revs.pending to include additional\n+ * necessary objects (like tags).\n+ *\n+ * Returns the number of refs written, or negative\n+ * on error.\n+ */\n+static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n {\n-\tstatic struct lock_file lock;\n-\tint bundle_fd = -1;\n-\tint bundle_to_stdout;\n-\tint i, ref_count = 0;\n-\tstruct rev_info revs;\n-\n-\tbundle_to_stdout = !strcmp(path, \"-\");\n-\tif (bundle_to_stdout)\n-\t\tbundle_fd = 1;\n-\telse\n-\t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n-\t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n-\n-\t/* write signature */\n-\twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n-\n-\t/* init revs to list objects for pack-objects later */\n-\tsave_commit_buffer = 0;\n-\tinit_revisions(&revs, NULL);\n-\n-\t/* write prerequisites */\n-\tif (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))\n-\t\treturn -1;\n-\n-\t/* write references */\n-\targc = setup_revisions(argc, argv, &revs, NULL);\n-\n-\tif (argc > 1)\n-\t\treturn error(_(\"unrecognized argument: %s\"), argv[1]);\n-\n-\tobject_array_remove_duplicates(&revs.pending);\n+\tint i;\n+\tint ref_count = 0;\n \n-\tfor (i = 0; i < revs.pending.nr; i++) {\n-\t\tstruct object_array_entry *e = revs.pending.objects + i;\n+\tfor (i = 0; i < revs->pending.nr; i++) {\n+\t\tstruct object_array_entry *e = revs->pending.objects + i;\n \t\tunsigned char sha1[20];\n \t\tchar *ref;\n \t\tconst char *display_ref;\n@@ -361,7 +340,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\tdisplay_ref = (flag & REF_ISSYMREF) ? e->name : ref;\n \n \t\tif (e->item->type == OBJ_TAG &&\n-\t\t\t\t!is_tag_in_date_range(e->item, &revs)) {\n+\t\t\t\t!is_tag_in_date_range(e->item, revs)) {\n \t\t\te->item->flags |= UNINTERESTING;\n \t\t\tcontinue;\n \t\t}\n@@ -407,7 +386,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\t\t\t */\n \t\t\t\tobj = parse_object_or_die(sha1, e->name);\n \t\t\t\tobj->flags |= SHOWN;\n-\t\t\t\tadd_pending_object(&revs, obj, e->name);\n+\t\t\t\tadd_pending_object(revs, obj, e->name);\n \t\t\t}\n \t\t\tfree(ref);\n \t\t\tcontinue;\n@@ -420,11 +399,51 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\twrite_or_die(bundle_fd, \"\\n\", 1);\n \t\tfree(ref);\n \t}\n-\tif (!ref_count)\n-\t\tdie(_(\"Refusing to create empty bundle.\"));\n \n \t/* end header */\n \twrite_or_die(bundle_fd, \"\\n\", 1);\n+\treturn ref_count;\n+}\n+\n+int create_bundle(struct bundle_header *header, const char *path,\n+\t\t  int argc, const char **argv)\n+{\n+\tstatic struct lock_file lock;\n+\tint bundle_fd = -1;\n+\tint bundle_to_stdout;\n+\tint ref_count = 0;\n+\tstruct rev_info revs;\n+\n+\tbundle_to_stdout = !strcmp(path, \"-\");\n+\tif (bundle_to_stdout)\n+\t\tbundle_fd = 1;\n+\telse\n+\t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n+\t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n+\n+\t/* write signature */\n+\twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n+\n+\t/* init revs to list objects for pack-objects later */\n+\tsave_commit_buffer = 0;\n+\tinit_revisions(&revs, NULL);\n+\n+\t/* write prerequisites */\n+\tif (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))\n+\t\treturn -1;\n+\n+\targc = setup_revisions(argc, argv, &revs, NULL);\n+\n+\tif (argc > 1)\n+\t\treturn error(_(\"unrecognized argument: %s\"), argv[1]);\n+\n+\tobject_array_remove_duplicates(&revs.pending);\n+\n+\tref_count = write_bundle_refs(bundle_fd, &revs);\n+\tif (!ref_count)\n+\t\tdie(_(\"Refusing to create empty bundle.\"));\n+\telse if (ref_count < 0)\n+\t\treturn -1;\n \n \t/* write pack */\n \tif (write_pack_data(bundle_fd, &lock, &revs))\n-- \n2.1.2.596.g7379948\n"},{"id":"251255","messageId":"FEC7DC4C920D4F97B5F165B10BC564D2@PhilipOakley","threadId":"37826","inReplyTo":"20141030213523.GA21017@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-10-31T00:13:51Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" <peff@peff.net>\n> On Wed, Oct 29, 2014 at 12:16:05PM -0700, Junio C Hamano wrote:\n>\n>> Probably three helper functions:\n>>\n>>  - The first is to find tops and bottoms (this translates fuzzy\n>>    specifications such as \"--since 30.days\" into a more concrete\n>>    revision range \"^A ^B ... Z\" to establish bundle prerequisites),\n>>    which is done by running a \"rev-list --boundary\".\n>>\n>>  - The second is to show refs, while paying attention to things like\n>>    \"--10 maint master\" which may result in the tip of 'maint' not\n>>    being shown at all.  I am not sure if this part can/should take\n>>    advantage of revs.cmdline, though.\n>>\n>>  - The last is to create the actual pack data.\n>>\n>> I agree with your analysis on the change in column.c and trailer.c\n>\n> I was not planning to work on this, but since you did the first and\n> third bullet points, I think it makes sense to start the second one \n> with\n> this cleanup:\n>\n> -- >8 --\n> Subject: bundle: split out ref writing from bundle_create\n>\n> The bundle_create() function has a number of logical steps:\n> process the input, write the refs, and write the packfile.\n> Recent commits split the first and third into separate\n> sub-functions. It's worth splitting the middle step out,\n> too, if only because it makes the progression of the steps\n> more obvious.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Obviously this should be dropped if somebody is actively working on \n> the\n> revs.cmdline thing you mentioned, as it would conflict horribly. But I\n> think it is a nice step for somebody working on it later, because the\n> revs.cmdline changes should be isolated to write_bundle_refs.\n\nAs a side project (slow time) I've been looking at the loss of the HEAD \nsymbolic ref when multiple heads are bundled that point at the same rev. \nThat is, when the HEAD detection heuristic fails.\n\nWhat I've noticed so far is that a duplicated ref (added to the bundle \nmanually) simply creates a warning (currently, and the warning names \nthat ref) rather than failing when cloned (and by implication fetched). \nNot only that the bundle verify command does not report any error for \nsuch a bundle containing a duplicate ref.\n\nGiven that, if the refs were sorted, and HEAD was listed last (as is the \ncase with '--all'), then one could add the duplicate ref immediately \nafter HEAD, of it's symbolic ref. I.e if a duplicate ref is found after \nHEAD then that ref is the true HEAD ref. This duplicate ref would only \nneed to be present if there are multiple (two or more) heads that point \nto the same rev, and the HEAD isn't detatched. Sorting is necessary to \nensure that HEAD is last and its duplicate refs/head ref immediately \nfollows.\n\nThus the first step is to ensure that the positive refs list is sorted \nsuch that HEAD (and it's ilk) is last.\n\nI'd also planned an option '--HEAD' which would add such a duplicate ref \nto a V2 bundle (resulting in a warning for older users which displays \nthe duplicate head ref!)\n\nAn option '--V3' would be the same as '--HEAD' but would also change the \nbundle string version number (to V3), and so would not be acceptable to \nolder systems (for those that require such separation) but the \nclone/fetch on a newer system would detect the V3 in the header string \nand then use the extra ref rather than the heuristic to determine HEAD \n(with appropriate checks).\n\nI hadn't finished my studies of the refs.c code to fully understand what \nI'd need to change, but hopefully the changes in this patch can be \naligned in the same direction (or the errors in my reasoning be pointed \nout;-)\n\nThe need to sort the refs in this method would separate the \ndetermination of the correct refs from the writing of the refs. All \nassuming the idea has merit...\n\n--\nPhilip\n\n>\n> bundle.c | 97 \n> ++++++++++++++++++++++++++++++++++++++--------------------------\n> 1 file changed, 58 insertions(+), 39 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index 0ca8737..ca4803b 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -310,43 +310,22 @@ static int compute_and_write_prerequistes(int \n> bundle_fd,\n>  return 0;\n> }\n>\n> -int create_bundle(struct bundle_header *header, const char *path,\n> -   int argc, const char **argv)\n> +/*\n> + * Write out bundle refs based on the tips already\n> + * parsed into revs.pending. As a side effect, may\n> + * manipulate revs.pending to include additional\n> + * necessary objects (like tags).\n> + *\n> + * Returns the number of refs written, or negative\n> + * on error.\n> + */\n> +static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n> {\n> - static struct lock_file lock;\n> - int bundle_fd = -1;\n> - int bundle_to_stdout;\n> - int i, ref_count = 0;\n> - struct rev_info revs;\n> -\n> - bundle_to_stdout = !strcmp(path, \"-\");\n> - if (bundle_to_stdout)\n> - bundle_fd = 1;\n> - else\n> - bundle_fd = hold_lock_file_for_update(&lock, path,\n> -       LOCK_DIE_ON_ERROR);\n> -\n> - /* write signature */\n> - write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n> -\n> - /* init revs to list objects for pack-objects later */\n> - save_commit_buffer = 0;\n> - init_revisions(&revs, NULL);\n> -\n> - /* write prerequisites */\n> - if (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))\n> - return -1;\n> -\n> - /* write references */\n> - argc = setup_revisions(argc, argv, &revs, NULL);\n> -\n> - if (argc > 1)\n> - return error(_(\"unrecognized argument: %s\"), argv[1]);\n> -\n> - object_array_remove_duplicates(&revs.pending);\n> + int i;\n> + int ref_count = 0;\n>\n> - for (i = 0; i < revs.pending.nr; i++) {\n> - struct object_array_entry *e = revs.pending.objects + i;\n> + for (i = 0; i < revs->pending.nr; i++) {\n> + struct object_array_entry *e = revs->pending.objects + i;\n>  unsigned char sha1[20];\n>  char *ref;\n>  const char *display_ref;\n> @@ -361,7 +340,7 @@ int create_bundle(struct bundle_header *header, \n> const char *path,\n>  display_ref = (flag & REF_ISSYMREF) ? e->name : ref;\n>\n>  if (e->item->type == OBJ_TAG &&\n> - !is_tag_in_date_range(e->item, &revs)) {\n> + !is_tag_in_date_range(e->item, revs)) {\n>  e->item->flags |= UNINTERESTING;\n>  continue;\n>  }\n> @@ -407,7 +386,7 @@ int create_bundle(struct bundle_header *header, \n> const char *path,\n>  */\n>  obj = parse_object_or_die(sha1, e->name);\n>  obj->flags |= SHOWN;\n> - add_pending_object(&revs, obj, e->name);\n> + add_pending_object(revs, obj, e->name);\n>  }\n>  free(ref);\n>  continue;\n> @@ -420,11 +399,51 @@ int create_bundle(struct bundle_header *header, \n> const char *path,\n>  write_or_die(bundle_fd, \"\\n\", 1);\n>  free(ref);\n>  }\n> - if (!ref_count)\n> - die(_(\"Refusing to create empty bundle.\"));\n>\n>  /* end header */\n>  write_or_die(bundle_fd, \"\\n\", 1);\n> + return ref_count;\n> +}\n> +\n> +int create_bundle(struct bundle_header *header, const char *path,\n> +   int argc, const char **argv)\n> +{\n> + static struct lock_file lock;\n> + int bundle_fd = -1;\n> + int bundle_to_stdout;\n> + int ref_count = 0;\n> + struct rev_info revs;\n> +\n> + bundle_to_stdout = !strcmp(path, \"-\");\n> + if (bundle_to_stdout)\n> + bundle_fd = 1;\n> + else\n> + bundle_fd = hold_lock_file_for_update(&lock, path,\n> +       LOCK_DIE_ON_ERROR);\n> +\n> + /* write signature */\n> + write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n> +\n> + /* init revs to list objects for pack-objects later */\n> + save_commit_buffer = 0;\n> + init_revisions(&revs, NULL);\n> +\n> + /* write prerequisites */\n> + if (compute_and_write_prerequistes(bundle_fd, &revs, argc, argv))\n> + return -1;\n> +\n> + argc = setup_revisions(argc, argv, &revs, NULL);\n> +\n> + if (argc > 1)\n> + return error(_(\"unrecognized argument: %s\"), argv[1]);\n> +\n> + object_array_remove_duplicates(&revs.pending);\n> +\n> + ref_count = write_bundle_refs(bundle_fd, &revs);\n> + if (!ref_count)\n> + die(_(\"Refusing to create empty bundle.\"));\n> + else if (ref_count < 0)\n> + return -1;\n>\n>  /* write pack */\n>  if (write_pack_data(bundle_fd, &lock, &revs))\n> -- \n> 2.1.2.596.g7379948\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"251276","messageId":"xmqqvbmzsyfy.fsf@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"FEC7DC4C920D4F97B5F165B10BC564D2@PhilipOakley","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-31T21:48:17Z","receivedAt":"2014-10-31T21:48:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" <philipoakley@iee.org> writes:\n\n> As a side project (slow time) I've been looking at the loss of the\n> HEAD symbolic ref when multiple heads are bundled that point at the\n> same rev. That is, when the HEAD detection heuristic fails.\n\nIt think you are talking about the logic used by the \"clone\", where\n\n - if there is only one branch ref that matches the value of HEAD,\n   that is the branch;\n\n - if there are more than one refs that match the value of HEAD,\n   and if one of them is 'master', then that is the branch;\n\n - if there are more than one refs that match the value of HEAD,\n   and if none of them is 'master', then pick the earliest one.\n\nSo you would be in trouble _if_ you have two refs pointing at the\nsame commit, one of them being 'master', and the current branch is\nthe other ref.  All other cases you shouldn't have to change the\nfile format and have older client understand which branch is the\ncurrent one.\n\nPrograms that read a pack data stream unpack-objects were originally\ndesigned to ignore cruft after the pack data stream ends, and\nbecause the bundle file format ends with pack data stream, you\nshould have been able to append extra information at the end without\nbreaking older clients.  Alas, this principle is still true for\nunpack-objects, but index-pack broke it fairly early on, and we use\nthe latter to deal with bundles, so we cannot just tuck extra info\nat the end of an existing bundle.  You'd instead need a new option\nto create a bundle that cannot be read by existing clients X-<.\n"},{"id":"251282","messageId":"20141101033327.GA8307@peff.net","threadId":"37826","inReplyTo":"xmqqvbmzsyfy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-01T03:33:27Z","receivedAt":"2014-11-01T03:33:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 31, 2014 at 02:48:17PM -0700, Junio C Hamano wrote:\n\n> Programs that read a pack data stream unpack-objects were originally\n> designed to ignore cruft after the pack data stream ends, and\n> because the bundle file format ends with pack data stream, you\n> should have been able to append extra information at the end without\n> breaking older clients.  Alas, this principle is still true for\n> unpack-objects, but index-pack broke it fairly early on, and we use\n> the latter to deal with bundles, so we cannot just tuck extra info\n> at the end of an existing bundle.  You'd instead need a new option\n> to create a bundle that cannot be read by existing clients X-<.\n\nI think you could use a similar NUL-trick to what we do in the online\nprotocol, and have a ref section like:\n\n  ...sha1... refs/heads/master\n  ...sha1... refs/heads/confused-with-master\n  ...sha1... HEAD\\0symref=refs/heads/master\n\nThe current parser reads into a strbuf up to the newline, but we ignore\neverything after the NUL, treating it like a C string. Prior to using\nstrbufs, we used fgets, which behaves similarly (you could not know from\nfgets that there is extra data after the NUL, but that is OK; we only\nwant older versions to ignore the data, not do anything useful with it).\n\n-Peff\n"},{"id":"251294","messageId":"F44397C122BB4E63B89EC9BE26007B2E@PhilipOakley","threadId":"37826","inReplyTo":"20141101033327.GA8307@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-11-02T19:06:47Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" <peff@peff.net>\n> On Fri, Oct 31, 2014 at 02:48:17PM -0700, Junio C Hamano wrote:\n>\n>> Programs that read a pack data stream unpack-objects were originally\n>> designed to ignore cruft after the pack data stream ends, and\n>> because the bundle file format ends with pack data stream, you\n>> should have been able to append extra information at the end without\n>> breaking older clients.\n\nIt's an option, I'd been looking at sneaking the information into the \nrefs header section.\n\n>> Alas, this principle is still true for\n>> unpack-objects, but index-pack broke it fairly early on, and we use\n>> the latter to deal with bundles, so we cannot just tuck extra info\n>> at the end of an existing bundle.  You'd instead need a new option\n>> to create a bundle that cannot be read by existing clients X-<.\n>\n> I think you could use a similar NUL-trick to what we do in the online\n\nI like this 'trick'. I'd not appreciated the use of the null separator\n for breaking a line into separate strings that way before (I'd \nunderstood it, just never appreciated it!).\n\n> protocol, and have a ref section like:\n>\n>  ...sha1... refs/heads/master\n>  ...sha1... refs/heads/confused-with-master\n>  ...sha1... HEAD\\0symref=refs/heads/master\n>\n> The current parser reads into a strbuf up to the newline, but we\n> ignore\n> everything after the NUL, treating it like a C string. Prior to using\n> strbufs, we used fgets, which behaves similarly (you could not know\n> from\n> fgets that there is extra data after the NUL, but that is OK; we only\n> want older versions to ignore the data, not do anything useful with\n> it).\n>\n\nThis certainly looks the way to go. The one extra question would be\nwhether the symref should be included by default when HEAD is present, \nor only if there was possible ambiguity between the other listed refs. \nPreviously I'd assumed the latter. The former would appear stronger, as \nlong as the symref was within the listed refs, and excluded otherwise.\n\nPhilip \n"},{"id":"251321","messageId":"xmqqmw88rvh3.fsf@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"F44397C122BB4E63B89EC9BE26007B2E@PhilipOakley","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-03T18:26:48Z","receivedAt":"2014-11-03T18:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" <philipoakley@iee.org> writes:\n\n> This certainly looks the way to go. The one extra question would be\n> whether the symref should be included by default when HEAD is present,\n> or only if there was possible ambiguity between the other listed\n> refs.\n\nJust include the \"\\0symref=...\" for any symbolic ref you mention,\nand the ref in question does not even have to be \"HEAD\", I would\nsay.\n\nThe mechanism chosen should be something that will be transparently\nignored by existing implementations, there is no need to make the\ndata format conditional.  If the new implementations of the reading\nside want to make a choice between following the new \"\\0symref=...\"\nand ignoring it and use the traditional heuristics for some\nunknown/unanticipated reason, that should be the choice for the\nreaders, not for the writers.\n"},{"id":"251364","messageId":"20141103220408.GA12462@peff.net","threadId":"37826","inReplyTo":"xmqqmw88rvh3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-03T22:04:08Z","receivedAt":"2014-11-03T22:04:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 03, 2014 at 10:26:48AM -0800, Junio C Hamano wrote:\n\n> \"Philip Oakley\" <philipoakley@iee.org> writes:\n> \n> > This certainly looks the way to go. The one extra question would be\n> > whether the symref should be included by default when HEAD is present,\n> > or only if there was possible ambiguity between the other listed\n> > refs.\n> \n> Just include the \"\\0symref=...\" for any symbolic ref you mention,\n> and the ref in question does not even have to be \"HEAD\", I would\n> say.\n> \n> The mechanism chosen should be something that will be transparently\n> ignored by existing implementations, there is no need to make the\n> data format conditional.\n\nOne thing I glossed over in my suggestion of the NUL trick: it works on\ngit.git, but no clue about elsewhere. I can imagine that other non-C\nimplementations might treat the whole thing (NUL and extra data\nincluded) as the refname. Back when we did the NUL trick to the online\nprotocol, git.git was the only serious implementation. But nowadays we\nshould at least consider the impact on JGit, libgit2, and/or dulwich\n(breaking them is not necessarily a showstopper IMHO, but we should at\nleast know what we are breaking).\n\nI peeked at libgit2 and I think it does not support bundles at all yet,\nso that is safe. Grepping for \"bundle\" in dulwich turns up no hits,\neither.\n\nLooks like JGit does support them. I did a very brief test, and it seems\nto silently ignore a HEAD ref that has the NUL (I guess maybe it just\nrejects it as a malformed refname).\n\nWe could make JGit happier either by:\n\n  1. Only including the symref magic in ambiguous cases, so that regular\n     ones Just Work as usual.\n\n  2. Including two lines, like:\n\n        $sha1 HEAD\\0symref=refs/heads/master\n\t$sha1 HEAD\n\n     which JGit does the right thing with (and git.git seems to, as\n     well).\n\n-Peff\n"},{"id":"251370","messageId":"xmqq389zrguw.fsf@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"20141103220408.GA12462@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-03T23:42:31Z","receivedAt":"2014-11-03T23:42:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I peeked at libgit2 and I think it does not support bundles at all yet,\n> so that is safe. Grepping for \"bundle\" in dulwich turns up no hits,\n> either.\n>\n> Looks like JGit does support them. I did a very brief test, and it seems\n> to silently ignore a HEAD ref that has the NUL (I guess maybe it just\n> rejects it as a malformed refname).\n>\n> We could make JGit happier either by:\n>\n>   1. Only including the symref magic in ambiguous cases, so that regular\n>      ones Just Work as usual.\n>\n>   2. Including two lines, like:\n>\n>         $sha1 HEAD\\0symref=refs/heads/master\n> \t$sha1 HEAD\n>\n>      which JGit does the right thing with (and git.git seems to, as\n>      well).\n\nSounds sensible, even though it looks ugly X-<.\n"},{"id":"251389","messageId":"xmqq4muepr40.fsf@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"xmqq389zrguw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-04T21:56:15Z","receivedAt":"2014-11-04T21:56:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> I peeked at libgit2 and I think it does not support bundles at all yet,\n>> so that is safe. Grepping for \"bundle\" in dulwich turns up no hits,\n>> either.\n>>\n>> Looks like JGit does support them. I did a very brief test, and it seems\n>> to silently ignore a HEAD ref that has the NUL (I guess maybe it just\n>> rejects it as a malformed refname).\n>>\n>> We could make JGit happier either by:\n>>\n>>   1. Only including the symref magic in ambiguous cases, so that regular\n>>      ones Just Work as usual.\n>>\n>>   2. Including two lines, like:\n>>\n>>         $sha1 HEAD\\0symref=refs/heads/master\n>> \t$sha1 HEAD\n>>\n>>      which JGit does the right thing with (and git.git seems to, as\n>>      well).\n>\n> Sounds sensible, even though it looks ugly X-<.\n\nI have a mild preference for a syntax that is more similar to the\non-wire protocol, so that connect.c::parse_feature_value() can be\nreused to parse it and possibly annotate_refs_with_symref_info() can\nalso be reused by calling it from transport.c::get_refs_from_bundle().\n"},{"id":"251394","messageId":"20141104233215.GA16091@peff.net","threadId":"37826","inReplyTo":"xmqq4muepr40.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-04T23:32:15Z","receivedAt":"2014-11-04T23:32:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote:\n\n> >>   2. Including two lines, like:\n> >>\n> >>         $sha1 HEAD\\0symref=refs/heads/master\n> >>         $sha1 HEAD\n> >>\n> >>      which JGit does the right thing with (and git.git seems to, as\n> >>      well).\n> >\n> > Sounds sensible, even though it looks ugly X-<.\n> \n> I have a mild preference for a syntax that is more similar to the\n> on-wire protocol, so that connect.c::parse_feature_value() can be\n> reused to parse it and possibly annotate_refs_with_symref_info() can\n> also be reused by calling it from transport.c::get_refs_from_bundle().\n\nYeah, what I wrote above was the simplest thing that could work, and\ndoes not need to be the final form.  I know that you already know what\nI'm about to describe below, Junio, but I want to expand on the\nsituation for the benefit of onlookers (and potential implementers like\nPhilip).\n\nThe online protocol is hampered by the \"if you see something after a\nNUL, it is a capabilities string, and you must throw out the previous\ncapabilities string and replace it with this one\" historical rule. And\nthat's why we cannot do:\n\n  $sha1 refs/heads/master\\0thin-pack side-band etc\n  $sha1 HEAD\\0symref=refs/heads/master\n\nas it would throw out \"thin-pack\", \"side-band\", etc. Instead we do it\nmore like:\n\n  $sha1 refs/heads/master\\0thin-pack side-band etc symref=HEAD:refs/heads/master\n  $sha1 HEAD\n\nto shove _all_ of the symref mappings into the capability string, rather\nthan letting them ride along with their respective refs. The downside is\nthat we are bounded in the number of symref mappings we can send (by the\nmaximum length for a single pkt-line), and therefore send only the value\nof HEAD.\n\nThe bundle code is not bound by this historical legacy, and could do it\nin a different (and more efficient and flexible) way. But it is probably\nsaner to just keep them identical. It makes the code simpler, and having\nbundle as the only transport which has the extra flexibility does not\nreally buy us much (and probably just invites confusion).\n\n-Peff\n"},{"id":"251395","messageId":"xmqqvbmuo5bt.fsf@gitster.dls.corp.google.com","threadId":"37826","inReplyTo":"20141104233215.GA16091@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-05T00:32:06Z","receivedAt":"2014-11-05T00:32:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The bundle code is not bound by this historical legacy, and could do it\n> in a different (and more efficient and flexible) way. But it is probably\n> saner to just keep them identical. It makes the code simpler, and having\n> bundle as the only transport which has the extra flexibility does not\n> really buy us much (and probably just invites confusion).\n\nYeah, so let's have only symref=HEAD:refs/heads/master for now.\n\nI would like to have the protocol update on the on-wire side during\n2015 to lift various limits and correct inefficiencies (the largest\nof which is the \"who speaks first\" issue).  We should make sure that\nthe bundle format can be enhanced to match when it happens.\n\nThanks.\n"},{"id":"251407","messageId":"D4F1F843014841509E8BFB9ACC7CDBCC@PhilipOakley","threadId":"37826","inReplyTo":"xmqq389zrguw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-11-05T12:44:29Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" <gitster@pobox.com>\nSent: Monday, November 03, 2014 11:42 PM\n> Jeff King <peff@peff.net> writes:\n>\n>> I peeked at libgit2 and I think it does not support bundles at all \n>> yet,\n>> so that is safe. Grepping for \"bundle\" in dulwich turns up no hits,\n>> either.\n>>\n>> Looks like JGit does support them. I did a very brief test, and it \n>> seems\n>> to silently ignore a HEAD ref that has the NUL (I guess maybe it just\n>> rejects it as a malformed refname).\n>>\n>> We could make JGit happier either by:\n>>\n>>   1. Only including the symref magic in ambiguous cases, so that \n>> regular\n>>      ones Just Work as usual.\n>>\n>>   2. Including two lines, like:\n>>\n>>         $sha1 HEAD\\0symref=refs/heads/master\n>> $sha1 HEAD\n>>\n>>      which JGit does the right thing with (and git.git seems to, as\n>>      well).\n>\n> Sounds sensible, even though it looks ugly X-<.\n>\nI believe that the 'two HEADs' mechanism would also fall foul of the \n'duplicate refs' warning (untested).\n\nPhilip \n"},{"id":"251408","messageId":"95BF305A6E8D485F9F63503ACE7FC2AB@PhilipOakley","threadId":"37826","inReplyTo":"20141104233215.GA16091@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-11-05T12:44:29Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" <peff@peff.net>\nSubject: Re: [PATCH] use child_process_init() to initialize struct \nchild_process variables\n\n\n> On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote:\n>\n>> >>   2. Including two lines, like:\n>> >>\n>> >>         $sha1 HEAD\\0symref=refs/heads/master\n>> >>         $sha1 HEAD\n>> >>\n>> >>      which JGit does the right thing with (and git.git seems to, \n>> >> as\n>> >>      well).\n>> >\n>> > Sounds sensible, even though it looks ugly X-<.\n>>\n>> I have a mild preference for a syntax that is more similar to the\n>> on-wire protocol, so that connect.c::parse_feature_value() can be\n>> reused to parse it and possibly annotate_refs_with_symref_info() can\n>> also be reused by calling it from \n>> transport.c::get_refs_from_bundle().\n>\n> Yeah, what I wrote above was the simplest thing that could work, and\n> does not need to be the final form.  I know that you already know what\n> I'm about to describe below, Junio, but I want to expand on the\n> situation for the benefit of onlookers (and potential implementers \n> like\n> Philip).\n\nI think I'm keeping up ;-)\n>\n> The online protocol is hampered by the \"if you see something after a\n> NUL, it is a capabilities string, and you must throw out the previous\n> capabilities string and replace it with this one\" historical rule. And\n> that's why we cannot do:\n>\n>  $sha1 refs/heads/master\\0thin-pack side-band etc\n>  $sha1 HEAD\\0symref=refs/heads/master\n>\n> as it would throw out \"thin-pack\", \"side-band\", etc. Instead we do it\n> more like:\n>\n>  $sha1 refs/heads/master\\0thin-pack side-band etc \n> symref=HEAD:refs/heads/master\n>  $sha1 HEAD\n>\n> to shove _all_ of the symref mappings into the capability string, \n> rather\n> than letting them ride along with their respective refs. The downside \n> is\n> that we are bounded in the number of symref mappings we can send (by \n> the\n> maximum length for a single pkt-line), and therefore send only the \n> value\n> of HEAD.\n>\n> The bundle code is not bound by this historical legacy, and could do \n> it\n> in a different (and more efficient and flexible) way. But it is \n> probably\n> saner to just keep them identical. It makes the code simpler, and \n> having\n> bundle as the only transport which has the extra flexibility does not\n> really buy us much (and probably just invites confusion).\n>\n> -Peff\n>\nObviously bundles are always off-line, so it's reasonable to be cautious \nabout using an on-line sideband method, though the re-use of a standard \nformat is good.\n\nFinding the right parsing method will be important, as well as ensuring \nthere are no races from the update of unsorted refs.\n\nPhilip \n"},{"id":"251416","messageId":"20141105193557.GA12620@peff.net","threadId":"37826","inReplyTo":"D4F1F843014841509E8BFB9ACC7CDBCC@PhilipOakley","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-05T19:35:57Z","receivedAt":"2014-11-05T19:35:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 05, 2014 at 01:35:21PM -0000, Philip Oakley wrote:\n\n> >>  2. Including two lines, like:\n> [...]\n> I believe that the 'two HEADs' mechanism would also fall foul of the\n> 'duplicate refs' warning (untested).\n\nIt didn't in my very brief testing of what I posted above, but maybe\nthere is some other case that triggers it that I didn't exercise.\n\nI grepped through the code and the only \"duplicate ref\" warning I see\ncomes from the refs.c code, which comes from commit_packed_refs(). If\nthe duplicate line is HEAD, I think it shouldn't trigger that, as it is\nnot a regular ref. That would explain why I didn't see it in my testing.\n\n-Peff\n"},{"id":"251421","messageId":"05777A2415DD49F78B81EEA886A98F48@PhilipOakley","threadId":"37826","inReplyTo":"20141105193557.GA12620@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-11-05T22:58:21Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" <peff@peff.net>\n> On Wed, Nov 05, 2014 at 01:35:21PM -0000, Philip Oakley wrote:\n>\n>> >>  2. Including two lines, like:\n>> [...]\n>> I believe that the 'two HEADs' mechanism would also fall foul of the\n>> 'duplicate refs' warning (untested).\n>\n> It didn't in my very brief testing of what I posted above, but maybe\n> there is some other case that triggers it that I didn't exercise.\n\nI'd been testing the inclusion of a duplicate of the ref that matched \nthe HEAD symref (rather than HEAD itself), and had hit that message a \nfew times, hence my concern.\n>\n> I grepped through the code and the only \"duplicate ref\" warning I see\n> comes from the refs.c code, which comes from commit_packed_refs().\n\nI had it from is_dup_ref(), also in refs.c, though I may have followed \nthe call stack incorrectly back to the bundle effects.\n\n> If\n> the duplicate line is HEAD, I think it shouldn't trigger that, as it \n> is\n> not a regular ref. That would explain why I didn't see it in my \n> testing.\n\nI've now also done a test and found the same (no warning/error) when \nthere are two HEADs listed in the bundle preamble. Sorry for the \nconfusion.\n\n--\nPhilip\n"},{"id":"251611","messageId":"545F7106.7070300@web.de","threadId":"37826","inReplyTo":"20141029172109.GA32234@peff.net","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-11-09T13:49:58Z","receivedAt":"2014-11-09T13:49:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.10.2014 um 18:21 schrieb Jeff King:\n> On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:\n>> diff --git a/trailer.c b/trailer.c\n>> index 8514566..7ff036c 100644\n>> --- a/trailer.c\n>> +++ b/trailer.c\n>> @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg)\n>>   \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n>>   \n>>   \targv[0] = cmd.buf;\n>> -\tmemset(&cp, 0, sizeof(cp));\n>> +\tchild_process_init(&cp);\n>>   \tcp.argv = argv;\n>>   \tcp.env = local_repo_env;\n>>   \tcp.no_stdin = 1;\n> \n> I think this one can use CHILD_PROCESS_INIT in the declaration. I guess\n> it is debatable whether that is actually preferable, but I tend to think\n> it is cleaner and less error-prone.\n\nAgreed, thanks.\n\n-- >8 --\nSubject: [PATCH] trailer: use CHILD_PROCESS_INIT in apply_command()\n\nInitialize the struct child_process variable cp at declaration time.\nThis is shorter, saves a function call and prevents using the variable\nbefore initialization by mistake.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n trailer.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 7ff036c..6ae7865 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -228,7 +228,7 @@ static const char *apply_command(const char *command, const char *arg)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct child_process cp;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n \tconst char *argv[] = {NULL, NULL};\n \tconst char *result;\n \n@@ -237,7 +237,6 @@ static const char *apply_command(const char *command, const char *arg)\n \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n \n \targv[0] = cmd.buf;\n-\tchild_process_init(&cp);\n \tcp.argv = argv;\n \tcp.env = local_repo_env;\n \tcp.no_stdin = 1;\n-- \n2.1.3\n"},{"id":"251657","messageId":"20141110071410.GE7677@peff.net","threadId":"37826","inReplyTo":"545F7106.7070300@web.de","subject":"Re: [PATCH] use child_process_init() to initialize struct child_process variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-10T07:14:10Z","receivedAt":"2014-11-10T07:14:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 09, 2014 at 02:49:58PM +0100, René Scharfe wrote:\n\n> -- >8 --\n> Subject: [PATCH] trailer: use CHILD_PROCESS_INIT in apply_command()\n> \n> Initialize the struct child_process variable cp at declaration time.\n> This is shorter, saves a function call and prevents using the variable\n> before initialization by mistake.\n> \n> Suggested-by: Jeff King <peff@peff.net>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  trailer.c | 3 +--\n>  1 file changed, 1 insertion(+), 2 deletions(-)\n\nThanks, both this one and the other you just sent (to use\nchild_process.args in more places) look good to me.\n\n-Peff\n"}]}