* Re: commit gone after merge - how to debug?
From: Igor Lautar @ 2012-11-26 13:24 UTC (permalink / raw)
To: Tomas Carnecky; +Cc: git
In-Reply-To: <1353935441-ner-9639@calvin>
On Mon, Nov 26, 2012 at 2:10 PM, Tomas Carnecky
<tomas.carnecky@gmail.com> wrote:
> On Mon, 26 Nov 2012 14:06:09 +0100, Igor Lautar <igor.lautar@gmail.com> wrote:
>> git log <file modified by commit>
>> - commit NOT shown in file history any more and file does not have this change
>
> does `git log --full-history <file modified by commit>` show the commit?
Indeed it does.
Did the merge with verbosity set to 5. It says the commit I'm merging
in is virtual (probably as it is a merge commit in itself?).
Why would commit be left behind after merge? What kind of history
triggers this scenario?
Just trying to understand reasoning as its counter-intuitive to what I
know now. This may affect our workflow (ie., change it so we avoid it
happening).
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Igor Lautar @ 2012-11-26 13:29 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Tomas Carnecky, git
In-Reply-To: <vpqr4ngsdjl.fsf@grenoble-inp.fr>
On Mon, Nov 26, 2012 at 2:23 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> The other related question being: does reading the section "History
> Simplification" in "man git-log" help? ;-)
Somewhat, but it does not explain why the file no longer has that
change. I can understand omitting history if end result is the same,
but here it shouldn't be - I cannot find a commit that reversed that
change, so the change should still be in after the merge?
The file in question was not modified on mirror, nor was modified on
origin after that change.
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Matthieu Moy @ 2012-11-26 13:38 UTC (permalink / raw)
To: Igor Lautar; +Cc: Tomas Carnecky, git
In-Reply-To: <CAO1Khk9mzJjnysnc1iDFeMgqnRq0z35t0kgC-8nrsjJ-oOvdOg@mail.gmail.com>
Igor Lautar <igor.lautar@gmail.com> writes:
> On Mon, Nov 26, 2012 at 2:23 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> The other related question being: does reading the section "History
>> Simplification" in "man git-log" help? ;-)
>
> Somewhat, but it does not explain why the file no longer has that
> change.
It still has, but it's not shown by "git log <file>", probably because
one of the parent of the merge commit introduces no change for this
file, so one side of the merge is not needed to explain you how you went
from the origin of time to the last commit.
Try this:
commit=<sha1 of your merge commit>
# Show diff with first parent:
git diff "$commit" "$commit"^1
# Show diff with second parent:
git diff "$commit" "$commit"^2
> I can understand omitting history if end result is the same, but here
> it shouldn't be - I cannot find a commit that reversed that change, so
> the change should still be in after the merge?
revert is not the only situation that can lead to history
simplification. I'm no expert in the domain, but I think if you did the
same change in two branches, the merge will be candidate for history
simplification.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Igor Lautar @ 2012-11-26 13:58 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Tomas Carnecky, git
In-Reply-To: <vpqehjgscv3.fsf@grenoble-inp.fr>
On Mon, Nov 26, 2012 at 2:38 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Igor Lautar <igor.lautar@gmail.com> writes:
>> Somewhat, but it does not explain why the file no longer has that
>> change.
>
> It still has, but it's not shown by "git log <file>", probably because
> one of the parent of the merge commit introduces no change for this
> file, so one side of the merge is not needed to explain you how you went
> from the origin of time to the last commit.
No, the change is not there. See below.
> Try this:
>
> commit=<sha1 of your merge commit>
> # Show diff with first parent:
> git diff "$commit" "$commit"^1
> # Show diff with second parent:
> git diff "$commit" "$commit"^2
Yes, change is shown in commit^2, but actual file after merge does not have it.
I've double and triple checked, it is just not there. In the end, I've
cherry-picked the same commit after the merge and change is applied.
If change would be there after the merge, cherry-pick would not have
anything to do (whole commit is a one line change in single file).
So its not that the history is hidden, the change *is* missing after the merge.
Is there anything else I can try to figure out why its missing (other
than actually debugging git code/scripts)? Like debug output for each
change being considered/merged in?
Regards,
Igor
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Matthieu Moy @ 2012-11-26 14:03 UTC (permalink / raw)
To: Igor Lautar; +Cc: Tomas Carnecky, git
In-Reply-To: <CAO1Khk8=nrKknfqY-k6XaGPDbLrHyrK-8fxfB7XXUWeB7L4EUA@mail.gmail.com>
Igor Lautar <igor.lautar@gmail.com> writes:
> Yes, change is shown in commit^2, but actual file after merge does not have it.
Your initial message was about the output of "git log". Do you mean that
the file, on the filesystem, does not have the line introduced by the
commit?
If so, check the content registered in the repository too:
git show <merge-commit>:<file-name>
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] fast-export: Allow pruned-references in mark file
From: Felipe Contreras @ 2012-11-26 14:04 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Junio C Hamano, git
In-Reply-To: <CALWbr2yZpAT=eSahGcGKw5weoz1MjTzbb16pdQndKDFcn_3VJg@mail.gmail.com>
On Mon, Nov 26, 2012 at 2:23 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> On Mon, Nov 26, 2012 at 12:37 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Mon, Nov 26, 2012 at 5:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Is this a safe and sane thing to do, and if so why? Could you
>>> describe that in the log message here?
>> Why would fast-export try to export something that was pruned? Doesn't
>> that mean it wasn't reachable?
>
> Hello Junio,
> Hello Felipe,
>
> Actually the issue happened while using Felipe's branch with his
> git-remote-hg. Everything was going fine until I (or did it run
> automatically, I dont remember) ran git gc that pruned unreachable
> objects. Of course some of the branch I had pushed to the hg remote
> had been changed (most likely rebased). References no longer exists
> in the repository (cleaned by gc), but the reference still exists in
> mark file, as it was exported earlier. Thus the failure when git
> fast-export reads the mark file.
Ah, I see, so these objects are _before_ fast-export tries to do
anything, it's just importing the marks without any knowledge if these
objects are going to be used in the export or not.
If that's the case, I don't think it should throw a warning even just skip them.
Then, in the actual export if some of these objects are referenced the
export would fail anyway (but they won't).
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] fast-export: Allow pruned-references in mark file
From: Antoine Pelisse @ 2012-11-26 14:14 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Junio C Hamano, git
In-Reply-To: <CAMP44s3Xo2ko6X1-SO3hLiTYHA3+i912jTGOQCUihixxcbEuRQ@mail.gmail.com>
> If that's the case, I don't think it should throw a warning even just skip them.
Removing the warning seems fine to me.
> Then, in the actual export if some of these objects are referenced the
> export would fail anyway (but they won't).
Of course it will fail to export anything that requires the missing object.
As they are unreachable, it will be hard to provide a ref that needs
it anyway.
On the other hand, I'm afraid that your file
'.git/hg/<remote>/marks-hg' needs consistent references to mark.
If a mark is removed, and then replaced by another object, can it
break somehow git-remote-hg ? If not, I can provide a simpler patch.
If it does, it will be more complicated.
Cheers,
Antoine
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Igor Lautar @ 2012-11-26 14:15 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Tomas Carnecky, git
In-Reply-To: <vpqhaocqx4k.fsf@grenoble-inp.fr>
On Mon, Nov 26, 2012 at 3:03 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Your initial message was about the output of "git log". Do you mean that
> the file, on the filesystem, does not have the line introduced by the
> commit?
Yes, sorry if I was not clear enough.
> If so, check the content registered in the repository too:
>
> git show <merge-commit>:<file-name>
Content shown is identical to the one in working copy, ie., it is
missing one line change.
git annotate <file> <merge commit>
- shows that particular line as if it has originated from when the
file was originally added to repo.
git annotate <file> <merge commit>^2
- shows line as being modified by a commit done after file was added
- ie., state I would expect after a merge
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Matthieu Moy @ 2012-11-26 14:19 UTC (permalink / raw)
To: Igor Lautar; +Cc: Tomas Carnecky, git
In-Reply-To: <CAO1Khk9Y_SC8q4iHnv848Z+dXMaeUOWxzW76yPSj_as317_u5g@mail.gmail.com>
Igor Lautar <igor.lautar@gmail.com> writes:
> git annotate <file> <merge commit>^2
> - shows line as being modified by a commit done after file was added
> - ie., state I would expect after a merge
What about "git annotate <file> <merge-commit>^1"?
Was the merge completely automatic, or were there any conflict?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Igor Lautar @ 2012-11-26 14:23 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Tomas Carnecky, git
In-Reply-To: <vpqboekqwei.fsf@grenoble-inp.fr>
On Mon, Nov 26, 2012 at 3:19 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> What about "git annotate <file> <merge-commit>^1"?
No change, line version goes back to when file was added.
> Was the merge completely automatic, or were there any conflict?
No conflicts at all. In fact, that particular file was not touched by
one side of merge, only by another. It seems like git ignored the
change, but still recorded history (shown only with --full-history).
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Matthieu Moy @ 2012-11-26 14:50 UTC (permalink / raw)
To: Igor Lautar; +Cc: Tomas Carnecky, git
In-Reply-To: <CAO1Khk8=nrKknfqY-k6XaGPDbLrHyrK-8fxfB7XXUWeB7L4EUA@mail.gmail.com>
[ Jumping back in time ]
Igor Lautar <igor.lautar@gmail.com> writes:
>> Try this:
>>
>> commit=<sha1 of your merge commit>
>> # Show diff with first parent:
>> git diff "$commit" "$commit"^1
>> # Show diff with second parent:
>> git diff "$commit" "$commit"^2
>
> Yes, change is shown in commit^2, but actual file after merge does not have it.
My commands had the wrong order. It should have been git diff
"$commit"^2 "$commit". So, it showed the reverse of the modification
introduced by the commit. If you see your change here, it means the
change was reverted by the merge.
My understanding of the situation up to now is:
M
|\
A C
|/
B
($commit = M, $commit^1 = A and $commit^2 = B)
Your file had a content (say, "old") at revision B. It changed content
(say, to "new") at revision C, and at some point. A did not change it so
it had the content "old". Then you merged, expected the merge commit M
to get content "new", and actually got "old".
So, your history looks like:
M (old)
| \
| `---.
| \
A (old) C (new)
| /
| .---'
| /
|/
B (old)
and "git diff C M" shows the diff between new and old.
Something went wrong during the merge, I guess it used an ancestor (B
above) that had "new" as content. I don't see how this happened (rather
clearly, your history is more complex than my example above), but
"GIT_MERGE_VERBOSITY=5 git merge" will show you which common ancestor
was used, it may help.
What's possible is that someone had already merged the branch containing
"new", got conflicts, and resolved it in favor of "old" somewhere in the
history of your master branch.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: git-fetch does not work from .git subdirectory
From: Timur Tabi @ 2012-11-26 16:09 UTC (permalink / raw)
To: Patrik Gornicz; +Cc: git
In-Reply-To: <50AD77F3.3080702@mail.pgornicz.com>
Patrik Gornicz wrote:
> Just a hunch but your remote's location uses a relative path
> '../linux-2.6.git', perhaps git is messing up what the path is relative
> to.
That makes sense. Git is looking at the URL and not realizing that it's
relative to the home directory.
[remote "upstream"]
url = ../linux-2.6.git/
fetch = +refs/heads/*:refs/remotes/upstream/*
> Note sure what the fix will be though as it'll likely break existing
> repositories that use relative paths either way. Can you try an
> absolute path to see if that fixes thing?
If I change that to
[remote "upstream"]
url = /home/b04825/git/linux-2.6.git/
fetch = +refs/heads/*:refs/remotes/upstream/*
then everything works.
IMHO, this is a bug in git.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH v5 15/15] fast-export: don't handle uninteresting refs
From: Johannes Schindelin @ 2012-11-26 16:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Felipe Contreras, Jeff King, Jonathan Nieder, git, Max Horn,
Sverre Rabbelier, Brandon Casey, Brandon Casey, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <7v7gp9udsl.fsf@alter.siamese.dyndns.org>
Hi Junio,
On Sun, 25 Nov 2012, Junio C Hamano wrote:
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Subject: Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
> Date: Fri, 2 Nov 2012 16:17:14 +0100 (CET)
> Message-ID: <alpine.DEB.1.00.1211021612320.7256@s15462909.onlinehome-server.info>
>
> (which is $gmane/208946) that says:
>
> Note that
>
> $ git branch foo master~1
> $ git fast-export foo master~1..master
>
> still does not update the "foo" ref, but a partial fix is better
> than no fix.
If you changed your stance on the patch Sverre and I sent to fix this, we
could get a non-partial fix for this. You wanted a fix for a bigger
problem, though, which I am unwilling to fix because it is not my itch to
scratch and I have to balance my time.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 5/5] git-send-email: allow edit invalid email address
From: Krzysztof Mazur @ 2012-11-26 17:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Felipe Contreras, Andreas Schwab, Felipe Balbi,
Tomi Valkeinen
In-Reply-To: <7vobikthpp.fsf@alter.siamese.dyndns.org>
On Mon, Nov 26, 2012 at 09:08:34AM -0800, Junio C Hamano wrote:
> Krzysztof Mazur <krzysiek@podlesie.net> writes:
>
> > In some cases the user may want to send email with "Cc:" line with
> > email address we cannot extract. Now we allow user to extract
> > such email address for us.
> >
> > Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> > ---
> > git-send-email.perl | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index d42dca2..9996735 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -851,10 +851,10 @@ sub extract_valid_address_or_die {
> >
> > sub validate_address {
> > my $address = shift;
> > - if (!extract_valid_address($address)) {
> > + while (!extract_valid_address($address)) {
> > print STDERR "error: unable to extract a valid address from: $address\n";
> > - $_ = ask("What to do with this address? ([q]uit|[d]rop): ",
> > - valid_re => qr/^(?:quit|q|drop|d)/i,
> > + $_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): ",
> > + valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
> > default => 'q');
> > if (/^d/i) {
> > return undef;
> > @@ -862,6 +862,9 @@ sub validate_address {
> > cleanup_compose_files();
> > exit(0);
> > }
> > + $address = ask("Who should the email be sent to (if any)? ",
> > + default => "",
> > + valid_re => qr/\@.*\./, confirm_only => 1);
>
> Not having this new code inside "elsif (/^e/) { }" feels somewhat
> sloppy, even though it is not *too* bad. Also do we know this
ok, I will fix that.
> function will never be used for addresses other than recipients' (I
> gave a cursory look to see what is done to the $sender and it does
> not seem to go through this function, tho)?
Yes, this function is called only from validate_address_just()
to filter @initial_to, @initial_cc, @bcc_list as early as possible,
and filter @to and @cc added in each email.
Krzysiek
^ permalink raw reply
* Re: [PATCH] fast-export: Allow pruned-references in mark file
From: Junio C Hamano @ 2012-11-26 17:41 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Felipe Contreras, git
In-Reply-To: <CALWbr2yZpAT=eSahGcGKw5weoz1MjTzbb16pdQndKDFcn_3VJg@mail.gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
> On Mon, Nov 26, 2012 at 12:37 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Mon, Nov 26, 2012 at 5:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Is this a safe and sane thing to do, and if so why? Could you
>>> describe that in the log message here?
>> Why would fast-export try to export something that was pruned? Doesn't
>> that mean it wasn't reachable?
>
> Hello Junio,
> Hello Felipe,
>
> Actually the issue happened while using Felipe's branch with his
> git-remote-hg. Everything was going fine until I (or did it run
> automatically, I dont remember) ran git gc that pruned unreachable
> objects. Of course some of the branch I had pushed to the hg remote
> had been changed (most likely rebased). References no longer exists
> in the repository (cleaned by gc), but the reference still exists in
> mark file, as it was exported earlier. Thus the failure when git
> fast-export reads the mark file.
You described that part very well in your proposed log message and I
got it just fine.
> Then, is it safe ?
> Updating the last_idnum as I do in the patch doesn't work because
> if the reference is the last, the number is going to be overwriten
> in the next run.
> From git point of view, I guess it is fine. The file is fully read at
> the beginning of fast-export and fully written at the end.
I am not sure I follow the above, but anyway, I think the patch does
is safe because (1) future "fast-export" will not refer to these
pruned objects in its output (we have decided that these pruned
objects are not used anywhere in the history so nobody will refer to
them) and (2) we still need to increment the id number so that later
objects in the marks file get assigned the same id number as they
were assigned originally (otherwise we will not name these objects
consistently when we later talk about them).
And I wanted to see that kind of reasoning behind the patch in the
proposed log message, because other people will need to refer to it
when they read "git log" output to understand the change.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/5] git-send-email: allow edit invalid email address
From: Junio C Hamano @ 2012-11-26 17:08 UTC (permalink / raw)
To: Krzysztof Mazur
Cc: git, Felipe Contreras, Andreas Schwab, Felipe Balbi,
Tomi Valkeinen
In-Reply-To: <1353607932-10436-5-git-send-email-krzysiek@podlesie.net>
Krzysztof Mazur <krzysiek@podlesie.net> writes:
> In some cases the user may want to send email with "Cc:" line with
> email address we cannot extract. Now we allow user to extract
> such email address for us.
>
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---
> git-send-email.perl | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index d42dca2..9996735 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -851,10 +851,10 @@ sub extract_valid_address_or_die {
>
> sub validate_address {
> my $address = shift;
> - if (!extract_valid_address($address)) {
> + while (!extract_valid_address($address)) {
> print STDERR "error: unable to extract a valid address from: $address\n";
> - $_ = ask("What to do with this address? ([q]uit|[d]rop): ",
> - valid_re => qr/^(?:quit|q|drop|d)/i,
> + $_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): ",
> + valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
> default => 'q');
> if (/^d/i) {
> return undef;
> @@ -862,6 +862,9 @@ sub validate_address {
> cleanup_compose_files();
> exit(0);
> }
> + $address = ask("Who should the email be sent to (if any)? ",
> + default => "",
> + valid_re => qr/\@.*\./, confirm_only => 1);
Not having this new code inside "elsif (/^e/) { }" feels somewhat
sloppy, even though it is not *too* bad. Also do we know this
function will never be used for addresses other than recipients' (I
gave a cursory look to see what is done to the $sender and it does
not seem to go through this function, tho)?
^ permalink raw reply
* Re: [PATCH v5 15/15] fast-export: don't handle uninteresting refs
From: Junio C Hamano @ 2012-11-26 17:56 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Felipe Contreras, Jeff King, Jonathan Nieder, git, Max Horn,
Sverre Rabbelier, Brandon Casey, Brandon Casey, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <alpine.DEB.1.00.1211261726260.7256@s15462909.onlinehome-server.info>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> If you changed your stance on the patch Sverre and I sent to fix this, we
> could get a non-partial fix for this.
This is long time ago so I may be misremembering the details, but I
thought the original patch was (ab)using object flags to mark "this
was explicitly asked for, even though some other range operation may
have marked it uninteresting". Because it predated the introduction
of the rev_cmdline_info mechanism to record what was mentioned on
the command line separately from what objects are uninteresting
(i.e. object flags), it may have been one convenient way to record
this information, but it still looked unnecessarily ugly hack to me,
in that it allocated scarce object flag bits to represent a narrow
special case (iirc, only a freestanding "A" on the command line but
not "A" spelled in "B..A", or something), making it more expensive
to record other kinds of command line information in a way
consistent with the approach chosen (we do not want to waste object
flag bits in order to record "this was right hand side tip of the
symmetric difference range" and such).
If you are calling "do not waste object flags to represent one
special case among endless number of possibilities, as it will make
it impossible to extend it" my stance, that hasn't changed.
We added rev_cmdline_info since then so that we can tell what refs
were given from the command line in what way, and I thought that we
applied a patch from Sverre that uses it instead of the object
flags. Am I misremembering things?
^ permalink raw reply
* Re: [RFC/PATCH] Option to revert order of parents in merge commit
From: Junio C Hamano @ 2012-11-26 17:58 UTC (permalink / raw)
To: Kacper Kornet; +Cc: git
In-Reply-To: <20121126124200.GA29859@camk.edu.pl>
Kacper Kornet <draenog@pld-linux.org> writes:
>> Is there any interaction between this "pull --reverse-parents"
>> change and possible conflict resolution when the command stops and
>> asks the user for help? For example, whom should "--ours" and "-X
>> ours" refer to? Us, or the upstream?
>
> The change of order of parents happens at the very last moment, so
> "ours" in merge options is local version and "theirs" upstream.
That may be something that wants to go to the proposed commit log
message. I am neutral on the "feature" (i.e. not against it but not
extremely enthusiastic about it either).
Thanks.
^ permalink raw reply
* Re: Long clone time after "done."
From: Uri Moszkowicz @ 2012-11-26 18:06 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <CAMJd5AQOH=uu1c3enr5fES+Bw4FHZuP++J7-aCM2B+f_G3YMtg@mail.gmail.com>
Hi guys,
Any further interest on this scalability problem or should I move on?
Thanks,
Uri
On Thu, Nov 8, 2012 at 5:35 PM, Uri Moszkowicz <uri@4refs.com> wrote:
> I tried on the local disk as well and it didn't help. I managed to
> find a SUSE11 machine and tried it there but no luck so I think we can
> eliminate NFS and OS as significant factors now.
>
> I ran with perf and here's the report:
>
> ESC[31m 69.07%ESC[m git /lib64/libc-2.11.1.so
> [.] memcpy
> ESC[31m 12.33%ESC[m git
> <prefix>/git-1.8.0.rc2.suse11/bin/git [.]
> blk_SHA1_Block
> ESC[31m 5.11%ESC[m git
> <prefix>/zlib/local/lib/libz.so.1.2.5 [.]
> inflate_fast
> ESC[32m 2.61%ESC[m git
> <prefix>/zlib/local/lib/libz.so.1.2.5 [.]
> adler32
> ESC[32m 1.98%ESC[m git /lib64/libc-2.11.1.so
> [.] _int_malloc
> ESC[32m 0.86%ESC[m git [kernel]
> [k] clear_page_c
>
> Does this help? Machine has 396GB of RAM if it matters.
>
> On Thu, Nov 8, 2012 at 4:33 PM, Jeff King <peff@peff.net> wrote:
>> On Thu, Nov 08, 2012 at 04:16:59PM -0600, Uri Moszkowicz wrote:
>>
>>> I ran "git cat-file commit some-tag" for every tag. They seem to be
>>> roughly uniformly distributed between 0s and 2s and about 2/3 of the
>>> time seems to be system. My disk is mounted over NFS so I tried on the
>>> local disk and it didn't make a difference.
>>>
>>> I have only one 1.97GB pack. I ran "git gc --aggressive" before.
>>
>> Ah. NFS. That is almost certainly the source of the problem. Git will
>> aggressively mmap. I would not be surprised to find that RHEL4's NFS
>> implementation is not particularly fast at mmap-ing 2G files, and is
>> spending a bunch of time in the kernel servicing the requests.
>>
>> Aside from upgrading your OS or getting off of NFS, I don't have a lot
>> of advice. The performance characteristics you are seeing are so
>> grossly off of what is normal that using git is probably going to be
>> painful. Your 2s cat-files should be more like .002s. I don't think
>> there's anything for git to fix here.
>>
>> You could try building with NO_MMAP, which will emulate it with pread.
>> That might fare better under your NFS implementation. Or it might be
>> just as bad.
>>
>> -Peff
^ permalink raw reply
* Re: [PATCH 3/5] git-send-email: remove invalid addresses earlier
From: Junio C Hamano @ 2012-11-26 17:02 UTC (permalink / raw)
To: Krzysztof Mazur
Cc: git, Felipe Contreras, Andreas Schwab, Felipe Balbi,
Tomi Valkeinen
In-Reply-To: <1353607932-10436-3-git-send-email-krzysiek@podlesie.net>
Krzysztof Mazur <krzysiek@podlesie.net> writes:
> Some addresses are passed twice to unique_email_list() and invalid addresses
> may be reported twice per send_message. Now we warn about them earlier
> and we also remove invalid addresses.
>
> This also removes using of undefined values for string comparison
> for invalid addresses in cc list processing.
>
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---
I think there are three kinds of address-looking things we deal
with:
* Possibly invalid but meant for human consumption, e.g.
Cc: Stable Kernel <stable@k.org> # for v3.5 and upwards
in the commit log message trailer.
* Meant to be fed to our MSA, without losing the human readable
part, e.g.
Cc: Stable Kernel <stable@k.org>
in the header of the outgoing message.
* Without the human-readable part, e.g.
stable@k.org
that is returned by extract_valid_address.
My understanding is that our input typically comes from the first
kind and sanitize_address() is meant to massage it into the second
kind. The last kind is to be used to drive the underlying sendmail
machinery and meant to go in the envelope (this includes message-id
generation).
I do not think send-email adds the first kind (invalid ones) in its
output, even though it reads them from its input and copy them to
its output in the e-mail body part of the payload, but I think it
adds new addresses to the e-mail header part of the payload (that is
what $from, @initial_to, @initial_cc and @bcclist are all about).
We would want to feed them in the third form (i.e. output from
extract-valid-address on them) when driving the underlying sendmail
machinery to place them in the envelope part, but they should be in
the second form when we place them on e-mail header lines. As far
as I can tell, the resulting code looks correct in this regard. The
addresses are sanitized into the second form upfront and validated
before they are placed in @initial_to and friends, and we carry the
second form around most of the time, until we call unique_email_list
in send_message to pass them through extract_valid_address to turn
them into the third form to drive the underlying sendmail.
I however found it a bit confusing while reading the callers of
validate_address{,_list} functions, which not just validate (and
warns) but return the ones that pass the test. Perhaps we would
want a brief comment before validate_address, validate_address_list,
and extract_valid_address{,_or_die} to clarify what they are doing
(especially what they return)?
The result still feels somewhat yucky (the yuckiness comes primarily
from the current code, not from the patch but I am mostly focused on
the result after applying the patch), in that extract-valid-address
that has problem with invalid email addresses will still die when
fed an address that is not "sanitized" first, so any future patch
that adds a new address source may still have to suffer the same
problem the part that dealt with the Cc list suffered (which your
1/5 fixed earlier), but I do not offhand think of a good way to
reorganize them. We could of course make validate_address() call
sanitize_address(), but that would be mostly redundant since the new
code sanitizes the input upfront.
So overall, looks good to me. Thanks.
> git-send-email.perl | 52 +++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 356f99d..5056fdc 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -786,9 +786,11 @@ sub expand_one_alias {
> }
>
> @initial_to = expand_aliases(@initial_to);
> -@initial_to = (map { sanitize_address($_) } @initial_to);
> +@initial_to = validate_address_list(sanitize_address_list(@initial_to));
> @initial_cc = expand_aliases(@initial_cc);
> +@initial_cc = validate_address_list(sanitize_address_list(@initial_cc));
> @bcclist = expand_aliases(@bcclist);
> +@bcclist = validate_address_list(sanitize_address_list(@bcclist));
^ permalink raw reply
* Re: [PATCH] Fix typo in remote set-head usage
From: Junio C Hamano @ 2012-11-26 18:16 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: git, tim.henigan, Jiang Xin
In-Reply-To: <1353851019-27254-1-git-send-email-apelisse@gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
> parenthesis are not matching in `builtin_remote_sethead_usage`
> as a square bracket is closing something never opened.
> ---
> This will also break all translation files, should I also send a patch
> to update them ?
"git grep -A2 -e 'remote set-head <name>' po/" tells me that we
already have both the correct variant and the broken one, and they
are both translated ;-) so I do not think we have much to worry
about the i18n fallout in this case, but thanks anyway for thinking
about it.
You would need to sign off your patch, but otherwise looks good to
me.
Thanks.
>
> Cheers,
> Antoine Pelisse
>
> builtin/remote.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index a5a4b23..937484d 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -39,7 +39,7 @@ static const char * const builtin_remote_rm_usage[] = {
> };
>
> static const char * const builtin_remote_sethead_usage[] = {
> - N_("git remote set-head <name> (-a | -d | <branch>])"),
> + N_("git remote set-head <name> (-a | -d | <branch>)"),
> NULL
> };
>
> --
> 1.7.9.5
^ permalink raw reply
* Re: commit gone after merge - how to debug?
From: Igor Lautar @ 2012-11-26 17:01 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Tomas Carnecky, git
In-Reply-To: <vpqa9u4pgew.fsf@grenoble-inp.fr>
On Mon, Nov 26, 2012 at 3:50 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> What's possible is that someone had already merged the branch containing
> "new", got conflicts, and resolved it in favor of "old" somewhere in the
> history of your master branch.
This is exactly what happened. I've actually found a merge of origin
to mirror which reversed the change some time back and was
subsequently merged back to origin later on. Most probably human error
during merge.
Interestingly, this was my first thought as well, but I've must have
overlooked that particular merge the first time.
Anyhow, it sorted now, many thanks for your help,
Igor
^ permalink raw reply
* Re: [PATCH] makefile: hide stderr of curl-config test
From: Junio C Hamano @ 2012-11-26 18:30 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: git
In-Reply-To: <1353554397-27162-1-git-send-email-paul.gortmaker@windriver.com>
Paul Gortmaker <paul.gortmaker@windriver.com> writes:
> Currently, if you don't have curl installed, you will get
>
> $ make distclean 2>&1 | grep curl
> /bin/sh: curl-config: not found
> /bin/sh: curl-config: not found
> /bin/sh: curl-config: not found
> /bin/sh: curl-config: not found
> /bin/sh: curl-config: not found
> $
>
> The intent is not to alarm the user, but just to test if there is
> a new enough curl installed. However, if you look at search engine
> suggested completions, the above "error" messages are confusing
> people into thinking curl is a hard requirement.
Good observation and identification of an issue to tackle. But why
isn't the patch like this?
PROGRAMS += $(REMOTE_CURL_NAMES)
- curl_check := $(shell (echo 070908; curl-config --vernum) | sort -r | sed -ne 2p)
+ curl_check := $(shell (echo 070908; curl-config --vernum) 2>/dev/null | sort -r | sed -ne 2p)
ifeq "$(curl_check)" "070908"
Removal of the "reject old libcURL" is logically a separate thing
regardless of the "alarming output from make", and it probably is
better done as a separate step in a two-patch series. Doing things
that way, when somebody objects to this:
> It wants to ensure curl is newer than 070908. The oldest
> machine I could find (RHEL 4.6) is 2007 vintage according
> to /proc/version data, and it has curl 070C01.
saying that their installation still cares about older libcURL, we
can still keep the "remove alarming output from make" bit.
>
> The failure here is to mask stderr in the test. However, since
> the chance of curl being installed, but too old is essentially
> nil, lets just check for existence and drop the ancient version
> threshold check, if for no other reason, than to simplifly the
> parsing of what the makefile is trying to do by humans.
>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>
> diff --git a/Makefile b/Makefile
> index 9bc5e40..56f55f6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1573,8 +1573,8 @@ else
> REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
> PROGRAM_OBJS += http-fetch.o
> PROGRAMS += $(REMOTE_CURL_NAMES)
> - curl_check := $(shell (echo 070908; curl-config --vernum) | sort -r | sed -ne 2p)
> - ifeq "$(curl_check)" "070908"
> + curl_check := $(shell curl-config --vernum 2>/dev/null)
> + ifneq "$(curl_check)" ""
> ifndef NO_EXPAT
> PROGRAM_OBJS += http-push.o
> endif
^ permalink raw reply
* Re: [PATCH 1/7] push: return reject reasons via a mask
From: Junio C Hamano @ 2012-11-26 18:43 UTC (permalink / raw)
To: Chris Rorvick
Cc: git, Angelo Borsotti, Drew Northup, Michael Haggerty,
Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
Felipe Contreras
In-Reply-To: <1353644515-17349-2-git-send-email-chris@rorvick.com>
Chris Rorvick <chris@rorvick.com> writes:
> Pass all rejection reasons back from transport_push(). The logic is
> simpler and more flexible with regard to providing useful feedback.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
> ---
Thanks for a reroll.
I do not think they are "masks", by the way. They are set of flags
(i.e. a bitset).
A bitset is often called "a mask" when it is used to "mask" a subset
of bits in another bitset that has the real information, either to
ignore irrelevant bits or to pick only the relevant bits from the
latter. And your "reject_mask" is never used as a mask in this
patch---it is the bitset that has the real information and it gets
masked by constant masks like REJECT_NON_FF_HEAD.
In any case, naming it as "reject_mask" is like calling a counter as
"counter_int". It is more important to name it after its purpose
than after its type, and because this is to record the reasons why
the push was rejected, "rejection_reason" might be a better name for
it.
^ permalink raw reply
* Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish
From: Junio C Hamano @ 2012-11-26 18:53 UTC (permalink / raw)
To: Chris Rorvick
Cc: git, Angelo Borsotti, Drew Northup, Michael Haggerty,
Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
Felipe Contreras
In-Reply-To: <1353644515-17349-8-git-send-email-chris@rorvick.com>
Chris Rorvick <chris@rorvick.com> writes:
> Pushes must already (by default) update to a commit-ish due the fast-
> forward check in set_ref_status_for_push(). But rejecting for not being
> a fast-forward suggests the situation can be resolved with a merge.
> Flag these updates (i.e., to a blob or a tree) as not forwardable so the
> user is presented with more appropriate advice.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
> ---
> remote.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/remote.c b/remote.c
> index f5bc4e7..ee0c1e5 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)
> if (!o || o->type != OBJ_COMMIT)
> return 0;
>
> + /* new object must be commit-ish */
> + o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
> + if (!o || o->type != OBJ_COMMIT)
> + return 0;
> +
I think the original code used ref_newer() which took commit-ish on
both sides.
With this code, the old must be a commit but new can be a tag that
points at a commit? Why?
^ 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