* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Junio C Hamano @ 2016-11-10 0:18 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <20161109231720.luuhezzziuhx4r75@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
>
> I'm slightly confused. Did you mean "supporting any in-tree symlink to
> an out-of-tree destination" in your first sentence?
I was trying to say that these "control files used solely by git"
have no business being a symbolic link pointing at anywhere, even
inside the same tree; actually, especially if it is inside the same
tree.
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Jeff King @ 2016-11-10 0:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <xmqqoa1o6vca.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 09, 2016 at 04:18:29PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
> >
> > I'm slightly confused. Did you mean "supporting any in-tree symlink to
> > an out-of-tree destination" in your first sentence?
>
> I was trying to say that these "control files used solely by git"
> have no business being a symbolic link pointing at anywhere, even
> inside the same tree; actually, especially if it is inside the same
> tree.
OK. That is what my patch does (modulo .gitmodules, which I did not
think of). But I think that is the opposite of Duy's opinion, as his
review seemed to object to that.
As you know my ulterior motive is dealing with malicious out-of-tree
symlinks, and I would be happy to deal with that directly. That still
leaves symlinked ".gitmodules" etc in a funny state (they work in the
filesystem but not in the index), but since nobody is _complaining_,
it's a bug we could leave for another day.
So I dunno.
-Peff
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-09 23:40 UTC (permalink / raw)
To: Jeff King; +Cc: git, Johannes Schindelin, Lars Schneider
In-Reply-To: <20161108004038.a7gyoe6wpucxjmvz@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think we also need to make a final decision on the indent/compaction
> heuristic naming. After reading Michael's [0], and the claim by you and
> Stefan that the "compaction" name was declared sufficiently experimental
> that we could later take it away, I'm inclined to follow this plan:
>
> 1. Ship v2.11 with what is in master; i.e., both compaction and indent
> heuristics, triggerable by config or command line.
>
> 2. Post-v2.11, retire the compaction heuristic as a failed experiment.
> Keeping it in v2.11 doesn't hurt anything (it was already
> released), and lets us take our time coming up with and cooking the
> patch.
>
> 3. Post-v2.11, flip the default for diff.indentHeuristic to "true".
> Keep at least the command line option around indefinitely for
> experimenting (i.e., "this diff looks funny; I wonder if
> --no-indent-heuristic makes it look better").
>
> Config option can either stay or go at that point. I have no
> preference.
>
> The nice thing about that plan is it punts on merging any new code to
> post-v2.11. :)
OK, I can go with that plan. Thanks for an outline.
^ permalink raw reply
* Re: [PATCH v1 0/2] Fix default macOS build locally and on Travis CI
From: Junio C Hamano @ 2016-11-09 23:39 UTC (permalink / raw)
To: larsxschneider; +Cc: git, tboegi
In-Reply-To: <20161017002550.88782-1-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Apple removed the OpenSSL header files in macOS and therefore Git does
> not build out of the box on macOS anymore. See previous discussion with
> Torsten here: http://public-inbox.org/git/565B3036.8000604@web.de/
>
> This mini series makes Git build out of the box on macOS, again, and
> disables the HTTPD tests on macOS TravisCI as they don't work anymore
> with the new macOS TravisCI default image:
> https://blog.travis-ci.com/2016-10-04-osx-73-default-image-live/
>
> Thanks,
> Lars
>
>
> Lars Schneider (2):
> config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11
> travis-ci: disable GIT_TEST_HTTPD for macOS
>
> .travis.yml | 3 ++-
> config.mak.uname | 6 ++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
I've followed what was available at the public-inbox archive, but it
is unclear what the conclusion was.
For the first one your "how about" non-patch, to which Peff said
"that's simple and good", looked good to me as well, but is it
available as a final patch that I can just take and apply (otherwise
I think I can do the munging myself, but I'd rather be spoon-fed
when able ;-).
I do not have a strong opinion on the second one. For an interim
solution, disabling webserver tests certainly is expedite and safe,
so I am fine taking it as-is, but I may have missed strong
objections.
Thanks.
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-09 23:35 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, git, Johannes Schindelin, Lars Schneider
In-Reply-To: <20161109165125.t4x2w7u5uxe57xm2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> It's just that I found myself writing up notes like "this should be
> merged", and it occurred to me that I could communicate the same things
> by sending you a proposed history. So I'm curious if you find dissecting
> it via "git log" more or less convenient than a list of notes. :)
Yes, I think this is a very readable form of communication.
What's especially nice in what you did is that comparing outputs of
these two commands
$ git log --no-merges --oneline master..peff/for-junio/master
$ git log --no-merges --oneline ^pu master..peff/for-junio/master
clearly shows that you reused what I already had in 'pu' and the
ones missing from 'pu' are the ones I need to check and possibly
may want to sign-off myself.
I also need to note that while this is very handy format for the
recipient, hence a very good way to do "pass the pumpkin" between
maintainers, it is a less "open" way of communication than laying
out everything on the list (cf. http://youtu.be/L8OOzaqS37s), but
it is the most appropriate method in this case, I would think.
> It looks like Johannes prefers replacements for some of the fixes from
> yesterday.
As to that change, I agree with you that I do not care too deeply
about the shape of an interim step as long as the finished product
uses the better one between the alternatives, but at the same time,
because we are including it as a hot-fix in a released product, even
though it is an interim step in the bigger picture, I want it to be
using the better one, too. So I am leaning towards taking what you
queued for me for the reasons you stated in the other thread [*1*].
That would probably mean Dscho needs a bit of rebase in the
remaining parts of his series that adds new elements to the enum,
but I am hoping that he can afford that time now in the feature
freeze period. If we take the _INVALID thing instead now, the
remaining series from Dscho that we would review and queue for the
next cycle would need to begin with a "fixup" patch that stops doing
the _INVALID thing and instead using the "compare after casting to
size_t", so it is just the matter of doing the adjustment before or
after the remainder of the series are posted on the list for the
next release, and the smaller number of "oops that was not nice,
let's fix it" patches in the published history, the better the long
term health of the project, I would think.
The other fixes in your collection looked all sensible. Thanks.
[Footnote]
*1* The convention of keeping _INVALID at the end can be arguably
made safe from future programmer screw-ups, as long as it is
guarded by a good comment nearby. But the solution to cast
would avoid having to have that potential brittleness in the
first place. I find the other merit of taking care of negative
enum automatically quite attractive, too.
^ permalink raw reply
* Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate
From: Ian Jackson @ 2016-11-09 23:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jacob Keller, Git mailing list, Paul Mackerras
In-Reply-To: <xmqqeg2k8cwz.fsf@gitster.mtv.corp.google.com>
Junio C Hamano writes ("Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate"):
> Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
> > This is not correct, because as I have explained, this should be a
> > per-tree configuration:
>
> I do not have fundamental opposition to make it part of .git/config,
> but the name "gitk.something" or if you are enhancing git-gui at the
> time perhaps "gui.something" would be appropriate.
>
> But it is still silly to have this kind of information that is very
> specific to Gitk in two places, one that is pretty Gitk specific
> that core-git does not know anything about, the other that are part
> of the configuration storage of the core-git. In the longer term,
> it is necessary for them to be accessible from gitk's "Edit ->
> Preferences" mechanism somehow, I would think, rather than forcing
> users to sometimes go to GUI to tweak and sometimes run "git config".
I am proposing to set this configuration setting automatically in
dgit. Other tools that work with particular git tags would do the
same. There would be no need for users to do anything.
Having this as an option in a menu would be quite wrong, because it
would end up with the user and the tooling fighting. This is why I
don't want to put this in gitk's existing config file mechanism.
It would be wrong for dgit to edit the user's gitk config file, for
many reasons.
To put it another way, this setting is a way for a tool like dgit to
communicate with gitk (or other programs which have to make guesses
about how prominently to present certain information to the user).
It's not intended to be a way for users, certainly not non-expert
users, to communicate with gitk.
The way I have structured my proposed patches in gitk would make it
easy to provide a gui option to adjust these settings. Such a gui
option ought to save its value in the gitk config file, and those
values ought to override what comes from `git config'.
But such a system would not obviate the need for a legitimate way for
programs like dgit to communicate with gitk.
Thanks,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Jeff King @ 2016-11-09 23:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <xmqqmvh88dlu.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > Let's err on the safe side and disable symlinks to outside repo by
> > default (or even all symlinks on .gitattributes and .gitignore as the
> > first step)
> >
> > What I learned from my changes in .gitignore is, if we have not
> > forbidden something, people likely find some creative use for it.
>
> Yup. Supporting any symlink in-tree is like requiring Git to be
> used only on symlink-capable filesystems. Not allowing it sounds
> like a very sensible option and unlike true contents, there is no
> downside to give that limitation to things like .git<anything>.
I'm slightly confused. Did you mean "supporting any in-tree symlink to
an out-of-tree destination" in your first sentence?
> Shouldn't we do the same for .gitmodules while we are at it?
Good catch. Though I am inclined to have a flag that just covers all
out-of-tree symlinks, regardless of names.
-Peff
^ permalink raw reply
* Re: [PATCH v5 01/16] Git.pm: add subroutines for commenting lines
From: Junio C Hamano @ 2016-11-09 23:16 UTC (permalink / raw)
To: Jakub Narębski
Cc: Vasco Almeida, git, Jiang Xin,
Ævar Arnfjörð Bjarmason, Jean-Noël AVILA,
David Aguilar
In-Reply-To: <b01d0d90-f87c-f708-5d2a-79f6268b4f44@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
>> I prefer to have like this instead
>>
>> sub prefix_lines {
>> my $prefix = shift;
>> my $string = join("\n", @_);
>> $string =~ s/^/$prefix/mg;
>> return $string;
>> }
>>
>> So both subroutines can take several strings as arguments.
>
> I like the interface, but the implementation looks a bit inefficient.
> Why not simply:
> ...
> If those strings can contain embedded newlines (so that they can be
> called as in Junio example), then your solution is a must-be
>
> sub prefix_lines {
> my $prefix = shift;
> my $string = join("\n", @_);
> $string =~ s/^/$prefix/mg;
> return $string;
> }
>
> Well, nevermind then
OK, so in short, is that a "Reviewed-by:" from you ;-)?
I agree with the conclusion. Thanks for a review.
^ permalink raw reply
* Re: git mergetool indefinite hang on version 2.10+
From: Jeff King @ 2016-11-09 23:13 UTC (permalink / raw)
To: Mike Dacre; +Cc: git
In-Reply-To: <CAPd9ww_B7gCxvSuuBzH9AbnLsOF1bC_2+mfk0sVfLFF7YHWvNA@mail.gmail.com>
On Wed, Nov 09, 2016 at 02:54:59PM -0800, Mike Dacre wrote:
> I have no idea how to debug this. As of git version 2.10.0 git hangs
> indefinitely when I run `git mergetool`, the result is the same if I
> run `git mergetool --tool-help`.
Mergetool is a shell script. Try setting GIT_TRACE=1 in the environment
to see which git programs it's running. Of course it may be
hanging in a non-git program, too. Try:
PATH=$(git --exec-path):$PATH
sh -x $(which git-mergetool) --tool-help
to get a dump from the shell.
> I am on Arch Linux, with all of my software updated to the latest
> versions as of this morning. git 2.9.3 and lower works perfectly.
>
> Any ideas?
If it works on v2.9.3 and the bug reproduces, you should be able to use
"git bisect" to find the commit that introduces the problem. There
weren't a lot of changes to the shell scripts in that time frame, so
just as a guess, it may be related to d323c6b64 (i18n: git-sh-setup.sh:
mark strings for translation, 2016-06-17), which was the only
significant change in that time.
-Peff
^ permalink raw reply
* Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate
From: Junio C Hamano @ 2016-11-09 23:13 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jeff King, Jacob Keller, Git mailing list, Paul Mackerras
In-Reply-To: <22562.65461.845411.29907@chiark.greenend.org.uk>
Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
>> But I think the right
>> place to do so would be Edit -> Preferences menu in Gitk, and the
>> settings will be stored in ~/.gitk or ~/.config/git/gitk or whatever
>> gitk-specific place.
>
> This is not correct, because as I have explained, this should be a
> per-tree configuration:
I do not have fundamental opposition to make it part of .git/config,
but the name "gitk.something" or if you are enhancing git-gui at the
time perhaps "gui.something" would be appropriate.
But it is still silly to have this kind of information that is very
specific to Gitk in two places, one that is pretty Gitk specific
that core-git does not know anything about, the other that are part
of the configuration storage of the core-git. In the longer term,
it is necessary for them to be accessible from gitk's "Edit ->
Preferences" mechanism somehow, I would think, rather than forcing
users to sometimes go to GUI to tweak and sometimes run "git config".
^ permalink raw reply
* Re: [PATCH 0/3] gitk: memory consumption improvements
From: Junio C Hamano @ 2016-11-09 23:04 UTC (permalink / raw)
To: Markus Hitter; +Cc: git@vger.kernel.org, Paul Mackerras
In-Reply-To: <8eac2a5b-071f-6d17-4d81-0744db16910d@jump-ing.de>
Markus Hitter <mah@jump-ing.de> writes:
> Am 08.11.2016 um 22:37 schrieb Junio C Hamano:
>> Are all semi-modern Tcl/Tk in service have this -undo thing so that
>> we can pass unconditionally to the text widget like the patch does?
>
> Good point. As far as my research goes, this flag was introduced in Nov. 2001:
>
> http://core.tcl.tk/tk/info/5265df93d207cec0
Sounds safe enough then ;-)
Thanks for an additional research.
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Junio C Hamano @ 2016-11-09 22:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Git Mailing List
In-Reply-To: <CACsJy8BoEXDjwe=ZX5ZOC_mvaMjYrB3i7wcMmiOP3mm5-rwC5Q@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Let's err on the safe side and disable symlinks to outside repo by
> default (or even all symlinks on .gitattributes and .gitignore as the
> first step)
>
> What I learned from my changes in .gitignore is, if we have not
> forbidden something, people likely find some creative use for it.
Yup. Supporting any symlink in-tree is like requiring Git to be
used only on symlink-capable filesystems. Not allowing it sounds
like a very sensible option and unlike true contents, there is no
downside to give that limitation to things like .git<anything>.
Shouldn't we do the same for .gitmodules while we are at it?
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Junio C Hamano @ 2016-11-09 22:57 UTC (permalink / raw)
To: Josh Triplett
Cc: Jacob Keller, Christian Couder, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <20161106173311.lqoxxgcklx4jlrg7@x>
Josh Triplett <josh@joshtriplett.org> writes:
>> This would definitely need protocol extension when transferring
>> objects across repositories.
>
> It'd also need a repository format extension locally. Otherwise, if you
> ever touched that repository with an older git (or a tool built on an
> older libgit2 or JGit or other library), you could lose data.
True. Thanks for sanity-checking me.
^ permalink raw reply
* git mergetool indefinite hang on version 2.10+
From: Mike Dacre @ 2016-11-09 22:54 UTC (permalink / raw)
To: git
Hi all,
I have no idea how to debug this. As of git version 2.10.0 git hangs
indefinitely when I run `git mergetool`, the result is the same if I
run `git mergetool --tool-help`.
The result is identical regardless of which version of vim I have
installed, or where vim in installed. It is also the same if I remove
my .gitconfig file.
I am on Arch Linux, with all of my software updated to the latest
versions as of this morning. git 2.9.3 and lower works perfectly.
Any ideas?
Thanks,
Mike
^ permalink raw reply
* Re: [PATCHv2 32/36] pathspec: allow querying for attributes
From: Stefan Beller @ 2016-11-09 22:42 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Brandon Williams, Git Mailing List
In-Reply-To: <CACsJy8BKwvaw8CsorZhoYDw6gxoosv0gPRRpdwW87+YMASCVcA@mail.gmail.com>
On Wed, Nov 9, 2016 at 1:57 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Oct 29, 2016 at 1:54 AM, Stefan Beller <sbeller@google.com> wrote:
>> The pathspec mechanism is extended via the new
>> ":(attr:eol=input)pattern/to/match" syntax to filter paths so that it
>> requires paths to not just match the given pattern but also have the
>> specified attrs attached for them to be chosen.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> Documentation/glossary-content.txt | 20 +++++
>> dir.c | 35 ++++++++
>
> Pathspec can be processed in a couple more places. The big two are
> match_pathspec and tree_entry_interesting, the former traverses a list
> while the latter does a tree. You don't have to implement attr
> matching in tree_entry_interesting right now because nobody needs it,
> probably. But you need to make sure if somebody accidentally calls
> tree_entry_interesting with an attr pathspec, then it should
> die("BUG"), not silently ignore attr.
>
I am looking into this now.
^ permalink raw reply
* Re: [PATCH] sequencer: shut up clang warning
From: Jakub Narębski @ 2016-11-09 22:28 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Torsten Bögershausen, Lars Schneider,
Jeff King
In-Reply-To: <efbba4b32515fed7096c1c81dbe97eedd44083b0.1478699713.git.johannes.schindelin@gmx.de>
W dniu 09.11.2016 o 14:56, Johannes Schindelin pisze:
> When comparing a value of type `enum todo_command` with a value that is
> outside the defined enum constants, clang greets the developer with this
> warning:
>
> comparison of constant 2 with expression of type
> 'const enum todo_command' is always true
>
> While this is arguably true *iff* the value was never cast from a
> free-form int, we should keep the cautious code in place.
>
> To shut up clang, we simply introduce an otherwise pointless enum constant
> and compare against that.
>
> Noticed by Torsten Bögershausen.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5fd75f3..f80e9c0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -619,7 +619,8 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
>
> enum todo_command {
> TODO_PICK = 0,
> - TODO_REVERT
> + TODO_REVERT,
> + TODO_INVALID
> };
Why not name it TODO_N, or N_TODO, or something like that?
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v5 01/16] Git.pm: add subroutines for commenting lines
From: Jakub Narębski @ 2016-11-09 22:25 UTC (permalink / raw)
To: Vasco Almeida, Junio C Hamano
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, David Aguilar
In-Reply-To: <1478710943.28771.4.camel@sapo.pt>
W dniu 09.11.2016 o 18:02, Vasco Almeida pisze:
> A Ter, 08-11-2016 às 17:06 -0800, Junio C Hamano escreveu:
>> Vasco Almeida <vascomalmeida@sapo.pt> writes:
>>> +sub comment_lines {
>>> + my $comment_line_char = config("core.commentchar") || '#';
>>> + return prefix_lines("$comment_line_char ", @_);
>>> +}
>>> +
>>
>> This makes it appear as if comment_lines can take arbitrary number
>> of strings as its arguments (because the outer caller just passes @_
>> thru), but in fact because prefix_lines ignores anything other than
>> $_[0] and $_[1], only the first parameter given to comment_lineS sub
>> is inspected for lines in it and the prefix-char prefixed at the
>> beginning of each of them.
>>
>> Which is not a great interface, as it is quite misleading.
>>
>> Perhaps
>>
>> prefix_lines("#", join("\n", @_));
>>
>> or something like that may make it less confusing.
>
> I prefer to have like this instead
>
> sub prefix_lines {
> my $prefix = shift;
> my $string = join("\n", @_);
> $string =~ s/^/$prefix/mg;
> return $string;
> }
>
> So both subroutines can take several strings as arguments.
I like the interface, but the implementation looks a bit inefficient.
Why not simply:
sub prefix_lines {
my $prefix = shift;
return "$prefix" . join("\n$prefix", @_) . "\n";
}
That is, if we can assume that those lines are not terminated by
newlines themselves.
If they can be (but cannot have embedded newlines), then
sub prefix_lines {
my $prefix = shift;
return "$prefix" . join("\n$prefix", map(chomp, @_)) . "\n";
}
If those strings can contain embedded newlines (so that they can be
called as in Junio example), then your solution is a must-be
sub prefix_lines {
my $prefix = shift;
my $string = join("\n", @_);
$string =~ s/^/$prefix/mg;
return $string;
}
Well, nevermind then
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 32/36] pathspec: allow querying for attributes
From: Junio C Hamano @ 2016-11-09 22:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: Duy Nguyen, Git Mailing List, Brandon Williams
In-Reply-To: <CAGZ79kZ=9QeZLKKrH27U2iE9x3WxgVe4RvCZpbdzZriMArV6Sg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Nov 9, 2016 at 1:45 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, Oct 28, 2016 at 1:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> ...
>>> The strategy this round takes to make it unnecessary to punt
>>> preloading (i.e. dropping "pathspec: disable preload-index when
>>> attribute pathspec magic is in use" patch the old series had) is to
>>> make the attribute subsystem thread-safe. But that thread-safety in
>>> the initial round is based on a single Big Attribute Lock, so it may
>>> turn out that the end result performs better for this codepath if we
>>> did not to make any call into the attribute subsystem.
>>
>> It does sound good and I want to say "yes please do this", but is it
>> making pathspec api a bit more complex (to express "assume all
>> attr-related criteria match")? I guess we can have an api to simply
>> filter out attr-related magic (basically set attr_match_nr back to
>> zero) then pass a safe (but more relaxing) pathspec to the threaded
>> code. That would not add big maintenance burden.
>
> So with the current implementation, we already have the shortcut as:
>
> if (item->attr_match_nr && !match_attrs(name, namelen, item))
>
> i.e. if attr_match_nr is zero, we do not even look at the mutexes and such,
> so I am not sure what you intend to say in this email?
I am not sure what relevance the "we call into attribute subsystem
only when there is any need to check attributes" obvious short-cut
has to what is being discussed.
The issue is specific to what preloading is about. It is merely an
attempt to run cheap checks that could be easily multi-threaded with
multiple threads early in the program that we _know_ we would need
to eventually refresh the index before doing some interesting work.
A full refresh_index() will be done eventually, and because it needs
to trigger thread-unsafe part of the API, it needs to be done in the
main thread. Doing the preload allows us to mark index entries that
do not have to be scanned again in the upcoming refresh_index()
call. It is OK for preload-index.c::preload_thread() to skip and
not mark some index entries (iow, its sole purpose is to leave a
note in each index entry "this is fresh, you do not need to look at
it again", and it can choose to skip an entry, which essentially
means "this I didn't check, so you, refresh_index(), need to check
yourself").
preload_thread() for example skips index entries that needs to
trigger "racy Git avoidance" logic that is heavyweight (it has to go
to the filesystem and the object store), and it is a sensible thing
to skip because they are rather rare.
The message by Duy you are responding to was his response to me who
wondered if the attribute based pathspec match also falls into the
same category. Just like racy Git code was deemed too heavyweight
to be called from preloading codepath and CE_MATCH_RACY_IS_DIRTY bit
was added as a way to ask ie_match_stat() API to avoid it (and hence
we are skipping, the caller is also telling "if you suspect a racy,
without checking for real, just answer 'I cannot say it is
clean/fresh'"), if we invent a new flag and pass it through
match_pathspec() down to match_pathspec_item() and have that if()
statement you quoted also skip match_attrs() for a pathspec element
with attribute based narrowing (as we are skipping, the flag may
also have say "instead of checking the attributes, just pretend that
the path did not satisfy the attribute narrowing"), would it benefit
the overall performance?
To answer Duy's "would it make sense to force the caller to create a
new pathspec from an existing one by filtering out pathspec elements
with attr-based narrowing?" question, I do not think it does.
^ permalink raw reply
* [PATCH] remote-curl: don't hang when a server dies before any output
From: David Turner @ 2016-11-09 22:18 UTC (permalink / raw)
To: git, spearce; +Cc: David Turner
In the event that a HTTP server closes the connection after giving a
200 but before giving any packets, we don't want to hang forever
waiting for a response that will never come. Instead, we should die
immediately.
One case where this happens is when attempting to fetch a dangling
object by SHA.
Still to do: it would be good to give a better error message
than "fatal: The remote end hung up unexpectedly".
Signed-off-by: David Turner <dturner@twosigma.com>
---
Note: if you run t5551 before applying the code patch, the second new
test will hang forever.
FWIW, I also saw this kind of hang at Twitter from time to time, but I
was never able to reliably reproduce it there, and thus never able to
fix it. I suspect that a bad load balancer might have been killing
connections just after the headers, but this is pure speculation.
I am sorry that this patch does not provide a more useful error
message. For us, it is important to fix the hangs (so that we can
retry on another server, for instance), but less important to be
user-friendly (since we were only seeing these with automated
processes). I hope that someone who actually understands the http
code, and has some time, could help improve this aspect of the code.
If not, then at least we won't have inexplicable hangs.
remote-curl.c | 8 ++++++++
t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index f14c41f..ee44236 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -400,6 +400,7 @@ struct rpc_state {
size_t pos;
int in;
int out;
+ int any_written;
struct strbuf result;
unsigned gzip_request : 1;
unsigned initial_buffer : 1;
@@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
{
size_t size = eltsize * nmemb;
struct rpc_state *rpc = buffer_;
+ if (size)
+ rpc->any_written = 1;
write_or_die(rpc->in, ptr, size);
return size;
}
@@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+
+ rpc->any_written = 0;
err = run_slot(slot, NULL);
if (err == HTTP_REAUTH && !large_request) {
credential_fill(&http_auth);
@@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
if (err != HTTP_OK)
err = -1;
+ if (!rpc->any_written)
+ err = -1;
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1ec5b27..43665ab 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
test_line_count = 2 posts
'
+test_expect_success 'test allowreachablesha1inwant' '
+ test_when_finished "rm -rf test_reachable.git" &&
+ server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+ git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+ git init --bare test_reachable.git &&
+ git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+ git -C test_reachable.git fetch origin "$master_sha"
+'
+
+test_expect_success 'test allowreachablesha1inwant with unreachable' '
+ test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" &&
+
+ #create unreachable sha
+ echo content >file2 &&
+ git add file2 &&
+ git commit -m two &&
+ git push public HEAD:refs/heads/doomed &&
+ git push public :refs/heads/doomed &&
+
+ server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+ git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+ git init --bare test_reachable.git &&
+ git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+ test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+'
+
test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
(
cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* Re: [git-for-windows] [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: Lars Schneider @ 2016-11-09 22:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git-for-windows, Git Mailing List, me
In-Reply-To: <alpine.DEB.2.20.1611051025030.3108@virtualbox>
On 05 Nov 2016, at 10:50, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> Dear Git users,
>
> I finally got around to rebase the Windows-specific patches (which seem to
> not make it upstream as fast as we get new ones) on top of upstream Git
> v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
>
> https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
I tested a new feature in 2.11 on Windows today and it failed. After some
confusion I realized that the feature is not on your 2.11 branch. Consider this:
My last patch on your 2.11 branch is
"pack-protocol: fix maximum pkt-line size" (7841c4):
https://github.com/git-for-windows/git/commits/v2.11.0-rc0.windows.1?author=larsxschneider
My last patch on the Git 2.11 branch is
"read-cache: make sure file handles are not inherited by child processes" (a0a6cb):
https://github.com/git/git/commits/v2.11.0-rc0?author=larsxschneider
Do you have a clue what is going on?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Jeff King @ 2016-11-09 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <xmqqd1i49yj1.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 09, 2016 at 12:41:22PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > For matching specific names, we have to deal with case-folding. It's
> > easy to hit the common ones like ".GITIGNORE" with fspathcmp(). But if
> > this is actually protection against malicious repositories, we have to
> > match all of the horrible filesystem-specific junk that we did for
> > ".git".
> >
> > Symlinks are likewise tricky.
>
> Wouldn't it be the simplest to say these:
>
> (1) The code attempts to read ".gitignore" (or ".git<something>")
> in general from the filesystem, or the index, or a tree. No
> case permutations are attempted.
>
> (2) When the code tries to do the above, we open with nofollow (or
> protect racily with lstat(2) which may be the best we could do)
> when reading from the filesystem, or check the ce_mode type
> when reading from the index or from a tree, and ignore if the
> path we are using is a symbolic link.
>
> That way, case funny filesystems that cause trouble like the ".git"
> thing would not have a chance to interfere and fool us, no?
That's what my series does. The question is just whether people will be
unhappy with (2), because they are using symbolic links in their
.gitignore files.
-Peff
^ permalink raw reply
* Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes
From: Junio C Hamano @ 2016-11-09 20:41 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <20161108222127.mejb74maewzhn3qg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> For matching specific names, we have to deal with case-folding. It's
> easy to hit the common ones like ".GITIGNORE" with fspathcmp(). But if
> this is actually protection against malicious repositories, we have to
> match all of the horrible filesystem-specific junk that we did for
> ".git".
>
> Symlinks are likewise tricky.
Wouldn't it be the simplest to say these:
(1) The code attempts to read ".gitignore" (or ".git<something>")
in general from the filesystem, or the index, or a tree. No
case permutations are attempted.
(2) When the code tries to do the above, we open with nofollow (or
protect racily with lstat(2) which may be the best we could do)
when reading from the filesystem, or check the ce_mode type
when reading from the index or from a tree, and ignore if the
path we are using is a symbolic link.
That way, case funny filesystems that cause trouble like the ".git"
thing would not have a chance to interfere and fool us, no?
^ permalink raw reply
* Re: [PATCH v2] rebase: add --forget to cleanup rebase, leave everything else untouched
From: Junio C Hamano @ 2016-11-09 20:12 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161109091131.17933-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> ---
> v2 changes just the subject line
That's not sufficient, is it? What you did in the documentation
would raise the same "Hmph, is this only about HEAD?" and unlike the
commit subject, it will carve it in stone for end-users.
^ permalink raw reply
* Cleaning ignored files
From: Roman Terekhov @ 2016-11-09 18:23 UTC (permalink / raw)
To: git
Hi,
I want to ask about git clean -dXf command behaviour.
I do the following:
$ mkdir gitignore_test
$ cd gitignore_test/
$ git init
Initialized empty Git repository in ~/gitignore_test/.git/
$ echo *.sln > .gitignore
$ git add .gitignore
$ git commit -m "add gitignore"
[master (root-commit) ef78a3c] add gitignore
1 file changed, 1 insertion(+)
create mode 100644 .gitignore
$ mkdir src
$ touch test.sln
$ touch src/test.sln
$ tree
.
├── src
│ └── test.sln
└── test.sln
1 directory, 2 files
$ git clean -dXf
Removing test.sln
$ tree
.
└── src
└── test.sln
1 directory, 1 file
Why git clean -dXf does not remove all my test.sln files, but just one of them?
Roman Terekhov
^ permalink raw reply
* Re: [PATCH 32/36] pathspec: allow querying for attributes
From: Stefan Beller @ 2016-11-09 18:08 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List, Brandon Williams
In-Reply-To: <CACsJy8C2MLg4ncLBXXJGf+=mPF_rRoKs2vN6=+chZeNeXWZsbg@mail.gmail.com>
On Wed, Nov 9, 2016 at 1:45 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, Oct 28, 2016 at 1:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The reason why I am bringing this up in this discussion thread on
>> this patch is because I wonder if we would benefit by a similar
>> "let's not do too involved things and be cheap by erring on the safe
>> and lazy side" strategy in the call to ce_path_match() call made in
>> this function to avoid making calls to the attr subsystem.
>>
>> In other words, would it help the system by either simplifying the
>> processing done or reducing the cycle spent in preload_thread() if
>> we could tell ce_path_match() "A pathspec we are checking may
>> require not just the pattern to match but also attributes given to
>> the path to satisfy the criteria, but for the purpose of preloading,
>> pretend that the attribute satisfies the match criteria" (or
>> "pretend that it does not match"), thereby not having to make any
>> call into the attribute subsystem at all from this codepath?
>>
>> The strategy this round takes to make it unnecessary to punt
>> preloading (i.e. dropping "pathspec: disable preload-index when
>> attribute pathspec magic is in use" patch the old series had) is to
>> make the attribute subsystem thread-safe. But that thread-safety in
>> the initial round is based on a single Big Attribute Lock, so it may
>> turn out that the end result performs better for this codepath if we
>> did not to make any call into the attribute subsystem.
>
> It does sound good and I want to say "yes please do this", but is it
> making pathspec api a bit more complex (to express "assume all
> attr-related criteria match")? I guess we can have an api to simply
> filter out attr-related magic (basically set attr_match_nr back to
> zero) then pass a safe (but more relaxing) pathspec to the threaded
> code. That would not add big maintenance burden.
>
So with the current implementation, we already have the shortcut as:
if (item->attr_match_nr && !match_attrs(name, namelen, item))
i.e. if attr_match_nr is zero, we do not even look at the mutexes and such,
so I am not sure what you intend to say in this email?
^ 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