headers
Junio C Hamano | 1 Aug 2012 01:05
Picon
Picon
Favicon
Gravatar

Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson <at> yhbt.net> writes:

> Michael G Schwern <schwern <at> pobox.com> wrote:
>> It just doesn't matter.
>> 
>> Why are we arguing over which solution will be 4% better two years from now,
>> or if my commits are formatted perfectly, when tremendous amounts of basic
>> work to be done improving git-svn?  The code is undocumented, lacking unit
>> tests, difficult to understand and riddled with bugs.
>
> Yes it does matter.
>
> git-svn has the problems it has because it traditionally had lower
> review standards than the rest of git.  So yes, we're being more careful
> nowadays about the long-term ramifications of changes.

Thanks.  I know it takes guts to publicly admit that over time your
own creation has become less ideal than you wish it to be, but it
needed to be said.

Michael, please realize that the only reason people comment on the
patch series is because they care about what the series brings to
us.  In other words, your effort is appreciated.  For a change that
we want to have in our codebase, the functionality of the code
immediately after the change is applied of course is important, but
the maintainability of the result also matters.

We want to make sure that anybody who wants to understand and
improve the system can read the code without distraction from
inconsistent coding styles used in different sections of code.  We
(Continue reading)

Permalink | Reply | 1 comment |
headers
Junio C Hamano | 1 Aug 2012 01:22
Picon
Picon
Favicon
Gravatar

Re: [PATCH/RFC 2/2] grep: rename "grep.extendedRegexp" option to "grep.patternType"

J Smith <dark.panda <at> gmail.com> writes:

> With the addition of the "basic", "extended", "fixed", and "perl"
> values for the "grep.extendedRegexp" option the name "grep.patternType"
> better represents the option's functionality. "grep.extendedRegexp"
> remains available as an alias to "grep.patternType" for the purposes of
> backwards compatibility.
> ---

Sorry for not bringing this up earlier when we discussed grep.patternType,
but my preference would be to introduce grep.patternType with these
type names (including basic and perl) from the beginning, and then
ignore grep.extendedRegexp if grep.patternType is set.

The core part of the change may look something like this...

diff --git a/builtin/grep.c b/builtin/grep.c
index 29adb0a..260a7db 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
 <at>  <at>  -260,6 +260,57  <at>  <at>  static int wait_all(void)
 }
 #endif

