{"thread":{"id":"63938","subject":"[PATCH v2] diff: --no-index should ignore the worktree","startedAt":"2025-08-10T00:20:39Z","lastAt":"2025-08-15T22:01:29Z","messageCount":4,"participants":["Junio C Hamano","opensource@gregoirege.is"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"523880","messageId":"xmqq1ppk58ob.fsf@gitster.g","threadId":"63938","inReplyTo":null,"subject":"[PATCH v2] diff: --no-index should ignore the worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-10T00:20:36Z","receivedAt":"2025-08-10T00:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The act of giving \"--no-index\" tells Git to pretend that the current\ndirectory is not under control of any Git index or repository, so\neven when you happen to be in a Git controlled working tree, where\nin that working tree should not matter.\n\nBut the start-up sequence tries to discover the top of the working\ntree and chdir(2)'s there, even before Git passes control to the\nsubcommand being run.  When diff_no_index() starts running, it\nstarts at a wrong (from the end-user's point of view who thinks \"git\ndiff --no-index\" is merely a better version of GNU diff) directory,\nand the original directory the user started the command is at\n\"prefix\".\n\nBecause the paths given from argv[] have already been adjusted to\naccount for this path shuffling by prepending the prefix, and\nshowing the resulting path by stripping the prefix, the effect of\nthese nonsense operations (nonsense in the context of \"--no-index\",\nthat is) is usually not observable.\n\nExcept for special cases like \"-\", where it is not preprocessed by\nprepending the prefix.\n\nInstead of papering over by adding more special cases only to cater\nto the no-index codepath in the generic code, drive the diff\nmachinery more faithfully to what is going on.  If the user started\n\"git diff --no-index\" in directory X/Y/Z in a working tree\ncontrolled by Git, and the start up sequence of Git chdir(2)'ed up\nto directory X and left Y/Z in the prefix, revert the effect of the\nstart up sequence by chdir'ing back to Y/Z and emptying the prefix.\n\nReported-by: Gregoire Geis <opensource@gregoirege.is>\nHelped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/diff.c           | 15 +++++++++++++++\n t/t4053-diff-no-index.sh | 17 +++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 9a89e25a98..0b23c41456 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -487,6 +487,21 @@ int cmd_diff(int argc,\n \n \tinit_diff_ui_defaults();\n \trepo_config(the_repository, git_diff_ui_config, NULL);\n+\n+\t/*\n+\t * If we are ignoring the fact that our current directory may\n+\t * be part of a working tree controlled by a Git repository to\n+\t * pretend to be a \"better GNU diff\", we should undo the\n+\t * effect of the setup code that did a chdir() to the top of\n+\t * the working tree.  Where we came from is recorded in the\n+\t * prefix.\n+\t */\n+\tif (no_index && prefix) {\n+\t\tif (chdir(prefix))\n+\t\t\tdie(_(\"cannot come back to cwd\"));\n+\t\tprefix = NULL;\n+\t}\n+\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n \n \trepo_init_revisions(the_repository, &rev, prefix);\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 01db9243ab..44b4b13f5d 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -26,6 +26,23 @@ test_expect_success 'git diff --no-index directories' '\n \ttest_line_count = 14 cnt\n '\n \n+test_expect_success 'git diff --no-index with -' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/- b/-\n+\tnew file mode 100644\n+\t--- /dev/null\n+\t+++ b/-\n+\t@@ -0,0 +1 @@\n+\t+frotz\n+\tEOF\n+\t(\n+\t\tcd a &&\n+\t\techo frotz |\n+\t\ttest_expect_code 1 git diff --no-index /dev/null - >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git diff --no-index relative path outside repo' '\n \t(\n \t\tcd repo &&\n\nRange-diff against v1:\n1:  456a265746 ! 1:  4166ccb163 diff: --no-index should ignore the worktree\n    @@ builtin/diff.c: int cmd_diff(int argc,\n     +\t * prefix.\n     +\t */\n     +\tif (no_index && prefix) {\n    -+\t\tchdir(prefix);\n    ++\t\tif (chdir(prefix))\n    ++\t\t\tdie(_(\"cannot come back to cwd\"));\n     +\t\tprefix = NULL;\n     +\t}\n     +\n-- \n2.51.0-rc1-130-g831cc762e6\n\n"},{"id":"524049","messageId":"B90C049E-677E-449A-A899-B6071F852298@gregoirege.is","threadId":"63938","inReplyTo":"xmqq1ppk58ob.fsf@gitster.g","subject":"Re: [PATCH v2] diff: --no-index should ignore the worktree","fromName":"","fromEmail":"opensource@gregoirege.is","sentAt":"2025-08-12T14:37:39Z","receivedAt":"2025-08-12T14:37:54Z","isPatch":true,"sender":{"key":"opensource@gregoirege.is","avatar":null},"body":"Hey,\n\nI don't know how the review process goes at this point as you took\nover the patch. The handling of the chdir failure in v2 LGTM. I ran\nthe t*-diff*.sh tests again and tried a couple of commands, and\neverything seems to work.\n"},{"id":"524054","messageId":"xmqq8qjowpc1.fsf@gitster.g","threadId":"63938","inReplyTo":"B90C049E-677E-449A-A899-B6071F852298@gregoirege.is","subject":"Re: [PATCH v2] diff: --no-index should ignore the worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-12T15:07:42Z","receivedAt":"2025-08-12T15:07:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"opensource@gregoirege.is writes:\n\n> I don't know how the review process goes at this point as you took\n> over the patch. The handling of the chdir failure in v2 LGTM. I ran\n> the t*-diff*.sh tests again and tried a couple of commands, and\n> everything seems to work.\n\nThanks.\n"},{"id":"524281","messageId":"xmqqsehsmeh4.fsf@gitster.g","threadId":"63938","inReplyTo":"xmqq1ppk58ob.fsf@gitster.g","subject":"[PATCH] diff: simplify --no-index taking advantage of the fact that prefix==NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-15T22:01:27Z","receivedAt":"2025-08-15T22:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Since the caller of the diff_no_index() function now undoes the\neffect of setup.c:setup_git_directory_gently(), i.e. figuring out\nwhere in the working tree the cwd of the process is, recording the\ncwd relative to that top directory in the prefix, and chdir()ing up\nto the top directory, we are now guaranteed to always have NULL in\nthe prefix.  When started outside a repository, prefix would already\nbe NULL.\n\nTake advantage of this fact to simplify the implementation.\n\n - The code that conditionally calls prefix_filename(), except for\n   \"-\" given from the command line, can go away, as there is nothing\n   to prefix.\n\n - The temporary variable that points at the memory allocated by\n   prefix_filename() for later freeing can go; so can the loop to\n   free them.\n\n - As the prefix_filename() loop that had to special case \"-\" is\n   gone, we do not rewrite \"-\" to another constant address that has\n   the same string, file_from_standard_input[].  Teach get_mode()\n   function to strcmp() between the path(s) and \"-\" directly for\n   simplicity.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff-no-index.c | 40 ++++++++++++++--------------------------\n 1 file changed, 14 insertions(+), 26 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 88ae4cee56..8bdd0021b1 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -71,9 +71,8 @@ static int read_directory_contents(const char *path, struct string_list *list,\n }\n \n /*\n- * This should be \"(standard input)\" or something, but it will\n- * probably expose many more breakages in the way no-index code\n- * is bolted onto the diff callchain.\n+ * stdin should be spelled as \"-\"; if you have path that is \"-\",\n+ * spell it as \"./-\".\n  */\n static const char file_from_standard_input[] = \"-\";\n \n@@ -97,7 +96,7 @@ static int get_mode(const char *path, int *mode, enum special *special)\n \t} else if (!strcasecmp(path, \"nul\")) {\n \t\t*mode = 0;\n #endif\n-\t} else if (path == file_from_standard_input) {\n+\t} else if (!strcmp(path, file_from_standard_input)) {\n \t\t*mode = create_ce_mode(0666);\n \t\t*special = SPECIAL_STDIN;\n \t} else if (lstat(path, &st)) {\n@@ -305,18 +304,18 @@ static int fixup_paths(const char **path, struct strbuf *replacement)\n \tunsigned int isdir0 = 0, isdir1 = 0;\n \tunsigned int ispipe0 = 0, ispipe1 = 0;\n \n-\tif (path[0] != file_from_standard_input && !stat(path[0], &st)) {\n+\tif (strcmp(path[0], file_from_standard_input) && !stat(path[0], &st)) {\n \t\tisdir0 = S_ISDIR(st.st_mode);\n \t\tispipe0 = S_ISFIFO(st.st_mode);\n \t}\n \n-\tif (path[1] != file_from_standard_input && !stat(path[1], &st)) {\n+\tif (strcmp(path[1], file_from_standard_input) && !stat(path[1], &st)) {\n \t\tisdir1 = S_ISDIR(st.st_mode);\n \t\tispipe1 = S_ISFIFO(st.st_mode);\n \t}\n \n-\tif ((path[0] == file_from_standard_input && isdir1) ||\n-\t    (isdir0 && path[1] == file_from_standard_input))\n+\tif ((isdir1 && !strcmp(path[0], file_from_standard_input)) ||\n+\t    (isdir0 && !strcmp(path[1], file_from_standard_input)))\n \t\tdie(_(\"cannot compare stdin to a directory\"));\n \n \tif ((isdir0 && ispipe1) || (ispipe0 && isdir1))\n@@ -349,9 +348,7 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \tint i, no_index, skip1 = 0, skip2 = 0;\n \tint ret = 1;\n \tconst char *paths[2];\n-\tchar *to_free[ARRAY_SIZE(paths)] = { 0 };\n \tstruct strbuf replacement = STRBUF_INIT;\n-\tconst char *prefix = revs->prefix;\n \tstruct option no_index_options[] = {\n \t\tOPT_BOOL_F(0, \"no-index\", &no_index, \"\",\n \t\t\t   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),\n@@ -359,8 +356,11 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \t};\n \tstruct option *options;\n \n+\tif (revs->prefix)\n+\t\tBUG(\"caller of diff_no_index() forgot to undo setup\");\n+\n \toptions = add_diff_options(no_index_options, &revs->diffopt);\n-\targc = parse_options(argc, argv, revs->prefix, options,\n+\targc = parse_options(argc, argv, NULL, options,\n \t\t\t     diff_no_index_usage, 0);\n \tif (argc < 2) {\n \t\tif (implicit_no_index)\n@@ -368,18 +368,8 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \t\t\t\t  \"compare two paths outside a working tree\"));\n \t\tusage_with_options(diff_no_index_usage, options);\n \t}\n-\tfor (i = 0; i < 2; i++) {\n-\t\tconst char *p = argv[i];\n-\t\tif (!strcmp(p, \"-\"))\n-\t\t\t/*\n-\t\t\t * stdin should be spelled as \"-\"; if you have\n-\t\t\t * path that is \"-\", spell it as \"./-\".\n-\t\t\t */\n-\t\t\tp = file_from_standard_input;\n-\t\telse if (prefix)\n-\t\t\tp = to_free[i] = prefix_filename(prefix, p);\n-\t\tpaths[i] = p;\n-\t}\n+\tfor (i = 0; i < 2; i++)\n+\t\tpaths[i] = argv[i];\n \n \tif (fixup_paths(paths, &replacement)) {\n \t\tparse_pathspec(&pathspec, PATHSPEC_FROMTOP | PATHSPEC_ATTR,\n@@ -406,7 +396,7 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \trevs->diffopt.flags.no_index = 1;\n \n \trevs->diffopt.flags.relative_name = 1;\n-\trevs->diffopt.prefix = prefix;\n+\trevs->diffopt.prefix = NULL;\n \n \trevs->max_count = -2;\n \tdiff_setup_done(&revs->diffopt);\n@@ -428,8 +418,6 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \tret = diff_result_code(revs);\n \n out:\n-\tfor (i = 0; i < ARRAY_SIZE(to_free); i++)\n-\t\tfree(to_free[i]);\n \tstrbuf_release(&replacement);\n \tif (ps)\n \t\tclear_pathspec(ps);\n-- \n2.51.0-rc2-158-gf97fc618fa\n\n"}]}