{"thread":{"id":"51937","subject":"[PATCH v3] commit-graph: remove a duplicate assignment","startedAt":"2019-09-29T00:56:40Z","lastAt":"2019-09-30T07:34:20Z","messageCount":3,"participants":["Alex Henrie","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"383092","messageId":"20190929005558.733699-1-alexhenrie24@gmail.com","threadId":"51937","inReplyTo":null,"subject":"[PATCH v3] commit-graph: remove a duplicate assignment","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-29T00:55:58Z","receivedAt":"2019-09-29T00:56:40Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The variable g was being set to the same value both at the beginning of\nthe function and before the loop. The assignment before the loop was\nkept because it helps clarify what the loop does, and the redundant\nassignment at the beginning of the function was removed.\n\nReviewed-by: Derrick Stolee \nSigned-off-by: Alex Henrie \n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b02d2c426..d0e1f9e1f2 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1522,7 +1522,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n {\n-\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n+\tstruct commit_graph *g;\n \tuint32_t num_commits = ctx->commits.nr;\n \tuint32_t i;\n \n-- \n2.23.0\n\n"},{"id":"383109","messageId":"xmqqimpainmj.fsf@gitster-ct.c.googlers.com","threadId":"51937","inReplyTo":"20190929005558.733699-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v3] commit-graph: remove a duplicate assignment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-30T01:22:44Z","receivedAt":"2019-09-30T01:22:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> The variable g was being set to the same value both at the beginning of\n> the function and before the loop. The assignment before the loop was\n> kept because it helps clarify what the loop does, and the redundant\n> assignment at the beginning of the function was removed.\n\nWriting these mostly in the past tense is misleading to those who\nare used to read \"git log\" from this project. Give orders to the\ncodebase to \"become like so\" instead. Perhaps like\n\n\tLeave the variable 'g' uninitialized before it is set just\n\tbefore its first use in front of a loop, which is a lot more\n\tappropriate place to indicate what it is used for.\n\n> static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n> {\n> -\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n> +\tstruct commit_graph *g;\n> \tuint32_t num_commits = ctx->commits.nr;\n\nStepping back a bit, doesn't the same justification you gave to this\nchange apply to 'num_commits'? If you make it uninitialized before\nits first use and assign ctx->commits.nr to it near where 'g' is\ngiven its first value, wouldn't it make it even clearer that these\ntwo variables are almost always used together and how they are used\nin the loop?\n\n"},{"id":"383127","messageId":"CAMMLpeSY9DJbJJDExS2RcdyXbr8dEW5S67zSyNeCHsr5h2upTg@mail.gmail.com","threadId":"51937","inReplyTo":"xmqqimpainmj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] commit-graph: remove a duplicate assignment","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-30T07:34:03Z","receivedAt":"2019-09-30T07:34:20Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Sun, Sep 29, 2019 at 7:22 PM Junio C Hamano wrote:\n>\n> Alex Henrie writes:\n>\n> > The variable g was being set to the same value both at the beginning of\n> > the function and before the loop. The assignment before the loop was\n> > kept because it helps clarify what the loop does, and the redundant\n> > assignment at the beginning of the function was removed.\n>\n> Writing these mostly in the past tense is misleading to those who\n> are used to read \"git log\" from this project. Give orders to the\n> codebase to \"become like so\" instead. Perhaps like\n>\n> Leave the variable 'g' uninitialized before it is set just\n> before its first use in front of a loop, which is a lot more\n> appropriate place to indicate what it is used for.\n\nOkay, thanks for the guidance. I'll use your text on the next version\nof the patch.\n\n> > static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n> > {\n> > - struct commit_graph *g = ctx->r->objects->commit_graph;\n> > + struct commit_graph *g;\n> > uint32_t num_commits = ctx->commits.nr;\n>\n> Stepping back a bit, doesn't the same justification you gave to this\n> change apply to 'num_commits'? If you make it uninitialized before\n> its first use and assign ctx->commits.nr to it near where 'g' is\n> given its first value, wouldn't it make it even clearer that these\n> two variables are almost always used together and how they are used\n> in the loop?\n\nYes, that makes sense. I'll send a revised patch that includes that\nchange as well.\n\n-Alex\n"}]}