git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Igor Djordjevic <igor.d.djordjevic@gmail.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: alexmv@dropbox.com, git@vger.kernel.org
Subject: Re: [PATCH] status: handle worktree renames
Date: Mon, 25 Dec 2017 19:26:27 +0100	[thread overview]
Message-ID: <b3e90960-d743-3299-ba43-150849b591d2@gmail.com> (raw)
In-Reply-To: <20171225103718.24443-1-pclouds@gmail.com>

Hi Duy,

On 25/12/2017 11:37, Nguyễn Thái Ngọc Duy wrote:
> Before 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist
> in index" - 2016-10-24) there are never "new files" in the index, which
> essentially disables rename detection because we only detect renames
> when a new file appears in a diff pair.
> 
> After that commit, an i-t-a entry can appear as a new file in "git
> diff-files". But the diff callback function in wt-status.c does not
> handle this case and produces incorrect status output.
> 
> Handle this rename case. While at there make sure unhandled diff status
> code is reported to catch cases like this easier in the future.
> 
> The reader may notice that this patch adds a new xstrdup() but not a
> free(). Yes we leak memory (the same for head_path). But wt_status so
> far has been short lived, this leak should not matter in practice.
> 
> Noticed-by: Alex Vandiver <alexmv@dropbox.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t2203-add-intent.sh | 15 +++++++++++++++
>  wt-status.c           | 24 +++++++++++++++++++-----
>  wt-status.h           |  1 +
>  3 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 1bdf38e80d..41a8874e60 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -150,5 +150,20 @@ test_expect_success 'commit: ita entries ignored in empty commit check' '
>  	)
>  '
>  
> +test_expect_success 'rename detection finds the right names' '
> +	git init rename-detection &&
> +	(
> +		cd rename-detection &&
> +		echo contents > original-file &&
> +		git add original-file &&
> +		git commit -m first-commit &&
> +		mv original-file new-file &&
> +		git add -N new-file &&
> +		git status --porcelain | grep -v actual >actual &&
> +		echo " R original-file -> new-file" >expected &&
> +		test_cmp expected actual
> +	)
> +'
> +
>  test_done
>  
> diff --git a/wt-status.c b/wt-status.c
> index ef26f07446..f0b5b3d46a 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -376,6 +376,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
>  			strbuf_addch(&extra, ')');
>  		}
>  		status = d->worktree_status;
> +		if (d->worktree_path)
> +			one_name = d->worktree_path;
>  		break;
>  	default:
>  		die("BUG: unhandled change_type %d in wt_longstatus_print_change_data",
> @@ -432,7 +434,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
>  		struct wt_status_change_data *d;
>  
>  		p = q->queue[i];
> -		it = string_list_insert(&s->change, p->one->path);
> +		it = string_list_insert(&s->change, p->two->path);
>  		d = it->util;
>  		if (!d) {
>  			d = xcalloc(1, sizeof(*d));
> @@ -459,6 +461,12 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
>  			/* mode_worktree is zero for a delete. */
>  			break;
>  
> +		case DIFF_STATUS_COPIED:
> +		case DIFF_STATUS_RENAMED:
> +			d->worktree_path = xstrdup(p->one->path);
> +			d->score = p->score * 100 / MAX_SCORE;
> +			/* fallthru */
> +
>  		case DIFF_STATUS_MODIFIED:
>  		case DIFF_STATUS_TYPE_CHANGED:
>  		case DIFF_STATUS_UNMERGED:
> @@ -467,8 +475,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
>  			oidcpy(&d->oid_index, &p->one->oid);
>  			break;
>  
> -		case DIFF_STATUS_UNKNOWN:
> -			die("BUG: worktree status unknown???");
> +		default:
> +			die("BUG: unhandled worktree status '%c'", p->status);
>  			break;
>  		}
>  
> @@ -548,6 +556,10 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
>  			 * values in these fields.
>  			 */
>  			break;
> +
> +		default:
> +			die("BUG: unhandled worktree status '%c'", p->status);
> +			break;
>  		}
>  	}
>  }
> @@ -1724,8 +1736,10 @@ static void wt_shortstatus_status(struct string_list_item *it,
>  	} else {
>  		struct strbuf onebuf = STRBUF_INIT;
>  		const char *one;
> -		if (d->head_path) {
> -			one = quote_path(d->head_path, s->prefix, &onebuf);
> +
> +		one = d->head_path ? d->head_path : d->worktree_path;
> +		if (one) {
> +			one = quote_path(one, s->prefix, &onebuf);
>  			if (*one != '"' && strchr(one, ' ') != NULL) {
>  				putchar('"');
>  				strbuf_addch(&onebuf, '"');
> diff --git a/wt-status.h b/wt-status.h
> index fe27b465e2..572a720123 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -48,6 +48,7 @@ struct wt_status_change_data {
>  	int mode_head, mode_index, mode_worktree;
>  	struct object_id oid_head, oid_index;
>  	char *head_path;
> +	char *worktree_path;
>  	unsigned dirty_submodule       : 2;
>  	unsigned new_submodule_commits : 1;
>  };
> 

Thanks, I`ve tested it and the reported case seems to work correctly, 
indeed.

But I`ve noticed that "--porcelain=v2" output might still be buggy - 
this is what having both files staged shows:

    $ git status --porcelain=v2
    2 R. N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 R100 new-file	original-file

..., where having old/deleted file unstaged, and new/created file 
staged with `git add -N` shows this:

    $ git status --porcelain=v2
    1 .R N... 100644 100644 100644 12f00e90b6ef79117ce6e650416b8cf517099b78 12f00e90b6ef79117ce6e650416b8cf517099b78 new-file

So even though unstaged value is correctly recognized as "R" (renamed), 
first number is "1" (instead of "2" to signal rename/copy), and both 
rename score and original file name are missing.

Not sure if this is a bug, but it seems so, as `git status` "Porcelain 
Format Version 2"[1] says the last path is "pathname in the commit at 
HEAD" (in case of copy/rename), which is missing here.

Regards, Buga

[1] https://git-scm.com/docs/git-status#_porcelain_format_version_2

  reply	other threads:[~2017-12-25 18:26 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-23  2:42 [BUG] File move with `add -N` shows as rename to same name Alex Vandiver
2017-12-25  9:00 ` Duy Nguyen
2017-12-25 10:37 ` [PATCH] status: handle worktree renames Nguyễn Thái Ngọc Duy
2017-12-25 18:26   ` Igor Djordjevic [this message]
2017-12-25 19:45     ` Igor Djordjevic
2017-12-25 21:49       ` Igor Djordjevic
2017-12-26  2:11     ` Duy Nguyen
2017-12-26  2:53       ` Duy Nguyen
2017-12-27 18:17         ` Junio C Hamano
2017-12-27 18:12       ` Junio C Hamano
2018-01-02 21:14         ` Jeff Hostetler
2018-01-10  9:26           ` Duy Nguyen
2017-12-26  9:10   ` [PATCH v2 0/7] Renames in git-status "changed not staged" section Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 1/7] t2203: test status output with porcelain v2 format Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 2/7] Use DIFF_DETECT_RENAME for detect_rename assignments Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 3/7] wt-status.c: coding style fix Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 4/7] wt-status.c: rename wt_status_change_data::score Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 5/7] wt-status.c: catch unhandled diff status codes Nguyễn Thái Ngọc Duy
2017-12-26  9:10     ` [PATCH v2 6/7] wt-status.c: handle worktree renames Nguyễn Thái Ngọc Duy
2017-12-26 18:14       ` Igor Djordjevic
2017-12-27  1:06         ` Duy Nguyen
2017-12-28  0:50           ` Igor Djordjevic
2017-12-28  2:14             ` Igor Djordjevic
2017-12-26  9:10     ` [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format Nguyễn Thái Ngọc Duy
2017-12-26 22:14       ` Igor Djordjevic
2017-12-27  0:49         ` Duy Nguyen
2017-12-27 23:53           ` Igor Djordjevic
2017-12-27 10:18     ` [PATCH v3 0/6] Renames in git-status "changed not staged" section Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 1/6] t2203: test status output with porcelain v2 format Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 2/6] Use DIFF_DETECT_RENAME for detect_rename assignments Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 3/6] wt-status.c: coding style fix Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 4/6] wt-status.c: catch unhandled diff status codes Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 5/6] wt-status.c: rename rename-related fields in wt_status_change_data Nguyễn Thái Ngọc Duy
2017-12-27 10:18       ` [PATCH v3 6/6] wt-status.c: handle worktree renames Nguyễn Thái Ngọc Duy
2017-12-28  0:59       ` [PATCH v3 0/6] Renames in git-status "changed not staged" section Igor Djordjevic
2018-01-02 21:22       ` Jeff Hostetler
2017-12-26 18:04   ` [PATCH] status: handle worktree renames Torsten Bögershausen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b3e90960-d743-3299-ba43-150849b591d2@gmail.com \
    --to=igor.d.djordjevic@gmail.com \
    --cc=alexmv@dropbox.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --subject='Re: [PATCH] status: handle worktree renames' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).