subject:"\[PATCH v5 00\/34\] Add directory rename detection to git"

Re: [PATCH v5 00/34] Add directory rename detection to git

2018-01-03 Thread Johannes Sixt

Am 03.01.2018 um 22:02 schrieb Elijah Newren:

On Wed, Jan 3, 2018 at 2:57 AM, Johannes Sixt wrote:

I tested the series on Windows recently. It requires the patch below.
I don't know whether this is indicating some portability issues of grep
(^ being used in the middle of a RE instead of at the very beginning) or
just a quirk in my setup.

Thanks for testing it out. What version of Windows were you running
on? With cygwin or without? I tested previously on cygwin (I think
on Windows Server 2012??) and got all the tests passing there,
eventually[1]. I'm not sure I can find access to any other Windows
systems, but I'd be happy to take a look if I can.

[1]
https://public-inbox.org/git/cabpp-bej6-mry0ocz1wwetrtg_iehkzodcuon_puukvywau...@mail.gmail.com/

I have an ancient MinGW setup, where I build "vanilla" Git (not exactly
vanilla, but also not with the many patches that Git for Windows carries).

The need to backslash escape a caret for a literal match when it
appears in the middle of the string makes sense. Thanks for sending
along the patch. Would you prefer I squashed it into the series
(still sitting in 'pu'), or keep your patch separate? I'm fine with
either, I'm just unsure the protocol here.

Please squash into the relevant commits so that the series is bisectable
if the need arises.

But it still does not pass the test suite because the system does not
like file names such as y/c~HEAD:

++ grep 'Refusing to lose dirty file at z/c' out
Refusing to lose dirty file at z/c
++ grep -q stuff x/b y/a y/c y/c~HEAD z/c
grep: y/c: Invalid request code
error: last command exited with $?=2
not ok 94 - 11d-check: Avoid losing not-uptodate with rename + D/F conflict

This is exceptionally odd. The actual line from the testsuite was
grep -q stuff */*

which suggests your shell is both doing the pathname expansion and
treating the resulting filename not as a string but as something to be
interpreted that happens to have some kind of special
characters/commands, and then choking on the result. Super weird. I
could probably work around this by just running
grep -q stuff z/c

I think I had the asterisks in there because I was thinking in terms
of directory rename detection potentially moving the file, but that's
probably just overkill. Does the test pass for you with that change?

I can test on Monday at the earliest. If it's that easy to fix my
failures, I'd appreciate to go this route. But otherwise, I can deal
with the situation, so we don't need to complicate things just to please
my exotic setup.

(If so, there are also two similar tests that I'd need to make similar
changes to.)

However, although that might fix this particular case, it suggests
some fragility of the tests and filenames for whatever system you
happen to be using. merge-recursive.c's unique_path has created
filenames with tilde's in them for many years, it may just be that I'm
the first to use the resulting file in combination with grep to ensure
the contents are as we expect. There may be other issues lurking
(even if not yet appearing in the testsuite) for your system when
dealing with merge conflicts.

I can't recall having seen issues around tildas in file names, either.
It may be a new situation. I'll investigate.

-- Hannes

Re: [PATCH v5 00/34] Add directory rename detection to git

2018-01-03 Thread Elijah Newren

On Wed, Jan 3, 2018 at 2:57 AM, Johannes Sixt  wrote:

> I tested the series on Windows recently. It requires the patch below.
> I don't know whether this is indicating some portability issues of grep
> (^ being used in the middle of a RE instead of at the very beginning) or
> just a quirk in my setup.

Thanks for testing it out.  What version of Windows were you running
on?  With cygwin or without?  I tested previously on cygwin (I think
on Windows Server 2012??) and got all the tests passing there,
eventually[1].  I'm not sure I can find access to any other Windows
systems, but I'd be happy to take a look if I can.

[1] 
https://public-inbox.org/git/cabpp-bej6-mry0ocz1wwetrtg_iehkzodcuon_puukvywau...@mail.gmail.com/

The need to backslash escape a caret for a literal match when it
appears in the middle of the string makes sense.  Thanks for sending
along the patch.  Would you prefer I squashed it into the series
(still sitting in 'pu'), or keep your patch separate?  I'm fine with
either, I'm just unsure the protocol here.

> But it still does not pass the test suite because the system does not
> like file names such as y/c~HEAD:
>
> ++ grep 'Refusing to lose dirty file at z/c' out
> Refusing to lose dirty file at z/c
> ++ grep -q stuff x/b y/a y/c y/c~HEAD z/c
> grep: y/c: Invalid request code
> error: last command exited with $?=2
> not ok 94 - 11d-check: Avoid losing not-uptodate with rename + D/F conflict

This is exceptionally odd.  The actual line from the testsuite was
  grep -q stuff */*

which suggests your shell is both doing the pathname expansion and
treating the resulting filename not as a string but as something to be
interpreted that happens to have some kind of special
characters/commands, and then choking on the result.  Super weird.  I
could probably work around this by just running
  grep -q stuff z/c

I think I had the asterisks in there because I was thinking in terms
of directory rename detection potentially moving the file, but that's
probably just overkill.  Does the test pass for you with that change?
(If so, there are also two similar tests that I'd need to make similar
changes to.)

However, although that might fix this particular case, it suggests
some fragility of the tests and filenames for whatever system you
happen to be using.  merge-recursive.c's unique_path has created
filenames with tilde's in them for many years, it may just be that I'm
the first to use the resulting file in combination with grep to ensure
the contents are as we expect.  There may be other issues lurking
(even if not yet appearing in the testsuite) for your system when
dealing with merge conflicts.

Thanks,
Elijah

>
>  8< 
> From: Johannes Sixt 
> Date: Fri, 22 Dec 2017 09:33:13 +0100
> Subject: [PATCH] fixup directory rename tests
>
> Signed-off-by: Johannes Sixt 
> ---
>  t/t6043-merge-rename-directories.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t6043-merge-rename-directories.sh 
> b/t/t6043-merge-rename-directories.sh
> index f0af66b8a9..b8cd428341 100755
> --- a/t/t6043-merge-rename-directories.sh
> +++ b/t/t6043-merge-rename-directories.sh
> @@ -2940,8 +2940,8 @@ test_expect_success '10b-check: Overwrite untracked 
> with dir rename + delete' '
> echo contents >y/e &&
>
> test_must_fail git merge -s recursive B^0 >out 2>err &&
> -   test_i18ngrep "CONFLICT (rename/delete).*Version B^0 of y/d 
> left in tree at y/d~B^0" out &&
> -   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
> writing to y/e~B^0 instead" out &&
> +   test_i18ngrep "CONFLICT (rename/delete).*Version B\^0 of y/d 
> left in tree at y/d~B\^0" out &&
> +   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
> writing to y/e~B\^0 instead" out &&
>
> test 3 -eq $(git ls-files -s | wc -l) &&
> test 2 -eq $(git ls-files -u | wc -l) &&
> @@ -3010,7 +3010,7 @@ test_expect_success '10c-check: Overwrite untracked 
> with dir rename/rename(1to2)
>
> test_must_fail git merge -s recursive B^0 >out 2>err &&
> test_i18ngrep "CONFLICT (rename/rename)" out &&
> -   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
> as y/c~B^0 instead" out &&
> +   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
> as y/c~B\^0 instead" out &&
>
> test 6 -eq $(git ls-files -s | wc -l) &&
> test 3 -eq $(git ls-files -u | wc -l) &&
> --
> 2.14.2.808.g3bc32f2729

Re: [PATCH v5 00/34] Add directory rename detection to git

2018-01-03 Thread Johannes Sixt

