-
Notifications
You must be signed in to change notification settings - Fork 162
Last preparations before upstreaming Git for Windows' symlink support #2017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: js/test-symlink-windows
Are you sure you want to change the base?
Changes from all commits
53b99c0
b4c97e5
f08fcf7
60189b5
04c2f10
37dd867
96a699b
4a59267
051face
77dfd22
1928738
31497b0
dba2810
db1feb2
3521180
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -324,8 +324,8 @@ static enum fsync_component parse_fsync_components(const char *var, const char * | |
| return (current & ~negative) | positive; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Dec 16, 2025 at 03:33:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/setup.c b/setup.c
> index 7086741e6c..42e4e7a690 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2611,7 +2611,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
> * have set up the repository format such that we can evaluate
> * includeIf conditions correctly in the case of re-initialization.
> */
> - repo_config(the_repository, platform_core_config, NULL);
> + repo_config(the_repository, git_default_core_config, NULL);
>
> safe_create_dir(the_repository, git_dir, 0);
Two lines further down we call `create_default_files()`, and there we
end up calling `repo_config(the_repository, git_default_config, NULL)`
as one of the first things. We do so after copying templates though, so
indeed this comes too late.
We also cannot really merge these two calls: we need to re-parse the
configuration after having copied over the template, as the template may
contain a gitconfig file itself.
Furthermore, `git_default_core_config()` already knows to call
`platform_core_config()`, as well. So we're not losing any of that
information, either.
All to say that this change makes sense to me and should be safe, as we
don't end up parsing _more_ configuration keys, we only parse a subset
of it a bit earlier.
Patrick |
||
| } | ||
|
|
||
| static int git_default_core_config(const char *var, const char *value, | ||
| const struct config_context *ctx, void *cb) | ||
| int git_default_core_config(const char *var, const char *value, | ||
| const struct config_context *ctx, void *cb) | ||
| { | ||
| /* This needs a better name */ | ||
| if (!strcmp(var, "core.filemode")) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -566,24 +566,22 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f) | |
| return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Dec 16, 2025 at 03:33:48PM +0000, Karsten Blees via GitGitGadget wrote:
> diff --git a/strbuf.c b/strbuf.c
> index 44a8f6a554..fa4e30f112 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -566,8 +566,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
> return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
> }
>
> -#define STRBUF_MAXLINK (2*PATH_MAX)
> -
> int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> {
> size_t oldalloc = sb->alloc;
> @@ -575,7 +573,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> if (hint < 32)
> hint = 32;
>
> - while (hint < STRBUF_MAXLINK) {
> + for (;;) {
> ssize_t len;
>
> strbuf_grow(sb, hint + 1);
This makes me wonder whether we have a better way to figure out the
actual size of the buffer that we ultimately need to allocate. But
reading through readlink(3p) doesn't indicate anything, and I'm not sure
whether we can always rely on lstat(3p) to return the correct size for
symlink contents on all platforms.
One thing that _is_ noted though is that calling the function with a
buffer size larger than SSIZE_MAX is implementation-defined. It does
make me a bit uneasy in that light to grow indefinitely.
Which makes me wonder whether Windows has a limit for the symlink
contents that we could enforce in theory so that we can reasonably turn
this into a bounded loop again?
PatrickThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Junio C Hamano wrote (reply to this): "Karsten Blees via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Karsten Blees <blees@dcon.de>
>
> The `strbuf_readlink()` function refuses to read link targets that
> exceed PATH_MAX (even if a sufficient size was specified by the caller).
>
> As some platforms (*cough* Windows *cough*) support longer paths, remove
> this restriction (similar to `strbuf_getcwd()`).
>
> Signed-off-by: Karsten Blees <blees@dcon.de>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> strbuf.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
We've been bitten before by platforms that sets PATH_MAX too low
(i.e., lower than what they comfortably support), so this is a
welcome change.
> diff --git a/strbuf.c b/strbuf.c
> index 44a8f6a554..fa4e30f112 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -566,8 +566,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
> return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
> }
>
> -#define STRBUF_MAXLINK (2*PATH_MAX)
> -
> int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> {
> size_t oldalloc = sb->alloc;
> @@ -575,7 +573,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> if (hint < 32)
> hint = 32;
>
> - while (hint < STRBUF_MAXLINK) {
> + for (;;) {
> ssize_t len;
>
> strbuf_grow(sb, hint + 1);
I briefly wondered if this would cause us loop infinitely on a truly
broken platform, where readlink() somehow keeps returning negative,
but we only retry when we got ERANGE (which can be seen several
lines below the postimage of hte patch), so we should be safe.
Thanks. |
||
| } | ||
|
|
||
| #define STRBUF_MAXLINK (2*PATH_MAX) | ||
|
|
||
| int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) | ||
| { | ||
| size_t oldalloc = sb->alloc; | ||
|
|
||
| if (hint < 32) | ||
| hint = 32; | ||
|
|
||
| while (hint < STRBUF_MAXLINK) { | ||
| for (;;) { | ||
| ssize_t len; | ||
|
|
||
| strbuf_grow(sb, hint); | ||
| len = readlink(path, sb->buf, hint); | ||
| strbuf_grow(sb, hint + 1); | ||
| len = readlink(path, sb->buf, hint + 1); | ||
| if (len < 0) { | ||
| if (errno != ERANGE) | ||
| break; | ||
| } else if (len < hint) { | ||
| } else if (len <= hint) { | ||
| strbuf_setlen(sb, len); | ||
| return 0; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):