+static int parse_pattern_type_arg(const char *opt, const char *arg)
+{
+	switch (git_config_maybe_bool(opt, arg)) {
+	case 1:
+		return GREP_PATTERN_TYPE_ERE;
+	case 0:
(Continue reading)

Permalink | Reply | 1 comment |
headers
Michael G Schwern | 1 Aug 2012 01:24
Picon
Favicon
Gravatar

Re: Fix git-svn for SVN 1.7

On 2012.7.31 1:01 PM, Eric Wong wrote:
> Michael G Schwern <schwern <at> pobox.com> wrote:
>> It just doesn't matter.
>>
>> Why are we arguing over which solution will be 4% better two years from now,
>> or if my commits are formatted perfectly, when tremendous amounts of basic
>> work to be done improving git-svn?  The code is undocumented, lacking unit
>> tests, difficult to understand and riddled with bugs.
> 
> Yes it does matter.
> 
> git-svn has the problems it has because it traditionally had lower
> review standards than the rest of git.  So yes, we're being more careful
> nowadays about the long-term ramifications of changes.

Yes, review does matter.  And so far we've been arguing over whether reviewing
objects-with-overloading or objects-without-overloading would be better.  And
we can argue about that forever.

That's the part that doesn't matter.  People matter.

I think we can all agree that either solution is a vast improvement along
multiple axes, including review.  So what really matters is making sure one of
them gets done.  Once either of them is done, we can see how it works out in
practice instead of arguing theoretical futures.  Once either of them is done,
it's much easier to switch to the other.

What I'm trying to say is I have much less interest in doing it without the
overloading.  It's not interesting to me.  It's no fun.  No fun means no
patch.  No patch means no improvement.  No improvement is the worst of all
(Continue reading)

Permalink | Reply | 1 comment |
headers
Michael G Schwern | 1 Aug 2012 01:28
Picon
Favicon
Gravatar

Re: Fix git-svn for SVN 1.7

On 2012.7.31 4:05 PM, Junio C Hamano wrote:
> What I won't accept is "maintainability does not matter".  It does.

I'm sorry, that's not what I intended to convey at all.  My reply to Eric lays
it out more clearly, I think.

--

-- 
Reality is that which, when you stop believing in it, doesn't go away.
    -- Phillip K. Dick
Permalink | Reply | 0 comments |
headers
Ammon Riley | 1 Aug 2012 01:44
Picon

Broken git-svn tests known?

Hi,

On a freshly checked out copy of the maint branch (0e4c8822), the
t9100-git-svn-basic.sh tests are failing 21 of 25 tests. Is this
known, or am I missing some dependencies? Is it possibly due to
using subversion 1.7?

I've run into a small bug with git-svn, and wanted to make sure
the test suite still passed with my patch applied.

Cheers,
Ammon
Permalink | Reply | 2 comments |
headers
Jeff King | 1 Aug 2012 02:42
Gravatar

Re: [WIP PATCH] Manual rename correction

On Tue, Jul 31, 2012 at 01:20:42PM -0700, Junio C Hamano wrote:

> Jeff King <peff <at> peff.net> writes:
> 
> > A much better hint is to annotate pairs of sha1s, to say "do not bother
> > doing inexact rename correlation on this pair; I promise that they have
> > value N".
> 
> Surely.  And I suspect that the patch to the current codebase to do
> so would be much less impact if you go that way.

Yes. You may remember I wrote a generic caching subsystem last summer
when we were talking about caching commit generations. Plugging in a new
map type to map sha1 pairs to 32-bit integers was pretty simple, and
that gives the basis for a rename cache.

It's fairly unimpressive on git.git. My best-of-five for "git log
--format=%H --raw -M" went from 5.83s to 5.74s, which is pretty much
within the run-to-run noise. The resulting cache was 155K.

However, it's easy to come up with much more pathological cases. I have
a really ugly rename-and-tweak-tags commit on my photo repository, and
those blobs are relatively big. My timings for "git show" on that were:

  before: 49.724s
  after, run 1: 54.904s
  after, run 2:  0.117s

Which is pretty darn nice. The resulting cache is 5.3M (the repository
itself is in the gigabytes, but that's not really relevant; the cache
(Continue reading)

Permalink | Reply | 7 comments |
headers
Nguyen Thai Ngoc Duy | 1 Aug 2012 03:10
Picon
Gravatar

Re: [WIP PATCH] Manual rename correction

On Wed, Aug 1, 2012 at 2:23 AM, Jeff King <peff <at> peff.net> wrote:
>> It is a good direction to go in, I would think, to give users a way
>> to explicitly tell that "in comparison between these two trees, I
>> know path B in the postimage corresponds to path A in the preimage".
>
> I do not think that is the right direction. Let's imagine that I have a
> commit "A" and I annotate it (via notes or whatever) to say "between
> A^^{tree} and A^{tree}, foo.c became bar.c". That will help me when
> doing "git show" or "git log". But it will not help me when I later try
> to merge "A" (or its descendent). In that case, I will compute the diff
> between "A" and the merge-base (or worse, some descendent of "A" and the
> merge-base), and I will miss this hint entirely.
>
> A much better hint is to annotate pairs of sha1s, to say "do not bother
> doing inexact rename correlation on this pair; I promise that they have
> value N".

I haven't had time to think it through yet but I throw my thoughts in
any way. I actually went with your approach first. But it's more
difficult to control the renaming. Assume we want to tell git to
rename SHA-1 "A" to SHA-1 "B". What happens if we have two As in the
source tree and two Bs in the target tree? What happens if two As and
one B, or one A and two Bs? What if a user defines A -> B and A -> C,
and we happen to have two As in source tree and B and C in target
tree?

There's also the problem with transferring this information. With
git-notes I think I can transfer it (though not automatically). How do
we transfer sha1 map (that you mentioned in the commit generation mail
in this thread)?
(Continue reading)

Permalink | Reply | 22 comments |
headers
Jeff King | 1 Aug 2012 04:01
Gravatar

Re: [WIP PATCH] Manual rename correction

On Wed, Aug 01, 2012 at 08:10:12AM +0700, Nguyen Thai Ngoc Duy wrote:

> > I do not think that is the right direction. Let's imagine that I have a
> > commit "A" and I annotate it (via notes or whatever) to say "between
> > A^^{tree} and A^{tree}, foo.c became bar.c". That will help me when
> > doing "git show" or "git log". But it will not help me when I later try
> > to merge "A" (or its descendent). In that case, I will compute the diff
> > between "A" and the merge-base (or worse, some descendent of "A" and the
> > merge-base), and I will miss this hint entirely.
> >
> > A much better hint is to annotate pairs of sha1s, to say "do not bother
> > doing inexact rename correlation on this pair; I promise that they have
> > value N".
> 
> I haven't had time to think it through yet but I throw my thoughts in
> any way. I actually went with your approach first. But it's more
> difficult to control the renaming. Assume we want to tell git to
> rename SHA-1 "A" to SHA-1 "B". What happens if we have two As in the
> source tree and two Bs in the target tree? What happens if two As and
> one B, or one A and two Bs? What if a user defines A -> B and A -> C,
> and we happen to have two As in source tree and B and C in target
> tree?

Yes, it disregards path totally. But if you had the exact same movement
of content from one path to another in one instance, and it is
considered a rename, wouldn't it also be a rename in a second instance?

> There's also the problem with transferring this information. With
> git-notes I think I can transfer it (though not automatically). How do
> we transfer sha1 map (that you mentioned in the commit generation mail
(Continue reading)

Permalink | Reply | 21 comments |
headers
J Smith | 1 Aug 2012 05:38
Picon

Re: [PATCH/RFC 2/2] grep: rename "grep.extendedRegexp" option to "grep.patternType"


On 2012-07-31, at 7:22 PM, Junio C Hamano <gitster <at> pobox.com> wrote:

> J Smith <dark.panda <at> gmail.com> writes:
> 
>> With the addition of the "basic", "extended", "fixed", and "perl"
>> values for the "grep.extendedRegexp" option the name "grep.patternType"
>> better represents the option's functionality. "grep.extendedRegexp"
>> remains available as an alias to "grep.patternType" for the purposes of
>> backwards compatibility.
>> ---
> 
> Sorry for not bringing this up earlier when we discussed grep.patternType,
> but my preference would be to introduce grep.patternType with these
> type names (including basic and perl) from the beginning, and then
> ignore grep.extendedRegexp if grep.patternType is set.
> 
> The core part of the change may look something like this...

Ah, I see. Yeah, that's not a problem. I'll make the appropriate changes tomorrow and post a new patch. --
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Nguyen Thai Ngoc Duy | 1 Aug 2012 06:36
Picon
Gravatar

Re: [WIP PATCH] Manual rename correction

On Wed, Aug 1, 2012 at 9:01 AM, Jeff King <peff <at> peff.net> wrote:
> On Wed, Aug 01, 2012 at 08:10:12AM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> > I do not think that is the right direction. Let's imagine that I have a
>> > commit "A" and I annotate it (via notes or whatever) to say "between
>> > A^^{tree} and A^{tree}, foo.c became bar.c". That will help me when
>> > doing "git show" or "git log". But it will not help me when I later try
>> > to merge "A" (or its descendent). In that case, I will compute the diff
>> > between "A" and the merge-base (or worse, some descendent of "A" and the
>> > merge-base), and I will miss this hint entirely.
>> >
>> > A much better hint is to annotate pairs of sha1s, to say "do not bother
>> > doing inexact rename correlation on this pair; I promise that they have
>> > value N".
>>
>> I haven't had time to think it through yet but I throw my thoughts in
>> any way. I actually went with your approach first. But it's more
>> difficult to control the renaming. Assume we want to tell git to
>> rename SHA-1 "A" to SHA-1 "B". What happens if we have two As in the
>> source tree and two Bs in the target tree? What happens if two As and
>> one B, or one A and two Bs? What if a user defines A -> B and A -> C,
>> and we happen to have two As in source tree and B and C in target
>> tree?
>
> Yes, it disregards path totally. But if you had the exact same movement
> of content from one path to another in one instance, and it is
> considered a rename, wouldn't it also be a rename in a second instance?

Yes. This is probably cosmetics only, but without path information, we
leave it to chance to decide which A to pair with B and C (in the
(Continue reading)

Permalink | Reply | 20 comments |

Gmane