* Re: SHA1 collisions found
From: Marc Stevens @ 2017-02-28 22:50 UTC (permalink / raw)
To: Dan Shumow, Linus Torvalds, Junio C Hamano
Cc: Jeff King, Joey Hess, Git Mailing List
In-Reply-To: <CY1PR0301MB21078DDCA8C679983D22821FC4560@CY1PR0301MB2107.namprd03.prod.outlook.com>
You can also keep me in this thread, so we can help or answer any
further questions,
but I also appreciate the feedback on our project.
Like Dan Shumow said, our main focus on the library has been correctness
and then performance.
The entire files ubc_check.c and ubc_check.h are generated based on
cryptanalytic data,
in particular a list of 32 disturbance vectors and their unavoidable
attack conditions,
see https://github.com/cr-marcstevens/sha1collisiondetection-tools.
sha1.c and sha1.h were coded to work for any such generated ubc_check.c
and ubc_check.h.
That means that indeed we might have some superfluous code, once used
for testing, or for generality,
but nothing that should noticeably impact runtime performance.
Because we only have 32 disturbance vectors to check, we have DVMASKSIZE
equal to 1 and maski always 0.
In the more general case when we add disturbance vectors this will not
remain the case.
Of course for dedicated code this can be simplified, and some parts
could be further optimized.
Regarding the recompression functions, the ones needed are given in the
sha1_dvs table,
but also via preprocessor defines that are used to actually only store
needed internal states:
#define DOSTORESTATE58
#define DOSTORESTATE65
For each disturbance vector there is a window of which states you can
start the recompression from,
we've optimized it so there are only 2 unique starting points (58,65)
instead of 32.
These defines should be easy to use to remove superfluous compiled
recompression functions.
Note that as each disturbance vector has its own unique message differences
(leading to different values for ctx->m2), our code does not loop over
just 2 items.
It loops over 32 distinct computations which have either of the 2
starting points.
Finally, thanks for taking a close look at our code,
this helps bringing the library in better shape also for other software
projects.
Best regards,
Marc Stevens
On 2/28/2017 10:22 PM, Dan Shumow wrote:
> [Responses inline]
>
> No need to keep me "bcc'd" (though thanks for the consideration) -- I'm happy to ignore anything I don't want to be pulled into ;-)
>
> Here's a rollup of what needs to be done based on the discussion below:
>
> 1) Remove extraneous exports from sha1.h
> 2) Remove "safe mode" support.
> 3) Remove sha1_compression_W if it is not needed by the performance improvements.
> 4) Evaluate logic around storing states and generating recompression states. Remove defines that bloat code footprint.
>
> Thanks,
> Dan
>
>
> -----Original Message-----
> From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus Torvalds
> Sent: Tuesday, February 28, 2017 11:34 AM
> To: Junio C Hamano <gitster@pobox.com>
> Cc: Jeff King <peff@peff.net>; Joey Hess <id@joeyh.name>; Git Mailing List <git@vger.kernel.org>
> Subject: Re: SHA1 collisions found
>
> On Tue, Feb 28, 2017 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> In a way similar to 8415558f55 ("sha1dc: avoid c99
>> declaration-after-statement", 2017-02-24), we would want this on top.
> There's a few other simplifications that could be done:
>
> (1) make the symbols static that aren't used.
>
> The sha1.h header ends up declaring several things that shouldn't have been exported.
>
> I suspect the code may have had some debug mode that got stripped out from it before making it public (or that was never there, and was just something the generating code could add).
>
> [danshu] Yes, this is reasonable. The emphasis of the code, heretofore, had been the illustration of our unavoidable bit condition performance improvement to counter cryptanalysis. I'm happy to remove the unused stuff from the public header.
>
> (2) get rid of the "safe mode" support.
>
> That one is meant for non-checking replacements where it generates a *different* hash for input with the collision fingerpring, but that's pointless for the git use when we abort on a collision fingerprint.
>
> [danshu] Yes, I agree that if you aren't using this it can be taken out. I believe Marc has some use cases / potentially consumers of this algorithm in mind. We can move it into separate header/source files for anyone who wants to use it.
>
> I think the first one will show that the sha1_compression() function isn't actually used, and with the removal of safe-mode I think
> sha1_compression_W() also is unused.
>
> [danshu] Some of the performance experiments that I've looked at involve putting the sha1_compression_W(...) back in. Though, that doesn't look like it's helping. If it is unused after the performance improvements, we'll take it out, or move it into its own file.
>
> Finally, only states 58 and 65 (out of all 80 states) are actually used, and from what I can tell, the 'maski' value is always 0, so the looping over 80 state masks is really just a loop over two.
>
> [danshu] So, while looking at performance optimizations, I specifically looked at how much removing storing the intermediate states helps -- And I found that it doesn't seem to make a difference for performance. My cursory hypothesis is because nothing is waiting on those writes to memory, the code moves on quickly. That said, it is a bunch of code that is essentially doing nothing and removing that is worthwhile. Though, partially what we're seeing here is that, as you point out below, we're working with generated code that we want to be general. Specifically, right now, we're checking only disturbance vectors that we know can be used to efficiently attack the compression function. It may be the case that further cryptanalysis uncovers more. We want to have a general enough approach that we can add scanning for new disturbance vectors if they're found later. Over specializing the code makes that more difficult, as currently the algorithm is data driven, and we don't need to write new code, but rather just add more data to check. One other note -- the "maski" field of the dv_info_t struct is not an index to check the state, but rather an index into the mask generated by the ubc check code, so that doesn't pertain to looping over the states. More on this below.
>
> The file has code top *generate* all the 80 sha1_recompression_step() functions, and I don't think the compiler is smart enough to notice that only two of them matter.
>
> [danshu] That's a good observation -- We should clean up the unused recompression steps, especially because that will generate a ton of object code. We should add some logic to only compile the functions that are used.
>
> And because 'maski' is always zero, thisL
>
> ubc_dv_mask[sha1_dvs[i].maski]
>
> code looks like it might as well just use ubc_dv_mask[0] - in fact the ubc_dv_mask[] "array" really is just a single-entry array anyway:
>
> #define DVMASKSIZE 1
>
> [danshu] The idea here is that we are currently checking 32 disturbance vectors with our bit mask. We're checking 32 DVs, because we have 32 bits of mask that we can use. The DVs are ordered by their probability of leading to an attack (which is directly correlated to the complexity of finding a collision.) Several of those DVs correspond to very low probability / high cost attacks, which we wouldn't expect to see in practice. We just have the space to check, so why not? However, improvements in cryptanalysis may make those attacks cheaper, in which case, we would potentially want to add more DVs to check, in which case we would expand the number of DVs and the mask.
>
> so that code has a few oddities in it. It's generated code, which is probably why.
>
> [danshu] Accurate, we're also just trying to be general enough that we can easily add more DVs later if need be. I don't know how likely that is, certainly the DVs that we're checking now are based on solid conjectures and rigorous analysis of the problem. Though we don't want to rule out that there will be subsequent cryptanalytic developments later. Marc can comment more here.
>
> Basically, some of it could be improved. In particular, the "generate code for 80 different recompression cases, but only ever use two of them" really looks like it would blow up the code generation footprint a lot.
>
> I'm adding Marc Stevens and Dan Shumow to this email (bcc'd, so that they don't get dragged into any unrelated email threads) in case they want to comment.
>
> I'm wondering if they perhaps have a cleaned-up version somewhere, or maybe they can tell me that I'm just full of sh*t and missed something.
>
> [danshu] Naw man, it looks pretty good, modulo a little bit of understandable confusion over 'maski' -- No fake news or alternative facts here ;-)
>
> Linus
^ permalink raw reply
* Re: [PATCH 2/3] revision: exclude trees/blobs given commit
From: Junio C Hamano @ 2017-02-28 23:12 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff, peartben, benpeart
In-Reply-To: <7082d91f30663b2e6d7fb1795c5ea37d3fe3446c.1487984670.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> When the --objects argument is given to rev-list, an argument of the
> form "^$tree" can be given to exclude all blobs and trees reachable from
> that tree, but an argument of the form "^$commit" only excludes that
> commit, not any blob or tree reachable from it. Make "^$commit" behave
> consistent to "^$tree".
So with this:
$ git rev-list --objects ^HEAD^@ HEAD ^HEAD^{tree}
should be a round-about way to say
$ git rev-parse HEAD
;-)
The expression wants to list everything reachable from HEAD, but it
does not want to show its parents (i.e. ^HEAD^@) and it does not
want to show its tree (i.e. ^HEAD^{tree}), so the only thing that
remains is the commit object HEAD and nothing else?
I agree with Peff's comment about objects that may appear beyond the
boundary (i.e. merge base between interesting ones and uninteresting
ones); whether that inaccuracy matters depends on what you want to
use this for---if you want to hide sensitive objects it does, if you
want to reduce the network cost without incurring too much cpu cost,
it probably does not.
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-28 22:56 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Junio C Hamano, Jeff King, Joey Hess, Git Mailing List
In-Reply-To: <CAJo=hJuB9JkTZSRbhN2DX0gBqpjddU=Sk8iRV9++TYRv4xKA6Q@mail.gmail.com>
On Tue, Feb 28, 2017 at 11:52 AM, Shawn Pearce <spearce@spearce.org> wrote:
>
>> and from what I can tell, the 'maski' value is always 0, so the
>> looping over 80 state masks is really just a loop over two.
>
> Actually, look closer at that loop:
No, sorry, I wasn't clear and took some shortcuts in writing that made
that sentence not parse right.
There's two issues going on. This loop:
> for (i = 0; sha1_dvs[i].dvType != 0; ++i)
loops over all the dvs - and then inside it has that useless "maski"
thing as part of the test that is always zero.
But the "80 state masks" was not that "maski' value, but the
"ctx->states[5][80]" thing.
So we have 80 of those 5-word state values, but only two of them are
actually used: iterations 58 and 65). You can see how the code
actually optimizes away (by hand) the SHA1_STORE_STATE() thing by
using DOSTORESTATE58 and DOSTORESTATE65, but it does actually generate
the code for all of them.
You can see the "only two steps" part in this:
(sha1_recompression_step[sha1_dvs[i].testt])(...)
if you notice how there are only those two different cases for "testt".
So there is code generated for 80 different recompression step
functions in that array, but there are only two different functions
that are actually ever used.
Those are not small functions, either. When I check the build, they
generate about 3.5kB of code each. So it's literally about 250kB of
completely wasted space in the binary.
See what I'm saying? Two different issues. One is that the code
generates tons of (fairly big) functions, and only uses two of them,
the rest are useless and lying around. The other is that it uses a
variable that is only ever zero.
So I think that loop would actually be better not as a loop at all,
but as a "generated code expanded from the dv_data". It would have
been more obvious. Right now it loads values from the array, and it's
not obvious that some of the values it loads are very very limited (to
the point of one of them just being the constant "0").
Anyway, Dan Shumow already answered and addressed both issues (and the
smaller stylistic ones).
Linus
^ permalink raw reply
* Re: [BUG] branch renamed to 'HEAD'
From: Jacob Keller @ 2017-02-28 22:48 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
On Tue, Feb 28, 2017 at 4:06 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 27, 2017 at 07:53:02PM -0500, Jeff King wrote:
>
>> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:
>>
>> > A flag to affect the behaviour (as opposed to &flag as a secondary
>> > return value, like Peff's patch does) can be made to work. Perhaps
>> > a flag that says "keep the input as is if the result is not a local
>> > branch name" would pass an input "@" intact and that may be
>> > sufficient to allow "git branch -m @" to rename the current branch
>> > to "@" (I do not think it is a sensible rename, though ;-). But
>> > probably some callers need to keep the original input and compare
>> > with the result to see if we expanded anything if we go that route.
>> > At that point, I am not sure if there are much differences in the
>> > ease of use between the two approaches.
>>
>> I just went into more detail in my reply to Jacob, but I do think this
>> is a workable approach (and fortunately we seem to have banned bare "@"
>> as a name, along with anything containing "@{}", so I think we would end
>> up rejecting these nonsense names).
>>
>> I'll see if I can work up a patch. We'll still need to pass the flag
>> around through the various functions, but at least it will be a flag and
>> not a confusing negated out-parameter.
>
> OK, I have a series which fixes this (diffstat below). When I audited
> the other callers of interpret_branch_name() and strbuf_branchname(), it
> turned out to be even more complicated. The callers basically fall into
> a few buckets:
>
> 1. Callers like get_sha1() and merge_name() pass the result to
> dwim_ref(), and are prepared to handle anything.
>
> 2. Some callers stick "refs/heads/" in front of the result, and
> obviously only want local names. Most of git-branch and
> git-checkout fall into this boat.
>
> 3. "git branch -d" can delete local _or_ remote branches, depending on
> the "-r" flag. So the expansion it wants varies, and we need to
> handle "just local" or "just remote".
>
> So I converted the "only_branch" flag to an "allowed" bit-field. No
> callers actually ask for more than a single type at once, but it was
> easy to do it that way. It serves all of the callers, and will easily
> adapt for the future (e.g., if "git branch -a -d" were ever allowed).
>
> [1/8]: interpret_branch_name: move docstring to header file
> [2/8]: strbuf_branchname: drop return value
> [3/8]: strbuf_branchname: add docstring
> [4/8]: interpret_branch_name: allow callers to restrict expansions
> [5/8]: t3204: test git-branch @-expansion corner cases
> [6/8]: branch: restrict @-expansions when deleting
> [7/8]: strbuf_check_ref_format(): expand only local branches
> [8/8]: checkout: restrict @-expansions when finding branch
>
> builtin/branch.c | 5 +-
> builtin/checkout.c | 2 +-
> builtin/merge.c | 2 +-
> cache.h | 32 ++++++++-
> refs.c | 2 +-
> revision.c | 2 +-
> sha1_name.c | 76 ++++++++++-----------
> strbuf.h | 21 +++++-
> t/t3204-branch-name-interpretation.sh | 122 ++++++++++++++++++++++++++++++++++
> 9 files changed, 220 insertions(+), 44 deletions(-)
> create mode 100755 t/t3204-branch-name-interpretation.sh
>
> -Peff
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"
^ permalink raw reply
* Re: [PATCH 2/3] revision: exclude trees/blobs given commit
From: Jeff King @ 2017-02-28 22:12 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster, peartben, benpeart
In-Reply-To: <7082d91f30663b2e6d7fb1795c5ea37d3fe3446c.1487984670.git.jonathantanmy@google.com>
On Fri, Feb 24, 2017 at 05:18:37PM -0800, Jonathan Tan wrote:
> When the --objects argument is given to rev-list, an argument of the
> form "^$tree" can be given to exclude all blobs and trees reachable from
> that tree, but an argument of the form "^$commit" only excludes that
> commit, not any blob or tree reachable from it. Make "^$commit" behave
> consistent to "^$tree".
Like Junio, I suspect this is going to be quite expensive. This is
similar to the "--objects-edge" and "--objects-edge-aggressive" options,
which we had to pull back on the use of because of their expensiveness.
(And as an aside, wouldn't those options be the right place for what
you're doing?).
I also think that the mechanism here is not 100% accurate. The commit
traversal will stop once it has painted down, so you're effectively
exploring the trees of the merge bases. But older history could mention
an object that has resurfaced again (e.g., due to a cherry-pick).
Getting the 100% accurate answer is _really_ expensive, though with
reachability bitmaps it's not too bad. I just wonder if that's a better
approach to be taking.
-Peff
^ permalink raw reply
* Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions
From: Jeff King @ 2017-02-28 21:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <xmqqr32ijc1q.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 28, 2017 at 12:27:45PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The original purpose of interpret_branch_name() was to be used by
> > get_sha1() in resolving refs. As such, some of its expansions may
> > point to refs outside of the local "refs/heads" namespace.
>
> I am not sure the reference to "get_sha1()" is entirely correct.
>
> Until it was renamed at 431b1969fc ("Rename interpret/substitute
> nth_last_branch functions", 2009-03-21), the function was called
> interpret_nth_last_branch() which was originally introduced for the
> name, not sha1, at ae5a6c3684 ("checkout: implement "@{-N}" shortcut
> name for N-th last branch", 2009-01-17). The use of the same syntax
> and function for the object name came a bit later.
>
> But I think that is an insignificant detail. Let's read on.
Yeah, sorry, I was lazy about digging up the history. I think the
problem actually started in ae0ba8e20a (Teach @{upstream} syntax to
strbuf_branchanme(), 2010-01-19), when the features were ported over
from get_sha1() to interpret_branch_name().
Since I need to re-roll anyway, I'll tweak this to be more accurate.
> > @@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
> > static char *substitute_branch_name(const char **string, int *len)
> > {
> > struct strbuf buf = STRBUF_INIT;
> > - int ret = interpret_branch_name(*string, *len, &buf);
> > + int ret = interpret_branch_name(*string, *len, &buf, 0);
> >
> > if (ret == *len) {
> > size_t size;
>
> This is the one used by dwim_ref/log, so we'd need to allow it to
> resolve to anything, e.g. "@" -> "HEAD", and pretend that the user
> typed that expansion. OK.
Yeah. Left them all as "0" here, and then split the updates into their
own commits. So there's no commit that says "and we are leaving this
spot, because it is correct as-is". The other notable one is the
strbuf_branchname() call in merge_name().
I can mention those in the commit message here (I think I did in the
cover letter, but it would be nice to stick it in the history, since
that will be what comes up if you blame those lines).
-Peff
^ permalink raw reply
* Re: format-patch subject-prefix gets truncated when using the --numbered flag
From: Junio C Hamano @ 2017-02-28 19:33 UTC (permalink / raw)
To: Adrian Dudau; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqinnuky9e.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Adrian Dudau <Adrian.Dudau@enea.com> writes:
>
>> I noticed that the --subject-prefix string gets truncated sometimes,
>> but only when using the --numbered flat. Here's an example:
>>
>> addu@sestofb11:/data/fb/addu/git$ export longm="very very very very
>> very very very very very very very very very very long prefix"
>
> This looks like "dr, my arm hurts when I twist it this way---don't
> do that then" ;-). Truncation is designed and intended to preserve
> space for the real title of commit.
>
> Having said that...
> ...
> I think this is just an old oversight. The latter should have been
> even shorter than the former one (or the former should be longer
> than the latter) to account for the width of the counter e.g. 01/27.
And having said all that, if we really want to allow overlong
subject prefix that would end up hiding the real title of the patch,
a modern way to do so would be to use xstrfmt() like the attached
toy-patch does. Note that this is merely a small first step---you'd
notice that "subject" is kept around as a "static" and only freed
upon entry to this function for the second time, to preserve the
ownership model of the original code. In a real "fix" (if this
needs to be "fixed", that is), I think the ownership model of the
storage used for *subject_p and *extra_headers_p needs to be updated
so that it will become caller's responsibility to free them
(similarly, the ownership model of opt->diffopt.stat_sep that is
assigned the address of the static buffer[] in the same function
needs to be revisited).
That "buffer" thing I think would need to be a bit more careful even
in the current code, which _does_ use snprintf() correctly to avoid
overflowing the buffer[], by the way. If you have an overlong
opt->mime_boundary, the resulting "e-mail" looking output can become
structurely broken. The truncation may happen way before the full
line for Content-Transfer-Encoding: is written, for example.
So this function seems to have a lot more graver problems that need
to be looked at.
log-tree.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 8c2415747a..24c98f5a80 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -337,29 +337,23 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **extra_headers_p,
int *need_8bit_cte_p)
{
- const char *subject = NULL;
+ static const char *subject = NULL;
const char *extra_headers = opt->extra_headers;
const char *name = oid_to_hex(opt->zero_commit ?
&null_oid : &commit->object.oid);
+ free((void *)subject);
*need_8bit_cte_p = 0; /* unknown */
if (opt->total > 0) {
- static char buffer[64];
- snprintf(buffer, sizeof(buffer),
- "Subject: [%s%s%0*d/%d] ",
- opt->subject_prefix,
- *opt->subject_prefix ? " " : "",
- digits_in_number(opt->total),
- opt->nr, opt->total);
- subject = buffer;
+ subject = xstrfmt("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) {
- static char buffer[256];
- snprintf(buffer, sizeof(buffer),
- "Subject: [%s] ",
- opt->subject_prefix);
- subject = buffer;
+ subject = xstrfmt("Subject: [%s] ", opt->subject_prefix);
} else {
- subject = "Subject: ";
+ subject = xstrdup("Subject: ");
}
fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
^ permalink raw reply related
* Re: [PATCH 0/6] Use time_t
From: Jeff King @ 2017-02-28 22:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <xmqqh93ehrxx.fsf@gitster.mtv.corp.google.com>
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. But I
haven't even looked at the code changes needed for time_t. I suspect
Dscho has a better feel for it at this point.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info
From: Jeff King @ 2017-02-28 22:06 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster, peartben, benpeart
In-Reply-To: <06a84f8c77924b275606384ead8bb2fd7d75f7b6.1487984670.git.jonathantanmy@google.com>
On Fri, Feb 24, 2017 at 05:18:36PM -0800, Jonathan Tan wrote:
> Whenever tree_objects is set to 1 in revision.h's struct rev_info,
> blob_objects is likewise set, and vice versa. Combine those two fields
> into one.
>
> Some of the existing code does not handle tree_objects being different
> from blob_objects properly. For example, "handle_commit" in revision.c
> recurses from an UNINTERESTING tree into its subtree if tree_objects ==
> 1, completely ignoring blob_objects; it probably should still recurse if
> tree_objects == 0 and blob_objects == 1 (to mark the blobs), and should
> behave differently depending on blob_objects (controlling the
> instantiation and marking of blob objects). This commit resolves the
> issue by forbidding tree_objects from being different to blob_objects.
Yeah, I agree that is awkward. I'm OK with the rule "if blob_objects is
set, then tree_objects must also be set". It's the other way around I
care more about.
> 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. However, this results in
> a minor performance savings at best in that objects no longer need to be
> instantiated (causing memory allocations and hashtable insertions) - no
> disk reads are being done for objects whether blob_objects is set or
> not.
In a full object-graph traversal, we actually spend a big chunk of our
time in hash lookups. My measurements (admittedly from 2013, which I
haven't repeated lately) show something like a 20-25% speedup for this
case.
My only use for it (and the source of those timings) was to compute
archive reachability, which nobody seems to care too much about. But I
suspect we could speed up your case, too, when we are just computing the
reachability of a non-blob. I.e., you should be able to turn on the
smallest subset of "commits only", "commits and trees", and "commits,
trees, and blobs", based on what the other side has asked for.
-Peff
^ permalink raw reply
* Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions
From: Junio C Hamano @ 2017-02-28 20:27 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228121434.2dhngs4peq5acic2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The original purpose of interpret_branch_name() was to be used by
> get_sha1() in resolving refs. As such, some of its expansions may
> point to refs outside of the local "refs/heads" namespace.
I am not sure the reference to "get_sha1()" is entirely correct.
Until it was renamed at 431b1969fc ("Rename interpret/substitute
nth_last_branch functions", 2009-03-21), the function was called
interpret_nth_last_branch() which was originally introduced for the
name, not sha1, at ae5a6c3684 ("checkout: implement "@{-N}" shortcut
name for N-th last branch", 2009-01-17). The use of the same syntax
and function for the object name came a bit later.
But I think that is an insignificant detail. Let's read on.
> Over time, the function has been picked up by other callers
> who want to use the ref-expansion to give the user access to
> the same shortcuts (e.g., allowing "git branch" to delete
> via "@{-1}" or "@{upstream}"). These uses have confusing
> corner cases when the expansion isn't in refs/heads/ (for
> instance, deleting "@" tries to delete refs/heads/HEAD,
> which is nonsense).
>
> Callers can't know from the returned string how the
> expansion happened (e.g., did the user really ask for a
> branch named "HEAD", or did we do a bogus expansion?). One
> fix would be to return some out-parameters describing the
> types of expansion that occurred. This has the benefit that
> the caller can generate precise error messages ("I
> understood @{upstream} to mean origin/master, but that is a
> remote tracking branch, so you cannot create it as a local
> name").
>
> However, out-parameters make calling interface somewhat
> cumbersome. Instead, let's do the opposite: let the caller
> tell us which elements to expand. That's easier to pass in,
> and none of the callers give more precise error messages
> than "@{upstream} isn't a valid branch name" anyway (which
> should be sufficient).
>
> The strbuf_branchname() function needs a similar parameter,
> as most of the callers access interpret_branch_name()
> through it. For now, we'll pass "0" for "no restrictions" in
> each caller, and update them individually in subsequent
> patches.
OK.
> diff --git a/cache.h b/cache.h
> index c67995caa..a8816c914 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as s
> *
> * If the input was ok but there are not N branch switches in the
> * reflog, it returns 0.
> - */
> -extern int interpret_branch_name(const char *str, int len, struct strbuf *);
> + *
> + * If "allowed" is non-zero, it is a treated as a bitfield of allowable
> + * expansions: local branches ("refs/heads/"), remote branches
> + * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
> + * allowed, even ones to refs outside of those namespaces.
> + */
Answering the question in your follow-up, I personally do not find
"0 means anything goes" too confusing, but for satisfying those who
do, spelling ~0 is not too bad, either.
> +#define INTERPRET_BRANCH_LOCAL (1<<0)
> +#define INTERPRET_BRANCH_REMOTE (1<<1)
> +#define INTERPRET_BRANCH_HEAD (1<<2)
> +extern int interpret_branch_name(const char *str, int len, struct strbuf *,
> + unsigned allowed);
> extern int get_oid_mb(const char *str, struct object_id *oid);
> diff --git a/refs.c b/refs.c
> index 6d0961921..da62119c2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
> static char *substitute_branch_name(const char **string, int *len)
> {
> struct strbuf buf = STRBUF_INIT;
> - int ret = interpret_branch_name(*string, *len, &buf);
> + int ret = interpret_branch_name(*string, *len, &buf, 0);
>
> if (ret == *len) {
> size_t size;
This is the one used by dwim_ref/log, so we'd need to allow it to
resolve to anything, e.g. "@" -> "HEAD", and pretend that the user
typed that expansion. OK.
^ permalink raw reply
* Re: format-patch subject-prefix gets truncated when using the --numbered flag
From: Jeff King @ 2017-02-28 18:17 UTC (permalink / raw)
To: Adrian Dudau; +Cc: git@vger.kernel.org
In-Reply-To: <1488297556.2955.11.camel@enea.com>
On Tue, Feb 28, 2017 at 03:59:16PM +0000, Adrian Dudau wrote:
> This is happening on the latest master branch, so I dug through the
> code and tracked the issue to this piece of code in log-tree.c:
>
> if (opt->total > 0) {
> static char buffer[64];
> snprintf(buffer, sizeof(buffer),
> "Subject: [%s%s%0*d/%d] ",
> opt->subject_prefix,
> *opt->subject_prefix ? " " : "",
> digits_in_number(opt->total),
> opt->nr, opt->total);
> subject = buffer;
> } else if (opt->total == 0 && opt->subject_prefix && *opt-
> >subject_prefix) {
> static char buffer[256];
> snprintf(buffer, sizeof(buffer),
> "Subject: [%s] ",
> opt->subject_prefix);
> subject = buffer;
> } else {
> subject = "Subject: ";
> }
>
> Apparently the size of the "buffer" var is different in the two
> situations. Anybody knows if this is by design or just an old
> oversight?
I think it's just an old oversight. There are some other fixed-size
buffers later, too, which could similarly truncate.
I think these should all be "static strbuf", and entering the function
they should get strbuf_reset(), followed by a strbuf_addf(). The static
strbufs will be the owners of the allocated heap memory, and it will get
reused from call to call.
That stops the immediate problem. As a function interface, it's pretty
ugly. It would probably make more sense for the caller to pass in a
strbuf rather than have us pass out pointers to static storage. For the
call in make_cover_letter(), that would be fine. For show_log(), it's
less clear. That's called for every commit in "git log", which might be
a little sensitive to allocations.
The only persistent storage it has is via the rev_info. Perhaps it could
hold some scratch buffers for the traversal (though I don't think we
currently do any resource-freeing when done with a rev_info, so it
effectively becomes a leak).
-Peff
^ permalink raw reply
* Re: [PATCH v8 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Junio C Hamano @ 2017-02-28 22:15 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <20170228203340.18723-5-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> + git reset ${GIT_QUIET:+-q} -- "$@"
> + git ls-files -z --modified -- "$@" |
> + git checkout-index -z --force --stdin
> + git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
I think you forgot to remove this line, whose correction was added
as two lines immediately before it. I'll remove it while queuing.
> + git clean --force ${GIT_QUIET:+-q} -d -- "$@"
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info
From: Jeff King @ 2017-02-28 21:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git, peartben, benpeart
In-Reply-To: <xmqq1suij8kr.fsf@gitster.mtv.corp.google.com>
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).
The whole series (from 2013!) is at:
git://github.com/peff/git jk/archive-reachability
but the relevant commits are below.
I don't think the same logic holds for this case, though, because
somebody actually can ask for a single blob.
-- >8 --
From: Jeff King <peff@peff.net>
Date: Wed, 5 Jun 2013 17:57:02 -0400
Subject: [PATCH] list-objects: optimize "revs->blob_objects = 0" case
If we are traversing trees during a "--objects"
traversal, we may skip blobs if the "blob_objects" field of
rev_info is not set. But we do so as the first thing in
process_blob(), only after we have actually created the
"struct blob" object, incurring a hash lookup. We can
optimize out this no-op call completely.
This does not actually affect any current code, as all of
the current traversals always set blob_objects when looking
at objects, anyway.
Signed-off-by: Jeff King <peff@peff.net>
---
list-objects.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/list-objects.c b/list-objects.c
index f3ca6aafb..58ad69557 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -117,7 +117,7 @@ static void process_tree(struct rev_info *revs,
process_gitlink(revs, entry.oid->hash,
show, base, entry.path,
cb_data);
- else
+ else if (revs->blob_objects)
process_blob(revs,
lookup_blob(entry.oid->hash),
show, base, entry.path,
--
2.12.0.359.gd4c8c42e9
-- >8 --
From: Jeff King <peff@peff.net>
Date: Wed, 5 Jun 2013 18:02:42 -0400
Subject: [PATCH] archive: ignore blob objects when checking reachability
We cannot create an archive from a blob object, so we would
not expect anyone to provide one to us. And if they do, we
will fail anyway just after the reachability check. We can
therefore optimize our reachability check to ignore blobs
completely, and not even create a "struct blob" for them.
Depending on the repository size and the exact place we find
the reachable object in the traversal, this can save 20-25%,
a we can avoid many lookups in the object hash.
The downside of this is that a blob provided to a remote
archive process will fail with "no such object" rather than
"object is not a tree" (we could organize the code to retain
the old message, but since we no longer know whether the
blob is reachable or not, we would potentially be leaking
information about the existence of unreachable objects).
Signed-off-by: Jeff King <peff@peff.net>
---
archive.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/archive.c b/archive.c
index ef89b2556..489115f9f 100644
--- a/archive.c
+++ b/archive.c
@@ -383,6 +383,7 @@ static int object_is_reachable(const unsigned char *sha1)
save_commit_buffer = 0;
init_revisions(&data.revs, NULL);
setup_revisions(ARRAY_SIZE(argv) - 1, argv, &data.revs, NULL);
+ data.revs.blob_objects = 0;
if (prepare_revision_walk(&data.revs))
return 0;
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* Re: git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Junio C Hamano @ 2017-02-28 21:50 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git, Mike Crowe
In-Reply-To: <d98aa589-3e08-249d-0c88-72dbcee1a568@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> On 2017-02-27 21:17, Junio C Hamano wrote:
>
>> Torsten, you've been quite active in fixing various glitches around
>> the EOL conversion in the latter half of last year. Have any
>> thoughts to share on this topic?
>>
>> Thanks.
>
> Sorry for the delay, being too busy with other things.
> I followed the discussion, but didn't have good things to contribute.
> I am not an expert in diff.c, but there seems to be a bug, thanks everybody
> for digging.
>
> Back to business:
>
> My understanding is that git diff --quiet should be quiet, when
> git add will not do anything.
Yes, I think that is a sensible criterion. What I was interested to
hear from you the most was to double check if Mike's expectation is
reasonable. Earlier we had a lengthy discussion on what to do when
convert-to-git and convert-to-working-tree conversions do not round
trip, and I was wondering if this was one of those cases.
^ permalink raw reply
* Re: [PATCH v2 1/2] Documentation: Improve description for core.quotePath
From: Andreas Heiduk @ 2017-02-28 20:55 UTC (permalink / raw)
To: Jakub Narębski, gitster; +Cc: git
In-Reply-To: <f6845604-005d-06bb-e5e6-61f683cdbaf8@gmail.com>
Am 24.02.2017 um 22:43 schrieb Jakub Narębski:
> W dniu 24.02.2017 o 21:37, Andreas Heiduk pisze:
>> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
>
> Thanks. This is good work.
:-)
>
>> ---
>> Documentation/config.txt | 24 ++++++++++++++----------
>> 1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
[...]
>> +
>
> This empty line should not be here, I think.
Missed that bugger...
[...]
>> + values larger than 0x80 (e.g. octal `\302\265` for "micro" in
>
> I wonder if we can put UTF-8 in AsciiDoc, that is write "μ"
> instead of spelling it "micro" (or: Greek letter "mu").
>
> Or "µ" / "µ", though I wonder how well it is supported
> in manpage, info and PDF outputs...
... and fonts installed on client machines!
Since I started with a two-line diff for just git-ls-files I decided to
play safe with this,
^ permalink raw reply
* RE: SHA1 collisions found
From: Dan Shumow @ 2017-02-28 21:22 UTC (permalink / raw)
To: Linus Torvalds, Junio C Hamano
Cc: Jeff King, Joey Hess, Git Mailing List,
'marc.stevens@cwi.nl'
In-Reply-To: <CA+55aFxTWqsTTiDKo4DBZT-8Z9t80bGMD3uijzKONa_bYEZABQ@mail.gmail.com>
[Responses inline]
No need to keep me "bcc'd" (though thanks for the consideration) -- I'm happy to ignore anything I don't want to be pulled into ;-)
Here's a rollup of what needs to be done based on the discussion below:
1) Remove extraneous exports from sha1.h
2) Remove "safe mode" support.
3) Remove sha1_compression_W if it is not needed by the performance improvements.
4) Evaluate logic around storing states and generating recompression states. Remove defines that bloat code footprint.
Thanks,
Dan
-----Original Message-----
From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus Torvalds
Sent: Tuesday, February 28, 2017 11:34 AM
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>; Joey Hess <id@joeyh.name>; Git Mailing List <git@vger.kernel.org>
Subject: Re: SHA1 collisions found
On Tue, Feb 28, 2017 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> In a way similar to 8415558f55 ("sha1dc: avoid c99
> declaration-after-statement", 2017-02-24), we would want this on top.
There's a few other simplifications that could be done:
(1) make the symbols static that aren't used.
The sha1.h header ends up declaring several things that shouldn't have been exported.
I suspect the code may have had some debug mode that got stripped out from it before making it public (or that was never there, and was just something the generating code could add).
[danshu] Yes, this is reasonable. The emphasis of the code, heretofore, had been the illustration of our unavoidable bit condition performance improvement to counter cryptanalysis. I'm happy to remove the unused stuff from the public header.
(2) get rid of the "safe mode" support.
That one is meant for non-checking replacements where it generates a *different* hash for input with the collision fingerpring, but that's pointless for the git use when we abort on a collision fingerprint.
[danshu] Yes, I agree that if you aren't using this it can be taken out. I believe Marc has some use cases / potentially consumers of this algorithm in mind. We can move it into separate header/source files for anyone who wants to use it.
I think the first one will show that the sha1_compression() function isn't actually used, and with the removal of safe-mode I think
sha1_compression_W() also is unused.
[danshu] Some of the performance experiments that I've looked at involve putting the sha1_compression_W(...) back in. Though, that doesn't look like it's helping. If it is unused after the performance improvements, we'll take it out, or move it into its own file.
Finally, only states 58 and 65 (out of all 80 states) are actually used, and from what I can tell, the 'maski' value is always 0, so the looping over 80 state masks is really just a loop over two.
[danshu] So, while looking at performance optimizations, I specifically looked at how much removing storing the intermediate states helps -- And I found that it doesn't seem to make a difference for performance. My cursory hypothesis is because nothing is waiting on those writes to memory, the code moves on quickly. That said, it is a bunch of code that is essentially doing nothing and removing that is worthwhile. Though, partially what we're seeing here is that, as you point out below, we're working with generated code that we want to be general. Specifically, right now, we're checking only disturbance vectors that we know can be used to efficiently attack the compression function. It may be the case that further cryptanalysis uncovers more. We want to have a general enough approach that we can add scanning for new disturbance vectors if they're found later. Over specializing the code makes that more difficult, as currently the algorithm is data driven, and we don't need to write new code, but rather just add more data to check. One other note -- the "maski" field of the dv_info_t struct is not an index to check the state, but rather an index into the mask generated by the ubc check code, so that doesn't pertain to looping over the states. More on this below.
The file has code top *generate* all the 80 sha1_recompression_step() functions, and I don't think the compiler is smart enough to notice that only two of them matter.
[danshu] That's a good observation -- We should clean up the unused recompression steps, especially because that will generate a ton of object code. We should add some logic to only compile the functions that are used.
And because 'maski' is always zero, thisL
ubc_dv_mask[sha1_dvs[i].maski]
code looks like it might as well just use ubc_dv_mask[0] - in fact the ubc_dv_mask[] "array" really is just a single-entry array anyway:
#define DVMASKSIZE 1
[danshu] The idea here is that we are currently checking 32 disturbance vectors with our bit mask. We're checking 32 DVs, because we have 32 bits of mask that we can use. The DVs are ordered by their probability of leading to an attack (which is directly correlated to the complexity of finding a collision.) Several of those DVs correspond to very low probability / high cost attacks, which we wouldn't expect to see in practice. We just have the space to check, so why not? However, improvements in cryptanalysis may make those attacks cheaper, in which case, we would potentially want to add more DVs to check, in which case we would expand the number of DVs and the mask.
so that code has a few oddities in it. It's generated code, which is probably why.
[danshu] Accurate, we're also just trying to be general enough that we can easily add more DVs later if need be. I don't know how likely that is, certainly the DVs that we're checking now are based on solid conjectures and rigorous analysis of the problem. Though we don't want to rule out that there will be subsequent cryptanalytic developments later. Marc can comment more here.
Basically, some of it could be improved. In particular, the "generate code for 80 different recompression cases, but only ever use two of them" really looks like it would blow up the code generation footprint a lot.
I'm adding Marc Stevens and Dan Shumow to this email (bcc'd, so that they don't get dragged into any unrelated email threads) in case they want to comment.
I'm wondering if they perhaps have a cleaned-up version somewhere, or maybe they can tell me that I'm just full of sh*t and missed something.
[danshu] Naw man, it looks pretty good, modulo a little bit of understandable confusion over 'maski' -- No fake news or alternative facts here ;-)
Linus
^ permalink raw reply
* Re: Transition plan for git to move to a new hash function
From: brian m. carlson @ 2017-02-28 21:47 UTC (permalink / raw)
To: Ian Jackson
Cc: Jeff King, Ævar Arnfjörð Bjarmason, Linus Torvalds,
Jason Cooper, ankostis, Junio C Hamano, Git Mailing List,
Stefan Beller, David Lang, Joey Hess
In-Reply-To: <22708.8913.864049.452252@chiark.greenend.org.uk>
[-- Attachment #1: Type: text/plain, Size: 3838 bytes --]
On Mon, Feb 27, 2017 at 01:00:01PM +0000, Ian Jackson wrote:
> I said I was working on a transition plan. Here it is. This is
> obviously a draft for review, and I have no official status in the git
> project. But I have extensive experience of protocol compatibility
> engineering, and I hope this will be helpful.
>
> Ian.
>
>
> Subject: Transition plan for git to move to a new hash function
>
>
> BASIC PRINCIPLES
> ================
>
> We run multiple hashes in parallel. Each object is named by exactly
> one hash. We define that objects with identical content, but named by
> different hash functions, are different objects.
I think this is fine.
> Objects of one hash may refer to objects named by a different hash
> function to their own. Preference rules arrange that normally, new
> hash objects refer to other new hash objects.
The existing codebase isn't really intended with that in mind.
It's not that I am arguing against this because I think it's a bad idea,
I'm arguing against it because as a contributor, I'm doubtful that this
is easily achievable given the state of the codebase.
> The intention is that for most projects, the existing SHA-1 based
> history will be retained and a new history built on top of it.
> (Rewriting is also possible but means a per-project hard switch.)
I like Peff's suggested approach in which we essentially rewrite history
under the hood, but have a lookup table which looks up the old hash
based on the new hash. That allows us to refer to old objects, but not
have to share serialized data that mentions both hashes.
Obviously only the SHA-1 versions of old tags and commits will be able
to be validated, but that shouldn't be an issue. We can hook that code
into a conversion routine that can handle on-the-fly object conversion.
We also can implement (optionally disabled) fallback functionality to
look up old SHA-1 hash names based on the new hash.
> We extend the textual object name syntax to explicitly name the hash
> used. Every program that invokes git or speaks git protocols will
> need to understand the extended object name syntax.
>
> Packfiles need to be extended to be able to contain objects named by
> new hash functions. Blob objects with identical contents but named by
> different hash functions would ideally share storage.
>
> Safety catches preferent accidental incorporation into a project of
> incompatibly-new objects, or additional deprecatedly-old objects.
> This allows for incremental deployment.
We have a compatibility mechanism already in place: if the
repositoryFormatVersion option is set to 1, but an unknown extension
flag is set, Git will bail out.
For network protocols, we have the server offer a hash=foo extension,
and make the client echo it back, and either bail or convert on the fly.
This makes it fast for new clients, and slow for old clients, which
encourages migration.
We could also store old-style packs for easy fetch by clients.
> TEXTUAL SYNTAX
> ==============
>
> The object name textual syntax is extended. The new syntax may be
> used in all textual git objects and protocols (commits, tags, command
> lines, etc.).
>
> We declare that the object name syntax is henceforth
> [A-Z]+[0-9a-z]+ | [0-9a-f]+
> and that names [A-Z].* are deprecated as ref name components.
I'd simply say that we have data always be in the new format if it's
available, and tag the old SHA-1 versions instead. Otherwise, as Peff
pointed out, we're going to be stuck typing a bunch of identical stuff
every time. Again, this encourages migration.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] revision: exclude trees/blobs given commit
From: Junio C Hamano @ 2017-02-28 21:44 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff, peartben, benpeart
In-Reply-To: <7082d91f30663b2e6d7fb1795c5ea37d3fe3446c.1487984670.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> When the --objects argument is given to rev-list, an argument of the
> form "^$tree" can be given to exclude all blobs and trees reachable from
> that tree, but an argument of the form "^$commit" only excludes that
> commit, not any blob or tree reachable from it. Make "^$commit" behave
> consistent to "^$tree".
>
> Also, formalize this behavior in unit tests. (Some of the added tests
> would already pass even before this commit, but are included
> nevertheless for completeness.)
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> revision.c | 2 ++
> t/t6000-rev-list-misc.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 90 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 5e49d9e0e..e6a62da98 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -254,6 +254,8 @@ static struct commit *handle_commit(struct rev_info *revs,
> die("unable to parse commit %s", name);
> if (flags & UNINTERESTING) {
> mark_parents_uninteresting(commit);
> + if (revs->tree_and_blob_objects)
> + mark_tree_uninteresting(commit->tree);
I fear that this may end up to be quite expensive. Can we have a
perf test?
^ permalink raw reply
* Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info
From: Junio C Hamano @ 2017-02-28 21:42 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff, peartben, benpeart
In-Reply-To: <06a84f8c77924b275606384ead8bb2fd7d75f7b6.1487984670.git.jonathantanmy@google.com>
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.
Thanks.
^ permalink raw reply
* Re: Typesafer git hash patch
From: Jeff King @ 2017-02-28 20:37 UTC (permalink / raw)
To: brian m. carlson, Linus Torvalds, Junio C Hamano,
Git Mailing List
In-Reply-To: <20170228203312.jc7gia7f44goqjmj@genre.crustytoothpaste.net>
On Tue, Feb 28, 2017 at 08:33:13PM +0000, brian m. carlson wrote:
> On Tue, Feb 28, 2017 at 03:26:34PM -0500, Jeff King wrote:
> > Yeah, a lot of brian's patches have been focused around the fixing the
> > related size assumptions. We've got GIT_SHA1_HEXSZ which doesn't solve
> > the problem, but at least makes it easy to find. And a big improvement
> > in the most recent series is a parse_oid() function that lets you parse
> > object-ids left-to-right without knowing the size up front. So things
> > like:
> >
> > if (len > 82 &&
> > !get_sha1_hex(buf, sha1_a) &&
> > get_sha1_hex(buf + 41, sha1_b))
> >
> > becomes more like:
> >
> > if (parse_oid(p, oid_a, &p) && *p++ == ' ' &&
> > parse_oid(p, oid_b, &p) && *p++ == '\n')
>
> What I could do instead of using GIT_SHA1_HEXSZ is use GIT_MAX_HEXSZ for
> things that are about allocating enough memory and create a global (or
> function) for things that only care about what the current hash size is.
> That might be a desirable approach. If other people agree, I can make a
> patch to do that.
I was going to say "don't worry about it, and focus on converting to
constants at all for now". But I guess while you are doing that, it does
not hurt to split the MAX_HEXSZ cases out. It will save work in sorting
them later.
-Peff
^ permalink raw reply
* Re: [PATCH] strbuf: add strbuf_add_real_path()
From: Brandon Williams @ 2017-02-28 20:42 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Jeff King
In-Reply-To: <3063b6fb-02aa-703c-0b56-300109cf054d@web.de>
On 02/27, René Scharfe wrote:
> Am 27.02.2017 um 19:22 schrieb Brandon Williams:
> >On 02/25, René Scharfe wrote:
> >>+void strbuf_add_real_path(struct strbuf *sb, const char *path)
> >>+{
> >>+ if (sb->len) {
> >>+ struct strbuf resolved = STRBUF_INIT;
> >>+ strbuf_realpath(&resolved, path, 1);
> >>+ strbuf_addbuf(sb, &resolved);
> >>+ strbuf_release(&resolved);
> >>+ } else
> >>+ strbuf_realpath(sb, path, 1);
> >
> >I know its not required but I would have braces on the 'else' branch
> >since they were needed on the 'if' branch. But that's up to you and
> >your style :)
>
> Personally I'd actually prefer them as well, but the project's style
> has traditionally been to avoid braces on such trailing single-line
> branches to save lines. The CodingGuidelines for this topic have
> been clarified recently, though, and seem to require them now.
> Interesting.
>
> René
Having the project's guidelines align with your own preference makes
things a bit easier!
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 3/5] grep: fix bug when recuring with relative pathspec
From: Junio C Hamano @ 2017-02-28 21:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <20170224235100.52627-4-bmwill@google.com>
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?
> 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 &&
^ permalink raw reply
* Re: 'git submodules update' ignores credential.helper config of the parent repository
From: Jeff King @ 2017-02-28 20:32 UTC (permalink / raw)
To: Stefan Beller; +Cc: Dmitry Neverov, Duy Nguyen, Junio C Hamano, Git List
In-Reply-To: <CAGZ79kZ8ANzjauzJAbPh7m7zYoBrB=ZjgDXHxNb57_H=RYm8cQ@mail.gmail.com>
On Tue, Feb 28, 2017 at 12:21:57PM -0800, Stefan Beller wrote:
> > I'm still open to the idea that we simply improve the documentation to
> > make it clear that per-repo config really is per-repo, and is not shared
> > between super-projects and submodules. And then something like Duy's
> > proposed conditional config lets you set global config that flexibly
> > covers a set of repos.
>
> How would the workflow for that look like?
> My naive thought on that is:
>
> (1) $EDIT .git/config_to_be_included
> (2) $ git config add-config-inclusion .git/config_to_be_included
> (3) $ git submodule foreach git config add-inclusion-config
> .git/config_to_be_included
>
> which sounds a bit cumbersome to me.
> So I guess we'd want some parts of that as part of another command, e.g.
> (3) could be part of (2).
I think it would be more like:
(1) $EDIT ~/.gitconfig-super
(2) git config --global \
includeIf.gitdir:/path/to/super.path .gitconfig-super
I know that is probably a bit more cumbersome to figure out than
treating the super/sub relationship in a special way. But I suspect for
a lot of cases that it actually ends up even better, because the
situation is more like:
(1) $EDIT ~/.gitconfig-work
(2) git config --global includeIf.gitdir:~/work.path .gitconfig-work
and then it covers all of your projects in ~/work, whether they are
super-projects, submodules, or regular repos.
> I am also open and willing to document this better; but were would
> we want to put documentation? Obviously we would not want to put it
> alongside each potentially useful config option to be inherited to
> submodules. (that would imply repeating ourselves quite a lot in
> the config man page).
>
> I guess putting it into "man gitmodules" that I was writing tentatively
> would make sense.
Yeah, I think it is worth mentioning in "gitmodules", and probably in
git-config where we define per-repo config.
It may also be worth calling it out especially for url.insteadOf, just
because it is not clear there when the URL rewriting happens (it's not
insane to think that it happens in the super-project, that just doesn't
happen to be how it's implemented).
-Peff
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Jeff King @ 2017-02-28 21:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, René Scharfe, git
In-Reply-To: <alpine.DEB.2.20.1702282149160.3767@virtualbox>
On Tue, Feb 28, 2017 at 09:54:58PM +0100, Johannes Schindelin wrote:
> > Right now, they may be able to have future timestamps ranging to
> > year 2100 and switching to time_t would limit their ability to
> > express future time to 2038 but they would be able to express
> > timestamp in the past to cover most of 20th century. Given that
> > these 32-bit time_t software platforms will die off before year 2038
> > (either by underlying hardware getting obsolete, or software updated
> > to handle 64-bit time_t), the (temporary) loss of 2038-2100 range
> > would not be too big a deal to warrant additional complexity.
>
> You seem to assume that time_t is required to be signed. But from my
> understanding that is only guaranteed by POSIX, not by ISO C.
I wonder how common that is in practice, and whether it is worth
treating it as a quality-of-implementation issue. IOW, to say "your
platform time_t doesn't handle negative times, so you get Jan 1 1970 for
any dates before then. Complain to your platform vendor".
I'm not sure how much complexity it would add to the code. Either way,
when we parse an ascii-decimal timestamp from an object, we need to do
bounds checking. Whether that bound is at "0" or "LONG_MIN", I don't
think that it changes much.
Meanwhile, if we were to have a negative timestamp_t but the system
time_t is unsigned, we have to do a bounds-check any time we use a
system function like gmtime(), or risk funny wrap-around bugs.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/2] Documentation: Link descriptions of -z to core.quotePath
From: Andreas Heiduk @ 2017-02-28 21:30 UTC (permalink / raw)
To: Jakub Narębski, Junio C Hamano; +Cc: git
In-Reply-To: <f534b9c7-c8a8-1a05-5fcb-020122dccaac@gmail.com>
Am 24.02.2017 um 22:54 schrieb Jakub Narębski:
> W dniu 24.02.2017 o 21:37, Andreas Heiduk pisze:
>> 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".
>
> This patch I am not sure about. On one hand it improves consistency
> (and makes information more complete), on the other hand it removes
> information at hand and instead refers to other manpage.
Indeed this is a trade-off. My intention was to establish some kind of
well-known term by citing "core.quotePath" everywhere. So after a while
"quoting" and "core.quotePath" should be known by everyone interested.
> Perhaps a better solution would be to craft a short description that
> is both sufficiently complete, and refers to "core.quotePath" for
> more details, and then transclude it with "include::quotepath.txt[]".
On the other hand this makes every section quite long. Personally, I
find long and repetitive documentation harder to comprehend.
>>
>> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
>> ---
>> 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 +++----
>> 8 files changed, 35 insertions(+), 29 deletions(-)
>>
>> 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 e6215c3..7c28e73 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 munged 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..a7a001b 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 munged 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
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