{"thread":{"id":"53543","subject":"Question about behaviour of git-checkout --patch option","startedAt":"2020-05-25T20:17:47Z","lastAt":"2020-05-27T21:27:22Z","messageCount":7,"participants":["Merlin Büge","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"398551","messageId":"20200525221100.31d36d4d.toni@bluenox07.de","threadId":"53543","inReplyTo":null,"subject":"Question about behaviour of git-checkout --patch option","fromName":"Merlin Büge","fromEmail":"toni@bluenox07.de","sentAt":"2020-05-25T20:11:00Z","receivedAt":"2020-05-25T20:17:47Z","isPatch":false,"sender":{"key":"toni@bluenox07.de","avatar":null},"body":"Hi all!\n\n\nI have a question about the behaviour of the git-checkout --patch/-p\noption.\n\nI have a small local git repository containing empty and non-empty\nfiles. I wanted to replicate it in another directory and did the\nfollowing:\n\n\tcd <target_dir>\n\tgit init\n\tgit remote add origin <origin_dir>\n\tgit fetch\n\tgit branch --track master origin/master\n\tgit checkout master .\n\nThis works like expected, I end up with a 1:1 copy of the original\nworktree, including empty files. However, if I include the -p option in\nthe last step:\n\n\tgit checkout -p master .\n\n... I correctly get asked for any non-empty files/hunks if I want to\napply them - but not for empty ones. It would just display e.g.\n\n\tdiff --git b/emptyfile a/emptyfile\n\tnew file mode 100644\n\tindex 0000000..e69de29\n\nand then skip over it, asking for the next non-empty hunk.\n\nWhy does it skip over empty hunks?\n\n\nThanks!\n\n-- \nMerlin Büge\n"},{"id":"398618","messageId":"20200527075648.GA4006373@coredump.intra.peff.net","threadId":"53543","inReplyTo":"20200525221100.31d36d4d.toni@bluenox07.de","subject":"Re: Question about behaviour of git-checkout --patch option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-27T07:56:48Z","receivedAt":"2020-05-27T07:56:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 25, 2020 at 10:11:00PM +0200, Merlin Büge wrote:\n\n> This works like expected, I end up with a 1:1 copy of the original\n> worktree, including empty files. However, if I include the -p option in\n> the last step:\n> \n> \tgit checkout -p master .\n> \n> ... I correctly get asked for any non-empty files/hunks if I want to\n> apply them - but not for empty ones. It would just display e.g.\n> \n> \tdiff --git b/emptyfile a/emptyfile\n> \tnew file mode 100644\n> \tindex 0000000..e69de29\n> \n> and then skip over it, asking for the next non-empty hunk.\n> \n> Why does it skip over empty hunks?\n\nI think this case was just never anticipated, and it's a bug. The\noriginal patch-selection code was written for \"add -p\", and the\nfundamental unit it works on is a hunk. We hacked around that to handle\ndeletions back in 24ab81ae4d (add-interactive: handle deletion of empty\nfiles, 2009-10-27). But \"add -p\" would never see a new file, since we\nonly consider the set of tracked files in the index.\n\nHowever, when it was extended for \"checkout -p\", etc, we could see new\nfiles. I guess the right fix is something along these lines, extending\nthe deletion concept to new files:\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex c4b75bcb7f..9c8844434e 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -768,7 +768,7 @@ sub parse_diff_header {\n \tfor (my $i = 0; $i < @{$src->{TEXT}}; $i++) {\n \t\tmy $dest =\n \t\t   $src->{TEXT}->[$i] =~ /^(old|new) mode (\\d+)$/ ? $mode :\n-\t\t   $src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :\n+\t\t   $src->{TEXT}->[$i] =~ /^(deleted|new) file/ ? $deletion :\n \t\t   $head;\n \t\tpush @{$dest->{TEXT}}, $src->{TEXT}->[$i];\n \t\tpush @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];\n\nwhich does do the right thing. But:\n\n  - all of this is in the process of being rewritten in C, so it should\n    probably go into add-interactive.c (it's not yet the default to use\n    the C version, but it probably will be soon).\n\n  - that still says \"Apply deletion...\" in the UI. We'd need to update\n    the messages to differentiate the two cases.\n\n-Peff\n"},{"id":"398619","messageId":"20200527080009.GB4006373@coredump.intra.peff.net","threadId":"53543","inReplyTo":"20200527075648.GA4006373@coredump.intra.peff.net","subject":"Re: Question about behaviour of git-checkout --patch option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-27T08:00:09Z","receivedAt":"2020-05-27T08:00:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2020 at 03:56:48AM -0400, Jeff King wrote:\n\n> which does do the right thing. But:\n> \n>   - all of this is in the process of being rewritten in C, so it should\n>     probably go into add-interactive.c (it's not yet the default to use\n>     the C version, but it probably will be soon).\n\nTrying your case with:\n\n  git -c add.interactive.usebuiltin=true checkout -p master .\n\nshows that it does not list the added file at all. I suspect the form of\nthe fix will be similar, but it may have to plumb through the file\naddition from the diff layer, as well.\n\n-Peff\n"},{"id":"398623","messageId":"20200527123234.737efc3f.toni@bluenox07.de","threadId":"53543","inReplyTo":"20200527080009.GB4006373@coredump.intra.peff.net","subject":"Re: Question about behaviour of git-checkout --patch option","fromName":"Merlin Büge","fromEmail":"toni@bluenox07.de","sentAt":"2020-05-27T10:32:34Z","receivedAt":"2020-05-27T10:32:40Z","isPatch":false,"sender":{"key":"toni@bluenox07.de","avatar":null},"body":"\n\nOn Wed, 27 May 2020 04:00:09 -0400\nJeff King <peff@peff.net> wrote:\n\n> Trying your case with:\n> \n>   git -c add.interactive.usebuiltin=true checkout -p master .\n> \n> shows that it does not list the added file at all. I suspect the form of\n> the fix will be similar, but it may have to plumb through the file\n> addition from the diff layer, as well.\n\nThanks a lot for your explanation! Good to know what's going on there.\n\nOut of curiosity: How does the git community keep track of open bugs?\nOn https://git-scm.com/ I read that bug reports should be just send to\nthe mailing list. But if they are not fixed within a few days, wouldn't\nthey just get lost...?\n\nThanks.\n\n-- \nMerlin Büge\n"},{"id":"398680","messageId":"nycvar.QRO.7.76.6.2005270348240.56@tvgsbejvaqbjf.bet","threadId":"53543","inReplyTo":"20200527075648.GA4006373@coredump.intra.peff.net","subject":"Re: Question about behaviour of git-checkout --patch option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-27T01:49:25Z","receivedAt":"2020-05-27T18:23:35Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Merlin,\n\nOn Wed, 27 May 2020, Jeff King wrote:\n\n> On Mon, May 25, 2020 at 10:11:00PM +0200, Merlin Büge wrote:\n>\n> > This works like expected, I end up with a 1:1 copy of the original\n> > worktree, including empty files. However, if I include the -p option in\n> > the last step:\n> >\n> > \tgit checkout -p master .\n> >\n> > ... I correctly get asked for any non-empty files/hunks if I want to\n> > apply them - but not for empty ones. It would just display e.g.\n> >\n> > \tdiff --git b/emptyfile a/emptyfile\n> > \tnew file mode 100644\n> > \tindex 0000000..e69de29\n> >\n> > and then skip over it, asking for the next non-empty hunk.\n> >\n> > Why does it skip over empty hunks?\n>\n> I think this case was just never anticipated, and it's a bug. The\n> original patch-selection code was written for \"add -p\", and the\n> fundamental unit it works on is a hunk. We hacked around that to handle\n> deletions back in 24ab81ae4d (add-interactive: handle deletion of empty\n> files, 2009-10-27). But \"add -p\" would never see a new file, since we\n> only consider the set of tracked files in the index.\n>\n> However, when it was extended for \"checkout -p\", etc, we could see new\n> files. I guess the right fix is something along these lines, extending\n> the deletion concept to new files:\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index c4b75bcb7f..9c8844434e 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -768,7 +768,7 @@ sub parse_diff_header {\n>  \tfor (my $i = 0; $i < @{$src->{TEXT}}; $i++) {\n>  \t\tmy $dest =\n>  \t\t   $src->{TEXT}->[$i] =~ /^(old|new) mode (\\d+)$/ ? $mode :\n> -\t\t   $src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :\n> +\t\t   $src->{TEXT}->[$i] =~ /^(deleted|new) file/ ? $deletion :\n>  \t\t   $head;\n>  \t\tpush @{$dest->{TEXT}}, $src->{TEXT}->[$i];\n>  \t\tpush @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];\n>\n> which does do the right thing. But:\n>\n>   - all of this is in the process of being rewritten in C, so it should\n>     probably go into add-interactive.c (it's not yet the default to use\n>     the C version, but it probably will be soon).\n>\n>   - that still says \"Apply deletion...\" in the UI. We'd need to update\n>     the messages to differentiate the two cases.\n\nThe C version (with \"Apply addition\"... in the UI) would look somewhat\nlike this:\n\n-- snipsnap --\ndiff --git a/add-patch.c b/add-patch.c\nindex d8bfe379be4d..c0d8bf2df0c5 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -10,7 +10,7 @@\n #include \"prompt.h\"\n\n enum prompt_mode_type {\n-\tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK,\n+\tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n \tPROMPT_MODE_MAX, /* must be last */\n };\n\n@@ -33,6 +33,7 @@ static struct patch_mode patch_mode_add = {\n \t.prompt_mode = {\n \t\tN_(\"Stage mode change [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Stage deletion [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Stage addition [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Stage this hunk [y,n,q,a,d%s,?]? \")\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -54,6 +55,7 @@ static struct patch_mode patch_mode_stash = {\n \t.prompt_mode = {\n \t\tN_(\"Stash mode change [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Stash deletion [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Stash addition [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Stash this hunk [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -77,6 +79,7 @@ static struct patch_mode patch_mode_reset_head = {\n \t.prompt_mode = {\n \t\tN_(\"Unstage mode change [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Unstage deletion [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Unstage addition [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Unstage this hunk [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -99,6 +102,7 @@ static struct patch_mode patch_mode_reset_nothead = {\n \t.prompt_mode = {\n \t\tN_(\"Apply mode change to index [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Apply deletion to index [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Apply addition to index [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Apply this hunk to index [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -121,6 +125,7 @@ static struct patch_mode patch_mode_checkout_index = {\n \t.prompt_mode = {\n \t\tN_(\"Discard mode change from worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Discard deletion from worktree [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Discard addition from worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Discard this hunk from worktree [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -143,6 +148,7 @@ static struct patch_mode patch_mode_checkout_head = {\n \t.prompt_mode = {\n \t\tN_(\"Discard mode change from index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Discard deletion from index and worktree [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Discard addition from index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Discard this hunk from index and worktree [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -164,6 +170,7 @@ static struct patch_mode patch_mode_checkout_nothead = {\n \t.prompt_mode = {\n \t\tN_(\"Apply mode change to index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Apply deletion to index and worktree [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Apply addition to index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Apply this hunk to index and worktree [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -186,6 +193,7 @@ static struct patch_mode patch_mode_worktree_head = {\n \t.prompt_mode = {\n \t\tN_(\"Discard mode change from index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Discard deletion from index and worktree [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Discard addition from index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Discard this hunk from index and worktree [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -207,6 +215,7 @@ static struct patch_mode patch_mode_worktree_nothead = {\n \t.prompt_mode = {\n \t\tN_(\"Apply mode change to index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Apply deletion to index and worktree [y,n,q,a,d%s,?]? \"),\n+\t\tN_(\"Apply addition to index and worktree [y,n,q,a,d%s,?]? \"),\n \t\tN_(\"Apply this hunk to index and worktree [y,n,q,a,d%s,?]? \"),\n \t},\n \t.edit_hunk_hint = N_(\"If the patch applies cleanly, the edited hunk \"\n@@ -248,7 +257,7 @@ struct add_p_state {\n \t\tstruct hunk head;\n \t\tstruct hunk *hunk;\n \t\tsize_t hunk_nr, hunk_alloc;\n-\t\tunsigned deleted:1, mode_change:1,binary:1;\n+\t\tunsigned deleted:1, added:1, mode_change:1,binary:1;\n \t} *file_diff;\n \tsize_t file_diff_nr;\n\n@@ -442,7 +451,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \tpend = p + plain->len;\n \twhile (p != pend) {\n \t\tchar *eol = memchr(p, '\\n', pend - p);\n-\t\tconst char *deleted = NULL, *mode_change = NULL;\n+\t\tconst char *deleted = NULL, *added = NULL, *mode_change = NULL;\n\n \t\tif (!eol)\n \t\t\teol = pend;\n@@ -465,7 +474,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\t; /* keep the rest of the file in a single \"hunk\" */\n \t\telse if (starts_with(p, \"@@ \") ||\n \t\t\t (hunk == &file_diff->head &&\n-\t\t\t  skip_prefix(p, \"deleted file\", &deleted))) {\n+\t\t\t  (skip_prefix(p, \"deleted file\", &deleted) ||\n+\t\t\t   skip_prefix(p, \"new file\", &added)))) {\n \t\t\tif (marker == '-' || marker == '+')\n \t\t\t\t/*\n \t\t\t\t * Should not happen; previous hunk did not end\n@@ -485,6 +495,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n\n \t\t\tif (deleted)\n \t\t\t\tfile_diff->deleted = 1;\n+\t\t\telse if (added)\n+\t\t\t\tfile_diff->added = 1;\n \t\t\telse if (parse_hunk_header(s, hunk) < 0)\n \t\t\t\treturn -1;\n\n@@ -537,8 +549,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\t   starts_with(p, \"Binary files \"))\n \t\t\tfile_diff->binary = 1;\n\n-\t\tif (file_diff->deleted && file_diff->mode_change)\n-\t\t\tBUG(\"diff contains delete *and* a mode change?!?\\n%.*s\",\n+\t\tif (!!file_diff->deleted + !!file_diff->added +\n+\t\t    !!file_diff->mode_change > 1)\n+\t\t\tBUG(\"diff can only contain delete *or* add *or* a \"\n+\t\t\t    \"mode change?!?\\n%.*s\",\n \t\t\t    (int)(eol - (plain->buf + file_diff->head.start)),\n \t\t\t    plain->buf + file_diff->head.start);\n\n@@ -1392,11 +1406,13 @@ static int patch_update_file(struct add_p_state *s,\n \t\tif (hunk->splittable_into > 1)\n \t\t\tstrbuf_addstr(&s->buf, \",s\");\n \t\tif (hunk_index + 1 > file_diff->mode_change &&\n-\t\t    !file_diff->deleted)\n+\t\t    !file_diff->deleted && !file_diff->added)\n \t\t\tstrbuf_addstr(&s->buf, \",e\");\n\n \t\tif (file_diff->deleted)\n \t\t\tprompt_mode_type = PROMPT_DELETION;\n+\t\telse if (file_diff->added)\n+\t\t\tprompt_mode_type = PROMPT_ADDITION;\n \t\telse if (file_diff->mode_change && !hunk_index)\n \t\t\tprompt_mode_type = PROMPT_MODE_CHANGE;\n \t\telse\n"},{"id":"398696","messageId":"nycvar.QRO.7.76.6.2005270646550.56@tvgsbejvaqbjf.bet","threadId":"53543","inReplyTo":"20200527123234.737efc3f.toni@bluenox07.de","subject":"Re: Question about behaviour of git-checkout --patch option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-27T04:48:28Z","receivedAt":"2020-05-27T21:22:33Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Merlin,\n\nOn Wed, 27 May 2020, Merlin Büge wrote:\n\n> On Wed, 27 May 2020 04:00:09 -0400\n> Jeff King <peff@peff.net> wrote:\n>\n> > Trying your case with:\n> >\n> >   git -c add.interactive.usebuiltin=true checkout -p master .\n> >\n> > shows that it does not list the added file at all. I suspect the form of\n> > the fix will be similar, but it may have to plumb through the file\n> > addition from the diff layer, as well.\n>\n> Thanks a lot for your explanation! Good to know what's going on there.\n>\n> Out of curiosity: How does the git community keep track of open bugs?\n> On https://git-scm.com/ I read that bug reports should be just send to\n> the mailing list. But if they are not fixed within a few days, wouldn't\n> they just get lost...?\n\nThere is not really any centralized way to keep track of open bugs.\n\nAnd yes, sometimes they get lost after a few days.\n\nIn your case, however, there is now\nhttps://lore.kernel.org/git/pull.646.git.1590613746507.gitgitgadget@gmail.com/T/#u\nwhich is better than just a ticket: it is already a patch.\n\nCiao,\nJohannes\n"},{"id":"398697","messageId":"20200527232717.64b2e124.toni@bluenox07.de","threadId":"53543","inReplyTo":"nycvar.QRO.7.76.6.2005270646550.56@tvgsbejvaqbjf.bet","subject":"Re: Question about behaviour of git-checkout --patch option","fromName":"Merlin Büge","fromEmail":"toni@bluenox07.de","sentAt":"2020-05-27T21:27:17Z","receivedAt":"2020-05-27T21:27:22Z","isPatch":false,"sender":{"key":"toni@bluenox07.de","avatar":null},"body":"\n\nOn Wed, 27 May 2020 06:48:28 +0200 (CEST), Johannes Schindelin wrote:\n\n> There is not really any centralized way to keep track of open bugs.\n> \n> And yes, sometimes they get lost after a few days.\n> \n> In your case, however, there is now\n> https://lore.kernel.org/git/pull.646.git.1590613746507.gitgitgadget@gmail.com/T/#u\n> which is better than just a ticket: it is already a patch.\n\nWow, that was fast!\n\nThanks a lot!\n\n-- \nMerlin Büge\n"}]}