* Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
From: Junio C Hamano @ 2013-02-20 8:54 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Shawn O. Pearce
In-Reply-To: <20130220084754.GA17055@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
>
>In the meantime, please hold off on what I've posted so far (that
>includes the jk/smart-http-robustify topic).
Surely. I'm done for the night already. Looking forward to see the reroll tomorrow.
Thanks.
--
Pardon terseness, typo and HTML from a tablet.
^ permalink raw reply
* Re: Git Merge 2013 Conference, Berlin
From: Jeff King @ 2013-02-20 8:57 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Junio C Hamano, Scott Chacon, Thomas Rast, git list
In-Reply-To: <512483DA.3040602@drmicha.warpmail.net>
On Wed, Feb 20, 2013 at 09:05:46AM +0100, Michael J Gruber wrote:
> Maybe, we can - for the next time - try to coordinate the date with the
> various international IT conferences which take place here, like
> Linux-Tag in Berlin (just a few weeks apart), CEBIT in Hannover or the
> smaller Chemnitzer Linux-Tage (or coordinate with events somewhere else
> in Europe). That would give contributors not only more incentive to come
> to the Git event, but also a better chance for successful negotiations
> with their employers. Extending a business trip by a few days is a minor
> issue, granting one in the first place not always.
I actually have the opposite preference; I hate when conferences are
back-to-back, because by the fourth day I'm pretty tired of conferencing
and ready to sit in my hotel room.
Of course, my employer is rather generous with giving time for Git
conferences, so I do not feel the time crunch the way other people do,
and can optimize for relaxation. But I wonder where other people come
down in that tradeoff.
-Peff
^ permalink raw reply
* [RFC] Provide a mechanism to turn off symlink resolution in ceiling paths
From: Michael Haggerty @ 2013-02-20 9:09 UTC (permalink / raw)
To: git
Cc: Anders Kaseorg, David Aguilar, Jiang Xin, Lea Wiemann,
David Reiss, Johannes Sixt, Lars R. Damerow, Jeff King,
Marc Jordan, Junio C Hamano, Michael Haggerty
In-Reply-To: <7vmwuzzd76.fsf@alter.siamese.dyndns.org>
Commit 1b77d83cab 'setup_git_directory_gently_1(): resolve symlinks in
ceiling paths' changed the setup code to resolve symlinks in the
entries in GIT_CEILING_DIRECTORIES. Because those entries are
compared textually to the symlink-resolved current directory, an entry
in GIT_CEILING_DIRECTORIES that contained a symlink would have no
effect. It was known that this could cause performance problems if
the symlink resolution *itself* touched slow filesystems, but it was
thought that such use cases would be unlikely.
After this change was released, Anders Kaseorg <andersk@mit.edu>
reported:
> [...] my computer has been acting so slow when I’m not connected to
> the network. I put various network filesystem paths in
> $GIT_CEILING_DIRECTORIES, such as
> /afs/athena.mit.edu/user/a/n/andersk (to avoid hitting its parents
> /afs/athena.mit.edu, /afs/athena.mit.edu/user/a, and
> /afs/athena.mit.edu/user/a/n which all live in different AFS
> volumes). Now when I’m not connected to the network, every
> invocation of Git, including the __git_ps1 in my shell prompt, waits
> for AFS to timeout.
So provide the following mechanism to turn off symlink resolution in
GIT_CEILING_DIRECTORIES entries: if that environment variable contains
an empty entry (e.g., GIT_CEILING_DIRECTORIES=:/foo/bar:/xyzzy or
GIT_CEILING_DIRECTORIES=/foo/bar::/xyzzy), then do not resolve
symlinks in paths that follow the empty entry.
---
This is a possible implementation (untested!) of Junio's suggestion,
with the slight twist that the empty entry can appear anywhere in
GIT_CEILING_DIRECTORIES and only turns off symlink expansion for
subsequent entries. (The original suggestion would be similarly easy
to implement if it is preferred.)
Unfortunately I am swamped with other work right now so I don't have
time to test the code and might not be able to respond promptly to
feedback.
Another alternative (not implemented here) would be to support a
second environment variable with an ugly name like
GIT_CEILING_DIRECTORIES_NO_SYMLINKS which, when set, tells Git not to
resolve symlinks in GIT_CEILING_DIRECTORIES. Hopefully the variable
name itself would warn the user that symlinks are an issue.
The ugliness of the situation unfortunately seems to preclude a
non-ugly solution of one form or another.
Documentation/git.txt | 18 ++++++++++++------
setup.c | 32 ++++++++++++++++++++++----------
2 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index da0115f..35c0517 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -674,12 +674,18 @@ git so take care if using Cogito etc.
The '--namespace' command-line option also sets this value.
'GIT_CEILING_DIRECTORIES'::
- This should be a colon-separated list of absolute paths.
- If set, it is a list of directories that git should not chdir
- up into while looking for a repository directory.
- It will not exclude the current working directory or
- a GIT_DIR set on the command line or in the environment.
- (Useful for excluding slow-loading network directories.)
+ This should be a colon-separated list of absolute paths. If
+ set, it is a list of directories that git should not chdir up
+ into while looking for a repository directory (useful for
+ excluding slow-loading network directories). It will not
+ exclude the current working directory or a GIT_DIR set on the
+ command line or in the environment. Normally, Git has to read
+ the entries in this list are read to resolve any symlinks that
+ might be present. However, if even this access is slow, you
+ can add an empty entry to the list to tell Git that the
+ subsequent entries are not symlinks and needn't be resolved;
+ e.g.,
+ 'GIT_CEILING_DIRECTORIES=/maybe/symlink::/very/slow/non/symlink'.
'GIT_DISCOVERY_ACROSS_FILESYSTEM'::
When run in a directory that does not have ".git" repository
diff --git a/setup.c b/setup.c
index f108c4b..1b12017 100644
--- a/setup.c
+++ b/setup.c
@@ -624,22 +624,32 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
/*
* A "string_list_each_func_t" function that canonicalizes an entry
* from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
- * discards it if unusable.
+ * discards it if unusable. The presence of an empty entry in
+ * GIT_CEILING_DIRECTORIES turns off canonicalization for all
+ * subsequent entries.
*/
static int canonicalize_ceiling_entry(struct string_list_item *item,
- void *unused)
+ void *cb_data)
{
+ int *empty_entry_found = cb_data;
char *ceil = item->string;
- const char *real_path;
- if (!*ceil || !is_absolute_path(ceil))
+ if (!*ceil) {
+ *empty_entry_found = 1;
return 0;
- real_path = real_path_if_valid(ceil);
- if (!real_path)
+ } else if (!is_absolute_path(ceil)) {
return 0;
- free(item->string);
- item->string = xstrdup(real_path);
- return 1;
+ } else if (*empty_entry_found) {
+ /* Keep entry but do not canonicalize it */
+ return 1;
+ } else {
+ const char *real_path = real_path_if_valid(ceil);
+ if (!real_path)
+ return 0;
+ free(item->string);
+ item->string = xstrdup(real_path);
+ return 1;
+ }
}
/*
@@ -679,9 +689,11 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
if (env_ceiling_dirs) {
+ int empty_entry_found = 0;
+
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
filter_string_list(&ceiling_dirs, 0,
- canonicalize_ceiling_entry, NULL);
+ canonicalize_ceiling_entry, &empty_entry_found);
ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0);
}
--
1.8.1.1
^ permalink raw reply related
* Re: [PATCH] Bugfix: undefined htmldir in config.mak.autogen
From: John Keeping @ 2013-02-20 9:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jiang Xin, Christoph J. Thompson, Git List
In-Reply-To: <7v4nh750v3.fsf@alter.siamese.dyndns.org>
On Tue, Feb 19, 2013 at 03:40:16PM -0800, Junio C Hamano wrote:
> I am not sure if such a layout can be actually used for installing,
> though. Didn't we see some issues around the relativeness of
> htmldir and mandir vs passing them down to Documentation/Makefile,
> or is it not an issue when ./configure and config.mak.autogen is
> used?
If these variables are set explicitly in config.mak.autogen (or indeed
config.mak) then these values should override the ones calculated in the
Makefiles so that we avoid that problem - the problem occurs if the
relative paths from the top-level Makefile are exported to
Documentation/Makefile.
John
^ permalink raw reply
* Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks
From: Anders Kaseorg @ 2013-02-20 9:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, David Aguilar, Jiang Xin, Lea Wiemann,
David Reiss, Johannes Sixt, git, Lars R. Damerow, Jeff King,
Marc Jordan
In-Reply-To: <7vmwuzzd76.fsf@alter.siamese.dyndns.org>
On Tue, 19 Feb 2013, Junio C Hamano wrote:
> Assuming that this says "yes":
>
> D=/afs/athena.mit.edu/user/a/n/andersk/my/dir
> cd "$D"
> test "$(/bin/pwd)" = "$D" && echo yes
Correct.
> Perhaps existing of an empty element in the list would do? E.g.
>
> GIT_CEILING_DIRECTORIES=:/afs/athena.mit.edu/users/a/n/andesk
>
> or something like that. And in such a case, we do not run realpath on
> the elements on the list before comparing them with what we get from
> getcwd(3).
That seems reasonable, and has the advantage of backwards compatibility
with versions before 1.8.1.2, I guess.
Anders
^ permalink raw reply
* Re: [PATCH] Bugfix: undefined htmldir in config.mak.autogen
From: Jiang Xin @ 2013-02-20 10:42 UTC (permalink / raw)
To: Stefano Lattarini; +Cc: Junio C Hamano, Christoph J. Thompson, Git List
In-Reply-To: <512487CB.3050908@gmail.com>
2013/2/20 Stefano Lattarini <stefano.lattarini@gmail.com>:
> On 02/20/2013 02:39 AM, Jiang Xin wrote:
>>
>> [SNIP]
>>
>> I am not familiar with autoconf. After clone autoconf and check,
>> I cannot find a neat way to change "htmldir" default location to
>> use ${datarootdir} (just like mandir).
>>
> This one-line change should be enough to do what you want:
>
> diff --git a/configure.ac b/configure.ac
> index 1991258..2bfbec9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -149,6 +149,8 @@ AC_CONFIG_SRCDIR([git.c])
> config_file=config.mak.autogen
> config_in=config.mak.in
>
> +AC_SUBST([htmldir], ['${datarootdir}'])
> +
> GIT_CONF_SUBST([AUTOCONFIGURED], [YesPlease])
>
If changed like that, set:
AC_SUBST([htmldir], ['${datarootdir}/doc/git-doc'])
In the generated "configure" file, this instruction will be inserted
after the option_parse block (not before), and will override what
the user provided by running "./configure --htmldir=DOCDIR".
BTW, add "docdir = @docdir@" to "config.mak.in", also let
"./configure --docdir=DIR" works properly.
--
Jiang Xin
^ permalink raw reply
* Re: Re* [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root
From: Adam Spiers @ 2013-02-20 10:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <7vppzv3dd8.fsf@alter.siamese.dyndns.org>
On Tue, Feb 19, 2013 at 06:53:07PM -0800, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
> > OK, thanks for the information. IMHO it would be nice if 'git
> > format-patch' and 'git am' supported this style of inline patch
> > inclusion, but maybe there are good reasons to discourage it?
>
> "git am --scissors" is a way to process such e-mail where the patch
> submitter continues discussion in the top part of a message,
> concludes the message with:
>
> A patch to do so is attached.
> -- >8 --
>
> and then tells the MUA to read in an output from format-patch into
> the e-mail buffer.
Ah, nice! I didn't know about that.
> You still need to strip out unneeded headers
> like the "From ", "From: " and "Date: " lines when you add the
> scissors anyway, and this is applicable only for a single-patch
> series, so the "feature" does not fit well as a format-patch option.
Rather than requiring the user to manually strip out unneeded headers,
wouldn't it be friendlier and less error-prone to add a new --inline
option to format-patch which omitted them in the first place? It
should be easy to make it bail with an error when multiple revisions
are requested.
^ permalink raw reply
* Re: Google Summer of Code 2013 (GSoC13)
From: Christian Couder @ 2013-02-20 12:07 UTC (permalink / raw)
To: Shawn Pearce
Cc: Jeff King, Thomas Rast, git, Jakub Narebski, Pat Thoyts,
Paul Mackerras, Carlos Martín Nieto, Thomas Gummerer,
David Michael Barr, Ramkumar Ramachandra, Jens Lehmann,
Nguyen Thai Ngoc Duy
In-Reply-To: <CAJo=hJvknVedGba5OxjjvZi2=JZyDuDoP2tD+LKQKdZNJ4NcsA@mail.gmail.com>
Hi,
On Wed, Feb 20, 2013 at 7:50 AM, Shawn Pearce <spearce@spearce.org> wrote:
> On Mon, Feb 18, 2013 at 9:42 AM, Jeff King <peff@peff.net> wrote:
>> On Mon, Feb 18, 2013 at 06:23:01PM +0100, Thomas Rast wrote:
>>
>>> * We need an org admin. AFAIK this was done by Peff and Shawn in
>>> tandem last year. Would you do it again?
>>
>> I will do it again, if people feel strongly about Git being a part of
>> it. However, I have gotten a little soured on the GSoC experience. Not
>> because of anything Google has done; it's a good idea, and I think they
>> do a fine of administering the program. But I have noticed that the work
>> that comes out of GSoC the last few years has quite often not been
>> merged, or not made a big impact in the codebase, and nor have the
>> participants necessarily stuck around.
>
> This.
I think it is ok if the code doesn't make a big impact in the code
base and it is ok too if the participants don't stuck around.
Of course I would love both of these things to happen, but we have to
be realistic and just stop expecting it.
> I actually think Git should take a year off from GSoC and not
> participate. Consequently I will not be volunteering as backup org
> admin.
>
> Git has been involved since 2007. In all of that time we have had very
> few student projects merge successfully into their upstream project
> (e.g. git.git, JGit or libgit2) before the end of GSoC. Even fewer
> students have stuck around and remained active contributors. When I
> look at the amount of effort we contributors put into GSoC, I think we
> are misusing our limited time and resources.
I don't think so, at least not for me. I feel happy to mentor or
co-mentor GSoC student and I don't think I would work much more on git
these days if git was not participating to the GSoC.
> The intention of the GSoC
> program is to grow new open source developers, and increase our
> community of contributors. Somehow I think Git is falling well short
> of its potential here. This is especially true if you compare Git's
> GSoC program to some other equally long-running GSoC programs.
>
>> And I do not want to blame the students here (some of whom are on the cc
>> list :) ). They are certainly under no obligation to stick around after
>> GSoC ends, and I know they have many demands on their time. But I am
>> also thinking about what Git wants to get out of GSoC (and to my mind,
>> the most important thing is contributors).
>
> I agree, our students have been pretty terrific. I think the
> shortcomings in our GSoC program are on the mentoring side. Our
> program has not really had much success with keeping students active
> and engaged post GSoC. I see that primarily as a mentoring failure.
> And its one we keep repeating each year.
I don't quite agree with this. My experience has been the following:
- 2008: the student I co-mentored did pretty well though he didn't
send to the list his patch series early enough.
So there was some mentoring failure, but anyway the student stuck
around for 9 months and managed to get 53 commits merged.
- 2009: if I remember well, it was decided to have only 2 GSoC student
that year, and that 5 people would co-mentor both of them together.
One of the student did nearly nothing. The other one sent his patch
series too late to the list. My opinion is that he relied too much on
the people mentoring him and he worked on something that was difficult
to merge.
- 2010: the student I co-mentored stopped working 3 weeks before the
mid-term evaluation despite some warnings from me and Peff, and he had
not been doing much a few weeks before that, so we decided to fail him
at the mid term evaluation.
- 2011: I was lucky to mentor Ram who did well and is still around.
So my opinion is that we have some students who are just not doing
enough (2 out of 5).
Then we have some good students, 2 out of 5 who could sometimes do
better if we insisted more on submitting earlier to the mailing list.
And we have a few students (1 out of 5) who work difficult to merge
projects and who could do better if we insisted more on submitting
earlier to the mailing list.
So my conclusions are:
- it's quite often going well or well enough
- when it's not going well often the student is responsible
- yes, we could improve mentoring by providing better projects and
insisting even more on submitting earlier
[...]
>> - There is also the angle that even if _Git_ doesn't benefit directly
>> from people sticking around, those people may float into other open
>> source projects and work on them. Which makes the world a better
>> place on the whole.
>
> Yes, sure, OK. But if Git doesn't participate in GSoC this year
> another org will, and this same benefit will still be had by the
> greater open source community.
The greater open source community benefits a lot these days when Git
is improved and get new contributors, as git is now by far the most
widely used version control system in the open source community.
So my opinion is that we should have has many GSoC student as we can
properly handle.
Best regards,
Christian.
^ permalink raw reply
* Re: Google Summer of Code 2013 (GSoC13)
From: Matthieu Moy @ 2013-02-20 12:26 UTC (permalink / raw)
To: Christian Couder
Cc: Shawn Pearce, Jeff King, Thomas Rast, git, Jakub Narebski,
Pat Thoyts, Paul Mackerras, Carlos Martín Nieto,
Thomas Gummerer, David Michael Barr, Ramkumar Ramachandra,
Jens Lehmann, Nguyen Thai Ngoc Duy
In-Reply-To: <CAP8UFD01bUgUz1LST6DPjhQ4qsNEA4-ndpLQ97XqH_fOEdew9w@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> - yes, we could improve mentoring by providing better projects and
> insisting even more on submitting earlier
A few words about my experience, not with GSoC, but with school projects
(I've been proposing a few students in Ensimag to contribute to Git each
year since 2010).
Last year, we've been using Scrum, and the "definition of done" was a
very helpful tool. In Scrum, nothing is ever "half done", it is either
"done" or "not done". Out of a 3 weeks project, the definition of done
was initially "ready to be sent to the list", then "sent to the list, no
major criticism in reviews" the second week, and "sent to the list, no
more objections in reviews" the last week. At the beginning of each week
("sprint" in Scrum), students were commiting to a list of tasks, and at
the end of each week, we evaluated how many were done. This encouraged
students to avoid overcommiting and send patches early. Some of them
validated nothing at all the first week: they hadn't realized the
distance between their notion of clean working code and the one on this
list, but at least they realized it early enough.
Of course, even with that, I had to continue the work to push it to
master for some patch series, and discard some series that were
basically not there.
Having several small projects instead of one big was very important. I'm
not sure how the GSoC would feel about a list of small tasks instead of
one ambitious project however.
My main disappointment is that I never managed to keep students in the
community past the end of the project.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [RFC v2] git-multimail: a replacement for post-receive-email
From: Matthieu Moy @ 2013-02-20 12:28 UTC (permalink / raw)
To: Michael Haggerty
Cc: git discussion list, Andy Parkins, Sitaram Chamarty,
Junio C Hamano, Marc Branchaud,
Ævar Arnfjörð Bjarmason, Chris Hiestand
In-Reply-To: <5104E738.602@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> A while ago, I submitted an RFC for adding a new email notification
> script to "contrib" [1].
We've discussed offline with Michael, a few patches have been merged,
and there are still a few pending pull requests. I liked the script
already, but it's getting even cooler ;-).
A few more random thoughts (not on my personal todo-list):
* It may make sense to add the short sha1 of the new reference in email
titles (branch foo updated -> branch foo updated to $sha1), so that
gmail users do not get a single huge thread "branch foo updated".
(Yes, I do know about the Reference field, but gmail uses Subject: for
threading).
* Perhaps we should allow a per-branch configuration, like
[multimailhook]
mailingList = some@list.com
[multimailhook "refs/heads/my-branch"]
mailingList = some-other@list.com
<whateverOtherConfig> = <whateverOtherValue>
Branch specific would override value for Config.get(), and
Config.get_all() should probably list both the branch-specific and the
other keys.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] Bugfix: undefined htmldir in config.mak.autogen
From: Stefano Lattarini @ 2013-02-20 12:30 UTC (permalink / raw)
To: Jiang Xin; +Cc: Junio C Hamano, Christoph J. Thompson, Git List
In-Reply-To: <CANYiYbHpEd8kgv2nPvJgDvNLhzHbYJ8uN2gLNWWwCWvrVExpBA@mail.gmail.com>
On 02/20/2013 11:42 AM, Jiang Xin wrote:
> 2013/2/20 Stefano Lattarini <stefano.lattarini@gmail.com>:
>> On 02/20/2013 02:39 AM, Jiang Xin wrote:
>>>
>>> [SNIP]
>>>
>>> I am not familiar with autoconf. After clone autoconf and check,
>>> I cannot find a neat way to change "htmldir" default location to
>>> use ${datarootdir} (just like mandir).
>>>
>> This one-line change should be enough to do what you want:
>>
>> diff --git a/configure.ac b/configure.ac
>> index 1991258..2bfbec9 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -149,6 +149,8 @@ AC_CONFIG_SRCDIR([git.c])
>> config_file=config.mak.autogen
>> config_in=config.mak.in
>>
>> +AC_SUBST([htmldir], ['${datarootdir}'])
>> +
>> GIT_CONF_SUBST([AUTOCONFIGURED], [YesPlease])
>>
>
> If changed like that, set:
>
> AC_SUBST([htmldir], ['${datarootdir}/doc/git-doc'])
>
> In the generated "configure" file, this instruction will be inserted
> after the option_parse block (not before), and will override what
> the user provided by running "./configure --htmldir=DOCDIR".
>
Yikes, you're right. Scratch my suggestion then; the issue should
probably be brought up on the autoconf mailing list. Albeit I think
it is by design that autoconf doesn't let a package to override the
defaults for installation directory: this way, the end users can
expect consistent, well-documented defaults for all autoconf-based
packages.
> BTW, add "docdir = @docdir@" to "config.mak.in", also let
> "./configure --docdir=DIR" works properly.
>
Thanks, and sorry for the noise,
Stefano
^ permalink raw reply
* Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root
From: Adam Spiers @ 2013-02-20 12:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <7vtxp73dms.fsf@alter.siamese.dyndns.org>
On Tue, Feb 19, 2013 at 06:47:23PM -0800, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
> >> Remove a sweep-the-issue-under-the-rug conditional in check-ignore
> >> that avoided to pass an empty string to the callchain while at it.
> >> It is a valid question to ask for check-ignore if the top-level is
> >> set to be ignored by default, even though the answer is most likely
> >> no, if only because there is currently no way to specify such an
> >
> > Hmm, I see very little difference between the use of "most likely" and
> > the use of the words "much" and "typically" which you previously
> > considered "a sure sign that the design of the fix is iffy".
>
> Your patch were "The reason why feeding empty string upsets
^^^^^^^^^^
"patches were", or "patch was"? It's not clear which patch(es) you're
referring to.
> hash_name() were not investigated; by punting the '.' as input, and
> ignoring the possibility that such a question might make sense, I
> can work around the segfault.
I don't see how explicitly referring to the possibility can be counted
as ignoring it.
> I do not even question if hash_name()
> that misbehaves on an empty string is a bug. Just make sure we do
> not tickle the function with a problematic input".
Presumably the "I" here refers to anthropomorphized commit message
rather than to me personally, since I did question hash_name()'s
behaviour several times already.
> The patch you are responding to declares that hash_name() should
> work sensibly on an empty string, and that is the _only_ necessary
> change for the fix. We could keep "&& path[0]", but even without
> it, by fixing the hash_name(), we will no longer segfault.
Yes, and as already stated, I agree that is a good thing.
> My "most likely" is about "the special case '&& path[0]' produces
> correct result,
Sorry, I can't understand this. You are paraphrasing something and
placing it inside "" quotes, but I can't find the corresponding
source. I presumed it refers to this extract of your proposed patch's
commit message:
"Remove a sweep-the-issue-under-the-rug conditional in check-ignore
that avoided to pass an empty string to the callchain while at it.
It is a valid question to ask for check-ignore if the top-level is
set to be ignored by default, even though the answer is most
likely no"
but I can't reconcile this extract with the paraphrase "the special
case '&& path[0]' produces correct result".
> and it is likely to stay so in the near future until
> we update .gitignore format to allow users to say 'ignore the top by
> default', which is not likely to happen soon". It is not about the
> nature of the fix at all.
>
> Still do not see the difference?
I think I *might* be beginning to see you were getting at, although my
understanding is still clouded by the ambiguities detailed above. Is
your point that the use of words like 'much' and 'typically' are a
"sure sign" of "iffy design" _when_used_to_talk_about_fixes_ but not
necessarily in other contexts? If so then it makes a bit more sense
to me, even though I tend to disagree with such broadly sweeping
generalizations, especially when the qualifying context is missing.
That aside, your idea of looking out for "bad smells" not only in code
but also in the spoken language contained by commit messages and
design discussions is an interesting one. I will try to bear that
technique in mind more consciously in the future, and see how well it
serves me.
> The removal of the "&& path[0]" is about allowing such a question
> whose likeliness may be remote. In the current .gitignore format,
> you may not be able to say "ignore the whole thing by default", so
> in that sense, the answer to the question this part of the code is
> asking when given "." may always be "no". Keeping the "&& path[0]"
> will optimize for that case.
>
> And "unusual thing to ask" below is to judge if answering such a
> question is worth optimizing for (the verdict is "no, it is not a
> common thing to do").
Yes, I understand and agree with these paragraphs.
> >> entry in the .gitignore file. But it is an unusual thing to ask and
> >> it is not worth optimizing for it by special casing at the top level
> >> of the call chain.
> >
> > Although I agree with your proposed patch's sentiment of avoiding
> > sweeping this corner case under the rug, 'check-ignore .' still
> > wouldn't match anything if for example './' was a supported mechanism
> > for ignoring the top level.
>
> It indicates that there may be more bugs (that may not result in
> segv) somewhere in check-ignore codepath, if (1)
>
> echo ./ >.gitignore
>
> were to say "ignore everything in the tree by default.", and (2) the
> real ignore check does work that way, but (3)
>
> git check-ignore .
>
> says "we do not ignore that one".
Yes, I think we are saying exactly the same thing here, although if
"It indicates that [...]" refers to your proposed patch's commit
message then I don't think it indicates the possibility of these bugs
in the most obvious or explicit way.
> Such a bug may come from some
> code that is not prepared to see an empty pathname that refers to
> the top-level in the codepath, which was why I originally asked
>
> Does the callchain that goes down to this function have other places
> that assume they will never see an empty string, like this function
> does, which I _think_ is the real issue?
>
> in one of the previous messages.
Agreed.
> But is
>
> echo ./ >.gitignore
>
> a way to say "ignore everything in the tree by default" in the first
> place? I think historically that has never been the case, I recall
> that the list had discussions on that topic in the past (it may be
> before you appeared here), and I do not recall we reached a concensus
> that we should make it mean that nor applied a patch to do so.
Good question. './' and '/' both seem like reasonable values here,
but if noone screamed loud enough for a decision and implementation
yet, it reinforces my description of it as a corner case, and suggests
that we probably shouldn't spend much more time on this thread of
diminishing returns ;-)
^ permalink raw reply
* Re: [PATCH] Documentation/githooks: Explain pre-rebase parameters
From: W. Trevor King @ 2013-02-20 16:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <7vk3q45dg2.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]
On Tue, Feb 19, 2013 at 11:08:29AM -0800, Junio C Hamano wrote:
> "W. Trevor King" <wking@tremily.us> writes:
> > Also, it appears that the `git-rebase--*.sh` handlers don't use the
> > pre-rebase hook. Is this intentional?
>
> The codeflow of git-rebase front-end, when you start rebasing, will
> call run_pre_rebase_hook before calling run_specific_rebase. It
> will be redundant for handlers to then call it again, no?
>
> In "rebase --continue" and later steps, you would not want to see
> the hook trigger.
Ah, that makes sense.
> > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> > index b9003fe..bc837c6 100644
> > --- a/Documentation/githooks.txt
> > +++ b/Documentation/githooks.txt
> > @@ -140,9 +140,10 @@ the outcome of 'git commit'.
> > pre-rebase
> > ~~~~~~~~~~
> >
> > -This hook is called by 'git rebase' and can be used to prevent a branch
> > -from getting rebased.
> > -
> > +This hook is called by 'git rebase' and can be used to prevent a
> > +branch from getting rebased. The hook takes two parameters: the
> > +upstream the series was forked from and the branch being rebased. The
> > +second parameter will be empty when rebasing the current branch.
>
> Technically this is incorrect.
>
> We call it with one or two parameters, and sometimes the second
> parameter is _missing_, which is different from calling with an
> empty string. For a script written in some scripting languages like
> shell and perl, the distinction may not matter (i.e. $2 and $ARGV[1]
> will be an empty string when stringified) but not all (accessing
> sys.argv[2] may give you an IndexError in Python).
Will fix in v2.
Since $upstream_arg will always be set, would it make sense to change
the `${1+"$@"}` syntax in run_pre_rebase_hook() to a plain "$@"?
Cheers,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: Credentials and the Secrets API...
From: Ted Zlatanov @ 2013-02-20 17:01 UTC (permalink / raw)
To: git
In-Reply-To: <CAEBDL5VQxhnL+wdkf_5=MmG4ptBr4TFyyAvbMWxRom9SRxJ6Lg@mail.gmail.com>
On Sat, 9 Feb 2013 05:58:47 -0500 John Szakmeister <john@szakmeister.net> wrote:
JS> On Thu, Feb 7, 2013 at 9:46 AM, Ted Zlatanov <tzz@lifelogs.com> wrote:
>> On Thu, 27 Oct 2011 12:05:03 -0400 John Szakmeister <john@szakmeister.net> wrote:
>>
JS> Just wanted to keep folks in the loop. It turns out that the Secrets
JS> API is still to young. I asked about the format to store credentials
JS> in (as far as attributes), and got a response from a KDE developer
JS> that says it's still to young on their front. They hope to have
JS> support in the next release of KDE. But there's still the issue of
JS> what attributes to use.
>> Do you think the Secrets API has matured enough? KDE has had a new
>> release since your post...
JS> Yes, I think it has. Several other applications appear to be using
JS> it, including some things considered "core" in Fedora--which is a good
JS> sign.
Wonderful. Do you still have interest in working on this credential?
Ted
^ permalink raw reply
* Re: [PATCH] Documentation/githooks: Explain pre-rebase parameters
From: Junio C Hamano @ 2013-02-20 17:23 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git
In-Reply-To: <20130220163621.GI14102@odin.tremily.us>
"W. Trevor King" <wking@tremily.us> writes:
> Since $upstream_arg will always be set, would it make sense to change
> the `${1+"$@"}` syntax in run_pre_rebase_hook() to a plain "$@"?
I suspect that there no longer is a need for ${1+"$@"} in today's
world even when you do not have arguments, and it certainly is fine
if you want to update that particular instance in the function with
a single caller that calls it with 1 or 2 arguments, especially if
you are updating the code in the vicinity.
I however do not think it is worth blindly replacing them tree-wide
just for the sake of changing them. The upside of helping beginning
shell programers by possibly better readability does not look great,
compared to the downside of possibly breaking somebody who is still
on a broken shell that the old idiom is still helping.
^ permalink raw reply
* Re: [RFC] Provide a mechanism to turn off symlink resolution in ceiling paths
From: Junio C Hamano @ 2013-02-20 17:41 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Anders Kaseorg, David Aguilar, Jiang Xin, Lea Wiemann,
David Reiss, Johannes Sixt, Lars R. Damerow, Jeff King,
Marc Jordan
In-Reply-To: <1361351364-15479-1-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> This is a possible implementation (untested!) of Junio's suggestion,
> with the slight twist that the empty entry can appear anywhere in
> GIT_CEILING_DIRECTORIES and only turns off symlink expansion for
> subsequent entries.
Sounds like a good way to go, I think.
Originally I thought that checking with the elements literally and
stop when we have a match, and then falling back to call realpath()
on them to compare, might be a solution, but I do not think it is.
I haven't thought things through to convince myself that is the best
approach to require the users to explicitly tell us to omit calls to
realpath(). Perhaps it is the best we could do, but there might be
even better ways somebody can think up.
^ permalink raw reply
* Merge with staged and unstaged changes
From: Edward Thomson @ 2013-02-20 19:17 UTC (permalink / raw)
To: git@vger.kernel.org
Hi-
I've been investigating the cases where merge is allowed to proceed when
there are staged changes in the index or unstaged files in the working
directory. There are cases where I find the behavior surprising and I
hope I can get clarification. There are also two cases that I will report
as bugs, where it appears that the unstaged file contents are deleted.
For these cases below, please consider the contents of a single path.
In the tables below, we will show the contents of a file across each input
and output of the merge - consider that we're merging a single file in
some branch "theirs" into the current branch "ours" and that these two
branches have a common ancestor "anc". The state of that file in our
index and workdir are represented by "idx" and "wd", respectively.
Unless otherwise noted, these cases are true for both git-merge-resolve
and git-merge-recursive.
For completeness and illustration purposes, I'll included the cases where
there are no changes staged or unstaged. These succeed, as expected:
input result
anc ours theirs idx wd merge result idx wd
1 A A B A A take B B B
2 A B A B B take A A A
Merge is also expected to proceed if the contents of our branch are the
merge result, and there are unstaged changes for that file in the workdir.
In this case, the file remains unstaged:
input result
anc ours theirs idx wd merge result idx wd
3 A B A B C take B B C
What was surprising to me was that my merge can proceed if I stage a change
that is identical to the merge result. That is, if my merge result would
be to take the contents from "theirs", then my merge can proceed if I've
already staged the same contents:
input result
anc ours theirs idx wd merge result idx wd
4 A A B B B take B B B
5 A A B B C take B B C
This seems unexpected - is there a use-case that this enables or is
this accidental?
Another surprising result was that if I have deleted a file (and staged
the deletion or not) then the merge will proceed and the file in question
will be recreated. Consider "X" to be a missing file:
input result
anc ours theirs idx wd merge result idx wd
6 A A B A X take B B B
7 A A B X X take B B B
I wouldn't have expected a file I deleted to be recreated with the other
branch's contents. Is this behavior also intentional?
Finally, there are cases when you have staged a deletion of the file and
you have unstaged changes in your workdir where the merge will silently
delete the unstaged data. If there is a conflict, the xdiff output will
overwrite the unstaged file:
input result
anc ours theirs idx wd merge result idx wd
8 A B C X D conflict X diff3_file
And similarly, while git-merge-recursive (only) will also remove my
untracked file when there are no changes in our branch but the file was
deleted in their branch:
input result
anc ours theirs idx wd merge result idx wd
9 A A X X B delete file X X
I trust the last two cases, where data is lost, are bugs to report, but
could I get clarification on the other situations?
Thanks-
-ed
^ permalink raw reply
* [PATCHv3 0/19] pkt-line cleanups and fixes
From: Jeff King @ 2013-02-20 19:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
Here's another round of my pkt-line fixes. The more I dug, the more
interesting corners I found. :)
There are really several potentially independent topics rolled together
here. There are dependencies between some of them, so I tried to float
the most independent and non-controversial bits to the beginning. We may
want those as a separate topic to merge sooner, and have the rest as a
topic build on top.
Overall, the diffstat shows a reduction in lines (and I even added a few
dozen lines of comments), which is nice. The intent was to fix some bugs
and corner cases, but I found a lot of cleanup opportunities in the
middle.
builtin/archive.c | 17 ++--
builtin/fetch-pack.c | 11 +-
builtin/receive-pack.c | 10 +-
builtin/send-pack.c | 4 +-
builtin/upload-archive.c | 45 +++------
cache.h | 4 +-
connect.c | 13 +--
daemon.c | 4 +-
fetch-pack.c | 18 ++--
http-backend.c | 8 +-
http.c | 1 +
pkt-line.c | 126 ++++++++++-------------
pkt-line.h | 72 +++++++++++++-
remote-curl.c | 188 ++++++++++++++++-------------------
send-pack.c | 22 ++--
sideband.c | 11 +-
sideband.h | 3 -
t/t5503-tagfollow.sh | 38 ++++---
t/t5700-clone-reference.sh | 10 +-
transport.c | 6 +-
upload-pack.c | 40 +++-----
write_or_die.c | 19 ++--
22 files changed, 321 insertions(+), 349 deletions(-)
The patches are:
[01/19]: upload-pack: use get_sha1_hex to parse "shallow" lines
New in this round; fixes a potential interoperability problem.
[02/19]: upload-pack: do not add duplicate objects to shallow list
New. Fixes a potential memory-consumption denial-of-service.
[03/19]: upload-pack: remove packet debugging harness
New. Optional cleanup, but later patches textually depend on it.
[04/19]: fetch-pack: fix out-of-bounds buffer offset in get_ack
New. Fixes a potential interoperability problem.
[05/19]: send-pack: prefer prefixcmp over memcmp in receive_status
New. Optional cleanup.
[06/19]: upload-archive: do not copy repo name
[07/19]: upload-archive: use argv_array to store client arguments
New. Optional cleanup.
[08/19]: write_or_die: raise SIGPIPE when we get EPIPE
[09/19]: pkt-line: move a misplaced comment
[10/19]: pkt-line: drop safe_write function
The latter two were in the last round; but it's 08/19 that makes
doing 10/19 safe. I think it's also a sane thing to be doing in
general for existing callers of write_or_die.
These can really be pulled into a separate topic if we want, as
there isn't even a lot of textual dependency.
[11/19]: pkt-line: provide a generic reading function with options
This is an alternative to the proliferation of different reading
functions that round 2 had. I think it ends up cleaner. It also
addresses Jonathan's function-signature concerns.
[12/19]: pkt-line: teach packet_read_line to chomp newlines
New. A convenience cleanup that drops a lot of lines. Technically
optional, but later patches depend heavily on it (textually, and for
splitting line-readers from binary-readers).
[13/19]: pkt-line: move LARGE_PACKET_MAX definition from sideband
[14/19]: pkt-line: provide a LARGE_PACKET_MAX static buffer
New. Another cleanup that makes packet_read_line callers a bit
simpler, and bumps the packet size limits throughout git, as we
discussed.
[15/19]: pkt-line: share buffer/descriptor reading implementation
[16/19]: teach get_remote_heads to read from a memory buffer
[17/19]: remote-curl: pass buffer straight to get_remote_heads
These are more or less ported from v2's patches 6-8, except that the
earlier pkt-line changes make the first one way more pleasant.
[18/19]: remote-curl: move ref-parsing code up in file
[19/19]: remote-curl: always parse incoming refs
...and the yak is shaved. More or less a straight rebase of their v2
counterparts, and the thing that actually started me on this topic.
I know it's a big series, but I tried hard to break it down into
bite-sized chunks. Thanks for your reviewing patience.
-Peff
^ permalink raw reply
* [PATCH v3 01/19] upload-pack: use get_sha1_hex to parse "shallow" lines
From: Jeff King @ 2013-02-20 19:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
When we receive a line like "shallow <sha1>" from the
client, we feed the <sha1> part to get_sha1. This is a
mistake, as the argument on a shallow line is defined by
Documentation/technical/pack-protocol.txt to contain an
"obj-id". This is never defined in the BNF, but it is clear
from the text and from the other uses that it is meant to be
a hex sha1, not an arbitrary identifier (and that is what
fetch-pack has always sent).
We should be using get_sha1_hex instead, which doesn't allow
the client to request arbitrary junk like "HEAD@{yesterday}".
Because this is just marking shallow objects, the client
couldn't actually do anything interesting (like fetching
objects from unreachable reflog entries), but we should keep
our parsing tight to be on the safe side.
Because get_sha1 is for the most part a superset of
get_sha1_hex, in theory the only behavior change should be
disallowing non-hex object references. However, there is
one interesting exception: get_sha1 will only parse
a 40-character hex sha1 if the string has exactly 40
characters, whereas get_sha1_hex will just eat the first 40
characters, leaving the rest. That means that current
versions of git-upload-pack will not accept a "shallow"
packet that has a trailing newline, even though the protocol
documentation is clear that newlines are allowed (even
encouraged) in non-binary parts of the protocol.
This never mattered in practice, though, because fetch-pack,
contrary to the protocol documentation, does not include a
newline in its shallow lines. JGit follows its lead (though
it correctly is strict on the parsing end about wanting a
hex object id).
We do not adjust fetch-pack to send newlines here, as it
would break communication with older versions of git (and
there is no actual benefit to doing so, except for
consistency with other parts of the protocol).
Signed-off-by: Jeff King <peff@peff.net>
---
I couldn't trigger anything interestingly malicious from this, but I
didn't look very hard. Maybe somebody who knows the shallow protocol
better could think of something clever (not that it matters much).
upload-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/upload-pack.c b/upload-pack.c
index 30146a0..b058e8d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -596,7 +596,7 @@ static void receive_needs(void)
if (!prefixcmp(line, "shallow ")) {
unsigned char sha1[20];
struct object *object;
- if (get_sha1(line + 8, sha1))
+ if (get_sha1_hex(line + 8, sha1))
die("invalid shallow line: %s", line);
object = parse_object(sha1);
if (!object)
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 02/19] upload-pack: do not add duplicate objects to shallow list
From: Jeff King @ 2013-02-20 19:54 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
When the client tells us it has a shallow object via
"shallow <sha1>", we make sure we have the object, mark it
with a flag, then add it to a dynamic array of shallow
objects. This means that a client can get us to allocate
arbitrary amounts of memory just by flooding us with shallow
lines (whether they have the objects or not). You can
demonstrate it easily with:
yes '0035shallow e83c5163316f89bfbde7d9ab23ca2e25604af290' |
git-upload-pack git.git
We already protect against duplicates in want lines by
checking if our flag is already set; let's do the same thing
here. Note that a client can still get us to allocate some
amount of memory by marking every object in the repo as
"shallow" (or "want"). But this at least bounds it with the
number of objects in the repository, which is not under the
control of an upload-pack client.
Signed-off-by: Jeff King <peff@peff.net>
---
Looking over upload-pack, I think this is the only "consume arbitrary
memory" spot. Since you can convince git to go to quite a bit of work
just processing a big repo, the distinction may not be important, but
drawing the line between "large" and "arbitrarily large" seemed
reasonable to me (and it's a trivial fix).
upload-pack.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index b058e8d..1aee407 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -603,8 +603,10 @@ static void receive_needs(void)
die("did not find object for %s", line);
if (object->type != OBJ_COMMIT)
die("invalid shallow object %s", sha1_to_hex(sha1));
- object->flags |= CLIENT_SHALLOW;
- add_object_array(object, NULL, &shallows);
+ if (!(object->flags & CLIENT_SHALLOW)) {
+ object->flags |= CLIENT_SHALLOW;
+ add_object_array(object, NULL, &shallows);
+ }
continue;
}
if (!prefixcmp(line, "deepen ")) {
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 03/19] upload-pack: remove packet debugging harness
From: Jeff King @ 2013-02-20 19:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
If you set the GIT_DEBUG_SEND_PACK environment variable,
upload-pack will dump lines it receives in the receive_needs
phase to a descriptor. This debugging harness is a strict
subset of what GIT_TRACE_PACKET can do. Let's just drop it
in favor of that.
A few tests used GIT_DEBUG_SEND_PACK to confirm which
objects get sent; we have to adapt them to the new output
format.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5503-tagfollow.sh | 38 +++++++++++++++++---------------------
t/t5700-clone-reference.sh | 10 +++++-----
upload-pack.c | 9 ---------
3 files changed, 22 insertions(+), 35 deletions(-)
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 60de2d6..d181c96 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -5,7 +5,7 @@ if ! test_have_prereq NOT_MINGW; then
. ./test-lib.sh
if ! test_have_prereq NOT_MINGW; then
- say "GIT_DEBUG_SEND_PACK not supported - skipping tests"
+ say "GIT_TRACE_PACKET not supported - skipping tests"
fi
# End state of the repository:
@@ -42,21 +42,26 @@ test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
test_expect_success NOT_MINGW 'setup expect' '
cat - <<EOF >expect
-#S
want $A
-#E
EOF
'
+get_needs () {
+ perl -alne '
+ next unless $F[1] eq "upload-pack<";
+ last if $F[2] eq "0000";
+ print $F[2], " ", $F[3];
+ ' "$@"
+}
+
test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=3 git fetch 3>../$U &&
test $A = $(git rev-parse --verify origin/master)
) &&
- test -s $U &&
- cut -d" " -f1,2 $U >actual &&
+ get_needs $U >actual &&
test_cmp expect actual
'
@@ -74,10 +79,8 @@ want $T
test_expect_success NOT_MINGW 'setup expect' '
cat - <<EOF >expect
-#S
want $C
want $T
-#E
EOF
'
@@ -85,13 +88,12 @@ test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=3 git fetch 3>../$U &&
test $C = $(git rev-parse --verify origin/cat) &&
test $T = $(git rev-parse --verify tag1) &&
test $A = $(git rev-parse --verify tag1^0)
) &&
- test -s $U &&
- cut -d" " -f1,2 $U >actual &&
+ get_needs $U >actual &&
test_cmp expect actual
'
@@ -113,10 +115,8 @@ want $S
test_expect_success NOT_MINGW 'setup expect' '
cat - <<EOF >expect
-#S
want $B
want $S
-#E
EOF
'
@@ -124,22 +124,19 @@ want $S
rm -f $U &&
(
cd cloned &&
- GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=3 git fetch 3>../$U &&
test $B = $(git rev-parse --verify origin/master) &&
test $B = $(git rev-parse --verify tag2^0) &&
test $S = $(git rev-parse --verify tag2)
) &&
- test -s $U &&
- cut -d" " -f1,2 $U >actual &&
+ get_needs $U >actual &&
test_cmp expect actual
'
test_expect_success NOT_MINGW 'setup expect' '
cat - <<EOF >expect
-#S
want $B
want $S
-#E
EOF
'
@@ -151,15 +148,14 @@ test_expect_success NOT_MINGW 'new clone fetch master and tags' '
cd clone2 &&
git init &&
git remote add origin .. &&
- GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=3 git fetch 3>../$U &&
test $B = $(git rev-parse --verify origin/master) &&
test $S = $(git rev-parse --verify tag2) &&
test $B = $(git rev-parse --verify tag2^0) &&
test $T = $(git rev-parse --verify tag1) &&
test $A = $(git rev-parse --verify tag1^0)
) &&
- test -s $U &&
- cut -d" " -f1,2 $U >actual &&
+ get_needs $U >actual &&
test_cmp expect actual
'
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index c47d450..9cd3b4d 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -55,10 +55,10 @@ test_expect_success 'fetched no objects' \
rm -f "$U.D"
test_expect_success 'cloning with reference (no -l -s)' \
-'GIT_DEBUG_SEND_PACK=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
+'GIT_TRACE_PACKET=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
test_expect_success 'fetched no objects' \
-'! grep "^want" "$U.D"'
+'! grep " want" "$U.D"'
cd "$base_dir"
@@ -173,12 +173,12 @@ test_expect_success 'fetch with incomplete alternates' '
(
cd K &&
git remote add J "file://$base_dir/J" &&
- GIT_DEBUG_SEND_PACK=3 git fetch J 3>"$U.K"
+ GIT_TRACE_PACKET=3 git fetch J 3>"$U.K"
) &&
master_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/master) &&
- ! grep "^want $master_object" "$U.K" &&
+ ! grep " want $master_object" "$U.K" &&
tag_object=$(cd A && git for-each-ref --format="%(objectname)" refs/tags/HEAD) &&
- ! grep "^want $tag_object" "$U.K"
+ ! grep " want $tag_object" "$U.K"
'
test_done
diff --git a/upload-pack.c b/upload-pack.c
index 1aee407..63cea91 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -42,7 +42,6 @@ static int use_sideband;
* otherwise maximum packet size (up to 65520 bytes).
*/
static int use_sideband;
-static int debug_fd;
static int advertise_refs;
static int stateless_rpc;
@@ -580,8 +579,6 @@ static void receive_needs(void)
int has_non_tip = 0;
shallow_nr = 0;
- if (debug_fd)
- write_str_in_full(debug_fd, "#S\n");
for (;;) {
struct object *o;
const char *features;
@@ -590,8 +587,6 @@ static void receive_needs(void)
reset_timeout();
if (!len)
break;
- if (debug_fd)
- write_in_full(debug_fd, line, len);
if (!prefixcmp(line, "shallow ")) {
unsigned char sha1[20];
@@ -653,8 +648,6 @@ static void receive_needs(void)
add_object_array(o, NULL, &want_obj);
}
}
- if (debug_fd)
- write_str_in_full(debug_fd, "#E\n");
/*
* We have sent all our refs already, and the other end
@@ -845,8 +838,6 @@ int main(int argc, char **argv)
if (is_repository_shallow())
die("attempt to fetch/clone from a shallow repository");
git_config(upload_pack_config, NULL);
- if (getenv("GIT_DEBUG_SEND_PACK"))
- debug_fd = atoi(getenv("GIT_DEBUG_SEND_PACK"));
upload_pack();
return 0;
}
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 04/19] fetch-pack: fix out-of-bounds buffer offset in get_ack
From: Jeff King @ 2013-02-20 20:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
When we read acks from the remote, we expect either:
ACK <sha1>
or
ACK <sha1> <multi-ack-flag>
We parse the "ACK <sha1>" bit from the line, and then start
looking for the flag strings at "line+45"; if we don't have
them, we assume it's of the first type. But if we do have
the first type, then line+45 is not necessarily inside our
string at all!
It turns out that this works most of the time due to the way
we parse the packets. They should come in with a newline,
and packet_read puts an extra NUL into the buffer, so we end
up with:
ACK <sha1>\n\0
with the newline at offset 44 and the NUL at offset 45. We
then strip the newline, putting a NUL at offset 44. So
when we look at "line+45", we are looking past the end of
our string; but it's OK, because we hit the terminator from
the original string.
This breaks down, however, if the other side does not
terminate their packets with a newline. In that case, our
packet is one character shorter, and we start looking
through uninitialized memory for the flag. No known
implementation sends such a packet, so it has never come up
in practice.
This patch tightens the check by looking for a short,
flagless ACK before trying to parse the flag.
Signed-off-by: Jeff King <peff@peff.net>
---
This is the absolute minimal fix, which just checks for the no-flag case
early; we still treat arbitrary crud in the flag field as just an ACK.
From my understanding of the protocol, a saner parsing scheme would be:
const char *flag = line + 44; /* we already parsed "ACK <sha1>" */
if (!*flag)
return ACK;
if (!strcmp(flag, " continue"))
return ACK_continue;
if (!strcmp(flag, " common"))
return ACK_continue;
if (!strcmp(flag, " ready"))
return ACK_ready;
die("fetch-pack expected multi-ack flag, got: %s", line);
But that is much tighter, and I wasn't sure if the looseness was there
to facilitate future expansion or something (though I'd think we would
need a new capability for that).
fetch-pack.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fetch-pack.c b/fetch-pack.c
index 6d8926a..27a3e80 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -226,6 +226,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
return NAK;
if (!prefixcmp(line, "ACK ")) {
if (!get_sha1_hex(line+4, result_sha1)) {
+ if (len < 45)
+ return ACK;
if (strstr(line+45, "continue"))
return ACK_continue;
if (strstr(line+45, "common"))
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 05/19] send-pack: prefer prefixcmp over memcmp in receive_status
From: Jeff King @ 2013-02-20 20:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
This code predates prefixcmp, so it used memcmp along with
static sizes. Replacing these memcmps with prefixcmp makes
the code much more readable, and the lack of static sizes
will make refactoring it in future patches simpler.
Note that we used to be unnecessarily liberal in parsing the
"unpack" status line, and would accept "unpack ok\njunk". No
version of git has ever produced that, and it violates the
BNF in Documentation/technical/pack-protocol.txt. Let's take
this opportunity to tighten the check by converting the
prefix comparison into a strcmp.
While we're in the area, let's also fix a vague error
message that does not follow our usual conventions (it
writes directly to stderr and does not use the "error:"
prefix).
Signed-off-by: Jeff King <peff@peff.net>
---
send-pack.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 97ab336..e91cbe2 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -109,9 +109,9 @@ static int receive_status(int in, struct ref *refs)
char line[1000];
int ret = 0;
int len = packet_read_line(in, line, sizeof(line));
- if (len < 10 || memcmp(line, "unpack ", 7))
+ if (prefixcmp(line, "unpack "))
return error("did not receive remote status");
- if (memcmp(line, "unpack ok\n", 10)) {
+ if (strcmp(line, "unpack ok\n")) {
char *p = line + strlen(line) - 1;
if (*p == '\n')
*p = '\0';
@@ -125,9 +125,8 @@ static int receive_status(int in, struct ref *refs)
len = packet_read_line(in, line, sizeof(line));
if (!len)
break;
- if (len < 3 ||
- (memcmp(line, "ok ", 3) && memcmp(line, "ng ", 3))) {
- fprintf(stderr, "protocol error: %s\n", line);
+ if (prefixcmp(line, "ok ") && prefixcmp(line, "ng ")) {
+ error("invalid ref status from remote: %s", line);
ret = -1;
break;
}
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 06/19] upload-archive: do not copy repo name
From: Jeff King @ 2013-02-20 20:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
According to the comment, enter_repo will modify its input.
However, this has not been the case since 1c64b48
(enter_repo: do not modify input, 2011-10-04). Drop the
now-useless copy.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/upload-archive.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index b928beb..c3d134e 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -27,13 +27,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
if (argc != 2)
usage(upload_archive_usage);
- if (strlen(argv[1]) + 1 > sizeof(buf))
- die("insanely long repository name");
-
- strcpy(buf, argv[1]); /* enter-repo smudges its argument */
-
- if (!enter_repo(buf, 0))
- die("'%s' does not appear to be a git repository", buf);
+ if (!enter_repo(argv[1], 0))
+ die("'%s' does not appear to be a git repository", argv[1]);
/* put received options in sent_argv[] */
sent_argc = 1;
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 07/19] upload-archive: use argv_array to store client arguments
From: Jeff King @ 2013-02-20 20:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
The current parsing scheme for upload-archive is to pack
arguments into a fixed-size buffer, separated by NULs, and
put a pointer to each argument in the buffer into a
fixed-size argv array.
This works fine, and the limits are high enough that nobody
reasonable is going to hit them, but it makes the code hard
to follow. Instead, let's just stuff the arguments into an
argv_array, which is much simpler. That lifts the "all
arguments must fit inside 4K together" limit.
We could also trivially lift the MAX_ARGS limitation (in
fact, we have to keep extra code to enforce it). But that
would mean a client could force us to allocate an arbitrary
amount of memory simply by sending us "argument" lines. By
limiting the MAX_ARGS, we limit an attacker to about 4
megabytes (64 times a maximum 64K packet buffer). That may
sound like a lot compared to the 4K limit, but it's not a
big deal compared to what git-archive will actually allocate
while working (e.g., to load blobs into memory). The
important thing is that it is bounded.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/upload-archive.c | 35 ++++++++++++++---------------------
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index c3d134e..3393cef 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -7,6 +7,7 @@
#include "pkt-line.h"
#include "sideband.h"
#include "run-command.h"
+#include "argv-array.h"
static const char upload_archive_usage[] =
"git upload-archive <repo>";
@@ -18,10 +19,9 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
{
- const char *sent_argv[MAX_ARGS];
+ struct argv_array sent_argv = ARGV_ARRAY_INIT;
const char *arg_cmd = "argument ";
- char *p, buf[4096];
- int sent_argc;
+ char buf[4096];
int len;
if (argc != 2)
@@ -31,33 +31,26 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
die("'%s' does not appear to be a git repository", argv[1]);
/* put received options in sent_argv[] */
- sent_argc = 1;
- sent_argv[0] = "git-upload-archive";
- for (p = buf;;) {
+ argv_array_push(&sent_argv, "git-upload-archive");
+ for (;;) {
/* This will die if not enough free space in buf */
- len = packet_read_line(0, p, (buf + sizeof buf) - p);
+ len = packet_read_line(0, buf, sizeof(buf));
if (len == 0)
break; /* got a flush */
- if (sent_argc > MAX_ARGS - 2)
- die("Too many options (>%d)", MAX_ARGS - 2);
+ if (sent_argv.argc > MAX_ARGS)
+ die("Too many options (>%d)", MAX_ARGS - 1);
- if (p[len-1] == '\n') {
- p[--len] = 0;
+ if (buf[len-1] == '\n') {
+ buf[--len] = 0;
}
- if (len < strlen(arg_cmd) ||
- strncmp(arg_cmd, p, strlen(arg_cmd)))
- die("'argument' token or flush expected");
- len -= strlen(arg_cmd);
- memmove(p, p + strlen(arg_cmd), len);
- sent_argv[sent_argc++] = p;
- p += len;
- *p++ = 0;
+ if (prefixcmp(buf, arg_cmd))
+ die("'argument' token or flush expected");
+ argv_array_push(&sent_argv, buf + strlen(arg_cmd));
}
- sent_argv[sent_argc] = NULL;
/* parse all options sent by the client */
- return write_archive(sent_argc, sent_argv, prefix, 0, NULL, 1);
+ return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 1);
}
__attribute__((format (printf, 1, 2)))
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
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