* Re: [RFC/PATCH 1/2] bundle: allowing to read from an unseekable fd
From: Phil Hord @ 2011-10-14 12:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
In-Reply-To: <7vpqi034l0.fsf@alter.siamese.dyndns.org>
On Thu, Oct 13, 2011 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The current code opens a given file with fopen(), reads it until the end
> of the header and runs ftell(), and reopens the same file with open() and
> seeks to skip the header. This structure makes it hard to retarget the
> code to read from input that is not seekable, such as a network socket.
>
> This patch by itself does not reach that goal yet, but I think it is a
> right step in that direction.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * It would be nice if we can avoid byte-by-byte reading from the file
> descriptor by over-reading into the strbuf and pass the remainder to
> the caller of read_bundle_header(), but I suspect that it would require
> us to carry the "here is the remainder from the previous read" buffer
> around throughout the transport layer. Parsing of the header wouldn't
> be performance critical compared to the computation cost of actually
> reading the rest of the bundle, hopefully, so...
>
> bundle.c | 99 ++++++++++++++++++++++++++++++++++++++++----------------------
> 1 files changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index f48fd7d..3aa715c 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -23,49 +23,78 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
> list->nr++;
> }
>
> -/* returns an fd */
> +/* Eventually this should go to strbuf.[ch] */
> +static int strbuf_readline_fd(struct strbuf *sb, int fd)
A size limiter would be useful here. Since it's readline, maybe the
limit can even be hardcoded.
Without a limit, calling this with something stupid like "/dev/zero"
will consume all memory and never return.
Phil
^ permalink raw reply
* Re: [PATCH 2/2] bundle: add parse_bundle_header() helper function
From: Phil Hord @ 2011-10-14 12:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Shawn O. Pearce, Phil Hord
In-Reply-To: <7vliso34gc.fsf@alter.siamese.dyndns.org>
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, but I did pose a question in the message
section. Is there a rule for what marks something as RFC (if not the
RFC label in the subject line)?
Your implementation looks fine to me.
Phil
^ permalink raw reply
* Re: [PATCH] pack-objects: protect against disappearing packs
From: Jeff King @ 2011-10-14 13:02 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git, git-dev, Shawn O. Pearce
In-Reply-To: <alpine.LFD.2.02.1110132234210.17040@xanadu.home>
On Thu, Oct 13, 2011 at 10:42:28PM -0400, Nicolas Pitre wrote:
> > ---
>
> you should put your SOB above that line I would think.
Thanks. I cheated and wrote my "---" cover letter in the commit message
locally, knowing that it would get included by format-patch but stripped
by am on Junio's end. Which does work, except that "format-patch -s"
puts the SOB in the wrong place. :)
> Acked-by: Nicolas Pitre <nico@fluxnic.net>
Thanks for reviewing.
> > We're seeing this at GitHub because we prune pretty
> > aggressively. We let pushes go into individual repositories,
> > but then we kick off a job to move the resulting objects
> > into the repository's "network" repo, which is basically a
> > big alternates repository for related repos.
>
> While this patch certainly has value, it doesn't provide 100%
> reliability for that use case. Maybe the github infrastructure should
> simply skip any auto-repack on push if some other object maintenance
> operation is ongoing, possibly via the pre-auto-gc hook.
I'm not sure I understand the problem. We already serialize the
re-packing jobs in a queue, so you won't have two repacks going at once.
The problematic pack-objects is the one started by upload-pack when
somebody fetches. Or do you mean turning off receive.autogc? I'd have to
check if we do that, but we definitely should; it's useless to us
(though it would be unlikely to trigger anyway, because we are manually
repacking so frequently).
-Peff
^ permalink raw reply
* Re: [PATCH] pack-objects: protect against disappearing packs
From: Jeff King @ 2011-10-14 13:07 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, git-dev, Shawn O. Pearce, Nicolas Pitre
In-Reply-To: <4E97DF63.104@viscovery.net>
On Fri, Oct 14, 2011 at 09:06:11AM +0200, Johannes Sixt wrote:
> Am 10/14/2011 3:23, schrieb Jeff King:
> > 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.
>
> On Windows, we cannot remove files that are open. If I understand
> correctly, this patch keeps more files open for a longer time. Is there
> any chance that packfiles remain now open until an unlink() call?
>
> I am not worried about parallel processes (we already have a problem
> there), but that this can now happen within a single process, i.e., that a
> single git-repack -a -d -f would now try to unlink a pack file that it
> opened itself and did not close timely.
>
> I'll test your patch later this weekend to see whether the test suite
> finds something. But perhaps you know the answer already?
With two parallel processes, this will definitely increase the
likelihood of a deleted file being open. That is the point. :)
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.
-Peff
^ permalink raw reply
* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Jeff King @ 2011-10-14 13:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyen Thai Ngoc Duy, git, Ilari Liusvaara, Johannes Sixt,
Jonathan Nieder
In-Reply-To: <7vvcrs181e.fsf@alter.siamese.dyndns.org>
On Thu, Oct 13, 2011 at 10:01:01PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So if we want to do anything, I would think it would be a hook. Except
> > that we may or may not have a repo, so it would not be a hook in
> > $GIT_DIR/hooks, but rather some script to be run passed on the command
> > line, like:
> >
> > git daemon --informative-errors=/path/to/hook
>
> I don't think it is necessarily good to have such a variation across
> hosting sites. Your "something like this" patch looked like it was giving
> a reasonable level of detail, IMO.
Yeah. With arbitrary messages, the client has no way of programatically
deciphering which message is which, so localization becomes impossible.
HTTP solved this by having a response code _and_ still allowing content
for custom pages. That kind of works, though most of the time I find
things like custom 404 pages to just be junk.
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.
-Peff
^ permalink raw reply
* Re: [PATCH 0/6] http-auth-early
From: Jeff King @ 2011-10-14 13:19 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1318577792.git.git@drmicha.warpmail.net>
On Fri, Oct 14, 2011 at 09:40:34AM +0200, Michael J Gruber wrote:
> Here are the early parts of Jeff's http-auth-keyring series.
> It contains only parts which are not using the credential API (which
> is still under discussion), so that this can go in (and help users)
> and alleviates the pressure on the credential discussion:
>
> Early bits with cleanups to http.c.
> Cherry-picked bit for improved prompts ("Username for ..." etc.)
> Cherry-pickes bit for using configured pushurls.
>
> I tried to pick/resolve in a way which should help rebasing Jeff's series
> on top of this.
Thanks for working on this. One of my intended tasks for today is to
rebase my series, so it is nice to wake up to half of the work done. :)
> Jeff King (5):
> url: decode buffers that are not NUL-terminated
> improve httpd auth tests
> remote-curl: don't retry auth failures with dumb protocol
> http: retry authentication failures for all http requests
> http_init: accept separate URL parameter
>
> Michael J Gruber (1):
> http: use hostname in credential description
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.
-Peff
^ permalink raw reply
* Re: [PATCH 0/6] http-auth-early
From: Michael J Gruber @ 2011-10-14 13:24 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20111014131932.GE7808@sigill.intra.peff.net>
Jeff King venit, vidit, dixit 14.10.2011 15:19:
> On Fri, Oct 14, 2011 at 09:40:34AM +0200, Michael J Gruber wrote:
>
>> Here are the early parts of Jeff's http-auth-keyring series.
>> It contains only parts which are not using the credential API (which
>> is still under discussion), so that this can go in (and help users)
>> and alleviates the pressure on the credential discussion:
>>
>> Early bits with cleanups to http.c.
>> Cherry-picked bit for improved prompts ("Username for ..." etc.)
>> Cherry-pickes bit for using configured pushurls.
>>
>> I tried to pick/resolve in a way which should help rebasing Jeff's series
>> on top of this.
>
> Thanks for working on this. One of my intended tasks for today is to
> rebase my series, so it is nice to wake up to half of the work done. :)
Good morning :)
>> Jeff King (5):
>> url: decode buffers that are not NUL-terminated
>> improve httpd auth tests
>> remote-curl: don't retry auth failures with dumb protocol
>> http: retry authentication failures for all http requests
>> http_init: accept separate URL parameter
>>
>> Michael J Gruber (1):
>> http: use hostname in credential description
>
> 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.
git_getpass_my_life_will_be_short_and_ended_by_credentials
I don't care. In fact, I wasn't sure whether I should I even change the
author on this one. It's not a straight resolution and does involve
choices, but the meat is from your series.
Michael
^ permalink raw reply
* Bug? url.insteadOf overwrites remote.pushUrl
From: Kirill Likhodedov @ 2011-10-14 13:55 UTC (permalink / raw)
To: git
I've found that defining url.<base>.insteadOf overrides explicit remote.<name>.pushUrl.
On the other hand, pushInsteadOf doesn't override explicit pushUrl.
Is it a bug?
# 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)
See also http://kerneltrap.org/mailarchive/git/2009/9/7/11264/thread - patch introducing pushInsteadOf & discussion
----------------------------------
Kirill Likhodedov
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"
^ permalink raw reply
* Re: defined behaviour for multiple urls for a remote
From: Sitaram Chamarty @ 2011-10-14 14:01 UTC (permalink / raw)
To: Kirill Likhodedov; +Cc: git
In-Reply-To: <loom.20111014T113208-660@post.gmane.org>
On Fri, Oct 14, 2011 at 3:06 PM, Kirill Likhodedov
<Kirill.Likhodedov@jetbrains.com> wrote:
> Sitaram Chamarty <sitaramc <at> gmail.com> writes:
>
>> What's the defined behaviour if I do this:
>>
>> [remote "both"]
>> url = https://code.google.com/p/gitolite/
>> url = git <at> github.com:sitaramc/gitolite.git
>>
>> I know what I'm seeing (a fetch only goes to the first URL, and does a
>> HEAD->FETCH_HEAD because I didn't provide a refspec line, while a push
>> seems to push all to both), but I was curious what the official
>> position is, because I couldn't find it in the docs.
>
> Please see the message from Linus about that: http://marc.info/?l=git&m=116231242118202&w=2
cool; thanks!
> You also may check how Git understands your remotes by running
> git remote -v
Aah that's very clear!
> It will show, where it is going to fetch from and push to.
>
> I agree though, that documentation should be updated.
well if it never came up once in the 5 years since Linus wrote that email... :-)
^ permalink raw reply
* [PATCH] fix alias expansion with new Git::config_path()
From: Cord Seele @ 2011-10-14 14:25 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Git Mailing List, Junio C Hamano, Jakub Narebski
In-Reply-To: <4E982B27.8050807@drmicha.warpmail.net>
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:
>
> ./git-send-email --cc=junio --dry-run
> 0001-t7800-avoid-arithmetic-expansion-notation.patch
> 0001-t7800-avoid-arithmetic-expansion-notation.patch
> Who should the emails appear to be from? [Michael J Gruber
> <git@drmicha.warpmail.net>]
> Emails will be sent from: Michael J Gruber <git@drmicha.warpmail.net>
> Dry-OK. Log says:
> Sendmail: /home/mjg/bin/msmtp-fastmail-git -i git@vger.kernel.org junio
> git@drmicha.warpmail.net
> From: Michael J Gruber <git@drmicha.warpmail.net>
> To: git@vger.kernel.org
> Cc: junio
> ...
>
> Happens with both "--cc junio" and "--cc=junio".
>
> Reverting cec5dae brings my aliases back. Relevant config:
>
> git config --get-regexp sendemail.alias\*
> sendemail.aliasesfile /home/mjg/git/gitauthors
> sendemail.aliasfiletype mutt
>
> Can I please have alias expansion 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)?
-- Cord
Signed-off-by: Cord Seele <cowose@gmail.com>
---
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) {
--
1.7.6.4
^ permalink raw reply related
* Re: [PATCH] fix alias expansion with new Git::config_path()
From: Michael J Gruber @ 2011-10-14 14:30 UTC (permalink / raw)
To: Git Mailing List, Junio C Hamano, Jakub Narebski
In-Reply-To: <20111014142557.GB13680@laptop>
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:
>>
>> ./git-send-email --cc=junio --dry-run
>> 0001-t7800-avoid-arithmetic-expansion-notation.patch
>> 0001-t7800-avoid-arithmetic-expansion-notation.patch
>> Who should the emails appear to be from? [Michael J Gruber
>> <git@drmicha.warpmail.net>]
>> Emails will be sent from: Michael J Gruber <git@drmicha.warpmail.net>
>> Dry-OK. Log says:
>> Sendmail: /home/mjg/bin/msmtp-fastmail-git -i git@vger.kernel.org junio
>> git@drmicha.warpmail.net
>> From: Michael J Gruber <git@drmicha.warpmail.net>
>> To: git@vger.kernel.org
>> Cc: junio
>> ...
>>
>> Happens with both "--cc junio" and "--cc=junio".
>>
>> Reverting cec5dae brings my aliases back. Relevant config:
>>
>> git config --get-regexp sendemail.alias\*
>> sendemail.aliasesfile /home/mjg/git/gitauthors
>> sendemail.aliasfiletype mutt
>>
>> Can I please have alias expansion 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)?
>
> -- Cord
>
>
> 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.)
> ---
> 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) {
^ permalink raw reply
* 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
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