File move with `add -N` shows as rename to same name
I just stumbled across the following oddity:
mv tracked-file new-path
git add -N new-path
git status
..shows:
On branch master
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: tracked-file -> tracked-file
no changes added to commit (use "git add" and/or "git commit -a")
Bisect points at 425a28e0a ("diff-lib: allow ita entries treated as "not yet exist in index"", 2016-10-24), but I don't have enough context to suggest the right fix.
Failing test is included below.
- Alex
--------------------8<--------------------
t/t2203-add-intent.sh +14 -0
@@ -150,5 +150,19 @@ test_expect_success 'commit: ita entries ignored in empty commit check' ' ) ' +test_expect_success 'rename detection finds the right names' ' + git init rename-detection && + ( + cd rename-detection && + echo contents > original-file + git add original-file + git commit -m first-commit + mv original-file new-file + git add -N new-file + git status --porcelain > actual + grep new-file actual + ) +' + test_done
--------------------8<--------------------
Quoted 1 line · Alex Vandiver
I just stumbled across the following oddity:
Thanks. I'm looking into it.
Signature · 1 line
Duy
PATCH 1/1 status: handle worktree renames
In reply to Alex VandiverBefore 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist in index" - 2016-10-24) there are never "new files" in the index, which essentially disables rename detection because we only detect renames when a new file appears in a diff pair.
After that commit, an i-t-a entry can appear as a new file in "git diff-files". But the diff callback function in wt-status.c does not handle this case and produces incorrect status output.
Handle this rename case. While at there make sure unhandled diff status code is reported to catch cases like this easier in the future.
The reader may notice that this patch adds a new xstrdup() but not a free(). Yes we leak memory (the same for head_path). But wt_status so far has been short lived, this leak should not matter in practice.
--- t/t2203-add-intent.sh | 15 +++++++++++++++ wt-status.c | 24 +++++++++++++++++++----- wt-status.h | 1 + 3 files changed, 35 insertions(+), 5 deletions(-)
t/t2203-add-intent.sh +15 -0
@@ -150,5 +150,20 @@ test_expect_success 'commit: ita entries ignored in empty commit check' ' ) ' +test_expect_success 'rename detection finds the right names' ' + git init rename-detection && + ( + cd rename-detection && + echo contents > original-file && + git add original-file && + git commit -m first-commit && + mv original-file new-file && + git add -N new-file && + git status --porcelain | grep -v actual >actual && + echo " R original-file -> new-file" >expected && + test_cmp expected actual + ) +' + test_done
wt-status.c +19 -5
@@ -376,6 +376,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,strbuf_addch (&extra,')' ); } status = d->worktree_status; +if (d->worktree_path) + one_name = d->worktree_path;break ;default : die("BUG: unhandled change_type %d in wt_longstatus_print_change_data",
@@ -432,7 +434,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,struct wt_status_change_data *d; p = q->queue[i]; - it =string_list_insert (&s->change, p->one->path); + it =string_list_insert (&s->change, p->two->path); d = it->util;if (!d) { d =xcalloc (1 ,sizeof (*d));
@@ -459,6 +461,12 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,/* mode_worktree is zero for a delete. */ break ; +case DIFF_STATUS_COPIED : +case DIFF_STATUS_RENAMED : + d->worktree_path =xstrdup (p->one->path); + d->score = p->score *100 /MAX_SCORE ; +/* fallthru */ +case DIFF_STATUS_MODIFIED :case DIFF_STATUS_TYPE_CHANGED :case DIFF_STATUS_UNMERGED :
@@ -467,8 +475,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,oidcpy (&d->oid_index, &p->one->oid);break ; -case DIFF_STATUS_UNKNOWN : -die ("BUG: worktree status unknown???" ); +default : +die ("BUG: unhandled worktree status '%c'" , p->status);break ; }
@@ -548,6 +556,10 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,* values in these fields. */ break ; + +default : +die ("BUG: unhandled worktree status '%c'" , p->status); +break ; } } }
@@ -1724,8 +1736,10 @@ static void wt_shortstatus_status(struct string_list_item *it, }else {struct strbuf onebuf =STRBUF_INIT ;const char *one; -if (d->head_path) { - one =quote_path (d->head_path, s->prefix, &onebuf); + + one = d->head_path ? d->head_path : d->worktree_path; +if (one) { + one =quote_path (one, s->prefix, &onebuf);if (*one !='"' &&strchr (one,' ' ) !=NULL ) {putchar ('"' );strbuf_addch (&onebuf,'"' );
wt-status.h +1 -0
@@ -48,6 +48,7 @@ struct wt_status_change_data {int mode_head, mode_index, mode_worktree;struct object_id oid_head, oid_index;char *head_path; +char *worktree_path; unsigned dirty_submodule : 2; unsigned new_submodule_commits : 1; };
Signature · 1 line
2.15.0.320.g0453912d77
Re: [PATCH] status: handle worktree renames
Hi Duy,
Quoted 125 lines · Nguyễn Thái Ngọc Duy
Before
425a28e0a4(diff-lib: allow ita entries treated as "not yet exist in index" - 2016-10-24) there are never "new files" in the index, which essentially disables rename detection because we only detect renames when a new file appears in a diff pair.After that commit, an i-t-a entry can appear as a new file in "git diff-files". But the diff callback function in
wt-status.cdoes not handle this case and produces incorrect status output.Handle this rename case. While at there make sure unhandled diff status code is reported to catch cases like this easier in the future.
The reader may notice that this patch adds a new
xstrdup()but not afree(). Yes we leak memory (the same forhead_path). Butwt_statusso far has been short lived, this leak should not matter in practice.Noticed-by Alex Vandiver <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- t/t2203-add-intent.sh | 15 +++++++++++++++ wt-status.c | 24 +++++++++++++++++++----- wt-status.h | 1 + 3 files changed, 35 insertions(+), 5 deletions(-)t/t2203-add-intent.sh +15
-0@@ -150,5 +150,20 @@ test_expect_success 'commit: ita entries ignored in empty commit check' ' ) ' +test_expect_success 'rename detection finds the right names' ' + git init rename-detection && + ( + cd rename-detection && + echo contents > original-file && + git add original-file && + git commit -m first-commit && + mv original-file new-file && + git add -N new-file && + git status --porcelain | grep -v actual >actual && + echo " R original-file -> new-file" >expected && + test_cmp expected actual + ) +' + test_donewt-status.c +19
-5@@ -376,6 +376,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,strbuf_addch (&extra,')' ); } status = d->worktree_status; +if (d->worktree_path) + one_name = d->worktree_path;break ;default : die("BUG: unhandled change_type %d in wt_longstatus_print_change_data",@@ -432,7 +434,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,struct wt_status_change_data *d; p = q->queue[i]; - it =string_list_insert (&s->change, p->one->path); + it =string_list_insert (&s->change, p->two->path); d = it->util;if (!d) { d =xcalloc (1 ,sizeof (*d));@@ -459,6 +461,12 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,/* mode_worktree is zero for a delete. */ break ; +case DIFF_STATUS_COPIED : +case DIFF_STATUS_RENAMED : + d->worktree_path =xstrdup (p->one->path); + d->score = p->score *100 /MAX_SCORE ; +/* fallthru */ +case DIFF_STATUS_MODIFIED :case DIFF_STATUS_TYPE_CHANGED :case DIFF_STATUS_UNMERGED :@@ -467,8 +475,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,oidcpy (&d->oid_index, &p->one->oid);break ; -case DIFF_STATUS_UNKNOWN : -die ("BUG: worktree status unknown???" ); +default : +die ("BUG: unhandled worktree status '%c'" , p->status);break ; }@@ -548,6 +556,10 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,* values in these fields. */ break ; + +default : +die ("BUG: unhandled worktree status '%c'" , p->status); +break ; } } }@@ -1724,8 +1736,10 @@ static void wt_shortstatus_status(struct string_list_item *it, }else {struct strbuf onebuf =STRBUF_INIT ;const char *one; -if (d->head_path) { - one =quote_path (d->head_path, s->prefix, &onebuf); + + one = d->head_path ? d->head_path : d->worktree_path; +if (one) { + one =quote_path (one, s->prefix, &onebuf);if (*one !='"' &&strchr (one,' ' ) !=NULL ) {putchar ('"' );strbuf_addch (&onebuf,'"' );wt-status.h +0
-0
wt-status.h struct wt_status_change_data { · line 54
Quoted 7 lines
wt-status.h +1
-0@@ -48,6 +48,7 @@ struct wt_status_change_data {int mode_head, mode_index, mode_worktree;struct object_id oid_head, oid_index;char *head_path; +char *worktree_path; unsigned dirty_submodule : 2; unsigned new_submodule_commits : 1; };
Thanks, I`ve tested it and the reported case seems to work correctly, indeed.
But I`ve noticed that "--porcelain=v2" output might still be buggy - this is what having both files staged shows:
$ git status --porcelain=v2
2 R. N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 new-file original-file..., where having old/deleted file unstaged, and new/created file staged with git add -N shows this:
$ git status --porcelain=v2
1 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 new-fileSo even though unstaged value is correctly recognized as "R" (renamed), first number is "1" (instead of "2" to signal rename/copy), and both rename score and original file name are missing.
Not sure if this is a bug, but it seems so, as git status "Porcelain Format Version 2"[1] says the last path is "pathname in the commit at HEAD" (in case of copy/rename), which is missing here.
Regards, Buga
[1] https://git-scm.com/docs/git-status#_porcelain_format_version_2
Re: [PATCH] status: handle worktree renames
Quoted 10 lines · Igor Djordjevic
But I`ve noticed that "--porcelain=v2" output might still be buggy - this is what having both files staged shows:
$ git status --porcelain=v2 2 R. N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 new-file original-file..., where having old/deleted file unstaged, and new/created file staged with
git add -Nshows this:
Quoted 6 lines
$ git status --porcelain=v2 1 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 new-fileSo even though unstaged value is correctly recognized as "R" (renamed), first number is "1" (instead of "2" to signal rename/copy), and both rename score and original file name are missing.
As an exercise, might be something like this as a fixup on top of your patch could work.
I`ve tried to follow your lead on what you did yourself, but please note that, besides being relatively new to Git codebase, this is my first C code for almost 10 years (since university), so... :)
I guess an additional test for this would be good, too.
Regards, Buga
--- wt-status.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
wt-status.c +6 -5
@@ -2050,7 +2050,7 @@ static void wt_porcelain_v2_print_changed_entry(const char *path_head =NULL ;char key[3 ];char submodule_token[5 ]; -char sep_char, eol_char; +char sep_char, eol_char, score_char;wt_porcelain_v2_fix_up_changed (it, s);wt_porcelain_v2_submodule_state (d, submodule_token);
@@ -2059,6 +2059,8 @@ static void wt_porcelain_v2_print_changed_entry( key[1 ] = d->worktree_status ? d->worktree_status :'.' ; key[2 ] =0 ; + path_head = d->head_path ? d->head_path : d->worktree_path; + score_char = d->index_status ? key[0 ] : key[1 ];if (s->null_termination) {/* * In -z mode, we DO NOT C-quote pathnames. Current path is ALWAYS first.
@@ -2067,7 +2069,6 @@ static void wt_porcelain_v2_print_changed_entry( sep_char ='\0' ; eol_char ='\0' ; path_index = it->string; - path_head = d->head_path; }else {/* * Path(s) are C-quoted if necessary. Current path is ALWAYS first.
@@ -2078,8 +2079,8 @@ static void wt_porcelain_v2_print_changed_entry( sep_char ='\t' ; eol_char ='\n' ; path_index =quote_path (it->string, s->prefix, &buf_index); -if (d->head_path) - path_head =quote_path (d->head_path, s->prefix, &buf_head); +if (path_head) + path_head =quote_path (path_head, s->prefix, &buf_head); }if (path_head)
@@ -2087,7 +2088,7 @@ static void wt_porcelain_v2_print_changed_entry( key, submodule_token, d->mode_head, d->mode_index, d->mode_worktree,oid_to_hex (&d->oid_head),oid_to_hex (&d->oid_index), - key[0 ], d->score, + score_char, d->score, path_index, sep_char, path_head, eol_char); else fprintf(s->fp, "1 %s %s %06o %06o %06o %s %s %s%c",
Signature · 1 line
2.15.1.windows.2
Re: [PATCH] status: handle worktree renames
Quoted 2 lines · Igor Djordjevic
I guess an additional test for this would be good, too.
... aaand here it is. Again based on your test, but please double check, Im not sure if its ok to compare file modes like that, expecting them to be the same (hashes should be fine, I guess).
--- t/t2203-add-intent.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
t/t2203-add-intent.sh +15 -0
@@ -165,5 +165,20 @@ test_expect_success 'rename detection finds the right names' ' ) ' +test_expect_success 'rename detection finds the right names (porcelain v2)' ' + git init rename-detection-v2 && + ( + cd rename-detection-v2 && + echo contents > original-file && + git add original-file && + git commit -m first-commit && + mv original-file new-file && + git add -N new-file && + git status --porcelain=v2 | grep -v actual >actual && + echo "2 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 new-file original-file" >expected && + test_cmp expected actual + ) +' + test_done
Signature · 1 line
2.15.1.windows.2
Re: [PATCH] status: handle worktree renames
In reply to Igor DjordjevicQuoted 12 lines · Igor Djordjevic
But I`ve noticed that "--porcelain=v2" output might still be buggy - this is what having both files staged shows:
$ git status --porcelain=v2 2 R. N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 new-file original-file..., where having old/deleted file unstaged, and new/created file staged with
git add -Nshows this:$ git status --porcelain=v2 1 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 new-file
Quoted 7 lines
So even though unstaged value is correctly recognized as "R" (renamed), first number is "1" (instead of "2" to signal rename/copy), and both rename score and original file name are missing.
Not sure if this is a bug, but it seems so, as
git status"Porcelain Format Version 2"[1] says the last path is "pathname in the commit at HEAD" (in case of copy/rename), which is missing here.
Yeah v2 looks problematic. The way the document is written, it's not prepared to deal with a rename pair coming from comparing the index (with intent-to-add entries) with worktree, only from comparing with HEAD. So either we could ajust v2 semantics slightly like this
Documentation/git-status.txt +4 -4
@@ -309,13 +309,13 @@ Renamed or copied entries have the following format: of similarity between the source and target of the move or copy). For example "R100" or "C75". <path> The pathname. In a renamed/copied entry, this - is the path in the index and in the working tree. + is the path in the index. <sep> When the `-z` option is used, the 2 pathnames are separated with a NUL (ASCII 0x00) byte; otherwise, a tab (ASCII 0x09) byte separates them. - <origPath> The pathname in the commit at HEAD. This is only - present in a renamed/copied entry, and tells - where the renamed/copied contents came from. + <origPath> The pathname in the commit at HEAD or in the worktree. + This is only present in a renamed/copied entry, and + tells where the renamed/copied contents came from. -------------------------------------------------------- Unmerged entries have the following format; the first character is
The problem is, you cannot know if it's a rename from HEAD or from worktree with this updated v2 (or perhaps you could because HEAD name should be all zero?).
Or we disable rename-from-worktree when porcelain v2 is requested (and optionally introduce v3 to support it). Jeff, any preference? --
Duy
Re: [PATCH] status: handle worktree renames
Quoted 49 lines · Duy Nguyen
Quoted 12 lines · Igor Djordjevic
But I`ve noticed that "--porcelain=v2" output might still be buggy - this is what having both files staged shows:
$ git status --porcelain=v2 2 R. N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 new-file original-file..., where having old/deleted file unstaged, and new/created file staged with
git add -Nshows this:$ git status --porcelain=v2 1 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 new-fileQuoted 7 lines
So even though unstaged value is correctly recognized as "R" (renamed), first number is "1" (instead of "2" to signal rename/copy), and both rename score and original file name are missing.
Not sure if this is a bug, but it seems so, as
git status"Porcelain Format Version 2"[1] says the last path is "pathname in the commit at HEAD" (in case of copy/rename), which is missing here.Yeah v2 looks problematic. The way the document is written, it's not prepared to deal with a rename pair coming from comparing the index (with intent-to-add entries) with worktree, only from comparing with HEAD. So either we could ajust v2 semantics slightly like this
Documentation/git-status.txt +4
-4@@ -309,13 +309,13 @@ Renamed or copied entries have the following format: of similarity between the source and target of the move or copy). For example "R100" or "C75". <path> The pathname. In a renamed/copied entry, this - is the path in the index and in the working tree. + is the path in the index. <sep> When the `-z` option is used, the 2 pathnames are separated with a NUL (ASCII 0x00) byte; otherwise, a tab (ASCII 0x09) byte separates them. - <origPath> The pathname in the commit at HEAD. This is only - present in a renamed/copied entry, and tells - where the renamed/copied contents came from. + <origPath> The pathname in the commit at HEAD or in the worktree. + This is only present in a renamed/copied entry, and + tells where the renamed/copied contents came from. -------------------------------------------------------- Unmerged entries have the following format; the first character is
Quoted 3 lines
The problem is, you cannot know if it's a rename from HEAD or from worktree with this updated v2 (or perhaps you could because HEAD name should be all zero?).
I'm wrong about this. the "<XY>" code for HEAD rename would be "R." while worktree rename is ".R" so I think we're good.
Signature · 1 line
Duy
Re: [PATCH] status: handle worktree renames
Quoted 6 lines · Duy Nguyen
Quoted 3 lines
The problem is, you cannot know if it's a rename from HEAD or from worktree with this updated v2 (or perhaps you could because HEAD name should be all zero?).
I'm wrong about this. the "<XY>" code for HEAD rename would be "R." while worktree rename is ".R" so I think we're good.
Ah, OK, then. Thanks.
Re: [PATCH] status: handle worktree renames
In reply to Duy NguyenQuoted 2 lines · Duy Nguyen
Or we disable rename-from-worktree when porcelain v2 is requested (and optionally introduce v3 to support it). Jeff, any preference?
I actually think disabling rename-from-worktree consistently may be the best way to go.
Re: [PATCH] status: handle worktree renames
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · Duy Nguyen
Or we disable rename-from-worktree when porcelain v2 is requested (and optionally introduce v3 to support it). Jeff, any preference?
Sorry for the delay, I was on vacation last week.
I like the "R." and ".R" lines in your 3rd patch series as that keeps porcelain V2 output consistent with the changes that you added to plain and porcelain V1 output. All 3 formats now report 2 types of renames. Having a "RR" line would be more consistent with a "MM" line, but I don't think that happens often enough to define a porcelain V3 format with a 3 path row variant.
I like that we can now show "unstaged renames" (in all 3 formats) as I think that is less confusing to the novice user than a new-file/delete pair.
Having said that, I am a little concerned about us changing V1 and V2 output at all -- we are breaking the porcelain contract we have with scripts. I like the change, so I'm not bothered about it, but others may think differently.
Also, does this introduce any new cases for reporting conflicts? I haven't really thought about it too much yet, but if there was a divergent rename in both branches of a merge, do we now have to handle showing possibly 4 pathnames for a file? (merge-base, branch-a, branch-b, worktree)
Jeff
Re: [PATCH] status: handle worktree renames
Quoted 5 lines · Jeff Hostetler
Also, does this introduce any new cases for reporting conflicts? I haven't really thought about it too much yet, but if there was a divergent rename in both branches of a merge, do we now have to handle showing possibly 4 pathnames for a file? (merge-base, branch-a, branch-b, worktree)
It's an interesting question but unfortunately I don't have an answer (I read your mail earlier but waited for so long because I didn't know the answer then).
Signature · 1 line
Duy
v2 cover Renames in git-status "changed not staged" section
In reply to Nguyễn Thái Ngọc DuyThe changes in 425a28e0a4 allow new files to show up in "git diff-files" (aka "changed but not staged") which is a problem because status code does not handle renaming in this case.
The main change to fix this is 6/7. The interesting corner case is in 7/7 where I decided to go with a middle ground, disabling only double renames. We have two other options to go:
- unconditionally disable rename in wt_status_collect_changes_worktree which turns this 7-patch series into one patch
- support double renames too but this may change porcelain output (or we have to add new output format). This could be done in a follow series if we want to.
Nguyễn Thái Ngọc Duy (7):
t2203: test status output with porcelain v2 format Use DIFF_DETECT_RENAME for detect_rename assignments wt-status.c: coding style fix
wt-status.c: rename wt_status_change_data::score wt-status.c: catch unhandled diff status codes wt-status.c: handle worktree renames
wt-status.c: avoid double renames in short/porcelain format
builtin/commit.c | 2 +- diff.c | 2 +- t/t2203-add-intent.sh | 73 ++++++++++++++++++++++++++ wt-status.c | 141 ++++++++++++++++++++++++++++++++++++++++---------- wt-status.h | 4 +- 5 files changed, 191 insertions(+), 31 deletions(-)
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v2 1/7 t2203: test status output with porcelain v2 format
--- t/t2203-add-intent.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
t/t2203-add-intent.sh +12 -0
@@ -25,6 +25,18 @@ test_expect_success 'git status' ' test_cmp expect actual ' +test_expect_success 'git status with porcelain v2' ' + git status --porcelain=v2 | grep -v "^?" >actual && + nam1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d && + nam2=ce013625030ba8dba906f756967f9e9ca394464a && + cat >expect <<-EOF && + 1 DA N... 100644 000000 100644 $nam1 $_z40 1.t + 1 A. N... 000000 100644 100644 $_z40 $nam2 elif + 1 .A N... 000000 000000 100644 $_z40 $_z40 file + EOF + test_cmp expect actual +' + test_expect_success 'check result of "add -N"' ' git ls-files -s file >actual && empty=$(git hash-object --stdin </dev/null) &&
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v2 2/7 Use DIFF_DETECT_RENAME for detect_rename assignments
In reply to Nguyễn Thái Ngọc DuyThis field can have two values (2 for copy). Use this name instead for clarity. Many places have already used this constant.
Note, the detect_rename assignments in merge-recursive.c remain unchanged because it's actually a boolean there.
--- builtin/commit.c | 2 +- diff.c | 2 +- wt-status.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
builtin/commit.c +1 -1
@@ -1507,7 +1507,7 @@ static void print_summary(const char *prefix, const struct object_id *oid, rev.show_root_diff =1 ;get_commit_format (format.buf, &rev); rev.always_show_header =0 ; - rev.diffopt.detect_rename =1 ; + rev.diffopt.detect_rename =DIFF_DETECT_RENAME ; rev.diffopt.break_opt =0 ;diff_setup_done (&rev.diffopt);
diff.c +1 -1
@@ -246,7 +246,7 @@ static int parse_ws_error_highlight(const char *arg)*/ void init_diff_ui_defaults (void ) { - diff_detect_rename_default =1 ; + diff_detect_rename_default =DIFF_DETECT_RENAME ; }int git_diff_heuristic_config (const char *var,const char *value,void *cb)
wt-status.c +2 -2
@@ -602,7 +602,7 @@ static void wt_status_collect_changes_index(struct wt_status *s) rev.diffopt.output_format |=DIFF_FORMAT_CALLBACK ; rev.diffopt.format_callback = wt_status_collect_updated_cb; rev.diffopt.format_callback_data = s; - rev.diffopt.detect_rename =1 ; + rev.diffopt.detect_rename =DIFF_DETECT_RENAME ; rev.diffopt.rename_limit =200 ; rev.diffopt.break_opt =0 ;copy_pathspec (&rev.prune_data, &s->pathspec);
@@ -962,7 +962,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)setup_revisions (0 ,NULL , &rev, &opt); rev.diffopt.output_format |=DIFF_FORMAT_PATCH ; - rev.diffopt.detect_rename =1 ; + rev.diffopt.detect_rename =DIFF_DETECT_RENAME ; rev.diffopt.file = s->fp; rev.diffopt.close_file =0 ;/*
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v2 3/7 wt-status.c: coding style fix
In reply to Nguyễn Thái Ngọc Duy--- wt-status.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
wt-status.c +2 -1
@@ -406,7 +406,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,strbuf_release (&twobuf); } -static char short_submodule_status (struct wt_status_change_data *d) { +static char short_submodule_status (struct wt_status_change_data *d) +{if (d->new_submodule_commits)return 'M' ;if (d->dirty_submodule &DIRTY_SUBMODULE_MODIFIED )
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v2 4/7 wt-status.c: rename wt_status_change_data::score
In reply to Nguyễn Thái Ngọc DuyWe are about to adding support for "diff-files" rename, which has its own rename score in addition to the "diff-index" one. Rename score to head_score to indicate this score comes from diff-index. The new score will be named worktree_score.
--- wt-status.c | 4 ++-- wt-status.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
wt-status.c +2 -2
@@ -532,7 +532,7 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,case DIFF_STATUS_COPIED :case DIFF_STATUS_RENAMED : d->head_path =xstrdup (p->one->path); - d->score = p->score *100 /MAX_SCORE ; + d->head_score = p->score *100 /MAX_SCORE ;/* fallthru */ case DIFF_STATUS_MODIFIED :case DIFF_STATUS_TYPE_CHANGED :
@@ -2074,7 +2074,7 @@ static void wt_porcelain_v2_print_changed_entry( key, submodule_token, d->mode_head, d->mode_index, d->mode_worktree,oid_to_hex (&d->oid_head),oid_to_hex (&d->oid_index), - key[0 ], d->score, + key[0 ], d->head_score, path_index, sep_char, path_head, eol_char); else fprintf(s->fp, "1 %s %s %06o %06o %06o %s %s %s%c",
wt-status.h +1 -1
@@ -44,7 +44,7 @@ struct wt_status_change_data {int worktree_status;int index_status;int stagemask; -int score; +int head_score;int mode_head, mode_index, mode_worktree;struct object_id oid_head, oid_index;char *head_path;
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v2 5/7 wt-status.c: catch unhandled diff status codes
In reply to Nguyễn Thái Ngọc Duy--- wt-status.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
wt-status.c +6 -2
@@ -468,8 +468,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,oidcpy (&d->oid_index, &p->one->oid);break ; -case DIFF_STATUS_UNKNOWN : -die ("BUG: worktree status unknown???" ); +default : +die ("BUG: unhandled diff-files status '%c'" , p->status);break ; }
@@ -549,6 +549,10 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,* values in these fields. */ break ; + +default : +die ("BUG: unhandled diff-index status '%c'" , p->status); +break ; } } }
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v2 6/7 wt-status.c: handle worktree renames
In reply to Nguyễn Thái Ngọc DuyBefore 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist in index" - 2016-10-24) there are never "new files" in the index, which essentially disables rename detection because we only detect renames when a new file appears in a diff pair.
After that commit, an i-t-a entry can appear as a new file in "git diff-files". But the diff callback function in wt-status.c does not handle this case and produces incorrect status output.
PS. The reader may notice that this patch adds a new xstrdup() but not a free(). Yes we leak memory (the same for head_path). But wt_status so far has been short lived, this leak should not matter in practice.
--- t/t2203-add-intent.sh | 28 ++++++++++++++++++++ wt-status.c | 72 +++++++++++++++++++++++++++++++++++++++------------ wt-status.h | 4 +-- 3 files changed, 85 insertions(+), 19 deletions(-)
t/t2203-add-intent.sh +28 -0
@@ -162,5 +162,33 @@ test_expect_success 'commit: ita entries ignored in empty commit check' ' ) ' +test_expect_success 'rename detection finds the right names' ' + git init rename-detection && + ( + cd rename-detection && + echo contents > first && + git add first && + git commit -m first && + mv first third && + git add -N third && + + git status | grep -v "^?" >actual.1 && + test_i18ngrep "renamed: *first -> third" actual.1 && + + git status --porcelain | grep -v "^?" >actual.2 && + cat >expected.2 <<-\EOF && + R first -> third + EOF + test_cmp expected.2 actual.2 && + + oid=12f00e90b6ef79117ce6e650416b8cf517099b78 && + git status --porcelain=v2 | grep -v "^?" >actual.3 && + cat >expected.3 <<-EOF && + 2 .R N... 100644 100644 100644 $oid $oid R100 first third + EOF + test_cmp expected.3 actual.3 + ) +' + test_done
wt-status.c +55 -17
145 lines not on this page · raw message
wt-status.h +2 -2
@@ -44,10 +44,10 @@ struct wt_status_change_data {int worktree_status;int index_status;int stagemask; -int head_score; +int head_score, worktree_score;int mode_head, mode_index, mode_worktree;struct object_id oid_head, oid_index; -char *head_path; +char *head_path, *worktree_path; unsigned dirty_submodule : 2; unsigned new_submodule_commits : 1; };
Signature · 1 line
2.15.0.320.g0453912d77
Re: [PATCH v2 6/7] wt-status.c: handle worktree renames
Hi Duy,
Quoted 50 lines · Nguyễn Thái Ngọc Duy
Before
425a28e0a4(diff-lib: allow ita entries treated as "not yet exist in index" - 2016-10-24) there are never "new files" in the index, which essentially disables rename detection because we only detect renames when a new file appears in a diff pair.After that commit, an i-t-a entry can appear as a new file in "git diff-files". But the diff callback function in
wt-status.cdoes not handle this case and produces incorrect status output.PS. The reader may notice that this patch adds a new
xstrdup()but not afree(). Yes we leak memory (the same forhead_path). Butwt_statusso far has been short lived, this leak should not matter in practice.Noticed-by Alex Vandiver <[email protected]>Helped-by Igor Djordjevic <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- t/t2203-add-intent.sh | 28 ++++++++++++++++++++ wt-status.c | 72 +++++++++++++++++++++++++++++++++++++++------------ wt-status.h | 4 +-- 3 files changed, 85 insertions(+), 19 deletions(-)t/t2203-add-intent.sh +19
-0@@ -162,5 +162,33 @@ test_expect_success 'commit: ita entries ignored in empty commit check' ' ) ' +test_expect_success 'rename detection finds the right names' ' + git init rename-detection && + ( + cd rename-detection && + echo contents > first && + git add first && + git commit -m first && + mv first third && + git add -N third && + + git status | grep -v "^?" >actual.1 && + test_i18ngrep "renamed: *first -> third" actual.1 && + + git status --porcelain | grep -v "^?" >actual.2 && + cat >expected.2 <<-\EOF && + R first -> third + EOF + test_cmp expected.2 actual.2 && +
t/t2203-add-intent.sh test_expect_success 'commit: ita entries ignored in empty commit check' ' · line 193
Quoted 10 lines
t/t2203-add-intent.sh +9
-0+ oid=12f00e90b6ef79117ce6e650416b8cf517099b78 && + git status --porcelain=v2 | grep -v "^?" >actual.3 && + cat >expected.3 <<-EOF && + 2 .R N... 100644 100644 100644 $oid $oid R100 first third + EOF + test_cmp expected.3 actual.3 + ) +' + test_done
Im afraid "--porcelain=v2" test might be incorrect here, as git status --porcelain=v2` output seems to be too, with this v2 series applied. Test I sent previously[1] fails, and it looks valid.
This is output I now get, with old/deleted file unstaged and new/created file staged with git add -N:
$ git status --porcelain=v2
2 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 original-file new-fileNote "original-file" listed first, where "new-file" listed second (last). According the "v2" documentation[2] (excerpt):
... <path><sep><origPath>
<path> The pathname. In a renamed/copied entry, this
is the path in the index and in the working tree.
...
<origPath> The pathname in the commit at HEAD. This is only
present in a renamed/copied entry, and tells where the renamed/copied contents came from.
If I`m reading this correctly, it should be vice-versa - value from HEAD, being "original-file", should come last, where value from working tree ("new-file") should be first.
If I stage both files and try again, output is as expected ("new-file" comes first):
$ git status --porcelain=v2
2 R. N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 new-file original-fileQuoted 14 lines
wt-status.c +2
-0@@ -376,6 +376,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,strbuf_addch (&extra,')' ); } status = d->worktree_status; +if (d->worktree_path) + two_name = d->worktree_path;break ;default : die("BUG: unhandled change_type %d in wt_longstatus_print_change_data",
wt-status.c static void wt_status_collect_changed_cb(struct diff_queue_struct *q, · line 467
Quoted 6 lines
wt-status.c +3
-0@@ -460,6 +462,12 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,/* mode_worktree is zero for a delete. */ break ; +case DIFF_STATUS_COPIED : +case DIFF_STATUS_RENAMED : + d->worktree_path =xstrdup (p->two->path);
^^^
This is changed from v1 of this patch, where it was:
+ d->worktree_path = xstrdup(p->one->path);
^^^
..., and might be it introduced the issue here...? Or, if this is correct now, then might be the other part further below should then be adapted accordingly, inside wt_porcelain_v2_print_changed_entry()?
Quoted 86 lines
wt-status.c +34
-10+ d->worktree_score = p->score *100 /MAX_SCORE ; +/* fallthru */ +case DIFF_STATUS_MODIFIED :case DIFF_STATUS_TYPE_CHANGED :case DIFF_STATUS_UNMERGED :@@ -1712,6 +1720,7 @@ static void wt_shortstatus_status(struct string_list_item *it, struct wt_status *s) {struct wt_status_change_data *d = it->util; +const char *from, *to;if (d->index_status)color_fprintf (s->fp,color (WT_STATUS_UPDATED , s),"%c" , d->index_status);@@ -1722,15 +1731,30 @@ static void wt_shortstatus_status(struct string_list_item *it,else putchar (' ' );putchar (' ' ); + +if (d->head_path && d->worktree_path) +die ("BUG: to be addressed in the next patch" ); + +if (d->head_path) { + from = d->head_path; + to = it->string; + }else if (d->worktree_path) { + from = it->string; + to = d->worktree_path; + }else { + from = it->string; + to =NULL ; + }if (s->null_termination) { -fprintf (stdout,"%s%c" , it->string,0 ); -if (d->head_path) -fprintf (stdout,"%s%c" , d->head_path,0 ); +fprintf (stdout,"%s%c" , from,0 ); +if (to) +fprintf (stdout,"%s%c" , to,0 ); }else {struct strbuf onebuf =STRBUF_INIT ;const char *one; -if (d->head_path) { - one =quote_path (d->head_path, s->prefix, &onebuf); + +if (to) { + one =quote_path (from, s->prefix, &onebuf);if (*one !='"' &&strchr (one,' ' ) !=NULL ) {putchar ('"' );strbuf_addch (&onebuf,'"' );@@ -1738,8 +1762,9 @@ static void wt_shortstatus_status(struct string_list_item *it, }printf ("%s -> " , one);strbuf_release (&onebuf); - } - one =quote_path (it->string, s->prefix, &onebuf); + one =quote_path (to, s->prefix, &onebuf); + }else + one =quote_path (from, s->prefix, &onebuf);if (*one !='"' &&strchr (one,' ' ) !=NULL ) {putchar ('"' );strbuf_addch (&onebuf,'"' );@@ -2036,12 +2061,13 @@ static void wt_porcelain_v2_print_changed_entry( {struct wt_status_change_data *d = it->util;struct strbuf buf_index =STRBUF_INIT ; -struct strbuf buf_head =STRBUF_INIT ; +struct strbuf buf_other =STRBUF_INIT ;const char *path_index =NULL ; -const char *path_head =NULL ; -char key[3 ]; +const char *path_other =NULL ; +char key[3 ], status_other;char submodule_token[5 ];char sep_char, eol_char; +int score;wt_porcelain_v2_fix_up_changed (it, s);wt_porcelain_v2_submodule_state (d, submodule_token);@@ -2050,6 +2076,19 @@ static void wt_porcelain_v2_print_changed_entry( key[1 ] = d->worktree_status ? d->worktree_status :'.' ; key[2 ] =0 ; +if (d->head_path && d->worktree_path) +die ("BUG: to be addressed in the next patch" ); +
wt-status.c static void wt_porcelain_v2_print_changed_entry( · line 2091
Quoted 10 lines
wt-status.c +10
-0+if (d->head_path) { + path_other = d->head_path; + status_other = d->index_status; + score = d->head_score; + }else if (d->worktree_path) { + path_other = d->worktree_path; + status_other = d->worktree_status; + score = d->worktree_score; + } +
"path_other" calculation seems incorrect here...? As that one is later used as "<origPath>", coming last, it should be value from HEAD exclusively (in case of rename/copy, otherwise empty).
Here, in case HEAD is missing, "path_other" is set to worktree path - which should be only (possibly) found inside "<path>", being the first listed of the two paths (currently represented by "path_index" further below).
That said, might be even better to have previously existing "path_head" variable restored, and "path_index" replaced with this new "path_other" instead, as that`s what documentation seems to talk about.
Quoted 21 lines
wt-status.c +2
-3if (s->null_termination) {/* * In -z mode, we DO NOT C-quote pathnames. Current path is ALWAYS first. @@ -2058,7 +2097,6 @@ static void wt_porcelain_v2_print_changed_entry( sep_char ='\0' ; eol_char ='\0' ; path_index = it->string; - path_head = d->head_path; }else {/* * Path(s) are C-quoted if necessary. Current path is ALWAYS first. @@ -2069,17 +2107,17 @@ static void wt_porcelain_v2_print_changed_entry( sep_char ='\t' ; eol_char ='\n' ; path_index =quote_path (it->string, s->prefix, &buf_index); -if (d->head_path) - path_head =quote_path (d->head_path, s->prefix, &buf_head); +if (path_other) + path_other =quote_path (path_other, s->prefix, &buf_other); }
wt-status.c static void wt_porcelain_v2_print_changed_entry( · line 2120
Quoted 10 lines
wt-status.c +3
-3-if (path_head) +if (path_other)fprintf (s->fp,"2 %s %s %06o %06o %06o %s %s %c%d %s%c%s%c" , key, submodule_token, d->mode_head, d->mode_index, d->mode_worktree,oid_to_hex (&d->oid_head),oid_to_hex (&d->oid_index), - key[0 ], d->head_score, - path_index, sep_char, path_head, eol_char); + status_other, score, + path_index, sep_char, path_other, eol_char);
^^^^^^^^^^ ^^^^^^^^^^
This seems mixed up a bit - first value (now "path_index") should be "index and/or working tree" (so "path_other", I guess), and second value (now "path_other") should be "path_head" (exclusively).
Quoted 20 lines
wt-status.c +1
-1else fprintf(s->fp, "1 %s %s %06o %06o %06o %s %s %s%c", key, submodule_token,@@ -2088,7 +2126,7 @@ static void wt_porcelain_v2_print_changed_entry( path_index, eol_char);strbuf_release (&buf_index); -strbuf_release (&buf_head); +strbuf_release (&buf_other); }/* wt-status.h +0
-0@@ -44,10 +44,10 @@ struct wt_status_change_data {int worktree_status;int index_status;
wt-status.h struct wt_status_change_data { · line 53
Quoted 10 lines
wt-status.h +2
-2int stagemask; -int head_score; +int head_score, worktree_score;int mode_head, mode_index, mode_worktree;struct object_id oid_head, oid_index; -char *head_path; +char *head_path, *worktree_path; unsigned dirty_submodule : 2; unsigned new_submodule_commits : 1; };
Funny thing is that, overall, changes this patch introduces regarding "--porcelain=v2" are (functionally) pretty much what I did in that exercise patch I sent earlier[3], but while that one worked (on top of your v1), this one seems not to...
Could it be because of that "p->one->path" vs "p->two->path" difference introduced inside wt_status_collect_changed_cb() in patch series v2, where wt_porcelain_v2_print_changed_entry() (using the affected value) stayed the same as in v1?
Regards, Buga
[1] https://public-inbox.org/git/[email protected]/T/#m866cf8b7edfd40172972771079c50bd5ff5bd535
[2] https://git-scm.com/docs/git-status#_porcelain_format_version_2
[3] https://public-inbox.org/git/[email protected]/T/#m095c33d69994c6ecb4f1adbf80dd48eab66750d8
Re: [PATCH v2 6/7] wt-status.c: handle worktree renames
Quoted 10 lines · Igor Djordjevic
I
m afraid "--porcelain=v2" test might be incorrect here, asgit status --porcelain=v2` output seems to be too, with this v2 series applied. Test I sent previously[1] fails, and it looks valid.This is output I now get, with old/deleted file unstaged and new/created file staged with
git add -N:$ git status --porcelain=v2 2 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 original-file new-file
Quoted 7 lines
Note "original-file" listed first, where "new-file" listed second (last). According the "v2" documentation[2] (excerpt):
... <path><sep><origPath>
<path> The pathname. In a renamed/copied entry, this
is the path in the index and in the working tree.
Gaah.. as you may see in the other mail when I quoted this (incorrectly). I must have modified this file at some point and thought it was true (my version did not have "and in the worktree").
The "and" is still problematic if you take this very seriously (because in this case index name and worktree name are different) but I think it's ok to ignore that "and" and switch it to "or".
Quoted 9 lines
...
<origPath> The pathname in the commit at HEAD. This is only
present in a renamed/copied entry, and tells where the renamed/copied contents came from.If I`m reading this correctly, it should be vice-versa - value from HEAD, being "original-file", should come last, where value from working tree ("new-file") should be first.
Yeah I think the "where the renamed/copied contents came from" clears up my confusion in this format. Back to v1 it is!
Signature · 1 line
Duy
Re: [PATCH v2 6/7] wt-status.c: handle worktree renames
Quoted 9 lines · Duy Nguyen
Quoted 5 lines
... <path><sep><origPath>
<path> The pathname. In a renamed/copied entry, this
is the path in the index and in the working tree.Gaah.. as you may see in the other mail when I quoted this (incorrectly). I must have modified this file at some point and thought it was true (my version did not have "and in the worktree").
Ah, this explains a lot... :) I got really confused with your v2, it felt as the series took a strange turn, and in a kind of a subtle way :P
Quoted 3 lines
The "and" is still problematic if you take this very seriously (because in this case index name and worktree name are different) but I think it's ok to ignore that "and" and switch it to "or".
Yes, I agree, and the change does feel like a good thing. But, I also now hope this doesn`t break any expectations, because... (read below)
Quoted 7 lines
Quoted 7 lines
<origPath> The pathname in the commit at HEAD. This is only
present in a renamed/copied entry, and tells where the renamed/copied contents came from.If I`m reading this correctly, it should be vice-versa - value from HEAD, being "original-file", should come last, where value from working tree ("new-file") should be first.
... it totally slipped me that documentation is/was pretty strict about <origPath> being HEAD path (exclusively), where I was still expecting it to show renamed working tree "from" value as <origPath> in case of working tree (double) rename, too - where that exact (already renamed in index) path wasnt to be found inside HEAD at all, so the working tree rename couldnt really be shown as "source" and "target" rename pair, strictly following the "porcelain v2" specification... :/
I see now that your initial reply[1] was talking about this, but I didn`t focus on it much as you replied to it yourself shortly afterwards, and later v2 of the series came up.
Might be this is where you changed your offline documentation version, too, as that is what the sample patch was about :)
Quoted 2 lines
Yeah I think the "where the renamed/copied contents came from" clears up my confusion in this format. Back to v1 it is!
I see you addressed this by loosening the restriction here a bit, too, making <origPath> be "pathname in the commit at HEAD _or in the index_", in your "[PATCH v3 6/6] wt-status.c: handle worktree renames"[2].
I repeat that this looks like the correct approach, making fully described working tree rename detection possible in porcelain in the first place, but also aligning output of "status" --porcelain variants with its default (--long) form.
Hopefully, on top of everything positive, it also doesn`t break anything (too much?)... :P Latest revision should now provide all the necessary ingredients to resolve what happened, for the (small?) price of tweaking previous expectations a bit.
Regards, Buga
[1] https://public-inbox.org/git/CACsJy8A=jZ9LAuM50GVjNT5gtdiYYMyMuPBSrJFO4LmKVQsETQ@mail.gmail.com/T/#mf2f5ae672ec6f4e1abecbd5fe65283e9d8fbed57
[2] https://public-inbox.org/git/[email protected]/T/#u
Re: [PATCH v2 6/7] wt-status.c: handle worktree renames
p.s. An extra note for the casual reader, just in case:
Quoted 10 lines · Igor Djordjevic
... it totally slipped me that documentation is/was pretty strict about <origPath> being HEAD path (exclusively), where I was still expecting it to show renamed working tree "from" value as <origPath> in case of working tree (double) rename, too - where that exact (already renamed in index) path wasn
t to be found inside HEAD at all, so the working tree rename couldnt really be shown as "source" and "target" rename pair, strictly following the "porcelain v2" specification... :/
Quoted 6 lines
...
I repeat that this looks like the correct approach, making fully described working tree rename detection possible in porcelain in the first place, but also aligning output of "status" --porcelain variants with its default (--long) form.
Wherever "working tree rename detection" is discussed above, its all still about renamed file git add -N` case only (where old name, appearing "deleted" from HEAD/index, is not staged yet), not some new functionality where renamed file inside working tree is instantly / magically recognized without being staged in the first place.
PATCH v2 7/7 wt-status.c: avoid double renames in short/porcelain format
In reply to Nguyễn Thái Ngọc DuyThe 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 -> thirdBut for now it's safer and simpler to just break the "second -> third" rename pair and show
RD first -> second
A thirdlike we have been showing until now.
--- t/t2203-add-intent.sh | 33 +++++++++++++++++++++++++++++ wt-status.c | 58 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 84 insertions(+), 7 deletions(-)
t/t2203-add-intent.sh +33 -0
@@ -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 000000 $oid $oid R100 second first + 1 .A N... 000000 000000 100644 $_z40 $_z40 third + EOF + test_cmp expected.3 actual.3 + ) +' + test_done
wt-status.c +51 -7
113 lines not on this page · raw message
Signature · 1 line
2.15.0.320.g0453912d77
Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
Hi Duy,
Quoted 27 lines · Nguyễn Thái Ngọc Duy
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 -> thirdor we could show renames as two lines instead of one, for example something like this for short form:
R first -> second R second -> third
Quoted 8 lines
But for now it's safer and simpler to just break the "second -> third" rename pair and show
RD first -> second A thirdlike 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").
Im inclined to say this doesnt 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 thats 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 thirdHere, 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 dont think this looks like what we have been showing until now (unless I misunderstood which exact "now" are we talking about), so I dont 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 secondHere, 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, its possible to recreate hypothetical "first -> second -> third" change from there. With v2 output, that is impossible, that information is lost as second line doesnt relate to the first one in any way.
Now, unless Im totally missing something here, the only thing left is that you mentioned v2 approach being "safer and simpler" than v1, something Im 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/[email protected]/T/#mf60e88fd351f7ff6a076279794c8343a79835f67
[2] https://public-inbox.org/git/[email protected]/T/#m095c33d69994c6ecb4f1adbf80dd48eab66750d8
Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
Quoted 36 lines · Igor Djordjevic
Hi Duy,
Quoted 33 lines · Nguyễn Thái Ngọc Duy
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 -> thirdor we could show renames as two lines instead of one, for example something like this for short form:
R first -> second R second -> thirdBut for now it's safer and simpler to just break the "second -> third" rename pair and show
RD first -> second A third
Quoted 7 lines
Quoted 2 lines
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 thirdThe second line is different and is what 425a28e0a4 tries to fix.
Quoted 12 lines
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 firstHere, 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").
Quoted 8 lines
I
m inclined to say this doesnt align with whatgit statusshows, disrespectingadd -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 thats what defaultgit 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?
Quoted 23 lines
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
Quoted 8 lines
Here, porcelain statuses make situation a bit better, as now at least
add -Nis 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 statusstill 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.
Quoted 30 lines
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 dont 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
Quoted 7 lines
(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 secondHere, both "--porcelain" outputs (2) and (3) seem to much better replicate what default
git statusis showing, too - namely separate renames in comparison to HEAD for both "index" (2) and "working tree" (3).
There is a problem with my v1 (or my misunderstanding of the code and the porcelain v2 format). The first path column must always be the path in index according to git-status.txt
<path> The pathname. In a renamed/copied entry, this
is the path in the index.
which means the paths in the second line are swapped to
2 .R .... R100 second<tab>third
The current code ('master', before any of my changes) seems to also key all entries by index path with those "string_list_insert(&s->change,...". And it fits porcelain v2 definition. But it introduces this problem.
Perhaps keying by index path is just not intentional (and porcelain v2 accidentally exposes that), I don't know. If we relax this a bit like in my v1 (and conclude that we are not breaking any porcelain formats), then yes, the problem goes away. The new definition would be
<path> The pathname. In a renamed/copied entry, this
is the target path.
(and we leave the user to determine whether the target is in index or in worktree using <XY> code). I like this actually, "R100 second third" looks just weird, but I'm not porcelain user and don't know how it's actually used.
Quoted 13 lines
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 statusoutput, 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 doesnt relate to the first one in any way.
Quoted 5 lines
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 Im 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...? :)
Yeah it's "safe" to not break porcelain formats (and "simpler" than changing them). But if it turns out I'm just paranoid, I'll happily revert back to v1.
Quoted 6 lines
If not that much more complicated/unsafe, of course.
Thanks, Buga
[1] https://public-inbox.org/git/[email protected]/T/#mf60e88fd351f7ff6a076279794c8343a79835f67
[2] https://public-inbox.org/git/[email protected]/T/#m095c33d69994c6ecb4f1adbf80dd48eab66750d8
Signature · 1 line
Duy
Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format
Quoted 6 lines · Duy Nguyen
Quoted 5 lines
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...?
Quoted 8 lines
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". Before425a28e0a4rename 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
v3 cover Renames in git-status "changed not staged" section
In reply to Nguyễn Thái Ngọc Duyv3 more or less goes back to v1 after my discussion with Igor about porcelain formats. So 7/7 is not needed anymore. 4/7 becomes 5/6. The meat is still in 6/6, now with some more updates in git-status.txt and to address the comment from Torsten.
Nguyễn Thái Ngọc Duy (6):
t2203: test status output with porcelain v2 format
Use DIFF_DETECT_RENAME for detect_rename assignments wt-status.c: coding style fix
wt-status.c: catch unhandled diff status codes wt-status.c: rename rename-related fields in wt_status_change_data wt-status.c: handle worktree renames
Documentation/git-status.txt | 23 ++++++------ builtin/commit.c | 2 +- diff.c | 2 +- t/t2203-add-intent.sh | 72 ++++++++++++++++++++++++++++++++++++++ wt-status.c | 83 ++++++++++++++++++++++++++++---------------- wt-status.h | 5 +-- 6 files changed, 143 insertions(+), 44 deletions(-)
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v3 1/6 t2203: test status output with porcelain v2 format
--- t/t2203-add-intent.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
t/t2203-add-intent.sh +12 -0
@@ -25,6 +25,18 @@ test_expect_success 'git status' ' test_cmp expect actual ' +test_expect_success 'git status with porcelain v2' ' + git status --porcelain=v2 | grep -v "^?" >actual && + nam1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d && + nam2=ce013625030ba8dba906f756967f9e9ca394464a && + cat >expect <<-EOF && + 1 DA N... 100644 000000 100644 $nam1 $_z40 1.t + 1 A. N... 000000 100644 100644 $_z40 $nam2 elif + 1 .A N... 000000 000000 100644 $_z40 $_z40 file + EOF + test_cmp expect actual +' + test_expect_success 'check result of "add -N"' ' git ls-files -s file >actual && empty=$(git hash-object --stdin </dev/null) &&
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v3 2/6 Use DIFF_DETECT_RENAME for detect_rename assignments
In reply to Nguyễn Thái Ngọc DuyThis field can have two values (2 for copy). Use this name instead for clarity. Many places have already used this constant.
Note, the detect_rename assignments in merge-recursive.c remain unchanged because it's actually a boolean there.
--- builtin/commit.c | 2 +- diff.c | 2 +- wt-status.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
builtin/commit.c +1 -1
@@ -1507,7 +1507,7 @@ static void print_summary(const char *prefix, const struct object_id *oid, rev.show_root_diff =1 ;get_commit_format (format.buf, &rev); rev.always_show_header =0 ; - rev.diffopt.detect_rename =1 ; + rev.diffopt.detect_rename =DIFF_DETECT_RENAME ; rev.diffopt.break_opt =0 ;diff_setup_done (&rev.diffopt);
diff.c +1 -1
@@ -246,7 +246,7 @@ static int parse_ws_error_highlight(const char *arg)*/ void init_diff_ui_defaults (void ) { - diff_detect_rename_default =1 ; + diff_detect_rename_default =DIFF_DETECT_RENAME ; }int git_diff_heuristic_config (const char *var,const char *value,void *cb)
wt-status.c +2 -2
@@ -602,7 +602,7 @@ static void wt_status_collect_changes_index(struct wt_status *s) rev.diffopt.output_format |=DIFF_FORMAT_CALLBACK ; rev.diffopt.format_callback = wt_status_collect_updated_cb; rev.diffopt.format_callback_data = s; - rev.diffopt.detect_rename =1 ; + rev.diffopt.detect_rename =DIFF_DETECT_RENAME ; rev.diffopt.rename_limit =200 ; rev.diffopt.break_opt =0 ;copy_pathspec (&rev.prune_data, &s->pathspec);
@@ -962,7 +962,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)setup_revisions (0 ,NULL , &rev, &opt); rev.diffopt.output_format |=DIFF_FORMAT_PATCH ; - rev.diffopt.detect_rename =1 ; + rev.diffopt.detect_rename =DIFF_DETECT_RENAME ; rev.diffopt.file = s->fp; rev.diffopt.close_file =0 ;/*
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v3 3/6 wt-status.c: coding style fix
In reply to Nguyễn Thái Ngọc Duy--- wt-status.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
wt-status.c +2 -1
@@ -406,7 +406,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,strbuf_release (&twobuf); } -static char short_submodule_status (struct wt_status_change_data *d) { +static char short_submodule_status (struct wt_status_change_data *d) +{if (d->new_submodule_commits)return 'M' ;if (d->dirty_submodule &DIRTY_SUBMODULE_MODIFIED )
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v3 4/6 wt-status.c: catch unhandled diff status codes
In reply to Nguyễn Thái Ngọc Duy--- wt-status.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
wt-status.c +6 -2
@@ -468,8 +468,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,oidcpy (&d->oid_index, &p->one->oid);break ; -case DIFF_STATUS_UNKNOWN : -die ("BUG: worktree status unknown???" ); +default : +die ("BUG: unhandled diff-files status '%c'" , p->status);break ; }
@@ -549,6 +549,10 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,* values in these fields. */ break ; + +default : +die ("BUG: unhandled diff-index status '%c'" , p->status); +break ; } } }
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v3 5/6 wt-status.c: rename rename-related fields in wt_status_change_data
In reply to Nguyễn Thái Ngọc DuyThese field "head_path" is used for rename display only. In the next patch we introduce another rename pair where the rename source is no longer HEAD. Rename it to something more generic.
While at there, rename "score" as well and store the rename diff code in a separate field instead of hardcoding key[0] (i.e. diff-index) in porcelain v2 code.
--- wt-status.c | 50 ++++++++++++++++++++++++++------------------------ wt-status.h | 5 +++-- 2 files changed, 29 insertions(+), 26 deletions(-)
wt-status.c +26 -24
@@ -360,8 +360,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,switch (change_type) {case WT_STATUS_UPDATED : status = d->index_status; -if (d->head_path) - one_name = d->head_path; +if (d->rename_source) + one_name = d->rename_source;break ;case WT_STATUS_CHANGED :if (d->new_submodule_commits || d->dirty_submodule) {
@@ -391,7 +391,7 @@ static void wt_longstatus_print_change_data(struct wt_status *s,die ("BUG: unhandled diff status %c" , status); len = label_width -utf8_strwidth (what);assert (len >=0 ); -if (status ==DIFF_STATUS_COPIED || status ==DIFF_STATUS_RENAMED ) +if (one_name != two_name)status_printf_more (s, c,"%s%.*s%s -> %s" , what, len, padding, one, two);else
@@ -531,8 +531,9 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,case DIFF_STATUS_COPIED :case DIFF_STATUS_RENAMED : - d->head_path =xstrdup (p->one->path); - d->score = p->score *100 /MAX_SCORE ; + d->rename_source =xstrdup (p->one->path); + d->rename_score = p->score *100 /MAX_SCORE ; + d->rename_status = p->status;/* fallthru */ case DIFF_STATUS_MODIFIED :case DIFF_STATUS_TYPE_CHANGED :
@@ -1724,13 +1725,14 @@ static void wt_shortstatus_status(struct string_list_item *it,putchar (' ' );if (s->null_termination) {fprintf (stdout,"%s%c" , it->string,0 ); -if (d->head_path) -fprintf (stdout,"%s%c" , d->head_path,0 ); +if (d->rename_source) +fprintf (stdout,"%s%c" , d->rename_source,0 ); }else {struct strbuf onebuf =STRBUF_INIT ;const char *one; -if (d->head_path) { - one =quote_path (d->head_path, s->prefix, &onebuf); + +if (d->rename_source) { + one =quote_path (d->rename_source, s->prefix, &onebuf);if (*one !='"' &&strchr (one,' ' ) !=NULL ) {putchar ('"' );strbuf_addch (&onebuf,'"' );
@@ -2035,10 +2037,10 @@ static void wt_porcelain_v2_print_changed_entry( struct wt_status *s) {struct wt_status_change_data *d = it->util; -struct strbuf buf_index =STRBUF_INIT ; -struct strbuf buf_head =STRBUF_INIT ; -const char *path_index =NULL ; -const char *path_head =NULL ; +struct strbuf buf =STRBUF_INIT ; +struct strbuf buf_from =STRBUF_INIT ; +const char *path =NULL ; +const char *path_from =NULL ;char key[3 ];char submodule_token[5 ];char sep_char, eol_char;
@@ -2057,8 +2059,8 @@ static void wt_porcelain_v2_print_changed_entry(*/ sep_char ='\0' ; eol_char ='\0' ; - path_index = it->string; - path_head = d->head_path; + path = it->string; + path_from = d->rename_source; }else {/* * Path(s) are C-quoted if necessary. Current path is ALWAYS first.
@@ -2068,27 +2070,27 @@ static void wt_porcelain_v2_print_changed_entry(*/ sep_char ='\t' ; eol_char ='\n' ; - path_index =quote_path (it->string, s->prefix, &buf_index); -if (d->head_path) - path_head =quote_path (d->head_path, s->prefix, &buf_head); + path =quote_path (it->string, s->prefix, &buf); +if (d->rename_source) + path_from =quote_path (d->rename_source, s->prefix, &buf_from); } -if (path_head) +if (path_from)fprintf (s->fp,"2 %s %s %06o %06o %06o %s %s %c%d %s%c%s%c" , key, submodule_token, d->mode_head, d->mode_index, d->mode_worktree,oid_to_hex (&d->oid_head),oid_to_hex (&d->oid_index), - key[0 ], d->score, - path_index, sep_char, path_head, eol_char); + d->rename_status, d->rename_score, + path, sep_char, path_from, eol_char);else fprintf (s->fp,"1 %s %s %06o %06o %06o %s %s %s%c" , key, submodule_token, d->mode_head, d->mode_index, d->mode_worktree,oid_to_hex (&d->oid_head),oid_to_hex (&d->oid_index), - path_index, eol_char); + path, eol_char); -strbuf_release (&buf_index); -strbuf_release (&buf_head); +strbuf_release (&buf); +strbuf_release (&buf_from); }/*
wt-status.h +3 -2
@@ -44,10 +44,11 @@ struct wt_status_change_data {int worktree_status;int index_status;int stagemask; -int score;int mode_head, mode_index, mode_worktree;struct object_id oid_head, oid_index; -char *head_path; +int rename_status; +int rename_score; +char *rename_source; unsigned dirty_submodule : 2; unsigned new_submodule_commits : 1; };
Signature · 1 line
2.15.0.320.g0453912d77
PATCH v3 6/6 wt-status.c: handle worktree renames
In reply to Nguyễn Thái Ngọc DuyBefore 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist in index" - 2016-10-24) there are never "new files" in the index, which essentially disables rename detection because we only detect renames when a new file appears in a diff pair.
After that commit, an i-t-a entry can appear as a new file in "git diff-files". But the diff callback function in wt-status.c does not handle this case and produces incorrect status output.
PS. The reader may notice that this patch adds a new xstrdup() but not a free(). Yes we leak memory (the same for head_path). But wt_status so far has been short lived, this leak should not matter in practice.
--- Documentation/git-status.txt | 23 +++++++++-------- t/t2203-add-intent.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++ wt-status.c | 22 +++++++++++++--- 3 files changed, 92 insertions(+), 13 deletions(-)
Documentation/git-status.txt +13 -10
@@ -149,14 +149,15 @@ the status.relativePaths config option below. Short Format ~~~~~~~~~~~~ -In the short-format, the status of each path is shown as +In the short-format, the status of each path is shown as one of these +forms - XY PATH1 -> PATH2 + XY PATH + XY ORIG_PATH -> PATH -where `PATH1` is the path in the `HEAD`, and the " `-> PATH2`" part is -shown only when `PATH1` corresponds to a different path in the -index/worktree (i.e. the file is renamed). The `XY` is a two-letter -status code. +where `ORIG_PATH` is where the renamed/copied contents came +from. `ORIG_PATH` is only shown when the entry is renamed or +copied. The `XY` is a two-letter status code. The fields (including the `->`) are separated from each other by a single space. If a filename contains whitespace or other nonprintable
@@ -192,6 +193,8 @@ in which case `XY` are `!!`. [MARC] index and work tree matches [ MARC] M work tree changed since index [ MARC] D deleted in work tree + [ D] R renamed in work tree + [ D] C copied in work tree ------------------------------------------------- D D unmerged, both deleted A U unmerged, added by us
@@ -309,13 +312,13 @@ Renamed or copied entries have the following format: of similarity between the source and target of the move or copy). For example "R100" or "C75". <path> The pathname. In a renamed/copied entry, this - is the path in the index and in the working tree. + is the target path. <sep> When the `-z` option is used, the 2 pathnames are separated with a NUL (ASCII 0x00) byte; otherwise, a tab (ASCII 0x09) byte separates them. - <origPath> The pathname in the commit at HEAD. This is only - present in a renamed/copied entry, and tells - where the renamed/copied contents came from. + <origPath> The pathname in the commit at HEAD or in the index. + This is only present in a renamed/copied entry, and + tells where the renamed/copied contents came from. -------------------------------------------------------- Unmerged entries have the following format; the first character is
t/t2203-add-intent.sh +60 -0
65 lines not on this page · raw message
wt-status.c +19 -3
@@ -360,8 +360,6 @@ static void wt_longstatus_print_change_data(struct wt_status *s,switch (change_type) {case WT_STATUS_UPDATED : status = d->index_status; -if (d->rename_source) - one_name = d->rename_source;break ;case WT_STATUS_CHANGED :if (d->new_submodule_commits || d->dirty_submodule) {
@@ -382,6 +380,14 @@ static void wt_longstatus_print_change_data(struct wt_status *s, change_type); } +/* +* Only pick up the rename it's relevant. If the rename is for +* the changed section and we're printing the updated section, +* ignore it. +*/ +if (d->rename_status == status) + one_name = d->rename_source; + one =quote_path (one_name, s->prefix, &onebuf); two =quote_path (two_name, s->prefix, &twobuf);
@@ -433,7 +439,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,struct wt_status_change_data *d; p = q->queue[i]; - it =string_list_insert (&s->change, p->one->path); + it =string_list_insert (&s->change, p->two->path); d = it->util;if (!d) { d =xcalloc (1 ,sizeof (*d));
@@ -460,6 +466,14 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,/* mode_worktree is zero for a delete. */ break ; +case DIFF_STATUS_COPIED : +case DIFF_STATUS_RENAMED : +if (d->rename_status) +die ("BUG: multiple renames on the same target? how?" ); + d->rename_source =xstrdup (p->one->path); + d->rename_score = p->score *100 /MAX_SCORE ; + d->rename_status = p->status; +/* fallthru */ case DIFF_STATUS_MODIFIED :case DIFF_STATUS_TYPE_CHANGED :case DIFF_STATUS_UNMERGED :
@@ -531,6 +545,8 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,case DIFF_STATUS_COPIED :case DIFF_STATUS_RENAMED : +if (d->rename_status) +die ("BUG: multiple renames on the same target? how?" ); d->rename_source =xstrdup (p->one->path); d->rename_score = p->score *100 /MAX_SCORE ; d->rename_status = p->status;
Signature · 1 line
2.15.0.320.g0453912d77
Re: [PATCH v3 0/6] Renames in git-status "changed not staged" section
In reply to Nguyễn Thái Ngọc DuyHi Duy,
Quoted 5 lines · Nguyễn Thái Ngọc Duy
v3 more or less goes back to v1 after my discussion with Igor about porcelain formats. So 7/7 is not needed anymore. 4/7 becomes 5/6. The meat is still in 6/6, now with some more updates in git-status.txt and to address the comment from Torsten.
Albeit a tiny concern expressed in that last e-mail[1], this now seems fine, and a few tests I did came back as expected. Thanks!
Regards, Buga
Re: [PATCH v3 0/6] Renames in git-status "changed not staged" section
In reply to Nguyễn Thái Ngọc DuyQuoted 13 lines · Nguyễn Thái Ngọc Duy
v3 more or less goes back to v1 after my discussion with Igor about porcelain formats. So 7/7 is not needed anymore. 4/7 becomes 5/6. The meat is still in 6/6, now with some more updates in git-status.txt and to address the comment from Torsten.
Nguyễn Thái Ngọc Duy (6):
t2203: test status output with porcelain v2 format
UseDIFF_DETECT_RENAMEfordetect_renameassignmentswt-status.c: coding style fix
wt-status.c: catch unhandled diff status codeswt-status.c: rename rename-related fields inwt_status_change_datawt-status.c: handle worktree renames
Quoted 8 lines
Documentation/git-status.txt | 23 ++++++------ builtin/commit.c | 2 +- diff.c | 2 +- t/t2203-add-intent.sh | 72 ++++++++++++++++++++++++++++++++++++++ wt-status.c | 83 ++++++++++++++++++++++++++++---------------- wt-status.h | 5 +-- 6 files changed, 143 insertions(+), 44 deletions(-)
Re: [PATCH] status: handle worktree renames
In reply to Nguyễn Thái Ngọc DuyOn 2017-12-25 11:37, Nguyễn Thái Ngọc Duy wrote:
[]
Quoted 9 lines
wt-status.c | 24 +++++++++++++++++++----- wt-status.h | 1 + 3 files changed, 35 insertions(+), 5 deletions(-)t/t2203-add-intent.sh +0
-0
t/t2203-add-intent.sh test_expect_success 'commit: ita entries ignored in empty commit check' ' · line 157
Quoted 8 lines
t/t2203-add-intent.sh +5
-0@@ -150,5 +150,20 @@ test_expect_success 'commit: ita entries ignored in empty commit check' ' ) ' +test_expect_success 'rename detection finds the right names' ' + git init rename-detection && + ( + cd rename-detection && + echo contents > original-file &&
Micro-nit, please no " " after ">":
echo contents >original-file