Re: [PATCH v2] merge-tree: don't print entries that match "local"

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Apr 27, 2013, 11:35 UTC
Message-ID: <517BB81D.10909@lsrfire.ath.cx>
In-Reply-To: <20130407210751.GM2222@serenity.lan>

Am 07.04.2013 23:07, schrieb John Keeping:

Show 43 quoted lines

> The documentation says:
>
> 	the output from the command omits entries that match the
> 	<branch1> tree.
>
> But currently "added in branch1" and "removed in branch1" (both while
> unchanged in branch2) do print output.  Change this so that the
> behaviour matches the documentation.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>   builtin/merge-tree.c  | 26 +++++++++++++-------------
>   t/t4300-merge-tree.sh | 10 ----------
>   2 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index bc912e3..ed25d81 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -155,6 +155,11 @@ static int same_entry(struct name_entry *a, struct name_entry *b)
>   		a->mode == b->mode;
>   }
>
> +static int both_empty(struct name_entry *a, struct name_entry *b)
> +{
> +	return !(a->sha1 || b->sha1);
> +}
> +
>   static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsigned char *sha1, const char *path)
>   {
>   	struct merge_list *res = xcalloc(1, sizeof(*res));
> @@ -297,13 +302,10 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
>   static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *info)
>   {
>   	/* Same in both? */
> -	if (same_entry(entry+1, entry+2)) {
> -		if (entry[0].sha1) {
> -			/* Modified identically */
> -			resolve(info, NULL, entry+1);
> -			return mask;
> -		}
> -		/* "Both added the same" is left unresolved */
> +	if (same_entry(entry+1, entry+2) || both_empty(entry+0, entry+2)) {

Shouldn't this zero be a one instead?

Show 17 quoted lines

> +		/* Modified, added or removed identically */
> +		resolve(info, NULL, entry+1);
> +		return mask;
>   	}
>
>   	if (same_entry(entry+0, entry+1)) {
> @@ -319,12 +321,10 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
>   		 */
>   	}
>
> -	if (same_entry(entry+0, entry+2)) {
> -		if (entry[1].sha1 && !S_ISDIR(entry[1].mode)) {
> -			/* We modified, they did not touch -- take ours */
> -			resolve(info, NULL, entry+1);
> -			return mask;
> -		}
> +	if (same_entry(entry+0, entry+2) || both_empty(entry+0, entry+2)) {

Otherwise the both_empty check here can be removed because we'd have already returned above if it was true.

But do we actually want to resolve the removal of a file on both sides silently? The added comment above says so, but the commit message doesn't mention it.

Show 49 quoted lines

> +		/* We added, modified or removed, they did not touch -- take ours */
> +		resolve(info, NULL, entry+1);
> +		return mask;
>   	}
>
>   	unresolved(info, entry);
> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> index d0b2a45..bd43b3d 100755
> --- a/t/t4300-merge-tree.sh
> +++ b/t/t4300-merge-tree.sh
> @@ -26,8 +26,6 @@ EXPECTED
>
>   test_expect_success 'file add !A, B' '
>   	cat >expected <<\EXPECTED &&
> -added in local
> -  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
>   EXPECTED
>
>   	git reset --hard initial &&
> @@ -38,9 +36,6 @@ EXPECTED
>
>   test_expect_success 'file add A, B (same)' '
>   	cat >expected <<\EXPECTED &&
> -added in both
> -  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> -  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
>   EXPECTED
>
>   	git reset --hard initial &&
> @@ -181,9 +176,6 @@ AAA" &&
>
>   test_expect_success 'file remove A, !B' '
>   	cat >expected <<\EXPECTED &&
> -removed in local
> -  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> -  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
>   EXPECTED
>
>   	git reset --hard initial &&
> @@ -283,8 +275,6 @@ test_expect_success 'turn tree to file' '
>   	test_commit "make-file" "dir" "CCC" &&
>   	git merge-tree add-tree add-another-tree make-file >actual &&
>   	cat >expect <<-\EOF &&
> -	added in local
> -	  our    100644 ba629238ca89489f2b350e196ca445e09d8bb834 dir/another
>   	removed in remote
>   	  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
>   	  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
>