Re: [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink
Thank you for info, I totally missed that. Yes, this fixes the issue perfectly. Petr On 14.10.2016 15:42, Jeff King wrote: > On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote: > >> I have realized that this wasn't fixed after all when refs.c >> was "rewritten". Issue is caused by broken symlink under refs/heads, >> which causes infinite loop for "git ls-tree" command. It was replied >> earlier [0] and Michael previously fixed that in better way probably, >> then my proposed patch 2/2, but it contains more changes and I have not >> enough time to study changes in refs* code, so I propose now just my >> little patch, which was previously modified by Michael. >> >> If you prefer different solution, I am ok with this. It is really >> just quick proposal. Patch 1/2 contains test for this issue. If you >> will prefer different solution with different exit code, test should >> be corrected. Basic idea is, that timeout should'n expire with >> exit code 124. >> >> [0] http://marc.info/?l=git=142712669103790=2 >> [1] https://github.com/mhagger/git, branch "ref-broken-symlinks" > > I think I fixed this semi-independently last week; see the thread at: > > > http://public-inbox.org/git/20161006164723.ocg2nbgtulpjc...@sigill.intra.peff.net/ > > I say semi-independently because I didn't actually know about your > previous report, but saw it in the wild myself. But I did talk with > Michael off-list, and he suggested the belt-and-suspenders retry counter > in my second patch. > > The fix is at e8c42cb in Junio's tree, and it's currently merged to > 'next'. > > -Peff > signature.asc Description: OpenPGP digital signature
[PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries
From: Michael Haggerty <mhag...@alum.mit.edu> If there is a broken symlink where a loose reference file is expected, then the attempt to open() it fails with ENOENT. This error is misinterpreted to mean that the loose reference file itself has disappeared due to a race, causing the lookup to be retried. But in this scenario, the retries all suffer from the same problem, causing an infinite loop. So put a limit (of 5) on the number of times that the stat_ref step can be retried. Based-on-patch-by: Petr Stodulka <pstod...@redhat.com> Signed-off-by: Michael Haggerty <mhag...@alum.mit.edu> --- refs/files-backend.c | 6 -- refs/refs-internal.h | 6 ++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index d16feb1..245a0b5 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1353,6 +1353,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, int fd; int ret = -1; int save_errno; + int retries = 0; *type = 0; strbuf_reset(_path); @@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, if (S_ISLNK(st.st_mode)) { strbuf_reset(_contents); if (strbuf_readlink(_contents, path, 0) < 0) { - if (errno == ENOENT || errno == EINVAL) + if ((errno == ENOENT || errno == EINVAL) && + retries++ < MAXRETRIES) /* inconsistent with lstat; retry */ goto stat_ref; else @@ -1426,7 +1428,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, */ fd = open(path, O_RDONLY); if (fd < 0) { - if (errno == ENOENT) + if (errno == ENOENT && retries++ < MAXRETRIES) /* inconsistent with lstat; retry */ goto stat_ref; else diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 708b260..37e6b99 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const char *new_refname); /* We allow "recursive" symbolic refs. Only within reason, though */ #define SYMREF_MAXDEPTH 5 +/* + * We allow only MAXRETRIES tries to jump on stat_ref, because of possible + * infinite loop + */ +#define MAXRETRIES 5 + /* Include broken references in a do_for_each_ref*() iteration: */ #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 -- 2.5.5
[PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads
git ls-tree goes into an infinite loop while serving pretty vanilla requests, if the refs/heads/ directory contains a symlink that's broken. Added test which check if git ends with expected exit code or timeout expires. --- t/t3103-ls-tree-misc.sh | 9 + 1 file changed, 9 insertions(+) diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh index 09dcf04..faf79c4 100755 --- a/t/t3103-ls-tree-misc.sh +++ b/t/t3103-ls-tree-misc.sh @@ -21,4 +21,13 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' test_must_fail git ls-tree -r HEAD ' +test_expect_success 'ls-tree fails due to broken symlink instead of infinite loop' ' + mkdir foo_infinit && + cd foo_infinit && + git init testrepo && + cd testrepo && + mkdir -p .git/refs/remotes && + ln -s ../remotes/foo .git/refs/heads/bar && + test_expect_code 128 timeout 2 git ls-tree bar +' test_done -- 2.5.5
[PATCH 0/2] infinite loop in "git ls-tree" for broken symlink
Hi, I have realized that this wasn't fixed after all when refs.c was "rewritten". Issue is caused by broken symlink under refs/heads, which causes infinite loop for "git ls-tree" command. It was replied earlier [0] and Michael previously fixed that in better way probably, then my proposed patch 2/2, but it contains more changes and I have not enough time to study changes in refs* code, so I propose now just my little patch, which was previously modified by Michael. If you prefer different solution, I am ok with this. It is really just quick proposal. Patch 1/2 contains test for this issue. If you will prefer different solution with different exit code, test should be corrected. Basic idea is, that timeout should'n expire with exit code 124. [0] http://marc.info/?l=git=142712669103790=2 [1] https://github.com/mhagger/git, branch "ref-broken-symlinks" Michael Haggerty (1): resolve_ref_unsafe(): limit the number of "stat_ref" retries Petr Stodulka (1): Add test for ls-tree with broken symlink under refs/heads refs/files-backend.c| 6 -- refs/refs-internal.h| 6 ++ t/t3103-ls-tree-misc.sh | 9 + 3 files changed, 19 insertions(+), 2 deletions(-) -- 2.5.5
Re: [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries
FYI, I modified the patch slightly. On 14.10.2016 15:16, Petr Stodulka wrote: > From: Michael Haggerty <mhag...@alum.mit.edu> > > If there is a broken symlink where a loose reference file is expected, > then the attempt to open() it fails with ENOENT. This error is > misinterpreted to mean that the loose reference file itself has > disappeared due to a race, causing the lookup to be retried. But in > this scenario, the retries all suffer from the same problem, causing > an infinite loop. > > So put a limit (of 5) on the number of times that the stat_ref step > can be retried. > > Based-on-patch-by: Petr Stodulka <pstod...@redhat.com> > Signed-off-by: Michael Haggerty <mhag...@alum.mit.edu> > --- > refs/files-backend.c | 6 -- > refs/refs-internal.h | 6 ++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index d16feb1..245a0b5 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -1353,6 +1353,7 @@ static int files_read_raw_ref(struct ref_store > *ref_store, > int fd; > int ret = -1; > int save_errno; > + int retries = 0; > > *type = 0; > strbuf_reset(_path); > @@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store > *ref_store, > if (S_ISLNK(st.st_mode)) { > strbuf_reset(_contents); > if (strbuf_readlink(_contents, path, 0) < 0) { > - if (errno == ENOENT || errno == EINVAL) > + if ((errno == ENOENT || errno == EINVAL) && > + retries++ < MAXRETRIES) > /* inconsistent with lstat; retry */ > goto stat_ref; > else > @@ -1426,7 +1428,7 @@ static int files_read_raw_ref(struct ref_store > *ref_store, >*/ > fd = open(path, O_RDONLY); > if (fd < 0) { > - if (errno == ENOENT) > + if (errno == ENOENT && retries++ < MAXRETRIES) > /* inconsistent with lstat; retry */ > goto stat_ref; > else > diff --git a/refs/refs-internal.h b/refs/refs-internal.h > index 708b260..37e6b99 100644 > --- a/refs/refs-internal.h > +++ b/refs/refs-internal.h > @@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const > char *new_refname); > /* We allow "recursive" symbolic refs. Only within reason, though */ > #define SYMREF_MAXDEPTH 5 > > +/* > + * We allow only MAXRETRIES tries to jump on stat_ref, because of possible > + * infinite loop > + */ > +#define MAXRETRIES 5 > + > /* Include broken references in a do_for_each_ref*() iteration: */ > #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 > > signature.asc Description: OpenPGP digital signature
[PATCH] Replace deprecated CURLAUTH_GSSNEGOTIATE with CURLAUTH_NEGOTIATE.
Macro CURLAUTH_GSSNEGOTIATE is deprecated since cURL v7.38.0 and should be used CURLAUTH_NEGOTIATE instead. For compatibility with older versions of cURL is CURLAUTH_NEGOTIATE set as alias to CURLAUTH_GSSNEGOTIATE Signed-off-by: Petr Stodulka <pstod...@redhat.com> --- http.c| 4 ++-- http.h| 7 +++ remote-curl.c | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index 82ed542..7c2d9ef 100644 --- a/http.c +++ b/http.c @@ -79,7 +79,7 @@ static struct { } proxy_authmethods[] = { { "basic", CURLAUTH_BASIC }, { "digest", CURLAUTH_DIGEST }, - { "negotiate", CURLAUTH_GSSNEGOTIATE }, + { "negotiate", CURLAUTH_NEGOTIATE }, { "ntlm", CURLAUTH_NTLM }, #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY { "anyauth", CURLAUTH_ANY }, @@ -1277,7 +1277,7 @@ static int handle_curl_result(struct slot_results *results) return HTTP_NOAUTH; } else { #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY - http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE; + http_auth_methods &= ~CURLAUTH_NEGOTIATE; #endif return HTTP_REAUTH; } diff --git a/http.h b/http.h index 5ab9d9c..bcc7d7d 100644 --- a/http.h +++ b/http.h @@ -42,6 +42,13 @@ #endif /* + * Keep it compatible on system with cURL < 7.38.0 + * */ +#ifndef CURLAUTH_NEGOTIATE +#define CURLAUTH_NEGOTIATE CURLAUTH_GSSNEGOTIATE +#endif + +/* * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4, * and the constants were known as CURLFTPSSL_* */ diff --git a/remote-curl.c b/remote-curl.c index 6b83b77..d4fce63 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -542,7 +542,7 @@ static int post_rpc(struct rpc_state *rpc) if (err != HTTP_OK) return -1; - if (results.auth_avail & CURLAUTH_GSSNEGOTIATE) + if (results.auth_avail & CURLAUTH_NEGOTIATE) needs_100_continue = 1; } -- 2.5.5
Re: [PATCH] http: Control GSSAPI credential delegation.
On 28.9.2016 19:16, Jeff King wrote: > On Wed, Sep 28, 2016 at 06:05:52PM +0200, Petr Stodulka wrote: > >> Delegation of credentials is disabled by default in libcurl since >> version 7.21.7 due to security vulnerability CVE-2011-2192. Which >> makes troubles with GSS/kerberos authentication where delegation >> of credentials is required. This can be changed with option >> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter >> since libcurl version 7.22.0. > > I don't have any real knowledge of GSSAPI, so I'll refrain from > commenting on that aspect. But I did notice one mechanical issue: > Me neither. I have just basic knowledge and I am not able to configure virtual machine, which really need set delegation in libcurl (I need just negotiation, which is in git possible, I guess since v2.8.0). However, I discuss it with libcurl maintainer and he confirm that this option can be required in some cases and this is what I need to do. this already. I tested just setting of parameter in libcurl according to description and nothing else seems broken. So anyone else who will be able to test complete behaviour, where delegation is needed, is welcomed. [snip] > We only declare the curl_deleg variable if we have a new-enough curl. > But... > >> @@ -323,6 +335,10 @@ static int http_options(const char *var, const char >> *value, void *cb) >> return 0; >> } >> >> +if (!strcmp("http.delegation", var)) { >> +return git_config_string(_deleg, var, value); >> +} >> + > > ...here we try to use it regardless. I think you want another #ifdef, > and probably to warn the user in the #else block (similar to what the > http.pinnedpubkey code does). > > -Peff > You are right. Thanks. I sent new version of patch with fix. Petr signature.asc Description: OpenPGP digital signature
[PATCH v2] http: Control GSSAPI credential delegation.
Delegation of credentials is disabled by default in libcurl since version 7.21.7 due to security vulnerability CVE-2011-2192. Which makes troubles with GSS/kerberos authentication when delegation of credentials is required. This can be changed with option CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter since libcurl version 7.22.0. This patch provides new configuration variable http.delegation which corresponds to curl parameter "--delegation" (see man 1 curl). The following values are supported: * none (default). * policy * always Signed-off-by: Petr Stodulka <pstod...@redhat.com> --- Documentation/config.txt | 14 ++ http.c | 37 + 2 files changed, 51 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index e78293b..a179474 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1736,6 +1736,20 @@ http.emptyAuth:: a username in the URL, as libcurl normally requires a username for authentication. +http.delegation:: + Control GSSAPI credential delegation. The delegation is disabled + by default in libcurl since version 7.21.7. Set parameter to tell + the server what it is allowed to delegate when it comes to user + credentials. Used with GSS/kerberos. Possible values are: ++ +-- +* `none` - Don't allow any delegation. +* `policy` - Delegates if and only if the OK-AS-DELEGATE flag is set in the + Kerberos service ticket, which is a matter of realm policy. +* `always` - Unconditionally allow the server to delegate. +-- + + http.extraHeader:: Pass an additional HTTP header when communicating with a server. If more than one such entry exists, all of them are added as extra diff --git a/http.c b/http.c index 82ed542..0c65639 100644 --- a/http.c +++ b/http.c @@ -90,6 +90,18 @@ static struct { * here, too */ }; +#if LIBCURL_VERSION_NUM >= 0x071600 +static const char *curl_deleg; +static struct { + const char *name; + long curl_deleg_param; +} curl_deleg_levels[] = { + { "none", CURLGSSAPI_DELEGATION_NONE }, + { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG }, + { "always", CURLGSSAPI_DELEGATION_FLAG }, +}; +#endif + static struct credential proxy_auth = CREDENTIAL_INIT; static const char *curl_proxyuserpwd; static const char *curl_cookie_file; @@ -323,6 +335,15 @@ static int http_options(const char *var, const char *value, void *cb) return 0; } + if (!strcmp("http.delegation", var)) { +#if LIBCURL_VERSION_NUM >= 0x071600 + return git_config_string(_deleg, var, value); +#else + warning(_("Delegation control is not supported with cURL < 7.22.0")); + return 0; +#endif + } + if (!strcmp("http.pinnedpubkey", var)) { #if LIBCURL_VERSION_NUM >= 0x072c00 return git_config_pathname(_pinnedkey, var, value); @@ -629,6 +650,22 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY); #endif +#if LIBCURL_VERSION_NUM >= 0x071600 + if (curl_deleg) { + int i; + for (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) { + if (!strcmp(curl_deleg, curl_deleg_levels[i].name)) { + curl_easy_setopt(result, CURLOPT_GSSAPI_DELEGATION, + curl_deleg_levels[i].curl_deleg_param); + break; + } + } + if (i == ARRAY_SIZE(curl_deleg_levels)) + warning("Unknown delegation method '%s': using default", + curl_deleg); + } +#endif + if (http_proactive_auth) init_curl_http_auth(result); -- 2.5.5
Re: [PATCH] http: Control GSSAPI credential delegation.
On 28.9.2016 18:05, Petr Stodulka wrote: > Delegation of credentials is disabled by default in libcurl since > version 7.21.7 due to security vulnerability CVE-2011-2192. Which > makes troubles with GSS/kerberos authentication where delegation > of credentials is required. This can be changed with option > CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter > since libcurl version 7.22.0. Correction: Which makes troubles with GSS/kerberos authentication when delegation of credentials is required. signature.asc Description: OpenPGP digital signature
[PATCH] http: Control GSSAPI credential delegation.
Delegation of credentials is disabled by default in libcurl since version 7.21.7 due to security vulnerability CVE-2011-2192. Which makes troubles with GSS/kerberos authentication where delegation of credentials is required. This can be changed with option CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter since libcurl version 7.22.0. This patch provides new configuration variable http.delegation which corresponds to curl parameter "--delegation" (see man 1 curl). The following values are supported: * none (default). * policy * always Signed-off-by: Petr Stodulka <pstod...@redhat.com> --- Documentation/config.txt | 14 ++ http.c | 32 2 files changed, 46 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index e78293b..a179474 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1736,6 +1736,20 @@ http.emptyAuth:: a username in the URL, as libcurl normally requires a username for authentication. +http.delegation:: + Control GSSAPI credential delegation. The delegation is disabled + by default in libcurl since version 7.21.7. Set parameter to tell + the server what it is allowed to delegate when it comes to user + credentials. Used with GSS/kerberos. Possible values are: ++ +-- +* `none` - Don't allow any delegation. +* `policy` - Delegates if and only if the OK-AS-DELEGATE flag is set in the + Kerberos service ticket, which is a matter of realm policy. +* `always` - Unconditionally allow the server to delegate. +-- + + http.extraHeader:: Pass an additional HTTP header when communicating with a server. If more than one such entry exists, all of them are added as extra diff --git a/http.c b/http.c index 82ed542..5f8fab3 100644 --- a/http.c +++ b/http.c @@ -90,6 +90,18 @@ static struct { * here, too */ }; +#if LIBCURL_VERSION_NUM >= 0x071600 +static const char *curl_deleg; +static struct { + const char *name; + long curl_deleg_param; +} curl_deleg_levels[] = { + { "none", CURLGSSAPI_DELEGATION_NONE }, + { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG }, + { "always", CURLGSSAPI_DELEGATION_FLAG }, +}; +#endif + static struct credential proxy_auth = CREDENTIAL_INIT; static const char *curl_proxyuserpwd; static const char *curl_cookie_file; @@ -323,6 +335,10 @@ static int http_options(const char *var, const char *value, void *cb) return 0; } + if (!strcmp("http.delegation", var)) { + return git_config_string(_deleg, var, value); + } + if (!strcmp("http.pinnedpubkey", var)) { #if LIBCURL_VERSION_NUM >= 0x072c00 return git_config_pathname(_pinnedkey, var, value); @@ -629,6 +645,22 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY); #endif +#if LIBCURL_VERSION_NUM >= 0x071600 + if (curl_deleg) { + int i; + for (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) { + if (!strcmp(curl_deleg, curl_deleg_levels[i].name)) { + curl_easy_setopt(result, CURLOPT_GSSAPI_DELEGATION, + curl_deleg_levels[i].curl_deleg_param); + break; + } + } + if (i == ARRAY_SIZE(curl_deleg_levels)) + warning("Unknown delegation method '%s': using default", + curl_deleg); + } +#endif + if (http_proactive_auth) init_curl_http_auth(result); -- 2.5.5
Re: Forgotten declaration of function path_name() in revision.h?
On 16.3.2016 19:48, Jeff King wrote: > On Wed, Mar 16, 2016 at 07:05:49PM +0100, Petr Stodulka wrote: > >> according to commit 9831e92 (merge) there is maybe by mistake kept >> declaration >> of function path_name() in revision.h, whose definition was removed >> and isn't used in git anymore. > > Yes, this should have been part of de1e67d (list-objects: pass full > pathname to callbacks, 2016-02-11), but I missed it. > > The patch itself looks fine to me (though it probably makes more sense > to point to de1e67d than the merge). > > Note also that: > >> == >> From ae72c8f9085b3b7bd84f94f90ff5b0416db59d67 Mon Sep 17 00:00:00 2001 >> From: Petr Stodulka <pstod...@redhat.com> >> Date: Wed, 16 Mar 2016 18:51:53 +0100 >> Subject: [PATCH] remove obsoleted function path_name() from header file >> revision.h > > It makes things easier on the maintainer if you format your patch such > that "git am" can apply it directly. Use scissors like: > > -- >8 -- > > to separate the patch from anything that should not go into the commit > message (rather than "=..." as you have here). > > Drop the "From " line which says nothing (it is an mbox separator, but > we are already inside a message). > > The rest of the headers do not hurt, but are generally redundant with > what is in your email header (though in this case, the Subject is > different, so you would want to retain that). > > -Peff > Thanks for advice. I will remember this. When I have time, I will look at git-send-mail eventually. Petr signature.asc Description: OpenPGP digital signature
Forgotten declaration of function path_name() in revision.h?
Hi, according to commit 9831e92 (merge) there is maybe by mistake kept declaration of function path_name() in revision.h, whose definition was removed and isn't used in git anymore. Regards, Petr == From ae72c8f9085b3b7bd84f94f90ff5b0416db59d67 Mon Sep 17 00:00:00 2001 From: Petr Stodulka <pstod...@redhat.com> Date: Wed, 16 Mar 2016 18:51:53 +0100 Subject: [PATCH] remove obsoleted function path_name() from header file revision.h The function was removed by commit 9831e92 due to possible security issue. --- revision.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/revision.h b/revision.h index dca0d38..f20168e 100644 --- a/revision.h +++ b/revision.h @@ -257,8 +257,6 @@ extern void put_revision_mark(const struct rev_info *revs, extern void mark_parents_uninteresting(struct commit *commit); extern void mark_tree_uninteresting(struct tree *tree); -char *path_name(struct strbuf *path, const char *name); - extern void show_object_with_name(FILE *, struct object *, const char *); extern void add_pending_object(struct rev_info *revs, -- 2.4.3 signature.asc Description: OpenPGP digital signature
Re: COPYING tabs vs whitespaces
On 9.2.2016 19:40, Stefan Beller wrote: > As a reference for this discussion, > see c376d96 in git.git or e00bfcbf04 in linux.git > which deals with whitespaces in the developers certificate of origin. > > Just send a patch, I'd say. > > Thanks, > Stefan > -- > 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 > Just patch makes from whole thread more and more absurd discussion, no offense. Don't matter if it is used or not. For me it's significant previous response from Junio. I am not familiar with git send-email, so: ===[PATCH 1/2] From b2509773edf7e886513f450cb8e215a87278cdfc Mon Sep 17 00:00:00 2001 From: Petr Stodulka <pstod...@redhat.com> Date: Tue, 9 Feb 2016 19:52:28 +0100 Subject: [PATCH 1/2] COPYING: replace tabs by spaces in license file GPLv2 license file presented in gnu.org has replaced all tabs by spaces. Unify whitespaces to make equivalent files (except preamble). --- COPYING | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/COPYING b/COPYING index 536e555..158a9b1 100644 --- a/COPYING +++ b/COPYING @@ -19,15 +19,16 @@ - GNU GENERAL PUBLIC LICENSE - Version 2, June 1991 + +GNU GENERAL PUBLIC LICENSE + Version 2, June 1991 Copyright (C) 1989, 1991 Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. - Preamble +Preamble The licenses for most software are designed to take away your freedom to share and change it. By contrast, the GNU General Public @@ -77,7 +78,7 @@ patent must be licensed for everyone's free use or not licensed at all. The precise terms and conditions for copying, distribution and modification follow. - GNU GENERAL PUBLIC LICENSE +GNU GENERAL PUBLIC LICENSE TERMS AND CONDITIONS FOR COPYING, DISTRIBUTION AND MODIFICATION 0. This License applies to any program or other work which contains @@ -276,7 +277,7 @@ make exceptions for this. Our decision will be guided by the two goals of preserving the free status of all derivatives of our free software and of promoting the sharing and reuse of software generally. - NO WARRANTY +NO WARRANTY 11. BECAUSE THE PROGRAM IS LICENSED FREE OF CHARGE, THERE IS NO WARRANTY FOR THE PROGRAM, TO THE EXTENT PERMITTED BY APPLICABLE LAW. EXCEPT WHEN @@ -298,9 +299,9 @@ YOU OR THIRD PARTIES OR A FAILURE OF THE PROGRAM TO OPERATE WITH ANY OTHER PROGRAMS), EVEN IF SUCH HOLDER OR OTHER PARTY HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH DAMAGES. -END OF TERMS AND CONDITIONS + END OF TERMS AND CONDITIONS - How to Apply These Terms to Your New Programs +How to Apply These Terms to Your New Programs If you develop a new program, and you want it to be of the greatest possible use to the public, the best way to achieve this is to make it -- 2.4.3 ===[PATCH 2/2] From 5a7bfe0b05f35238c7bc8d2680b168ef729d0e91 Mon Sep 17 00:00:00 2001 From: Petr Stodulka <pstod...@redhat.com> Date: Tue, 9 Feb 2016 20:08:44 +0100 Subject: [PATCH 2/2] Unify tabs and spaces in preamble of COPYING file --- COPYING | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/COPYING b/COPYING index 158a9b1..bc404d9 100644 --- a/COPYING +++ b/COPYING @@ -9,13 +9,13 @@ decision, you might note so in your copyright message, ie something like - This file is licensed under the GPL v2, or a later version - at the discretion of Linus. +This file is licensed under the GPL v2, or a later version +at the discretion of Linus. might avoid issues. But we can also just decide to synchronize and contact all copyright holders on record if/when the occasion arises. - Linus Torvalds +Linus Torvalds -- 2.4.3 signature.asc Description: OpenPGP digital signature
Re: COPYING tabs vs whitespaces
On 4.2.2016 20:15, Junio C Hamano wrote: > Petr Stodulka <pstod...@redhat.com> writes: > >> I found that license file COPYING is different as compared with >> http://www.gnu.org/licenses/gpl-2.0.txt If I pass over with >> Linus's preamble, change is only about whitespaces - tabs >> vs. space. Probably it's minor non-essential change, but some >> projects do this change, so rather I ask about that. > > Interesting. I cannot quite connect "some projects do this change" > and "so rather I ask". Are you asking why this project changed it? Nope. I apologize for my czenglish. It means: From my colleagues I hear, that some projects had same differences (tabs vs. spaces) in their copy of the license file and they make it later equivalent with the one in gnu.org. So I ask rather here / point out this difference, if you know about that or you want to have same one. Yes, I know, it's just about whitespaces. Petr. > > After running "diff" between the two, I think the changes are only > on the indented section title lines, and "git blame" tells us that > the section title lines in the copy we have has always been that way > since Linus added to it at 075b845a (Add a COPYING notice, making it > explicit that the license is GPLv2., 2005-04-11). > > So, perhaps the copy Linus got originally had leading runs of spaces > that are multiples-of-8-long unexpanded to leading tabs back then, > or perhaps he did that unexpanding himself. > > signature.asc Description: OpenPGP digital signature
Re: COPYING tabs vs whitespaces
On 8.2.2016 18:28, Junio C Hamano wrote: > Petr Stodulka <pstod...@redhat.com> writes: > >> On 4.2.2016 20:15, Junio C Hamano wrote: >>> Petr Stodulka <pstod...@redhat.com> writes: >>> >>>> I found that license file COPYING is different as compared with >>>> http://www.gnu.org/licenses/gpl-2.0.txt If I pass over with >>>> Linus's preamble, change is only about whitespaces - tabs >>>> vs. space. Probably it's minor non-essential change, but some >>>> projects do this change, so rather I ask about that. >>> >>> Interesting. I cannot quite connect "some projects do this change" >>> and "so rather I ask". Are you asking why this project changed it? >> >> Nope. I apologize for my czenglish. It means: From my colleagues I hear, >> that some projects had same differences (tabs vs. spaces) in their copy >> of the license file and they make it later equivalent with the one in >> gnu.org. > > I'd guess that these projects (among which Linux kernel still has > these indentation the same as the copy we have) and we independently > obtained the COPYING file from GNU in some past, and back then the > copy at GNU was indented that way--which later was changed. > > The Wayback Machine supports this theory. > > https://web.archive.org/web/20070713225446/http://www.gnu.org/licenses/gpl-2.0.txt > > i.e. the FSF copy back in 2007-07 indented these section headers > with tabs, so those projects that obtained this copy would have > their sections indented with tabs. > > At 703601d6 (Update COPYING with GPLv2 with new FSF address, > 2010-01-15), we did a fresh update directly from the URL you cited > above to primarily replace the addresses of the FSF office. > > https://web.archive.org/web/20100105100239/http://www.gnu.org/licenses/gpl-2.0.txt > > matches what we use (minus Linus's preamble, of course). > > The file before that change was what Linus copied from Linux kernel > project. The kernel project did their equivalent change at their > b3358a11 ([PATCH] update FSF address in COPYING, 2005-09-10), and > the log message says http://www.gnu.org/licenses/gpl.txt was used. > > The Wayback Machine agrees. > > https://web.archive.org/web/20050901115237/http://www.gnu.org/licenses/gpl.txt > > i.e. the FSF copy back in 2005-09 matches what the kernel uses > (again, minus Linus's preamble). I have expected that license was copied correctly in the past from gnu.org, when same differences are in various projects. I just point out on another change. Thank You for tip about web.archive.org - I really don't know about that web and it can be useful. > >> So I ask rather here / point out this difference, if you know >> about that or you want to have same one. > > So the answers are: > > - No, I didn't personally know about the differences, and I suspect >nobody particularly cared. > > - Not really, unless the difference has more substance. For an >example of an update with substance, the update we did in 2010 >had not just the FSF address change but also updated the fully >spelled name of LGPL from Library to Lesser. Thank You for reponse. > > You may want to bug the kernel folks to update their copy; they > still spell it as Library General Public License. > Everyone can do that. I believe that someone report it already or at least constult it. I write about this here because I should do that. When You know about this difference in license in kernel, I believe that they know it too and they decide it is ok. Regards, Petr signature.asc Description: OpenPGP digital signature
COPYING tabs vs whitespaces
Hi, I found that license file COPYING is different as compared with http://www.gnu.org/licenses/gpl-2.0.txt If I pass over with Linus's preamble, change is only about whitespaces - tabs vs. space. Probably it's minor non-essential change, but some projects do this change, so rather I ask about that. Regards, Petr signature.asc Description: OpenPGP digital signature
Re: [PATCH] request-pull: short sha handling, manual update
Hi folks, can you someone look at it? Or were these troubles mentioned somewhere earlier and I miss that? On 2.6.2015 16:14, Petr Stodulka wrote: request-pull prints incorrectly warn messages about not found commits and man pages don't say anything about todays changed behaviour. People are confused and try look for errors at wrong places. At least these should be fixed/modified. Warn massage says that commit can't be found ar remote, however there it is in and is missing on local repository in 'many' cases. So I don't know if better solution is check, where commit is truly missing or transform warning message. Something like: warn: No match for commit commit found at url or local repository. warn: Are you sure you have synchronized branch with remote repository? .. man page could be changed like this: --- diff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt index 283577b..6d34fc7 100644 --- a/Documentation/git-request-pull.txt +++ b/Documentation/git-request-pull.txt @@ -73,6 +73,17 @@ then you can ask that to be pulled with git request-pull v1.0 https://git.ko.xz/project master:for-linus +NOTES +- + +Since git version 2.0.0 is behaviour of git request-pull little different. +It is recommended use of third argument for each request-pull, otherwise +you can get error message like: + + warn: No match for commit commit found at url + warn: Are you sure you pushed 'HEAD' there? + + GIT --- Part of the linkgit:git[1] suite --- Second patch provides right processing of third parameter when short version of sha hash is used (e.g. 897a111). Now is supported only full hash, what is different behaviour against first parameter or what can be found in other functions. Extra solves one of cases of wrong warn message. --- diff --git a/git-request-pull.sh b/git-request-pull.sh index d5500fd..2dc735e 100755 --- a/git-request-pull.sh +++ b/git-request-pull.sh @@ -92,9 +92,11 @@ find_matching_ref=' chomp; my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/; my ($pattern); + my ($pattern2); next unless ($sha1 eq $headrev); $pattern=/$head\$; + $pattern2=^$head; if ($ref eq $head) { $found = $ref; } @@ -104,6 +106,9 @@ find_matching_ref=' if ($sha1 eq $head) { $found = $sha1; } + elsif ($sha1 =~ /$pattern2/ and (length $head) gt 7) { + $found = $sha1 + } } if ($found) { print $found\n; --- -- 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
[PATCH] request-pull: short sha handling, manual update
request-pull prints incorrectly warn messages about not found commits and man pages don't say anything about todays changed behaviour. People are confused and try look for errors at wrong places. At least these should be fixed/modified. Warn massage says that commit can't be found ar remote, however there it is in and is missing on local repository in 'many' cases. So I don't know if better solution is check, where commit is truly missing or transform warning message. Something like: warn: No match for commit commit found at url or local repository. warn: Are you sure you have synchronized branch with remote repository? .. man page could be changed like this: --- diff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt index 283577b..6d34fc7 100644 --- a/Documentation/git-request-pull.txt +++ b/Documentation/git-request-pull.txt @@ -73,6 +73,17 @@ then you can ask that to be pulled with git request-pull v1.0 https://git.ko.xz/project master:for-linus +NOTES +- + +Since git version 2.0.0 is behaviour of git request-pull little different. +It is recommended use of third argument for each request-pull, otherwise +you can get error message like: + + warn: No match for commit commit found at url + warn: Are you sure you pushed 'HEAD' there? + + GIT --- Part of the linkgit:git[1] suite --- Second patch provides right processing of third parameter when short version of sha hash is used (e.g. 897a111). Now is supported only full hash, what is different behaviour against first parameter or what can be found in other functions. Extra solves one of cases of wrong warn message. --- diff --git a/git-request-pull.sh b/git-request-pull.sh index d5500fd..2dc735e 100755 --- a/git-request-pull.sh +++ b/git-request-pull.sh @@ -92,9 +92,11 @@ find_matching_ref=' chomp; my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/; my ($pattern); + my ($pattern2); next unless ($sha1 eq $headrev); $pattern=/$head\$; + $pattern2=^$head; if ($ref eq $head) { $found = $ref; } @@ -104,6 +106,9 @@ find_matching_ref=' if ($sha1 eq $head) { $found = $sha1; } + elsif ($sha1 =~ /$pattern2/ and (length $head) gt 7) { + $found = $sha1 + } } if ($found) { print $found\n; --- -- 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: [BUG] [PATCH] infinite loop due to broken symlink
On 25.3.2015 23:53 Michael Haggerty wrote: On 03/23/2015 05:04 PM, Petr Stodulka wrote: git goes into an infinite loop due to broken symlink (minimal reproducer [0]). Affected code is in function resolve_ref_unsafe in file refs.c - notice 'stat_ref'. There is comment about problem with race condition, hovewer in that case it's regular broken symlink which cause infinite loop. Thanks for the bug report. I can confirm the problem. In fact, I noticed the same problem when I was working on a refactoring in the area, but I still haven't submitted those patches. Luckily, modern Git doesn't use symlinks in the loose reference hierarchy, so most users will never encounter this problem. In fact I think it is only the open() call that can lead to the infinite loop. Meanwhile, there is another problem caused by falling through the symlink-handling code, namely that st reflects the lstat() of the symlink rather than the stat() of the file that it points to. My approach was to run stat() on the path if the symlink-handling code falls through. You can see my work in progress in my GitHub repo [1]. Yes, I thought up about similar solution, but I wasn't sure about this way because of race condition (I don't know well code of git yet). In the case of approved lstat/stat patch, I am more familiar with this solution. Possible patch could be something like this: --- diff --git a/refs.c b/refs.c index e23542b..9efe8d2 100644 --- a/refs.c +++ b/refs.c @@ -1356,6 +1356,7 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs) /* We allow recursive symbolic refs. Only within reason, though */ #define MAXDEPTH 5 #define MAXREFLEN (1024) +#define MAXLOOP 1024 /* * Called by resolve_gitlink_ref_recursive() after it failed to read @@ -1482,6 +1483,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned char buffer[256]; static char refname_buffer[256]; int bad_name = 0; +int loop_counter = 0; if (flags) *flags = 0; @@ -1546,7 +1548,8 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned if (S_ISLNK(st.st_mode)) { len = readlink(path, buffer, sizeof(buffer)-1); if (len 0) { - if (errno == ENOENT || errno == EINVAL) + if (loop_counter++ MAXLOOP +(errno == ENOENT || errno == EINVAL)) /* inconsistent with lstat; retry */ goto stat_ref; else @@ -1579,7 +1582,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned */ fd = open(path, O_RDONLY); if (fd 0) { - if (errno == ENOENT) + if (loop_counter++ MAXLOOP errno == ENOENT) /* inconsistent with lstat; retry */ goto stat_ref; else --- If I understand well that simple check of broken symlink is not possible due to race conditions. This change also prevents the infinite loop, in fact in a more failproof way that doesn't depend on errno values being interpreted correctly. I would suggest a few stylistic changes, like for example here [2]. On the other hand, this change doesn't solve the lstat()/stat() problem. Nevertheless, I wouldn't object to a fix like this being accepted in addition to the stat() fix when it's ready. It doesn't hurt to wear both a belt *and* suspenders. Michael [1] https://github.com/mhagger/git, branch wip/refactor-resolve-ref. See especially commit d2425d8ac8cf73494b318078b92f5b1c510a68fb. [2] https://github.com/mhagger/git, branch ref-broken-symlinks When stat/lstat is ready, probably this will not be valuable anymore, but it could be reliable 'stop' for some unexpected/unknown possibly ways in future. Petr. -- 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
[BUG] [PATCH] infinite loop due to broken symlink
Hi guys, git goes into an infinite loop due to broken symlink (minimal reproducer [0]). Affected code is in function resolve_ref_unsafe in file refs.c - notice 'stat_ref'. There is comment about problem with race condition, hovewer in that case it's regular broken symlink which cause infinite loop. Possible patch could be something like this: --- diff --git a/refs.c b/refs.c index e23542b..9efe8d2 100644 --- a/refs.c +++ b/refs.c @@ -1356,6 +1356,7 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs) /* We allow recursive symbolic refs. Only within reason, though */ #define MAXDEPTH 5 #define MAXREFLEN (1024) +#define MAXLOOP 1024 /* * Called by resolve_gitlink_ref_recursive() after it failed to read @@ -1482,6 +1483,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned char buffer[256]; static char refname_buffer[256]; int bad_name = 0; +int loop_counter = 0; if (flags) *flags = 0; @@ -1546,7 +1548,8 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned if (S_ISLNK(st.st_mode)) { len = readlink(path, buffer, sizeof(buffer)-1); if (len 0) { - if (errno == ENOENT || errno == EINVAL) + if (loop_counter++ MAXLOOP +(errno == ENOENT || errno == EINVAL)) /* inconsistent with lstat; retry */ goto stat_ref; else @@ -1579,7 +1582,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned */ fd = open(path, O_RDONLY); if (fd 0) { - if (errno == ENOENT) + if (loop_counter++ MAXLOOP errno == ENOENT) /* inconsistent with lstat; retry */ goto stat_ref; else --- If I understand well that simple check of broken symlink is not possible due to race conditions. Regards, Petr [0] https://bugzilla.redhat.com/show_bug.cgi?id=1204193 -- 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
[BUG] resolved deltas
Hi guys, I wanted post you patch here for this bug, but I can't find primary source of this problem [0], because I don't understand some ideas in the code. So what I investigate: Bug is reprodusible since git version 1.8.3.1 (may earlier 1.8.xx, but I don't test it) to actual upstream version. This problem doesn't exists in version 1.7.xx - or more precisely is not reproducible. May this is reproducible since commit 7218a215 - in this commit was added assert in file builtin/index-pack.c (actual line is 918), but I didn't test this. This assert tests if object's (child) real type == OBJ_REF_DELTA. This will failure for object with real_type == OBJ_TREE (set as parent's type) and type == OBJ_REF_DELTA. Here some prints of important variables before failure assert() (from older version but I think that values are still actual in this case): -- (gdb) p base-ref_first $9 = 3223 (gdb) p deltas[3223] $10 = { base = { sha1 = \274\070k\343K\324x\037q\273h\327*n\n\356\061$ \036, offset = 2267795834784135356 }, obj_no = 11152 } (gdb) p *child $11 = { idx = { sha1 = J\242i\251\261\273\305\067\236%CE\022\257\252\342[;\tD, crc32 = 2811659028, offset = 10392153 }, size = 30, hdr_size = 22, type = OBJ_REF_DELTA, real_type = OBJ_TREE, delta_depth = 0, base_object_no = 5458 } (gdb) p objects[5458] $13 = { idx = { sha1 = \274\070k\343K\324x\037q\273h\327*n\n\356\061$ \036, crc32 = 3724458534, offset = 6879168 }, size = 236, hdr_size = 2, type = OBJ_TREE, real_type = OBJ_TREE, delta_depth = 0, base_object_no = 0 } --- base_object_no is static 5458. base-ref_first child's object are dynamic. If you want stop process in same position my recommendation for gdb (if you use gdb) when you will be in file index-pack.c: br 1093 cont set variable nr_threads = 1 br cond 2 i == 6300 cont br 916 cont --- compilated without any optimisation, line numbers modified for commit 6c4ab27f2378ce67940b4496365043119d72 Condition i == 6300 --- last iteration before failure has dynamic rank in range 6304 to 6309 in the most cases (that's weird for me, when count of downloaded objects is statically 12806, may wrong search of children?) --- Here I am lost. I don't know really what I can do next here, because I don't understand some ideas in code. e.g. searching of child - functions find_delta(), find_delta_children(). Calculation on line 618: int next = (first+last) / 2; I still don't understand. I didn't find description of this searching algorithm in tech. documentation but I didn't read all yet. However I think that source of problems could be somewhere in these two functions. When child is found, its real_type is set to parent's type in function resolve_delta() on the line 865 and then lasts wait for failure. I don't think that problem is in repository itself [1], but it is possible. Any next ideas/hints or explanation of these functions? I began study source code and mechanisms of the git this week, so don't beat me yet please :-) Regards, Petr [0] https://bugzilla.redhat.com/show_bug.cgi?id=1099919 [1] git clone https://code.google.com/p/mapsforge/ mapsforge.git -- 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: [BUG] resolved deltas
snip Bug is reprodusible since git version 1.8.3.1 (may earlier 1.8.xx, but I don't test it) to actual upstream version. This problem doesn't exists in version 1.7.xx - or more precisely is not reproducible. May this is reproducible since commit 7218a215 - in this commit was added assert in file builtin/index-pack.c (actual line is 918), but I didn't test this. Ok so this is reproducible since this commit because of assert(). Here I am lost. I don't know really what I can do next here, because I don't understand some ideas in code. e.g. searching of child - functions find_delta(), find_delta_children(). Calculation on line 618: int next = (first+last) / 2; I still don't understand. I didn't find description of this searching algorithm in tech. documentation but I didn't read all yet. However I think that source of problems could be somewhere in these two functions. When child is found, its real_type is set to parent's type in function resolve_delta() on the line 865 and then lasts wait for failure. I don't think that problem is in repository itself [1], but it is possible. I read history of commits and my idea seems to be incorrect. It seems more like some error in repository itself. But I'd rather get opinion from someone who knows this code and ideas better. Regards, Petr [0] https://bugzilla.redhat.com/show_bug.cgi?id=1099919 [1] git clone https://code.google.com/p/mapsforge/ mapsforge.git -- 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