{"thread":{"id":"42521","subject":"[PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'","startedAt":"2016-06-03T16:58:51Z","lastAt":"2016-06-06T10:03:56Z","messageCount":5,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"288271","messageId":"20160603165852.12399-1-chriscool@tuxfamily.org","threadId":"42521","inReplyTo":null,"subject":"[PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-06-03T16:58:51Z","receivedAt":"2016-06-03T16:58:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nWe cannot have a 'struct lock_file' allocated on the stack, as lockfile.c\nkeeps a linked list of all created lock_file structures.\n\nAlso 'struct apply_state' users might later want the same 'struct lock_file'\ninstance to be reused by different series of calls to the apply api.\n\nSo let's add a 'struct lock_file *lock_file' pointer into 'struct apply_state'\nand have the user of 'struct apply_state' allocate memory for the actual\n'struct lock_file' instance.\n\nLet's also add an argument to init_apply_state(), so that the caller can\neasily supply a pointer to the allocated instance.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Christian Couder \nSigned-off-by: Junio C Hamano \n---\n\nThis is to replace:\n\n\"[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'\"\n\nfrom the \"libify apply and use lib in am, part 1\" patch series.\n\nThe changes are the following:\n\n - The \"static struct lock_file lock_file\" is not removed anymore.\n\n - init_apply_state() doesn't allocate a 'struct lock_file' anymore\n when passed a NULL 'struct lock_file' pointer.\n\n - A check that state->lock_file is not NULL has been added in\n check_apply_state().\n\n - Title and commit message have been modified to reflect the above\n changes.\n\nThis 2 patch long patch series on top of the other unchanged commits\nfrom v3 is available here:\n\nhttps://github.com/chriscool/git/commits/libify-apply63\n\n builtin/apply.c | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 5027f1b..cc635eb 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -52,6 +52,12 @@ struct apply_state {\n \tconst char *prefix;\n \tint prefix_length;\n \n+\t/*\n+\t * Since lockfile.c keeps a linked list of all created\n+\t * lock_file structures, it isn't safe to free(lock_file).\n+\t */\n+\tstruct lock_file *lock_file;\n+\n \t/* These control what gets looked at and modified */\n \tint apply; /* this is not a dry-run */\n \tint cached; /* apply to the index only */\n@@ -4547,7 +4553,7 @@ static int apply_patch(struct apply_state *state,\n \n \tstate->update_index = state->check_index && state->apply;\n \tif (state->update_index && newfd < 0)\n-\t\tnewfd = hold_locked_index(&lock_file, 1);\n+\t\tnewfd = hold_locked_index(state->lock_file, 1);\n \n \tif (state->check_index) {\n \t\tif (read_cache() < 0)\n@@ -4648,11 +4654,14 @@ static int option_parse_directory(const struct option *opt,\n \treturn 0;\n }\n \n-static void init_apply_state(struct apply_state *state, const char *prefix)\n+static void init_apply_state(struct apply_state *state,\n+\t\t\t const char *prefix,\n+\t\t\t struct lock_file *lock_file)\n {\n \tmemset(state, 0, sizeof(*state));\n \tstate->prefix = prefix;\n \tstate->prefix_length = state->prefix ? strlen(state->prefix) : 0;\n+\tstate->lock_file = lock_file;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n \tstate->p_value = 1;\n@@ -4705,6 +4714,8 @@ static void check_apply_state(struct apply_state *state, int force_apply)\n \t}\n \tif (state->check_index)\n \t\tstate->unsafe_paths = 0;\n+\tif (!state->lock_file)\n+\t\tdie(\"BUG: state->lock_file should not be NULL\");\n }\n \n static int apply_all_patches(struct apply_state *state,\n@@ -4769,7 +4780,7 @@ static int apply_all_patches(struct apply_state *state,\n \t}\n \n \tif (state->update_index) {\n-\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, state->lock_file, COMMIT_LOCK))\n \t\t\tdie(_(\"Unable to write new index file\"));\n \t}\n \n@@ -4852,7 +4863,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tinit_apply_state(&state, prefix);\n+\tinit_apply_state(&state, prefix, &lock_file);\n \n \targc = parse_options(argc, argv, state.prefix, builtin_apply_options,\n \t\t\tapply_usage, 0);\n-- \n2.8.2.444.g74f55c9.dirty\n"},{"id":"288272","messageId":"20160603165852.12399-2-chriscool@tuxfamily.org","threadId":"42521","inReplyTo":"20160603165852.12399-1-chriscool@tuxfamily.org","subject":"[PATCH v4 2/2] builtin/apply: move 'newfd' global into 'struct apply_state'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-06-03T16:58:52Z","receivedAt":"2016-06-03T16:58:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nTo libify the apply functionality the 'newfd' variable should\nnot be static and global to the file. Let's move it into\n'struct apply_state'.\n\nSigned-off-by: Christian Couder \nSigned-off-by: Junio C Hamano \n---\n\nThis is to replace:\n\n[PATCH v3 49/49] builtin/apply: move 'newfd' global into 'struct apply_state'\n\nfrom the \"libify apply and use lib in am, part 1\" patch series.\n\nThere is no change compared to the previous version except that this\npatch should apply cleanly on top of PATCH v4 1/2.\n\n builtin/apply.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex cc635eb..7338ee6 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -57,6 +57,7 @@ struct apply_state {\n \t * lock_file structures, it isn't safe to free(lock_file).\n \t */\n \tstruct lock_file *lock_file;\n+\tint newfd;\n \n \t/* These control what gets looked at and modified */\n \tint apply; /* this is not a dry-run */\n@@ -120,8 +121,6 @@ struct apply_state {\n \tint applied_after_fixing_ws;\n };\n \n-static int newfd = -1;\n-\n static const char * const apply_usage[] = {\n \tN_(\"git apply [] [...]\"),\n \tNULL\n@@ -4552,8 +4551,8 @@ static int apply_patch(struct apply_state *state,\n \t\tstate->apply = 0;\n \n \tstate->update_index = state->check_index && state->apply;\n-\tif (state->update_index && newfd < 0)\n-\t\tnewfd = hold_locked_index(state->lock_file, 1);\n+\tif (state->update_index && state->newfd < 0)\n+\t\tstate->newfd = hold_locked_index(state->lock_file, 1);\n \n \tif (state->check_index) {\n \t\tif (read_cache() < 0)\n@@ -4662,6 +4661,7 @@ static void init_apply_state(struct apply_state *state,\n \tstate->prefix = prefix;\n \tstate->prefix_length = state->prefix ? strlen(state->prefix) : 0;\n \tstate->lock_file = lock_file;\n+\tstate->newfd = -1;\n \tstate->apply = 1;\n \tstate->line_termination = '\\n';\n \tstate->p_value = 1;\n@@ -4782,6 +4782,7 @@ static int apply_all_patches(struct apply_state *state,\n \tif (state->update_index) {\n \t\tif (write_locked_index(&the_index, state->lock_file, COMMIT_LOCK))\n \t\t\tdie(_(\"Unable to write new index file\"));\n+\t\tstate->newfd = -1;\n \t}\n \n \treturn !!errs;\n-- \n2.8.2.444.g74f55c9.dirty\n"},{"id":"288277","messageId":"CAP8UFD1Br15+TtD2kVOyeRExFJsDx3k4PD1Ab-xJaAhUHVbPXw@mail.gmail.com","threadId":"42521","inReplyTo":"20160603165852.12399-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-06-03T17:23:06Z","receivedAt":"2016-06-03T17:23:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 3, 2016 at 6:58 PM, Christian Couder\n wrote:\n> From: Christian Couder \n\nSorry for this spurious \"From:\" line.\nIt looks like send-email added it, and I don't understand why it does it now.\n"},{"id":"288281","messageId":"xmqq4m9ap2ub.fsf@gitster.mtv.corp.google.com","threadId":"42521","inReplyTo":"20160603165852.12399-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-03T18:03:08Z","receivedAt":"2016-06-03T18:03:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This is to replace:\n>\n> \"[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'\"\n>\n> from the \"libify apply and use lib in am, part 1\" patch series.\n\nThanks; will replace the tip 2 patches and requeue.\n\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 5027f1b..cc635eb 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -52,6 +52,12 @@ struct apply_state {\n> \tconst char *prefix;\n> \tint prefix_length;\n> \n> +\t/*\n> +\t * Since lockfile.c keeps a linked list of all created\n> +\t * lock_file structures, it isn't safe to free(lock_file).\n> +\t */\n> +\tstruct lock_file *lock_file;\n\nIs this even an issue for this thing anymore? It is the\nresponsibilty of the API user to manage the lifetime of what\nlock_file points at. The caller may have it on heap and let it\nleak, or it may have in BSS in which case it won't even dream about\nfreeing it.\n\nIf a comment were needed for this field, it should say \"when\ndiscarding/freeing apply_state, just discard this pointer without\ntouching what the pointer points at; the storage the pointer points\nat does not belong to the API implementation but belongs to the API\nuser\".\n\nBut I do not think such a comment is needed, because the situation\nis the same as other fields like *prefix, and for the same reason we\ndo not do anything to these fields in clear_apply_state().\n\nOther than that this looks great.\n"},{"id":"288413","messageId":"CAP8UFD3DAg02u95+227ZbsF=nQn+1jC_P7DAQUY9jaOwBoCk1w@mail.gmail.com","threadId":"42521","inReplyTo":"xmqq4m9ap2ub.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-06-06T10:03:56Z","receivedAt":"2016-06-06T10:03:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 3, 2016 at 8:03 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> This is to replace:\n>>\n>> \"[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'\"\n>>\n>> from the \"libify apply and use lib in am, part 1\" patch series.\n>\n> Thanks; will replace the tip 2 patches and requeue.\n>\n>> diff --git a/builtin/apply.c b/builtin/apply.c\n>> index 5027f1b..cc635eb 100644\n>> --- a/builtin/apply.c\n>> +++ b/builtin/apply.c\n>> @@ -52,6 +52,12 @@ struct apply_state {\n>> const char *prefix;\n>> int prefix_length;\n>>\n>> + /*\n>> + * Since lockfile.c keeps a linked list of all created\n>> + * lock_file structures, it isn't safe to free(lock_file).\n>> + */\n>> + struct lock_file *lock_file;\n>\n> Is this even an issue for this thing anymore? It is the\n> responsibilty of the API user to manage the lifetime of what\n> lock_file points at. The caller may have it on heap and let it\n> leak, or it may have in BSS in which case it won't even dream about\n> freeing it.\n>\n> If a comment were needed for this field, it should say \"when\n> discarding/freeing apply_state, just discard this pointer without\n> touching what the pointer points at; the storage the pointer points\n> at does not belong to the API implementation but belongs to the API\n> user\".\n>\n> But I do not think such a comment is needed, because the situation\n> is the same as other fields like *prefix, and for the same reason we\n> do not do anything to these fields in clear_apply_state().\n\nOk, I just resent without this comment.\n\nI still don't understand why there is an added:\n\nFrom: Christian Couder \n\nat the beginning of the emails.\n\n> Other than that this looks great.\n\nThanks,\nChristian.\n"}]}