* 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
* Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter
From: Michael Haggerty @ 2011-10-12 22:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <7vwrca81c7.fsf@alter.siamese.dyndns.org>
On 10/12/2011 09:19 PM, Junio C Hamano wrote:
> 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.
Indeed, it is currently not possible for code outside of refs.c to
implement "forget everything" using the "forget one" function (because
there is no API for getting the list of caches that are currently in
memory).
A "forget everything" function might be useful for code that delegates
to a subprocess, if it does not know what submodules the subprocess has
tinkered with. Heiko, does that apply to the future submodule code?
Your specific suggestion would not work because currently
submodule==NULL signifies the main module. However, it would be easy to
add the few-line function when/if it is needed.
I guess the bigger issue for me is whether the whole submodule cache
thing is going to continue to be needed. I really am too ignorant of
how submodules work to be able to judge. From Heiko's recent email it
sounds like things might be moving in the direction of "top-level git
doesn't need to know much about submodules because it delegates to
subprocesses". He also said that submodule references are not modified
by the top-level git process, meaning that it might be sensible for the
submodule reference cache to be less capable than the main module
reference cache.
But if things move in the other direction (submodules handled by the
top-level git process), let alone if git is libified, then it seems
inevitable that there will someday be a "submodule" object that keeps
track of its own ref cache, with the submodule objects rather than the
submodule reference caches looked up by submodule name.
Given that I'm still very new to the codebase, I'm mostly making
"peephole changes" and so I'm happy to get your feedback about how this
fits into the grand scheme of things.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR
From: Jeff King @ 2011-10-12 21:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <7vvcru9k22.fsf_-_@alter.siamese.dyndns.org>
On Wed, Oct 12, 2011 at 10:49:41AM -0700, Junio C Hamano wrote:
> +static int refname_ok_at_root_level(const char *str, int len)
> +{
> + int seen_non_root_char = 0;
> +
> + while (len--) {
> + char ch = *str++;
> +
> + if (ch == '/')
> + return 1;
> + /*
> + * Only accept likes of .git/HEAD, .git/MERGE_HEAD at
> + * the root level as a ref.
> + */
> + if (ch != '_' && (ch < 'A' || 'Z' < ch))
> + seen_non_root_char = 1;
> + }
> + return !seen_non_root_char;
> +}
I thought from your earlier comment:
> I wanted to start as loose as possible to avoid negatively impacting
> existing users, later to tighten. As fsck and friends never look
> outside of refs/, I think the prefix refs/ is a reasonable restriction
> that is safe.
that you did agree with tightening this up to allow just refs/ as a
subdirectory.
Squashable patch is below.
diff --git a/refs.c b/refs.c
index 0f26d9d..b159c4a 100644
--- a/refs.c
+++ b/refs.c
@@ -994,21 +994,20 @@ int check_refname_format(const char *ref, int flags)
static int refname_ok_at_root_level(const char *str, int len)
{
- int seen_non_root_char = 0;
+ if (len >= 5 && !memcmp(str, "refs/", 5))
+ return 1;
while (len--) {
char ch = *str++;
- if (ch == '/')
- return 1;
/*
* Only accept likes of .git/HEAD, .git/MERGE_HEAD at
* the root level as a ref.
*/
if (ch != '_' && (ch < 'A' || 'Z' < ch))
- seen_non_root_char = 1;
+ return 0;
}
- return !seen_non_root_char;
+ return 1;
}
int refname_match(const char *abbrev_name, const char *full_name, const char **rules)
^ permalink raw reply related
* Re: [PATCH] http_init: accept separate URL parameter
From: Jeff King @ 2011-10-12 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <20111012214316.GA4393@sigill.intra.peff.net>
On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King 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>
Sorry, I forgot to mention: this is meant to go on top of the
http-auth-keyring topic.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR
From: Michael Haggerty @ 2011-10-12 21:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <7vr52i9j8g.fsf@alter.siamese.dyndns.org>
On 10/12/2011 08:07 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> On 10/12/2011 07:49 PM, Junio C Hamano wrote:
>>> diff --git a/refs.c b/refs.c
>>> index e3692bd..e54c482 100644
>>> --- a/refs.c
>>> +++ b/refs.c
>>> @@ -994,12 +994,34 @@ const char *ref_fetch_rules[] = {
>>> NULL
>>> };
>>>
>>> +static int refname_ok_at_root_level(const char *str, int len)
>>> +{
>>> + int seen_non_root_char = 0;
>>> +
>>> + while (len--) {
>>> + char ch = *str++;
>>> +
>>> + if (ch == '/')
>>> + return 1;
>>> + /*
>>> + * Only accept likes of .git/HEAD, .git/MERGE_HEAD at
>>> + * the root level as a ref.
>>> + */
>>> + if (ch != '_' && (ch < 'A' || 'Z' < ch))
>>> + seen_non_root_char = 1;
>>> + }
>>> + return !seen_non_root_char;
>>> +}
>>> +
>>
>> Nit: the seen_non_root_char variable can be replaced by an early "return
>> 0" from the loop and "return 1" if the loop falls through.
>
> Hmm, I thought that would fail when you feed "refs/heads/master" to the
> function.
You're right. My brain must be scrambled from all of the rebasing that
I have been doing ;-)
How about adding
/*
* 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.)
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH] http_init: accept separate URL parameter
From: Jeff King @ 2011-10-12 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <4E95FDC8.5030009@drmicha.warpmail.net>
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>
---
On Wed, Oct 12, 2011 at 10:51:20PM +0200, Michael J Gruber wrote:
> > 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?
I think we should go ahead with this one. I gave some thought to
tweaking the http code to figure out authentication closer to the point
of use, so we could be adaptive to things like redirects. But it's quite
an invasive change, since we now have to start possibly keeping a string
of credentials, each mapped from their context.
But more importantly, it changes the user-visible behavior. If I do
something like:
git fetch https://user@git.foo.com/repo.git
and give it a password, and then it redirects me to "git2.foo.com" or
something, then right now we will retry the same credential. I'm not
sure if people rely on that or not.
Arguably, it's wrong to do so in the general case. If I redirect to
"git.someotherdomain.com", you probably _do_ want to re-ask the
credential. So maybe it should be changed, and there should be some
magic with comparing the old and new contexts. I dunno.
At any rate, this is certainly an improvement in the meantime. If the
url parameter to http_init eventually goes away, it is easy enough to do
on top of this.
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 6e8f6d0..ecbfae5 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 d6b2d78..65d3aa7 100644
--- a/http.c
+++ b/http.c
@@ -356,7 +356,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;
@@ -420,11 +420,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 6c24ab1..d4d0910 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -850,7 +850,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)
--
1.7.7.rc2.21.gb9948
^ permalink raw reply related
* Re: [PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs
From: Junio C Hamano @ 2011-10-12 21:39 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Jeff King, mathstuf
In-Reply-To: <1318027869-4037-4-git-send-email-cmn@elego.de>
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.
> @@ -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.
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?
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.
> + for (i = 0; i < ref_count; ++i) {
> + refspec = &refs[i];
> +
> + /* No dst means it can't be used for prunning. */
> + if (!refspec->dst)
> + continue;
> +
> + /*
> + * No '*' means that it must match exactly. If it does
> + * have it, try to match it against the pattern. If
> + * the refspec matches, store the ref name as it would
> + * appear in the server in query->src.
> + */
> + if (!strchr(refspec->dst, '*')) {
> + if (!strcmp(query->dst, refspec->dst)) {
> + query->src = xstrdup(refspec->src);
> + return 0;
> + }
> + } else if (match_name_with_pattern(refspec->dst, query->dst,
> + refspec->src, &query->src)) {
> + return 0;
> + }
> + }
> +
> + return -1;
> +}
^ permalink raw reply
* Re: [PATCH/RFC] remote: support --all for the prune-subcommand
From: Jeff King @ 2011-10-12 21:36 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git
In-Reply-To: <CABPQNSb7WACrr=7FsR8YVMC1-q3i0zRhQtXiV8VshfCJn3qgEA@mail.gmail.com>
On Tue, Oct 04, 2011 at 10:22:35AM +0200, Erik Faye-Lund wrote:
> > I think the original rationale was that we didn't want fetch to be
> > "lossy". That is, if I were using upstream's "foo" branch as part of my
> > work (to diff against, or whatever), then doing a "git fetch" to update
> > should not suddenly make it hard to do my work. And not just hard as in
> > "I notice that it's gone and I adapt my workflow". But that you no
> > longer have _any_ record of where upstream's "foo" branch used to point,
> > so even doing something like:
> >
> > git rebase --onto new-foo foo my-topic
> >
> > is impossible.
>
> Following that logic, a user cannot _ever_ safely prune a remote if he
> wants to work on some of the branches. Doing something like "git
> remote foo -n" to check if the branch would get pruned before doing a
> proper prune is prone to a race-condition; the branch could be deleted
> on the remote between the dry-run and the actual pruning.
Right. And that's why we don't prune by default. In practice, it tends
to be safe if you pick a reasonable time to prune, and the upstream is
reasonable about their branches. But turning it on all the time takes
away the "pick a reasonable time".
> Besides, the owner of the repo can just as easily have deleted the
> branch and created a new one with the same name, causing the contents
> of the branch to be lost. This happens all the time with
> "for-upstream"-kind of branches, no?
They can do that, but on the local side, you will just see a jump in
history. But because we didn't _delete_ the ref on the local side, you
will retain your reflog.
IOW, the reflog can save us from anything the upstream will do. And
that's what makes deletion so special: we delete the local reflog.
> > The right solution, IMHO, is that ref deletion should actually keep the
> > reflog around in a graveyard of some sort. Entries would expire
> > naturally over time, as they do in regular reflogs. And then it becomes
> > a lot safer to prune on every fetch, because you still have 90 days look
> > at the reflog.
> >
> Fixing the reflog to expire for ref deletion rather than completely
> deleting it sounds like a good move, indeed.
This is on my long-term todo list, but if somebody gets around to it
before me, I won't be upset. :)
> While we're on the subject, an additional argument to change "git
> fetch" to always prune is that it's much much easier for user to grok
> "last known state of <remote>'s branches" than "the union of all the
> branches that were ever pulled from <remote>, unless --prune was
> specified". But that's not a technical one, and surely there's issues
> to resolve with the proposal before going in that direction.
Agreed. Really, everything argument points towards auto-prune except the
reflog-safety thing. I think once that is fixed, turning on pruning by
default becomes a no-brainer.
-Peff
^ permalink raw reply
* Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional
From: Junio C Hamano @ 2011-10-12 21:05 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Git List, Jonathan Nieder, Jeff King, Daniel Barkalow,
Christian Couder
In-Reply-To: <7vwrccn34l.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Since this is a part of clean-up series...
>
> Do you even need to have a sha1_abbrev[] local array that is limited to 40
> bytes here? The incoming _line_ is not "const char *start", so you should
> at least be able to temporarily terminate the commit object name with NUL
> (while remembering what byte there was before), give it to get_sha1(), and
> then restore the byte at the end before returning from this function.
Like this, perhaps. I did this on top of the whole series only as a
demonstration but the change should be squashed into this step when the
series is rerolled.
builtin/revert.c | 47 +++++++++++++++++++----------------------------
1 files changed, 19 insertions(+), 28 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index b28c3ca..170a6c1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -691,42 +691,34 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
return 0;
}
-static int parse_insn_line(char *start, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
{
unsigned char commit_sha1[20];
- char sha1_abbrev[40];
- const char *p, *q;
+ char *end_of_object_name;
+ int saved, status;
- p = start;
- if (!prefixcmp(start, "pick ")) {
+ if (!prefixcmp(bol, "pick ")) {
item->action = REPLAY_PICK;
- p += strlen("pick ");
- } else if (!prefixcmp(start, "revert ")) {
+ bol += strlen("pick ");
+ } else if (!prefixcmp(bol, "revert ")) {
item->action = REPLAY_REVERT;
- p += strlen("revert ");
+ bol += strlen("revert ");
} else {
- size_t len = strchrnul(p, '\n') - p;
- if (len > 255)
- len = 255;
- return error(_("Unrecognized action: %.*s"), (int)len, p);
+ return error(_("Unrecognized action: %s"), bol);
}
- q = p + strcspn(p, " \n");
- if (q - p + 1 > sizeof(sha1_abbrev)) {
- size_t len = q - p;
- if (len > 255)
- len = 255;
- return error(_("Object name too large: %.*s"), (int)len, p);
- }
- memcpy(sha1_abbrev, p, q - p);
- sha1_abbrev[q - p] = '\0';
+ end_of_object_name = bol + strcspn(bol, " \n");
+ saved = *end_of_object_name;
+ *end_of_object_name = '\0';
+ status = get_sha1(bol, commit_sha1);
+ *end_of_object_name = saved;
- if (get_sha1(sha1_abbrev, commit_sha1) < 0)
- return error(_("Malformed object name: %s"), sha1_abbrev);
+ if (status < 0)
+ return error(_("Malformed object name: %s"), bol);
item->operand = lookup_commit_reference(commit_sha1);
if (!item->operand)
- return error(_("Not a valid commit: %s"), sha1_abbrev);
+ return error(_("Not a valid commit: %s"), bol);
item->next = NULL;
return 0;
@@ -740,12 +732,11 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
int i;
for (i = 1; *p; i++) {
- if (parse_insn_line(p, &item) < 0)
+ char *eol = strchrnul(p, '\n');
+ if (parse_insn_line(p, eol, &item) < 0)
return error(_("on line %d."), i);
next = replay_insn_list_append(item.action, item.operand, next);
- p = strchrnul(p, '\n');
- if (*p)
- p++;
+ p = *eol ? eol + 1 : eol;
}
if (!*todo_list)
return error(_("No commits parsed."));
^ permalink raw reply related
* 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
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