* Re: [PATCH] pack-objects: protect against disappearing packs
From: Johannes Sixt @ 2011-10-14 14:35 UTC (permalink / raw)
To: Jeff King; +Cc: git, git-dev, Shawn O. Pearce, Nicolas Pitre
In-Reply-To: <20111014130703.GB7808@sigill.intra.peff.net>
Am 10/14/2011 15:07, schrieb Jeff King:
> Within a single process, I don't think so. This change impacts only
> pack-objects, which always runs as a separate process, and never deletes
> packs itself. The most likely problematic code path would be "git
> repack -d", but it waits for pack-objects to complete successfully
> before removing any packs.
Thanks. The test suite didn't find any issues on Windows.
-- Hannes
^ permalink raw reply
* Re: [PATCH] fix alias expansion with new Git::config_path()
From: Cord Seele @ 2011-10-14 14:42 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Git Mailing List, Junio C Hamano, Jakub Narebski
In-Reply-To: <4E984781.6050601@drmicha.warpmail.net>
On Fri 14 Oct 2011 16:30:25 +0200, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> Tested-by: Michael J Gruber <git@drmicha.warpmail.net>
Great.
> Thanks. (Though I'm still wondering what this is about overall.)
to make '~/' work in sendemail.aliasesfile
-- Cord
^ permalink raw reply
* Re: [PATCH] fix alias expansion with new Git::config_path()
From: Junio C Hamano @ 2011-10-14 15:05 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Git Mailing List, Jakub Narebski
In-Reply-To: <20111014144203.GC13680@laptop>
Cord Seele <cowose@googlemail.com> writes:
> On Fri 14 Oct 2011 16:30:25 +0200, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>
>> Tested-by: Michael J Gruber <git@drmicha.warpmail.net>
>
> Great.
>
>> Thanks. (Though I'm still wondering what this is about overall.)
>
> to make '~/' work in sendemail.aliasesfile
That is not an explanation.
^ permalink raw reply
* Re: Bug? url.insteadOf overwrites remote.pushUrl
From: Michael J Gruber @ 2011-10-14 15:35 UTC (permalink / raw)
To: Kirill Likhodedov; +Cc: git
In-Reply-To: <CAB6D58F-A3C9-4532-A9CC-10E43CD34E4E@jetbrains.com>
Kirill Likhodedov venit, vidit, dixit 14.10.2011 15:55:
>
> I've found that defining url.<base>.insteadOf overrides explicit
> remote.<name>.pushUrl.
It doesn't really override it. It is applied to it, i.e. transforms it.
> On the other hand, pushInsteadOf doesn't
> override explicit pushUrl. Is it a bug?
That is as described in the thread (thanks for linking to it).
> # cat .git/config [remote "origin"] fetch =
> +refs/heads/*:refs/remotes/origin/* url = github.com/klikh/Test.git
> pushUrl = jetbrains.com/klikh/Test.git [url "http://"] insteadOf=jet
>
> # git remote -v origin github.com/klikh/Test.git (fetch) origin
> http://brains.com/klikh/Test.git (push)
The idea of "pushInsteadOf" was that, instead of having to define url
and pushurl separately, you can use different rules, say
github -> git://github.com (fetch)
github -> https://username@github.com (push)
github/ -> git@github.com: (push)
used with "url = github/otheruser/repo.git" alone, without pushurl.
pushurl predates pushinsteadof, and when the latter was introduced, one
could have argued for or against "insteadof" being applied to pushurls.
But that was necessary before, and existing behavior at the time when
pushinsteadof was introduced. So, I don't see a bug, nor anything we
could change now, though arguably most people use either pushinstead of
or pushurl, but not both.
Cheers,
Michael
^ permalink raw reply
* [BUG] remote-curl.c: honor pushurl
From: Michael Schubert @ 2011-10-14 15:37 UTC (permalink / raw)
To: git; +Cc: rctay89
When doing a push (fetch, ..) over http(s), git calls git-remote-http to
communicate with the server.
git-remote-http <remote> [<url>]
Git correctly honors a configured pushurl and passes it to git-remote-http,
but git-remote-http is initiating the http connection with the url defined
for remote (remote->url) rather than the passed url. This undermines the
purpose of a config like
url = https://example.com/repo.git
pushurl = https://user@example.com/repo.git
Introduced around 888692b7 - CC'ing Tay Ray Chuan. (I don't know if it was
working before, though.)
^ permalink raw reply
* Re: [BUG] remote-curl.c: honor pushurl
From: Jeff King @ 2011-10-14 15:48 UTC (permalink / raw)
To: Michael Schubert; +Cc: git, rctay89
In-Reply-To: <4E985744.6050904@elegosoft.com>
On Fri, Oct 14, 2011 at 05:37:40PM +0200, Michael Schubert wrote:
> When doing a push (fetch, ..) over http(s), git calls git-remote-http to
> communicate with the server.
>
> git-remote-http <remote> [<url>]
>
> Git correctly honors a configured pushurl and passes it to git-remote-http,
> but git-remote-http is initiating the http connection with the url defined
> for remote (remote->url) rather than the passed url. This undermines the
> purpose of a config like
>
> url = https://example.com/repo.git
> pushurl = https://user@example.com/repo.git
>
> Introduced around 888692b7 - CC'ing Tay Ray Chuan. (I don't know if it was
> working before, though.)
Already noticed and fixed last week.
See this thread, starting at the focused message.
http://thread.gmane.org/gmane.comp.version-control.git/182752/focus=182872
-Peff
^ permalink raw reply
* Re: [PATCH] fix alias expansion with new Git::config_path()
From: Jakub Narebski @ 2011-10-14 16:38 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <4E984781.6050601@drmicha.warpmail.net>
Michael J Gruber wrote:
> Cord Seele venit, vidit, dixit 14.10.2011 16:25:
>> On Fri 14 Oct 2011 14:29:27 +0200, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>>
>>> cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30)
>>> broke the expansion of aliases for me:
[...]
>>> Reverting cec5dae brings my aliases back. [...]
[...]
>>
>> The following patch fixes it for me, please give it a try.
>>
>> Since this fix is simply copy&pasting some code from the config_settings path
>> someone with better perl understanding might wnat to refactor it
>> (Junio/Jacob)?
[missing commit message]
>> Signed-off-by: Cord Seele <cowose@gmail.com>
>
> Tested-by: Michael J Gruber <git@drmicha.warpmail.net>
>
> Thanks. (Though I'm still wondering what this is about overall.)
There were a few issues that were responsible for this error:
1. %config_bool_settings and %config_settings despite similar name have
different semantic.
%config_bool_settings values are arrays where the first element is
(reference to) the variable to set, and second element is default
value... which admittedly is a bit cryptic. More readable if more
verbose option would be to use hash reference, e.g.:
my %config_bool_settings = (
"thread" => { variable => \$thread, default => 1},
[...]
Or something like that.
%config_settings values are either either reference to scalar variable
or reference to array. In second case it means that option (or config
option) is multi-valued. BTW. this is similar to what Getopt::Long does.
2. In cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30)
the setting "aliasesfile" was moved from %config_settings to newly
introduced %config_path_settings. But the loop that parses settings
from %config_path_settings was copy'n'pasted *wrongly* from
%config_bool_settings instead of from %config_settings.
It looks like cec5dae author cargo-culted this change...
3. 994d6c6 (send-email: address expansion for common mailers, 2006-05-14)
didn't add test for alias expansion to t9001-send-email.sh
>> ---
>> git-send-email.perl | 12 ++++++++++--
>> 1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 91607c5..6885dfa 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -337,8 +337,16 @@ sub read_config {
>> }
>>
>> foreach my $setting (keys %config_path_settings) {
>> - my $target = $config_path_settings{$setting}->[0];
>> - $$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
>> + my $target = $config_path_settings{$setting};
>> + if (ref($target) eq "ARRAY") {
>> + unless (@$target) {
>> + my @values = Git::config_path(@repo, "$prefix.$setting");
>> + @$target = @values if (@values && defined $values[0]);
>> + }
>> + }
>> + else {
>> + $$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
>> + }
>> }
>>
>> foreach my $setting (keys %config_settings) {
Or a bit simpler (though still duplicated somewhat code with
%config_settings) case:
diff --git i/git-send-email.perl w/git-send-email.perl
index 91607c5..eed241e 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl
@@ -337,8 +337,13 @@ sub read_config {
}
foreach my $setting (keys %config_path_settings) {
- my $target = $config_path_settings{$setting}->[0];
- $$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+ my $target = $config_path_settings{$setting};
+ if (ref($target) eq "ARRAY" && !@$target) {
+ my @values = Git::config_path(@repo, "$prefix.$setting");
+ @$target = @values if (@values && defined $values[0]);
+ } elsif (!defined $$target) {
+ $$target = Git::config_path(@repo, "$prefix.$setting");
+ }
}
foreach my $setting (keys %config_settings) {
P.S. Junio, does t9001 pass for you? For me it fails very strangely on
some tests:
not ok - 21 reject long lines
not ok - 22 no patch was sent
not ok - 28 In-Reply-To without --chain-reply-to
not ok - 29 In-Reply-To with --chain-reply-to
not ok - 39 sendemail.cccmd
not ok - 49 --suppress-cc=bodycc
not ok - 51 --suppress-cc=cc
not ok - 56 confirm by default (due to cc)
not ok - 70 warning with an implicit --chain-reply-to
# failed 9 among 93 test(s)
--
Jakub Narebski
Poland
^ permalink raw reply related
* [PATCH] t1304: fall back to $USER if $LOGNAME is not defined
From: René Scharfe @ 2011-10-14 17:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Brandon Casey, Matthieu Moy
For some reason $LOGNAME is not set anymore for me after an upgrade from
Ubuntu 11.04 to 11.10. Use $USER in such a case.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
The only other use of $LOGNAME is in git-cvsimport, which does the same.
t/t1304-default-acl.sh | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index b5d89a2..2b962cf 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -25,6 +25,11 @@ else
test_set_prereq SETFACL
fi
+if test -z "$LOGNAME"
+then
+ LOGNAME=$USER
+fi
+
check_perms_and_acl () {
test -r "$1" &&
getfacl "$1" > actual &&
--
1.7.7
^ permalink raw reply related
* Re: url.<base>.insteadOf with empty value
From: Junio C Hamano @ 2011-10-14 17:57 UTC (permalink / raw)
To: Kirill Likhodedov; +Cc: git, Josh Triplett
In-Reply-To: <54556728-92C0-4992-9831-0D582C383235@jetbrains.com>
Kirill Likhodedov <Kirill.Likhodedov@jetbrains.com> writes:
> If I don't specify any value for url.<base>.insteadOf or url.<base>.pushInsteadOf, Git substitutes all urls for remotes defined in .git/config
>
> Probably that's because any url starts with empty string and thus has to be substituted.
> But it might be a bit confusing, because on the other hand if no value is given to the property insteadOf, user may expect this property to be ignored.
>
> Please check if current Git behavior is correct.
>
> If it is not a bug, I'd suggest to add a note to man git-config about this.
Please assume that what the documentation says is clear enough for whoever
wrote it and need no further clarification, so you would need to help them
understand what additional things you may want the documentation to say,
by clarifying "add a note" and "about this" a bit.
The "insteadOf" replacement is meant to apply for any URL we use. I would
be surprised if it did not affect pushURL; it would be a bug if it didn't.
On the other hand, the rewrite done by "pushinsteadof" is meant to apply
only when remote.<any>.url is used for pushing. See t/t5516-fetch-push.sh
part of the patch for 1c2eafb (Add url.<base>.pushInsteadOf: URL rewriting
for push only, 2009-09-07). It would clarify what the intended interaction
among these configuration variables.
Thanks.
^ permalink raw reply
* Re: [PATCH] t7800: avoid arithmetic expansion notation
From: Junio C Hamano @ 2011-10-14 18:00 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Sitaram Chamarty
In-Reply-To: <837ad77348b459aa5f5f79e556dbeeeba41027e7.1318594392.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> ba959de (git-difftool: allow skipping file by typing 'n' at prompt, 2011-10-08)
> introduced shell code like
>
> $((foo; bar) | baz)
>
> which some shells (e.g. bash, dash) interpret as an unfinished arithmetic
> evaluation $(( expr )).
Ahh, thanks, I should have caught this. I recall I rewrote a similar one
to $( (command; command) | command ) more than once before.
^ permalink raw reply
* [PATCH 1/2] pack-objects: protect against disappearing packs
From: Jeff King @ 2011-10-14 18:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20111014012320.GA4395@sigill.intra.peff.net>
It's possible that while pack-objects is running, a
simultaneously running prune process might delete a pack
that we are interested in. Because we load the pack indices
early on, we know that the pack contains our item, but by
the time we try to open and map it, it is gone.
Since c715f78, we already protect against this in the normal
object access code path, but pack-objects accesses the packs
at a lower level. In the normal access path, we call
find_pack_entry, which will call find_pack_entry_one on each
pack index, which does the actual lookup. If it gets a hit,
we will actually open and verify the validity of the
matching packfile (using c715f78's is_pack_valid). If we
can't open it, we'll issue a warning and pretend that we
didn't find it, causing us to go on to the next pack (or on
to loose objects).
Furthermore, we will cache the descriptor to the opened
packfile. Which means that later, when we actually try to
access the object, we are likely to still have that packfile
opened, and won't care if it has been unlinked from the
filesystem.
Notice the "likely" above. If there is another pack access
in the interim, and we run out of descriptors, we could
close the pack. And then a later attempt to access the
closed pack could fail (we'll try to re-open it, of course,
but it may have been deleted). In practice, this doesn't
happen because we tend to look up items and then access them
immediately.
Pack-objects does not follow this code path. Instead, it
accesses the packs at a much lower level, using
find_pack_entry_one directly. This means we skip the
is_pack_valid check, and may end up with the name of a
packfile, but no open descriptor.
We can add the same is_pack_valid check here. Unfortunately,
the access patterns of pack-objects are not quite as nice
for keeping lookup and object access together. We look up
each object as we find out about it, and the only later when
writing the packfile do we necessarily access it. Which
means that the opened packfile may be closed in the interim.
In practice, however, adding this check still has value, for
three reasons.
1. If you have a reasonable number of packs and/or a
reasonable file descriptor limit, you can keep all of
your packs open simultaneously. If this is the case,
then the race is impossible to trigger.
2. Even if you can't keep all packs open at once, you
may end up keeping the deleted one open (i.e., you may
get lucky).
3. The race window is shortened. You may notice early that
the pack is gone, and not try to access it. Triggering
the problem without this check means deleting the pack
any time after we read the list of index files, but
before we access the looked-up objects. Triggering it
with this check means deleting the pack means deleting
the pack after we do a lookup (and successfully access
the packfile), but before we access the object. Which
is a smaller window.
Acked-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Jeff King <peff@peff.net>
---
Re-post with ack from Nicolas and my SOB fixed. No changes from earlier
version in this thread.
builtin/pack-objects.c | 4 ++++
cache.h | 1 +
sha1_file.c | 2 +-
3 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 2b18de5..8681ccd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -804,6 +804,10 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
off_t offset = find_pack_entry_one(sha1, p);
if (offset) {
if (!found_pack) {
+ if (!is_pack_valid(p)) {
+ error("packfile %s cannot be accessed", p->pack_name);
+ continue;
+ }
found_offset = offset;
found_pack = p;
}
diff --git a/cache.h b/cache.h
index e39e160..495b468 100644
--- a/cache.h
+++ b/cache.h
@@ -1055,6 +1055,7 @@ struct extra_have_objects {
extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);
extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);
+extern int is_pack_valid(struct packed_git *);
extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
diff --git a/sha1_file.c b/sha1_file.c
index 3401301..a22c5b4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1987,7 +1987,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
return 0;
}
-static int is_pack_valid(struct packed_git *p)
+int is_pack_valid(struct packed_git *p)
{
/* An already open pack is known to be valid. */
if (p->pack_fd != -1)
--
1.7.6.4.37.g43b58b
^ permalink raw reply related
* [PATCH 2/2] downgrade "packfile cannot be accessed" errors to warnings
From: Jeff King @ 2011-10-14 18:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20111014012320.GA4395@sigill.intra.peff.net>
These can happen if another process simultaneously prunes a
pack. But that is not usually an error condition, because a
properly-running prune should have repacked the object into
a new pack. So we will notice that the pack has disappeared
unexpectedly, print a message, try other packs (possibly
after re-scanning the list of packs), and find it in the new
pack.
Acked-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Jeff King <peff@peff.net>
---
Repost with ack from Nicolas, and more obvious subject line. No changes
from earlier version in this thread.
builtin/pack-objects.c | 2 +-
sha1_file.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8681ccd..ba3705d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -805,7 +805,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
if (offset) {
if (!found_pack) {
if (!is_pack_valid(p)) {
- error("packfile %s cannot be accessed", p->pack_name);
+ warning("packfile %s cannot be accessed", p->pack_name);
continue;
}
found_offset = offset;
diff --git a/sha1_file.c b/sha1_file.c
index a22c5b4..27f3b9b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2038,7 +2038,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
* was loaded!
*/
if (!is_pack_valid(p)) {
- error("packfile %s cannot be accessed", p->pack_name);
+ warning("packfile %s cannot be accessed", p->pack_name);
goto next;
}
e->offset = offset;
--
1.7.6.4.37.g43b58b
^ permalink raw reply related
* Re: [PATCH] fix alias expansion with new Git::config_path()
From: Junio C Hamano @ 2011-10-14 18:13 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Michael J Gruber, Git Mailing List
In-Reply-To: <201110141838.19118.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> P.S. Junio, does t9001 pass for you?
It seems to.
Thanks for a detailed write-up. I'd appreciate a final fix in an
apply-able patch form.
^ permalink raw reply
* Re: [PATCH 2/2] bundle: add parse_bundle_header() helper function
From: Junio C Hamano @ 2011-10-14 18:14 UTC (permalink / raw)
To: Phil Hord; +Cc: Junio C Hamano, git, Shawn O. Pearce, Phil Hord
In-Reply-To: <CABURp0otqxeKjLe-Rk3htZRa0M7+rerfrbVrXx-7Dr1tK3tTTg@mail.gmail.com>
Phil Hord <phil.hord@gmail.com> writes:
> On Thu, Oct 13, 2011 at 6:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Move most of the code from read_bundle_header() to parse_bundle_header()
>> that takes a file descriptor that is already opened for reading, and make
>> the former responsible only for opening the file and noticing errors.
> ...
>
>> * It generally is a bad practice to base a non-RFC patch on an RFC one,
>> but in any case here is how I would do the is_bundle() helper.
>
> I didn't label it RFC,...
I was referring to the fact that _my_ 2/2 you are responding to was meant
to be applied on top of _my_ 1/2 that was marked as RFC.
^ permalink raw reply
* Re: [PATCH] t1304: fall back to $USER if $LOGNAME is not defined
From: Junio C Hamano @ 2011-10-14 18:41 UTC (permalink / raw)
To: René Scharfe; +Cc: Git Mailing List, Brandon Casey, Matthieu Moy
In-Reply-To: <4E98750D.1020106@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> For some reason $LOGNAME is not set anymore for me after an upgrade from
> Ubuntu 11.04 to 11.10. Use $USER in such a case.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
> The only other use of $LOGNAME is in git-cvsimport, which does the same.
While the change to assign $USER to $LOGNAME when the latter is not set is
not wrong per-se, I have to wonder (1) why this test should use LOGNAME
not USER in the first place, and (2) why you lost LOGNAME.
LOGNAME
The system shall initialize this variable at the time of login to be the
user's login name. See <pwd.h> . For a value of LOGNAME to be portable
across implementations of POSIX.1-2008, the value should be composed of
characters from the portable filename character set.
Will apply anyway. Thanks.
^ permalink raw reply
* [PATCH] send-email: Fix %config_path_settings handling
From: Jakub Narebski @ 2011-10-14 18:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, Git Mailing List, Cord Seele, Cord Seele
In-Reply-To: <7vwrc7zbk2.fsf@alter.siamese.dyndns.org>
From: Cord Seele <cowose@gmail.com>
cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30) broke
the expansion of aliases.
This was caused by treating %config_path_settings, newly introduced in
said patch, like %config_bool_settings instead of like %config_settings.
Copy from %config_settings, making it more readable.
Nb. there were a few issues that were responsible for this error:
1. %config_bool_settings and %config_settings despite similar name have
different semantic.
%config_bool_settings values are arrays where the first element is
(reference to) the variable to set, and second element is default
value... which admittedly is a bit cryptic. More readable if more
verbose option would be to use hash reference, e.g.:
my %config_bool_settings = (
"thread" => { variable => \$thread, default => 1},
[...]
Or something like that.
%config_settings values are either either reference to scalar variable
or reference to array. In second case it means that option (or config
option) is multi-valued. BTW. this is similar to what Getopt::Long does.
2. In cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30)
the setting "aliasesfile" was moved from %config_settings to newly
introduced %config_path_settings. But the loop that parses settings
from %config_path_settings was copy'n'pasted *wrongly* from
%config_bool_settings instead of from %config_settings.
It looks like cec5dae author cargo-culted this change...
3. 994d6c6 (send-email: address expansion for common mailers, 2006-05-14)
didn't add test for alias expansion to t9001-send-email.sh
Signed-off-by: Cord Seele <cowose@gmail.com>
Tested-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio C Hamano wrote:
> Thanks for a detailed write-up. I'd appreciate a final fix in an
> apply-able patch form.
Something like this?
Nb. I was not sure whether to keep Cord authorship...
git-send-email.perl | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 91607c5..41807b6 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -337,8 +337,15 @@ sub read_config {
}
foreach my $setting (keys %config_path_settings) {
- my $target = $config_path_settings{$setting}->[0];
- $$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+ my $target = $config_path_settings{$setting};
+ if (ref($target) eq "ARRAY" && !@$target) {
+ # multi-valued and not set
+ my @values = Git::config_path(@repo, "$prefix.$setting");
+ @$target = @values if (@values && defined $values[0]);
+ } elsif (!defined $$target) {
+ # multi-valued and not set
+ $$target = Git::config_path(@repo, "$prefix.$setting");
+ }
}
foreach my $setting (keys %config_settings) {
--
1.7.6
^ permalink raw reply related
* Re: [PATCH 0/6] http-auth-early
From: Junio C Hamano @ 2011-10-14 18:59 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
In-Reply-To: <20111014131932.GE7808@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Your changes all look right. The naming of git_getpass_one in the
> cherry-picked commit is a little odd without the rest of the series as
> context. I would maybe have called it "git_getpass_with_description" or
> something.
Thanks both; will queue with that updated function name.
^ permalink raw reply
* Re: [PATCH] t7800: avoid arithmetic expansion notation
From: Junio C Hamano @ 2011-10-14 19:00 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Sitaram Chamarty
In-Reply-To: <7v4nzb1mjd.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> ba959de (git-difftool: allow skipping file by typing 'n' at prompt, 2011-10-08)
>> introduced shell code like
>>
>> $((foo; bar) | baz)
>>
>> which some shells (e.g. bash, dash) interpret as an unfinished arithmetic
>> evaluation $(( expr )).
>
> Ahh, thanks, I should have caught this. I recall I rewrote a similar one
> to $( (command; command) | command ) more than once before.
Forgot to mention that I love to see the issues on topics discovered and
corrected before they are still cooking in 'next', like this ;-)
Thanks all.
^ permalink raw reply
* Re: url.<base>.insteadOf with empty value
From: Kirill Likhodedov @ 2011-10-14 19:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Josh Triplett
In-Reply-To: <7v8von1mnd.fsf@alter.siamese.dyndns.org>
14.10.2011, в 21:57, Junio C Hamano writes:
>> If I don't specify any value for url.<base>.insteadOf or url.<base>.pushInsteadOf, Git substitutes all urls for remotes defined in .git/config
>>
>> If it is not a bug, I'd suggest to add a note to man git-config about this.
>
> Please assume that what the documentation says is clear enough for whoever
> wrote it and need no further clarification, so you would need to help them
> understand what additional things you may want the documentation to say,
> by clarifying "add a note" and "about this" a bit.
I would add the following to the "url.<base>.insteadOf" section of git-config:
:::: If empty value is specified in insteadOf, <base> will be added to the start of any URL.
> The "insteadOf" replacement is meant to apply for any URL we use. I would
> be surprised if it did not affect pushURL; it would be a bug if it didn't.
That was the question I wanted to clarify.
I thought that pushURL shouldn't be affected by insteadOf, because it is not affected by pushInsteadOf.
Thanks for clarifying that out.
> On the other hand, the rewrite done by "pushinsteadof" is meant to apply
> only when remote.<any>.url is used for pushing. See t/t5516-fetch-push.sh
> part of the patch for 1c2eafb (Add url.<base>.pushInsteadOf: URL rewriting
> for push only, 2009-09-07). It would clarify what the intended interaction
> among these configuration variables.
Yeah, that's clear. My question was only about insteadOf behavior. Sorry for making it not clear enough. The behavior of pushInsteadOf is completely clear from the discussion thread I mentioned and from t5516-fetch-push.sh
Thanks.
^ permalink raw reply
* Re: Bug? url.insteadOf overwrites remote.pushUrl
From: Kirill Likhodedov @ 2011-10-14 19:16 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <4E9856BD.3010807@drmicha.warpmail.net>
>> On the other hand, pushInsteadOf doesn't
>> override explicit pushUrl. Is it a bug?
>
> That is as described in the thread (thanks for linking to it).
> pushurl predates pushinsteadof, and when the latter was introduced, one
> could have argued for or against "insteadof" being applied to pushurls.
> But that was necessary before, and existing behavior at the time when
> pushinsteadof was introduced. So, I don't see a bug, nor anything we
> could change now, though arguably most people use either pushinstead of
> or pushurl, but not both.
Yeah, that's clear now. As I already answered to Junio in another thread, my question was only about insteadOf, not pushInsteadOf behavior.
The behavior of pushInsteadOf is completely clear from the thread and from t5516-fetch-push.sh
Sorry for making it not clear enough.
Anyway, thanks for giving a one more explanation and example on how url, pushUrl and insteadOf configs work, it is always useful to go though it once again.
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Jeff King @ 2011-10-14 19:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyen Thai Ngoc Duy, git, Ilari Liusvaara, Johannes Sixt,
Jonathan Nieder
In-Reply-To: <20111014131041.GC7808@sigill.intra.peff.net>
On Fri, Oct 14, 2011 at 09:10:41AM -0400, Jeff King wrote:
> Let's start with my original patch (which I'll clean and repost). And if
> somebody really wants to push towards customized messages, that is easy
> enough to do on top later.
Here it is, with a few minor tweaks:
1. git-daemon respects --no-informative-errors, too
2. the new flag is documented
3. I switched the format from:
fatal: remote: /path/in/your/git/url: access denied
to:
fatal: remote: access denied: /path/in/your/git/url
I find the latter easier to read, and it would be easier for a
client to recognize.
4. there's now a commit message
-- >8 --
Subject: [PATCH] daemon: give friendlier error messages to clients
When the git-daemon is asked about an inaccessible
repository, it simply hangs up the connection without saying
anything further. This makes it hard to distinguish between
a repository we cannot access (e.g., due to typo), and a
service or network outage.
Instead, let's print an "ERR" line, which git clients
understand since v1.6.1 (2008-12-24).
Because there is a risk of leaking information about
non-exported repositories, by default all errors simply say
"access denied". Open sites can pass a flag to turn on more
specific messages.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-daemon.txt | 8 ++++++++
daemon.c | 25 +++++++++++++++++++++----
2 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 69a1e4a..ac57c6d 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -161,6 +161,14 @@ the facility of inet daemon to achieve the same before spawning
repository configuration. By default, all the services
are overridable.
+--informative-errors::
+ Return more verbose errors to the client, differentiating
+ conditions like "no such repository" from "repository not
+ exported". This is more convenient for clients, but may leak
+ information about the existence of unexported repositories.
+ Without this option, all errors report "access denied" to the
+ client.
+
<directory>::
A directory to add to the whitelist of allowed directories. Unless
--strict-paths is specified this will also include subdirectories
diff --git a/daemon.c b/daemon.c
index 4c8346d..e5869ec 100644
--- a/daemon.c
+++ b/daemon.c
@@ -20,6 +20,7 @@
static int log_syslog;
static int verbose;
static int reuseaddr;
+static int informative_errors;
static const char daemon_usage[] =
"git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
return 0;
}
+static int daemon_error(const char *dir, const char *msg)
+{
+ if (!informative_errors)
+ msg = "access denied";
+ packet_write(1, "ERR %s: %s", msg, dir);
+ return -1;
+}
+
static int run_service(char *dir, struct daemon_service *service)
{
const char *path;
@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
if (!enabled && !service->overridable) {
logerror("'%s': service not enabled.", service->name);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "service not enabled");
}
if (!(path = path_ok(dir)))
- return -1;
+ return daemon_error(dir, "no such repository");
/*
* Security on the cheap.
@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
logerror("'%s': repository not exported.", path);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "repository not exported");
}
if (service->overridable) {
@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
logerror("'%s': service not enabled for '%s'",
service->name, path);
errno = EACCES;
- return -1;
+ return daemon_error(dir, "service not enabled");
}
/*
@@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
make_service_overridable(arg + 18, 0);
continue;
}
+ if (!prefixcmp(arg, "--informative-errors")) {
+ informative_errors = 1;
+ continue;
+ }
+ else if (!prefixcmp(arg, "--no-informative-errors")) {
+ informative_errors = 0;
+ continue;
+ }
if (!strcmp(arg, "--")) {
ok_paths = &argv[i+1];
break;
--
1.7.6.4.37.g43b58b
^ permalink raw reply related
* [PATCH 1/3] git-gui: fix unintended line break in message string
From: Bert Wesarg @ 2011-10-14 19:25 UTC (permalink / raw)
To: Pat Thoyts; +Cc: Heiko Voigt, git, Bert Wesarg
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Probably because of a white-space damaged patch.
---
lib/index.tcl | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/lib/index.tcl b/lib/index.tcl
index e38b647..3a9c8b7 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -367,8 +367,7 @@ proc do_add_all {} {
}
}
if {[llength $unknown_paths]} {
- set reply [ask_popup [mc "There are unknown files do you also want
-to stage those?"]]
+ set reply [ask_popup [mc "There are unknown files do you also want to stage those?"]]
if {$reply} {
set paths [concat $paths $unknown_paths]
}
--
1.7.6.789.gb4599
^ permalink raw reply related
* [PATCH 2/3] git-gui: use "untracked" for files which are not known to git
From: Bert Wesarg @ 2011-10-14 19:25 UTC (permalink / raw)
To: Pat Thoyts; +Cc: Heiko Voigt, git, Bert Wesarg
In-Reply-To: <0f862de296a94b06495e4418bc731b5d201d5767.1318620267.git.bert.wesarg@googlemail.com>
"untracked" is the right phrase for files new to git. For example
git-status uses this phrase. Also make the question shorter.
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
lib/index.tcl | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/lib/index.tcl b/lib/index.tcl
index 3a9c8b7..014acf9 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -356,20 +356,20 @@ proc do_add_all {} {
global file_states
set paths [list]
- set unknown_paths [list]
+ set untracked_paths [list]
foreach path [array names file_states] {
switch -glob -- [lindex $file_states($path) 0] {
U? {continue}
?M -
?T -
?D {lappend paths $path}
- ?O {lappend unknown_paths $path}
+ ?O {lappend untracked_paths $path}
}
}
- if {[llength $unknown_paths]} {
- set reply [ask_popup [mc "There are unknown files do you also want to stage those?"]]
+ if {[llength $untracked_paths]} {
+ set reply [ask_popup [mc "Stage also untracked files?"]]
if {$reply} {
- set paths [concat $paths $unknown_paths]
+ set paths [concat $paths $untracked_paths]
}
}
add_helper {Adding all changed files} $paths
--
1.7.6.789.gb4599
^ permalink raw reply related
* [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Bert Wesarg @ 2011-10-14 19:25 UTC (permalink / raw)
To: Pat Thoyts; +Cc: Heiko Voigt, git, Bert Wesarg
In-Reply-To: <0f862de296a94b06495e4418bc731b5d201d5767.1318620267.git.bert.wesarg@googlemail.com>
The default is the current "ask".
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
git-gui.sh | 1 +
lib/index.tcl | 14 +++++++++++++-
lib/option.tcl | 18 ++++++++++++++++++
3 files changed, 32 insertions(+), 1 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index f897160..77deff7 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -859,6 +859,7 @@ set font_descs {
{fontui font_ui {mc "Main Font"}}
{fontdiff font_diff {mc "Diff/Console Font"}}
}
+set default_config(gui.stageuntracked) ask
######################################################################
##
diff --git a/lib/index.tcl b/lib/index.tcl
index 014acf9..45094c2 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -367,7 +367,19 @@ proc do_add_all {} {
}
}
if {[llength $untracked_paths]} {
- set reply [ask_popup [mc "Stage also untracked files?"]]
+ set reply 0
+ switch -- [get_config gui.stageuntracked] {
+ no {
+ set reply 0
+ }
+ yes {
+ set reply 1
+ }
+ ask -
+ default {
+ set reply [ask_popup [mc "Stage also untracked files?"]]
+ }
+ }
if {$reply} {
set paths [concat $paths $untracked_paths]
}
diff --git a/lib/option.tcl b/lib/option.tcl
index 3807c8d..719103a 100644
--- a/lib/option.tcl
+++ b/lib/option.tcl
@@ -156,6 +156,7 @@ proc do_options {} {
{i-0..99 gui.commitmsgwidth {mc "Commit Message Text Width"}}
{t gui.newbranchtemplate {mc "New Branch Name Template"}}
{c gui.encoding {mc "Default File Contents Encoding"}}
+ {s gui.stageuntracked {mc "Staging of untracked files"} {list "yes" "no" "ask"}}
} {
set type [lindex $option 0]
set name [lindex $option 1]
@@ -208,6 +209,23 @@ proc do_options {} {
}
pack $w.$f.$optid -side top -anchor w -fill x
}
+ s {
+ set opts [eval [lindex $option 3]]
+ ${NS}::frame $w.$f.$optid
+ ${NS}::label $w.$f.$optid.l -text "$text:"
+ if {$use_ttk} {
+ ttk::combobox $w.$f.$optid.v \
+ -textvariable ${f}_config_new($name) \
+ -values $opts -state readonly
+ } else {
+ eval tk_optionMenu $w.$f.$optid.v \
+ ${f}_config_new($name) \
+ $opts
+ }
+ pack $w.$f.$optid.l -side left -anchor w -fill x
+ pack $w.$f.$optid.v -side right -anchor e -padx 5
+ pack $w.$f.$optid -side top -anchor w -fill x
+ }
}
}
}
--
1.7.6.789.gb4599
^ permalink raw reply related
* Re: [PATCH] send-email: Fix %config_path_settings handling
From: Junio C Hamano @ 2011-10-14 19:26 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Michael J Gruber, Git Mailing List, Cord Seele, Cord Seele
In-Reply-To: <201110142049.32734.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> From: Cord Seele <cowose@gmail.com>
> value... which admittedly is a bit cryptic. More readable if more
> verbose option would be to use hash reference, e.g.:
>
> my %config_bool_settings = (
> "thread" => { variable => \$thread, default => 1},
> [...]
>
> Or something like that.
Do you really want to leave this "Or something like that" here?
> 3. 994d6c6 (send-email: address expansion for common mailers, 2006-05-14)
> didn't add test for alias expansion to t9001-send-email.sh
I was hoping that an updated patch to have a new test or two here...
> Signed-off-by: Cord Seele <cowose@gmail.com>
> Tested-by: Michael J Gruber <git@drmicha.warpmail.net>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Is this the version tested by Michael?
> + my $target = $config_path_settings{$setting};
> + if (ref($target) eq "ARRAY" && !@$target) {
> + # multi-valued and not set
> + my @values = Git::config_path(@repo, "$prefix.$setting");
> + @$target = @values if (@values && defined $values[0]);
> + } elsif (!defined $$target) {
> + # multi-valued and not set
> + $$target = Git::config_path(@repo, "$prefix.$setting");
> + }
If the target is an array ref and for whatever reason the array is already
populated, wouldn't you check "if (!defined $$target)" with this change?
^ 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