Re: [PATCH] status: handle worktree renames

[Igor Djordjevic <igor.d.djordjevic@gmail.com>]

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 <alexmv@dropbox.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@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