Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
On 27/12/2017 01:49, Duy Nguyen wrote: > > > I lost you a bit here, partially because of what seems to be an > > incomplete setup script, partially because of this last sentence, as > > Git v2.15.1 doesn`t seem to be showing this, so not sure about "like > > we have been showing until now" part...? > > Yeah I missed a "git add -N third" in the setup. And "until now" was a > poor choice of words. It should have been "before 425a28e0a4", where > "new files" could not show up, which prevented rename detection in the > "Changed bot not staged for commit" section in the first place. > ... > Sorry again about "now". Before 425a28e0a4 rename detection would not > kick in to find "second -> third" so people wouldn't know about rename > anyway. Yeah, no worries, I had I hunch that it might be what you meant, but got too involved in all the rest that I forgot to bring that one up, too. Thanks for clarifying, though. Regards, Buga
Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
On Wed, Dec 27, 2017 at 5:14 AM, Igor Djordjevic wrote: > Hi Duy, > > On 26/12/2017 10:10, Nguyễn Thái Ngọc Duy wrote: >> >> The presence of worktree rename leads to an interesting situation, >> what if the same index entry is renamed twice, compared to HEAD and to >> worktree? We can have that with this setup >> >> echo first > first && git add first && git commit -m first >> git mv first second # rename reported in "diff --cached" >> mv second third # rename reported in "diff-files" >> >> For the long format this is fine because we print two "->" rename >> lines, one in the "updated" section, one in "changed" one. >> >> For other output formats, it gets tricky because they combine both >> diffs in one line but can only display one rename per line. The result >> "XY" column of short format, for example, would be "RR" in that case. >> >> This case either needs some extension in short/porcelain format >> to show something crazy like >> >> RR first -> second -> third >> >> or we could show renames as two lines instead of one, for example >> something like this for short form: >> >> R first -> second >> R second -> third >> >> But for now it's safer and simpler to just break the "second -> third" >> rename pair and show >> >> RD first -> second >> A third >> >> like we have been showing until now. >> > > I lost you a bit here, partially because of what seems to be an > incomplete setup script, partially because of this last sentence, as > Git v2.15.1 doesn`t seem to be showing this, so not sure about "like > we have been showing until now" part...? Yeah I missed a "git add -N third" in the setup. And "until now" was a poor choice of words. It should have been "before 425a28e0a4", where "new files" could not show up, which prevented rename detection in the "Changed bot not staged for commit" section in the first place. Though it's not _exactly_ like before. If you replace "ita_invisible_in_index = 1" with "ita_invisible_in_index = 0" in wt-status.c, you effectively roll back 425a28e0a4 and "git status --short" would show RD first -> second AM third The second line is different and is what 425a28e0a4 tries to fix. > Now, still using v2.15.1, let`s see porcelain statuses: > > (2) $ git status --porcelain > RR first -> second > > (3) $ git status --porcelain=v2 > 2 RR N... 100644 100644 00 9c59e24b8393179a5d712de4f990178df5734d99 > 9c59e24b8393179a5d712de4f990178df5734d99 R100 secondfirst > > Here, they both report renames in _both_ index and working tree (RR), > but they show "index" renamed path only ("second", in comparison to > original value in HEAD, "first"). > > I`m inclined to say this doesn`t align with what `git status` shows, > disrespecting `add -N` (or respecting it only partially, through that > second R, but not showing the actual working tree rename, "third"). > > Without influencing porcelain format, and to fully respect `add -N`, > I believe showing two renames (index and working tree) as two lines > would be the correct approach - and that`s what default `git status` > does, too. I agree. What worries me though, is the path in index seems to be be the unique "key" of each line. Porcelain v2 shows this clearer when "second" is always in the same column. By showing two lines with the same key (i.e. "second"), I'm not sure if we are breaking the porcelain format. Perhaps this is undefined area that we can just tweak? > Now, let`s examine this patch series v2 outputs: > > (1) $ git status > On branch master > Changes to be committed: > (use "git reset HEAD ..." to unstage) > > renamed:first -> second > > Changes not staged for commit: > (use "git add ..." to update what will be committed) > (use "git checkout -- ..." to discard changes in working > directory) > > renamed:second -> third > > (2) $ git status --porcelain > RD first -> second > A third > > (3) $ git status --porcelain=v2 > 2 RD N... 100644 100644 00 9c59e24b8393179a5d712de4f990178df5734d99 > 9c59e24b8393179a5d712de4f990178df5734d99 R100 secondfirst > 1 .A N... 00 00 100644 > third > > Here, porcelain statuses make situation a bit better, as now at least > `add -N` is respected, showing new "tracked" path appearing in the > working tree. > > But, we now lost any idea about the rename that happened there as > well - which Git v2.15.1 porcelain was partially showing (through > RR), and which `git status` still reports correctly - and which we > still differ from. Sorry again about "now". Before 425a28e0a4 rename detection would not kick in to find "second -> third" so people wouldn't know about rename anyway. > > I don`t think this looks like what we have been showing until now > (unless I misunderstood which exact "now" are we talking about), so I > don`t see that as a valid argument
Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
Hi Duy, On 26/12/2017 10:10, Nguyễn Thái Ngọc Duy wrote: > > The presence of worktree rename leads to an interesting situation, > what if the same index entry is renamed twice, compared to HEAD and to > worktree? We can have that with this setup > > echo first > first && git add first && git commit -m first > git mv first second # rename reported in "diff --cached" > mv second third # rename reported in "diff-files" > > For the long format this is fine because we print two "->" rename > lines, one in the "updated" section, one in "changed" one. > > For other output formats, it gets tricky because they combine both > diffs in one line but can only display one rename per line. The result > "XY" column of short format, for example, would be "RR" in that case. > > This case either needs some extension in short/porcelain format > to show something crazy like > > RR first -> second -> third > > or we could show renames as two lines instead of one, for example > something like this for short form: > > R first -> second > R second -> third > > But for now it's safer and simpler to just break the "second -> third" > rename pair and show > > RD first -> second > A third > > like we have been showing until now. > I lost you a bit here, partially because of what seems to be an incomplete setup script, partially because of this last sentence, as Git v2.15.1 doesn`t seem to be showing this, so not sure about "like we have been showing until now" part...? Here, with your setup script, with plain Git v2.15.1, we have: $ git status On branch master Changes to be committed: (use "git reset HEAD ..." to unstage) renamed:first -> second Changes not staged for commit: (use "git add/rm ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) deleted:second Untracked files: (use "git add ..." to include in what will be committed) third Might be an additional `git add -N -- third` is needed here, to show what (I assume) you wanted...? If so: $ git add -N third (1) $ git status On branch master Changes to be committed: (use "git reset HEAD ..." to unstage) renamed:first -> second Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) renamed:second -> second ^^ Now we can see two renames I believe you were talking about...? (Note original bug showing above, which started this thread.) Now, still using v2.15.1, let`s see porcelain statuses: (2) $ git status --porcelain RR first -> second (3) $ git status --porcelain=v2 2 RR N... 100644 100644 00 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 secondfirst Here, they both report renames in _both_ index and working tree (RR), but they show "index" renamed path only ("second", in comparison to original value in HEAD, "first"). I`m inclined to say this doesn`t align with what `git status` shows, disrespecting `add -N` (or respecting it only partially, through that second R, but not showing the actual working tree rename, "third"). Without influencing porcelain format, and to fully respect `add -N`, I believe showing two renames (index and working tree) as two lines would be the correct approach - and that`s what default `git status` does, too. Now, let`s examine this patch series v2 outputs: (1) $ git status On branch master Changes to be committed: (use "git reset HEAD ..." to unstage) renamed:first -> second Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) renamed:second -> third (2) $ git status --porcelain RD first -> second A third (3) $ git status --porcelain=v2 2 RD N... 100644 100644 00 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 secondfirst 1 .A N... 00 00 100644 third Here, porcelain statuses make situation a bit better, as now at least `add -N` is respected, showing new "tracked" path appearing in the working tree. But, we now lost any idea about the rename that happened there as well - which Git v2.15.1 porcelain was partially showing (through RR), and which `git status` still reports correctly - and which we still differ from. I don`t think this looks like what we have been showing until now (unless I misunderstood which exact "now" are we talking about), so I don`t see that as a valid argument to support this case.
[PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
The presence of worktree rename leads to an interesting situation, what if the same index entry is renamed twice, compared to HEAD and to worktree? We can have that with this setup echo first > first && git add first && git commit -m first git mv first second # rename reported in "diff --cached" mv second third # rename reported in "diff-files" For the long format this is fine because we print two "->" rename lines, one in the "updated" section, one in "changed" one. For other output formats, it gets tricky because they combine both diffs in one line but can only display one rename per line. The result "XY" column of short format, for example, would be "RR" in that case. This case either needs some extension in short/porcelain format to show something crazy like RR first -> second -> third or we could show renames as two lines instead of one, for example something like this for short form: R first -> second R second -> third But for now it's safer and simpler to just break the "second -> third" rename pair and show RD first -> second A third like we have been showing until now. Signed-off-by: Nguyễn Thái Ngọc Duy --- t/t2203-add-intent.sh | 33 + wt-status.c | 58 --- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index e5bfda1853..79aca93810 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -190,5 +190,38 @@ test_expect_success 'rename detection finds the right names' ' ) ' +test_expect_success 'double rename detection in status' ' + git init rename-detection-2 && + ( + cd rename-detection-2 && + echo contents > first && + git add first && + git commit -m first && + git mv first second && + mv second third && + git add -N third && + + git status | grep -v "^?" >actual.1 && + test_i18ngrep "renamed: *first -> second" actual.1 && + test_i18ngrep "renamed: *second -> third" actual.1 && + + + git status --porcelain | grep -v "^?" >actual.2 && + cat >expected.2 <<-\EOF && + RD first -> second +A third + EOF + test_cmp expected.2 actual.2 && + + oid=12f00e90b6ef79117ce6e650416b8cf517099b78 && + git status --porcelain=v2 | grep -v "^?" >actual.3 && + cat >expected.3 <<-EOF && + 2 RD N... 100644 100644 00 $oid $oid R100 secondfirst + 1 .A N... 00 00 100644 $_z40 $_z40 third + EOF + test_cmp expected.3 actual.3 + ) +' + test_done diff --git a/wt-status.c b/wt-status.c index d5bdf4c2e9..e62853f748 100644 --- a/wt-status.c +++ b/wt-status.c @@ -419,6 +419,47 @@ static char short_submodule_status(struct wt_status_change_data *d) return d->worktree_status; } +static struct string_list_item * break_double_rename( + struct wt_status *s, struct string_list_item *it, + int *status, struct diff_filepair *p) +{ + struct wt_status_change_data *d; + struct string_list_item *new_it; + + d = it->util; + /* +* _collect_index_changes() must have been called or +* d->head_path does not contain a real value. +*/ + if (!d || !d->head_path) + return it; + + switch (s->status_format) { + case STATUS_FORMAT_SHORT: + case STATUS_FORMAT_PORCELAIN: + case STATUS_FORMAT_PORCELAIN_V2: + break; + case STATUS_FORMAT_LONG: + case STATUS_FORMAT_NONE: + /* this output can handle double renames ok */ + return it; + default: + die("BUG: finalize_deferred_config() should have been called"); + } + + switch (*status) { + case DIFF_STATUS_RENAMED: + d->worktree_status = DIFF_STATUS_DELETED; + /* fallthru */ + case DIFF_STATUS_COPIED: + *status = DIFF_STATUS_ADDED; + new_it = string_list_insert(&s->change, p->two->path); + return new_it; + } + + return it; +} + static void wt_status_collect_changed_cb(struct diff_queue_struct *q, struct diff_options *options, void *data) @@ -433,16 +474,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q, struct diff_filepair *p; struct string_list_item *it; struct wt_status_change_data *d; + int status; p = q->queue[i]; + status = p->status; it = string_list_insert(&s->change, p->one->path); + it = break_