* Re: [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees
From: Nguyen Thai Ngoc Duy @ 2012-12-15 2:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Jonathon Mah
In-Reply-To: <7v4njq1ze7.fsf@alter.siamese.dyndns.org>
On Thu, Dec 13, 2012 at 9:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,
>> if (!sub)
>> die("cache-tree.c: '%.*s' in '%s' not found",
>> entlen, path + baselen, path);
>> - i += sub->cache_tree->entry_count - 1;
>> + i--; /* this entry is already counted in "sub" */
>
> Huh?
>
> The "-1" in the original is the bog-standard compensation for the
> for(;;i++) loop.
Exactly. It took me a while to figure out what " - 1" was for and I
wanted to avoid that for other developers. Only I worded it badly.
I'll replace the for loop with a while loop to make it clearer...
>
>> + if (sub->cache_tree->entry_count < 0) {
>> + i -= sub->cache_tree->entry_count;
>> + sub->cache_tree->entry_count = -1;
>> + to_invalidate = 1;
>> + }
>> + else
>> + i += sub->cache_tree->entry_count;
>
> While the rewritten version is not *wrong* per-se, I have a feeling
> that it may be much easier to read if written like this:
>
> if (sub->cache_tree_entry_count < 0) {
> to_invalidate = 1;
> to_skip = 0 - sub->cache_tree_entry_count;
> sub->cache_tree_entry_count = -1;
> } else {
> to_skip = sub->cache_tree_entry_count;
> }
> i += to_skip - 1;
>
..or this would be fine too. Which way to go?
A while we're still at the cache tree
> - if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
> - continue; /* entry being removed or placeholder */
> + /*
> + * CE_REMOVE entries are removed before the index is
> + * written to disk. Skip them to remain consistent
> + * with the future on-disk index.
> + */
> + if (ce->ce_flags & CE_REMOVE)
> + continue;
> +
A CE_REMOVE entry is removed later from the index, however it is still
counted in entry_count. entry_count serves two purposes: to skip the
number of processed entries in the in-memory index, and to record the
number of entries in the on-disk index. These numbers do not match
when CE_REMOVE is present. We have correct in-memory entry_count,
which means incorrect on-disk entry_count in this case.
I tested with an index that has a/b and a/c. The latter has CE_REMOVE.
After writing cache tree I get:
$ git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a/b
$ ../test-dump-cache-tree
878e27c626266ac04087a203e4bdd396dcf74763 (2 entries, 1 subtrees)
878e27c626266ac04087a203e4bdd396dcf74763 #(ref) (1 entries, 1 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a a/ (2 entries, 0 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a #(ref) a/ (1 entries, 0 subtrees)
If I throw out that index, create a new one with a/b alone and write-tree, I get
$ ../test-dump-cache-tree
878e27c626266ac04087a203e4bdd396dcf74763 (1 entries, 1 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a a/ (1 entries, 0 subtrees)
Shall we fix this too? I'm thinking of adding "skip" argument to update_one and
i += sub->cache_tree->entry_count - 1;
would become
i += sub->cache_tree->entry_count + skip - 1;
and entry_count would always reflect on-disk value. This "skip" can be
reused for this i-t-a patch as well.
--
Duy
^ permalink raw reply
* Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized
From: Nguyen Thai Ngoc Duy @ 2012-12-15 3:07 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121214220903.GA18418@sigill.intra.peff.net>
On Sat, Dec 15, 2012 at 5:09 AM, Jeff King <peff@peff.net> wrote:
> I always compile git with "gcc -Wall -Werror", because it catches a lot
> of dubious constructs, and we usually keep the code warning-free.
> However, I also typically compile with "-O0" because I end up debugging
> a fair bit.
>
> Sometimes, though, I compile with -O3, which yields a bunch of new
> "variable might be used uninitialized" warnings. What's happening is
> that as functions get inlined, the compiler can do more static analysis
> of the variables. So given two functions like:
>
> int get_foo(int *foo)
> {
> if (something_that_might_fail() < 0)
> return error("unable to get foo");
> *foo = 0;
> return 0;
> }
>
> void some_fun(void)
> {
> int foo;
> if (get_foo(&foo) < 0)
> return -1;
> printf("foo is %d\n", foo);
> }
>
> If get_foo() is not inlined, then when compiling some_fun, gcc sees only
> that a pointer to the local variable is passed, and must assume that it
> is an out parameter that is initialized after get_foo returns.
>
> However, when get_foo() is inlined, the compiler may look at all of the
> code together and see that some code paths in get_foo() do not
> initialize the variable. And we get the extra warnings.
Other options:
- Any __attribute__ or #pragma to aid flow analysis (or would gcc dev be
willing to add one)?
- Maintain a list of false positives and filter them out from gcc output?
And if we do this, should we support other compilers as well? I tried
clang once a long while ago and got a bunch of warnings iirc.
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Felipe Contreras @ 2012-12-15 3:14 UTC (permalink / raw)
To: Max Horn; +Cc: Junio C Hamano, git
In-Reply-To: <F151B265-7E3E-4989-AA16-EB7CAC298126@quendi.de>
On Fri, Dec 14, 2012 at 7:11 AM, Max Horn <postbox@quendi.de> wrote:
> please stop referring to "facts" and "obvious". You pretend to be a being of pure reason and that everything you say is logical, drawn from facts. But you forget or perhaps do not know that logic by itself proofs nothing, it all depends on the axioms you impose. And yours are quite different from what others consider as such, and apparently also inconsistent.
When I say something is a fact, I do mean it. It's not my opinion,
it's not what I think, it's a fact. The Earth revolves around the Sun,
Junio is the maintainer, Junio can merge whatever he wants, those are
facts.
If you think the Earth revolving around the Sun is not a fact, that's
your choice, but if you want to convince others that this is not a
fact, you need to show *why*.
> So, instead of trying to twist things around so that broken things in your code are not broken after all, why not simply re-roll your patch with the "obvious" fixes applies?
First of all, it depends on your definition of "broken". To me
something broken is something that does not *work*. The code works,
the tests have some cruft in it, but it *works*, it's not broken.
And I said multiple times now that I WILL FIX IT. I'm not going to
repeat it again, and quite frankly, I don't think I'm going to reply
to this thread any more. What makes you think that you owe my free
time? I do with my free time whatever I want. I will fix it, when I
feel like fixing it. This code will not go into 1.8.1, so there's no
point hurrying the fix, I have other things to do.
> As you write yourself, time is not pressing at all -- so I don't see why your patch should be merged now, and fixed later, contrary to how other people's patches are treated? Why not fix them first, and then apply? We do have time, after all! And nobody is expecting you to do that while you are on vacation, either. Nor that you do it instantly.
Time is not pressing because Junio decided not to merge, and he
decided not to merge, because of this "issue". This hurts users, and
benefits no one.
> Just say: "OK, I see there is a problem with the patches; even though I consider it unimportant, I will play by the rules everybody here has to follow, and re-roll the patch series. But this is of low priority for me, so I cannot say right now when it will happen".
That is what I said. What part of "I didn't say it was irrelevant, it
should be fixed" didn't you understand? And no, this is not a "rule",
you assume it *must* be re-rolled, it doesn't.
> Everybody would be happy then. Except perhaps the hypothetical users, who would have to wait a bit longer -- but oh, not really, because they can just use remote-bzr from your repo, yay :-). I really like that about it, it lives in contrib, so one can use it w/o it being merged into git.git.
It's not because of me that users would have to wait, it was Junio's
decision, there was literally nothing I could do, before, or now, for
this code to be merged for 1.8.1. And yes, people can use my repo *if*
they know about it, most people just follow the mainline, and many
don't even read the news.
> Instead, you make claims that make you look like a foolish and arrogant ass, all the while insulting Junio and me implicitly. Why do you do that??? It delays acceptance of your nice work. As you write, this hurts the users. So why do it?
Show me *exactly* where I insulted anybody, and show me where I made a
false claim. And no, this discussion doesn't delay the acceptance, the
acceptance was already delayed before this discussion, there was
nothing I could do, neither in the past or present.
> Since you keep complaining that nobody ever really can point to anything wrong your said, I'll do you the favor by deconstructing one of the claims you made:
Please.
> On 13.12.2012, at 20:06, Felipe Contreras wrote:
>
>> On Thu, Dec 13, 2012 at 6:04 AM, Max Horn <postbox@quendi.de> wrote:
>>>
>>> On 13.12.2012, at 11:08, Felipe Contreras wrote:
>>>
>>>> On Thu, Dec 13, 2012 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>>>> New remote helper for bzr (v3). With minor fixes, this may be ready
>>>>>>> for 'next'.
>>>>>>
>>>>>> What minor fixes?
>>>>>
>>>>> Lookng at the above (fixup), $gmane/210744 comes to mind
>>>>
>>>> That doesn't matter. The code and the tests would work just fine.
>>>
>>>
>>> It doesn't matter? I find that statement hard to align with what the maintainer of git, and thus the person who decides whether your patch series gets merged or not, wrote just above? In fact, it seems to me that what Junio said matters a great deal...
>>
>> So you think Junio knows more about remote-bzr than I do?
>
>
> This is a classical straw man argument.
It was not a straw man argument, in fact, it's not an argument at all,
it's a *question*.
> No, I do not think that. But I do think that Junio knows enough to review your code, and I do think that the point he raised is valid. You disagree with the importance of his point
No, you don't understand. Junio pointed out correctly that the code
didn't "work" in the sense that it didn't do anything, but in fact,
not doing anything was good, because we don't need that code. That
second part Junio did not see, that is a fact. So, my claim that it
didn't matter was based on information Junio did not have, but you
implied that because Junio said it mattered, it did.
Please select to whom does this matter:
a) users of remote-bzr
b) git developers that run make -C t
c) git developers that run make -C contrib/remote-helpers
d) maintainers of remote-bzr
e) Junio
Presumably, the answer is e), so is that really important? I don't
think so, you and Junio, of course, can disagree, but disagreeing is
not insulting. And the fact that very very few people will be affected
negatively if the code is merged is a *fact*, not my opinion.
>> I repeat; it
>> doesn't affect the tests, it doesn't affect the code, it doesn't cause
>> any problem. remote-bzr could be merged today, in fact, it could have
>> been merged a month ago.
>>
>> You don't trust me? Here, look:
>>
> [..]
>
>> All this code is a no-op, because, as Junio pointed out, cmd is null.
>> How is that a problem? It's not.
>
> It is a problem. Because either the code inside the if is important, and then this is a bug. Or it is not important -- then it should not be there in the first place.
Everything is a problem. You can say that the fact that bazaar exists
at all is a problem, and ideally bazaar should disappear, and the
whole remote-bzr gone. The there are problems, and there are
*problems*.
I ask you again, who will be affected negatively by this "problem"?
> Either way, the patch series should be re-rolled.
No. A re-roll is one of the many solutions. Another solution is to
merge first, fix with a separate patch later.
In fact, it was Junio himself that proposed to do later fixes of
remote-bzr on top of what was already merged (v2). Shock horror! This
"broken" code was already merged in 'next', and guess what, nothing
exploded, because, as I said; it's not broken, it's not a big
"problem".
http://article.gmane.org/gmane.comp.version-control.git/210677
> Of course in a whatever time frame suits you. If you are not willing to do that, this is sad, but of course also your right!
Again, as I said multiple times now, I will fix it, eventually.
> [...]
>
>>> This is a very strange attitude...
>>>
>>> In another email, you complained about nobody reviewing your patches respectively nobody voicing any constructive criticism. Yet Junio did just that, and again in $gmane/210745 -- and you replied to neither, and acted on neither (not even by refuting the points brought up), and now summarily dismiss them as irrelevant. I find that quite disturbing :-(.
>>
>> I didn't say it was irrelevant, it should be fixed,
>
> Actually, you wrote:
>
> "That doesn't matter."
>
> So I paraphrased. In any case, I am glad to hear you finally agree that it should be fixed (which you did *not* say in your initial reply). So the problem we have seems to be that you do not understand how patches typically handled in git.git.
Please, spare me the condescension, I think I have enough patches
landed in git.git.
> Well, based on my observation: If reviews point out things in a patch series that are not optimal or even broken, it is expected that the submitter fixes this locally and resubmits a new version of the series. In some cases, it is possible to make exceptions, e.g. trivial typo fixes can be applied on the fly. But otherwise, you re-roll, you do not get your stuff merged just based on the promise that you'll submit a series of fixes later. Esp. if the fixes are relatively easy.
No code is ever perfect. And stop saying "this should be re-rolled",
Junio himself already had this code merged and argued that further
fixes should be *on top*, of what was already there.
I sent v3 on November 11, he made the mistake of picking v2, if he
hadn't done that, remote-bzr would be on track for 1.8.1, the same if
I hadn't asked to pick v3 instead and we continued with v2. The no-op
"problem", might or might not have been spotted, and if it did, Junio
himself might have written and pushed a fix *on top* of what was
already there (not "re-roll"). Even if the problem wasn't spotted,
users of 1.8.1 wouldn't be affected negatively in any way, and I
explained before, *nobody* would have been affected negatively.
Eventually, I might have spotted the issue myself, and sent a patch
*on top*, which might be picked for 1.8.2, 1.8.1.1, or maybe even
1.8.1.
Insisting that this is re-rolled, and that *I* do the fix, is
insisting on a synthetic problem that just wouldn't be there if Junio
hadn't made the mistake of picking v2, or if v2 wasn't reverted.
>> but Junio said
>> "With minor fixes, this may be ready for 'next'." which is no true
>> IMO, it's ready *now*, it was ready one month ago. For 'next', this
>> problem doesn't matter.
>>
>> The feedback is appreciated, but delaying the merging of this code for
>> no reason makes little sense to me.
>
> And here goes the insult. You say Junio has no reason to delay the merging.
That is my opinion. Are you saying that having opinions is insulting?
Or are you saying that voicing my opinion is insulting? Note that I'm
being careful here, and I'm not stating it as a fact, because it's
not. I'm not saying that Junio did something that doesn't make sense,
I said *to me*, it doesn't make much sense. It follows then, that it
might make sense to others, it's a matter of opinion, and having
different opinions is not insulting.
> When you really mean that you don't agree with his reasons.
That is exactly what I said.
> So you attack his professionalism and integrity by alluding that he has some ulterior motives to delay the patch.
I never said anything like that. This, actually, is a prime example of
what a straw man argument looks like.
> E.g. that he is hates you, is just mean, does it out of stubbornness, etc.
Don't put words on my mouth.
>> Junio, of course, can do whatever he wants. The removal of this no-op code can wait, or it can be done
>> on top of v3, there's no need for re-roll, and Junio already
>> complained about the v3 re-roll.
>>
>> And I didn't act because I was on vacations, git development is not my
>> only priority.
>
> Of course. Nobody is complaining that you take too long to reply. We are just unhappy in the way you reply when you do reply :-(.
I know. But I'm only stating facts. The code could be merged to 'next'
today, that is a fact. And in fact, it was already merged in 'next'.
>> And even if I had time, I don't see why I should
>> prioritize this fix, it's not important, the code is ready.
>
> Another straw man: Nobody asked you to prioritize the fix, take your time.
> It was you who asked that the series should be applied without any further fixes.
I didn't ask for anything, I said the series *could* be applied
without any further fixes, the fixes can come next.
And I repeat, this "broken" code was already applied! Hadn't I asked
Junio to pick v3 of the series instead, the fix would have to come as
a separate patch *on top* of what was already merged.
You keep assuming that there's only one way to apply the fix, as a
re-roll, stop doing that.
>>>>> but there may be others. It is the responsibility of a contributor to keep
>>>>> track of review comments others give to his or her patches and
>>>>> reroll them, so I do not recall every minor details, sorry.
>>>>
>>>> There is nothing that prevents remote-bzr from being merged.
>>>
>>> Well, I think that is up to Junio to decide in the end, though :-). He wrote
>>
>> No. He can decide if the code gets merged, but he is not the voice of
>> truth. Nothing prevents him from merging the code, except himself.
>> There is no known issue with the code, that is a true fact.
>
> Here are a multitude of fallacies hidden, partially explainable by a differing set of axioms, and/or shear arrogance.
>
> Let us re-reread what was said: Initially, you claimed that "There is nothing that prevents remote-bzr from being merged".
>
> Now, it wasn't said in the above, but let me make it explicit: This statement is "obviously"[1] wrong. There are parts of the patch series Junio thinks are not up to par, and he made it quite clear that he will not merge it until these things are resolved.
That is his *choice*, but nothing is preventing him from merging.
You are not looking at the claim objectively. If I'm on top of a
skyscraper and say "there's nothing that prevents me from jumping
down, except myself" that might be true, even if I have no intentions
of jumping, because, in fact, the reasons for not jumping are mine.
However, the top of the skyscraper might be surrounded by a glass
wall, in which case, there is something that would prevent me from
jumping, other than myself.
Of course, completely objectively, the claim that Junio can merge it,
doesn't say much, merely that he has commit access, that github is not
down, and so on. It just says that *physically* Junio is not
constrained from merging. But if you take a step further, it means
that among the guidelines of what constitutes reasons for merging, or
not merging something, this series does in fact fulfill those reasons.
But ultimately the proof that there was nothing preventing Junio from
merging to 'next' is that *HE ALREADY DID IT*:
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=ad38af72b334150e6cf1978721c37077ae3c6d7f
See? Now tell me that my claim is wrong, and he could in fact not do
it, if the already did.
> Hence, ignoring all else, there obviously *is* something that prevents remote-bzr from being merged. That is a "fact". You even admit so yourself, also contradicting yourself:
>
> "Nothing prevents him from merging the code, except himself."
>
>
> So how is it possible that you can claim that there is nothing that prevents the merge? Ignoring the self-contradicting aspects of what you wrote, the basis for your differing conclusion seems to be that you change the terms of discussion and are using a different set of axioms. In particular, you apparently redefine
> "things that prevent remote-bzr from being merged"
> as
> "things that in Felipe's view prevent remote-bzr from being merged".
No. No. No. No.
It's not me that makes something mergeable or not. It's not me who
determines if there's something that prevents a person from jumping
from a skyscraper. Either there are walls, or there are not.
And I don't think you know what an axiom means.
Either way, even if my claim was wrong (it isn't, because Junio
already did it in the past), it wouldn't be a fallacy.
> Of course one can arbitrarily bend the rules by this definition. For example, we could redefine "nothing prevents the merge" as
> "no technical reasons prevent the merge", and the latter is indeed quite true; your patch series applies perfectly fine, git can do that. Of course the same holds for a patch which removes git.c from the repository, so I don't think this definition is particularly useful...
You go to great extents to say that my claim is wrong, and then you
say: except if... Sorry, that's not logic works.
> This is something what a lot of people would consider a strong insult towards the professionalism and integrity of Junio. There are more examples of this in previous communications between you and other people in the list.
So you do think disagreeing is insulting. Good to know. I disagree.
> You finally add "There is no known issue with the code, that is a true fact.". Within your axiom set, this is certainly true. It certainly is not true in mine or Junio's... Yet you very strongly emphasis with your statement that your set of axioms is the correct one to use here, although I would guess that most people would disagree.
Now it's obvious that you don't know what axiom means:
"An axiom is a premise or starting point of reasoning. As classically
conceived, an axiom is a premise so evident as to be accepted as true
without controversy."
If there's controversy, it's not an axiom, because if you change
axioms, you can argue *anything*.
No, it is a fact even with commonly shared axioms. Tell me, what
outstanding issues with this series are going to affect negatively
anybody? That is the only way you can dispute my claim, by providing
actual, real, objective issues.
> This is especially arrogant in view of the another straw man argument you are employing: By writing "[Junio] he is not the voice of truth.", you implicate that I or anybody were of this opinion.
I did not imply that. If it's not clear, objective issues either
exist, or they don't. If there are no objective issues, Junio's
thoughts can't make them appear.
> But I am not, and what I wrote cannot logically be construed as saying so. At least not within what most people would consider as axiom set; of course if your axiom set includes "Max believes Junio is the voice of truth", your claim because truth, albeit a tautological one. But let me make clear that any such axioms, or set of axioms leading to that implicating, are inconsistent: In my view, of course Junio is fallible and makes mistakes, and can be wrong etc. -- like any human being. Including most definitely me and you.
Again with the shady definition of axioms. The truth is the truth, it
doesn't matter what you, Junio, or I, say; the truth remains the same.
Axioms are things that can't possibly be wrong, not what anybody
considers is the truth.
And even if you were correct in all that, you haven't provided a
single possible fallacy. Here is a list:
http://en.wikipedia.org/wiki/List_of_fallacies
I haven't seen you arguing that I committed any of those.
> This is a horrible way of working within a team effort :-(. I find this a great pity, because I believe you are doing some really nice work, I esp. like your remote-hg which works much better for me than the others I tried so far.
I'm not going to voice my opinion on this, because to you, apparently
voicing my opinion is insulting.
> [1] As a mathematician, I was taught to avoid the word "obvious" in any written form of proof, as it makes you sound arrogant, and it also discourages the reader from thinking critical about a statement, which is considered extremely bad. But since you like it so much, I am using here on purpose.
We are not writing mathematical proofs, and even if we were, avoiding
the word "obvious" would be a guideline, right? Your paper wouldn't be
rejected if you did use it.
I'm going to say it one last time; merging this patch series either
creates issues for the users, or not. There is a reality out there,
independent of what you, Junio, or me think or say. And the fact is,
that if this patch series is going to create issues for the users,
*nobody* has pointed out why, so, since there's no evidence for it,
the only rational thing to do is believe that there will be no issues
for the users.
There is no known issue with the code, that is a fact. This code could
be easily merged today, and in fact, it was merged by Junio already
(but then reverted). There are no positive outcomes from the delay,
only negative ones. I will address the minute issue about the extra
cruft, eventually.
I'm not going to reply to this thread any more, because when you have
to explain in a discussion what 'axiom' means, you know the points of
view could hardly be more distant, and it would take a lifetime to
converge. Same for what a fallacy is, what an argument is, and what
straw man means.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: FW: Git log --graph doesn't output color when redirected
From: Nguyen Thai Ngoc Duy @ 2012-12-15 3:23 UTC (permalink / raw)
To: Jeff King; +Cc: Srb, Michal, git@vger.kernel.org
In-Reply-To: <20121213131329.GA5042@sigill.intra.peff.net>
On Thu, Dec 13, 2012 at 8:13 PM, Jeff King <peff@peff.net> wrote:
> If you are using --format="%C(red)" or similar placeholders,
> they are the odd duck by not respecting the auto-color mode.
But they should, shouldn't they? Just asking. I may do it to when I
revive nd/pretty-placeholder-with-color-option.
--
Duy
^ permalink raw reply
* Unexpected behaviour when trying to merge two new repositories.
From: Ben Aveling @ 2012-12-15 5:18 UTC (permalink / raw)
To: git
Hi,
I have a directory of files that have been copied between different
machines, and then drifted apart.
I was trying to merge the different versions into a single unified
repository.
I succeeded, but I had to navigate though, and recover from, what seems
to me to be counter-intuitive behaviour.
Let me walk through a mock up of what I did.
Scenario 1. Merging two repositories.
Mock up first directory, and commit it.
> mkdir a
> git init a
Initialized empty Git repository in /tmp/a/.git/
> cd a
> echo 1 > 1
> echo 2 > 2
> git add .
> git commit -m "add 1 and 2"
[master (root-commit) 9a649ab] add 1 and 2
2 files changed, 2 insertions(+), 0 deletions(-)
create mode 100644 1
create mode 100644 2
Mock up second directory. Add files, but don't commit.
> mkdir b
> cd ../b
> git init
Initialized empty Git repository in /tmp/b/.git/
> echo 1 > 1
> echo 3 > 3
> git pull ../a
remote: Counting objects: 4, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 4 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (4/4), done.
From ../a
* branch HEAD -> FETCH_HEAD
error: Untracked working tree file '1' would be overwritten by merge.
> git add 1 3
Now merge in the first directory.
> git pull ../a master
From ../a
* branch master -> FETCH_HEAD
Already up-to-date.
> git status
# On branch master
# Changes to be committed:
# (use "git reset HEAD <file>..." to unstage)
#
# deleted: 2
# new file: 3
#
This surprised me. I wasn't expecting 2 to be deleted. And I wasn't
expecting to be told that it was "Already up to date".
Scenario 2. Merging two repositories with everything committed.
Mock up a third directory. Add files and this time, commit. Then merge
from a.
> mkdir ../c
> cd ../c
> git init
Initialized empty Git repository in /tmp/c/.git/
> echo 1 > 1
> echo 4 > 4
> git add 1 4
> git commit -m "add 1 and 4"
[master (root-commit) b6081d9] add 1 and 4
2 files changed, 2 insertions(+), 0 deletions(-)
create mode 100644 1
create mode 100644 4
> git pull ../a master
warning: no common commits
remote: Counting objects: 4, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 4 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (4/4), done.
From ../a
* branch master -> FETCH_HEAD
Merge made by recursive.
2 | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 2
> ll *
-rw-rw-r-- 1 ben ben 2 2012-12-15 15:02 1
-rw-rw-r-- 1 ben ben 2 2012-12-15 15:10 2
-rw-rw-r-- 1 ben ben 2 2012-12-15 15:03 4
This is what I was expecting the first time.
It seems that if there are no commits in the new repository, git doesn't
detect that there are no common commits.
Scenario 3. Merge with uncommitted changes.
Create another mock directory. Create some of the same files, but with
different contents. Again, add them but don't commit.
> mkdir ../d
> cd ../d
> git init
Initialized empty Git repository in /tmp/d/.git/
> echo 1a > 1
> echo 5 > 5
> git add 1 5
> git commit -m "add 1 and 5"
[master (root-commit) b6081d9] add 1 and 5
2 files changed, 2 insertions(+), 0 deletions(-)
create mode 100644 1
create mode 100644 5
> git pull ../a master
remote: Counting objects: 4, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 4 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (4/4), done.
From ../a
* branch master -> FETCH_HEAD
> git status
# On branch master
nothing to commit (working directory clean)
> ls -l
total 8
-rw-rw-r-- 1 ben ben 2 2012-12-15 15:31 1
-rw-rw-r-- 1 ben ben 2 2012-12-15 15:31 2
This too, surprised me. This time, the incoming file was added where
before it was deleted. And file 5 has vanished.
> git fsck
dangling blob a8994dc188ebbbc9e8a885470651a7fbb9127528
dangling blob 7ed6ff82de6bcc2a78243fc9c54d3ef5ac14da69
> cat 1
1
> git cat-file -p a8994dc188ebbbc9e8a885470651a7fbb9127528
1a
> git cat-file -p 7ed6ff82de6bcc2a78243fc9c54d3ef5ac14da69
5
In fact, both file 1 and file 5 have been 'lost'. It's not obvious to
me why this happens, except that it seems to be a feature of the need to
merge.
This seems inconsistent. In one case, we have merged all 3 files. In one
case, we have deleted incoming files. And in one case, we have 'lost'
files that were already in the repository.
In one way, this is a slightly obscure edge case - merging into a new
repository and then adding but not committing the files. But the people
most likely to do this are new git users - the ones who have to work
hardest to recover the deleted/lost files.
Regards, Ben
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Michael Haggerty @ 2012-12-15 7:09 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Max Horn, Junio C Hamano, git
In-Reply-To: <CAMP44s0r_KAKt7Lm1cdumN1cOWzjab3ruYqxp-s6OR1g1qqbcQ@mail.gmail.com>
On 12/15/2012 04:14 AM, Felipe Contreras wrote:
> I'm going to say it one last time; merging this patch series either
> creates issues for the users, or not. There is a reality out there,
> independent of what you, Junio, or me think or say. And the fact is,
> that if this patch series is going to create issues for the users,
> *nobody* has pointed out why, so, since there's no evidence for it,
> the only rational thing to do is believe that there will be no issues
> for the users.
>
> There is no known issue with the code, that is a fact. This code could
> be easily merged today, and in fact, it was merged by Junio already
> (but then reverted). There are no positive outcomes from the delay,
> only negative ones. I will address the minute issue about the extra
> cruft, eventually.
Cruft in the codebase is a problem for git *developers* because it makes
the code harder to maintain and extend.
And therefore cruft is a problem for git *users* because it slows down
future development (in whatever small amount).
Moreover, it is dangerous for a project to accept crufty code based on a
contributor's promise to clean up the code later:
* The developer might not get around to it, or might take longer than
expected.
* Until it is cleaned up, the cruft hinders other potential developers
to that code.
* The presence of cruft lowers the expectation of quality for the whole
project; cruft breeds more cruft.
It is simpler and fairer to have a policy "no crufty code" than to try
to evaluate each instance on a case-by-case basis.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Felipe Contreras @ 2012-12-15 7:45 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Max Horn, Junio C Hamano, git
In-Reply-To: <50CC2244.4040103@alum.mit.edu>
On Sat, Dec 15, 2012 at 1:09 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 12/15/2012 04:14 AM, Felipe Contreras wrote:
>> I'm going to say it one last time; merging this patch series either
>> creates issues for the users, or not. There is a reality out there,
>> independent of what you, Junio, or me think or say. And the fact is,
>> that if this patch series is going to create issues for the users,
>> *nobody* has pointed out why, so, since there's no evidence for it,
>> the only rational thing to do is believe that there will be no issues
>> for the users.
>>
>> There is no known issue with the code, that is a fact. This code could
>> be easily merged today, and in fact, it was merged by Junio already
>> (but then reverted). There are no positive outcomes from the delay,
>> only negative ones. I will address the minute issue about the extra
>> cruft, eventually.
Couple of facts first:
a) This code was already merged
b) This code is for a test
c) I'm the only developer so far
> Cruft in the codebase is a problem for git *developers* because it makes
> the code harder to maintain and extend.
A problem big enough to warrant the rejection of the patch series? No.
> And therefore cruft is a problem for git *users* because it slows down
> future development (in whatever small amount).
Don't confuse potential issues with real ones. It *might* slow down
future development, but will it do it with absolute certainty and
beyond any reasonable doubt? No, it might not slow anything at all.
And even if it does, by how much? 50%? 10%? 1%? Chances are it would
be barely noticeable to the users.
And even if it was substantial, this is on *test* code. Most users
survive just fine with most of the contrib code not having tests at
all, they can probably survive with the development of the test code
for remote-bzr being a tad slower.
But who are these developers that would be slowed down? So far I'm the
only contributor, and I'm not going to be slowed. If and when somebody
else contributes, and find his or her development is slowed down by
this, he or her would probably start by removing that code his or
herself, and submit the appropriate patch.
> Moreover, it is dangerous for a project to accept crufty code based on a
> contributor's promise to clean up the code later:
But it was already accepted:
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=ad38af72b334150e6cf1978721c37077ae3c6d7f
The world didn't end the first time, presumably, if this code is
accepted again, the world will not end either.
> * The developer might not get around to it, or might take longer than
> expected.
Somebody else could do it. This is collaborative development after
all, is it not?
I don't see people halting because something is somebody else's code.
> * Until it is cleaned up, the cruft hinders other potential developers
> to that code.
How many *potential* developers are we talking about? By how much?
> * The presence of cruft lowers the expectation of quality for the whole
> project; cruft breeds more cruft.
Please. This is in test code for the contrib area, most code in that
area doesn't even have tests.
> It is simpler and fairer to have a policy "no crufty code" than to try
> to evaluate each instance on a case-by-case basis.
Even then, the problem can be easily solved by simply removing the
whole file (contrib/remote-helpers/test-bzr.sh), I say that has more
potential to hurt users and developers, but hey, "no crufty code".
Since most code in the contrib area doesn't have tests, we would still
be following the "policy".
None of this benefits the *real* users one iota.
Anyway, these theoretical minute problems aren't worth worrying about,
nor discussing. If you want to damage real users, go ahead.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* RE: [PATCH 4/4] Declare that HP NonStop systems require strings.h
From: Joachim Schmitz @ 2012-12-15 8:25 UTC (permalink / raw)
To: 'Johannes Sixt'; +Cc: gitster, fedora.dm0, 'Git Mailing List'
In-Reply-To: <50CBB34E.8090609@kdbg.org>
> From: Johannes Sixt [mailto:j6t@kdbg.org]
> Sent: Saturday, December 15, 2012 12:17 AM
> To: Joachim Schmitz
> Cc: gitster@pobox.com; fedora.dm0@gmail.com; Git Mailing List
> Subject: Re: [PATCH 4/4] Declare that HP NonStop systems require strings.h
>
> Am 14.12.2012 23:46, schrieb Joachim Schmitz:
> > Johannes Sixt wrote:
> >> Am 14.12.2012 20:57, schrieb David Michael:
> >>> This platform previously included strings.h automatically. However,
> >>> the build system now requires an explicit option to do so.
> >>>
> >>> Signed-off-by: David Michael <fedora.dm0@gmail.com>
> >>> ---
> >>> Makefile | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/Makefile b/Makefile
> >>> index fb78f7f..e84b0cb 100644
> >>> --- a/Makefile
> >>> +++ b/Makefile
> >>> @@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> >>> # Added manually, see above.
> >>> NEEDS_SSL_WITH_CURL = YesPlease
> >>> HAVE_LIBCHARSET_H = YesPlease
> >>> + HAVE_STRINGS_H = YesPlease
> >>> NEEDS_LIBICONV = YesPlease
> >>> NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
> >>> NO_SYS_SELECT_H = UnfortunatelyYes
> >>
> >> If NONSTOP_KERNEL is the platform that defines __TANDEM, then this
> >> should be squashed into the previous patch, shouldn't it?
> >
> > Patch 4/4 does not work without 3/4, Not for HP-NonStop.
>
> That is clear from the order of the patches in the series.
>
> To put it in a different way: Do all supported platforms still work
> after 3/4, but without 4/4?
Sorry I didn't make myself clear, I left out a "and vice versa"
Bye, Jojo
^ permalink raw reply
* [Patch] Renumber list in api-command.txt
From: Thomas Ackermann @ 2012-12-15 8:29 UTC (permalink / raw)
To: git
In-Reply-To: <1479174763.154268.1350408444997.JavaMail.ngmail@webmail15.arcor-online.net>
- Start list with 1 instead of 0 because ASCIIDOC will renumber it anyway
Signed-off-by: Thomas Ackermann <th.acker@arcor.de>
---
Documentation/technical/api-command.txt | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/Documentation/technical/api-command.txt b/Documentation/technical/api-command.txt
index ea9b2ed..d3b9781 100644
--- a/Documentation/technical/api-command.txt
+++ b/Documentation/technical/api-command.txt
@@ -71,28 +71,28 @@ Integrating a command
Here are the things you need to do when you want to merge a new
subcommand into the git tree.
-0. Don't forget to sign off your patch!
+1. Don't forget to sign off your patch!
-1. Append your command name to one of the variables BUILTIN_OBJS,
+2. Append your command name to one of the variables BUILTIN_OBJS,
EXTRA_PROGRAMS, SCRIPT_SH, SCRIPT_PERL or SCRIPT_PYTHON.
-2. Drop its test in the t directory.
+3. Drop its test in the t directory.
-3. If your command is implemented in an interpreted language with a
+4. If your command is implemented in an interpreted language with a
p-code intermediate form, make sure .gitignore in the main directory
includes a pattern entry that ignores such files. Python .pyc and
.pyo files will already be covered.
-4. If your command has any dependency on a particular version of
+5. If your command has any dependency on a particular version of
your language, document it in the INSTALL file.
-5. There is a file command-list.txt in the distribution main directory
+6. There is a file command-list.txt in the distribution main directory
that categorizes commands by type, so they can be listed in appropriate
subsections in the documentation's summary command list. Add an entry
for yours. To understand the categories, look at git-cmmands.txt
in the main directory.
-6. Give the maintainer one paragraph to include in the RelNotes file
+7. Give the maintainer one paragraph to include in the RelNotes file
to describe the new feature; a good place to do so is in the cover
letter [PATCH 0/n].
--
1.8.0.msysgit.0
---
Thomas
^ permalink raw reply related
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Nguyen Thai Ngoc Duy @ 2012-12-15 8:44 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Michael Haggerty, Max Horn, Junio C Hamano, git
In-Reply-To: <CAMP44s0=R3rdnD-Zpzz_7wY6HKKsAL1sPVYh4pc3z1CBbX2ODg@mail.gmail.com>
On Sat, Dec 15, 2012 at 2:45 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Couple of facts first:
> a) This code was already merged
> b) This code is for a test
> c) I'm the only developer so far
>
>> Cruft in the codebase is a problem for git *developers* because it makes
>> the code harder to maintain and extend.
>
> A problem big enough to warrant the rejection of the patch series? No.
May I suggest you maintain remote-bzr as a separate project? You have
total control that way without anyone's disagreeing with you. So you
may be more productive and we have less of these emails back and
forth. And speaking from someone whose series may take months to get
in, why the rush?
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Felipe Contreras @ 2012-12-15 9:24 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Michael Haggerty, Max Horn, Junio C Hamano, git
In-Reply-To: <CACsJy8A=-3-C29LKQasfUih24cSZrRuQRJ28WjP0zKg=NaFuUA@mail.gmail.com>
On Sat, Dec 15, 2012 at 2:44 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Sat, Dec 15, 2012 at 2:45 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>>> Cruft in the codebase is a problem for git *developers* because it makes
>>> the code harder to maintain and extend.
>>
>> A problem big enough to warrant the rejection of the patch series? No.
>
> May I suggest you maintain remote-bzr as a separate project? You have
> total control that way without anyone's disagreeing with you.
Which disagreement? We have all agreed how the code should look like
in the end. The disagreement is on how and when this code should be
merged to git.git. A separate project is not going to help there.
> And speaking from someone whose series may take months to get
> in, why the rush?
Why the delay? Junio had already merged this code to 'next'.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized
From: Jeff King @ 2012-12-15 10:09 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8BqOEvEHy7i89fKSgQH5kUYFWvchJwD_fQsYjagrh+X2w@mail.gmail.com>
On Sat, Dec 15, 2012 at 10:07:54AM +0700, Nguyen Thai Ngoc Duy wrote:
> > If get_foo() is not inlined, then when compiling some_fun, gcc sees only
> > that a pointer to the local variable is passed, and must assume that it
> > is an out parameter that is initialized after get_foo returns.
> >
> > However, when get_foo() is inlined, the compiler may look at all of the
> > code together and see that some code paths in get_foo() do not
> > initialize the variable. And we get the extra warnings.
>
> Other options:
>
> - Any __attribute__ or #pragma to aid flow analysis (or would gcc dev be
> willing to add one)?
I looked through the full list of __attribute__ flags and couldn't find
anything that would help.
> - Maintain a list of false positives and filter them out from gcc output?
I think it would be just as simple to use the "int foo = foo" hack,
which accomplishes the same thing without any post-processing step.
> And if we do this, should we support other compilers as well? I tried
> clang once a long while ago and got a bunch of warnings iirc.
I don't use clang myself, but I don't have any problem with other people
submitting patches to clean up its warnings, provided they don't make
the code harder to read or write.
-Peff
^ permalink raw reply
* Re: FW: Git log --graph doesn't output color when redirected
From: Jeff King @ 2012-12-15 10:16 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Srb, Michal, git@vger.kernel.org
In-Reply-To: <CACsJy8AgtwtJfMXzArJLiHQtR+WNRJxRdRgUts30EN-QvgGT=g@mail.gmail.com>
On Sat, Dec 15, 2012 at 10:23:10AM +0700, Nguyen Thai Ngoc Duy wrote:
> On Thu, Dec 13, 2012 at 8:13 PM, Jeff King <peff@peff.net> wrote:
> > If you are using --format="%C(red)" or similar placeholders,
> > they are the odd duck by not respecting the auto-color mode.
>
> But they should, shouldn't they? Just asking. I may do it to when I
> revive nd/pretty-placeholder-with-color-option.
If I were designing --format today, I would certainly say so. The only
thing holding me back would be backwards compatibility. We could get
around that by introducing a new placeholder like %c(color) that behaves
like %C(color), except respects the --color flag.
It looks like this came up before:
http://article.gmane.org/gmane.comp.version-control.git/169225
but never got pushed to inclusion.
-Peff
^ permalink raw reply
* Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized
From: Johannes Sixt @ 2012-12-15 10:49 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121214220903.GA18418@sigill.intra.peff.net>
Am 14.12.2012 23:09, schrieb Jeff King:
> Can anybody think of a clever way to expose the constant return value of
> error() to the compiler? We could do it with a macro, but that is also
> out for error(), as we do not assume the compiler has variadic macros. I
> guess we could hide it behind "#ifdef __GNUC__", since it is after all
> only there to give gcc's analyzer more information. But I'm not sure
> there is a way to make a macro that is syntactically identical. I.e.,
> you cannot just replace "error(...)" in "return error(...);" with a
> function call plus a value for the return statement. You'd need
> something more like:
>
> #define RETURN_ERROR(fmt, ...) \
> do { \
> error(fmt, __VA_ARGS__); \
> return -1; \
> } while(0) \
>
> which is awfully ugly.
Does
#define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)
cause problems when not used in a return statement?
-- Hannes
^ permalink raw reply
* Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized
From: Jeff King @ 2012-12-15 11:09 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <50CC55B5.8000205@kdbg.org>
On Sat, Dec 15, 2012 at 11:49:25AM +0100, Johannes Sixt wrote:
> Am 14.12.2012 23:09, schrieb Jeff King:
> > Can anybody think of a clever way to expose the constant return value of
> > error() to the compiler? We could do it with a macro, but that is also
> > out for error(), as we do not assume the compiler has variadic macros. I
> > guess we could hide it behind "#ifdef __GNUC__", since it is after all
> > only there to give gcc's analyzer more information. But I'm not sure
> > there is a way to make a macro that is syntactically identical. I.e.,
> > you cannot just replace "error(...)" in "return error(...);" with a
> > function call plus a value for the return statement. You'd need
> > something more like:
> >
> > #define RETURN_ERROR(fmt, ...) \
> > do { \
> > error(fmt, __VA_ARGS__); \
> > return -1; \
> > } while(0) \
> >
> > which is awfully ugly.
>
> Does
>
> #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)
>
> cause problems when not used in a return statement?
Thanks, that was the cleverness I was missing. The only problem is that
in standard C, doing this:
error("no other arguments");
generates:
(error_impl(fmt, ), 1);
which is bogus. This is a common problem with variadic macros, and
fortunately gcc has a solution (and since we are already inside a
gcc-only #ifdef, we should be OK).
So doing this works for me:
diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..a036323 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -285,9 +285,18 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
+#ifdef __GNUC__
+#define ERROR_FUNC_NAME error_impl
+#define error(fmt, ...) (error_impl((fmt), ##__VA_ARGS__), -1)
+#else
+#define ERROR_FUNC_NAME error
+#endif
+
+extern int ERROR_FUNC_NAME(const char *err, ...)
+__attribute__((format (printf, 1, 2)));
+
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
extern void set_error_routine(void (*routine)(const char *err, va_list params));
diff --git a/usage.c b/usage.c
index 8eab281..d1a58fa 100644
--- a/usage.c
+++ b/usage.c
@@ -130,7 +130,7 @@ void NORETURN die_errno(const char *fmt, ...)
va_end(params);
}
-int error(const char *err, ...)
+int ERROR_FUNC_NAME(const char *err, ...)
{
va_list params;
I think we could even get rid of the ERROR_FUNC_NAME ugliness by just
calling it "error", and doing an "#undef error" right before we define
it in usage.c.
-Peff
^ permalink raw reply related
* Re: [PATCH 1/3] remote-testsvn: fix unitialized variable
From: Florian Achleitner @ 2012-12-15 10:29 UTC (permalink / raw)
To: Jeff King; +Cc: git, Florian Achleitner
In-Reply-To: <20121214221144.GA19677@sigill.intra.peff.net>
On Friday 14 December 2012 17:11:44 Jeff King wrote:
> [...]
> We can fix it by returning "-1" when no note is found (so on
> a zero return, we always found a valid value).
Good fix. Parsing of the note now always fails if the note doesn't contain the
expected string, as it should.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I think this is the right fix, but I am not too familiar with this code,
> so I might be missing a case where a missing "Revision-number" should
> provide some sentinel value (like "0") instead of returning an error. In
> fact, of the two callsites, one already does such a zero-initialization.
>
> remote-testsvn.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/remote-testsvn.c b/remote-testsvn.c
> index 51fba05..5ddf11c 100644
> --- a/remote-testsvn.c
> +++ b/remote-testsvn.c
> @@ -90,10 +90,12 @@ static int parse_rev_note(const char *msg, struct
> rev_note *res) if (end == value || i < 0 || i > UINT32_MAX)
> return -1;
> res->rev_nr = i;
> + return 0;
> }
> msg += len + 1;
> }
> - return 0;
> + /* didn't find it */
> + return -1;
> }
>
> static int note2mark_cb(const unsigned char *object_sha1,
^ permalink raw reply
* Re: possible Improving diff algoritm
From: Javier Domingo @ 2012-12-15 12:16 UTC (permalink / raw)
To: Bernhard R. Link; +Cc: Junio C Hamano, Geert Bosch, Morten Welinder, Kevin, git
In-Reply-To: <20121214222938.GB19410@client.brlink.eu>
If just empty lines alone, then it is forced to say there is a new
line. That is beyond this use-case.
Javier Domingo
2012/12/14 Bernhard R. Link <brl+git@mail.brlink.eu>:
> * Javier Domingo <javierdo1@gmail.com> [121214 13:20]:
>> I think the idea of being preferable to have a blank line at the end
>> of the added/deleted block is key in this case.
>
> For symmetry I'd suggest to make it preferable to have blank lines
> at the end or the beginning.
>
> {
> old
> + }
> +
> + {
> + new
> }
>
> vs
>
> {
> old
> }
> +
> + {
> + new
> + }
>
> is just the same case in blue.
> (Although empty lines alone feels not quite optimal, it is at least a
> good start).
>
> Bernhard R. Link
^ permalink raw reply
* [PATCH] Makefile: detect when PYTHON_PATH changes
From: Christian Couder @ 2012-12-15 14:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
When make is run the python script are created from *.py files that
are changed to use the python given by PYTHON_PATH. And PYTHON_PATH
is set by default to /usr/bin/python on Linux.
This is nice except when you run make another time setting a
different PYTHON_PATH, because, as the python scripts have already
been created, make finds nothing to do.
The goal of this patch is to detect when the PYTHON_PATH changes and
to create the python scripts again when this happens. To do that we
use the same trick that is done to track prefix, flags and tcl/tk
path. We update a GIT-PYTHON-VARS file with the PYTHON_PATH and check
if it changed.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Makefile | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 4ad6fbd..bd86063 100644
--- a/Makefile
+++ b/Makefile
@@ -2245,7 +2245,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
endif # NO_PERL
ifndef NO_PYTHON
-$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX
+$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
$(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
$(QUIET_GEN)$(RM) $@ $@+ && \
INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
@@ -2636,6 +2636,18 @@ GIT-GUI-VARS: FORCE
fi
endif
+### Detect Python interpreter path changes
+ifndef NO_PYTHON
+TRACK_VARS = $(subst ','\'',-DPYTHON_PATH='$(PYTHON_PATH_SQ)')
+
+GIT-PYTHON-VARS: FORCE
+ @VARS='$(TRACK_VARS)'; \
+ if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
+ echo 1>&2 " * new Python interpreter location"; \
+ echo "$$VARS" >$@; \
+ fi
+endif
+
test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
all:: $(TEST_PROGRAMS) $(test_bindir_programs)
--
1.8.1.rc1.1.g9537e17
^ permalink raw reply related
* [PATCH] Documentation: don't link to example mail addresses
From: John Keeping @ 2012-12-15 15:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Mail addresses in documentation are converted into mailto: hyperlinks in
the HTML output and footnotes in man pages. This isn't desirable for
cases where the address is used as an example and is not valid.
Particularly annoying is the example "jane@laptop.(none)" which appears
in git-shortlog(1) as "jane@laptop[1].(none)", with note 1 saying:
1. jane@laptop
mailto:jane@laptop
Fix this by quoting example mail addresses with "$$", preventing
Asciidoc from processing them.
In the case of mailmap.txt, render the address monospaced so that it
matches the block examples surrounding that paragraph.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
I can't see any other uses of the "$$" quote in the documentation, so
it's probably worth noting that I've tested this with Asciidoc 8.6.8,
although I can't see anything in the changelog to indicate that
Asciidoc's treatment of it has changed recently.
Documentation/git-fast-import.txt | 2 +-
Documentation/git-tag.txt | 2 +-
Documentation/mailmap.txt | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index d1844ea..05913cc 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -427,7 +427,7 @@ they made it.
Here `<name>` is the person's display name (for example
``Com M Itter'') and `<email>` is the person's email address
-(``cm@example.com''). `LT` and `GT` are the literal less-than (\x3c)
+(``$$cm@example.com$$''). `LT` and `GT` are the literal less-than (\x3c)
and greater-than (\x3e) symbols. These are required to delimit
the email address from the other fields in the line. Note that
`<name>` and `<email>` are free-form and may contain any sequence
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 247534e..ed63edb 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -129,7 +129,7 @@ This option is only applicable when listing tags without annotation lines.
CONFIGURATION
-------------
By default, 'git tag' in sign-with-default mode (-s) will use your
-committer identity (of the form "Your Name <your@email.address>") to
+committer identity (of the form "Your Name $$<your@email.address>$$") to
find a key. If you want to use a different default key, you can specify
it in the repository configuration as follows:
diff --git a/Documentation/mailmap.txt b/Documentation/mailmap.txt
index 288f04e..dd89fca 100644
--- a/Documentation/mailmap.txt
+++ b/Documentation/mailmap.txt
@@ -46,7 +46,7 @@ Jane Doe <jane@desktop.(none)>
Joe R. Developer <joe@example.com>
------------
-Note how there is no need for an entry for <jane@laptop.(none)>, because the
+Note how there is no need for an entry for `<jane@laptop.(none)>`, because the
real name of that author is already correct.
Example 2: Your repository contains commits from the following
--
1.8.0
^ permalink raw reply related
* Re: [PATCH] Documentation: don't link to example mail addresses
From: Jeff King @ 2012-12-15 17:20 UTC (permalink / raw)
To: John Keeping; +Cc: git, Junio C Hamano
In-Reply-To: <20121215150314.GC2725@river.lan>
On Sat, Dec 15, 2012 at 03:03:15PM +0000, John Keeping wrote:
> Mail addresses in documentation are converted into mailto: hyperlinks in
> the HTML output and footnotes in man pages. This isn't desirable for
> cases where the address is used as an example and is not valid.
>
> Particularly annoying is the example "jane@laptop.(none)" which appears
> in git-shortlog(1) as "jane@laptop[1].(none)", with note 1 saying:
>
> 1. jane@laptop
> mailto:jane@laptop
Thanks, this is definitely worth fixing.
> Fix this by quoting example mail addresses with "$$", preventing
> Asciidoc from processing them.
>
> In the case of mailmap.txt, render the address monospaced so that it
> matches the block examples surrounding that paragraph.
I think I'd just render them monospace everywhere. We are very
inconsistent about which form of quotes we use in the documentation (I
think because most of the developers read the source directly and not
the rendered asciidoc). And then we don't have to worry about the "$$"
construct (and IMHO it makes the source much more readable, and marking
the address as a literal looks good in the output, too).
-Peff
^ permalink raw reply
* Re: [PATCH] Makefile: detect when PYTHON_PATH changes
From: Junio C Hamano @ 2012-12-15 17:29 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <20121215140719.2409.27365.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
> @@ -2636,6 +2636,18 @@ GIT-GUI-VARS: FORCE
> fi
> endif
>
> +### Detect Python interpreter path changes
> +ifndef NO_PYTHON
> +TRACK_VARS = $(subst ','\'',-DPYTHON_PATH='$(PYTHON_PATH_SQ)')
> +
> +GIT-PYTHON-VARS: FORCE
> + @VARS='$(TRACK_VARS)'; \
> + if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
> + echo 1>&2 " * new Python interpreter location"; \
> + echo "$$VARS" >$@; \
> + fi
> +endif
Do we have the same issue when you decide to use /usr/local/bin/sh
after building with /bin/sh set to SHELL_PATH?
- If yes, I presume that there will be follow-up patches to this
one, for SHELL_PATH, PERL_PATH, and TCLTK_PATH (there may be
other languages but I didn't bother to check). How would the use
of language agnostic looking TRACK_VARS variable in this patch
affect such follow-up patches?
- If no (in other words, if we rebuild shell-scripted porcelains
when SHELL_PATH changes), wouldn't it be better to see how it is
done and hook into the same mechanism?
- If no, and if the approach taken in this patch is better than the
one used to rebuild shell scripts (it may limit the scope of
rebuilding when path changes, or something, but again, I didn't
bother to check), then again follow-up patches to this one to
describe dependencies to build scripts in other languages in a
similar way to this patch would come in the future. The same
question regarding TRACK_VARS applies in this case.
By the way, "1>&2" is easier to read if written as just ">&2", I
think.
Thanks.
^ permalink raw reply
* [PATCH/RFCv2 0/2] compiling git with gcc -O3 -Wuninitialized
From: Jeff King @ 2012-12-15 17:36 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <20121215110930.GA23727@sigill.intra.peff.net>
On Sat, Dec 15, 2012 at 06:09:30AM -0500, Jeff King wrote:
> > Does
> >
> > #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)
> >
> > cause problems when not used in a return statement?
>
> Thanks, that was the cleverness I was missing.
Here it is as patches. One problem with this method is that if the
function implementation ever changes to _not_ return -1, then we get no
warning that our macro and the function implementation have diverged in
meaning.
[1/2]: make error()'s constant return value more visible
[2/2]: silence some -Wuninitialized false positives
These would go on top of 1/3 from the original series to make -Wall -O3
clean (I'll repost the series as a whole when it is more obvious what we
want to do).
-Peff
^ permalink raw reply
* [PATCH 1/2] make error()'s constant return value more visible
From: Jeff King @ 2012-12-15 17:37 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <20121215173621.GA21011@sigill.intra.peff.net>
When git is compiled with "gcc -Wuninitialized -O3", some
inlined calls provide an additional opportunity for the
compiler to do static analysis on variable initialization.
For example, with two functions like this:
int get_foo(int *foo)
{
if (something_that_might_fail() < 0)
return error("unable to get foo");
*foo = 0;
return 0;
}
void some_fun(void)
{
int foo;
if (get_foo(&foo) < 0)
return -1;
printf("foo is %d\n", foo);
}
If get_foo() is not inlined, then when compiling some_fun,
gcc sees only that a pointer to the local variable is
passed, and must assume that it is an out parameter that
is initialized after get_foo returns.
However, when get_foo() is inlined, the compiler may look at
all of the code together and see that some code paths in
get_foo() do not initialize the variable. As a result, it
prints a warning. But what the compiler can't see is that
error() always returns -1, and therefore we know that either
we return early from some_fun, or foo ends up initialized,
and the code is safe. The warning is a false positive.
If we can make the compiler aware that error() will always
return -1, it can do a better job of analysis. The simplest
method would be to inline the error() function. However,
this doesn't work, because gcc will not inline a variadc
function. We can work around this by defining a macro. This
relies on two gcc extensions:
1. Variadic macros (these are present in C99, but we do
not rely on that).
2. Gcc treats the "##" paste operator specially between a
comma and __VA_ARGS__, which lets our variadic macro
work even if no format parameters are passed to
error().
Since we are using these extra features, we hide the macro
behind an #ifdef. This is OK, though, because our goal was
just to help gcc.
Signed-off-by: Jeff King <peff@peff.net>
---
git-compat-util.h | 11 +++++++++++
usage.c | 1 +
2 files changed, 12 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..9002bca 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -288,6 +288,17 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
+/*
+ * Let callers be aware of the constant return value; this can help
+ * gcc with -Wuninitialized analysis. We have to restrict this trick to
+ * gcc, though, because of the variadic macro and the magic ## comma pasting
+ * behavior. But since we're only trying to help gcc, anyway, it's OK; other
+ * compilers will fall back to using the function as usual.
+ */
+#ifdef __GNUC__
+#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
+#endif
+
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
extern void set_error_routine(void (*routine)(const char *err, va_list params));
diff --git a/usage.c b/usage.c
index 8eab281..40b3de5 100644
--- a/usage.c
+++ b/usage.c
@@ -130,6 +130,7 @@ void NORETURN die_errno(const char *fmt, ...)
va_end(params);
}
+#undef error
int error(const char *err, ...)
{
va_list params;
--
1.8.0.2.4.g59402aa
^ permalink raw reply related
* [PATCH 2/2] silence some -Wuninitialized false positives
From: Jeff King @ 2012-12-15 17:42 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <20121215173621.GA21011@sigill.intra.peff.net>
There are a few error functions that simply wrap error() and
provide a standardized message text. Like error(), they
always return -1; knowing that can help the compiler silence
some false positive -Wuninitialized warnings.
One strategy would be to just declare these as inline in the
header file so that the compiler can see that they always
return -1. However, gcc does not always inline them (e.g.,
it will not inline opterror, even with -O3), which renders
our change pointless.
Instead, let's follow the same route we did with error() in
the last patch, and define a macro that makes the constant
return value obvious to the compiler.
Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to force inlining with
__attribute(always_inline)__. But I don't like that, as we are
affecting the generated code in that case (and any time we are
overriding gcc's decision, I have to assume that it is smarter about
when to inline than we are).
Other variants include:
1. Inline functions, but keep them as one-liners. E.g.:
int opterror(...)
{
real_opterror(...);
return -1;
}
2. Using these macros even when __GNUC__ isn't set. Unlike the
variadic error() macro, these do not use any special features.
If we used them everywhere, the functions themselves could be
converted to a void return. That would make it less likely that
somebody modifying the function in the future would fail to realize
that the error return must always be -1.
I dunno. All the solutions are a bit ugly. I really do like being -Wall
clean, but I wonder if this is spending too much effort to work around
the compiler (we could also just mark these few cases as "int foo =
foo" to say we have manually verified that they're OK).
cache.h | 3 +++
config.c | 1 +
parse-options.c | 18 +++++++++---------
parse-options.h | 4 ++++
4 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 18fdd18..0e8e5d8 100644
--- a/cache.h
+++ b/cache.h
@@ -1136,6 +1136,9 @@ extern int config_error_nonbool(const char *);
extern int git_env_bool(const char *, int);
extern int git_config_system(void);
extern int config_error_nonbool(const char *);
+#ifdef __GNUC__
+#define config_error_nonbool(s) (config_error_nonbool(s), -1)
+#endif
extern const char *get_log_output_encoding(void);
extern const char *get_commit_output_encoding(void);
diff --git a/config.c b/config.c
index fb3f868..526f682 100644
--- a/config.c
+++ b/config.c
@@ -1660,6 +1660,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
* Call this to report error for your variable that should not
* get a boolean value (i.e. "[my] var" means "true").
*/
+#undef config_error_nonbool
int config_error_nonbool(const char *var)
{
return error("Missing value for '%s'", var);
diff --git a/parse-options.c b/parse-options.c
index c1c66bd..67e98a6 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -18,15 +18,6 @@ int optbug(const struct option *opt, const char *reason)
return error("BUG: switch '%c' %s", opt->short_name, reason);
}
-int opterror(const struct option *opt, const char *reason, int flags)
-{
- if (flags & OPT_SHORT)
- return error("switch `%c' %s", opt->short_name, reason);
- if (flags & OPT_UNSET)
- return error("option `no-%s' %s", opt->long_name, reason);
- return error("option `%s' %s", opt->long_name, reason);
-}
-
static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
int flags, const char **arg)
{
@@ -594,3 +585,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
return usage_with_options_internal(ctx, usagestr, opts, 0, err);
}
+#undef opterror
+int opterror(const struct option *opt, const char *reason, int flags)
+{
+ if (flags & OPT_SHORT)
+ return error("switch `%c' %s", opt->short_name, reason);
+ if (flags & OPT_UNSET)
+ return error("option `no-%s' %s", opt->long_name, reason);
+ return error("option `%s' %s", opt->long_name, reason);
+}
diff --git a/parse-options.h b/parse-options.h
index 71a39c6..e703853 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,6 +177,10 @@ extern int opterror(const struct option *opt, const char *reason, int flags);
extern int optbug(const struct option *opt, const char *reason);
extern int opterror(const struct option *opt, const char *reason, int flags);
+#ifdef __GNUC__
+#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
+#endif
+
/*----- incremental advanced APIs -----*/
enum {
--
1.8.0.2.4.g59402aa
^ permalink raw reply related
* [PATCH] completion: complete refs for "git commit -c"
From: Jeff King @ 2012-12-15 17:46 UTC (permalink / raw)
To: git
The "-c" and "-C" options take an existing commit, so let's
complete refs, just as we would for --squash or --fixup.
Signed-off-by: Jeff King <peff@peff.net>
---
I notice that the existing code just handles the "--foo=bar" form of most
options. By checking $prev, we can also handle "--foo bar" form (which
we have to do for short options like "-c"). But it would mean
duplicating all of the existing logic. I don't know if it's worth it or
not, given that nobody has complained.
contrib/completion/git-completion.bash | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0b77eb1..a4c48e1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -971,6 +971,13 @@ _git_commit ()
{
__git_has_doubledash && return
+ case "$prev" in
+ -c|-C)
+ __gitcomp_nl "$(__git_refs)" "" "${cur}"
+ return
+ ;;
+ esac
+
case "$cur" in
--cleanup=*)
__gitcomp "default strip verbatim whitespace
--
1.8.0.2.4.g59402aa
^ 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