* Re: [PATCH/RFC 0/6] commit caching
From: Jeff King @ 2013-01-30 7:18 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Shawn O. Pearce
In-Reply-To: <CACsJy8BE3LdxbZzdQXuvEJop23KnnLbCTgPos9CywKV7EY2q9g@mail.gmail.com>
On Wed, Jan 30, 2013 at 10:31:43AM +0700, Nguyen Thai Ngoc Duy wrote:
> On Tue, Jan 29, 2013 at 4:14 PM, Jeff King <peff@peff.net> wrote:
> > The timings from this one are roughly similar to what I posted earlier.
> > Unlike the earlier version, this one keeps the data for a single commit
> > together for better cache locality (though I don't think it made a big
> > difference in my tests, since my cold-cache timing test ends up touching
> > every commit anyway). The short of it is that for an extra 31M of disk
> > space (~4%), I get a warm-cache speedup for "git rev-list --all" of
> > ~4.2s to ~0.66s.
>
> Some data point on caching 1-parent vs 2-parent commits on webkit
> repo, 26k commits. With your changes (caching 2-parent commits), the
> .commits file takes 2241600 bytes. "rev-list --all --quiet":
Hmm. My webkit repo has zero merges in it (though it is the older
svn-based one). What percentage of the one you have are merges? How does
your 1-parent cache perform on something like git.git, where about 25%
of all commits are merges?
> The performance loss in 1-parent case is not significant while disk
> saving is (although it'll be less impressive after you do Shawn's
> suggestion not storing SHA-1 directly)
Yeah, I think moving to offsets instead of sha1s is going to be a big
enough win that it won't matter anymore.
-Peff
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Junio C Hamano @ 2013-01-30 7:17 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <20130130071209.GD11147@sigill.intra.peff.net>
> True, but it is even less headache if the file is totally separate and
> optional.
Once you start thinking about using an offset to some list of SHA-1,
perhaps? A section inside the same file can never go out of sync.
Also a longer-term advantage is that you can teach index-pack to do
this.
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Jeff King @ 2013-01-30 7:12 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Shawn O. Pearce
In-Reply-To: <CACsJy8COCpr_CvQPhBFtVnsKztA3xgo01=zKG4WPcFfCbNTmgw@mail.gmail.com>
On Wed, Jan 30, 2013 at 10:36:10AM +0700, Nguyen Thai Ngoc Duy wrote:
> On Tue, Jan 29, 2013 at 4:16 PM, Jeff King <peff@peff.net> wrote:
> > +int commit_metapack(unsigned char *sha1,
> > + uint32_t *timestamp,
> > + unsigned char **tree,
> > + unsigned char **parent1,
> > + unsigned char **parent2)
> > +{
>
> Nit picking. tree, parent1 and parent2 can/should be "const unsigned char **".
Thanks, I'll fix it in the next version.
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Jeff King @ 2013-01-30 7:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <7vtxpzq2uv.fsf@alter.siamese.dyndns.org>
On Tue, Jan 29, 2013 at 10:08:08AM -0800, Junio C Hamano wrote:
> > In order to reduce the disk footprint and I/O cost, the future
> > direction for this mechanism may want to point into an existing
> > store of SHA-1 hashes with a shorter file offset, and the .idx file
> > could be such a store, and in order to move in that direction, you
> > cannot avoid tying a metapack to a pack.
>
> Have you considered if we can extend the .idx format version 2
> without actually having to bump the version number? My quick
> reading of check_packed_git_idx() tells me that we have a gap after
> the "large offset table" that we can place extensions, but I may be
> mistaken. If we indeed can, then perhaps adding a series of
>
> 4-byte "id" header
> 4-byte extension length (or 8-byte)
> ... N-byte worth of extension data ...
>
> followed by
>
> 20-byte SHA-1 checksum of all the extension sections
> 8-byte file offset to the first extension section
I considered it, but didn't look into it closely. My feeling was that it
added extra complexity, without adding any real advantage that a
separate file would not.
>From this:
> Then it will be very natural for the extension data that store the
> commit metainfo to name objects in the pack the .idx file describes
> by the offset in the SHA-1 table.
I guess your argument is that putting it all in the same file makes it
more natural for there to be a data dependency.
> As we always say, .idx is a local cache and bumping its version is
> not a huge headache compared to other longer term storage items.
True, but it is even less headache if the file is totally separate and
optional.
-Peff
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Jeff King @ 2013-01-30 7:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <7vy5fbq48t.fsf@alter.siamese.dyndns.org>
On Tue, Jan 29, 2013 at 09:38:10AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > +int commit_metapack(unsigned char *sha1,
> > + uint32_t *timestamp,
> > + unsigned char **tree,
> > + unsigned char **parent1,
> > + unsigned char **parent2)
> > +{
> > + struct commit_metapack *p;
> > +
> > + prepare_commit_metapacks();
> > + for (p = commit_metapacks; p; p = p->next) {
> > + unsigned char *data;
> > + int pos = sha1_entry_pos(p->index, 20, 0, 0, p->nr, p->nr, sha1);
>
> This is a tangent, but isn't it about time to rip out the check for
> GIT_USE_LOOKUP in find_pack_entry_one(), I wonder.
Rip it out and always use sha1_entry_pos, or rip it out and never use
it? I have never been able to measure any speedup from it; I just use it
here to avoid rewriting the binary search myself. I do not think there
is any disadvantage to using it, so I'd be in favor of just
standardizing on it for any sha1 binary searches.
> These cached properties of a single commit will not change no matter
> which pack it appears in, and it feels logically wrong, especially
> when you record these object names in the full SHA-1 form, to tie a
> "commit metapack" to a pack. Logically there needs only one commit
> metapack that describes all the commits known to the repository when
> the metapack was created.
True. I had originally envisioned it as tied to the packfile to help
manage the lifecycle. You know you need to generate a metapack for some
objects when they get packed, and you know you can throw it away when
the associated pack goes away. And it means you can verify the integrity
of the metapack by matching it to a particular packfile.
However, if you just have a commit cache, you can always blow the whole
thing away and regenerate it for all objects in the repo. It does not
have tied to a pack (you do end up doing some extra work at regeneration
when you are not doing a full repack, but it is really not enough to
worry about).
> In order to reduce the disk footprint and I/O cost, the future
> direction for this mechanism may want to point into an existing
> store of SHA-1 hashes with a shorter file offset, and the .idx file
> could be such a store, and in order to move in that direction, you
> cannot avoid tying a metapack to a pack.
Yes. That was not one of my original goals for the commit cache, but I
do think it's a useful direction to go in. And reachability bitmaps
(which would eventually be their own metapacks) would generally want to
be per-pack, too, for the same reason.
> > + *tail = &commit_list_insert(c, *tail)->next;
> > +}
>
> It feels somewhat wasteful to:
>
> - use commit_list for this, rather than an array of commit
> objects. If you have a rough estimate of the number of commits
> in the pack, you could just preallocate a single array and use
> ALLOC_GROW() on it, no?
We don't have a rough estimate, but yes, we could just use an array and
trust ALLOC_GROW to be reasonable. The use of commit_list did not have a
particular reason other than that it was simple (an array means stuffing
the array pointer and the nr and alloc counts into a struct to get to
the callback). The performance of writing the cache is dominated by
accessing the objects themselves, anyway. I don't mind changing it,
though, if you think it's clearer as an array.
> - iterate over the .idx file and run sha1_object_info() and
> parse_commit() on many objects in the SHA-1 order. Iterating in
> the way builtin/pack-objects.c::get_object_details() does avoids
> jumping around in existing packfiles, which may be more
> efficient, no?
Probably, though generating the complete commit cache for linux-2.6.git
takes only about 7 seconds on my machine. I wasn't too concerned with
optimizing generation, since it will typically be dwarfed by repacking
costs. It might make more of a difference for doing a metapack on all
objects (e.g., reachability bitmaps).
The reason I do it in .idx order is that it feeds the callback in sorted
order, so a writer could in theory just use that output as-is (and my
initial version did that, as it wrote separate metapacks for each
element). This version now puts all data elements together (for cache
locality), and builds the in-memory list so we do not have to re-do
sha1_object_info repeatedly. So it could very easily just generate the
list in pack order and sort it at the end.
I'll look into that for the next version.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/4] mergetool--lib: simplify command expressions
From: Junio C Hamano @ 2013-01-30 7:04 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <1359526854-25132-1-git-send-email-davvid@gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Update variable assignments to always use $(command "$arg")
> in their RHS instead of "$(command "$arg")" as the latter
> is harder to read. Make get_merge_tool_cmd() simpler by
> avoiding "echo" and $(command) substitutions completely.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This is a replacement patch for what's currently in pu to
> fix the empty "test -z" expression.
Thanks.
I think I already pushed out what I locally amended; will double
check.
^ permalink raw reply
* Re: [PATCH 3/6] introduce pack metadata cache files
From: Jeff King @ 2013-01-30 6:50 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Shawn O. Pearce
In-Reply-To: <CACsJy8Af9g9Tdqr0dXyucq-M2aP5U2HvX3ANSsabqtuX=w4Q0w@mail.gmail.com>
On Wed, Jan 30, 2013 at 08:30:57AM +0700, Nguyen Thai Ngoc Duy wrote:
> On Tue, Jan 29, 2013 at 4:15 PM, Jeff King <peff@peff.net> wrote:
> > +static void write_meta_header(struct metapack_writer *mw, const char *id,
> > + uint32_t version)
> > +{
> > + version = htonl(version);
> > +
> > + sha1write(mw->out, "META", 4);
> > + sha1write(mw->out, "\0\0\0\1", 4);
> > + sha1write(mw->out, mw->pack->sha1, 20);
> > + sha1write(mw->out, id, 4);
> > + sha1write(mw->out, &version, 4);
> > +}
>
> Because you go with all-commit-info-in-one-file, perhaps we should
> have an uint32_t bitmap to describe what info this cache contains? So
> far we need 4 bits for date, tree, 1st and 2nd parents (yes, I still
> want to check if storing 1-parent commits only gains us anything on
> some other repos). When commit count comes, it can take the fifth bit.
> Reachability bitmap offsets can take the sixth bit, if we just append
> the bitmaps at the end of the same file.
I thought about having some programmatic self-describing header like
that. But it makes the implementation much harder to verify, and it is
not like there is much point in picking and choosing those bits. My plan
was to do a combination of:
1. Put truly optional bits into a separate metapack (e.g.,
reachability bitmaps).
2. When something becomes obviously obsolete (e.g., we move to
generation numbers instead of timestamps in commits), bump the
version number.
-Peff
^ permalink raw reply
* Re: [PATCH 3/6] introduce pack metadata cache files
From: Jeff King @ 2013-01-30 6:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <7v622friy7.fsf@alter.siamese.dyndns.org>
On Tue, Jan 29, 2013 at 09:35:12AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > +static void write_meta_header(struct metapack_writer *mw, const char *id,
> > + uint32_t version)
> > +{
> > + version = htonl(version);
> > +
> > + sha1write(mw->out, "META", 4);
> > + sha1write(mw->out, "\0\0\0\1", 4);
> > + sha1write(mw->out, mw->pack->sha1, 20);
> > + sha1write(mw->out, id, 4);
> > + sha1write(mw->out, &version, 4);
> > +}
>
> It seems that you are very close to actually having a plumbing that
> could also do the pack .idx files. Until/unless that can be done, I
> am not sure how much benefit we would be getting from a file format
> that records a subtype "id" and a generic "META" type, instead of
> just a single "id" as the type ehader. But it is OK to use 8 extra
> bytes if we can potentially gain something later.
Yeah, I considered going that route. I had initially envisioned having a
generic META file type that provided some services (like fixed-size
records), and then having individual subtypes below that. But as I
simplified the design, the META format became pretty much pointless. I
left it in as the 8 bytes are not really a big problem, and it means we
can treat metapacks generically in some cases without necessarily
knowing what is in them. But I don't have a specific use case in mind,
so perhaps it is just useless and confusing. I don't mind simplifying.
> Shouldn't id be validated with at least something like
>
> if (strlen(id) < 3)
> die("Bad id: %s", id);
>
> to catch a call
>
> write_meta_header(&mw, "me", 47);
>
> that will stuff 'm', 'e', NUL and the garbage the compiler/linker
> combo has placed after that constant string in the 4-byte id field?
Yes, the id does need to be at least 4 bytes. Since the id is intended
to be a static string, I had planned to just document the requirement in
the API documentation. I don't mind putting in a run-time check. I had
originally had a separate "id" parameter that could be "char id[4]", but
found that it was just redundant with the "name" parameter: you ended up
passing ("commit", "CMIT") or similar.
> > + strbuf_addstr(&path, pack_idx);
> > + strbuf_chompstr(&path, ".idx");
> > + strbuf_addch(&path, '.');
> > + strbuf_addstr(&path, name);
>
> Your chompstr() does not even validate if the given name ends with
> ".idx",
Yeah, my intent was that it would be liberal in its input (i.e., take
just "pack-*"). E.g., you can run "git metapack pack/pack-XXXX".
> so this sounds like a glorified way to say
>
> strbuf_splice(&path, path->len - strlen("idx"), strlen("idx"),
> name, strlen(name));
>
> to me.
Yup, though my version handles edge cases by not chomping (e.g., what
does splice do when path->len is less than 3?).
> > +void metapack_writer_finish(struct metapack_writer *mw)
> > +{
> > + const char *tmp = mw->out->name;
> > +
> > + sha1close(mw->out, NULL, CSUM_FSYNC);
> > + if (rename(tmp, mw->path))
> > + die_errno("unable to rename temporary metapack file");
>
> Who is responsible for running adjust_shared_perm()? The caller, or
> this function?
I didn't think about it at all, but it seems pretty obvious to me that
this function should do so. Thanks for pointing it out.
-Peff
^ permalink raw reply
* [PATCH v3 1/4] mergetool--lib: simplify command expressions
From: David Aguilar @ 2013-01-30 6:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
In-Reply-To: <7vip6foc9m.fsf@alter.siamese.dyndns.org>
Update variable assignments to always use $(command "$arg")
in their RHS instead of "$(command "$arg")" as the latter
is harder to read. Make get_merge_tool_cmd() simpler by
avoiding "echo" and $(command) substitutions completely.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is a replacement patch for what's currently in pu to
fix the empty "test -z" expression.
git-mergetool--lib.sh | 42 ++++++++++++++++++------------------------
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1d0fb12..1ff6d38 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -32,17 +32,10 @@ check_unchanged () {
fi
}
-valid_tool_config () {
- if test -n "$(get_merge_tool_cmd "$1")"
- then
- return 0
- else
- return 1
- fi
-}
-
valid_tool () {
- setup_tool "$1" || valid_tool_config "$1"
+ setup_tool "$1" && return 0
+ cmd=$(get_merge_tool_cmd "$1")
+ test -n "$cmd"
}
setup_tool () {
@@ -96,14 +89,13 @@ setup_tool () {
}
get_merge_tool_cmd () {
- # Prints the custom command for a merge tool
merge_tool="$1"
if diff_mode
then
- echo "$(git config difftool.$merge_tool.cmd ||
- git config mergetool.$merge_tool.cmd)"
+ git config "difftool.$merge_tool.cmd" ||
+ git config "mergetool.$merge_tool.cmd"
else
- echo "$(git config mergetool.$merge_tool.cmd)"
+ git config "mergetool.$merge_tool.cmd"
fi
}
@@ -114,7 +106,7 @@ run_merge_tool () {
GIT_PREFIX=${GIT_PREFIX:-.}
export GIT_PREFIX
- merge_tool_path="$(get_merge_tool_path "$1")" || exit
+ merge_tool_path=$(get_merge_tool_path "$1") || exit
base_present="$2"
status=0
@@ -145,7 +137,7 @@ run_merge_tool () {
# Run a either a configured or built-in diff tool
run_diff_cmd () {
- merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+ merge_tool_cmd=$(get_merge_tool_cmd "$1")
if test -n "$merge_tool_cmd"
then
( eval $merge_tool_cmd )
@@ -158,11 +150,11 @@ run_diff_cmd () {
# Run a either a configured or built-in merge tool
run_merge_cmd () {
- merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+ merge_tool_cmd=$(get_merge_tool_cmd "$1")
if test -n "$merge_tool_cmd"
then
- trust_exit_code="$(git config --bool \
- mergetool."$1".trustExitCode || echo false)"
+ trust_exit_code=$(git config --bool \
+ "mergetool.$1.trustExitCode" || echo false)
if test "$trust_exit_code" = "false"
then
touch "$BACKUP"
@@ -253,7 +245,7 @@ guess_merge_tool () {
# Loop over each candidate and stop when a valid merge tool is found.
for i in $tools
do
- merge_tool_path="$(translate_merge_tool_path "$i")"
+ merge_tool_path=$(translate_merge_tool_path "$i")
if type "$merge_tool_path" >/dev/null 2>&1
then
echo "$i"
@@ -300,9 +292,11 @@ get_merge_tool_path () {
fi
if test -z "$merge_tool_path"
then
- merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
+ merge_tool_path=$(translate_merge_tool_path "$merge_tool")
fi
- if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
+
+ merge_tool_cmd=$(get_merge_tool_cmd "$merge_tool")
+ if test -z "$merge_tool_cmd" &&
! type "$merge_tool_path" >/dev/null 2>&1
then
echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\
@@ -314,11 +308,11 @@ get_merge_tool_path () {
get_merge_tool () {
# Check if a merge tool has been configured
- merge_tool="$(get_configured_merge_tool)"
+ merge_tool=$(get_configured_merge_tool)
# Try to guess an appropriate merge tool if no tool has been set.
if test -z "$merge_tool"
then
- merge_tool="$(guess_merge_tool)" || exit
+ merge_tool=$(guess_merge_tool) || exit
fi
echo "$merge_tool"
}
--
1.8.0.9.g3370a50
^ permalink raw reply related
* Re: [PATCH 2/6] strbuf: add string-chomping functions
From: Michael Haggerty @ 2013-01-30 5:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <20130129111028.GA11055@sigill.intra.peff.net>
On 01/29/2013 12:10 PM, Jeff King wrote:
> On Tue, Jan 29, 2013 at 11:15:34AM +0100, Michael Haggerty wrote:
>> Please document the new functions in
>> Documentation/technical/api-strbuf.txt. Personally I would also
>> advocate a "docstring" in the header file, but obviously that preference
>> is the exception rather than the rule in the git project :-(
>
> Will do. I need to document the metapack functions, too, so I was thinking
> about experimenting with some inline documentation systems.
That would be great; it would make future API documentation much easier
and therefore (hopefully) more common.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Duy Nguyen @ 2013-01-30 3:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130129091610.GD9999@sigill.intra.peff.net>
On Tue, Jan 29, 2013 at 4:16 PM, Jeff King <peff@peff.net> wrote:
> +int commit_metapack(unsigned char *sha1,
> + uint32_t *timestamp,
> + unsigned char **tree,
> + unsigned char **parent1,
> + unsigned char **parent2)
> +{
Nit picking. tree, parent1 and parent2 can/should be "const unsigned char **".
--
Duy
^ permalink raw reply
* Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
From: Junio C Hamano @ 2013-01-30 3:34 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <7vwquvmkon.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Heh, I actually was hoping that you will send in a replacement for
> David's patch ;-)
>
> Here is what I will squash into the one we have been discussing. In
> a few hours, I expect I'll be able to push this out in the 'pu'
> branch.
I ended up doing this a bit differently; will push out the result
after merging the other topics to 'pu'.
^ permalink raw reply
* Re: [PATCH/RFC 0/6] commit caching
From: Duy Nguyen @ 2013-01-30 3:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130129091434.GA6975@sigill.intra.peff.net>
On Tue, Jan 29, 2013 at 4:14 PM, Jeff King <peff@peff.net> wrote:
> The timings from this one are roughly similar to what I posted earlier.
> Unlike the earlier version, this one keeps the data for a single commit
> together for better cache locality (though I don't think it made a big
> difference in my tests, since my cold-cache timing test ends up touching
> every commit anyway). The short of it is that for an extra 31M of disk
> space (~4%), I get a warm-cache speedup for "git rev-list --all" of
> ~4.2s to ~0.66s.
Some data point on caching 1-parent vs 2-parent commits on webkit
repo, 26k commits. With your changes (caching 2-parent commits), the
.commits file takes 2241600 bytes. "rev-list --all --quiet":
0.06user 0.00system 0:00.08elapsed 95%CPU (0avgtext+0avgdata 26288maxresident)k
0inputs+0outputs (0major+2094minor)pagefaults 0swaps
With caching 1-parent commits only, the .commits file takes 1707900
bytes (30% less), the same rev-list command:
0.07user 0.00system 0:00.07elapsed 96%CPU (0avgtext+0avgdata 24144maxresident)k
0inputs+0outputs (0major+1960minor)pagefaults 0swaps
Compared to the timing without caching at all:
0.72user 0.02system 0:00.76elapsed 98%CPU (0avgtext+0avgdata 108976maxresident)k
0inputs+0outputs (0major+7272minor)pagefaults 0swaps
The performance loss in 1-parent case is not significant while disk
saving is (although it'll be less impressive after you do Shawn's
suggestion not storing SHA-1 directly)
--
Duy
^ permalink raw reply
* Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
From: Junio C Hamano @ 2013-01-30 3:08 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130129230607.GG1342@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Tue, Jan 29, 2013 at 02:55:26PM -0800, Junio C Hamano wrote:
> ...
>> I can work with John to get this part into a shape to support his
>> extended use sometime toward the end of this week, by which time
>> hopefully you have some time to comment on the result. John, how
>> does that sound?
>
> My email crossed with yours - that sounds good to me. If
> da/mergetool-docs is in a reasonable state by tomorrow evening (GMT) I
> should be able to have a look at it then - if not I'm happy to hold off
> longer.
Heh, I actually was hoping that you will send in a replacement for
David's patch ;-)
Here is what I will squash into the one we have been discussing. In
a few hours, I expect I'll be able to push this out in the 'pu'
branch.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 29 Jan 2013 18:57:55 -0800
Subject: [PATCH] [SQUASH] mergetools: tweak show_tool_names and its users
Use show_tool_names as a function to produce output, not as a
function to compute a string. Indicate if any output was given
with its return status, so that the caller can say "if it didn't
give any output, I'll say this instead" easily.
To be squashed into the previous; no need to keep this log message.
---
git-mergetool--lib.sh | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 135da96..79cbdc7 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -22,7 +22,7 @@ is_available () {
show_tool_names () {
condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
- ( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
+ ( cd "$MERGE_TOOLS_DIR" && ls ) |
while read toolname
do
if setup_tool "$toolname" 2>/dev/null &&
@@ -36,6 +36,7 @@ show_tool_names () {
printf "%s%s\n" "$per_line_prefix" "$tool"
fi
done
+ test -n "$preamble"
}
diff_mode() {
@@ -236,27 +237,30 @@ list_merge_tool_candidates () {
show_tool_help () {
tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"
- available=$(show_tool_names 'mode_ok && is_available' '\t\t' \
- "$tool_opt may be set to one of the following:")
- unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \
- "The following tools are valid, but not currently available:")
- if test -n "$available"
+
+ tab=' ' av_shown= unav_shown=
+
+ if show_tool_names 'mode_ok && is_available' "$tab$tab" \
+ "$tool_opt may be set to one of the following:"
then
- echo "$available"
+ av_shown=yes
else
echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
+ av_shown=no
fi
- if test -n "$unavailable"
+
+ if show_tool_names 'mode_ok && ! is_available' "$tab$tab" \
+ "The following tools are valid, but not currently available:"
then
- echo
- echo "$unavailable"
+ unav_shown=yes
fi
- if test -n "$unavailable$available"
- then
+
+ case ",$av_shown,$unav_shown," in
+ *,yes,*)
echo
echo "Some of the tools listed above only work in a windowed"
echo "environment. If run in a terminal-only session, they will fail."
- fi
+ esac
exit 0
}
--
1.8.1.2.555.gedafe79
^ permalink raw reply related
* Re: [RFC v2] git-multimail: a replacement for post-receive-email
From: Chris Hiestand @ 2013-01-30 2:27 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Michael Haggerty, git discussion list, Andy Parkins,
Sitaram Chamarty, Matthieu Moy, Junio C Hamano, Marc Branchaud
In-Reply-To: <CACBZZX7RA7dLcFhaHmmK97Kxfa9zLmozfdx5s9C=29DJOceq-A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 833 bytes --]
On Jan 29, 2013, at 7:25 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Sun, Jan 27, 2013 at 9:37 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> A while ago, I submitted an RFC for adding a new email notification
>> script to "contrib" [1]. The reaction seemed favorable and it was
>> suggested that the new script should replace post-receive-email rather
>> than be added separately, ideally with some kind of migration support.
>
> I just want to say since I think this thread hasn't been getting the
> attention it deserves: I'm all for this. I've used git-multimail and
> it's a joy to configure and extend compared to the existing hacky
> shellscript.
This seems good to me as long as it's okay for git contrib to depend on python.
I've started testing git-multimail in my environment.
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4798 bytes --]
^ permalink raw reply
* Re: [PATCH 3/6] introduce pack metadata cache files
From: Duy Nguyen @ 2013-01-30 1:30 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130129091555.GC9999@sigill.intra.peff.net>
On Tue, Jan 29, 2013 at 4:15 PM, Jeff King <peff@peff.net> wrote:
> +static void write_meta_header(struct metapack_writer *mw, const char *id,
> + uint32_t version)
> +{
> + version = htonl(version);
> +
> + sha1write(mw->out, "META", 4);
> + sha1write(mw->out, "\0\0\0\1", 4);
> + sha1write(mw->out, mw->pack->sha1, 20);
> + sha1write(mw->out, id, 4);
> + sha1write(mw->out, &version, 4);
> +}
Because you go with all-commit-info-in-one-file, perhaps we should
have an uint32_t bitmap to describe what info this cache contains? So
far we need 4 bits for date, tree, 1st and 2nd parents (yes, I still
want to check if storing 1-parent commits only gains us anything on
some other repos). When commit count comes, it can take the fifth bit.
Reachability bitmap offsets can take the sixth bit, if we just append
the bitmaps at the end of the same file.
--
Duy
^ permalink raw reply
* Re: [PATCH v2] status: show the branch name if possible in in-progress info
From: Duy Nguyen @ 2013-01-30 1:13 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Matthieu Moy
In-Reply-To: <20130129184435.GA18266@google.com>
On Wed, Jan 30, 2013 at 1:44 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> - # You are currently rebasing.
>> + # You are currently rebasing branch '\''rebase_conflicts'\'' on '\''000106f'\''.
>
> SHA1-in-tests radar blinking.
>
> Would it be possible to compute the expected output, as in
>
> dest=$(git rev-parse --short HEAD^^)
> cat >expected <<-EOF &&
> # Not currently on any branch.
> # You are currently rebasing branch '\''rebase_conflicts'\'' on '\''$dest'\''.
>
> ?
That may be better. Yeah.
> I'm not sure what to think about the actual change itself yet. Can you
> give an example of when you felt the need for this, so it can be
> included in the commit message or documentation?
http://thread.gmane.org/gmane.comp.version-control.git/214932/focus=214937
--
Duy
^ permalink raw reply
* Re: [PATCHv3] git-send-email: add ~/.authinfo parsing
From: Junio C Hamano @ 2013-01-30 0:34 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: git, Krzysztof Mazur, Michal Nazarewicz
In-Reply-To: <5d18d777d6ddf6f01bbf460f37af637d3dc28ed5.1359503987.git.mina86@mina86.com>
Michal Nazarewicz <mpn@google.com> writes:
>> Is it bad to use Net::Netrc instead? This looks like exactly the
>> use case that module was written for, no?
>
> I don't think that's the case. For one, Net::Netrc does not seem to
> process port number.
>
> There is a Text::Authinfo module but it just uses Text::CSV.
>
> I can change the code to use Net::Netrc, but I dunno if that's really
> the best option, since I feel people would expect parsing to be
> somehow compatible with
> <http://www.gnu.org/software/emacs/manual/html_node/gnus/NNTP.html>
> rather than the original .netrc file format.
Thanks for pushing back (I wish more contributors did so when I
suggest nonsense ;-)); you are right that both canned modules are
lacking.
Will queue. Thanks.
^ permalink raw reply
* Re: Cloning remote HTTP repository: Can only see 'master' branch
From: Michael Tyson @ 2013-01-30 0:06 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <20130129082317.GA6396@sigill.intra.peff.net>
Ah! Lovely, thank you, Jeff!
Alas, it's a shared server so I'm limited to what the host provides, but that solves my problem.
Cheers!
On 29 Jan 2013, at 19:23, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 29, 2013 at 04:54:13PM +1100, Michael Tyson wrote:
>
>> I've a readonly git repository that I'm hosting via HTTP (a bare git
>> repository located within the appropriate directory on the server). I
>> push to it via my own SSH account (local repository with a remote
>> pointing to the ssh:// URL).
>>
>> This has all worked fine so far - I push via ssh, and others can clone
>> and pull via the HTTP URL.
>>
>> I've recently added a branch - "beta" - which pushed just fine, but
>> now cloning via the HTTP URL doesn't seem to show the new branch -
>> just master:
>
> If you are using the "dumb" http protocol (i.e., the web server knows
> nothing about git, and just serves the repo files), you need to run "git
> update-server-info" after each push in order to update the static file
> that tells the git client about each ref. You can have git do it
> automatically for you by setting receive.updateServerInfo in the server
> repo's config.
>
> If the server is yours to control, consider setting up the "smart" http
> protocol, as it is much more efficient. Details are in "git help
> http-backend".
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCHv3] git-send-email: add ~/.authinfo parsing
From: Michal Nazarewicz @ 2013-01-30 0:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Krzysztof Mazur, Michal Nazarewicz
In-Reply-To: <7vehh3obs0.fsf@alter.siamese.dyndns.org>
From: Michal Nazarewicz <mina86@mina86.com>
Make git-send-email read password from a ~/.authinfo or ~/.netrc file
instead of requiring it to be stored in git configuration, passed as
command line argument or typed in.
There are various other applications that use this file for
authentication information so letting users use it for git-send-email
is convinient. Furthermore, some users store their ~/.gitconfig file
in a public repository and having to store password there makes it
easy to publish the password.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
Documentation/git-send-email.txt | 47 +++++++++++++++++---
git-send-email.perl | 93 ++++++++++++++++++++++++++++++++++------
2 files changed, 122 insertions(+), 18 deletions(-)
On Tue, Jan 29 2013, Junio C Hamano wrote:
> But .netrc/.authinfo format separates its entries with SP, HT, or
> LF. An entry begins with "machine <remote-hostname>" token pair.
>
> split(/\s+/) will not work for an entry that span multiple lines but
> CSV will not help, either.
>
> Is it bad to use Net::Netrc instead? This looks like exactly the
> use case that module was written for, no?
I don't think that's the case. For one, Net::Netrc does not seem to
process port number.
There is a Text::Authinfo module but it just uses Text::CSV.
I can change the code to use Net::Netrc, but I dunno if that's really
the best option, since I feel people would expect parsing to be
somehow compatible with
<http://www.gnu.org/software/emacs/manual/html_node/gnus/NNTP.html>
rather than the original .netrc file format.
> Hmph. I would have expected to see getservbyname.
Ha! Even better. :]
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index eeb561c..ac020d1 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -158,14 +158,49 @@ Sending
--smtp-pass[=<password>]::
Password for SMTP-AUTH. The argument is optional: If no
argument is specified, then the empty string is used as
- the password. Default is the value of 'sendemail.smtppass',
- however '--smtp-pass' always overrides this value.
+ the password. Default is the value of 'sendemail.smtppass'
+ or value read from ~/.authinfo file, however '--smtp-pass'
+ always overrides this value.
+
-Furthermore, passwords need not be specified in configuration files
-or on the command line. If a username has been specified (with
+Furthermore, passwords need not be specified in configuration files or
+on the command line. If a username has been specified (with
'--smtp-user' or a 'sendemail.smtpuser'), but no password has been
-specified (with '--smtp-pass' or 'sendemail.smtppass'), then the
-user is prompted for a password while the input is masked for privacy.
+specified (with '--smtp-pass', 'sendemail.smtppass' or via
+~/.authinfo file), then the user is prompted for a password while
+the input is masked for privacy.
++
+The ~/.authinfo file should contain a line with the following
+format:
++
+ machine <domain> port <port> login <user> password <pass>
++
+Instead of `machine <domain>` pair a `default` token can be used
+instead in which case all domains will match. Similarly, `port
+<port>` and `login <user>` pairs can be omitted in which case matching
+of the given value will be skipped. `<port>` can be either an integer
+or a symbolic name. Lines are interpreted in order and password from
+the first line that matches will be used. For instance, one may end
+up with:
++
+ machine example.com login jane port ssmtp password smtppassword
+ machine example.com login jane password janepassword
+ default login janedoe password doepassword
++
+if she wants to use `smtppassword` for authenticating as `jane` to
+a service at example.com:465 (SSMTP), `janepassword` for all other
+services at example.com; and `doepassword` when authonticating as
+`janedoe` to any service. If ~/.authinfo file is missing,
+'git-send-email' will also try ~/.netrc file (even though parsing is
+not fully compatible with ftp's .netrc file format).
++
+Note that you should never make ~/.authinfo file world-readable. To
+help guarantee that, you might want to create the file with the
+following command:
++
+ ( umask 077; cat >~/.authinfo <<EOF
+ ... file contents ...
+ EOF
+ )
--smtp-server=<host>::
If set, specifies the outgoing SMTP server to use (e.g.
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..a62dfa4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1045,6 +1045,86 @@ sub maildomain {
return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
}
+
+sub read_password_from_stdin {
+ my $line;
+
+ system "stty -echo";
+
+ do {
+ print "Password: ";
+ $line = <STDIN>;
+ print "\n";
+ } while (!defined $line);
+
+ system "stty echo";
+
+ chomp $line;
+ return $line;
+}
+
+sub authinfo_is_port_eq {
+ my ($from_file, $value, $filename) = @_;
+
+ if (!defined $from_file) {
+ return 1;
+ } elsif ($from_file =~ /^\d+$/) {
+ return $from_file == $value;
+ }
+
+ my $port = getservbyname $from_file, 'tcp';
+ if (!defined $port) {
+ print STDERR "$filename: invalid port name: $from_file\n";
+ return;
+ }
+
+ return $port == $value;
+}
+
+sub read_password_from_authinfo {
+ my $filename = join '/', $ENV{'HOME'}, $_[0] // '.authinfo';
+ my $fd;
+ if (!open $fd, '<', $filename) {
+ return;
+ }
+
+ my $password;
+ while (my $line = <$fd>) {
+ $line =~ s/^\s+|\s+$//g;
+ my @line = split /\s+/, $line;
+ my %line;
+ while (@line) {
+ my $token = shift @line;
+ if ($token eq 'default') {
+ $line{'machine'} = $smtp_server;
+ } elsif (@line) {
+ $line{$token} = shift @line;
+ }
+ }
+
+ if (defined $line{'password'} &&
+ defined $line{'machine'} &&
+ $line{'machine'} eq $smtp_server &&
+ (!defined $line{'login'} ||
+ $line{'login'} eq $smtp_authuser) &&
+ authinfo_is_port_eq($line{'port'}, $smtp_server_port, $filename)) {
+ $password = $line{'password'};
+ last;
+ }
+ }
+
+ close $fd;
+ return $password;
+}
+
+sub read_password {
+ return
+ read_password_from_authinfo '.authinfo' ||
+ read_password_from_authinfo '.netrc' ||
+ read_password_from_stdin;
+}
+
+
# Returns 1 if the message was sent, and 0 otherwise.
# In actuality, the whole program dies when there
# is an error sending a message.
@@ -1194,18 +1274,7 @@ X-Mailer: git-send-email $gitversion
};
if (!defined $smtp_authpass) {
-
- system "stty -echo";
-
- do {
- print "Password: ";
- $_ = <STDIN>;
- print "\n";
- } while (!defined $_);
-
- chomp($smtp_authpass = $_);
-
- system "stty echo";
+ $smtp_authpass = read_password
}
$auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
--
1.8.1
^ permalink raw reply related
* Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
From: John Keeping @ 2013-01-29 23:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7va9rroazl.fsf@alter.siamese.dyndns.org>
On Tue, Jan 29, 2013 at 02:55:26PM -0800, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
>
> > I don't want to stomp on your feet and poke at this file too much if
> > you're planning on building on top of it (I already did a few times
> > ;-). My git time is a bit limited for the next few days so I don't
> > want to hold you up. I can help shepherd through small fixups that
> > come up until the weekend rolls around and I have more time, but I
> > also don't want to hold you back until then.
>
> I can work with John to get this part into a shape to support his
> extended use sometime toward the end of this week, by which time
> hopefully you have some time to comment on the result. John, how
> does that sound?
My email crossed with yours - that sounds good to me. If
da/mergetool-docs is in a reasonable state by tomorrow evening (GMT) I
should be able to have a look at it then - if not I'm happy to hold off
longer.
John
^ permalink raw reply
* Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
From: John Keeping @ 2013-01-29 23:02 UTC (permalink / raw)
To: David Aguilar; +Cc: git, Junio C Hamano
In-Reply-To: <CAJDDKr4e=pg=YJ4CfUk7guUCcikBtXTveVX9j6CV5NtGvPB=9Q@mail.gmail.com>
On Tue, Jan 29, 2013 at 02:27:21PM -0800, David Aguilar wrote:
> I don't want to stomp on your feet and poke at this file too much if
> you're planning on building on top of it (I already did a few times
> ;-). My git time is a bit limited for the next few days so I don't
> want to hold you up. I can help shepherd through small fixups that
> come up until the weekend rolls around and I have more time, but I
> also don't want to hold you back until then.
>
> I will have some time tonight. If you guys would prefer an
> incremental patch I can send one that changes the "ls" expression and
> the way the unavailable block is structured. Otherwise, I can send
> replacements to handle the "test -z" thing, $TAB$TAB, and the
> simplification of the unavailable block.
>
> Later patches (that are working towards the new feature) can
> generalize show_tool_names() further and eliminate the need for the
> available/unavailable variables altogether. John, I would imagine
> that you'd want to pick that up since you're driving towards having
> --tool-help honor custom tools.
>
> What do you think?
I was planning to hold off until this series is in a reasonable state -
there's no rush as far as I'm concerned, but if Junio's happy to leave
these patches with just the small fixups I'm happy to build on that with
a patch that removes the available and unavailable variables before
adding the tools from git-config.
John
^ permalink raw reply
* Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
From: Junio C Hamano @ 2013-01-29 22:55 UTC (permalink / raw)
To: David Aguilar; +Cc: John Keeping, git
In-Reply-To: <CAJDDKr4e=pg=YJ4CfUk7guUCcikBtXTveVX9j6CV5NtGvPB=9Q@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> I don't want to stomp on your feet and poke at this file too much if
> you're planning on building on top of it (I already did a few times
> ;-). My git time is a bit limited for the next few days so I don't
> want to hold you up. I can help shepherd through small fixups that
> come up until the weekend rolls around and I have more time, but I
> also don't want to hold you back until then.
I can work with John to get this part into a shape to support his
extended use sometime toward the end of this week, by which time
hopefully you have some time to comment on the result. John, how
does that sound?
^ permalink raw reply
* Re: [PATCHv2] git-send-email: add ~/.authinfo parsing
From: Junio C Hamano @ 2013-01-29 22:38 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: git, Krzysztof Mazur, Michal Nazarewicz
In-Reply-To: <d58b0709cd86e0d336902b52d72e06dd9b52d70d.1359493459.git.mina86@mina86.com>
Michal Nazarewicz <mpn@google.com> writes:
>> It is rather strange to require a comma-separated-values parser to
>> read a file format this simple, isn't it?
>
> I was worried about spaces in password. CVS should handle such case
> nicely, whereas simple split won't. Nonetheless, I guess that in the
> end this is not likely enough to add the dependency.
But .netrc/.authinfo format separates its entries with SP, HT, or
LF. An entry begins with "machine <remote-hostname>" token pair.
split(/\s+/) will not work for an entry that span multiple lines but
CSV will not help, either.
Is it bad to use Net::Netrc instead? This looks like exactly the
use case that module was written for, no?
>> Perhaps you can convert at least some popular ones yourself? After
>> all, the user may be using an _existing_ .authinfo/.netrc that she
>> has been using with other programs that do understand symbolic port
>> names. Rather than forcing all such users to update their files,
>> the patch can work a bit harder for them and the world will be a
>> better place, no?
>
> Parsing /etc/services added.
Hmph. I would have expected to see getservbyname.
^ permalink raw reply
* Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions
From: Junio C Hamano @ 2013-01-29 22:27 UTC (permalink / raw)
To: David Aguilar; +Cc: John Keeping, git
In-Reply-To: <CAJDDKr4CFyQrAec3jCxyDCx0+4BMXmQAciG1YcnMYS=PAeW-Mw@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> On Tue, Jan 29, 2013 at 11:22 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
>>> Update variable assignments to always use $(command "$arg")
>>> in their RHS instead of "$(command "$arg")" as the latter
>>> is harder to read. Make get_merge_tool_cmd() simpler by
>>> avoiding "echo" and $(command) substitutions completely.
>>>
>>> Signed-off-by: David Aguilar <davvid@gmail.com>
>>> ---
>>> @@ -300,9 +292,9 @@ get_merge_tool_path () {
>>> fi
>>> if test -z "$merge_tool_path"
>>> then
>>> - merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
>>> + merge_tool_path=$(translate_merge_tool_path "$merge_tool")
>>> fi
>>> - if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
>>> + if test -z $(get_merge_tool_cmd "$merge_tool") &&
>>
>> This change should be reverted to avoid calling "test -z" without any
>> other arguments, as Johannes pointed out in v1.
>>
>> The rest of this patch looks good to me.
>
> You're right. My eyes have probably been staring at it too long and I
> missed this (even though I thought I had checked).
By now you (and people who were following this thread) are beginning
to see why I said "I'd feel safer with extra dq" ;-)
I'll amend locally and push the result out.
^ 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