* Re: Tag peeling peculiarities
From: Michael Haggerty @ 2013-03-16 13:38 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <20130316093441.GA26260@sigill.intra.peff.net>
On 03/16/2013 10:34 AM, Jeff King wrote:
> On Sat, Mar 16, 2013 at 09:48:42AM +0100, Michael Haggerty wrote:
>
>> My patch series is nearly done. I will need another day or two to
>> review and make it submission-ready, but I wanted to give you an idea of
>> what I'm up to and I could also use your feedback on some points.
>
> I was just sending out my cleaned up patches to do do fully-peeled, too.
> I think the duplication is OK, though, as your topic would be for
> "master" and mine can go to "maint".
Yes, though I'll have to rebase mine on top of yours.
>> * Implement fully-peeled packed-refs files, as discussed upthread: peel
>> references outside of refs/tags, and keep rigorous track of
>> which references have REF_KNOWS_PEELED.
>
> Looks pretty similar to mine. We even added similar tests.
>
> I notice that you do the "add REF_KNOWS_PEELED if we actually got a peel
> line" optimization. I didn't bother, because this will never be
> triggered by a git-written file (either we do not write the entry, or we
> set fully-peeled). I wonder if any other implementation does peel every
> ref, though.
We read the peeled value for refs outside of refs/tags even if
fully-peeled is not set, so it seemed somehow inconsistent not to set
REF_KNOWS_PEELED. But I agree that Git itself should never write such
entries, except of course for the interval between your first patch and
your second patch ;-)
>> * Change pack-refs to use the peeled information from ref_entries if it
>> is available, rather than looking up the references again.
>
> I don't know that it matters from a performance standpoint (we don't
> really care how long pack-refs takes, as long as it is within reason,
> because it is run as part of a gc). But it would mean that any errors
> in the file are propagated from one run of pack-refs to the next. I
> don't know if we want to spend the extra time to be more robust.
I thought about this argument too. Either way is OK with me. BTW would
it make sense to build a consistency check of peeled references into fsck?
>> * repack_without_ref() writes peeled refs to the packed-refs file.
>> Use the old values if available; otherwise look them up.
>
> Whereas here we probably _do_ want the performance optimization, because
> we do care about the performance of a ref deletion.
Agreed.
>> 1. There are multiple functions for peeling refs:
>> peel_ref() (and peel_object(), which was extracted from it);
>> peel_entry() (derived from pack-refs.c:pack_one_ref()). It would be
>> nice to combine these into the One True Function. But given the
>> problem that you mentioned above (which is rooted in parts of the
>> code that I don't know) I don't know what that version should do.
>
> I'm not sure I understand the question. Just skimming your patches, it
> looks like peel_entry could just call peel_object?
I believe that the version in peel_object() is the one that you
optimized in your now-famous 435c833 patches, which your earlier email
said was connected to a null pointer dereference. I'm not at all
familiar with the API being used there, so I don't know whether the two
versions are interchangeable, or whether you need to fix your
optimization, or whether your optimization will need to be reverted
because of the problem you discovered.
>> 2. repack_without_ref() now writes peeled refs, peeling them if
>> necessary. It does this *without* referring to the loose refs
>> to avoid having to load all of the loose references, which can be
>> time-consuming. But this means that it might try to lookup SHA1s
>> that are not the current value of the corresponding reference any
>> more, and might even have been garbage collected.
>
> Yuck. I really wonder if repack_without_ref should literally just be
> writing out the exact same file, minus the lines for the deleted ref.
> Is there a reason we need to do any processing at all? I guess the
> answer is that you want to avoid re-parsing the current file, and just
> write it out from memory. But don't we need to refresh the memory cache
> from disk under the lock anyway? That was the pack-refs race that I
> fixed recently.
It would certainly be thinkable to rewrite the file textually without
parsing it again. But I did it this way for two reasons:
1. It would be better to have one packed-refs file parser and writer
rather than two. (I'm working towards unifying the two writers, and
will continue once I understand their differences.)
2. Given how peeled references were added, it seems dangerous to read,
modify, and write data that might be in a future format that we don't
entirely understand. For example, suppose a new feature is added to
mark Junio's favorite tags:
# pack-refs with: peeled fully-peeled junios-favorites \n
ce432cac30f98b291be609a0fc974042a2156f55 refs/heads/master
83b3dd7151e7eb0e5ac62ee03aca7e6bccaa8d07 refs/tags/evil-dogs
^e1d04f8aad59397cbd55e72bf8a1bd75606f5350
7ed863a85a6ce2c4ac4476848310b8f917ab41f9 refs/tags/lolcats
^990f856a62a24bfd56bac1f5e4581381369e4ede
^^^junios-favorite
b0517166ae2ad92f3b17638cbdee0f04b8170d99 refs/tags/nonsense
^4baff50551545e2b6825973ec37bcaf03edb95fe
This would be backwards-compatible with the current code (granted, one
would lose the favorite information if the file is rewritten with an
older version of the code). But if we delete the lolcats tag textually,
we would cause the favorite annotation to be moved to a different tag
and thereby corrupt the data.
>> Is the code that
>> I use to do this, in peel_entry(), safe to call for nonexistent
>> SHA1s (I would like it to return 0 if the SHA1 is invalid)?:
>>
>> o = parse_object(entry->u.value.sha1);
>> if (o->type == OBJ_TAG) {
>> o = deref_tag(o, entry->name, 0);
>> if (o) {
>> hashcpy(entry->u.value.peeled, o->sha1);
>> entry->flag |= REF_KNOWS_PEELED;
>> return 1;
>> }
>> }
>> return 0;
>
> I think this approach is safe, as parse_object will silently return NULL
> for a missing object. You do need to check for "o != NULL" in your
> conditional, though.
Thanks; will fix.
>> 3. This same change to repack_with_ref() means that it could become
>> significantly slower to delete a packed ref if the packed-ref file
>> is not fully-peeled. On the plus side, once this is done once, the
>> packed-refs files will be rewritten in fully-peeled form. But if
>> two versions of Git are being used in a repository, this cost could
>> be incurred repeatedly. Does anybody object?
>
> I think it's OK in concept. But I still am wondering if it would be
> simpler still to just pass the file through while locked.
See above.
>> 4. Should I change the locking policy as discussed in this thread?:
>>
>> http://article.gmane.org/gmane.comp.version-control.git/212131
>
> I think it's overall a sane direction. It does increase lock contention
> slightly when two processes are deleting at the same time, but it would
> fix the weird corner cases I described (mostly deleted refs reappearing
> due to races). And the lock contention is already there in a
> fully-packed repo anyway. I.e., right now we read the packed-refs file
> and lock it if our to-delete ref is in there; with the proposed change,
> we would lock before even reading it. So the increased contention is
> only when two deleters race each other, _and_ one of them is not
> deleting a packed ref.
OK, I'll work on that too.
Thanks for your feedback!
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags
From: Michael Haggerty @ 2013-03-16 13:50 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20130316090103.GA26855@sigill.intra.peff.net>
Looks good aside from a couple of minor points mentioned below.
On 03/16/2013 10:01 AM, Jeff King wrote:
> When we pack an annotated tag ref, we write not only the
> sha1 of the tag object along with the ref, but also the sha1
> obtained by peeling the tag. This lets readers of the
> pack-refs file know the peeled value without having to
> actually load the object, speeding up upload-pack's ref
> advertisement.
>
> The writer marks a packed-refs file with peeled refs using
> the "peeled" trait at the top of the file. When the reader
> sees this trait, it knows that each ref is either followed
> by its peeled value, or it is not an annotated tag.
>
> However, there is a mismatch between the assumptions of the
> reader and writer. The writer will only peel refs under
> refs/tags, but the reader does not know this; it will assume
> a ref without a peeled value must not be a tag object. Thus
> an annotated tag object placed outside of the refs/tags
> hierarchy will not have its peeled value printed by
> upload-pack.
>
> The simplest way to fix this is to start writing peel values
> for all refs. This matches what the reader expects for both
> new and old versions of git.
>
> Signed-off-by: Jeff King <peff@peff.net>
I like your explanation of the problem.
> ---
> pack-refs.c | 16 ++++++++--------
> t/t3211-peel-ref.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 50 insertions(+), 8 deletions(-)
> create mode 100755 t/t3211-peel-ref.sh
>
> diff --git a/pack-refs.c b/pack-refs.c
> index f09a054..10832d7 100644
> --- a/pack-refs.c
> +++ b/pack-refs.c
> @@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
> int flags, void *cb_data)
> {
> struct pack_refs_cb_data *cb = cb_data;
> + struct object *o;
> int is_tag_ref;
>
> /* Do not pack the symbolic refs */
> @@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
> return 0;
>
> fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
> - if (is_tag_ref) {
> - struct object *o = parse_object(sha1);
> - if (o->type == OBJ_TAG) {
> - o = deref_tag(o, path, 0);
> - if (o)
> - fprintf(cb->refs_file, "^%s\n",
> - sha1_to_hex(o->sha1));
> - }
> +
> + o = parse_object(sha1);
> + if (o->type == OBJ_TAG) {
You suggested that I add a test (o != NULL) at the equivalent place in
my code (which was derived from this code). Granted, my code was
explicitly intending to pass invalid SHA1 values to parse_object(). But
wouldn't it be a good defensive step to add the same check here?
> + o = deref_tag(o, path, 0);
> + if (o)
> + fprintf(cb->refs_file, "^%s\n",
> + sha1_to_hex(o->sha1));
> }
>
> if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
> diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
> new file mode 100755
> index 0000000..dd5b48b
> --- /dev/null
> +++ b/t/t3211-peel-ref.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +
> +test_description='tests for the peel_ref optimization of packed-refs'
> +. ./test-lib.sh
> +
> +test_expect_success 'create annotated tag in refs/tags' '
> + test_commit base &&
> + git tag -m annotated foo
> +'
> +
> +test_expect_success 'create annotated tag outside of refs/tags' '
> + git update-ref refs/outside/foo refs/tags/foo
> +'
> +
> +# This matches show-ref's output
> +print_ref() {
> + echo "`git rev-parse "$1"` $1"
> +}
> +
CodingGuidelines prefers $() over ``.
> +test_expect_success 'set up expected show-ref output' '
> + {
> + print_ref "refs/heads/master" &&
> + print_ref "refs/outside/foo" &&
> + print_ref "refs/outside/foo^{}" &&
> + print_ref "refs/tags/base" &&
> + print_ref "refs/tags/foo" &&
> + print_ref "refs/tags/foo^{}"
> + } >expect
> +'
> +
> +test_expect_success 'refs are peeled outside of refs/tags (loose)' '
> + git show-ref -d >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'refs are peeled outside of refs/tags (packed)' '
> + git pack-refs --all &&
> + git show-ref -d >actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
>
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 2/2] pack-refs: add fully-peeled trait
From: Michael Haggerty @ 2013-03-16 14:06 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20130316090116.GB26855@sigill.intra.peff.net>
ACK, with one ignorable comment.
Michael
On 03/16/2013 10:01 AM, Jeff King wrote:
> Older versions of pack-refs did not write peel lines for
> refs outside of refs/tags. This meant that on reading the
> pack-refs file, we might set the REF_KNOWS_PEELED flag for
> such a ref, even though we do not know anything about its
> peeled value.
>
> The previous commit updated the writer to always peel, no
> matter what the ref is. That means that packed-refs files
> written by newer versions of git are fine to be read by both
> old and new versions of git. However, we still have the
> problem of reading packed-refs files written by older
> versions of git, or by other implementations which have not
> yet learned the same trick.
>
> The simplest fix would be to always unset the
> REF_KNOWS_PEELED flag for refs outside of refs/tags that do
> not have a peel line (if it has a peel line, we know it is
> valid, but we cannot assume a missing peel line means
> anything). But that loses an important optimization, as
> upload-pack should not need to load the object pointed to by
> refs/heads/foo to determine that it is not a tag.
>
> Instead, we add a "fully-peeled" trait to the packed-refs
> file. If it is set, we know that we can trust a missing peel
> line to mean that a ref cannot be peeled. Otherwise, we fall
> back to assuming nothing.
Another nice, clear explanation of the issue.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> pack-refs.c | 2 +-
> refs.c | 16 +++++++++++++++-
> t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/pack-refs.c b/pack-refs.c
> index 10832d7..87ca04d 100644
> --- a/pack-refs.c
> +++ b/pack-refs.c
> @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)
> die_errno("unable to create ref-pack file structure");
>
> /* perhaps other traits later as well */
> - fprintf(cbdata.refs_file, "# pack-refs with: peeled \n");
> + fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
>
> for_each_ref(handle_one_ref, &cbdata);
> if (ferror(cbdata.refs_file))
> diff --git a/refs.c b/refs.c
> index 175b9fc..6a38c41 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -808,6 +808,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
> struct ref_entry *last = NULL;
> char refline[PATH_MAX];
> int flag = REF_ISPACKED;
> + int fully_peeled = 0;
>
> while (fgets(refline, sizeof(refline), f)) {
> unsigned char sha1[20];
> @@ -818,13 +819,26 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
> const char *traits = refline + sizeof(header) - 1;
> if (strstr(traits, " peeled "))
> flag |= REF_KNOWS_PEELED;
> + if (strstr(traits, " fully-peeled "))
> + fully_peeled = 1;
> /* perhaps other traits later as well */
> continue;
> }
>
> refname = parse_ref_line(refline, sha1);
> if (refname) {
> - last = create_ref_entry(refname, sha1, flag, 1);
> + /*
> + * Older git did not write peel lines for anything
> + * outside of refs/tags/; if the fully-peeled trait
> + * is not set, we are dealing with such an older
> + * git and cannot assume an omitted peel value
> + * means the ref is not a tag object.
> + */
> + int this_flag = flag;
> + if (!fully_peeled && prefixcmp(refname, "refs/tags/"))
> + this_flag &= ~REF_KNOWS_PEELED;
> +
> + last = create_ref_entry(refname, sha1, this_flag, 1);
> add_ref(dir, last);
> continue;
> }
I have to admit that I am partial to my variant of this code [1] because
the logic makes it clearer when the affirmative decision can be made to
set the REF_KNOWS_PEELED flag. But this version also looks correct to
me and equivalent (aside from the idea that a few lines later if a
peeled value is found then the REF_KNOWS_PEELED bit could also be set).
> diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
> index dd5b48b..a8eb1aa 100755
> --- a/t/t3211-peel-ref.sh
> +++ b/t/t3211-peel-ref.sh
> @@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '
> test_cmp expect actual
> '
>
> +test_expect_success 'create old-style pack-refs without fully-peeled' '
> + # Git no longer writes without fully-peeled, so we just write our own
> + # from scratch; we could also munge the existing file to remove the
> + # fully-peeled bits, but that seems even more prone to failure,
> + # especially if the format ever changes again. At least this way we
> + # know we are emulating exactly what an older git would have written.
> + {
> + echo "# pack-refs with: peeled " &&
> + print_ref "refs/heads/master" &&
> + print_ref "refs/outside/foo" &&
> + print_ref "refs/tags/base" &&
> + print_ref "refs/tags/foo" &&
> + echo "^$(git rev-parse "refs/tags/foo^{}")"
> + } >tmp &&
> + mv tmp .git/packed-refs
> +'
> +
> +test_expect_success 'refs are peeled outside of refs/tags (old packed)' '
> + git show-ref -d >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
>
[1]
https://github.com/mhagger/git/commit/1c8d4daa2de15a03637d753471a9e5222b01b968
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: git svn error "Not a valid object name"
From: Thomas Rast @ 2013-03-16 14:13 UTC (permalink / raw)
To: Adam Retter
Cc: git, Dannes Wessels, Wolfgang Meier, Leif-Jöran Olsson,
Eric Wong
In-Reply-To: <CAJKLP9ZQBXf5ZZY9FccOAL5QU+q1b5SnAfvP9BpARdqvzPuWeg@mail.gmail.com>
[+Cc Eric]
Adam Retter <adam@exist-db.org> writes:
> $ git svn init -t tags -b stable -T trunk
> file:///home/ec2-user/svn-rsync/code new-git-repo
> $ cd new-git-repo
> $ git config svn-remote.svn.preserve-empty-dirs true
> $ git config svn-remote.svn.rewriteRoot https://svn.code.sf.net/p/exist/code
> $ git svn fetch -A /home/ec2-user/.svn2git/authors.txt
>
> It all started well and was running away for quite some hours, when
> the following error occurred:
>
> fatal: Not a valid object name
> ls-tree -z ./webapp/api/: command returned error: 128
The important observation is that the object name is missing; the error
is misleading in that it simply tells us that what is *taken* for the
object name is invalid.
There appear to be only two uses of ls-tree -z without further options
in git-svn, namely:
SVN/Fetcher.pm:165: my ($tree) = (command('ls-tree', '-z', $self->{c}, "./$gpath")
SVN/Fetcher.pm:197: ($mode, $blob) = (command('ls-tree', '-z', $self->{c}, "./$gpath
In either one, $self->{c} is empty if they fail in this way. And that
seems to come from 'sub new', which says
if (exists $git_svn->{last_commit}) {
$self->{c} = $git_svn->{last_commit};
$self->{empty_symlinks} =
_mark_empty_symlinks($git_svn, $switch_path);
}
So for some reason new() thinks it's okay to leave $self->{c}
uninitialized, but delete_entry() and open_file() expect it to be set.
It does seem that the ls-tree $self->{c} usage in both of those routines
is from approximately the beginning of time. See these two, if you
compiled your git with log -L:
git log -L:delete_entry:git-svn.perl a6180325^
git log -L:open_file:git-svn.perl a6180325^
Unfortunately that's pretty much where my git-svn knowledge ends. Maybe
Eric can help?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [RFC/PATCH] Documentation/technical/api-fswatch.txt: start with outline
From: Thomas Rast @ 2013-03-16 14:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Karsten Blees, Duy Nguyen, Ramkumar Ramachandra, Git List,
Torsten Bögershausen, Robert Zeh, Jeff King, Erik Faye-Lund,
Drew Northup
In-Reply-To: <7vtxof146d.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Karsten Blees <karsten.blees@gmail.com> writes:
>
>> However, AFAIK inotify doesn't work recursively, so the daemon
>> would at least have to track the directory structure to be able to
>> register / unregister inotify handlers as directories come and go.
>
> Yes, and you would need one inotify per directory but you do not
> have an infinite supply of outstanding inotify watch (wasn't the
> limit like 8k per a single uid or something?), so the daemon must be
> prepared to say "I'll watch this, that and that directories, but the
> consumers should check other directories themselves."
Those are tunable limits though. For example I run this silly hack
https://github.com/trast/watch
with the shell snippets to be able to quickly cd a shell to where
something recently happened. I am able to watch most of my "working
set" even under default limits, which here (opensuse tumbleweed, kernel
3.8.x, x86_64) are
$ cat /proc/sys/fs/inotify/max_user_watches
65536
$ cat /proc/sys/fs/inotify/max_user_instances
128
I'm not sure if other distros impose tighter limits by default, but as
it stands you're not very likely to hit the 65k watches limit in any
given repo. It seems more likely that you might hit the 128 instances
limit if we go with a design that uses one daemon per repo, if you run a
script that accesses many repos. For example, in an android tree I have
lying around,
$ repo list | wc -l
297
That alone might indicate it would be a good idea to have one "global"
git-agent that starts on demand, rather than a per-repo daemon.
Otherwise we'd have to find a way to discover "old" daemons and tell
them to quit when we hit max_user_instances.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* [PATCH] Makefile: keep LIB_H entries together and sorted
From: René Scharfe @ 2013-03-16 15:58 UTC (permalink / raw)
To: git discussion list; +Cc: Junio C Hamano, Jonathan Nieder
As a follow-up to 60d24dd25 (Makefile: fold XDIFF_H and VCSSVN_H into
LIB_H), let the unconditional additions to LIB_H form a single sorted
list. Also drop the duplicate entry for xdiff/xdiff.h, which was easy
to spot after sorting.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Makefile | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/Makefile b/Makefile
index 26d3332..598d631 100644
--- a/Makefile
+++ b/Makefile
@@ -617,22 +617,6 @@ LIB_FILE = libgit.a
XDIFF_LIB = xdiff/lib.a
VCSSVN_LIB = vcs-svn/lib.a
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-
GENERATED_H += common-cmds.h
LIB_H += advice.h
@@ -734,11 +718,24 @@ LIB_H += url.h
LIB_H += userdiff.h
LIB_H += utf8.h
LIB_H += varint.h
+LIB_H += vcs-svn/fast_export.h
+LIB_H += vcs-svn/line_buffer.h
+LIB_H += vcs-svn/repo_tree.h
+LIB_H += vcs-svn/sliding_window.h
+LIB_H += vcs-svn/svndiff.h
+LIB_H += vcs-svn/svndump.h
LIB_H += walker.h
LIB_H += wildmatch.h
LIB_H += wt-status.h
LIB_H += xdiff-interface.h
LIB_H += xdiff/xdiff.h
+LIB_H += xdiff/xdiffi.h
+LIB_H += xdiff/xemit.h
+LIB_H += xdiff/xinclude.h
+LIB_H += xdiff/xmacros.h
+LIB_H += xdiff/xprepare.h
+LIB_H += xdiff/xtypes.h
+LIB_H += xdiff/xutils.h
LIB_OBJS += abspath.o
LIB_OBJS += advice.o
--
1.8.2
^ permalink raw reply related
* Re: git svn error "Not a valid object name"
From: Eric Wong @ 2013-03-16 17:13 UTC (permalink / raw)
To: Adam Retter; +Cc: git, Dannes Wessels, Wolfgang Meier, Leif-Jöran Olsson
In-Reply-To: <CAJKLP9aX20i+oy7AMhh5+uAmP2Np4tUGTEvR+XDHyG1a_DSXtQ@mail.gmail.com>
Adam Retter <adam@exist-db.org> wrote:
> If your able, any idea of when you might be able to take a look at the
> bug? Our svn repo is publicly available for all.
svn ls https://svn.code.sf.net/p/exist/code/trunk
...Is asking me for username
^ permalink raw reply
* [PATCH] advice: Remove unused advice_push_non_ff_default
From: Ramsay Jones @ 2013-03-16 17:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GIT Mailing-list
Commit 27f76a2b ('push: switch default from "matching" to "simple"',
04-01-2013) removed the last use of the 'advice_push_non_ff_default'
variable, along with the advice message which it used to suppress.
Remove the 'advice_push_non_ff_default' variable definition, along
with the now redundant advice.pushnonffdefault config variable
(including it's documentation).
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Junio,
Commit 27f76a2b is in next, but you are about to re-build next, right?
Could you please squash this (or something like it) into commit 27f76a2b.
Note that this patch was built on the tip of the pu branch. Also, I have
not re-built the documentation to check the formatting; I stopped building
the documentation some time ago!
NOTE: I just noticed that I missed a use of 'pushNonFFDefault' in the
documentation of 'advice.pushUpdateRejected', just above the change made
to Documentation/config.txt below. Sorry about that!
Thanks!
ATB,
Ramsay Jones
Documentation/config.txt | 6 ------
advice.c | 2 --
advice.h | 1 -
3 files changed, 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4730dd2..06327cd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -149,12 +149,6 @@ advice.*::
pushNonFFCurrent::
Advice shown when linkgit:git-push[1] fails due to a
non-fast-forward update to the current branch.
- pushNonFFDefault::
- Advice to set 'push.default' to 'upstream' or 'current'
- when you ran linkgit:git-push[1] and pushed 'matching
- refs' by default (i.e. you did not provide an explicit
- refspec, and no 'push.default' configuration was set)
- and it resulted in a non-fast-forward error.
pushNonFFMatching::
Advice shown when you ran linkgit:git-push[1] and pushed
'matching refs' explicitly (i.e. you used ':', or
diff --git a/advice.c b/advice.c
index 780f58d..88c4fea 100644
--- a/advice.c
+++ b/advice.c
@@ -2,7 +2,6 @@
int advice_push_update_rejected = 1;
int advice_push_non_ff_current = 1;
-int advice_push_non_ff_default = 1;
int advice_push_non_ff_matching = 1;
int advice_push_already_exists = 1;
int advice_push_fetch_first = 1;
@@ -19,7 +18,6 @@ static struct {
} advice_config[] = {
{ "pushupdaterejected", &advice_push_update_rejected },
{ "pushnonffcurrent", &advice_push_non_ff_current },
- { "pushnonffdefault", &advice_push_non_ff_default },
{ "pushnonffmatching", &advice_push_non_ff_matching },
{ "pushalreadyexists", &advice_push_already_exists },
{ "pushfetchfirst", &advice_push_fetch_first },
diff --git a/advice.h b/advice.h
index fad36df..4d1e1f9 100644
--- a/advice.h
+++ b/advice.h
@@ -5,7 +5,6 @@
extern int advice_push_update_rejected;
extern int advice_push_non_ff_current;
-extern int advice_push_non_ff_default;
extern int advice_push_non_ff_matching;
extern int advice_push_already_exists;
extern int advice_push_fetch_first;
--
1.8.2
^ permalink raw reply related
* [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()
From: René Scharfe @ 2013-03-16 18:29 UTC (permalink / raw)
To: git discussion list; +Cc: Junio C Hamano, Matthieu Moy
The only caller of diagnose_invalid_sha1_path() extracts a substring from
an object name by creating a NUL-terminated copy of the interesting part.
Add a length parameter to the function and thus avoid the need for an
allocation, thereby simplifying the code.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
sha1_name.c | 32 ++++++++++++++------------------
1 file changed, 14 insertions(+), 18 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 95003c7..4cea6d3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1137,7 +1137,8 @@ int get_sha1_blob(const char *name, unsigned char *sha1)
static void diagnose_invalid_sha1_path(const char *prefix,
const char *filename,
const unsigned char *tree_sha1,
- const char *object_name)
+ const char *object_name,
+ int object_name_len)
{
struct stat st;
unsigned char sha1[20];
@@ -1147,8 +1148,8 @@ static void diagnose_invalid_sha1_path(const char *prefix,
prefix = "";
if (!lstat(filename, &st))
- die("Path '%s' exists on disk, but not in '%s'.",
- filename, object_name);
+ die("Path '%s' exists on disk, but not in '%.*s'.",
+ filename, object_name_len, object_name);
if (errno == ENOENT || errno == ENOTDIR) {
char *fullname = xmalloc(strlen(filename)
+ strlen(prefix) + 1);
@@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,
if (!get_tree_entry(tree_sha1, fullname,
sha1, &mode)) {
die("Path '%s' exists, but not '%s'.\n"
- "Did you mean '%s:%s' aka '%s:./%s'?",
+ "Did you mean '%.*s:%s' aka '.*%.*s:./%s'?",
fullname,
filename,
- object_name,
+ object_name_len, object_name,
fullname,
- object_name,
+ object_name_len, object_name,
filename);
}
- die("Path '%s' does not exist in '%s'",
- filename, object_name);
+ die("Path '%s' does not exist in '%.*s'",
+ filename, object_name_len, object_name);
}
}
@@ -1332,13 +1333,8 @@ static int get_sha1_with_context_1(const char *name,
}
if (*cp == ':') {
unsigned char tree_sha1[20];
- char *object_name = NULL;
- if (only_to_die) {
- object_name = xmalloc(cp-name+1);
- strncpy(object_name, name, cp-name);
- object_name[cp-name] = '\0';
- }
- if (!get_sha1_1(name, cp-name, tree_sha1, GET_SHA1_TREEISH)) {
+ int len = cp - name;
+ if (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) {
const char *filename = cp+1;
char *new_filename = NULL;
@@ -1348,8 +1344,8 @@ static int get_sha1_with_context_1(const char *name,
ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
if (ret && only_to_die) {
diagnose_invalid_sha1_path(prefix, filename,
- tree_sha1, object_name);
- free(object_name);
+ tree_sha1,
+ name, len);
}
hashcpy(oc->tree, tree_sha1);
strncpy(oc->path, filename,
@@ -1360,7 +1356,7 @@ static int get_sha1_with_context_1(const char *name,
return ret;
} else {
if (only_to_die)
- die("Invalid object name '%s'.", object_name);
+ die("Invalid object name '%.*s'.", len, name);
}
}
return ret;
--
1.8.2
^ permalink raw reply related
* Re: git svn error "Not a valid object name"
From: Dannes Wessels @ 2013-03-16 18:52 UTC (permalink / raw)
To: Eric Wong
Cc: Adam Retter, git@vger.kernel.org, Dannes Wessels, Wolfgang Meier,
Leif-Jöran Olsson
In-Reply-To: <20130316171354.GA2427@dcvr.yhbt.net>
Http:// should provide access without password ......
--
Dannes Wessels
On 16 mrt. 2013, at 18:13, Eric Wong <normalperson@yhbt.net> wrote:
> Adam Retter <adam@exist-db.org> wrote:
>> If your able, any idea of when you might be able to take a look at the
>> bug? Our svn repo is publicly available for all.
>
> svn ls https://svn.code.sf.net/p/exist/code/trunk
> ...Is asking me for username
^ permalink raw reply
* Re: git svn error "Not a valid object name"
From: Adam Retter @ 2013-03-16 18:54 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Dannes Wessels, Wolfgang Meier, Leif-Jöran Olsson
In-Reply-To: <20130316171354.GA2427@dcvr.yhbt.net>
Ah right yes, sorry the HTTPS version needs a username the HTTP
version does not. Please use:
http://svn.code.sf.net/p/exist/code
On 16 March 2013 17:13, Eric Wong <normalperson@yhbt.net> wrote:
> Adam Retter <adam@exist-db.org> wrote:
>> If your able, any idea of when you might be able to take a look at the
>> bug? Our svn repo is publicly available for all.
>
> svn ls https://svn.code.sf.net/p/exist/code/trunk
> ...Is asking me for username
--
Adam Retter
eXist Developer
{ United Kingdom }
adam@exist-db.org
irc://irc.freenode.net/existdb
^ permalink raw reply
* [PATCH] rev-parse: Clarify documentation of @{upstream} syntax
From: Kacper Kornet @ 2013-03-16 18:51 UTC (permalink / raw)
To: git
git-rev-parse interprets string in string@{upstream} as a name of
a branch not a ref. For example refs/heads/master@{upstream} looks
for an upstream branch that is merged by git-pull to ref
refs/heads/refs/heads/master not to refs/heads/master. However the
documentation could misled a user to believe that the string is
interpreted as ref.
Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
Documentation/revisions.txt | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 678d175..314e25d 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -88,10 +88,10 @@ some output processing may assume ref names in UTF-8.
The construct '@\{-<n>\}' means the <n>th branch checked out
before the current one.
-'<refname>@\{upstream\}', e.g. 'master@\{upstream\}', '@\{u\}'::
- The suffix '@\{upstream\}' to a ref (short form '<refname>@\{u\}') refers to
- the branch the ref is set to build on top of. A missing ref defaults
- to the current branch.
+'<branchname>@\{upstream\}', e.g. 'master@\{upstream\}', '@\{u\}'::
+ The suffix '@\{upstream\}' to a branchname (short form '<branchname>@\{u\}')
+ refers to the branch that the branch specified by branchname is set to build on
+ top of. A missing branchname defaults to the current one.
'<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
A suffix '{caret}' to a revision parameter means the first parent of
--
1.8.2
^ permalink raw reply related
* [PATCH] safe_create_leading_directories: fix race that could give a false negative
From: Steven Walter @ 2013-03-16 19:30 UTC (permalink / raw)
To: git; +Cc: Steven Walter
If two processes are racing to create the same directory tree, they will
both see that the directory doesn't exist, both try to mkdir(), and one
of them will fail. This is okay, as we only care that the directory
gets created. So, we add a check for EEXIST from mkdir, and continue if
the directory now exists.
---
sha1_file.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..c7b7fec 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -123,6 +123,13 @@ int safe_create_leading_directories(char *path)
}
}
else if (mkdir(path, 0777)) {
+ if (errno == EEXIST) {
+ /* We could be racing with another process to
+ * create the directory. As long as the
+ * directory gets created, we don't care. */
+ if (stat(path, &st) && S_ISDIR(st.st_mode))
+ continue;
+ }
*pos = '/';
return -1;
}
--
1.7.10.4
^ permalink raw reply related
* [PATCH] gitk: Add user-configurable branch bg color
From: Manuel Bua @ 2013-03-16 20:05 UTC (permalink / raw)
To: git; +Cc: paulus, Manuel Bua
In some cases, the default branch background color (green) isn't
an optimal choice, thus it can be difficult to read.
This provides a way for the user to customize the color used to
fill the rectangle around the branch name, by choosing another
one from the "Preferences" dialog.
The default behavior of using "green" as the default color is
maintained.
Signed-off-by: Manuel Bua <manuel.bua@gmail.com>
---
gitk | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/gitk b/gitk
index b3706fc..310c9a9 100755
--- a/gitk
+++ b/gitk
@@ -2720,7 +2720,7 @@ proc savestuff {w} {
global maxwidth showneartags showlocalchanges
global viewname viewfiles viewargs viewargscmd viewperm nextviewnum
global cmitmode wrapcomment datetimeformat limitdiffs
- global colors uicolor bgcolor fgcolor diffcolors diffcontext selectbgcolor
+ global colors uicolor bgcolor fgcolor diffcolors diffcontext selectbgcolor branchcolor
global autoselect autosellen extdifftool perfile_attrs markbgcolor use_ttk
global hideremotes want_ttk maxrefs
@@ -2760,6 +2760,7 @@ proc savestuff {w} {
puts $f [list set selectbgcolor $selectbgcolor]
puts $f [list set extdifftool $extdifftool]
puts $f [list set perfile_attrs $perfile_attrs]
+ puts $f [list set branchcolor $branchcolor]
puts $f "set geometry(main) [wm geometry .]"
puts $f "set geometry(state) [wm state .]"
@@ -6344,7 +6345,7 @@ proc bindline {t id} {
proc drawtags {id x xt y1} {
global idtags idheads idotherrefs mainhead
global linespc lthickness
- global canv rowtextx curview fgcolor bgcolor ctxbut
+ global canv rowtextx curview fgcolor bgcolor ctxbut branchcolor
set marks {}
set ntags 0
@@ -6399,7 +6400,7 @@ proc drawtags {id x xt y1} {
} else {
# draw a head or other ref
if {[incr nheads -1] >= 0} {
- set col green
+ set col $branchcolor
if {$tag eq $mainhead} {
set font mainfontbold
}
@@ -11033,7 +11034,7 @@ proc prefspage_general {notebook} {
}
proc prefspage_colors {notebook} {
- global NS uicolor bgcolor fgcolor ctext diffcolors selectbgcolor markbgcolor
+ global NS uicolor bgcolor fgcolor ctext diffcolors selectbgcolor markbgcolor branchcolor
set page [create_prefs_page $notebook.colors]
@@ -11077,6 +11078,12 @@ proc prefspage_colors {notebook} {
${NS}::button $page.selbgbut -text [mc "Select bg"] \
-command [list choosecolor selectbgcolor {} $page.selbgsep [mc "background"] setselbg]
grid x $page.selbgbut $page.selbgsep -sticky w
+ label $page.selbranchcolsep -padx 40 -relief sunk -background $branchcolor
+ ${NS}::button $page.selbranchcolbut -text [mc "Select branch bg"] \
+ -command [list choosecolor branchcolor {} $page.selbranchcolsep \
+ [mc "branch background"] \
+ setbranchcolor]
+ grid x $page.selbranchcolbut $page.selbranchcolsep -sticky w
return $page
}
@@ -11221,6 +11228,10 @@ proc setfg {c} {
$canv itemconf markid -outline $c
}
+proc setbranchcolor {c} {
+ reloadcommits
+}
+
proc prefscan {} {
global oldprefs prefstop
@@ -11742,6 +11753,7 @@ set diffcontext 3
set ignorespace 0
set worddiff ""
set markbgcolor "#e0e0ff"
+set branchcolor green
set circlecolors {white blue gray blue blue}
--
1.8.2
^ permalink raw reply related
* Re: SSH version on Git 1.8.1.2 for Windows is outdated.
From: Sebastian Schuberth @ 2013-03-16 20:54 UTC (permalink / raw)
To: Patrik Gornicz; +Cc: Joshua Jensen, Konstantin Khomoutov, Kristof Mattei, git
In-Reply-To: <5144DAD7.50203@mail.pgornicz.com>
On Sat, Mar 16, 2013 at 9:49 PM, Patrik Gornicz
<patrik-git@mail.pgornicz.com> wrote:
> Any idea as to when this mingwGitDevEnv project will be mature enough to use
> as a replacement for msysgit? One of the reasons I gave up trying to tweak
Due to a lack of contributors this is taking longer than I
anticipated. But I realize I need to make contributing easier, and
also find the time to merge some outstanding pull requests. I hope to
be able to make some significant improvements at the end of March.
--
Sebastian Schuberth
^ permalink raw reply
* Re: SSH version on Git 1.8.1.2 for Windows is outdated.
From: Patrik Gornicz @ 2013-03-16 20:49 UTC (permalink / raw)
To: Sebastian Schuberth
Cc: Joshua Jensen, Konstantin Khomoutov, Kristof Mattei, git
In-Reply-To: <51447052.1020407@gmail.com>
On 03/16/13 09:14, Sebastian Schuberth wrote:
> On 15.03.2013 21:11, Joshua Jensen wrote:
>
>>> Yes, you should grab the msysGit (the Git for Windows build
>>> environment) [2], tweak it to include the new OpenSSH binary, ensure it
>>> builds and works OK and then send a pull request (or post your patchset
>>> to the msysgit mailing list [3].
>>>
>> Wow, we can do that now?
>>
>> When I brought up the vastly improved performance from a newer SSH
>> executable, I was told the only way to get it in would be to build from
>> source [1].
>
> "tweak it to include the new OpenSSH binary" is supposed to include the
> step to adjust the release.sh script to grab the updated sources and
> build the binary.
I attempted to do just this about a month ago though things really
started to snowball. To get openssh to compile you need to update a
bunch of other programs in the msys branch and I just lost interest in
doing so. More info can be found in the msysgit issue "Upgrade SSH" [1].
> However, another option is to take a look at the new mingwGitDevEnv
> project [1], which relies on mingw-get to retrieve binary packages. I
> hopefully find soon the time to include OpenSSH 6.1p1 incl. HPN-SSH
> patches [2].
Any idea as to when this mingwGitDevEnv project will be mature enough to
use as a replacement for msysgit? One of the reasons I gave up trying to
tweak things to get openssh compiling was that this seemed like a much
better idea. Though if it's fair off I might find time to give it
another shot.
> [1] https://github.com/sschuberth/mingwGitDevEnv
> [2] https://github.com/sschuberth/mingwGitDevEnv/pull/5
If someone is keen to try and update openssh in msysgit I'd be willing
to share my hacks from a month ago which could be used as a starting
point. It was left nowhere near ready for a pull request, partially due
to binary issues mentioned above, but at least it'll provide reasonable
ideas to a few of the things that need to happen.
[1] https://github.com/msysgit/msysgit/issues/31
Patrik
^ permalink raw reply
* Re: [PATCH/RFC] http_init: only initialize SSL for https
From: Daniel Stenberg @ 2013-03-16 22:58 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, kusmabite, git, msysgit
In-Reply-To: <20130316120300.GA2626@sigill.intra.peff.net>
On Sat, 16 Mar 2013, Jeff King wrote:
> But are we correct in assuming that curl will barf if it gets a redirect to
> an ssl-enabled protocol? My testing seems to say yes:
Ah yes. If it switches over to an SSL-based protocol it will pretty much
require that it had been initialized previously.
With redirects taken into account, I can't think of any really good way around
avoiding this init...
--
/ daniel.haxx.se
--
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
---
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
^ permalink raw reply
* Re: [RFC/PATCH] Introduce remote.pushdefault
From: Ramkumar Ramachandra @ 2013-03-17 0:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder, Jeff King
In-Reply-To: <7v4nhm1s85.fsf@alter.siamese.dyndns.org>
Sorry about the horribly late response- I just got around to
re-rolling this, and had a doubt.
Junio C Hamano <gitster@pobox.com> wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9b11597..82a4a78 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1884,6 +1884,10 @@ receive.updateserverinfo::
>> If set to true, git-receive-pack will run git-update-server-info
>> after receiving data from git-push and updating refs.
>>
>> +remote.pushdefault::
>> + The remote to push to by default. Overrides the
>> + branch-specific configuration `branch.<name>.remote`.
>
> It feels unexpected to see "I may have said while on this branch I
> push there and on that branch I push somewhere else, but no, with
> this single configuration I'm invalidating all these previous
> statements, and all pushes go to this new place".
>
> Shouldn't the default be the default that is to be overridden by
> other configuration that is more specific? That is, "I would
> normally push to this remote and unless I say otherwise that is all
> I have to say, but for this particular branch, I push to somehwere
> else".
I'm a little confused as to where this configuration variable will be
useful. On a fresh clone from Github, I get branch.master.remote
configured to "origin". How will adding remote.pushdefault have any
impact, unless I explicitly remove this branch-specific remote
configuration? Besides, without branch.<name>.remote configured, I
can't even pull and expect changes to be merged. So, really: what is
the use of remote.pushdefault?
I'm dropping this patch, and just going with branch.<name>.pushremote,
unless you convince me otherwise.
^ permalink raw reply
* Re: [BUG?] google code http auth weirdness
From: Shawn Pearce @ 2013-03-17 1:11 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130315115947.GA30675@sigill.intra.peff.net>
On Fri, Mar 15, 2013 at 4:59 AM, Jeff King <peff@peff.net> wrote:
> I tried pushing to a repository at Google Code for the first time today,
> and I encountered some weird behavior with respect to asking for
> credentials.
>
> If I use the url "https://code.google.com/r/repo/", everything works; I
> get prompted for my username/password.
>
> But if I instead use the url "https://myuser@code.google.com/r/repo/",
> it doesn't work. I get:
>
> $ git push
> fatal: remote error: Invalid username/password.
> You may need to use your generated googlecode.com password; see
> https://code.google.com/hosting/settings
>
> without any prompt at all. I bisected it to 986bbc0 (http: don't always
> prompt for password, 2011-11-04), but I think that is a red herring. It
> worked before that commit because we always asked for the password,
> before we even talked to the server.
>
> After that commit, we should be reacting to an HTTP 401. But it seems that
> Google Code's 401 behavior is odd. When t5540 checks this, the
> conversation goes something like:
>
> 1. We do a GET with no "Authorization" header.
>
> 2. The server returns a 401, asking for credentials.
>
> 3. Curl repeats the GET request, putting the username into the
> Authorization header.
>
> 4. The server returns a 401, again, as our credential is not
> sufficient.
>
> 5. Curl returns the 401 to git; git prompts for the credential, feeds
> it to curl, and then repeats the request. It works.
>
> But with Google Code, the first three steps are identical. But then for
> step 4, the server returns this:
>
> < HTTP/1.1 200 OK
> < Content-Type: application/x-git-receive-pack-advertisement
> < X-Content-Type-Options: nosniff
> < Date: Fri, 15 Mar 2013 11:43:14 GMT
> < Server: git_frontend
> < Content-Length: 175
> < X-XSS-Protection: 1; mode=block
> <
> * Connection #0 to host code.google.com left intact
> packet: git< # service=git-receive-pack
> packet: git< 0000
> packet: git< ERR Invalid username/password [...]
>
> That seems kind of crazy to me. It's generating an HTTP 200 just to tell
> us the credentials are wrong. Which kind of makes sense; it's the only
> way to convince the git client to show a custom message when it aborts
> (rather than producing its own client-side "authorization failed"
> message).
Actually the reason here wasn't to use a custom message. It was to
avoid the client from entering into the old /info/refs fallback path
that existed between 703e6e76 (Git 1.6.6.2) and 6ac964a62 (Git
1.7.12.3). During these versions non-200 responses from the smart
request usually led to a useless error in the client. I suggested to
the Google Code team when they implemented Git support to use a 200
response with the ERR header to give the end-user something easier to
understand than what the Git client have otherwise printed.
But in hindsight maybe I should have told them to always return 401
and let the client handle the error.
FWIW this same "misfeature" probably exists in Gerrit Code Review and
does exist on {gerrit,android}.googlesource.com because it also came
from me.
> But it takes the retry decision out of the client's hands. And
> in this case, it is wrong; the failed credential is a result of curl
> trying the username only, and git never even gets a chance to provide
> the real credential.
>
> Technically this did work before 986bbc0, so it could be considered a
> regression in git, but I really think that Google Code is in the wrong
> here. It should either:
>
> 1. Always return a 401 for bad credentials. This means it would lose
> the custom message. But I think that is a good indication that the
> client should be putting more effort into showing the body of the
> 401. Probably not all the time, as we do not want to spew HTML at
> the user, but we could perhaps relay error bodies if the
> content-type is "application/x-git-error" or something.
>
> The downside is that even if we make such a client change and the
> the Google Code server change sit's behavior, people on older git
> clients will lose the benefit of the message.
I don't think this is the way to handle errors in the protocol. I
prefer the existing approach of sending a 200 OK with the ERR header
to indicate the message to show to the client. It works since 1.6.6
when smart HTTP was introduced, and it has a very specific meaning.
The 200 is stating the transport worked, and the ERR is stating the
Git operation did not. :-)
Where we have an issue is on a recoverable authentication error.
Recoverable authentication errors should use 401 so the client can try
again. I don't know how older (e.g. 1.6.6) clients behave here with a
401 response. I guess I should crack out a 1.6.6 build and test.
> 2. Treat a credential with a non-empty username and an empty password
> specially, and return a 401. This covers the specific case of
> https://user@host/, but continues to show the custom message when
> the user provides a wrong password. It does mean that the custom
> message will not be shown if the user actually enters a blank
> password at the prompt (but it will still be shown if they get
> prompted for both username and password and leave both blank).
>
> So it's a little hacky, but I think it would work OK in practice.
I don't work on Google Code, but I have asked the team to consider
making at least this change.
We haven't deployed it yet, but I made the change for
{android,gerrit}.googlesource.com, and should try to get the same
thing into Gerrit Code Review.
^ permalink raw reply
* experimenting with JGit and bitmaps
From: Shawn Pearce @ 2013-03-17 1:29 UTC (permalink / raw)
To: git; +Cc: Colby Ranger
JGit has merged the bitmap work Colby and I were working on[1] and
plans to ship it in JGit 2.4. The bitmaps are now stored in a separate
".bitmap" file alongside of a pack, making the entire system
backward-compatible with git-core.
If you have Java and Maven installed you can try this out locally:
$ git clone https://eclipse.googlesource.com/jgit/jgit
# (fastest mirror of jgit is self-hosted here)
$ cd jgit
$ mvn package -Dmaven.test.skip=true -Dmaven.javadoc.skip=true
$ org.eclipse.jgit.pgm/target/jgit debug-gc
$ org.eclipse.jgit.pgm/target/jgit daemon --export-all --listen 127.0.0.1 . &
$ git clone git://127.0.0.1/. tmp
I will be presenting some of this work at EclipseCon and ALMConnect.
JGit's GC implementation is still being worked on, hence the
"debug-gc" name. I would only run it on a backup of a repository.
Fortunately this is easy with Git. :-)
[1] http://thread.gmane.org/gmane.comp.version-control.git/206457
[2] http://www.eclipsecon.org/2013/sessions/scaling-jgit
http://www.eclipsecon.org/2013/sessions/deploying-gerrit-code-review
^ permalink raw reply
* Re: [BUG?] google code http auth weirdness
From: Jeff King @ 2013-03-17 3:00 UTC (permalink / raw)
To: Shawn Pearce; +Cc: git
In-Reply-To: <CAJo=hJt5P4616VLGcjdGFQ0YA_+gT+o1Vw3zWSbafDT6Cu6M2w@mail.gmail.com>
On Sat, Mar 16, 2013 at 06:11:32PM -0700, Shawn O. Pearce wrote:
> > That seems kind of crazy to me. It's generating an HTTP 200 just to tell
> > us the credentials are wrong. Which kind of makes sense; it's the only
> > way to convince the git client to show a custom message when it aborts
> > (rather than producing its own client-side "authorization failed"
> > message).
>
> Actually the reason here wasn't to use a custom message. It was to
> avoid the client from entering into the old /info/refs fallback path
> that existed between 703e6e76 (Git 1.6.6.2) and 6ac964a62 (Git
> 1.7.12.3). During these versions non-200 responses from the smart
> request usually led to a useless error in the client. I suggested to
> the Google Code team when they implemented Git support to use a 200
> response with the ERR header to give the end-user something easier to
> understand than what the Git client have otherwise printed.
OK, that makes sense. I do wonder, though, if the loss of the custom
message will be bad for Google Code. The current message is:
fatal: remote error: Invalid username/password.
You may need to use your generated googlecode.com password; see
https://code.google.com/hosting/settings
which is an important clue (this being the first time I had used Google
Code to authenticate, I was quite surprised that the password was
different from the web page's password, and that pointer helped me
figure it out more quickly).
> > 1. Always return a 401 for bad credentials. This means it would lose
> > the custom message. But I think that is a good indication that the
> > client should be putting more effort into showing the body of the
> > 401. Probably not all the time, as we do not want to spew HTML at
> > the user, but we could perhaps relay error bodies if the
> > content-type is "application/x-git-error" or something.
> >
> > The downside is that even if we make such a client change and the
> > the Google Code server change sit's behavior, people on older git
> > clients will lose the benefit of the message.
>
> I don't think this is the way to handle errors in the protocol. I
> prefer the existing approach of sending a 200 OK with the ERR header
> to indicate the message to show to the client. It works since 1.6.6
> when smart HTTP was introduced, and it has a very specific meaning.
> The 200 is stating the transport worked, and the ERR is stating the
> Git operation did not. :-)
Sorry, I should have been more clear. I mean only to use it for
transport errors, not git errors. So we should not in general be
converting 200 responses into something else (with the exception of the
401, because it does have a transport meaning). But when we _do_ issue
an unsuccessful HTTP code, we should include a message that can be
relayed to the user.
> Where we have an issue is on a recoverable authentication error.
> Recoverable authentication errors should use 401 so the client can try
> again. I don't know how older (e.g. 1.6.6) clients behave here with a
> 401 response. I guess I should crack out a 1.6.6 build and test.
Before 42653c0 (Prompt for a username when an HTTP request 401s,
2010-04-01), in v1.7.1.1, I think you just get something like:
error: 401 Authorization failed
fatal: HTTP request failed
> I don't work on Google Code, but I have asked the team to consider
> making at least this change.
Thanks for looking into it.
-Peff
^ permalink raw reply
* [PATCH] Preallocate hash tables when the number of inserts are known in advance
From: Nguyễn Thái Ngọc Duy @ 2013-03-17 3:28 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
This avoids unnecessary re-allocations and reinsertions. On webkit.git
(i.e. about 182k inserts to the name hash table), this reduces about
100ms out of 3s user time.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
nd/read-directory-recursive-optim reduces the number of input (from
182k to 11k on webkit) to exclude machinery that all patches in the
exclude optimization series I posted seem insignificant. So I won't
repost them for inclusion unless you think it has cleanup values.
This one is worth doing though. I think keeping "untracked index"
would help avoid looking up in name-hash, where all user-space CPU
cycles are spent. But I have nothing to show about that.
diffcore-rename.c | 1 +
hash.h | 7 +++++++
name-hash.c | 2 ++
3 files changed, 10 insertions(+)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 512d0ac..8d3d9bb 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -389,6 +389,7 @@ static int find_exact_renames(struct diff_options *options)
struct hash_table file_table;
init_hash(&file_table);
+ preallocate_hash(&file_table, (rename_src_nr + rename_dst_nr) * 2);
for (i = 0; i < rename_src_nr; i++)
insert_file_table(&file_table, -1, i, rename_src[i].p->one);
diff --git a/hash.h b/hash.h
index b875ce6..244d1fe 100644
--- a/hash.h
+++ b/hash.h
@@ -40,4 +40,11 @@ static inline void init_hash(struct hash_table *table)
table->array = NULL;
}
+static inline void preallocate_hash(struct hash_table *table, unsigned int size)
+{
+ assert(table->size == 0 && table->nr == 0 && table->array == NULL);
+ table->size = size;
+ table->array = xcalloc(sizeof(struct hash_table_entry), size);
+}
+
#endif
diff --git a/name-hash.c b/name-hash.c
index 942c459..12364d1 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -92,6 +92,8 @@ static void lazy_init_name_hash(struct index_state *istate)
if (istate->name_hash_initialized)
return;
+ if (istate->cache_nr)
+ preallocate_hash(&istate->name_hash, istate->cache_nr * 2);
for (nr = 0; nr < istate->cache_nr; nr++)
hash_index_entry(istate, istate->cache[nr]);
istate->name_hash_initialized = 1;
--
1.8.2.83.gc99314b
^ permalink raw reply related
* Re: [PATCH] rev-parse: Clarify documentation of @{upstream} syntax
From: Eric Sunshine @ 2013-03-17 4:31 UTC (permalink / raw)
To: Kacper Kornet; +Cc: git
In-Reply-To: <1363459903-32358-1-git-send-email-draenog@pld-linux.org>
On Sat, Mar 16, 2013 at 2:51 PM, Kacper Kornet <draenog@pld-linux.org> wrote:
> git-rev-parse interprets string in string@{upstream} as a name of
> a branch not a ref. For example refs/heads/master@{upstream} looks
> for an upstream branch that is merged by git-pull to ref
> refs/heads/refs/heads/master not to refs/heads/master. However the
> documentation could misled a user to believe that the string is
s/misled/mislead/
> interpreted as ref.
^ permalink raw reply
* Re: [PATCH] status: hint the user about -uno if read_directory takes too long
From: Junio C Hamano @ 2013-03-17 4:47 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Duy Nguyen, git, artagnon, robert.allan.zeh, finnag
In-Reply-To: <51441D8E.7090303@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
>> Or we can be more explicit and say
>>
>> # It took 2.58 seconds to search for untracked files. 'status -uno'
>> # may speed it up, but you have to be careful not to forget to add
>> # new files yourself (see 'git help status').
>>
> Thanks, that looks good for me
OK, then I'll squash this in to the version queued to 'pu', but we
should start thinking about merging these multi-line messages into
one multi-line strings that is split by the output layer to help the
localization folks, using something like strbuf_commented_addf() and
strbuf_add_commented_lines().
diff --git a/wt-status.c b/wt-status.c
index 6e75468..53c2222 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1027,10 +1027,14 @@ void wt_status_print(struct wt_status *s)
if (advice_status_u_option && 2000 < s->untracked_in_ms) {
status_printf_ln(s, GIT_COLOR_NORMAL, "");
status_printf_ln(s, GIT_COLOR_NORMAL,
- _("It took %.2f seconds to enumerate untracked files."),
+ _("It took %.2f seconds to enumerate untracked files."
+ " 'status -uno'"),
s->untracked_in_ms / 1000.0);
status_printf_ln(s, GIT_COLOR_NORMAL,
- _("Consider the -u option for a possible speed-up?"));
+ _("may speed it up, but you have to be careful not"
+ " to forget to add"));
+ status_printf_ln(s, GIT_COLOR_NORMAL,
+ _("new files yourself (see 'git help status')."));
}
} else if (s->commitable)
status_printf_ln(s, GIT_COLOR_NORMAL, _("Untracked files not listed%s"),
^ permalink raw reply related
* Re: [PATCH] pull: Apply -q and -v options to rebase mode as well
From: Junio C Hamano @ 2013-03-17 4:53 UTC (permalink / raw)
To: Peter Eisentraut; +Cc: git
In-Reply-To: <1363314368.14066.3.camel@vanquo.pezone.net>
Peter Eisentraut <peter@eisentraut.org> writes:
> git pull passed -q and -v only to git merge, but they can be useful for
> git rebase as well, so pass them there, too. In particular, using -q
> shuts up the "Already up-to-date." message. Add test cases to prove it.
>
> Signed-off-by: Peter Eisentraut <peter@eisentraut.org>
> ---
Looks quite straight-forward.
I wouldn't call our test cases "proving" anything, though. The
reason we add tests is to make sure that others who touch the code
later will not break the feature you add today by documenting the
behaviour we expect out of our code.
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 1b06691..aa31abe 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -19,6 +19,17 @@ test_expect_success 'git pull -q' '
> test ! -s out)
> '
>
> +test_expect_success 'git pull -q --rebase' '
> + mkdir clonedqrb &&
> + (cd clonedqrb && git init &&
> + git pull -q --rebase "../parent" >out 2>err &&
> + test ! -s err &&
> + test ! -s out &&
> + git pull -q --rebase "../parent" >out 2>err &&
> + test ! -s err &&
> + test ! -s out)
> +'
Pulling twice is a good thing here, to see how it behaves when there
is something to be fetched, and when you are up to date. I think it
is a good idea to add it to the normal 'pull -q' test.
Thanks.
^ 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