Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format

[Igor Djordjevic]

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

[Duy Nguyen]

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 

Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format

[Igor Djordjevic]

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