{"thread":{"id":"52546","subject":"[PATCH 0/1] merge-recursive: remove unnecessary oid_eq function","startedAt":"2020-01-01T05:26:33Z","lastAt":"2020-01-02T20:52:41Z","messageCount":4,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"389142","messageId":"pull.685.git.git.1577856057.gitgitgadget@gmail.com","threadId":"52546","inReplyTo":null,"subject":"[PATCH 0/1] merge-recursive: remove unnecessary oid_eq function","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-01T05:20:56Z","receivedAt":"2020-01-01T05:26:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"merge-recursive has an unnecessary wrapper for oideq. Remove it.\n\nElijah Newren (1):\n  merge-recursive: remove unnecessary oid_eq function\n\n merge-recursive.c | 33 +++++++++++----------------------\n 1 file changed, 11 insertions(+), 22 deletions(-)\n\n\nbase-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-685%2Fnewren%2Fremove_oid_eq_wrapper-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-685/newren/remove_oid_eq_wrapper-v1\nPull-Request: https://github.com/git/git/pull/685\n-- \ngitgitgadget\n"},{"id":"389143","messageId":"c653a9b8d3863b3484eff224bbfbde65c250eaf0.1577856057.git.gitgitgadget@gmail.com","threadId":"52546","inReplyTo":"pull.685.git.git.1577856057.gitgitgadget@gmail.com","subject":"[PATCH 1/1] merge-recursive: remove unnecessary oid_eq function","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-01T05:20:57Z","receivedAt":"2020-01-01T05:26:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nBack when merge-recursive was first introduced in commit 6d297f8137\n(Status update on merge-recursive in C, 2006-07-08), it created a\nsha_eq() function.  This function pre-dated the introduction of\nhashcmp() to cache.h by about a month, but was switched over to using\nhashcmp() as part of commit 9047ebbc22 (Split out merge_recursive() to\nmerge-recursive.c, 2008-08-12).  In commit b4da9d62f9 (merge-recursive:\nconvert leaf functions to use struct object_id, 2016-06-24), sha_eq() was\nrenamed to oid_eq() and its hashcmp() call was switched to oideq().\n\noid_eq() is basically just a wrapper around oideq() that has some extra\nchecks to protect against NULL arguments or to allow short-circuiting if\none of the arguments is NULL.  I don't know if any caller ever tried to\ncall with NULL arguments, but certainly none do now which means the\nextra checks serve no purpose.  (Also, if these checks were genuinely\nuseful, then they probably should be added to the main oideq() so all\ncallers could benefit from them.)\n\nReduce the cognitive overhead of having both oid_eq() and oideq(), by\ngetting rid of merge-recursive's special oid_eq() wrapper.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 33 +++++++++++----------------------\n 1 file changed, 11 insertions(+), 22 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 11869ad81c..10dca5644b 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -224,17 +224,6 @@ static struct commit *make_virtual_commit(struct repository *repo,\n \treturn commit;\n }\n \n-/*\n- * Since we use get_tree_entry(), which does not put the read object into\n- * the object pool, we cannot rely on a == b.\n- */\n-static int oid_eq(const struct object_id *a, const struct object_id *b)\n-{\n-\tif (!a && !b)\n-\t\treturn 2;\n-\treturn a && b && oideq(a, b);\n-}\n-\n enum rename_type {\n \tRENAME_NORMAL = 0,\n \tRENAME_VIA_DIR,\n@@ -805,7 +794,7 @@ static int was_tracked_and_matches(struct merge_options *opt, const char *path,\n \n \t/* See if the file we were tracking before matches */\n \tce = opt->priv->orig_index.cache[pos];\n-\treturn (oid_eq(&ce->oid, &blob->oid) && ce->ce_mode == blob->mode);\n+\treturn (oideq(&ce->oid, &blob->oid) && ce->ce_mode == blob->mode);\n }\n \n /*\n@@ -1317,7 +1306,7 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \t\t\toidcpy(&result->blob.oid, &b->oid);\n \t\t}\n \t} else {\n-\t\tif (!oid_eq(&a->oid, &o->oid) && !oid_eq(&b->oid, &o->oid))\n+\t\tif (!oideq(&a->oid, &o->oid) && !oideq(&b->oid, &o->oid))\n \t\t\tresult->merge = 1;\n \n \t\t/*\n@@ -1333,9 +1322,9 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \t\t\t}\n \t\t}\n \n-\t\tif (oid_eq(&a->oid, &b->oid) || oid_eq(&a->oid, &o->oid))\n+\t\tif (oideq(&a->oid, &b->oid) || oideq(&a->oid, &o->oid))\n \t\t\toidcpy(&result->blob.oid, &b->oid);\n-\t\telse if (oid_eq(&b->oid, &o->oid))\n+\t\telse if (oideq(&b->oid, &o->oid))\n \t\t\toidcpy(&result->blob.oid, &a->oid);\n \t\telse if (S_ISREG(a->mode)) {\n \t\t\tmmbuffer_t result_buf;\n@@ -1368,7 +1357,7 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \t\t\tswitch (opt->recursive_variant) {\n \t\t\tcase MERGE_VARIANT_NORMAL:\n \t\t\t\toidcpy(&result->blob.oid, &a->oid);\n-\t\t\t\tif (!oid_eq(&a->oid, &b->oid))\n+\t\t\t\tif (!oideq(&a->oid, &b->oid))\n \t\t\t\t\tresult->clean = 0;\n \t\t\t\tbreak;\n \t\t\tcase MERGE_VARIANT_OURS:\n@@ -2836,15 +2825,15 @@ static int process_renames(struct merge_options *opt,\n \t\t\tdst_other.mode = ren1->dst_entry->stages[other_stage].mode;\n \t\t\ttry_merge = 0;\n \n-\t\t\tif (oid_eq(&src_other.oid, &null_oid) &&\n+\t\t\tif (oideq(&src_other.oid, &null_oid) &&\n \t\t\t    ren1->dir_rename_original_type == 'A') {\n \t\t\t\tsetup_rename_conflict_info(RENAME_VIA_DIR,\n \t\t\t\t\t\t\t   opt, ren1, NULL);\n-\t\t\t} else if (oid_eq(&src_other.oid, &null_oid)) {\n+\t\t\t} else if (oideq(&src_other.oid, &null_oid)) {\n \t\t\t\tsetup_rename_conflict_info(RENAME_DELETE,\n \t\t\t\t\t\t\t   opt, ren1, NULL);\n \t\t\t} else if ((dst_other.mode == ren1->pair->two->mode) &&\n-\t\t\t\t   oid_eq(&dst_other.oid, &ren1->pair->two->oid)) {\n+\t\t\t\t   oideq(&dst_other.oid, &ren1->pair->two->oid)) {\n \t\t\t\t/*\n \t\t\t\t * Added file on the other side identical to\n \t\t\t\t * the file being renamed: clean merge.\n@@ -2859,7 +2848,7 @@ static int process_renames(struct merge_options *opt,\n \t\t\t\t\t\t      1, /* update_cache */\n \t\t\t\t\t\t      0  /* update_wd    */))\n \t\t\t\t\tclean_merge = -1;\n-\t\t\t} else if (!oid_eq(&dst_other.oid, &null_oid)) {\n+\t\t\t} else if (!oideq(&dst_other.oid, &null_oid)) {\n \t\t\t\t/*\n \t\t\t\t * Probably not a clean merge, but it's\n \t\t\t\t * premature to set clean_merge to 0 here,\n@@ -3037,7 +3026,7 @@ static int blob_unchanged(struct merge_options *opt,\n \n \tif (a->mode != o->mode)\n \t\treturn 0;\n-\tif (oid_eq(&o->oid, &a->oid))\n+\tif (oideq(&o->oid, &a->oid))\n \t\treturn 1;\n \tif (!renormalize)\n \t\treturn 0;\n@@ -3478,7 +3467,7 @@ static int merge_trees_internal(struct merge_options *opt,\n \t\t\t\t\t       opt->subtree_shift);\n \t}\n \n-\tif (oid_eq(&merge_base->object.oid, &merge->object.oid)) {\n+\tif (oideq(&merge_base->object.oid, &merge->object.oid)) {\n \t\toutput(opt, 0, _(\"Already up to date!\"));\n \t\t*result = head;\n \t\treturn 1;\n-- \ngitgitgadget\n"},{"id":"389169","messageId":"xmqqtv5demg4.fsf@gitster-ct.c.googlers.com","threadId":"52546","inReplyTo":"c653a9b8d3863b3484eff224bbfbde65c250eaf0.1577856057.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] merge-recursive: remove unnecessary oid_eq function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-02T18:35:07Z","receivedAt":"2020-01-02T18:35:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> Back when merge-recursive was first introduced in commit 6d297f8137\n> (Status update on merge-recursive in C, 2006-07-08), it created a\n> sha_eq() function.  This function pre-dated the introduction of\n> hashcmp() to cache.h by about a month, but was switched over to using\n> hashcmp() as part of commit 9047ebbc22 (Split out merge_recursive() to\n> merge-recursive.c, 2008-08-12).  In commit b4da9d62f9 (merge-recursive:\n> convert leaf functions to use struct object_id, 2016-06-24), sha_eq() was\n> renamed to oid_eq() and its hashcmp() call was switched to oideq().\n>\n> oid_eq() is basically just a wrapper around oideq() that has some extra\n> checks to protect against NULL arguments or to allow short-circuiting if\n> one of the arguments is NULL.  I don't know if any caller ever tried to\n> call with NULL arguments, but certainly none do now which means the\n> extra checks serve no purpose.  (Also, if these checks were genuinely\n> useful, then they probably should be added to the main oideq() so all\n> callers could benefit from them.)\n\nJust for some historical yuck values ^W^W entertainment,\n6d297f81373:merge-recursive.c shows how the function was called and\nneeded to prepare for NULL inputs.\n\nI agree that today's code won't need the \"two NULLs are equal, and NULL\nis never equal to anything\" hack.  We've come a long way ;-)\n\n"},{"id":"389179","messageId":"nycvar.QRO.7.76.6.2001022151490.46@tvgsbejvaqbjf.bet","threadId":"52546","inReplyTo":"c653a9b8d3863b3484eff224bbfbde65c250eaf0.1577856057.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] merge-recursive: remove unnecessary oid_eq function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-02T20:52:17Z","receivedAt":"2020-01-02T20:52:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 1 Jan 2020, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> Back when merge-recursive was first introduced in commit 6d297f8137\n> (Status update on merge-recursive in C, 2006-07-08), it created a\n> sha_eq() function.  This function pre-dated the introduction of\n> hashcmp() to cache.h by about a month, but was switched over to using\n> hashcmp() as part of commit 9047ebbc22 (Split out merge_recursive() to\n> merge-recursive.c, 2008-08-12).  In commit b4da9d62f9 (merge-recursive:\n> convert leaf functions to use struct object_id, 2016-06-24), sha_eq() was\n> renamed to oid_eq() and its hashcmp() call was switched to oideq().\n>\n> oid_eq() is basically just a wrapper around oideq() that has some extra\n> checks to protect against NULL arguments or to allow short-circuiting if\n> one of the arguments is NULL.  I don't know if any caller ever tried to\n> call with NULL arguments, but certainly none do now which means the\n> extra checks serve no purpose.  (Also, if these checks were genuinely\n> useful, then they probably should be added to the main oideq() so all\n> callers could benefit from them.)\n>\n> Reduce the cognitive overhead of having both oid_eq() and oideq(), by\n> getting rid of merge-recursive's special oid_eq() wrapper.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n\nThe patch and the commit message look good to me!\n\nThanks,\nDscho\n\n>  merge-recursive.c | 33 +++++++++++----------------------\n>  1 file changed, 11 insertions(+), 22 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 11869ad81c..10dca5644b 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -224,17 +224,6 @@ static struct commit *make_virtual_commit(struct repository *repo,\n>  \treturn commit;\n>  }\n>\n> -/*\n> - * Since we use get_tree_entry(), which does not put the read object into\n> - * the object pool, we cannot rely on a == b.\n> - */\n> -static int oid_eq(const struct object_id *a, const struct object_id *b)\n> -{\n> -\tif (!a && !b)\n> -\t\treturn 2;\n> -\treturn a && b && oideq(a, b);\n> -}\n> -\n>  enum rename_type {\n>  \tRENAME_NORMAL = 0,\n>  \tRENAME_VIA_DIR,\n> @@ -805,7 +794,7 @@ static int was_tracked_and_matches(struct merge_options *opt, const char *path,\n>\n>  \t/* See if the file we were tracking before matches */\n>  \tce = opt->priv->orig_index.cache[pos];\n> -\treturn (oid_eq(&ce->oid, &blob->oid) && ce->ce_mode == blob->mode);\n> +\treturn (oideq(&ce->oid, &blob->oid) && ce->ce_mode == blob->mode);\n>  }\n>\n>  /*\n> @@ -1317,7 +1306,7 @@ static int merge_mode_and_contents(struct merge_options *opt,\n>  \t\t\toidcpy(&result->blob.oid, &b->oid);\n>  \t\t}\n>  \t} else {\n> -\t\tif (!oid_eq(&a->oid, &o->oid) && !oid_eq(&b->oid, &o->oid))\n> +\t\tif (!oideq(&a->oid, &o->oid) && !oideq(&b->oid, &o->oid))\n>  \t\t\tresult->merge = 1;\n>\n>  \t\t/*\n> @@ -1333,9 +1322,9 @@ static int merge_mode_and_contents(struct merge_options *opt,\n>  \t\t\t}\n>  \t\t}\n>\n> -\t\tif (oid_eq(&a->oid, &b->oid) || oid_eq(&a->oid, &o->oid))\n> +\t\tif (oideq(&a->oid, &b->oid) || oideq(&a->oid, &o->oid))\n>  \t\t\toidcpy(&result->blob.oid, &b->oid);\n> -\t\telse if (oid_eq(&b->oid, &o->oid))\n> +\t\telse if (oideq(&b->oid, &o->oid))\n>  \t\t\toidcpy(&result->blob.oid, &a->oid);\n>  \t\telse if (S_ISREG(a->mode)) {\n>  \t\t\tmmbuffer_t result_buf;\n> @@ -1368,7 +1357,7 @@ static int merge_mode_and_contents(struct merge_options *opt,\n>  \t\t\tswitch (opt->recursive_variant) {\n>  \t\t\tcase MERGE_VARIANT_NORMAL:\n>  \t\t\t\toidcpy(&result->blob.oid, &a->oid);\n> -\t\t\t\tif (!oid_eq(&a->oid, &b->oid))\n> +\t\t\t\tif (!oideq(&a->oid, &b->oid))\n>  \t\t\t\t\tresult->clean = 0;\n>  \t\t\t\tbreak;\n>  \t\t\tcase MERGE_VARIANT_OURS:\n> @@ -2836,15 +2825,15 @@ static int process_renames(struct merge_options *opt,\n>  \t\t\tdst_other.mode = ren1->dst_entry->stages[other_stage].mode;\n>  \t\t\ttry_merge = 0;\n>\n> -\t\t\tif (oid_eq(&src_other.oid, &null_oid) &&\n> +\t\t\tif (oideq(&src_other.oid, &null_oid) &&\n>  \t\t\t    ren1->dir_rename_original_type == 'A') {\n>  \t\t\t\tsetup_rename_conflict_info(RENAME_VIA_DIR,\n>  \t\t\t\t\t\t\t   opt, ren1, NULL);\n> -\t\t\t} else if (oid_eq(&src_other.oid, &null_oid)) {\n> +\t\t\t} else if (oideq(&src_other.oid, &null_oid)) {\n>  \t\t\t\tsetup_rename_conflict_info(RENAME_DELETE,\n>  \t\t\t\t\t\t\t   opt, ren1, NULL);\n>  \t\t\t} else if ((dst_other.mode == ren1->pair->two->mode) &&\n> -\t\t\t\t   oid_eq(&dst_other.oid, &ren1->pair->two->oid)) {\n> +\t\t\t\t   oideq(&dst_other.oid, &ren1->pair->two->oid)) {\n>  \t\t\t\t/*\n>  \t\t\t\t * Added file on the other side identical to\n>  \t\t\t\t * the file being renamed: clean merge.\n> @@ -2859,7 +2848,7 @@ static int process_renames(struct merge_options *opt,\n>  \t\t\t\t\t\t      1, /* update_cache */\n>  \t\t\t\t\t\t      0  /* update_wd    */))\n>  \t\t\t\t\tclean_merge = -1;\n> -\t\t\t} else if (!oid_eq(&dst_other.oid, &null_oid)) {\n> +\t\t\t} else if (!oideq(&dst_other.oid, &null_oid)) {\n>  \t\t\t\t/*\n>  \t\t\t\t * Probably not a clean merge, but it's\n>  \t\t\t\t * premature to set clean_merge to 0 here,\n> @@ -3037,7 +3026,7 @@ static int blob_unchanged(struct merge_options *opt,\n>\n>  \tif (a->mode != o->mode)\n>  \t\treturn 0;\n> -\tif (oid_eq(&o->oid, &a->oid))\n> +\tif (oideq(&o->oid, &a->oid))\n>  \t\treturn 1;\n>  \tif (!renormalize)\n>  \t\treturn 0;\n> @@ -3478,7 +3467,7 @@ static int merge_trees_internal(struct merge_options *opt,\n>  \t\t\t\t\t       opt->subtree_shift);\n>  \t}\n>\n> -\tif (oid_eq(&merge_base->object.oid, &merge->object.oid)) {\n> +\tif (oideq(&merge_base->object.oid, &merge->object.oid)) {\n>  \t\toutput(opt, 0, _(\"Already up to date!\"));\n>  \t\t*result = head;\n>  \t\treturn 1;\n> --\n> gitgitgadget\n>\n"}]}