From: "Shawn O. Pearce" <spearce@spearce•org>
To: Dana How <danahow@gmail•com>
Cc: Junio C Hamano <junkio@cox•net>, Git Mailing List <git@vger•kernel.org>
Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()
Date: Tue, 1 May 2007 02:03:40 -0400 [thread overview]
Message-ID: <20070501060340.GD5942@spearce.org> (raw)
In-Reply-To: <56b7f5510704302241n79601619kda8251a9f7776884@mail.gmail.com>
Dana How <danahow@gmail•com> wrote:
> On 4/30/07, Shawn O. Pearce <spearce@spearce•org> wrote:
> >Why not
> >refactor both to use the same implementation and stuff it away in
> >say pack-check.c (for lack of a better place), or start a new file
> >(pack-write.c)?
> Actually I didn't just copy it, I tried to rewrite it for my use
> as well as the fast-import.c use (note there is a 3rd copy
> in some *index*.c file which I didn't try to merge in yet).
> However I didn't yet put it in a new file or change fast-import.c
> to call it since I wanted to change as little as possible.
...
> I agree with all your arguments. I had several reasons
> to avoid extra rearrangements/refactorings:
> (a) First patch to git, not previously known to me;
> (b) I prefer to separate new functionality from "clean-up" work;
A really good reason. ;-)
But I'd still rather see it done right the first time, then done
partially (copied) and wait for someone to clean it up later.
Sometimes that cleanup doesn't happen. Anyway, I'm not against
you copying it, I just think there's a better way (refactor at
least fast-import's use of it).
> (c) I didn't really view myself as the person to make the *writing*
> code in git as well organized/minimized as the *reading* code
> [e.g. the sliding mmap stuff -- nice!];
Not sure I follow that argument, but thanks for the compliment on
the sliding mmap work. I think I was trying to point out that that
one particular function is actually quite simple, and you did a
direct copy of it from fast-import.c. Instead a simple refactoring
that allows both pack-objects.c and fast-import.c share the same
implementation of those 30 or so lines of code would be a good
start towards cleaning up the writing code. Yes it doesn't help
the index-pack.c usage of same logic, but baby steps will help us
to cleanup the mess we have already made!
We like baby steps around here... ;-)
> (d) Apparently you and Nicolas Pitre have a lot of pending changes
> affecting the packing code.
Yes, but Nico's work has also destroyed in pack v4 topic. Nico has
promised to start working on porting some of that work over, but I
don't know if he has been able to start doing so yet. I personally
have been too busy this past month and a half to really work on
packv4, but I'm hoping to circle back to it before the end of May
(if Nico doesn't beat me to it!).
> I'd have no problem submitting a follow-on patch later containing
> some clean-up work if you & NP clear it, so I know I won't have
> problems from (d). Note I had to completely rewrite this patch
> when NP submitted some of his pending stuff.
Yea, hazard of working in this part of the code when Nico is
also active. My own sliding mmap stuff was written twice too,
for the same reason - Nico doing much needed improvements right in
the same spot as I was working, at the same time.
> NP wrote that you posted a summary of v3/v4 pack ideas,
> but I couldn't find it in the list archives. Could you either
> email it to me, or (re-)post it to the list?
You can start here:
http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=40799
http://article.gmane.org/gmane.comp.version-control.git/42423
http://article.gmane.org/gmane.comp.version-control.git/45406
http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=43046
anything by me or Nico in those threads is good reading on pack v4.
The last link is probably the best thread available on the subject.
--
Shawn.
next prev parent reply other threads:[~2007-05-01 6:03 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-04-30 23:21 [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Dana How
2007-05-01 5:06 ` Shawn O. Pearce
2007-05-01 5:41 ` Dana How
2007-05-01 6:03 ` Shawn O. Pearce [this message]
2007-05-01 7:32 ` Johannes Schindelin
2007-05-01 17:48 ` Nicolas Pitre
2007-05-01 17:58 ` Dana How
2007-05-01 18:39 ` Nicolas Pitre
-- strict thread matches above, loose matches on Subject: below --
2007-04-08 23:22 Dana How
2007-04-09 0:04 ` Junio C Hamano
2007-04-09 0:18 ` Nicolas Pitre
2007-04-09 17:38 ` Shawn O. Pearce
2007-04-09 18:30 ` Nicolas Pitre
2007-04-09 18:40 ` Shawn O. Pearce
2007-04-09 19:11 ` Dana How
2007-04-09 19:33 ` Nicolas Pitre
2007-04-09 21:38 ` Dana How
2007-04-09 23:22 ` Nicolas Pitre
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20070501060340.GD5942@spearce.org \
--to=spearce@spearce$(echo .)org \
--cc=danahow@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=junkio@cox$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox