[BUG] File move with `add -N` shows as rename to same name

[BUG] File move with `add -N` shows as rename to same name

[Alex Vandiver]

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<--------------------
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1bdf38e80..97b6c0f05 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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<--------------------

Re: [BUG] File move with `add -N` shows as rename to same name

[Duy Nguyen]

On Sat, Dec 23, 2017 at 9:42 AM, Alex Vandiver <alexmv@dropbox.com> wrote:
> I just stumbled across the following oddity:

Thanks. I'm looking into it.
-- 
Duy

[PATCH] status: handle worktree renames

[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.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;
 };
-- 
2.15.0.320.g0453912d77

Re: [PATCH] status: handle worktree renames

[Igor Djordjevic]

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

Re: [PATCH] status: handle worktree renames

[Igor Djordjevic]

On 25/12/2017 19:26, Igor Djordjevic wrote:
> 
> 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.

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(-)

diff --git a/wt-status.c b/wt-status.c
index f0b5b3d46..55c0ad249 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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",
-- 
2.15.1.windows.2

Re: [PATCH] status: handle worktree renames

[Igor Djordjevic]

On 25/12/2017 20:45, Igor Djordjevic wrote:
> 
> I guess an additional test for this would be good, too.

... aaand here it is. Again based on your test, but please double 
check, I`m not sure if it`s 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(+)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 41a8874e6..394b1047c 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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
 
-- 
2.15.1.windows.2

Re: [PATCH] status: handle worktree renames

[Duy Nguyen]

On Mon, Dec 25, 2017 at 07:26:27PM +0100, Igor Djordjevic wrote:
> 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.

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

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 81cab9aefb..3da10020aa 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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

[Duy Nguyen]

On Tue, Dec 26, 2017 at 9:11 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Dec 25, 2017 at 07:26:27PM +0100, Igor Djordjevic wrote:
>> 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.
>
> 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
>
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 81cab9aefb..3da10020aa 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -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?).

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.
-- 
Duy

Re: [PATCH] status: handle worktree renames

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> 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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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

[Jeff Hostetler]

On 12/27/2017 1:12 PM, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
> 
>> 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

[Duy Nguyen]

On Wed, Jan 3, 2018 at 4:14 AM, Jeff Hostetler <git@jeffhostetler.com> wrote:
> 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).
-- 
Duy

[PATCH v2 0/7] Renames in git-status "changed not staged" section

[Nguyễn Thái Ngọc Duy]

The 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(-)

-- 
2.15.0.320.g0453912d77

[PATCH v2 1/7] t2203: test status output with porcelain v2 format

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2203-add-intent.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1bdf38e80d..878e73fe98 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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) &&
-- 
2.15.0.320.g0453912d77

[PATCH v2 2/7] Use DIFF_DETECT_RENAME for detect_rename assignments

[Nguyễn Thái Ngọc Duy]

This 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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c | 2 +-
 diff.c           | 2 +-
 wt-status.c      | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..1f11e3992d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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 --git a/diff.c b/diff.c
