| Message ID | patch-1.1-93adb856b0c-20210909T012244Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gc: remove unused launchctl_get_uid() call | expand |
Hi Ævar, On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote: > When the launchctl_boot_plist() function was added in > a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is > registered, 2021-08-24), an unused call to launchctl_get_uid() was > added along with it. That call appears to have been copy/pasted from > launchctl_boot_plist(). > > Since we can remove that, we can also get rid of the "result" > variable, whose only purpose was allow for the free() between its > assignment and the return. That pattern also appears to have been > copy/pasted from launchctl_boot_plist(). I don't find the most crucial information in that commit message: what is the fall-out of the removal of this call? Such an analysis (_with_ a summary of it in the commit message) is definitely required. And it should not be left as an exercise for the reader. Ciao, Johannes
On Thu, Sep 09 2021, Johannes Schindelin wrote: > Hi Ævar, > > On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote: > >> When the launchctl_boot_plist() function was added in >> a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is >> registered, 2021-08-24), an unused call to launchctl_get_uid() was >> added along with it. That call appears to have been copy/pasted from >> launchctl_boot_plist(). >> >> Since we can remove that, we can also get rid of the "result" >> variable, whose only purpose was allow for the free() between its >> assignment and the return. That pattern also appears to have been >> copy/pasted from launchctl_boot_plist(). > > I don't find the most crucial information in that commit message: what is > the fall-out of the removal of this call? > > Such an analysis (_with_ a summary of it in the commit message) is > definitely required. And it should not be left as an exercise for the > reader. Do you mean an assurance to the reader that the removed code doesn't have any side-effects? E.g. an addition of As the patch shows the returned value wasn't used at all in this function, the launchctl_get_uid() function itself just calls xstrfmt() and getuid(), neither of which have any subtle global side-effects, so this removal is safe. ?
Hi Ævar, On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Sep 09 2021, Johannes Schindelin wrote: > > > Hi Ævar, > > > > On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote: > > > >> When the launchctl_boot_plist() function was added in > >> a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is > >> registered, 2021-08-24), an unused call to launchctl_get_uid() was > >> added along with it. That call appears to have been copy/pasted from > >> launchctl_boot_plist(). > >> > >> Since we can remove that, we can also get rid of the "result" > >> variable, whose only purpose was allow for the free() between its > >> assignment and the return. That pattern also appears to have been > >> copy/pasted from launchctl_boot_plist(). > > > > I don't find the most crucial information in that commit message: what is > > the fall-out of the removal of this call? > > > > Such an analysis (_with_ a summary of it in the commit message) is > > definitely required. And it should not be left as an exercise for the > > reader. > > Do you mean an assurance to the reader that the removed code doesn't > have any side-effects? E.g. an addition of > > As the patch shows the returned value wasn't used at all in this > function, the launchctl_get_uid() function itself just calls > xstrfmt() and getuid(), neither of which have any subtle global > side-effects, so this removal is safe. > > ? Yes. You want to refrain from forcing every reader to have to go look at the definition of that function at that revision. The accumulated time spent tallies up rather in disfavor of doing the work diligently on the contributor's side and save every reader some time. I mean, you forced me to spend the time, and then to spend more time to point out the missing analysis, and then you provided the paragraph as a question, forcing me to spend even more time on answering. All this time could have been saved in the first place. In this instance, it is too late to do anything about it. But I'm sure you plan on contributing other patches. Hopefully it will be more efficient next time. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Yes. You want to refrain from forcing every reader to have to go look at > the definition of that function at that revision. The accumulated time > spent tallies up rather in disfavor of doing the work diligently on the > contributor's side and save every reader some time. I mean, you forced me > to spend the time, and then to spend more time to point out the missing > analysis, and then you provided the paragraph as a question, forcing me to > spend even more time on answering. All this time could have been saved in > the first place. In this instance, it is too late to do anything about it. While I very much like to see that you subscribe to the principle, and I do wish that the "question" were posed as a comment after the three-dash-line in a rerolled patch that is written under the assumption that the answer to the question is "yes", which would have saved one extra roundtrip, I do not think the question was not something to be scolded like the above. All writers tend to assume that their readers know more than they do, and that is where the "do not force the reader to know or go look at things if they don't" principle comes from. But you're assuming that your "what's the fallout?" question did not need clarification, but it is understandable if it weren't. > But I'm sure you plan on contributing other patches. Hopefully it will be > more efficient next time. Thanks.
diff --git a/builtin/gc.c b/builtin/gc.c index 9a48eb535fb..b024f0e04e9 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1602,9 +1602,7 @@ static int launchctl_remove_plists(const char *cmd) static int launchctl_list_contains_plist(const char *name, const char *cmd) { - int result; struct child_process child = CHILD_PROCESS_INIT; - char *uid = launchctl_get_uid(); strvec_split(&child.args, cmd); strvec_pushl(&child.args, "list", name, NULL); @@ -1615,12 +1613,8 @@ static int launchctl_list_contains_plist(const char *name, const char *cmd) if (start_command(&child)) die(_("failed to start launchctl")); - result = finish_command(&child); - - free(uid); - /* Returns failure if 'name' doesn't exist. */ - return !result; + return !finish_command(&child); } static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)
When the launchctl_boot_plist() function was added in a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is registered, 2021-08-24), an unused call to launchctl_get_uid() was added along with it. That call appears to have been copy/pasted from launchctl_boot_plist(). Since we can remove that, we can also get rid of the "result" variable, whose only purpose was allow for the free() between its assignment and the return. That pattern also appears to have been copy/pasted from launchctl_boot_plist(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- I happen to have a local topic that refactored the launchctl_get_uid() function away, that didn't compile with the updated "master", I figured I'd need to add it back since it had a new user, but as it turns out that hopefully won't be needed. builtin/gc.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)