Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string
On Thu, Jun 26, 2014 at 4:09 AM, Tanay Abhra wrote: > > On 6/25/2014 10:15 AM, Eric Sunshine wrote: >> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote: >>> diff --git a/branch.c b/branch.c >>> index 660097b..c9a2a0d 100644 >>> --- a/branch.c >>> +++ b/branch.c >>> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const >>> char *orig_ref, >>> int read_branch_desc(struct strbuf *buf, const char *branch_name) >>> { >>> - struct branch_desc_cb cb; >>> + const char *value = NULL; >>> + struct branch_desc desc; >>> struct strbuf name = STRBUF_INIT; >>> strbuf_addf(&name, "branch.%s.description", branch_name); >>> - cb.config_name = name.buf; >>> - cb.value = NULL; >>> - if (git_config(read_branch_desc_cb, &cb) < 0) { >>> + desc.config_name = name.buf; >>> + desc.value = NULL; >>> + git_config_get_string(desc.config_name, &value); >>> + if (git_config_string(&desc.value, desc.config_name, value) < 0) { >> >> Although it works in this case, it's somewhat ugly that you ignore the >> return value of git_config_get_string(), and a person reading the code >> has to spend extra time digging into git_config_string() to figure out >> why this is safe. If might be clearer for future readers by rephrasing >> like this: >> >> if (git_config_get_string(desc.config_name, &value) < 0 || >> git_config_string(&desc.value, desc.config_name, value) < 0) { >> > > Noted, also didn't the old code leak desc.value as it was xstrduped > by git_config_string()? Thanks for the review. Looks that way. -- 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: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string
On 6/25/2014 10:15 AM, Eric Sunshine wrote: > On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote: >> diff --git a/branch.c b/branch.c >> index 660097b..c9a2a0d 100644 >> --- a/branch.c >> +++ b/branch.c >> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const >> char *orig_ref, >> return 0; >> } >> >> -struct branch_desc_cb { >> +struct branch_desc { >> const char *config_name; >> const char *value; >> }; > > What is the purpose of retaining this structure? Following your > changes, it is never used outside of read_branch_desc(), and > 'config_name' and 'value' would be more naturally declared as > variables local to that function. Done. :) > >> -static int read_branch_desc_cb(const char *var, const char *value, void *cb) >> -{ >> - struct branch_desc_cb *desc = cb; >> - if (strcmp(desc->config_name, var)) >> - return 0; >> - free((char *)desc->value); >> - return git_config_string(&desc->value, var, value); >> -} >> - >> int read_branch_desc(struct strbuf *buf, const char *branch_name) >> { >> - struct branch_desc_cb cb; >> + const char *value = NULL; >> + struct branch_desc desc; >> struct strbuf name = STRBUF_INIT; >> strbuf_addf(&name, "branch.%s.description", branch_name); >> - cb.config_name = name.buf; >> - cb.value = NULL; >> - if (git_config(read_branch_desc_cb, &cb) < 0) { >> + desc.config_name = name.buf; >> + desc.value = NULL; >> + git_config_get_string(desc.config_name, &value); >> + if (git_config_string(&desc.value, desc.config_name, value) < 0) { > > Although it works in this case, it's somewhat ugly that you ignore the > return value of git_config_get_string(), and a person reading the code > has to spend extra time digging into git_config_string() to figure out > why this is safe. If might be clearer for future readers by rephrasing > like this: > > if (git_config_get_string(desc.config_name, &value) < 0 || > git_config_string(&desc.value, desc.config_name, value) < 0) { > Noted, also didn't the old code leak desc.value as it was xstrduped by git_config_string()? Thanks for the review. >> strbuf_release(&name); >> return -1; >> } >> - if (cb.value) >> - strbuf_addstr(buf, cb.value); >> + strbuf_addstr(buf, desc.value); >> strbuf_release(&name); >> return 0; >> } >> -- >> 1.9.0.GIT > -- 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: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote: > Use git_config_get_string instead of git_config to take advantage of > the config hash-table api which provides a cleaner control flow. > > Signed-off-by: Tanay Abhra > --- > branch.c | 24 > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/branch.c b/branch.c > index 660097b..c9a2a0d 100644 > --- a/branch.c > +++ b/branch.c > @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const > char *orig_ref, > return 0; > } > > -struct branch_desc_cb { > +struct branch_desc { > const char *config_name; > const char *value; > }; What is the purpose of retaining this structure? Following your changes, it is never used outside of read_branch_desc(), and 'config_name' and 'value' would be more naturally declared as variables local to that function. > -static int read_branch_desc_cb(const char *var, const char *value, void *cb) > -{ > - struct branch_desc_cb *desc = cb; > - if (strcmp(desc->config_name, var)) > - return 0; > - free((char *)desc->value); > - return git_config_string(&desc->value, var, value); > -} > - > int read_branch_desc(struct strbuf *buf, const char *branch_name) > { > - struct branch_desc_cb cb; > + const char *value = NULL; > + struct branch_desc desc; > struct strbuf name = STRBUF_INIT; > strbuf_addf(&name, "branch.%s.description", branch_name); > - cb.config_name = name.buf; > - cb.value = NULL; > - if (git_config(read_branch_desc_cb, &cb) < 0) { > + desc.config_name = name.buf; > + desc.value = NULL; > + git_config_get_string(desc.config_name, &value); > + if (git_config_string(&desc.value, desc.config_name, value) < 0) { Although it works in this case, it's somewhat ugly that you ignore the return value of git_config_get_string(), and a person reading the code has to spend extra time digging into git_config_string() to figure out why this is safe. If might be clearer for future readers by rephrasing like this: if (git_config_get_string(desc.config_name, &value) < 0 || git_config_string(&desc.value, desc.config_name, value) < 0) { > strbuf_release(&name); > return -1; > } > - if (cb.value) > - strbuf_addstr(buf, cb.value); > + strbuf_addstr(buf, desc.value); > strbuf_release(&name); > return 0; > } > -- > 1.9.0.GIT -- 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