* 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
* Re: [RFC/PATCH]: reverse bisect v 2.0
From: Jeff King @ 2011-10-12 20:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Andrew Ardill, Christian Couder, Michal Vyskocil, git,
Sverre Rabbelier, Johannes Sixt
In-Reply-To: <7vr52ibydy.fsf@alter.siamese.dyndns.org>
On Tue, Oct 11, 2011 at 09:57:13PM -0700, Junio C Hamano wrote:
> With an obvious addition of non-interactive short-cut subcommands "git
> bisect yes" and "git bisect no", I think --removed= is a much better
> wording than --used-to= I suggested in the discussion.
Agreed.
> I however am still worried about the flipping of the mapping between
> <good,bad> and <yes,no> this design requires. What are we going to do to
> the labels of low-level machinery (i.e $GIT_DIR/refs/bisect/bad and
> $GIT/refs/bisect/good)? They appear in "bisect visualize" and I was hoping
> that it would be simpler in the code if we do not have to change them in
> such a way that depends on this introduced/removed switch, and that was
> the reason why I was trying to see if we can solve this without the
> switchable mapping between <good,bad> and <yes,no>.
Hmm. I hadn't thought about the labels. In a yes/no situation, though,
couldn't you use the labels as the user sees them?
Then it is simply a matter of flipping yes/no inside the bisect script
whenever we interact with the user (i.e., "git bisect yes") or when we
interact with the on-disk labels.
Certainly it's more complex than not allowing reversing, though.
> More specifically, I was hoping that we can rename "good" to "old" and
> "bad" to "new" unconditionally and be done with it. We would ask the user
> "What did the code used to do in the olden days?" and "Does this version
> behave the same as it used to?". The possible answers the user can give
> are "git bisect old" (it behaves the same as the older versions) and "git
> bisect new" (it behaves the same as the newer versions). Then we do not
> have to worry about having to flip the meaning of <yes> and <no> at the UI
> level.
Hmm. I think this is not quite as nice, but it is way simpler. It may be
worth trying for a bit to see how people like it. If they don't, the
cost of failure is that we have to maintain "old/new" forever, even
after we implement a yes/no reversible scheme. But maintaining the
old/new mapping from yes/no would not be any harder than the good/bad
mapping, which we would need to do anyway.
So it sounds like a reasonable first step.
-Peff
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Jeff King @ 2011-10-12 20:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Ilari Liusvaara, Junio C Hamano, Johannes Sixt,
Jonathan Nieder
In-Reply-To: <1317678909-19383-1-git-send-email-pclouds@gmail.com>
On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:
> The message is chosen to avoid leaking information, yet let users know
> that they are deliberately not allowed to use the service, not a fault
> in service configuration or the service itself.
I do think this is an improvement, but I wonder if the verbosity should
be configurable. Then open sites like kernel.org could be friendlier to
their users. Something like this instead:
---
daemon.c | 21 +++++++++++++++++----
1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/daemon.c b/daemon.c
index 4c8346d..ec88fd0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -20,6 +20,7 @@
static int log_syslog;
static int verbose;
static int reuseaddr;
+static int informative_errors;
static const char daemon_usage[] =
"git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
return 0;
}
+static int daemon_error(const char *dir, const char *msg)
+{
+ if (!informative_errors)
+ msg = "access denied";
+ packet_write(1, "ERR %s: %s", dir, msg);
+ return -1;
+}
+
static int run_service(char *dir, struct daemon_service *service)
{
const char *path;
@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
if (!enabled && !service->overridable) {
logerror("'%s': service not enabled.", service->name);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "service not enabled");
}
if (!(path = path_ok(dir)))
- return -1;
+ return daemon_error(dir, "no such repository");
/*
* Security on the cheap.
@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
logerror("'%s': repository not exported.", path);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "repository not exported");
}
if (service->overridable) {
@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
logerror("'%s': service not enabled for '%s'",
service->name, path);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "service not enabled");
}
/*
@@ -1167,6 +1176,10 @@ int main(int argc, char **argv)
make_service_overridable(arg + 18, 0);
continue;
}
+ if (!prefixcmp(arg, "--informative-errors")) {
+ informative_errors = 1;
+ continue;
+ }
if (!strcmp(arg, "--")) {
ok_paths = &argv[i+1];
break;
--
1.7.7.rc2.21.gb9948
^ permalink raw reply related
* Re: [PATCH] Makefile: add a knob to turn off hardlinks within same directory
From: Jonathan Nieder @ 2011-10-12 19:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Bastian Blank, Cedric Staniewski
In-Reply-To: <7vaa969go4.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Wouldn't your use case be better served with
>
> $ tar zcf dist.tar.gz --hard-dereference $list_of_files_to_tar_up
>
> instead?
No, because that duplicates the files instead of making symlinks.
^ permalink raw reply
* Re: [PATCH] Make is_gitfile a non-static generic function
From: Phil Hord @ 2011-10-12 19:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phil Hord, git@vger.kernel.org
In-Reply-To: <7vmxd69j72.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> sez:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> After looking at this patch and the way the other caller in transport.c
>>> uses it, I am more and more convinced that "is_gitfile()" is a stupid and
>>> horrible mistake.
>>
>> I think I misunderstood your objection before. Now I think I
>> understand. Tell me if I am right.
>>
>>
>> I think you mean that instead of this:
>> } else if (is_local(url) && is_file(url) && !is_gitfile(url)) {
>>
>> you would like to see this:
>> } else if (is_local(url) && is_file(url) && is_bundle(url)) {
>>
>> Or maybe even this:
>> } else if (is_bundle(url)) {
>
> Exactly.
I tried to refactor read_bundle_header, but it was too ugly and I don't quite
understand all the error paths. Plus, the whole is_bundle part can be just
10 lines of code. Maybe read_bundle_header() should be shortened slightly to
use is_bundle() for code duplication avoidance.
-- >8 --
Subject: [PATCH] transport: Add/use is_bundle() to identify a url
Transport currently decides that any local url which is a file
must be a bundle. This is wrong now if the local url is a
gitfile, and it will be wrong in the future when some other
exception shows up. Teach transport to verify the file is
really a bundle instead of just assuming it is so.
---
bundle.c | 16 ++++++++++++++++
bundle.h | 1 +
transport.c | 10 +---------
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/bundle.c b/bundle.c
index f82baae..3a64a43 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,6 +23,22 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
list->nr++;
}
+int is_bundle(const char *path)
+{
+ char buffer[100];
+ FILE *ffd = fopen(path, "rb");
+ int ret=1;
+
+ if (!ffd)
+ return 0;
+
+ if (!fgets(buffer, sizeof(buffer), ffd) ||
+ strcmp(buffer, bundle_signature))
+ ret=0;
+ fclose(ffd);
+ return ret;
+}
+
/* returns an fd */
int read_bundle_header(const char *path, struct bundle_header *header)
{
diff --git a/bundle.h b/bundle.h
index c5a22c8..35aa0eb 100644
--- a/bundle.h
+++ b/bundle.h
@@ -14,6 +14,7 @@ struct bundle_header {
struct ref_list references;
};
+int is_bundle(const char *path);
int read_bundle_header(const char *path, struct bundle_header *header);
int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv);
diff --git a/transport.c b/transport.c
index f3195c0..bcd9b74 100644
--- a/transport.c
+++ b/transport.c
@@ -881,14 +881,6 @@ static int is_gitfile(const char *url)
return !prefixcmp(buf, "gitdir: ");
}
-static int is_file(const char *url)
-{
- struct stat buf;
- if (stat(url, &buf))
- return 0;
- return S_ISREG(buf.st_mode);
-}
-
static int external_specification_len(const char *url)
{
return strchr(url, ':') - url;
@@ -929,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->fetch = fetch_objs_via_rsync;
ret->push = rsync_transport_push;
ret->smart_options = NULL;
- } else if (is_local(url) && is_file(url) && !is_gitfile(url)) {
+ } else if (is_local(url) && is_bundle(url)) {
struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
ret->get_refs_list = get_refs_from_bundle;
--
1.7.7.334.g311c9.dirty
^ permalink raw reply related
* Re: [PATCH v3 0/7] Provide API to invalidate refs cache
From: Junio C Hamano @ 2011-10-12 19:28 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318445067-19279-1-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> These patches are re-rolled onto master.
Thanks.
^ permalink raw reply
* Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Jeff King @ 2011-10-12 19:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <7vipnu81al.fsf@alter.siamese.dyndns.org>
On Wed, Oct 12, 2011 at 12:20:18PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > We also look at ref_rev_parse_rules in shorten_unambiguous_ref. So even
> > with your patch, I still get the warning with:
> >
> > $ git branch config
> > $ git for-each-ref --format='%(refname:short)' refs/heads/
> >
> > It looks like we also use it in remote.c:count_refspec_match, but I
> > haven't figured out if that can trigger a warning or not.
>
> I do not think this is "do not trigger a warning" exercise. This is "we no
> longer consider names outside refs/ as potential magic refs unless they
> are all uppercase".
>
> If the updated dwim_ref() says $GIT_DIR/frotz will no longer create
> ambiguity with $GIT_DIR/refs/heads/frotz, then refname_match() needs to
> know about it, and cause count_refspec_match() to say that "frotz"
> uniquely names "refs/heads/frotz".
>
> The same stands for shorten_unambiguous_ref(). If $GIT_DIR/frotz no longer
> is ambiguous with $GIT_DIR/refs/heads/frotz, then %(refname:shrot) for
> refs/heads/frotz should yield "frotz", and we should not have to qualify
> it as "heads/frotz".
Absolutely. I was lazy in how I woreded it, but I meant that the warning
was the indicator that we were doing something buggy, not that it was
the bug itself. It just happened to be a convenient way to test. :)
Your most recent patch looks right.
-Peff
^ permalink raw reply
* Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Junio C Hamano @ 2011-10-12 19:20 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <20111011230749.GA29785@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We also look at ref_rev_parse_rules in shorten_unambiguous_ref. So even
> with your patch, I still get the warning with:
>
> $ git branch config
> $ git for-each-ref --format='%(refname:short)' refs/heads/
>
> It looks like we also use it in remote.c:count_refspec_match, but I
> haven't figured out if that can trigger a warning or not.
I do not think this is "do not trigger a warning" exercise. This is "we no
longer consider names outside refs/ as potential magic refs unless they
are all uppercase".
If the updated dwim_ref() says $GIT_DIR/frotz will no longer create
ambiguity with $GIT_DIR/refs/heads/frotz, then refname_match() needs to
know about it, and cause count_refspec_match() to say that "frotz"
uniquely names "refs/heads/frotz".
The same stands for shorten_unambiguous_ref(). If $GIT_DIR/frotz no longer
is ambiguous with $GIT_DIR/refs/heads/frotz, then %(refname:shrot) for
refs/heads/frotz should yield "frotz", and we should not have to qualify
it as "heads/frotz".
^ permalink raw reply
* Re: [PATCH 2/2] t1300: test mixed-case variable retrieval
From: Junio C Hamano @ 2011-10-12 19:19 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, git
In-Reply-To: <20111012183002.GB18948@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I was surprised this wasn't tested anywhere, but I couldn't find any
> such place. I think it makes sense to document the desired behavior in
> the form of a few tests.
Thanks. The patch looks good.
But oh boy the original test in the old style was ugly....
^ permalink raw reply
* Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter
From: Junio C Hamano @ 2011-10-12 19:19 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318445067-19279-3-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Instead of invalidating the ref cache on an all-or-nothing basis,
> allow the cache for individual submodules to be invalidated.
That "allow" does not seem to describe what this patch does. It disallows
the wholesale invalidation and forces the caller to invalidate ref cache
individually.
Probably that is what all the existing callers want, but I would have
expected that an existing feature would be kept, perhaps like this
instead:
if (!submodule) {
struct ref_cache *c;
for (c = ref_cache; c; c = c->next)
clear_ref_cache(c);
} else {
clear_ref_cache(get_ref_cache(submodule);
}
Not a major "vetoing" objection, just a comment.
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 12 ++++--------
> 1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 120b8e4..cc72609 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -202,13 +202,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
> return refs;
> }
>
> -static void invalidate_ref_cache(void)
> +static void invalidate_ref_cache(const char *submodule)
> {
> - struct cached_refs *refs = cached_refs;
> - while (refs) {
> - clear_cached_refs(refs);
> - refs = refs->next;
> - }
> + clear_cached_refs(get_cached_refs(submodule));
> }
^ permalink raw reply
* Re: [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()
From: Junio C Hamano @ 2011-10-12 19:14 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318445067-19279-2-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> It is the cache that is being invalidated, not the references.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> diff --git a/refs.c b/refs.c
> index 9911c97..120b8e4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
> return refs;
> }
>
> -static void invalidate_cached_refs(void)
> +static void invalidate_ref_cache(void)
> {
> struct cached_refs *refs = cached_refs;
> while (refs) {
If you call the operation "invalidate ref_cache", shouldn't the data
structure that holds that cache also be renamed to "struct ref_cache" from
"struct "cached_refs" at the same time?
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2011, #04; Wed, 12)
From: Junio C Hamano @ 2011-10-12 19:05 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111012190213.GA19578@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Oct 12, 2011 at 11:48:48AM -0700, Junio C Hamano wrote:
>
>> * jk/name-hash-dirent (2011-10-07) 1 commit
>> (merged to 'next' on 2011-10-11 at e2ea68b)
>> + fix phantom untracked files when core.ignorecase is set
>
> I didn't see any comment on the original patch, so I assume you're OK
> with the few extra bytes added to each cache entry? Otherwise, I can try
> to retool it to keep the directory entries in a separate hash, so only
> case-insensitive people pay the extra price.
>
> I did a few trivial timings, and the extra bytes didn't seem to make any
> difference.
I tend to agree with you that 8 extra bytes per cache entry wouldn't hurt.
I also suspected that the cost of code complexity coming from both
maintaining and conditionally looking up a separate hash would outweigh
the benefit.
^ permalink raw reply
* Re: [PATCH] Makefile: add a knob to turn off hardlinks within same directory
From: Junio C Hamano @ 2011-10-12 19:02 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Bastian Blank, Cedric Staniewski
In-Reply-To: <20111012083842.GA21969@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Typically someone using this setting would want to set
> NO_CROSS_DIRECTORY_HARDLINKS, too, but that is not enforced, so you
> can make $(bindir)/git and $(gitexecdir)/git into hardlinks to the
> same inode and still make sure your tarball avoids the btrfs limits if
> you want.
>
> Requested-by: Bastian Blank <waldi@debian.org>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Hi,
>
> See <http://bugs.debian.org/642603> for context. Sane?
>
> Makefile | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9afdcf2a..ab64ff4c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -226,6 +226,10 @@ all::
> # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
> # programs as a tar, where bin/ and libexec/ might be on different file systems.
> #
> +# Define NO_HARDLINKS if you plan to distribute the installed programs as a tar
> +# that might be extracted on a filesystem like btrfs that does not cope well
> +# with many links to one inode in one directory.
> +#
Hmm.... I would understand if the change were to eventually remove these
"git-foo" anywhere from the filesystem perhaps after a large version bump
in Git 2.0, but that is not what you are trying to do.
Wouldn't your use case be better served with
$ tar zcf dist.tar.gz --hard-dereference $list_of_files_to_tar_up
instead?
^ permalink raw reply
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