* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-11-02 15:34 UTC (permalink / raw)
To: Jeff King
Cc: Jonathan Nieder, git, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121102131255.GB2598@sigill.intra.peff.net>
On Fri, Nov 2, 2012 at 2:12 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:
>
>> If the commit does not have the SHOWN or UNINTERESTING flag set but it
>> is going to get the UNINTERESTING flag set during the walk because of
>> a negative commit listed on the command line, this patch won't help.
>
> Right, so my understanding of the situation is that doing this:
>
> $ git branch foo master~1
> $ git fast-export foo master~1..master
>
> won't show "foo", which seems wrong to me. _But_ we currently get that
> wrong already, so Felipe's patches are not making anything worse, but
> are fixing some situations (namely when master~1 is not mentioned on the
> command-line, but rather in a marks file).
>
> Is that correct?
Yes, that's correct. But my patch ("make sure refs are updated
properly") does _not_ change in any shape or form what happens with
what you specify in the command line, only what happens with marks.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance
From: Jeff King @ 2012-11-02 15:26 UTC (permalink / raw)
To: karsten.blees; +Cc: git, msysgit, pro-logic
In-Reply-To: <OF831F4AE9.23F46743-ONC1257AA7.00353C1F-C1257AA7.00361535@dcon.de>
On Tue, Oct 30, 2012 at 10:50:42AM +0100, karsten.blees@dcon.de wrote:
> 'update-index --refresh' and 'diff-index' (without --cached) don't honor
> the core.preloadindex setting yet. Porcelain commands using these (such as
> git [svn] rebase) suffer from this, especially on Windows.
>
> Use read_cache_preload to improve performance.
>
> Additionally, in builtin/diff.c, don't preload index status if we don't
> access the working copy (--cached).
>
> Results with msysgit on WebKit repo (2GB in 200k files):
>
> | update-index | diff-index | rebase
> ----------------+--------------+------------+---------
> msysgit-v1.8.0 | 9.157s | 10.536s | 42.791s
> + preloadindex | 9.157s | 10.536s | 28.725s
> + this patch | 2.329s | 2.752s | 15.152s
> + fscache [1] | 0.731s | 1.171s | 8.877s
Cool numbers. On my quad-core SSD Linux box, I saw a few speedups, too.
Here are the numbers for "update-index --refresh" on the WebKit repo
(all are wall clock time, best-of-five):
| before | after
-----------+--------+--------
cold cache | 4.513s | 2.059s
warm cache | 0.252s | 0.164s
Not as dramatic, but still nice. I wonder how a spinning disk would fare
on the cold-cache case, though. I also tried it with all but one CPU
disabled, and the warm cache case was a little bit slower.
Still, I don't think we need to worry about performance regressions,
because people who don't have a setup suitable for it will not turn on
core.preloadindex in the first place. And if they have it on, the more
places we use it, probably the better.
-Peff
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Jeff King @ 2012-11-02 15:20 UTC (permalink / raw)
To: Felipe Contreras
Cc: Johannes Sixt, Jonathan Nieder, git, Junio C Hamano,
Ævar Arnfjörð
In-Reply-To: <CAMP44s2bZa4BnTnSv0Zgdh52Vy613R3cWE3nSxXdR4dDVhJAyw@mail.gmail.com>
On Fri, Nov 02, 2012 at 04:17:27PM +0100, Felipe Contreras wrote:
> > And me, who is trying to figure out what to do with this patch. It is
> > presented on its own, outside of a series, with only the description "no
> > reason not to do this".
>
> Yeah, because I think it stands on its own. But I'll include it in the
> remote-hg patch series, I already have it queued up.
Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Jeff King @ 2012-11-02 15:19 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jonathan Nieder, Felipe Contreras, git, Junio C Hamano,
Sverre Rabbelier, Elijah Newren
In-Reply-To: <alpine.DEB.1.00.1211021612320.7256@s15462909.onlinehome-server.info>
On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote:
> > If so, then this series isn't regressing behavior; the only downside is
> > that it's an incomplete fix. In theory this could get in the way of the
> > full fix later on, but given the commit messages and the archive of this
> > discussion, it would be simple enough to revert it later in favor of a
> > more full fix. Is that accurate?
>
> From my understanding, yes.
>
> > Sorry if I am belaboring the discussion. I just want to make sure I
> > understand the situation before deciding what to do with the topic. It
> > sounds like the consensus at this point is "not perfect, but good enough
> > to make forward progress".
>
> I appreciate that stance very much. The patch Sverre and I proposed was
> also an incomplete fix (although I suspect it would fix the issue you
> pointed out above), so I agree with the "perfect is the enemy of the good"
> approach, obviously.
Thanks for the response.
> May I just ask to include a summary of that rationale into the commit
> message rather than relying on people having internet access and knowing
> where to look? Adding the following to the commit message would be good
> enough for me:
>
> 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.
Yes, I think that makes a lot of sense.
Felipe, I notice that you sent out a big "fast-export improvements"
series. Does that supersede this?
-Peff
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Felipe Contreras @ 2012-11-02 15:17 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Sixt, Jonathan Nieder, git, Junio C Hamano,
Ævar Arnfjörð
In-Reply-To: <20121102131726.GC2598@sigill.intra.peff.net>
On Fri, Nov 2, 2012 at 2:17 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote:
>
>> Am 31.10.2012 03:28, schrieb Felipe Contreras:
>> > On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> >> Felipe Contreras wrote:
>> >>
>> >>> It's all fun and games to write explanations for things, but it's not
>> >>> that easy when you want those explanations to be actually true, and
>> >>> corrent--you have to spend time to make sure of that.
>> >>
>> >> That's why it's useful for the patch submitter to write them, asking
>> >> for help when necessary.
>> >>
>> >> As a bonus, it helps reviewers understand the effect of the patch.
>> >> Bugs averted!
>> >
>> > Yeah, that would be nice. Too bad I don't have that information, and
>> > have _zero_ motivation to go and get it for you.
>>
>> Just to clarify: That information is not just for Jonathan, but for
>> everyone on this list and those who dig the history a year down the
>> road. Contributors who have _zero_ motiviation to find out that
>> information are not welcome here because they cause friction and take
>> away time from many others for _zero_ gain.
>
> And me, who is trying to figure out what to do with this patch. It is
> presented on its own, outside of a series, with only the description "no
> reason not to do this".
Yeah, because I think it stands on its own. But I'll include it in the
remote-hg patch series, I already have it queued up.
> But AFAICT, it is _required_ for the tests in
> the remote-hg series to work. Isn't that kind of an important
> motivation?
It's not _requied_, you will see that error at the end, and the
aggregate results would all be 0, but the tests would still work.
> Yet it is not in the commit message, nor does the remote-hg series
> indicate that it should be built on top. Or am I wrong that the one is
> dependent on the other?
They are not dependent.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Johannes Schindelin @ 2012-11-02 15:17 UTC (permalink / raw)
To: Jeff King
Cc: Jonathan Nieder, Felipe Contreras, git, Junio C Hamano,
Sverre Rabbelier, Elijah Newren
In-Reply-To: <20121102131255.GB2598@sigill.intra.peff.net>
Hi Peff,
On Fri, 2 Nov 2012, Jeff King wrote:
> On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:
>
> > If the commit does not have the SHOWN or UNINTERESTING flag set but it
> > is going to get the UNINTERESTING flag set during the walk because of
> > a negative commit listed on the command line, this patch won't help.
>
> Right, so my understanding of the situation is that doing this:
>
> $ git branch foo master~1
> $ git fast-export foo master~1..master
>
> won't show "foo", which seems wrong to me. _But_ we currently get that
> wrong already, so Felipe's patches are not making anything worse, but
> are fixing some situations (namely when master~1 is not mentioned on the
> command-line, but rather in a marks file).
>
> Is that correct?
>
> If so, then this series isn't regressing behavior; the only downside is
> that it's an incomplete fix. In theory this could get in the way of the
> full fix later on, but given the commit messages and the archive of this
> discussion, it would be simple enough to revert it later in favor of a
> more full fix. Is that accurate?
>From my understanding, yes.
> Sorry if I am belaboring the discussion. I just want to make sure I
> understand the situation before deciding what to do with the topic. It
> sounds like the consensus at this point is "not perfect, but good enough
> to make forward progress".
I appreciate that stance very much. The patch Sverre and I proposed was
also an incomplete fix (although I suspect it would fix the issue you
pointed out above), so I agree with the "perfect is the enemy of the good"
approach, obviously.
May I just ask to include a summary of that rationale into the commit
message rather than relying on people having internet access and knowing
where to look? Adding the following to the commit message would be good
enough for me:
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.
Thanks,
Dscho
^ permalink raw reply
* Re: Set core.ignorecase globally
From: Konstantin Khomoutov @ 2012-11-02 15:15 UTC (permalink / raw)
To: Konstantin Khomoutov; +Cc: Kirill Likhodedov, git
In-Reply-To: <20121102190337.66a08366de41d29e745b0e4c@domain007.com>
On Fri, 2 Nov 2012 19:03:37 +0400
Konstantin Khomoutov <flatworm@users.sourceforge.net> wrote:
> > Currently, core.ignorecase is set to true on case insensitive system
> > like Windows or Mac on `git init` and `git clone`, and this setting
> > is local to the created/cloned repository.
> [...]
> > I suggest to set this globally by default when Git is installed,
> > because there is little sense to have this option false on case
> > insensitive systems (it will lead to confusions when renaming a file
> > by changing only the case of letters).
>
> Case sensitivity is a property of a file system, not the OS.
> What if I mount a device with ext3 file system via ext2fsd driver in
> on my Windows workstation? extN have POSIX semantics so it's
> pointless to enforce case insensitivity on them. The same possibly
> applies to NFS mounts.
>
> Also note that NTFS (at least by default) is case insensitive but is
> case preserving, observe:
[...]
On the other hand, on NTFS, if I unset core.ignorecase or set it to
false locally, `git mv foo Foo` fails to rename a tracked file "foo"
with the "destination file exists" error. I would say I would expect it
to work under the conditions I've just described. Not sure if this
thould be considered a bug in Git for Windows or not -- would be great
to hear opinions of the msysgit port developers.
^ permalink raw reply
* Re: Set core.ignorecase globally
From: Konstantin Khomoutov @ 2012-11-02 15:03 UTC (permalink / raw)
To: Kirill Likhodedov; +Cc: git
In-Reply-To: <6DB2AD2A-6531-497E-B87E-0E884C4BCFBE@jetbrains.com>
On Fri, 2 Nov 2012 18:39:26 +0400
Kirill Likhodedov <Kirill.Likhodedov@jetbrains.com> wrote:
> Currently, core.ignorecase is set to true on case insensitive system
> like Windows or Mac on `git init` and `git clone`, and this setting
> is local to the created/cloned repository.
[...]
> I suggest to set this globally by default when Git is installed,
> because there is little sense to have this option false on case
> insensitive systems (it will lead to confusions when renaming a file
> by changing only the case of letters).
Case sensitivity is a property of a file system, not the OS.
What if I mount a device with ext3 file system via ext2fsd driver in on
my Windows workstation? extN have POSIX semantics so it's pointless to
enforce case insensitivity on them. The same possibly applies to NFS
mounts.
Also note that NTFS (at least by default) is case insensitive but is
case preserving, observe:
C:\tmp>dir /b
foo
C:\tmp>rename foo Foo
C:\tmp>dir /b
Foo
C:\tmp>del fOO
C:\tmp>dir /b
C:\tmp>ver
Microsoft Windows XP [Версия 5.1.2600]
I don't really know what to make out of this, but I'd not change the
defaults until the reasons to do this are not really pressing (and
they're not for now).
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Jonathan Nieder @ 2012-11-02 14:55 UTC (permalink / raw)
To: Jeff King
Cc: Felipe Contreras, git, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121102131255.GB2598@sigill.intra.peff.net>
Jeff King wrote:
> If so, then this series isn't regressing behavior; the only downside is
> that it's an incomplete fix. In theory this could get in the way of the
> full fix later on, but given the commit messages and the archive of this
> discussion, it would be simple enough to revert it later in favor of a
> more full fix. Is that accurate?
>
> Sorry if I am belaboring the discussion. I just want to make sure I
> understand the situation before deciding what to do with the topic. It
> sounds like the consensus at this point is "not perfect, but good enough
> to make forward progress".
Patch 1, 2, and 4 are good modulo their descriptions. They should
work fine without patch 3.
Patch 3 is a regression in comprehensibility. I think we can do
better. Maybe all it would take is a less confusing description, and
tweaks to the code (to loop over revs->cmdline instead of
revs->pending) could come on top.
^ permalink raw reply
* Re: [PATCH] gitweb.perl: fix %highlight_ext
From: Jeff King @ 2012-11-02 14:54 UTC (permalink / raw)
To: rh; +Cc: git
In-Reply-To: <20121029094207.deafafed22ea70fbc4fbd4bb@lavabit.com>
On Mon, Oct 29, 2012 at 09:42:07AM -0700, rh wrote:
> I also consolidated exts where applicable.
> i.e. c and h maps to c
>
>
> --
>
> diff --git a/a/gitweb.cgi b/b/gitweb.cgi
> index 060db27..155b238 100755
> --- a/a/gitweb.cgi
> +++ b/b/gitweb.cgi
> @@ -246,19 +246,19 @@ our %highlight_basename = (
> 'Makefile' => 'make',
> );
> # match by extension
> +
> our %highlight_ext = (
> # main extensions, defining name of syntax;
> # see files in /usr/share/highlight/langDefs/ directory
> - map { $_ => $_ }
> - qw(py c cpp rb java css php sh pl js tex bib xml awk bat ini spec tcl sql make),
> + (map { $_ => $_ } qw(py rb java css js tex bib xml awk bat ini spec tcl sql)),
> # alternate extensions, see /etc/highlight/filetypes.conf
> - 'h' => 'c',
> - map { $_ => 'sh' } qw(bash zsh ksh),
> - map { $_ => 'cpp' } qw(cxx c++ cc),
> - map { $_ => 'php' } qw(php3 php4 php5 phps),
> - map { $_ => 'pl' } qw(perl pm), # perhaps also 'cgi'
> - map { $_ => 'make'} qw(mak mk),
> - map { $_ => 'xml' } qw(xhtml html htm),
> + (map { $_ => 'c' } qw(c h)),
> + (map { $_ => 'sh' } qw(sh bash zsh ksh)),
> + (map { $_ => 'cpp' } qw(cpp cxx c++ cc)),
> + (map { $_ => 'php' } qw(php php3 php4 php5 phps)),
> + (map { $_ => 'pl' } qw(pl perl pm)), # perhaps also 'cgi'
> + (map { $_ => 'make'} qw(make mak mk)),
> + (map { $_ => 'xml' } qw(xml xhtml html htm)),
I think the patch itself looks OK, but:
1. It isn't formatted to apply with git-am. Please use
git-format-patch.
2. The commit message does not explain the reason for the change.
3. It isn't signed-off.
The first two are things I can fix up (though it is inconvenient for me
to do so), but the third is a show-stopper. Please look through
Documentation/SubmittingPatches, especially the bit about the
Developer's Certificate of Origin, and re-send.
-Peff
^ permalink raw reply
* Re: [PATCH v4 00/13] New remote-hg helper
From: Jeff King @ 2012-11-02 14:48 UTC (permalink / raw)
To: Felipe Contreras
Cc: Michael J Gruber, Johannes Schindelin, git, Junio C Hamano,
Sverre Rabbelier, Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <CAMP44s3UHQE69O__EVK29uN_VPdZN=a0-Gczeh-Tbjp1ZAAbJw@mail.gmail.com>
On Thu, Nov 01, 2012 at 05:08:52AM +0100, Felipe Contreras wrote:
> > Turns out msysgit's remote-hg is not exporting the whole repository,
> > that's why it's faster =/
>
> It seems the reason is that it would only export to the point where
> the branch is checked out. After updating the to the tip I noticed
> there was a performance difference.
>
> I investigated and found two reasons:
>
> 1) msysgit's version doesn't export files twice, I've now implemented the same
> 2) msysgit's version uses a very simple algorithm to find out file changes
>
> This second point causes msysgit to miss some file changes. Using the
> same algorithm I get the same performance, but the output is not
> correct.
Do you have a test case that demonstrates this? It would be helpful for
reviewers, but also helpful to msysgit people if they want to fix their
implementation.
-Peff
^ permalink raw reply
* Re: RFD: fast-import is picky with author names (and maybe it should - but how much so?)
From: Michael J Gruber @ 2012-11-02 14:47 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <5093DC0C.5000603@drmicha.warpmail.net>
Some additional input:
[mjg@localhost git]$ git commit --author='"is this<ok@or.not>"
<whats@up>' --allow-empty -m test
[detached HEAD 0734308] test
Author: is thisok@or.not <whats@up>
[mjg@localhost git]$ git show
commit 0734308b7bf372227bf9f5b9fd6b4b403df33b9e
Author: is thisok@or.not <whats@up>
Date: Fri Nov 2 15:45:23 2012 +0100
test
^ permalink raw reply
* Re: [PATCH v4 00/13] New remote-hg helper
From: Jeff King @ 2012-11-02 14:46 UTC (permalink / raw)
To: Felipe Contreras
Cc: Michael J Gruber, Johannes Schindelin, git, Junio C Hamano,
Sverre Rabbelier, Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <CAMP44s2PDZwTW55NDho9DyB2XZmsG0-KH4e78grJ2OFRVZkfjg@mail.gmail.com>
On Wed, Oct 31, 2012 at 05:11:39PM +0100, Felipe Contreras wrote:
> > As a patch
> > submitter, you ("generic you") want the attention of others as
> > reviewers. It's in your own (again "generic you") interest not to put
> > them off, in the same way as it's up to the submitter to argue why a
> > patch is desirable and correct.
>
> Ah, so you are making me a favor by reviewing the code?
I do not want to get embroiled in a discussion of manners and netiquette
(or, for that matter, nazis). But I think this point is worth calling
attention to, because it seems to be at the crux of the matter.
Basically, my opinion is that yes, he is doing a favor to you by
reviewing the code. Just as you have done us a favor by submitting the
code. And this is not specific to this topic or to you as a submitter.
It is a part of how the open source process works.
We have an existing code base that works well. It certainly has some
bugs, and it certainly is missing some features. But people use it every
day and are happy. The maintainers of that code base would want it to
improve over time, but they would also have to be careful not to
introduce regressions. And not just specific regressions in behavior; I
mean regressions in overall quality. A half-implemented feature that
crashes is worse than no feature at all. A change that fixes one bug but
hurts the readability of the code, leading to many future bugs, is a net
negative.
So when a contributor shows up with code, we are very grateful that
they've spent their time improving our software. But at the same time,
we must recognize that the contributor is generally scratching their own
itch. And we must make sure that in doing so, they did not hurt other
people's use cases, nor regress the overall quality of the code base.
It is the job of the maintainer to measure the risk and reward of each
change and to find a balance in accepting patches. But it's difficult to
do alone, and that is why volunteer reviewers on the list are very
valuable. They distribute the reviewing load across many brains, and in
many cases have expertise in particular areas that the maintainer can
rely on.
A submitter has scratched their own itch by writing the code. But if
they cannot cooperate with reviewers enough to get feedback, then the
maintainer has only two choices: review the patches themselves, or
reject the change. And when there is conflict with the regular reviewers
and the submitter, it is a red flag to the maintainer that it might not
be worth spending a lot of time there.
Does the code base suffer for this in the end? Perhaps. There are
features we might reject that could have benefited everybody. But we
might also be saving ourselves from the headaches caused by poorly
thought-out changes. The system cannot work if everybody does not show
up and cooperate.
Now, as for this specific topic: it is proposed for contrib, which means
that expectations are lower, and the rest of git does not suffer too
much if it has rough edges. At the same time, it also means that it
could live fairly easily outside of the tree. In fact, I think Michael
and others have been reasonably happy with their own out-of-tree
implementation.
I do think the proliferation of various implementations has made it hard
for users to see which ones are worth trying. So I think there is value
in carrying something in contrib/, as it would focus the attention of
users, and of other developers to make improvements.
So I think what I'd like to do is take your latest series into pu, with
the intention of merging it into next soon, and then cooking it in next
for a while. That would hopefully make it very easy for people following
'next' to try it out and see how it compares in the field with other
tools they have used (the msysgit one, or others).
I'm a little worried about hurting progress on the msysgit version; it
sounds like the functionality of your implementation is at parity with
that one (thanks to both you and Johannes for answering my other email
asking for a summary). Johannes did mention that the design of their
tool was meant to eventually facilitate more backends. That's something
that might be valuable; on the other hand, that development hasn't been
happening, and there has been no effort lately on getting it merged into
git.git. I don't want to hold working code hostage to a future plan that
might or might not happen. So I hope by keeping it in next for a bit,
that will give msysgit people time to check it out and mobilize their
efforts to improve their version if they would like.
-Peff
^ permalink raw reply
* Set core.ignorecase globally
From: Kirill Likhodedov @ 2012-11-02 14:39 UTC (permalink / raw)
To: git
Hi,
Currently, core.ignorecase is set to true on case insensitive system like Windows or Mac on `git init` and `git clone`, and this setting is local to the created/cloned repository.
Here is the man entry:
core.ignorecase
If true, this option enables various workarounds to enable git to work better on filesystems that are
not case sensitive, like FAT. For example, if a directory listing finds "makefile" when git expects
"Makefile", git will assume it is really the same file, and continue to remember it as "Makefile".
The default is false, except git-clone(1) or git-init(1) will probe and set core.ignorecase true if
appropriate when the repository is created.
I suggest to set this globally by default when Git is installed, because there is little sense to have this option false on case insensitive systems (it will lead to confusions when renaming a file by changing only the case of letters).
----------------------------------
Kirill Likhodedov
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"
^ permalink raw reply
* RFD: fast-import is picky with author names (and maybe it should - but how much so?)
From: Michael J Gruber @ 2012-11-02 14:43 UTC (permalink / raw)
To: Git Mailing List
It seems that our fast-import is super picky with regards to author
names. I've encountered author names like
Foo Bar<foo.bar@dev.null>
Foo Bar <foo.bar@dev.null
foo.bar@dev.null
in the self-hosting repo of some other dvcs, and the question is how to
translate them faithfully into a git author name. In general, we try to do
fullotherdvcsname <none@none>
if the other system's entry does not parse as a git author name, but
fast-import does not accept either of
Foo Bar<foo.bar@dev.null> <none@none>
"Foo Bar<foo.bar@dev.null>" <none@none>
because of the way it parses for <>. While the above could be easily
turned into
Foo Bar <foo.bar@dev.null>
it would not be a faithful representation of the original commit in the
other dvcs.
So the question is:
- How should we represent botched author entries faithfully?
As a cororollary, fast-import may need to change or not.
Michael
P.S.: Yes, dvcs=hg, and the "earlier" remote-hg helper chokes on these.
garbage in crash out :(
^ permalink raw reply
* Re: bisect: "Needed a single revision" message is confusing
From: Michael J Gruber @ 2012-11-02 14:10 UTC (permalink / raw)
To: Daniel Bonniot; +Cc: git
In-Reply-To: <CAO5QaZkZYxvSYoAQGgrrH1ULM8rnwX5=0Ln-_zfh8OwdegNNoA@mail.gmail.com>
Daniel Bonniot venit, vidit, dixit 02.11.2012 14:23:
> Hi,
>
> Suppose I'm doing a git bisect, say:
>
> $ git bisect good 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0
>
> That works fine. The sha1 could also be a substring, as long as it's
> unambiguous, e.g.:
>
> $ git bisect good 8c7a786b6c
>
> Now if it's ambiguous, I get an error message:
>
> $ git bisect good 8
> fatal: Needed a single revision
> Bad rev input: 8
>
> All fine and good. But what if I somehow input a non-existing sha1 (in
> my case see [1]), e,g:
>
> $ git bisect good 8c7a786b6c8eae8eac91083cdc9a6e337bc133b1
> fatal: Needed a single revision
> Bad rev input: 8c7a786b6c8eae8eac91083cdc9a6e337bc133b1
>
> I understand that technically both "no revision" and "multiple
> revisions" qualify as "not a single revision", but they correspond to
> quite different situations. I think it would be more helpful to get
> different error messages, something like:
>
> Bad rev input: 8 refers to multiple revisions
> Bad rev input: 8c7a786b6c8eae8eac91083cdc9a6e337bc133b1 does not refer
> to a valid revision
>
> (and avoid outputing the "fatal: Needed a single revision" message).
>
> Is this a good idea? Anybody can think of better error messages?
>
> I'm not familiar with the code base at all, but I could give a try at
> implementing it, unless it's trivial enough that someone does it
> earlier. After a quick look, it looks like either git-bisect itself or
> rev-parse would need to be touched, any pointers and hints welcome.
>
> Cheers,
>
> Daniel
>
> [1] if you want to know, I got a sha1 from one repository and used it
> in another, which probably should work, except that when using git-svn
> they don't. And the "single revision" error message lead me on a
> tangent.
>
The error comes from rev-parse, which is called by bisect. The problem
is that
git rev-parse deadbeef
git rev-parse --verify deadbeef
give two very different error messages (if there's no dead beef there),
and that "git ref-parse --verify" gives the same error message for
non-existing as for ambiguous revs.
There are 3 places in builtins/rev-parse.c which call
die_no_single_rev(), and at least some of them should probably choose
the error message more carefully.
Cheers,
Michael
^ permalink raw reply
* Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
From: Jeff King @ 2012-11-02 14:00 UTC (permalink / raw)
To: Stefano Lattarini
Cc: Felipe Contreras, git, Junio C Hamano, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <5093D0DD.3050801@gmail.com>
On Fri, Nov 02, 2012 at 02:55:41PM +0100, Stefano Lattarini wrote:
> > --- /dev/null
> > +++ b/git-remote-testgit
> > @@ -0,0 +1,62 @@
> > +#!/bin/bash
> >
> I think git can't assume the existence of bash unconditionally, neither
> in its scripts, nor in its tests (the exception being the tests on
> bash completion, of course). This script probably need to be re-written
> to be a valid POSIX shell script.
No, we can't assume bash. But this is replacing a script written in
python, which we also can't assume. So if it were optional, and skipped
gracefully when bash was not available, that would be OK. Of course if
it could be done in portable bourne shell, that would be even better (I
haven't looked closely, but your comments all seem reasonable).
-Peff
^ permalink raw reply
* Re: [PATCH v4 09/14] remote-testgit: report success after an import
From: Stefano Lattarini @ 2012-11-02 13:58 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <1351821738-17526-10-git-send-email-felipe.contreras@gmail.com>
On 11/02/2012 03:02 AM, Felipe Contreras wrote:
> Doesn't make a difference for the tests, but it does for the ones
> seeking reference.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> git-remote-testgit | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> index 6c348b0..4e8b356 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -59,7 +59,18 @@ while read line; do
> sed -e "s#refs/heads/#${prefix}/heads/#g"
> ;;
> export)
> + declare -A before after
> +
If you convert this script to be a valid POSIX shell script (as I've
suggested in my reply to [PATCH v4 04/14]), you'll need to get rid of
this bashism (and those below) as well.
> + eval $(git for-each-ref --format='before[%(refname)]=%(objectname)')
> +
> git fast-import --{import,export}-marks="$testgitmarks" --quiet
> +
> + eval $(git for-each-ref --format='after[%(refname)]=%(objectname)')
> +
> + for ref in "${!after[@]}"; do
> + test "${before[$ref]}" == "${after[$ref]}" && continue
> + echo "ok $ref"
> + done
> echo
> ;;
> '')
Regards,
Stefano
^ permalink raw reply
* Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
From: Stefano Lattarini @ 2012-11-02 13:55 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <1351821738-17526-5-git-send-email-felipe.contreras@gmail.com>
On 11/02/2012 03:02 AM, Felipe Contreras wrote:
> It's way simpler. It exerceises the same features of remote helpers.
> It's easy to read and understand. It doesn't depend on python.
>
> It does _not_ exercise the python remote helper framework; there's
> another tool and another test for that.
>
> For now let's just copy the old remote-helpers test script, although
> some of those tests don't make sense for this testgit (they still pass).
>
> In addition, this script would be able to test other features not
> currently being tested.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> Documentation/git-remote-testgit.txt | 2 +-
> git-remote-testgit | 62 ++++++++++++++++
> t/t5801-remote-helpers.sh | 134 +++++++++++++++++++++++++++++++++++
> 3 files changed, 197 insertions(+), 1 deletion(-)
> create mode 100755 git-remote-testgit
> create mode 100755 t/t5801-remote-helpers.sh
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> new file mode 100755
> index 0000000..6650402
> --- /dev/null
> +++ b/git-remote-testgit
> @@ -0,0 +1,62 @@
> +#!/bin/bash
>
I think git can't assume the existence of bash unconditionally, neither
in its scripts, nor in its tests (the exception being the tests on
bash completion, of course). This script probably need to be re-written
to be a valid POSIX shell script.
It almost is, anyway, apart from the nits below ...
> +# Copyright (c) 2012 Felipe Contreras
> +
> +alias="$1"
>
Just FYI: the double quoting here (and in several variable assignments
below) is redundant. You can portably write it as:
alias=$1
and still be safe in the face of spaces and metacharacters in $1.
I'm not sure whether the Git coding guidelines suggest the use of
quoting in this situation though; if this is the case, feel free
to disregard my observation.
> +url="$2"
> +
> +# huh?
> +url="${url#file://}"
> +
> +dir="$GIT_DIR/testgit/$alias"
> +prefix="refs/testgit/$alias"
> +refspec="refs/heads/*:${prefix}/heads/*"
> +
> +gitmarks="$dir/git.marks"
> +testgitmarks="$dir/testgit.marks"
> +
> +export GIT_DIR="$url/.git"
> +
I believe this should be rewritten as:
GIT_DIR="$url/.git"; export GIT_DIR
in order to be portable to all the POSIX shells targeted by Git.
> +mkdir -p "$dir"
> +
> +test -e "$gitmarks" || echo -n > "$gitmarks"
> +test -e "$testgitmarks" || echo -n > "$testgitmarks"
> +
The '-n' option to echo is not portable. To create an empty
file, you can just use
: > file
or
true > file
> +while read line; do
> + case "$line" in
>
Useless double quoting (my previous observation about Git coding
guidelines applies here as well, of course).
> + capabilities)
> + echo 'import'
> + echo 'export'
> + echo "refspec $refspec"
> + echo "*import-marks $gitmarks"
> + echo "*export-marks $gitmarks"
> + echo
> + ;;
> + list)
> + git for-each-ref --format='? %(refname)' 'refs/heads/'
> + head=$(git symbolic-ref HEAD)
> + echo "@$head HEAD"
> + echo
> + ;;
> + import*)
> + # read all import lines
> + while true; do
> + ref="${line#* }"
> + refs="$refs $ref"
> + read line
> + test "${line%% *}" != "import" && break
> + done
> +
> + echo "feature import-marks=$gitmarks"
> + echo "feature export-marks=$gitmarks"
> + git fast-export --use-done-feature --{import,export}-marks="$testgitmarks" $refs | \
>
Better avoid the tricky {foo,bar} bashism:
git fast-export --use-done-feature \
--import-marks="$testgitmarks" \
--export-marks="$testgitmarks" \
$refs | \
> + sed -e "s#refs/heads/#${prefix}/heads/#g"
> + ;;
> + export)
> + git fast-import --{import,export}-marks="$testgitmarks" --quiet
>
Here too.
> + echo
> + ;;
> + '')
> + exit
>
I'd put an explicit exit status here, for clarity (this is a matter
of personal preference, so feel free to disregard this nit).
> + ;;
> + esac
> +done
Regards,
Stefano
^ permalink raw reply
* Re: Overlong lines with git-merge --log
From: Stephen Bash @ 2012-11-02 13:49 UTC (permalink / raw)
To: Tim Janik; +Cc: git, Jeff King
In-Reply-To: <5093C98D.90807@gnu.org>
----- Original Message -----
> From: "Tim Janik" <timj@gnu.org>
> Sent: Friday, November 2, 2012 9:24:29 AM
> Subject: Re: Overlong lines with git-merge --log
>
> On 02.11.2012 11:05, Jeff King wrote:
>
> > Taking just the first line of those often cuts off the useful part.
> > It was deemed less bad to show the whole message as a subject rather
> > than cut it off arbitrarily.
>
> Thanks a lot for the explanation, I'm using git directly here, but the
> two cases I had indeed lacked this newline.
FWIW we use merge --log quite extensively here at the office, and I've developed a habit to skim the extremely long lines and attempt to cut them intelligently (something I don't trust the computer to do for me). Sometimes that means taking the second or third sentence if it's a better summary, sometimes it's just abbreviating the first. Now that merge automatically spawns an editor, this is quite convenient (though it does take a bit longer).
Thanks,
Stephen
^ permalink raw reply
* Re: Overlong lines with git-merge --log
From: Tim Janik @ 2012-11-02 13:24 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121102100519.GC30221@sigill.intra.peff.net>
On 02.11.2012 11:05, Jeff King wrote:
> Taking just the first line of those often cuts off the useful part. It
> was deemed less bad to show the whole message as a subject rather than
> cut it off arbitrarily.
Thanks a lot for the explanation, I'm using git directly here, but the
two cases I had indeed lacked this newline.
--
Yours sincerely,
Tim Janik
---
http://timj.testbit.eu/ - Free software Author
^ permalink raw reply
* bisect: "Needed a single revision" message is confusing
From: Daniel Bonniot @ 2012-11-02 13:23 UTC (permalink / raw)
To: git
Hi,
Suppose I'm doing a git bisect, say:
$ git bisect good 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0
That works fine. The sha1 could also be a substring, as long as it's
unambiguous, e.g.:
$ git bisect good 8c7a786b6c
Now if it's ambiguous, I get an error message:
$ git bisect good 8
fatal: Needed a single revision
Bad rev input: 8
All fine and good. But what if I somehow input a non-existing sha1 (in
my case see [1]), e,g:
$ git bisect good 8c7a786b6c8eae8eac91083cdc9a6e337bc133b1
fatal: Needed a single revision
Bad rev input: 8c7a786b6c8eae8eac91083cdc9a6e337bc133b1
I understand that technically both "no revision" and "multiple
revisions" qualify as "not a single revision", but they correspond to
quite different situations. I think it would be more helpful to get
different error messages, something like:
Bad rev input: 8 refers to multiple revisions
Bad rev input: 8c7a786b6c8eae8eac91083cdc9a6e337bc133b1 does not refer
to a valid revision
(and avoid outputing the "fatal: Needed a single revision" message).
Is this a good idea? Anybody can think of better error messages?
I'm not familiar with the code base at all, but I could give a try at
implementing it, unless it's trivial enough that someone does it
earlier. After a quick look, it looks like either git-bisect itself or
rev-parse would need to be touched, any pointers and hints welcome.
Cheers,
Daniel
[1] if you want to know, I got a sha1 from one repository and used it
in another, which probably should work, except that when using git-svn
they don't. And the "single revision" error message lead me on a
tangent.
^ permalink raw reply
* Re: [PATCH] commit: fixup misplacement of --no-post-rewrite description
From: Jeff King @ 2012-11-02 13:22 UTC (permalink / raw)
To: Andreas Schwab; +Cc: git
In-Reply-To: <m2hap88a2g.fsf@igel.home>
On Fri, Nov 02, 2012 at 01:26:47PM +0100, Andreas Schwab wrote:
> In e858af6 (commit: document a couple of options) the description of the
> --no-post-rewrite option was put inside the paragraph for the --amend
> option. Move it down after the paragraph.
>
> Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
Thanks, the modified output looks much more sane.
-Peff
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Jeff King @ 2012-11-02 13:17 UTC (permalink / raw)
To: Felipe Contreras
Cc: Johannes Sixt, Jonathan Nieder, git, Junio C Hamano,
Ævar Arnfjörð
In-Reply-To: <509167C8.6090600@kdbg.org>
On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote:
> Am 31.10.2012 03:28, schrieb Felipe Contreras:
> > On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> >> Felipe Contreras wrote:
> >>
> >>> It's all fun and games to write explanations for things, but it's not
> >>> that easy when you want those explanations to be actually true, and
> >>> corrent--you have to spend time to make sure of that.
> >>
> >> That's why it's useful for the patch submitter to write them, asking
> >> for help when necessary.
> >>
> >> As a bonus, it helps reviewers understand the effect of the patch.
> >> Bugs averted!
> >
> > Yeah, that would be nice. Too bad I don't have that information, and
> > have _zero_ motivation to go and get it for you.
>
> Just to clarify: That information is not just for Jonathan, but for
> everyone on this list and those who dig the history a year down the
> road. Contributors who have _zero_ motiviation to find out that
> information are not welcome here because they cause friction and take
> away time from many others for _zero_ gain.
And me, who is trying to figure out what to do with this patch. It is
presented on its own, outside of a series, with only the description "no
reason not to do this". But AFAICT, it is _required_ for the tests in
the remote-hg series to work. Isn't that kind of an important
motivation?
Yet it is not in the commit message, nor does the remote-hg series
indicate that it should be built on top. Or am I wrong that the one is
dependent on the other?
-Peff
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Jeff King @ 2012-11-02 13:12 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Felipe Contreras, git, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031003721.GV15167@elie.Belkin>
On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.
Right, so my understanding of the situation is that doing this:
$ git branch foo master~1
$ git fast-export foo master~1..master
won't show "foo", which seems wrong to me. _But_ we currently get that
wrong already, so Felipe's patches are not making anything worse, but
are fixing some situations (namely when master~1 is not mentioned on the
command-line, but rather in a marks file).
Is that correct?
If so, then this series isn't regressing behavior; the only downside is
that it's an incomplete fix. In theory this could get in the way of the
full fix later on, but given the commit messages and the archive of this
discussion, it would be simple enough to revert it later in favor of a
more full fix. Is that accurate?
Sorry if I am belaboring the discussion. I just want to make sure I
understand the situation before deciding what to do with the topic. It
sounds like the consensus at this point is "not perfect, but good enough
to make forward progress".
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox