git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Elijah Newren <newren@gmail.com>
Cc: gitster@pobox.com, git@vger.kernel.org, Jeff King <peff@peff.net>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH v4] log,diff-tree: add --combined-all-names option
Date: Tue, 5 Feb 2019 10:48:42 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1902051045460.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190204200754.16413-1-newren@gmail.com>

Hi Elijah,

On Mon, 4 Feb 2019, Elijah Newren wrote:

> The combined diff format for merges will only list one filename, even if
> rename or copy detection is active.  For example, with raw format one
> might see:
> 
>   ::100644 100644 100644 fabadb8 cc95eb0 4866510 MM	describe.c
>   ::100755 100755 100755 52b7a2d 6d1ac04 d2ac7d7 RM	bar.sh
>   ::100644 100644 100644 e07d6c5 9042e82 ee91881 RR	phooey.c
> 
> This doesn't let us know what the original name of bar.sh was in the
> first parent, and doesn't let us know what either of the original names
> of phooey.c were in either of the parents.  In contrast, for non-merge
> commits, raw format does provide original filenames (and a rename score
> to boot).  In order to also provide original filenames for merge
> commits, add a --combined-all-names option (which must be used with
> either -c or --cc, and is likely only useful with rename or copy
> detection active) so that we can print tab-separated filenames when
> renames are involved.  This transforms the above output to:
> 
>   ::100644 100644 100644 fabadb8 cc95eb0 4866510 MM	desc.c	desc.c	desc.c
>   ::100755 100755 100755 52b7a2d 6d1ac04 d2ac7d7 RM	foo.sh	bar.sh	bar.sh
>   ::100644 100644 100644 e07d6c5 9042e82 ee91881 RR	fooey.c	fuey.c	phooey.c
> 
> Further, in patch format, this changes the from/to headers so that
> instead of just having one "from" header, we get one for each parent.
> For example, instead of having
> 
>   --- a/phooey.c
>   +++ b/phooey.c
> 
> we would see
> 
>   --- a/fooey.c
>   --- a/fuey.c
>   +++ b/phooey.c
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>

I do not really see where this needs to have tests with filenames
containing tabs, but your test does create such files. Which makes it
break on filesystems that do not allow tabs in file names, such as
NTFS. I need this to make the test pass again:

-- snip --
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 5bccc323f648..596705985baa 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -435,7 +435,7 @@ test_expect_success 'combine diff gets tree sorting right' '
 	test_cmp expect actual
 '
 
-test_expect_success 'setup for --combined-with-paths' '
+test_expect_success FUNNYNAMES 'setup for --combined-with-paths' '
 	git branch side1c &&
 	git branch side2c &&
 	git checkout side1c &&
@@ -456,7 +456,7 @@ test_expect_success 'setup for --combined-with-paths' '
 	git commit
 '
 
-test_expect_success '--combined-all-names and --raw' '
+test_expect_success FUNNYNAMES '--combined-all-names and --raw' '
 	cat <<-\EOF >expect &&
 	::100644 100644 100644 f00c965d8307308469e537302baa73048488f162 088bd5d92c2a8e0203ca8e7e4c2a5c692f6ae3f7 333b9c62519f285e1854830ade0fe1ef1d40ee1b RR	"file\twith\ttabs"	"i\tam\ttabbed"	"fickle\tnaming"
 	EOF
@@ -465,13 +465,13 @@ test_expect_success '--combined-all-names and --raw' '
 	test_cmp expect actual
 '
 
-test_expect_success '--combined-all-names and --raw -and -z' '
+test_expect_success FUNNYNAMES '--combined-all-names and --raw -and -z' '
 	printf "0f9645804ebb04cc3eef91f799eb7fb54d70cefb\0::100644 100644 100644 f00c965d8307308469e537302baa73048488f162 088bd5d92c2a8e0203ca8e7e4c2a5c692f6ae3f7 333b9c62519f285e1854830ade0fe1ef1d40ee1b RR\0file\twith\ttabs\0i\tam\ttabbed\0fickle\tnaming\0" >expect &&
 	git diff-tree -c -M --raw --combined-all-names -z HEAD >actual &&
 	test_cmp -a expect actual
 '
 
-test_expect_success '--combined-all-names and --cc' '
+test_expect_success FUNNYNAMES '--combined-all-names and --cc' '
 	cat <<-\EOF >expect &&
 	--- "a/file\twith\ttabs"
 	--- "a/i\tam\ttabbed"
-- snap --

But maybe you want to get rid of the funny file names? Or test for them in
a separate, dedicated test case so that we do not have to guard *all* of
your added tests behind FUNNYNAMES?

Ciao,
Dscho

  parent reply	other threads:[~2019-02-05  9:49 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-24 16:46 [PATCH] log,diff-tree: add --combined-with-paths options for merges with renames Elijah Newren
2019-01-25  2:19 ` brian m. carlson
2019-01-25 16:27   ` Elijah Newren
2019-01-25 14:45 ` Derrick Stolee
2019-01-25 16:30   ` Elijah Newren
2019-01-25 16:54 ` [PATCH v2] " Elijah Newren
2019-01-25 17:40   ` Derrick Stolee
2019-01-25 17:52     ` Elijah Newren
2019-01-25 19:51       ` Eric Sunshine
2019-01-26 22:07         ` Elijah Newren
2019-01-27  1:52       ` brian m. carlson
2019-01-26 22:18   ` [PATCH v3] log,diff-tree: add --combined-all-names option Elijah Newren
2019-02-04 20:07     ` [PATCH v4] " Elijah Newren
2019-02-04 21:20       ` Junio C Hamano
2019-02-05 15:51         ` Elijah Newren
2019-02-05 20:39           ` Junio C Hamano
2019-02-07 19:50             ` Elijah Newren
2019-02-07 20:25               ` Junio C Hamano
2019-02-07 22:10                 ` Elijah Newren
2019-02-07 23:31                   ` Junio C Hamano
2019-02-07 23:48                     ` Elijah Newren
2019-02-05  9:48       ` Johannes Schindelin [this message]
2019-02-05 15:54         ` Elijah Newren
2019-02-05 18:04           ` Junio C Hamano
2019-02-07 22:28       ` Junio C Hamano
2019-02-07 23:48         ` Elijah Newren
2019-02-08  1:12       ` [PATCH v5 0/2] add --combined-all-paths option to log and diff-tree Elijah Newren
2019-02-08  1:12         ` [PATCH v5 1/2] log,diff-tree: add --combined-all-paths option Elijah Newren
2019-02-08  4:00           ` Junio C Hamano
2019-02-08  6:52             ` Elijah Newren
2019-02-08 17:50               ` Junio C Hamano
2019-02-08  1:12         ` [PATCH v5 2/2] squash! " Elijah Newren
2019-02-08  4:14           ` Junio C Hamano
2019-02-08  6:48             ` Elijah Newren
2019-01-25 19:29 ` [PATCH] log,diff-tree: add --combined-with-paths options for merges with renames Junio C Hamano
2019-01-25 20:04   ` Elijah Newren
2019-01-25 22:21     ` Junio C Hamano
2019-01-26 22:12       ` Elijah Newren
2019-01-28  0:19         ` Junio C Hamano

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=nycvar.QRO.7.76.6.1902051045460.41@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    --cc=stolee@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).