* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 20:59 UTC (permalink / raw)
To: Jacob Keller, Jeff King; +Cc: Git mailing list
In-Reply-To: <CA+P7+xqmVM-bEc7sZcn+p3qhFUUJvC+rko7CYu+KgyTAeiOifw@mail.gmail.com>
On 28/10/16 08:48, Jacob Keller wrote:
> I would strongly prefer rc.d style directories either with a "if the
> .gitignore is a directory treat it like rc.d" or even "add support for
> .gitignore.d as well as .gitignore"
I think adding .gitignore.d shouldn't break existing systems, is
intuitive, and solves my issue.
Does git know when it is in a repo that is too new to comprehend?
My current thinking is that anywhere a .gitignore can go, so can a
.gitignore.d (named appropriately of course.) Any existing .gitignore
should take precedence to the result of parsing the directory.
I haven't looked at the implementation of precedence yet, but I'd be
surprised if the existing mechanism can't be employed.
> One thing to keep in mind would be that we should make sure we can
> handle the .gitignore being a submodule or a git repository, so that
> users could just do something like
>
> "git submodule add <repo> .gitignore and then track git ignore
> contents from a repository in a nice way.
>
> By this I mean that the reading of files in .gitignore directory
> should exclude reading .git or other hidden files in some documented
> manor so as to avoid problems when linking to a git directory for its
> contents.
Nice! I like this a lot.
^ permalink raw reply
* Re: [PATCH 17/36] attr: expose validity check for attribute names
From: Stefan Beller @ 2016-10-27 20:57 UTC (permalink / raw)
To: Ramsay Jones
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <CAGZ79ka_zr_NXKoxC45swFrj168fP6S7_nQ1jjcfPOtTN4Jd1A@mail.gmail.com>
On Mon, Oct 24, 2016 at 2:07 PM, Stefan Beller <sbeller@google.com> wrote:
> On Sun, Oct 23, 2016 at 8:07 AM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>>
>> On 23/10/16 00:32, Stefan Beller wrote:
>>> From: Junio C Hamano <gitster@pobox.com>
>>>
>>> Export attr_name_valid() function, and a helper function that
>>> returns the message to be given when a given <name, len> pair
>>> is not a good name for an attribute.
>>>
>>> We could later update the message to exactly spell out what the
>>> rules for a good attribute name are, etc.
>>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>
>> [snip]
>>
>>> +extern int attr_name_valid(const char *name, size_t namelen);
>>> +extern void invalid_attr_name_message(struct strbuf *, const char *, int);
>>> +
>>
>> The symbol 'attr_name_valid()' is not used outside of attr.c, even
>> by the end of this series. Do you expect this function to be used
>> in any future series? (The export is deliberate and it certainly
>> seems like it should be part of the public interface, but ...)
>>
refactoring this series again, I will make use of attr_name_valid
outside of attr.c, so I'll keep this patch.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-27 20:55 UTC (permalink / raw)
To: Aaron Pelly; +Cc: git
In-Reply-To: <b20b458c-440d-df09-d2c7-e510ac20492c@pelly.co>
On Fri, Oct 28, 2016 at 09:28:23AM +1300, Aaron Pelly wrote:
> > - we parse possibly-hostile .gitignore files from cloned repositories.
> > What happens when I include ask to include /etc/passwd? Probably
> > nothing, but there are setups where it might matter (e.g., something
> > like Travis that auto-builds untrusted repositories, and you could
> > potentially leak the contents of files via error messages). It's
> > nice to avoid the issue entirely.
>
> I understand the issue.
>
> It's not obvious to me how using a .d solves this problem though.
It doesn't by itself. But we are worried only about tracked .gitignore
files (recall that even repo-level files in $GIT_DIR/info are generated
fresh by the clone process, and don't come from the remote). If we apply
the feature only to core.excludeFile and $GIT_DIR/info/exclude, those
are already under the user's control.
It's true that we could make a similar exception for an "include"
feature, and respect include directives only in those "safe" files.
Somehow that seems more confusing to me, though, than doing adding the
feature at the file level, as it introduces slightly varying syntax
between the locations.
> > Whereas letting any of the user- or repo-level exclude files be a
> > directory, and simply reading all of the files inside, seems simple and
> > obvious.
>
> Apart from backwards compatibility, unless there's something I'm missing.
I'm not sure what you mean. If we make:
mkdir .git/info/exclude
echo whatever >.git/info/exclude/local
work, I don't think we have to care about backwards compatibility. That
was nonsensical before, and never did anything useful (so somebody might
have done it, but we can assume anybody relying on it not to read the
contents is crazy).
> > It
> > can't handle all cases (some items in "00foo" want precedence over "01bar"
> > and vice versa), but I don't think there's an easy solution. That's a
> > good sign that one or more of the files should be broken up.
>
> I've been burned by this myself by packages interfering with each other
> in /etc/sysctl.d
>
> Could we put this down to caveat emptor? I think this sorting should be
> intuitive to most people these days, and simple to document and comprehend.
Yeah, I think caveat emptor is fine there. Sorry if I implied otherwise.
-Peff
^ permalink raw reply
* Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture
From: Eric Wong @ 2016-10-27 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Gavin Lambert
In-Reply-To: <xmqqmvhp60gp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Just peeking from the sideline, but the your squash looks like an
> improvement to me.
Thanks.
> Hopefully the final version after your interaction with Dscho can
> come to me via another "pull this now"?
Not sure if I'll be online the next few days,
but I've preeptively pushed the patch + squash to my repo:
The following changes since commit 2cc2e70264e0fcba04f9ef791d144bbc8b501206:
Eleventh batch for 2.11 (2016-10-26 13:28:47 -0700)
are available in the git repository at:
git://bogomips.org/git-svn.git svn-cache
for you to fetch changes up to a2c761ce5b7a5fd8b505b036f3509a9e6617dee8:
git-svn: do not reuse caches memoized for a different architecture (2016-10-27 20:17:36 +0000)
----------------------------------------------------------------
Gavin Lambert (1):
git-svn: do not reuse caches memoized for a different architecture
perl/Git/SVN.pm | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Eric Wong @ 2016-10-27 20:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, git
In-Reply-To: <alpine.DEB.2.20.1610191049040.3847@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> I know you are a fan of testing things thoroughly in the test suite, but I
> have to say that it is getting out of hand, in particular due to our
> over-use of shell script idioms (which really only run fast on Linux, not
> a good idea for a portable software).
How much effort would it take to optimize a /bin/sh?
Would replacing uses of fork+execve posix_spawn be fast and
portable enough?
Even on Linux, performance sucks for me. I've been hoping dash
can use posix_spawn (or using vfork directly) to see if that can
help things.
That won't help with subshells, though...
(I'm back to using a Centrino laptop from 2005)
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 20:28 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161027105026.e752znq5jv5a6xea@sigill.intra.peff.net>
On 27/10/16 23:50, Jeff King wrote:
> I'd shy away from an actual include directive, as it raises a lot of
> complications:
I'm leaning that way now too.
> - we parse possibly-hostile .gitignore files from cloned repositories.
> What happens when I include ask to include /etc/passwd? Probably
> nothing, but there are setups where it might matter (e.g., something
> like Travis that auto-builds untrusted repositories, and you could
> potentially leak the contents of files via error messages). It's
> nice to avoid the issue entirely.
I understand the issue.
It's not obvious to me how using a .d solves this problem though.
> - finding a backwards-compatible syntax
using .d directories solves this nicely in my opinion
> Whereas letting any of the user- or repo-level exclude files be a
> directory, and simply reading all of the files inside, seems simple and
> obvious.
Apart from backwards compatibility, unless there's something I'm missing.
> If you go that route, it probably makes sense to teach
> gitattributes the same trick.
Understood. I'll keep that in mind.
>> In the case of a directory the plan would be to add links to files
>> stored/sourced elsewhere. This does pose a precedence question which I
>> haven't thought about yet, but probably makes it too hard for the
>> limited value it brings.
>
> I think the normal behavior in such "foo.d" directory is to just sort
> the contents lexically and read them in order, as if they were all
> concatenated together, and with no recursion. I.e., behave "as if" the
> user had run "cat $dir/*".
>
> That lets you handle precedence via the filenames (or symlink names).
That was my thinking at first, but I didn't want to bias the discussion.
> It
> can't handle all cases (some items in "00foo" want precedence over "01bar"
> and vice versa), but I don't think there's an easy solution. That's a
> good sign that one or more of the files should be broken up.
I've been burned by this myself by packages interfering with each other
in /etc/sysctl.d
Could we put this down to caveat emptor? I think this sorting should be
intuitive to most people these days, and simple to document and comprehend.
^ permalink raw reply
* Re: [PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
From: Christian Couder @ 2016-10-27 20:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Pranit Bauva, Matthieu Moy, Alex Henrie, Antoine Delaite
In-Reply-To: <xmqqvawd7mnr.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 6:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Cc'ed those who touched either "git-bisect.sh" or "builtin/bisect-helper.c"
> in our relatively recent past.
>
> Does any of you (and others on the list) have time and inclination
> to review this series?
As part of my mentoring Pranit for the GSoC I already took a look at
those patches some months ago on GitHub.
I could take another look at them but my eyes will not be fresh
anymore, so I don know if it will be valuable.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-27 20:05 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Johannes Schindelin, git@vger.kernel.org,
Simon Ruderich, Jeff King
In-Reply-To: <CAGZ79kY_fZ_pDtVnwJoDkR6PjTNoqDMN5OC70Z8SH_J0Wvkq-w@mail.gmail.com>
Am 27.10.2016 um 21:08 schrieb Stefan Beller:
> On Thu, Oct 27, 2016 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>>> ...
>>> It is not possible to mark a mutex uninitialized on Windows without an
>>> extra piece of data. A solution would become quite complicated quite
>>> quickly, and at the cost of additional operations that are in the same
>>> ballpark as an uncontended mutex. I'm not enthused.
>>>
>>>> The positive aspect of this way the patch proposes would be that any
>>>> future contributor not knowing the details of how to do mutexes right
>>>> on Windows, would not totally break the whole system, i.e. this seems
>>>> to be more maintainable in the future as it reduces the friction between
>>>> pthreads mutexes and the way we can do things in Git in a platform
>>>> independent way
>>>
>>> This is a non-argument. Coders have to know their tools.
>
> Windows is not my tool.
The tool I meant is pthreads. For example, you can't have a
pthread_mutex_t variable and not initialize it with either
PTHREAD_MUTEX_INITIALIZER or pthread_mutex_init.
>> The codebase should strive to give coders a coherent abstraction
>> that can be implemented efficiently on platforms, so that coders do
>> not have to care too deeply about quirks that exist on individual
>> platforms.
>
> Currently working as a coder I care about "submodules, that work on
> linux." I do not care about Windows in the big picture.
I don't know what you meant to say with this sentence, but taken
literally, are you on the right ship here, I have to ask?
> I am however
> willing to go an extra step to not break Windows.
"Not break Windows" is equivalent to "make it work on Windows", mind
you. We can't have a new feature only on Linux when there is no reason
not to have it on Windows as well. Sorry, "Git is for Unix only" is long
over.
> However that requires
> a little bit of effort from both me and you:
> * I need to be aware of what I cannot do with "not-my-tools". (So please
> somebody tell me, also in the future when I break obscure platforms. Mind
> that I don't equate obscure with not widely used or in any other way negative.
> It's just that people working on linux find some quirks on Windows
> not "obvious")
That goes without saying.
> * the workaround should not be too time consuming in the bigger picture,
That, however, is wishful thinking. If you want to have a feature
dearly, you have to make it work, and that may take its time. I'm not
saying that *you* have to make it work (there are platform experts
around to lend a hand), but just an extra step to "not break" an
important platform is not enough.
> which is why I propose to make the API a bit clearer by emulating posix
> mutexes harder. (From a Windows POV this might sound like making it
> more obscure because posix mutexes itself are obscure.)
So, when you think that POSIX mutexes are obscure, why don't we settle
on the simpler Windows critical sections? Emulating them with pthreads
should be child's play.
> The implementation under discussion (well we did not discuss the
> implementation a
> whole lot yet) ...
There's not a whole lot to discuss: it must be rewritten from scratch
(it's not just the memory barriers, it is everything else, too). But
time is much better spent on an attr_start() solution.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2 1/2] read-cache: factor out get_sha1_from_index() helper
From: Junio C Hamano @ 2016-10-27 19:57 UTC (permalink / raw)
To: tboegi; +Cc: git
In-Reply-To: <20161012134726.28326-1-tboegi@web.de>
tboegi@web.de writes:
> From: Torsten Bögershausen <tboegi@web.de>
>
> Factor out the retrieval of the sha1 for a given path in
> read_blob_data_from_index() into the function get_sha1_from_index().
>
> This will be used in the next commit, when convert.c can do the
> analyze for "text=auto" without slurping the whole blob into memory
> at once.
>
> Add a wrapper definition get_sha1_from_cache().
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> cache.h | 3 +++
> read-cache.c | 29 ++++++++++++++++++-----------
> 2 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 1604e29..04de209 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -380,6 +380,7 @@ extern void free_name_hash(struct index_state *istate);
> #define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at)
> #define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec)
> #define read_blob_data_from_cache(path, sz) read_blob_data_from_index(&the_index, (path), (sz))
> +#define get_sha1_from_cache(path) get_sha1_from_index (&the_index, (path))
Should have caught this earlier, but there is an extra SP after "from_index"
which I'll remove (the topic is not in 'next' yet, lucky us).
I re-read this to ensure that it does not break read_blob_data_from_index()
the new function borrows the logic from.
Thanks.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jacob Keller @ 2016-10-27 19:48 UTC (permalink / raw)
To: Jeff King; +Cc: Aaron Pelly, Git mailing list
In-Reply-To: <20161027105026.e752znq5jv5a6xea@sigill.intra.peff.net>
On Thu, Oct 27, 2016 at 3:50 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Oct 27, 2016 at 01:22:43PM +1300, Aaron Pelly wrote:
>
>> The use case for this is where I did not write my own rules, but I want
>> to keep them updated. https://github.com/github/gitignore is a damn good
>> resource, but I want to pull it and include relevant bits project by
>> project and/or system wide. I don't want to have to update many projects
>> manually if that, or any other, repo changes.
>
> That seems like a reasonable thing to want.
>
Agreed, this seems useful to me.
>> A very brief look at dir.c would indicate that a recursive call from
>> add_excludes to itself when it parses some sort of include tag would do
>> it within a file. I'm sure it'd be pretty straight forward to hook into
>> something in dir.c to parse directories too.
>>
>> I'm thinking something like ". path/to/include/file" in an ignore file,
>> and/or creating .gitignore.d and/or allowing $HOME/.config/git/ignore
>> and $GIT_DIR/info/exclude to be directories. Or some sane and consistent
>> mixture of these things.
>
> I'd shy away from an actual include directive, as it raises a lot of
> complications:
>
> - as you noted, cycles in the include graph need to be detected and
> broken
>
> - we parse possibly-hostile .gitignore files from cloned repositories.
> What happens when I include ask to include /etc/passwd? Probably
> nothing, but there are setups where it might matter (e.g., something
> like Travis that auto-builds untrusted repositories, and you could
> potentially leak the contents of files via error messages). It's
> nice to avoid the issue entirely.
>
> - finding a backwards-compatible syntax
>
> Whereas letting any of the user- or repo-level exclude files be a
> directory, and simply reading all of the files inside, seems simple and
> obvious. If you go that route, it probably makes sense to teach
> gitattributes the same trick.
Yep that would be easier and simpler.
>
>> In the case of a directory the plan would be to add links to files
>> stored/sourced elsewhere. This does pose a precedence question which I
>> haven't thought about yet, but probably makes it too hard for the
>> limited value it brings.
>
> I think the normal behavior in such "foo.d" directory is to just sort
> the contents lexically and read them in order, as if they were all
> concatenated together, and with no recursion. I.e., behave "as if" the
> user had run "cat $dir/*".
Yea, this is the normal behavior, and the user is expected to order
their files lexically such as "00-name", "50-name" and so on. Pretty
traditional for a lot of newer configurations.
>
> That lets you handle precedence via the filenames (or symlink names). It
> can't handle all cases (some items in "00foo" want precedence over "01bar"
> and vice versa), but I don't think there's an easy solution. That's a
> good sign that one or more of the files should be broken up.
>
Yea if you have these inter-dependent relationships it can't be
expressed, but then neither can it really be expressed in the simple
"include" case above.
I would strongly prefer rc.d style directories either with a "if the
.gitignore is a directory treat it like rc.d" or even "add support for
.gitignore.d as well as .gitignore"
One thing to keep in mind would be that we should make sure we can
handle the .gitignore being a submodule or a git repository, so that
users could just do something like
"git submodule add <repo> .gitignore and then track git ignore
contents from a repository in a nice way.
By this I mean that the reading of files in .gitignore directory
should exclude reading .git or other hidden files in some documented
manor so as to avoid problems when linking to a git directory for its
contents.
Thanks,
Jake
> -Peff
^ permalink raw reply
* Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture
From: Junio C Hamano @ 2016-10-27 19:44 UTC (permalink / raw)
To: Eric Wong; +Cc: Johannes Schindelin, git, Gavin Lambert
In-Reply-To: <20161025212357.GA8683@starla>
Eric Wong <e@80x24.org> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>> +++ b/perl/Git/SVN.pm
>> @@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
>> if ($memo_backend > 0) {
>> tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
>> } else {
>> + # first verify that any existing file can actually be loaded
>> + # (it may have been saved by an incompatible version)
>> + if (-e "$path.db") {
>> + unlink "$path.db" unless eval { retrieve("$path.db"); 1 };
>> + }
>
> That retrieve() call is unlikely to work without "use Storable"
> to import it into the current package.
>
> I also favor setting "$path.db" once to detect typos and avoid
> going over 80 columns. Additionally, having error-checking for
> unlink might be useful.
>
> So perhaps squashing this on top:
>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 025c894..b3c1460 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1660,10 +1660,15 @@ sub tie_for_persistent_memoization {
> } else {
> # first verify that any existing file can actually be loaded
> # (it may have been saved by an incompatible version)
> - if (-e "$path.db") {
> - unlink "$path.db" unless eval { retrieve("$path.db"); 1 };
> + my $db = "$path.db";
> + if (-e $db) {
> + use Storable qw(retrieve);
> +
> + if (!eval { retrieve($db); 1 }) {
> + unlink $db or die "unlink $db failed: $!";
> + }
> }
> - tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
> + tie %$hash => 'Memoize::Storable', $db, 'nstore';
> }
> }
>
>
> Thoughts? Thanks.
Just peeking from the sideline, but the your squash looks like an
improvement to me.
Hopefully the final version after your interaction with Dscho can
come to me via another "pull this now"?
Thanks.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Stefan Beller @ 2016-10-27 19:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org,
Simon Ruderich, Jeff King
In-Reply-To: <xmqqr3716301.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>> ...
>> It is not possible to mark a mutex uninitialized on Windows without an
>> extra piece of data. A solution would become quite complicated quite
>> quickly, and at the cost of additional operations that are in the same
>> ballpark as an uncontended mutex. I'm not enthused.
>>
>>> The positive aspect of this way the patch proposes would be that any
>>> future contributor not knowing the details of how to do mutexes right
>>> on Windows, would not totally break the whole system, i.e. this seems
>>> to be more maintainable in the future as it reduces the friction between
>>> pthreads mutexes and the way we can do things in Git in a platform
>>> independent way
>>
>> This is a non-argument. Coders have to know their tools.
Windows is not my tool.
>
> The codebase should strive to give coders a coherent abstraction
> that can be implemented efficiently on platforms, so that coders do
> not have to care too deeply about quirks that exist on individual
> platforms.
Currently working as a coder I care about "submodules, that work on
linux." I do not care about Windows in the big picture. I am however
willing to go an extra step to not break Windows. However that requires
a little bit of effort from both me and you:
* I need to be aware of what I cannot do with "not-my-tools". (So please
somebody tell me, also in the future when I break obscure platforms. Mind
that I don't equate obscure with not widely used or in any other way negative.
It's just that people working on linux find some quirks on Windows
not "obvious")
* the workaround should not be too time consuming in the bigger picture,
which is why I propose to make the API a bit clearer by emulating posix
mutexes harder. (From a Windows POV this might sound like making it
more obscure because posix mutexes itself are obscure.)
>
> It is OK to argue that the particular solution Stefan lifted from
> somewhere (perhaps msdn article he cited???)
A stack overflow article that I found with my search engine of choice, because
I could not believe that Windows cannot have statically initialized mutexes.
> does not qualify as
> such an abstraction.
The implementation under discussion (well we did not discuss the
implementation a
whole lot yet) may even contain an error as the first memory barrier
needs to be in front
of the first condition.
> But I do not agree with your "Coders have to
> know" as a blanket statement.
Well I do to some extent, I just disagree what my tools are.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Junio C Hamano @ 2016-10-27 18:49 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Johannes Schindelin, git@vger.kernel.org,
Simon Ruderich, Jeff King
In-Reply-To: <34c88c40-2088-fd74-5d26-56c0599b7eb9@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
> ...
> It is not possible to mark a mutex uninitialized on Windows without an
> extra piece of data. A solution would become quite complicated quite
> quickly, and at the cost of additional operations that are in the same
> ballpark as an uncontended mutex. I'm not enthused.
>
>> The positive aspect of this way the patch proposes would be that any
>> future contributor not knowing the details of how to do mutexes right
>> on Windows, would not totally break the whole system, i.e. this seems
>> to be more maintainable in the future as it reduces the friction between
>> pthreads mutexes and the way we can do things in Git in a platform
>> independent way
>
> This is a non-argument. Coders have to know their tools.
The codebase should strive to give coders a coherent abstraction
that can be implemented efficiently on platforms, so that coders do
not have to care too deeply about quirks that exist on individual
platforms.
It is OK to argue that the particular solution Stefan lifted from
somewhere (perhaps msdn article he cited???) does not qualify as
such an abstraction. But I do not agree with your "Coders have to
know" as a blanket statement.
^ permalink raw reply
* Re: [PATCH] Documentation/git-diff: document git diff with 3+ commits
From: Junio C Hamano @ 2016-10-27 18:40 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jacob Keller
In-Reply-To: <e74d4d84-d848-00be-8ca5-75204cf47262@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> It did not exist, but even at that point in time, "git log A..B" listed
> only commits between the merge base and B, not those which are only in A
> and not in B. Whereas "git diff A B" shows the differences between the
> endpoints A and B.
I think you are speaking with 20/20 hindsight at this point.
Do you genuinely think you would have thought that way when there
weren't any concept of "merge-base" or "git log A...B" known to you?
>>> @@ -12,6 +12,7 @@ SYNOPSIS
>>> 'git diff' [options] [<commit>] [--] [<path>...]
>>> 'git diff' [options] --cached [<commit>] [--] [<path>...]
>>> 'git diff' [options] <commit> <commit> [--] [<path>...]
>>> +'git diff' [options] <commit> <commit> <commit> [<commit>...]
>>
>> Made me wonder "is [<A>...] 0-or-more As or 1-or-more As?".
>
> 0-or-more, at least that's the way it is used in all lines here.
>
>> Don't we allow pathspecs in this case?
>
> Yes, the combinded diff mode kicks in only with no blobs (not: no
> pathspec, which I had misread) and N>=3 commits. Maybe I should update
> the code comments in builtin/diff.c to describe this, too.
I don't know about the code comments, but the above SYNOPSIS needs
an update, I would think. That is why I wondered if [<A>...] were
0-or-more or 1-or-moer.
You can replace
>>> 'git diff' [options] <commit> <commit> [--] [<path>...]
with
>>> 'git diff' [options] <commit> <commit> [<commit>...] [--] [<path>...]
without adding a new line to cover the new(ly discovered) case.
^ permalink raw reply
* Re: [PATCH] Update git rebase documentation to clarify HEAD behavior
From: Philip Oakley @ 2016-10-27 14:06 UTC (permalink / raw)
To: Junio C Hamano, Cody Sehl; +Cc: git
In-Reply-To: <xmqq8tta8f0x.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> Cody Sehl <cody.sehl@gmail.com> writes:
>
>> The first few paragraphs in the git-rebase.txt documentation lay out the
>> steps git takes during a rebase:
>> 1. everything from `<upstream>..HEAD` is saved to a temporary area
>> 2. `HEAD` is set to `<upstream>`
>> 3. the changes held in the temporary area are applied one by one in order
>> on top of the new `HEAD`
>>
>> The second step was described using the phrase `The current branch is
>> reset to <upstream>`, which is true (because `HEAD` == current branch),
>> but not clear.
>> ---
>
> Please wrap your lines to reasonable lengths like 70 columns or so.
> Please sign off your patch.
>
>> Documentation/git-rebase.txt | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index de222c8..c47ca11 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -33,7 +33,7 @@ of commits that would be shown by `git log
>> <upstream>..HEAD`; or by
>> description on `--fork-point` below); or by `git log HEAD`, if the
>> `--root` option is specified.
>>
>> -The current branch is reset to <upstream>, or <newbase> if the
>> +HEAD is reset to <upstream>, or <newbase> if the
>> --onto option was supplied. This has the exact same effect as
>> `git reset --hard <upstream>` (or <newbase>). ORIG_HEAD is set
>> to point at the tip of the branch before the reset.
>
> This is describing an ancient behaviour before 6fd2f5e60d ("rebase:
> operate on a detached HEAD", 2007-11-08) in v1.5.4 timeframe. We
> apparently failed to update the description.
>
> This depends on the desired technical detail of the description, but
> a correct rewrite would be "HEAD is detached at <upstream>, or
> <newbase>, and ORIG_HEAD is set to point at the tip of the branch
> before this happens". Detaching the HEAD at <upstream> no longer
> has the same effect as "git reset --hard <upstream>" (or <newbase>),
> so that sentence must go. It was the primary point of the ancient
> change at 6fd2f5e60d after all.
>
> And then there is a new step (to be numbered 4. in your description
> in the proposed log message), which updates the tip of the branch to
> the resulting HEAD (after replaying all these changes) and check the
> branch out, which needs to be added. Perhaps after "one by one, in
> order." Oh, the mention of "reapplied to the current branch" also
> needs to be updated to "reapplied to the detached HEAD", too.
>
> On the other hand, if we do not aim for that deep level of technical
> correctness, but want to tell a white lie to make it easier to
> understand at the conceptual level to new readers who haven't
> grasped the detached HEAD, then the current description is fine. By
> bringing up "HEAD", you seem to be aiming for techincal correctness
> (which I tend to agree is a good direction to go in this part of the
> documentation), so the existing text needs a bit more work than your
> patch to be brought to the modern world.
>
A third option is to start with a short "Conceptually, .." description which
gives that summary overview, and then have a few "In detail, .." paragraphs
that cover the ORIG_HEAD and Detatched HEAD, etc., that users should at
least be aware of (so they know they can come back and read the gory details
when they need it;-)
--
Philip
^ permalink raw reply
* Re: [PATCH 32/36] pathspec: allow querying for attributes
From: Junio C Hamano @ 2016-10-27 18:29 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, pclouds
In-Reply-To: <20161022233225.8883-33-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> 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.
I was looking at preload-index.c and its history again, because I
wanted to ensure that Lars's process filter stuff introduces no
unexpected interactions with getting multi-threaded, similar to the
problem we had in the earlier incarnations of this step, which we
worked around with <xmqqshwvvaxq.fsf@gitster.mtv.corp.google.com>
"pathspec: disable preload-index when attribute pathspec magic is in
use".
I think that Lars's series is safe, because the only thing among
what preload-index.c::preload_thread() does that can go outside the
simple stat data and pattern matching with the pathname is the
racy-git check in ie_match_stat(), and the object store access in
that call was explicitly disabled by 7c4ea599b0 ("Fix index
preloading for racy dirty case", 2008-11-17) long time ago.
By passing CE_MATCH_RACY_IS_DIRTY option to the call, this caller
effectively says "A cache entry whose stat data matches may be
actually dirty when the timestamp is racy, in which case we usually
compare data to determine if it really is clean, but it is OK to err
on the safe and lazy side by declaring it dirty and not marking it
up-to-date while we are preloading. Do not bother to go to the
object store".
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.
I am probably being two step ahead of ourselves by saying the above,
which is just something to keep in mind as a possible solution if
performance in this preload codepath becomes an issue when the
pathspec has attributes match specified.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-27 18:22 UTC (permalink / raw)
To: Stefan Beller, Junio C Hamano
Cc: Johannes Schindelin, git@vger.kernel.org, Simon Ruderich,
Jeff King
In-Reply-To: <CAGZ79kbP3pgPHgv-x1Q-Q1QwmXc=gOyxWhXh2SngO8WSZc3PFA@mail.gmail.com>
Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>> Johannes Sixt <j6t@kdbg.org> writes:
>> This is the pessimization that I am talking about. I would not mind at all if
>> it were only for the attribute subsystem, but the proposed patch would
>> pessimize *all* uses of pthread_mutex_lock.
>
> It would only pessimize *uninitialized* mutexes? For initialized mutexes
> the added burden is super cheap (one additional condition).
It is not possible to mark a mutex uninitialized on Windows without an
extra piece of data. A solution would become quite complicated quite
quickly, and at the cost of additional operations that are in the same
ballpark as an uncontended mutex. I'm not enthused.
> The positive aspect of this way the patch proposes would be that any
> future contributor not knowing the details of how to do mutexes right
> on Windows, would not totally break the whole system, i.e. this seems
> to be more maintainable in the future as it reduces the friction between
> pthreads mutexes and the way we can do things in Git in a platform
> independent way
This is a non-argument. Coders have to know their tools.
-- Hannes
^ permalink raw reply
* Re: [PATCH] git-compat-util: move content inside ifdef/endif guards
From: Junio C Hamano @ 2016-10-27 17:37 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161027173029.bu233oiekpfoh6lw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Commit 3f2e2297b9 (add an extra level of indirection to
> main(), 2016-07-01) added a declaration to git-compat-util.h,
> but it was accidentally placed after the final #endif that
> guards against multiple inclusions.
>
> This doesn't have any actual impact on the code, since it's
> not incorrect to repeat a function declaration in C. But
> it's a bad habit, and makes it more likely for somebody else
> to make the same mistake. It also defeats gcc's optimization
> to avoid opening header files whose contents are completely
> guarded.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I just happened to notice this today while doing something unrelated in
> the file.
Thanks.
>
> git-compat-util.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 91e366d1dd..771ea29f31 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1042,6 +1042,6 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
> #define getc_unlocked(fh) getc(fh)
> #endif
>
> -#endif
> -
> extern int cmd_main(int, const char **);
> +
> +#endif
^ permalink raw reply
* [PATCH] git-compat-util: move content inside ifdef/endif guards
From: Jeff King @ 2016-10-27 17:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Commit 3f2e2297b9 (add an extra level of indirection to
main(), 2016-07-01) added a declaration to git-compat-util.h,
but it was accidentally placed after the final #endif that
guards against multiple inclusions.
This doesn't have any actual impact on the code, since it's
not incorrect to repeat a function declaration in C. But
it's a bad habit, and makes it more likely for somebody else
to make the same mistake. It also defeats gcc's optimization
to avoid opening header files whose contents are completely
guarded.
Signed-off-by: Jeff King <peff@peff.net>
---
I just happened to notice this today while doing something unrelated in
the file.
git-compat-util.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 91e366d1dd..771ea29f31 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1042,6 +1042,6 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
#define getc_unlocked(fh) getc(fh)
#endif
-#endif
-
extern int cmd_main(int, const char **);
+
+#endif
--
2.10.1.916.g0d2035c
^ permalink raw reply related
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Jeff King @ 2016-10-27 17:11 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Johannes Sixt, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich
In-Reply-To: <CAGZ79kbP3pgPHgv-x1Q-Q1QwmXc=gOyxWhXh2SngO8WSZc3PFA@mail.gmail.com>
On Thu, Oct 27, 2016 at 10:01:02AM -0700, Stefan Beller wrote:
> > That function can
> > be called from main() of platforms that cannot statically initialize
> > mutices,
>
> By main you mean the main() in common-main.c or cmd_main in git.c ?
Any setup or initialization for library functions needs to go in main()
of common-main.c. Otherwise, only builtins get it, and external
programs are left to segfault (or whatever).
> Those both look like the wrong place. Of course it would work adding it
> there, but it smells like a maintenance nightmare.
I agree it's ugly, but I suspect it would work OK in practice.
We don't have that many mutexes and initializing them is cheap.
Languages like C++ have non-constant initializers, and the compiler
takes care of running the startup code before main() begins. There's no
portable way to do that in C, but our cmd_main() is roughly the same
thing.
I still prefer the lazy initialization if it can work without incurring
measurable overhead. That's the normal way to do things in C; the only
complication here is the thread safety.
-Peff
^ permalink raw reply
* Re: [PATCH v4 00/14] Mark strings in Perl scripts for translation
From: Junio C Hamano @ 2016-10-27 17:08 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161010125449.7929-1-vascomalmeida@sapo.pt>
> Vasco Almeida (14):
> i18n: add--interactive: mark strings for translation
> i18n: add--interactive: mark simple here-documents for translation
> i18n: add--interactive: mark strings with interpolation for
> translation
> i18n: clean.c: match string with git-add--interactive.perl
> i18n: add--interactive: mark plural strings
> i18n: add--interactive: mark patch prompt for translation
> i18n: add--interactive: i18n of help_patch_cmd
> i18n: add--interactive: mark edit_hunk_manually message for
> translation
> i18n: add--interactive: remove %patch_modes entries
> i18n: add--interactive: mark status words for translation
> i18n: send-email: mark strings for translation
> i18n: send-email: mark warnings and errors for translation
> i18n: send-email: mark string with interpolation for translation
> i18n: difftool: mark warnings for translation
It seems that nobody has anything to say on this series, either
positive or negative?
The topic has been in "Waiting for review" pile but I'll mark it as
"Will merge to 'next'" unless I hear otherwise within a few days
(positives may make it faster, negatives may require a reroll).
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/2] Stream and fast search
From: Junio C Hamano @ 2016-10-27 17:02 UTC (permalink / raw)
To: git; +Cc: tboegi, Lars Schneider, Duy Nguyen
In-Reply-To: <20161012134724.28287-1-tboegi@web.de>
Cc'ed those who touched convert.c or read-cache.c in our relatively
recent past with a change that affects the eol conversion codepath.
Does any of you (and others on the list) have time and inclination
to review this series?
Thanks.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Stefan Beller @ 2016-10-27 17:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org,
Simon Ruderich, Jeff King
In-Reply-To: <xmqqd1im8foi.fsf@gitster.mtv.corp.google.com>
> Johannes Sixt <j6t@kdbg.org> writes:
> This is the pessimization that I am talking about. I would not mind at all if
> it were only for the attribute subsystem, but the proposed patch would
> pessimize *all* uses of pthread_mutex_lock.
It would only pessimize *uninitialized* mutexes? For initialized mutexes
the added burden is super cheap (one additional condition).
The positive aspect of this way the patch proposes would be that any
future contributor not knowing the details of how to do mutexes right
on Windows, would not totally break the whole system, i.e. this seems
to be more maintainable in the future as it reduces the friction between
pthreads mutexes and the way we can do things in Git in a platform
independent way
On Wed, Oct 26, 2016 at 11:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Lazy on-demand initialization as needed, perhaps? The on-demand
>> initialization mechanism may become no-op on some platforms that can
>> do static initialization.
>
> Ah, I think I misunderstood your "please rewrite". Did you mean to
> add "void attr_start(void)" helper function to attr.c that does
> series of pthread_mutex_init() calls as needed?
Well one init for now.
> That function can
> be called from main() of platforms that cannot statically initialize
> mutices,
By main you mean the main() in common-main.c or cmd_main in git.c ?
Those both look like the wrong place. Of course it would work adding it
there, but it smells like a maintenance nightmare.
And then we would modify the attr_start command depending on the
platform, i.e.
#ifdef WIN32
void attr_start(void)
{
pthread_mutex_init(..);
}
#else
void attr_start(void)
{
/* nothing as it is statically init'd */
}
#endif
> while on other platforms it can be a no-op as long as the
> variables are statically initialized? If so, that would not pessimize
> any platform, I would think.
I would think this pessimizes ALL platforms from a maintenance perspective
(but what do I know about maintaining stuff as an eager young contributor ;)
So I am willing to go that route, though I do not quite understand where exactly
you'd expect me to put the initializer as all places I can think of
are not the right
place.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
From: Junio C Hamano @ 2016-10-27 16:59 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Matthieu Moy, Alex Henrie,
Antoine Delaite
In-Reply-To: <01020157c38b19e0-81123fa5-5d9d-4f64-8f1b-ff336e83ebe4-000000@eu-west-1.amazonses.com>
Cc'ed those who touched either "git-bisect.sh" or "builtin/bisect-helper.c"
in our relatively recent past.
Does any of you (and others on the list) have time and inclination
to review this series?
Thanks.
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Junio C Hamano @ 2016-10-27 16:23 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <87d1il3hhs.fsf@linux-m68k.org>
Andreas Schwab <schwab@linux-m68k.org> writes:
> On Okt 25 2016, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Somebody with a bright idea decided that vc-git-resolve-conflicts
>> variable should be on by default in Emacs 25.1 X-<
>
> This is consistent with the behaviour of the other VC backends, where it
> isn't even customizable.
The problem I had was "M-x save-buffer" (after resolving the
conflicts in it manually) running "git add" on the resulting file,
robbing from "git diff" an opportunity to help the user to see how
the result looks relative to both branches.
Do you mean that VC mode broke the same feature equally other SCMs,
too? Do other SCM supported by VC backends take advantage of
unmerged stages in the index (until you say "$scm add") by allowing
you to verify the combined diff with "$scm diff"?
^ 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