{"thread":{"id":"61640","subject":"[PATCH 1/2] cat-file: configurable number of symlink resolutions","startedAt":"2024-06-17T09:03:52Z","lastAt":"2024-06-17T19:35:26Z","messageCount":6,"participants":["Miguel Ángel Pastor Olivar via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"497234","messageId":"cbf38c7281de33289f622c9926c75744323311af.1718615028.git.gitgitgadget@gmail.com","threadId":"61640","inReplyTo":"pull.1751.git.1718615028.gitgitgadget@gmail.com","subject":"[PATCH 1/2] cat-file: configurable number of symlink resolutions","fromName":"Miguel Ángel Pastor Olivar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-17T09:03:47Z","receivedAt":"2024-06-17T09:03:52Z","isPatch":true,"sender":{"key":"name:Miguel Ángel Pastor Olivar","avatar":null},"body":"From: =?UTF-8?q?Miguel=20=C3=81ngel=20Pastor=20Olivar?= <migue@github.com>\n\nSometimes, it can be useful to limit the number of symlink resolutions\nperformed while looking for a tree entry.\n\nThe goal is to provide the ability to resolve up to a particular depth,\ninstead of reaching the end of the link chain.\n\nThe current code already provides a limit to the maximum number of\nresolutions that can be performed\n(GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS). This patch introduces a new\nconfig setting to make the previous property configurable. No logical\nchanges are introduced in this patch\n\nSigned-off-by: Miguel Ángel Pastor Olivar <migue@github.com>\n---\n Documentation/config/core.txt |  5 +++++\n config.c                      |  5 +++++\n environment.c                 |  1 +\n environment.h                 |  1 +\n t/t1006-cat-file.sh           | 17 +++++++++++++++++\n tree-walk.c                   |  7 ++++++-\n 6 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 93d65e1dfd2..ca2d1eede52 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -757,3 +757,8 @@ core.maxTreeDepth::\n \ttree (e.g., \"a/b/cde/f\" has a depth of 4). This is a fail-safe\n \tto allow Git to abort cleanly, and should not generally need to\n \tbe adjusted. The default is 4096.\n+\n+core.maxSymlinkDepth::\n+\tThe maximum number of symlinks Git is willing to resolve while\n+\tlooking for a tree entry.\n+\tThe default is GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS.\n\\ No newline at end of file\ndiff --git a/config.c b/config.c\nindex abce05b7744..d69e9a3ae6b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1682,6 +1682,11 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.maxsymlinkdepth\")) {\n+\t\tmax_symlink_depth = git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn platform_core_config(var, value, ctx, cb);\n }\ndiff --git a/environment.c b/environment.c\nindex 701d5151354..6d7a5001eb1 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -95,6 +95,7 @@ int max_allowed_tree_depth =\n #else\n \t2048;\n #endif\n+int max_symlink_depth = -1;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\ndiff --git a/environment.h b/environment.h\nindex e9f01d4d11c..ea39c2887b1 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -141,6 +141,7 @@ extern size_t delta_base_cache_limit;\n extern unsigned long big_file_threshold;\n extern unsigned long pack_size_limit_cfg;\n extern int max_allowed_tree_depth;\n+extern int max_symlink_depth;\n \n /*\n  * Accessors for the core.sharedrepository config which lazy-load the value\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex e12b2219721..fd7ab1d1eff 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -878,6 +878,9 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '\n test_expect_success 'prep for symlink tests' '\n \techo_without_newline \"$hello_content\" >morx &&\n \ttest_ln_s_add morx same-dir-link &&\n+\ttest_ln_s_add same-dir-link link-to-symlink-1 &&\n+\ttest_ln_s_add link-to-symlink-1 link-to-symlink-2 &&\n+\ttest_ln_s_add link-to-symlink-2 link-to-symlink-3 &&\n \ttest_ln_s_add dir link-to-dir &&\n \ttest_ln_s_add ../fleem out-of-repo-link &&\n \ttest_ln_s_add .. out-of-repo-link-dir &&\n@@ -1096,6 +1099,20 @@ test_expect_success 'git cat-file --batch --follow-symlink returns correct sha a\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch --follow-symlink stop resolving symlinks' '\n+\tprintf \"loop 22\\nHEAD:link-to-symlink-3\\n\">expect &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=1 cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=2 cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=3 cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\toid=$(git rev-parse HEAD:morx) &&\n+\tprintf \"${oid} blob 11\\nHello World\\n\" >expect &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=4 cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cat-file --batch-all-objects shows all objects' '\n \t# make new repos so we know the full set of objects; we will\n \t# also make sure that there are some packed and some loose\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 6565d9ad993..3ec2302309e 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -664,7 +664,12 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r,\n \tstruct object_id current_tree_oid;\n \tstruct strbuf namebuf = STRBUF_INIT;\n \tstruct tree_desc t;\n-\tint follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n+\tint follows_remaining =\n+\t\tmax_symlink_depth > -1 &&\n+\t\t\t\tmax_symlink_depth <=\n+\t\t\t\t\tGET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS ?\n+\t\t\tmax_symlink_depth :\n+\t\t\tGET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n \n \tinit_tree_desc(&t, NULL, NULL, 0UL);\n \tstrbuf_addstr(&namebuf, name);\n-- \ngitgitgadget\n\n"},{"id":"497235","messageId":"5de72c45e767c4d704503c8cd5c8e6dce4ea04d6.1718615028.git.gitgitgadget@gmail.com","threadId":"61640","inReplyTo":"pull.1751.git.1718615028.gitgitgadget@gmail.com","subject":"[PATCH 2/2] cat-file: configurable \"best effort mode\" for symlink resolution","fromName":"Miguel Ángel Pastor Olivar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-17T09:03:48Z","receivedAt":"2024-06-17T09:03:53Z","isPatch":true,"sender":{"key":"name:Miguel Ángel Pastor Olivar","avatar":null},"body":"From: =?UTF-8?q?Miguel=20=C3=81ngel=20Pastor=20Olivar?= <migue@github.com>\n\nThis patch introduces a new \"best effort mode\" where the object found at\nresolution step N is returned. If we've reached the end of the chain, the\nreturned object will be the file at the end of the chain, however, if, after\nn resolutions we haven't reached the end of the chain, the returned object\nwill represent a symlink\n\nThe goal is to extend the symlink resolution process and provide the ability\nto return the object found at the designated depth instead of returning an\nerror.\n\nThe current code already provides a limit to the maximum number of\nresolutions that can be performed and something similar to this is returned\nback to the caller:\n\nloop SP <size> LF <object> LF\n\nWith the new config setting we are looking to return the actual information\nof the object where the resolution stopped. Something similar to:\n\n<oid> blob <size>\\ndata\\n\n\nSigned-off-by: Miguel Ángel Pastor Olivar <migue@github.com>\n---\n Documentation/config/core.txt | 16 +++++++++++++++-\n config.c                      | 13 +++++++++++++\n environment.c                 |  2 ++\n environment.h                 |  7 +++++++\n t/t1006-cat-file.sh           | 29 +++++++++++++++++++++++++++++\n tree-walk.c                   | 11 +++++++++++\n 6 files changed, 77 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex ca2d1eede52..706f316c89e 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -761,4 +761,18 @@ core.maxTreeDepth::\n core.maxSymlinkDepth::\n \tThe maximum number of symlinks Git is willing to resolve while\n \tlooking for a tree entry.\n-\tThe default is GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS.\n\\ No newline at end of file\n+\tThe default is GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS.\n+\n+core.symlinkResolutionMode::\n+\tThe result returned by the symlink resolution process when\n+\tcore.maxSymlinkDepth is reached. When set to \"error\"\n+\t`\n+\tloop SP <size> LF\n+\t<object> LF\n+\t` is returned.\n+\tIf `best-effort` is set, the resolution process will return\n+\tsomething like:\n+\t`\n+\t<oid> blob <size> 120000\\nname\\n\n+\t`\n+\tThe default is \"error\".\n\\ No newline at end of file\ndiff --git a/config.c b/config.c\nindex d69e9a3ae6b..fa753565e68 100644\n--- a/config.c\n+++ b/config.c\n@@ -1687,6 +1687,19 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.symlinkresolutionmode\")) {\n+\t\tif (!value)\n+\t\t\tsymlink_resolution_mode = SYMLINK_RESOLUTION_MODE_ERROR;\n+\t\tif (!strcmp(value, \"error\"))\n+\t\t\tsymlink_resolution_mode = SYMLINK_RESOLUTION_MODE_ERROR;\n+\t\telse if (!strcmp(value, \"best-effort\"))\n+\t\t\tsymlink_resolution_mode =\n+\t\t\t\tSYMLINK_RESOLUTION_MODE_BEST_EFFORT;\n+\t\telse\n+\t\t\twarning(_(\"ignoring unknown core.symlinkresolutionmode value '%s'\"),\n+\t\t\t\tvalue);\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn platform_core_config(var, value, ctx, cb);\n }\ndiff --git a/environment.c b/environment.c\nindex 6d7a5001eb1..a497331f2bc 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -96,6 +96,8 @@ int max_allowed_tree_depth =\n \t2048;\n #endif\n int max_symlink_depth = -1;\n+enum symlink_resolution_mode symlink_resolution_mode =\n+\tSYMLINK_RESOLUTION_MODE_ERROR;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\ndiff --git a/environment.h b/environment.h\nindex ea39c2887b1..5a6eebb061b 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -143,6 +143,13 @@ extern unsigned long pack_size_limit_cfg;\n extern int max_allowed_tree_depth;\n extern int max_symlink_depth;\n \n+enum symlink_resolution_mode {\n+\tSYMLINK_RESOLUTION_MODE_ERROR = 0,\n+\tSYMLINK_RESOLUTION_MODE_BEST_EFFORT\n+};\n+\n+extern enum symlink_resolution_mode symlink_resolution_mode;\n+\n /*\n  * Accessors for the core.sharedrepository config which lazy-load the value\n  * from the config (if not already set). The \"reset\" function can be\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex fd7ab1d1eff..c1d807a0d7f 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -1113,6 +1113,35 @@ test_expect_success 'git cat-file --batch --follow-symlink stop resolving symlin\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch --follow-symlink stop resolving symlink at designated depth with error mode config' '\n+\tprintf \"loop 22\\nHEAD:link-to-symlink-3\\n\">expect &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=1 -c core.symlinkresolutionmode=error cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=2 -c core.symlinkresolutionmode=error cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=3 -c core.symlinkresolutionmode=error cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual &&\n+\toid=$(git rev-parse HEAD:morx) &&\n+\tprintf \"${oid} blob 11\\nHello World\\n\" >expect &&\n+\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=4 cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git cat-file --batch --follow-symlink return object info at designated depth' '\n+\toid=$(git rev-parse HEAD:link-to-symlink-1) &&\n+\tprintf \"${oid} blob 13\\nsame-dir-link\\n\" >expect &&\n+\tprintf 'HEAD:link-to-symlink-1' | git -c core.maxsymlinkdepth=1  -c core.symlinkresolutionmode=best-effort cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks >actual &&\n+\ttest_cmp expect actual &&\n+\toid=$(git rev-parse HEAD:same-dir-link) &&\n+\tprintf \"${oid} blob 4\\nmorx\\n\" > expect &&\n+\tprintf 'HEAD:link-to-symlink-1' | git -c core.maxsymlinkdepth=2  -c core.symlinkresolutionmode=best-effort cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks >actual &&\n+\ttest_cmp expect actual &&\n+\toid=$(git rev-parse HEAD:morx) &&\n+\tprintf \"${oid} blob 11\\nHello World\\n\" > expect &&\n+\tprintf 'HEAD:link-to-symlink-1' | git -c core.maxsymlinkdepth=3  -c core.symlinkresolutionmode=best-effort cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cat-file --batch-all-objects shows all objects' '\n \t# make new repos so we know the full set of objects; we will\n \t# also make sure that there are some packed and some loose\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 3ec2302309e..ee861fd6351 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -821,6 +821,17 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r,\n \t\t\tcontents_start = contents;\n \n \t\t\tparent = &parents[parents_nr - 1];\n+\n+\t\t\tif (follows_remaining == 0 &&\n+\t\t\t    symlink_resolution_mode ==\n+\t\t\t\t    SYMLINK_RESOLUTION_MODE_BEST_EFFORT) {\n+\t\t\t\tstrbuf_addstr(result_path, contents);\n+\t\t\t\toidcpy(result, &current_tree_oid);\n+\t\t\t\tfree(contents);\n+\t\t\t\tretval = FOUND;\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\n \t\t\tinit_tree_desc(&t, &parent->oid, parent->tree, parent->size);\n \t\t\tstrbuf_splice(&namebuf, 0, len,\n \t\t\t\t      contents_start, link_len);\n-- \ngitgitgadget\n"},{"id":"497236","messageId":"pull.1751.git.1718615028.gitgitgadget@gmail.com","threadId":"61640","inReplyTo":null,"subject":"[PATCH 0/2] Symlink resolutions: limits and return modes","fromName":"Miguel Ángel Pastor Olivar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-17T09:03:46Z","receivedAt":"2024-06-17T09:04:09Z","isPatch":true,"sender":{"key":"name:Miguel Ángel Pastor Olivar","avatar":null},"body":"It can be useful to limit the number of symlink resolutions performed while\nlooking for a tree entry. The goal is to provide the ability to resolve up\nto a particular depth, instead of reaching the end of the link chain.\n\nIn addition, I would like to extend the symlink resolution process and\nprovide the ability to return the object found at the designated depth\ninstead of returning an error.\n\nThe current code already provides a limit to the maximum number of\nresolutions that can be performed, and something similar to this is returned\nto the caller:\n\nloop SP <size> LF\n<object> LF\n\n\nWith these patches, we are looking to return the actual information of the\nobject where the resolution stopped. Something similar to:\n\n<oid> blob <size>\\nndata\\n\n\n\nMiguel Ángel Pastor Olivar (2):\n  cat-file: configurable number of symlink resolutions\n  cat-file: configurable \"best effort mode\" for symlink resolution\n\n Documentation/config/core.txt | 19 +++++++++++++++\n config.c                      | 18 ++++++++++++++\n environment.c                 |  3 +++\n environment.h                 |  8 ++++++\n t/t1006-cat-file.sh           | 46 +++++++++++++++++++++++++++++++++++\n tree-walk.c                   | 18 +++++++++++++-\n 6 files changed, 111 insertions(+), 1 deletion(-)\n\n\nbase-commit: d63586cb314731c851f28e14fc8012988467e2da\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1751%2Fmigue%2Fmigue%2Ffollow-symlinks-max-depth-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1751/migue/migue/follow-symlinks-max-depth-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1751\n-- \ngitgitgadget\n"},{"id":"497253","messageId":"xmqq34pb749x.fsf@gitster.g","threadId":"61640","inReplyTo":"pull.1751.git.1718615028.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Symlink resolutions: limits and return modes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T19:33:46Z","receivedAt":"2024-06-17T19:33:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Miguel Ángel Pastor Olivar via GitGitGadget\"\n<gitgitgadget@gmail.com> writes:\n\n> The current code already provides a limit to the maximum number of\n> resolutions that can be performed, and something similar to this is returned\n> to the caller:\n>\n> loop SP <size> LF\n> <object> LF\n>\n>\n> With these patches, we are looking to return the actual information of the\n> object where the resolution stopped. Something similar to:\n>\n> <oid> blob <size>\\nndata\\n\n\nJust a random and idle thought, but is it all that interesting to\nlearn only about the object at the horizon?\n\nIf recursive resolutions are limited to say 3 levels, I wonder if it\nis beneficial to give full record from each iteration without losing\ninformation, e.g., saying \"A points at B which in turn points at C,\nand I stopped there but C is still not the final thing\", instead of\nsaying \"I followed links and C was the last one I saw after I\nrepeated for the maximum number of times the configuration allows me\nto\".\n"},{"id":"497254","messageId":"xmqqwmmn5ppd.fsf@gitster.g","threadId":"61640","inReplyTo":"cbf38c7281de33289f622c9926c75744323311af.1718615028.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] cat-file: configurable number of symlink resolutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T19:33:50Z","receivedAt":"2024-06-17T19:33:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Miguel Ángel Pastor Olivar via GitGitGadget\"\n<gitgitgadget@gmail.com> writes:\n\n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> index 93d65e1dfd2..ca2d1eede52 100644\n> --- a/Documentation/config/core.txt\n> +++ b/Documentation/config/core.txt\n> @@ -757,3 +757,8 @@ core.maxTreeDepth::\n>  \ttree (e.g., \"a/b/cde/f\" has a depth of 4). This is a fail-safe\n>  \tto allow Git to abort cleanly, and should not generally need to\n>  \tbe adjusted. The default is 4096.\n> +\n> +core.maxSymlinkDepth::\n> +\tThe maximum number of symlinks Git is willing to resolve while\n> +\tlooking for a tree entry.\n> +\tThe default is GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS.\n> \\ No newline at end of file\n\nStyle: please do not end our text files with an incomplete line.\n\nRegarding the patch contents, this is an end-user facing document.\nHow would they learn what the actual value is?\n\nIs there a \"valid range\" the users are allowed to set this value to?\nIf so, what is the range?  What do users get when they set it outside\nthe allowed range?  Do they get warned?  Do they get die()?  Is the\nvalue silently ignored?\n\nIf there is no upper limit for the \"valid range\", how does a user\nset it to \"infinity\", and what's the downside of doing so?  What\nhappens when the user sets it to 0, or a negative value, if there is\nno lower limit for the \"valid range\"?  The questions in this\nparagraph your updated documentation text do not have to answer if\nyour \"valid range\" does have both upper and lower limit, but the\ndocumentation must answer questions in the previous paragraph.\n\n> diff --git a/config.c b/config.c\n> index abce05b7744..d69e9a3ae6b 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1682,6 +1682,11 @@ static int git_default_core_config(const char *var, const char *value,\n>  \t\treturn 0;\n>  \t}\n>  \n> +\tif (!strcmp(var, \"core.maxsymlinkdepth\")) {\n> +\t\tmax_symlink_depth = git_config_int(var, value, ctx->kvi);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \t/* Add other config variables here and to Documentation/config.txt. */\n>  \treturn platform_core_config(var, value, ctx, cb);\n>  }\n> diff --git a/environment.c b/environment.c\n> index 701d5151354..6d7a5001eb1 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -95,6 +95,7 @@ int max_allowed_tree_depth =\n>  #else\n>  \t2048;\n>  #endif\n> +int max_symlink_depth = -1;\n\nWhy set it to -1 here, instead of initializing it to the\nGET_TREE_ENTRY_FOLLOW_SYMLINKS?  By introducing a configuration\nvariable (which by the way I am not convinced is necessarily a good\nidea to begin with), you are surfacing that built-in default value\nas a more prominent thing, not hidden away in a little corner of\ntree-walk.c implementation detail.  If you do define a \"valid range\nof values\", the code that parses core.maxsymlinkdepth in config.c\nmay want to learn what the value of GET_TREE_ENTRY_FOLLOW_SYMLINKS\nis, which means the symbol may need to be visible in some common\nheader file anyway.\n\nBy the way, this is not a new problem this patch introduces, as the\ndefault GET_TREE_ENTRY_FOLLOW_SYMLINKS came from 275721c2\n(tree-walk: learn get_tree_entry_follow_symlinks, 2015-05-20), but I\nwonder if the default number should somehow be aligned with the\nother upper limit, SYMREF_MAXDEPTH for a symbolic ref pointing at\nanother symbolic ref pointing at yet another ...\n\n> +test_expect_success 'git cat-file --batch --follow-symlink stop resolving symlinks' '\n> +\tprintf \"loop 22\\nHEAD:link-to-symlink-3\\n\">expect &&\n> +\tprintf 'HEAD:link-to-symlink-3' | git -c core.maxsymlinkdepth=1 cat-file --batch=\"%(objectname) %(objecttype) %(objectsize)\" --follow-symlinks > actual &&\n\nStyle: a redirection operator needs a single SP before it and no SP\nbetween it and its target, i.e.\n\n\tprintf \"loop 22...\" >expect &&\n\tprintf \"HEAD:link ...\" |\n        git ... cat-file ... >actual &&\n\nAlso fold overly long line after \"|\" pipeline.\n\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 6565d9ad993..3ec2302309e 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -664,7 +664,12 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r,\n>  \tstruct object_id current_tree_oid;\n>  \tstruct strbuf namebuf = STRBUF_INIT;\n>  \tstruct tree_desc t;\n> -\tint follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n> +\tint follows_remaining =\n> +\t\tmax_symlink_depth > -1 &&\n> +\t\t\t\tmax_symlink_depth <=\n> +\t\t\t\t\tGET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS ?\n> +\t\t\tmax_symlink_depth :\n> +\t\t\tGET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n\nStrange indentation.\n\nIf you range-limit at the place the configuration was parsed, you do\nnot have to do any of this here, but if you insist hiding\nGET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS from others (yet still use\nit in the end-user facing documentation???), then\n\n\tint follows_remaining =\n\t\t(-1 < max_symlink_depth &&\n\t\t max_symlink_depth <= GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS)\n\t\t? max_symlink_depth\n\t\t: GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n\nor perhaps a lot easier to read form, i.e.\n\n\tint follows_remaining = max_symlink_depth;\n\n        if (follows_remaining < -1 ||\n            GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS < follows_remaining)\n\t\tfollows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n\n>  \tinit_tree_desc(&t, NULL, NULL, 0UL);\n>  \tstrbuf_addstr(&namebuf, name);\n\n\nThanks.\n"},{"id":"497255","messageId":"xmqqr0cv5pp9.fsf@gitster.g","threadId":"61640","inReplyTo":"5de72c45e767c4d704503c8cd5c8e6dce4ea04d6.1718615028.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] cat-file: configurable \"best effort mode\" for symlink resolution","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T19:33:54Z","receivedAt":"2024-06-17T19:35:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Miguel Ángel Pastor Olivar via GitGitGadget\"\n<gitgitgadget@gmail.com> writes:\n\n> From: =?UTF-8?q?Miguel=20=C3=81ngel=20Pastor=20Olivar?= <migue@github.com>\n>\n> This patch introduces a new \"best effort mode\" where the object found at\n> resolution step N is returned. If we've reached the end of the chain, the\n> returned object will be the file at the end of the chain, however, if, after\n> n resolutions we haven't reached the end of the chain, the returned object\n> will represent a symlink\n>\n> The goal is to extend the symlink resolution process and provide the ability\n> to return the object found at the designated depth instead of returning an\n> error.\n>\n> The current code already provides a limit to the maximum number of\n> resolutions that can be performed and something similar to this is returned\n> back to the caller:\n>\n> loop SP <size> LF <object> LF\n>\n> With the new config setting we are looking to return the actual information\n> of the object where the resolution stopped. Something similar to:\n>\n> <oid> blob <size>\\ndata\\n\n\nI do not think this should be a configuration variable at all.\nEither a command line option, or even better yet would be an\nin-stream instruction (\"flip into the 'tell me the last symlink\nyou saw before you gave up' mode\"), is understandable though, given\nthat this is strictly for the \"batch\" mode.\n\nFor that matter, it is dubious that the previous one that added a\nconfiguration variable to lower the symlink recursion limit is a\ngood idea.  It does not affect anything but \"cat-file --batch\" and\nan in-stream instruction, e.g. \"in this session, do not resolve more\nthan 3 levels\", sounds like a much better fit to what this wants to\ndo.  That way, it will be a lot better isolated from unrelated code\npaths.  It might even make sense not to introduce the new\nmax_symlink_depth global variable, but pass it through as a new\nmember in \"struct object_context\" given to get_oid_with_context(),\nwhich in turn is passed as a new parameter to\nget_tree_entry_follow_symlinks() function.\n\nSo, I am supportive to solving the problem this series attempts to\nsolve, but I am not on board with the design this series took.\n\nThanks.\n"}]}