Converts the bundle.c code to use the string-list.c API, getting rid
of some duplication in the codebase, while doing that stop the bundle
command-line tool and its API from leaking memory in some common
cases.
Ævar Arnfjörð Bjarmason (3):
bundle cmd: stop leaking memory from parse_options_cmd_bundle()
bundle.c: use a temporary variable for OIDs and names
bundle: remove "ref_list" in favor of string-list.c API
builtin/bundle.c | 91 ++++++++++++++++++++++++++++++++----------------
bundle.c | 72 +++++++++++++++++++++-----------------
bundle.h | 20 +++++------
transport.c | 11 ++++--
4 files changed, 119 insertions(+), 75 deletions(-)
--
2.32.0.571.gdba276db2c
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 79 +++++++++++++++++++++++++++++++++---------------
1 file changed, 55 insertions(+), 24 deletions(-)
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,constchar*prefix,constchar*constusagestr[],conststructoptionoptions[],-constchar**bundle_file){+char**bundle_file){intnewargc;newargc=parse_options(argc,argv,NULL,options,usagestr,PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,8 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {intprogress=isatty(STDERR_FILENO);structstrvecpack_opts;intversion=-1;-+intdie_no_repo=0;+intret;structoptionoptions[]={OPT_SET_INT('q',"quiet",&progress,N_("do not show progress meter"),0),
@@ -76,7 +77,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {N_("specify bundle format version")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_create_usage,options,&bundle_file);
@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {if(progress&&all_progress_implied)strvec_push(&pack_opts,"--all-progress-implied");-if(!startup_info->have_repository)+if(!startup_info->have_repository){+die_no_repo=1;+gotocleanup;+}+ret=!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+cleanup:+free(bundle_file);+if(die_no_repo)die(_("Need a repository to create a bundle."));-return!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+returnret;}staticintcmd_bundle_verify(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;intquiet=0;-+intret;structoptionoptions[]={OPT_BOOL('q',"quiet",&quiet,N_("do not show bundle details")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_verify_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-if(verify_bundle(the_repository,&header,!quiet))-return1;+if(verify_bundle(the_repository,&header,!quiet)){+ret=1;+gotocleanup;+}+fprintf(stderr,_("%s is okay\n"),bundle_file);-return0;+ret=0;+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_list_heads(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_list_heads_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-return!!list_bundle_refs(&header,argc,argv);+ret=!!list_bundle_refs(&header,argc,argv);+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_unbundle(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intdie_no_repo=0;+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_unbundle_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;-if(!startup_info->have_repository)-die(_("Need a repository to unbundle."));-return!!unbundle(the_repository,&header,bundle_fd,0)||+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}+if(!startup_info->have_repository){+die_no_repo=1;+gotocleanup;+}+ret=!!unbundle(the_repository,&header,bundle_fd,0)||list_bundle_refs(&header,argc,argv);+cleanup:+if(die_no_repo)+die(_("Need a repository to unbundle."));+free(bundle_file);+returnret;}intcmd_bundle(intargc,constchar**argv,constchar*prefix)
Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.
That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)
We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.
Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.
Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 12 ++++-----
bundle.c | 64 ++++++++++++++++++++++++------------------------
bundle.h | 20 +++++++--------
transport.c | 10 +++++---
4 files changed, 55 insertions(+), 51 deletions(-)
@@ -79,7 +70,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,/* The bundle header ends with an empty line */while(!strbuf_getwholeline_fd(&buf,fd,'\n')&&buf.len&&buf.buf[0]!='\n'){-structobject_idoid;+structobject_id*oid;intis_prereq=0;constchar*p;
@@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(argc>1){intj;for(j=1;j<argc;j++)-if(!strcmp(r->list[i].name,argv[j]))+if(!strcmp(r->items[i].string,argv[j]))break;if(j==argc)continue;}-oid=&r->list[i].oid;-name=r->list[i].name;+oid=r->items[i].util;+name=r->items[i].string;printf("%s %s\n",oid_to_hex(oid),name);}return0;
@@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,*Dofastcheck,thenifanyprereqsaremissingthengolinebyline*tobeverboseabouttheerrors*/-structref_list*p=&header->prerequisites;+structstring_list*p=&header->prerequisites;structrev_inforevs;constchar*argv[]={NULL,"--all",NULL};structcommit*commit;
@@ -198,17 +192,17 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+structobject_id*oid=e->util;structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;-add_pending_object(&revs,o,e->name);+add_pending_object(&revs,o,e->string);continue;}if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(oid),e->name);+error("%s %s",oid_to_hex(oid),e->string);}if(revs.pending.nr!=p->nr)returnret;
@@ -224,28 +218,28 @@ int verify_bundle(struct repository *r,i--;for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+conststructobject_id*oid=e->util;structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)continue;if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(oid),e->name);+error("%s %s",oid_to_hex(oid),e->string);}/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+structobject_id*oid=e->util;commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}if(verbose){-structref_list*r;+structstring_list*r;r=&header->references;printf_ln(Q_("The bundle contains this ref:",
@@ -147,13 +147,14 @@ static struct ref *get_refs_from_bundle(struct transport *transport,transport->hash_algo=data->header.hash_algo;for(i=0;i<data->header.references.nr;i++){-structref_list_entry*e=data->header.references.list+i;-structref*ref=alloc_ref(e->name);-structobject_id*oid=&e->oid;+structstring_list_item*e=data->header.references.items+i;+structref*ref=alloc_ref(e->string);+conststructobject_id*oid=e->util;oidcpy(&ref->old_oid,oid);ref->next=result;result=ref;}+string_list_clear(&data->header.references,1);returnresult;}
@@ -176,6 +177,7 @@ static int close_bundle(struct transport *transport)structbundle_transport_data*data=transport->data;if(data->fd>0)close(data->fd);+bundle_header_release(&data->header);free(data);return0;}
@@ -1082,6 +1084,8 @@ struct transport *transport_get(struct remote *remote, const char *url)die(_("git-over-rsync is no longer supported"));}elseif(url_is_local_not_ssh(url)&&is_file(url)&&is_bundle(url,1)){structbundle_transport_data*data=xcalloc(1,sizeof(*data));+string_list_init(&data->header.prerequisites,1);+string_list_init(&data->header.references,1);transport_check_allowed("file");ret->data=data;ret->vtable=&bundle_vtable;
In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
bundle.c | 22 +++++++++++++++-------
transport.c | 3 ++-
2 files changed, 17 insertions(+), 8 deletions(-)
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)inti;for(i=0;i<r->nr;i++){+structobject_id*oid;+constchar*name;+if(argc>1){intj;for(j=1;j<argc;j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(j==argc)continue;}-printf("%s %s\n",oid_to_hex(&r->list[i].oid),-r->list[i].name);++oid=&r->list[i].oid;+name=r->list[i].name;+printf("%s %s\n",oid_to_hex(oid),name);}return0;}
@@ -194,7 +199,8 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;add_pending_object(&revs,o,e->name);
@@ -202,7 +208,7 @@ int verify_bundle(struct repository *r,}if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),e->name);}if(revs.pending.nr!=p->nr)returnret;
@@ -219,19 +225,21 @@ int verify_bundle(struct repository *r,for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)continue;if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),e->name);}/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-commit=lookup_commit_reference_gently(r,&e->oid,1);+structobject_id*oid=&e->oid;+commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}
@@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(argc>1){intj;for(j=1;j<argc;j++)-if(!strcmp(r->list[i].name,argv[j]))+if(!strcmp(r->items[i].string,argv[j]))break;if(j==argc)continue;}-oid=&r->list[i].oid;-name=r->list[i].name;+oid=r->items[i].util;+name=r->items[i].string;printf("%s %s\n",oid_to_hex(oid),name);
In function `list_refs` variable `name` that is used in a call to printf
has been extracted by the previous patch.
quoted hunk
}
return 0;
@@ -186,7 +180,7 @@ int verify_bundle(struct repository *r, * Do fast check, then if any prereqs are missing then go line by line * to be verbose about the errors */- struct ref_list *p = &header->prerequisites;+ struct string_list *p = &header->prerequisites; struct rev_info revs; const char *argv[] = {NULL, "--all", NULL}; struct commit *commit;
@@ -198,17 +192,17 @@ int verify_bundle(struct repository *r, repo_init_revisions(r, &revs, NULL); for (i = 0; i < p->nr; i++) {- struct ref_list_entry *e = p->list + i;- struct object_id *oid = &e->oid;+ struct string_list_item *e = p->items + i;+ struct object_id *oid = e->util; struct object *o = parse_object(r, oid); if (o) { o->flags |= PREREQ_MARK;- add_pending_object(&revs, o, e->name);+ add_pending_object(&revs, o, e->string); continue; } if (++ret == 1) error("%s", message);- error("%s %s", oid_to_hex(oid), e->name);+ error("%s %s", oid_to_hex(oid), e->string); } if (revs.pending.nr != p->nr) return ret;
@@ -224,28 +218,28 @@ int verify_bundle(struct repository *r, i--; for (i = 0; i < p->nr; i++) {- struct ref_list_entry *e = p->list + i;- struct object_id *oid = &e->oid;+ struct string_list_item *e = p->items + i;+ const struct object_id *oid = e->util; struct object *o = parse_object(r, oid); assert(o); /* otherwise we'd have returned early */ if (o->flags & SHOWN) continue; if (++ret == 1) error("%s", message);- error("%s %s", oid_to_hex(oid), e->name);+ error("%s %s", oid_to_hex(oid), e->string);
However, `e->name` in three places in function `verify_bundle` for two
different instances of `struct ref_list_entry *` wasn't extracted into
a variable by the previous patch. Could you please clarify this
discrepancy?
[snip]
In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
bundle.c | 26 ++++++++++++++++++--------
transport.c | 6 ++++--
2 files changed, 22 insertions(+), 10 deletions(-)
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)inti;for(i=0;i<r->nr;i++){+structobject_id*oid;+constchar*name;+if(argc>1){intj;for(j=1;j<argc;j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(j==argc)continue;}-printf("%s %s\n",oid_to_hex(&r->list[i].oid),-r->list[i].name);++oid=&r->list[i].oid;+name=r->list[i].name;+printf("%s %s\n",oid_to_hex(oid),name);}return0;}
@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+constchar*name=e->name;+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;-add_pending_object(&revs,o,e->name);+add_pending_object(&revs,o,name);continue;}if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),name);}if(revs.pending.nr!=p->nr)returnret;
@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+constchar*name=e->name;+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)continue;if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),name);}/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-commit=lookup_commit_reference_gently(r,&e->oid,1);+structobject_id*oid=&e->oid;+commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 79 +++++++++++++++++++++++++++++++++---------------
1 file changed, 55 insertions(+), 24 deletions(-)
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,constchar*prefix,constchar*constusagestr[],conststructoptionoptions[],-constchar**bundle_file){+char**bundle_file){intnewargc;newargc=parse_options(argc,argv,NULL,options,usagestr,PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,8 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {intprogress=isatty(STDERR_FILENO);structstrvecpack_opts;intversion=-1;-+intdie_no_repo=0;+intret;structoptionoptions[]={OPT_SET_INT('q',"quiet",&progress,N_("do not show progress meter"),0),
@@ -76,7 +77,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {N_("specify bundle format version")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_create_usage,options,&bundle_file);
@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {if(progress&&all_progress_implied)strvec_push(&pack_opts,"--all-progress-implied");-if(!startup_info->have_repository)+if(!startup_info->have_repository){+die_no_repo=1;+gotocleanup;+}+ret=!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+cleanup:+free(bundle_file);+if(die_no_repo)die(_("Need a repository to create a bundle."));-return!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+returnret;}staticintcmd_bundle_verify(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;intquiet=0;-+intret;structoptionoptions[]={OPT_BOOL('q',"quiet",&quiet,N_("do not show bundle details")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_verify_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-if(verify_bundle(the_repository,&header,!quiet))-return1;+if(verify_bundle(the_repository,&header,!quiet)){+ret=1;+gotocleanup;+}+fprintf(stderr,_("%s is okay\n"),bundle_file);-return0;+ret=0;+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_list_heads(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_list_heads_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-return!!list_bundle_refs(&header,argc,argv);+ret=!!list_bundle_refs(&header,argc,argv);+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_unbundle(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intdie_no_repo=0;+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_unbundle_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;-if(!startup_info->have_repository)-die(_("Need a repository to unbundle."));-return!!unbundle(the_repository,&header,bundle_fd,0)||+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}+if(!startup_info->have_repository){+die_no_repo=1;+gotocleanup;+}+ret=!!unbundle(the_repository,&header,bundle_fd,0)||list_bundle_refs(&header,argc,argv);+cleanup:+if(die_no_repo)+die(_("Need a repository to unbundle."));+free(bundle_file);+returnret;}intcmd_bundle(intargc,constchar**argv,constchar*prefix)
Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.
That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)
We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.
Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.
Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 12 +++++-----
bundle.c | 62 ++++++++++++++++++++++++------------------------
bundle.h | 20 ++++++++--------
transport.c | 10 +++++---
4 files changed, 54 insertions(+), 50 deletions(-)
@@ -79,7 +70,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,/* The bundle header ends with an empty line */while(!strbuf_getwholeline_fd(&buf,fd,'\n')&&buf.len&&buf.buf[0]!='\n'){-structobject_idoid;+structobject_id*oid;intis_prereq=0;constchar*p;
@@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(argc>1){intj;for(j=1;j<argc;j++)-if(!strcmp(r->list[i].name,argv[j]))+if(!strcmp(r->items[i].string,argv[j]))break;if(j==argc)continue;}-oid=&r->list[i].oid;-name=r->list[i].name;+oid=r->items[i].util;+name=r->items[i].string;printf("%s %s\n",oid_to_hex(oid),name);}return0;
@@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,*Dofastcheck,thenifanyprereqsaremissingthengolinebyline*tobeverboseabouttheerrors*/-structref_list*p=&header->prerequisites;+structstring_list*p=&header->prerequisites;structrev_inforevs;constchar*argv[]={NULL,"--all",NULL};structcommit*commit;
@@ -198,9 +192,9 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-constchar*name=e->name;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+constchar*name=e->string;+structobject_id*oid=e->util;structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;
@@ -225,9 +219,9 @@ int verify_bundle(struct repository *r,i--;for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-constchar*name=e->name;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+constchar*name=e->string;+conststructobject_id*oid=e->util;structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)
@@ -239,15 +233,15 @@ int verify_bundle(struct repository *r,/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+structobject_id*oid=e->util;commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}if(verbose){-structref_list*r;+structstring_list*r;r=&header->references;printf_ln(Q_("The bundle contains this ref:",
@@ -147,14 +147,15 @@ static struct ref *get_refs_from_bundle(struct transport *transport,transport->hash_algo=data->header.hash_algo;for(i=0;i<data->header.references.nr;i++){-structref_list_entry*e=data->header.references.list+i;-constchar*name=e->name;+structstring_list_item*e=data->header.references.items+i;+constchar*name=e->string;structref*ref=alloc_ref(name);-structobject_id*oid=&e->oid;+structobject_id*oid=e->util;oidcpy(&ref->old_oid,oid);ref->next=result;result=ref;}+string_list_clear(&data->header.references,1);returnresult;}
@@ -177,6 +178,7 @@ static int close_bundle(struct transport *transport)structbundle_transport_data*data=transport->data;if(data->fd>0)close(data->fd);+bundle_header_release(&data->header);free(data);return0;}
@@ -1083,6 +1085,8 @@ struct transport *transport_get(struct remote *remote, const char *url)die(_("git-over-rsync is no longer supported"));}elseif(url_is_local_not_ssh(url)&&is_file(url)&&is_bundle(url,1)){structbundle_transport_data*data=xcalloc(1,sizeof(*data));+string_list_init(&data->header.prerequisites,1);+string_list_init(&data->header.references,1);transport_check_allowed("file");ret->data=data;ret->vtable=&bundle_vtable;
From: Jeff King <hidden> Date: 2021-06-24 16:54:26
On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
Looking at that old commit, it seems like this is a good candidate for
just inserting a single UNLEAK(bundle_file) into cmd_bundle(). But it
looks like the allocation has now migrated into all of the individual
sub-command functions, so we have to deal with it multiple times. They
could still use UNLEAK() if you want to avoid the "ret = foo(); free();
return ret" dance in each one, though.
We should avoid UNLEAK() in library-ish functions, but sub-commands that
are just one step away from cmd_bundle() returning are OK uses, IMHO.
quoted hunk
@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) { if (progress && all_progress_implied) strvec_push(&pack_opts, "--all-progress-implied");- if (!startup_info->have_repository)+ if (!startup_info->have_repository) {+ die_no_repo = 1;+ goto cleanup;+ }+ ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);+cleanup:+ free(bundle_file);+ if (die_no_repo) die(_("Need a repository to create a bundle."));- return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);+ return ret; }
This die_no_repo stuff confused me at first. But I think you are trying
to make sure we call free(bundle_file) before die? There is no point in
spending any effort on that, I think. When we exit() via die(), the
variable is still on the stack, and hence not leaked. And there are
probably a zillion other places we can hit a die() inside
create_bundle() anyway, which would produce the same effect. There's not
much point treating this one specially.
-Peff
From: Jeff King <hidden> Date: 2021-06-24 17:11:29
On Mon, Jun 21, 2021 at 05:16:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.
That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)
I think this is a good direction, and I didn't see any errors in the
code. It's slightly sad that we end up with more lines than we started
with, but I think that's mostly because you're actually freeing the
memory now.
Two small nitpicks:
quoted hunk
@@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header, * Prerequisites have object name that is optionally * followed by SP and subject line. */- if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||+ oid = xmalloc(sizeof(struct object_id));+ if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) || (*p && !isspace(*p)) || (!is_prereq && !*p)) { if (report_path) error(_("unrecognized header: %s%s (%d)"), (is_prereq ? "-" : ""), buf.buf, (int)buf.len); status = -1;+ free(oid); break; } else {
This would be slightly simpler if you kept a local "struct object_id",
and then called:
string_list_append(list, string)->util = oiddup(&oid);
later when you know you want to save it. And then you don't have to
worry about the extra cleanup here. That does require an extra oidcpy()
under the hood, but I suspect that is lost in the noise.
I'm OK with it either way.
I'm usually a big fan of the ternary operator, and using variable
indirection to make it clear that we always call a function. But here I
think it makes things more confusing. The two sides of the if/else are
sufficiently simple that it's easy to see they both make the same
function call. And because there are two variables, we check is_prereq
twice, making it much harder to see the two cases.
I.e., I think:
if (is_prereq)
string_list_append(&header->prerequisites, "")->util = oid;
else
string_list_append(&header->references, p + 1)->util = oid;
is much more obvious.
-Peff
From: Jeff King <hidden> Date: 2021-06-24 17:14:14
On Mon, Jun 21, 2021 at 05:16:11PM +0200, Ævar Arnfjörð Bjarmason wrote:
This v2 addresses an omission noted by Andrei Rybak[1]. I didn't
factor out the "name" into a variable in 2/3 for ease of reading
3/3. That's now done.
This all looks OK to me. I left a few small comments on the patches
themselves.
The UNLEAK() thing I suggested for patch 1 does make that patch much
smaller and easier to read, but I suspect makes patch 3 harder (i.e.,
you are reusing the "cleanup" sections there to do the bundle header
release. That could _also_ get UNLEAKed, but at some point it becomes
more clear to actually clean up after ourselves, and I think patch 3
probably crosses that point). So I'm OK to ignore that.
I would prefer to see the "die()" thing I mentioned there addressed, as
well as the ternary thing from patch 3. But neither of them is incorrect
as-is; it's just a style/preference thing.
-Peff
On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
Looking at that old commit, it seems like this is a good candidate for
just inserting a single UNLEAK(bundle_file) into cmd_bundle(). But it
looks like the allocation has now migrated into all of the individual
sub-command functions, so we have to deal with it multiple times. They
could still use UNLEAK() if you want to avoid the "ret = foo(); free();
return ret" dance in each one, though.
We should avoid UNLEAK() in library-ish functions, but sub-commands that
are just one step away from cmd_bundle() returning are OK uses, IMHO.
I'll change it if you feel strongly about it, but I always read UNLEAK()
as "ok, this is too hard, we won't bother, it's just a one-off
built-in", and not necessarily a recommendation for a desired pattern.
I think it's nice to have e.g. valgrind be able to report no leaks in
the binaries we build by default, not just if you compile with
-DSUPPRESS_ANNOTATED_LEAKS.
quoted
@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) { if (progress && all_progress_implied) strvec_push(&pack_opts, "--all-progress-implied");- if (!startup_info->have_repository)+ if (!startup_info->have_repository) {+ die_no_repo = 1;+ goto cleanup;+ }+ ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);+cleanup:+ free(bundle_file);+ if (die_no_repo) die(_("Need a repository to create a bundle."));- return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);+ return ret; }
This die_no_repo stuff confused me at first. But I think you are trying
to make sure we call free(bundle_file) before die? There is no point in
spending any effort on that, I think. When we exit() via die(), the
variable is still on the stack, and hence not leaked. And there are
probably a zillion other places we can hit a die() inside
create_bundle() anyway, which would produce the same effect. There's not
much point treating this one specially.
Right, it's there just for the free(), and yeah, there's a bunch of
places we'll leak anyway.
I do think per the above with valgrind that there's value in making the
common non-dying codepaths not leak though.
On Mon, Jun 21, 2021 at 05:16:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.
That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)
I think this is a good direction, and I didn't see any errors in the
code. It's slightly sad that we end up with more lines than we started
with, but I think that's mostly because you're actually freeing the
memory now.
Two small nitpicks:
quoted
@@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header, * Prerequisites have object name that is optionally * followed by SP and subject line. */- if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||+ oid = xmalloc(sizeof(struct object_id));+ if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) || (*p && !isspace(*p)) || (!is_prereq && !*p)) { if (report_path) error(_("unrecognized header: %s%s (%d)"), (is_prereq ? "-" : ""), buf.buf, (int)buf.len); status = -1;+ free(oid); break; } else {
This would be slightly simpler if you kept a local "struct object_id",
and then called:
string_list_append(list, string)->util = oiddup(&oid);
later when you know you want to save it. And then you don't have to
worry about the extra cleanup here. That does require an extra oidcpy()
under the hood, but I suspect that is lost in the noise.
I'm OK with it either way.
I'm usually a big fan of the ternary operator, and using variable
indirection to make it clear that we always call a function. But here I
think it makes things more confusing. The two sides of the if/else are
sufficiently simple that it's easy to see they both make the same
function call. And because there are two variables, we check is_prereq
twice, making it much harder to see the two cases.
I.e., I think:
if (is_prereq)
string_list_append(&header->prerequisites, "")->util = oid;
else
string_list_append(&header->references, p + 1)->util = oid;
is much more obvious.
Hah, that's actually the exact code I wrote to begin with, before
thinking "hrm, someone will probably say I should just use a ternary
here". I'll change it back :)
From: Junio C Hamano <hidden> Date: 2021-06-29 01:04:23
Jeff King [off-list ref] writes:
I think this is a good direction, and I didn't see any errors in the
code. It's slightly sad that we end up with more lines than we started
with, but I think that's mostly because you're actually freeing the
memory now.
...
I.e., I think:
if (is_prereq)
string_list_append(&header->prerequisites, "")->util = oid;
else
string_list_append(&header->references, p + 1)->util = oid;
is much more obvious.
Nicely done and nicely reviewed and improved.
Together with the "no point in freeing just before dying" on the
earlier step, polishing this topic to incorporate the suggested
changes should be fairly an easy task. Let's not leave too many
loose ends hanging around and close this one with the last final
reroll (hopefully without "I did this too while at it" that meets
"oh, well, that is a bit controversi8al" to drag it unnecessarily
out).
Thanks.
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
An earlier version of this change went out of its way to not leak
memory on the die() codepaths here, but that was deemed too verbose to
worry about in a built-in that's dying anyway. The only reason we'd
need that is to appease a mode like SANITIZE=leak within the scope of
an entire test file.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 21 deletions(-)
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,constchar*prefix,constchar*constusagestr[],conststructoptionoptions[],-constchar**bundle_file){+char**bundle_file){intnewargc;newargc=parse_options(argc,argv,NULL,options,usagestr,PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {intprogress=isatty(STDERR_FILENO);structstrvecpack_opts;intversion=-1;-+intret;structoptionoptions[]={OPT_SET_INT('q',"quiet",&progress,N_("do not show progress meter"),0),
@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {N_("specify bundle format version")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_create_usage,options,&bundle_file);
@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {if(!startup_info->have_repository)die(_("Need a repository to create a bundle."));-return!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+ret=!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+free(bundle_file);+returnret;}staticintcmd_bundle_verify(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;intquiet=0;-+intret;structoptionoptions[]={OPT_BOOL('q',"quiet",&quiet,N_("do not show bundle details")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_verify_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-if(verify_bundle(the_repository,&header,!quiet))-return1;+if(verify_bundle(the_repository,&header,!quiet)){+ret=1;+gotocleanup;+}+fprintf(stderr,_("%s is okay\n"),bundle_file);-return0;+ret=0;+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_list_heads(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_list_heads_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-return!!list_bundle_refs(&header,argc,argv);+ret=!!list_bundle_refs(&header,argc,argv);+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_unbundle(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_unbundle_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}if(!startup_info->have_repository)die(_("Need a repository to unbundle."));-return!!unbundle(the_repository,&header,bundle_fd,0)||+ret=!!unbundle(the_repository,&header,bundle_fd,0)||list_bundle_refs(&header,argc,argv);+cleanup:+free(bundle_file);+returnret;}intcmd_bundle(intargc,constchar**argv,constchar*prefix)
In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
bundle.c | 26 ++++++++++++++++++--------
transport.c | 6 ++++--
2 files changed, 22 insertions(+), 10 deletions(-)
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)inti;for(i=0;i<r->nr;i++){+structobject_id*oid;+constchar*name;+if(argc>1){intj;for(j=1;j<argc;j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(j==argc)continue;}-printf("%s %s\n",oid_to_hex(&r->list[i].oid),-r->list[i].name);++oid=&r->list[i].oid;+name=r->list[i].name;+printf("%s %s\n",oid_to_hex(oid),name);}return0;}
@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+constchar*name=e->name;+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;-add_pending_object(&revs,o,e->name);+add_pending_object(&revs,o,name);continue;}if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),name);}if(revs.pending.nr!=p->nr)returnret;
@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+constchar*name=e->name;+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)continue;if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),name);}/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-commit=lookup_commit_reference_gently(r,&e->oid,1);+structobject_id*oid=&e->oid;+commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}
Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.
That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)
We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.
Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.
Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 12 +++++------
bundle.c | 53 ++++++++++++++++++++++++++----------------------
bundle.h | 21 ++++++++++---------
transport.c | 8 +++++---
4 files changed, 51 insertions(+), 43 deletions(-)
@@ -162,14 +167,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(argc>1){intj;for(j=1;j<argc;j++)-if(!strcmp(r->list[i].name,argv[j]))+if(!strcmp(r->items[i].string,argv[j]))break;if(j==argc)continue;}-oid=&r->list[i].oid;-name=r->list[i].name;+oid=r->items[i].util;+name=r->items[i].string;printf("%s %s\n",oid_to_hex(oid),name);}return0;
@@ -186,7 +191,7 @@ int verify_bundle(struct repository *r,*Dofastcheck,thenifanyprereqsaremissingthengolinebyline*tobeverboseabouttheerrors*/-structref_list*p=&header->prerequisites;+structstring_list*p=&header->prerequisites;structrev_inforevs;constchar*argv[]={NULL,"--all",NULL};structcommit*commit;
@@ -198,9 +203,9 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-constchar*name=e->name;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+constchar*name=e->string;+structobject_id*oid=e->util;structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;
@@ -225,9 +230,9 @@ int verify_bundle(struct repository *r,i--;for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-constchar*name=e->name;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+constchar*name=e->string;+conststructobject_id*oid=e->util;structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)
@@ -239,15 +244,15 @@ int verify_bundle(struct repository *r,/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+structobject_id*oid=e->util;commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}if(verbose){-structref_list*r;+structstring_list*r;r=&header->references;printf_ln(Q_("The bundle contains this ref:",
@@ -147,10 +147,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,transport->hash_algo=data->header.hash_algo;for(i=0;i<data->header.references.nr;i++){-structref_list_entry*e=data->header.references.list+i;-constchar*name=e->name;+structstring_list_item*e=data->header.references.items+i;+constchar*name=e->string;structref*ref=alloc_ref(name);-structobject_id*oid=&e->oid;+structobject_id*oid=e->util;oidcpy(&ref->old_oid,oid);ref->next=result;result=ref;
@@ -177,6 +177,7 @@ static int close_bundle(struct transport *transport)structbundle_transport_data*data=transport->data;if(data->fd>0)close(data->fd);+bundle_header_release(&data->header);free(data);return0;}
@@ -1083,6 +1084,7 @@ struct transport *transport_get(struct remote *remote, const char *url)die(_("git-over-rsync is no longer supported"));}elseif(url_is_local_not_ssh(url)&&is_file(url)&&is_bundle(url,1)){structbundle_transport_data*data=xcalloc(1,sizeof(*data));+bundle_header_init(&data->header);transport_check_allowed("file");ret->data=data;ret->vtable=&bundle_vtable;
From: Jeff King <hidden> Date: 2021-06-30 17:26:24
On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
Thanks, this looks good to me.
One thing, though...
An earlier version of this change went out of its way to not leak
memory on the die() codepaths here, but that was deemed too verbose to
worry about in a built-in that's dying anyway. The only reason we'd
need that is to appease a mode like SANITIZE=leak within the scope of
an entire test file.
Obviously you changed this as I asked, but this final sentence makes me
think we're not on the same page with respect to die(). I don't think
any kind of mode matters here. When we call die(), whatever we have on
the stack is _not_ a leak, by LSan's or valgrind's standards. Because we
still have access to those bytes. And nor can we ever get rid of such
cases. If we ever do:
void foo(const char *str)
{
char *x = xstrdup(str);
bar(x);
free(x);
}
void bar(const char *x)
{
if (!strcmp(x, "foo"))
die("whatever");
}
Then "x" will always still be allocated when we die(). We cannot free it
in bar(), where it is read-only. We cannot free it in foo() before we
call bar(), because it is needed there. But control never returns to the
free() statement.
So this code is perfectly fine and unavoidable. In the case you were
touching it was foo() that was calling die() directly, so we could work
around it with some conditionals. But from the leak-checker's
perspective the two cases are the same.
-Peff
From: Jeff King <hidden> Date: 2021-06-30 17:34:09
On Wed, Jun 30, 2021 at 04:06:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
Refactor the bundle API to use the string_list API instead of its own
version of a similar API. See [1] for v2.
Addresses comments by Jeff King about us being too overzelous in
trying not to leak memory (the 'die_no_repo' is gone), and other
flow/style comments of his.
I also added a bundle_header_init() function for use in transport.c,
and noticed a redundant call to string_list_clear() there.
Thanks, all three look good to me.
As an aside, having to have a separate bundle_header_init() and
BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
date with each other), but quite common in our code base. I wonder if
writing:
void bundle_header_init(struct bundle_header *header)
{
struct bundle_header blank = BUNDLE_HEADER_INIT;
memcpy(header, &blank, sizeof(*header));
}
would let a smart enough compiler just init "header" in place without
the extra copy (the performance of a single bundle_header almost
certainly doesn't matter, but it might for other types).
Just musing. ;)
-Peff
From: Jeff King <hidden> Date: 2021-06-30 17:45:14
On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:
As an aside, having to have a separate bundle_header_init() and
BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
date with each other), but quite common in our code base. I wonder if
writing:
void bundle_header_init(struct bundle_header *header)
{
struct bundle_header blank = BUNDLE_HEADER_INIT;
memcpy(header, &blank, sizeof(*header));
}
would let a smart enough compiler just init "header" in place without
the extra copy (the performance of a single bundle_header almost
certainly doesn't matter, but it might for other types).
Just musing. ;)
For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9
With "gcc -O2" the memcpy goes away and we init "header" directly.
If we want to start using this technique widely, I don't think it should
be part of your series, though. This probably applies to quite a few
data structures, so it would make more sense to have a series which
converts several.
-Peff
On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:
quoted
As an aside, having to have a separate bundle_header_init() and
BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
date with each other), but quite common in our code base. I wonder if
writing:
void bundle_header_init(struct bundle_header *header)
{
struct bundle_header blank = BUNDLE_HEADER_INIT;
memcpy(header, &blank, sizeof(*header));
}
would let a smart enough compiler just init "header" in place without
the extra copy (the performance of a single bundle_header almost
certainly doesn't matter, but it might for other types).
Just musing. ;)
For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9
With "gcc -O2" the memcpy goes away and we init "header" directly.
If we want to start using this technique widely, I don't think it should
be part of your series, though. This probably applies to quite a few
data structures, so it would make more sense to have a series which
converts several.
That's cool, yeah that would make quite a lot of code better. Thanks!
On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
Thanks, this looks good to me.
One thing, though...
quoted
An earlier version of this change went out of its way to not leak
memory on the die() codepaths here, but that was deemed too verbose to
worry about in a built-in that's dying anyway. The only reason we'd
need that is to appease a mode like SANITIZE=leak within the scope of
an entire test file.
Obviously you changed this as I asked, but this final sentence makes me
think we're not on the same page with respect to die(). I don't think
any kind of mode matters here. When we call die(), whatever we have on
the stack is _not_ a leak, by LSan's or valgrind's standards. Because we
still have access to those bytes. And nor can we ever get rid of such
cases. If we ever do:
void foo(const char *str)
{
char *x = xstrdup(str);
bar(x);
free(x);
}
void bar(const char *x)
{
if (!strcmp(x, "foo"))
die("whatever");
}
Then "x" will always still be allocated when we die(). We cannot free it
in bar(), where it is read-only. We cannot free it in foo() before we
call bar(), because it is needed there. But control never returns to the
free() statement.
So this code is perfectly fine and unavoidable. In the case you were
touching it was foo() that was calling die() directly, so we could work
around it with some conditionals. But from the leak-checker's
perspective the two cases are the same.
I've got you to blame for introducing SANITIZE=*. Now I've got these
leak checkers all mixed up :)
Yes you're right, FWIW I (re-)wrote this commit message just before
sending and should have said "a heap leak checker" instead of
"SANITIZE=leak", I really meant valgrind.
I originally ended with the "we are about to die" tracking because I was
tracing things with valgrind, which does complain about this sort of
thing. I.e.:
24 bytes in 1 blocks are still reachable in loss record 8 of 21
at 0x48356AF: malloc (vg_replace_malloc.c:298)
by 0x4837DE7: realloc (vg_replace_malloc.c:826)
by 0x3C06F1: xrealloc (wrapper.c:126)
by 0x380EC9: strbuf_grow (strbuf.c:98)
by 0x381A14: strbuf_add (strbuf.c:297)
by 0x20ADC5: strbuf_addstr (strbuf.h:304)
by 0x20B66D: prefix_filename (abspath.c:277)
by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55)
by 0x13D565: cmd_bundle_unbundle (bundle.c:170)
by 0x13D829: cmd_bundle (bundle.c:214)
by 0x1279F4: run_builtin (git.c:461)
by 0x127DFB: handle_builtin (git.c:714)
Re what I mentioned/asked in
https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was
experimenting with doing leak checking in the tests.
In this case we have 21 in total under --show-leak-kinds=all (and it was
20 in v2 of this series).
I've found that only including the file tho builtin is in cuts down on
it to a meaningful set of leaks. I.e. to throw out everything not
including /\bbundle\.c:/. We leak in a lot of things we call from
common-main.c, git.c, exec-cmd.c etc.
Maybe if we do end up with a test mode for this we shouldn't care about
checkers like valgrind and only cater to the SANITIZE=leak mode.
I'm still partial to the idea that we'll get the most win out of
something that we can run in the tests by default, i.e. we'll only need
to have a valgrind on the system & have someone run "make test" to run a
(limited set of) tests with this.
Whereas SANITIZE=leak is always a dev-only feature people may not have
on all the time.
On Wed, Jun 30 2021, Ævar Arnfjörð Bjarmason wrote:
On Wed, Jun 30 2021, Jeff King wrote:
quoted
On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:
quoted
As an aside, having to have a separate bundle_header_init() and
BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
date with each other), but quite common in our code base. I wonder if
writing:
void bundle_header_init(struct bundle_header *header)
{
struct bundle_header blank = BUNDLE_HEADER_INIT;
memcpy(header, &blank, sizeof(*header));
}
would let a smart enough compiler just init "header" in place without
the extra copy (the performance of a single bundle_header almost
certainly doesn't matter, but it might for other types).
Just musing. ;)
For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9
With "gcc -O2" the memcpy goes away and we init "header" directly.
If we want to start using this technique widely, I don't think it should
be part of your series, though. This probably applies to quite a few
data structures, so it would make more sense to have a series which
converts several.
That's cool, yeah that would make quite a lot of code better. Thanks!
From: Jeff King <hidden> Date: 2021-07-01 15:41:31
On Wed, Jun 30, 2021 at 08:00:50PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
So this code is perfectly fine and unavoidable. In the case you were
touching it was foo() that was calling die() directly, so we could work
around it with some conditionals. But from the leak-checker's
perspective the two cases are the same.
I've got you to blame for introducing SANITIZE=*. Now I've got these
leak checkers all mixed up :)
Yes you're right, FWIW I (re-)wrote this commit message just before
sending and should have said "a heap leak checker" instead of
"SANITIZE=leak", I really meant valgrind.
Ah, OK, that makes more sense.
I think the distinction here isn't "heap leak checker" (since valgrind
does still look at the whole memory space to find references), but
rather the treatment of "still reachable" items. I think LSan/ASan
ignore these entirely.
I originally ended with the "we are about to die" tracking because I was
tracing things with valgrind, which does complain about this sort of
thing. I.e.:
24 bytes in 1 blocks are still reachable in loss record 8 of 21
at 0x48356AF: malloc (vg_replace_malloc.c:298)
by 0x4837DE7: realloc (vg_replace_malloc.c:826)
by 0x3C06F1: xrealloc (wrapper.c:126)
by 0x380EC9: strbuf_grow (strbuf.c:98)
by 0x381A14: strbuf_add (strbuf.c:297)
by 0x20ADC5: strbuf_addstr (strbuf.h:304)
by 0x20B66D: prefix_filename (abspath.c:277)
by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55)
by 0x13D565: cmd_bundle_unbundle (bundle.c:170)
by 0x13D829: cmd_bundle (bundle.c:214)
by 0x1279F4: run_builtin (git.c:461)
by 0x127DFB: handle_builtin (git.c:714)
Re what I mentioned/asked in
https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was
experimenting with doing leak checking in the tests.
In this case we have 21 in total under --show-leak-kinds=all (and it was
20 in v2 of this series).
IMHO these "still reachable" leaks are not interesting. They'll fall
into one of two categories:
- allocations still on the stack when we exit() without unwinding.
These are always uninteresting, since by definition we are exiting
the process.
- globals that hang on to allocations (which will still be present
even if we exit the program by returning up through main()). Most of
these will be items we intend to last for the program length (e.g.,
fields in the_repository).
It's _possible_ that some of these could be interesting. E.g., a
program might have two phases: in the first, we rely on some
subsystem which uses global variables to cache some data (say,
parsed config values in remote.c or userdiff.c), and in the second
phase we no longer use that subsystem. We could be instructing the
subsystem to free up the memory after the first phase. But in
practice this is awkward, because the program-level code doesn't
know about subsystem allocations (or even which subsystems might
have been recursively triggered).
And while still-reachable tracking could be used to find
opportunities like this, I think there's a better approach to
finding these. If subsystems avoid global-variable caches and
instead stick their allocations into context structs, then the
memory is associated with those structs. So for example, if
userdiff attached its storage to a diff_options, which is attached
to a rev_info, then any "leaking" is all predicated on the rev_info
(which the main program either cleans up, or annotates with UNLEAK).
Maybe if we do end up with a test mode for this we shouldn't care about
checkers like valgrind and only cater to the SANITIZE=leak mode.
I do find SANITIZE=leak to be a more useful tool in general, but I'm not
at all against using valgrind if people find it convenient. I just think
"reachable" leaks are not interesting enough to be part of what we're
searching for when we run the test suite.
I'm still partial to the idea that we'll get the most win out of
something that we can run in the tests by default, i.e. we'll only need
to have a valgrind on the system & have someone run "make test" to run a
(limited set of) tests with this.
Whereas SANITIZE=leak is always a dev-only feature people may not have
on all the time.
That is exactly why I think "SANITIZE=leak" is better: it is easier for
people to run. valgrind is _extremely_ slow. It's also not available
everywhere; ASan/LSan aren't either, but they're pretty standard in
clang and gcc these days.
It is nice that valgrind can run on an un-instrumented binary, but I
sort of assume that anybody running "make test" will be able to build
the binary, since "make" is going to want to do that. I.e., I think
"make SANITIZE=leak test" is already doing what we want (we just need to
further annotate known-failures).
-Peff
Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).
As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
An earlier version of this change[1] went out of its way to not leak
memory on the die() codepaths here, but doing so will only avoid
reports of potential leaks under heap-only leak trackers such as
valgrind, not the SANITIZE=leak mode.
Avoiding those leaks as well might be useful to enable us to run
cleanly under the likes of valgrind in the future. But for now the
relative verbosity of the resulting code, and the fact that we don't
have some valgrind or SANITIZE=leak mode as part of our CI (it's only
run ad-hoc, see [2]), means we're not worrying about that for now.
1. https://lore.kernel.org/git/87v95vdxrc.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 21 deletions(-)
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,constchar*prefix,constchar*constusagestr[],conststructoptionoptions[],-constchar**bundle_file){+char**bundle_file){intnewargc;newargc=parse_options(argc,argv,NULL,options,usagestr,PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {intprogress=isatty(STDERR_FILENO);structstrvecpack_opts;intversion=-1;-+intret;structoptionoptions[]={OPT_SET_INT('q',"quiet",&progress,N_("do not show progress meter"),0),
@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {N_("specify bundle format version")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_create_usage,options,&bundle_file);
@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {if(!startup_info->have_repository)die(_("Need a repository to create a bundle."));-return!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+ret=!!create_bundle(the_repository,bundle_file,argc,argv,&pack_opts,version);+free(bundle_file);+returnret;}staticintcmd_bundle_verify(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;intquiet=0;-+intret;structoptionoptions[]={OPT_BOOL('q',"quiet",&quiet,N_("do not show bundle details")),OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_verify_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-if(verify_bundle(the_repository,&header,!quiet))-return1;+if(verify_bundle(the_repository,&header,!quiet)){+ret=1;+gotocleanup;+}+fprintf(stderr,_("%s is okay\n"),bundle_file);-return0;+ret=0;+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_list_heads(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_list_heads_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}close(bundle_fd);-return!!list_bundle_refs(&header,argc,argv);+ret=!!list_bundle_refs(&header,argc,argv);+cleanup:+free(bundle_file);+returnret;}staticintcmd_bundle_unbundle(intargc,constchar**argv,constchar*prefix){structbundle_headerheader;intbundle_fd=-1;-+intret;structoptionoptions[]={OPT_END()};-constchar*bundle_file;+char*bundle_file;argc=parse_options_cmd_bundle(argc,argv,prefix,builtin_bundle_unbundle_usage,options,&bundle_file);/* bundle internals use argv[1] as further parameters */memset(&header,0,sizeof(header));-if((bundle_fd=read_bundle_header(bundle_file,&header))<0)-return1;+if((bundle_fd=read_bundle_header(bundle_file,&header))<0){+ret=1;+gotocleanup;+}if(!startup_info->have_repository)die(_("Need a repository to unbundle."));-return!!unbundle(the_repository,&header,bundle_fd,0)||+ret=!!unbundle(the_repository,&header,bundle_fd,0)||list_bundle_refs(&header,argc,argv);+cleanup:+free(bundle_file);+returnret;}intcmd_bundle(intargc,constchar**argv,constchar*prefix)
This re-roll of v3 changes the discussion in the 1/3 commit message,
it incorrectly referred to SANITIZE=leak when I meant valgrind.
I also changed the bundle_header_init() pattern to use the same
"memcpy() a blank" as in my parallel series to do that more generally.
v3 at:
https://lore.kernel.org/git/cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (3):
bundle cmd: stop leaking memory from parse_options_cmd_bundle()
bundle.c: use a temporary variable for OIDs and names
bundle: remove "ref_list" in favor of string-list.c API
builtin/bundle.c | 74 ++++++++++++++++++++++++++++++------------------
bundle.c | 64 +++++++++++++++++++++++++----------------
bundle.h | 21 +++++++-------
transport.c | 10 +++++--
4 files changed, 104 insertions(+), 65 deletions(-)
Range-diff against v3:
1: 3d0d7a8e8b5 ! 1: 8e1d08113e5 bundle cmd: stop leaking memory from parse_options_cmd_bundle()
@@ Commit message
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.
- An earlier version of this change went out of its way to not leak
- memory on the die() codepaths here, but that was deemed too verbose to
- worry about in a built-in that's dying anyway. The only reason we'd
- need that is to appease a mode like SANITIZE=leak within the scope of
- an entire test file.
+ An earlier version of this change[1] went out of its way to not leak
+ memory on the die() codepaths here, but doing so will only avoid
+ reports of potential leaks under heap-only leak trackers such as
+ valgrind, not the SANITIZE=leak mode.
+
+ Avoiding those leaks as well might be useful to enable us to run
+ cleanly under the likes of valgrind in the future. But for now the
+ relative verbosity of the resulting code, and the fact that we don't
+ have some valgrind or SANITIZE=leak mode as part of our CI (it's only
+ run ad-hoc, see [2]), means we're not worrying about that for now.
+
+ 1. https://lore.kernel.org/git/87v95vdxrc.fsf@evledraar.gmail.com/
+ 2. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
2: e47646d3a98 = 2: 5ce376682b3 bundle.c: use a temporary variable for OIDs and names
3: f1066ee1b9a ! 3: 3e5972e4184 bundle: remove "ref_list" in favor of string-list.c API
@@ Commit message
Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.
+ In the bundle_header_init() function we're using a clever trick to
+ memcpy() what we'd get from the corresponding
+ BUNDLE_HEADER_INIT. There is a concurrent series to make use of that
+ pattern more generally, see [1].
+
+ 1. https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/
+
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## builtin/bundle.c ##
@@ bundle.c: static struct {
- oidcpy(&list->list[list->nr].oid, oid);
- list->list[list->nr].name = xstrdup(name);
- list->nr++;
-+ memset(header, 0, sizeof(*header));
-+ string_list_init(&header->prerequisites, 1);
-+ string_list_init(&header->references, 1);
++ struct bundle_header blank = BUNDLE_HEADER_INIT;
++ memcpy(header, &blank, sizeof(*header));
+}
+
+void bundle_header_release(struct bundle_header *header)
--
2.32.0.632.g49a94b9226d
In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
bundle.c | 26 ++++++++++++++++++--------
transport.c | 6 ++++--
2 files changed, 22 insertions(+), 10 deletions(-)
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)inti;for(i=0;i<r->nr;i++){+structobject_id*oid;+constchar*name;+if(argc>1){intj;for(j=1;j<argc;j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(j==argc)continue;}-printf("%s %s\n",oid_to_hex(&r->list[i].oid),-r->list[i].name);++oid=&r->list[i].oid;+name=r->list[i].name;+printf("%s %s\n",oid_to_hex(oid),name);}return0;}
@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+constchar*name=e->name;+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;-add_pending_object(&revs,o,e->name);+add_pending_object(&revs,o,name);continue;}if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),name);}if(revs.pending.nr!=p->nr)returnret;
@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-structobject*o=parse_object(r,&e->oid);+constchar*name=e->name;+structobject_id*oid=&e->oid;+structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)continue;if(++ret==1)error("%s",message);-error("%s %s",oid_to_hex(&e->oid),e->name);+error("%s %s",oid_to_hex(oid),name);}/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){structref_list_entry*e=p->list+i;-commit=lookup_commit_reference_gently(r,&e->oid,1);+structobject_id*oid=&e->oid;+commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}
Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.
That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)
We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.
Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.
Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.
In the bundle_header_init() function we're using a clever trick to
memcpy() what we'd get from the corresponding
BUNDLE_HEADER_INIT. There is a concurrent series to make use of that
pattern more generally, see [1].
1. https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bundle.c | 12 +++++------
bundle.c | 52 ++++++++++++++++++++++++++----------------------
bundle.h | 21 +++++++++----------
transport.c | 8 +++++---
4 files changed, 50 insertions(+), 43 deletions(-)
@@ -162,14 +166,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)if(argc>1){intj;for(j=1;j<argc;j++)-if(!strcmp(r->list[i].name,argv[j]))+if(!strcmp(r->items[i].string,argv[j]))break;if(j==argc)continue;}-oid=&r->list[i].oid;-name=r->list[i].name;+oid=r->items[i].util;+name=r->items[i].string;printf("%s %s\n",oid_to_hex(oid),name);}return0;
@@ -186,7 +190,7 @@ int verify_bundle(struct repository *r,*Dofastcheck,thenifanyprereqsaremissingthengolinebyline*tobeverboseabouttheerrors*/-structref_list*p=&header->prerequisites;+structstring_list*p=&header->prerequisites;structrev_inforevs;constchar*argv[]={NULL,"--all",NULL};structcommit*commit;
@@ -198,9 +202,9 @@ int verify_bundle(struct repository *r,repo_init_revisions(r,&revs,NULL);for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-constchar*name=e->name;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+constchar*name=e->string;+structobject_id*oid=e->util;structobject*o=parse_object(r,oid);if(o){o->flags|=PREREQ_MARK;
@@ -225,9 +229,9 @@ int verify_bundle(struct repository *r,i--;for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-constchar*name=e->name;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+constchar*name=e->string;+conststructobject_id*oid=e->util;structobject*o=parse_object(r,oid);assert(o);/* otherwise we'd have returned early */if(o->flags&SHOWN)
@@ -239,15 +243,15 @@ int verify_bundle(struct repository *r,/* Clean up objects used, as they will be reused. */for(i=0;i<p->nr;i++){-structref_list_entry*e=p->list+i;-structobject_id*oid=&e->oid;+structstring_list_item*e=p->items+i;+structobject_id*oid=e->util;commit=lookup_commit_reference_gently(r,oid,1);if(commit)clear_commit_marks(commit,ALL_REV_FLAGS);}if(verbose){-structref_list*r;+structstring_list*r;r=&header->references;printf_ln(Q_("The bundle contains this ref:",
@@ -147,10 +147,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,transport->hash_algo=data->header.hash_algo;for(i=0;i<data->header.references.nr;i++){-structref_list_entry*e=data->header.references.list+i;-constchar*name=e->name;+structstring_list_item*e=data->header.references.items+i;+constchar*name=e->string;structref*ref=alloc_ref(name);-structobject_id*oid=&e->oid;+structobject_id*oid=e->util;oidcpy(&ref->old_oid,oid);ref->next=result;result=ref;
@@ -177,6 +177,7 @@ static int close_bundle(struct transport *transport)structbundle_transport_data*data=transport->data;if(data->fd>0)close(data->fd);+bundle_header_release(&data->header);free(data);return0;}
@@ -1083,6 +1084,7 @@ struct transport *transport_get(struct remote *remote, const char *url)die(_("git-over-rsync is no longer supported"));}elseif(url_is_local_not_ssh(url)&&is_file(url)&&is_bundle(url,1)){structbundle_transport_data*data=xcalloc(1,sizeof(*data));+bundle_header_init(&data->header);transport_check_allowed("file");ret->data=data;ret->vtable=&bundle_vtable;
From: Jeff King <hidden> Date: 2021-07-03 10:52:52
On Fri, Jul 02, 2021 at 11:57:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
This re-roll of v3 changes the discussion in the 1/3 commit message,
it incorrectly referred to SANITIZE=leak when I meant valgrind.
I also changed the bundle_header_init() pattern to use the same
"memcpy() a blank" as in my parallel series to do that more generally.
Thanks, this looks good to me.
I'd probably word the discussion about die() a bit differently, but you've
already seen my expositions on leak-checking, and it's all tangent here.
So let's move forward with this, and we can let leak-checking
philosophies iron themselves out as we fix more cases. :)
-Peff
On Fri, Jul 02, 2021 at 11:57:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
This re-roll of v3 changes the discussion in the 1/3 commit message,
it incorrectly referred to SANITIZE=leak when I meant valgrind.
I also changed the bundle_header_init() pattern to use the same
"memcpy() a blank" as in my parallel series to do that more generally.
Thanks, this looks good to me.
I'd probably word the discussion about die() a bit differently, but you've
already seen my expositions on leak-checking, and it's all tangent here.
So let's move forward with this, and we can let leak-checking
philosophies iron themselves out as we fix more cases. :)
Thanks for the detailed review over multiple rounds.