* Re: Delta compression not so effective
From: Junio C Hamano @ 2017-03-01 16:17 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Git Mailing List
In-Reply-To: <CAPc5daWXafzN0dpyd+kdHcLU_YSmZpiNz_i2rXn_2hbMN-9Xww@mail.gmail.com>
On Wed, Mar 1, 2017 at 8:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Just a hunch. s/F/f/ perhaps? "-F" does not allow Git to recover from poor
Nah, sorry for the noise. Between -F and -f there shouldn't be any difference.
^ permalink raw reply
* Re: Delta compression not so effective
From: Junio C Hamano @ 2017-03-01 16:06 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Git Mailing List
In-Reply-To: <4d2a1852-8c84-2869-78ad-3c863f6dcaf7@gmail.com>
On Wed, Mar 1, 2017 at 5:51 AM, Marius Storm-Olsen <mstormo@gmail.com> wrote:
> ... which brought it down to 206GB in a single pack. I then ran
> git repack -a -d -F --window=350 --depth=250
> which took it down to 203GB, where I'm at right now.
Just a hunch. s/F/f/ perhaps? "-F" does not allow Git to recover from poor
delta-base choice the original importer may have made (and if the
original importer
used fast-import, it is known that its choice of the delta-base is suboptimal).
^ permalink raw reply
* Re: [PATCH v7 3/3] config: add conditional include
From: Ramsay Jones @ 2017-03-01 17:16 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Jeff King, sschuberth, Matthieu Moy,
Philip Oakley
In-Reply-To: <20170301112631.16497-4-pclouds@gmail.com>
On 01/03/17 11:26, Nguyễn Thái Ngọc Duy wrote:
> Sometimes a set of repositories want to share configuration settings
> among themselves that are distinct from other such sets of repositories.
> A user may work on two projects, each of which have multiple
> repositories, and use one user.email for one project while using another
> for the other.
>
> Setting $GIT_DIR/.config works, but if the penalty of forgetting to
> update $GIT_DIR/.config is high (especially when you end up cloning
> often), it may not be the best way to go. Having the settings in
> ~/.gitconfig, which would work for just one set of repositories, would
> not well in such a situation. Having separate ${HOME}s may add more
> problems than it solves.
>
> Extend the include.path mechanism that lets a config file include
> another config file, so that the inclusion can be done only when some
> conditions hold. Then ~/.gitconfig can say "include config-project-A
> only when working on project-A" for each project A the user works on.
>
> In this patch, the only supported grouping is based on $GIT_DIR (in
> absolute path), so you would need to group repositories by directory, or
> something like that to take advantage of it.
>
> We already have include.path for unconditional includes. This patch goes
> with includeIf.<condition>.path to make it clearer that a condition is
> required. The new config has the same backward compatibility approach as
> include.path: older git versions that don't understand includeIf will
> simply ignore them.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Documentation/config.txt | 64 +++++++++++++++++++++++++++++++++
> config.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++
> t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++++
> 3 files changed, 212 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1fad746efd..2a41e84bab 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -90,6 +90,59 @@ found at the location of the include directive. If the value of the
> be relative to the configuration file in which the include directive
> was found. See below for examples.
>
> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include a config file from another conditionally by setting a
> +`includeIf.<condition>.path` variable to the name of the file to be
> +included. The variable's value is treated the same way as
> +`include.path`. `includeIf.<condition>path` supports multiple key
----------------------------------------^^
s/<condition>path/<condition>.path/
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Junio C Hamano @ 2017-03-01 17:23 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20170228223325.vabdrwas2qn52gup@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 28, 2017 at 02:27:22PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > ... We can certainly stick with it for now (it's awkward if you
>> > really do have an entry on Jan 1 1970, but other than that it's an OK
>> > marker). I agree that the most negatively value is probably a saner
>> > choice, but we can switch to it after the dust settles.
>>
>> I was trying to suggest that we should strive to switch to the most
>> negative or whatever the most implausible value in the new range
>> (and leave it as a possible bug to be fixed if we missed a place
>> that still used "0 is impossible") while doing the ulong to time_t
>> (or timestamp_t that is i64).
>>
>> "safer in the short term" wasn't meant to be "let's not spend time
>> to do quality work". As long as we are switching, we should follow
>> it through.
>
> Sure, I'd be much happier to see it done now. I just didn't want to pile
> on the requirements to the point that step 1 doesn't get done.
Ah, that was what you meant.
I was assuming that we are switching to a longer _signed_ type. It
felt silly to tell users "you can use timestamps before the epoch
now with this change, but you cannot express time exactly at the
epoch".
I am perfectly OK with switching to a longer _unsigned_ type with
the "0 is impossible" [*1*] intact, aka "safer in the short term",
if we want to it make our first step. That may be a smaller step,
but still a step in the right direction.
[Footnote]
*1* It could be "-1 is impossible", I didn't actually check.
Funnily enough, ISO C99 uses (time_t)(-1) to signal an error
when returning value from mktime() and time().
^ permalink raw reply
* Re: [BUG] branch renamed to 'HEAD'
From: Junio C Hamano @ 2017-03-01 17:35 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <CA+P7+xpfRjr4BKkLLcbCS-yXXYoiHeWr9Y9ubHA-Xxqa9c7q8w@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> I didn't find any problems besides what you had already outlined
> before I started reading the series. It looks pretty much like I
> thought it would. I like the idea of saying "I want X" rather than the
> command returning "This was a Y"
Yeah, thanks for reading it through. Except for one disambiguation
glitch Peff noticed himself, I found the entire series a pleasant
read.
^ permalink raw reply
* Re: Delta compression not so effective
From: Linus Torvalds @ 2017-03-01 17:36 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Git Mailing List
In-Reply-To: <4d2a1852-8c84-2869-78ad-3c863f6dcaf7@gmail.com>
On Wed, Mar 1, 2017 at 5:51 AM, Marius Storm-Olsen <mstormo@gmail.com> wrote:
>
> When first importing, I disabled gc to avoid any repacking until completed.
> When done importing, there was 209GB of all loose objects (~670k files).
> With the hopes of quick consolidation, I did a
> git -c gc.autoDetach=0 -c gc.reflogExpire=0 \
> -c gc.reflogExpireUnreachable=0 -c gc.rerereresolved=0 \
> -c gc.rerereunresolved=0 -c gc.pruneExpire=now \
> gc --prune
> which brought it down to 206GB in a single pack. I then ran
> git repack -a -d -F --window=350 --depth=250
> which took it down to 203GB, where I'm at right now.
Considering that it was 209GB in loose objects, I don't think it
delta-packed the big objects at all.
I wonder if the big objects end up hitting some size limit that causes
the delta creation to fail.
For example, we have that HASH_LIMIT that limits how many hashes
we'll create for the same hash bucket, because there's some quadratic
behavior in the delta algorithm. It triggered with things like big
files that have lots of repeated content.
We also have various memory limits, in particular
'window_memory_limit'. That one should default to 0, but maybe you
limited it at some point in a config file and forgot about it?
Linus
^ permalink raw reply
* Re: [PATCH v7 1/3] config.txt: clarify multiple key values in include.path
From: Junio C Hamano @ 2017-03-01 17:40 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Jeff King, sschuberth, Matthieu Moy, Philip Oakley,
Ramsay Jones
In-Reply-To: <20170301112631.16497-2-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> The phrasing in this paragraph may give an impression that you can only
> use it once. Rephrase it a bit.
>
> Helped-by: Philip Oakley <philipoakley@iee.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Documentation/config.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 015346c417..4748efbf36 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -79,10 +79,10 @@ escape sequences) are invalid.
> Includes
> ~~~~~~~~
>
> -You can include one config file from another by setting the special
> +You can include a config file from another by setting the special
> `include.path` variable to the name of the file to be included. The
> variable takes a pathname as its value, and is subject to tilde
> -expansion.
> +expansion. `include.path` supports multiple key values.
Multiple key values or just simply multiple values? I think it is
the latter.
But to the intended target audience, I think
`include.path` can be given multiple times.
is easier to understand. It's not like you can (or want to)
enumerate with "git config --get-all include.path" to learn all the
values (for the single key "include.value"), and it is better not to
lead readers to think of this in terms of <key,value> in the first
place (which is already clarified in the text that follows).
> The
> included file is expanded immediately, as if its contents had been
^ permalink raw reply
* Re: [PATCH v7 0/3] Conditional config include
From: Junio C Hamano @ 2017-03-01 17:52 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Jeff King, sschuberth, Matthieu Moy, Philip Oakley,
Ramsay Jones
In-Reply-To: <20170301112631.16497-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Nguyễn Thái Ngọc Duy (3):
> config.txt: clarify multiple key values in include.path
> config.txt: reflow the second include.path paragraph
> config: add conditional include
Thanks. The primary change looked good (it looked good already in
the previous round).
Here is a list of minor suggestions on top.
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2a41e84bab..5faabc7934 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -82,7 +82,7 @@ Includes
You can include a config file from another by setting the special
`include.path` variable to the name of the file to be included. The
variable takes a pathname as its value, and is subject to tilde
-expansion. `include.path` supports multiple key values.
+expansion. `include.path` can be given multiple times.
The included file is expanded immediately, as if its contents had been
found at the location of the include directive. If the value of the
@@ -96,8 +96,7 @@ Conditional includes
You can include a config file from another conditionally by setting a
`includeIf.<condition>.path` variable to the name of the file to be
included. The variable's value is treated the same way as
-`include.path`. `includeIf.<condition>path` supports multiple key
-values.
+`include.path`. `includeIf.<condition>.path` can be given multiple times.
The condition starts with a keyword followed by a colon and some data
whose format and meaning depends on the keyword. Supported keywords
^ permalink raw reply related
* Re: Delta compression not so effective
From: Marius Storm-Olsen @ 2017-03-01 17:57 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <CA+55aFzQ0o2R2kShS=AuKu0TLnfPV-0JCkViqx5J_afCK0Yt5g@mail.gmail.com>
On 3/1/2017 11:36, Linus Torvalds wrote:
> On Wed, Mar 1, 2017 at 5:51 AM, Marius Storm-Olsen <mstormo@gmail.com> wrote:
>>
>> When first importing, I disabled gc to avoid any repacking until completed.
>> When done importing, there was 209GB of all loose objects (~670k files).
>> With the hopes of quick consolidation, I did a
>> git -c gc.autoDetach=0 -c gc.reflogExpire=0 \
>> -c gc.reflogExpireUnreachable=0 -c gc.rerereresolved=0 \
>> -c gc.rerereunresolved=0 -c gc.pruneExpire=now \
>> gc --prune
>> which brought it down to 206GB in a single pack. I then ran
>> git repack -a -d -F --window=350 --depth=250
>> which took it down to 203GB, where I'm at right now.
>
> Considering that it was 209GB in loose objects, I don't think it
> delta-packed the big objects at all.
>
> I wonder if the big objects end up hitting some size limit that causes
> the delta creation to fail.
You're likely on to something here.
I just ran
git verify-pack --verbose
objects/pack/pack-9473815bc36d20fbcd38021d7454fbe09f791931.idx | sort
-k3n | tail -n15
and got no blobs with deltas in them.
feb35d6dc7af8463e038c71cc3893d163d47c31c blob 36841958 36461935
3259424358
007b65e603cdcec6644ddc25c2a729a394534927 blob 36845345 36462120
3341677889
0727a97f68197c99c63fcdf7254e5867f8512f14 blob 37368646 36983862
3677338718
576ce2e0e7045ee36d0370c2365dc730cb435f40 blob 37399203 37014740
3639613780
7f6e8b22eed5d8348467d9b0180fc4ae01129052 blob 125296632 83609223
5045853543
014b9318d2d969c56d46034a70223554589b3dc4 blob 170113524 6124878
1118227958
22d83cb5240872006c01651eb1166c8db62c62d8 blob 170113524 65941491
1257435955
292ac84f48a3d5c4de8d12bfb2905e055f9a33b1 blob 170113524 67770601
1323377446
2b9329277e379dfbdcd0b452b39c6b0bf3549005 blob 170113524 7656690
1110571268
37517efb4818a15ad7bba79b515170b3ee18063b blob 170113524 133083119
1124352836
55a4a70500eb3b99735677d0025f33b1bb78624a blob 170113524 6592386
1398975989
e669421ea5bf2e733d5bf10cf505904d168de749 blob 170113524 7827942
1391148047
e9916da851962265a9d5b099e72f60659a74c144 blob 170113524 73514361
966299538
f7bf1313752deb1bae592cc7fc54289aea87ff19 blob 170113524 70756581
1039814687
8afc6f2a51f0fa1cc4b03b8d10c70599866804ad blob 248959314 237612609
606692699
In fact, I don't see a single "deltified" blob until 6355th last line!
> For example, we have that HASH_LIMIT that limits how many hashes
> we'll create for the same hash bucket, because there's some quadratic
> behavior in the delta algorithm. It triggered with things like big
> files that have lots of repeated content.
>
> We also have various memory limits, in particular
> 'window_memory_limit'. That one should default to 0, but maybe you
> limited it at some point in a config file and forgot about it?
Indeed, I did do a
-c pack.threads=20 --window-memory=6g
to 'git repack', since the machine is a 20-core (40 threads) machine
with 126GB of RAM.
So I guess with these sized objects, even at 6GB per thread, it's not
enough to get a big enough Window for proper delta-packing?
This repo took >14hr to repack on 20 threads though ("compression" step
was very fast, but stuck 95% of the time in "writing objects"), so I can
only imagine how long a pack.threads=1 will take :)
But arent't the blobs sorted by some metric for reasonable delta-pack
locality, so even with a 6GB window it should have seen ~25 similar
objects to deltify against?
--
.marius
^ permalink raw reply
* Re: [PATCH 2/2] pretty: use fmt_output_email_subject()
From: Junio C Hamano @ 2017-03-01 18:08 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, Adrian Dudau, git@vger.kernel.org
In-Reply-To: <58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de>
René Scharfe <l.s.r@web.de> writes:
> Add the email-style subject prefix (e.g. "Subject: [PATCH] ") directly
> when it's needed instead of letting log_write_email_headers() prepare
> it in a static buffer in advance. This simplifies storage ownership and
> code flow.
It certainly does. At first I wondered if there are places other
than log-write-email-headers that sets its own string to pp.subject,
but this patch removes the field from the structure to guarantee
that such a place, if existed, is properly dealt with. Otherwise,
the resulting code would not compile.
> @@ -1005,6 +1004,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> msg = body;
> pp.fmt = CMIT_FMT_EMAIL;
> pp.date_mode.type = DATE_RFC2822;
> + pp.rev = rev;
> + pp.print_email_subject = 1;
These two are always set together? We'll shortly see that it is not
the case below.
> pp_user_info(&pp, NULL, &sb, committer, encoding);
> pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
> pp_remainder(&pp, &msg, &sb, 0);
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index c9585d475d..f78bb4818d 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -148,7 +148,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
>
> ctx.fmt = CMIT_FMT_USERFORMAT;
> ctx.abbrev = log->abbrev;
> - ctx.subject = "";
> + ctx.print_email_subject = 1;
Here we see .rev is left to NULL here, with print_email_subject set
to true. And in the entire patch this is the only such place.
> diff --git a/pretty.c b/pretty.c
> index 5e683830d9..d0f86f5d85 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1607,8 +1607,9 @@ void pp_title_line(struct pretty_print_context *pp,
> pp->preserve_subject ? "\n" : " ");
>
> strbuf_grow(sb, title.len + 1024);
> - if (pp->subject) {
> - strbuf_addstr(sb, pp->subject);
> + if (pp->print_email_subject) {
> + if (pp->rev)
> + fmt_output_email_subject(sb, pp->rev);
A hidden assumption this code makes is that anybody who does not
want .rev (aka "doing it as part of format-patch that may want
nr/total etc") does not want _any_ "Subject: ". It obviously holds
true in today's code (the one in shortlog-add-commit is the only one
and it sets an empty string to .subject).
Does the loss of flexibility to the future callers matter, though?
I cannot tell offhand.
Thanks. Let's see what others think.
^ permalink raw reply
* Re: [PATCH 3/6] Introduce a new "printf format" for timestamps
From: Junio C Hamano @ 2017-03-01 18:20 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <f4ff525dda1964dd0e7cef1d0507e1f2403469e1.1488231002.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> So let's introduce the pseudo format "PRItime" (currently simply being
> "lu") so that it is easy later on to change the data type to time_t.
The problem being solved is a good thing to solve, and
> - printf("author-time %lu\n", ci.author_time);
> + printf("author-time %"PRItime"\n", ci.author_time);
is one of the two ingredients to the solution for this line. But
the final form would require casting ci.author_time to the type
expected by %PRItime format specifier. With this change alone, you
could define PRItime to expect an winder type in the next step but
that would be a bad conversion. IOW, changing only the format
without introducing an explicit cast appears to invite future
mistakes.
It would be better to introduce the timestamp_t we discussed earlier
before (or at) this step, and typedef it to ulong first, and then in
this step, change the above to
printf("author-time %"PRItime"\n", (timestamp_t)ci.author_time);
to keep them in sync. And at a later step in the series, you can
update definition of PRItime and timestamp_t to make them wider at
the same time, and the changes in this patch like the above line
would not need to be touched again.
^ permalink raw reply
* Re: gpg verify git sub modules useful?
From: Patrick Schleizer @ 2017-03-01 18:28 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Whonix-devel
In-Reply-To: <xmqqmvd6kykc.fsf@gitster.mtv.corp.google.com>
Good questions, thank you for trying to figure out what I am asking. :)
Junio C Hamano:
> Patrick Schleizer <patrick-mailinglists@whonix.org> writes:
>
>> When using git submodules, is there value in iterating about the git
>> submodules running "git verfiy-commit HEAD" or would that be already
>> covered by the git submodule verification?
>
> That depends on what you are referring to with the "git submodule
> verification"
cd submodule
if ! git verfiy-commit HEAD ; then
error
fi
> and more importantly what threat you are guarding
> against.
All main (non-submodule) (merge) commits and submodule (merge) commits
are signed by me.
1) git --recursive clone main (non-submodule) git repository
2) cd git main repository
3) git verify-commit HEAD or git verify-tag tag-name
4) git submodule update
What if the main (non-submodule) git repository gpg signature was okay
but then after git fetched the submodules these compromised (MITM'ed) ones?
Does the having gpg verified the root (main git repository) ensure that
submodule commits are also quasi verified?
> "git -C <submodule-dir> verify-commit HEAD" may make sure
> that the contents of that commit object is GPG signed by whoever you
> trust--is that what you want to make sure?
> Or do you want all
> commits in the submodule history to be similarly signed because the
> tree of the superproject can switch to some other commit there?
I guess so.
^ permalink raw reply
* [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: tboegi @ 2017-03-01 17:04 UTC (permalink / raw)
To: git, mac; +Cc: Junio C Hamano, Torsten Bögershausen
In-Reply-To: <xmqqshmyhtnu.fsf@gitster.mtv.corp.google.com>
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
--
2.10.0
^ permalink raw reply related
* Re: [PATCH] Put sha1dc on a diet
From: Junio C Hamano @ 2017-03-01 18:42 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Marc Stevens, Dan Shumow, Git Mailing List
In-Reply-To: <alpine.LFD.2.20.1702281621050.22202@i7.lan>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> I notice that sha1dc is in the 'pu' branch now, so let's put my money
> where my mouth is, and send in the sha1dc diet patch.
I see //c99 comments and also T array[] = { [58] = val } both of
which I think we stay away from (and the former is from the initial
import), so some people on other platforms MAY have trouble with
this topic.
Let's see what happens by queuing it on 'pu' ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Linus Torvalds @ 2017-03-01 18:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Marc Stevens, Dan Shumow, Git Mailing List
In-Reply-To: <xmqq7f48hm8g.fsf@gitster.mtv.corp.google.com>
On Wed, Mar 1, 2017 at 10:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I see //c99 comments
sha1dc is already full of // style comments. I just followed the
existing practice.
> and also T array[] = { [58] = val } both of
> which I think we stay away from (and the former is from the initial
> import), so some people on other platforms MAY have trouble with
> this topic.
Hmm. The "{ [58] = val; }" kind of initialization would be easy to
work around by just filling in everything else with NULL, but it would
make for a pretty nasty readability issue.
That said, if you mis-count the NULL's, the end result will pretty
immediately SIGSEGV, so I guess it wouldn't be much of a maintenance
problem.
But if you're just willing to take the "let's see" approach, I think
the explicitly numbered initializer is much better.
The main people who I assume would really want to use the sha1dc
library are hosting places. And they won't be using crazy compilers
from the last century.
That said, I think that it would be lovely to just default to
USE_SHA1DC and just put the whole attack behind us. Yes, it's slower.
No, it doesn't really seem to matter that much in practice.
Linus
^ permalink raw reply
* Re: gpg verify git sub modules useful?
From: Stefan Beller @ 2017-03-01 19:08 UTC (permalink / raw)
To: Patrick Schleizer; +Cc: Junio C Hamano, git@vger.kernel.org, Whonix-devel
In-Reply-To: <8cdd9f2d-415c-1b60-0017-bf973e8cf914@riseup.net>
On Wed, Mar 1, 2017 at 10:28 AM, Patrick Schleizer
<patrick-mailinglists@whonix.org> wrote:
> Good questions, thank you for trying to figure out what I am asking. :)
>
> Junio C Hamano:
>> Patrick Schleizer <patrick-mailinglists@whonix.org> writes:
>>
>>> When using git submodules, is there value in iterating about the git
>>> submodules running "git verfiy-commit HEAD" or would that be already
>>> covered by the git submodule verification?
>>
>> That depends on what you are referring to with the "git submodule
>> verification"
>
> cd submodule
> if ! git verfiy-commit HEAD ; then
> error
> fi
>
>> and more importantly what threat you are guarding
>> against.
>
> All main (non-submodule) (merge) commits and submodule (merge) commits
> are signed by me.
>
> 1) git --recursive clone main (non-submodule) git repository
> 2) cd git main repository
> 3) git verify-commit HEAD or git verify-tag tag-name
> 4) git submodule update
>
> What if the main (non-submodule) git repository gpg signature was okay
> but then after git fetched the submodules these compromised (MITM'ed) ones?
The signing in Git is just signing the commit hash essentially.
> Does the having gpg verified the root (main git repository) ensure that
> submodule commits are also quasi verified?
That is my understanding. There is no difference between the security of
a file or a submodule, just the way of obtaining and its reporting is different.
Both a file and a submodule are referred to via a hash (currently sha1).
Obtaining a file is implicit whereas obtaining the submodule is explicit.
The reporting (in e.g. git-status) ... depends on a lot of options to be set.
When signing the superproject, you acknowledge the submodules
being in the state as recorded. (Same with s/submodules/files/)
So I am not sure what kind of additional signing you're looking
for in the submodules.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Jeff King @ 2017-03-01 19:07 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Marc Stevens, Dan Shumow, Git Mailing List
In-Reply-To: <alpine.LFD.2.20.1702281621050.22202@i7.lan>
On Tue, Feb 28, 2017 at 04:30:26PM -0800, Linus Torvalds wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Tue, 28 Feb 2017 16:12:32 -0800
> Subject: [PATCH] Put sha1dc on a diet
>
> This removes the unnecessary parts of the sha1dc code, shrinking things from
> [...]
So obviously the smaller object size is nice, and the diffstat is
certainly satisfying. My only qualm would be whether this conflicts with
the optimizations that Dan is working on (probably not conceptually, but
textually).
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pretty: use fmt_output_email_subject()
From: Jeff King @ 2017-03-01 19:22 UTC (permalink / raw)
To: René Scharfe; +Cc: Adrian Dudau, git@vger.kernel.org
In-Reply-To: <58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de>
On Wed, Mar 01, 2017 at 12:37:07PM +0100, René Scharfe wrote:
> Add the email-style subject prefix (e.g. "Subject: [PATCH] ") directly
> when it's needed instead of letting log_write_email_headers() prepare
> it in a static buffer in advance. This simplifies storage ownership and
> code flow.
This looks much cleaner to me.
I suspect we can do the same thing with the mime headers below there.
They end up in extra_headers, which is shown via after_subject. But we
could mostly replace after_subject with a call to fmt_output_email_mime()
or similar.
The only other use of extra_headers seems to be the static to/cc fields
one can add via format-patch. But those _should_ be treated differently,
as they can be allocated once in the rev_info, not per-commit. Which I
think shows off another bug. If you have a large to/cc list, that all
gets lumped into the same 1024-byte buffer, and may cause truncation.
I think the diffopt.stat_sep thing could get similar handling, too. It
appears to be set only in this one spot, and gets looked at in exactly
one. That could be replaced with an on-the-fly function call.
> This slows down the last three tests in p4000 by ca. 3% for some reason,
> so we may want to only do the first part for now, which is performance
> neutral on my machine.
It sounds like the bitfield was the cause, so that should be an easy
fix. The other question is whether it makes "--format=email" any slower.
It shouldn't, as your new approach doesn't do any extra per-commit
allocations (and in fact, it avoids some useless buffer-copying).
I couldn't measure any difference.
-Peff
^ permalink raw reply
* Re: [PATCH v7 3/3] config: add conditional include
From: Junio C Hamano @ 2017-03-01 17:47 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Jeff King, sschuberth, Matthieu Moy, Philip Oakley,
Ramsay Jones
In-Reply-To: <20170301112631.16497-4-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> int git_config_include(const char *var, const char *value, void *data)
> {
> struct config_include_data *inc = data;
> + const char *cond, *key;
> + int cond_len;
> int ret;
>
> /*
> @@ -185,6 +271,12 @@ int git_config_include(const char *var, const char *value, void *data)
>
> if (!strcmp(var, "include.path"))
> ret = handle_path_include(value, inc);
> +
> + if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
> + (cond && include_condition_is_true(cond, cond_len)) &&
> + !strcmp(key, "path"))
> + ret = handle_path_include(value, inc);
> +
> return ret;
> }
So "includeif.path" (misspelled one without any condition) falls
through to "return ret" and gives the value we got from inc->fn().
I am OK with that (i.e. "missing condition is false").
Or we can make it go to handle_path_include(), effectively making
the "include.path" a short-hand for "includeIf.path". I am also OK
with that (i.e. "missing condition is true").
Or we could even have "include.[<condition>.]path" without
"includeIf"? I am not sure if it is a bad idea that paints
ourselves in a corner, but somehow I find it tempting.
^ permalink raw reply
* Re: [PATCH 2/2] pretty: use fmt_output_email_subject()
From: Jeff King @ 2017-03-01 19:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Adrian Dudau, git@vger.kernel.org
In-Reply-To: <xmqqfuiwhnud.fsf@gitster.mtv.corp.google.com>
On Wed, Mar 01, 2017 at 10:08:10AM -0800, Junio C Hamano wrote:
> > strbuf_grow(sb, title.len + 1024);
> > - if (pp->subject) {
> > - strbuf_addstr(sb, pp->subject);
> > + if (pp->print_email_subject) {
> > + if (pp->rev)
> > + fmt_output_email_subject(sb, pp->rev);
>
> A hidden assumption this code makes is that anybody who does not
> want .rev (aka "doing it as part of format-patch that may want
> nr/total etc") does not want _any_ "Subject: ". It obviously holds
> true in today's code (the one in shortlog-add-commit is the only one
> and it sets an empty string to .subject).
>
> Does the loss of flexibility to the future callers matter, though?
> I cannot tell offhand.
>
> Thanks. Let's see what others think.
I would think that future callers would just need to provide a dummy
pp->rev. I guess that logic could be pushed down into
fmt_output_email_subject(), so that it skips looking at
opt->subject_prefix, etc, when "opt" is NULL, and just hits the
"Subject:" case arm.
I don't think it's a big deal, but it would be easy to fix now, like:
diff --git a/log-tree.c b/log-tree.c
index 4618dd04c..c73df6857 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -334,13 +334,13 @@ void fmt_output_commit(struct strbuf *filename,
void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
{
- if (opt->total > 0) {
+ if (opt && opt->total > 0) {
strbuf_addf(sb, "Subject: [%s%s%0*d/%d] ",
opt->subject_prefix,
*opt->subject_prefix ? " " : "",
digits_in_number(opt->total),
opt->nr, opt->total);
- } else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {
+ } else if (opt && opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {
strbuf_addf(sb, "Subject: [%s] ",
opt->subject_prefix);
} else {
diff --git a/pretty.c b/pretty.c
index d0f86f5d8..6b321c68c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1608,8 +1608,7 @@ void pp_title_line(struct pretty_print_context *pp,
strbuf_grow(sb, title.len + 1024);
if (pp->print_email_subject) {
- if (pp->rev)
- fmt_output_email_subject(sb, pp->rev);
+ fmt_output_email_subject(sb, pp->rev);
if (needs_rfc2047_encoding(title.buf, title.len, RFC2047_SUBJECT))
add_rfc2047(sb, title.buf, title.len,
encoding, RFC2047_SUBJECT);
-Peff
^ permalink raw reply related
* Re: [PATCH 2/2] pretty: use fmt_output_email_subject()
From: Junio C Hamano @ 2017-03-01 19:38 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Adrian Dudau, git@vger.kernel.org
In-Reply-To: <20170301193210.tgvnjgc2hfaeebqf@sigill.intra.peff.net>
On Wed, Mar 1, 2017 at 11:32 AM, Jeff King <peff@peff.net> wrote:
> I would think that future callers would just need to provide a dummy
> pp->rev. I guess that logic could be pushed down into
> fmt_output_email_subject(), so that it skips looking at
> opt->subject_prefix, etc, when "opt" is NULL, and just hits the
> "Subject:" case arm.
The "flexibility" I was wondering about is that the current .subject can
point at any caller-supplied string, not "Subject:".
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Jeff King @ 2017-03-01 19:53 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Marc Stevens, Dan Shumow, Git Mailing List
In-Reply-To: <CA+55aFx1wAS-nHS2awuW2waX=cvig4UoZqmN5H3v93yDE7ukyQ@mail.gmail.com>
On Wed, Mar 01, 2017 at 10:49:55AM -0800, Linus Torvalds wrote:
> That said, I think that it would be lovely to just default to
> USE_SHA1DC and just put the whole attack behind us. Yes, it's slower.
> No, it doesn't really seem to matter that much in practice.
My biggest concern is the index-pack operation. Try this:
time git clone --no-local --bare linux tmp.git
with and without USE_SHA1DC. I get:
[w/ openssl]
real 1m52.307s
user 2m47.928s
sys 0m14.992s
[w/ sha1dc]
real 3m4.043s
user 6m16.412s
sys 0m13.772s
That's real latency the user will see. It's hard to break it down,
though. The actual "receiving" phase is generally going to be network
bound. The delta-resolution that happens afterwards is totally local and
CPU-bound (but does run in parallel).
And of course this repository tends to the larger side (though certainly
there are bigger ones), and you only feel the pain on clone or when
doing an initial push, not day-to-day.
So maybe we just suck it up and accept that it's a bit slower.
-Peff
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Junio C Hamano @ 2017-03-01 19:41 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Marc Stevens, Dan Shumow, Git Mailing List
In-Reply-To: <CA+55aFx1wAS-nHS2awuW2waX=cvig4UoZqmN5H3v93yDE7ukyQ@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> That said, I think that it would be lovely to just default to
> USE_SHA1DC and just put the whole attack behind us. Yes, it's slower.
> No, it doesn't really seem to matter that much in practice.
Yes. It would be a very good goal.
^ permalink raw reply
* Re: [PATCH 3/6] Introduce a new "printf format" for timestamps
From: Junio C Hamano @ 2017-03-01 19:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <xmqqbmtkhna9.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> So let's introduce the pseudo format "PRItime" (currently simply being
>> "lu") so that it is easy later on to change the data type to time_t.
>
> The problem being solved is a good thing to solve, and
>
>> - printf("author-time %lu\n", ci.author_time);
>> + printf("author-time %"PRItime"\n", ci.author_time);
> ...
> It would be better to introduce the timestamp_t we discussed earlier
> before (or at) this step, and typedef it to ulong first, and then in
> this step, change the above to
>
> printf("author-time %"PRItime"\n", (timestamp_t)ci.author_time);
>
> to keep them in sync.
Nah, ignore me. This was just me being silly.
I was somehow expecting (incorrecty) that we would pick one single
PRItime for everybody and end up doing an equivalent of
printf("%llu", (unsigned long long)(something_that_is_time_t))
But as long as the plan is to configure PRItime for the platform's
time_t (or whatever the final type for timestamp_t is), we do not
have to have any extra cast here. The endgame will use the type
that is consistent with %PRItime for variables and structure fields,
and we do not want an extra cast.
^ permalink raw reply
* Re: [PATCH 01/10] submodule: decouple url and submodule existence
From: Brandon Williams @ 2017-03-01 20:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller
In-Reply-To: <xmqq8tov1ev7.fsf@gitster.mtv.corp.google.com>
On 02/24, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > Currently the submodule.<name>.url config option is used to determine
> > if a given submodule exists and is interesting to the user. This
> > however doesn't work very well because the URL is a config option for
> > the scope of a repository, whereas the existence of a submodule is an
> > option scoped to the working tree.
> >
> > In a future with worktree support for submodules, there will be multiple
> > working trees, each of which may only need a subset of the submodules
> > checked out. The URL (which is where the submodule repository can be
> > obtained) should not differ between different working trees.
> >
> > It may also be convenient for users to more easily specify groups of
> > submodules they are interested in as apposed to running "git submodule
> > init <path>" on each submodule they want checked out in their working
> > tree.
> >
> > To this end, the config option submodule.active is introduced which
> > holds a pathspec that specifies which submodules should exist in the
> > working tree.
>
> Hmph. submodule.active in .git/config would be shared the same way
> submodule.<name>.url in .git/config is shared across the worktrees
> that stems from the same primary repository, no?
>
> Perhaps there are some other uses of this submodule.active idea, but
> I do not see how it is relevant to solving "multiple worktrees"
> issue. Per-worktree config would solve it with the current
> submodule.<name>.url without submodule.active list, I would think [*1*].
Correct, I should update the language to indicate this allows the URL to
be shared between worktrees, but a per-worktree config must exist before
submodule.active can actually be used to select different groups of
submodules per-worktree. The idea is that if submodule.active is set
then you no longer look at the URLs to see what is interesting but
rather at the paths.
> Also as a grouping measure, submodule.active that lists submodule
> paths feels hard to use. When switching between two branches in the
> superproject that have the same submodule bound at two different
> paths, who is responsible for updating submodule.active in
> superproject's config? If it were a list of submodule names, this
> objection does not apply, though.
I agree that if you are listing every submodule path by hand then this
may not be the best approach and would be difficult to use. The idea is
that this would allow a user to set a general pathspec to identify a
group of modules they are interested in. Perhaps once attributes can be
used in pathspecs a user could group submodules by setting a particular
attribute and then submodule.active would have a value like
":(attr:foo)" to indicate I'm interested in all submodules with the
"foo" attribute.
>
>
>
> [Footnote]
>
> *1* At the conceptual level, I agree that .url that also means "we
> are interested in this one" feels like somewhat an unclean
> design, but that is not what you are "fixing", is it?
>
--
Brandon Williams
^ 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