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
next prev parent 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).