* Re: [PATCH] Put sha1dc on a diet
From: Johannes Schindelin @ 2017-03-02 14:39 UTC (permalink / raw)
To: Jeff King
Cc: Linus Torvalds, Junio C Hamano, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <20170301231921.2puf7o7jkrujscwn@sigill.intra.peff.net>
Hi Peff,
On Wed, 1 Mar 2017, Jeff King wrote:
> I do think that could argue for turning on the collision detection only
> during object-write operations, which is where it matters. It would be
> really trivial to flip the "check collisions" bit on sha1dc. But I
> suspect you could go faster still by compiling against two separate
> implementations: the fast-as-possible one (which could be openssl or
> blk-sha1), and the slower-but-careful sha1dc.
Given the speed difference between OpenSSL and sha1dc, it would be a wise
thing indeed to do sha1dc only where objects enter from possibly untrusted
sources, and use OpenSSL for all other hashing.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Lars Schneider @ 2017-03-02 11:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.2.20.1703021210170.3767@virtualbox>
> On 02 Mar 2017, at 12:24, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> Hi Lars,
>
> On Thu, 2 Mar 2017, Lars Schneider wrote:
>
>> The patch looks good to me in general but I want to propose the following
>> changes:
>
> I know you are using your script to generate this mail, but I would have
> liked to see v2 in the subject ;-)
Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in
my email client. Now I am wondering... is the next version v2 or v3 :D
>> (1) Move all the docker magic into a dedicated file
>> "ci/run-linux-32-build.sh" This way people should be able to run this
>> build on their local machines without TravisCI. However, I haven't
>> tested this.
>
> I considered this, but there is serious overlap: the `docker pull` call
> and the `docker run` call *have* to refer to the same image. It's very
> easy for them to get out of sync if you have that information in two
> files. Maybe make that an option of the script, defaulting to
> daald/ubuntu32:xenial?
Right. I missed that. How about something like that?
before_install:
- ci/run-linux32-build.sh --pull-container
before_script:
script: ci/run-linux32-build.sh
> BTW speaking of Docker: it would be nicer if there was a Docker image that
> already had the build-essentials installed, to save on startup time. But I
> did not find any that was reasonably up-to-date.
True. But installing everything just takes a minute and we don't need to
maintain anything...
>> +set -e
>
> Is this really necessary? I really like to avoid `set -e`, in particular
> when we do pretty much everything in && chains anyway.
Agreed, not really necessary here as we just invoke one command.
Out of curiosity: Why do you try to avoid it? I set it by default in all
my scripts.
>> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
>> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
>> +
>> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
>> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
>> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
>> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
>> +
>> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
>> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
>> +
>> +sudo docker run \
>> + --interactive --volume "${PWD}:/usr/src/git" \
>> + daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
>
> Hmm. Since it is a script now, it would be more readable this way, I
> think:
>
> sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
> linux32 --32bit i386 sh -c '
> : update packages first &&
> apt update >/dev/null &&
> apt install -y build-essential libcurl4-openssl-dev libssl-dev \
> libexpat-dev gettext python >/dev/null &&
>
> : now build and test &&
> cd /usr/src/git &&
> DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
> GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
> GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
> GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
> make -j2 test
> '
That looks better! I'll try it!
- Lars
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Johannes Schindelin @ 2017-03-02 14:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Junio C Hamano, Jeff King, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <CA+55aFy9=jBJT36FC2HiAeabJBssY=jE=zLxwrXWzhpiFkMUXg@mail.gmail.com>
Hi Linus,
On Wed, 1 Mar 2017, Linus Torvalds wrote:
> On Wed, Mar 1, 2017 at 2:51 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > But I think bigger than just developers on Windows OS. There are many
> > developers out there working on large repositories (yes, much larger
> > than Linux). Also using Macs and Linux. I am not at all sure that we
> > want to give them an updated Git they cannot fail to notice to be much
> > slower than before.
>
> Johannes, have you *tried* the patches?
>
> I really don't think you have. It is completely unnoticeable in any
> normal situation. The two cases that it's noticeable is:
>
> - a full fsck is noticeable slower
>
> - a full non-local clone is slower (but not really noticeably so
> since the network traffic dominates).
>
> In other words, I think you're making shit up. I don't think you
> understand how little the SHA1 performance actually matters. It's
> noticeable in benchmarks. It's not noticeable in any normal operation.
>
> .. and yes, I've actually been running the patches locally since I
> posted my first version (which apparently didn't go out to the list
> because of list size limits) and now running the version in 'pu'.
If you think that the Linux repository is a big one, then your reaction is
understandable.
I have zero interest in potty language, therefore my reply is very terse:
yes, I have been looking ad SHA-1 performance, and yes, it matters. Think
an index file of 300-400MB.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Johannes Schindelin @ 2017-03-02 14:22 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster
In-Reply-To: <CFA1C4B4-0FDA-424D-87A4-EEE1F9BB3712@gmail.com>
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
> > On 02 Mar 2017, at 12:24, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 2 Mar 2017, Lars Schneider wrote:
> >
> >> The patch looks good to me in general but I want to propose the
> >> following changes:
> >
> > I know you are using your script to generate this mail, but I would
> > have liked to see v2 in the subject ;-)
>
> Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in my
> email client. Now I am wondering... is the next version v2 or v3 :D
Since there was no v2, the next one should *definitely* be v2... ;-)
> >> (1) Move all the docker magic into a dedicated file
> >> "ci/run-linux-32-build.sh" This way people should be able to run this
> >> build on their local machines without TravisCI. However, I haven't
> >> tested this.
> >
> > I considered this, but there is serious overlap: the `docker pull`
> > call and the `docker run` call *have* to refer to the same image. It's
> > very easy for them to get out of sync if you have that information in
> > two files. Maybe make that an option of the script, defaulting to
> > daald/ubuntu32:xenial?
>
> Right. I missed that. How about something like that?
>
> before_install:
> - ci/run-linux32-build.sh --pull-container
> before_script:
> script: ci/run-linux32-build.sh
I'd prefer
before_install:
- docker pull daald/ubuntu32:xenial
before_script:
script: ci/run-linux32-build.sh daald/ubuntu32:xenial
> > BTW speaking of Docker: it would be nicer if there was a Docker image
> > that already had the build-essentials installed, to save on startup
> > time. But I did not find any that was reasonably up-to-date.
>
> True. But installing everything just takes a minute and we don't need to
> maintain anything...
And when there are network problems (like there were on Tuesday, right
when I developed the first v1 of this patch) then we have another set of
problems that make Travis fail. Even if the code in the PR or branch is
actually good. I'd like to avoid false positives, if possible.
> >> +set -e
> >
> > Is this really necessary? I really like to avoid `set -e`, in
> > particular when we do pretty much everything in && chains anyway.
>
> Agreed, not really necessary here as we just invoke one command. Out of
> curiosity: Why do you try to avoid it? I set it by default in all my
> scripts.
I try to avoid it because it encourages a style that omits helpful error
messages.
> >> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
> >> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
> >> +
> >> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
> >> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
> >> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
> >> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
> >> +
> >> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
> >> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
> >> +
> >> +sudo docker run \
> >> + --interactive --volume "${PWD}:/usr/src/git" \
> >> + daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
> >
> > Hmm. Since it is a script now, it would be more readable this way, I
> > think:
> >
> > sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
> > linux32 --32bit i386 sh -c '
> > : update packages first &&
> > apt update >/dev/null &&
> > apt install -y build-essential libcurl4-openssl-dev libssl-dev \
> > libexpat-dev gettext python >/dev/null &&
> >
> > : now build and test &&
> > cd /usr/src/git &&
> > DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
> > GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
> > GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
> > GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
> > make -j2 test
> > '
>
> That looks better! I'll try it!
Thanks!
Dscho
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Mike Crowe @ 2017-03-02 14:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: tboegi, git, Jeff King
In-Reply-To: <xmqqa894fyst.fsf@gitster.mtv.corp.google.com>
On Wednesday 01 March 2017 at 13:54:26 -0800, Junio C Hamano wrote:
> Now I thought about it through a bit more thoroughly, I think this
> is the right approach, so here is my (tenative) final version.
>
> I seem to be getty really rusty---after all the codepaths involved
> are practically all my code and I should have noticed the real
> culprit during my first attempt X-<.
>
> Thanks for helping.
>
> -- >8 --
> Subject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()
>
> Callers of diff_populate_filespec() can choose to ask only for the
> size of the blob without grabbing the blob data, and the function,
> after running lstat() when the filespec points at a working tree
> file, returns by copying the value in size field of the stat
> structure into the size field of the filespec when this is the case.
>
> However, this short-cut cannot be taken if the contents from the
> path needs to go through convert_to_git(), whose resulting real blob
> data may be different from what is in the working tree file.
>
> As "git diff --quiet" compares the .size fields of filespec
> structures to skip content comparison, this bug manifests as a
> false "there are differences" for a file that needs eol conversion,
> for example.
>
> Reported-by: Mike Crowe <mac@mcrowe.com>
> Helped-by: Torsten Bögershausen <tboegi@web.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff.c | 19 ++++++++++++++++++-
> t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+), 1 deletion(-)
> create mode 100755 t/t0028-diff-converted.sh
>
> diff --git a/diff.c b/diff.c
> index 8c78fce49d..dc51dceb44 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> s->should_free = 1;
> return 0;
> }
> - if (size_only)
> +
> + /*
> + * Even if the caller would be happy with getting
> + * only the size, we cannot return early at this
> + * point if the path requires us to run the content
> + * conversion.
> + */
> + if (!would_convert_to_git(s->path) && size_only)
> return 0;
> +
> + /*
> + * Note: this check uses xsize_t(st.st_size) that may
> + * not be the true size of the blob after it goes
> + * through convert_to_git(). This may not strictly be
> + * correct, but the whole point of big_file_threashold
> + * and is_binary check being that we want to avoid
> + * opening the file and inspecting the contents, this
> + * is probably fine.
> + */
> if ((flags & CHECK_BINARY) &&
> s->size > big_file_threshold && s->is_binary == -1) {
> s->is_binary = 1;
This patch solves the problem for me. Including my tests where the file
size doesn't change but the file has been touched. It also doesn't have the
side effect of failing to report the extra trailing newline that the
original fix suffered from.
All the solutions presented so far do cause a small change in behaviour
when using git diff --quiet: they may now cause warning messages like:
warning: CRLF will be replaced by LF in crlf.txt.
The file will have its original line endings in your working directory.
to be emitted (unless of course core.safecrlf=false.) I think this is an
unavoidable side-effect of doing the job properly but it might be worth
mentioning.
> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh
> new file mode 100755
> index 0000000000..3d5ab9565b
> --- /dev/null
> +++ b/t/t0028-diff-converted.sh
> @@ -0,0 +1,27 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2017 Mike Crowe
> +#
> +# These tests ensure that files changing line endings in the presence
> +# of .gitattributes to indicate that line endings should be ignored
> +# don't cause 'git diff' or 'git diff --quiet' to think that they have
> +# been changed.
> +
> +test_description='git diff with files that require CRLF conversion'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> + echo "* text=auto" >.gitattributes &&
> + printf "Hello\r\nWorld\r\n" >crlf.txt &&
> + git add .gitattributes crlf.txt &&
> + git commit -m "initial"
> +'
> +
> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '
> + printf "Hello\r\nWorld\n" >crlf.txt &&
> + git status &&
> + git diff --quiet
> +'
> +
> +test_done
As I said before, this doesn't actually test the case when the file sizes
match. However, given the way that the code has changed the actual file
sizes are not compared, so perhaps this doesn't matter.
Thanks for all your help investigating this.
Mike.
^ permalink raw reply
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Ramsay Jones @ 2017-03-02 15:17 UTC (permalink / raw)
To: Johannes Schindelin, Lars Schneider; +Cc: git, gitster
In-Reply-To: <alpine.DEB.2.20.1703021210170.3767@virtualbox>
On 02/03/17 11:24, Johannes Schindelin wrote:
> Hi Lars,
>
> On Thu, 2 Mar 2017, Lars Schneider wrote:
>
[snip]
>> One thing that still bugs me: In the Linux32 environment prove adds the
>> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00
>> csys ... Has anyone an idea why that happens and how we can disable it?
>
> I have no idea.
I have no idea either, but it is not unique to this 32bit Linux, but
rather the version of prove. For example, I am seeing this on Linux
Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do
this. (They used different Ubuntu LTS releases).
[Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]
ATB,
Ramsay Jones
^ permalink raw reply
* git status reports file modified when only line-endings have changed (was git diff --quiet exits with 1 on clean tree with CRLF conversions)
From: Mike Crowe @ 2017-03-02 15:38 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Junio C Hamano, git
In-Reply-To: <d98aa589-3e08-249d-0c88-72dbcee1a568@web.de>
On Tuesday 28 February 2017 at 19:06:44 +0100, Torsten Bögershausen wrote:
> My understanding is that git diff --quiet should be quiet, when
> git add will not do anything (but the file is "touched".
> The touched means that Git will detect e.g a new mtime or inode
> or file size when doing lstat().
Does the same apply to "git status"?
If so, then whilst investigating the "git diff --quiet" problems in this
thread I've found a similar bug with "git status". It reports the file has
modifications even if only the line-endings have changed, and issuing "git
add" causes the perceived modification to disappear.
It can be very confusing for users if "git status" reports a modification
but for "git diff" to claim that the files are identical.
This bug is still reproducible even with the fix from
https://public-inbox.org/git/xmqqshmyhtnu.fsf@gitster.mtv.corp.google.com/T/#m67cbfad1f2efe721f0c2afac2a1523b743bb57ca
Here's the test case. Test 3 is the part that currently fails:
commit de5f3f1d9161cdd46342689abe38a046fc71850e
Author: Mike Crowe <mac@mcrowe.com>
Date: Sat Feb 25 09:28:37 2017 +0000
status: Add tests for status output when file line endings change
diff --git a/t/t7518-status-eol-change.sh b/t/t7518-status-eol-change.sh
new file mode 100755
index 0000000..e18186f
--- /dev/null
+++ b/t/t7518-status-eol-change.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Mike Crowe
+#
+
+test_description='git status with files that require CRLF conversion'
+
+. ./test-lib.sh
+
+cat >expected_no_change <<EOF
+On branch master
+nothing to commit, working tree clean
+EOF
+
+test_expect_success setup '
+ echo "* text=auto" > .gitattributes &&
+ printf "Hello\r\nWorld\r\n" > crlf.txt &&
+ printf "expected_no_change\nactual\n" > .gitignore &&
+ git add .gitignore .gitattributes crlf.txt &&
+ git commit -m "initial"
+'
+test_expect_success 'git status reports no change if file regenerated' '
+ printf "Hello\r\nWorld\r\n" > crlf.txt &&
+ git status >actual &&
+ test_cmp expected_no_change actual
+'
+test_expect_success 'git status reports no change if line endings change' '
+ printf "Hello\nWorld\n" > crlf.txt &&
+ git status >actual &&
+ test_cmp expected_no_change actual
+'
+test_expect_success 'git status reports no change if line ending change is staged' '
+ git add crlf.txt &&
+ git status >actual &&
+ test_cmp expected_no_change actual
+'
+test_done
^ permalink raw reply related
* Re: [PATCH] Put sha1dc on a diet
From: Johannes Schindelin @ 2017-03-02 14:45 UTC (permalink / raw)
To: Duy Nguyen
Cc: Jeff King, Linus Torvalds, Junio C Hamano, Marc Stevens,
Dan Shumow, Git Mailing List
In-Reply-To: <CACsJy8D3h1KAaKi_Esc98za3LqXaB=YeW0Yu+VAV9UnX5vmttg@mail.gmail.com>
Hi Duy,
On Thu, 2 Mar 2017, Duy Nguyen wrote:
> On Thu, Mar 2, 2017 at 6:19 AM, Jeff King <peff@peff.net> wrote:
> > You have to remember that some of the Git for Windows users are doing
> > horrific things like using repositories with 450MB .git/index files,
> > and the speed to compute the sha1 during an update is noticeable
> > there.
>
> We probably should separate this use case from the object hashing
> anyway. Here we need a better, more reliable crc32 basically, to detect
> bit flips. Even if we move to SHA-something, we can keep staying with
> SHA-1 here (and with the fastest implementation)
I guess it was convenient to use the same hash algorithm for all hashing
purposes in the beginning. The downside, of course, was that we kept
talking about SHA-1s instead of commit hashes and the index checksum (i.e.
using labels based on implementation details rather than semantically
meaningful names).
In the meantime, we use different hash algorithms where appropriate, of
course, and we typically encapsulate the exact hash algorithm so that it
is easy to switch when/if necessary (think the hash functions for strings
in our hashtables, and the hash functions in xdiff).
It would probably make sense to switch the index integrity check away from
SHA-1 because we really only care about detecting bit flips there, and we
have no need for the computational overhead of using a full-blown
cryptographic hash for that purpose.
Ciao,
Johannes
^ permalink raw reply
* Re: Rebase sequencer changes prevent exec commands from modifying the todo file?
From: Johannes Schindelin @ 2017-03-02 15:25 UTC (permalink / raw)
To: Stephen Hicks; +Cc: git
In-Reply-To: <CAKNkOnM366uiJKkz31hS8V3NTa8qksP2pXrH4+F-zodZaNdsqg@mail.gmail.com>
Hi Stephen,
On Wed, 1 Mar 2017, Stephen Hicks wrote:
> I have a preferred rebase workflow wherein I generate my own TODO file
> with 'exec' lines that further modify the TODO file. I recently noticed
> that these changes weren't sticking: they seemed to persist until the
> end of the exec'd command, and then as soon as the next command ran
> (i.e. GIT_EDITOR=cat git rebase --edit-todo) they were gone.
>
> I don't understand the changes that have been going through, but I suspect
> this is a result of the sequencer refactoring. Is there a way to prevent
> git rebase from overwriting these changes?
>
> To reproduce:
> $ git rebase -i HEAD -x "echo x false >> \"$(git rev-parse
> --git-dir)/rebase-merge/git-rebase-todo\""
>
> This should cause the rebase to fail (and indeed, it does on 2.11.0.390)
> since it inserts an "x false". But somewhere between there and
> 2.12.0.rc1.440, this behavior is changed.
Do you also modify the author-script file to execute arbitrary shell
commands? ;-)
Seriously again, it should not be too much of a deal to handle your use
case by re-reading the git-rebase-todo file if it has changed after an
`exec` has modified it. We already force a re-read of the index.
I won't be able to take care of that immediately, though, as I have a
pressing other patch series I need to get done.
If you want to take a crack at it in the meantime, I think this is where I
would start:
https://github.com/git-for-windows/git/blob/ce4c6ca554/sequencer.c#L2020-L2027
I would probably try to make the code smart by looking at the
timestamp/size/inode fields of the stat data of git-rebase-todo, and only
force a re-read in case those fields are different, the `res` variable is
0, and then the code would need to `continue;` in order to skip the
increment of `todo_list->current`.
And before that, I'd turn your example into a test case in
t/t3405-rebase-malformed.sh...
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Christian Couder @ 2017-03-02 15:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Lars Schneider, git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1703021519330.3767@virtualbox>
On Thu, Mar 2, 2017 at 3:22 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
>> >> +set -e
>> >
>> > Is this really necessary? I really like to avoid `set -e`, in
>> > particular when we do pretty much everything in && chains anyway.
>>
>> Agreed, not really necessary here as we just invoke one command. Out of
>> curiosity: Why do you try to avoid it? I set it by default in all my
>> scripts.
>
> I try to avoid it because it encourages a style that omits helpful error
> messages.
Yeah, we prefer to define and use a die() function like this:
die () {
printf >&2 '%s\n' "$*"
exit 1
}
do_something || die "meaningful error message"
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Linus Torvalds @ 2017-03-02 16:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Duy Nguyen, Jeff King, Junio C Hamano, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1703021539330.3767@virtualbox>
On Thu, Mar 2, 2017 at 6:45 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> It would probably make sense to switch the index integrity check away from
> SHA-1 because we really only care about detecting bit flips there, and we
> have no need for the computational overhead of using a full-blown
> cryptographic hash for that purpose.
Which index do you actually see as being a problem, btw? The main file
index (.git/index) or the pack-file indexes?
We definitely don't need the checking version of sha1 for either of
those, but as Jeff already did the math, at least the pack-file index
is almost negligible, because the pack-file operations that update it
end up doing SHA1 over the objects - and the object SHA1 calculations
are much bigger.
And I don't think we even check the pack-file index hashes except on fsck.
Now, if your _file_ index is 300-400MB (and I do think we check the
SHA fingerprint on that even on just reading it - verify_hdr() in
do_read_index()), then that's going to be a somewhat noticeable hit on
every normal "git diff" etc.
But I'd have expected the stat() calls of all the files listed by that
index to be the _much_ bigger problem in that case. Or do you just
turn those off with assume-unchanged?
Yeah, those stat calls are threaded when preloading, but even so..
Anyway, the file index SHA1 checking could probably just be disabled
entirely (with a config flag). It's a corruption check that simply
isn't that important. So if that's your main SHA1 issue, that would be
easy to fix.
Everything else - like pack-file generation etc for a big clone() may
end up using a ton of SHA1 too, but the SHA1 costs all scale with the
other costs that drown them out (ie zlib, network, etc).
I'd love to see a profile if you have one.
Linus
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Junio C Hamano @ 2017-03-02 17:07 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, Jeff King, git, Jeff Hostetler
In-Reply-To: <fa38e22e-3114-fab8-38ed-5f52125e81bb@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 24.02.2017 um 22:54 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>> I'll use the patch for daily work for a while to see whether it hurts.
>>
>> Please ping this thread again when you have something to add. For
>> now, I'll demote this patch from 'next' to 'pu' when we rewind and
>> rebuild 'next' post 2.12 release.
>
> ... I've used the patch in
> production for some time now and did not notice any slowdowns.
Thanks. Dscho obviously thinks this is the right thing for Windows,
and you agree. That's more than sufficient votes to make me feel
safe ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH 3/5] grep: fix bug when recuring with relative pathspec
From: Brandon Williams @ 2017-03-02 18:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, pclouds
In-Reply-To: <xmqqefyijacj.fsf@gitster.mtv.corp.google.com>
On 02/28, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > /* Add super prefix */
> > + quote_path_relative(name, opt->prefix, &buf);
>
> Hmph, do you want a quoted version here, not just relative_path()?
>
> Perhaps add a test with an "unusual" byte (e.g. a double-quote) in
> the path?
You're absolutely correct.
>
> > argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
> > super_prefix ? super_prefix : "",
> > - name);
> > + buf.buf);
> > + strbuf_release(&buf);
> > argv_array_push(&cp.args, "grep");
> >
> > /*
> > @@ -1199,7 +1202,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >
> > parse_pathspec(&pathspec, 0,
> > PATHSPEC_PREFER_CWD |
> > - (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
> > + (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |
> > + (super_prefix ? PATHSPEC_FROMROOT : 0),
> > prefix, argv + i);
> > pathspec.max_depth = opt.max_depth;
> > pathspec.recursive = 1;
> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> > index 418ba68fe..e0932b2b7 100755
> > --- a/t/t7814-grep-recurse-submodules.sh
> > +++ b/t/t7814-grep-recurse-submodules.sh
> > @@ -227,7 +227,7 @@ test_expect_success 'grep history with moved submoules' '
> > test_cmp expect actual
> > '
> >
> > -test_expect_failure 'grep using relative path' '
> > +test_expect_success 'grep using relative path' '
> > test_when_finished "rm -rf parent sub" &&
> > git init sub &&
> > echo "foobar" >sub/file &&
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Torsten Bögershausen @ 2017-03-02 18:17 UTC (permalink / raw)
To: Mike Crowe; +Cc: git, Junio C Hamano
In-Reply-To: <20170301212535.GA6878@mcrowe.com>
On 2017-03-01 22:25, Mike Crowe wrote:
> On Wednesday 01 March 2017 at 18:04:44 +0100, tboegi@web.de wrote:
>> From: Junio C Hamano <gitster@pobox.com>
>>
>> git diff --quiet may take a short-cut to see if a file is changed
>> in the working tree:
>> Whenever the file size differs from what is recorded in the index,
>> the file is assumed to be changed and git diff --quiet returns
>> exit with code 1
>>
>> This shortcut must be suppressed whenever the line endings are converted
>> or a filter is in use.
>> The attributes say "* text=auto" and a file has
>> "Hello\nWorld\n" in the index with a length of 12.
>> The file in the working tree has "Hello\r\nWorld\r\n" with a length of 14.
>> (Or even "Hello\r\nWorld\n").
>> In this case "git add" will not do any changes to the index, and
>> "git diff -quiet" should exit 0.
>>
>> Add calls to would_convert_to_git() before blindly saying that a different
>> size means different content.
>>
>> Reported-By: Mike Crowe <mac@mcrowe.com>
>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>> ---
>> This is what I can come up with, collecting all the loose ends.
>> I'm not sure if Mike wan't to have the Reported-By with a
>> Signed-off-by ?
>> The other question is, if the commit message summarizes the discussion
>> well enough ?
>>
>> diff.c | 18 ++++++++++++++----
>> t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++
>> 2 files changed, 41 insertions(+), 4 deletions(-)
>> create mode 100755 t/t0028-diff-converted.sh
>>
>> diff --git a/diff.c b/diff.c
>> index 051761b..c264758 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
>> * differences.
>> *
>> * 2. At this point, the file is known to be modified,
>> - * with the same mode and size, and the object
>> - * name of one side is unknown. Need to inspect
>> - * the identical contents.
>> + * with the same mode and size, the object
>> + * name of one side is unknown, or size comparison
>> + * cannot be depended upon. Need to inspect the
>> + * contents.
>> */
>> if (!DIFF_FILE_VALID(p->one) || /* (1) */
>> !DIFF_FILE_VALID(p->two) ||
>> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
>> (p->one->mode != p->two->mode) ||
>> diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
>> diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
>> - (p->one->size != p->two->size) ||
>> +
>> + /*
>> + * only if eol and other conversions are not involved,
>> + * we can say that two contents of different sizes
>> + * cannot be the same without checking their contents.
>> + */
>> + (!would_convert_to_git(p->one->path) &&
>> + !would_convert_to_git(p->two->path) &&
>> + (p->one->size != p->two->size)) ||
>> +
>> !diff_filespec_is_identical(p->one, p->two)) /* (2) */
>> p->skip_stat_unmatch_result = 1;
>> return p->skip_stat_unmatch_result;
>> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh
>> new file mode 100755
>> index 0000000..3d5ab95
>> --- /dev/null
>> +++ b/t/t0028-diff-converted.sh
>> @@ -0,0 +1,27 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2017 Mike Crowe
>> +#
>> +# These tests ensure that files changing line endings in the presence
>> +# of .gitattributes to indicate that line endings should be ignored
>> +# don't cause 'git diff' or 'git diff --quiet' to think that they have
>> +# been changed.
>> +
>> +test_description='git diff with files that require CRLF conversion'
>> +
>> +. ./test-lib.sh
>> +
>> +test_expect_success setup '
>> + echo "* text=auto" >.gitattributes &&
>> + printf "Hello\r\nWorld\r\n" >crlf.txt &&
>> + git add .gitattributes crlf.txt &&
>> + git commit -m "initial"
>> +'
>> +
>> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '
>> + printf "Hello\r\nWorld\n" >crlf.txt &&
>> + git status &&
>> + git diff --quiet
>> +'
>> +
>> +test_done
>
> Hi Torsten,
>
> Thanks for investigating this.
>
> I think that you've simplified the test to the point where it doesn't
> entirely prove the fix. Although you test the case where the file has
> changed size, you don't test the case where it hasn't.
>
> Unfortunately that was the part of my test that could only reproduce the
> problem with the sleeps. Maybe someone who understands how the cache works
> fully could explain an alternative way to force the cache not to be used.
>
> Also, I think I've found a behaviour change with this fix. Consider:
>
> echo "* text=auto" >.gitattributes
> printf "Hello\r\nWorld\r\n" >crlf.txt
That should give
"Hello\nWorld\n" in the index:
git add .gitattributes crlf.txt
warning: CRLF will be replaced by LF in ttt/crlf.txt.
The file will have its original line endings in your working directory.
tb@mac:/tmp/ttt> git commit -m "initial"
[master (root-commit) 354f657] initial
2 files changed, 3 insertions(+)
create mode 100644 ttt/.gitattributes
create mode 100644 ttt/crlf.txt
tb@mac:/tmp/ttt> git ls-files --eol
i/lf w/lf attr/text=auto .gitattributes
i/lf w/crlf attr/text=auto crlf.txt
tb@mac:/tmp/ttt>
> git add .gitattributes crlf.txt
> git commit -m "initial"
>
> printf "\r\n" >>crlf.txt
>
> With the above patch, both "git diff" and "git diff --quiet" report that
> there are no changes. Previously Git would report the extra newline
> correctly.
Wait a second.
Which extra newline "correctly" ?
The "git diff" command is about the changes which will be done to the index.
Regardless if you have any of these in the working tree on disk:
"Hello\nWorld\n"
"Hello\nWorld\r\n"
"Hello\r\nWorld\n"
"Hello\r\nWorld\r\n"
"git status" and "git diff --quiet"
should not report any changes.
So I don't know if there is a mis-understanding about "git diff" on your side,
or if I miss something.
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Junio C Hamano @ 2017-03-02 18:33 UTC (permalink / raw)
To: Mike Crowe; +Cc: tboegi, git, Jeff King
In-Reply-To: <20170302142056.GB7821@mcrowe.com>
Mike Crowe <mac@mcrowe.com> writes:
> All the solutions presented so far do cause a small change in behaviour
> when using git diff --quiet: they may now cause warning messages like:
>
> warning: CRLF will be replaced by LF in crlf.txt.
> The file will have its original line endings in your working directory.
That is actually a good thing, I think. As the test modifies a file
that originally has "Hello\r\nWorld\r\n" in it to this:
>> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '
>> + printf "Hello\r\nWorld\n" >crlf.txt &&
If you did "git add" at this point, you would get the same warning,
because the lack of CR on the second line could well be a mistake
you may want to notice and fix before going forward. Otherwise
you'd be losing information that _might_ matter to you (i.e. the
fact that the first line had CRLF while the second had LF) and it is
the whole point of safe_crlf setting.
I also think it is a good thing if "git status" reported this path
as modified for the same reason (I didn't actually check if that is
the case).
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Jeff Hostetler @ 2017-03-02 18:37 UTC (permalink / raw)
To: Linus Torvalds, Johannes Schindelin
Cc: Duy Nguyen, Jeff King, Junio C Hamano, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <CA+55aFzscLaviJac-SB65WFYViY=wyAF3EWOnhHSuzSuFLdPTA@mail.gmail.com>
On 3/2/2017 11:35 AM, Linus Torvalds wrote:
> On Thu, Mar 2, 2017 at 6:45 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> It would probably make sense to switch the index integrity check away from
>> SHA-1 because we really only care about detecting bit flips there, and we
>> have no need for the computational overhead of using a full-blown
>> cryptographic hash for that purpose.
> Which index do you actually see as being a problem, btw? The main file
> index (.git/index) or the pack-file indexes?
>
> We definitely don't need the checking version of sha1 for either of
> those, but as Jeff already did the math, at least the pack-file index
> is almost negligible, because the pack-file operations that update it
> end up doing SHA1 over the objects - and the object SHA1 calculations
> are much bigger.
>
> And I don't think we even check the pack-file index hashes except on fsck.
>
> Now, if your _file_ index is 300-400MB (and I do think we check the
> SHA fingerprint on that even on just reading it - verify_hdr() in
> do_read_index()), then that's going to be a somewhat noticeable hit on
> every normal "git diff" etc.
Yes, the .git/index is 450MB with ~3.1M entries. verify_hdr() is called
each time
we read it into memory.
We have been testing a patch in GfW to run the verification in a
separate thread
while the main thread parses (and mallocs) the cache_entries. This does
help
offset the time.
https://github.com/git-for-windows/git/pull/978/files
> But I'd have expected the stat() calls of all the files listed by that
> index to be the _much_ bigger problem in that case. Or do you just
> turn those off with assume-unchanged?
>
> Yeah, those stat calls are threaded when preloading, but even so..
Yes, the stat() calls are more significant percentage of the time (and
having
core.fscache and core.preloadindex help that greatly), but the total
time for a command
is just that -- the total -- so using the philosophy of "every little
bit helps", the faster
routines help us here.
> Anyway, the file index SHA1 checking could probably just be disabled
> entirely (with a config flag). It's a corruption check that simply
> isn't that important. So if that's your main SHA1 issue, that would be
> easy to fix.
Yes, in the GVFS effort, we disabled the verification with a config
setting and haven't
had any incidents.
> Everything else - like pack-file generation etc for a big clone() may
> end up using a ton of SHA1 too, but the SHA1 costs all scale with the
> other costs that drown them out (ie zlib, network, etc).
>
> I'd love to see a profile if you have one.
>
> Linus
^ permalink raw reply
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Junio C Hamano @ 2017-03-02 18:03 UTC (permalink / raw)
To: Lars Schneider; +Cc: Johannes Schindelin, git
In-Reply-To: <CFA1C4B4-0FDA-424D-87A4-EEE1F9BB3712@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> On 02 Mar 2017, at 12:24, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>>
>> Hi Lars,
>>
>> On Thu, 2 Mar 2017, Lars Schneider wrote:
>>
>>> The patch looks good to me in general but I want to propose the following
>>> changes:
>>
>> I know you are using your script to generate this mail, but I would have
>> liked to see v2 in the subject ;-)
>
> Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in
> my email client. Now I am wondering... is the next version v2 or v3 :D
Another question is which v3 people mean in the discussion, when you
and Dscho work on improvements at the same time and each post the
"next" version marked as "v3", and they comment on one of them?
Between "v2" and "v3", it would not make too much difference to the
readership, when it is clear that two people are working to produce
competing improvements without much coordination (i.e. lack of "ok,
I'll send a reroll marked as vX in a minite"--"ok, I'll wait and
comment on it" exchange). People watching from the sideline know
"ah this is v3 from Lars which is the highest numbered one from him
on this topic". As long as you do not mark your next one "v1",
you'd be OK ;-).
^ permalink raw reply
* [PATCH v3] Documentation: Improve description for core.quotePath
From: Andreas Heiduk @ 2017-03-02 19:03 UTC (permalink / raw)
To: gitster; +Cc: Andreas Heiduk, git
Linking the description for pathname quoting to the configuration
variable "core.quotePath" removes inconstistent and incomplete
sections while also giving two hints how to deal with it: Either with
"-c core.quotePath=false" or with "-z".
Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
---
Documentation/config.txt | 23 +++++++++++++----------
Documentation/diff-format.txt | 7 ++++---
Documentation/diff-generate-patch.txt | 7 +++----
Documentation/diff-options.txt | 7 +++----
Documentation/git-apply.txt | 7 +++----
Documentation/git-commit.txt | 9 ++++++---
Documentation/git-ls-files.txt | 10 ++++++----
Documentation/git-ls-tree.txt | 10 +++++++---
Documentation/git-status.txt | 7 +++----
9 files changed, 48 insertions(+), 39 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015346c..23233d8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -350,16 +350,19 @@ core.checkStat::
all fields, including the sub-second part of mtime and ctime.
core.quotePath::
- The commands that output paths (e.g. 'ls-files',
- 'diff'), when not given the `-z` option, will quote
- "unusual" characters in the pathname by enclosing the
- pathname in a double-quote pair and with backslashes the
- same way strings in C source code are quoted. If this
- variable is set to false, the bytes higher than 0x80 are
- not quoted but output as verbatim. Note that double
- quote, backslash and control characters are always
- quoted without `-z` regardless of the setting of this
- variable.
+ Commands that output paths (e.g. 'ls-files', 'diff'), will
+ quote "unusual" characters in the pathname by enclosing the
+ pathname in double-quotes and escaping those characters with
+ backslashes in the same way C escapes control characters (e.g.
+ `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
+ values larger than 0x80 (e.g. octal `\302\265` for "micro" in
+ UTF-8). If this variable is set to false, bytes higher than
+ 0x80 are not considered "unusual" any more. Double-quotes,
+ backslash and control characters are always escaped regardless
+ of the setting of this variable. A simple space character is
+ not considered "unusual". Many commands can output pathnames
+ completely verbatim using the `-z` option. The default value
+ is true.
core.eol::
Sets the line ending type to use in the working directory for
diff --git a/Documentation/diff-format.txt b/Documentation/diff-format.txt
index cf52626..706916c 100644
--- a/Documentation/diff-format.txt
+++ b/Documentation/diff-format.txt
@@ -78,9 +78,10 @@ Example:
:100644 100644 5be4a4...... 000000...... M file.c
------------------------------------------------
-When `-z` option is not used, TAB, LF, and backslash characters
-in pathnames are represented as `\t`, `\n`, and `\\`,
-respectively.
+Without the `-z` option, pathnames with "unusual" characters are
+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]). Using `-z` the filename is output
+verbatim and the line is terminated by a NUL byte.
diff format for merges
----------------------
diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index d2a7ff5..231105c 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -53,10 +53,9 @@ The index line includes the SHA-1 checksum before and after the change.
The <mode> is included if the file mode does not change; otherwise,
separate lines indicate the old and the new mode.
-3. TAB, LF, double quote and backslash characters in pathnames
- are represented as `\t`, `\n`, `\"` and `\\`, respectively.
- If there is need for such substitution then the whole
- pathname is put in double quotes.
+3. Pathnames with "unusual" characters are quoted as explained for
+ the configuration variable `core.quotePath` (see
+ linkgit:git-config[1]).
4. All the `file1` files in the output refer to files before the
commit, and all the `file2` files refer to files after the commit.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index d91ddbd..89cc0f4 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -192,10 +192,9 @@ ifndef::git-log[]
given, do not munge pathnames and use NULs as output field terminators.
endif::git-log[]
+
-Without this option, each pathname output will have TAB, LF, double quotes,
-and backslash characters replaced with `\t`, `\n`, `\"`, and `\\`,
-respectively, and the pathname will be enclosed in double quotes if
-any of those replacements occurred.
+Without this option, pathnames with "unusual" characters are quoted as
+explained for the configuration variable `core.quotePath` (see
+linkgit:git-config[1]).
--name-only::
Show only names of changed files.
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 8ddb207..631cbd8 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -108,10 +108,9 @@ the information is read from the current index instead.
When `--numstat` has been given, do not munge pathnames,
but use a NUL-terminated machine-readable format.
+
-Without this option, each pathname output will have TAB, LF, double quotes,
-and backslash characters replaced with `\t`, `\n`, `\"`, and `\\`,
-respectively, and the pathname will be enclosed in double quotes if
-any of those replacements occurred.
+Without this option, pathnames with "unusual" characters are quoted as
+explained for the configuration variable `core.quotePath` (see
+linkgit:git-config[1]).
-p<n>::
Remove <n> leading slashes from traditional diff paths. The
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 4f8f20a..25dcdcc 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -117,9 +117,12 @@ OPTIONS
-z::
--null::
- When showing `short` or `porcelain` status output, terminate
- entries in the status output with NUL, instead of LF. If no
- format is given, implies the `--porcelain` output format.
+ When showing `short` or `porcelain` status output, print the
+ filename verbatim and terminate the entries with NUL, instead of LF.
+ If no format is given, implies the `--porcelain` output format.
+ Without the `-z` option, filenames with "unusual" characters are
+ quoted as explained for the configuration variable `core.quotePath`
+ (see linkgit:git-config[1]).
-F <file>::
--file=<file>::
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 446209e..1cab703 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -77,7 +77,8 @@ OPTIONS
succeed.
-z::
- \0 line termination on output.
+ \0 line termination on output and do not quote filenames.
+ See OUTPUT below for more information.
-x <pattern>::
--exclude=<pattern>::
@@ -196,9 +197,10 @@ the index records up to three such pairs; one from tree O in stage
the user (or the porcelain) to see what should eventually be recorded at the
path. (see linkgit:git-read-tree[1] for more information on state)
-When `-z` option is not used, TAB, LF, and backslash characters
-in pathnames are represented as `\t`, `\n`, and `\\`,
-respectively.
+Without the `-z` option, pathnames with "unusual" characters are
+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]). Using `-z` the filename is output
+verbatim and the line is terminated by a NUL byte.
Exclude Patterns
diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
index dbc91f9..9dee7be 100644
--- a/Documentation/git-ls-tree.txt
+++ b/Documentation/git-ls-tree.txt
@@ -53,7 +53,8 @@ OPTIONS
Show object size of blob (file) entries.
-z::
- \0 line termination on output.
+ \0 line termination on output and do not quote filenames.
+ See OUTPUT FORMAT below for more information.
--name-only::
--name-status::
@@ -82,8 +83,6 @@ Output Format
-------------
<mode> SP <type> SP <object> TAB <file>
-Unless the `-z` option is used, TAB, LF, and backslash characters
-in pathnames are represented as `\t`, `\n`, and `\\`, respectively.
This output format is compatible with what `--index-info --stdin` of
'git update-index' expects.
@@ -95,6 +94,11 @@ Object size identified by <object> is given in bytes, and right-justified
with minimum width of 7 characters. Object size is given only for blobs
(file) entries; for other entries `-` character is used in place of size.
+Without the `-z` option, pathnames with "unusual" characters are
+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]). Using `-z` the filename is output
+verbatim and the line is terminated by a NUL byte.
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 725065e..ba87365 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -322,10 +322,9 @@ When the `-z` option is given, pathnames are printed as is and
without any quoting and lines are terminated with a NUL (ASCII 0x00)
byte.
-Otherwise, all pathnames will be "C-quoted" if they contain any tab,
-linefeed, double quote, or backslash characters. In C-quoting, these
-characters will be replaced with the corresponding C-style escape
-sequences and the resulting pathname will be double quoted.
+Without the `-z` option, pathnames with "unusual" characters are
+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]).
CONFIGURATION
^ permalink raw reply related
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Junio C Hamano @ 2017-03-02 17:52 UTC (permalink / raw)
To: Jeff King; +Cc: tboegi, git, mac
In-Reply-To: <20170302085313.r6dox4wa2kqnp7ao@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> diff --git a/diff.c b/diff.c
>> index 8c78fce49d..dc51dceb44 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
>> s->should_free = 1;
>> return 0;
>> }
>> - if (size_only)
>> +
>> + /*
>> + * Even if the caller would be happy with getting
>> + * only the size, we cannot return early at this
>> + * point if the path requires us to run the content
>> + * conversion.
>> + */
>> + if (!would_convert_to_git(s->path) && size_only)
>> return 0;
>
> The would_convert_to_git() function is a little expensive (it may have
> to do an attribute lookup). It may be worth swapping the two halves of
> the conditional here to get the short-circuit.
Yes. I think it makes sense.
>> +
>> + /*
>> + * Note: this check uses xsize_t(st.st_size) that may
>> + * not be the true size of the blob after it goes
>> + * through convert_to_git(). This may not strictly be
>> + * correct, but the whole point of big_file_threashold
>
> s/threashold/threshold/
Thanks. I felt there was something wrong and looked at the line
three times but somehow failed to spot exactly what was wrong ;-)
>
>> + * and is_binary check being that we want to avoid
>> + * opening the file and inspecting the contents, this
>> + * is probably fine.
>> + */
>> if ((flags & CHECK_BINARY) &&
>> s->size > big_file_threshold && s->is_binary == -1) {
>> s->is_binary = 1;
>
> I'm trying to think how this "not strictly correct" could bite us.
Note that the comment is just documenting what I learned and thought
while working on an unrelated thing that happened to be sitting next
to it.
Nobody asks "I am OK without the contents i.e. size-only" and "Can
you see if this is binary?" at the same time (and if a caller did,
it would never have got is_binary with the original code). s->size
is still a copy of st.size at this point of the code (we have not
actually updated it to the size of the real blob, which happens a
bit later in the flow of this codepath where we actually slurp the
thing in and run the conversion). So with or without this patch,
there shouldn't be any change in the behaviour wrt CHECK_BINARY.
> For
> line-ending conversion, I'd say that the before/after are going to be
> approximately the same size. But what about something like LFS? If I
> have a 600MB file that convert_to_git() filters into a short LFS
> pointer, I think this changes the behavior. Before, we would diff the
> pointer file, but now we'll get "binary file changed".
To be quite honest, I do not think this code should cater to LFS or
any other conversion hack. They all install their own diff driver
and they can tell diff_filespec_is_binary() if the thing is binary
or not without falling back to this heuristics codepath.
What is done here *is* inconsistent with what is done on the other
side of if/else, that compares big_file_threshold with in-git size,
not in-working-tree size. That may want to be addressed, but I am
not sure if it is worth it.
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Jeff King @ 2017-03-02 19:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: tboegi, git, mac
In-Reply-To: <xmqqmvd3d0ru.fsf@gitster.mtv.corp.google.com>
On Thu, Mar 02, 2017 at 09:52:21AM -0800, Junio C Hamano wrote:
> >> + * and is_binary check being that we want to avoid
> >> + * opening the file and inspecting the contents, this
> >> + * is probably fine.
> >> + */
> >> if ((flags & CHECK_BINARY) &&
> >> s->size > big_file_threshold && s->is_binary == -1) {
> >> s->is_binary = 1;
> >
> > I'm trying to think how this "not strictly correct" could bite us.
>
> Note that the comment is just documenting what I learned and thought
> while working on an unrelated thing that happened to be sitting next
> to it.
Yeah, sorry, this is obviously not a blocker to your patch. I'm just
wondering if there is more work needed.
> To be quite honest, I do not think this code should cater to LFS or
> any other conversion hack. They all install their own diff driver
> and they can tell diff_filespec_is_binary() if the thing is binary
> or not without falling back to this heuristics codepath.
Yeah, you're right, I was just being silly. Whatever configured the
filter already has an opportunity to give us this knowledge in a better
way, and we should rely on that.
-Peff
^ permalink raw reply
* [PATCH v2] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()
From: Junio C Hamano @ 2017-03-02 18:51 UTC (permalink / raw)
To: git; +Cc: tboegi, Jeff King, mac
In-Reply-To: <20170302085313.r6dox4wa2kqnp7ao@sigill.intra.peff.net>
Callers of diff_populate_filespec() can choose to ask only for the
size of the blob without grabbing the blob data, and the function,
after running lstat() when the filespec points at a working tree
file, returns by copying the value in size field of the stat
structure into the size field of the filespec when this is the case.
However, this short-cut cannot be taken if the contents from the
path needs to go through convert_to_git(), whose resulting real blob
data may be different from what is in the working tree file.
As "git diff --quiet" compares the .size fields of filespec
structures to skip content comparison, this bug manifests as a
false "there are differences" for a file that needs eol conversion,
for example.
Reported-by: Mike Crowe <mac@mcrowe.com>
Helped-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* With "test size_only to avoid more expensive would_convert call"
fix applied. Also the new test is now in t4xxx that it belongs
to.
diff.c | 19 ++++++++++++++++++-
t/t4035-diff-quiet.sh | 9 +++++++++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index 059123c5dc..37e60ca601 100644
--- a/diff.c
+++ b/diff.c
@@ -2783,8 +2783,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
s->should_free = 1;
return 0;
}
- if (size_only)
+
+ /*
+ * Even if the caller would be happy with getting
+ * only the size, we cannot return early at this
+ * point if the path requires us to run the content
+ * conversion.
+ */
+ if (size_only && !would_convert_to_git(s->path))
return 0;
+
+ /*
+ * Note: this check uses xsize_t(st.st_size) that may
+ * not be the true size of the blob after it goes
+ * through convert_to_git(). This may not strictly be
+ * correct, but the whole point of big_file_threshold
+ * and is_binary check being that we want to avoid
+ * opening the file and inspecting the contents, this
+ * is probably fine.
+ */
if ((flags & CHECK_BINARY) &&
s->size > big_file_threshold && s->is_binary == -1) {
s->is_binary = 1;
diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index 461f4bb583..2f1737fcef 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -152,4 +152,13 @@ test_expect_success 'git diff --quiet ignores stat-change only entries' '
test_expect_code 1 git diff --quiet
'
+test_expect_success 'git diff --quiet on a path that need conversion' '
+ echo "crlf.txt text=auto" >.gitattributes &&
+ printf "Hello\r\nWorld\r\n" >crlf.txt &&
+ git add .gitattributes crlf.txt &&
+
+ printf "Hello\r\nWorld\n" >crlf.txt &&
+ git diff --quiet crlf.txt
+'
+
test_done
--
2.12.0-352-gb05ccab5eb
^ permalink raw reply related
* Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info
From: Junio C Hamano @ 2017-03-02 18:36 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, peartben, benpeart
In-Reply-To: <20170228215937.yd4juycjf7y3vish@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 28, 2017 at 01:42:44PM -0800, Junio C Hamano wrote:
>
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>> > It could be argued that in the future, Git might need to distinguish
>> > tree_objects from blob_objects - in particular, a user might want
>> > rev-list to print the trees but not the blobs.
>>
>> That was exactly why these bits were originally made to "appear
>> independent but in practice nobody sets only one and leaves others
>> off".
>>
>> And it didn't happen in the past 10 years, which tells us that we
>> should take this patch.
>
> I actually have a patch which uses the distinction. It's for
> upload-archive doing reachability checks (which seems rather familiar to
> what's going on here).
OK. Thanks for stopping me ;-)
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Torsten Bögershausen @ 2017-03-02 18:20 UTC (permalink / raw)
To: Mike Crowe, Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <20170302142056.GB7821@mcrowe.com>
On 2017-03-02 15:20, Mike Crowe wrote:
> ll the solutions presented so far do cause a small change in behaviour
> when using git diff --quiet: they may now cause warning messages like:
>
> warning: CRLF will be replaced by LF in crlf.txt.
> The file will have its original line endings in your working directory.
Ah,
that is not ideal.
I can have a look at it later (or due to the weekend)
^ permalink raw reply
* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
From: Junio C Hamano @ 2017-03-02 19:36 UTC (permalink / raw)
To: Devin J. Pohly; +Cc: Johannes Schindelin, Charles Bailey, Jeff King, git
In-Reply-To: <20170223213333.GA3490@prospect.localdomain>
"Devin J. Pohly" <djpohly@gmail.com> writes:
> I think your point is interesting too, though. If a commit is also
> TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
> reasonable that someone might want to leave it in the filtered branch as
> an empty commit while pruning empt*ied* commits. I would imagine that
> as another option (--prune-newly-empty?).
I was hoping to hear from others who may care about filter-branch to
comment on this topic to help me decide, but I haven't heard
anything, so here is my tentative thinking.
I am leaning to:
* Take your series as-is, which would mean --prune-empty will
change the behaviour to unconditionally lose the empty root.
* Then, people who care deeply about it can add a new option that
prunes commits that become empty while keeping the originally
empty ones.
Thoughts?
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Linus Torvalds @ 2017-03-02 19:04 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Johannes Schindelin, Duy Nguyen, Jeff King, Junio C Hamano,
Marc Stevens, Dan Shumow, Git Mailing List
In-Reply-To: <85221b97-759f-b7a9-1256-21515d163cbf@jeffhostetler.com>
On Thu, Mar 2, 2017 at 10:37 AM, Jeff Hostetler <git@jeffhostetler.com> wrote:
>>
>> Now, if your _file_ index is 300-400MB (and I do think we check the
>> SHA fingerprint on that even on just reading it - verify_hdr() in
>> do_read_index()), then that's going to be a somewhat noticeable hit on
>> every normal "git diff" etc.
>
> Yes, the .git/index is 450MB with ~3.1M entries. verify_hdr() is called
> each time we read it into memory.
Ok. So that's really just a purely historical artifact.
The file index is actually the first part of git to have ever been
written. You can't even see it in the history, because the initial
revision from Apr 7, 2005, obviously depended on the actual object
hashing.
But the file index actually came first. You can _kind_ of see that in
the layout of the original git tree, and how the main header file is
still called "cache.h", and how the original ".git" directory was
actually called ".dircache".
And the two biggest files (by a fairly big margin) are "read-cache.c"
and "update-cache.c".
So that file index cache was in many ways _the_ central part of the
original git model. The sha1 file indexing and object database was
just the backing store for the file index.
But part of that history is then how much I worried about corruption
of that index (and, let's face it, general corruption resistance _was_
one of the primary design goals - performance was high up there too,
but safety in the face of filesystem corruption was and is a primary
issue).
But realistically, I don't think we've *ever* hit anything serious on
the index file, and it's obviously not a security issue. It also isn't
even a compatibility issue, so it would be trivial to just bump the
version header and saying that the signature changes the meaning of
the checksum.
That said:
> We have been testing a patch in GfW to run the verification in a separate thread
> while the main thread parses (and mallocs) the cache_entries. This does help
> offset the time.
Yeah, that seems an even better solution, honestly.
The patch would be cleaner without the NO_PTHREADS things.
I wonder how meaningful that thing even is today. Looking at what
seems to select NO_PTHREADS, I suspect that's all entirely historical.
For example, you'll see it for QNX etc, which seems wrong - QNX
definitely has pthreads according to their docs, for example.
Linus
^ 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