Support long file and branch names in checkout conflict creation #4520
Conversation
novalis
force-pushed
the
long-names
branch
7 times, most recently
from
6a5a01e
to
fc724d2
Compare
|
(please ignore for a bit, hacking Windows stuff which I can only do through appveyor since I don't run Windows) |
novalis
force-pushed
the
long-names
branch
17 times, most recently
from
9d2eb71
to
fbda733
Compare
novalis
force-pushed
the
long-names
branch
4 times, most recently
from
a1df0e9
to
a39c294
Compare
novalis
force-pushed
the
long-names
branch
5 times, most recently
from
f718232
to
6e30306
Compare
|
OK, did what I should have done in the first place. Please review. |
There was a problem hiding this comment.
Thanks for working on this. I'll relay the longpath-handling stuff to @ethomson, as I'm not familiar with that on Windows.
I do feel like you should somehow split out that logic to only be used on Windows. On Unix-based systems this is not the right thing to do, I'd argue, but there we should probably just error out. Also, PATH_MAX is usually set to 4096 there, which is much more reasonable than 256.
src/checkout.c
Outdated
| size_t sequence_len; | ||
| char sequence_buf[(CHAR_BIT * sizeof(int) / 3) + 3]; | ||
|
|
||
| /* Figure out the length of the suffix inclusive of the |
There was a problem hiding this comment.
We try to use either
/*
* Foobar
*/
or
/* Foobar */
style comments
src/checkout.c
Outdated
| size_t basename_len, | ||
| const char *suffix, | ||
| size_t suffix_len, | ||
| int sequence) { |
There was a problem hiding this comment.
The function-opening brace belongs on the next line
| const char *suffix, | ||
| size_t suffix_len, | ||
| int sequence) { | ||
| size_t basename_max = (NAME_MAX * 2) / 3; |
There was a problem hiding this comment.
A comment explaining this would help a lot
src/checkout.c
Outdated
| /* Figure out the length of the suffix inclusive of the | ||
| * sequence number */ | ||
| if (sequence != -1) { | ||
| /* Would like to use snprintf here, but it's C99 */ |
src/checkout.c
Outdated
| @@ -1966,34 +1966,109 @@ static int conflict_entry_name( | |||
| return 0; | |||
| } | |||
|
|
|||
| static int checkout_path_suffixed_0( | |||
There was a problem hiding this comment.
The name is a bit awkward. What exactly does this function do that checkout_path_suffixed doesn't?
src/checkout.c
Outdated
|
|
||
| if ((error = git_buf_putc(path, '~')) < 0 || (error = git_buf_puts(path, suffix)) < 0) | ||
| return -1; | ||
| git_buf_put(&saved_path, git_buf_cstr(path), git_buf_len(path)); |
There was a problem hiding this comment.
You should probably check for an error here
| * follows: filename: 2/3; suffix and digits: 1/3 - 1 (for the | ||
| * '~'). But if either filename or suffix doesn't completely | ||
| * fill its allotted space, we allow the other to take up the | ||
| * slack. |
There was a problem hiding this comment.
Aren't these (at least partially) implementation details of checkout_path_suffixed_0?
src/checkout.c
Outdated
|
|
||
| i++; | ||
| while (git_path_exists(git_buf_cstr(path)) && i < INT_MAX) { |
There was a problem hiding this comment.
INT_MAX? I feel like we should restrict this to a sane value, shouldn't we? Checking 2 billion paths hardly feels like the right thing to do
There was a problem hiding this comment.
I don't know what value would be reasonable. Whatever limit we put here, someone could create a situation that fails it. How would you feel if I made it a config value with a default of (say) 10?
There was a problem hiding this comment.
I agree. I'd aim a bit higher than 10, though. After lots of back and forth I think that 64 is the correct value.
tests/checkout/conflict.c
Outdated
| @@ -50,13 +50,13 @@ struct checkout_index_entry { | |||
| uint16_t mode; | |||
| char oid_str[GIT_OID_HEXSZ+1]; | |||
| int stage; | |||
| char path[128]; | |||
| char path[256]; | |||
There was a problem hiding this comment.
Why don't you just use PATH_MAX here and in the following declarations?
There was a problem hiding this comment.
I think these are not actually paths, but filenames. So NAME_MAX + 1 is probably what's wanted.
I don't understand what you're saying here. This patch is in fact Unix-only. It doesn't work on Windows because Windows doesn't support long file names without funny shenanigans (see #3053, mentioned in the comments here). We hit this issue on Unix, where we couldn't check out certain files. I would love to fix it for Windows as well, but when I messed around with it, I discovered that I would have to fix #3053 first, and that's a fair amount of work. |
|
Other than the INT_MAX thing, I pushed a change for the rest of the requested changes, I think |
|
Sorry, I was confused a bit. Whenever I hear "long" and "file" I actually think about the Windows thing "core.longpath". So please just ignore that bit |
When we have a conflict, we generate conflict files of the form filename~branchname, or sometimes filename~branchname_[digits], if there already exists filename~branchname. When the length of this would be over NAME_MAX, shorten it by truncating one or more of the components.
|
The Travis build seems to be messed up -- it can't download from Debian. Maybe it needs a kick? |
|
Yeah, Travis is having problems again. I already started kicking some other PRs, but now it's the macOS builds which cannot catch up. I'll give jobs another kick as soon as the queue has been processed |
|
Is this ready-to-go? |
When we have a conflict, we generate conflict files of the form
filename~branchname, or sometimes filename~branchname_[digits], if
there already exists filename~branchname. When the length of this
would be over NAME_MAX, shorten it by truncating one or more
of the components.