* 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
* [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
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>
While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone. Unfortunately
git currently offers no such option. git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.
Allow 'git stash push' to take pathspec to specify which paths to stash.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 9 ++++-
git-stash.sh | 37 +++++++++++++------
t/t3903-stash.sh | 76 ++++++++++++++++++++++++++++++++++++++
t/t3905-stash-include-untracked.sh | 26 +++++++++++++
4 files changed, 136 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index d240df4af7..88369ed8b6 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
[-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>...]
'git stash' clear
'git stash' create [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -48,7 +49,7 @@ OPTIONS
-------
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
-push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
@@ -58,6 +59,12 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
only <message> does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
+
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec. The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
If the `--keep-index` option is used, all changes already added to the
index are left intact.
+
diff --git a/git-stash.sh b/git-stash.sh
index ef5d1b45be..57828f926d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -11,6 +11,7 @@ USAGE="list [<options>]
[-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
@@ -35,15 +36,15 @@ else
fi
no_changes () {
- git diff-index --quiet --cached HEAD --ignore-submodules -- &&
- git diff-files --quiet --ignore-submodules &&
+ git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+ git diff-files --quiet --ignore-submodules -- "$@" &&
(test -z "$untracked" || test -z "$(untracked_files)")
}
untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
- git ls-files -o -z $excl_opt
+ git ls-files -o -z $excl_opt -- "$@"
}
clear_stash () {
@@ -71,12 +72,16 @@ create_stash () {
shift
untracked=${1?"BUG: create_stash () -u requires an argument"}
;;
+ --)
+ shift
+ break
+ ;;
esac
shift
done
git update-index -q --refresh
- if no_changes
+ if no_changes "$@"
then
exit 0
fi
@@ -108,7 +113,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless commit, for
# ease of unpacking later.
u_commit=$(
- untracked_files | (
+ untracked_files "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -131,7 +136,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
@@ -145,7 +150,7 @@ create_stash () {
# find out what the user wants
GIT_INDEX_FILE="$TMP-index" \
- git add--interactive --patch=stash -- &&
+ git add--interactive --patch=stash -- "$@" &&
# state of the working tree
w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -273,27 +278,37 @@ push_stash () {
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
fi
+ test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
+
git update-index -q --refresh
- if no_changes
+ if no_changes "$@"
then
say "$(gettext "No local changes to save")"
exit 0
fi
+
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
- create_stash -m "$stash_msg" -u "$untracked"
+ create_stash -m "$stash_msg" -u "$untracked" -- "$@"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
if test -z "$patch_mode"
then
- git reset --hard ${GIT_QUIET:+-q}
+ if test $# != 0
+ then
+ git reset ${GIT_QUIET:+-q} -- "$@"
+ git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
+ git clean --force ${GIT_QUIET:+-q} -d -- "$@"
+ else
+ git reset --hard ${GIT_QUIET:+-q}
+ fi
test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
if test -n "$untracked"
then
- git clean --force --quiet -d $CLEAN_X_OPTION
+ git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
fi
if test "$keep_index" = "t" && test -n "$i_tree"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ffe3549ea5..4d8a096773 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,80 @@ test_expect_success 'create with multiple arguments for the message' '
test_cmp expect actual
'
+test_expect_success 'stash -- <pathspec> stashes and restores the file' '
+ >foo &&
+ >bar &&
+ git add foo bar &&
+ git stash push -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
+test_expect_success 'stash with multiple pathspec arguments' '
+ >foo &&
+ >bar &&
+ >extra &&
+ git add foo bar extra &&
+ git stash push -- foo bar &&
+ test_path_is_missing bar &&
+ test_path_is_missing foo &&
+ test_path_is_file extra &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ test_path_is_file extra
+'
+
+test_expect_success 'stash with file including $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash push -- "foo b*" &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
+test_expect_success 'stash push -p with pathspec shows no changes only once' '
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push -p foo >actual &&
+ echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
+ test_cmp expect actual
+'
+
+test_expect_success 'stash push with pathspec shows no changes when there are none' '
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push foo >actual &&
+ echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
+ test_cmp expect actual
+'
+
+test_expect_success 'stash push with pathspec not in the repository errors out' '
+ >untracked &&
+ test_must_fail git stash push untracked &&
+ test_path_is_file untracked
+'
+
+test_expect_success 'untracked files are left in place when -u is not given' '
+ >file &&
+ git add file &&
+ >untracked &&
+ git stash push file &&
+ test_path_is_file untracked
+'
+
test_done
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index f372fc8ca8..193adc7b68 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -185,4 +185,30 @@ test_expect_success 'stash save --all is stash poppable' '
test -s .gitignore
'
+test_expect_success 'stash push --include-untracked with pathspec' '
+ >foo &&
+ >bar &&
+ git stash push --include-untracked -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file bar &&
+ test_path_is_file foo
+'
+
+test_expect_success 'stash push with $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash push --include-untracked -- "foo b*" &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
test_done
--
2.11.0.301.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 1/6] stash: introduce push verb
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>
Introduce a new git stash push verb in addition to git stash save. The
push verb is used to transition from the current command line arguments
to a more conventional way, in which the message is given as an argument
to the -m option.
This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say which
subset of paths to stash (and leave others behind).
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 3 +++
git-stash.sh | 46 ++++++++++++++++++++++++++++++++++++++++++---
t/t3903-stash.sh | 9 +++++++++
3 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9e344cd7..d240df4af7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,6 +15,8 @@ SYNOPSIS
'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]
+ [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
'git stash' clear
'git stash' create [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -46,6 +48,7 @@ OPTIONS
-------
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>]::
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..8365ebba2a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,6 +9,8 @@ USAGE="list [<options>]
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>]
or: $dashless clear"
SUBDIRECTORY_OK=Yes
@@ -189,10 +191,11 @@ store_stash () {
return $ret
}
-save_stash () {
+push_stash () {
keep_index=
patch_mode=
untracked=
+ stash_msg=
while test $# != 0
do
case "$1" in
@@ -216,6 +219,11 @@ save_stash () {
-a|--all)
untracked=all
;;
+ -m|--message)
+ shift
+ test -z ${1+x} && usage
+ stash_msg=$1
+ ;;
--help)
show_help
;;
@@ -251,8 +259,6 @@ save_stash () {
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
fi
- stash_msg="$*"
-
git update-index -q --refresh
if no_changes
then
@@ -291,6 +297,36 @@ save_stash () {
fi
}
+save_stash () {
+ push_options=
+ while test $# != 0
+ do
+ case "$1" in
+ --)
+ shift
+ break
+ ;;
+ -*)
+ # pass all options through to push_stash
+ push_options="$push_options $1"
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done
+
+ stash_msg="$*"
+
+ if test -z "$stash_msg"
+ then
+ push_stash $push_options
+ else
+ push_stash $push_options -m "$stash_msg"
+ fi
+}
+
have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
}
@@ -617,6 +653,10 @@ save)
shift
save_stash "$@"
;;
+push)
+ shift
+ push_stash "$@"
+ ;;
apply)
shift
apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..3577115807 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial renames' '
test_path_is_missing file
'
+test_expect_success 'push -m shows right message' '
+ >foo &&
+ git add foo &&
+ git stash push -m "test message" &&
+ echo "stash@{0}: On master: test message" >expect &&
+ git stash list -1 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.11.0.301.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 0/6] stash: support pathspec argument
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: <20170219110313.24070-1-t.gummerer@gmail.com>
Thanks Junio for more comments on the last round, and Peff for reading
through it as well.
Changes since v6:
- If no --include-untracked option is given to git stash push, and a
pathspec is not in the repository, error out instead of ignoring
it. This brings it in line with things like checkout, and also
allows us to simplify the code internally.
- Simplify the code for rolling back the changes from the working
tree. This is enabled by the changes to the pathspec handling.
- There's no more "xargs -0", so there should be no more portability
concerns.
- Adjust the tests and improve some of the titles a bit
Interdiff:
diff --git a/git-stash.sh b/git-stash.sh
index 18aba1346f..28d0624c75 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -278,12 +278,15 @@ push_stash () {
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
fi
+ test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
+
git update-index -q --refresh
if no_changes "$@"
then
say "$(gettext "No local changes to save")"
exit 0
fi
+
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
@@ -296,23 +299,9 @@ push_stash () {
then
if test $# != 0
then
- saved_untracked=
- if test -n "$(git ls-files --others -- "$@")"
- then
- saved_untracked=$(
- git ls-files -z --others -- "$@" |
- xargs -0 git stash create -u all --)
- fi
- git ls-files -z -- "$@" | xargs -0 git reset ${GIT_QUIET:+-q} --
- git ls-files -z --modified -- "$@" | xargs -0 git checkout ${GIT_QUIET:+-q} HEAD --
- if test -n "$(git ls-files -z --others -- "$@")"
- then
- git ls-files -z --others -- "$@" | xargs -0 git clean --force -d ${GIT_QUIET:+-q} --
- fi
- if test -n "$saved_untracked"
- then
- git stash pop -q $saved_untracked
- fi
+ git reset ${GIT_QUIET:+-q} -- "$@"
+ git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
+ git clean --force ${GIT_QUIET:+-q} -d -- "$@"
else
git reset --hard ${GIT_QUIET:+-q}
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index c0ae41e724..f7733b4dd4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -800,7 +800,7 @@ test_expect_success 'create with multiple arguments for the message' '
test_cmp expect actual
'
-test_expect_success 'stash -- <filename> stashes and restores the file' '
+test_expect_success 'stash -- <pathspec> stashes and restores the file' '
>foo &&
>bar &&
git add foo bar &&
@@ -812,7 +812,7 @@ test_expect_success 'stash -- <filename> stashes and restores the file' '
test_path_is_file bar
'
-test_expect_success 'stash with multiple filename arguments' '
+test_expect_success 'stash with multiple pathspec arguments' '
>foo &&
>bar &&
>extra &&
@@ -842,25 +842,29 @@ test_expect_success 'stash with file including $IFS character' '
test_path_is_file bar
'
-test_expect_success 'stash push -p with pathspec shows no changes only onece' '
- >file &&
- git add file &&
- git stash push -p not-file >actual &&
+test_expect_success 'stash push -p with pathspec shows no changes only once' '
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push -p foo >actual &&
echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
test_cmp expect actual
'
test_expect_success 'stash push with pathspec shows no changes when there are none' '
- >file &&
- git add file &&
- git stash push not-file >actual &&
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push foo >actual &&
echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
test_cmp expect actual
'
-test_expect_success 'untracked file is not removed when using pathspecs' '
+test_expect_success 'stash push with pathspec not in the repository errors out' '
>untracked &&
- git stash push untracked &&
+ test_must_fail git stash push untracked &&
test_path_is_file untracked
'
Thomas Gummerer (6):
stash: introduce push verb
stash: add test for the create command line arguments
stash: refactor stash_create
stash: teach 'push' (and 'create_stash') to honor pathspec
stash: use stash_push for no verb form
stash: allow pathspecs in the no verb form
Documentation/git-stash.txt | 25 ++++++--
git-stash.sh | 114 +++++++++++++++++++++++++++-------
t/t3903-stash.sh | 122 ++++++++++++++++++++++++++++++++++++-
t/t3905-stash-include-untracked.sh | 26 ++++++++
4 files changed, 257 insertions(+), 30 deletions(-)
--
2.11.0.301.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 6/6] stash: allow pathspecs in the 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 stash_push is used in the no verb form of stash, allow
specifying the command line for this form as well. Always use -- to
disambiguate pathspecs from other non-option arguments.
Also make git stash -p an alias for git stash push -p. This allows
users to use git stash -p <pathspec>.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 11 +++++++----
git-stash.sh | 3 +++
t/t3903-stash.sh | 15 +++++++++++++++
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 4d8d30f179..70191d06b6 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -54,10 +54,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
The <message> part is optional and gives
- the description along with the stashed state. For quickly making
- a snapshot, you can omit _both_ "save" and <message>, but giving
- only <message> does not trigger this action to prevent a misspelled
- subcommand from making an unwanted stash.
+ the description along with the stashed state.
++
+For quickly making a snapshot, you can omit "push". In this mode,
+non-option arguments are not allowed to prevent a misspelled
+subcommand from making an unwanted stash. The two exceptions to this
+are `stash -p` which acts as alias for `stash push -p` and pathspecs,
+which are allowed after a double hyphen `--` for disambiguation.
+
When pathspec is given to 'git stash push', the new stash records the
modified states only for the files that match the pathspec. The index
diff --git a/git-stash.sh b/git-stash.sh
index 2d7b30ec5e..28d0624c75 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -655,12 +655,15 @@ apply_to_branch () {
}
}
+test "$1" = "-p" && set "push" "$@"
+
PARSE_CACHE='--not-parsed'
# The default command is "push" if nothing but options are given
seen_non_option=
for opt
do
case "$opt" in
+ --) break ;;
-*) ;;
*) seen_non_option=t; break ;;
esac
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2f5888df0d..f7733b4dd4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -876,4 +876,19 @@ test_expect_success 'untracked files are left in place when -u is not given' '
test_path_is_file untracked
'
+test_expect_success 'stash without verb with pathspec' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash -- "foo b*" &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
test_done
--
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