{"thread":{"id":"59206","subject":"[GSoC][PATCH] commit: warn the usage of reverse_commit_list() helper","startedAt":"2023-02-07T15:04:43Z","lastAt":"2023-02-08T16:00:22Z","messageCount":5,"participants":["Kousik Sanagavarapu","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471684","messageId":"20230207150359.177641-1-five231003@gmail.com","threadId":"59206","inReplyTo":null,"subject":"[GSoC][PATCH] commit: warn the usage of reverse_commit_list() helper","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-07T15:03:59Z","receivedAt":"2023-02-07T15:04:43Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The helper function reverse_commit_list() has destructive behavior when\nused to reverse a list in-place. Warn about this behavior.\n\nSigned-off-by: Kousik Sanagavarapu \n---\n\nThis patch has been sent based on the confusion that can be caused while\nusing the reverse_commit_list() helper function. One example of this is\na recent patch that I submitted[1] where the use of this function broke\ntry_merge_strategy() in merge.\n\nIt is also based on the discussions[2] there that I send this patch.\n\n[1]: https://lore.kernel.org/git/20230202165137.118741-1-five231003@gmail.com/\n[2]: https://lore.kernel.org/git/xmqqmt5uo9ea.fsf@gitster.g/\n\n commit.h | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.h b/commit.h\nindex fa39202fa6..9dba07748f 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -198,7 +198,12 @@ void commit_list_sort_by_date(struct commit_list **list);\n /* Shallow copy of the input list */\n struct commit_list *copy_commit_list(struct commit_list *list);\n \n-/* Modify list in-place to reverse it, returning new head; list will be tail */\n+/*\n+ * Modify list in-place to reverse it, returning new head; list will be tail.\n+ *\n+ * NOTE! The reversed list is constructed using the elements of the original\n+ * list, hence losing the original list.\n+ */\n struct commit_list *reverse_commit_list(struct commit_list *list);\n \n void free_commit_list(struct commit_list *list);\n-- \n2.25.1\n\n"},{"id":"471702","messageId":"230207.86o7q52vxu.gmgdl@evledraar.gmail.com","threadId":"59206","inReplyTo":"20230207150359.177641-1-five231003@gmail.com","subject":"Re: [GSoC][PATCH] commit: warn the usage of reverse_commit_list() helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-07T17:56:26Z","receivedAt":"2023-02-07T18:05:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 07 2023, Kousik Sanagavarapu wrote:\n\n> The helper function reverse_commit_list() has destructive behavior when\n> used to reverse a list in-place. Warn about this behavior.\n>\n> Signed-off-by: Kousik Sanagavarapu \n> ---\n>\n> This patch has been sent based on the confusion that can be caused while\n> using the reverse_commit_list() helper function. One example of this is\n> a recent patch that I submitted[1] where the use of this function broke\n> try_merge_strategy() in merge.\n>\n> It is also based on the discussions[2] there that I send this patch.\n>\n> [1]: https://lore.kernel.org/git/20230202165137.118741-1-five231003@gmail.com/\n> [2]: https://lore.kernel.org/git/xmqqmt5uo9ea.fsf@gitster.g/\n>\n> commit.h | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit.h b/commit.h\n> index fa39202fa6..9dba07748f 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -198,7 +198,12 @@ void commit_list_sort_by_date(struct commit_list **list);\n> /* Shallow copy of the input list */\n> struct commit_list *copy_commit_list(struct commit_list *list);\n> \n> -/* Modify list in-place to reverse it, returning new head; list will be tail */\n> +/*\n> + * Modify list in-place to reverse it, returning new head; list will be tail.\n> + *\n> + * NOTE! The reversed list is constructed using the elements of the original\n> + * list, hence losing the original list.\n> + */\n> struct commit_list *reverse_commit_list(struct commit_list *list);\n\nJunio can clarify, but I understood from his original comment on this\nsuggesting a comment that he wasn't aware of the existing documentation.\n\nI think it's better just to chuck this up to an understandable one-off\nmistake, if we're going to update the docs here I don't really see\nwhat's being added by this addition.\n\nIt seems to me that this is just rephrasing what's being said more\nsuccinctly with \"modifies in-place\", it's understood that any function\nwhich does that is going to schred the input data for its own purposes.\n\nIf that wording is thought to be too technical or obscure wouldn't we be\nbetter off with replacing the existing wording with something using less\njargon, rather than keeping the jargon & adding a rephrasing of it?\n\nHaving said that, I think the existing version is fine, and we could\njust ascribe the issue that prompted this to a one-off mistake :)\n\nI think if you want to pursue this, a much better improvement here would\nbe to show what the user *should* do.\n\nE.g. show one code example of using the API in-place, and then the\npreferred pattern if one wants to produce a new reversed commit list,\nwhile retaining the original (presumably just copy_commit_list()\nfollowed by reverse_commit_list()).\n"},{"id":"471716","messageId":"xmqqk00tz4rt.fsf@gitster.g","threadId":"59206","inReplyTo":"20230207150359.177641-1-five231003@gmail.com","subject":"Re: [GSoC][PATCH] commit: warn the usage of reverse_commit_list() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-07T18:53:10Z","receivedAt":"2023-02-07T18:53:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> -/* Modify list in-place to reverse it, returning new head; list will be tail */\n> +/*\n> + * Modify list in-place to reverse it, returning new head; list will be tail.\n> + *\n> + * NOTE! The reversed list is constructed using the elements of the original\n> + * list, hence losing the original list.\n> + */\n\nAfter re-reading the original, I realize that \"in-place\" is good\nenough clue to say that this is destructive.\n\n\n\n"},{"id":"471768","messageId":"20230208155350.186187-1-five231003@gmail.com","threadId":"59206","inReplyTo":"230207.86o7q52vxu.gmgdl@evledraar.gmail.com","subject":"Re: [GSoC][PATCH] commit: warn the usage of reverse_commit_list() helper","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-08T15:53:50Z","receivedAt":"2023-02-08T15:54:51Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Tue, 7 Feb 2023 at 23:35, Ævar Arnfjörð Bjarmason wrote:\n>\n>[...]\n>\n> Having said that, I think the existing version is fine, and we could\n> just ascribe the issue that prompted this to a one-off mistake :)\n\nI understand it now. Thanks.\n\n> I think if you want to pursue this, a much better improvement here would\n> be to show what the user *should* do.\n>\n> E.g. show one code example of using the API in-place, and then the\n> preferred pattern if one wants to produce a new reversed commit list,\n> while retaining the original (presumably just copy_commit_list()\n> followed by reverse_commit_list()).\n\nFollowing the response by Junio, I think it's better off that I leave\nit this way?\n\nThanks,\nKousik\n"},{"id":"471770","messageId":"20230208160013.186288-1-five231003@gmail.com","threadId":"59206","inReplyTo":"xmqqk00tz4rt.fsf@gitster.g","subject":"Re: [GSoC][PATCH] commit: warn the usage of reverse_commit_list() helper","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-08T16:00:13Z","receivedAt":"2023-02-08T16:00:22Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Wed, 8 Feb 2023 at 00:23, Junio C Hamano wrote:\n>\n> After re-reading the original, I realize that \"in-place\" is good\n> enough clue to say that this is destructive.\n\nI see. Thanks for the review.\n\nKousik\n"}]}