[PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Christian Couder]

From: Christian Couder <christian.couder@gmail.com>

We cannot have a 'struct lock_file' allocated on the stack, as lockfile.c
keeps a linked list of all created lock_file structures.

Also 'struct apply_state' users might later want the same 'struct lock_file'
instance to be reused by different series of calls to the apply api.

So let's add a 'struct lock_file *lock_file' pointer into 'struct apply_state'
and have the user of 'struct apply_state' allocate memory for the actual
'struct lock_file' instance.

Let's also add an argument to init_apply_state(), so that the caller can
easily supply a pointer to the allocated instance.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

This is to replace:

"[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'"

from the "libify apply and use lib in am, part 1" patch series.

The changes are the following:

  - The "static struct lock_file lock_file" is not removed anymore.

  - init_apply_state() doesn't allocate a 'struct lock_file' anymore
    when passed a NULL 'struct lock_file' pointer.

  - A check that state->lock_file is not NULL has been added in
    check_apply_state().

  - Title and commit message have been modified to reflect the above
    changes.

This 2 patch long patch series on top of the other unchanged commits
from v3 is available here:

https://github.com/chriscool/git/commits/libify-apply63

 builtin/apply.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 5027f1b..cc635eb 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -52,6 +52,12 @@ struct apply_state {
 	const char *prefix;
 	int prefix_length;
 
+	/*
+	 * Since lockfile.c keeps a linked list of all created
+	 * lock_file structures, it isn't safe to free(lock_file).
+	 */
+	struct lock_file *lock_file;
+
 	/* These control what gets looked at and modified */
 	int apply; /* this is not a dry-run */
 	int cached; /* apply to the index only */
@@ -4547,7 +4553,7 @@ static int apply_patch(struct apply_state *state,
 
 	state->update_index = state->check_index && state->apply;
 	if (state->update_index && newfd < 0)
-		newfd = hold_locked_index(&lock_file, 1);
+		newfd = hold_locked_index(state->lock_file, 1);
 
 	if (state->check_index) {
 		if (read_cache() < 0)
@@ -4648,11 +4654,14 @@ static int option_parse_directory(const struct option *opt,
 	return 0;
 }
 
-static void init_apply_state(struct apply_state *state, const char *prefix)
+static void init_apply_state(struct apply_state *state,
+			     const char *prefix,
+			     struct lock_file *lock_file)
 {
 	memset(state, 0, sizeof(*state));
 	state->prefix = prefix;
 	state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
+	state->lock_file = lock_file;
 	state->apply = 1;
 	state->line_termination = '\n';
 	state->p_value = 1;
@@ -4705,6 +4714,8 @@ static void check_apply_state(struct apply_state *state, int force_apply)
 	}
 	if (state->check_index)
 		state->unsafe_paths = 0;
+	if (!state->lock_file)
+		die("BUG: state->lock_file should not be NULL");
 }
 
 static int apply_all_patches(struct apply_state *state,
@@ -4769,7 +4780,7 @@ static int apply_all_patches(struct apply_state *state,
 	}
 
 	if (state->update_index) {
-		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+		if (write_locked_index(&the_index, state->lock_file, COMMIT_LOCK))
 			die(_("Unable to write new index file"));
 	}
 
@@ -4852,7 +4863,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	init_apply_state(&state, prefix);
+	init_apply_state(&state, prefix, &lock_file);
 
 	argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
 			apply_usage, 0);
-- 
2.8.2.444.g74f55c9.dirty

[PATCH v4 2/2] builtin/apply: move 'newfd' global into 'struct apply_state'

[Christian Couder]

From: Christian Couder <christian.couder@gmail.com>

To libify the apply functionality the 'newfd' variable should
not be static and global to the file. Let's move it into
'struct apply_state'.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

This is to replace:

[PATCH v3 49/49] builtin/apply: move 'newfd' global into 'struct apply_state'

from the "libify apply and use lib in am, part 1" patch series.

There is no change compared to the previous version except that this
patch should apply cleanly on top of PATCH v4 1/2.

 builtin/apply.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index cc635eb..7338ee6 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -57,6 +57,7 @@ struct apply_state {
 	 * lock_file structures, it isn't safe to free(lock_file).
 	 */
 	struct lock_file *lock_file;
+	int newfd;
 
 	/* These control what gets looked at and modified */
 	int apply; /* this is not a dry-run */
@@ -120,8 +121,6 @@ struct apply_state {
 	int applied_after_fixing_ws;
 };
 
-static int newfd = -1;
-
 static const char * const apply_usage[] = {
 	N_("git apply [<options>] [<patch>...]"),
 	NULL
@@ -4552,8 +4551,8 @@ static int apply_patch(struct apply_state *state,
 		state->apply = 0;
 
 	state->update_index = state->check_index && state->apply;
-	if (state->update_index && newfd < 0)
-		newfd = hold_locked_index(state->lock_file, 1);
+	if (state->update_index && state->newfd < 0)
+		state->newfd = hold_locked_index(state->lock_file, 1);
 
 	if (state->check_index) {
 		if (read_cache() < 0)
@@ -4662,6 +4661,7 @@ static void init_apply_state(struct apply_state *state,
 	state->prefix = prefix;
 	state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
 	state->lock_file = lock_file;
+	state->newfd = -1;
 	state->apply = 1;
 	state->line_termination = '\n';
 	state->p_value = 1;
@@ -4782,6 +4782,7 @@ static int apply_all_patches(struct apply_state *state,
 	if (state->update_index) {
 		if (write_locked_index(&the_index, state->lock_file, COMMIT_LOCK))
 			die(_("Unable to write new index file"));
+		state->newfd = -1;
 	}
 
 	return !!errs;
-- 
2.8.2.444.g74f55c9.dirty

Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Christian Couder]

On Fri, Jun 3, 2016 at 6:58 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> From: Christian Couder <christian.couder@gmail.com>

Sorry for this spurious "From:" line.
It looks like send-email added it, and I don't understand why it does it now.

Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> This is to replace:
>
> "[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'"
>
> from the "libify apply and use lib in am, part 1" patch series.

Thanks; will replace the tip 2 patches and requeue.

> diff --git a/builtin/apply.c b/builtin/apply.c
> index 5027f1b..cc635eb 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -52,6 +52,12 @@ struct apply_state {
>  	const char *prefix;
>  	int prefix_length;
>  
> +	/*
> +	 * Since lockfile.c keeps a linked list of all created
> +	 * lock_file structures, it isn't safe to free(lock_file).
> +	 */
> +	struct lock_file *lock_file;

Is this even an issue for this thing anymore?  It is the
responsibilty of the API user to manage the lifetime of what
lock_file points at.  The caller may have it on heap and let it
leak, or it may have in BSS in which case it won't even dream about
freeing it.

If a comment were needed for this field, it should say "when
discarding/freeing apply_state, just discard this pointer without
touching what the pointer points at; the storage the pointer points
at does not belong to the API implementation but belongs to the API
user".

But I do not think such a comment is needed, because the situation
is the same as other fields like *prefix, and for the same reason we
do not do anything to these fields in clear_apply_state().

Other than that this looks great.

Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Christian Couder]

On Fri, Jun 3, 2016 at 8:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> This is to replace:
>>
>> "[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'"
>>
>> from the "libify apply and use lib in am, part 1" patch series.
>
> Thanks; will replace the tip 2 patches and requeue.
>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index 5027f1b..cc635eb 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -52,6 +52,12 @@ struct apply_state {
>>       const char *prefix;
>>       int prefix_length;
>>
>> +     /*
>> +      * Since lockfile.c keeps a linked list of all created
>> +      * lock_file structures, it isn't safe to free(lock_file).
>> +      */
>> +     struct lock_file *lock_file;
>
> Is this even an issue for this thing anymore?  It is the
> responsibilty of the API user to manage the lifetime of what
> lock_file points at.  The caller may have it on heap and let it
> leak, or it may have in BSS in which case it won't even dream about
> freeing it.
>
> If a comment were needed for this field, it should say "when
> discarding/freeing apply_state, just discard this pointer without
> touching what the pointer points at; the storage the pointer points
> at does not belong to the API implementation but belongs to the API
> user".
>
> But I do not think such a comment is needed, because the situation
> is the same as other fields like *prefix, and for the same reason we
> do not do anything to these fields in clear_apply_state().

Ok, I just resent without this comment.

I still don't understand why there is an added:

From: Christian Couder <christian.couder@gmail.com>

at the beginning of the emails.

> Other than that this looks great.

Thanks,
Christian.