git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Igor Djordjevic <igor.d.djordjevic@gmail.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: alexmv@dropbox.com, git@vger.kernel.org
Subject: Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
Date: Tue, 26 Dec 2017 23:14:04 +0100	[thread overview]
Message-ID: <cd764b8d-8a85-964f-eaf2-3e6fb784a2ff@gmail.com> (raw)
In-Reply-To: <20171226091012.24315-8-pclouds@gmail.com>

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 <file>..." to unstage)
    
            renamed:    first -> second
    
    Changes not staged for commit:
      (use "git add/rm <file>..." to update what will be committed)
      (use "git checkout -- <file>..." to discard changes in working directory)
    
            deleted:    second
    
    Untracked files:
      (use "git add <file>..." 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 <file>..." to unstage)
    
            renamed:    first -> second
    
    Changes not staged for commit:
      (use "git add <file>..." to update what will be committed)
      (use "git checkout -- <file>..." 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 000000 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second        first

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 <file>..." to unstage)
    
    	renamed:    first -> second
    
    Changes not staged for commit:
      (use "git add <file>..." to update what will be committed)
      (use "git checkout -- <file>..." 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 000000 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second	first
    1 .A N... 000000 000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 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.

So, while we still changed output of what we were showing so far to 
two-line output, it seems there`s no real gain, as it looks like we 
replaced one partial output (recognize rename, omit path) for the 
other (recognize path, omit rename).

Finally, let`s see your initial patch v1[1], with my exercise 
patch[2] on top:

(1) $ git status
    On branch master
    Changes to be committed:
      (use "git reset HEAD <file>..." to unstage)
    
    	renamed:    first -> second
    
    Changes not staged for commit:
      (use "git add <file>..." to update what will be committed)
      (use "git checkout -- <file>..." to discard changes in working directory)
    
    	renamed:    second -> third

(2) $ git status --porcelain
    R  first -> second
     R second -> third

(3) $ git status --porcelain=v2
    2 R. N... 100644 100644 100644 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second	first
    2 .R N... 100644 100644 100644 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 third	second

Here, both "--porcelain" outputs (2) and (3) seem to much better 
replicate what default `git status` is showing, too - namely separate 
renames in comparison to HEAD for both "index" (2) and "working tree" (3).

And if you don`t like two lines here in comparison to one (incomplete) 
line from Git v2.15.1, I would remark that patch series v2 prints two 
lines as well (so different from v2.15.1 in a same way), but with 
what looks like inferior output in comparison to v1 shown above, where 
both renames are correctly recognized and reported - and finally 
fully compatible with default `git status` output, too.

And if we really think about it, what v1 shows is what actually 
happened - and more important, it`s possible to recreate hypothetical 
"first -> second -> third" change from there. With v2 output, that is 
impossible, that information is lost as second line doesn`t relate to 
the first one in any way.

Now, unless I`m totally missing something here, the only thing left 
is that you mentioned v2 approach being "safer and simpler" than v1, 
something I`m not really competent to comment on, but just wanted to 
provide a second opinion, maybe helping to change your mind in favor 
of v1 outputs, which seem to be _the_ correct ones...? :)

If not that much more complicated/unsafe, of course.

Thanks, Buga

[1] https://public-inbox.org/git/20171226091012.24315-8-pclouds@gmail.com/T/#mf60e88fd351f7ff6a076279794c8343a79835f67
[2] https://public-inbox.org/git/20171226091012.24315-8-pclouds@gmail.com/T/#m095c33d69994c6ecb4f1adbf80dd48eab66750d8

  reply	other threads:[~2017-12-26 22:14 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-23  2:42 [BUG] File move with `add -N` shows as rename to same name Alex Vandiver
2017-12-25  9:00 ` Duy Nguyen
2017-12-25 10:37 ` [PATCH] status: handle worktree renames Nguyễn Thái Ngọc Duy
2017-12-25 18:26   ` Igor Djordjevic
2017-12-25 19:45     ` Igor Djordjevic
2017-12-25 21:49       ` Igor Djordjevic
2017-12-26  2:11     ` Duy Nguyen
2017-12-26  2:53       ` Duy Nguyen
2017-12-27 18:17         ` Junio C Hamano
2017-12-27 18:12       ` Junio C Hamano
2018-01-02 21:14         ` Jeff Hostetler
2018-01-10  9:26           ` Duy Nguyen
2017-12-26  9:10   ` [PATCH v2 0/7] Renames in git-status "changed not staged" section Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 1/7] t2203: test status output with porcelain v2 format Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 2/7] Use DIFF_DETECT_RENAME for detect_rename assignments Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 3/7] wt-status.c: coding style fix Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 4/7] wt-status.c: rename wt_status_change_data::score Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 5/7] wt-status.c: catch unhandled diff status codes Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 6/7] wt-status.c: handle worktree renames Nguyễn Thái Ngọc Duy
2017-12-26 18:14       ` Igor Djordjevic
2017-12-27  1:06         ` Duy Nguyen
2017-12-28  0:50           ` Igor Djordjevic
2017-12-28  2:14             ` Igor Djordjevic
2017-12-26  9:10     ` [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format Nguyễn Thái Ngọc Duy
2017-12-26 22:14       ` Igor Djordjevic [this message]
2017-12-27  0:49         ` Duy Nguyen
2017-12-27 23:53           ` Igor Djordjevic
2017-12-27 10:18     ` [PATCH v3 0/6] Renames in git-status "changed not staged" section Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 1/6] t2203: test status output with porcelain v2 format Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 2/6] Use DIFF_DETECT_RENAME for detect_rename assignments Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 3/6] wt-status.c: coding style fix Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 4/6] wt-status.c: catch unhandled diff status codes Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 5/6] wt-status.c: rename rename-related fields in wt_status_change_data Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 6/6] wt-status.c: handle worktree renames Nguyễn Thái Ngọc Duy
2017-12-28  0:59       ` [PATCH v3 0/6] Renames in git-status "changed not staged" section Igor Djordjevic
2018-01-02 21:22       ` Jeff Hostetler
2017-12-26 18:04   ` [PATCH] status: handle worktree renames Torsten Bögershausen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cd764b8d-8a85-964f-eaf2-3e6fb784a2ff@gmail.com \
    --to=igor.d.djordjevic@gmail.com \
    --cc=alexmv@dropbox.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).