index 3fb445a54d..51fe31c7aa 100644
--- a/diff.c
+++ b/diff.c
@@ -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)
diff --git a/wt-status.c b/wt-status.c
index ef26f07446..59338adb8b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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;
 	/*
-- 
2.15.0.320.g0453912d77

[PATCH v2 3/7] wt-status.c: coding style fix

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 59338adb8b..db06fc7c85 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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)
-- 
2.15.0.320.g0453912d77

[PATCH v2 4/7] wt-status.c: rename wt_status_change_data::score

[Nguyễn Thái Ngọc Duy]

We 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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 4 ++--
 wt-status.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index db06fc7c85..0f089c5789 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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",
diff --git a/wt-status.h b/wt-status.h
index fe27b465e2..f9330982ac 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -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;
-- 
2.15.0.320.g0453912d77

[PATCH v2 5/7] wt-status.c: catch unhandled diff status codes

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 0f089c5789..c124d7589c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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;
 		}
 	}
 }
-- 
2.15.0.320.g0453912d77

[PATCH v2 6/7] wt-status.c: handle worktree renames

[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.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.

Noticed-by: Alex Vandiver <alexmv@dropbox.com>
Helped-by: Igor Djordjevic <igor.d.djordjevic@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2203-add-intent.sh | 28 ++++++++++++++++++++
 wt-status.c           | 72 +++++++++++++++++++++++++++++++++++++++------------
 wt-status.h           |  4 +--
 3 files changed, 85 insertions(+), 19 deletions(-)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 878e73fe98..e5bfda1853 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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
 
diff --git a/wt-status.c b/wt-status.c
index c124d7589c..d5bdf4c2e9 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)
+			two_name = d->worktree_path;
 		break;
 	default:
 		die("BUG: unhandled change_type %d in wt_longstatus_print_change_data",
@@ -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);
+			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");
+
+	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;
+	}
+
 	if (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);
 	}
 
-	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);
 	else
 		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);
 }
 
 /*
diff --git a/wt-status.h b/wt-status.h
index f9330982ac..332ff545aa 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -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;
 };
-- 
2.15.0.320.g0453912d77

Re: [PATCH v2 6/7] wt-status.c: handle worktree renames

[Igor Djordjevic]

Hi Duy,

On 26/12/2017 10:10, 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.
> 
> 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.
> 
> Noticed-by: Alex Vandiver <alexmv@dropbox.com>
> Helped-by: Igor Djordjevic <igor.d.djordjevic@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t2203-add-intent.sh | 28 ++++++++++++++++++++
>  wt-status.c           | 72 +++++++++++++++++++++++++++++++++++++++------------
>  wt-status.h           |  4 +--
>  3 files changed, 85 insertions(+), 19 deletions(-)
> 
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 878e73fe98..e5bfda1853 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -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

I`m 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-file

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.
  ...
  <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-file

> diff --git a/wt-status.c b/wt-status.c
> index c124d7589c..d5bdf4c2e9 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)
> +			two_name = d->worktree_path;
>  		break;
>  	default:
>  		die("BUG: unhandled change_type %d in wt_longstatus_print_change_data",
> @@ -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()?

> +			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");
> +
> +	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.

>  	if (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);
>  	}
>  
> -	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).

>  	else
>  		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);
>  }
>  
>  /*
> diff --git a/wt-status.h b/wt-status.h
> index f9330982ac..332ff545aa 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -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;
>  };
> 

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/20171226091012.24315-2-pclouds@gmail.com/T/#m866cf8b7edfd40172972771079c50bd5ff5bd535
[2] https://git-scm.com/docs/git-status#_porcelain_format_version_2
[3] https://public-inbox.org/git/20171226091012.24315-2-pclouds@gmail.com/T/#m095c33d69994c6ecb4f1adbf80dd48eab66750d8

Re: [PATCH v2 6/7] wt-status.c: handle worktree renames

[Duy Nguyen]

On Wed, Dec 27, 2017 at 1:14 AM, Igor Djordjevic
<igor.d.djordjevic@gmail.com> wrote:
> I`m 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-file
>
> 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".

>   ...
>   <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!
-- 
Duy

Re: [PATCH v2 6/7] wt-status.c: handle worktree renames

[Igor Djordjevic]

On 27/12/2017 02:06, Duy Nguyen wrote:
> 
> >   ... <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

> 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)

> >   <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 wasn`t to be found inside HEAD at 
all, so the working tree rename couldn`t 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 :)

> 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/20171227101839.26427-7-pclouds@gmail.com/T/#u

Re: [PATCH v2 6/7] wt-status.c: handle worktree renames

[Igor Djordjevic]

p.s. An extra note for the casual reader, just in case:

On 28/12/2017 01:50, Igor Djordjevic wrote:
> 
> ... 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 couldn`t really be shown as "source" 
> and "target" rename pair, strictly following the "porcelain v2" 
> specification... :/
> 
> ...
> 
> 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, it`s 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

[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 -> third

or we could show renames as two lines instead of one, for example
something like this for short form:

    R  first -> second
     R second -> third

But for now it's safer and simpler to just break the "second -> third"
rename pair and show

    RD first -> second
     A third

like we have been showing until now.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2203-add-intent.sh | 33 +++++++++++++++++++++++++++++
 wt-status.c           | 58 ++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 84 insertions(+), 7 deletions(-)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index e5bfda1853..79aca93810 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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
 
diff --git a/wt-status.c b/wt-status.c
index d5bdf4c2e9..e62853f748 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -419,6 +419,47 @@ static char short_submodule_status(struct wt_status_change_data *d)
 	return d->worktree_status;
 }
 
+static struct string_list_item * break_double_rename(
+		struct wt_status *s, struct string_list_item *it,
+		int *status, struct diff_filepair *p)
+{
+	struct wt_status_change_data *d;
+	struct string_list_item *new_it;
+
+	d = it->util;
+	/*
+	 * _collect_index_changes() must have been called or
+	 * d->head_path does not contain a real value.
+	 */
+	if (!d || !d->head_path)
+		return it;
+
+	switch (s->status_format) {
+	case STATUS_FORMAT_SHORT:
+	case STATUS_FORMAT_PORCELAIN:
+	case STATUS_FORMAT_PORCELAIN_V2:
+		break;
+	case STATUS_FORMAT_LONG:
+	case STATUS_FORMAT_NONE:
+		/* this output can handle double renames ok */
+		return it;
+	default:
+		die("BUG: finalize_deferred_config() should have been called");
+	}
+
+	switch (*status) {
+	case DIFF_STATUS_RENAMED:
+		d->worktree_status = DIFF_STATUS_DELETED;
+		/* fallthru */
+	case DIFF_STATUS_COPIED:
+		*status = DIFF_STATUS_ADDED;
+		new_it = string_list_insert(&s->change, p->two->path);
+		return new_it;
+	}
+
+	return it;
+}
+
 static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 					 struct diff_options *options,
 					 void *data)
