{"thread":{"id":"55440","subject":"[Bug report] diff.noprefix config is ignored for interactive `add`","startedAt":"2021-04-06T10:15:39Z","lastAt":"2021-04-06T22:47:27Z","messageCount":4,"participants":["Nikita Bobko","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"421051","messageId":"CAMJzOtyzu8y5mWdKXe3MPe8ZoJs8O=me8Xuu0t77YVdAMc7Tgg@mail.gmail.com","threadId":"55440","inReplyTo":null,"subject":"[Bug report] diff.noprefix config is ignored for interactive `add`","fromName":"Nikita Bobko","fromEmail":"nikitabobko@gmail.com","sentAt":"2021-04-06T10:15:28Z","receivedAt":"2021-04-06T10:15:39Z","isPatch":false,"sender":{"key":"nikitabobko@gmail.com","avatar":null},"body":"Steps to reproduce:\n1. Run `git -c diff.noprefix=true add -p`\n\nExpected behavior:\n`a/` and `b/` prefixes are not shown during interactive `add`\nlike this:\n```\ndiff --git foo foo\nindex 257cc56..1715acd 100644\n--- foo\n+++ foo\n@@ -1 +1 @@\n-foo\n+bar\n(1/1) Stage this hunk [y,n,q,a,d,e,?]?\n```\n\nActual behavior:\n`a/` and `b/` prefixes are shown instead. Like this:\n```\ndiff --git a/foo b/foo\nindex 257cc56..1715acd 100644\n--- a/foo\n+++ b/foo\n@@ -1 +1 @@\n-foo\n+bar\n(1/1) Stage this hunk [y,n,q,a,d,e,?]?\n```\n\n[System Info]\ngit version:\ngit version 2.30.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.4.98-1-lts #1 SMP Sat, 13 Feb 2021 19:22:14 +0000 x86_64\ncompiler info: gnuc: 10.2\nlibc info: glibc: 2.33\n$SHELL (typically, interactive shell): /bin/zsh\n\n[Enabled Hooks]\n"},{"id":"421095","messageId":"YGzUerPL7V8jDxHo@coredump.intra.peff.net","threadId":"55440","inReplyTo":"CAMJzOtyzu8y5mWdKXe3MPe8ZoJs8O=me8Xuu0t77YVdAMc7Tgg@mail.gmail.com","subject":"Re: [Bug report] diff.noprefix config is ignored for interactive `add`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-06T21:36:58Z","receivedAt":"2021-04-06T21:37:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 06, 2021 at 12:15:28PM +0200, Nikita Bobko wrote:\n\n> Steps to reproduce:\n> 1. Run `git -c diff.noprefix=true add -p`\n> \n> Expected behavior:\n> `a/` and `b/` prefixes are not shown during interactive `add`\n> like this:\n> [...]\n> Actual behavior:\n> `a/` and `b/` prefixes are shown instead. Like this:\n\nThat's not too surprising. The add-interactive script is built around\nthe diff-tree plumbing command, which does not respect a lot of\n\"cosmetic\" config, especially if it would make the diff hard to parse.\nSo the add-interactive command would have to manually plumb through the\nconfig option, like we do for some other options.\n\nIn this case, I think you'd want to enable the option only for the\nversion of the diff we show to the user (since the machine-readable one\nhas to actually be parsed by git-apply). This is what we do for\nsupporting color diffs, for example.\n\nI imagine something like this:\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex bc3a1e8eff..1a249e15cb 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -43,6 +43,12 @@\n \t\t$repo->get_color('color.diff.new', 'green'),\n \t) : ();\n \n+# options to feed to diff to produce human-readable version\n+my @human_diff_opts = (\n+\t$diff_use_color ? '--color' : (),\n+\t$repo->config_bool('diff.noprefix') ? qw(--no-prefix) : (),\n+);\n+\n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n my $diff_algorithm = $repo->config('diff.algorithm');\n@@ -714,8 +720,8 @@ sub parse_diff {\n \t}\n \tmy @diff = run_cmd_pipe(\"git\", @diff_cmd, qw(--no-color --), $path);\n \tmy @colored = ();\n-\tif ($diff_use_color) {\n-\t\tmy @display_cmd = (\"git\", @diff_cmd, qw(--color --), $path);\n+\tif (@human_diff_opts) {\n+\t\tmy @display_cmd = (\"git\", @diff_cmd, @human_diff_opts, '--', $path);\n \t\tif (defined $diff_filter) {\n \t\t\t# quotemeta is overkill, but sufficient for shell-quoting\n \t\t\tmy $diff = join(' ', map { quotemeta } @display_cmd);\n\nwould work, but I didn't really test it. There's another hitch, which is\nthat this subsystem has all been re-written in C. So we'd really want to\nimplement it in the new code (possibly in both places, though maybe it\nis time to consider cutting over from the perl script to the C one by\ndeafult?).\n\nBut hopefully this illustrates the general idea, and gives somebody\ninterested in the feature enough to work up their own patch.\n\n-Peff\n"},{"id":"421097","messageId":"xmqqsg4313fp.fsf@gitster.g","threadId":"55440","inReplyTo":"YGzUerPL7V8jDxHo@coredump.intra.peff.net","subject":"Re: [Bug report] diff.noprefix config is ignored for interactive `add`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-06T21:57:46Z","receivedAt":"2021-04-06T21:57:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I imagine something like this:\n> ...\n> would work, but I didn't really test it. There's another hitch, which is\n> that this subsystem has all been re-written in C. So we'd really want to\n> implement it in the new code (possibly in both places, though maybe it\n> is time to consider cutting over from the perl script to the C one by\n> deafult?).\n>\n> But hopefully this illustrates the general idea, and gives somebody\n> interested in the feature enough to work up their own patch.\n\nThese \"cosmetic appearance\" configuration that would affect the\noutput from diff shown to the user would not be limited to just the\n.noprefix, though.  Depending on the users, they would care just as\ndeeply about any of these:\n\n\t.context\n\t.interHunkContext\n\t.mnemonicPrefix\n        .noprefix\n\t.relative\n\t.orderFile\n\nas Nikita does for .noprefix to send a bug report.\n\nLuckily or unluckily, .suppressBlankEmpty and the per-filetype\n.xfuncname patterns do impact the output from the plumbing, because\ngit_diff_basic_config() does read them, even though they are merely\n\"cosmetic\" configurations.\n\nI am unsure how much we should cater to end-user controlled\nconfiguration when we are generating diff output for our own\nconsumption, but if we were to tweak \"add -p\" and friends to\npay attention to .noprefix, we probably should do the same for\nall the others.\n\nThanks.\n\n\n"},{"id":"421099","messageId":"YGzk/C9V3jkPr3xo@coredump.intra.peff.net","threadId":"55440","inReplyTo":"xmqqsg4313fp.fsf@gitster.g","subject":"Re: [Bug report] diff.noprefix config is ignored for interactive `add`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-06T22:47:24Z","receivedAt":"2021-04-06T22:47:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 06, 2021 at 02:57:46PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I imagine something like this:\n> > ...\n> > would work, but I didn't really test it. There's another hitch, which is\n> > that this subsystem has all been re-written in C. So we'd really want to\n> > implement it in the new code (possibly in both places, though maybe it\n> > is time to consider cutting over from the perl script to the C one by\n> > deafult?).\n> >\n> > But hopefully this illustrates the general idea, and gives somebody\n> > interested in the feature enough to work up their own patch.\n> \n> These \"cosmetic appearance\" configuration that would affect the\n> output from diff shown to the user would not be limited to just the\n> .noprefix, though.  Depending on the users, they would care just as\n> deeply about any of these:\n> \n> \t.context\n> \t.interHunkContext\n> \t.mnemonicPrefix\n>         .noprefix\n> \t.relative\n> \t.orderFile\n> \n> as Nikita does for .noprefix to send a bug report.\n> \n> Luckily or unluckily, .suppressBlankEmpty and the per-filetype\n> .xfuncname patterns do impact the output from the plumbing, because\n> git_diff_basic_config() does read them, even though they are merely\n> \"cosmetic\" configurations.\n> \n> I am unsure how much we should cater to end-user controlled\n> configuration when we are generating diff output for our own\n> consumption, but if we were to tweak \"add -p\" and friends to\n> pay attention to .noprefix, we probably should do the same for\n> all the others.\n\nYes. We already have gone through this with other options (e.g.,\ndiff.algorithm). I would be happy if somebody wanted to handle the\ncomplete set. But I am also OK with stumbling towards completeness, as\npeople who care about a particular option plumb it through.\n\n-Peff\n"}]}