{"thread":{"id":"59342","subject":"[PATCH] add -p: obey diff.noprefix option if set","startedAt":"2023-03-04T12:39:17Z","lastAt":"2023-03-06T10:31:51Z","messageCount":4,"participants":["Marcel Partap","Jeff King","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473003","messageId":"20230304123900.358048-1-mpartap@gmx.net","threadId":"59342","inReplyTo":null,"subject":"[PATCH] add -p: obey diff.noprefix option if set","fromName":"Marcel Partap","fromEmail":"mpartap@gmx.net","sentAt":"2023-03-04T12:39:00Z","receivedAt":"2023-03-04T12:39:17Z","isPatch":true,"sender":{"key":"mpartap@gmx.net","avatar":"https://gravatar.com/avatar/48334bf11d5314a55b0d9bb4e0eeb8f9ccd836ed2f978181850423d8ddf284ee?d=mp&s=160"},"body":"If the user has set the diff.noprefix option, he likely will expect\nthis display setting to also apply when interactively adding hunks.\n\nSigned-off-by: Marcel Partap <mpartap@gmx.net>\n---\n add-patch.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git add-patch.c add-patch.c\nindex a86a92e164..520faae9cb 100644\n--- add-patch.c\n+++ add-patch.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"config.h\"\n #include \"add-interactive.h\"\n #include \"strbuf.h\"\n #include \"run-command.h\"\n@@ -404,11 +405,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \tsize_t file_diff_alloc = 0, i, color_arg_index;\n \tstruct file_diff *file_diff = NULL;\n \tstruct hunk *hunk = NULL;\n-\tint res;\n+\tint res, noprefix;\n\n \tstrvec_pushv(&args, s->mode->diff_cmd);\n \tif (diff_algorithm)\n \t\tstrvec_pushf(&args, \"--diff-algorithm=%s\", diff_algorithm);\n+\tif (!git_config_get_bool(\"diff.noprefix\", &noprefix) && noprefix)\n+\t\tstrvec_pushf(&args, \"--no-prefix\");\n \tif (s->revision) {\n \t\tstruct object_id oid;\n \t\tstrvec_push(&args,\n--\n2.38.1\n\n"},{"id":"473042","messageId":"ZAWnDUkgO5clf6qu@coredump.intra.peff.net","threadId":"59342","inReplyTo":"20230304123900.358048-1-mpartap@gmx.net","subject":"Re: [PATCH] add -p: obey diff.noprefix option if set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-06T08:40:45Z","receivedAt":"2023-03-06T08:40:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 04, 2023 at 01:39:00PM +0100, Marcel Partap wrote:\n\n> If the user has set the diff.noprefix option, he likely will expect\n> this display setting to also apply when interactively adding hunks.\n\nI think it's reasonable for the interactive display to respect the\nconfigured preferences here. But unfortunately, it's not quite as simple\nas your patch.\n\n> diff --git add-patch.c add-patch.c\n> index a86a92e164..520faae9cb 100644\n\nA semi-aside: I note that this patch was also generated with\ndiff.noprefix. It has to be applied with \"git am -p0\" (and anybody\nreceiving it has to know to do that).\n\nThe \"aside\" part is that this is (IMHO) a bug or at least a misfeature\nin format-patch. Looks like it has come up a few times recently, too\n(though AFAICT it has been this way since the option was added in 2010):\n\n  https://lore.kernel.org/git/xmqqr1auvs7m.fsf@gitster.g/\n\n  https://lore.kernel.org/git/CAAHpriMPdahH2xbrrQbeCJPYpLhr6tuvT6xsG3nACmskKF1v2w@mail.gmail.com/\n\nThe not-aside part is that this same problem is important for what your\npatch is trying to do. ;)\n\nIf we generate the diff with \"--no-prefix\", then it has to be applied\nwith \"-p0\". But your patch touches only the generation side, so it\ndoesn't work at all:\n\n  $ echo foo >>Makefile\n  $ ./git -c diff.noprefix add -p\n  diff --git Makefile Makefile\n  [...etc...]\n  +foo\n  (1/1) Stage this hunk [y,n,q,a,d,e,?]? y\n  error: git diff header lacks filename information when removing 1 leading pathname component (line 5)\n  error: 'git apply' failed\n\nThere are two options, I think.\n\nOne is that we have a similar issue with color. To handle that, we\ngenerate the diff twice, once with color and once without. We could\nprobably do the same thing here, by sticking the \"--no-prefix\" part with\nthe color setup. Though it turns out to be a little tricky to do because\nof the way the code is written, and IIRC there are probably some corner\ncases lurking (e.g., after splitting, I think we'll try to re-colorize\nthe diff headers ourselves).\n\nThe second is to just remember that we set noprefix and to add the\nmatching \"-p0\". Unfortunately we have to do so in a few places, but it's\nnot _too_ bad (and possibly some refactoring could make it less ugly).\nSomething like:\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 520faae9cba..6e5390621c0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1189,13 +1189,16 @@ static int run_apply_check(struct add_p_state *s,\n \t\t\t   struct file_diff *file_diff)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tint noprefix;\n \n \tstrbuf_reset(&s->buf);\n \treassemble_patch(s, file_diff, 1, &s->buf);\n \n \tsetup_child_process(s, &cp,\n \t\t\t    \"apply\", \"--check\", NULL);\n \tstrvec_pushv(&cp.args, s->mode->apply_check_args);\n+\tif (!git_config_get_bool(\"diff.noprefix\", &noprefix) && noprefix)\n+\t\tstrvec_pushf(&cp.args, \"-p1\");\n \tif (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0))\n \t\treturn error(_(\"'git apply --cached' failed\"));\n \n@@ -1695,7 +1698,10 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\tapply_for_checkout(s, &s->buf,\n \t\t\t\t\t   s->mode->is_reverse);\n \t\telse {\n+\t\t\tint noprefix;\n \t\t\tsetup_child_process(s, &cp, \"apply\", NULL);\n+\t\t\tif (!git_config_get_bool(\"diff.noprefix\", &noprefix) && noprefix)\n+\t\t\t\tstrvec_pushf(&cp.args, \"-p0\");\n \t\t\tstrvec_pushv(&cp.args, s->mode->apply_args);\n \t\t\tif (pipe_command(&cp, s->buf.buf, s->buf.len,\n \t\t\t\t\t NULL, 0, NULL, 0))\n\n>  add-patch.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n\nWe'd probably want at least one test using \"add -p\" with diff.noprefix\n(probably in t3701). That would demonstrate that the feature works, as\nwell as protect it from future regressions (the test suite doesn't fail\neven with your broken patch because no test sets noprefix).\n\n-Peff\n"},{"id":"473045","messageId":"0ba2f495-892c-3e27-a32c-9f136e86fc26@dunelm.org.uk","threadId":"59342","inReplyTo":"ZAWnDUkgO5clf6qu@coredump.intra.peff.net","subject":"Re: [PATCH] add -p: obey diff.noprefix option if set","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-06T09:39:07Z","receivedAt":"2023-03-06T09:39:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/03/2023 08:40, Jeff King wrote:\n> On Sat, Mar 04, 2023 at 01:39:00PM +0100, Marcel Partap wrote:\n> There are two options, I think.\n> \n> One is that we have a similar issue with color. To handle that, we\n> generate the diff twice, once with color and once without. We could\n> probably do the same thing here, by sticking the \"--no-prefix\" part with\n> the color setup. Though it turns out to be a little tricky to do because\n> of the way the code is written, and IIRC there are probably some corner\n> cases lurking (e.g., after splitting, I think we'll try to re-colorize\n> the diff headers ourselves).\n> \n> The second is to just remember that we set noprefix and to add the\n> matching \"-p0\". Unfortunately we have to do so in a few places, but it's\n> not _too_ bad (and possibly some refactoring could make it less ugly).\n> Something like:\n\nI think that is the better approach. Looking at how we handle \ndiff.algorithm we should maybe add a \"noprefix\" member to \"struct \nadd_i_state\" and initialize it in init_add_i_state() (which is in \nadd-interactive.c). That way we're consistent with the existing code and \nwe don't need to keep calling git_config_get_bool() whenever we want the \nvalue of diff.noPrefix.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 520faae9cba..6e5390621c0 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1189,13 +1189,16 @@ static int run_apply_check(struct add_p_state *s,\n>   \t\t\t   struct file_diff *file_diff)\n>   {\n>   \tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tint noprefix;\n>   \n>   \tstrbuf_reset(&s->buf);\n>   \treassemble_patch(s, file_diff, 1, &s->buf);\n>   \n>   \tsetup_child_process(s, &cp,\n>   \t\t\t    \"apply\", \"--check\", NULL);\n>   \tstrvec_pushv(&cp.args, s->mode->apply_check_args);\n> +\tif (!git_config_get_bool(\"diff.noprefix\", &noprefix) && noprefix)\n> +\t\tstrvec_pushf(&cp.args, \"-p1\");\n\nI think you meant \"-p0\" here\n\nBest Wishes\n\nPhillip\n\n>   \tif (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0))\n>   \t\treturn error(_(\"'git apply --cached' failed\"));\n>   \n> @@ -1695,7 +1698,10 @@ static int patch_update_file(struct add_p_state *s,\n>   \t\t\tapply_for_checkout(s, &s->buf,\n>   \t\t\t\t\t   s->mode->is_reverse);\n>   \t\telse {\n> +\t\t\tint noprefix;\n>   \t\t\tsetup_child_process(s, &cp, \"apply\", NULL);\n> +\t\t\tif (!git_config_get_bool(\"diff.noprefix\", &noprefix) && noprefix)\n> +\t\t\t\tstrvec_pushf(&cp.args, \"-p0\");\n>   \t\t\tstrvec_pushv(&cp.args, s->mode->apply_args);\n>   \t\t\tif (pipe_command(&cp, s->buf.buf, s->buf.len,\n>   \t\t\t\t\t NULL, 0, NULL, 0))\n> \n>>   add-patch.c | 5 ++++-\n>>   1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> We'd probably want at least one test using \"add -p\" with diff.noprefix\n> (probably in t3701). That would demonstrate that the feature works, as\n> well as protect it from future regressions (the test suite doesn't fail\n> even with your broken patch because no test sets noprefix).\n> \n> -Peff\n"},{"id":"473047","messageId":"ZAXA5ikipcTCfptl@coredump.intra.peff.net","threadId":"59342","inReplyTo":"0ba2f495-892c-3e27-a32c-9f136e86fc26@dunelm.org.uk","subject":"Re: [PATCH] add -p: obey diff.noprefix option if set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-06T10:31:02Z","receivedAt":"2023-03-06T10:31:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 06, 2023 at 09:39:07AM +0000, Phillip Wood wrote:\n\n> > The second is to just remember that we set noprefix and to add the\n> > matching \"-p0\". Unfortunately we have to do so in a few places, but it's\n> > not _too_ bad (and possibly some refactoring could make it less ugly).\n> > Something like:\n> \n> I think that is the better approach. Looking at how we handle diff.algorithm\n> we should maybe add a \"noprefix\" member to \"struct add_i_state\" and\n> initialize it in init_add_i_state() (which is in add-interactive.c). That\n> way we're consistent with the existing code and we don't need to keep\n> calling git_config_get_bool() whenever we want the value of diff.noPrefix.\n\nYeah, that was exactly the kind of refactoring I had in mind (but I\ndidn't work on it, even as a \"maybe something like this\" patch).\n\nI agree it's the better approach.\n\n> >   \tstrvec_pushv(&cp.args, s->mode->apply_check_args);\n> > +\tif (!git_config_get_bool(\"diff.noprefix\", &noprefix) && noprefix)\n> > +\t\tstrvec_pushf(&cp.args, \"-p1\");\n> \n> I think you meant \"-p0\" here\n\nWhoops, yes.\n\n-Peff\n"}]}