* [PATCH 00/14] Tidying up references code
From: mhagger @ 2011-10-13 7:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
From: Michael Haggerty <mhagger@alum.mit.edu>
This is the next installment of the reference changes that I have been
working on. This batch includes a lot of tidying up in preparation
for the real changes.
The last few patches have a little bit of meat on them. They start
changing the innards of refs.c to work less with strings and more with
objects. This work will continue in later patches with the ultimate
goal of swapping the data structure used to store cached references
out from under the module--changing it from a sorted array of pointers
into a hierarchical tree shaped like the reference namespace
tree.
This patch series should be applied on top of "[PATCH v3] Provide API
to invalidate refs cache". It has textual dependencies on that patch
series, though logically I don't think that they interact.
Michael Haggerty (14):
cache.h: add comments for git_path() and git_path_submodule()
struct ref_list: document name member
refs.c: rename some local "refname" variables
refs: rename some parameters result -> sha1
clear_ref_list(): rename from free_ref_list()
resolve_gitlink_ref(): improve docstring
is_refname_available(): remove the "quiet" argument
parse_ref_line(): add docstring
add_ref(): add docstring
is_dup_ref(): extract function from sort_ref_list()
refs: change signatures of get_packed_refs() and get_loose_refs()
get_ref_dir(): change signature
Pass a (cached_refs *) to the resolve_gitlink_*() functions
resolve_gitlink_ref_recursive(): change to work with struct
cached_refs
cache.h | 15 +++
refs.c | 418 +++++++++++++++++++++++++++++++++-----------------------------
refs.h | 34 +++--
3 files changed, 258 insertions(+), 209 deletions(-)
--
1.7.7.rc2
^ permalink raw reply
* [PATCH 01/14] cache.h: add comments for git_path() and git_path_submodule()
From: mhagger @ 2011-10-13 7:58 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: <1318492715-5931-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
cache.h | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index e39e160..b94855e 100644
--- a/cache.h
+++ b/cache.h
@@ -660,7 +660,22 @@ extern char *git_pathdup(const char *fmt, ...)
/* Return a statically allocated filename matching the sha1 signature */
extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
+
+/*
+ * Return the path of a file within get_git_dir(). The arguments
+ * should be printf-like arguments that produce the filename relative
+ * to get_git_dir(). Return the resulting path, or "/bad-path/" if
+ * there is an error.
+ */
extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
+
+/*
+ * Return the path of a file within the submodule located at path.
+ * The other arguments should be printf-like arguments that produce
+ * the filename relative to "<path>/.git". If "<path>/.git" is a
+ * gitlink file, follow it to find the actual submodule git path.
+ * Return the resulting path, or "/bad-path/" if there is an error.
+ */
extern char *git_path_submodule(const char *path, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
--
1.7.7.rc2
^ permalink raw reply related
* Re: [PATCH] http_init: accept separate URL parameter
From: Michael J Gruber @ 2011-10-13 7:26 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111012224625.GA11408@sigill.intra.peff.net>
Jeff King venit, vidit, dixit 13.10.2011 00:46:
> On Wed, Oct 12, 2011 at 03:38:27PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:
>>> ...
>>>> 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>
>>>
>>> Sorry, I forgot to mention: this is meant to go on top of the
>>> http-auth-keyring topic.
>>
>> Hmm, of course the patch was written to help http-auth-keyring topic, but
>> wouldn't this be an improvement that is general enough? I.e. it could
>> even go to the bottom of the topic, no?
>
> Yes, it could, and probably should. I suspect it might need some
> rebasing to do that.
>
> I'm going to float some other possible designs for the topic as soon as
> I put enough polish on them. So I'll try to move this down when I
> re-roll. In the meantime, if you want to throw it on top, great. If you
> want to ignore it until then, no problem. :)
>
> -Peff
Thanks, Jeff.
To clarify:
Without http-auth-keyring, this helps in the sense that git reads the
username from a user@host URL and asks for the password only. When using
GIT_ASKPASS or such, the askpass helper is called with "Password:" only.
With (parts of) http-auth-keyring, the askpass helper is called with
"Password for:user@host", which helps the user identify the request, and
which helps helpers such as ksshaskpass to store the password with a
meaningful key in a wallet.
I'm not sure whether it's feasible/worth taking these bits of
http-auth-keyring (improved prompt) out and apply them early. That is,
I'm sure it's worth it (it would alleviate the need for credential
helpers for some users at least), I haven't looked at feasibility ;)
Michael
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Nguyen Thai Ngoc Duy @ 2011-10-13 7:02 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, git, Ilari Liusvaara, Junio C Hamano, Johannes Sixt
In-Reply-To: <CACsJy8C6o_-SM4oCM6o5-VDXFy5PBXsE0oL_uhYH1_Zk9h06QQ@mail.gmail.com>
On Thu, Oct 13, 2011 at 5:56 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> Translation could be fun to do
By translation, I don't mean inter-language translation. More like
personification. Instead of "service not enabled" you may want
"service is off, you want to attack me or what?"
--
Duy
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Nguyen Thai Ngoc Duy @ 2011-10-13 6:56 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, git, Ilari Liusvaara, Junio C Hamano, Johannes Sixt
In-Reply-To: <20111013055924.GA24019@elie.hsd1.il.comcast.net>
On Thu, Oct 13, 2011 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyen Thai Ngoc Duy wrote:
>
>> How about allow users to select which messages they want to print? We
>> can even go further, allowing users to specify the messages themselves..
> [...]
>> + { "service not enabled", "message.serviceNotEnabled" },
>> + { "no such repository", "message.noSuchRepository" },
>> + { "repository not exported", "message.repositoryNotExported" },
>
> I administer a private server that is only accessible as "localhost".
> :) This much customization would leave me confused about what the
> right choices are and what the choices mean (even if I were to make
> the server public and start having security worries).
--informative-errors is your friend. All errors are enabled.
> What is the intended use --- translation? The idealist in me thinks
> that should be taken care of on the client side, if at all. (This
> way, we would not be preventing especially friendly clients from
> offering pertinent detailed advice for each error condition.
> Alternatively, maybe some day the protocol will want to provide a way
> for clients to indicate a preferred language and message verbosity.)
Translation could be fun to do, but it's more about how much admins
want to reveal. For example, I may only want to show "service not
enabled" and "no such repository", not the last one, which simply
becomes "access denied".
Again I'm not real admin and this may be just bogus.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/6] Recognize magic pathspec as filenames
From: Junio C Hamano @ 2011-10-13 6:06 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8CY=myfLpAbEA0=LCm+tCgS7jzEAxH3rnwQt4-RXyjW9w@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> It's unfortunate that ":" has so many meanings attached to it.
Yeah, I still recall that there were people who said ":/$path looks
similar to the way how the object at the $path in the index is named" as
if it were an advantage. Maybe they were all misguided ;-).
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2011, #04; Wed, 12)
From: Jakub Narebski @ 2011-10-13 6:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Cord Seele, Cord Seele
In-Reply-To: <7vipnu9hbj.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> * cs/perl-config-path-send-email (2011-09-30) 2 commits
> (merged to 'next' on 2011-10-06 at 93c00f0)
> + use new Git::config_path() for aliasesfile
> + Add Git::config_path()
>
> Originally merged to 'next' on 2011-10-05.
> Will merge to 'master' in the second wave.
What about
. Refactor Git::config_*
from
[PATCH/RFC 3/2] Refactor Git::config_*
http://thread.gmane.org/gmane.comp.version-control.git/182310/focus=183111
Ahhh... sorry, it is marked as an RFC. Should I resend?
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Jonathan Nieder @ 2011-10-13 5:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Jeff King, git, Ilari Liusvaara, Junio C Hamano, Johannes Sixt
In-Reply-To: <20111013044544.GA27890@duynguyen-vnpc.dek-tpc.internal>
Nguyen Thai Ngoc Duy wrote:
> How about allow users to select which messages they want to print? We
> can even go further, allowing users to specify the messages themselves..
[...]
> + { "service not enabled", "message.serviceNotEnabled" },
> + { "no such repository", "message.noSuchRepository" },
> + { "repository not exported", "message.repositoryNotExported" },
I administer a private server that is only accessible as "localhost".
:) This much customization would leave me confused about what the
right choices are and what the choices mean (even if I were to make
the server public and start having security worries).
What is the intended use --- translation? The idealist in me thinks
that should be taken care of on the client side, if at all. (This
way, we would not be preventing especially friendly clients from
offering pertinent detailed advice for each error condition.
Alternatively, maybe some day the protocol will want to provide a way
for clients to indicate a preferred language and message verbosity.)
^ permalink raw reply
* Re: [PATCH 3/3] t5403: do not use access repos with GIT_DIR when worktree is involved
From: Nguyen Thai Ngoc Duy @ 2011-10-13 4:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwrc95xe5.fsf@alter.siamese.dyndns.org>
2011/10/13 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> Setting GIT_DIR alone means worktree is current directory for legacy
>> reasons. Avoid using that, instead go to the worktree and execute
>> commands there.
>>
>> The troublesome command is "GIT_DIR=clone2/.git git add clone2/b". The
>> real worktree is clone2, but that command tells git worktree is $(pwd).
>> What does user expect to add then? Should the new entry in index be "b"
>> or "clone2/b"?
>
> There is no troublesomeness here, as the semantics has been clearly
> defined and (hopefully) stayed constant before the days when GIT_WORK_TREE
> was invented.
It's not wrong per se. The trouble comes from the people who reads the
test. It's not clear the author wants to add "b" or "clone2/b".
Another thing, because "clone2" has .git inside, should "clone2" in
this case be considered a submodule and "git add" refuse to add
anything "clone2/*" but "clone2" itself?
> Unless we are trying to break them without knowing, and declare that we
> deprecated it after the fact, which is not exactly the way we want to
> remove existing (mis)feature.
I had problems with my read_directory() rewrite but it's been some
time since I touched the code. I don't remember what went wrong. Need
to have another look.
--
Duy
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Nguyen Thai Ngoc Duy @ 2011-10-13 4:45 UTC (permalink / raw)
To: Jeff King
Cc: git, Ilari Liusvaara, Junio C Hamano, Johannes Sixt,
Jonathan Nieder
In-Reply-To: <20111012200916.GA1502@sigill.intra.peff.net>
On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:
> 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:
How about allow users to select which messages they want to print? We
can even go further, allowing users to specify the messages themselves..
I don't know. I'm not a real server admin so maybe I'm just too
paranoid. Any admins care to speak up?
On the other hand, grouping all messages at one place may be easier to
audit, even if we don't allow customization.
Anyway, two cents on top of your patch..
-- 8< --
diff --git a/daemon.c b/daemon.c
index ec88fd0..a846ef1 100644
--- a/daemon.c
+++ b/daemon.c
@@ -17,10 +17,25 @@
#define initgroups(x, y) (0) /* nothing */
#endif
+/* Must match messages[] order below */
+#define MSG_SERVICE_NOT_ENABLED 0
+#define MSG_NO_SUCH_REPOSITORY 1
+#define MSG_REPOSITORY_NOT_EXPORTED 2
+
+static struct daemon_message
+{
+ const char *message;
+ const char *config;
+ int enabled;
+} messages[] = {
+ { "service not enabled", "message.serviceNotEnabled" },
+ { "no such repository", "message.noSuchRepository" },
+ { "repository not exported", "message.repositoryNotExported" },
+};
+
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"
@@ -238,20 +253,31 @@ static int service_enabled;
static int git_daemon_config(const char *var, const char *value, void *cb)
{
+ int i;
+
if (!prefixcmp(var, "daemon.") &&
!strcmp(var + 7, service_looking_at->config_name)) {
service_enabled = git_config_bool(var, value);
return 0;
}
+ for (i = 0; i < ARRAY_SIZE(messages); i++)
+ if (!strcmp(var, messages[i].config)) {
+ messages[i].enabled = git_config_bool(var, value);
+ return 0;
+ }
+
/* we are not interested in parsing any other configuration here */
return 0;
}
-static int daemon_error(const char *dir, const char *msg)
+static int daemon_error(const char *dir, int msg_id)
{
- if (!informative_errors)
+ const char *msg;
+ if (!messages[msg_id].enabled)
msg = "access denied";
+ else
+ msg = messages[msg_id].message;
packet_write(1, "ERR %s: %s", dir, msg);
return -1;
}
@@ -266,11 +292,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 daemon_error(dir, "service not enabled");
+ return daemon_error(dir, MSG_SERVICE_NOT_ENABLED);
}
if (!(path = path_ok(dir)))
- return daemon_error(dir, "no such repository");
+ return daemon_error(dir, MSG_NO_SUCH_REPOSITORY);
/*
* Security on the cheap.
@@ -286,7 +312,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 daemon_error(dir, "repository not exported");
+ return daemon_error(dir, MSG_REPOSITORY_NOT_EXPORTED);
}
if (service->overridable) {
@@ -300,7 +326,7 @@ static int run_service(char *dir, struct daemon_service *service)
logerror("'%s': service not enabled for '%s'",
service->name, path);
errno = EACCES;
- return daemon_error(dir, "service not enabled");
+ return daemon_error(dir, MSG_SERVICE_NOT_ENABLED);
}
/*
@@ -1177,7 +1203,9 @@ int main(int argc, char **argv)
continue;
}
if (!prefixcmp(arg, "--informative-errors")) {
- informative_errors = 1;
+ int i;
+ for (i = 0; i < ARRAY_SIZE(messages); i++)
+ messages[i].enabled = 1;
continue;
}
if (!strcmp(arg, "--")) {
-- 8< --
^ permalink raw reply related
* Re: [PATCH 3/3] t5403: do not use access repos with GIT_DIR when worktree is involved
From: Junio C Hamano @ 2011-10-13 4:27 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1318412105-13595-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Setting GIT_DIR alone means worktree is current directory for legacy
> reasons. Avoid using that, instead go to the worktree and execute
> commands there.
>
> The troublesome command is "GIT_DIR=clone2/.git git add clone2/b". The
> real worktree is clone2, but that command tells git worktree is $(pwd).
> What does user expect to add then? Should the new entry in index be "b"
> or "clone2/b"?
There is no troublesomeness here, as the semantics has been clearly
defined and (hopefully) stayed constant before the days when GIT_WORK_TREE
was invented.
Assuming that there is no core.worktree in clone2/.git/config and there is
no GIT_WORK_TREE environment variable, "GIT_DIR=$anything git add
clone2/b" should add a path "clone2/b" to the index controlled by that
$GIT_DIR no matter where $anything is.
I would like to find out the motivation behind this patch. Even though I
find the ancient style stale and unsightly, we would want to keep the
(GIT_DIR is set, GIT_WORK_TREE is not set anywhere) combination working
for people who have work habits (read: old scripts) that rely on it. So we
would discourage new tests from using ancient style, but at the same time,
we would not want to remove _all_ existing ones.
Unless we are trying to break them without knowing, and declare that we
deprecated it after the fact, which is not exactly the way we want to
remove existing (mis)feature.
Same comment applies to the other patch that removes the test that uses
update-index && update-ref combination to a lessor degree.
The [PATCH 2/3] is a genuine improvement, though.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/6] Recognize magic pathspec as filenames
From: Nguyen Thai Ngoc Duy @ 2011-10-13 4:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4nze7x61.fsf@alter.siamese.dyndns.org>
2011/10/13 Junio C Hamano <gitster@pobox.com>:
> "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.
Yeah, I forgot the ":/" ref syntax. But because it's ambiguous anyway,
should we disallow "git log :/foo"? Either "git log :/foo --" or "git
log -- :/foo" is OK.
> 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].
It's unfortunate that ":" has so many meanings attached to it. I hope
that our ambiguation detection code can be smart, based on context, to
avoid unnecessary "--". For example, "git log :(icase)foo" can only
mean pathspec magic (I hope..)
--
Duy
^ permalink raw reply
* Re: [PATCH 1/3] t5403.1: simplify commit creation
From: Nguyen Thai Ngoc Duy @ 2011-10-13 3:54 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Junio C Hamano
In-Reply-To: <4E95A0BF.2060003@viscovery.net>
2011/10/13 Johannes Sixt <j.sixt@viscovery.net>:
> Am 10/12/2011 11:35, schrieb Nguyễn Thái Ngọc Duy:
>> test_expect_success setup '
>> echo Data for commit0. >a &&
>> echo Data for commit0. >b &&
>> - git update-index --add a &&
>> - git update-index --add b &&
>> - tree0=$(git write-tree) &&
>> - commit0=$(echo setup | git commit-tree $tree0) &&
>> - git update-ref refs/heads/master $commit0 &&
>> + git add a b &&
>> + git commit -m setup &&
>> git clone ./. clone1 &&
>> git clone ./. clone2 &&
>> GIT_DIR=clone2/.git git branch new2 &&
>
> I don't think this change is necessary. It doesn't hurt to use plumbing
> commands here and there in the test suite to exercise them to a degree
> that they deserve.
I'm fine either way, for the record.
--
Duy
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Jonathan Nieder @ 2011-10-13 2:14 UTC (permalink / raw)
To: Jeff King
Cc: Nguyễn Thái Ngọc Duy, git, Ilari Liusvaara,
Junio C Hamano, Johannes Sixt, Andreas Ericsson
In-Reply-To: <20111012200916.GA1502@sigill.intra.peff.net>
(+cc: Andreas[*])
Jeff King wrote:
> 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:
FWIW the more verbose version you suggest also sounds fine to me. A
person trying to find the names of local users by checking for
repositories with names like "/home/user" would always receive the
error "no such repository", whether that user exists or not and
whether the actual error encountered was ENOENT, EACCES, lack of git
metadata, or the path running afoul of a whitelist or blacklist.
Either Duy's patch or this patch sounds very good to me. Thanks to
both of you for working on it.
[*] context:
http://thread.gmane.org/gmane.comp.version-control.git/182529/focus=183409
^ permalink raw reply
* Re: [PATCH] http_init: accept separate URL parameter
From: Tay Ray Chuan @ 2011-10-13 2:06 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Michael J Gruber, git
In-Reply-To: <20111012214316.GA4393@sigill.intra.peff.net>
On Thu, Oct 13, 2011 at 5:43 AM, Jeff King <peff@peff.net> wrote:
> 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>
This is excellent.
Acked-by: Tay Ray Chuan <rctay89@gmail.com>
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: [PATCH 2/9] completion: optimize refs completion
From: Junio C Hamano @ 2011-10-13 0:50 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Shawn O. Pearce, Jonathan Nieder
In-Reply-To: <1318085683-29830-3-git-send-email-szeder@ira.uka.de>
SZEDER Gábor <szeder@ira.uka.de> writes:
> After a unique command or option is completed, in most cases it is a
> good thing to add a trailing a space, but sometimes it doesn't makes
s/makes/make/;
> __gitcomp() therefore iterates over all possible completion words it
> got as argument, and checks each word whether a trailing space is
> necessary or not. This is ok for commands, options, etc., i.e. when
> the number of words is relatively small, but can be noticeably slow
> for large number of refs. However, while options might or might not
> need that trailing space, refs are always handled uniformly and always
> get that trailing space (or a trailing '.' for 'git config
> branch.<head>.').
> ...
> So, add a specialized variant of __gitcomp() that only deals with
> possible completion words separated by a newline and uniformly appends
> the trailing space to all words using 'compgen -S' (or any other
> suffix, if specified), so no iteration over all words is done.
s/is done./is needed./;
I think I followed your logic (very well written ;-), but feel somewhat
dirty, as you are conflating the "These things are separated with newlines"
with "These things do not need inspection --- they all need suffix", which
has one obvious drawback --- you may find other class of words that always
want a SP after each of them but the source that generates such a class of
words may not separate the list elements with a newline.
Because a ref cannot have $IFS whitespace in its name anyway, I think you
can rename __gitcomp_nl to a name that conveys more clearly what it does
(i.e. "complete and always append suffix"), drop the IFS fiddling from the
function, and get the same optimization, no?
^ permalink raw reply
* Re: [PATCH 5/6] Convert simple init_pathspec() cases to parse_pathspec()
From: Junio C Hamano @ 2011-10-13 0:29 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1318373083-13840-6-git-send-email-pclouds@gmail.com>
I gave a cursory look up to this one. I didn't read 4/6 very carefully
though.
I think teaching the users of "raw[]" field of "struct pathspec" to use
corresponding "items[]" element should come before this series (up to
5/6). After that, once parse_pathspec() API stabilizes, we should teach
users of get_pathspec() to get and pass around "struct pathspec" and when
we reach the point where we can get rid of use of "raw[]" field (the field
itself can stay to serve as a debugging aid if we choose to), everybody
that uses pathspec would be ready to start taking more magic pathspecs.
Having said that, I think the series itself is going in the right
direction.
Thanks for working on this.
^ permalink raw reply
* Re: [PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs
From: Carlos Martín Nieto @ 2011-10-12 23:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, mathstuf
In-Reply-To: <7vsjmx7uur.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 4652 bytes --]
On Wed, 2011-10-12 at 14:39 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
>
> > -static int prune_refs(struct transport *transport, struct ref *ref_map)
> > +static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
> > {
> > int result = 0;
> > - struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
> > + struct ref *ref, *stale_refs = get_stale_heads(ref_map, refs, ref_count);
>
> So in short, get_state_heads() used to take a ref_map and a remote. The
> ref_map is what we actually observed from the remote after talking
> ls-remote with it. It tried to see if any existing ref in our refspace may
> have come from that remote by inspecting the fetch refspec associated with
> that remote (and the ones that does not exist anymore are queued in the
> stale ref list).
>
> Now get_state_heads() takes a ref_map and <refs, ref_count> (you made the
> patch unnecessarily harder to read by swapping the order of parameters).
> The latter "pair" roughly corresponds to what the "remote" parameter used
> to mean, but instead of using the refspec associated with that remote, we
> would use the refspec used for this particular fetch to determine which
> refs we have are stale.
Right. The only reason that the remote was passed was in order to use
its refspec. The order reversal wasn't on purpose, I'll change that.
>
> > @@ -699,8 +699,12 @@ static int do_fetch(struct transport *transport,
> > free_refs(ref_map);
> > return 1;
> > }
> > - if (prune)
> > - prune_refs(transport, ref_map);
> > + if (prune) {
> > + if (ref_count)
> > + prune_refs(refs, ref_count, ref_map);
> > + else
> > + prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
> > + }
>
> And this is consistent to my two paragraph commentary above.
>
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index f2a9c26..79d898b 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -349,7 +349,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
> > else
> > string_list_append(&states->tracked, abbrev_branch(ref->name));
> > }
> > - stale_refs = get_stale_heads(states->remote, fetch_map);
> > + stale_refs = get_stale_heads(fetch_map, states->remote->fetch,
> > + states->remote->fetch_refspec_nr);
>
> So is this.
>
> > diff --git a/remote.c b/remote.c
> > index b8ecfa5..13c9153 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -1681,36 +1681,84 @@ struct ref *guess_remote_head(const struct ref *head,
> > }
> >
> > struct stale_heads_info {
> > - struct remote *remote;
> > struct string_list *ref_names;
> > struct ref **stale_refs_tail;
> > + struct refspec *refs;
> > + int ref_count;
> > };
> >
> > +/* Returns 0 on success, -1 if it couldn't find a match in the refspecs. */
> > +static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)
> > +{
> > + int i;
> > + struct refspec *refspec;
>
> This function replaces the role remote_find_tracking() used to play in the
> old code and the difference in the behaviour (except the obvious lack of
> "find_src/find_dst") feels gratuitous.
remote_find_tracking wants a remote, and that's what we don't have
anymore. The main reason was that it does "too much". The previous
versions had the callback doing more by itself, so I overlooked the
possibilities of remote_find_tracking when rewriting it. Looking at the
code again, it does look like what we want.
>
> The original code in remote_find_tracking() uses "->pattern" to see if a
> pattern match is necessary, but this scans the refspec for an asterisk,
> assuring a breakage when the refspec language is updated to understand
> other glob magic in the future. Why isn't refspec->pattern used here?
Trees, forest etc. I noticed that a bit late. I have a patch on top of
this one that does use ->pattern, which I was going to ask you to squash
in, but it's moot now, as I need to rewrite the patch anyway.
>
> Can't these two functions share more logic? It appears to me that by
> enhancing the logic here a little bit, it may be possible to implement
> remote_find_tracking() ed in terms of this function as a helper.
Yes, remote_find_tracking should use a version of this function; or
probably better, its loop should become the next version of
find_in_refs, so remote_find_tracking is just a wrapper for when we want
to use the remote's fetch refspec.
I'll resend the series with these changes.
cmn
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: Git attributes ignored for root directory
From: Junio C Hamano @ 2011-10-12 23:12 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Gioele Barabucci, git
In-Reply-To: <4E961626.4030201@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 10/05/2011 07:38 PM, Junio C Hamano wrote:
>> - If the pattern is a single dot and nothing else, it matches everything
>> in the current directory.
>
> This disagrees with shell usage, where "." represents a directory
> itself, not the files within a directory.
Either you misread me, or what I wrote was fuzzy, or perhaps both.
The suggested update to the list of rules very much wants a '.' to mean
the directory itself. The problem I was solving, which turned out to be
something different from the original issue in the thread was this.
Suppose you have a directory "foo" and want to say "I want to ignore
everything in that directory". You would say "foo/" in .gitignore in the
higher level and you are happy.
How would you say the same thing if the directory to be ignored weren't
"foo" but at the top-level of the working tree? There is no such level
higher than the top-level where you can say "<the name of your project>/"
in its .gitignore file.
The best you could do is to say "./" in the .gitignore file at the
top-level directory, and the update rule you quoted is specifically
designed to address it.
Of course, you could list both ".*" and "*" in the .gitignore file at the
top-level directory for the same effect, but that works only because you
do not have to give values to the entry in .gitignore mechanism. It would
be cumbersome to duplicate two entries in .gitattributes file like that as
a workaround.
^ permalink raw reply
* Re: [PATCH] http_init: accept separate URL parameter
From: Jeff King @ 2011-10-12 22:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vk4897s4c.fsf@alter.siamese.dyndns.org>
On Wed, Oct 12, 2011 at 03:38:27PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:
> > ...
> >> 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>
> >
> > Sorry, I forgot to mention: this is meant to go on top of the
> > http-auth-keyring topic.
>
> Hmm, of course the patch was written to help http-auth-keyring topic, but
> wouldn't this be an improvement that is general enough? I.e. it could
> even go to the bottom of the topic, no?
Yes, it could, and probably should. I suspect it might need some
rebasing to do that.
I'm going to float some other possible designs for the topic as soon as
I put enough polish on them. So I'll try to move this down when I
re-roll. In the meantime, if you want to throw it on top, great. If you
want to ignore it until then, no problem. :)
-Peff
^ permalink raw reply
* Re: [PATCH] http_init: accept separate URL parameter
From: Junio C Hamano @ 2011-10-12 22:38 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
In-Reply-To: <20111012214610.GA4578@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:
> ...
>> 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>
>
> Sorry, I forgot to mention: this is meant to go on top of the
> http-auth-keyring topic.
Hmm, of course the patch was written to help http-auth-keyring topic, but
wouldn't this be an improvement that is general enough? I.e. it could
even go to the bottom of the topic, no?
^ permalink raw reply
* Re: Git attributes ignored for root directory
From: Michael Haggerty @ 2011-10-12 22:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Gioele Barabucci, git
In-Reply-To: <7vd3eb8hkb.fsf@alter.siamese.dyndns.org>
On 10/05/2011 07:38 PM, Junio C Hamano wrote:
> - If the pattern is a single dot and nothing else, it matches everything
> in the current directory.
This disagrees with shell usage, where "." represents a directory
itself, not the files within a directory. By the trailing slash rule
that you quoted, "./" should also represent the containing directory.
Given that git currently tries to ignore directories and only think
about files, the consistent thing to do would be to declare "." and "./"
as undefined. Because someday git *may* want to get a bit smarter about
directories, and at that time it would be a shame not to have "." and/or
"./" available to represent the containing directory. (But implementing
it will require some special-casing within the attr module, which is
currently pretty stupid.)
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* 群发软件+买家搜索机+109届广交会买家、海关数据,B2B询盘买家500万。
From: 仅10元每天 @ 2011-10-12 22:33 UTC (permalink / raw)
To: girlsitem, girlygoogietutus, girotto, gisbertrattan,
gisellealcock-ryan, gismbkyfty, git
群发软件+109届广交会买家、海关数据、搜索引擎买家,B2B询盘买家共500万,仅10元每天。
群发软件+109届广交会买家、海关数据、搜索引擎买家,B2B询盘买家共500万,仅10元每天。
保证每天都有买家回复。
保证每天都有买家回复。
保证每天都有买家回复。
1、群发软件： 操作简单，功能强大，模仿人工操作模式，到达率高，日发送5万封以上。
2、500万买家资源： 赠送的500万买家资源库，更新 (可以按照您的产品提取出来，更精准开发)。
3、超级海外买家Email搜索机： 内置了600万行业关键词，根据长尾词搜索，结果更精确匹配；每天能搜索1-2万以上买家真实EMAIL，成单率高。
要的抓紧联系QQ: 1339625218 或者立即回复邮箱: 1339625218@qq.com
要的抓紧联系QQ: 1339625218 或者立即回复邮箱: 1339625218@qq.com
要的抓紧联系QQ: 1339625218 或者立即回复邮箱: 1339625218@qq.com
免费赠送:
一共8个包(数据是全行业的，按照行业分好类，并且可以按照关键词查询的)：
1，2011春季109届广交会买家现场询盘数据库新鲜出炉，超级新鲜买家，新鲜数据，容易成单！
2，购买后可以免费更新2011秋季广交会+2012春季广交会买家数据。太超值了。
3，最新全球买家库,共451660条数据。 (最新更新日期 2011-05-16日)
4，2008年,2009年,2010年 春季+秋季广交会买家名录，103 104 105 106 107 108 共六届 共120.6万数据。
5，2010年国际促销协会（PPAI）成员名单 PPAI Members Directory，非常重要的大买家。
6，2010年到香港采购的国外客人名录(香港贸发局提供)，共7.2万数据，超级重要的买家。
7，48.68万条最新买家询盘，购买后每月更新 1-2万条，包括2部分，1，最新的询盘 2，最新的展会买家。免费更新36个月。
8，2009年海关提单数据piers版数据 1千万。
诚信为本，支持支付宝担保交易 (先发货并安装设置群发软件，然后付款) 彻底打消您的 顾虑。
精准数据-成单率极高
精准数据-成单率极高
精准数据-成单率极高
精准数据-成单率极高
精准数据-成单率极高
^ permalink raw reply
* Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR
From: Junio C Hamano @ 2011-10-12 22:26 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Jeff King, git, cmn, A Large Angry SCM,
Daniel Barkalow, Sverre Rabbelier
In-Reply-To: <4E9609E3.1000300@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> /*
> * Accept strings that are either ALL_CAPS or include a '/'.
> */
>
> (I think the underscore is implied by the example, but the comment could
> be expanded if necessary to be explicit.)
I like that comment hints "_" by having it between all and caps ;-).
^ permalink raw reply
* Re: [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()
From: Michael Haggerty @ 2011-10-12 22:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <7v1uui9g56.fsf@alter.siamese.dyndns.org>
On 10/12/2011 09:14 PM, Junio C Hamano wrote:
> 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?
I don't think it is a logical necessity but I agree that it would be
more consistent. I'll make the change in the next round.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
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