Am 03.01.2018 um 01:02 schrieb Elijah Newren:
> On Wed, Dec 27, 2017 at 8:13 PM, Elijah Newren  wrote:
>> This patchset introduces directory rename detection to merge-recursive.  See
>>https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
>> for the first series (including design considerations, etc.), and follow-up
>> series can be found at
>>https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/
>>https://public-inbox.org/git/20171121080059.32304-1-new...@gmail.com/
>>https://public-inbox.org/git/20171129014237.32570-1-new...@gmail.com/
>>
>> Changes since v4:
>>* Squashed Junio's GETTEXT_POISON fixes into the appropriate commits
> 
> As per Jonathan's request[1], shamelessly re-sending Stefan's request
> for further review.  :-)
> 
> Quoting Stefan:
> 
> "I have reviewed the first three patches (which could form an
> independent series)
> that it would warrant a Reviewed-By: Stefan Beller 
> 
> While I reviewed the earlier versions of the later patches, I would
> prefer if there is another reviewer for these as it seems like a bigger
> contribution at a core functionality.
> 
> I cc'd some people who were active in some form of rename detection
> work earlier; could you review this series, please?"
> 
> My note: Stefan also looked through the testcases pretty closely and
> even suggested additional tests, which would account for another 11
> patches or so, but extra eyes on any part of the series always
> welcome.

I tested the series on Windows recently. It requires the patch below.
I don't know whether this is indicating some portability issues of grep
(^ being used in the middle of a RE instead of at the very beginning) or
just a quirk in my setup.

But it still does not pass the test suite because the system does not
like file names such as y/c~HEAD:

++ grep 'Refusing to lose dirty file at z/c' out
Refusing to lose dirty file at z/c
++ grep -q stuff x/b y/a y/c y/c~HEAD z/c
grep: y/c: Invalid request code
error: last command exited with $?=2
not ok 94 - 11d-check: Avoid losing not-uptodate with rename + D/F conflict

I haven't debugged this any further, yet.

 8< 
From: Johannes Sixt 
Date: Fri, 22 Dec 2017 09:33:13 +0100
Subject: [PATCH] fixup directory rename tests

Signed-off-by: Johannes Sixt 
---
 t/t6043-merge-rename-directories.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index f0af66b8a9..b8cd428341 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2940,8 +2940,8 @@ test_expect_success '10b-check: Overwrite untracked with 
dir rename + delete' '
echo contents >y/e &&
 
test_must_fail git merge -s recursive B^0 >out 2>err &&
-   test_i18ngrep "CONFLICT (rename/delete).*Version B^0 of y/d 
left in tree at y/d~B^0" out &&
-   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
writing to y/e~B^0 instead" out &&
+   test_i18ngrep "CONFLICT (rename/delete).*Version B\^0 of y/d 
left in tree at y/d~B\^0" out &&
+   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
writing to y/e~B\^0 instead" out &&
 
test 3 -eq $(git ls-files -s | wc -l) &&
test 2 -eq $(git ls-files -u | wc -l) &&
@@ -3010,7 +3010,7 @@ test_expect_success '10c-check: Overwrite untracked with 
dir rename/rename(1to2)
 
test_must_fail git merge -s recursive B^0 >out 2>err &&
test_i18ngrep "CONFLICT (rename/rename)" out &&
-   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
as y/c~B^0 instead" out &&
+   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
as y/c~B\^0 instead" out &&
 
test 6 -eq $(git ls-files -s | wc -l) &&
test 3 -eq $(git ls-files -u | wc -l) &&
-- 
2.14.2.808.g3bc32f2729

Re: [PATCH v5 00/34] Add directory rename detection to git

2018-01-02 Thread Elijah Newren

On Wed, Dec 27, 2017 at 8:13 PM, Elijah Newren  wrote:
> This patchset introduces directory rename detection to merge-recursive.  See
>   https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
> for the first series (including design considerations, etc.), and follow-up
> series can be found at
>   https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/
>   https://public-inbox.org/git/20171121080059.32304-1-new...@gmail.com/
>   https://public-inbox.org/git/20171129014237.32570-1-new...@gmail.com/
>
> Changes since v4:
>   * Squashed Junio's GETTEXT_POISON fixes into the appropriate commits

As per Jonathan's request[1], shamelessly re-sending Stefan's request
for further review.  :-)

Quoting Stefan:

"I have reviewed the first three patches (which could form an
independent series)
that it would warrant a Reviewed-By: Stefan Beller 

