* Re: [PATCH] Fix problem with authentification on http repository.
From: Mike Hommey @ 2008-07-09 8:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <7vhcazlbch.fsf@gitster.siamese.dyndns.org>
On Wed, Jul 09, 2008 at 01:18:22AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > On Tue, Jul 08, 2008 at 11:28:15PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > ...
> >> >> Actually Documentation/howto/setup-git-server-over-http.txt
> >> >> talks about http://user@server/path/ format. How well does this
> >> >> work in practice? If it does, we should update Documentation/urls.txt
> >> >> to allow optional user@ there like...
> >> >
> >> > AFAIK, there is nothing to deal with asking a password to the user in git
> >> > in this case, so that doesn't work.
> >>
> >> Then perhaps the howto needs updating?
> >
> > Actually, the howto tells to setup a ~/.netrc file, which should make
> > the http://user@server/path/ format work. I don't know if dropping user@
> > would still work, that depends how libcurl works with it.
>
> Ok, so the documentation update I suggested earlier may actually be
> valid, right?
The problem is that these urls won't work out of the box if the user
doesn't setup a .netrc file. I'd say it would be better not to add these
forms in Documentation/urls.txt until git prompts for the password
when it is missing.
Note that http://user:pass@server/path/ /should/ work (but that would
need validation), though not a good idea on command line.
Mike
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Junio C Hamano @ 2008-07-09 8:18 UTC (permalink / raw)
To: Mike Hommey; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <20080709075345.GA22022@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Tue, Jul 08, 2008 at 11:28:15PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> >> Actually Documentation/howto/setup-git-server-over-http.txt
>> >> talks about http://user@server/path/ format. How well does this
>> >> work in practice? If it does, we should update Documentation/urls.txt
>> >> to allow optional user@ there like...
>> >
>> > AFAIK, there is nothing to deal with asking a password to the user in git
>> > in this case, so that doesn't work.
>>
>> Then perhaps the howto needs updating?
>
> Actually, the howto tells to setup a ~/.netrc file, which should make
> the http://user@server/path/ format work. I don't know if dropping user@
> would still work, that depends how libcurl works with it.
Ok, so the documentation update I suggested earlier may actually be
valid, right?
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Daniel Stenberg @ 2008-07-09 8:04 UTC (permalink / raw)
To: git
In-Reply-To: <20080709075345.GA22022@glandium.org>
On Wed, 9 Jul 2008, Mike Hommey wrote:
> Actually, the howto tells to setup a ~/.netrc file, which should make
> the http://user@server/path/ format work. I don't know if dropping user@
> would still work, that depends how libcurl works with it.
git has the netrc parser set to "optional", which makes libcurl use the login
name given in the URL (if present) and search for the associated password in
the .netrc file.
Typical users probably only have one user per host name in the .netrc file and
then the name part in the URL provides no particular extra goodness but can
just as well be excluded.
--
/ daniel.haxx.se
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Mike Hommey @ 2008-07-09 7:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <7vlk0blgg0.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 08, 2008 at 11:28:15PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > On Tue, Jul 08, 2008 at 10:44:58PM -0700, Junio C Hamano wrote:
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>
> >> > On Sat, 29 Sep 2007, Junio C Hamano wrote:
> >> >
> >> >> We never supported URLs with embedded credentials (see
> >> >> Documentation/urls.txt), partly because nobody asked for it, but
> >> >> more importantly because giving -n to curl to have it read from
> >> >> user's .netrc is generally much more preferred approach.
> >> >
> >> > To elaborate on that: if you fetch from somewhere, your url, username and
> >> > password can be read from the output of "ps ax | grep http" very easily.
> >>
> >> Actually Documentation/howto/setup-git-server-over-http.txt
> >> talks about http://user@server/path/ format. How well does this
> >> work in practice? If it does, we should update Documentation/urls.txt
> >> to allow optional user@ there like...
> >
> > AFAIK, there is nothing to deal with asking a password to the user in git
> > in this case, so that doesn't work.
>
> Then perhaps the howto needs updating?
Actually, the howto tells to setup a ~/.netrc file, which should make
the http://user@server/path/ format work. I don't know if dropping user@
would still work, that depends how libcurl works with it.
Mike
^ permalink raw reply
* Re: [PATCH] parse-options: add PARSE_OPT_FAKELASTARG flag.
From: Pierre Habouzit @ 2008-07-09 7:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Hjemli
In-Reply-To: <7vvdzfoo1s.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]
On Wed, Jul 09, 2008 at 01:15:43AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > If you set this for a given flag, and the flag appears without a value on
> > the command line, then the `defval' is used to fake a new argument.
> >
> > Note that this flag is meaningless in presence of OPTARG or NOARG flags.
> > (in the current implementation it will be ignored, but don't rely on it).
> >
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> >
> > > (3) inspired from (1) and (2), have a flag for options that says
> > > "I do take an argument, but if I'm the last option on the
> > > command line, please fake this argument for me.
> > >
> > > I really like (3) more FWIW as it doesn't generate ambiguous
> > > parsers like (2) would, and it's not horrible like (1). And cherry
> > > on top it's pretty trivial to implement I think.
>
> Yeah, I do not particularly want a major rewrite that only introduces
> possible ambiguity to the option parser (even though arguably it would add
> to the usability very much, just like we accept revs and paths when
> unambiguous), so I think this is a reasonable compromise.
>
> It feels more like LASTARG_DEFAULT but that is bikeshedding ;-)
I absolutely don't like this FAKELASTARG name, so really, use what you
like.
> But I see one thinko (fix below) and another issue I am not sure what the
> best fix would be.
Like I said it was just a draft, I did not test the new feature, so
I'm not really surprised it's partly broken ;)
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] branch --merged/--not-merged: allow specifying arbitrary commit
From: Pierre Habouzit @ 2008-07-09 7:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Hjemli
In-Reply-To: <7vmykronr1.fsf_-_@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]
On Wed, Jul 09, 2008 at 01:22:10AM +0000, Junio C Hamano wrote:
> "git-branch --merged" is a handy way to list all the branches that have
> already been merged to the current branch, but it did not allow checking
> against anything but the current branch. Having to check out only for
> that purpose made the command practically useless.
>
> This updates the option parser so that "git branch --merged next" is
> accepted when you are on 'master' branch.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * This does have an issue. --no-<option>=<value> is often nonsense and
> parse-options does not accept it (and I do not think we would want to
> change it). The use of "--no-merged" was a mistake, but nobody has
> perfect foresight.
>
> This adds --not-merged <commit> and allows the <commit> to default to
> HEAD if not given to work it around. This and the previous one are not
> for application but primarily meant for discussion on what further
> flexibility we may want to have in parse-options.
There's a way: declare --merge as (PARSE_OPT_)NONEG to tell parse-opt
not to generate --no-merge by itself, and declare a --no-merge option as
well. I think it works, and if not, we should make parse-opt work with
that. This is a quite disgusting hack, and no *new* options should be
written this way, but we have to be flexible enough for backward
compatibility's sake.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* [PATCH] Correct pack memory leak causing git gc to try to exceed ulimit
From: Shawn O. Pearce @ 2008-07-09 7:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nicolas Pitre
When recursing to unpack a delta base we must unuse_pack() so that
the pack window for the current object does not remain pinned in
memory while the delta base is itself being unpacked and materialized
for our use.
On a long delta chain of 50 objects we may need to access 6 different
windows from a very large (>3G) pack file in order to obtain all
of the delta base content. If the process ulimit permits us to
map/allocate only 1.5G we must release windows during this recursion
to ensure we stay within the ulimit and transition memory from pack
cache to standard malloc, or other mmap needs.
Inserting an unuse_pack() call prior to the recursion allows us to
avoid pinning the current window, making it available for garbage
collection if memory runs low.
This has been broken since at least before 1.5.1-rc1, and very
likely earlier than that. Its fixed now. :)
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
sha1_file.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index d1c4060..10346b6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1609,6 +1609,7 @@ static void *unpack_delta_entry(struct packed_git *p,
off_t base_offset;
base_offset = get_delta_base(p, w_curs, &curpos, *type, obj_offset);
+ unuse_pack(w_curs);
base = cache_or_unpack_entry(p, base_offset, &base_size, type, 0);
if (!base)
die("failed to read delta base object"
--
1.5.6.74.g8a5e
^ permalink raw reply related
* Re: [PATCH] Fix problem with authentification on http repository.
From: Junio C Hamano @ 2008-07-09 6:28 UTC (permalink / raw)
To: Mike Hommey; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <20080709055150.GA28482@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Tue, Jul 08, 2008 at 10:44:58PM -0700, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > On Sat, 29 Sep 2007, Junio C Hamano wrote:
>> >
>> >> We never supported URLs with embedded credentials (see
>> >> Documentation/urls.txt), partly because nobody asked for it, but
>> >> more importantly because giving -n to curl to have it read from
>> >> user's .netrc is generally much more preferred approach.
>> >
>> > To elaborate on that: if you fetch from somewhere, your url, username and
>> > password can be read from the output of "ps ax | grep http" very easily.
>>
>> Actually Documentation/howto/setup-git-server-over-http.txt
>> talks about http://user@server/path/ format. How well does this
>> work in practice? If it does, we should update Documentation/urls.txt
>> to allow optional user@ there like...
>
> AFAIK, there is nothing to deal with asking a password to the user in git
> in this case, so that doesn't work.
Then perhaps the howto needs updating?
^ permalink raw reply
* Re: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Junio C Hamano @ 2008-07-09 6:24 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: Johannes Schindelin, git@vger.kernel.org
In-Reply-To: <7f9d599f0807082226oee83bedrf13d254ae12be274@mail.gmail.com>
"Geoffrey Irving" <irving@naml.us> writes:
> On Tue, Jul 8, 2008 at 10:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>>> }
Please don't quote the whole thing without trimming if you do not have any
interspersed comments/responses to quoted part.
> Should I rewrite the patch sequence to incorporate these changes into
> the first commit, or add them as a forth commit off the end?
I strongly encourage the latter. We try not to keep early mistakes in the
history (see my comments on your [2/3]).
It is not unusal for any sizeable new code to go through a few round of
review cycle without even queued to 'pu', and the general rule is until
the series hits 'next', it is either "rejected (dropped on the floor),
please resend an improved version" or "ok now it is good, will queue".
After queued in 'next', improvements will continue incrementally.
Think of this procedure as giving a chance for you to hide early
embarrassment under the rug ;-)
^ permalink raw reply
* Re: [PATCH] git-submodule - make "submodule add" more strict, and document it
From: Junio C Hamano @ 2008-07-09 6:04 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git, sylvain.joyeux, hjemli, pkufranky
In-Reply-To: <1215575965-3588-1-git-send-email-mlevedahl@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> This change makes "submodule add" much more strict in the arguments it
> takes, and is intended to address confusion as recently noted on the
> git-list. With this change, the required syntax is:
> $ git submodule add URL path
>
Please have an extra blank line on both sides of examples.
> Specifically, this eliminates the form
> $git submodule add URL
> which was confused by more than one person as
> $git submodule add path
s/\$/& /;
> The change eliminates one form of URL: a path relative to the current
> working directory....
> ...
> Following this change, there are exactly four variants of
> submodule-add, as both arguments have two flavors:
>
> URL can be absolute, or can begin with ./|../ and thus name the origin
> relative to the top-level origin.
Now this _is_ confusing. Examples of a path relative to the current
working directory would be ./foo or ../../foo but the previous paragraph
says the form is eliminated. I think I know what you want to say, but it
still is confusing.
> add::
> Add the given repository as a submodule at the given path
> + to the changeset to be committed next. This requires two arguments,
> + <repository> and <path>. <repository> is the URL of the new
> + submodule's origin repository. This may be either an absolute URL,
> + or (if it begins with ./ or ../), the location relative
> + to the parent repository's origin.
This is much better than the part I found confusing above. Here, "parent
repository" actually means "this repository", right? It is the one that
owns the index you are adding the gitlink to and tracks the .gitmodules
file you are adding an entry for this submodule to.
> + <path> is the relative location for the cloned submodule to
> + exist in the current tree. If <path> does not exist, then the
> + module is created by cloning from the named URL. If <path> does
> + exist and is already a valid git repository, then this is added
> + to the changeset without cloning. This second form is provided
> + to ease adding a submodule to a project the first time, and presumes
> + the user will later push the new submodule to the given URL.
> +
> + In either case, the given URL is recorded into .gitmodules for
> + use by subsequent users cloning the project. If the URL is given
> + relative to the parent, the presumption is the main and sub-modules
We seem to say "superproject" and "submodule" elsewhere, including Linus's
talk.
> + will be kept together in the same relative location, and only the
> + top-level URL need be provided: git-submodule will correctly
> + locat the submodules using the hint in .gitmodules.
s/locat/&e/;
> @@ -150,16 +138,27 @@ cmd_add()
> ...
> + # assure repo is absolute or relative to parent
> + case "$repo" in
> + ./*|../*)
> + # dereference source url relative to parent's url
> + realrepo=$(resolve_relative_url "$repo") || exit
> + ;;
> + *:*|/*)
Funny indentation; "case/esac" and its arms align, like so:
case "$repo" in
./* | ../*)
...
;;
esac
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Mike Hommey @ 2008-07-09 5:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <7v4p6zmx0l.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 08, 2008 at 10:44:58PM -0700, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Sat, 29 Sep 2007, Junio C Hamano wrote:
> >
> >> We never supported URLs with embedded credentials (see
> >> Documentation/urls.txt), partly because nobody asked for it, but
> >> more importantly because giving -n to curl to have it read from
> >> user's .netrc is generally much more preferred approach.
> >
> > To elaborate on that: if you fetch from somewhere, your url, username and
> > password can be read from the output of "ps ax | grep http" very easily.
>
> Actually Documentation/howto/setup-git-server-over-http.txt
> talks about http://user@server/path/ format. How well does this
> work in practice? If it does, we should update Documentation/urls.txt
> to allow optional user@ there like...
AFAIK, there is nothing to deal with asking a password to the user in git
in this case, so that doesn't work.
Mike
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Junio C Hamano @ 2008-07-09 5:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: jean.guyader, git, Jean Guyader
In-Reply-To: <7v4phdja01.fsf@gitster.siamese.dyndns.org>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Sat, 29 Sep 2007, Junio C Hamano wrote:
>
>> We never supported URLs with embedded credentials (see
>> Documentation/urls.txt), partly because nobody asked for it, but
>> more importantly because giving -n to curl to have it read from
>> user's .netrc is generally much more preferred approach.
>
> To elaborate on that: if you fetch from somewhere, your url, username and
> password can be read from the output of "ps ax | grep http" very easily.
Actually Documentation/howto/setup-git-server-over-http.txt
talks about http://user@server/path/ format. How well does this
work in practice? If it does, we should update Documentation/urls.txt
to allow optional user@ there like...
---
Documentation/urls.txt | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/urls.txt b/Documentation/urls.txt
index e67f914..c9aab86 100644
--- a/Documentation/urls.txt
+++ b/Documentation/urls.txt
@@ -6,8 +6,8 @@ to name the remote repository:
===============================================================
- rsync://host.xz/path/to/repo.git/
-- http://host.xz/path/to/repo.git/
-- https://host.xz/path/to/repo.git/
+- http://{startsb}user@{endsb}host.xz/path/to/repo.git/
+- https://{startsb}user@{endsb}host.xz/path/to/repo.git/
- git://host.xz/path/to/repo.git/
- git://host.xz/~user/path/to/repo.git/
- ssh://{startsb}user@{endsb}host.xz{startsb}:port{endsb}/path/to/repo.git/
^ permalink raw reply related
* Re: [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
From: Junio C Hamano @ 2008-07-09 5:27 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <7f9d599f0807082056o666fced9nf87cc81447e16e05@mail.gmail.com>
"Geoffrey Irving" <irving@naml.us> writes:
> From c4e60c28fe66985ac8224da832589c982010744e Mon Sep 17 00:00:00 2001
> From: Geoffrey Irving <irving@naml.us>
> Date: Tue, 8 Jul 2008 19:47:22 -0700
> Subject: [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
>
> Pulling common code from get_cached_sha1_entry and set_cached_sha1_entry
> into static find_helper function.
> ---
Sign-off?
> cached-sha1-map.c | 68 +++++++++++++++++++++++++++++-----------------------
> 1 files changed, 38 insertions(+), 30 deletions(-)
The refactoring is good, and it should have been that way from the
beginning. Please don't send in "introduce foo.c [1/N]", "oops, initial
version of foo.c was crap, here is a fixup [2/N]".
> diff --git a/cached-sha1-map.c b/cached-sha1-map.c
> index e363745..147c7a2 100644
> --- a/cached-sha1-map.c
> +++ b/cached-sha1-map.c
> @@ -140,43 +144,47 @@ int get_cached_sha1_entry(struct cached_sha1_map *cache,
> mask = cache->size - 1;
>
> for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
> - if (!hashcmp(key, cache->entries[i].key)) {
> - hashcpy(value, cache->entries[i].value);
> - return 0;
> - } else if (is_null_sha1(cache->entries[i].key))
> - return -1;
> + if (!hashcmp(key, cache->entries[i].key))
> + return i;
> + else if (is_null_sha1(cache->entries[i].key))
> + return ~i;
> }
> }
>
> +int get_cached_sha1_entry(struct cached_sha1_map *cache,
> + const unsigned char *key, unsigned char *value)
> +{
> + long i = find_helper(cache, key);
> + if(i < 0)
> + return -1;
Does this have to be extern?
If you are designing an API from scratch, and if you want a long, use it
consistently. Do not demote an int to shorter int in a callchain
unnecessarily.
^ permalink raw reply
* Re: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Geoffrey Irving @ 2008-07-09 5:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git@vger.kernel.org
In-Reply-To: <7vfxqjmyg2.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 8, 2008 at 10:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Geoffrey Irving" <irving@naml.us> writes:
>
>> From a3afd1455d215a541e1481e0f064df743d9219cc Mon Sep 17 00:00:00 2001
>
> Please drop this line.
>
>> From: Geoffrey Irving <irving@naml.us>
>> Date: Sat, 7 Jun 2008 16:03:49 -0700
>> Subject: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
>
> These are Ok _if_ the difference between them and what you have in your
> e-mail header really matter (e.g. you are forwarding somebody else's
> patch). I don't think it is in this case, though.
>
>> Added cached-sha-map.[hc] implementing a persistent hash map from sha1 to
>> sha1.
>
> "Add cached-sha1-map.[ch]" (imperative mood), not "(here is what I) _did_".
>
>> diff --git a/cached-sha1-map.c b/cached-sha1-map.c
>> new file mode 100644
>> index 0000000..e363745
>> --- /dev/null
>> +++ b/cached-sha1-map.c
>> @@ -0,0 +1,182 @@
>> +#include "cached-sha1-map.h"
>> +
>> +union cached_sha1_map_header {
>> + struct {
>> + char signature[4]; /* HASH */
>> + off_t count, size;
>> + };
>> + struct cached_sha1_entry padding; /* pad header out to 40 bytes */
>> +};
>> +
>> +static const char *signature = "HASH";
>
> That sounds a bit too generic, doesn't it, to protect ourselves from
> getting confused by some other filetype?
>
>> +static void init_cached_sha1_map(struct cached_sha1_map *cache)
>> +{
>> + int fd;
>> + union cached_sha1_map_header header;
>> +
>> + if (cache->initialized)
>> + return;
>> +
>> + fd = open(git_path(cache->filename), O_RDONLY);
>> + if (fd < 0) {
>> + init_empty_map(cache, 64);
>> + return;
>
> Check errno and do this only when ENOENT. Other errors should be caught
> and reported.
>
>> + }
>> +
>> + if (read_in_full(fd, &header, sizeof(header)) != sizeof(header))
>> + die("cannot read %s header", cache->filename);
>> +
>> + if (memcmp(header.signature, signature, 4))
>> + die("%s has invalid header", cache->filename);
>> +
>> + if (header.size & (header.size-1))
>> + die("%s size %lld is not a power of two", cache->filename,
>> + (long long)header.size);
>
> Two issues and a half:
>
> - Isn't it gcc extension to be able to say header.signature, bypassing
> the anonymous structure inside the union that the "header" itself is?
>
> - The signature header (count and size) is defined to be off_t, which
> makes the cached file unusable across architectures. The map header
> structure should be specified with explicit size:
>
> union {
> struct {
> char sig[4];
> uint32_t version;
> uint32_t count;
> unit32_t size;
> } u;
> struct cached_sha1_entry pad;
> };
>
> the uint32_t fields should be treated as network byte order integers,
> e.g.
>
> cache->count = ntohl(header.u.count);
> header.u.size = htonl(cache->size);
>
> - If this file is truly intended as "cache", shouldn't corruption of it
> be detected, reported but otherwise ignored, so that the lookup would
> continue in degraded uncached mode?
>
>> + /* check off_t to size_t conversion */
>> + if (cache->count != header.count || cache->size != header.size)
>> + die("%s is too large to hold in memory", cache->filename);
>
> This does not make sense to me. What you are checking does not match the
> error message.
>
> If you are making the file format architecture dependent (which I would
> suggest strongly against), you can use the same type and be done with it.
> Otherwise, if you are making the format portable across architectures,
> then you would know how large the on-disk integer will be, so as long as
> you use appropriate type that is large enough for cache->count you should
> be Ok.
>
> What you may want to check is that (header.u.size + 1) * sizeof(entry)
> does not wrap around, but you don't.
>
>> + /* mmap entire file so that file / memory blocks are aligned */
>> + cache->entries = xmmap(NULL,
>> + sizeof(struct cached_sha1_entry) * (header.size + 1),
>> + PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>
> I think this will die() if the file is too large to map. Ideally you
> would want to allow this mmap to fail if the cache is too large, in which
> case you can gracefully degrade to cacheless mode of operation, but that
> can probably be left to 47th round.
>
>> +int write_cached_sha1_map(struct cached_sha1_map *cache)
>> +{
>> + union cached_sha1_map_header header;
>> + struct lock_file update_lock;
>> + int fd;
>> + size_t entry_size;
>> +
>> + if (!cache->initialized || !cache->dirty)
>> + return 0;
>> +
>> + fd = hold_lock_file_for_update(&update_lock,
>> + git_path(cache->filename), 0);
>> +
>> + if (fd < 0)
>> + return error("could not construct %s", cache->filename);
>
> Use a "const char *" to hold git_path(cache->filename) upfront in the
> function, use it to obtain lock _and_ for reporting.
>
>> + memcpy(header.signature, signature, 4);
>> + header.count = cache->count;
>> + header.size = cache->size;
>
> And here will be htonl().
>
>> + entry_size = sizeof(struct cached_sha1_entry) * cache->size;
>
> Typically "entry_size" means the size of individual entry; this is the
> size of the whole thing.
>
>> + if (write_in_full(fd, &header, sizeof(header)) != sizeof(header)
>> + || write_in_full(fd, cache->entries, entry_size) != entry_size)
>> + return error("could not write %s", cache->filename);
>> +
>> + if (commit_lock_file(&update_lock) < 0)
>> + return error("could not write %s", cache->filename);
>> +
>> + cache->dirty = 0;
>> + return 0;
>> +}
>
> But it is good that you used this intermediate variable; the above
> write_in_full() is much easier to read than the xmmap() above at the end
> of init_cached_sha1_map() function.
>
>> +static size_t get_hash_index(const unsigned char *sha1)
>> +{
>> + return ntohl(*(size_t*)sha1);
>> +}
>
> Two issues:
>
> - I do not see any guarantee that sha1 is suitably aligned for reading
> size_t bytes off of;
>
> - size_t is architecture dependent, so you would get different hash value
> depending on the architecture, which again makes this file format
> unportable.
>
>> diff --git a/patch-ids.c b/patch-ids.c
>> index 3be5d31..36332f3 100644
>> --- a/patch-ids.c
>> +++ b/patch-ids.c
>> @@ -2,17 +2,31 @@
>> #include "diff.h"
>> #include "commit.h"
>> #include "patch-ids.h"
>> +#include "cached-sha1-map.h"
>> +
>> +struct cached_sha1_map patch_id_cache;
>
> Does this have to be extern?
>
>> static int commit_patch_id(struct commit *commit, struct diff_options *options,
>> unsigned char *sha1)
>> {
>> + /* pull patch-id out of the cache if possible */
>> + patch_id_cache.filename = "patch-id-cache";
>> + if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
>> + return 0;
>> +
>> if (commit->parents)
>> diff_tree_sha1(commit->parents->item->object.sha1,
>> commit->object.sha1, "", options);
>> else
>> diff_root_tree_sha1(commit->object.sha1, "", options);
>> diffcore_std(options);
>> - return diff_flush_patch_id(options, sha1);
>> + int ret = diff_flush_patch_id(options, sha1);
>
> Decl-after-statement.
>
>> + if (ret)
>> + return ret;
>> +
>> + /* record commit, patch-id pair in cache */
>> + set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
>> + return 0;
>> }
Thanks. I'll fix these in the next few days.
Should I rewrite the patch sequence to incorporate these changes into
the first commit, or add them as a forth commit off the end?
Geoffrey
^ permalink raw reply
* Re: [PATCH v2 2/4] Add git-sequencer prototype documentation
From: Karl Hasselström @ 2008-07-09 5:20 UTC (permalink / raw)
To: Jakub Narebski
Cc: Stephan Beyer, git, Christian Couder, Daniel Barkalow,
Johannes Schindelin, Junio C Hamano
In-Reply-To: <200807081237.51456.jnareb@gmail.com>
(I've hijacked this thread and started talking mostly about StGit
instead of git-sequencer; if you're not interested, you can stop
reading now.)
On 2008-07-08 12:37:50 +0200, Jakub Narebski wrote:
> 1. Splitting a patch
>
> I cannot comment well on git-sequencer, as I have started using
> StGIT patch management interface instead of git-rebase in times when
> there were no "git rebase --interactive". Nevertheless working with
> StGIT is a bit similar to working with interactive rebase...
>
> I don't find myself wanting to join two patches into one (to squadh
> a commit) perhaps because when I want to add something to a commit
> (to a patch) I simply go to this patch, edit files, and refresh the
> patch.
You can do this without having to manually go to the right patch with
the -p <patchname> flag to stg refresh. In my experimental branch,
this even works together with path limited refresh, or refresh of just
the index.
My own workflow is different: I generally make a large number of
rather small "work-in-progress" commits without much of a commit
message, and every now and then (while I still have everything in
short-term memory) I use "stg coalesce" to make one or more "real"
patches out of them. Because I've committed such small pieces in the
first place, I rarely need to split a patch.
> From time to time however I find myself SPLITTING a patch, for
> example extracting something added "by the way"/"while at it" into
> separate commit (like late separate better documenting project_index
> file format from adding optional description field to project_index
> file format).
The best way I've found of splitting a patch in StGit is to open the
diff in an Emacs buffer, then pop the patch, and then use Emacs' cool
diff-mode features to apply hunks selectively, split hunks, edit files
in place, etc., and committing at the points where I want patch
boundaries. Occasionally, I'll push and pop the patch to get a new
diff with only the remaining stuff.
I imagine something like this could work without StGit as well, since
it's mostly Emacs doing all the hard work. (And I suppose there are
other tools besides Emacs that can do this?)
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Junio C Hamano @ 2008-07-09 5:14 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <7f9d599f0807082053w4603d0bbgfead9127c33b78b5@mail.gmail.com>
"Geoffrey Irving" <irving@naml.us> writes:
> From a3afd1455d215a541e1481e0f064df743d9219cc Mon Sep 17 00:00:00 2001
Please drop this line.
> From: Geoffrey Irving <irving@naml.us>
> Date: Sat, 7 Jun 2008 16:03:49 -0700
> Subject: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
These are Ok _if_ the difference between them and what you have in your
e-mail header really matter (e.g. you are forwarding somebody else's
patch). I don't think it is in this case, though.
> Added cached-sha-map.[hc] implementing a persistent hash map from sha1 to
> sha1.
"Add cached-sha1-map.[ch]" (imperative mood), not "(here is what I) _did_".
> diff --git a/cached-sha1-map.c b/cached-sha1-map.c
> new file mode 100644
> index 0000000..e363745
> --- /dev/null
> +++ b/cached-sha1-map.c
> @@ -0,0 +1,182 @@
> +#include "cached-sha1-map.h"
> +
> +union cached_sha1_map_header {
> + struct {
> + char signature[4]; /* HASH */
> + off_t count, size;
> + };
> + struct cached_sha1_entry padding; /* pad header out to 40 bytes */
> +};
> +
> +static const char *signature = "HASH";
That sounds a bit too generic, doesn't it, to protect ourselves from
getting confused by some other filetype?
> +static void init_cached_sha1_map(struct cached_sha1_map *cache)
> +{
> + int fd;
> + union cached_sha1_map_header header;
> +
> + if (cache->initialized)
> + return;
> +
> + fd = open(git_path(cache->filename), O_RDONLY);
> + if (fd < 0) {
> + init_empty_map(cache, 64);
> + return;
Check errno and do this only when ENOENT. Other errors should be caught
and reported.
> + }
> +
> + if (read_in_full(fd, &header, sizeof(header)) != sizeof(header))
> + die("cannot read %s header", cache->filename);
> +
> + if (memcmp(header.signature, signature, 4))
> + die("%s has invalid header", cache->filename);
> +
> + if (header.size & (header.size-1))
> + die("%s size %lld is not a power of two", cache->filename,
> + (long long)header.size);
Two issues and a half:
- Isn't it gcc extension to be able to say header.signature, bypassing
the anonymous structure inside the union that the "header" itself is?
- The signature header (count and size) is defined to be off_t, which
makes the cached file unusable across architectures. The map header
structure should be specified with explicit size:
union {
struct {
char sig[4];
uint32_t version;
uint32_t count;
unit32_t size;
} u;
struct cached_sha1_entry pad;
};
the uint32_t fields should be treated as network byte order integers,
e.g.
cache->count = ntohl(header.u.count);
header.u.size = htonl(cache->size);
- If this file is truly intended as "cache", shouldn't corruption of it
be detected, reported but otherwise ignored, so that the lookup would
continue in degraded uncached mode?
> + /* check off_t to size_t conversion */
> + if (cache->count != header.count || cache->size != header.size)
> + die("%s is too large to hold in memory", cache->filename);
This does not make sense to me. What you are checking does not match the
error message.
If you are making the file format architecture dependent (which I would
suggest strongly against), you can use the same type and be done with it.
Otherwise, if you are making the format portable across architectures,
then you would know how large the on-disk integer will be, so as long as
you use appropriate type that is large enough for cache->count you should
be Ok.
What you may want to check is that (header.u.size + 1) * sizeof(entry)
does not wrap around, but you don't.
> + /* mmap entire file so that file / memory blocks are aligned */
> + cache->entries = xmmap(NULL,
> + sizeof(struct cached_sha1_entry) * (header.size + 1),
> + PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
I think this will die() if the file is too large to map. Ideally you
would want to allow this mmap to fail if the cache is too large, in which
case you can gracefully degrade to cacheless mode of operation, but that
can probably be left to 47th round.
> +int write_cached_sha1_map(struct cached_sha1_map *cache)
> +{
> + union cached_sha1_map_header header;
> + struct lock_file update_lock;
> + int fd;
> + size_t entry_size;
> +
> + if (!cache->initialized || !cache->dirty)
> + return 0;
> +
> + fd = hold_lock_file_for_update(&update_lock,
> + git_path(cache->filename), 0);
> +
> + if (fd < 0)
> + return error("could not construct %s", cache->filename);
Use a "const char *" to hold git_path(cache->filename) upfront in the
function, use it to obtain lock _and_ for reporting.
> + memcpy(header.signature, signature, 4);
> + header.count = cache->count;
> + header.size = cache->size;
And here will be htonl().
> + entry_size = sizeof(struct cached_sha1_entry) * cache->size;
Typically "entry_size" means the size of individual entry; this is the
size of the whole thing.
> + if (write_in_full(fd, &header, sizeof(header)) != sizeof(header)
> + || write_in_full(fd, cache->entries, entry_size) != entry_size)
> + return error("could not write %s", cache->filename);
> +
> + if (commit_lock_file(&update_lock) < 0)
> + return error("could not write %s", cache->filename);
> +
> + cache->dirty = 0;
> + return 0;
> +}
But it is good that you used this intermediate variable; the above
write_in_full() is much easier to read than the xmmap() above at the end
of init_cached_sha1_map() function.
> +static size_t get_hash_index(const unsigned char *sha1)
> +{
> + return ntohl(*(size_t*)sha1);
> +}
Two issues:
- I do not see any guarantee that sha1 is suitably aligned for reading
size_t bytes off of;
- size_t is architecture dependent, so you would get different hash value
depending on the architecture, which again makes this file format
unportable.
> diff --git a/patch-ids.c b/patch-ids.c
> index 3be5d31..36332f3 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -2,17 +2,31 @@
> #include "diff.h"
> #include "commit.h"
> #include "patch-ids.h"
> +#include "cached-sha1-map.h"
> +
> +struct cached_sha1_map patch_id_cache;
Does this have to be extern?
> static int commit_patch_id(struct commit *commit, struct diff_options *options,
> unsigned char *sha1)
> {
> + /* pull patch-id out of the cache if possible */
> + patch_id_cache.filename = "patch-id-cache";
> + if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
> + return 0;
> +
> if (commit->parents)
> diff_tree_sha1(commit->parents->item->object.sha1,
> commit->object.sha1, "", options);
> else
> diff_root_tree_sha1(commit->object.sha1, "", options);
> diffcore_std(options);
> - return diff_flush_patch_id(options, sha1);
> + int ret = diff_flush_patch_id(options, sha1);
Decl-after-statement.
> + if (ret)
> + return ret;
> +
> + /* record commit, patch-id pair in cache */
> + set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
> + return 0;
> }
^ permalink raw reply
* [JGIT PATCH 3/4] Automatically initialize a new dumb repository during push
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-3-git-send-email-spearce@spearce.org>
If we are pushing into a repository which has no refs and no packs
we can reasonably assume the repository is brand new. In such
cases we must ensure there is a valid HEAD and config file in the
repository, otherwise Git clients reading the repository may not
be able to recognize its contents correctly.
By default we try to create HEAD from refs/heads/master, if that
is among the branches being pushed by the user. This mirrors what
git-init would have initialized HEAD to be. However if master was
not among the branches pushed then another branch is selected as a
reasonable default.
We initialize .git/config to a very bare configuration, listing
only the repository version. We avoid setting up core.filemode as
we cannot genrally detect if the remote side understands executable
mode bits.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../spearce/jgit/transport/WalkPushConnection.java | 54 ++++++++++++++++++++
1 files changed, 54 insertions(+), 0 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
index e11b85a..bb5a653 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
@@ -48,6 +48,7 @@ import java.util.TreeMap;
import org.spearce.jgit.errors.TransportException;
import org.spearce.jgit.lib.AnyObjectId;
+import org.spearce.jgit.lib.Constants;
import org.spearce.jgit.lib.ObjectId;
import org.spearce.jgit.lib.PackWriter;
import org.spearce.jgit.lib.ProgressMonitor;
@@ -151,6 +152,12 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
for (final RemoteRefUpdate u : updates)
updateCommand(u);
+ // Is this a new repository? If so we should create additional
+ // metadata files so it is properly initialized during the push.
+ //
+ if (!updates.isEmpty() && isNewRepository())
+ createNewRepository(updates);
+
if (!packedRefUpdates.isEmpty()) {
try {
dest.writePackedRefs(newRefs.values());
@@ -307,4 +314,51 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
u.setMessage(e.getMessage());
}
}
+
+ private boolean isNewRepository() {
+ return getRefsMap().isEmpty() && packNames != null
+ && packNames.isEmpty();
+ }
+
+ private void createNewRepository(final List<RemoteRefUpdate> updates)
+ throws TransportException {
+ try {
+ final String ref = "ref: " + pickHEAD(updates) + "\n";
+ final byte[] bytes = ref.getBytes(Constants.CHARACTER_ENCODING);
+ dest.writeFile("../HEAD", bytes);
+ } catch (IOException e) {
+ throw new TransportException(uri, "cannot create HEAD", e);
+ }
+
+ try {
+ final String config = "[core]\n"
+ + "\trepositoryformatversion = 0\n";
+ final byte[] bytes = config.getBytes(Constants.CHARACTER_ENCODING);
+ dest.writeFile("../config", bytes);
+ } catch (IOException e) {
+ throw new TransportException(uri, "cannot create config", e);
+ }
+ }
+
+ private static String pickHEAD(final List<RemoteRefUpdate> updates) {
+ // Try to use master if the user is pushing that, it is the
+ // default branch and is likely what they want to remain as
+ // the default on the new remote.
+ //
+ for (final RemoteRefUpdate u : updates) {
+ final String n = u.getRemoteName();
+ if (n.equals(Constants.HEADS_PREFIX + "/" + Constants.MASTER))
+ return n;
+ }
+
+ // Pick any branch, under the assumption the user pushed only
+ // one to the remote side.
+ //
+ for (final RemoteRefUpdate u : updates) {
+ final String n = u.getRemoteName();
+ if (n.startsWith(Constants.HEADS_PREFIX + "/"))
+ return n;
+ }
+ return updates.get(0).getRemoteName();
+ }
}
--
1.5.6.74.g8a5e
^ permalink raw reply related
* [JGIT PATCH 4/4] Use a singleton for the NullProgressMonitor implementation
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-4-git-send-email-spearce@spearce.org>
No reason to create a new instance every time we need to shove
a null monitor into a variable because someone passed us null.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../org/spearce/jgit/lib/NullProgressMonitor.java | 3 +++
.../src/org/spearce/jgit/util/TemporaryBuffer.java | 2 +-
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java b/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
index de75b90..191dc00 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
@@ -42,6 +42,9 @@ package org.spearce.jgit.lib;
* A NullProgressMonitor does not report progress anywhere.
*/
public class NullProgressMonitor implements ProgressMonitor {
+ /** Immutable instance of a null progress monitor. */
+ public static final NullProgressMonitor INSTANCE = new NullProgressMonitor();
+
public void start(int totalTasks) {
// Do not report.
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java b/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
index 20db580..d597c38 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
@@ -200,7 +200,7 @@ public class TemporaryBuffer extends OutputStream {
public void writeTo(final OutputStream os, ProgressMonitor pm)
throws IOException {
if (pm == null)
- pm = new NullProgressMonitor();
+ pm = NullProgressMonitor.INSTANCE;
if (blocks != null) {
// Everything is in core so we can stream directly to the output.
//
--
1.5.6.74.g8a5e
^ permalink raw reply related
* [JGIT PATCH 2/4] Fix pushing of annotated tags to actually include the tag object
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-2-git-send-email-spearce@spearce.org>
When we pushed an annotated tag to a remote repository jgit was
failing to include the annotated tag object itself as ObjectWalk
nextObject() failed to pop the object out of the internal pending
queue and return it for inclusion in the generated pack file.
We need to use two different flags for SEEN and IN_PENDING as a blob
or tree object can be SEEN during a TreeWalk, yet may also have been
inserted into the pending object list. SEEN means we have output
the object to the caller, IN_PENDING means we have placed it into
the pending object list. Together these prevent duplicates in the
queue and duplicate return values.
Noticed by Robin while trying to push an annotated tag:
$ git tag -m foo X
$ git rev-parse X
49aa0e93621...
$ jgit push somerepo refs/tags/X
error: unpack should have generated 49aa0e93621...
To somerepo.git
! [remote rejected] X -> X (bad pack)
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../src/org/spearce/jgit/revwalk/ObjectWalk.java | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
index 6a5b857..1ba21eb 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
@@ -66,6 +66,15 @@ import org.spearce.jgit.treewalk.TreeWalk;
* commits that are returned first.
*/
public class ObjectWalk extends RevWalk {
+ /**
+ * Indicates a non-RevCommit is in {@link #pendingObjects}.
+ * <p>
+ * We can safely reuse {@link RevWalk#REWRITE} here for the same value as it
+ * is only set on RevCommit and {@link #pendingObjects} never has RevCommit
+ * instances inserted into it.
+ */
+ private static final int IN_PENDING = RevWalk.REWRITE;
+
private final TreeWalk treeWalk;
private BlockObjQueue pendingObjects;
@@ -361,8 +370,8 @@ public class ObjectWalk extends RevWalk {
}
private void addObject(final RevObject o) {
- if ((o.flags & SEEN) == 0) {
- o.flags |= SEEN;
+ if ((o.flags & IN_PENDING) == 0) {
+ o.flags |= IN_PENDING;
pendingObjects.add(o);
}
}
--
1.5.6.74.g8a5e
^ permalink raw reply related
* [JGIT PATCH 1/4] Avoid deadlock while fetching from local repository
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-1-git-send-email-spearce@spearce.org>
We cannot send a packet line end command to git-upload-pack after we
have sent our want list and obtained a pack back from it. Once the
want list has ended git-upload-pack wants no further data sent to
it, and attempting to write more may cause us to deadlock as the
pipe won't accept the data.
Not sending the packet line end if we don't send a want list is a
(minor) protocol error. To avoid these protocol errors (which
may display on stderr from git-upload-pack) we still send the end
during close if we have not yet sent a want list.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../spearce/jgit/transport/BasePackConnection.java | 7 ++++++-
.../jgit/transport/BasePackFetchConnection.java | 1 +
.../jgit/transport/BasePackPushConnection.java | 1 +
3 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
index a878f01..7dc4620 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
@@ -83,6 +83,9 @@ abstract class BasePackConnection extends BaseConnection {
/** Packet line encoder around {@link #out}. */
protected PacketLineOut pckOut;
+ /** Send {@link PacketLineOut#end()} before closing {@link #out}? */
+ protected boolean outNeedsEnd;
+
/** Capability tokens advertised by the remote side. */
private final Set<String> remoteCapablities = new HashSet<String>();
@@ -99,6 +102,7 @@ abstract class BasePackConnection extends BaseConnection {
pckIn = new PacketLineIn(in);
pckOut = new PacketLineOut(out);
+ outNeedsEnd = true;
}
protected void readAdvertisedRefs() throws TransportException {
@@ -195,7 +199,8 @@ abstract class BasePackConnection extends BaseConnection {
public void close() {
if (out != null) {
try {
- pckOut.end();
+ if (outNeedsEnd)
+ pckOut.end();
out.close();
} catch (IOException err) {
// Ignore any close errors.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
index 04a91bf..12b36f2 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
@@ -253,6 +253,7 @@ abstract class BasePackFetchConnection extends BasePackConnection implements
pckOut.writeString(line.toString());
}
pckOut.end();
+ outNeedsEnd = false;
return !first;
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
index 784a578..6d95eaf 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
@@ -148,6 +148,7 @@ class BasePackPushConnection extends BasePackConnection implements
if (monitor.isCancelled())
throw new TransportException(uri, "push cancelled");
pckOut.end();
+ outNeedsEnd = false;
}
private String enableCapabilties() {
--
1.5.6.74.g8a5e
^ permalink raw reply related
* [JGIT PATCH 0/4] Misc. push transport fixes
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
Few minor bugs related to our push implementation. A deadlock
is fixed in the local fetch case and pack generation to include
annotated tags is also fixed.
We also now do essentially "git init" on the remote repository
if we are creating it over a dumb transport during the first push
request made to it.
Shawn O. Pearce (4):
Avoid deadlock while fetching from local repository
Fix pushing of annotated tags to actually include the tag object
Automatically initialize a new dumb repository during push
Use a singleton for the NullProgressMonitor implementation
.../org/spearce/jgit/lib/NullProgressMonitor.java | 3 +
.../src/org/spearce/jgit/revwalk/ObjectWalk.java | 13 ++++-
.../spearce/jgit/transport/BasePackConnection.java | 7 ++-
.../jgit/transport/BasePackFetchConnection.java | 1 +
.../jgit/transport/BasePackPushConnection.java | 1 +
.../spearce/jgit/transport/WalkPushConnection.java | 54 ++++++++++++++++++++
.../src/org/spearce/jgit/util/TemporaryBuffer.java | 2 +-
7 files changed, 77 insertions(+), 4 deletions(-)
^ permalink raw reply
* Re: [StGit PATCH 0/2] stg status rename fix (stable)
From: Karl Hasselström @ 2008-07-09 4:05 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git, Jon Smirl
In-Reply-To: <20080708195214.24866.61663.stgit@yoghurt>
On 2008-07-08 21:54:19 +0200, Karl Hasselström wrote:
> This fixes the bug on the stable branch. (stgit.diff-opts doesn't
> exist there, so it's less of an issue, but still.)
>
> Also available at
>
> git://repo.or.cz/stgit/kha.git safe
s/safe/stable/ here, of course.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* [PATCH] git-submodule - make "submodule add" more strict, and document it
From: Mark Levedahl @ 2008-07-09 3:59 UTC (permalink / raw)
To: gitster; +Cc: git, sylvain.joyeux, hjemli, pkufranky, Mark Levedahl
In-Reply-To: <7vhcb0x6ak.fsf@gitster.siamese.dyndns.org>
This change makes "submodule add" much more strict in the arguments it
takes, and is intended to address confusion as recently noted on the
git-list. With this change, the required syntax is:
$ git submodule add URL path
Specifically, this eliminates the form
$git submodule add URL
which was confused by more than one person as
$git submodule add path
The change eliminates one form of URL: a path relative to the current
working directory. This form was confusing, and does not seem to
correspond to an important use-case. Specifically, no-one has identified
the use to clone from a repository already in the tree, but if this is
needed it is easily done using an absolute URL: $(pwd)/relative-path. So,
no functionality is lost.
Following this change, there are exactly four variants of
submodule-add, as both arguments have two flavors:
URL can be absolute, or can begin with ./|../ and thus name the origin
relative to the top-level origin.
Note: With this patch, "submodule add" discerns an absolute URL as
matching /*|*:*: e.g., URL begins with /, or it contains a :. This works
for all valid URLs, an absolute path in POSIX, as well as an absolute
path on Windows).
path can either already exist as a valid git repo, or will be cloned from
the given URL. The first form here allows easy addition of a new
submodule to an existing project as the module can be added and tested
in-tree before pushing to the public repository. However, the more
usual form is the second, where the repo is cloned from the given URL.
This specifically addresses the issue of
$ git submodule add a/b/c
attempting to clone from a repository at "a/b/c" to create a new module
in "c". This also simplifies description of "relative URL" as there is now
exactly *one* form: a URL relative to the parent's origin repo.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
Documentation/git-submodule.txt | 31 +++++++++++++++------
git-submodule.sh | 55 ++++++++++++++------------------------
2 files changed, 42 insertions(+), 44 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 105fc2d..6f24f92 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
SYNOPSIS
--------
[verse]
-'git submodule' [--quiet] add [-b branch] [--] <repository> [<path>]
+'git submodule' [--quiet] add [-b branch] [--] <repository> <path>
'git submodule' [--quiet] status [--cached] [--] [<path>...]
'git submodule' [--quiet] init [--] [<path>...]
'git submodule' [--quiet] update [--init] [--] [<path>...]
@@ -20,14 +20,26 @@ COMMANDS
--------
add::
Add the given repository as a submodule at the given path
- to the changeset to be committed next. If path is a valid
- repository within the project, it is added as is. Otherwise,
- repository is cloned at the specified path. path is added to the
- changeset and registered in .gitmodules. If no path is
- specified, the path is deduced from the repository specification.
- If the repository url begins with ./ or ../, it is stored as
- given but resolved as a relative path from the main project's
- url when cloning.
+ to the changeset to be committed next. This requires two arguments,
+ <repository> and <path>. <repository> is the URL of the new
+ submodule's origin repository. This may be either an absolute URL,
+ or (if it begins with ./ or ../), the location relative
+ to the parent repository's origin.
+
+ <path> is the relative location for the cloned submodule to
+ exist in the current tree. If <path> does not exist, then the
+ module is created by cloning from the named URL. If <path> does
+ exist and is already a valid git repository, then this is added
+ to the changeset without cloning. This second form is provided
+ to ease adding a submodule to a project the first time, and presumes
+ the user will later push the new submodule to the given URL.
+
+ In either case, the given URL is recorded into .gitmodules for
+ use by subsequent users cloning the project. If the URL is given
+ relative to the parent, the presumption is the main and sub-modules
+ will be kept together in the same relative location, and only the
+ top-level URL need be provided: git-submodule will correctly
+ locat the submodules using the hint in .gitmodules.
status::
Show the status of the submodules. This will print the SHA-1 of the
@@ -85,6 +97,7 @@ OPTIONS
<path>::
Path to submodule(s). When specified this will restrict the command
to only operate on the submodules found at the specified paths.
+ (This argument is required with add).
FILES
-----
diff --git a/git-submodule.sh b/git-submodule.sh
index 3eb78cc..d227907 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,7 +5,7 @@
# Copyright (c) 2007 Lars Hjemli
USAGE="[--quiet] [--cached] \
-[add <repo> [-b branch]|status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
+[add <repo> [-b branch] <path>]|[status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
[--] [<path>...]"
OPTIONS_SPEC=
. git-sh-setup
@@ -27,18 +27,6 @@ say()
fi
}
-# NEEDSWORK: identical function exists in get_repo_base in clone.sh
-get_repo_base() {
- (
- cd "`/bin/pwd`" &&
- cd "$1" || cd "$1.git" &&
- {
- cd .git
- pwd
- }
- ) 2>/dev/null
-}
-
# Resolve relative url by appending to parent's url
resolve_relative_url ()
{
@@ -115,7 +103,7 @@ module_clone()
#
# Add a new submodule to the working tree, .gitmodules and the index
#
-# $@ = repo [path]
+# $@ = repo path
#
# optional branch is stored in global branch variable
#
@@ -150,16 +138,27 @@ cmd_add()
repo=$1
path=$2
- if test -z "$repo"; then
+ if test -z "$repo" -o -z "$path"; then
usage
fi
- # Guess path from repo if not specified or strip trailing slashes
- if test -z "$path"; then
- path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
- else
- path=$(echo "$path" | sed -e 's|/*$||')
- fi
+ # assure repo is absolute or relative to parent
+ case "$repo" in
+ ./*|../*)
+ # dereference source url relative to parent's url
+ realrepo=$(resolve_relative_url "$repo") || exit
+ ;;
+ *:*|/*)
+ # absolute url
+ realrepo=$repo
+ ;;
+ *)
+ die "repo URL: '$repo' must be absolute or begin with ./|../"
+ ;;
+ esac
+
+ # strip trailing slashes from path
+ path=$(echo "$path" | sed -e 's|/*$||')
git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
die "'$path' already exists in the index"
@@ -175,20 +174,6 @@ cmd_add()
die "'$path' already exists and is not a valid git repo"
fi
else
- case "$repo" in
- ./*|../*)
- # dereference source url relative to parent's url
- realrepo=$(resolve_relative_url "$repo") || exit
- ;;
- *)
- # Turn the source into an absolute path if
- # it is local
- if base=$(get_repo_base "$repo"); then
- repo="$base"
- fi
- realrepo=$repo
- ;;
- esac
module_clone "$path" "$realrepo" || exit
(unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) ||
--
1.5.6.2.271.g73ad8
^ permalink raw reply related
* [PATCH 3/3] cherry: add cherry.cachepatchids option
From: Geoffrey Irving @ 2008-07-09 3:57 UTC (permalink / raw)
To: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
>From 7a51a1808fb440b1aca58780ccc09ffe11d4d3d6 Mon Sep 17 00:00:00 2001
From: Geoffrey Irving <irving@naml.us>
Date: Tue, 8 Jul 2008 20:25:53 -0700
Subject: [PATCH 3/3] cherry: add cherry.cachepatchids option
The patch-id caching optimization in git-cherry is still enabled by default,
but now it can be disabled by setting cherry.cachepatchids = false.
---
Documentation/config.txt | 5 +++++
builtin-log.c | 12 ++++++++++++
patch-ids.c | 12 +++++++++---
patch-ids.h | 2 ++
4 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 838794d..02b8113 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -468,6 +468,11 @@ browser.<tool>.path::
browse HTML help (see '-w' option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+cherry.cachepatchids::
+ If true, linkgit:git-cherry will store a cache of computed patch-ids
+ in $GIT_DIR/patch-id-cache in order to make repeated invocations faster.
+ Defaults to true.
+
clean.requireForce::
A boolean to make git-clean do nothing unless given -f
or -n. Defaults to true.
diff --git a/builtin-log.c b/builtin-log.c
index 430d876..fbfefbd 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1081,6 +1081,16 @@ static int add_pending_commit(const char *arg,
struct rev_info *revs, int flags)
return -1;
}
+static int git_cherry_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "cherry.cachepatchids")) {
+ cache_patch_ids = git_config_bool(var, value);
+ return 0;
+ }
+
+ return 0;
+}
+
static const char cherry_usage[] =
"git-cherry [-v] <upstream> [<head>] [<limit>]";
int cmd_cherry(int argc, const char **argv, const char *prefix)
@@ -1094,6 +1104,8 @@ int cmd_cherry(int argc, const char **argv,
const char *prefix)
const char *limit = NULL;
int verbose = 0;
+ git_config(git_cherry_config, NULL);
+
if (argc > 1 && !strcmp(argv[1], "-v")) {
verbose = 1;
argc--;
diff --git a/patch-ids.c b/patch-ids.c
index 36332f3..7e3a563 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -4,6 +4,7 @@
#include "patch-ids.h"
#include "cached-sha1-map.h"
+int cache_patch_ids = 1;
struct cached_sha1_map patch_id_cache;
static int commit_patch_id(struct commit *commit, struct diff_options *options,
@@ -11,7 +12,8 @@ static int commit_patch_id(struct commit *commit,
struct diff_options *options,
{
/* pull patch-id out of the cache if possible */
patch_id_cache.filename = "patch-id-cache";
- if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
+ if (cache_patch_ids && !get_cached_sha1_entry(&patch_id_cache,
+ commit->object.sha1, sha1))
return 0;
if (commit->parents)
@@ -25,7 +27,8 @@ static int commit_patch_id(struct commit *commit,
struct diff_options *options,
return ret;
/* record commit, patch-id pair in cache */
- set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
+ if (cache_patch_ids)
+ set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
return 0;
}
@@ -151,7 +154,10 @@ int free_patch_ids(struct patch_ids *ids)
free(patches);
}
- write_cached_sha1_map(&patch_id_cache);
+ /* write cached patch-ids and ignore any errors that arise
+ * (e.g. if the repository is write protected) */
+ if (cache_patch_ids)
+ write_cached_sha1_map(&patch_id_cache);
return 0;
}
diff --git a/patch-ids.h b/patch-ids.h
index c8c7ca1..c0ebdc1 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -18,4 +18,6 @@ int free_patch_ids(struct patch_ids *);
struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);
struct patch_id *has_commit_patch_id(struct commit *, struct patch_ids *);
+extern int cache_patch_ids;
+
#endif /* PATCH_IDS_H */
--
1.5.6.2.258.g7a51a
^ permalink raw reply related
* [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
From: Geoffrey Irving @ 2008-07-09 3:56 UTC (permalink / raw)
To: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
>From c4e60c28fe66985ac8224da832589c982010744e Mon Sep 17 00:00:00 2001
From: Geoffrey Irving <irving@naml.us>
Date: Tue, 8 Jul 2008 19:47:22 -0700
Subject: [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
Pulling common code from get_cached_sha1_entry and set_cached_sha1_entry
into static find_helper function.
---
cached-sha1-map.c | 68 +++++++++++++++++++++++++++++-----------------------
1 files changed, 38 insertions(+), 30 deletions(-)
diff --git a/cached-sha1-map.c b/cached-sha1-map.c
index e363745..147c7a2 100644
--- a/cached-sha1-map.c
+++ b/cached-sha1-map.c
@@ -129,10 +129,14 @@ static size_t get_hash_index(const unsigned char *sha1)
return ntohl(*(size_t*)sha1);
}
-int get_cached_sha1_entry(struct cached_sha1_map *cache,
- const unsigned char *key, unsigned char *value)
+/*
+ * Returns the index if the entry exists, and the complemented index of
+ * the next free entry otherwise.
+ */
+static long find_helper(struct cached_sha1_map *cache,
+ const unsigned char *key)
{
- size_t i, mask;
+ long i, mask;
if (!cache->initialized)
init_cached_sha1_map(cache);
@@ -140,43 +144,47 @@ int get_cached_sha1_entry(struct cached_sha1_map *cache,
mask = cache->size - 1;
for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
- if (!hashcmp(key, cache->entries[i].key)) {
- hashcpy(value, cache->entries[i].value);
- return 0;
- } else if (is_null_sha1(cache->entries[i].key))
- return -1;
+ if (!hashcmp(key, cache->entries[i].key))
+ return i;
+ else if (is_null_sha1(cache->entries[i].key))
+ return ~i;
}
}
+int get_cached_sha1_entry(struct cached_sha1_map *cache,
+ const unsigned char *key, unsigned char *value)
+{
+ long i = find_helper(cache, key);
+ if(i < 0)
+ return -1;
+
+ /* entry found, return value */
+ hashcpy(value, cache->entries[i].value);
+ return 0;
+}
+
void set_cached_sha1_entry(struct cached_sha1_map *cache,
const unsigned char *key, const unsigned char *value)
{
- size_t i, mask;
+ long i;
struct cached_sha1_entry *entry;
- if (!cache->initialized)
- init_cached_sha1_map(cache);
-
- if (4*cache->count >= 3*cache->size)
- grow_map(cache);
-
- mask = cache->size - 1;
-
- for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
- entry = cache->entries+i;
-
- if (is_null_sha1(entry->key)) {
- hashcpy(entry->key, key);
+ i = find_helper(cache, key);
+
+ if (i < 0) { /* write new entry */
+ entry = cache->entries + ~i;
+ hashcpy(entry->key, key);
+ hashcpy(entry->value, value);
+ cache->count++;
+ cache->dirty = 1;
+ } else { /* overwrite existing entry */
+ entry = cache->entries + i;
+ if (hashcmp(value, entry->value)) {
hashcpy(entry->value, value);
- cache->count++;
cache->dirty = 1;
- return;
- } else if(!hashcmp(key, entry->key)) {
- if (hashcmp(value, entry->value)) {
- hashcpy(entry->value, value);
- cache->dirty = 1;
- }
- return;
}
}
+
+ if (4*cache->count >= 3*cache->size)
+ grow_map(cache);
}
--
1.5.6.2.258.g7a51a
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox