Re: [PATCH] fast-import: add options to enable/disable case folding
On 18/04/15 08:36, Mike Hommey wrote: On Fri, Apr 17, 2015 at 11:44:00AM -0700, Junio C Hamano wrote: So perhaps we should rip the case folding out altogether instead? The entry for the change in the Release Notes may say: * git fast-import incorrectly case-folded the paths recorded in the history when core.ignorease is set (i.e. the repository's working tree is incapable of expressing paths that differ only in their cases); this old bug was reported in 2012 and was finally corrected. or something like that? Is anything else then git-p4 known to rely on case folding? If not, I guess that's a reasonable plan. We could even add an option to fast-import that would allow to turn case folding back on, and make git-p4 use it, so that its expectations are fulfilled. Although at some point, it could (should?) do case folding itself(?) git-p4 has a single line of code that checks if core.ignorecase is turned on, and uses this to decide whether to skip files that are outside the depot being tracked and I *think* is not really related to fast-import. I don't know to what extent though git-p4 relies on the current behaviour of git fast-import to fold case for it. There's a 'p4 info' command which tells you what the server thinks it's doing: $ p4 info | grep Case Case Handling: sensitive I don't know how long that support has been present (it might not work on older servers that some people are still using). It's also possible to force the server to be case-insensitive on the Linux version. That's useful, as it we could construct some test cases to see what we're likely to break without having to force people to install a case-insensitive OS in order to run the git regression tests. Luke Mike -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fast-import: add options to enable/disable case folding
On Fri, Apr 17, 2015 at 11:44:00AM -0700, Junio C Hamano wrote: So perhaps we should rip the case folding out altogether instead? The entry for the change in the Release Notes may say: * git fast-import incorrectly case-folded the paths recorded in the history when core.ignorease is set (i.e. the repository's working tree is incapable of expressing paths that differ only in their cases); this old bug was reported in 2012 and was finally corrected. or something like that? Is anything else then git-p4 known to rely on case folding? If not, I guess that's a reasonable plan. We could even add an option to fast-import that would allow to turn case folding back on, and make git-p4 use it, so that its expectations are fulfilled. Although at some point, it could (should?) do case folding itself(?) Mike -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fast-import: add options to enable/disable case folding
On 04/17/2015 01:52 PM, Mike Hommey wrote:
Currently, fast-import does case folding depending on `core.ignorecase`.
`core.ignorecase` depends on the file system where the working tree is.
However, different kind of imports require different kinds of semantics,
and they usually aren't tied with the file system, but with the data being
imported.
Good that you take up this issue, thanks for the patch
More comments inline.
Add command line options to enable or disable case folding. Also expose
them as features in the fast-import stream. Features instead of options,
because a stream that needs case folding enabled or disabled won't work
as expected if fast-import doesn't support the case folding options.
---
Documentation/git-fast-import.txt | 11 ++
fast-import.c | 19 --
t/t9300-fast-import.sh| 79
+++
3 files changed, 106 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-fast-import.txt
b/Documentation/git-fast-import.txt
index 690fed3..22eba87 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -50,6 +50,13 @@ OPTIONS
memory used by fast-import during this run. Showing this output
is currently the default, but can be disabled with \--quiet.
+--[no-]fold-case::
+ When files/directories with the same name but a different case
+ are detected, they are treated as the same (--fold-case) or as
+ being different (--no-fold-case). The default is --fold-case
+ when `core.ignorecase` is set to `true`, and --no-fold-case when
+ it is `false`.
+
Most often the we use the term ignore-case, could that be a better name ?
Other opinions, pros/cons ?
Options for Frontends
~
@@ -1027,6 +1034,8 @@ date-format::
export-marks::
relative-marks::
no-relative-marks::
+fold-case::
+no-fold-case::
force::
Act as though the corresponding command-line option with
a leading '--' was passed on the command line
@@ -1091,6 +1100,8 @@ not be passed as option:
* import-marks
* export-marks
* cat-blob-fd
+* fold-case
+* no-fold-case
* force
`done`
diff --git a/fast-import.c b/fast-import.c
index 6378726..958f3da 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -371,10 +371,18 @@ static volatile sig_atomic_t checkpoint_requested;
/* Where to write output of cat-blob commands */
static int cat_blob_fd = STDOUT_FILENO;
+/* Whether to enable case folding */
+static int fold_case;
+
static void parse_argv(void);
static void parse_cat_blob(const char *p);
static void parse_ls(const char *p, struct branch *b);
+static int strncmp_foldcase(const char *a, const char *b, size_t count)
+{
+ return fold_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
+}
+
static void write_branch_report(FILE *rpt, struct branch *b)
{
fprintf(rpt, %s:\n, b-name);
@@ -1507,7 +1515,7 @@ static int tree_content_set(
t = root-tree;
for (i = 0; i t-entry_count; i++) {
e = t-entries[i];
- if (e-name-str_len == n !strncmp_icase(p,
e-name-str_dat, n)) {
+ if (e-name-str_len == n !strncmp_foldcase(p,
e-name-str_dat, n)) {
if (!*slash1) {
if (!S_ISDIR(mode)
e-versions[1].mode == mode
@@ -1597,7 +1605,7 @@ static int tree_content_remove(
t = root-tree;
for (i = 0; i t-entry_count; i++) {
e = t-entries[i];
- if (e-name-str_len == n !strncmp_icase(p,
e-name-str_dat, n)) {
+ if (e-name-str_len == n !strncmp_foldcase(p,
e-name-str_dat, n)) {
if (*slash1 !S_ISDIR(e-versions[1].mode))
/*
* If p names a file in some subdirectory, and a
@@ -1664,7 +1672,7 @@ static int tree_content_get(
t = root-tree;
for (i = 0; i t-entry_count; i++) {
e = t-entries[i];
- if (e-name-str_len == n !strncmp_icase(p,
e-name-str_dat, n)) {
+ if (e-name-str_len == n !strncmp_foldcase(p,
e-name-str_dat, n)) {
if (!*slash1)
goto found_entry;
if (!S_ISDIR(e-versions[1].mode))
@@ -3246,6 +3254,10 @@ static int parse_one_feature(const char *feature, int
from_stream)
relative_marks_paths = 1;
} else if (!strcmp(feature, no-relative-marks)) {
relative_marks_paths = 0;
+ } else if (!strcmp(feature, fold-case)) {
+ fold_case = 1;
+ } else if (!strcmp(feature, no-fold-case)) {
+ fold_case = 0;
} else if (!strcmp(feature, done)) {
require_explicit_termination = 1;
} else if (!strcmp(feature, force)) {
@@ -3372,6 +3384,7 @@ int main(int argc, char
Re: [PATCH] fast-import: add options to enable/disable case folding
Torsten Bögershausen tbo...@web.de writes: +--[no-]fold-case:: +When files/directories with the same name but a different case +are detected, they are treated as the same (--fold-case) or as +being different (--no-fold-case). The default is --fold-case +when `core.ignorecase` is set to `true`, and --no-fold-case when +it is `false`. + Most often the we use the term ignore-case, could that be a better name ? Other opinions, pros/cons ? Yeah, --[no-]ignore-case sounds more in line with how other commands' options are spelled. But I somehow thought this case-folding was deliberately done as an improvement against the original that did not have a way to do the ignore-case? http://thread.gmane.org/gmane.comp.version-control.git/200597/focus=200625 I am not sure why not until now I did not find the original justification dubious, but I think fast-export should never do case folding---Joshua talks about working trees on a file system that is incapable of expressing different cases, but export is about reading in-repository histories, whose trees are fully capable of expressing paths in different cases just fine, and spitting out a file that can be processed by fast-import. I do not see why it should collapse two different paths that differ in case at export time. If the original history is broken by Perforce or whatever and recording the history of the same path in different case combinations in different commits, perhaps the right thing to do is to fix the original history in Git repository before exporting in the first place. I do not see how such a corruption is related to the characteristics of the filesystem where export is run. Perhaps a case-insensitive filesystem may helped Perforce to corrupt the history when initial import of the history into Git was done, but core.ignorecase of the current repository does not help us decide if that was actually the case---the import may have been done on a completely different machine. So perhaps we should rip the case folding out altogether instead? The entry for the change in the Release Notes may say: * git fast-import incorrectly case-folded the paths recorded in the history when core.ignorease is set (i.e. the repository's working tree is incapable of expressing paths that differ only in their cases); this old bug was reported in 2012 and was finally corrected. or something like that? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fast-import: add options to enable/disable case folding
On Fri, Apr 17, 2015 at 06:56:43PM +0200, Torsten Bögershausen wrote:
On 04/17/2015 01:52 PM, Mike Hommey wrote:
+test_expect_success 'V: default case folding with ignorecase=true' '
+ git config core.ignorecase true
+ git fast-import input
+ git ls-tree refs/heads/V actual
+ git update-ref -d refs/heads/V
+ cat expected \EOF
+100644 blob 78981922613b2afb6025042ff6bd878ac1994e85 a
+EOF
+ test_cmp expected actual'
+
+test_expect_success 'V: default case folding with ignorecase=false' '
+ git config core.ignorecase false
+ git fast-import input
+ git ls-tree refs/heads/V actual
+ git update-ref -d refs/heads/V
+ cat expected \EOF
+100644 blob 78981922613b2afb6025042ff6bd878ac1994e85 A
+EOF
+ test_cmp expected actual'
+
+test_expect_success 'V: forced case folding with ignorecase=true' '
+ git config core.ignorecase true
+ git fast-import --fold-case input
+ git ls-tree refs/heads/V actual
+ git update-ref -d refs/heads/V
+ cat expected \EOF
+100644 blob 78981922613b2afb6025042ff6bd878ac1994e85 a
+EOF
+ test_cmp expected actual'
If you want to make it shorter (and try to avoid repetition):
The forced true cases could be collected in a loop.
(and the same for forced=false)
I was also going to suggest squashing the repetition. Here's what I
had in mind:
--- 8 ---
test_foldcase() {
ignore=$1
case $2 in
true) fold=--fold-case folded=true ;;
false) fold=--no-fold-case folded=false ;;
*) fold= folded=$ignore ;;
esac
case $folded in true) folded=a ;; false) folded=A ;; esac
test_expect_success V: case folding: ignorecase=$ignore${fold:+
$fold}
git -c core.ignorecase=$ignore fast-import $fold input
git ls-tree refs/heads/V actual
git update-ref -d refs/heads/V
cat expect -EOF
100644 blob 78981922613b2afb6025042ff6bd878ac1994e85$folded
EOF
test_cmp expect actual
}
for o in '' true false
do
for c in true false
do
test_foldcase $c $o
done
done
--- 8 ---
which outputs:
--- 8 ---
ok 176 - V: case folding: ignorecase=true
ok 177 - V: case folding: ignorecase=false
ok 178 - V: case folding: ignorecase=true --fold-case
ok 179 - V: case folding: ignorecase=false --fold-case
ok 180 - V: case folding: ignorecase=true --no-fold-case
ok 181 - V: case folding: ignorecase=false --no-fold-case
--- 8 ---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
