Re: [PATCH] status: handle worktree renames

2018-01-10 Thread Duy Nguyen 
On Wed, Jan 3, 2018 at 4:14 AM, Jeff Hostetler  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

Re: [PATCH] status: handle worktree renames

2018-01-02 Thread Jeff Hostetler 



On 12/27/2017 1:12 PM, Junio C Hamano wrote:

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

2017-12-27 Thread Junio C Hamano 
Duy Nguyen  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 "" 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

2017-12-27 Thread Junio C Hamano 
Duy Nguyen  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

2017-12-26 Thread 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

Re: [PATCH] status: handle worktree renames

2017-12-25 Thread Duy Nguyen 
On Tue, Dec 26, 2017 at 9:11 AM, Duy Nguyen  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-fileoriginal-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".
>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.
> 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.
> -  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.
> +  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 "" 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

2017-12-25 Thread 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-fileoriginal-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".
   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.
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.
-  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.
+  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

2017-12-25 Thread 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-fileoriginal-file" 
>expected &&
+   test_cmp expected actual
+   )
+'
+
 test_done
 
-- 
2.15.1.windows.2

Re: [PATCH] status: handle worktree renames

2017-12-25 Thread 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-fileoriginal-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, _index);
-   if (d->head_path)
-   path_head = quote_path(d->head_path, s->prefix, 
_head);
+   if (path_head)
+   path_head = quote_path(path_head, s->prefix, _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(>oid_head), 
oid_to_hex(>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

2017-12-25 Thread 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 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  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(, ')');
>   }
>   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(>change, p->one->path);
> + it = string_list_insert(>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(>oid_index, >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, );
> +
> + one = d->head_path ? d->head_path : d->worktree_path;
> + if (one) {
> + one = quote_path(one, s->prefix, );
>   if (*one != '"' && strchr(one, ' ') != NULL) {
>   putchar('"');
>

[PATCH] status: handle worktree renames

2017-12-25 Thread 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 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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(, ')');
}
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(>change, p->one->path);
+   it = string_list_insert(>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(>oid_index, >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, );
+
+   one = d->head_path ? d->head_path : d->worktree_path;
+   if (one) {
+   one = quote_path(one, s->prefix, );
if (*one != '"' && strchr(one, ' ') != NULL) {
putchar('"');
strbuf_addch(, '"');
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