* Re: [RFC/PATCH] remote-curl: Obey passed URL
From: Michael J Gruber @ 2011-10-12 20:51 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce, Tay Ray Chuan
In-Reply-To: <20111006133758.GA18033@sigill.intra.peff.net>
Jeff King venit, vidit, dixit 06.10.2011 15:37:
> On Thu, Oct 06, 2011 at 09:25:00AM -0400, Jeff King wrote:
>
>> Your analysis is correct, but tweaking the remote object seems kind
>> of ugly. I think a nicer solution would be to pass the URL in
>> separately to http_init. Of the three existing callers:
>
> Here's what that patch looks like. It's definitely an improvement and
> fixes a real bug, so it may be worth applying. But I'm still going to
> look into pushing the url examination closer to the point of use.
It definitely is an improvement. I've been running happily with this
(and without my askpass helper/workaround). Are you going forward with
this one?
>
> -- >8 --
> Subject: [PATCH] http_init: accept separate URL parameter
>
> The http_init function takes a "struct remote". Part of its
> initialization procedure is to look at the remote's url and
> grab some auth-related parameters. However, using the url
> included in the remote is:
>
> - wrong; the remote-curl helper may have a separate,
> unrelated URL (e.g., from remote.*.pushurl). Looking at
> the remote's configured url is incorrect.
>
> - incomplete; http-fetch doesn't have a remote, so passes
> NULL. So http_init never gets to see the URL we are
> actually going to use.
>
> - cumbersome; http-push has a similar problem to
> http-fetch, but actually builds a fake remote just to
> pass in the URL.
>
> Instead, let's just add a separate URL parameter to
> http_init, and all three callsites can pass in the
> appropriate information.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> http-fetch.c | 2 +-
> http-push.c | 10 +---------
> http.c | 8 ++++----
> http.h | 2 +-
> remote-curl.c | 2 +-
> 5 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/http-fetch.c b/http-fetch.c
> index 3af4c71..e341872 100644
> --- a/http-fetch.c
> +++ b/http-fetch.c
> @@ -63,7 +63,7 @@ int main(int argc, const char **argv)
>
> git_config(git_default_config, NULL);
>
> - http_init(NULL);
> + http_init(NULL, url);
> walker = get_http_walker(url);
> walker->get_tree = get_tree;
> walker->get_history = get_history;
> diff --git a/http-push.c b/http-push.c
> index 376331a..215b640 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1747,7 +1747,6 @@ int main(int argc, char **argv)
> int i;
> int new_refs;
> struct ref *ref, *local_refs;
> - struct remote *remote;
>
> git_extract_argv0_path(argv[0]);
>
> @@ -1821,14 +1820,7 @@ int main(int argc, char **argv)
>
> memset(remote_dir_exists, -1, 256);
>
> - /*
> - * Create a minimum remote by hand to give to http_init(),
> - * primarily to allow it to look at the URL.
> - */
> - remote = xcalloc(sizeof(*remote), 1);
> - ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
> - remote->url[remote->url_nr++] = repo->url;
> - http_init(remote);
> + http_init(NULL, repo->url);
>
> #ifdef USE_CURL_MULTI
> is_running_queue = 0;
> diff --git a/http.c b/http.c
> index b2ae8de..d9f9938 100644
> --- a/http.c
> +++ b/http.c
> @@ -357,7 +357,7 @@ static void set_from_env(const char **var, const char *envname)
> *var = val;
> }
>
> -void http_init(struct remote *remote)
> +void http_init(struct remote *remote, const char *url)
> {
> char *low_speed_limit;
> char *low_speed_time;
> @@ -421,11 +421,11 @@ void http_init(struct remote *remote)
> if (getenv("GIT_CURL_FTP_NO_EPSV"))
> curl_ftp_no_epsv = 1;
>
> - if (remote && remote->url && remote->url[0]) {
> - http_auth_init(remote->url[0]);
> + if (url) {
> + http_auth_init(url);
> if (!ssl_cert_password_required &&
> getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") &&
> - !prefixcmp(remote->url[0], "https://"))
> + !prefixcmp(url, "https://"))
> ssl_cert_password_required = 1;
> }
>
> diff --git a/http.h b/http.h
> index 0bf8592..3c332a9 100644
> --- a/http.h
> +++ b/http.h
> @@ -86,7 +86,7 @@ struct buffer {
> extern void step_active_slots(void);
> #endif
>
> -extern void http_init(struct remote *remote);
> +extern void http_init(struct remote *remote, const char *url);
> extern void http_cleanup(void);
>
> extern int data_received;
> diff --git a/remote-curl.c b/remote-curl.c
> index 69831e9..33d3d8c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -852,7 +852,7 @@ int main(int argc, const char **argv)
>
> url = strbuf_detach(&buf, NULL);
>
> - http_init(remote);
> + http_init(remote, url);
>
> do {
> if (strbuf_getline(&buf, stdin, '\n') == EOF)
^ permalink raw reply
* Re: [PATCH 1/6] Recognize magic pathspec as filenames
From: Junio C Hamano @ 2011-10-12 20:49 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1318373083-13840-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> .. so that "git log :/" works, not so sure this is correct though
At least the first half should be in the commit log message, and also it
should contrast it against "git log -- :/".
I doubt the approach taken by this particular patch (I do not know about
the rest of the series) is correct, as it breaks the symmetry between
verify_filename() and verify_non_filename().
When given a list of command line arguments, we do:
(1) If there is "--", then no verification is needed. Everything before
the double-dash that is not a "-flag" will be interpreted as revs
(and get_sha1() will error out otherwise), and everything after the
double-dash will be used as an pathspec element.
(2) If there is no "--", then earlier ones must be all revs and cannot be
pathnames in the working tree. The first argument that is not a rev
and everything after that must be a pathname in the working tree, and
must not be interpretable as revs.
That "must be a pathname in the working tree" is what verify_filename()
does. and you say that ":/foo" is OK to be a pathname in this patch.
But shouldn't the opposite "cannot be pathnames in the working tree" check
done by verify_non_filename() also be told the same logic? If ":/foo" is
OK to be a pathname, shouldn't check_filename() call in that function also
be avoided the same way?
And once that happens, I think you will be back to square one.
"git log :/foo" is ambiguous no matter how you slice it, if you are going
to look at only the first character in the string. It could be asking to
show only commits that touch the path in the top-level directory "foo" and
its subdirectories, or it could be asking to start traversal at a commit
with "foo" in the log message.
Longhand magic pathspecs e.g. ":(icase)foo" might have better chance, but
not by a wide margin. It can be a rev that names the blob object in the
index registered for the literal path "'(icase)foo", or it could be an
element in the pathspec to match [Ff][Oo][[Oo].
> setup.c | 16 +++++++---------
> 1 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 27c1d47..08f605b 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -58,15 +58,8 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
> unsigned char sha1[20];
> unsigned mode;
>
> - /*
> - * Saying "'(icase)foo' does not exist in the index" when the
> - * user gave us ":(icase)foo" is just stupid. A magic pathspec
> - * begins with a colon and is followed by a non-alnum; do not
> - * let get_sha1_with_mode_1(only_to_die=1) to even trigger.
> - */
> - if (!(arg[0] == ':' && !isalnum(arg[1])))
> - /* try a detailed diagnostic ... */
> - get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
> + /* try a detailed diagnostic ... */
> + get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
>
> /* ... or fall back the most general message. */
> die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
> @@ -85,6 +78,11 @@ void verify_filename(const char *prefix, const char *arg)
> {
> if (*arg == '-')
> die("bad flag '%s' used after filename", arg);
> +
> + /* If it's magic pathspec, just assume it's file name */
> + if (arg[0] == ':' && is_pathspec_magic(arg[1]))
> + return;
> +
> if (check_filename(prefix, arg))
> return;
> die_verify_filename(prefix, arg);
^ permalink raw reply
* Re: [PATCH v4] attr.c: respect core.ignorecase when matching attribute patterns
From: Junio C Hamano @ 2011-10-12 20:24 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, mhagger, Brandon Casey
In-Reply-To: <y0zSE7mQNTqQ3B_hRG_UvK2pQCTYIP8jr_bGkjb8qdCqyfJ544ZQhQCmwkL-_MxlparVZWmDgqL7VO68y3trgjciKw2QRAK_j7KNVcXXJW0@cipher.nrlssc.navy.mil>
Brandon Casey <casey@nrlssc.navy.mil> writes:
> On 10/11/2011 11:54 AM, Junio C Hamano wrote:
> ...
> Maybe that last paragraph in the commit message should just be dropped.
> I think the preceding paragraph explains the purpose of the tests, and
> this last one doesn't really add any value.
I wasn't saying the description was wrong per-se. It only makes difference
for "git check-attr" users, but it still _does_ make difference for them,
I think. So I kept the paragraph in the end.
> Do you want me to resubmit or can you fix it up? I ask not because I
> am too lazy to do 'commit --amend' myself, but because you may prefer
> to receive one less patch in your inbox if you can easily apply the
> change yourself.
It doesn't make much difference to me, as long as I don't forget a simple
and no-brainer amend I am supposed to do myself ;-).
Thanks.
^ permalink raw reply
* Re: [PATCH v4] attr.c: respect core.ignorecase when matching attribute patterns
From: Brandon Casey @ 2011-10-12 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, mhagger, Brandon Casey
In-Reply-To: <7v8vorh3kg.fsf@alter.siamese.dyndns.org>
On 10/11/2011 11:54 AM, Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> ... Currently, git builds the attr stack
>> based on the path supplied by the user, so we don't have to do anything
>> special (like use strcmp_icase) to handle the parts of that path that don't
>> match the filesystem with respect to case. If git instead built the attr
>> stack by scanning the repository, then the paths in the origin field would
>> not necessarily match the paths supplied by the user.
>
> I find this description somewhat misleading. "check-attr" at the plumbing
> level does take full path from the end user, but a common thing Git does
> is to ask the system to learn the prefix to the current directory with
> getcwd(3) append what fill_directory() enumerates as matching a pathspec
> given by the user with readdir(3) to the prefix to form the full path, and
> then feed that full path to git_check_attr().
>
> Without anybody changing anything, we already do build the attr stack by
> "scanning the repository" in that case, no?
Well, kind of. What I meant by "scanning the repository", was having
two separate mechanisms: one to build the attr stack, and one to scan it
to get the attributes. Right now, the two operations are tied together
and the stack is built as needed, and it is built using the same path
string that the scan operation will use for checking for attributes.
So, the leading paths will match.
When I wrote that commit message, I really was only thinking about a
user-supplied path, but the focus should be on prepare_attr_stack().
The reason the leading paths to a .gitattributes file will necessarily
match is because the attr stack is built using the path supplied to
prepare_attr_stack(), and the same path string is used when scanning
the stack to check for attributes. So each path that is supplied,
regardless of whether its case matches the case in the file system or
in the repository, will have an entry in the attr stack.
Maybe that last paragraph in the commit message should just be dropped.
I think the preceding paragraph explains the purpose of the tests, and
this last one doesn't really add any value.
Do you want me to resubmit or can you fix it up? I ask not because I
am too lazy to do 'commit --amend' myself, but because you may prefer
to receive one less patch in your inbox if you can easily apply the
change yourself.
-Brandon
^ permalink raw reply
* Re: [RFC/PATCH]: reverse bisect v 2.0
From: Jeff King @ 2011-10-12 20:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Andrew Ardill, Christian Couder, Michal Vyskocil, git,
Sverre Rabbelier, Johannes Sixt
In-Reply-To: <7vr52ibydy.fsf@alter.siamese.dyndns.org>
On Tue, Oct 11, 2011 at 09:57:13PM -0700, Junio C Hamano wrote:
> With an obvious addition of non-interactive short-cut subcommands "git
> bisect yes" and "git bisect no", I think --removed= is a much better
> wording than --used-to= I suggested in the discussion.
Agreed.
> I however am still worried about the flipping of the mapping between
> <good,bad> and <yes,no> this design requires. What are we going to do to
> the labels of low-level machinery (i.e $GIT_DIR/refs/bisect/bad and
> $GIT/refs/bisect/good)? They appear in "bisect visualize" and I was hoping
> that it would be simpler in the code if we do not have to change them in
> such a way that depends on this introduced/removed switch, and that was
> the reason why I was trying to see if we can solve this without the
> switchable mapping between <good,bad> and <yes,no>.
Hmm. I hadn't thought about the labels. In a yes/no situation, though,
couldn't you use the labels as the user sees them?
Then it is simply a matter of flipping yes/no inside the bisect script
whenever we interact with the user (i.e., "git bisect yes") or when we
interact with the on-disk labels.
Certainly it's more complex than not allowing reversing, though.
> More specifically, I was hoping that we can rename "good" to "old" and
> "bad" to "new" unconditionally and be done with it. We would ask the user
> "What did the code used to do in the olden days?" and "Does this version
> behave the same as it used to?". The possible answers the user can give
> are "git bisect old" (it behaves the same as the older versions) and "git
> bisect new" (it behaves the same as the newer versions). Then we do not
> have to worry about having to flip the meaning of <yes> and <no> at the UI
> level.
Hmm. I think this is not quite as nice, but it is way simpler. It may be
worth trying for a bit to see how people like it. If they don't, the
cost of failure is that we have to maintain "old/new" forever, even
after we implement a yes/no reversible scheme. But maintaining the
old/new mapping from yes/no would not be any harder than the good/bad
mapping, which we would need to do anyway.
So it sounds like a reasonable first step.
-Peff
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Jeff King @ 2011-10-12 20:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Ilari Liusvaara, Junio C Hamano, Johannes Sixt,
Jonathan Nieder
In-Reply-To: <1317678909-19383-1-git-send-email-pclouds@gmail.com>
On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:
> The message is chosen to avoid leaking information, yet let users know
> that they are deliberately not allowed to use the service, not a fault
> in service configuration or the service itself.
I do think this is an improvement, but I wonder if the verbosity should
be configurable. Then open sites like kernel.org could be friendlier to
their users. Something like this instead:
---
daemon.c | 21 +++++++++++++++++----
1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/daemon.c b/daemon.c
index 4c8346d..ec88fd0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -20,6 +20,7 @@
static int log_syslog;
static int verbose;
static int reuseaddr;
+static int informative_errors;
static const char daemon_usage[] =
"git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
return 0;
}
+static int daemon_error(const char *dir, const char *msg)
+{
+ if (!informative_errors)
+ msg = "access denied";
+ packet_write(1, "ERR %s: %s", dir, msg);
+ return -1;
+}
+
static int run_service(char *dir, struct daemon_service *service)
{
const char *path;
@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
if (!enabled && !service->overridable) {
logerror("'%s': service not enabled.", service->name);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "service not enabled");
}
if (!(path = path_ok(dir)))
- return -1;
+ return daemon_error(dir, "no such repository");
/*
* Security on the cheap.
@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
logerror("'%s': repository not exported.", path);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "repository not exported");
}
if (service->overridable) {
@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
logerror("'%s': service not enabled for '%s'",
service->name, path);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "service not enabled");
}
/*
@@ -1167,6 +1176,10 @@ int main(int argc, char **argv)
make_service_overridable(arg + 18, 0);
continue;
}
+ if (!prefixcmp(arg, "--informative-errors")) {
+ informative_errors = 1;
+ continue;
+ }
if (!strcmp(arg, "--")) {
ok_paths = &argv[i+1];
break;
--
1.7.7.rc2.21.gb9948
^ permalink raw reply related
* Re: [PATCH] Makefile: add a knob to turn off hardlinks within same directory
From: Jonathan Nieder @ 2011-10-12 19:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Bastian Blank, Cedric Staniewski
In-Reply-To: <7vaa969go4.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Wouldn't your use case be better served with
>
> $ tar zcf dist.tar.gz --hard-dereference $list_of_files_to_tar_up
>
> instead?
No, because that duplicates the files instead of making symlinks.
^ permalink raw reply
* Re: [PATCH] Make is_gitfile a non-static generic function
From: Phil Hord @ 2011-10-12 19:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phil Hord, git@vger.kernel.org
In-Reply-To: <7vmxd69j72.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> sez:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> After looking at this patch and the way the other caller in transport.c
>>> uses it, I am more and more convinced that "is_gitfile()" is a stupid and
>>> horrible mistake.
>>
>> I think I misunderstood your objection before. Now I think I
>> understand. Tell me if I am right.
>>
>>
>> I think you mean that instead of this:
>> } else if (is_local(url) && is_file(url) && !is_gitfile(url)) {
>>
>> you would like to see this:
>> } else if (is_local(url) && is_file(url) && is_bundle(url)) {
>>
>> Or maybe even this:
>> } else if (is_bundle(url)) {
>
> Exactly.
I tried to refactor read_bundle_header, but it was too ugly and I don't quite
understand all the error paths. Plus, the whole is_bundle part can be just
10 lines of code. Maybe read_bundle_header() should be shortened slightly to
use is_bundle() for code duplication avoidance.
-- >8 --
Subject: [PATCH] transport: Add/use is_bundle() to identify a url
Transport currently decides that any local url which is a file
must be a bundle. This is wrong now if the local url is a
gitfile, and it will be wrong in the future when some other
exception shows up. Teach transport to verify the file is
really a bundle instead of just assuming it is so.
---
bundle.c | 16 ++++++++++++++++
bundle.h | 1 +
transport.c | 10 +---------
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/bundle.c b/bundle.c
index f82baae..3a64a43 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,6 +23,22 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
list->nr++;
}
+int is_bundle(const char *path)
+{
+ char buffer[100];
+ FILE *ffd = fopen(path, "rb");
+ int ret=1;
+
+ if (!ffd)
+ return 0;
+
+ if (!fgets(buffer, sizeof(buffer), ffd) ||
+ strcmp(buffer, bundle_signature))
+ ret=0;
+ fclose(ffd);
+ return ret;
+}
+
/* returns an fd */
int read_bundle_header(const char *path, struct bundle_header *header)
{
diff --git a/bundle.h b/bundle.h
index c5a22c8..35aa0eb 100644
--- a/bundle.h
+++ b/bundle.h
@@ -14,6 +14,7 @@ struct bundle_header {
struct ref_list references;
};
+int is_bundle(const char *path);
int read_bundle_header(const char *path, struct bundle_header *header);
int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv);
diff --git a/transport.c b/transport.c
index f3195c0..bcd9b74 100644
--- a/transport.c
+++ b/transport.c
@@ -881,14 +881,6 @@ static int is_gitfile(const char *url)
return !prefixcmp(buf, "gitdir: ");
}
-static int is_file(const char *url)
-{
- struct stat buf;
- if (stat(url, &buf))
- return 0;
- return S_ISREG(buf.st_mode);
-}
-
static int external_specification_len(const char *url)
{
return strchr(url, ':') - url;
@@ -929,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->fetch = fetch_objs_via_rsync;
ret->push = rsync_transport_push;
ret->smart_options = NULL;
- } else if (is_local(url) && is_file(url) && !is_gitfile(url)) {
+ } else if (is_local(url) && is_bundle(url)) {
struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
ret->get_refs_list = get_refs_from_bundle;
--
1.7.7.334.g311c9.dirty
^ permalink raw reply related
* Re: [PATCH v3 0/7] Provide API to invalidate refs cache
From: Junio C Hamano @ 2011-10-12 19:28 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> These patches are re-rolled onto master.
Thanks.
^ permalink raw reply
* Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Jeff King @ 2011-10-12 19:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <7vipnu81al.fsf@alter.siamese.dyndns.org>
On Wed, Oct 12, 2011 at 12:20:18PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > We also look at ref_rev_parse_rules in shorten_unambiguous_ref. So even
> > with your patch, I still get the warning with:
> >
> > $ git branch config
> > $ git for-each-ref --format='%(refname:short)' refs/heads/
> >
> > It looks like we also use it in remote.c:count_refspec_match, but I
> > haven't figured out if that can trigger a warning or not.
>
> I do not think this is "do not trigger a warning" exercise. This is "we no
> longer consider names outside refs/ as potential magic refs unless they
> are all uppercase".
>
> If the updated dwim_ref() says $GIT_DIR/frotz will no longer create
> ambiguity with $GIT_DIR/refs/heads/frotz, then refname_match() needs to
> know about it, and cause count_refspec_match() to say that "frotz"
> uniquely names "refs/heads/frotz".
>
> The same stands for shorten_unambiguous_ref(). If $GIT_DIR/frotz no longer
> is ambiguous with $GIT_DIR/refs/heads/frotz, then %(refname:shrot) for
> refs/heads/frotz should yield "frotz", and we should not have to qualify
> it as "heads/frotz".
Absolutely. I was lazy in how I woreded it, but I meant that the warning
was the indicator that we were doing something buggy, not that it was
the bug itself. It just happened to be a convenient way to test. :)
Your most recent patch looks right.
-Peff
^ permalink raw reply
* Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Junio C Hamano @ 2011-10-12 19:20 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <20111011230749.GA29785@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We also look at ref_rev_parse_rules in shorten_unambiguous_ref. So even
> with your patch, I still get the warning with:
>
> $ git branch config
> $ git for-each-ref --format='%(refname:short)' refs/heads/
>
> It looks like we also use it in remote.c:count_refspec_match, but I
> haven't figured out if that can trigger a warning or not.
I do not think this is "do not trigger a warning" exercise. This is "we no
longer consider names outside refs/ as potential magic refs unless they
are all uppercase".
If the updated dwim_ref() says $GIT_DIR/frotz will no longer create
ambiguity with $GIT_DIR/refs/heads/frotz, then refname_match() needs to
know about it, and cause count_refspec_match() to say that "frotz"
uniquely names "refs/heads/frotz".
The same stands for shorten_unambiguous_ref(). If $GIT_DIR/frotz no longer
is ambiguous with $GIT_DIR/refs/heads/frotz, then %(refname:shrot) for
refs/heads/frotz should yield "frotz", and we should not have to qualify
it as "heads/frotz".
^ permalink raw reply
* Re: [PATCH 2/2] t1300: test mixed-case variable retrieval
From: Junio C Hamano @ 2011-10-12 19:19 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, git
In-Reply-To: <20111012183002.GB18948@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I was surprised this wasn't tested anywhere, but I couldn't find any
> such place. I think it makes sense to document the desired behavior in
> the form of a few tests.
Thanks. The patch looks good.
But oh boy the original test in the old style was ugly....
^ permalink raw reply
* Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter
From: Junio C Hamano @ 2011-10-12 19:19 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318445067-19279-3-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Instead of invalidating the ref cache on an all-or-nothing basis,
> allow the cache for individual submodules to be invalidated.
That "allow" does not seem to describe what this patch does. It disallows
the wholesale invalidation and forces the caller to invalidate ref cache
individually.
Probably that is what all the existing callers want, but I would have
expected that an existing feature would be kept, perhaps like this
instead:
if (!submodule) {
struct ref_cache *c;
for (c = ref_cache; c; c = c->next)
clear_ref_cache(c);
} else {
clear_ref_cache(get_ref_cache(submodule);
}
Not a major "vetoing" objection, just a comment.
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 12 ++++--------
> 1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 120b8e4..cc72609 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -202,13 +202,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
> return refs;
> }
>
> -static void invalidate_ref_cache(void)
> +static void invalidate_ref_cache(const char *submodule)
> {
> - struct cached_refs *refs = cached_refs;
> - while (refs) {
> - clear_cached_refs(refs);
> - refs = refs->next;
> - }
> + clear_cached_refs(get_cached_refs(submodule));
> }
^ permalink raw reply
* Re: [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()
From: Junio C Hamano @ 2011-10-12 19:14 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318445067-19279-2-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> It is the cache that is being invalidated, not the references.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> diff --git a/refs.c b/refs.c
> index 9911c97..120b8e4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
> return refs;
> }
>
> -static void invalidate_cached_refs(void)
> +static void invalidate_ref_cache(void)
> {
> struct cached_refs *refs = cached_refs;
> while (refs) {
If you call the operation "invalidate ref_cache", shouldn't the data
structure that holds that cache also be renamed to "struct ref_cache" from
"struct "cached_refs" at the same time?
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2011, #04; Wed, 12)
From: Junio C Hamano @ 2011-10-12 19:05 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111012190213.GA19578@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Oct 12, 2011 at 11:48:48AM -0700, Junio C Hamano wrote:
>
>> * jk/name-hash-dirent (2011-10-07) 1 commit
>> (merged to 'next' on 2011-10-11 at e2ea68b)
>> + fix phantom untracked files when core.ignorecase is set
>
> I didn't see any comment on the original patch, so I assume you're OK
> with the few extra bytes added to each cache entry? Otherwise, I can try
> to retool it to keep the directory entries in a separate hash, so only
> case-insensitive people pay the extra price.
>
> I did a few trivial timings, and the extra bytes didn't seem to make any
> difference.
I tend to agree with you that 8 extra bytes per cache entry wouldn't hurt.
I also suspected that the cost of code complexity coming from both
maintaining and conditionally looking up a separate hash would outweigh
the benefit.
^ permalink raw reply
* Re: [PATCH] Makefile: add a knob to turn off hardlinks within same directory
From: Junio C Hamano @ 2011-10-12 19:02 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Bastian Blank, Cedric Staniewski
In-Reply-To: <20111012083842.GA21969@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Typically someone using this setting would want to set
> NO_CROSS_DIRECTORY_HARDLINKS, too, but that is not enforced, so you
> can make $(bindir)/git and $(gitexecdir)/git into hardlinks to the
> same inode and still make sure your tarball avoids the btrfs limits if
> you want.
>
> Requested-by: Bastian Blank <waldi@debian.org>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Hi,
>
> See <http://bugs.debian.org/642603> for context. Sane?
>
> Makefile | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9afdcf2a..ab64ff4c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -226,6 +226,10 @@ all::
> # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
> # programs as a tar, where bin/ and libexec/ might be on different file systems.
> #
> +# Define NO_HARDLINKS if you plan to distribute the installed programs as a tar
> +# that might be extracted on a filesystem like btrfs that does not cope well
> +# with many links to one inode in one directory.
> +#
Hmm.... I would understand if the change were to eventually remove these
"git-foo" anywhere from the filesystem perhaps after a large version bump
in Git 2.0, but that is not what you are trying to do.
Wouldn't your use case be better served with
$ tar zcf dist.tar.gz --hard-dereference $list_of_files_to_tar_up
instead?
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2011, #04; Wed, 12)
From: Jeff King @ 2011-10-12 19:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vipnu9hbj.fsf@alter.siamese.dyndns.org>
On Wed, Oct 12, 2011 at 11:48:48AM -0700, Junio C Hamano wrote:
> * jk/name-hash-dirent (2011-10-07) 1 commit
> (merged to 'next' on 2011-10-11 at e2ea68b)
> + fix phantom untracked files when core.ignorecase is set
I didn't see any comment on the original patch, so I assume you're OK
with the few extra bytes added to each cache entry? Otherwise, I can try
to retool it to keep the directory entries in a separate hash, so only
case-insensitive people pay the extra price.
I did a few trivial timings, and the extra bytes didn't seem to make any
difference.
-Peff
^ permalink raw reply
* [PATCH v3 6/7] write_ref_sha1(): only invalidate the loose ref cache
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
Since write_ref_sha1() can only write loose refs and cannot write
symbolic refs, there is no need for it to invalidate the packed ref
cache.
Suggested by: Martin Fick <mfick@codeaurora.org>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/refs.c b/refs.c
index 9d962bd..9e9edf7 100644
--- a/refs.c
+++ b/refs.c
@@ -1534,7 +1534,7 @@ int write_ref_sha1(struct ref_lock *lock,
unlock_ref(lock);
return -1;
}
- invalidate_ref_cache(NULL);
+ clear_cached_loose_refs(get_cached_refs(NULL));
if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
(strcmp(lock->ref_name, lock->orig_ref_name) &&
log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
--
1.7.7.rc2
^ permalink raw reply related
* [PATCH v3 7/7] clear_cached_refs(): inline function
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
clear_cached_refs() was only called from one place, so inline it
there.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 10 +++-------
1 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index 9e9edf7..409314d 100644
--- a/refs.c
+++ b/refs.c
@@ -172,12 +172,6 @@ static void clear_cached_loose_refs(struct cached_refs *refs)
refs->did_loose = 0;
}
-static void clear_cached_refs(struct cached_refs *refs)
-{
- clear_cached_packed_refs(refs);
- clear_cached_loose_refs(refs);
-}
-
static struct cached_refs *create_cached_refs(const char *submodule)
{
int len;
@@ -215,7 +209,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
void invalidate_ref_cache(const char *submodule)
{
- clear_cached_refs(get_cached_refs(submodule));
+ struct cached_refs *refs = get_cached_refs(submodule);
+ clear_cached_packed_refs(refs);
+ clear_cached_loose_refs(refs);
}
static void read_packed_refs(FILE *f, struct ref_array *array)
--
1.7.7.rc2
^ permalink raw reply related
* [PATCH v3 5/7] clear_cached_refs(): extract two new functions
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
Extract two new functions from clear_cached_refs():
clear_loose_ref_cache() and clear_packed_ref_cache().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 79e3576..9d962bd 100644
--- a/refs.c
+++ b/refs.c
@@ -158,13 +158,24 @@ static void free_ref_array(struct ref_array *array)
array->refs = NULL;
}
-static void clear_cached_refs(struct cached_refs *refs)
+static void clear_cached_packed_refs(struct cached_refs *refs)
{
- if (refs->did_loose)
- free_ref_array(&refs->loose);
if (refs->did_packed)
free_ref_array(&refs->packed);
- refs->did_loose = refs->did_packed = 0;
+ refs->did_packed = 0;
+}
+
+static void clear_cached_loose_refs(struct cached_refs *refs)
+{
+ if (refs->did_loose)
+ free_ref_array(&refs->loose);
+ refs->did_loose = 0;
+}
+
+static void clear_cached_refs(struct cached_refs *refs)
+{
+ clear_cached_packed_refs(refs);
+ clear_cached_loose_refs(refs);
}
static struct cached_refs *create_cached_refs(const char *submodule)
--
1.7.7.rc2
^ permalink raw reply related
* [PATCH v3 4/7] clear_cached_refs(): rename parameter
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
...for consistency with the rest of this module.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index b08d476..79e3576 100644
--- a/refs.c
+++ b/refs.c
@@ -158,13 +158,13 @@ static void free_ref_array(struct ref_array *array)
array->refs = NULL;
}
-static void clear_cached_refs(struct cached_refs *ca)
+static void clear_cached_refs(struct cached_refs *refs)
{
- if (ca->did_loose)
- free_ref_array(&ca->loose);
- if (ca->did_packed)
- free_ref_array(&ca->packed);
- ca->did_loose = ca->did_packed = 0;
+ if (refs->did_loose)
+ free_ref_array(&refs->loose);
+ if (refs->did_packed)
+ free_ref_array(&refs->packed);
+ refs->did_loose = refs->did_packed = 0;
}
static struct cached_refs *create_cached_refs(const char *submodule)
--
1.7.7.rc2
^ permalink raw reply related
* [PATCH v3 0/7] Provide API to invalidate refs cache
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <7vty7ggzum.fsf@alter.siamese.dyndns.org>
These patches are re-rolled onto master.
This patch series provides an API for external code to invalidate the
ref cache that is used internally to refs.c. It also allows code
*within* refs.c to invalidate only the packed or only the loose refs
for a module/submodule.
IMPORTANT:
I won't myself have time to figure out who, outside of refs.c, has to
*call* invalidate_ref_cache(). The candidates that I know off the top
of my head are git-clone, git-submodule [1], and git-pack-refs. It
would be great if experts in those areas would insert calls to
invalidate_ref_cache() where needed.
Even better would be if the meddlesome code were changed to use the
refs API. I'd be happy to help expanding the refs API if needed to
accommodate your needs.
This is why the API for invalidating only packed or loose refs is
private. After code outside refs.c is changed to use the refs API, it
will get the optimal behavior for free (and at that time
invalidate_ref_cache() can be removed again).
[1] http://marc.info/?l=git&m=131827641227965&w=2
In this mailing list thread, Heiko Voigt stated that git-submodule
does not modify any references, so it should not have to use the
API.
Michael Haggerty (7):
invalidate_ref_cache(): rename function from invalidate_cached_refs()
invalidate_ref_cache(): take the submodule as parameter
invalidate_ref_cache(): expose this function in refs API
clear_cached_refs(): rename parameter
clear_cached_refs(): extract two new functions
write_ref_sha1(): only invalidate the loose ref cache
clear_cached_refs(): inline function
refs.c | 31 +++++++++++++++++--------------
refs.h | 8 ++++++++
2 files changed, 25 insertions(+), 14 deletions(-)
--
1.7.7.rc2
^ permalink raw reply
* [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
Instead of invalidating the ref cache on an all-or-nothing basis,
allow the cache for individual submodules to be invalidated.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 120b8e4..cc72609 100644
--- a/refs.c
+++ b/refs.c
@@ -202,13 +202,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
return refs;
}
-static void invalidate_ref_cache(void)
+static void invalidate_ref_cache(const char *submodule)
{
- struct cached_refs *refs = cached_refs;
- while (refs) {
- clear_cached_refs(refs);
- refs = refs->next;
- }
+ clear_cached_refs(get_cached_refs(submodule));
}
static void read_packed_refs(FILE *f, struct ref_array *array)
@@ -1228,7 +1224,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
ret |= repack_without_ref(refname);
unlink_or_warn(git_path("logs/%s", lock->ref_name));
- invalidate_ref_cache();
+ invalidate_ref_cache(NULL);
unlock_ref(lock);
return ret;
}
@@ -1527,7 +1523,7 @@ int write_ref_sha1(struct ref_lock *lock,
unlock_ref(lock);
return -1;
}
- invalidate_ref_cache();
+ invalidate_ref_cache(NULL);
if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
(strcmp(lock->ref_name, lock->orig_ref_name) &&
log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
--
1.7.7.rc2
^ permalink raw reply related
* [PATCH v3 3/7] invalidate_ref_cache(): expose this function in refs API
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
Make invalidate_ref_cache() an official part of the refs API. It is
currently a fact of life that code outside of refs.c mucks about with
references. This change gives such code a way of informing the refs
module that it should no longer trust its cache.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 2 +-
refs.h | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/refs.c b/refs.c
index cc72609..b08d476 100644
--- a/refs.c
+++ b/refs.c
@@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
return refs;
}
-static void invalidate_ref_cache(const char *submodule)
+void invalidate_ref_cache(const char *submodule)
{
clear_cached_refs(get_cached_refs(submodule));
}
diff --git a/refs.h b/refs.h
index 0229c57..f439c54 100644
--- a/refs.h
+++ b/refs.h
@@ -80,6 +80,14 @@ extern void unlock_ref(struct ref_lock *lock);
/** Writes sha1 into the ref specified by the lock. **/
extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
+/*
+ * Invalidate the reference cache for the specified submodule. Use
+ * submodule=NULL to invalidate the cache for the main module. This
+ * function must be called if references are changed via a mechanism
+ * other than the refs API.
+ */
+extern void invalidate_ref_cache(const char *submodule);
+
/** Setup reflog before using. **/
int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
--
1.7.7.rc2
^ permalink raw reply related
* [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()
From: Michael Haggerty @ 2011-10-12 18:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
It is the cache that is being invalidated, not the references.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 9911c97..120b8e4 100644
--- a/refs.c
+++ b/refs.c
@@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
return refs;
}
-static void invalidate_cached_refs(void)
+static void invalidate_ref_cache(void)
{
struct cached_refs *refs = cached_refs;
while (refs) {
@@ -1228,7 +1228,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
ret |= repack_without_ref(refname);
unlink_or_warn(git_path("logs/%s", lock->ref_name));
- invalidate_cached_refs();
+ invalidate_ref_cache();
unlock_ref(lock);
return ret;
}
@@ -1527,7 +1527,7 @@ int write_ref_sha1(struct ref_lock *lock,
unlock_ref(lock);
return -1;
}
- invalidate_cached_refs();
+ invalidate_ref_cache();
if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
(strcmp(lock->ref_name, lock->orig_ref_name) &&
log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
--
1.7.7.rc2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox