* Re: [PATCH v6 1/1] config: add conditional include
From: Philip Oakley @ 2017-02-26 12:27 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano, Jeff King, Sebastian Schuberth,
Matthieu Moy
In-Reply-To: <CACsJy8Bq-P8Aw+f0omndCmQU54n_p7ZwDVDK9APuH_3vXDac2g@mail.gmail.com>
From: "Duy Nguyen" <pclouds@gmail.com>
> On Sat, Feb 25, 2017 at 5:08 AM, Philip Oakley <philipoakley@iee.org>
> wrote:
>>> +Conditional includes
>>> +~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +You can include one config file from another conditionally by setting
>>
>>
>> On first reading I thought this implied you can only have one `includeIf`
>> within the config file.
>> I think it is meant to mean that each `includeIf`could include one other
>> file, and that users can have multiple `includeIf` lines.
>
> Yes. Not sure how to put it better though (I basically copied the
> first paragraph from the unconditional include section above, which
> shares the same confusion). Perhaps just write "the variable can be
> specified multiple times"? Or "multiple variables include multiple
> times, the last variable does not override the previous ones"?
> --
My attempt, based on updating the `Includes` section would be something
like:
`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.`
The subtle change was to s/one/a/ at the start, and then add the final short
sentence that states that the section's variables can have multiple key
values.
I copied the 'multiple key values' phrase from the man page intro for
consitency, though 'multivalued' could just as easily be used as it is the
term used by the 'Configuration File' section that this is part of
https://git-scm.com/docs/git-config#_configuration_file.
Even shorter may be:
`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
(can be multivalued) takes a pathname as its value, and is subject to tilde
expansion.`
The Conditional Includes would follow suit.
Philip
^ permalink raw reply
* Reference for quote "creating branch is not the issue, merging is", in context of Subversion/Git
From: Michael Hüttermann @ 2017-02-26 11:40 UTC (permalink / raw)
To: git
Hello team,
Linus Torvalds made a statement regarding merging/branching and stated (as far as I know) that "creating branch is not the issue, merge is", in context of Subversion/Git.
I do not find the origin source for that. Can you please help and point me to a statement or article where Linus elaborated on this?
Thanks for your help.
Kind regards
Michael
^ permalink raw reply
* Unconventional roles of git
From: ankostis @ 2017-02-26 11:51 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jason Cooper
On 26 February 2017 at 02:13, Jason Cooper <git@lakedaemon.net> wrote:
> As someone looking to deploy (and having previously deployed) git in
> unconventional roles, I'd like to add ...
We are developing a distributed storage for type approval files regarding all
vehicles registered in Europe.[1] To ensure integrity even after 10 or 30
years, the hash of a commit of these files (as contained in a tag) are
to be printed on the the paper certificates.
- Can you provide some hints for other similar unconventional roles of git?
- Any other comment on the above usage of git are welcomed.
Kind Regards,
Kostis Anagnostopoulos
[1] https://co2mpas.io
^ permalink raw reply
* Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec
From: Duy Nguyen @ 2017-02-26 9:53 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller
In-Reply-To: <20170224235100.52627-2-bmwill@google.com>
On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams <bmwill@google.com> wrote:
> When using the --recurse-submodules flag with a relative pathspec which
> includes "..", an error is produced inside the child process spawned for
> a submodule. When creating the pathspec struct in the child, the ".."
> is interpreted to mean "go up a directory" which causes an error stating
> that the path ".." is outside of the repository.
>
> While it is true that ".." is outside the scope of the submodule, it is
> confusing to a user who originally invoked the command where ".." was
> indeed still inside the scope of the superproject. Since the child
> process luanched for the submodule has some context that it is operating
s/luanched/launched/
I would prefer 1/5 t to be merged with 3/5 though. The problem
description is very light there, and the test demonstration in the
diff is simply switching from failure to success, which forces the
reader to come back here. It's easier to find here now, but it'll be a
bit harder when it enters master and we have to read it from git-log,
I think.
I'm still munching through the super-prefix patches. From how you
changed match_pathspec call in 0281e487fd (grep: optionally recurse
into submodules - 2016-12-16), I guess pathspecs should be handled
with super-prefix instead of the submodule's prefix (which is empty
anyway, I guess). The right solution wrt. handling relative paths may
be teach pathspec about super-prefix (and even original super's cwd)
then let it processes path in supermodule's context.
Does it handle relative paths with wildcards correctly btw? Ones that
cross submodules? I have a feeling it doesn't, but I haven't seen how
exactly super-prefix works yet.
There's another problem with passing pathspec from one process to
another. The issue with preserving the prefix, see 233c3e6c59
(parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN -
2013-07-14). :(icase) needs this because given a path
"<prefix>/foobar", only the "foobar" part is considered case
insensitive, the prefix part is always case-sensitive. For example, if
you have 4 paths "abc/def", "abc/DEF", "ABC/def" and "ABC/DEF" and are
standing at "abc", you would want ":(icase)def" to match the first two
only, not all of them.
> underneath a superproject, this error could be avoided.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> index 67247a01d..418ba68fe 100755
> --- a/t/t7814-grep-recurse-submodules.sh
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '
> test_cmp expect actual
> '
>
> +test_expect_failure 'grep using relative path' '
> + test_when_finished "rm -rf parent sub" &&
> + git init sub &&
> + echo "foobar" >sub/file &&
> + git -C sub add file &&
> + git -C sub commit -m "add file" &&
> +
> + git init parent &&
> + echo "foobar" >parent/file &&
> + git -C parent add file &&
> + mkdir parent/src &&
> + echo "foobar" >parent/src/file2 &&
> + git -C parent add src/file2 &&
> + git -C parent submodule add ../sub &&
> + git -C parent commit -m "add files and submodule" &&
> +
> + # From top works
> + cat >expect <<-\EOF &&
> + file:foobar
> + src/file2:foobar
> + sub/file:foobar
> + EOF
> + git -C parent grep --recurse-submodules -e "foobar" >actual &&
> + test_cmp expect actual &&
> +
> + # Relative path to top errors out
After 3/5, it's not "errors out" any more, is it?
> + cat >expect <<-\EOF &&
> + ../file:foobar
> + file2:foobar
> + ../sub/file:foobar
> + EOF
> + git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
> + test_cmp expect actual &&
> +
> + # Relative path to submodule errors out
ditto
> + cat >expect <<-\EOF &&
> + ../sub/file:foobar
> + EOF
> + git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
> + test_cmp expect actual
> +'
> +
> test_incompatible_with_recurse_submodules ()
> {
> test_expect_success "--recurse-submodules and $1 are incompatible" "
> --
> 2.11.0.483.g087da7b7c-goog
>
--
Duy
^ permalink raw reply
* Re: [PATCH v6 1/1] config: add conditional include
From: Jeff King @ 2017-02-26 6:12 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, sschuberth, Matthieu Moy
In-Reply-To: <20170224131425.32409-4-pclouds@gmail.com>
On Fri, Feb 24, 2017 at 08:14:25PM +0700, Nguyễn Thái Ngọc Duy wrote:
> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> + /* no condition (i.e., "include.path") is always true */
> + if (!cond)
> + return 1;
> +
> + if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
> + return include_by_gitdir(cond, cond_len, 0);
> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> + return include_by_gitdir(cond, cond_len, 1);
> +
> + error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
> + /* unknown conditionals are always false */
> + return 0;
> +}
This "error()" is a nice warning to people who have misspelled the
condition (or are surprised that an old version does not respect their
condition). But it seems out of place with the rest of the compatibility
strategy, which is to quietly ignore include types we don't understand.
For example, if your patch ships in v2.13 and we add the "foo:"
condition in v2.14, then with config like:
[includeif "foo:bar"]
path = whatever
you get:
v2.12: silently ignore
v2.13: loudly ignore
v2.14: works
which seems odd.
If we do keep it, it should probably be warning(). I would expect
"error:" to indicate that some requested operation could not be
completed, but this is just "by the way, I ignored this garbage".
-Peff
PS I know we try to avoid dashes in the section names, but "includeIf"
and "includeif" just look really ugly to me. Maybe it is just me,
though.
^ permalink raw reply
* Re: [PATCH v5 1/1] config: add conditional include
From: Jeff King @ 2017-02-26 6:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Git Mailing List, Sebastian Schuberth, Matthieu Moy
In-Reply-To: <xmqq1sun5vo6.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 24, 2017 at 09:46:17AM -0800, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
> >>> + if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
> >>> + return include_by_gitdir(cond, cond_len, 0);
> >>> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> >>> + return include_by_gitdir(cond, cond_len, 1);
> >>
> >> This may be OK for now, but it should be trivial to start from a
> >> table with two entries, i.e.
> >>
> >> struct include_cond {
> >> const char *keyword;
> >> int (*fn)(const char *, size_t);
> >> };
> >>
> >> and will show a better way to do things to those who follow your
> >> footsteps.
> >
> > Yeah I don't see a third include coming soon and did not go with that.
> > Let's way for it and refactor then.
>
> I would have said exactly that in my message if you already had
> include_by_gitdir() and include_by_gitdir_i() as separate functions.
>
> But I didn't, because the above code gives an excuse to the person
> who adds the third one to be lazy and add another "else if" for
> expediency, because making it table-driven would require an extra
> work to add two wrapper functions that do not have anything to do
> with the third one being added.
I don't think driving that with a two-entry table is the right thing
here. We are as likely to add another "foobar:" entry as we are to add
another modifier "/i" modifier to "gitdir:", and it is unclear whether
that modifier would be mutually exclusive with "/i".
E.g., imagine we add "/re" for a regex, but allow a case-insensitive
regex with an "i", giving something like "gitdir/i/re:". Now you would
want to drive it by matching "gitdir" first, and then collecting the
"/i/re" independently, to avoid an explosion of matches.
Driving that with a table is much more complex. I'd just as soon keep
things as simple as possible for now and worry about flagging it in
review when something new gets added.
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-26 5:18 UTC (permalink / raw)
To: Jason Cooper
Cc: Junio C Hamano, Linus Torvalds, Ian Jackson, Joey Hess,
Git Mailing List
In-Reply-To: <20170226011359.GI11350@io.lakedaemon.net>
On Sun, Feb 26, 2017 at 01:13:59AM +0000, Jason Cooper wrote:
> On Fri, Feb 24, 2017 at 10:10:01PM -0800, Junio C Hamano wrote:
> > I was thinking we would need mixed mode support for smoother
> > transition, but it now seems to me that the approach to stratify the
> > history into old and new is workable.
>
> As someone looking to deploy (and having previously deployed) git in
> unconventional roles, I'd like to add one caveat. The flag day in the
> history is great, but I'd like to be able to confirm the integrity of
> the old history.
>
> "Counter-hashing" the blobs is easy enough, but the trees, commits and
> tags would need to have, iiuc, some sort of cross-reference. As in my
> previous example, "git tag -v v3.16" also checks the counter hash to
> further verify the integrity of the history (yes, it *really* needs to
> check all of the old hashes, but I'd like to make sure I can do step one
> first).
>
> Would there be opposition to counter-hashing the old commits at the flag
> day?
I don't think a counter-hash needs to be embedded into the git objects
themselves. If the "modern" repo format stores everything primarily as
sha-256, say, it will probably need to maintain a (local) mapping table
of sha1/sha256 equivalence. That table can be generated at any time from
the object data (though I suspect we'll keep it up to date as objects
enter the repository).
At the flag day[1], you can make a signed tag with the "correct" mapping
in the tag body (so part of the actual GPG signed data, not referenced
by sha1). Then later you can compare that mapping to the object content
in the repo (or to the local copy of the mapping based on that data).
-Peff
[1] You don't even need to wait until the flag day. You can do it now.
This is conceptually similar to the git-evtag tool, though it just
signs the blob contents of the tag's current tree state. Signing the
whole mapping lets you verify the entirety of history, but of course
that mapping is quite big: 20 + 32 bytes per object for
sha1/sha-256, which is ~250MB for the kernel. So you'd probably not
want to do it more than once.
^ permalink raw reply
* Re: [PATCH v6 1/1] config: add conditional include
From: Duy Nguyen @ 2017-02-26 3:02 UTC (permalink / raw)
To: Philip Oakley
Cc: Git Mailing List, Junio C Hamano, Jeff King, Sebastian Schuberth,
Matthieu Moy
In-Reply-To: <29A09E4EDB1F4F4D9E77E67A7A8A33FF@PhilipOakley>
On Sat, Feb 25, 2017 at 5:08 AM, Philip Oakley <philipoakley@iee.org> wrote:
>> +Conditional includes
>> +~~~~~~~~~~~~~~~~~~~~
>> +
>> +You can include one config file from another conditionally by setting
>
>
> On first reading I thought this implied you can only have one `includeIf`
> within the config file.
> I think it is meant to mean that each `includeIf`could include one other
> file, and that users can have multiple `includeIf` lines.
Yes. Not sure how to put it better though (I basically copied the
first paragraph from the unconditional include section above, which
shares the same confusion). Perhaps just write "the variable can be
specified multiple times"? Or "multiple variables include multiple
times, the last variable does not override the previous ones"?
--
Duy
^ permalink raw reply
* Re: [PATCH] travis-ci: run scan-build every time
From: Samuel Lijin @ 2017-02-26 2:09 UTC (permalink / raw)
To: Lars Schneider; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <BAB1EE0E-B258-4108-AE24-110172086DE4@gmail.com>
On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
>>
>> It's worth noting that there seems to be a weird issue with scan-build
>> where it *will* generate a report for something locally, but won't do it
>> on Travis. See [2] for an example where I have a C program with a
>> very obvious memory leak but scan-build on Travis doesn't generate
>> a report (despite complaining about it in stdout), even though it does
>> on my local machine.
>>
>> [1] https://travis-ci.org/sxlijin/git/builds/204853233
>> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342
>
> Scan-build stores the report in some temp folder. I assume you can't access
> this folder on TravisCI. Try the scan-build option "-o scan-build-results"
> to store the report in the local directory.
That occurred to me, but I don't quite think that's the issue. I just
noticed that on the repo I use to test build matrices, jobs 1-8 don't
generate a report, but 9-14 and 19-20 do [1]. I don't think it's an
issue with write permissions (scan-build complains much more vocally
if that happens), but it doesn't seem to matter if the output dir is
in the tmpfs [2] or a local directory [3].
[1] https://travis-ci.org/sxlijin/travis-testing/builds/205054253
[2] https://travis-ci.org/sxlijin/git/jobs/205028920#L1000
[2] https://travis-ci.org/sxlijin/git/jobs/205411705#L998
>> @@ -78,9 +79,8 @@ before_install:
>> brew update --quiet
>> # Uncomment this if you want to run perf tests:
>> # brew install gnu-time
>> - brew install git-lfs gettext
>> - brew link --force gettext
>> - brew install caskroom/cask/perforce
>> + brew install git-lfs gettext caskroom/cask/perforce llvm
>> + brew link --force gettext llvm
>
> This wouldn't be necessary if we only scan on Linux.
Agreed. I'm not sure if macOS static analysis would bring any specific
benefits; I don't really have much experience with static analysis
tools one way or another, so I'm happy to defer on this decision.
>> -script: make --quiet test
>> +script: scan-build make --quiet test
>
> Why do you want to scan the tests?
Brain fart on my end.
> Cheers,
> Lars
^ permalink raw reply
* Re: SHA1 collisions found
From: Jason Cooper @ 2017-02-26 1:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Jeff King, Ian Jackson, Joey Hess,
Git Mailing List
In-Reply-To: <xmqqinnyztqe.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, Feb 24, 2017 at 10:10:01PM -0800, Junio C Hamano wrote:
> I was thinking we would need mixed mode support for smoother
> transition, but it now seems to me that the approach to stratify the
> history into old and new is workable.
As someone looking to deploy (and having previously deployed) git in
unconventional roles, I'd like to add one caveat. The flag day in the
history is great, but I'd like to be able to confirm the integrity of
the old history.
"Counter-hashing" the blobs is easy enough, but the trees, commits and
tags would need to have, iiuc, some sort of cross-reference. As in my
previous example, "git tag -v v3.16" also checks the counter hash to
further verify the integrity of the history (yes, it *really* needs to
check all of the old hashes, but I'd like to make sure I can do step one
first).
Would there be opposition to counter-hashing the old commits at the flag
day?
thx,
Jason.
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-26 0:46 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jakub Narębski, Joey Hess, git
In-Reply-To: <D74A82FF-BF00-481F-9B2A-4AF8EF3D062F@gmail.com>
On Sat, Feb 25, 2017 at 11:35:27PM +0100, Lars Schneider wrote:
> > So we don't actually know how Git would behave in the face of a SHA-1
> > collision. It would be pretty easy to simulate it with something like:
> [...]
>
> That's a good idea! I wonder if it would make sense to setup an
> additional job in TravisCI that patches every Git version with some hash
> collisions and then runs special tests. This way we could ensure Git
> behaves reasonable in case of a collision. E.g. by printing errors and
> not crashing or corrupting the repo. Do you think that would be worth
> the effort?
I think it would be interesting to see the results under various
scenarios. I don't know that it would be all that interesting from an
ongoing CI perspective. But we wouldn't know until somebody actually
writes the tests and we see what they do.
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Jason Cooper @ 2017-02-26 0:16 UTC (permalink / raw)
To: ankostis
Cc: Junio C Hamano, git, Stefan Beller, David Lang, Ian Jackson,
Joey Hess
In-Reply-To: <CA+dhYEVwLGNZh-hbcJm+kMR4W45VbwvSVY+7YKt0V9jg_b_M4g@mail.gmail.com>
Hi,
On Sat, Feb 25, 2017 at 01:31:32AM +0100, ankostis wrote:
> That is why I believe that some HASH (e.g. SHA-3) must be the blessed one.
> All git >= 3.x.x must support at least this one (for naming and
> cross-referencing between objects).
I would stress caution here. SHA3 has survived the NIST competition,
but that's about it. It has *not* received nearly as much scrutiny as
SHA2.
SHA2 is a similar construction to SHA1 (Merkle–Damgård [1]) so it makes
sense to be leery of it, but I would argue it's seasoning merits serious
consideration.
Ideally, bless SHA2-384 (minimum) as the next hash. Five or so years
down the road, if SHA3 is still in good standing, bless it as the next
hash.
thx,
Jason.
[1]
https://en.wikipedia.org/wiki/Merkle%E2%80%93Damg%C3%A5rd_construction
^ permalink raw reply
* Re: SHA1 collisions found
From: ankostis @ 2017-02-25 23:22 UTC (permalink / raw)
To: Jason Cooper; +Cc: Ian Jackson, Joey Hess, git
In-Reply-To: <20170224172335.GG11350@io.lakedaemon.net>
On 24 February 2017 at 18:23, Jason Cooper <git@lakedaemon.net> wrote:
> Hi Ian,
>
> On Fri, Feb 24, 2017 at 03:13:37PM +0000, Ian Jackson wrote:
>> Joey Hess writes ("SHA1 collisions found"):
>> > https://shattered.io/static/shattered.pdf
>> > https://freedom-to-tinker.com/2017/02/23/rip-sha-1/
>> >
>> > IIRC someone has been working on parameterizing git's SHA1 assumptions
>> > so a repository could eventually use a more secure hash. How far has
>> > that gotten? There are still many "40" constants in git.git HEAD.
>>
>> I have been thinking about how to do a transition from SHA1 to another
>> hash function.
>>
>> I have concluded that:
>>
>> * We can should avoid expecting everyone to rewrite all their
>> history.
>
> Agreed.
>
>> * Unfortunately, because the data formats (particularly, the commit
>> header) are not in practice extensible (because of the way existing
>> code parses them), it is not useful to try generate new data (new
>> commits etc.) containing both new hashes and old hashes: old
>> clients will mishandle the new data.
>
> My thought here is:
>
> a) re-hash blobs with sha256, hardlink to sha1 objects
> b) create new tree objects which are mirrors of each sha1 tree object,
> but purely sha256
> c) mirror commits, but they are also purely sha256
> d) future PGP signed tags would sign both hashes (or include both?)
IMHO that is a great idea that needs more attention.
You get to keep 2 or more hash-functions for extra security in a PQ world.
And to keep sketches for the future so far,
SHA-3 must be always one of the new hashes.
Actually, you can get rid of SHA-1 completely, and land on Linus's
current sketches for the way ahead.
Thanks,
Kostis
>
> Which would end up something like:
>
> .git/
> \... #usual files
> \objects
> \ef
> \3c39f7522dc55a24f64da9febcfac71e984366
> \objects-sha2_256
> \72
> \604fd2de5f25c89d692b01081af93bcf00d2af34549d8d1bdeb68bc048932
> \info
> \...
> \info-sha2_256
> \refs #uses sha256 commit identifiers
>
> Basically, keep the sha256 stuff out of the way for legacy clients, and
> new clients will still be able to use it.
>
> There shouldn't be a need to re-sign old signed tags if the underlying
> objects are counter-hashed. There might need to be some transition
> info, though.
>
> Say a new client does 'git tag -v tags/v3.16' in the kernel tree. I would
> expect it to check the sha1 hashes, verify the PGP signed tag, and then
> also check the sha256 counter-hashes of the relevant objects.
>
> thx,
>
> Jason.
^ permalink raw reply
* Re: [PATCH] travis-ci: run scan-build every time
From: Lars Schneider @ 2017-02-25 23:02 UTC (permalink / raw)
To: Jeff King; +Cc: Samuel Lijin, git@vger.kernel.org
In-Reply-To: <20170225223146.ixubnwqkfol5q2gn@sigill.intra.peff.net>
> On 25 Feb 2017, at 23:31, Jeff King <peff@peff.net> wrote:
>
> On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote:
>
>>
>>> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
>>>
>>> Introduces the scan-build static code analysis tool from the Clang
>>> project to all Travis CI builds. Installs clang (since scan-build
>>> needs clang as a dependency) to make this possible (on macOS, also
>>> updates PATH to allow scan-build to be invoked without referencing the
>>> full path).
>>
>> This is a pretty neat idea. However, I think this should become a
>> dedicated job in a TravisCI build (similar to the Documentation job [1])
>> because:
>> a) We don't want to build and test a scan-build version of Git (AFAIK
>> scan-build kind of proxies the compiler to do its job - I don't if
>> this has any side effects)
>> b) We don't want to slow down the other builds
>> c) It should be enough to run scan-build once on Linux per build
>
> Yeah. I am all for static analysis, but I agree it should be its own
> job. Especially as it can be quite noisy with false positives (and I
> really think before any static analysis is useful we need to figure out
> a way to suppress the false positives, so that we can see the signal in
> the noise).
>
> Fully a third of the problem cases found are dead assignments or
> increments. I looked at a few, and I think the right strategy is to tell
> the tool "no really, our code is fine". For instance, it complains
> about:
>
> argc = parse_options(argc, argv, ...);
>
> when argc is not used again later. Sure, that assignment is doing
> nothing. But from a maintainability perspective, I'd much rather have a
> dead assignment (that the compiler is free to remove) then for somebody
> to later add a loop like:
>
> for (i = 0; i < argc; i++)
> something(argv[i]);
>
> which will read past the end of the rearranged argv (and probably
> _wouldn't_ be caught by static analysis, because the hidden dependency
> between argc and argv is buried inside the parse_options() call).
>
> So there is definitely some bug-fixing to be done, but I think there is
> also some work in figuring out how to suppress these useless reports.
That makes sense. I suspected that this assignment was intentional
but I wasn't sure why. I didn't know about the rearrangement of argv.
Apparently an "(void)argc;" silences this warning. Would that be too
ugly to bear? :-)
> Turning off the dead-assignment checker is one option, but I actually
> think it _could_ produce useful results. It just isn't in these cases.
> So I'd much rather if we can somehow suppress the specific callsites.
>
>> I ran scan-build on the current master and it detected 72 potential bugs [2].
>> I looked through a few of them and they seem to be legitimate. If the list agrees
>> that running scan-build is a useful thing and that these problems should be fixed
>> then we could:
>>
>> (1) Add scan-build check to Travis CI but only print errors as warning
>> (2) Fix the 72 existing bugs over time
>> (3) Turn scan-build warnings into errors
>
> If they are warnings socked away in a Travis CI job that nobody looks
> out, then I doubt anybody is going to bother fixing them.
>
> Not that step (1) hurts necessarily, but I don't think it's really doing
> anything until step (2) is finished.
Agreed.
- Lars
^ permalink raw reply
* Re: SHA1 collisions found
From: Lars Schneider @ 2017-02-25 22:35 UTC (permalink / raw)
To: Jeff King; +Cc: Jakub Narębski, Joey Hess, git
In-Reply-To: <20170224230604.nt37uw5y3uehukfd@sigill.intra.peff.net>
> On 25 Feb 2017, at 00:06, Jeff King <peff@peff.net> wrote:
>
> On Fri, Feb 24, 2017 at 11:47:46PM +0100, Jakub Narębski wrote:
>
>> I have just read on ArsTechnica[1] that while Git repository could be
>> corrupted (though this would require attackers to spend great amount
>> of resources creating their own collision, while as said elsewhere
>> in this thread allegedly easy to detect), putting two proof-of-concept
>> different PDFs with same size and SHA-1 actually *breaks* Subversion.
>> Repository can become corrupt, and stop accepting new commits.
>>
>> From what I understand people tried this, and Git doesn't exhibit
>> such problem. I wonder what assumptions SVN made that were broken...
>
> To be clear, nobody has generated a sha1 collision in Git yet, and you
> cannot blindly use the shattered PDFs to do so. Git's notion of the
> SHA-1 of an object include the header, so somebody would have to do a
> shattered-level collision search for something that starts with the
> correct "blob 1234\0" header.
>
> So we don't actually know how Git would behave in the face of a SHA-1
> collision. It would be pretty easy to simulate it with something like:
>
> ---
> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
> index 22b125cf8..1be5b5ba3 100644
> --- a/block-sha1/sha1.c
> +++ b/block-sha1/sha1.c
> @@ -231,6 +231,16 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)
> memcpy(ctx->W, data, len);
> }
>
> +/* sha1 of blobs containing "foo\n" and "bar\n" */
> +static const unsigned char foo_sha1[] = {
> + 0x25, 0x7c, 0xc5, 0x64, 0x2c, 0xb1, 0xa0, 0x54, 0xf0, 0x8c,
> + 0xc8, 0x3f, 0x2d, 0x94, 0x3e, 0x56, 0xfd, 0x3e, 0xbe, 0x99
> +};
> +static const unsigned char bar_sha1[] = {
> + 0x57, 0x16, 0xca, 0x59, 0x87, 0xcb, 0xf9, 0x7d, 0x6b, 0xb5,
> + 0x49, 0x20, 0xbe, 0xa6, 0xad, 0xde, 0x24, 0x2d, 0x87, 0xe6
> +};
> +
> void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx)
> {
> static const unsigned char pad[64] = { 0x80 };
> @@ -248,4 +258,8 @@ void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx)
> /* Output hash */
> for (i = 0; i < 5; i++)
> put_be32(hashout + i * 4, ctx->H[i]);
> +
> + /* pretend "foo" and "bar" collide */
> + if (!memcmp(hashout, bar_sha1, 20))
> + memcpy(hashout, foo_sha1, 20);
> }
That's a good idea! I wonder if it would make sense to setup an
additional job in TravisCI that patches every Git version with some hash
collisions and then runs special tests. This way we could ensure Git
behaves reasonable in case of a collision. E.g. by printing errors and
not crashing or corrupting the repo. Do you think that would be worth
the effort?
- Lars
^ permalink raw reply
* Re: [PATCH] travis-ci: run scan-build every time
From: Jeff King @ 2017-02-25 22:31 UTC (permalink / raw)
To: Lars Schneider; +Cc: Samuel Lijin, git@vger.kernel.org
In-Reply-To: <BAB1EE0E-B258-4108-AE24-110172086DE4@gmail.com>
On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote:
>
> > On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
> >
> > Introduces the scan-build static code analysis tool from the Clang
> > project to all Travis CI builds. Installs clang (since scan-build
> > needs clang as a dependency) to make this possible (on macOS, also
> > updates PATH to allow scan-build to be invoked without referencing the
> > full path).
>
> This is a pretty neat idea. However, I think this should become a
> dedicated job in a TravisCI build (similar to the Documentation job [1])
> because:
> a) We don't want to build and test a scan-build version of Git (AFAIK
> scan-build kind of proxies the compiler to do its job - I don't if
> this has any side effects)
> b) We don't want to slow down the other builds
> c) It should be enough to run scan-build once on Linux per build
Yeah. I am all for static analysis, but I agree it should be its own
job. Especially as it can be quite noisy with false positives (and I
really think before any static analysis is useful we need to figure out
a way to suppress the false positives, so that we can see the signal in
the noise).
Fully a third of the problem cases found are dead assignments or
increments. I looked at a few, and I think the right strategy is to tell
the tool "no really, our code is fine". For instance, it complains
about:
argc = parse_options(argc, argv, ...);
when argc is not used again later. Sure, that assignment is doing
nothing. But from a maintainability perspective, I'd much rather have a
dead assignment (that the compiler is free to remove) then for somebody
to later add a loop like:
for (i = 0; i < argc; i++)
something(argv[i]);
which will read past the end of the rearranged argv (and probably
_wouldn't_ be caught by static analysis, because the hidden dependency
between argc and argv is buried inside the parse_options() call).
So there is definitely some bug-fixing to be done, but I think there is
also some work in figuring out how to suppress these useless reports.
Turning off the dead-assignment checker is one option, but I actually
think it _could_ produce useful results. It just isn't in these cases.
So I'd much rather if we can somehow suppress the specific callsites.
> I ran scan-build on the current master and it detected 72 potential bugs [2].
> I looked through a few of them and they seem to be legitimate. If the list agrees
> that running scan-build is a useful thing and that these problems should be fixed
> then we could:
>
> (1) Add scan-build check to Travis CI but only print errors as warning
> (2) Fix the 72 existing bugs over time
> (3) Turn scan-build warnings into errors
If they are warnings socked away in a Travis CI job that nobody looks
out, then I doubt anybody is going to bother fixing them.
Not that step (1) hurts necessarily, but I don't think it's really doing
anything until step (2) is finished.
I took a look at a few of the non-dead-assignment ones and some of them
are obviously false positives. E.g., in check_pbase_path(), it claims
that done_pbase_paths might be NULL. But that value just went through
ALLOC_GROW() with a non-zero value, which would either have allocated or
died.
There are other cases where it complains that a strbuf's "buf" parameter
might be NULL. That _shouldn't_ be the case, as it is an invariant of
strbuf. It might be a bug, but it is certainly not a bug where the
analyzer is pointing.
I won't be surprised at all if there are a bunch of real bugs in that
list. But I think the interesting work at this point is not a CI build,
but somebody locally slogging through scan-build and categorizing each
one.
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Mike Hommey @ 2017-02-25 22:09 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, Junio C Hamano, Ian Jackson, Joey Hess, git
In-Reply-To: <20170225192655.l5dbzq42cvk5surl@sigill.intra.peff.net>
On Sat, Feb 25, 2017 at 02:26:56PM -0500, Jeff King wrote:
> On Sat, Feb 25, 2017 at 06:50:50PM +0000, brian m. carlson wrote:
>
> > > As long as the reader can tell from the format of object names
> > > stored in the "new object format" object from what era is being
> > > referred to in some way [*1*], we can name new objects with only new
> > > hash, I would think. "new refers only to new" that stratifies
> > > objects into older and newer may make things simpler, but I am not
> > > convinced yet that it would give our users a smooth enough
> > > transition path (but I am open to be educated and pursuaded the
> > > other way).
> >
> > I would simply use multihash[0] for this purpose. New-style objects
> > serialize data in multihash format, so it's immediately obvious what
> > hash we're referring to. That makes future transitions less
> > problematic.
> >
> > [0] https://github.com/multiformats/multihash
>
> I looked at that earlier, because I think it's a reasonable idea for
> future-proofing. The first byte is a "varint", but I couldn't find where
> they defined that format.
>
> The closest I could find is:
>
> https://github.com/multiformats/unsigned-varint
>
> whose README says:
>
> This unsigned varint (VARiable INTeger) format is for the use in all
> the multiformats.
>
> - We have not yet decided on a format yet. When we do, this readme
> will be updated.
>
> - We have time. All multiformats are far from requiring this varint.
>
> which is not exactly confidence inspiring. They also put the length at
> the front of the hash. That's probably convenient if you're parsing an
> unknown set of hashes, but I'm not sure it's helpful inside Git objects.
> And there's an incentive to minimize header data at the front of a hash,
> because every byte is one more byte that every single hash will collide
> over, and people will have to type when passing hashes to "git show",
> etc.
>
> I'd almost rather use something _really_ verbose like
>
> sha256:1234abcd...
>
> in all of the objects. And then when we get an unadorned hash from the
> user, we guess it's sha256 (or whatever), and fallback to treating it as
> a sha1.
>
> Using a syntactically-obvious name like that also solves one other
> problem: there are sha1 hashes whose first bytes will encode as a "this
> is sha256" multihash, creating some ambiguity.
Indeed, multihash only really is interesting when *all* hashes use it.
And obviously, git can't change the existing sha1s.
Mike
^ permalink raw reply
* Re: [PATCH 2/2] commit: don't check for space twice when looking for header
From: Jeff King @ 2017-02-25 21:51 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <6462e74c-1ee7-7ed0-0695-9889df803943@web.de>
On Sat, Feb 25, 2017 at 10:39:29PM +0100, René Scharfe wrote:
> > > + (len == 8 && !memcmp(field, "encoding", 8)));
> >
> > Unrelated, but this could probably be spelled with a macro and strlen()
> > to avoid the magic numbers. It would probably be measurably slower for a
> > compiler which doesn't pre-compute strlen() on a string literal, though.
>
> sizeof(string_constant) - 1 might be a better choice here than strlen().
Yeah. If you use a macro, that works. If it's an inline function you'd
need strlen(). That's a tradeoff we've already made in skip_prefix_mem()
and strip_suffix(), but it's not like we expect this list to grow much,
so it may not be worth fussing with.
-Peff
^ permalink raw reply
* Re: [PATCH] travis-ci: run scan-build every time
From: Lars Schneider @ 2017-02-25 21:48 UTC (permalink / raw)
To: Samuel Lijin; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <CAJZjrdXP3n5fOLx4rEEkbJT7JBMPUqk4Qdutm6KpvMVUMwCSPQ@mail.gmail.com>
> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
>
> Introduces the scan-build static code analysis tool from the Clang
> project to all Travis CI builds. Installs clang (since scan-build
> needs clang as a dependency) to make this possible (on macOS, also
> updates PATH to allow scan-build to be invoked without referencing the
> full path).
This is a pretty neat idea. However, I think this should become a
dedicated job in a TravisCI build (similar to the Documentation job [1])
because:
a) We don't want to build and test a scan-build version of Git (AFAIK
scan-build kind of proxies the compiler to do its job - I don't if
this has any side effects)
b) We don't want to slow down the other builds
c) It should be enough to run scan-build once on Linux per build
I ran scan-build on the current master and it detected 72 potential bugs [2].
I looked through a few of them and they seem to be legitimate. If the list agrees
that running scan-build is a useful thing and that these problems should be fixed
then we could:
(1) Add scan-build check to Travis CI but only print errors as warning
(2) Fix the 72 existing bugs over time
(3) Turn scan-build warnings into errors
[1] https://github.com/git/git/blob/e7e07d5a4fcc2a203d9873968ad3e6bd4d7419d7/.travis.yml#L42-L53
[2] https://larsxschneider.github.io/git-scan/
> ---
>
> A build with this patch can be found at [1]. Note that if reports *are*
> generated, this doesn't allow us to access them, and if we dumped
> the reports as build artifacts, I'm not sure where we would want to
> dump them to.
We could upload the results to a Git repo and then use GitHub pages to serve
it. I did that with my run here: https://larsxschneider.github.io/git-scan/
> It's worth noting that there seems to be a weird issue with scan-build
> where it *will* generate a report for something locally, but won't do it
> on Travis. See [2] for an example where I have a C program with a
> very obvious memory leak but scan-build on Travis doesn't generate
> a report (despite complaining about it in stdout), even though it does
> on my local machine.
>
> [1] https://travis-ci.org/sxlijin/git/builds/204853233
> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342
Scan-build stores the report in some temp folder. I assume you can't access
this folder on TravisCI. Try the scan-build option "-o scan-build-results"
to store the report in the local directory.
>
> .travis.yml | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 9c63c8c3f..1038b1b3d 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -20,6 +20,7 @@ addons:
> - language-pack-is
> - git-svn
> - apache2
> + - clang
>
> env:
> global:
> @@ -78,9 +79,8 @@ before_install:
> brew update --quiet
> # Uncomment this if you want to run perf tests:
> # brew install gnu-time
> - brew install git-lfs gettext
> - brew link --force gettext
> - brew install caskroom/cask/perforce
> + brew install git-lfs gettext caskroom/cask/perforce llvm
> + brew link --force gettext llvm
This wouldn't be necessary if we only scan on Linux.
> ;;
> esac;
> echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)";
> @@ -92,9 +92,9 @@ before_install:
> mkdir -p $HOME/travis-cache;
> ln -s $HOME/travis-cache/.prove t/.prove;
>
> -before_script: make --jobs=2
> +before_script: scan-build make --jobs=2
I think we should run it like this:
scan-build -analyze-headers --status-bugs --keep-going --force-analyze-debug-code make --jobs=2
This way TravisCI would be notified via the return code if scan-build detected
errors I think.
> -script: make --quiet test
> +script: scan-build make --quiet test
Why do you want to scan the tests?
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH 2/2] commit: don't check for space twice when looking for header
From: René Scharfe @ 2017-02-25 21:39 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
In-Reply-To: <20170225201522.uan52fwey6zjosym@sigill.intra.peff.net>
Am 25.02.2017 um 21:15 schrieb Jeff King:
> On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:
>
>> Both standard_header_field() and excluded_header_field() check if
>> there's a space after the buffer that's handed to them. We already
>> check in the caller if that space is present. Don't bother calling
>> the functions if it's missing, as they are guaranteed to return 0 in
>> that case, and remove the now redundant checks from them.
>
> Makes sense, and I couldn't spot any errors in your logic or in the
> code.
Thanks for checking!
>> static inline int standard_header_field(const char *field, size_t len)
>> {
>> - return ((len == 4 && !memcmp(field, "tree ", 5)) ||
>> - (len == 6 && !memcmp(field, "parent ", 7)) ||
>> - (len == 6 && !memcmp(field, "author ", 7)) ||
>> - (len == 9 && !memcmp(field, "committer ", 10)) ||
>> - (len == 8 && !memcmp(field, "encoding ", 9)));
>> + return ((len == 4 && !memcmp(field, "tree", 4)) ||
>> + (len == 6 && !memcmp(field, "parent", 6)) ||
>> + (len == 6 && !memcmp(field, "author", 6)) ||
>> + (len == 9 && !memcmp(field, "committer", 9)) ||
>> + (len == 8 && !memcmp(field, "encoding", 8)));
>
> Unrelated, but this could probably be spelled with a macro and strlen()
> to avoid the magic numbers. It would probably be measurably slower for a
> compiler which doesn't pre-compute strlen() on a string literal, though.
sizeof(string_constant) - 1 might be a better choice here than strlen().
René
^ permalink raw reply
* Re: [PATCH 2/2] apply: handle assertion failure gracefully
From: René Scharfe @ 2017-02-25 21:21 UTC (permalink / raw)
To: Vegard Nossum, Junio C Hamano, git; +Cc: Christian Couder, Michal Zalewski
In-Reply-To: <20170225101307.24067-2-vegard.nossum@oracle.com>
Am 25.02.2017 um 11:13 schrieb Vegard Nossum:
> For the patches in the added testcases, we were crashing with:
>
> git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.
>
> As it turns out, check_preimage() is prepared to handle these conditions,
> so we can remove the assertion.
>
> Found using AFL.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>
> ---
>
> (I'm fully aware of how it looks to just delete an assertion to "fix" a
> bug without any other changes to accomodate the condition that was
> being tested for. I am definitely not an expert on this code, but as far
> as I can tell -- both by reviewing and testing the code -- the function
> really is prepared to handle the case where patch->is_new == 1, as it
> will always hit another error condition if that is true. I've tried to
> add more test cases to show what errors you can expect to see instead of
> the assertion failure when trying to apply these nonsensical patches. If
> you don't want to remove the assertion for whatever reason, please feel
> free to take the testcases and add "# TODO: known breakage" or whatever.)
> ---
> apply.c | 1 -
> t/t4154-apply-git-header.sh | 36 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index cbf7cc7f2..9219d2737 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
> if (!old_name)
> return 0;
>
> - assert(patch->is_new <= 0);
5c47f4c6 (builtin-apply: accept patch to an empty file) added that line.
Its intent was to handle diffs that contain an old name even for a
file that's created. Citing from its commit message: "When we cannot be
sure by parsing the patch that it is not a creation patch, we shouldn't
complain when if there is no such a file." Why not stop complaining
also in case we happen to know for sure that it's a creation patch?
I.e., why not replace the assert() with:
if (patch->is_new == 1)
goto is_new;
> previous = previous_patch(state, patch, &status);
>
> if (status)
> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
> index d651af4a2..c440c48ad 100755
> --- a/t/t4154-apply-git-header.sh
> +++ b/t/t4154-apply-git-header.sh
> @@ -12,4 +12,40 @@ rename new 0
> EOF
> '
>
> +test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
> + test_must_fail git apply << EOF
> +diff --git a/. b/.
> +deleted file mode
> +new file mode
> +EOF
> +'
> +
> +test_expect_success 'apply deleted file mode / new file mode / wrong type' '
> + mkdir x &&
> + chmod 755 x &&
> + test_must_fail git apply << EOF
> +diff --git a/x b/x
> +deleted file mode 160755
> +new file mode
> +EOF
> +'
> +
> +test_expect_success 'apply deleted file mode / new file mode / already exists' '
> + touch 1 &&
> + chmod 644 1 &&
> + test_must_fail git apply << EOF
> +diff --git a/1 b/1
> +deleted file mode 100644
> +new file mode
> +EOF
> +'
> +
> +test_expect_success 'apply new file mode / copy from / nonexistant file' '
> + test_must_fail git apply << EOF
> +diff --git a/. b/.
> +new file mode
> +copy from
> +EOF
> +'
> +
> test_done
>
^ permalink raw reply
* Re: [PATCH 1/2] apply: guard against renames of non-existant empty files
From: René Scharfe @ 2017-02-25 20:51 UTC (permalink / raw)
To: Vegard Nossum, Junio C Hamano, git; +Cc: Christian Couder, Michal Zalewski
In-Reply-To: <20170225101307.24067-1-vegard.nossum@oracle.com>
Am 25.02.2017 um 11:13 schrieb Vegard Nossum:
> If we have a patch like the one in the new test-case, then we will
> try to rename a non-existant empty file, i.e. patch->old_name will
> be NULL. In this case, a NULL entry will be added to fn_table, which
> is not allowed (a subsequent binary search will die with a NULL
> pointer dereference).
>
> The patch file is completely bogus as it tries to rename something
> that is known not to exist, so we can throw an error for this.
>
> Found using AFL.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> apply.c | 3 ++-
> t/t4154-apply-git-header.sh | 15 +++++++++++++++
> 2 files changed, 17 insertions(+), 1 deletion(-)
> create mode 100755 t/t4154-apply-git-header.sh
>
> diff --git a/apply.c b/apply.c
> index 0e2caeab9..cbf7cc7f2 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
> patch->old_name = xstrdup(patch->def_name);
> patch->new_name = xstrdup(patch->def_name);
> }
> - if (!patch->is_delete && !patch->new_name) {
> + if ((!patch->is_delete && !patch->new_name) ||
> + (patch->is_rename && !patch->old_name)) {
Would it make sense to mirror the previously existing condition and
check for is_new instead? I.e.:
if ((!patch->is_delete && !patch->new_name) ||
(!patch->is_new && !patch->old_name)) {
or
if (!(patch->is_delete || patch->new_name) ||
!(patch->is_new || patch->old_name)) {
René
^ permalink raw reply
* Re: git-clone --config order & fetching extra refs during initial clone
From: Jeff King @ 2017-02-25 20:50 UTC (permalink / raw)
To: Robin H. Johnson; +Cc: SZEDER Gábor, Git Mailing List
In-Reply-To: <robbat2-20170225T185056-448272755Z@orbis-terrarum.net>
[Re-sending, as I used an old address for Gábor on the original]
On Sat, Feb 25, 2017 at 07:12:38PM +0000, Robin H. Johnson wrote:
> TL;DR: git-clone ignores any fetch specs passed via --config.
I agree that this is a bug. There's some previous discussion and an RFC
patch from lat March (author cc'd):
http://public-inbox.org/git/1457313062-10073-1-git-send-email-szeder@ira.uka.de/
That discussion veered off into alternatives, but I think the v2 posted
in that thread is taking a sane approach.
-Peff
^ permalink raw reply
* Re: [PATCH] sha1_file: release fallback base's memory in unpack_entry()
From: Jeff King @ 2017-02-25 20:48 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <fe07ed70-0b93-8b2c-59f9-967d3dc1313f@web.de>
On Sat, Feb 25, 2017 at 11:02:28AM +0100, René Scharfe wrote:
> If a pack entry that's used as a delta base is corrupt, unpack_entry()
> marks it as unusable and then searches the object again in the hope that
> it can be found in another pack or in a loose file. The memory for this
> external base object is never released. Free it after use.
This looks good. I wondered if there were any tricks with passing the
resulting base to the delta-base cache. But I don't think so. We add to
the cache right _before_ the fallback check we're looking at here. And
the "base" variable does not persist to the next loop.
Arguably, one could do the fallback first and then add the result to the
delta base cache, but it probably isn't worth the trouble. However we
read the object via read_object() would hopefully have done so already.
-Peff
^ permalink raw reply
* [PATCH v7 5/6] stash: use stash_push for no verb form
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170225213306.2410-1-t.gummerer@gmail.com>
Now that we have stash_push, which accepts pathspec arguments, use
it instead of stash_save in git stash without any additional verbs.
Previously we allowed git stash -- -message, which is no longer allowed
after this patch. Messages starting with a hyphen was allowed since
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options"). However it was never the intent to allow that, but rather it
was allowed accidentally.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 8 ++++----
git-stash.sh | 16 ++++++++--------
t/t3903-stash.sh | 4 +---
3 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 88369ed8b6..4d8d30f179 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,11 +13,11 @@ SYNOPSIS
'git stash' drop [-q|--quiet] [<stash>]
'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
'git stash' branch <branchname> [<stash>]
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [<message>]]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [<message>]
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [-m|--message <message>]]
- [--] [<pathspec>...]
+ [--] [<pathspec>...]]
'git stash' clear
'git stash' create [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
diff --git a/git-stash.sh b/git-stash.sh
index 57828f926d..2d7b30ec5e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,11 +7,11 @@ USAGE="list [<options>]
or: $dashless drop [-q|--quiet] [<stash>]
or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
or: $dashless branch <branchname> [<stash>]
- or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [<message>]]
- or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [-m <message>]
- [-- <pathspec>...]
+ or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [<message>]
+ or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m <message>]
+ [-- <pathspec>...]]
or: $dashless clear"
SUBDIRECTORY_OK=Yes
@@ -656,7 +656,7 @@ apply_to_branch () {
}
PARSE_CACHE='--not-parsed'
-# The default command is "save" if nothing but options are given
+# The default command is "push" if nothing but options are given
seen_non_option=
for opt
do
@@ -666,7 +666,7 @@ do
esac
done
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
# Main command set
case "$1" in
@@ -717,7 +717,7 @@ branch)
*)
case $# in
0)
- save_stash &&
+ push_stash &&
say "$(gettext "(To restore them type \"git stash apply\")")"
;;
*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 4d8a096773..2f5888df0d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -274,9 +274,7 @@ test_expect_success 'stash --invalid-option' '
git add file2 &&
test_must_fail git stash --invalid-option &&
test_must_fail git stash save --invalid-option &&
- test bar5,bar6 = $(cat file),$(cat file2) &&
- git stash -- -message-starting-with-dash &&
- test bar,bar2 = $(cat file),$(cat file2)
+ test bar5,bar6 = $(cat file),$(cat file2)
'
test_expect_success 'stash an added file' '
--
2.11.0.301.g275aeb250c.dirty
^ permalink raw reply related
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