While I reviewed the earlier versions of the later patches, I would
prefer if there is another reviewer for these as it seems like a bigger
contribution at a core functionality.

I cc'd some people who were active in some form of rename detection
work earlier; could you review this series, please?"

My note: Stefan also looked through the testcases pretty closely and
even suggested additional tests, which would account for another 11
patches or so, but extra eyes on any part of the series always
welcome.

Thanks!
Elijah

[1] 
https://public-inbox.org/git/20180102221916.ge131...@aiede.mtv.corp.google.com/

[PATCH v5 00/34] Add directory rename detection to git

2017-12-27 Thread Elijah Newren

This patchset introduces directory rename detection to merge-recursive.  See
  https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
for the first series (including design considerations, etc.), and follow-up
series can be found at
  https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/
  https://public-inbox.org/git/20171121080059.32304-1-new...@gmail.com/
  https://public-inbox.org/git/20171129014237.32570-1-new...@gmail.com/

Changes since v4:
  * Squashed Junio's GETTEXT_POISON fixes into the appropriate commits

Elijah Newren (34):
  Tighten and correct a few testcases for merging and cherry-picking
  merge-recursive: fix logic ordering issue
  merge-recursive: add explanation for src_entry and dst_entry
  directory rename detection: basic testcases
  directory rename detection: directory splitting testcases
  directory rename detection: testcases to avoid taking detection too
far
  directory rename detection: partially renamed directory
testcase/discussion
  directory rename detection: files/directories in the way of some
renames
  directory rename detection: testcases checking which side did the
rename
  directory rename detection: more involved edge/corner testcases
  directory rename detection: testcases exploring possibly suboptimal
merges
  directory rename detection: miscellaneous testcases to complete
coverage
  directory rename detection: tests for handling overwriting untracked
files
  directory rename detection: tests for handling overwriting dirty files
  merge-recursive: move the get_renames() function
  merge-recursive: introduce new functions to handle rename logic
  merge-recursive: fix leaks of allocated renames and diff_filepairs
  merge-recursive: make !o->detect_rename codepath more obvious
  merge-recursive: split out code for determining diff_filepairs
  merge-recursive: add a new hashmap for storing directory renames
  merge-recursive: make a helper function for cleanup for handle_renames
  merge-recursive: add get_directory_renames()
  merge-recursive: check for directory level conflicts
  merge-recursive: add a new hashmap for storing file collisions
  merge-recursive: add computation of collisions due to dir rename &
merging
  merge-recursive: check for file level conflicts then get new name
  merge-recursive: when comparing files, don't include trees
  merge-recursive: apply necessary modifications for directory renames
  merge-recursive: avoid clobbering untracked files with directory
renames
  merge-recursive: fix overwriting dirty files involved in renames
  merge-recursive: fix remaining directory rename + dirty overwrite
cases
  directory rename detection: new testcases showcasing a pair of bugs
  merge-recursive: avoid spurious rename/rename conflict from dir
renames
  merge-recursive: ensure we write updates for directory-renamed file

 merge-recursive.c   | 1231 ++-
 merge-recursive.h   |   17 +
 strbuf.c|   16 +
 strbuf.h|   16 +
 t/t3501-revert-cherry-pick.sh   |5 +-
 t/t6043-merge-rename-directories.sh | 3823 +++
 t/t7607-merge-overwrite.sh  |7 +-
 unpack-trees.c  |4 +-
 unpack-trees.h  |4 +
 9 files changed, 5007 insertions(+), 116 deletions(-)
 create mode 100755 t/t6043-merge-rename-directories.sh

-- 
2.15.0.408.g8e199d483

Re: [PATCH v5 00/34] Add directory rename detection to git

Re: [PATCH v5 00/34] Add directory rename detection to git

Re: [PATCH v5 00/34] Add directory rename detection to git

Re: [PATCH v5 00/34] Add directory rename detection to git

[PATCH v5 00/34] Add directory rename detection to git

5 matches

Site Navigation

Mail list logo

Footer information