* [PATCH] valgrind: support test helpers
From: René Scharfe @ 2016-10-27 22:14 UTC (permalink / raw)
To: Git List; +Cc: Duy Nguyen, Johannes Schindelin, Junio C Hamano
Tests run with --valgrind call git commands through a wrapper script
that invokes valgrind on them. This script (valgrind.sh) is in turn
invoked through symlinks created for each command in t/valgrind/bin/.
Since e6e7530d (test helpers: move test-* to t/helper/ subdirectory)
these symlinks have been broken for test helpers -- they point to the
old locations in the root of the build directory. Fix that by teaching
the code for creating the links about the new location of the binaries,
and do the same in the wrapper script to allow it to find its payload.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
t/test-lib.sh | 9 ++++++++-
t/valgrind/valgrind.sh | 12 ++++++++++--
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b859db6..a724181 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -809,7 +809,14 @@ then
return;
base=$(basename "$1")
- symlink_target=$GIT_BUILD_DIR/$base
+ case "$base" in
+ test-*)
+ symlink_target="$GIT_BUILD_DIR/t/helper/$base"
+ ;;
+ *)
+ symlink_target="$GIT_BUILD_DIR/$base"
+ ;;
+ esac
# do not override scripts
if test -x "$symlink_target" &&
test ! -d "$symlink_target" &&
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 4215303..669ebaf 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -1,11 +1,19 @@
#!/bin/sh
base=$(basename "$0")
+case "$base" in
+test-*)
+ program="$GIT_VALGRIND/../../t/helper/$base"
+ ;;
+*)
+ program="$GIT_VALGRIND/../../$base"
+ ;;
+esac
TOOL_OPTIONS='--leak-check=no'
test -z "$GIT_VALGRIND_ENABLED" &&
-exec "$GIT_VALGRIND"/../../"$base" "$@"
+exec "$program" "$@"
case "$GIT_VALGRIND_MODE" in
memcheck-fast)
@@ -29,4 +37,4 @@ exec valgrind -q --error-exitcode=126 \
--log-fd=4 \
--input-fd=4 \
$GIT_VALGRIND_OPTIONS \
- "$GIT_VALGRIND"/../../"$base" "$@"
+ "$program" "$@"
--
2.10.1
^ permalink raw reply related
* Re: feature request
From: John Rood @ 2016-10-27 22:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZAfqxB699MOs6A6RL==Ku-qF7ABiW=eA+TSrqK+8e_sA@mail.gmail.com>
Unfortunately, in my case I'm on windows (my company's choice, not mine).
On Thu, Oct 27, 2016 at 5:01 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
>> Users should be able to configure Git to not send them into a Vim editor.
>
> See https://git-scm.com/docs/git-var
>
> GIT_EDITOR
>
> Text editor for use by Git commands. The value is meant to be interpreted
> by the shell when it is used. Examples: ~/bin/vi, $SOME_ENVIRONMENT_VARIABLE,
> "C:\Program Files\Vim\gvim.exe" --nofork. The order of preference is the
> $GIT_EDITOR environment variable, then core.editor configuration, then
> $VISUAL, then $EDITOR, and then the default chosen at compile time,
> which is usually vi.
>
>
> So maybe
>
> git config --global core.editor "nano"
>
> helps in your case?
^ permalink raw reply
* Re: feature request
From: Stefan Beller @ 2016-10-27 22:01 UTC (permalink / raw)
To: John Rood; +Cc: git@vger.kernel.org
In-Reply-To: <CALj-rGeoT_mpmuw8Put=6eRhzf-r2WUohu_Kd-wnpc=BvO5joA@mail.gmail.com>
On Thu, Oct 27, 2016 at 2:55 PM, John Rood <mr.john.rood@gmail.com> wrote:
> Users should be able to configure Git to not send them into a Vim editor.
See https://git-scm.com/docs/git-var
GIT_EDITOR
Text editor for use by Git commands. The value is meant to be interpreted
by the shell when it is used. Examples: ~/bin/vi, $SOME_ENVIRONMENT_VARIABLE,
"C:\Program Files\Vim\gvim.exe" --nofork. The order of preference is the
$GIT_EDITOR environment variable, then core.editor configuration, then
$VISUAL, then $EDITOR, and then the default chosen at compile time,
which is usually vi.
So maybe
git config --global core.editor "nano"
helps in your case?
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Stefan Beller @ 2016-10-27 21:59 UTC (permalink / raw)
To: Jacob Keller
Cc: Johannes Sixt, Junio C Hamano, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <CA+P7+xpckfaeHmoEGQBdLD-=Kf7gQ-jOxGFKrKmiFH1SBN7GjA@mail.gmail.com>
> there isn't really a great place to put a dynamic initialization.
Answering this question specifically (Where to put it?),
I am about to send out a patch that puts it in compat/mingw.c:2232:
diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5..9881c3d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2232,6 +2232,9 @@ void mingw_startup(void)
/* initialize critical section for waitpid pinfo_t list */
InitializeCriticalSection(&pinfo_cs);
+ /* initialize critical sections in the attr code */
+ start_attr();
+
/* set up default file mode and file modes for stdin/out/err */
_fmode = _O_BINARY;
_setmode(_fileno(stdin), _O_BINARY);
If I understood Johannes correctly this is the place to put it.
Junio seems to be ok with static mutex init for non windows platforms,
so I put it into the mingw specifc startup code.
Thanks,
Stefan
^ permalink raw reply related
* feature request
From: John Rood @ 2016-10-27 21:55 UTC (permalink / raw)
To: git
Users should be able to configure Git to not send them into a Vim editor.
When users pull commits, and a new commit needs to be created for a
merge, Git's current way of determining a commit message is to send
the user into a Vim window so that they can write a message. There are
2 reasons why this might not be the ideal way to prompt for a commit
message.
1. Many users are used to writing concise one-line commit messages and
would not expect to save a commit message in a multi-line file. Some
users will wonder why they are in a text editor or which file they are
editing. Others may not, in fact, realize at all that a text editor is
what they are in.
2. Many users are not familiar with Vim, and do not understand how to
modify, save, and exit. It is not very considerate to require a user
to learn Vim in order to finish a commit that they are in the middle
of.
The existing behavior should be optional, and there should be two new options:
1. Use a simple inline prompt for a commit message (in the same way
Git might prompt for a username).
2. Automatically assign names for commits in the form of "Merged x into y".
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 21:55 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161027205508.vqw44zlbnqpj2cvd@sigill.intra.peff.net>
On 28/10/16 09:55, Jeff King wrote:
> 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.
The things you say make sense from this perspective.
I was hoping to employ this mechanism throughout the git ecosystem.
Thinking out loud for a minute:
1) I clone a repo with a hostile ignore file. It includes files from
/etc/ssl/private or some such. Change. Don't pay attention. Commit.
Push. Problems.
What is the use case for reaching out of the repo in the first place?
2) I fetch a repo with a hostile ignore file. It includes files from
$GIT_DIR/test-data/ssl/private or some such. Change. Don't pay
attention. Commit. Push. Problems if my test data comes from production.
Is this mitigated currently?
Not that git should be an enabler, but surely it falls on the user of
untrusted software to ensure their own security?
> 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.
I'm quickly getting over the include file idea. But yes, that would be
non obvious.
>>> 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).
Seeing your perspective, now, I can see why you didn't understand me. In
your context this makes perfect sense.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Jacob Keller @ 2016-10-27 21:49 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Junio C Hamano, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <a2e5acd6-485d-0387-7a85-6042dee702f7@kdbg.org>
On Thu, Oct 27, 2016 at 1:05 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> 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
>
Ok, so I've been reading this thread. I don't understand your
objections to emulating in this way.. Could you clearly spell out why
you believe this solution isn't acceptable? So far all I've understood
was "it's not critical sections" and "it penalizes Windows too much"
but... If Windows cannot statically initialize a pthread mutex, then
we *have* to dynamically initialize it somewhere. This solution adds a
single check before each lock and is safe due to use of memory
barriers. Yes, this will cost a tiny bit extra overhead for each use
of "pthread_mutex_lock" but I fail to see how that is a huge
penalty...
So I'm trying to understand if that really is your only concern,
because I don't actually buy that it is a huge overhead to pay.
I agree with Stefan that there isn't really a great place to put a
dynamic initialization.
To be clear I am trying to understand the objections since so far all
I have read is "this is bad, and I object" without clearly explaining
reasoning.
Regards,
Jake
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-27 21:49 UTC (permalink / raw)
To: Jeff King
Cc: Linus Torvalds, git, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <20161027102419.dbzigj7wtr355ofh@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +cc Linus as the original author of 144bde78e9 in case there is
> something subtle I'm missing, but this really just seems like it's
> an outdated optimization.
>
> -- >8 --
> Subject: [PATCH] sha1_file: stop opening files with O_NOATIME
>
> When we open object files, we try to do so with O_NOATIME.
> This dates back to 144bde78e9 (Use O_NOATIME when opening
> the sha1 files., 2005-04-23), which is an optimization to
> avoid creating a bunch of dirty inodes when we're accessing
> many objects. But a few things have changed since then:
>
> 1. In June 2005, git learned about packfiles, which means
> we would do a lot fewer atime updates (rather than one
> per object access, we'd generally get one per packfile).
>
> 2. In late 2006, Linux learned about "relatime", which is
> generally the default on modern installs. So
> performance around atimes updates is a non-issue there
> these days.
>
> All the world isn't Linux, but as it turns out, Linux
> is the only platform to implement O_NOATIME in the
> first place.
>
> So it's very unlikely that this code is helping anybody
> these days.
>
> It's not a particularly large amount of code, but the
> fallback-retry creates complexity. E.g., we do a similar
> fallback for CLOEXEC; which one should take precedence, or
> should we try all possible combinations? Dropping O_NOATIME
> makes those questions go away.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
We may want to lose the surrounding for (;;) loop as there is only
one flag to retry without, which was the original code structure
back when 144bde78e9 ("Use O_NOATIME when opening the sha1 files.",
2005-04-23) was written and refactored by 44d1c19ee8 ("Make loose
object file reading more careful", 2008-06-14).
IOW, this on top. The update to ce_compare_data() Lars has in
a0a6cb9662 ("read-cache: make sure file handles are not inherited by
child processes", 2016-10-24) could then made into a call to
git_open().
sha1_file.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 6f02a57d8b..e18ea053e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1553,24 +1553,17 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
int git_open(const char *name)
{
- static int sha1_file_open_flag = O_CLOEXEC;
-
- for (;;) {
- int fd;
-
- errno = 0;
- fd = open(name, O_RDONLY | sha1_file_open_flag);
- if (fd >= 0)
- return fd;
+ static int cloexec = O_CLOEXEC;
+ int fd;
+ errno = 0;
+ fd = open(name, O_RDONLY | cloexec);
+ if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
- if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
- sha1_file_open_flag &= ~O_CLOEXEC;
- continue;
- }
-
- return -1;
+ cloexec &= ~O_CLOEXEC;
+ fd = open(name, O_RDONLY | cloexec);
}
+ return fd;
}
static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
^ permalink raw reply related
* Re: Expanding Includes in .gitignore
From: Jacob Keller @ 2016-10-27 21:39 UTC (permalink / raw)
To: Jeff King; +Cc: Aaron Pelly, Git mailing list
In-Reply-To: <20161027210458.ptzh4y75dkfaixeo@sigill.intra.peff.net>
On Thu, Oct 27, 2016 at 2:04 PM, Jeff King <peff@peff.net> wrote:
> I'm not convinced this is needed for in-repo .gitignore files. The point
> is that you are pulling together separate files that may be administered
> independently. But git repositories inherently have a whole-project
> view. I'm not sure that separate files buy you a lot there. And the
> compatibility issues are more complicated.
>
I had just assumed it would be used everywhere, so I hadn't really
considered whether that was any reason not to. I personally like the
idea of splitting my git ignores into separate files just so each file
can be about one thing, but I guess it's not really a problem either
way.
> I do agree that:
>
> cd .git/info
> git clone /my/exclude/repo exclude ;# or exclude.d
>
> should work; ignoring dotfiles when reading the directory solves that,
> and is a pretty standard solution.
>
> -Peff
^ permalink raw reply
* Re: [PATCH 32/36] pathspec: allow querying for attributes
From: Stefan Beller @ 2016-10-27 21:32 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List, Brandon Williams
In-Reply-To: <CACsJy8DhsaTY1w_e-0O5d4KLxr4Gmo3g9rDagQq1ooSHywcRoQ@mail.gmail.com>
On Wed, Oct 26, 2016 at 6:33 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> (sorry if this should have been answered if I went through the series
> patch by patch, I wanted to do a proper review but finally have to
> admit to myself I won't, so I just skim through a single giant diff
> instead)
>
> On Sun, Oct 23, 2016 at 6:32 AM, Stefan Beller <sbeller@google.com> wrote:
>> +attr;;
>> +After `attr:` comes a space separated list of "attribute
>> +requirements", all of which must be met in order for the
>> +path to be considered a match;
>
> What about (attr=abc def,attr=ghi lkj)? Does it mean (abc && def) ||
> (ghi && lkj), or abc && def && ghi && lkj? Or is it forbidden to have
> multiple 'attr' attribute in the same pathspec?
Good point. I'll add a test for that.
Remembering the original discussion, multiple attrs
are forbidden for now as it is unclear what you want to see
as a user.
To model (abc && def) || (ghi && lkj), you would need to give
multiple pathspec items as these are naturally ORed:
git ls-files :(attr:abc def) :(attr:ghi lkj) .
(compare "git ls-files Makefile README" which gives
2 files to you, that are named respectively.)
To get "abc && def && ghi && lkj" you go with
git ls-files :(attr:abc def ghi lkj) .
as then all things included into this one attr are
ANDed. I hope the documentation is clear for one
attr.
Thanks,
Stefan
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 21:26 UTC (permalink / raw)
To: Jeff King, Jacob Keller; +Cc: Git mailing list
In-Reply-To: <20161027210458.ptzh4y75dkfaixeo@sigill.intra.peff.net>
On 28/10/16 10:04, Jeff King wrote:
> On Thu, Oct 27, 2016 at 12:48:34PM -0700, Jacob Keller wrote:
>
>>> 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.
>
> One thing I will say about this approach is that you can implement it
> without any changes in git by doing:
>
> path=.git/info/exclude
> cat $path.d/* >$path
>
> and I have seen several config mechanisms basically do that (e.g.,
> Debian packaging for a program that doesn't have its own ".d" mechanism,
> but needs to grab config provided by several separate packages).
>
> The reason to teach that trick to git is convenience; you don't have to
> remember to build the master file from its parts because it's done
> dynamically whenever git needs to look at it.
Precisely.
>> 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
>
> I'm not convinced this is needed for in-repo .gitignore files. The point
> is that you are pulling together separate files that may be administered
> independently. But git repositories inherently have a whole-project
> view. I'm not sure that separate files buy you a lot there. And the
> compatibility issues are more complicated.
>
> I do agree that:
>
> cd .git/info
> git clone /my/exclude/repo exclude ;# or exclude.d
>
> should work; ignoring dotfiles when reading the directory solves that,
> and is a pretty standard solution.
You raise a good point about the requirement. I was about to make an
argument in favour of it, but I argued myself out of an argument. I will
say; why have it work in one place and not others?
^ permalink raw reply
* Re: [PATCH v2 2/2] convert.c: stream and fast search for binary
From: Junio C Hamano @ 2016-10-27 21:18 UTC (permalink / raw)
To: tboegi; +Cc: git
In-Reply-To: <20161012134727.28365-1-tboegi@web.de>
tboegi@web.de writes:
> From: Torsten Bögershausen <tboegi@web.de>
>
> When statistics are done for the autocrlf handling, the search in
> the content can be stopped, if e.g
> - a search for binary is done, and a NUL character is found
> - a search for CRLF is done, and the first CRLF is found.
>
> Similar when statistics for binary vs non-binary are gathered:
> Whenever a lone CR or NUL is found, the search can be aborted.
>
> When checking out files in "auto" mode, any file that has a "lone CR"
> or a CRLF will not be converted, so the search can be aborted early.
>
> Add the new bit, CONVERT_STAT_BITS_ANY_CR,
> which is set for either lone CR or CRLF.
>
> Many binary files have a NUL very early and it is often not necessary
> to load the whole content of a file or blob into memory.
>
> Split gather_stats() into gather_all_stats() and gather_stats_partly()
> to do a streaming handling for blobs and files in the worktree.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
I'll try to review in reverse order, as this seems to be doing too
many things at once and cannot get my head around it without going
top down.
> @@ -338,11 +401,15 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
> {
> char *to_free = NULL;
> struct text_stat stats;
> + unsigned search_only = 0;
>
> if (!len || output_eol(crlf_action) != EOL_CRLF)
> return 0;
>
> - gather_stats(src, len, &stats);
> + if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_CRLF)
> + search_only = CONVERT_STAT_BITS_ANY_CR | CONVERT_STAT_BITS_BIN;
> +
> + gather_all_stats(src, len, &stats, search_only);
> if (!will_convert_lf_to_crlf(len, &stats, crlf_action))
> return 0;
This special case to decide whether we would limit the search_only
flag has too much intimate knowledge of what happens inside
will_convert_lf_to_crlf(). It knows that output_eol(crlf_action)
not being EOL_CRLF is the very first thing the function checks, too.
Makes one wonder if the check for output_eol(crlf_action) can be
removed from will_convert_lf_to_crlf(), no? It is not apparent if
that is a good idea for the other caller in crlf_to_git().
gather_all_stats() will give up immediately when it sees either
ANY_CR or BIN. If CR appears before we see any BIN, stat_bits would
not have BITS_BIN even if the buffer may have BIN byte later. It is
OK because either lonecr or crlf would be non-zero, and
will_convert_lf_to_crlf() would return 0. If BIN apepars before we
see any CR, neither lonecr nor crlf will become non-zero even if the
buffer may have CR byte later, but again it is OK because
will_convert_lf_to_crlf() will return 0 in that case.
This looks too brittle, even though it is correct.
> @@ -253,7 +300,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> {
> struct text_stat stats;
> char *dst;
> - int convert_crlf_into_lf;
> + int has_crlf_to_convert;
> + unsigned search_only = 0;
>
> if (crlf_action == CRLF_BINARY ||
> (src && !len))
> @@ -266,12 +314,16 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> if (!buf && !src)
> return 1;
>
> - gather_stats(src, len, &stats);
> + if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF)
> + search_only = CONVERT_STAT_BITS_BIN;
> +
> + gather_all_stats(src, len, &stats, search_only);
> +
> /* Optimization: No CRLF? Nothing to convert, regardless. */
> - convert_crlf_into_lf = !!stats.crlf;
> + has_crlf_to_convert = !!stats.crlf;
The comment here may need to say a lot more, now we do not even
count .crlf in some cases because of "search_only" setting.
> if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
The new "search_only" criteria above was added to match this if
block; it is not as bad as the previous one in crlf_to_worktree()
that knows, and must be kept in sync with, what a separate function
will_convert_lf_to_crlf() does, but still it is horrible for both
maintainability and readability. Can you devise some mechanism to
ensure that these two if statements will stay in sync?
> - if (convert_is_binary(len, &stats))
> + if (stats.stat_bits & CONVERT_STAT_BITS_BIN)
> return 0;
We no longer need the helper function convert_is_binary() and
instead need only STATS_BITS_BIN bit, so the control flow that
reaches this point is obviously correct (assuming that
gather_all_stats() that is limited with search_only option counts
things correctly, that is).
But what happens when we don't return here? We didn't get the full
stats out of gather_all_stats() and continue. Let's see what
happens in that case...
> /*
> * If the file in the index has any CR in it, do not convert.
> @@ -280,24 +332,35 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> if (checksafe == SAFE_CRLF_RENORMALIZE)
> checksafe = SAFE_CRLF_FALSE;
> else if (has_cr_in_index(path))
> - convert_crlf_into_lf = 0;
> + has_crlf_to_convert = 0;
> }
So at this point, we cannot trust what is in "stats" (we may have
come out of the above if() statement because it wasn't binary after
all). It is unclear to me if we may or may not be able to trust
has_crlf_to_convert at this point. If crlf_action was one of the
three magic values that caused search_only for BITS_BIN set, then
stats.crlf may or may not have seen CRLF---if a NUL came before any
CRLF, gather_all_stats() would have returned without seeing a CRLF
that exists, and otherwise it may have seen one and counted, so
has_crlf_to_convert that is set immediately after gather_all_stats()
returned cannot be trusted at all, when BITS_BIN was set in the
result.
What saves this codepath is that we would have returned at this
point if BITS_BIN was set in the result, so stats.crlf immediately
after gather_all_stats() returned can be trusted in that case, which
in turn means has_crlf_to_convert can also be trusted here. Whew.
I hate to say this, and it certainly is not the fault of this patch,
but the result of applying this patch is undecipherable without a
great effort and is too brittle. A reviewer or any future developer
who has to touch this codepath should not be forced to do this kind
of analysis. Either the code should make everything I wrote above
clear by itself, or more in-code comment must talk about these
things.
Anyway, let's say we established that has_crlf_to_convert and
checksafe can be trustable at this point in the control flow, and
let's keep reading.
> if (checksafe && len) {
> struct text_stat new_stats;
> memcpy(&new_stats, &stats, sizeof(new_stats));
> /* simulate "git add" */
> - if (convert_crlf_into_lf) {
> + if (has_crlf_to_convert) {
> new_stats.lonelf += new_stats.crlf;
> new_stats.crlf = 0;
> + /* all crlf, if any, are gone. Update the bits */
> + new_stats.stat_bits = stats.stat_bits & CONVERT_STAT_BITS_BIN;
> + if (new_stats.lonelf)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_TXT_LF;
> + if (new_stats.lonecr)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_ANY_CR;
What's happening here? stats.crlf and stats.lonelf are both
trustable, because even when BITS_BIN optimization were asked when
calling gather_all_stats(), we wouldn't come here if the
optimization actually kicked in. If we convert crlf to lf, the
result would have more lonelf than the original by the number of
crlf to be converted, and the result would have no crlf, and
new_stats are adjusted to pretend as if we have ran the stat over
the buffer after conversion. You further adjust .stat_bits, which
were not necessary in the old code. The new comment says "Update
the bits" but what it should make it clear is why the new code cares
about the bits when the old code didn't. ANY_CR is the new thing
and is understandable, but old code didn't flip TXT_LF bit. Is it a
bugfix in the old code and it should have done so without your "find
partially and return early" optimization? If so, that should have
been a separate patch to be understandable.
I am guessing that this is an attempt to future-proof the contents
of new_stats, so that will_convert_lf_to_crlf() can take a short-cut
by looking at these bits, even though currently it does not look at
any bit other than BITS_BIN. If that is the case, that needs to be
told to the reader. "Update the bits" is something any reader can
see. You need to tell them why you are updating the bits.
> }
> /* simulate "git checkout" */
> if (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {
> new_stats.crlf += new_stats.lonelf;
> new_stats.lonelf = 0;
> + new_stats.stat_bits = stats.stat_bits & CONVERT_STAT_BITS_BIN;
> + if (new_stats.crlf)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_TXT_CRLF | CONVERT_STAT_BITS_ANY_CR;
> + if (new_stats.lonecr)
> + new_stats.stat_bits |= CONVERT_STAT_BITS_ANY_CR;
> }
> check_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);
Likewise. Is this future-proofing new_stats to allow check_safe_crlf()
to use them in the future?
> @@ -86,41 +97,62 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
> * The same heuristics as diff.c::mmfile_is_binary()
> * We treat files with bare CR as binary
> */
> -static int convert_is_binary(unsigned long size, const struct text_stat *stats)
> +static void convert_nonprintable(struct text_stat *stats)
> {
> - if (stats->lonecr)
> - return 1;
> - if (stats->nul)
> - return 1;
> if ((stats->printable >> 7) < stats->nonprintable)
> - return 1;
> - return 0;
> + stats->stat_bits |= CONVERT_STAT_BITS_BIN;
> }
When search_only is set to BIN, stat_bits would have BIN if we saw
any non-printable control byte, so calling convert_nonprintable() at
the end of gather_all_stats() to flip the BIN bit on with
printable/nonprintable ratio has no effect in that case. If we
didn't see a non-printable control byte, gather_stats_partly()
wouldn't have set BIN in the result but then we know the count of
printable and nonprintable are trustworthy, so comparison to flip
the bit makes sense.
The presense of lonecr does not matter, as that would have flipped
BIN, too. So does the check for NUL, too, has become unnecessary.
I think the end result may be likely to be correct, but with too
many things going on at once, I cannot be confident in saying so.
This probably should be done as four more patches to become
reviewable.
- One to use the CONVERT_STAT_BITS a lot more for the conversion
decision than before,
- another to allow the caller to tell gather_stats() to give up
early with the "search_only" bits,
- another to update the get_*_convert_stats() functions to use
get_convert_stats_sha1(), and then finally
- use the streaming interface when reading from blob and file.
or something line that.
Thanks.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-27 21:07 UTC (permalink / raw)
To: Aaron Pelly; +Cc: git
In-Reply-To: <20161027205508.vqw44zlbnqpj2cvd@sigill.intra.peff.net>
On Thu, Oct 27, 2016 at 04:55:08PM -0400, Jeff King wrote:
> 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.
Actually, I suppose even a ".gitignore.d" inside the repo solves that
problem, too, because the repository is not specifying paths, only
content (it can specify symlinks outside the repository, but like other
parts of git, we should be careful not to follow them in that case).
However, as I said elsewhere, I'm not convinced this feature is all that
helpful for in-repository .gitignore files, and I think it does
introduce compatibility complications. People with older git will not
respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
matter.
But perhaps there is a use case I'm missing.
-Peff
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-27 21:04 UTC (permalink / raw)
To: Jacob Keller; +Cc: Aaron Pelly, Git mailing list
In-Reply-To: <CA+P7+xqmVM-bEc7sZcn+p3qhFUUJvC+rko7CYu+KgyTAeiOifw@mail.gmail.com>
On Thu, Oct 27, 2016 at 12:48:34PM -0700, Jacob Keller wrote:
> > 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.
One thing I will say about this approach is that you can implement it
without any changes in git by doing:
path=.git/info/exclude
cat $path.d/* >$path
and I have seen several config mechanisms basically do that (e.g.,
Debian packaging for a program that doesn't have its own ".d" mechanism,
but needs to grab config provided by several separate packages).
The reason to teach that trick to git is convenience; you don't have to
remember to build the master file from its parts because it's done
dynamically whenever git needs to look at it.
> 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
I'm not convinced this is needed for in-repo .gitignore files. The point
is that you are pulling together separate files that may be administered
independently. But git repositories inherently have a whole-project
view. I'm not sure that separate files buy you a lot there. And the
compatibility issues are more complicated.
I do agree that:
cd .git/info
git clone /my/exclude/repo exclude ;# or exclude.d
should work; ignoring dotfiles when reading the directory solves that,
and is a pretty standard solution.
-Peff
^ permalink raw reply
* 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
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