[PATCH v4] for-each-ref: Always check stat_tracking_info()'s return value.
The code handling %(upstream:track) and %(upstream:trackshort) assumed
it always had a valid branch that had been sanitized earlier in
populate_value(), and thus did not check the return value of the call to
stat_tracking_info().
While there is indeed some sanitization code that basically corresponds
to stat_tracking_info() returning 0 (no base branch set), the function
can also return -1 when the base branch did exist but has since then
been deleted.
In this case, num_ours and num_theirs had undefined values and a call to
`git for-each-ref --format=%(upstream:track)` could print spurious
values such as
[behind -111794512]
[ahead 38881640, behind 5103867]
even for repositories with one single commit.
We now properly verify stat_tracking_info()'s return value and do not
print anything if it returns -1. This behavior also matches the
documentation (has no effect if the ref does not have tracking
information associated with it).
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Jeff King p...@peff.net
Signed-off-by: Raphael Kubo da Costa raphael.kubo.da.co...@intel.com
---
v4: Use Jeff's suggestion and simplify the test case and making it
easier to understand what is actually being tested.
builtin/for-each-ref.c | 11 +--
t/t6300-for-each-ref.sh | 14 ++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 603a90e..52e6323 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -717,7 +717,10 @@ static void populate_value(struct refinfo *ref)
starts_with(name, upstream)) {
char buf[40];
- stat_tracking_info(branch, num_ours,
num_theirs);
+ if (stat_tracking_info(branch, num_ours,
+ num_theirs) != 1)
+ continue;
+
if (!num_ours !num_theirs)
v-s = ;
else if (!num_ours) {
@@ -735,7 +738,11 @@ static void populate_value(struct refinfo *ref)
} else if (!strcmp(formatp, trackshort)
starts_with(name, upstream)) {
assert(branch);
- stat_tracking_info(branch, num_ours,
num_theirs);
+
+ if (stat_tracking_info(branch, num_ours,
+ num_theirs) != 1)
+ continue;
+
if (!num_ours !num_theirs)
v-s = =;
else if (!num_ours)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index bda354c..dcee7a0 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -335,6 +335,20 @@ test_expect_success 'Check that :track[short] cannot be
used with other atoms' '
'
cat expected EOF
+
+
+EOF
+
+test_expect_success 'Check that :track[short] works when upstream is invalid' '
+ test_when_finished git config branch.master.merge refs/heads/master
+ git config branch.master.merge refs/heads/does-not-exist
+ git for-each-ref \
+ --format=%(upstream:track)$LF%(upstream:trackshort) \
+ refs/heads actual
+ test_cmp expected actual
+'
+
+cat expected EOF
$(git rev-parse --short HEAD)
EOF
--
2.1.4
--
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] remote-curl: fall back to Basic auth if Negotiate fails.
On Dec 26, 2014, at 11:01 PM, brian m. carlson sand...@crustytoothpaste.net
wrote:
Apache servers using mod_auth_kerb can be configured to allow the user
to authenticate either using Negotiate (using the Kerberos ticket) or
Basic authentication (using the Kerberos password). Often, one will
want to use Negotiate authentication if it is available, but fall back
to Basic authentication if the ticket is missing or expired.
Teach the HTTP client code to stop trying authentication mechanisms that
don't use a password (currently Negotiate) after the first failure,
since if they failed the first time, they will never succeed.
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
I was able to reproduce the problem on my server. This fixes the
problem for me both when info/refs requires authentication and when it
does not. Dan, please try and see if this fixes the problem for you.
Has the patch changed since originally posted? Is there a newer one I should
try instead?
Thank you for your work on this, I will test this today.
—
Dan Langille
Infrastructure Operations
Talos Group
Sourcefire, Inc.
I'm not clear on whether NTLM is a passwordless authentication method.
Since I don't use Windows or NTLM, I can't test it, but if it is, just
adding it to HTTP_AUTH_PASSWORDLESS should be sufficient.
http.c| 14 ++
http.h| 5 -
remote-curl.c | 13 -
3 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/http.c b/http.c
index 040f362..e3e4c65 100644
--- a/http.c
+++ b/http.c
@@ -986,6 +986,16 @@ static void extract_content_type(struct strbuf *raw,
struct strbuf *type,
strbuf_addstr(charset, ISO-8859-1);
}
+void disable_passwordless_auth(struct active_request_slot *slot)
+{
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+#define HTTP_AUTH_PASSWORDLESS (CURLAUTH_GSSNEGOTIATE)
+ curl_easy_setopt(slot-curl, CURLOPT_HTTPAUTH,
+ CURLAUTH_ANY ~HTTP_AUTH_PASSWORDLESS);
+#endif
+}
+
+
/* http_request() targets */
#define HTTP_REQUEST_STRBUF 0
#define HTTP_REQUEST_FILE 1
@@ -1035,6 +1045,9 @@ static int http_request(const char *url,
curl_easy_setopt(slot-curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot-curl, CURLOPT_ENCODING, gzip);
+ if (options-no_passwordless_auth)
+ disable_passwordless_auth(slot);
+
ret = run_one_slot(slot, results);
if (options options-content_type) {
@@ -1139,6 +1152,7 @@ static int http_request_reauth(const char *url,
}
credential_fill(http_auth);
+ options-no_passwordless_auth = 1;
return http_request(url, result, target, options);
}
diff --git a/http.h b/http.h
index 473179b..fc42bf5 100644
--- a/http.h
+++ b/http.h
@@ -98,6 +98,8 @@ extern int handle_curl_result(struct slot_results *results);
int run_one_slot(struct active_request_slot *slot,
struct slot_results *results);
+void disable_passwordless_auth(struct active_request_slot *slot);
+
#ifdef USE_CURL_MULTI
extern void fill_active_slots(void);
extern void add_fill_function(void *data, int (*fill)(void *));
@@ -138,7 +140,8 @@ extern char *get_remote_object_url(const char *url, const
char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
- keep_error:1;
+ keep_error:1,
+ no_passwordless_auth:1;
/* If non-NULL, returns the content-type of the response. */
struct strbuf *content_type;
diff --git a/remote-curl.c b/remote-curl.c
index dd63bc2..89bf4ea 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -369,6 +369,8 @@ struct rpc_state {
struct strbuf result;
unsigned gzip_request : 1;
unsigned initial_buffer : 1;
+ /* Automatic authentication didn't work, so don't try it again. */
+ unsigned no_passwordless_auth : 1;
};
static size_t rpc_out(void *ptr, size_t eltsize,
@@ -467,6 +469,9 @@ static int probe_rpc(struct rpc_state *rpc, struct
slot_results *results)
curl_easy_setopt(slot-curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
curl_easy_setopt(slot-curl, CURLOPT_FILE, buf);
+ if (rpc-no_passwordless_auth)
+ disable_passwordless_auth(slot);
+
err = run_slot(slot, results);
curl_slist_free_all(headers);
@@ -510,8 +515,10 @@ static int post_rpc(struct rpc_state *rpc)
do {
err = probe_rpc(rpc, results);
- if (err == HTTP_REAUTH)
+ if (err == HTTP_REAUTH) {
credential_fill(http_auth);
+ rpc-no_passwordless_auth = 1;
+ }
} while (err == HTTP_REAUTH);
if (err != HTTP_OK)
return -1;
@@ -533,6 +540,9 @@ retry:
curl_easy_setopt(slot-curl, CURLOPT_URL, rpc-service_url);
[PATCH] clean: style fix for 9f93e46 (git-clean: use a git-add-interactive ...)
From: Junio C Hamano gits...@pobox.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
This is in the SQUASH??? commit on nd/untracked-cache on 'pu', but it
fixes a commit older than that series.
builtin/clean.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 1032563..3beeea6 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -67,7 +67,7 @@ struct menu_item {
char hotkey;
const char *title;
int selected;
- int (*fn)();
+ int (*fn)(void);
};
enum menu_stuff_type {
--
2.2.0.84.ge9c7a8a
--
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 v3] doc: core.ignoreStat update, and clarify the --assume-unchanged effect
The assume-unchanged bit, and consequently core.ignoreStat, can be misunderstood. Be assertive about the expectation that file changes should notified to Git. Overhaul the general wording thus: 1. direct description of what is ignored given first. 2. example instruction of the user manual action required. 3. use sideways indirection for assume-unchanged and update-index references. 4. add a 'normally' to give leeway for the change detection. Signed-off-by: Philip Oakley philipoak...@iee.org --- This is the corrected patch which now applies on top of next and has been checked on AsciiDoc. (was $gmane/261974/focus=262022) I have included the previous 'after three-dashes' comment and included them in the commit message. I'm happy for it to be tweaked as appropriate. --- Documentation/config.txt | 21 - 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 52eeadd..fe179d0 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -375,15 +375,18 @@ This is useful for excluding servers inside a firewall from proxy use, while defaulting to a common proxy for external domains. core.ignoreStat:: - If true, commands which modify both the working tree and the index - will mark the updated paths with the assume unchanged bit in the - index. These marked files are then expected to stay unchanged in the - working tree. If you change them you should mark their update manually. - Git will normally not detect the file changes by lstat() calls. - This is useful on systems where those calls are very slow, such as - cifs/Microsoft Windows. - See linkgit:git-update-index[1]. - False by default. + If true, Git will avoid using lstat() calls to detect if files have + changed. Git will set the assume-unchanged bit for those tracked files + which it has updated identically in both the index and working tree. ++ +When files are modified outside of Git, the user will need to stage +the modified files explicitly (e.g. see 'Examples' section in +linkgit:git-update-index[1]). +Git will not normally detect changes to those files. ++ +This is useful on systems where lstat() calls are very slow, such as +CIFS/Microsoft Windows. +False by default. core.preferSymlinkRefs:: Instead of the default symref format for HEAD -- 1.9.5.msysgit.0 -- 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: git 2.2.x: Unexpected, overstrict file permissions after git update-server-info
On 2015-01-05 20.07, Paul Sokolovsky wrote: Hello, We recently upgraded to git 2.2.1 from 2.1.x and faced issue with accessing repositories over dump HTTP protocol. In our setting, repositories are managed by Gerrit, so owned by Gerrit daemon user, but we also offer anon access via smart and dumb HTTP protocols. For the latter, we of course rely on git update-server-info being run. So, after the upgrade, users started to report that accessing info/refs file of a repo, as required for HTTP dump protocol, leads to 403 Forbidden HTTP error. We traced that to 0600 filesystem permissions for such files (for objects/info/packs too) (owner is gerrit user, to remind). After resetting permissions to 0644, they get back to 0600 after some time (we have a cronjob in addition to a hook to run git update-server-info). umask is permissive when running cronjob (0002). I traced the issue to: https://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec It says: Let's instead switch to using a unique tempfile via mkstemp. Reading man mkstemp: The file is created with permissions 0600. So, that's it. The patch above contains call to adjust_shared_perm(), but apparently it doesn't promote restrictive msktemp permissions to something more accessible. Hope this issue can be addressed. Thanks, Paul Does git config core.sharedRepository 0644 help? Unless the the repo is configured as shared, adjust_shared_perm() will not widen the access bits: http://git-htmldocs.googlecode.com/git/git-config.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 v2] remote-curl: fall back to Basic auth if Negotiate fails
On Mon, Jan 05, 2015 at 09:23:32PM +, Dan Langille (dalangil) wrote: I have tried both patches. Neither succeeds here. I patched git version 2.2.1 but I don’t think that affects this. You are patching the client side, correct? That's the side that needs patching here. Just so the list knows, I will be sending a reroll to the existing patch, but the patches I've posted do indeed work in my testing. Before I flood the list with debug runs, I wanted to make sure I was testing with an appropriate configuration: Location /git SSLOptions +StdenvVars Options +ExecCGI +FollowSymLinks +SymLinksIfOwnerMatch # By default, allow access to anyone. Order allow,deny Allow from All # Enable Kerberos authentication using mod_auth_kerb. AuthType Kerberos AuthName “us.example.org KrbAuthRealms us.example.org # I have tried both with and without the following line: KrbServiceName HTTP/us.example.org Krb5Keytab /usr/local/etc/apache22/repo-test.keytab KrbMethodNegotiate on KrbSaveCredentials on KrbVerifyKDC on KrbServiceName Any # I have tried with and without this line: KrbMethodk5Passwd on Require valid-user /Location I'm not sure why it's not working for you. Here's a snippet from my config: SetEnv GIT_HTTP_EXPORT_ALL 1 SetEnv REMOTE_USER $REDIRECT_REMOTE_USER AuthType Kerberos AuthName Kerberos Login KrbMethodNegotiate on KrbMethodK5Passwd off KrbAuthRealms CRUSTYTOOTHPASTE.NET Krb5Keytab /etc/krb5.apache.keytab When I was testing, I set KrbMethodK5Passwd to on and it did in fact work. I'm using Debian's Apache 2.4.10-9 with mod_auth_kerb 5.4-2.2. -- 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
Question about the revision walking API
Hi, I would like to know if the revision walking API works as one would expect with a calling sequence like the following: - init_revisions - add_pending_object/setup_revisions - prepare_revision_walk - get_revision (repeated) - reset_revision_walk (I guess) - add_pending_object - prepare_revision_walk - get_revision (repeated) That is, do a first revision walk based on a given rev_info, then reuse that rev_info with additional commit objects (in my case, I want to add more UNINTERESTING commits) and redo a revision walk based on the modified rev_info (so, avoid reinitializing a rev_info and filling it from scratch again with the additional UNINTERESTING commits). I guess I could try and see if that works, but I'd rather have an informed answer than to derive my own from the fact my testcase would happen to work by chance. Cheers, 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: git 2.2.x: Unexpected, overstrict file permissions after git update-server-info
On Mon, Jan 05, 2015 at 09:07:24PM +0200, Paul Sokolovsky wrote: So, after the upgrade, users started to report that accessing info/refs file of a repo, as required for HTTP dump protocol, leads to 403 Forbidden HTTP error. We traced that to 0600 filesystem permissions for such files (for objects/info/packs too) (owner is gerrit user, to remind). After resetting permissions to 0644, they get back to 0600 after some time (we have a cronjob in addition to a hook to run git update-server-info). umask is permissive when running cronjob (0002). I traced the issue to: https://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec Yeah, I didn't consider the mode impact of using mkstemp. That is definitely a regression that should be fixed. Though of course if you really do want 0644, you should set your umask to 0022. :) It says: Let's instead switch to using a unique tempfile via mkstemp. Reading man mkstemp: The file is created with permissions 0600. So, that's it. The patch above contains call to adjust_shared_perm(), but apparently it doesn't promote restrictive msktemp permissions to something more accessible. If you haven't set core.sharedrepository, then adjust_shared_perm is a noop. But you shouldn't have to do that. Git should just respect your umask in this case. Hope this issue can be addressed. Patches to follow. Thanks for the report. [1/2]: t1301: set umask in reflog sharedrepository=group test [2/2]: update-server-info: create info/* with mode 0666 -Peff -- 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 1/2] t1301: set umask in reflog sharedrepository=group test
The t1301 script sets the umask globally before many of the tests. Most of the tests that care about the umask then set it explicitly at the start of the test. However, one test does not, and relies on the 077 umask setting from earlier tests. This is fragile and can break if another test is added in between. Let's be more explicit. Signed-off-by: Jeff King p...@peff.net --- I suspect the world would be a better place if t1301 did all of its umask setting in subshells, as it may also affect things like writing out the test results. But nobody has complained, so I'm not inclined to spend a lot of time futzing with it. This is enough to protect the test I'm about to add in the next patch, so it's not worse than the status quo. t/t1301-shared-repo.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index de42d21..86ed901 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -112,6 +112,7 @@ do done test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' ' + umask 077 git config core.sharedRepository group git reflog expire --all actual=$(ls -l .git/logs/refs/heads/master) -- 2.2.1.425.g441bb3c -- 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 2/2] update-server-info: create info/* with mode 0666
Prior to d38379e (make update-server-info more robust, 2014-09-13), we used a straight fopen to create the info/refs and objects/info/packs files, which creates the file using mode 0666 (less the default umask). In d38379e, we switched to creating the file with mkstemp to get a unique filename. But mkstemp also uses the more restrictive 0600 mode to create the file. This was an unintended side effect that we did not want, and causes problems when the repository is served by a different user than the one running update-server-info (it is no longer readable by a dumb http server running as `www`, for example). We can fix this by using git_mkstemp_mode and specifying 0666. Note that we could also say just use core.sharedrepository, as we do call adjust_shared_perm on the result before renaming it into place. But that is not very friendly. The shared-repo config is usually about making things _writable_ for other users. Until d38379e, there was no explicit config needed to serve an otherwise readable repository, and we should consider it a regression. Signed-off-by: Jeff King p...@peff.net --- server-info.c | 2 +- t/t1301-shared-repo.sh | 10 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/server-info.c b/server-info.c index 31f4a74..34b0253 100644 --- a/server-info.c +++ b/server-info.c @@ -17,7 +17,7 @@ static int update_info_file(char *path, int (*generate)(FILE *)) FILE *fp = NULL; safe_create_leading_directories(path); - fd = mkstemp(tmp); + fd = git_mkstemp_mode(tmp, 0666); if (fd 0) goto out; fp = fdopen(fd, w); diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index 86ed901..feff55e 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -111,6 +111,16 @@ do done +test_expect_success POSIXPERM 'info/refs is readable in unshared repo' ' + rm -f .git/info/refs + test_unconfig core.sharedrepository + umask 002 + git update-server-info + echo -rw-rw-r-- expect + modebits .git/info/refs actual + test_cmp expect actual +' + test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' ' umask 077 git config core.sharedRepository group -- 2.2.1.425.g441bb3c -- 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: bearer token authorization with HTTPS transport
On Tue, Dec 30, 2014 at 08:42:10PM -0500, Jeff King wrote: Another option would be to just teach the credential code to accept a bearer field from a credential helper. We would need to: 1. Teach the credential code that getting a bearer token is sufficient (it does not need to prompt for username/password). 2. Teach the http code to pull the bearer field out and stick it in an Authorization header. For the benefit of someone trying to implement this, libcurl has the CURLOPT_XOAUTH2_BEARER option. It does requires libcurl 7.33, though. -- 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: [PATCHv9 1/9] receive-pack.c: shorten the execute_commands loop over all commands
On Sat, Jan 3, 2015 at 1:53 AM, Duy Nguyen pclo...@gmail.com wrote: On Sat, Jan 3, 2015 at 9:20 AM, Jonathan Nieder jrnie...@gmail.com wrote: - if (shallow_update !checked_connectivity) - error(BUG: run 'git fsck' for safety.\n - If there are errors, try to remove - the reported refs above); + if (shallow_update) + check_shallow_bugs(commands, si); In the same spirit of saving the reader from having to look at the body of check_shallow_bugs, would it make sense for the part that reacts to an error to be kept in the caller? E.g.: if (shallow_update warn_if_skipped_connectivity_check(commands, si)) The intention of that patch is to move the code unrelated to executing commands out of execute_commands. And I feel this error checking is not the core task of executing commands, hence it should be moved out completely. Having one part in warn_if_skipped_connectivity_check and then the other error part triggered by an unsuccessful return doesn't improve the situation IMHO. I think about moving the check for shallow_update inside that function and to have a if (!shallow_update) return; and additionally renaming the function to be more precise: assure_shallow_connectivity_checked(); These changes I can put into this patch. Replacing error by a die command will go in an extra patch on top. So I am understanding your answers such that we definitely want to prevent a push if this future bug happen. Instead of incorporating that into later patches of the series to abort in case of this possible bug, we could just go with s/error/die/ and the problem is solved. error(BUG: run 'git fsck for safety.\n If there are errors, try removing the refs reported above); Is this error possible, by the way? That code is to prevent bugs in future. The whole operation is spread out in many functions/steps and people may overlook. Then this patch also helps with introducing a dedicated function to assure the connectivity which we can reuse maybe. 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
Re: What's cooking in git.git (Dec 2014, #05; Mon, 29)
Stefan Beller wrote: On Mon, Dec 29, 2014 at 1:28 PM, Junio C Hamano gits...@pobox.com wrote: * sb/copy-fd-errno (2014-11-17) 1 commit - copy.c: make copy_fd preserve meaningful errno Will be rerolled as a part of a larger series. I am not expecting to reroll this as part of a larger series any more. So if this makes sense on its own, please pick it up? I think the errno-based approach is problematic. No callers today run into the problem the patch fixes and if a caller did, I fear that caller would be broken in other ways. That said, no one seemed too excited about the alternative that propagates the error message more explicitly with a strbuf. I'd be happy to re-send anyway, or I can work around this patch when rerolling. Thanks, Jonathan -- 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: [PATCHv10 02/10] receive-pack.c: die instead of error in assure_connectivity_checked
Stefan Beller wrote:
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1055,15 +1055,15 @@ static void assure_connectivity_checked(struct
command *commands,
for (cmd = commands; cmd; cmd = cmd-next) {
if (should_process_cmd(cmd) si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on ref
%s,
- cmd-ref_name);
+ die(BUG: connectivity check has not been run on ref
%s,
+ cmd-ref_name);
This could stay as error() so that the caller gets to see the list of
refs that didn't experience a connectivity check. Or if that list
isn't important, this error/die call could be dropped and the
'checked_connectivity = 0' setting would be enough.
checked_connectivity = 0;
}
}
if (!checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ die(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
I find this error message misleading and confusing. It makes it seem
like this is an expected error that we are trying to help the user
recover from. Instead, something impossible has happened. Try to
remove the reported refs is actively harmful advice --- that would be
a way for the user to work around a serious bug instead of figuring
out what went wrong and getting git fixed.
Jonathan
--
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: [PATCHv10 01/10] receive-pack.c: shorten the execute_commands loop over all commands
Stefan Beller wrote: --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c [...] @@ -1077,27 +1100,15 @@ static void execute_commands(struct command *commands, [...] + if (shallow_update) + assure_connectivity_checked(commands, si); Looking at this code alone, it seems like assure_connectivity_checked() is going to ensure that connectivity was checked, so that I can assume connectivity going forward. But the opposite is true --- it is a safety check that prints a warning and doesn't affect what I can assume. The factored-out function fails in what it is meant to do, which is to save the reader of execute_commands from having to look at the implementation of the parts they are not interested in. Would something like warn_if_skipped_connectivity_check() make sense? Jonathan -- 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
[PATCHv10 01/10] receive-pack.c: shorten the execute_commands loop over all commands
Make the main execute_commands loop in receive-pack easier to read
by splitting out some steps into helper functions. The new helper
'should_process_cmd' checks if a ref update is unnecessary, whether
due to an error having occurred or for another reason. The helper
'assure_connectivity_checked' warns if we have forgotten to run a
connectivity check on a ref which is shallow for the client which
would be a bug.
This will help us to duplicate less code in a later patch when we make
a second copy of the execute_commands loop.
No functional change intended.
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v10:
* rename check_shallow_bugs to assure_connectivity_checked.
* reword commit message.
v9:
* simplified should_process_cmd to be a one liner
* check_shallow_bugs doesn't check of shallow_update being set, rather
the function is just called if that option is set.
v8: no change
v7:
new in v7 as in v7 I'd split up the previous
[PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction
for atomic pushes
as suggested by Eric.
This is pretty much
patch 1: Factor out code into helper functions which will be needed by
the upcoming atomic and non-atomic worker functions. Example helpers:
'cmd-error_string' and cmd-skip_update' check; and the
'si-shallow_ref[cmd-index]' check and handling.
builtin/receive-pack.c | 43 +++
1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..ed428e4 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1042,11 +1042,34 @@ static void reject_updates_to_hidden(struct command
*commands)
}
}
+static int should_process_cmd(struct command *cmd)
+{
+ return !cmd-error_string !cmd-skip_update;
+}
+
+static void assure_connectivity_checked(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ int checked_connectivity = 1;
+
+ for (cmd = commands; cmd; cmd = cmd-next) {
+ if (should_process_cmd(cmd) si-shallow_ref[cmd-index]) {
+ error(BUG: connectivity check has not been run on ref
%s,
+ cmd-ref_name);
+ checked_connectivity = 0;
+ }
+ }
+ if (!checked_connectivity)
+ error(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
+}
+
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si)
{
- int checked_connectivity;
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
@@ -1077,27 +1100,15 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup(HEAD, 0, sha1, NULL);
- checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd-next) {
- if (cmd-error_string)
- continue;
-
- if (cmd-skip_update)
+ if (!should_process_cmd(cmd))
continue;
cmd-error_string = update(cmd, si);
- if (shallow_update !cmd-error_string
- si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on ref
%s,
- cmd-ref_name);
- checked_connectivity = 0;
- }
}
- if (shallow_update !checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ if (shallow_update)
+ assure_connectivity_checked(commands, si);
}
static struct command **queue_command(struct command **tail,
--
2.2.1.62.g3f15098
--
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
[PATCHv10 02/10] receive-pack.c: die instead of error in assure_connectivity_checked
Discussion on the previous patch revealed we rather want to err on the
safe side. To do so we need to stop receive-pack in case of the possible
future bug when connectivity is not checked on a shallow push.
Suggested-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v10:
* new in v10.
builtin/receive-pack.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ed428e4..7ebea7f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1055,15 +1055,15 @@ static void assure_connectivity_checked(struct command
*commands,
for (cmd = commands; cmd; cmd = cmd-next) {
if (should_process_cmd(cmd) si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on ref
%s,
- cmd-ref_name);
+ die(BUG: connectivity check has not been run on ref
%s,
+ cmd-ref_name);
checked_connectivity = 0;
}
}
if (!checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ die(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
}
static void execute_commands(struct command *commands,
--
2.2.1.62.g3f15098
--
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
git 2.2.x: Unexpected, overstrict file permissions after git update-server-info
Hello, We recently upgraded to git 2.2.1 from 2.1.x and faced issue with accessing repositories over dump HTTP protocol. In our setting, repositories are managed by Gerrit, so owned by Gerrit daemon user, but we also offer anon access via smart and dumb HTTP protocols. For the latter, we of course rely on git update-server-info being run. So, after the upgrade, users started to report that accessing info/refs file of a repo, as required for HTTP dump protocol, leads to 403 Forbidden HTTP error. We traced that to 0600 filesystem permissions for such files (for objects/info/packs too) (owner is gerrit user, to remind). After resetting permissions to 0644, they get back to 0600 after some time (we have a cronjob in addition to a hook to run git update-server-info). umask is permissive when running cronjob (0002). I traced the issue to: https://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec It says: Let's instead switch to using a unique tempfile via mkstemp. Reading man mkstemp: The file is created with permissions 0600. So, that's it. The patch above contains call to adjust_shared_perm(), but apparently it doesn't promote restrictive msktemp permissions to something more accessible. Hope this issue can be addressed. Thanks, Paul Linaro.org | Open source software for ARM SoCs Follow Linaro: http://www.facebook.com/pages/Linaro http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog -- 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
[RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body
When sending out patch series one of the last things doing is writing
the cover letter. The cover letter would be a good place to remind
people to check the todo list for sending patches. As people have
different levels of confidence and sloppiness this todo list may be
lengthier for some people compared to others. To make this possible
this adds a way to put your personal todo list for sending patches in
the cover letter, so you'll see it every time you intend to send patches.
This intentionally doesn't let you configure the subject line of the cover
letter
as send email will stop you if you want to send out the coverletter with
untouched
subject line (*** SUBJECT HERE***).
Not-signed-off-by: Stefan Beller sbel...@google.com as it's RFC.
---
builtin/log.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..84da54d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -34,6 +34,7 @@ static int default_show_root = 1;
static int decoration_style;
static int decoration_given;
static int use_mailmap_config;
+static const char *fmt_patch_body_text = *** BLURB HERE ***;
static const char *fmt_patch_subject_prefix = PATCH;
static const char *fmt_pretty;
@@ -374,6 +375,8 @@ static int git_log_config(const char *var, const char
*value, void *cb)
return git_config_string(fmt_pretty, var, value);
if (!strcmp(var, format.subjectprefix))
return git_config_string(fmt_patch_subject_prefix, var, value);
+ if (!strcmp(var, format.coverbodytext))
+ return git_config_string(fmt_patch_body_text, var, value);
if (!strcmp(var, log.abbrevcommit)) {
default_abbrev_commit = git_config_bool(var, value);
return 0;
@@ -904,8 +907,8 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
int quiet)
{
const char *committer;
- const char *body = *** SUBJECT HERE ***\n\n*** BLURB HERE ***\n;
- const char *msg;
+ const char *subject = *** SUBJECT HERE ***\n\n;
+ struct strbuf msg = STRBUF_INIT;
struct shortlog log;
struct strbuf sb = STRBUF_INIT;
int i;
@@ -937,16 +940,18 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
if (!branch_name)
branch_name = find_branch_name(rev);
- msg = body;
+ strbuf_addstr(msg, subject);
+ strbuf_addstr(msg, fmt_patch_body_text);
pp.fmt = CMIT_FMT_EMAIL;
pp.date_mode = DATE_RFC2822;
pp_user_info(pp, NULL, sb, committer, encoding);
- pp_title_line(pp, msg, sb, encoding, need_8bit_cte);
- pp_remainder(pp, msg, sb, 0);
+ pp_title_line(pp, msg.buf, sb, encoding, need_8bit_cte);
+ pp_remainder(pp, msg.buf, sb, 0);
add_branch_description(sb, branch_name);
printf(%s\n, sb.buf);
strbuf_release(sb);
+ strbuf_release(msg);
shortlog_init(log);
log.wrap_lines = 1;
--
2.2.1.62.g3f15098
--
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] test-lib.sh: do tests for color support after changing HOME
If ncurses needs ~/.terminfo for the current $TERM, then tput will
succeed before changing HOME to $TRASH_DIRECTORY but fail afterward.
Move the tests that determine whether there is color support after
changing HOME so that color=t is set if and only if tput would succeed
when say_color() is run.
This disables color support for those that need ~/.terminfo for their
TERM, but it's better than filling the screen with:
tput: unknown terminal custom-terminal-name-here
An alternative would be to symlink or copy the user's terminfo
database into $TRASH_DIRECTORY, but this is tricky due to the lack of
a standard name for the terminfo database (for example, instead of a
~/.terminfo directory, NetBSD uses a ~/.terminfo.cdb database file).
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/test-lib.sh | 90 +++
1 file changed, 47 insertions(+), 43 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9acdc88..65ecbed 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -184,16 +184,8 @@ export _x05 _x40 _z40 LF u200c
# This test checks if command xyzzy does the right thing...
# '
# . ./test-lib.sh
-[ x$ORIGINAL_TERM != xdumb ] (
- TERM=$ORIGINAL_TERM
- export TERM
- [ -t 1 ]
- tput bold /dev/null 21
- tput setaf 1 /dev/null 21
- tput sgr0 /dev/null 21
- )
- color=t
+unset color
while test $# -ne 0
do
case $1 in
@@ -258,40 +250,6 @@ then
verbose=t
fi
-if test -n $color
-then
- say_color () {
- (
- TERM=$ORIGINAL_TERM
- export TERM
- case $1 in
- error)
- tput bold; tput setaf 1;; # bold red
- skip)
- tput setaf 4;; # blue
- warn)
- tput setaf 3;; # brown/yellow
- pass)
- tput setaf 2;; # green
- info)
- tput setaf 6;; # cyan
- *)
- test -n $quiet return;;
- esac
- shift
- printf %s $*
- tput sgr0
- echo
- )
- }
-else
- say_color() {
- test -z $1 test -n $quiet return
- shift
- printf %s\n $*
- }
-fi
-
error () {
say_color error error: $*
GIT_EXIT_OK=t
@@ -857,6 +815,52 @@ HOME=$TRASH_DIRECTORY
GNUPGHOME=$HOME/gnupg-home-not-used
export HOME GNUPGHOME
+# run the tput tests *after* changing HOME (in case ncurses needs
+# ~/.terminfo for $TERM)
+test -n ${color+set} || [ x$ORIGINAL_TERM != xdumb ] (
+ TERM=$ORIGINAL_TERM
+ export TERM
+ [ -t 1 ]
+ tput bold /dev/null 21
+ tput setaf 1 /dev/null 21
+ tput sgr0 /dev/null 21
+ )
+ color=t
+
+if test -n $color
+then
+ say_color () {
+ (
+ TERM=$ORIGINAL_TERM
+ export TERM
+ case $1 in
+ error)
+ tput bold; tput setaf 1;; # bold red
+ skip)
+ tput setaf 4;; # blue
+ warn)
+ tput setaf 3;; # brown/yellow
+ pass)
+ tput setaf 2;; # green
+ info)
+ tput setaf 6;; # cyan
+ *)
+ test -n $quiet return;;
+ esac
+ shift
+ printf %s $*
+ tput sgr0
+ echo
+ )
+ }
+else
+ say_color() {
+ test -z $1 test -n $quiet return
+ shift
+ printf %s\n $*
+ }
+fi
+
if test -z $TEST_NO_CREATE_REPO
then
test_create_repo $TRASH_DIRECTORY
--
2.2.1
--
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: What's cooking in git.git (Dec 2014, #05; Mon, 29)
On Mon, Dec 29, 2014 at 1:28 PM, Junio C Hamano gits...@pobox.com wrote: * sb/copy-fd-errno (2014-11-17) 1 commit - copy.c: make copy_fd preserve meaningful errno Will be rerolled as a part of a larger series. I am not expecting to reroll this as part of a larger series any more. So if this makes sense on its own, please pick it up? -- 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 v2] remote-curl: fall back to Basic auth if Negotiate fails
I’ve found the latest patch. Trying this now. Thanks.
—
Dan Langille
Infrastructure Operations
Talos Group
Sourcefire, Inc.
On Jan 1, 2015, at 2:56 PM, brian m. carlson sand...@crustytoothpaste.net
wrote:
Apache servers using mod_auth_kerb can be configured to allow the user
to authenticate either using Negotiate (using the Kerberos ticket) or
Basic authentication (using the Kerberos password). Often, one will
want to use Negotiate authentication if it is available, but fall back
to Basic authentication if the ticket is missing or expired.
However, libcurl will try very hard to use something other than Basic
auth, even over HTTPS. If Basic and something else are offered, libcurl
will never attempt to use Basic, even if the other option fails.
Teach the HTTP client code to stop trying authentication mechanisms that
don't use a password (currently Negotiate) after the first failure,
since if they failed the first time, they will never succeed.
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
http.c| 16
http.h| 3 +++
remote-curl.c | 11 ++-
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/http.c b/http.c
index 040f362..815194d 100644
--- a/http.c
+++ b/http.c
@@ -62,6 +62,8 @@ static const char *user_agent;
static struct credential cert_auth = CREDENTIAL_INIT;
static int ssl_cert_password_required;
+/* Should we allow non-password-based authentication (e.g. GSSAPI)? */
+int http_passwordless_auth = 1;
static struct curl_slist *pragma_header;
static struct curl_slist *no_pragma_header;
@@ -986,6 +988,16 @@ static void extract_content_type(struct strbuf *raw,
struct strbuf *type,
strbuf_addstr(charset, ISO-8859-1);
}
+void disable_passwordless_auth(struct active_request_slot *slot)
+{
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+#define HTTP_AUTH_PASSWORDLESS (CURLAUTH_GSSNEGOTIATE)
+ curl_easy_setopt(slot-curl, CURLOPT_HTTPAUTH,
+ CURLAUTH_ANY ~HTTP_AUTH_PASSWORDLESS);
+#endif
+}
+
+
/* http_request() targets */
#define HTTP_REQUEST_STRBUF 0
#define HTTP_REQUEST_FILE 1
@@ -1035,6 +1047,9 @@ static int http_request(const char *url,
curl_easy_setopt(slot-curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot-curl, CURLOPT_ENCODING, gzip);
+ if (!http_passwordless_auth)
+ disable_passwordless_auth(slot);
+
ret = run_one_slot(slot, results);
if (options options-content_type) {
@@ -1139,6 +1154,7 @@ static int http_request_reauth(const char *url,
}
credential_fill(http_auth);
+ http_passwordless_auth = 0;
return http_request(url, result, target, options);
}
diff --git a/http.h b/http.h
index 473179b..71943d3 100644
--- a/http.h
+++ b/http.h
@@ -98,6 +98,8 @@ extern int handle_curl_result(struct slot_results *results);
int run_one_slot(struct active_request_slot *slot,
struct slot_results *results);
+void disable_passwordless_auth(struct active_request_slot *slot);
+
#ifdef USE_CURL_MULTI
extern void fill_active_slots(void);
extern void add_fill_function(void *data, int (*fill)(void *));
@@ -112,6 +114,7 @@ extern int active_requests;
extern int http_is_verbose;
extern size_t http_post_buffer;
extern struct credential http_auth;
+extern int http_passwordless_auth;
extern char curl_errorstr[CURL_ERROR_SIZE];
diff --git a/remote-curl.c b/remote-curl.c
index dd63bc2..4ca5447 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -467,6 +467,9 @@ static int probe_rpc(struct rpc_state *rpc, struct
slot_results *results)
curl_easy_setopt(slot-curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
curl_easy_setopt(slot-curl, CURLOPT_FILE, buf);
+ if (!http_passwordless_auth)
+ disable_passwordless_auth(slot);
+
err = run_slot(slot, results);
curl_slist_free_all(headers);
@@ -510,8 +513,10 @@ static int post_rpc(struct rpc_state *rpc)
do {
err = probe_rpc(rpc, results);
- if (err == HTTP_REAUTH)
+ if (err == HTTP_REAUTH) {
credential_fill(http_auth);
+ http_passwordless_auth = 0;
+ }
} while (err == HTTP_REAUTH);
if (err != HTTP_OK)
return -1;
@@ -533,6 +538,9 @@ retry:
curl_easy_setopt(slot-curl, CURLOPT_URL, rpc-service_url);
curl_easy_setopt(slot-curl, CURLOPT_ENCODING, gzip);
+ if (!http_passwordless_auth)
+ disable_passwordless_auth(slot);
+
if (large_request) {
/* The request body is large and the size cannot be predicted.
* We must use chunked encoding to send it.
@@ -617,6 +625,7 @@ retry:
err = run_slot(slot, NULL);
if (err == HTTP_REAUTH !large_request) {
Re: [PATCH v2] doc: core.ignoreStat update, and clarify the --assume-unchanged effect
From: Junio C Hamano gits...@pobox.com Philip Oakley philipoak...@iee.org writes: The assume-unchanged bit, and consequently core.ignoreStat, can be misunderstood. Be assertive about the expectation that file changes should notified to Git. Signed-off-by: Philip Oakley philipoak...@iee.org --- Overhaul the general wording thus: 1. direct description of what is ignored given first. 2. example instruction of the user manual action required. 3. use sideways indirection for assume-unchanged and update-index references. 4. add a 'normally' to give leeway for the change detection. This version is on top of the current master, and replaces the patch in next. Sorry, you do not replace anything in 'next', once it is in. My apologies. I'd been too quick to read the 'expecting reroll' to properly notice the promotion to next. I was about to say that I'll try to see if I turn it into incremental (at which point some of the above four points after the three-dash line might want to be in the log message proper), but the AsciiDoc mark-up looks somewhat suspect (we usually do not see indentation in the paragraphs continued with +). Does this format fine for manpage and HTML? I didn't have access to AsciiDoc at the time. You are right, it doesn't format correctly, I shouldn't have continued with the indentation tabs in the continuation paragraphs. My mistake. I can re-roll if that's easiest Thanks. --- Documentation/config.txt | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 6862e3e..32e42dd 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -375,13 +375,17 @@ This is useful for excluding servers inside a firewall from proxy use, while defaulting to a common proxy for external domains. core.ignoreStat:: - If true, commands which modify both the working tree and the index - will mark the updated paths with the assume unchanged bit in the - index. These marked files are then assumed to stay unchanged in the - working tree, until you mark them otherwise manually - Git will not - detect the file changes by lstat() calls. This is useful on systems - where those are very slow, such as Microsoft Windows. - See linkgit:git-update-index[1]. + If true, Git will avoid using lstat() calls to detect if files have + changed. Git will set the assume-unchanged bit for those tracked files + which it has updated identically in both the index and working tree. ++ + When files are modified outside of Git, the user will need to stage + the modified files explicitly (e.g. see 'Examples' section in + linkgit:git-update-index[1]). + Git will not normally detect changes to those files. ++ + This is useful on systems where lstat() calls are very slow, such as + CIFS/Microsoft Windows. False by default. core.preferSymlinkRefs:: -- Philip -- 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: [PATCHv10 01/10] receive-pack.c: shorten the execute_commands loop over all commands
On Mon, Jan 5, 2015 at 12:22 PM, Jonathan Nieder jrnie...@gmail.com wrote: Stefan Beller wrote: --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c [...] @@ -1077,27 +1100,15 @@ static void execute_commands(struct command *commands, [...] + if (shallow_update) + assure_connectivity_checked(commands, si); Looking at this code alone, it seems like assure_connectivity_checked() is going to ensure that connectivity was checked, so that I can assume connectivity going forward. But the opposite is true --- it is a safety check that prints a warning and doesn't affect what I can assume. I disagree on that. Combined with the next patch (s/error/die/) we can assume that the the connectivity is there as if it is not, git is dead. This is why I choose the word assure. Maybe check_assumption would be better? The factored-out function fails in what it is meant to do, which is to save the reader of execute_commands from having to look at the implementation of the parts they are not interested in. Would something like warn_if_skipped_connectivity_check() make sense? The next patch would then change this to die_if_... ? I'd be ok with that, but in your original email you would still have the last die(...) in the execute_command function which I dislike. So what about: if (shallow_update) (warn|die)_on_skipped_connectivity_check() ? Jonathan -- 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: [PATCHv10 02/10] receive-pack.c: die instead of error in assure_connectivity_checked
On Mon, Jan 5, 2015 at 12:17 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Stefan Beller wrote:
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1055,15 +1055,15 @@ static void assure_connectivity_checked(struct
command *commands,
for (cmd = commands; cmd; cmd = cmd-next) {
if (should_process_cmd(cmd) si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on ref
%s,
- cmd-ref_name);
+ die(BUG: connectivity check has not been run on ref
%s,
+ cmd-ref_name);
This could stay as error() so that the caller gets to see the list of
refs that didn't experience a connectivity check. Or if that list
isn't important, this error/die call could be dropped and the
'checked_connectivity = 0' setting would be enough.
Right. I was once again writing patches without brain activity.
So we'd keep this as an error.
checked_connectivity = 0;
}
}
if (!checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ die(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
I find this error message misleading and confusing. It makes it seem
like this is an expected error that we are trying to help the user
recover from. Instead, something impossible has happened. Try to
remove the reported refs is actively harmful advice --- that would be
a way for the user to work around a serious bug instead of figuring
out what went wrong and getting git fixed.
Maybe we should do both?
die (BUG: Some refs have not been checked for connectivity.
Please contact the git developers (git@vger.kernel.org) and
report the problem. As a workaround run 'git fsck'. If there
are errors, try to remove the reported refs above. (This
may lead to data loss, backup first.)
Just thinking out loud:
We are using die(...) for two kinds of problems. Either because of
some bad condition given to us by the user (wrong/meaningless arguments)
which we then to refuse to accept.
The other case is usually die(BUG: Git is broken in some way) as we're
discussing here. Would it make sense to have an extra die_bug function,
which amends the reported error string by something like
Please contact the git developers (git@vger.kernel.org) and
report the problem.
as above?
Thanks,
Stefan
Jonathan
--
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: [PATCHv10 01/10] receive-pack.c: shorten the execute_commands loop over all commands
Stefan Beller wrote: On Mon, Jan 5, 2015 at 12:22 PM, Jonathan Nieder jrnie...@gmail.com wrote: Stefan Beller wrote: --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c [...] @@ -1077,27 +1100,15 @@ static void execute_commands(struct command *commands, [...] + if (shallow_update) + assure_connectivity_checked(commands, si); Looking at this code alone, it seems like assure_connectivity_checked() is going to ensure that connectivity was checked, so that I can assume connectivity going forward. But the opposite is true --- it is a safety check that prints a warning and doesn't affect what I can assume. I disagree on that. Combined with the next patch (s/error/die/) we can assume that the the connectivity is there as if it is not, git is dead. This is why I choose the word assure. If this patch depends on the next one, would it make sense to put them in the opposite order? The factored-out function fails in what it is meant to do, which is to save the reader of execute_commands from having to look at the implementation of the parts they are not interested in. Would something like warn_if_skipped_connectivity_check() make sense? The next patch would then change this to die_if_... ? I'd be ok with that, but in your original email you would still have the last die(...) in the execute_command function which I dislike. So what about: if (shallow_update) (warn|die)_on_skipped_connectivity_check() ? My personal preference would be to refactor the preceding code to make the check unnecessary. But aside from that, anything that makes the code clearer is fine with me. I find ..._if_... clearer than ..._on_... here because it seems more obvious that it is not an expected condition (i.e., it's a kind of abbreviation for warn_if_skipped_connectivity_check_which_should_never_happen() ) but that's a more minor detail. An alternative way to make the code clearer would be to use a comment. Thanks, Jonathan -- 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 v2] remote-curl: fall back to Basic auth if Negotiate fails
I have tried both patches. Neither succeeds here. I patched git version 2.2.1
but I don’t think that affects this.
Before I flood the list with debug runs, I wanted to make sure I was testing
with an appropriate configuration:
Location /git
SSLOptions +StdenvVars
Options +ExecCGI +FollowSymLinks +SymLinksIfOwnerMatch
# By default, allow access to anyone.
Order allow,deny
Allow from All
# Enable Kerberos authentication using mod_auth_kerb.
AuthType Kerberos
AuthName “us.example.org
KrbAuthRealms us.example.org
# I have tried both with and without the following line:
KrbServiceName HTTP/us.example.org
Krb5Keytab /usr/local/etc/apache22/repo-test.keytab
KrbMethodNegotiate on
KrbSaveCredentials on
KrbVerifyKDC on
KrbServiceName Any
# I have tried with and without this line:
KrbMethodk5Passwd on
Require valid-user
/Location
With a valid ticket, the above works for a git clone.
—
Dan Langille
Infrastructure Operations
Talos Group
Sourcefire, Inc.
On Jan 1, 2015, at 2:56 PM, brian m. carlson sand...@crustytoothpaste.net
wrote:
Apache servers using mod_auth_kerb can be configured to allow the user
to authenticate either using Negotiate (using the Kerberos ticket) or
Basic authentication (using the Kerberos password). Often, one will
want to use Negotiate authentication if it is available, but fall back
to Basic authentication if the ticket is missing or expired.
However, libcurl will try very hard to use something other than Basic
auth, even over HTTPS. If Basic and something else are offered, libcurl
will never attempt to use Basic, even if the other option fails.
Teach the HTTP client code to stop trying authentication mechanisms that
don't use a password (currently Negotiate) after the first failure,
since if they failed the first time, they will never succeed.
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
http.c| 16
http.h| 3 +++
remote-curl.c | 11 ++-
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/http.c b/http.c
index 040f362..815194d 100644
--- a/http.c
+++ b/http.c
@@ -62,6 +62,8 @@ static const char *user_agent;
static struct credential cert_auth = CREDENTIAL_INIT;
static int ssl_cert_password_required;
+/* Should we allow non-password-based authentication (e.g. GSSAPI)? */
+int http_passwordless_auth = 1;
static struct curl_slist *pragma_header;
static struct curl_slist *no_pragma_header;
@@ -986,6 +988,16 @@ static void extract_content_type(struct strbuf *raw,
struct strbuf *type,
strbuf_addstr(charset, ISO-8859-1);
}
+void disable_passwordless_auth(struct active_request_slot *slot)
+{
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+#define HTTP_AUTH_PASSWORDLESS (CURLAUTH_GSSNEGOTIATE)
+ curl_easy_setopt(slot-curl, CURLOPT_HTTPAUTH,
+ CURLAUTH_ANY ~HTTP_AUTH_PASSWORDLESS);
+#endif
+}
+
+
/* http_request() targets */
#define HTTP_REQUEST_STRBUF 0
#define HTTP_REQUEST_FILE 1
@@ -1035,6 +1047,9 @@ static int http_request(const char *url,
curl_easy_setopt(slot-curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot-curl, CURLOPT_ENCODING, gzip);
+ if (!http_passwordless_auth)
+ disable_passwordless_auth(slot);
+
ret = run_one_slot(slot, results);
if (options options-content_type) {
@@ -1139,6 +1154,7 @@ static int http_request_reauth(const char *url,
}
credential_fill(http_auth);
+ http_passwordless_auth = 0;
return http_request(url, result, target, options);
}
diff --git a/http.h b/http.h
index 473179b..71943d3 100644
--- a/http.h
+++ b/http.h
@@ -98,6 +98,8 @@ extern int handle_curl_result(struct slot_results *results);
int run_one_slot(struct active_request_slot *slot,
struct slot_results *results);
+void disable_passwordless_auth(struct active_request_slot *slot);
+
#ifdef USE_CURL_MULTI
extern void fill_active_slots(void);
extern void add_fill_function(void *data, int (*fill)(void *));
@@ -112,6 +114,7 @@ extern int active_requests;
extern int http_is_verbose;
extern size_t http_post_buffer;
extern struct credential http_auth;
+extern int http_passwordless_auth;
extern char curl_errorstr[CURL_ERROR_SIZE];
diff --git a/remote-curl.c b/remote-curl.c
index dd63bc2..4ca5447 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -467,6 +467,9 @@ static int probe_rpc(struct rpc_state *rpc, struct
slot_results *results)
curl_easy_setopt(slot-curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
curl_easy_setopt(slot-curl, CURLOPT_FILE, buf);
+ if (!http_passwordless_auth)
+ disable_passwordless_auth(slot);
+
err = run_slot(slot, results);
curl_slist_free_all(headers);
@@ -510,8 +513,10 @@ static int post_rpc(struct rpc_state *rpc)
do {
Re: [PATCHv10 02/10] receive-pack.c: die instead of error in assure_connectivity_checked
Stefan Beller wrote: Maybe we should do both? die (BUG: Some refs have not been checked for connectivity. Please contact the git developers (git@vger.kernel.org) and report the problem. As a workaround run 'git fsck'. If there are errors, try to remove the reported refs above. (This may lead to data loss, backup first.) I personally find this kind of message grating when I run into it. The message is trying to tell me what to do, but it is not in a position to know what the best thing to do is. It could be that I am using an ancient version of git with known bugs. In that case I should upgrade. It could be that I am using faulty hardware that flips random bits and confuses software. It could be that I have a patched version of git, in which case I should contact the author of my patch instead of git@vger.kernel.org. It could be that this is a recent, terrible regression and git@vger.kernel.org is already bombarded with reports about it. If the message says fatal: BUG: connectivity check skipped??? then it has exactly the right amount of information to tell me what to do. Now I have - a short string to grep for in the source code (or on the web) to find out what happened - a clear indication that This Can't Happen, so I know to try to reproduce it and contact the author of my patched git or upstream or whoever, depending on the context - no irrelevant guesses to confuse me The workaround of running 'git fsck' could be actively harmful, depending on what the bug is. All that we know is that a bug has occured and we shouldn't proceed further. Just thinking out loud: [...] Would it make sense to have an extra die_bug function, Yes, I think something like the kernel's BUG_ON and WARN_ON would be very nice (though orthogonal to this change). Thanks, Jonathan -- 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
