| Message ID | 20220124204442.39353-7-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
Glen Choo <chooglen@google.com> writes: > Signed-off-by: Glen Choo <chooglen@google.com> > --- > branch.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/branch.c b/branch.c > index be33fe09fa..1e9a585633 100644 > --- a/branch.c > +++ b/branch.c > @@ -239,7 +239,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > if (track != BRANCH_TRACK_INHERIT) > for_each_remote(find_tracked_branch, &tracking); > else if (inherit_tracking(&tracking, orig_ref)) > - return; > + goto cleanup; > > if (!tracking.matches) > switch (track) { > @@ -249,7 +249,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > case BRANCH_TRACK_INHERIT: > break; > default: > - return; > + goto cleanup; > } > > if (tracking.matches > 1) > @@ -262,6 +262,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > tracking.remote, tracking.srcs) < 0) > exit(-1); > > +cleanup: > string_list_clear(tracking.srcs, 0); Makes sense. There is no other exit paths out of the function after the tracking_srcs variable gets initialized, so this should be covering everything. Two tangential findings: * I see exit(-1) in the precontext of the final hunk. We probably would want to fix it, as negative argument to exit(3) is misleading (the standard makes it clear that only the least significant 8 bits matter, so it is not that bad). * At the end, what is cleared is tracking.srcs, but because it is a pointer to the real resource we allocated on our stack, it would be cleaner to pass &tracking_srcs instead. Both are not what this patch introduces, and are outside the scope of this series. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >> Signed-off-by: Glen Choo <chooglen@google.com> >> --- >> branch.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/branch.c b/branch.c >> index be33fe09fa..1e9a585633 100644 >> --- a/branch.c >> +++ b/branch.c >> @@ -239,7 +239,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, >> if (track != BRANCH_TRACK_INHERIT) >> for_each_remote(find_tracked_branch, &tracking); >> else if (inherit_tracking(&tracking, orig_ref)) >> - return; >> + goto cleanup; >> >> if (!tracking.matches) >> switch (track) { >> @@ -249,7 +249,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, >> case BRANCH_TRACK_INHERIT: >> break; >> default: >> - return; >> + goto cleanup; >> } >> >> if (tracking.matches > 1) >> @@ -262,6 +262,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, >> tracking.remote, tracking.srcs) < 0) >> exit(-1); >> >> +cleanup: >> string_list_clear(tracking.srcs, 0); > > Makes sense. There is no other exit paths out of the function after > the tracking_srcs variable gets initialized, so this should be > covering everything. > > Two tangential findings: > > * I see exit(-1) in the precontext of the final hunk. We probably > would want to fix it, as negative argument to exit(3) is > misleading (the standard makes it clear that only the least > significant 8 bits matter, so it is not that bad). Thanks for the reminder. We had this discussion previously and the conclusion was that I would send this cleanup as a separate series [1]. Unlike this relatively obvious cleanup, I think exit codes might spark a more involved discussion. > > * At the end, what is cleared is tracking.srcs, but because it is a > pointer to the real resource we allocated on our stack, it would > be cleaner to pass &tracking_srcs instead. Ah yes, that is true. I'll do that. If you (or others) prefer, I can move this patch to its own series, or possibly as part of a grab bag with the exit code fix. > > Both are not what this patch introduces, and are outside the scope > of this series. > > Thanks. [1] https://lore.kernel.org/git/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com
diff --git a/branch.c b/branch.c index be33fe09fa..1e9a585633 100644 --- a/branch.c +++ b/branch.c @@ -239,7 +239,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, if (track != BRANCH_TRACK_INHERIT) for_each_remote(find_tracked_branch, &tracking); else if (inherit_tracking(&tracking, orig_ref)) - return; + goto cleanup; if (!tracking.matches) switch (track) { @@ -249,7 +249,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, case BRANCH_TRACK_INHERIT: break; default: - return; + goto cleanup; } if (tracking.matches > 1) @@ -262,6 +262,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, tracking.remote, tracking.srcs) < 0) exit(-1); +cleanup: string_list_clear(tracking.srcs, 0); }
Signed-off-by: Glen Choo <chooglen@google.com> --- branch.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)