@@ -433,16 +474,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 		struct diff_filepair *p;
 		struct string_list_item *it;
 		struct wt_status_change_data *d;
+		int status;
 
 		p = q->queue[i];
+		status = p->status;
 		it = string_list_insert(&s->change, p->one->path);
+		it = break_double_rename(s, it, &status, p);
 		d = it->util;
 		if (!d) {
 			d = xcalloc(1, sizeof(*d));
 			it->util = d;
 		}
 		if (!d->worktree_status)
-			d->worktree_status = p->status;
+			d->worktree_status = status;
 		if (S_ISGITLINK(p->two->mode)) {
 			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
@@ -451,7 +495,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 				d->worktree_status = short_submodule_status(d);
 		}
 
-		switch (p->status) {
+		switch (status) {
 		case DIFF_STATUS_ADDED:
 			d->mode_worktree = p->two->mode;
 			break;
@@ -477,7 +521,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 			break;
 
 		default:
-			die("BUG: unhandled diff-files status '%c'", p->status);
+			die("BUG: unhandled diff-files status '%c'", status);
 			break;
 		}
 
@@ -710,12 +754,12 @@ static void wt_status_collect_untracked(struct wt_status *s)
 
 void wt_status_collect(struct wt_status *s)
 {
-	wt_status_collect_changes_worktree(s);
-
 	if (s->is_initial)
 		wt_status_collect_changes_initial(s);
 	else
+		/* must be called before _collect_changes_worktree() */
 		wt_status_collect_changes_index(s);
+	wt_status_collect_changes_worktree(s);
 	wt_status_collect_untracked(s);
 }
 
@@ -1733,7 +1777,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 	putchar(' ');
 
 	if (d->head_path && d->worktree_path)
-		die("BUG: to be addressed in the next patch");
+		die("BUG: break_double_rename() fails to break this pair");
 
 	if (d->head_path) {
 		from = d->head_path;
@@ -2077,7 +2121,7 @@ static void wt_porcelain_v2_print_changed_entry(
 	key[2] = 0;
 
 	if (d->head_path && d->worktree_path)
-		die("BUG: to be addressed in the next patch");
+		die("BUG: break_double_rename() fails to break this pair");
 
 	if (d->head_path) {
 		path_other = d->head_path;
-- 
2.15.0.320.g0453912d77

Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format

[Igor Djordjevic]

Hi Duy,

On 26/12/2017 10:10, Nguyễn Thái Ngọc Duy wrote:
> 
> The presence of worktree rename leads to an interesting situation,
> what if the same index entry is renamed twice, compared to HEAD and to
> worktree? We can have that with this setup
> 
>     echo first > first && git add first && git commit -m first
>     git mv first second  # rename reported in "diff --cached"
>     mv second third      # rename reported in "diff-files"
> 
> For the long format this is fine because we print two "->" rename
> lines, one in the "updated" section, one in "changed" one.
> 
> For other output formats, it gets tricky because they combine both
> diffs in one line but can only display one rename per line. The result
> "XY" column of short format, for example, would be "RR" in that case.
> 
> This case either needs some extension in short/porcelain format
> to show something crazy like
> 
>     RR first -> second -> third
> 
> or we could show renames as two lines instead of one, for example
> something like this for short form:
> 
>     R  first -> second
>      R second -> third
> 
> But for now it's safer and simpler to just break the "second -> third"
> rename pair and show
> 
>     RD first -> second
>      A third
> 
> like we have been showing until now.
> 

I lost you a bit here, partially because of what seems to be an 
incomplete setup script, partially because of this last sentence, as 
Git v2.15.1 doesn`t seem to be showing this, so not sure about "like 
we have been showing until now" part...?

Here, with your setup script, with plain Git v2.15.1, we have:

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

Might be an additional `git add -N -- third` is needed here, to show 
what (I assume) you wanted...? If so:

    $ git add -N third
(1) $ git status
    On branch master
    Changes to be committed:
      (use "git reset HEAD <file>..." to unstage)
    
            renamed:    first -> second
    
    Changes not staged for commit:
      (use "git add <file>..." to update what will be committed)
      (use "git checkout -- <file>..." to discard changes in working directory)
    
            renamed:    second -> second
                                  ^^^^^^
Now we can see two renames I believe you were talking about...? (Note 
original bug showing above, which started this thread.) Now, still 
using v2.15.1, let`s see porcelain statuses:

(2) $ git status --porcelain
    RR first -> second
    
(3) $ git status --porcelain=v2
    2 RR N... 100644 100644 000000 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second        first

Here, they both report renames in _both_ index and working tree (RR), 
but they show "index" renamed path only ("second", in comparison to 
original value in HEAD, "first").

I`m inclined to say this doesn`t align with what `git status` shows, 
disrespecting `add -N` (or respecting it only partially, through that 
second R, but not showing the actual working tree rename, "third").

Without influencing porcelain format, and to fully respect `add -N`, 
I believe showing two renames (index and working tree) as two lines 
would be the correct approach - and that`s what default `git status` 
does, too.

Now, let`s examine this patch series v2 outputs:

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

(3) $ git status --porcelain=v2
    2 RD N... 100644 100644 000000 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second	first
    1 .A N... 000000 000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 third

Here, porcelain statuses make situation a bit better, as now at least 
`add -N` is respected, showing new "tracked" path appearing in the 
working tree.

But, we now lost any idea about the rename that happened there as 
well - which Git v2.15.1 porcelain was partially showing (through 
RR), and which `git status` still reports correctly - and which we 
still differ from.
 
I don`t think this looks like what we have been showing until now 
(unless I misunderstood which exact "now" are we talking about), so I 
don`t see that as a valid argument to support this case.

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

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

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

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

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

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

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

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

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

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

Thanks, Buga

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

Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format

[Duy Nguyen]

On Wed, Dec 27, 2017 at 5:14 AM, Igor Djordjevic
<igor.d.djordjevic@gmail.com> wrote:
> Hi Duy,
>
> On 26/12/2017 10:10, Nguyễn Thái Ngọc Duy wrote:
>>
>> The presence of worktree rename leads to an interesting situation,
>> what if the same index entry is renamed twice, compared to HEAD and to
>> worktree? We can have that with this setup
>>
>>     echo first > first && git add first && git commit -m first
>>     git mv first second  # rename reported in "diff --cached"
>>     mv second third      # rename reported in "diff-files"
>>
>> For the long format this is fine because we print two "->" rename
>> lines, one in the "updated" section, one in "changed" one.
>>
>> For other output formats, it gets tricky because they combine both
>> diffs in one line but can only display one rename per line. The result
>> "XY" column of short format, for example, would be "RR" in that case.
>>
>> This case either needs some extension in short/porcelain format
>> to show something crazy like
>>
>>     RR first -> second -> third
>>
>> or we could show renames as two lines instead of one, for example
>> something like this for short form:
>>
>>     R  first -> second
>>      R second -> third
>>
>> But for now it's safer and simpler to just break the "second -> third"
>> rename pair and show
>>
>>     RD first -> second
>>      A third
>>
>> like we have been showing until now.
>>
>
> I lost you a bit here, partially because of what seems to be an
> incomplete setup script, partially because of this last sentence, as
> Git v2.15.1 doesn`t seem to be showing this, so not sure about "like
> we have been showing until now" part...?

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 third

The second line is different and is what 425a28e0a4 tries to fix.

> Now, still using v2.15.1, let`s see porcelain statuses:
>
> (2) $ git status --porcelain
>     RR first -> second
>
> (3) $ git status --porcelain=v2
>     2 RR N... 100644 100644 000000 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second        first
>
> Here, they both report renames in _both_ index and working tree (RR),
> but they show "index" renamed path only ("second", in comparison to
> original value in HEAD, "first").
>
> I`m inclined to say this doesn`t align with what `git status` shows,
> disrespecting `add -N` (or respecting it only partially, through that
> second R, but not showing the actual working tree rename, "third").
>
> Without influencing porcelain format, and to fully respect `add -N`,
> I believe showing two renames (index and working tree) as two lines
> would be the correct approach - and that`s what default `git status`
> does, too.

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?

> Now, let`s examine this patch series v2 outputs:
>
> (1) $ git status
>     On branch master
>     Changes to be committed:
>       (use "git reset HEAD <file>..." to unstage)
>
>         renamed:    first -> second
>
>     Changes not staged for commit:
>       (use "git add <file>..." to update what will be committed)
>       (use "git checkout -- <file>..." to discard changes in working directory)
>
>         renamed:    second -> third
>
> (2) $ git status --porcelain
>     RD first -> second
>      A third
>
> (3) $ git status --porcelain=v2
>     2 RD N... 100644 100644 000000 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second        first
>     1 .A N... 000000 000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 third
>
> Here, porcelain statuses make situation a bit better, as now at least
> `add -N` is respected, showing new "tracked" path appearing in the
> working tree.
>
> But, we now lost any idea about the rename that happened there as
> well - which Git v2.15.1 porcelain was partially showing (through
> RR), and which `git status` still reports correctly - and which we
> still differ from.

Sorry again about "now". Before 425a28e0a4 rename detection would not
kick in to find "second -> third" so people wouldn't know about rename
anyway.

>
> I don`t think this looks like what we have been showing until now
> (unless I misunderstood which exact "now" are we talking about), so I
> don`t see that as a valid argument to support this case.
>
> So, while we still changed output of what we were showing so far to
> two-line output, it seems there`s no real gain, as it looks like we
> replaced one partial output (recognize rename, omit path) for the
> other (recognize path, omit rename).
>
> Finally, let`s see your initial patch v1[1], with my exercise
> patch[2] on top:
>
> (1) $ git status
>     On branch master
>     Changes to be committed:
>       (use "git reset HEAD <file>..." to unstage)
>
>         renamed:    first -> second
>
>     Changes not staged for commit:
>       (use "git add <file>..." to update what will be committed)
>       (use "git checkout -- <file>..." to discard changes in working directory)
>
>         renamed:    second -> third
>
> (2) $ git status --porcelain
>     R  first -> second
>      R second -> third
>
> (3) $ git status --porcelain=v2
>     2 R. N... 100644 100644 100644 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 second        first
>     2 .R N... 100644 100644 100644 9c59e24b8393179a5d712de4f990178df5734d99 9c59e24b8393179a5d712de4f990178df5734d99 R100 third second
>
> Here, both "--porcelain" outputs (2) and (3) seem to much better
> replicate what default `git status` is showing, too - namely separate
> renames in comparison to HEAD for both "index" (2) and "working tree" (3).

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.

> And if you don`t like two lines here in comparison to one (incomplete)
> line from Git v2.15.1, I would remark that patch series v2 prints two
> lines as well (so different from v2.15.1 in a same way), but with
> what looks like inferior output in comparison to v1 shown above, where
> both renames are correctly recognized and reported - and finally
> fully compatible with default `git status` output, too.
>
> And if we really think about it, what v1 shows is what actually
> happened - and more important, it`s possible to recreate hypothetical
> "first -> second -> third" change from there. With v2 output, that is
> impossible, that information is lost as second line doesn`t relate to
> the first one in any way.
>
> Now, unless I`m totally missing something here, the only thing left
> is that you mentioned v2 approach being "safer and simpler" than v1,
> something I`m not really competent to comment on, but just wanted to
> provide a second opinion, maybe helping to change your mind in favor
> of v1 outputs, which seem to be _the_ correct ones...? :)

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.

> If not that much more complicated/unsafe, of course.
>
> Thanks, Buga
>
> [1] https://public-inbox.org/git/20171226091012.24315-8-pclouds@gmail.com/T/#mf60e88fd351f7ff6a076279794c8343a79835f67
> [2] https://public-inbox.org/git/20171226091012.24315-8-pclouds@gmail.com/T/#m095c33d69994c6ecb4f1adbf80dd48eab66750d8
-- 
Duy

Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format

[Igor Djordjevic]

On 27/12/2017 01:49, Duy Nguyen wrote:
> 
> > 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.
> ...
> Sorry again about "now". Before 425a28e0a4 rename 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

[PATCH v3 0/6] Renames in git-status "changed not staged" section

[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
  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(-)

-- 
2.15.0.320.g0453912d77

[PATCH v3 1/6] t2203: test status output with porcelain v2 format

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2203-add-intent.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1bdf38e80d..878e73fe98 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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) &&
-- 
2.15.0.320.g0453912d77

[PATCH v3 2/6] Use DIFF_DETECT_RENAME for detect_rename assignments

[Nguyễn Thái Ngọc Duy]

This 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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c | 2 +-
 diff.c           | 2 +-
 wt-status.c      | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..1f11e3992d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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 --git a/diff.c b/diff.c
index 3fb445a54d..51fe31c7aa 100644
--- a/diff.c
+++ b/diff.c
@@ -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)
diff --git a/wt-status.c b/wt-status.c
index ef26f07446..59338adb8b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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;
 	/*
-- 
2.15.0.320.g0453912d77

[PATCH v3 3/6] wt-status.c: coding style fix

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 59338adb8b..db06fc7c85 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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)
-- 
2.15.0.320.g0453912d77

[PATCH v3 4/6] wt-status.c: catch unhandled diff status codes

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index db06fc7c85..05af895fe2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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;
 		}
 	}
 }
-- 
2.15.0.320.g0453912d77

[PATCH v3 5/6] wt-status.c: rename rename-related fields in wt_status_change_data

[Nguyễn Thái Ngọc Duy]

These 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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 50 ++++++++++++++++++++++++++------------------------
 wt-status.h |  5 +++--
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 05af895fe2..fab6951573 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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);
 }
 
 /*
diff --git a/wt-status.h b/wt-status.h
index fe27b465e2..3f84d5c29f 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -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;
 };
-- 
2.15.0.320.g0453912d77

[PATCH v3 6/6] wt-status.c: handle worktree renames

[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.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.

Noticed-by: Alex Vandiver <alexmv@dropbox.com>
Helped-by: Igor Djordjevic <igor.d.djordjevic@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-status.txt | 23 +++++++++--------
 t/t2203-add-intent.sh        | 60 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 22 +++++++++++++---
 3 files changed, 92 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 81cab9aefb..72bfb87f66 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 878e73fe98..78236dc7d8 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -162,5 +162,65 @@ 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 &&
+
+		hash=12f00e90b6ef79117ce6e650416b8cf517099b78 &&
+		git status --porcelain=v2 | grep -v "^?" >actual.3 &&
+		cat >expected.3 <<-EOF &&
+		2 .R N... 100644 100644 100644 $hash $hash R100 third	first
+		EOF
+		test_cmp expected.3 actual.3
+	)
+'
+
+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 &&
+		R  first -> second
+		 R second -> third
+		EOF
+		test_cmp expected.2 actual.2 &&
+
+		hash=12f00e90b6ef79117ce6e650416b8cf517099b78 &&
+		git status --porcelain=v2 | grep -v "^?" >actual.3 &&
+		cat >expected.3 <<-EOF &&
+		2 R. N... 100644 100644 100644 $hash $hash R100 second	first
+		2 .R N... 100644 100644 100644 $hash $hash R100 third	second
+		EOF
+		test_cmp expected.3 actual.3
+	)
+'
+
 test_done
 
diff --git a/wt-status.c b/wt-status.c
index fab6951573..f5debcd2b4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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;
-- 
2.15.0.320.g0453912d77

Re: [PATCH v3 0/6] Renames in git-status "changed not staged" section

[Igor Djordjevic]

Hi Duy,

On 27/12/2017 11:18, Nguyễn Thái Ngọc Duy wrote:
> 
> 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

[1] https://public-inbox.org/git/CACsJy8A=jZ9LAuM50GVjNT5gtdiYYMyMuPBSrJFO4LmKVQsETQ@mail.gmail.com/T/#m18b4e2cb2b7685fcc9650f3fb71b2191ef74cbe1

Re: [PATCH v3 0/6] Renames in git-status "changed not staged" section

[Jeff Hostetler]

On 12/27/2017 5:18 AM, Nguyễn Thái Ngọc Duy wrote:
> 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
>    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(-)
> 


Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>

Re: [PATCH] status: handle worktree renames

[Torsten Bögershausen]

On 2017-12-25 11:37, Nguyễn Thái Ngọc Duy wrote:
[]
>  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 &&
Micro-nit, please no " " after ">":
echo contents >original-file