* git-completion bug for 'git remote show <TAB>'
From: Peng Yu @ 2012-02-17 14:41 UTC (permalink / raw)
To: git
Hi,
I got the following error when I try to complete after 'git remote
show'. Could anybody take a look what is the problem?
~/dvcs_src/cron/media$ git remote show -bash: words_: bad array subscript
-bash: words_: bad array subscript
--
Regards,
Peng
^ permalink raw reply
* Re: [PATCH v2] Add a setting to require a filter to be successful
From: Junio C Hamano @ 2012-02-17 15:35 UTC (permalink / raw)
To: Johannes Sixt; +Cc: jehan, gitster, git
In-Reply-To: <4F3DFCD0.6070002@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> Am 2/17/2012 2:19, schrieb jehan@orb.com:
>> @@ -747,13 +753,19 @@ int convert_to_git(const char *path, const char *src, size_t len,
> ...
>> ret |= apply_filter(path, src, len, dst, filter);
>> + if (!ret && required)
>> + die("required filter '%s' failed", ca.drv->name);
>
> Wouldn't it be much more helpful if this were:
>
> die("%s: clean filter '%s' failed", path, ca.drv->name);
>
> Likewise (with s/clean/smudge/) in convert_to_working_tree_internal().
>
>> + ! git checkout -- test.fs
>
> test_must_fail git checkout -- test.fs
>
>> + ! git add test.fc
>
> test_must_fail git add test.fc
>
> -- Hannes
Thanks; I'll just squash these in in-place.
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Piotr Krukowiecki @ 2012-02-17 16:55 UTC (permalink / raw)
To: Thomas Rast; +Cc: Jeff King, Git Mailing List, Nguyen Thai Ngoc Duy
In-Reply-To: <87d39eswkx.fsf@thomas.inf.ethz.ch>
On Thu, Feb 16, 2012 at 3:05 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:
>
>> On Wed, Feb 15, 2012 at 8:03 PM, Jeff King <peff@peff.net> wrote:
>>> On Wed, Feb 15, 2012 at 09:57:29AM +0100, Piotr Krukowiecki wrote:
>>>>
>>> I notice that you're still I/O bound even after the repack:
>>>
>>>> $ time git status -- .
>>>> real 0m2.503s
>>>> user 0m0.160s
>>>> sys 0m0.096s
>>>>
>>>> $ time git status
>>>> real 0m9.663s
>>>> user 0m0.232s
>>>> sys 0m0.556s
>>>
>>> Did you drop caches here, too?
>>
>> Yes I did - with cache the status takes something like 0.1-0.3s on whole repo.
>
> So umm, I'm not sure that leaves anything to be improved.
But even with caches time on small directory is only half of time on whole repo:
0.15s vs 0.07s
> I looked at some strace dumps, and limiting the status to a subdirectory
> (in my case, '-- t' in git.git) does omit the lstat()s on uninteresting
> parts of the index-listed files, as well as the getdents() (i.e.,
> readdir()) for parts of the tree that are not interesting.
Same in my case.
I've run strace with -c which shows system calls times. As I
understand the results the time used by lstat() is very small. With
dropped caches:
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
90.70 0.065108 3 25361 12 lstat
6.78 0.004869 1 6534 getdents
1.94 0.001392 0 6964 3238 open
0.27 0.000194 0 3726 close
> BTW, some other parts of git-status's display may be responsible for the
> amount of data it pulls from disk. In particular, the "Your branch is
> ahead" display requires computing the merge-base between HEAD and
> @{upstream}. If your @{upstream} is way ahead/behind, or points at a
> disjoint chunk of history, this may mean essentially pulling all of the
> involved history from disk. If my memory of pack organization serves
> right, the commit objects involved would essentially be spread across
> the whole pack (corresponding to "time") and thus this operation would
> more or less load the entire pack from disk.
I don't think this is the case - I'm using git-svn and thus have no
upstream in git meaning.
--
Piotr Krukowiecki
^ permalink raw reply
* Re: [PATCH v3 0/3]
From: Junio C Hamano @ 2012-02-17 17:03 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <cover.1329472405.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> There were actually more mistakes lurking :-( so I am resending the
> whole series.
Ok, will requeue. The diff you attached to this cover letter looked at
least halfway sane, compared to the previous round ;-), though it is not
exactly clear what goes to lib-test-functions and what goes to lib-test
(for example, you moved test_expect_success to 'test-functions', but it
calls test_ok_ that is in 'test-lib', and test_ok_ is directly used by
test_perf in the new 't/perf/perf-lib.sh'), making it harder for people to
decide where to put their additions to the test infrastructure from now
on. There needs a bit of description in the first patch to guide them.
I seem to be getting intermittent test failures, and every time the
failing tests are different, when these three are queued to 'pu'. I didn't
look for what goes wrong and how.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/2] api-config documentation leftovers
From: Junio C Hamano @ 2012-02-17 17:04 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120217081743.GA11389@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Here they are, on top of what you have in jk/config-include. Squashing
> would involve breaking apart the second one into the "introduce
> git_config_with_options" part and the "and now it learns
> respect_includes" part. So it's probably not worth the effort.
>
> [1/2]: docs/api-config: minor clarifications
> [2/2]: docs/api-config: describe git_config_with_options
Thanks.
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Piotr Krukowiecki @ 2012-02-17 17:19 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, Git Mailing List, Nguyen Thai Ngoc Duy
In-Reply-To: <20120216192001.GB4348@sigill.intra.peff.net>
On Thu, Feb 16, 2012 at 8:20 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 16, 2012 at 02:37:47PM +0100, Piotr Krukowiecki wrote:
>
>> >> $ time git status -- .
>> >> real 0m2.503s
>> >> user 0m0.160s
>> >> sys 0m0.096s
>> >>
>> >> $ time git status
>> >> real 0m9.663s
>> >> user 0m0.232s
>> >> sys 0m0.556s
>> >
>> > Did you drop caches here, too?
>>
>> Yes I did - with cache the status takes something like 0.1-0.3s on whole repo.
>
> OK, then that makes sense. It's pretty much just I/O on the filesystem
> and on the object db.
>
> You can break status down a little more to see which is which. Try "git
> update-index --refresh" to see just how expensive the lstat and index
> handling is.
"git update-index --refresh" with dropped cache took
real 0m3.726s
user 0m0.024s
sys 0m0.404s
while "git status" with dropped cache takes
real 0m13.578s
user 0m0.240s
sys 0m0.600s
I'm not sure why it takes more than the 9s reported before - IIRC I
did the previous test in single mode under bare shell and this time
I'm testing under gnome. This or it's the effect of running
update-index :/
Now status on subdir takes 9.5s. But still the
not-much-faster-status-on-subdir rule is true.
> And then try "git diff-index HEAD" for an idea of how expensive it is to
> just read the objects and compare to the index.
The diff-index after dropping cache takes
real 0m14.095s
user 0m0.268s
sys 0m0.564s
>> > Not really. You're showing an I/O problem, and repacking is git's way of
>> > reducing I/O.
>>
>> So if I understand correctly, the reason is because git must compare
>> workspace files with packed objects - and the problem is
>> reading/seeking/searching in the packs?
>
> Mostly reading (we keep a sorted index and access the packfiles via
> mmap, so we only touch the pages we need). But you're also paying to
> lstat() the directory tree, too. And you're paying to load (probably)
> the whole index into memory, although it's relatively compact compared
> to the actual file data.
If the index is the objects/pack/*.idx files than it's 21MB
>> Is there a way to make packs better? I think most operations are on
>> workdir files - so maybe it'd be possible to tell gc/repack/whatever
>> to optimize access to files which I currently have in workdir?
>
> It already does optimize for that case. If you can make it even better,
> I'm sure people would be happy to see the numbers.
If I understand correctly, you only need to compute sha1 on the
workdir files and compare it with sha1 files recorded in index/gitdir.
It seems that to get the sha1 from index/gitdir I need to read the
packfiles? Maybe it'd be possible to cache/index it somehow, for
example in separate and smaller file?
> Mostly I think it is just the case that disk I/O is slow, and the
> operation you're asking for has to do a certain amount of it. What kind
> of disk/filesystem are you pulling off of?
>
> It's not a fuse filesystem by any chance, is it? I have a repo on an
> encfs-mounted filesystem, and the lstat times are absolutely horrific.
No, it's ext4 and the disk Seagate Barracuda 7200.12 500GB, as it
reads on the cover :)
But IMO faster disk won't help with this - times will be smaller, but
you'll still have to read the same data, so the subdir times will be
just 2x faster than whole repo, won't it? So maybe in my case it will
go down to e.g. 2s on subdir, but for someone with larger repository
it will still be 10s...
--
Piotr Krukowiecki
^ permalink raw reply
* Re: [PATCH] git-latexdiff: new command in contrib, to use latexdiff and Git
From: Junio C Hamano @ 2012-02-17 17:25 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Tim Haga, git
In-Reply-To: <vpqobsx7d9s.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> Look at what we have in the contrib/ area. I think what is common among
>> them is that their primary benefit is to enrich user's Git experience.
>
> ... and many of them is to enrich the user experience using Git with
> another tool (shell, text editor, foreign VCS).
... where the amount of the benefit they get does not change regardless of
the payload. That is what makes them tool in Git users' toolbox, not
shell scripters' or p4 users' toolbox.
> Git's _core_ already has some code to show diff hunks for various
> languages,...
I would have to say that it is an oranges vs squirrels comparison. I do
not see a justification to reject an addition of an entry to an array that
adds a few strings of regexp to drive the mechanism that is already in
core, when it gets compiled in, and it is useless by itself outside Git.
Unless the language is something obscure, that is.
Read git-latexdiff that is a free-standing 200+ line script again
yourself. The use of Git in the script is not more than how you would
emulate what you would use "cp -r" in order to populate the old/ and new/
directories if the two versions were stored in the file system outside
Git. If you rip the part that deals with "the two versions happen to be
stored in Git" out, the remainder deals with parsing the command line,
interfacing with latex and reporting the result, none of which is in any
way Git specific. That is much larger part of the script, which I view as
a clear sign that it is an application to serve the need for LaTeX users
better, which happens to be written for the subset of LaTeX users who have
their contents in Git.
Aren't there LaTeX tools archives that would be a much better home for
this tool?
^ permalink raw reply
* [PATCH] refresh_index: do not show unmerged path that is outside pathspec
From: Junio C Hamano @ 2012-02-17 18:27 UTC (permalink / raw)
To: git
When running "git add --refresh <pathspec>", we incorrectly showed the
path that is unmerged even if it is outside the specified pathspec, even
though we did honor pathspec and refreshed only the paths that matched.
Note that this cange does not affect "git update-index --refresh"; for
hysterical raisins, it does not take a pathspec (it takes real paths) and
more importantly itss command line options are parsed and executed one by
one as they are encountered, so "git update-index --refresh foo" means
"first refresh the index, and then update the entry 'foo' by hashing the
contents in file 'foo'", not "refresh only entry 'foo'".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I wondered if the "has_errors" that the caller can use to detect if
there are not-yet-added paths should be covered by the if (!filtered)
test as well. No caller that passes pathpec seems to use the return
value, so we probably should for the sake of consistency, but I didn't.
read-cache.c | 11 +++++++++--
t/t3700-add.sh | 15 +++++++++++++++
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index a51bba1..274e54b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1120,11 +1120,16 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
struct cache_entry *ce, *new;
int cache_errno = 0;
int changed = 0;
+ int filtered = 0;
ce = istate->cache[i];
if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
continue;
+ if (pathspec &&
+ !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
+ filtered = 1;
+
if (ce_stage(ce)) {
while ((i < istate->cache_nr) &&
! strcmp(istate->cache[i]->name, ce->name))
@@ -1132,12 +1137,14 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
i--;
if (allow_unmerged)
continue;
- show_file(unmerged_fmt, ce->name, in_porcelain, &first, header_msg);
+ if (!filtered)
+ show_file(unmerged_fmt, ce->name, in_porcelain,
+ &first, header_msg);
has_errors = 1;
continue;
}
- if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
+ if (filtered)
continue;
new = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 575d950..874b3a6 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -179,6 +179,21 @@ test_expect_success 'git add --refresh' '
test -z "`git diff-index HEAD -- foo`"
'
+test_expect_success 'git add --refresh with pathspec' '
+ git reset --hard &&
+ echo >foo && echo >bar && echo >baz &&
+ git add foo bar baz && H=$(git rev-parse :foo) && git rm -f foo &&
+ echo "100644 $H 3 foo" | git update-index --index-info &&
+ test-chmtime -60 bar baz &&
+ >expect &&
+ git add --refresh bar >actual &&
+ test_cmp expect actual &&
+
+ git diff-files --name-only >actual &&
+ ! grep bar actual&&
+ grep baz actual
+'
+
test_expect_success POSIXPERM,SANITY 'git add should fail atomically upon an unreadable file' '
git reset --hard &&
date >foo1 &&
--
1.7.9.1.265.g25f75
^ permalink raw reply related
* Re: [PATCH] git-latexdiff: new command in contrib, to use latexdiff and Git
From: Jakub Narebski @ 2012-02-17 18:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, Tim Haga, git
In-Reply-To: <7vwr7ltlrj.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Aren't there LaTeX tools archives that would be a much better home for
> this tool?
There is CTAN: Comprehensive TeX Archive Network (http://ctan.org),
which hosts tools such like latexmk (cousin of the general make
utility), autolatex (generates Makefile), chktex, ite (interactive TeX
editor),... latexdiff itself is also there.
--
Jakub Narebski
^ permalink raw reply
* Re: How to use git attributes to configure server-side checks?
From: Michael Haggerty @ 2012-02-17 18:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git discussion list
In-Reply-To: <7vy5xh1whq.fsf@alter.siamese.dyndns.org>
On 09/21/2011 10:17 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> I was thinking of using git attributes to configure a server-side
>> "update" hook that does some basic sanity checking before accepting a
>> push. I thought I could do something like
>> [...]
>> I see that there is an (undocumented) API involving
>> git_attr_set_direction() that seems to let gitattributes to be read out
>> of the index instead of the working tree. But I am still confused:
>
> The words "server side" automatically mean that there should be no working
> tree, and where there is no working tree there should be no index, so the
> direction should not make any difference. The attributes that are used to
> help whitespace checks should come from project.git/info/attributes in
> such a case [*...*].
I was just alerted by Scott Chacon's blog [1] to the fact that one can
set GIT_INDEX_FILE to an arbitrary filename, thereby causing the index
to be read/written from that file instead of $GIT_DIR/index. It
occurred to me that this feature, along with the addition of "git
check-attr --cached" in 1.7.8, can be used to give server-side access to
the gitattributes for an arbitrary commit:
GIT_INDEX_FILE="$(tempfile)"
export GIT_INDEX_FILE
git read-tree $COMMIT
git check-attr --cached attr -- pathname
...
rm "$GIT_INDEX_FILE"
Empirically it seems to work (and it is surprisingly fast). The use of
a temporary file prevents simultaneous accesses to the repository from
stepping on each other. This looks like a clean solution to my problem.
Or is there some hidden pitfall in this approach?
Michael
[1] http://progit.org/2010/04/11/environment.html
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [RFC/PATCH 0/3] push: add 'prune' option
From: Felipe Contreras @ 2012-02-17 19:12 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
Hi,
As mentioned in a previous thread[1], git is lacking some functionality to
synchronize completely with remote repositories.
As an example I put my use-case; I want to backup *all* my local branches to a
personal repository, and I want to remove branches that I have removed from my
local repository. git push personal 'refs/heads/*' mostly does the job, but it
doesn't remove anything, and that's where 'prune' comes from.
Do not confuse the remote branches with the upstream ones; you could have two
repositories where you want to synchronize branches to:
% git push --prune --force backup1 'refs/heads/*'
% git push --prune --force backup2 'refs/heads/*'
I still think a 'git remote sync' would be tremendously useful, but it can
actually use this --prune option.
I know, documentation and testing are missing, but perhaps there will be
comments on the code itself.
Cheers.
[1] http://article.gmane.org/gmane.comp.version-control.git/184990
Felipe Contreras (3):
remote: use a local variable in match_push_refs()
remote: reorganize check_pattern_match()
push: add 'prune' option
builtin/push.c | 2 +
remote.c | 91 +++++++++++++++++++++++++++++++++++--------------------
remote.h | 3 +-
transport.c | 2 +
transport.h | 1 +
5 files changed, 65 insertions(+), 34 deletions(-)
--
1.7.9.1
^ permalink raw reply
* [RFC/PATCH 1/3] remote: use a local variable in match_push_refs()
From: Felipe Contreras @ 2012-02-17 19:12 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
In-Reply-To: <1329505957-24595-1-git-send-email-felipe.contreras@gmail.com>
So that we can reuse src later on.
Will be useful in next patches.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
remote.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/remote.c b/remote.c
index 73a3809..55d68d1 100644
--- a/remote.c
+++ b/remote.c
@@ -1157,7 +1157,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
int send_mirror = flags & MATCH_REFS_MIRROR;
int errs;
static const char *default_refspec[] = { ":", NULL };
- struct ref **dst_tail = tail_ref(dst);
+ struct ref *ref, **dst_tail = tail_ref(dst);
if (!nr_refspec) {
nr_refspec = 1;
@@ -1167,14 +1167,14 @@ int match_push_refs(struct ref *src, struct ref **dst,
errs = match_explicit_refs(src, *dst, &dst_tail, rs, nr_refspec);
/* pick the remainder */
- for ( ; src; src = src->next) {
+ for (ref = src; ref; ref = ref->next) {
struct ref *dst_peer;
const struct refspec *pat = NULL;
char *dst_name;
- if (src->peer_ref)
+ if (ref->peer_ref)
continue;
- pat = check_pattern_match(rs, nr_refspec, src);
+ pat = check_pattern_match(rs, nr_refspec, ref);
if (!pat)
continue;
@@ -1184,13 +1184,14 @@ int match_push_refs(struct ref *src, struct ref **dst,
* including refs outside refs/heads/ hierarchy, but
* that does not make much sense these days.
*/
- if (!send_mirror && prefixcmp(src->name, "refs/heads/"))
+ if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
continue;
- dst_name = xstrdup(src->name);
+ dst_name = xstrdup(ref->name);
+
} else {
const char *dst_side = pat->dst ? pat->dst : pat->src;
- if (!match_name_with_pattern(pat->src, src->name,
+ if (!match_name_with_pattern(pat->src, ref->name,
dst_side, &dst_name))
die("Didn't think it matches any more");
}
@@ -1211,9 +1212,9 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* Create a new one and link it */
dst_peer = make_linked_ref(dst_name, &dst_tail);
- hashcpy(dst_peer->new_sha1, src->new_sha1);
+ hashcpy(dst_peer->new_sha1, ref->new_sha1);
}
- dst_peer->peer_ref = copy_ref(src);
+ dst_peer->peer_ref = copy_ref(ref);
dst_peer->force = pat->force;
free_name:
free(dst_name);
--
1.7.9.1
^ permalink raw reply related
* [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
From: Felipe Contreras @ 2012-02-17 19:12 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
In-Reply-To: <1329505957-24595-1-git-send-email-felipe.contreras@gmail.com>
There's a lot of code that can be consolidated there, and will be useful
for next patches.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
remote.c | 59 ++++++++++++++++++++++++++++++-----------------------------
1 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/remote.c b/remote.c
index 55d68d1..019aafc 100644
--- a/remote.c
+++ b/remote.c
@@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
return errs;
}
-static const struct refspec *check_pattern_match(const struct refspec *rs,
- int rs_nr,
- const struct ref *src)
+static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
+ int send_mirror, const struct refspec **ret_pat)
{
+ const struct refspec *pat;
+ char *name;
int i;
int matching_refs = -1;
for (i = 0; i < rs_nr; i++) {
@@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
continue;
}
- if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
- NULL, NULL))
- return rs + i;
+ if (rs[i].pattern) {
+ const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
+ if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+ matching_refs = i;
+ break;
+ }
+ }
}
- if (matching_refs != -1)
- return rs + matching_refs;
- else
+ if (matching_refs == -1)
return NULL;
+
+ pat = rs + matching_refs;
+ if (pat->matching) {
+ /*
+ * "matching refs"; traditionally we pushed everything
+ * including refs outside refs/heads/ hierarchy, but
+ * that does not make much sense these days.
+ */
+ if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
+ return NULL;
+ name = xstrdup(ref->name);
+ }
+ if (ret_pat)
+ *ret_pat = pat;
+ return name;
}
static struct ref **tail_ref(struct ref **head)
@@ -1171,36 +1189,19 @@ int match_push_refs(struct ref *src, struct ref **dst,
struct ref *dst_peer;
const struct refspec *pat = NULL;
char *dst_name;
+
if (ref->peer_ref)
continue;
- pat = check_pattern_match(rs, nr_refspec, ref);
- if (!pat)
+ dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
+ if (!dst_name)
continue;
- if (pat->matching) {
- /*
- * "matching refs"; traditionally we pushed everything
- * including refs outside refs/heads/ hierarchy, but
- * that does not make much sense these days.
- */
- if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
- continue;
- dst_name = xstrdup(ref->name);
-
-
- } else {
- const char *dst_side = pat->dst ? pat->dst : pat->src;
- if (!match_name_with_pattern(pat->src, ref->name,
- dst_side, &dst_name))
- die("Didn't think it matches any more");
- }
dst_peer = find_ref_by_name(*dst, dst_name);
if (dst_peer) {
if (dst_peer->peer_ref)
/* We're already sending something to this ref. */
goto free_name;
-
} else {
if (pat->matching && !(send_all || send_mirror))
/*
--
1.7.9.1
^ permalink raw reply related
* [RFC/PATCH 3/3] push: add 'prune' option
From: Felipe Contreras @ 2012-02-17 19:12 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
In-Reply-To: <1329505957-24595-1-git-send-email-felipe.contreras@gmail.com>
This will allow us to remove refs from the remote that have been removed
locally.
It's useful to conveniently synchronize all the local branches to
certain remote.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
builtin/push.c | 2 ++
remote.c | 29 ++++++++++++++++++++++++++---
remote.h | 3 ++-
transport.c | 2 ++
transport.h | 1 +
5 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..46b99b1 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
TRANSPORT_PUSH_SET_UPSTREAM),
OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+ OPT_BIT('p', "prune", &flags, "prune locally removed refs",
+ TRANSPORT_PUSH_PRUNE),
OPT_END()
};
diff --git a/remote.c b/remote.c
index 019aafc..0900bb5 100644
--- a/remote.c
+++ b/remote.c
@@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
}
static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
- int send_mirror, const struct refspec **ret_pat)
+ int send_mirror, int dir, const struct refspec **ret_pat)
{
const struct refspec *pat;
char *name;
@@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref
if (rs[i].pattern) {
const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
- if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+ int match;
+ if (dir == 0)
+ match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);
+ else
+ match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);
+ if (match) {
matching_refs = i;
break;
}
@@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
struct refspec *rs;
int send_all = flags & MATCH_REFS_ALL;
int send_mirror = flags & MATCH_REFS_MIRROR;
+ int send_prune = flags & MATCH_REFS_PRUNE;
int errs;
static const char *default_refspec[] = { ":", NULL };
struct ref *ref, **dst_tail = tail_ref(dst);
@@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
if (ref->peer_ref)
continue;
- dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
+ dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);
if (!dst_name)
continue;
@@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,
free_name:
free(dst_name);
}
+ if (send_prune) {
+ /* check for missing refs on the remote */
+ for (ref = *dst; ref; ref = ref->next) {
+ char *src_name;
+
+ if (ref->peer_ref)
+ /* We're already sending something to this ref. */
+ continue;
+
+ src_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);
+ if (src_name) {
+ if (!find_ref_by_name(src, src_name))
+ ref->peer_ref = try_explicit_object_name("");
+ free(src_name);
+ }
+ }
+ }
if (errs)
return -1;
return 0;
diff --git a/remote.h b/remote.h
index b395598..341142c 100644
--- a/remote.h
+++ b/remote.h
@@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);
enum match_refs_flags {
MATCH_REFS_NONE = 0,
MATCH_REFS_ALL = (1 << 0),
- MATCH_REFS_MIRROR = (1 << 1)
+ MATCH_REFS_MIRROR = (1 << 1),
+ MATCH_REFS_PRUNE = (1 << 2),
};
/* Reporting of tracking info */
diff --git a/transport.c b/transport.c
index cac0c06..c20267c 100644
--- a/transport.c
+++ b/transport.c
@@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,
match_flags |= MATCH_REFS_ALL;
if (flags & TRANSPORT_PUSH_MIRROR)
match_flags |= MATCH_REFS_MIRROR;
+ if (flags & TRANSPORT_PUSH_PRUNE)
+ match_flags |= MATCH_REFS_PRUNE;
if (match_push_refs(local_refs, &remote_refs,
refspec_nr, refspec, match_flags)) {
diff --git a/transport.h b/transport.h
index 059b330..5d30328 100644
--- a/transport.h
+++ b/transport.h
@@ -101,6 +101,7 @@ struct transport {
#define TRANSPORT_PUSH_MIRROR 8
#define TRANSPORT_PUSH_PORCELAIN 16
#define TRANSPORT_PUSH_SET_UPSTREAM 32
+#define TRANSPORT_PUSH_PRUNE 64
#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
--
1.7.9.1
^ permalink raw reply related
* Mixing and matching multiple projects
From: Junio C Hamano @ 2012-02-17 19:19 UTC (permalink / raw)
To: git
Imagine that there is an open source embedded appliance project "xyzzy"
that consists of two submodules and the top-level superproject that binds
them and serves as the scaffolding, perhaps organized like this:
/s/xyzzy/.git -- the superproject
/s/xyzzy/os/.git -- the "os" submodule
/s/xyzzy/u/.git -- the "userspace" submodule
The "os" submodule may contain various subdirectories, among which there
may be a device drivers directory. Further imagine that the Makefile in
that directory, /s/xyzzy/os/drivers/Makefile, is written in such a way
that it can run make in any subdirectory of drivers/ and can link with
resulting *.o files in these subdirectories.
Now suppose somebody wanted to add an add-on device driver for frotz
device that does not appear in the open source tree. Further suppose that
the open source folks do not want to accept support for "frotz" driver in
their tree for whatever reason.
At the working tree level, he can add /s/xyzzy/os/drivers/frotz/ directory
and populate its sources there. Everything else is already set up to build
and link to it properly. But how would he add that directory to git?
He could fork the /s/xyzzy/os/.git submodule and add "drivers/frotz/"
directory, and maintain his own fork of the submodule, which in turn means
that he has to fork the top-level superproject /s/xyzzy/.git because its
tree has to point at a commit in /s/xyzzy/os/.git repository that has the
"drivers/frotz" directory in it, which is different from the open source
version. A proprietary fork has to happen somewhere; that is a given.
But I wonder if we can do without forking the /s/xyzzy/os/.git submodule?
If we *could* do this:
cd /s/xyzzy
mkdir os/drivers/frotz && populate the directory with sources
git add os/drivers/frotz ;# to the top-level superproject!!
to add the "frotz driver" submodule directly to the superproject, then we
could leave /s/xyzzy/os/.git intact, letting it follow the open source
world. Because the superproject must be forked in order to keep track of
what happens in the appliance that supports the "frotz" driver anyway,
this could result in the minimum amount of forking from the end user's
point of view.
People who know the Git data structure of course can immediately see that
this is not supported (and I strongly suspect unsupportable, but my
suspicion has a history of being wrong from time to time, and that is why
I am sending this message). The index at /s/xyzzy/.git/index has to be
able to record "os" as a gitlink and "os/drivers/frotz/frotz.c" as a
regular blob at the same time, which is a D/F conflict.
Even if you modify the index D/F conflict check to allow this, you cannot
write the top-level tree object for the superproject, as "os" needs to be
recorded as a gitlink to bind the /s/xyzzy/os/.git submodule, while you
also have to have another "os" in the same top-level tree object as a tree
object that has a single entry "drivers" (which is a tree) in it, under
which all the "os/drivers/frotz" subdirectory hang. All tree traversal
code would be very unhappy to see such a tree with "duplicate" entry,
including the unpack-trees machinery used for merging and switching
branches, connectivity machinery used for fsck, object enumeration, and
object transfer. I imagine that it *could* be done, but it won't be a
mere two-weekend hack; it has to first destroy pretty much everything in
the current codebase and rebuild it to add such a support.
The "frotz" submodule could be added as /s/xyzzy/frotz/.git and the build
infrastructure at /s/xyzzy/.git (which we are going to fork anyway) could
be tweaked so that it creates a symbolic link in /s/xyzzy/os/drivers to
point there, pretending that there is a 'frotz' subdirectory. While that
would certainly be a workaround, I am wondering if people who have used
submodules and/or nested projects more than I have better solutions to
this puzzle.
Comments?
^ permalink raw reply
* Re: How to use git attributes to configure server-side checks?
From: Junio C Hamano @ 2012-02-17 19:26 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git discussion list
In-Reply-To: <4F3E9F86.3070802@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> I was just alerted by Scott Chacon's blog [1] to the fact that one can
> set GIT_INDEX_FILE to an arbitrary filename, thereby causing the index
> to be read/written from that file instead of $GIT_DIR/index.
That's very old fashioned. For almost five years, the preferred way to
say that has been "git read-tree --index-output=file" ;-)
But what you outlined should work.
^ permalink raw reply
* Re: Mixing and matching multiple projects
From: Junio C Hamano @ 2012-02-17 19:49 UTC (permalink / raw)
To: git
In-Reply-To: <7vhayptght.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> But I wonder if we can do without forking the /s/xyzzy/os/.git submodule?
>
> If we *could* do this:
>
> cd /s/xyzzy
> mkdir os/drivers/frotz && populate the directory with sources
A crucial step was missing here for my story to make *any* sense. Sorry.
A step to create a repository at /s/xyzzy/os/drivers/frotz/.git must be
here, like this:
(cd os/drivers/frotz && git init && git add . && git commit)
And then that is added as a submodule to the superproject.
> git add os/drivers/frotz ;# to the top-level superproject!!
>
> to add the "frotz driver" submodule directly to the superproject, then we
> could leave /s/xyzzy/os/.git intact, letting it follow the open source
> world. Because the superproject must be forked in order to keep track of
> what happens in the appliance that supports the "frotz" driver anyway,
> this could result in the minimum amount of forking from the end user's
> point of view.
>
> People who know the Git data structure of course can immediately see that
> this is not supported (and I strongly suspect unsupportable, but my
> suspicion has a history of being wrong from time to time, and that is why
> I am sending this message). The index at /s/xyzzy/.git/index has to be
> able to record "os" as a gitlink and "os/drivers/frotz/frotz.c" as a
> regular blob at the same time, which is a D/F conflict.
>
> Even if you modify the index D/F conflict check to allow this, you cannot
> write the top-level tree object for the superproject, as "os" needs to be
> recorded as a gitlink to bind the /s/xyzzy/os/.git submodule, while you
> also have to have another "os" in the same top-level tree object as a tree
> object that has a single entry "drivers" (which is a tree) in it, under
> which all the "os/drivers/frotz" subdirectory hang. All tree traversal
> code would be very unhappy to see such a tree with "duplicate" entry,
> including the unpack-trees machinery used for merging and switching
> branches, connectivity machinery used for fsck, object enumeration, and
> object transfer. I imagine that it *could* be done, but it won't be a
> mere two-weekend hack; it has to first destroy pretty much everything in
> the current codebase and rebuild it to add such a support.
>
> The "frotz" submodule could be added as /s/xyzzy/frotz/.git and the build
> infrastructure at /s/xyzzy/.git (which we are going to fork anyway) could
> be tweaked so that it creates a symbolic link in /s/xyzzy/os/drivers to
> point there, pretending that there is a 'frotz' subdirectory. While that
> would certainly be a workaround, I am wondering if people who have used
> submodules and/or nested projects more than I have better solutions to
> this puzzle.
>
> Comments?
^ permalink raw reply
* Re: How to use git attributes to configure server-side checks?
From: Junio C Hamano @ 2012-02-17 19:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git discussion list
In-Reply-To: <7vd39dtg5t.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> I was just alerted by Scott Chacon's blog [1] to the fact that one can
>> set GIT_INDEX_FILE to an arbitrary filename, thereby causing the index
>> to be read/written from that file instead of $GIT_DIR/index.
>
> That's very old fashioned. For almost five years, the preferred way to
> say that has been "git read-tree --index-output=file" ;-)
Ah, I take that back.
There is any benefit to be had by using "read-tree --index-output" in the
way used in your check-attr example. Setting up a single GIT_INDEX_FILE
and using it throughout as you did is indeed much more preferrable.
> But what you outlined should work.
^ permalink raw reply
* Re: Mixing and matching multiple projects
From: Seth Robertson @ 2012-02-17 20:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhayptght.fsf@alter.siamese.dyndns.org>
In message <7vhayptght.fsf@alter.siamese.dyndns.org>, Junio C Hamano writes:
/s/xyzzy/.git -- the superproject
/s/xyzzy/os/.git -- the "os" submodule
/s/xyzzy/u/.git -- the "userspace" submodule
At the working tree level, he can add /s/xyzzy/os/drivers/frotz/ directory
and populate its sources there. Everything else is already set up to build
and link to it properly. But how would he add that directory to git?
But I wonder if we can do without forking the /s/xyzzy/os/.git submodule?
I am wondering if people who have used submodules and/or nested
projects more than I have better solutions to this puzzle.
Using gitslave [http://gitslave.sf.net] instead of git-submodules, you
can *almost* get there.
Specifically, gitslave allows the superproject /s/xyzzy to attach a
subproject at /s/xyzzy/os/drivers/frotz (either recursively through
/os which you don't want to do or directly).
However, the problem is the .gitignore in the os subproject. If we
didn't have some way to ignore drivers/frotz, various git commands
would get annoyed. Adding it in the os subproject requires forking
which you said you didn't want to do. However, you could use
core.excludesfile in the os subproject to ignore the necessary file.
gitslave doesn't support automatically using core.excludesfile (and
indeed would automatically make an entry in os/.gitignore when you set
up the hierarchy). However, it isn't a lot of work to revert that
.gitignore commit and set up core.excludesfile; but setting up
core.excludesfile on every clone might get annoying. It probably
wouldn't be too difficult to tearch gitslave how to do that
automagically.
Is using this third party tool and making the necessary changes or
manually doing the work after every clone worth it? Do you need the
git-submodule semantics of freezing the subprojects at a particular
SHA instead of floating at the tip of a branch? Are the branches used
between xyzzy and os desynchronized? Only you can judge. Actually it
seems possible that you could use gitslave for os/drivers/frotz and
git-submodules for os. Whether that is wise…
I would be open to moving gitslave into contrib similar to what
git-subtree is under the process of doing, but gitslave would
certainly need some cleanup since it grew rather organically.
-Seth Robertson
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-17 20:37 UTC (permalink / raw)
To: Piotr Krukowiecki; +Cc: Thomas Rast, Git Mailing List, Nguyen Thai Ngoc Duy
In-Reply-To: <CAA01Csq6vSekW=Fa236bB0H3LVtN43Gb2aLMVE+A1wVyUqYJ7A@mail.gmail.com>
On Fri, Feb 17, 2012 at 06:19:06PM +0100, Piotr Krukowiecki wrote:
> "git update-index --refresh" with dropped cache took
> real 0m3.726s
> user 0m0.024s
> sys 0m0.404s
> [...]
> The diff-index after dropping cache takes
> real 0m14.095s
> user 0m0.268s
> sys 0m0.564s
OK, that suggests to me that the real culprit is the I/O we spend in
accessing the object db, since that is the main I/O that happens in the
second command but not the first.
> > Mostly reading (we keep a sorted index and access the packfiles via
> > mmap, so we only touch the pages we need). But you're also paying to
> > lstat() the directory tree, too. And you're paying to load (probably)
> > the whole index into memory, although it's relatively compact compared
> > to the actual file data.
>
> If the index is the objects/pack/*.idx files than it's 21MB
Yes, that's it. Though we don't necessarily read the whole thing. The
sorted list of sha1s is only a part of that. And we mmap and
binary-search that, so we only have to fault in pages that are actually
used in our binary search.
However, we're faulting in random pages of the index in series, so it
may actually have a lot of latency. You can see how expensive the I/O on
the index is with something like this:
[whole operation, for reference]
$ sudo sh -c 'echo 3 >/proc/sys/vm/drop_caches'
$ time git diff-index HEAD
real 0m2.636s
user 0m0.248s
sys 0m0.392s
[prime the cache with just the index]
$ sudo sh -c 'echo 3 >/proc/sys/vm/drop_caches'
$ time cat .git/objects/pack/*.idx >/dev/null
real 0m0.288s
user 0m0.000s
sys 0m0.028s
$ time git diff-index HEAD
real 0m2.175s
user 0m0.272s
sys 0m0.320s
So roughly 20% of the I/O time in my case went to faulting in the index.
You could pre-fault in the index, which would give the OS a chance to do
read-ahead caching. You can see that the combined cat and diff-index
times are still lower than the raw diff-index time. You could also do
them in parallel, but that will create some additional seeks as the
threads fight for the disk, but may be a win in the long run because we
can read bigger chunks. You can roughly simulate it by running the "cat"
and the "diff-index" above in parallel. I get:
real 0m2.464s
user 0m0.284s
sys 0m0.372s
which is almost exactly the same as doing them separately (though note
that this is on an SSD, so seeking is very cheap).
But the bulk of the time still goes to actually retrieving the object
data, so that's probably a more interesting area to focus, anyway (and
if we can reduce object accesses, we reduce their lookup, too :) ).
> If I understand correctly, you only need to compute sha1 on the
> workdir files and compare it with sha1 files recorded in index/gitdir.
> It seems that to get the sha1 from index/gitdir I need to read the
> packfiles? Maybe it'd be possible to cache/index it somehow, for
> example in separate and smaller file?
There are two diffs going on in "git status". One is a comparison
between index and worktree. In that one, you need to lstat each file to
make sure the cached sha1 we have in the index is up to date. Assuming
it is, you don't need to touch the file data at all. Then you compare
that sha1 to the stage 0 sha1 (i.e., what we typically think of as
"staged for commit"). If they match, you don't need to do more work.
But the expensive diff-index we've been doing above is comparing the
index to the HEAD tree. And doing that is a little trickier. The index
is a flat list of files with their sha1s. But the HEAD tree is stored
hierarchically. So to get the sha1 of foo/bar/baz, we have to access the
root tree object, find the "foo" entry, access its tree object, find the
"bar" entry, access its tree object, and then find the "baz" entry. Then
we compare the sha1 of the "baz" entry to what's in the index.
So what's where your I/O comes from: accessing each of the tree objects.
And that fact that it isn't just "compare the HEAD and index sha1s" is
that the index is stored as a list of flat files.
That being said, we do have an index extension to store the tree sha1 of
whole directories (i.e., we populate it when we write a whole tree or
subtree into the index from the object db, and it becomes invalidated
when a file becomes modified). This optimization is used by things like
"git commit" to avoid having to recreate the same sub-trees over and
over when creating tree objects from the index. But we could also use it
here to avoid having to even read the sub-tree objects from the object
db.
> No, it's ext4 and the disk Seagate Barracuda 7200.12 500GB, as it
> reads on the cover :)
>
> But IMO faster disk won't help with this - times will be smaller, but
> you'll still have to read the same data, so the subdir times will be
> just 2x faster than whole repo, won't it? So maybe in my case it will
> go down to e.g. 2s on subdir, but for someone with larger repository
> it will still be 10s...
Sure. But a certain amount of I/O is going to be unavoidable to get the
answer to your question. So you will never be able to achieve the
warm-cache case. I'm not saying we can't improve (e.g., I think the
index extension thing I mentioned above is a promising approach). But we
have to be realistic about what will make things faster; if I/O is your
problem, faster disk is one possible solution (especially because some
of this is related to seeking and latency, an SSD is a nice improvement
for cold-cache times).
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 3/3] push: add 'prune' option
From: Jeff King @ 2012-02-17 22:25 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Junio C Hamano
In-Reply-To: <1329505957-24595-4-git-send-email-felipe.contreras@gmail.com>
On Fri, Feb 17, 2012 at 09:12:37PM +0200, Felipe Contreras wrote:
> This will allow us to remove refs from the remote that have been removed
> locally.
>
> It's useful to conveniently synchronize all the local branches to
> certain remote.
Thanks for picking up this topic again.
There's one subtlety in the motivation above that you may find helpful
when you end up writing the documentation: "push --mirror" already does
this sort of pruning, but it _also_ implies that we are pushing
"refs/*:refs/*". So this is really about giving access to the pruning
half, but still being able to use custom refspecs. So the features
together might end up being explained something like:
--prune::
... prune things that no longer exist locally ...
--mirror::
... turn on --prune, and also match all refs ...
At least that is my understanding of how the code is meant to work.
> builtin/push.c | 2 ++
> remote.c | 29 ++++++++++++++++++++++++++---
> remote.h | 3 ++-
> transport.c | 2 ++
> transport.h | 1 +
> 5 files changed, 33 insertions(+), 4 deletions(-)
I've just given a quick read to the patches so far, but I did notice
this:
> static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
> - int send_mirror, const struct refspec **ret_pat)
> + int send_mirror, int dir, const struct refspec **ret_pat)
The "dir" flag looks like it is meant to be short for "direction". But
the callers only pass 0 or 1. I'm not clear which direction is which.
Either symbolic constants for directions, or perhaps giving it a more
boolean name like "match_to_dst" might make it more clear.
I'll try to take a closer look later tonight.
-Peff
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-17 22:25 UTC (permalink / raw)
To: Jeff King
Cc: Piotr Krukowiecki, Thomas Rast, Git Mailing List,
Nguyen Thai Ngoc Duy
In-Reply-To: <20120217203755.GA30114@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That being said, we do have an index extension to store the tree sha1 of
> whole directories (i.e., we populate it when we write a whole tree or
> subtree into the index from the object db, and it becomes invalidated
> when a file becomes modified). This optimization is used by things like
> "git commit" to avoid having to recreate the same sub-trees over and
> over when creating tree objects from the index. But we could also use it
> here to avoid having to even read the sub-tree objects from the object
> db.
Like b65982b (Optimize "diff-index --cached" using cache-tree, 2009-05-20)
perhaps?
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-17 22:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Piotr Krukowiecki, Thomas Rast, Git Mailing List,
Nguyen Thai Ngoc Duy
In-Reply-To: <7vaa4hrtbe.fsf@alter.siamese.dyndns.org>
On Fri, Feb 17, 2012 at 02:25:25PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > That being said, we do have an index extension to store the tree sha1 of
> > whole directories (i.e., we populate it when we write a whole tree or
> > subtree into the index from the object db, and it becomes invalidated
> > when a file becomes modified). This optimization is used by things like
> > "git commit" to avoid having to recreate the same sub-trees over and
> > over when creating tree objects from the index. But we could also use it
> > here to avoid having to even read the sub-tree objects from the object
> > db.
>
> Like b65982b (Optimize "diff-index --cached" using cache-tree, 2009-05-20)
> perhaps?
That's what I get for speaking before running "git log".
So yeah, we may be about as reasonably fast as we can go. Or maybe that
optimization isn't kicking in for some reason. I think going further
would require Piotr to do more profiling.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 1/3] remote: use a local variable in match_push_refs()
From: Junio C Hamano @ 2012-02-17 22:31 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <1329505957-24595-2-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> So that we can reuse src later on.
>
> Will be useful in next patches.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
If the 2/3 were much less complex, it might have made sense to squash this
into that, but I think it makes it easier to see what is happening if an
obvious and safe no-op change like this is done as a separate patch like
this series did.
Looking good so far.
^ permalink raw reply
* Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
From: Junio C Hamano @ 2012-02-17 22:34 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <1329505957-24595-3-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> There's a lot of code that can be consolidated there, and will be useful
> for next patches.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> remote.c | 59 ++++++++++++++++++++++++++++++-----------------------------
> 1 files changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 55d68d1..019aafc 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
> return errs;
> }
>
> -static const struct refspec *check_pattern_match(const struct refspec *rs,
> - int rs_nr,
> - const struct ref *src)
> +static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
> + int send_mirror, const struct refspec **ret_pat)
> {
For a change that not just adds parameters but removes an existing one,
this is way under-described with neither in-code comment nor log message.
So I'll have to think aloud in this review. Take it as a chance to learn
how the thought process of other people with lessor intelligence than you
have might work, and how to help those slower than you are ;-)
> + const struct refspec *pat;
> + char *name;
> int i;
> int matching_refs = -1;
> for (i = 0; i < rs_nr; i++) {
> @@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
> continue;
> }
>
> - if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
> - NULL, NULL))
> - return rs + i;
> + if (rs[i].pattern) {
> + const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
> + if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
> + matching_refs = i;
> + break;
We used to discard what match_name_with_pattern() finds out by matching a
wildcard refspec against the ref by passing two NULLs. This updates the
code to capture what destination ref ref->name is mapped to, by using the
same logic as the original and only caller, i.e. 'foo' without destination
maps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.
This function is not used by fetching side of the codepath, so we do not
have to worry about its need to use different dst_side selection logic
(i.e. 'foo' without destination maps to "do not store anywhere other than
FETCH_HEAD"). Good.
> + }
> + }
> }
> -...
> + if (matching_refs == -1)
> return NULL;
> +
> + pat = rs + matching_refs;
> + if (pat->matching) {
> + /*
> + * "matching refs"; traditionally we pushed everything
> + * including refs outside refs/heads/ hierarchy, but
> + * that does not make much sense these days.
> + */
> + if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
> + return NULL;
> + name = xstrdup(ref->name);
> + }
So you are moving some code from what the sole caller of this function
does after calling us, and that is where the new parameters come from.
And by doing so, you do not have to run the same match_name_with_pattern()
again. OK.
> + if (ret_pat)
> + *ret_pat = pat;
> + return name;
> }
You did not initialize name to anything at the beginning, but if the
earlier match_name_with_pattern() didn't find anything, we could only come
here after matching_refs is set by the if (rs[i].matching) codepath; by
the time we come here, we would have xstrdup(ref->name) in name, so we
would never return a garbage pointer to the caller. OK.
> static struct ref **tail_ref(struct ref **head)
> @@ -1171,36 +1189,19 @@ int match_push_refs(struct ref *src, struct ref **dst,
> struct ref *dst_peer;
> const struct refspec *pat = NULL;
> char *dst_name;
> +
> if (ref->peer_ref)
> continue;
>
> - pat = check_pattern_match(rs, nr_refspec, ref);
> - if (!pat)
> + dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
> + if (!dst_name)
> continue;
>
> - if (pat->matching) {
> - /*
> - * "matching refs"; traditionally we pushed everything
> - * including refs outside refs/heads/ hierarchy, but
> - * that does not make much sense these days.
> - */
> - if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
> - continue;
> - dst_name = xstrdup(ref->name);
> -
> -
> - } else {
> - const char *dst_side = pat->dst ? pat->dst : pat->src;
> - if (!match_name_with_pattern(pat->src, ref->name,
> - dst_side, &dst_name))
> - die("Didn't think it matches any more");
> - }
> dst_peer = find_ref_by_name(*dst, dst_name);
> if (dst_peer) {
> if (dst_peer->peer_ref)
> /* We're already sending something to this ref. */
> goto free_name;
> -
> } else {
> if (pat->matching && !(send_all || send_mirror))
> /*
OK, it is easy to tell that the patch is trivially correct, once a reader
figures out that the patch is really about:
Move code to check_pattern_match() from its sole caller to make it
unnecessary to call match_name_with_pattern() twice.
Saying so in the log message would have prepared the reader, instead of
the "There's a lot of code that can be consolidated there." which does not
give hints on what to look for in the patch.
Also this changes the semantics (because it changed the meaning of its
return value) of check_pattern_match() so much that it would deserve a
rename, which I will address in my review of 3/3.
Otherwise this step looks good.
^ 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