* Re: [PATCH 0/2] optimizing pack access on "read only" fetch repos
From: Junio C Hamano @ 2013-01-29 15:58 UTC (permalink / raw)
To: Jeff King; +Cc: Shawn O. Pearce, git
In-Reply-To: <20130129082939.GB6396@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I also wonder if we would be helped by another "repack" mode that
>> coalesces small packs into a single one with minimum overhead, and
>> run that often from "gc --auto", so that we do not end up having to
>> have 50 packfiles.
>> ...
>
> I'm not sure. If I understand you correctly, it would basically just be
> concatenating packs without trying to do delta compression between the
> objects which are ending up in the same pack. So it would save us from
> having to do (up to) 50 binary searches to find an object in a pack, but
> would not actually save us much space.
The point is not about space. Disk is cheap, and it is not making
it any worse than what happens to your target audience, that is a
fetch-only repository with only "gc --auto" in it, where nobody
passes "-f" to "repack" to cause recomputation of delta.
What I was trying to seek was a way to reduce the runtime penalty we
pay every time we run git in such a repository.
- Object look-up cost will become log2(50*n) from 50*log2(n), which
is about 50/log2(50) improvement;
- System resource cost we incur by having to keep 50 file
descriptors open and maintaining 50 mmap windows will reduce by
50 fold.
- Anything else I missed?
> I would be interested to see the timing on how quick it is compared to a
> real repack,...
Yes, that is what I meant by "wonder if we would be helped by" ;-)
> But how do these somewhat mediocre concatenated packs get turned into
> real packs?
How do they get processed in a fetch-only repositories that
sometimes run "gc --auto" today? By runninng "repack -a -d -f"
occasionally, perhaps?
At some point, you would need to run a repack that involves a real
object-graph traversal that feeds you the paths for objects to
obtain a reasonably compressed pack. We can inspect existing packs
and guess a rough traversal order for commits, but for trees and
blobs, you cannot unify existing delta chains from multiple packs
effectively with data in the pack alone.
^ permalink raw reply
* Re: [PATCH v2 0/4] Auto-generate mergetool lists
From: Junio C Hamano @ 2013-01-29 16:15 UTC (permalink / raw)
To: John Keeping; +Cc: Joachim Schmitz, git
In-Reply-To: <20130129120923.GE7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Tue, Jan 29, 2013 at 12:56:58PM +0100, Joachim Schmitz wrote:
>> John Keeping wrote:
>> > Currently I'm extracting the command word using:
>> >
>> > cmd=$(eval -- "set -- $(git config mergetool.$tool.cmd); echo
>> > \"$1\"")
>>
>> Shouldnt this work?
>> cmd=$((git config "mergetool.$tool.cmd" || git config "difftool.$tool.cmd")
>> | awk '{print $1}')
>
> That doesn't handle paths with spaces in, whereas the eval in a subshell
> does:
>
> $ cmd='"my command" $BASE $LOCAL $REMOTE'
> $ echo "$cmd" | awk '{print $1}'
> "my
> $ ( eval -- "set -- $cmd; echo \"\$1\"" )
> my command
I'd rather not to see you do any of the above.
With any backend that is non-trivial, it would not be unusual for
the *tool.cmd to look like:
[mergetool]
mytool = sh -c '
... some massaging to prepare the command line
... to run the real tool backend comes here, and
... then ...
my_real_tool $arg1 $arg2 ...
'
and you will end up detecting the presence of the shell, which is
not very useful.
I think it is perfectly fine to say "you configured it, so it must
exist; it may fail when we try to run it but it is your problem".
It is simpler to explain and requires one less eval.
^ permalink raw reply
* Re: [PATCH] status: show branch name if possible in in-progress info
From: Junio C Hamano @ 2013-01-29 16:18 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Duy Nguyen, git
In-Reply-To: <vpqd2worv23.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Not HEAD, but .git/rebase-merge/onto, i.e. the target of the rebase.
> Ideally, I would have loved to see "rebasing master on origin/master",
> but I do not think the target ref name is stored during rebase.
Perhaps do it with --format=%s then. It often is useless to know
only the commit object name (or the branch name for that matter, as
you wouldn't know what exact commit the branch tip happens to have)
you are rebasing onto.
^ permalink raw reply
* Re: [RFC] The design of new pathspec features
From: Junio C Hamano @ 2013-01-29 16:20 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8A5VJ4EchbeFEC5ikLYgHze47_xyLTtgH=kqVpP15Hogw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Jan 29, 2013 at 11:35 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> :(glob) magic
>> =============
>>
>> This magic is for people who want globbing. However, it does _not_ use
>> the same matching mechanism the non-magic pathspec does today. It uses
>> wildmatch(WM_PATHNAME), which basically means '*' does not match
>> slashes and "**" does.
>>
>> Global option --glob-pathspecs is added to add :(glob) to all
>> pathspec. :(literal) magic overrides this global option.
>
> I forgot one thing. The current pathspec behavior, the pattern "[a-z]"
> would match a file named "[a-z]" (iow, wildcards are also considered
> literal characters).
That sounds like a blatant bug to me (unless you are talking about
"literal" case). We should fix it before we include it in any
released version, I think.
^ permalink raw reply
* Re: [RFC] The design of new pathspec features
From: Junio C Hamano @ 2013-01-29 16:27 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <7vk3qwrmea.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Jan 29, 2013 at 11:35 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> :(glob) magic
>>> =============
>>>
>>> This magic is for people who want globbing. However, it does _not_ use
>>> the same matching mechanism the non-magic pathspec does today. It uses
>>> wildmatch(WM_PATHNAME), which basically means '*' does not match
>>> slashes and "**" does.
>>>
>>> Global option --glob-pathspecs is added to add :(glob) to all
>>> pathspec. :(literal) magic overrides this global option.
>>
>> I forgot one thing. The current pathspec behavior, the pattern "[a-z]"
>> would match a file named "[a-z]" (iow, wildcards are also considered
>> literal characters).
>
> That sounds like a blatant bug to me (unless you are talking about
> "literal" case). We should fix it before we include it in any
> released version, I think.
Ah, no, I misread what you meant.
Yes, historically when matching a path with a pattern "a/b[c-d]/e",
we tried to first literally match the path with it (this allows the
path "a/b[c-d]/e" to match with the pattern), and if it did not
match, used fnmatch(3) to allow "a/bc/e" to also match. This was an
ugly hack to cope with the possiblity that such a funny path is
actually used in projects.
With :(literal) magic and its friends you are working into the
system, the hack should disappear at the same time when you stop
running fnmatch(3) without FNM_PATHNAME. That should happen no
later than Git 2.0.
But until then, we should keep matching both paths "a/bc/e" and
"a/b[c-d]/e" with pattern "a/b[c-d]/e", for backward compatibility.
Sorry about the earlier confusion.
^ permalink raw reply
* Re: [PATCH] README: fix broken mailing list archive link
From: Junio C Hamano @ 2013-01-29 16:39 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1359456039-20562-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> marc.theaimsgroup.com does not exist anymore, so replace it
> with a link to the archive on GMane.
I think it has been at http://marc.info/?l=git for some time.
^ permalink raw reply
* Re: [PATCH v2 0/4] Auto-generate mergetool lists
From: John Keeping @ 2013-01-29 16:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Joachim Schmitz, git
In-Reply-To: <7vsj5krmng.fsf@alter.siamese.dyndns.org>
On Tue, Jan 29, 2013 at 08:15:15AM -0800, Junio C Hamano wrote:
> With any backend that is non-trivial, it would not be unusual for
> the *tool.cmd to look like:
>
> [mergetool]
> mytool = sh -c '
> ... some massaging to prepare the command line
> ... to run the real tool backend comes here, and
> ... then ...
> my_real_tool $arg1 $arg2 ...
> '
>
> and you will end up detecting the presence of the shell, which is
> not very useful.
>
> I think it is perfectly fine to say "you configured it, so it must
> exist; it may fail when we try to run it but it is your problem".
> It is simpler to explain and requires one less eval.
I think you're right. The even worse case from this point of view is if
you configure it as:
[mergetool]
mytool = 'f() {
... code to actually run the tool here ...
}; f $BASE $REMOTE $LOCAL $MERGED'
which results in a false "unavailable" rather than just a useless check.
John
^ permalink raw reply
* Re: [PATCH 3/6] introduce pack metadata cache files
From: Junio C Hamano @ 2013-01-29 17:35 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <20130129091555.GC9999@sigill.intra.peff.net>
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.
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?
> +void metapack_writer_init(struct metapack_writer *mw,
> + const char *pack_idx,
> + const char *name,
> + int version)
> +{
> + struct strbuf path = STRBUF_INIT;
> +
> + memset(mw, 0, sizeof(*mw));
> +
> + mw->pack = add_packed_git(pack_idx, strlen(pack_idx), 1);
> + if (!mw->pack || open_pack_index(mw->pack))
> + die("unable to open packfile '%s'", pack_idx);
> +
> + 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", so this sounds like a glorified way to say
strbuf_splice(&path, path->len - strlen("idx"), strlen("idx"),
name, strlen(name));
to me.
> + mw->path = strbuf_detach(&path, NULL);
> +
> + mw->out = create_meta_tmp();
> + write_meta_header(mw, name, version);
> +}
> +
> +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?
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Junio C Hamano @ 2013-01-29 17:38 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <20130129091610.GD9999@sigill.intra.peff.net>
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.
> + 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);
> + if (pos < 0)
> + continue;
> +
> + /* timestamp(4) + tree(20) + parents(40) */
> + data = p->data + 64 * pos;
> + *timestamp = *(uint32_t *)data;
> + *timestamp = ntohl(*timestamp);
> + data += 4;
> + *tree = data;
> + data += 20;
> + *parent1 = data;
> + data += 20;
> + *parent2 = data;
> +
> + return 0;
> + }
> +
> + return -1;
> +}
I am torn on this one.
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.
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.
> +static void get_commits(struct metapack_writer *mw,
> + const unsigned char *sha1,
> + void *data)
> +{
> + struct commit_list ***tail = data;
> + enum object_type type = sha1_object_info(sha1, NULL);
> + struct commit *c;
> +
> + if (type != OBJ_COMMIT)
> + return;
> +
> + c = lookup_commit(sha1);
> + if (!c || parse_commit(c))
> + die("unable to read commit %s", sha1_to_hex(sha1));
> +
> + /*
> + * Our fixed-size parent list cannot represent root commits, nor
> + * octopus merges. Just skip those commits, as we can fallback
> + * in those rare cases to reading the actual commit object.
> + */
> + if (!c->parents ||
> + (c->parents && c->parents->next && c->parents->next->next))
> + return;
> +
> + *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?
- 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?
> +void commit_metapack_write(const char *idx)
> +{
> + struct metapack_writer mw;
> + struct commit_list *commits = NULL, *p;
> + struct commit_list **tail = &commits;
> + uint32_t nr = 0;
> +
> + metapack_writer_init(&mw, idx, "commits", 1);
> +
> + /* Figure out how many eligible commits we've got in this pack. */
> + metapack_writer_foreach(&mw, get_commits, &tail);
> + for (p = commits; p; p = p->next)
> + nr++;
^ permalink raw reply
* Re: [PATCH] README: fix broken mailing list archive link
From: Ramkumar Ramachandra @ 2013-01-29 17:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7v622grlj6.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> marc.theaimsgroup.com does not exist anymore, so replace it
>> with a link to the archive on GMane.
>
> I think it has been at http://marc.info/?l=git for some time.
Isn't GMane what all of us refer to on the list though?
^ permalink raw reply
* Re: [PATCH 4/6] introduce a commit metapack
From: Junio C Hamano @ 2013-01-29 18:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen, Shawn O. Pearce
In-Reply-To: <7vy5fbq48t.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I am torn on this one.
>
> 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.
>
> 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
at that gap, immediately before the trailer of the .idx file written
by git_SHA1_Final(), in a way similar to the index extension is done
may give us a natural way to deal with this. The reader can first
read the offset recorded at the tail, checking if the offset is
plausible because it may not exist, the check the extension section
checksum, to see if the file has extension, or the file just ends
with the large offset table and the 28 bytes it tried to were from
the entries near the end of the large offset table.
If that is not worth attempting, perhaps we may still want to store
this as an optional extended section and bump the .idx format
version.
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.
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.
^ permalink raw reply
* Re: [PATCH] README: fix broken mailing list archive link
From: Junio C Hamano @ 2013-01-29 18:10 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <CALkWK0mm8UKBHE=tbXsiS4Z_qBGcRUUsmCKHHFequRiQRL3hsw@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>>> marc.theaimsgroup.com does not exist anymore, so replace it
>>> with a link to the archive on GMane.
>>
>> I think it has been at http://marc.info/?l=git for some time.
>
> Isn't GMane what all of us refer to on the list though?
Then your original log message is wrong, no? You earlier said that
we should replace it because marc no longer exists. Now you are
saying we replace it because we prefer gmane.
I do not mind listing both, but I think we should limit the
enumeration to the minimum and let "and other archival sites" cover
the rest.
^ permalink raw reply
* Re: [PATCH] The images from picon and gravatar are always used over http://, and browsers give mixed contents warning when gitweb is served over https://.
From: Junio C Hamano @ 2013-01-29 18:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Bryan Turner, git, Andrej E Baranov
In-Reply-To: <20130129041206.GA15442@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Odd. "https://www.gravatar.com/" also seems to work. I've put in a
> technical support query to find out what the Gravatar admins prefer.
Thanks; will hold onto Andrej's patch until we hear what the story
is.
Of course we could do something like this (untested).
gitweb/gitweb.perl | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c6bafe6..b59773b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -313,6 +313,14 @@ sub evaluate_uri {
'override' => 0,
'default' => [0]},
+ # Use https:// URL for embedded picons/gravatar images, to be used
+ # on installations that server gitweb over https://
+ 'subcontentssl' => {
+ 'sub' => sub { feature_bool('subcontentssl', @_) },
+ 'override' => 0,
+ 'default' => [0]},
+ }
+
# Enable the 'snapshot' link, providing a compressed archive of any
# tree. This can potentially generate high traffic if you have large
# project.
@@ -1111,6 +1119,7 @@ sub evaluate_git_dir {
}
our (@snapshot_fmts, $git_avatar);
+our ($gravatar_base_url, $picon_base_url);
sub configure_gitweb_features {
# list of supported snapshot formats
our @snapshot_fmts = gitweb_get_feature('snapshot');
@@ -1121,10 +1130,17 @@ sub configure_gitweb_features {
# if the provider name is invalid or the dependencies are not met,
# reset $git_avatar to the empty string.
our ($git_avatar) = gitweb_get_feature('avatar');
+ my $use_https = gitweb_check_feature('subcontentssl');
+
if ($git_avatar eq 'gravatar') {
$git_avatar = '' unless (eval { require Digest::MD5; 1; });
+ $gravatar_base_url = $use_https ?
+ "https://secure.gravatar.com/avatar/" :
+ "http://www.gravatar.com/avatar/";
} elsif ($git_avatar eq 'picon') {
- # no dependencies
+ $picon_base_url = $use_https ?
+ "http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/" :
+ "https://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/";
} else {
$git_avatar = '';
}
@@ -2068,7 +2084,7 @@ sub picon_url {
if (!$avatar_cache{$email}) {
my ($user, $domain) = split('@', $email);
$avatar_cache{$email} =
- "http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/" .
+ $picon_base_url .
"$domain/$user/" .
"users+domains+unknown/up/single";
}
@@ -2082,9 +2098,7 @@ sub picon_url {
sub gravatar_url {
my $email = lc shift;
my $size = shift;
- $avatar_cache{$email} ||=
- "http://www.gravatar.com/avatar/" .
- Digest::MD5::md5_hex($email) . "?s=";
+ $avatar_cache{$email} ||= $gravatar_base_url . Digest::MD5::md5_hex($email) . "?s=";
return $avatar_cache{$email} . $size;
}
^ permalink raw reply related
* Re: [PATCH v2] status: show the branch name if possible in in-progress info
From: Jonathan Nieder @ 2013-01-29 18:44 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Matthieu Moy
In-Reply-To: <1359471493-32531-1-git-send-email-pclouds@gmail.com>
Hi Duy,
Nguyễn Thái Ngọc Duy wrote:
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -76,7 +76,7 @@ test_expect_success 'status when rebase in progress before resolving conflicts'
> test_must_fail git rebase HEAD^ --onto HEAD^^ &&
> cat >expected <<-\EOF &&
> # Not currently on any branch.
> - # 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'\''.
?
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?
Thanks.
Jonathan
^ permalink raw reply
* Re: [PATCH] README: fix broken mailing list archive link
From: Junio C Hamano @ 2013-01-29 19:06 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1359456039-20562-1-git-send-email-artagnon@gmail.com>
How about doing this instead, to update all outdated or incorrect
information in that file? In addition to the marc.info update,
- refer to gmane;
- git-scm.org gives a CNAME with less commercial feeling to the
same thing;
- "A note from the maintainer" is not usually followed by useful
discussion to discuss status, direction nor tasks;
- There is no separate "What's in git (stable/topics)".
README | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/README b/README
index 49713ea..5b83a9a 100644
--- a/README
+++ b/README
@@ -38,7 +38,7 @@ CVS users may also want to read Documentation/gitcvs-migration.txt
("man gitcvs-migration" or "git help cvs-migration" if git is
installed).
-Many Git online resources are accessible from http://git-scm.com/
+Many Git online resources are accessible from http://git-scm.org/
including full documentation and Git related tools.
The user discussion and development of Git take place on the Git
@@ -47,11 +47,10 @@ requests, comments and patches to git@vger.kernel.org (read
Documentation/SubmittingPatches for instructions on patch submission).
To subscribe to the list, send an email with just "subscribe git" in
the body to majordomo@vger.kernel.org. The mailing list archives are
-available at http://marc.theaimsgroup.com/?l=git and other archival
-sites.
-
-The messages titled "A note from the maintainer", "What's in
-git.git (stable)" and "What's cooking in git.git (topics)" and
-the discussion following them on the mailing list give a good
-reference for project status, development direction and
-remaining tasks.
+available at http://news.gmane.org/gmane.comp.version-control.git/,
+http://marc.info/?l=git and other archival sites.
+
+The maintainer frequently sends the "What's cooking" reports that
+list the current status of various development topics to the mailing
+list. The discussion following them give a good reference for
+project status, development direction and remaining tasks.
^ permalink raw reply related
* Re: [PATCH] branch: show (rebasing) or (bisecting) instead of (no branch) when possible
From: Jonathan Nieder @ 2013-01-29 19:13 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1359461574-24529-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> In the spirit of status' in-progress info. I think showing this is
> more useful than "(no branch)". I tend to do "git br" more often than
> "git st" and this catches my eyes.
Very nice idea. This would also have been a nice way to avoid
confusion when my officemate used bisect for the first time.
Any particular reason the above explanation is after the triple-dash
instead of before it?
[...]
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -557,7 +557,15 @@ static void show_detached(struct ref_list *ref_list)
>
> if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
> struct ref_item item;
> - item.name = xstrdup(_("(no branch)"));
> + struct stat st;
> + if ((!stat(git_path("rebase-apply"), &st) &&
> + stat(git_path("rebase-apply/applying"), &st)) ||
> + !stat(git_path("rebase-merge"), &st))
Here's a straight translation of contrib/completion/prompt.sh for
comparison, skipping the cases that don't involve automatically
detaching HEAD:
if (!stat(git_path("rebase-merge"), &st) && S_ISDIR(st.st_mode))
item.name = xstrdup(_("(rebasing)"));
else if (!access(git_path("rebase-apply/rebasing"), F_OK))
item.name = xstrdup(_("(rebasing)"));
else if (!access(git_path("BISECT_LOG"), F_OK))
item.name = xstrdup(_("(bisecting)"));
else
item.name = xstrdup(_("(no branch)"));
That would mean:
* using access() instead of stat() to avoid unnecessary work
* relying on rebase--am to write .git/rebase-apply/rebasing when
appropriate instead of guessing
Not important, though. :)
Jonathan
^ permalink raw reply
* [PATCH] git-send-email: add ~/.authinfo parsing
From: Michal Nazarewicz @ 2013-01-29 19:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Krzysztof Mazur, Michal Nazarewicz
From: Michal Nazarewicz <mina86@mina86.com>
Make git-send-email read password from a ~/.authinfo 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.
Not to introduce any new dependencies, ~/.authinfo file is parsed only
if Text::CSV Perl module is installed. If it's not, a notification is
printed and the file is ignored.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
Documentation/git-send-email.txt | 15 +++++++--
git-send-email.perl | 69 +++++++++++++++++++++++++++++++++-------
2 files changed, 70 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index eeb561c..b83576e 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -158,14 +158,25 @@ 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
'--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.
++
+The '~/.authinfo' file is read if Text::CSV Perl module is installed
+on the system; if it's missing, a notification message will be printed
+and the file ignored altogether. The file should contain a line with
+the following format:
++
+ machine <domain> port <port> login <user> password <pass>
++
+Contrary to other tools, 'git-send-email' does not support symbolic
+port names like 'imap' thus `<port>` must be a number.
--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..d824098 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1045,6 +1045,62 @@ 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 read_password_from_authinfo {
+ my $fd;
+ if (!open $fd, '<', $ENV{'HOME'} . '/.authinfo') {
+ return;
+ }
+
+ if (!eval { require Text::CSV; 1 }) {
+ print STDERR "Text::CSV missing, won't read ~/.authinfo\n";
+ close $fd;
+ return;
+ }
+
+ my $csv = Text::CSV->new( { sep_char => ' ' } );
+ my $password;
+ while (my $line = <$fd>) {
+ chomp $line;
+ $csv->parse($line);
+ my %row = $csv->fields();
+ if (defined $row{'machine'} &&
+ defined $row{'login'} &&
+ defined $row{'port'} &&
+ defined $row{'password'} &&
+ $row{'machine'} eq $smtp_server &&
+ $row{'login'} eq $smtp_authuser &&
+ $row{'port'} eq $smtp_server_port) {
+ $password = $row{'password'};
+ last;
+ }
+ }
+
+ close $fd;
+ return $password;
+}
+
+sub read_password {
+ return read_password_from_authinfo || 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 +1250,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
* [RFC/PATCH v2] CodingGuidelines: add Python coding guidelines
From: John Keeping @ 2013-01-29 19:08 UTC (permalink / raw)
To: git
These are kept short by simply deferring to PEP-8. Most of the Python
code in Git is already very close to this style (some things in contrib/
are not).
Rationale for version suggestions:
- Amongst the noise in [1], there isn't any disagreement about using
2.6 as a base (see also [2]), although Brandon Casey recently added
support for 2.4 and 2.5 to git-p4 [3].
- Restricting ourselves to 2.6+ makes aiming for Python 3 compatibility
significantly easier [4].
- Advocating Python 3 support in all scripts is currently unrealistic
because:
- 'p4 -G' provides output in a format that is very hard to use with
Python 3 (and its documentation claims Python 3 is unsupported).
- Mercurial does not support Python 3.
- Bazaar does not support Python 3.
- But we should try to make new scripts compatible with Python 3
because all new Python development is happening on version 3 and the
Python community will eventually stop supporting Python 2 [5].
- Python 3.1 is required to support the 'surrogateescape' error handler
for encoding/decodng filenames to/from Unicode strings and Python 3.0
is not longer supported.
[1] http://thread.gmane.org/gmane.comp.version-control.git/210329
[2] http://article.gmane.org/gmane.comp.version-control.git/210429
[3] http://thread.gmane.org/gmane.comp.version-control.git/214579
[4] http://docs.python.org/3.3/howto/pyporting.html#try-to-support-python-2-6-and-newer-only
[5] http://www.python.org/dev/peps/pep-0404/
---
Changes since v1:
- Set 3.1 as the minimum Python 3 version
- Remove the section on Unicode literals - it just adds confusion and
doesn't apply to the current code; we can deal with any issues if they
ever arise.
Documentation/CodingGuidelines | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 69f7e9b..db7a416 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -179,6 +179,19 @@ For C programs:
- Use Git's gettext wrappers to make the user interface
translatable. See "Marking strings for translation" in po/README.
+For Python scripts:
+
+ - We follow PEP-8 (http://www.python.org/dev/peps/pep-0008/).
+
+ - As a minimum, we aim to be compatible with Python 2.6 and 2.7.
+
+ - Where required libraries do not restrict us to Python 2, we try to
+ also be compatible with Python 3.1 and later.
+
+ - We use the 'b' prefix for bytes literals. Note that even though
+ the Python documentation for version 2.6 does not mention this
+ prefix it is supported since version 2.6.0.
+
Writing Documentation:
Every user-visible change should be reflected in the documentation.
--
1.8.1.1.644.g4977e08
^ permalink raw reply related
* Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions
From: John Keeping @ 2013-01-29 19:22 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <1359334346-5879-2-git-send-email-davvid@gmail.com>
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.
> ! type "$merge_tool_path" >/dev/null 2>&1
> then
> echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\
^ permalink raw reply
* Re: [PATCH] README: fix broken mailing list archive link
From: Ramkumar Ramachandra @ 2013-01-29 19:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7v4nhzq066.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> - refer to gmane;
GMane has a wonderful interface, and deserves to be advertised.
> - git-scm.org gives a CNAME with less commercial feeling to the
> same thing;
Nice touch.
> - "A note from the maintainer" is not usually followed by useful
> discussion to discuss status, direction nor tasks;
True.
> - There is no separate "What's in git (stable/topics)".
Right.
Looks good to me. Thanks.
^ permalink raw reply
* Re: [RFC/PATCH v2] CodingGuidelines: add Python coding guidelines
From: Junio C Hamano @ 2013-01-29 19:34 UTC (permalink / raw)
To: John Keeping; +Cc: git
In-Reply-To: <20130129190844.GB1342@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> Changes since v1:
>
> - Set 3.1 as the minimum Python 3 version
>
> - Remove the section on Unicode literals - it just adds confusion and
> doesn't apply to the current code; we can deal with any issues if they
> ever arise.
> ...
> + - We use the 'b' prefix for bytes literals. Note that even though
> + the Python documentation for version 2.6 does not mention this
> + prefix it is supported since version 2.6.0.
Do we still need to single out the 'b' prefix? Even if it were
necessary, I'd like to see it toned down a bit to make it clear that
most of the time you can write strings as strings without having to
worry about the "is it unicode string or a string string" mess.
Like
- When you must make distinction between Unicode literals and
byte string literals, it is OK to use 'b' prefix. Even though
...
perhaps?
^ permalink raw reply
* gitk doesn't always shows boths tags in "gitk tag1..tag2"
From: Toralf Förster @ 2013-01-29 19:40 UTC (permalink / raw)
To: git
For a cloned BOINC git tree :
$> git clone git://boinc.berkeley.edu/boinc.git
the following 2 commands shows both starting and ending revisions :
$> gitk client_release_7.0.41..client_release_7.0.42
$> gitk client_release_7.0.43..client_release_7.0.44
however this command doesn't show the tag "client_release_7.0.44" :
$> gitk client_release_7.0.44..client_release_7.0.45
Now I'm wondering whether this is a side effect of the developer model
of BOINC or an issue in gitk ?
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
^ permalink raw reply
* Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
From: John Keeping @ 2013-01-29 19:48 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <1359334346-5879-4-git-send-email-davvid@gmail.com>
On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -2,6 +2,35 @@
> # git-mergetool--lib is a library for common merge tool functions
> MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
>
> +mode_ok () {
> + diff_mode && can_diff ||
> + merge_mode && can_merge
> +}
> +
> +is_available () {
> + merge_tool_path=$(translate_merge_tool_path "$1") &&
> + type "$merge_tool_path" >/dev/null 2>&1
> +}
> +
> +show_tool_names () {
> + condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
> +
> + ( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
Is the '*' necessary here? I would expect ls to list the current
directory if given no arguments, but perhaps some platforms behave
differently?
> + while read toolname
> + do
> + if setup_tool "$toolname" 2>/dev/null &&
> + (eval "$condition" "$toolname")
> + then
> + if test -n "$preamble"
> + then
> + echo "$preamble"
> + preamble=
> + fi
> + printf "%s%s\n" "$per_line_prefix" "$tool"
> + fi
> + done
> +}
> +
> diff_mode() {
> test "$TOOL_MODE" = diff
> }
> @@ -199,35 +228,21 @@ list_merge_tool_candidates () {
> }
>
> show_tool_help () {
> - unavailable= available= LF='
> -'
> - for i in "$MERGE_TOOLS_DIR"/*
> - do
> - tool=$(basename "$i")
> - setup_tool "$tool" 2>/dev/null || continue
> -
> - merge_tool_path=$(translate_merge_tool_path "$tool")
> - if type "$merge_tool_path" >/dev/null 2>&1
> - then
> - available="$available$tool$LF"
> - else
> - unavailable="$unavailable$tool$LF"
> - fi
> - done
> -
> - cmd_name=${TOOL_MODE}tool
> + 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"
> then
> - echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
> - echo "$available" | sort | sed -e 's/^/ /'
> + echo "$available"
> else
> echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
> fi
> if test -n "$unavailable"
> then
> echo
> - echo 'The following tools are valid, but not currently available:'
> - echo "$unavailable" | sort | sed -e 's/^/ /'
> + echo "$unavailable"
> fi
> if test -n "$unavailable$available"
> then
You haven't taken full advantage of the simplification Junio suggested
in response to v1 here. We can change the "unavailable" block to be:
show_tool_names 'mode_ok && ! is_available' "$TAB$TAB" \
"${LF}The following tools are valid, but not currently available:"
If you also add a "not_found_msg" parameter to show_tool_names then the
"available" case is also simplified:
show_tool_names 'mode_ok && is_available' "$TAB$TAB" \
"$tool_opt may be set to one of the following:" \
"No suitable tool for 'git $cmd_name --tool=<tool>' found."
with this at the end of show_tool_names:
test -n "$preamble" && test -n "$not_found_msg" && \
echo "$not_found_msg"
John
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Junio C Hamano @ 2013-01-29 19:53 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: git, Krzysztof Mazur, Michal Nazarewicz
In-Reply-To: <2f93ce7b6b5d3f6c6d1b99958330601a5560d4ba.1359486391.git.mina86@mina86.com>
Michal Nazarewicz <mpn@google.com> writes:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> Make git-send-email read password from a ~/.authinfo file instead of
> requiring it to be stored in git configuration, passed as command line
> argument or typed in.
Makes one wonder why .authinfo and not .netrc;
http://www.gnu.org/software/emacs/manual/html_node/auth/Help-for-users.html
phrases it amusingly:
“Netrc” files are usually called .authinfo or .netr
nowadays .authinfo seems to be more popular and the
auth-source library encourages this confusion by accepting
both
Either way it still encourages a plaintext password to be on disk,
which may not be what we want, even though it may be slight if not
really much of an improvement. Again the Help-for-users has this
amusing bit:
You could just say (but we don't recommend it, we're just
showing that it's possible)
password mypassword
to use the same password everywhere. Again, DO NOT DO THIS
or you will be pwned as the kids say.
> +The '~/.authinfo' file is read if Text::CSV Perl module is installed
> +on the system; if it's missing, a notification message will be printed
> +and the file ignored altogether. The file should contain a line with
> +the following format:
> ++
> + machine <domain> port <port> login <user> password <pass>
It is rather strange to require a comma-separated-values parser to
read a file format this simple, isn't it?
> ++
> +Contrary to other tools, 'git-send-email' does not support symbolic
> +port names like 'imap' thus `<port>` must be a number.
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?
^ permalink raw reply
* Re: [RFC/PATCH v2] CodingGuidelines: add Python coding guidelines
From: John Keeping @ 2013-01-29 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzjzrokag.fsf@alter.siamese.dyndns.org>
On Tue, Jan 29, 2013 at 11:34:31AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > Changes since v1:
> >
> > - Set 3.1 as the minimum Python 3 version
> >
> > - Remove the section on Unicode literals - it just adds confusion and
> > doesn't apply to the current code; we can deal with any issues if they
> > ever arise.
> > ...
> > + - We use the 'b' prefix for bytes literals. Note that even though
> > + the Python documentation for version 2.6 does not mention this
> > + prefix it is supported since version 2.6.0.
>
> Do we still need to single out the 'b' prefix? Even if it were
> necessary, I'd like to see it toned down a bit to make it clear that
> most of the time you can write strings as strings without having to
> worry about the "is it unicode string or a string string" mess.
> Like
>
> - When you must make distinction between Unicode literals and
> byte string literals, it is OK to use 'b' prefix. Even though
> ...
>
> perhaps?
Yeah, that's better.
I want to include it because if you look in the Python documentation
then you'll be misled into thinking that it's not available on 2.6 and
we will be supporting that for a while since it is the version included
in RHEL 6.
John
^ 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