* Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+
From: Junio C Hamano @ 2016-10-14 18:40 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Ævar Arnfjörð Bjarmason, git
In-Reply-To: <3fa9902f-3b01-5ec6-8129-34cff4c7cac9@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).
>>
>> I think it's fairly dubious to link to things matching [0-9a-fA-F]
>> here as opposed to just [0-9a-f], that dates back to the initial
>> version of gitweb from 161332a ("first working version",
>> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
>> them as far as I can tell.
>
> All right. If we decide to be more strict in what we accept, we can
> do it in a separate commit.
>
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> Acked-by: Jakub Narębski <jnareb@gmail.com>
Thanks for a review. As the topic is not yet in 'next', I'll squish
in your Acked-by: to them. I saw them only for 1 & 2/3; would
another for 3/3 be coming soon?
>
>> ---
>> gitweb/gitweb.perl | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index cba7405..92b5e91 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2036,7 +2036,7 @@ sub format_log_line_html {
>> my $line = shift;
>>
>> $line = esc_html($line, -nbsp=>1);
>> - $line =~ s{\b([0-9a-fA-F]{8,40})\b}{
>> + $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
>
> By the way, it is quite long commit message for one character change.
> Not that it is a bad thing...
>
>> $cgi->a({-href => href(action=>"object", hash=>$1),
>> -class => "text"}, $1);
>> }eg;
>>
^ permalink raw reply
* Re: [PATCH] fetch: use "quick" has_sha1_file for tag following
From: Jeff King @ 2016-10-14 18:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: Vegard Nossum, git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqfunyzv6f.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 14, 2016 at 10:39:52AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So it's certainly better. But 7500 packs is just silly, and squeezing
> > out ~400ms there is hardly worth it. If you repack this same case into a
> > single pack, the command drops to 5ms. So yes, there's close to an order
> > of magnitude speedup here, but you get that _and_ another order of
> > magnitude just by repacking.
>
> "7500 is silly" equally applies to the "quick" (and sloppy, if I am
> reading your "Failing in this direction doesn't make me feel great."
> correctly) approach, I think, which argues for not taking either
> change X-<.
I wouldn't quite agree that they're the same. 7500 packs is silly
because a bunch of _other_ things are going to get equally or more slow
before the prepare_packed_git() slowdown is noticeable. And it's in your
power to clean up, and we should encourage users to do so.
Whereas having a bunch of unfetched tags is a state that may linger
indefinitely, and be outside the user's control.
I _am_ open to the argument that calling reprepare_packed_git() over and
over doesn't really matter much if you have a sane number of packs. If
you tweak my perf test like so:
diff --git a/t/perf/p5550-fetch-tags.sh b/t/perf/p5550-fetch-tags.sh
index a5dc39f..7e7ae24 100755
--- a/t/perf/p5550-fetch-tags.sh
+++ b/t/perf/p5550-fetch-tags.sh
@@ -86,7 +86,7 @@ test_expect_success 'create child packs' '
cd child &&
git config gc.auto 0 &&
git config gc.autopacklimit 0 &&
- create_packs 500
+ create_packs 10
)
'
you get:
Test origin quick
--------------------------------------------------------
5550.4: fetch 0.06(0.02+0.02) 0.02(0.01+0.00) -66.7%
Still an impressive speedup as a percentage, but negligible in absolute
terms. But that's on a local filesystem on a Linux machine. I'd worry
much more about a system with a slow readdir(), e.g., due to NFS.
Somebody's real-world NFS case[1] was what prompted us to do 0eeb077
(index-pack: avoid excessive re-reading of pack directory, 2015-06-09).
It looks like I _did_ look into optimizing this into a single stat()
call in the thread at [1]. I completely forgot about that. I did find
there that naively using stat_validity() on a directory is racy, though
I wonder if we could do something clever with gettimeofday() instead.
IOW, the patches there only bothered to re-read when they saw the mtime
on the directory jump, which suffers from 1-second precision problems.
But if we instead compared the mtime to the current time, we could err
in favor of re-reading the packs, and get false positives for at most 1
second.
[1] http://public-inbox.org/git/7FAE15F0A93C0144AD8B5FBD584E1C5519758FC3@C111KXTEMBX51.ERF.thomson.com/
> I agree that the fallout from the inaccuracy of "quick" approach is
> probably acceptable and the next "fetch" will correct it anyway, so
> let's do the "quick but inaccurate" for now and perhaps cook it in
> 'next' for a bit longer than other topics?
I doubt that cooking in 'next' for longer will turn up anything useful.
The case we care about is the race between a repack and a fetch. We
lived with the "quick" version of has_sha1_file() everywhere for 8
years. I only noticed the race on a hosting cluster which puts through
millions of operations per day (I could in theory test the patch on that
same cluster, but we actually don't do very many fetches).
-Peff
^ permalink raw reply related
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-14 19:00 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <0e4173b6-8fbb-4223-b061-524c7ebfd4d7@oracle.com>
On Fri, Oct 14, 2016 at 08:55:29AM +0200, Vegard Nossum wrote:
> On 10/13/2016 10:43 PM, Jeff King wrote:
> > No problem. I do think you'll benefit a lot from packing everything into
> > a single pack, but it's also clear that Git was doing more work than it
> > needed to be. This was a known issue when we added the racy-check to
> > has_sha1_file(), and knew that we might have to field reports like this
> > one.
>
> After 'git gc' (with the .5G limit) I have 23 packs and with your patch
> I now get:
>
> $ time git fetch
>
> real 0m26.520s
> user 0m3.580s
> sys 0m0.636s
Cool. That's about what I'd expect based on the size numbers you gave
earlier. The other 23 seconds are likely being spent on passing the ref
advertisement over the network.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] gitweb: Link to "git describe"'d commits in log messages
From: Junio C Hamano @ 2016-10-14 19:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git, Jakub Narebski
In-Reply-To: <CACBZZX4HPci=n193WPm1RCes4PZfFXQtAdJuwxMwvTvF2NBVMA@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> I just ran into an example of a better reason for doing it like my
> patch is doing, which is that if you have some tag like:
>
> deployment-20160928-171914-16-g42e13d8
>
> With my patch the whole thing will be a link to the 42e13d8 commit,
> but with this suggestion both 20160928 and 42e13d8 would become commit
> links, the former one would be broken.
>
> Of course we could have some code that would detect that the whole \S+
> is part of one thing that ends in g<commit>,...
I think that this example shows a flaw not in the "suffix that looks
like an object name" approach, but more in the boundary regexp,
namely, the \b part; it is probably too loose.
And \S+ is not the right cue, either, for that matter. IOW, "we
only should take hexstring, optionally prefixed with 'g', that
appears before the whitespace" is too strict, as a sentence
We broke the system with deployment-g42e13d8.
does want to link to 42e13d8, even though full-stop at the end is
not whitespace, and the existing regexp uses \b there as a rough
equivalent to saying "Here must be a whitespace or punctuation".
An attempt to tighten "what a punctuation is" by excluding '-' may
make that "timestamp is in the tagname" example work, but is not a
good solution, either, because two sentences can be concatenated
with an em-dash that is often typed as two hyphen in plain text,
resulting in something like
We broke the system with deployment-g42e13d8--sigh.
and we do want to treat the '-' after 42e13d8 as a punctuation after
a described object name.
So I agree 3/3 is good thing to do as-is.
Thanks.
^ permalink raw reply
* Re: Can we make interactive add easier to use?
From: Jeff King @ 2016-10-14 19:12 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499AnuVximRgM0MKdq5JC-hwkrhox6bK_KA+XGrawoz2W+g@mail.gmail.com>
On Fri, Oct 14, 2016 at 08:20:40AM -0500, Robert Dailey wrote:
> Normally when I use interactive add, I just want to add files to the
> index via simple numbers, instead of typing paths. So I'll do this as
> quick as I can:
I'd generally second Matthieu's suggestion to use a combination of "git
add" and "git add -p". But if you really like the interactive updater,
then the optimizations I can think of are:
> 1. Type `git add -i`
> 2. Press `u` after prompt appears
> 3. Press numbers for the files I want to add, ENTER key
> 4. ENTER key again to go back to main add -i menu
> 5. Press `q` to exit interactive add
> 6. Type `git commit`
We have "git add -p" to avoid having to do a similar workflow just to
get to the "p"atch menu. So in theory we could have a similar shortcut
to get to "u"pdate. I think it's just not in common enough use that
anybody really bothered to implement it.
We also have "commit -p" (and "commit -i") already, though I do not use
them myself.
That would probably take you down to:
1. git commit --iu ;# obviously a terrible option name
2. Press numbers, then ENTER
3. 'q' or ENTER or ^D to exit, and jump into commit message
automatically.
You can also set the interactive.singleKey config option to turn (2)
into just "press numbers" (which works right now).
> This feels very tedious. Is there a simplified workflow for this? I
> remember using a "git index" unofficial extension to git that let you
> do a `git status` that showed numbers next to each item (including
> untracked files!) and you could do `git add 1, 2, 3-5`, etc.
TBH, that extension sounds a lot more generically useful, as it works at
the shell level. I _think_ I've seen similar features implemented that
are not even git specific (i.e., they work off of existing
shell-completion libraries and just let you pick the options by number
rather than tab-completing). Sorry I don't have any links, though. It's
not something I ever used myself.
-Peff
^ permalink raw reply
* Re: Change Default merge strategy options
From: Jeff King @ 2016-10-14 19:18 UTC (permalink / raw)
To: Daniel Lopez; +Cc: git@vger.kernel.org, Francisco Carreira
In-Reply-To: <HE1PR0101MB218771966DAC7634B9735634BDDC0@HE1PR0101MB2187.eurprd01.prod.exchangelabs.com>
On Thu, Oct 13, 2016 at 03:57:55PM +0000, Daniel Lopez wrote:
> How to use 'git config --global' to set default strategy like
> recursive.
I don't think there is a way to do so, though note that the default
strategy is already "recursive". You can set individual file merge
drivers with gitattributes, but I don't know of a way to set the overall
strategy.
> Example:
> Currently , when we want to enforce a specific strategic we need to
> include its reference on the command line :
> git.exe merge --strategy=recursive --strategy-option=ignore-all-space dev-local
Some strategy options have their own config already (like
merge.renormalize). I guess in theory we could grow config options for
the other ones, though I think config for "strategy and its options"
might be more elegant.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] gitweb: Link to "git describe"'d commits in log messages
From: Jakub Narębski @ 2016-10-14 20:06 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git; +Cc: Junio C Hamano
In-Reply-To: <20161006091135.29590-4-avarab@gmail.com>
W dniu 06.10.2016 o 11:11, Ævar Arnfjörð Bjarmason pisze:
> Change the log formatting function to know about "git describe" output
> such as "v2.8.0-4-g867ad08", in addition to just plain "867ad08".
>
> There are still many valid refnames that we don't link to
> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to
> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,
> similarly it's trivially possible to create some refnames like
> "æ/var-gf6727b0" or which won't be picked up by this regex.
It's important to cover most common cases occurring naturally in
people's repositories, while trying to avoid false positives (the latter
is important more now, where gitweb doesn't check for rev name validity).
I guess that most common is to use non-hierarchical tags with ASCII-only
names for describe output, so while "æ/var-gf6727b0" won't be picked,
it is IMVHO also much less likely to occur.
>
> There's surely room for improvement here, but I just wanted to address
> the very common case of sticking "git describe" output into commit
> messages without trying to link to all possible refnames, that's going
> to be a rather futile exercise given that this is free text, and it
> would be prohibitively expensive to look up whether the references in
> question exist in our repository.
>
> There was on-list discussion about how we could do better than this
> patch. Junio suggested to update parse_commits() to call a new
> "gitweb--helper" command which would pass each of the revision
> candidates through "rev-parse --verify --quiet". That would cut down
> on our false positives (e.g. we'll link to "deadbeef"), and also allow
> us to be more aggressive in selecting candidate revisions.
>
> That may be too expensive to work in practice, or it may
> not. Investigating that would be a good follow-up to this patch.
All right. That's good and enough for one patch.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
BTW. to add to whole "git describe" output or not discussion: having link
span whole revision marker makes for easier to use UI: larger are to hit.
> ---
> gitweb/gitweb.perl | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 92b5e91..7cf68f0 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2036,10 +2036,24 @@ sub format_log_line_html {
> my $line = shift;
>
> $line = esc_html($line, -nbsp=>1);
> - $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
> + $line =~ s{
> + \b
> + (
> + # The output of "git describe", e.g. v2.10.0-297-gf6727b0
> + # or hadoop-20160921-113441-20-g094fb7d
> + (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
> + [A-Za-z0-9.-]+
> + (?!\.) # refs can't end with ".", see check_refname_format()
> + -g[0-9a-fA-F]{7,40}
> + |
> + # Just a normal looking Git SHA1
> + [0-9a-fA-F]{7,40}
> + )
> + \b
Nice using of eXplained regexp. It is much easier to understand with
comments.
> + }{
> $cgi->a({-href => href(action=>"object", hash=>$1),
> -class => "text"}, $1);
> - }eg;
> + }egx;
>
> return $line;
> }
>
^ permalink raw reply
* Automagic `git checkout branchname` mysteriously fails
From: Martin Langhoff @ 2016-10-14 20:25 UTC (permalink / raw)
To: Git
In a (private) repo project I have, I recently tried (and failed) to do:
git checkout v4.1-support
getting a "pathspec did not match any files known to git" error.
There's an origin/v4.1-support, there is no v4.1-support "local"
branch. Creating the tracking branch explicitly worked.
Other similar branches in existence upstream did work. Autocomplete
matched git's own behaviour for this; where git checkout foo woudn't
work, autocomplete would not offer a completion.
Why is this?
One theory I have not explored is that I have other remotes, and some
have a v4.1-support branch. If that's the case, the error message is
not very helpful, and could be improved.
git --version
2.7.4
DWIM in git is remarkably good, even addictive... when it works :-)
cheers,
m
--
martin.langhoff@gmail.com
- ask interesting questions ~ http://linkedin.com/in/martinlanghoff
- don't be distracted ~ http://github.com/martin-langhoff
by shiny stuff
^ permalink raw reply
* Re: Automagic `git checkout branchname` mysteriously fails
From: Kevin Daudt @ 2016-10-14 20:58 UTC (permalink / raw)
To: Martin Langhoff; +Cc: Git
In-Reply-To: <CACPiFC+8+wVEcDt9JZgTW1dwCCFKszyXD6ysDxNQorcNkom7Lw@mail.gmail.com>
On Fri, Oct 14, 2016 at 04:25:49PM -0400, Martin Langhoff wrote:
> In a (private) repo project I have, I recently tried (and failed) to do:
>
> git checkout v4.1-support
>
> getting a "pathspec did not match any files known to git" error.
>
> There's an origin/v4.1-support, there is no v4.1-support "local"
> branch. Creating the tracking branch explicitly worked.
>
> Other similar branches in existence upstream did work. Autocomplete
> matched git's own behaviour for this; where git checkout foo woudn't
> work, autocomplete would not offer a completion.
>
> Why is this?
>
> One theory I have not explored is that I have other remotes, and some
> have a v4.1-support branch. If that's the case, the error message is
> not very helpful, and could be improved.
>
> git --version
> 2.7.4
>
> DWIM in git is remarkably good, even addictive... when it works :-)
>
> cheers,
>
Correct, this only works when it's unambiguous what branch you actually
mean.
if remote_a/branch and remote_b/branch exists, git cannot guess which
one you actually mean.
The message you get is because git checkout can be followed by several
things. Either a branch/commit or a file. Git complaining it cannot find
a file with that name is because it has exhausted all other options.
I do agree that message could be a bit more clear.
^ permalink raw reply
* Re: Automagic `git checkout branchname` mysteriously fails
From: Martin Langhoff @ 2016-10-14 21:06 UTC (permalink / raw)
To: Kevin Daudt; +Cc: Git
In-Reply-To: <20161014205842.GA6350@ikke.info>
On Fri, Oct 14, 2016 at 4:58 PM, Kevin Daudt <me@ikke.info> wrote:
> Correct, this only works when it's unambiguous what branch you actually
> mean.
That's not surprising, but there isn't a warning. IMHO, finding
several branch matches is a strong indication that it'll be worth
reporting to the user that the DWIM machinery got hits, but couldn't
work it out.
I get that process is not geared towards making a friendly msg easy,
but we could print to stderr something like:
"branch" matches more than one candidate ref, cannot choose automatically.
If you mean to check out a branch, try git branch command.
If you mean to check out a file, use -- before the pathname to
disambiguate.
and then continue with execution. With a bit of wordsmithing, the msg
can be made to be helpful in the various failure modes.
cheers,
m
--
martin.langhoff@gmail.com
- ask interesting questions ~ http://linkedin.com/in/martinlanghoff
- don't be distracted ~ http://github.com/martin-langhoff
by shiny stuff
^ permalink raw reply
* Re: [PATCH v15 14/27] t6030: no cleanup with bad merge base
From: Junio C Hamano @ 2016-10-14 21:43 UTC (permalink / raw)
To: Pranit Bauva; +Cc: git
In-Reply-To: <01020157c38b1adc-d5fa6b9a-ce13-4ee9-874e-e45fac99fba6-000000@eu-west-1.amazonses.com>
Pranit Bauva <pranit.bauva@gmail.com> writes:
> +test_expect_success 'check whether bisection cleanup is not done with bad merges' '
> + git bisect start $HASH7 $SIDE_HASH7 &&
> + test_expect_failure git bisect bad >out 2>out &&
I think you meant "test_must_fail" here.
> + test_i18ngrep "The merge base" out &&
> + test -e .git/BISECT_START
> +'
> +
> test_done
>
> --
> https://github.com/git/git/pull/287
^ permalink raw reply
* [RFC] test-lib: detect common misuse of test_expect_failure
From: Junio C Hamano @ 2016-10-14 22:38 UTC (permalink / raw)
To: git
It is a very easy mistake to make to say test_expect_failure when
making sure a step in the test fails, which must be spelled
"test_must_fail". By introducing a toggle $test_in_progress that is
turned on at the beginning of test_start_() and off at the end of
test_finish_() helper, we can detect this fairly easily.
Strictly speaking, writing "test_expect_success" inside another
test_expect_success (or inside test_expect_failure for that matter)
can be detected with the same mechanism if we really wanted to, but
that is a lot less likely confusion, so let's not bother.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* It is somewhat embarrassing to admit that I had to stare at the
offending code for more than 5 minutes to notice what went wrong
to come up with <xmqqr37iy5bw.fsf@gitster.mtv.corp.google.com>;
if we had something like this, it would have helped.
t/test-lib-functions.sh | 4 ++++
t/test-lib.sh | 3 +++
2 files changed, 7 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..fc8c10a061 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -381,6 +381,10 @@ test_verify_prereq () {
}
test_expect_failure () {
+ if test "$test_in_progress" = 1
+ then
+ error "bug in the test script: did you mean test_must_fail instead of test_expect_failure?"
+ fi
test_start_
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..4c360216e5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -344,6 +344,7 @@ test_count=0
test_fixed=0
test_broken=0
test_success=0
+test_in_progress=0
test_external_has_tap=0
@@ -625,6 +626,7 @@ test_run_ () {
}
test_start_ () {
+ test_in_progress=1
test_count=$(($test_count+1))
maybe_setup_verbose
maybe_setup_valgrind
@@ -634,6 +636,7 @@ test_finish_ () {
echo >&3 ""
maybe_teardown_valgrind
maybe_teardown_verbose
+ test_in_progress=0
}
test_skip () {
^ permalink raw reply related
* Re: [RFC] test-lib: detect common misuse of test_expect_failure
From: Jeff King @ 2016-10-14 23:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqk2day2ry.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 14, 2016 at 03:38:41PM -0700, Junio C Hamano wrote:
> It is a very easy mistake to make to say test_expect_failure when
> making sure a step in the test fails, which must be spelled
> "test_must_fail". By introducing a toggle $test_in_progress that is
> turned on at the beginning of test_start_() and off at the end of
> test_finish_() helper, we can detect this fairly easily.
>
> Strictly speaking, writing "test_expect_success" inside another
> test_expect_success (or inside test_expect_failure for that matter)
> can be detected with the same mechanism if we really wanted to, but
> that is a lot less likely confusion, so let's not bother.
I like the general idea, but I'm not sure how this would interact with
the tests in t0000 that test the test suite. It looks like that always
happens in a full sub-shell invocation (via run_sub_test_lib_test), so
we're OK as long as test_in_progress is not exported (and obviously the
subshell cannot accidentally overwrite our variable with a 0 when it is
finished).
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index fdaeb3a96b..fc8c10a061 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -381,6 +381,10 @@ test_verify_prereq () {
> }
>
> test_expect_failure () {
> + if test "$test_in_progress" = 1
> + then
> + error "bug in the test script: did you mean test_must_fail instead of test_expect_failure?"
> + fi
This follows existing practice for things like the &&-lint-checker, and
bails out on the whole test script. That sometimes makes it hard to find
the problematic test, especially if you're running via something like
"prove", because it doesn't make valid TAP output.
It might be nicer if we just said "this test is malformed, and therefore
fails", and then you get all the usual niceties for recording and
finding the failed test.
I don't think it would be robust enough to try to propagate the error up
to the outer test_expect_success block (and anyway, you'd also want to
know about it in a test_expect_failure block; it's a bug in the test,
not a known breakage). But perhaps error() could dump some TAP-like
output with a "virtual" failed test.
Something like:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bd..dc6b1f5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -299,9 +299,8 @@ TERM=dumb
export TERM
error () {
- say_color error "error: $*"
- GIT_EXIT_OK=t
- exit 1
+ test_failure_ "$@"
+ test_done
}
say () {
@@ -600,7 +599,7 @@ test_run_ () {
# code of other programs
test_eval_ "(exit 117) && $1"
if test "$?" != 117; then
- error "bug in the test script: broken &&-chain: $1"
+ error "bug in the test script: broken &&-chain" "$1"
fi
trace=$trace_tmp
fi
which lets "make prove" collect the broken test number.
It would perhaps need to cover the case when $test_count is "0"
separately. I dunno. It would be nicer still if we could continue
running other tests in the script, but I think it's impossible to
robustly jump back to the outer script.
These kinds of "bug in the test suite" are presumably rare enough that
the niceties don't matter that much, but I trigger the &&-checker
reasonably frequently (that and test_line_count, because I can never
remember the correct invocation).
Anyway. That's all orthogonal to your patch. I just wondered if we could
do better, but AFAICT the right way to do better is to hook into
error(), which means your patch would not have to care exactly how it
fails.
-Peff
^ permalink raw reply related
* Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+
From: Ævar Arnfjörð Bjarmason @ 2016-10-15 8:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narębski, Git Mailing List
In-Reply-To: <xmqq37jyzsdk.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 14, 2016 at 8:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narębski <jnareb@gmail.com> writes:
>
>> s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).
>>>
>>> I think it's fairly dubious to link to things matching [0-9a-fA-F]
>>> here as opposed to just [0-9a-f], that dates back to the initial
>>> version of gitweb from 161332a ("first working version",
>>> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
>>> them as far as I can tell.
>>
>> All right. If we decide to be more strict in what we accept, we can
>> do it in a separate commit.
>>
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>
>> Acked-by: Jakub Narębski <jnareb@gmail.com>
>
> Thanks for a review. As the topic is not yet in 'next', I'll squish
> in your Acked-by: to them. I saw them only for 1 & 2/3; would
> another for 3/3 be coming soon?
As far as I can tell the only outstanding "change this" is your
s/SHA1/SHA-1/ in <xmqq37k9jm86.fsf@gitster.mtv.corp.google.com>, do
you want to fix that up or should I submit another series?
>>
>>> ---
>>> gitweb/gitweb.perl | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index cba7405..92b5e91 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2036,7 +2036,7 @@ sub format_log_line_html {
>>> my $line = shift;
>>>
>>> $line = esc_html($line, -nbsp=>1);
>>> - $line =~ s{\b([0-9a-fA-F]{8,40})\b}{
>>> + $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
>>
>> By the way, it is quite long commit message for one character change.
>> Not that it is a bad thing...
>>
>>> $cgi->a({-href => href(action=>"object", hash=>$1),
>>> -class => "text"}, $1);
>>> }eg;
>>>
^ permalink raw reply
* [PATCH] cocci: avoid self-references in object_id transformations
From: René Scharfe @ 2016-10-15 8:25 UTC (permalink / raw)
To: brian m. carlson; +Cc: Git List, Junio C Hamano
The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively. Make sure
that the Coccinelle transformations for converting legacy function calls
are not applied to these wrappers themselves, which would result in
tautological declarations.
We can remove the added guards once the object_id functions stand on
their own.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/object_id.cocci | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci
index 0307624..09afdbf 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -17,10 +17,13 @@ expression E1;
+ oid_to_hex(&E1)
@@
+identifier f != oid_to_hex;
expression E1;
@@
+ f(...) {...
- sha1_to_hex(E1->hash)
+ oid_to_hex(E1)
+ ...}
@@
expression E1, E2;
@@ -29,10 +32,13 @@ expression E1, E2;
+ oid_to_hex_r(E1, &E2)
@@
+identifier f != oid_to_hex_r;
expression E1, E2;
@@
+ f(...) {...
- sha1_to_hex_r(E1, E2->hash)
+ oid_to_hex_r(E1, E2)
+ ...}
@@
expression E1;
@@ -41,10 +47,13 @@ expression E1;
+ oidclr(&E1)
@@
+identifier f != oidclr;
expression E1;
@@
+ f(...) {...
- hashclr(E1->hash)
+ oidclr(E1)
+ ...}
@@
expression E1, E2;
@@ -53,10 +62,13 @@ expression E1, E2;
+ oidcmp(&E1, &E2)
@@
+identifier f != oidcmp;
expression E1, E2;
@@
+ f(...) {...
- hashcmp(E1->hash, E2->hash)
+ oidcmp(E1, E2)
+ ...}
@@
expression E1, E2;
@@ -77,10 +89,13 @@ expression E1, E2;
+ oidcpy(&E1, &E2)
@@
+identifier f != oidcpy;
expression E1, E2;
@@
+ f(...) {...
- hashcpy(E1->hash, E2->hash)
+ oidcpy(E1, E2)
+ ...}
@@
expression E1, E2;
--
2.10.1
^ permalink raw reply related
* Re: [PATCH v15 14/27] t6030: no cleanup with bad merge base
From: Pranit Bauva @ 2016-10-15 8:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqr37iy5bw.fsf@gitster.mtv.corp.google.com>
Hey Junio,
On Sat, Oct 15, 2016 at 3:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> +test_expect_success 'check whether bisection cleanup is not done with bad merges' '
>> + git bisect start $HASH7 $SIDE_HASH7 &&
>> + test_expect_failure git bisect bad >out 2>out &&
>
> I think you meant "test_must_fail" here.
Oh yes! Thanks for pointing it out. I see that you have already
submitted a patch to catch this mistake which was previously ignored.
Thanks!
Regards,
Pranit Bauva
^ permalink raw reply
* [PATCH] t0040: convert all possible tests to use `test-parse-options --expect`
From: Pranit Bauva @ 2016-10-15 12:28 UTC (permalink / raw)
To: git
Use "test-parse-options --expect" to rewrite the tests to avoid checking
the whole variable dump by just testing what is required. This commit is
based on 8ca65aeb (t0040: convert a few tests to use test-parse-options;
Junio C Hamano; May 6, 2016).
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t0040-parse-options.sh | 183 ++++-------------------------------------------
1 file changed, 13 insertions(+), 170 deletions(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index db5f60d..74d2cd7 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -208,32 +208,15 @@ test_expect_success 'unambiguously abbreviated option' '
'
test_expect_success 'unambiguously abbreviated option with "="' '
- test-parse-options --int=2 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="integer: 2" --int=2
'
test_expect_success 'ambiguously abbreviated option' '
test_expect_code 129 test-parse-options --strin 123
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: 123
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'non ambiguous option (after two options it abbreviates)' '
- test-parse-options --st 123 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="string: 123" --st 123
'
cat >typo.err <<\EOF
@@ -256,24 +239,8 @@ test_expect_success 'detect possible typos' '
test_cmp typo.err output.err
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-arg 00: --quux
-EOF
-
test_expect_success 'keep some options as arguments' '
- test-parse-options --quux >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="arg 00: --quux" --quux
'
cat >expect <<\EOF
@@ -350,54 +317,20 @@ test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '
test_cmp expect output
'
-cat >expect <<\EOF
-boolean: 6
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'OPT_BIT() works' '
- test-parse-options -bb --or4 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="boolean: 6" -bb --or4
'
test_expect_success 'OPT_NEGBIT() works' '
- test-parse-options -bb --no-neg-or4 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="boolean: 6" -bb --no-neg-or4
'
test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
- test-parse-options + + + + + + >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="boolean: 6" + + + + + +
'
-cat >expect <<\EOF
-boolean: 0
-integer: 12345
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'OPT_NUMBER_CALLBACK() works' '
- test-parse-options -12345 >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="integer: 12345" -12345
'
cat >expect <<\EOF
@@ -435,118 +368,28 @@ test_expect_success '--no-list resets list' '
test_cmp expect output
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 3
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'multiple quiet levels' '
- test-parse-options -q -q -q >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="quiet: 3" -q -q -q
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: 3
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success 'multiple verbose levels' '
- test-parse-options -v -v -v >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="verbose: 3" -v -v -v
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-quiet sets --quiet to 0' '
- test-parse-options --no-quiet >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="quiet: 0" --no-quiet
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-quiet resets multiple -q to 0' '
- test-parse-options -q -q -q --no-quiet >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="quiet: 0" -q -q -q --no-quiet
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: 0
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-verbose sets verbose to 0' '
- test-parse-options --no-verbose >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="verbose: 0" --no-verbose
'
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 0
-string: (not set)
-abbrev: 7
-verbose: 0
-quiet: 0
-dry run: no
-file: (not set)
-EOF
-
test_expect_success '--no-verbose resets multiple verbose to 0' '
- test-parse-options -v -v -v --no-verbose >output 2>output.err &&
- test_must_be_empty output.err &&
- test_cmp expect output
+ test-parse-options --expect="verbose: 0" -v -v -v --no-verbose
'
test_done
--
https://github.com/git/git/pull/299
^ permalink raw reply related
* Re: [PATCH] cocci: avoid self-references in object_id transformations
From: brian m. carlson @ 2016-10-15 13:45 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <a5ed26c0-7fea-259c-74c1-0cd870a35290@web.de>
[-- Attachment #1: Type: text/plain, Size: 762 bytes --]
On Sat, Oct 15, 2016 at 10:25:34AM +0200, René Scharfe wrote:
> The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
> oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
> sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively. Make sure
> that the Coccinelle transformations for converting legacy function calls
> are not applied to these wrappers themselves, which would result in
> tautological declarations.
Ah, yes, this is a good idea. I've had to hack around this, but this is
much better than having to fix it up by hand.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-10-15 14:45 UTC (permalink / raw)
To: Jakub Narębski, Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <37c12539-3edd-e04b-6e09-e977a854661c@gmail.com>
@Peff: If you have time, it would be great if you could comment on
one question below prefixed with "@Peff". Thanks!
> On 12 Oct 2016, at 03:54, Jakub Narębski <jnareb@gmail.com> wrote:
>
> W dniu 12.10.2016 o 00:26, Lars Schneider pisze:
>>> On 09 Oct 2016, at 01:06, Jakub Narębski <jnareb@gmail.com> wrote:
>>>>
>>
>>>> After the filter started
>>>> Git sends a welcome message ("git-filter-client"), a list of
>>>> supported protocol version numbers, and a flush packet. Git expects
>>>> +to read a welcome response message ("git-filter-server") and exactly
>>>> +one protocol version number from the previously sent list. All further
>>>> +communication will be based on the selected version. The remaining
>>>> +protocol description below documents "version=2". Please note that
>>>> +"version=42" in the example below does not exist and is only there
>>>> +to illustrate how the protocol would look like with more than one
>>>> +version.
>>>> +
>>>> +After the version negotiation Git sends a list of all capabilities that
>>>> +it supports and a flush packet. Git expects to read a list of desired
>>>> +capabilities, which must be a subset of the supported capabilities list,
>>>> +and a flush packet as response:
>>>> +------------------------
>>>> +packet: git> git-filter-client
>>>> +packet: git> version=2
>>>> +packet: git> version=42
>>>> +packet: git> 0000
>>>> +packet: git< git-filter-server
>>>> +packet: git< version=2
>>>> +packet: git> clean=true
>>>> +packet: git> smudge=true
>>>> +packet: git> not-yet-invented=true
>>>> +packet: git> 0000
>>>> +packet: git< clean=true
>>>> +packet: git< smudge=true
>>>> +packet: git< 0000
>>>
>>> WARNING: This example is different from description!!!
>>
>> Can you try to explain the difference more clearly? I read it multiple
>> times and I think this is sound.
>
> I'm sorry it was *my mistake*. I have read the example exchange wrong.
>
> On the other hand that means that I have other comment, which I though
> was addressed already in v10, namely that not all exchanges ends with
> flush packet (inconsistency, and I think a bit of lack of extendability).
Well, this part of the protocol is not supposed to be extensible because
it is supposed to deal *only* with the version number. It needs to keep
the same structure to ensure forward and backward compatibility.
However, for consistency sake I will add a flush packet.
>>> In description above the example you have 4-part handshake, not 3-part;
>>> the filter is described to send list of supported capabilities last
>>> (a subset of what Git command supports).
>>
>> Part 1: Git sends a welcome message...
>> Part 2: Git expects to read a welcome response message...
>> Part 3: After the version negotiation Git sends a list of all capabilities...
>> Part 4: Git expects to read a list of desired capabilities...
>>
>> I think example and text match, no?
>
> Yes, it does; as I have said already, I have misread the example.
>
> Anyway, in some cases 4-way handshake, where Git sends list of
> supported capabilities first, is better. If the protocol has
> to prepare something for each of capabilities, and perhaps check
> those preparation status, it can do it after Git sends what it
> could need, and before it sends what it does support.
>
> Though it looks a bit strange that client (as Git is client here)
> sends its capabilities first...
Git tells the filter what it can do. Then the filter decides what
features it supports. I would prefer to keep it that way as I don't
see a strong advantage for the other way around.
>>> By the way, now I look at it, the argument for using the
>>> "<capability>=true" format instead of "capability=<capability>"
>>> (or "supported-command=<capability>") is weak. The argument for
>>> using "<variable>=<value>" to make it easier to implement parsing
>>> is sound, but the argument for "<capability>=true" is weak.
>>>
>>> The argument was that with "<capability>=true" one can simply
>>> parse metadata into hash / dictionary / hashmap, and choose
>>> response based on that. Hash / hashmap / associative array
>>> needs different keys, so the reasoning went for "<capability>=true"
>>> over "capability=<capability>"... but the filter process still
>>> needs to handle lines with repeating keys, namely "version=<N>"
>>> lines!
>>>
>>> So the argument doesn't hold water IMVHO, and we can choose
>>> version which reads better / is more natural.
>>
>> I have to agree that "capability=<capability>" might read a
>> little bit nicer. However, Peff suggested "<capability>=true"
>> as his preference and this is absolutely OK with me.
>
> From what I remember it was Peff stating that he thinks "<foo>=true"
> is easier for parsing (it is, but we still need to support the harder
> way parsing anyway), and offered that "<foo>" is good enough (if less
> consistent).
>
>> I am happy to change that if a second reviewer shares your
>> opinion.
>
> Also, with "capability=<foo>" we can be more self descriptive,
> for example "supported-command=<foo>"; though "capability" is good
> enough for me.
>
> For example
>
> packet: git> wants=clean
> packet: git> wants=smudge
> packet: git> wants=size
> packet: git> 0000
> packet: git< supports=clean
> packet: git< supports=smudge
> packet: git< 0000
>
> Though coming up with good names is hard; and as I said "capability"
> is good enough; OTOH with "smudge=true" etc. we don't need to come
> up with good name at all... though I wonder if it is a good thing `\_o,_/
How about this (I borrowed these terms from contract negotiation)?
packet: git> offers=clean
packet: git> offers=smudge
packet: git> offers=size
packet: git> 0000
packet: git< accepts=clean
packet: git< accepts=smudge
packet: git< 0000
@Peff: Would that be OK for you?
>>>> +------------------------
>>>> +packet: git< status=abort
>>>> +packet: git< 0000
>>>> +------------------------
>>>> +
>>>> +After the filter has processed a blob it is expected to wait for
>>>> +the next "key=value" list containing a command. Git will close
>>>> +the command pipe on exit. The filter is expected to detect EOF
>>>> +and exit gracefully on its own.
>>>
>>> Any "kill filter" solutions should probably be put here.
>>
>> Agreed.
>>
>>> I guess
>>> that filter exiting means EOF on its standard output when read
>>> by Git command, isn't it?
>>
>> Yes, but at this point Git is not listening anymore.
>
> I think it might be good idea to have here the information about
> what filter process should do if it needs maybe lengthy closing
> process, to not hold/stop Git command or to not be killed.
I've added:
"Git will wait until the filter process has stopped."
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v10 14/14] contrib/long-running-filter: add long running filter example
From: Lars Schneider @ 2016-10-15 14:47 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git, gitster, jnareb, peff
In-Reply-To: <3bdfebab-aae9-3263-218e-c8ae394084fc@web.de>
> On 08 Oct 2016, at 22:42, Torsten Bögershausen <tboegi@web.de> wrote:
>
> On 08.10.16 13:25, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Add a simple pass-thru filter as example implementation for the Git
>> filter protocol version 2. See Documentation/gitattributes.txt, section
>> "Filter Protocol" for more info.
>>
>
> Nothing wrong with code in contrib.
> I may have missed parts of the discussion, was there a good reason to
> drop the test case completely?
>
>> When adding a new feature, make sure that you have new tests to show
>> the feature triggers the new behavior when it should, and to show the
>> feature does not trigger when it shouldn't. After any code change, make
>> sure that the entire test suite passes.
>
> Or is there a plan to add them later ?
The test is part of the "main feature patch" 13/14:
http://public-inbox.org/git/20161008112530.15506-14-larsxschneider@gmail.com/
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH v10 04/14] run-command: add clean_on_exit_handler
From: Lars Schneider @ 2016-10-15 15:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, jnareb, peff
In-Reply-To: <alpine.DEB.2.20.1610111407080.3492@virtualbox>
> On 11 Oct 2016, at 05:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>
> Hi Lars,
>
> On Sat, 8 Oct 2016, larsxschneider@gmail.com wrote:
>
>> @@ -31,6 +32,15 @@ static void cleanup_children(int sig, int in_signal)
>> while (children_to_clean) {
>> struct child_to_clean *p = children_to_clean;
>> children_to_clean = p->next;
>> +
>> + if (p->process && !in_signal) {
>> + struct child_process *process = p->process;
>> + if (process->clean_on_exit_handler) {
>> + trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
>
> On Windows, pid_t translates to long long int, resulting in this build
> error:
>
> -- snip --
> In file included from cache.h:10:0,
> from run-command.c:1:
> run-command.c: In function 'cleanup_children':
> run-command.c:39:18: error: format '%d' expects argument of type 'int', but argument 5 has type 'pid_t {aka long long int}' [-Werror=format=]
> trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
> ^
> trace.h:81:53: note: in definition of macro 'trace_printf'
> trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
> ^~~~~~~~~~~
> cc1.exe: all warnings being treated as errors
> make: *** [Makefile:1987: run-command.o] Error 1
> -- snap --
>
> Maybe use PRIuMAX as we do elsewhere (see output of `git grep
> printf.*pid`):
>
> trace_printf("trace: run_command: running exit handler for pid %"
> PRIuMAX, (uintmax_t)p->pid);
Thanks for hint! I'll change it!
However, I am building on Win 8.1 with your latest SDK and I cannot
reproduce the error. Any idea why that might be the case?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH] convert: mark a file-local symbol static
From: Lars Schneider @ 2016-10-15 15:05 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <b21c8a92-4dd5-56d6-ec6a-5709028eaf5f@ramsayjones.plus.com>
> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>
> If you need to re-roll your 'ls/filter-process' branch, could you
> please squash this into commit 85290197
> ("convert: add filter.<driver>.process option", 08-10-2016).
>
>
> -void stop_multi_file_filter(struct child_process *process)
> +static void stop_multi_file_filter(struct child_process *process)
Done! Do you have some kind of script to detect these things
automatically or do you read the code that carefully?
Thanks,
Lars
^ permalink raw reply
* [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
From: René Scharfe @ 2016-10-15 16:23 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
Calculating offsets involving a NULL pointer is undefined. It works in
practice (for now?), but we should not rely on it. Allocate first and
then simply refer to the flexible array member by its name instead of
performing pointer arithmetic up front. The resulting code is slightly
shorter, easier to read and doesn't rely on undefined behaviour.
NB: The cast to a (non-const) void pointer is necessary to keep support
for flexible array members declared as const.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
This patch allows the test suite to largely pass (t7063 still fails)
for clang 3.8 with -fsanitize=undefined and -DNO_UNALIGNED_LOADS.
git-compat-util.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 43718da..f964e36 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -851,8 +851,9 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
* times, and it must be assignable as an lvalue.
*/
#define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
- (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
- (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \
+ size_t flex_array_len_ = (len); \
+ (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
+ memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
} while (0)
#define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
(x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
--
2.10.1
^ permalink raw reply related
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Torsten Bögershausen @ 2016-10-15 17:03 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Jakub Narębski, Johannes Sixt
In-Reply-To: <4e73ba3e8c1700259ffcc3224d1f66e6a760142d.1476120229.git.johannes.schindelin@gmx.de>
Not sure is this has been reported before:
sequencer.c:633:14: warning: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Wtautological-constant-out-of-range-compare]
if (command < ARRAY_SIZE(todo_command_strings))
~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
53f8024e (Johannes Schindelin 2016-10-10 19:25:07 +0200 633) if (command < ARRAY_SIZE(todo_command_strings))
^ permalink raw reply
* Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
From: Jeff King @ 2016-10-15 17:13 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <ccb15072-d949-fc84-ee45-45ba013f53c4@web.de>
On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:
> Calculating offsets involving a NULL pointer is undefined. It works in
> practice (for now?), but we should not rely on it. Allocate first and
> then simply refer to the flexible array member by its name instead of
> performing pointer arithmetic up front. The resulting code is slightly
> shorter, easier to read and doesn't rely on undefined behaviour.
Yeah, this NULL computation is pretty nasty. I recall trying to get rid
of it, but I think it is impossible to do so portably while still using
the generic xalloc_flex() helper. I'm not sure why I didn't think of
just inlining it as you do here. It's not that many lines of code, and I
I agree the result is conceptually simpler.
> #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
> - (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
> - (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \
> + size_t flex_array_len_ = (len); \
> + (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
> + memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
This looks correct. I wondered at first why you bothered with
flex_array_len, but it is to avoid evaluating the "len" parameter
multiple times.
> } while (0)
> #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
> (x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
Now that xalloc_flex() has only this one caller remaining, perhaps it
should just be inlined here, too, for simplicity.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox