Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return
Stefan Beller sbel...@google.com writes: On Fri, Mar 20, 2015 at 8:31 PM, Junio C Hamano gits...@pobox.com wrote: Stefan Beller sbel...@google.com writes: This frees `ce` would be leaking in the error path. At this point ce is not yet added to the index, so it is clear it is safe to free it---otherwise we will leak it. Good. Additionally a free is moved towards the return. I am on the fence on this one between two schools and do not have a strong preference. One school is to free as soon as you know you do not need it, which is a valid stance to take. Another is, as you did, not to care about the minimum necessary lifetime of the storage and free them all at the end, which is also valid. Technically, the former could be more performant while the latter is easier on the eyes. I only recall to have seen the latter school so far, which is why I made the change in the first place assuming the school of freeing ASAP has no strong supporters inside the git community. I can resend the patch dropping the reordering, if you prefer. I already said that I do not have a preference ;-) Will queue 3/15 as-is, drop 2/15 and wait on 1/15. -- 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
[PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return
This frees `ce` would be leaking in the error path.
Additionally a free is moved towards the return. This helps code
readability as we often have this pattern of freeing resources just
before return/exit and not in between the code.
Signed-off-by: Stefan Beller sbel...@google.com
---
read-cache.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 769897e..935f91a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -681,15 +681,18 @@ int add_to_index(struct index_state *istate, const char
*path, struct stat *st,
alias = index_file_exists(istate, ce-name, ce_namelen(ce),
ignore_case);
if (alias !ce_stage(alias) !ie_match_stat(istate, alias, st,
ce_option)) {
/* Nothing changed, really */
- free(ce);
if (!S_ISGITLINK(alias-ce_mode))
ce_mark_uptodate(alias);
alias-ce_flags |= CE_ADDED;
+
+ free(ce);
return 0;
}
if (!intent_only) {
- if (index_path(ce-sha1, path, st, HASH_WRITE_OBJECT))
+ if (index_path(ce-sha1, path, st, HASH_WRITE_OBJECT)) {
+ free(ce);
return error(unable to index file %s, path);
+ }
} else
set_object_name_for_intent_to_add_entry(ce);
--
2.3.0.81.gc37f363
--
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
Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return
Stefan Beller sbel...@google.com writes: This frees `ce` would be leaking in the error path. At this point ce is not yet added to the index, so it is clear it is safe to free it---otherwise we will leak it. Good. Additionally a free is moved towards the return. I am on the fence on this one between two schools and do not have a strong preference. One school is to free as soon as you know you do not need it, which is a valid stance to take. Another is, as you did, not to care about the minimum necessary lifetime of the storage and free them all at the end, which is also valid. Technically, the former could be more performant while the latter is easier on the eyes. -- 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
Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return
On Fri, Mar 20, 2015 at 8:31 PM, Junio C Hamano gits...@pobox.com wrote: Stefan Beller sbel...@google.com writes: This frees `ce` would be leaking in the error path. At this point ce is not yet added to the index, so it is clear it is safe to free it---otherwise we will leak it. Good. Additionally a free is moved towards the return. I am on the fence on this one between two schools and do not have a strong preference. One school is to free as soon as you know you do not need it, which is a valid stance to take. Another is, as you did, not to care about the minimum necessary lifetime of the storage and free them all at the end, which is also valid. Technically, the former could be more performant while the latter is easier on the eyes. I only recall to have seen the latter school so far, which is why I made the change in the first place assuming the school of freeing ASAP has no strong supporters inside the git community. I can resend the patch dropping the reordering, if you prefer. -- 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
