| Message ID | 20210215184118.11306-5-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: parse_options() cleanup | expand |
On Mon, Feb 15, 2021 at 07:41:17PM +0100, Ævar Arnfjörð Bjarmason wrote: > I think it's more readable to have one if/elsif/else chain here than > the code this replaces. FWIW, I find the pre-image more readable than what you are proposing replacing it with here. Of course, I have no doubts about the obvious correctness of this patch; I'm merely suggesting that I wouldn't be sad to see us apply the first three patches, and the fifth patch, but drop this one. Thanks, Taylor
On 2/15/2021 1:53 PM, Taylor Blau wrote: > On Mon, Feb 15, 2021 at 07:41:17PM +0100, Ævar Arnfjörð Bjarmason wrote: >> I think it's more readable to have one if/elsif/else chain here than >> the code this replaces. > > FWIW, I find the pre-image more readable than what you are proposing > replacing it with here. > > Of course, I have no doubts about the obvious correctness of this patch; > I'm merely suggesting that I wouldn't be sad to see us apply the first > three patches, and the fifth patch, but drop this one. I agree with all of your points here. I think that compared to the current code at-rest, the new version might be preferred. It's a little dense, which is my only complaint. The issue comes for the future: what if we need to add a third verb to 'git commit-graph'? Then extending this new option looks worse since we would check 'argc' three times. The other patches solve real readability problems or reorganize the code to use other concepts within the codebase. This one is much more optional. Thanks, -Stolee
On Tue, Feb 16 2021, Derrick Stolee wrote: > On 2/15/2021 1:53 PM, Taylor Blau wrote: >> On Mon, Feb 15, 2021 at 07:41:17PM +0100, Ævar Arnfjörð Bjarmason wrote: >>> I think it's more readable to have one if/elsif/else chain here than >>> the code this replaces. >> >> FWIW, I find the pre-image more readable than what you are proposing >> replacing it with here. >> >> Of course, I have no doubts about the obvious correctness of this patch; >> I'm merely suggesting that I wouldn't be sad to see us apply the first >> three patches, and the fifth patch, but drop this one. > > I agree with all of your points here. I think that compared to the > current code at-rest, the new version might be preferred. It's a little > dense, which is my only complaint. > > The issue comes for the future: what if we need to add a third verb > to 'git commit-graph'? Then extending this new option looks worse since > we would check 'argc' three times. > > The other patches solve real readability problems or reorganize the code > to use other concepts within the codebase. This one is much more optional. What do you think about https://lore.kernel.org/git/874kidapv7.fsf@evledraar.gmail.com/ ? :)
On 2/16/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Feb 16 2021, Derrick Stolee wrote: >> The other patches solve real readability problems or reorganize the code >> to use other concepts within the codebase. This one is much more optional. > > What do you think about > https://lore.kernel.org/git/874kidapv7.fsf@evledraar.gmail.com/ ? :) Using a 'goto' is a fine way to avoid a nesting level, but I'm not sure it "improves readability." Having the tab level makes it clear that that code is executed only when some condition is met, in this case "if (argc)," while with the 'goto' we need to know that execution was redirected. I don't feel too strongly either way. If the code was presented one way or the other, I probably wouldn't recommend changing to the other mode. In that sense, the change isn't necessary and causes me to break the tie in favor of leaving it where it is. Of course, if someone else says "I like this and would prefer it be used as an example for future contributors" then the tie is broken in the other way. Thanks, -Stolee
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index a7718b2025..66fbdb7cb1 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -334,13 +334,11 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) save_commit_buffer = 0; - if (argc > 0) { - if (!strcmp(argv[0], "verify")) - return graph_verify(argc, argv); - if (!strcmp(argv[0], "write")) - return graph_write(argc, argv); - } - - usage_with_options(builtin_commit_graph_usage, - builtin_commit_graph_options); + if (argc && !strcmp(argv[0], "verify")) + return graph_verify(argc, argv); + else if (argc && !strcmp(argv[0], "write")) + return graph_write(argc, argv); + else + usage_with_options(builtin_commit_graph_usage, + builtin_commit_graph_options); }
I think it's more readable to have one if/elsif/else chain here than the code this replaces. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/commit-graph.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)