Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems
David Turner writes:
>> Hmm, I would find it easier to read if it were:
>>
>> ... if (lstat(dst, &st) == 0 &&
>> !(ignore_case && !strcasecmp(src, dst))) {
>>
>> That is, "it is an error for dst to exist, unless we are on a case
>> insensitive filesystem and src and dst refer to the same file.", but
>> maybe it is just me.
>
> I personally dislike the double negative. I also considered breaking it
> out into a little function with a self-documenting name -- does that
> sound better?
No, it shows that it is just me. I did not say that the original is
unreadable.
>> More importantly, what is the end-user visible effect of this
>> change? Is it fair to summarize it like this?
>>
>> On a case-insensitive filesystem, "mv hello.txt Hello.txt"
>> always trigger the "dst already exists" error, because both
>> names refer to the same file to MS-DOS, requiring the user to
> ^^
> (I have not actually tested on Windows; I tested on the Mac since that's
> what I have handy)
>
>> pass the "--force" option. Allow it without "--force".
>
> Yes.
>
>> Overwriting an existing file with "mv hello.txt Hello.txt" on a case
>> sensitive filesystem *is* an unusual operation, and that is the
>> reason why we require "--force" to make sure that the user means it.
>> I have a slight suspicion that the same "mv hello.txt Hello.txt" on
>> a case insensitive filesystem, where two names are known (to the end
>> user of such a filesystem) to refer to the same path would equally
>> be a very unusual thing to do, and such an operation may deserve a
>> similar safety precaution to make sure that the user really meant to
>> do so by requiring "--force".
>>
>> So, I dunno.
>
> The argument against --force is that git's behavior should not
> significantly differ between sensitive and insensitive filesystems
> (where possible). I do not see a case-changing rename as unusual on a
> case-insensitive filesystem; these filesystems typically preserve case,
> and a user might reasonably care about the case of a filename either for
> aesthetic reasons or for functionality on sensible filesystems (e.g.
> developers who work on Macs but deploy on GNU/Linux, as is quite
> common).
>
> The Mac's interface itself provides conflicting evidence: on one hand,
> we might expect git mv to work like plain mv: nothing special is needed
> to do a case-changing mv). On the other hand, in the Finder, attempting
> a case-changing rename causes an error message (which there is no way to
> get around other than the two-rename dance). I read this as "ordinary
> users never intentionally change the case of files, but developers
> sometimes do", but that's not the only possible reading.
>
> I myself am not actually a Mac user; I simply support a bunch of Mac
> users (which is where the merge bug came from). So I don't know what
> Mac users would prefer. Maybe there are some on the git mailing list?
>
> I also have not tried on Windows. I put in an email to the one
> Windows-using friend I can think of to ask her to give Windows Explorer
> (or whatever it's called these days) a try. My guess (based on a quick
> Google search) would be is that it works without error, but I will send
> an update if I hear otherwise.
Alright. Thanks for sanity checking.
I've already queued your patch as-is when I was composing the
message you responded to and today's integration cycle is already
going, so unless other people have ideas to convince us both that
this is a bad idea, all is well without any further action.
Thanks.
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
> I also have not tried on Windows. I put in an email to the one > Windows-using friend I can think of to ask her to give Windows Explorer > (or whatever it's called these days) a try. My guess (based on a quick > Google search) would be is that it works without error, but I will send > an update if I hear otherwise. A case-only changing rename in the windows explorer in Win7 works perfectly without any problems. -- 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 2/2] ignorecase: Fix git mv on insensitive filesystems
On Thu, 2014-05-08 at 12:54 -0700, Junio C Hamano wrote:
> dtur...@twopensource.com writes:
>
> > From: David Turner
> >
> > Make it possible to change the case of a filename on a
> > case-insensitive filesystem using git mv. Change git mv to allow
> > moves where the destination file exists if the destination file has
> > the same name as the source file ignoring case.
> >
> > Signed-off-by: David Turner
> > ---
> > builtin/mv.c| 3 ++-
> > t/t6039-merge-ignorecase.sh | 2 +-
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/mv.c b/builtin/mv.c
> > index 45e57f3..f4d89d0 100644
> > --- a/builtin/mv.c
> > +++ b/builtin/mv.c
> > @@ -202,7 +202,8 @@ int cmd_mv(int argc, const char **argv, const char
> > *prefix)
> > }
> > } else if (cache_name_pos(src, length) < 0)
> > bad = _("not under version control");
> > - else if (lstat(dst, &st) == 0) {
> > + else if (lstat(dst, &st) == 0 &&
> > +(!ignore_case || strcasecmp(src, dst))) {
>
> Hmm, I would find it easier to read if it were:
>
> ... if (lstat(dst, &st) == 0 &&
> !(ignore_case && !strcasecmp(src, dst))) {
>
> That is, "it is an error for dst to exist, unless we are on a case
> insensitive filesystem and src and dst refer to the same file.", but
> maybe it is just me.
I personally dislike the double negative. I also considered breaking it
out into a little function with a self-documenting name -- does that
sound better?
> More importantly, what is the end-user visible effect of this
> change? Is it fair to summarize it like this?
>
> On a case-insensitive filesystem, "mv hello.txt Hello.txt"
> always trigger the "dst already exists" error, because both
> names refer to the same file to MS-DOS, requiring the user to
^^
(I have not actually tested on Windows; I tested on the Mac since that's
what I have handy)
> pass the "--force" option. Allow it without "--force".
Yes.
> Overwriting an existing file with "mv hello.txt Hello.txt" on a case
> sensitive filesystem *is* an unusual operation, and that is the
> reason why we require "--force" to make sure that the user means it.
> I have a slight suspicion that the same "mv hello.txt Hello.txt" on
> a case insensitive filesystem, where two names are known (to the end
> user of such a filesystem) to refer to the same path would equally
> be a very unusual thing to do, and such an operation may deserve a
> similar safety precaution to make sure that the user really meant to
> do so by requiring "--force".
>
> So, I dunno.
The argument against --force is that git's behavior should not
significantly differ between sensitive and insensitive filesystems
(where possible). I do not see a case-changing rename as unusual on a
case-insensitive filesystem; these filesystems typically preserve case,
and a user might reasonably care about the case of a filename either for
aesthetic reasons or for functionality on sensible filesystems (e.g.
developers who work on Macs but deploy on GNU/Linux, as is quite
common).
The Mac's interface itself provides conflicting evidence: on one hand,
we might expect git mv to work like plain mv: nothing special is needed
to do a case-changing mv). On the other hand, in the Finder, attempting
a case-changing rename causes an error message (which there is no way to
get around other than the two-rename dance). I read this as "ordinary
users never intentionally change the case of files, but developers
sometimes do", but that's not the only possible reading.
I myself am not actually a Mac user; I simply support a bunch of Mac
users (which is where the merge bug came from). So I don't know what
Mac users would prefer. Maybe there are some on the git mailing list?
I also have not tried on Windows. I put in an email to the one
Windows-using friend I can think of to ask her to give Windows Explorer
(or whatever it's called these days) a try. My guess (based on a quick
Google search) would be is that it works without error, but I will send
an update if I hear otherwise.
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
dtur...@twopensource.com writes:
> From: David Turner
>
> Make it possible to change the case of a filename on a
> case-insensitive filesystem using git mv. Change git mv to allow
> moves where the destination file exists if the destination file has
> the same name as the source file ignoring case.
>
> Signed-off-by: David Turner
> ---
> builtin/mv.c| 3 ++-
> t/t6039-merge-ignorecase.sh | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 45e57f3..f4d89d0 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -202,7 +202,8 @@ int cmd_mv(int argc, const char **argv, const char
> *prefix)
> }
> } else if (cache_name_pos(src, length) < 0)
> bad = _("not under version control");
> - else if (lstat(dst, &st) == 0) {
> + else if (lstat(dst, &st) == 0 &&
> + (!ignore_case || strcasecmp(src, dst))) {
Hmm, I would find it easier to read if it were:
... if (lstat(dst, &st) == 0 &&
!(ignore_case && !strcasecmp(src, dst))) {
That is, "it is an error for dst to exist, unless we are on a case
insensitive filesystem and src and dst refer to the same file.", but
maybe it is just me.
More importantly, what is the end-user visible effect of this
change? Is it fair to summarize it like this?
On a case-insensitive filesystem, "mv hello.txt Hello.txt"
always trigger the "dst already exists" error, because both
names refer to the same file to MS-DOS, requiring the user to
pass the "--force" option. Allow it without "--force".
Overwriting an existing file with "mv hello.txt Hello.txt" on a case
sensitive filesystem *is* an unusual operation, and that is the
reason why we require "--force" to make sure that the user means it.
I have a slight suspicion that the same "mv hello.txt Hello.txt" on
a case insensitive filesystem, where two names are known (to the end
user of such a filesystem) to refer to the same path would equally
be a very unusual thing to do, and such an operation may deserve a
similar safety precaution to make sure that the user really meant to
do so by requiring "--force".
So, I dunno.
> bad = _("destination exists");
> if (force) {
> /*
> diff --git a/t/t6039-merge-ignorecase.sh b/t/t6039-merge-ignorecase.sh
> index dfc9f17..a977653 100755
> --- a/t/t6039-merge-ignorecase.sh
> +++ b/t/t6039-merge-ignorecase.sh
> @@ -35,7 +35,7 @@ test_expect_success 'merge with case-changing rename on
> both sides' '
> git reset --hard baseline &&
> git branch -D with-camel &&
> git checkout -b with-camel &&
> - git mv --force TestCase testcase &&
> + git mv TestCase testcase &&
> git commit -m "recase on branch" &&
> >foo &&
> git add foo &&
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
On 05/08/2014 08:37 AM, Johannes Sixt wrote:
Am 5/7/2014 19:46, schrieb Junio C Hamano:
David Turner writes:
On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
} else if (cache_name_pos(src, length) < 0)
bad = _("not under version control");
- else if (lstat(dst, &st) == 0) {
+ else if (lstat(dst, &dst_st) == 0 &&
+(src_st.st_ino != dst_st.st_ino ||
+ (src_st.st_ino == 0 && strcasecmp(src, dst {
Don't do that. st_ino is zero on Windows only because we do not spend time
to fill in the field. Don't use it as an indicator for a case-insensitive
file system; zero may be a valid inode number on other systems.
I don't think it is a problem if zero is a valid inode. The only thing
that happens when there is a zero inode, is that we have to compare
filenames. The inode check is just an optimization to avoid doing a
bunch of strcasecmp on systems that don't have to.
Am I correct to rephrase you that the code assumes that any
filesystem that cannot give unique inum to different files will use
0 as the placeholder inum, so if src/dst share the same non-zero
inum, it is guaranteed that is not a placeholder and we know they
are different files without comparing the two paths?
"If src and dst share the same non-zero inum, it is guaranteed that it is
not a placeholder and we know they are the same file" is more correct.
What if both inums are zero? Can this happen on any sane POSIX system? I
don't know, but my gut feeling is that inode zero is too special to be
allocated for files or directories.
In that case, it is safe to assume that the st_ino field is just a
placeholder when it is zero, and we have to compare the file name. Then we
can either assume that this happens only for our emulation layer on MinGW
(and the comparison can be case-insensitive) or choose the comparison mode
based on core.ignorecase. This patch does the former, but I think we
should do the latter.
Whatever we do, we should "protect" the strcasecmp() with ignore_case:
!ignore_case || strcasecmp(src, dst)
(And once that is done, you don't need to look at st_ino at all)
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
Am 5/7/2014 19:46, schrieb Junio C Hamano:
> David Turner writes:
>
>> On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
} else if (cache_name_pos(src, length) < 0)
bad = _("not under version control");
- else if (lstat(dst, &st) == 0) {
+ else if (lstat(dst, &dst_st) == 0 &&
+ (src_st.st_ino != dst_st.st_ino ||
+(src_st.st_ino == 0 && strcasecmp(src, dst {
>>>
>>> Don't do that. st_ino is zero on Windows only because we do not spend time
>>> to fill in the field. Don't use it as an indicator for a case-insensitive
>>> file system; zero may be a valid inode number on other systems.
>>
>> I don't think it is a problem if zero is a valid inode. The only thing
>> that happens when there is a zero inode, is that we have to compare
>> filenames. The inode check is just an optimization to avoid doing a
>> bunch of strcasecmp on systems that don't have to.
>
> Am I correct to rephrase you that the code assumes that any
> filesystem that cannot give unique inum to different files will use
> 0 as the placeholder inum, so if src/dst share the same non-zero
> inum, it is guaranteed that is not a placeholder and we know they
> are different files without comparing the two paths?
"If src and dst share the same non-zero inum, it is guaranteed that it is
not a placeholder and we know they are the same file" is more correct.
What if both inums are zero? Can this happen on any sane POSIX system? I
don't know, but my gut feeling is that inode zero is too special to be
allocated for files or directories.
In that case, it is safe to assume that the st_ino field is just a
placeholder when it is zero, and we have to compare the file name. Then we
can either assume that this happens only for our emulation layer on MinGW
(and the comparison can be case-insensitive) or choose the comparison mode
based on core.ignorecase. This patch does the former, but I think we
should do the latter.
-- Hannes
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
On Tue, May 06, 2014 at 03:59:04PM -0700, dtur...@twopensource.com wrote:
> @@ -74,7 +74,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> };
> const char **source, **destination, **dest_path, **submodule_gitfile;
> enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
> - struct stat st;
> + struct stat src_st,dst_st;
Extremely minor nit: we generally put a space after the comma.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
signature.asc
Description: Digital signature
Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems
On Wed, 2014-05-07 at 10:46 -0700, Junio C Hamano wrote:
> David Turner writes:
>
> > On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
> >> > } else if (cache_name_pos(src, length) < 0)
> >> > bad = _("not under version control");
> >> > -else if (lstat(dst, &st) == 0) {
> >> > +else if (lstat(dst, &dst_st) == 0 &&
> >> > + (src_st.st_ino != dst_st.st_ino ||
> >> > + (src_st.st_ino == 0 && strcasecmp(src,
> >> > dst {
> >>
> >> Don't do that. st_ino is zero on Windows only because we do not spend time
> >> to fill in the field. Don't use it as an indicator for a case-insensitive
> >> file system; zero may be a valid inode number on other systems.
> >
> > I don't think it is a problem if zero is a valid inode. The only thing
> > that happens when there is a zero inode, is that we have to compare
> > filenames. The inode check is just an optimization to avoid doing a
> > bunch of strcasecmp on systems that don't have to.
>
> Am I correct to rephrase you that the code assumes that any
> filesystem that cannot give unique inum to different files will use
> 0 as the placeholder inum, so if src/dst share the same non-zero
> inum, it is guaranteed that is not a placeholder and we know they
> are different files without comparing the two paths?
Yes, this is indeed a fair rephrasing. In fact, the entire zero-check
should not be necessary, as POSIX requires that the st_ino field has a
"meaningful" value. So in the case that this ever runs into a problem,
we ought to wrap the lstat call with a compatibility layer anyway.
But maybe there is an OS I'm not thinking of which fills in st_ino with
something else?
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
David Turner writes:
> On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
>> >} else if (cache_name_pos(src, length) < 0)
>> >bad = _("not under version control");
>> > - else if (lstat(dst, &st) == 0) {
>> > + else if (lstat(dst, &dst_st) == 0 &&
>> > + (src_st.st_ino != dst_st.st_ino ||
>> > +(src_st.st_ino == 0 && strcasecmp(src, dst {
>>
>> Don't do that. st_ino is zero on Windows only because we do not spend time
>> to fill in the field. Don't use it as an indicator for a case-insensitive
>> file system; zero may be a valid inode number on other systems.
>
> I don't think it is a problem if zero is a valid inode. The only thing
> that happens when there is a zero inode, is that we have to compare
> filenames. The inode check is just an optimization to avoid doing a
> bunch of strcasecmp on systems that don't have to.
Am I correct to rephrase you that the code assumes that any
filesystem that cannot give unique inum to different files will use
0 as the placeholder inum, so if src/dst share the same non-zero
inum, it is guaranteed that is not a placeholder and we know they
are different files without comparing the two paths?
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
> > } else if (cache_name_pos(src, length) < 0)
> > bad = _("not under version control");
> > - else if (lstat(dst, &st) == 0) {
> > + else if (lstat(dst, &dst_st) == 0 &&
> > +(src_st.st_ino != dst_st.st_ino ||
> > + (src_st.st_ino == 0 && strcasecmp(src, dst {
>
> Don't do that. st_ino is zero on Windows only because we do not spend time
> to fill in the field. Don't use it as an indicator for a case-insensitive
> file system; zero may be a valid inode number on other systems.
I don't think it is a problem if zero is a valid inode. The only thing
that happens when there is a zero inode, is that we have to compare
filenames. The inode check is just an optimization to avoid doing a
bunch of strcasecmp on systems that don't have to.
--
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 2/2] ignorecase: Fix git mv on insensitive filesystems
Am 5/7/2014 0:59, schrieb dtur...@twopensource.com:
> From: David Turner
>
> Make it possible to change the case of a filename on a
> case-insensitive filesystem using git mv. Change git mv to allow
> moves where the destination file exists if either the destination file
> has the same inode as the source file (for Mac) or the same name
> ignoring case (for Win).
>
> Signed-off-by: David Turner
> ---
> builtin/mv.c| 18 ++
> t/t6039-merge-ignorecase.sh | 2 +-
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 45e57f3..8cead13 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -74,7 +74,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> };
> const char **source, **destination, **dest_path, **submodule_gitfile;
> enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
> - struct stat st;
> + struct stat src_st,dst_st;
> struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>
> gitmodules_config();
> @@ -102,8 +102,8 @@ int cmd_mv(int argc, const char **argv, const char
> *prefix)
> if (dest_path[0][0] == '\0')
> /* special case: "." was normalized to "" */
> destination = internal_copy_pathspec(dest_path[0], argv, argc,
> DUP_BASENAME);
> - else if (!lstat(dest_path[0], &st) &&
> - S_ISDIR(st.st_mode)) {
> + else if (!lstat(dest_path[0], &dst_st) &&
> + S_ISDIR(dst_st.st_mode)) {
> dest_path[0] = add_slash(dest_path[0]);
> destination = internal_copy_pathspec(dest_path[0], argv, argc,
> DUP_BASENAME);
> } else {
> @@ -122,13 +122,13 @@ int cmd_mv(int argc, const char **argv, const char
> *prefix)
> printf(_("Checking rename of '%s' to '%s'\n"), src,
> dst);
>
> length = strlen(src);
> - if (lstat(src, &st) < 0)
> + if (lstat(src, &src_st) < 0)
> bad = _("bad source");
> else if (!strncmp(src, dst, length) &&
> (dst[length] == 0 || dst[length] == '/')) {
> bad = _("can not move directory into itself");
> - } else if ((src_is_dir = S_ISDIR(st.st_mode))
> - && lstat(dst, &st) == 0)
> + } else if ((src_is_dir = S_ISDIR(src_st.st_mode))
> + && lstat(dst, &dst_st) == 0)
> bad = _("cannot move directory over file");
> else if (src_is_dir) {
> int first = cache_name_pos(src, length);
> @@ -202,14 +202,16 @@ int cmd_mv(int argc, const char **argv, const char
> *prefix)
> }
> } else if (cache_name_pos(src, length) < 0)
> bad = _("not under version control");
> - else if (lstat(dst, &st) == 0) {
> + else if (lstat(dst, &dst_st) == 0 &&
> + (src_st.st_ino != dst_st.st_ino ||
> + (src_st.st_ino == 0 && strcasecmp(src, dst {
Don't do that. st_ino is zero on Windows only because we do not spend time
to fill in the field. Don't use it as an indicator for a case-insensitive
file system; zero may be a valid inode number on other systems.
> bad = _("destination exists");
> if (force) {
> /*
>* only files can overwrite each other:
>* check both source and destination
>*/
> - if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))
> {
> + if (S_ISREG(dst_st.st_mode) ||
> S_ISLNK(dst_st.st_mode)) {
> if (verbose)
> warning(_("overwriting '%s'"),
> dst);
> bad = NULL;
> diff --git a/t/t6039-merge-ignorecase.sh b/t/t6039-merge-ignorecase.sh
> index ad06752..28cfb49 100755
> --- a/t/t6039-merge-ignorecase.sh
> +++ b/t/t6039-merge-ignorecase.sh
> @@ -35,7 +35,7 @@ test_expect_success 'merge with case-changing rename on
> both sides' '
> git reset --hard baseline &&
> git branch -D with-camel &&
> git checkout -b with-camel &&
> - git mv --force TestCase testcase &&
> + git mv TestCase testcase &&
> git commit -m "recase on branch" &&
> > foo &&
> git add foo &&
--
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
