* 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
* 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
* 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
* 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
* 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: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: [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 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: 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: 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: [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: [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: 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: 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] 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 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 v3 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-11-02 15:35 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Jonathan Nieder, git, Junio C Hamano,
Sverre Rabbelier, Elijah Newren
In-Reply-To: <20121102151955.GA24622@sigill.intra.peff.net>
On Fri, Nov 2, 2012 at 4:19 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote:
>> 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?
Yes. I noticed this patch fixes other tests.
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:38 UTC (permalink / raw)
To: karsten.blees; +Cc: git, msysgit, pro-logic
In-Reply-To: <20121102152616.GD11170@sigill.intra.peff.net>
On Fri, Nov 02, 2012 at 11:26:16AM -0400, Jeff King wrote:
> 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.
BTW, your patch was badly damaged in transit (wrapped, and tabs
converted to spaces). I was able to fix it up, but please check your
mailer's settings.
-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 v4 04/14] Add new simplified git-remote-testgit
From: Felipe Contreras @ 2012-11-02 15:42 UTC (permalink / raw)
To: Stefano Lattarini
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <5093D0DD.3050801@gmail.com>
On Fri, Nov 2, 2012 at 2:55 PM, Stefano Lattarini
<stefano.lattarini@gmail.com> wrote:
>> +#!/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.
Well, this is a _reference_ script, and that is used only for testing
purposes. The test itself can be like the bash completion tests, and
simply be skipped.
The reason I chose bash is because associative arrays, which you see
in a later patch.
> 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.
What happens when you call this with:
./script "alias with spaces"
>> +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.
_If_ we want this as POSIX, yeah.
>> +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
All right, thanks.
>> +while read line; do
>> + case "$line" in
>>
> Useless double quoting (my previous observation about Git coding
> guidelines applies here as well, of course).
What if line has multiple spaces? To me it makes sense to quote it.
>> + 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 | \
If that's what we want, yeah.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v4 09/14] remote-testgit: report success after an import
From: Felipe Contreras @ 2012-11-02 15:46 UTC (permalink / raw)
To: Stefano Lattarini
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <5093D193.3030108@gmail.com>
On Fri, Nov 2, 2012 at 2:58 PM, Stefano Lattarini
<stefano.lattarini@gmail.com> wrote:
> 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.
Yeah, but I don't want to. I originally used transitory files, and
used esoteric options of diff to do the same (which were probably not
portable).
In the end I liked this approach much better.
If you have a solution for this that works in POSIX shell, I'll be
glad to consider it, but for the moment, I think a simple, easy to
understand and maintain code is more important, and if it needs bash,
so be it.
>> + 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
>> ;;
>> '')
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
From: Stefano Lattarini @ 2012-11-02 16:03 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <CAMP44s0n_O-7j329WyTxsWgVbCV2mEAnPG-WQeWBFdmHdfOjnQ@mail.gmail.com>
On 11/02/2012 04:42 PM, Felipe Contreras wrote:
> On Fri, Nov 2, 2012 at 2:55 PM, Stefano Lattarini
> <stefano.lattarini@gmail.com> wrote:
>
>>> +#!/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.
>
> Well, this is a _reference_ script, and that is used only for testing
> purposes. The test itself can be like the bash completion tests, and
> simply be skipped.
>
> The reason I chose bash is because associative arrays, which you see
> in a later patch.
>
>> 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.
>
> What happens when you call this with:
>
> ./script "alias with spaces"
>
'$alias' will correctly expand to "alias with spaces". Try out:
$ sh -c 'alias=$1; echo "$alias"' dummy '1 2*3'
1 2*3
This works consistently with every known shell (even non-POSIX
relics like Solaris /bin/sh).
>>> +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.
>
> _If_ we want this as POSIX, yeah.
>
Why don't we? Why add an extra requirement for a test that
1. can be easily written in POSIX shell, and
2. tests a feature that doesn't require bash to work (unless
I'm sorely mistaken, that is)?
Honest question. But of course, if the Git active contributors
deem the extra requirement (which is not an invasive one, given
how often bash is installed even on non-Linux systems) acceptable
in order to have the test case simpler and clearer, feel free to
disregard all my observations in this thread.
>>> +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
>
> All right, thanks.
>
>>> +while read line; do
>>> + case "$line" in
>>>
>> Useless double quoting (my previous observation about Git coding
>> guidelines applies here as well, of course).
>
> What if line has multiple spaces?
>
Still no problem, as in the case of the 'alias=$1' assignment before:
$ sh -c 'case $1 in *x" "x*) echo ok;; *) exit 1;; esac' dummy 'x x'
ok
> To me it makes sense to quote it.
>
Surely it doesn't cause any problem to "over-quote" in this case;
it's better than risking to under-quote in other. I just pointed
out that the quoting it's not really necessary, in case you weren't
aware of that.
>>> + 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 | \
>
> If that's what we want, yeah.
>
Honestly, I find my longer-and-more-explicit version clearer, even
if you can assume bash for your script. But that's a matter of
personal preference (sorry for not stating that right away), so
feel free to ignore it if you decide to keep the bash requirement
in the end.
Regards,
Stefano
^ permalink raw reply
* Re: [PATCH v4 09/14] remote-testgit: report success after an import
From: Stefano Lattarini @ 2012-11-02 16:08 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <CAMP44s2ZPbda7yJ9UtOhWMqaKp4TaAgoyeUWUSrt0vco7RK+Tw@mail.gmail.com>
On 11/02/2012 04:46 PM, Felipe Contreras wrote:
>
> In the end I liked this approach much better.
>
> If you have a solution for this that works in POSIX shell, I'll be
> glad to consider it, but for the moment, I think a simple, easy to
> understand and maintain code is more important, and if it needs bash,
> so be it.
>
If this is a deliberate decision, it's ok with me. I'm just a "casual"
reviewer here, not an active contributor, so I'll gladly accept
preferences and decisions of the "active crew", once it's clear that
they are deliberate and not the result of mistakes or confusion.
In any case, I agree that having a clean, understandable code as a
starting point is better than having a more "portable" but trickier
one right away. If it will need converting to POSIX, that can be
done as a follow up (and as we've both noticed, this would be the
only point where such a conversion might be problematic -- the other
changes would be trivial, almost automatic).
Regards,
Stefano
^ permalink raw reply
* Re: Lack of netiquette, was Re: [PATCH v4 00/13] New remote-hg helper
From: Felipe Contreras @ 2012-11-02 16:09 UTC (permalink / raw)
To: Michael J Gruber
Cc: Andreas Ericsson, René Scharfe, Junio C Hamano,
Johannes Schindelin, Jonathan Nieder, Jeff King, git,
Sverre Rabbelier, Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <5093A873.9090701@drmicha.warpmail.net>
On Fri, Nov 2, 2012 at 12:03 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Andreas Ericsson venit, vidit, dixit 02.11.2012 10:38:
>> On 11/01/2012 02:46 PM, René Scharfe wrote:
>>>
>>> Also, and I'm sure you didn't know that, "Jedem das Seine" (to each
>>> his own) was the slogan of the Buchenwald concentration camp. For
>>> that reason some (including me) hear the unspoken cynical
>>> half-sentence "and some people just have to be sent to the gas
>>> chamber" when someone uses this proverb.
>>>
>>
>> It goes further back than that.
>>
>> "Suum cuique pulchrum est" ("To each his own is a beautiful thing") is
>> a latin phrase said to be used frequently in the roman senate when
>> senators politely agreed to disagree and let a vote decide the outcome
>> rather than debating further.
>>
>> Please don't let the twisted views of whatever nazi idiot thought it
>> meant "you may have the wrong faith and therefore deserve to die, so you
>> shall" pollute it. The original meaning is both poetic and democratic,
>> and I firmly believe most people have the original meaning to the fore
>> of their mind when using it. After all, very few people knowingly quote
>> nazi concentration camp slogans.
>>
>
> In fact, many German terms and words are "forbidden area" since Nazi
> times, but I don't think this one carries the same connotation.
>
> But that is a side track.
>
> Collaboration (and code review is a form of collaboration) requires
> communication. The linked code of conduct pages describe quite well how
> to ensure a productive environment in which "everyone" feels comfortable
> communicating and collaborating.
Yes, but that's assuming we want "everyone" to feel comfortable
communicating and collaborating. I cite again the example of the Linux
kernel, where certainly not "everyone" feels that way. But somehow
they manage to be perhaps the most successful software project in
history. And I would argue even more: it's _because_ not everyone
feels comfortable, it's because ideas and code are criticized freely,
and because only the ones that do have merit stand. If you are able to
take criticism, and you are not emotionally and personally attacked to
your code and your ideas, you would thrive in this environment. If you
don't want your precious little baby code to fight against the big
guys, then you shouldn't send it out to the world.
Junio mentioned "technical merit", and I believe for that open and
_honest_ communication is more important than making "everyone" feel
comfortable.
And FWIW I don't feel comfortable expressing my opinion any more,
because even if I criticize ideas and code on a *technical* basis, I'm
assumed to be referencing Nazism and whatnot without any regards of
what my original intentions were, or what I actually said, and
definitely not assuming good faith. And when asked for clarification
of what exactly that I said was offensive, I get no clear answer.
The dangers of "everyone" following the same style of communication,
and making "everyone" feel comfortable, is that "everyone" ends up
being the same kind of people, and the ones that don't fit the
definition of "everyone" feel like outsiders, or outright leave the
project. And you end up with an homogeneous group of people incapable
of criticizing each other honestly (on a technical basis), whether
it's because of lack of a different perspective, or unwillingness to
speak openly, or difficulty in finding the right polite words. I've
seen many projects fall into this, and erode with time, since nothing
important actually happens, and real deep issues within the code or
the community get ignored.
Anyway, I've yet to find what was actually wrong in the words I said.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
From: Felipe Contreras @ 2012-11-02 16:16 UTC (permalink / raw)
To: Stefano Lattarini
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <5093EEB4.3040402@gmail.com>
On Fri, Nov 2, 2012 at 5:03 PM, Stefano Lattarini
<stefano.lattarini@gmail.com> wrote:
> On 11/02/2012 04:42 PM, Felipe Contreras wrote:
>> What happens when you call this with:
>>
>> ./script "alias with spaces"
>>
> '$alias' will correctly expand to "alias with spaces". Try out:
>
> $ sh -c 'alias=$1; echo "$alias"' dummy '1 2*3'
> 1 2*3
>
> This works consistently with every known shell (even non-POSIX
> relics like Solaris /bin/sh).
All right.
>> _If_ we want this as POSIX, yeah.
>>
> Why don't we? Why add an extra requirement for a test that
>
> 1. can be easily written in POSIX shell, and
> 2. tests a feature that doesn't require bash to work (unless
> I'm sorely mistaken, that is)?
>
> Honest question. But of course, if the Git active contributors
> deem the extra requirement (which is not an invasive one, given
> how often bash is installed even on non-Linux systems) acceptable
> in order to have the test case simpler and clearer, feel free to
> disregard all my observations in this thread.
Because the code will be more complicated. Most of the changes
required are relatively small, so it's not a big issue, but I would
like to see how you replace the code that uses associative arrays. I
don't know know of a clean, simple way to do it in POSIX. If you can
find one, I don't see any strong reason to use bash.
>>>> +while read line; do
>>>> + case "$line" in
>>>>
>>> Useless double quoting (my previous observation about Git coding
>>> guidelines applies here as well, of course).
>>
>> What if line has multiple spaces?
>>
> Still no problem, as in the case of the 'alias=$1' assignment before:
>
> $ sh -c 'case $1 in *x" "x*) echo ok;; *) exit 1;; esac' dummy 'x x'
> ok
All right.
>>>> + 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 | \
>>
>> If that's what we want, yeah.
>>
> Honestly, I find my longer-and-more-explicit version clearer, even
> if you can assume bash for your script. But that's a matter of
> personal preference (sorry for not stating that right away), so
> feel free to ignore it if you decide to keep the bash requirement
> in the end.
And I prefer the other form. I fact, I would prefer if both tools
simply had a --marks option set both, and didn't require the file to
be created beforehand. I've _never_ seen a situation where separate
marks for import and export made sense. But that's off-topic.
If you find a way to replace the associative arrays code, I have no
trouble in switching this to the POSIX version.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v4 09/14] remote-testgit: report success after an import
From: Felipe Contreras @ 2012-11-02 16:19 UTC (permalink / raw)
To: Stefano Lattarini
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Elijah Newren, Ilari Liusvaara, Sverre Rabbelier
In-Reply-To: <5093EFF4.5080308@gmail.com>
On Fri, Nov 2, 2012 at 5:08 PM, Stefano Lattarini
<stefano.lattarini@gmail.com> wrote:
> On 11/02/2012 04:46 PM, Felipe Contreras wrote:
>>
>> In the end I liked this approach much better.
>>
>> If you have a solution for this that works in POSIX shell, I'll be
>> glad to consider it, but for the moment, I think a simple, easy to
>> understand and maintain code is more important, and if it needs bash,
>> so be it.
>>
> If this is a deliberate decision, it's ok with me. I'm just a "casual"
> reviewer here, not an active contributor, so I'll gladly accept
> preferences and decisions of the "active crew", once it's clear that
> they are deliberate and not the result of mistakes or confusion.
>
> In any case, I agree that having a clean, understandable code as a
> starting point is better than having a more "portable" but trickier
> one right away. If it will need converting to POSIX, that can be
> done as a follow up (and as we've both noticed, this would be the
> only point where such a conversion might be problematic -- the other
> changes would be trivial, almost automatic).
As things are the options are:
1) Remove this code and move to POSIX sh. People looking for reference
might scratch their heads as to why 'git push' is not showing the
update.
2) Keep this code and remain in bash.
Until we have a:
3) Replace this code with a clean POSIX sh alternative
I would rather vote for 2)
Cheers.
--
Felipe Contreras
^ 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