Hi Duy,
On 25/12/2017 11:37, Nguyễn Thái Ngọc Duy wrote:
> 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.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.
>
> Noticed-by: Alex Vandiver <ale...@dropbox.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> ---
> t/t2203-add-intent.sh | 15 +++++++++++++++
> wt-status.c | 24 +++++++++++++++++++-----
> wt-status.h | 1 +
> 3 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 1bdf38e80d..41a8874e60 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -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
>
> diff --git a/wt-status.c b/wt-status.c
> index ef26f07446..f0b5b3d46a 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -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, '"');
> diff --git a/wt-status.h b/wt-status.h
> index fe27b465e2..572a720123 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -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-file
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.
Regards, Buga
[1] https://git-scm.com/docs/git-status#_porcelain_format_version_2
