git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Mike Hommey <mh@glandium.org>
Cc: git@vger.kernel.org
Subject: Re: [RFC/PATCH] Add a --nosort option to pack-objects
Date: Fri, 07 Dec 2007 13:25:24 -0800	[thread overview]
Message-ID: <7v4peu19kr.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <1197061832-8489-1-git-send-email-mh@glandium.org> (Mike Hommey's message of "Fri, 7 Dec 2007 22:10:32 +0100")

Mike Hommey <mh@glandium.org> writes:

> The --nosort option disabled the internal sorting used by pack-objects,
> and runs the sliding window along the object list litterally as given on
> stdin.

I think this is a good way to give people an easier way to experiment.

But it makes me wonder if this is disabling too much, and if the list
should be sorted at least by type, as we won't delta different types of
objects against each other.

At the beginning of try_delta(), when we see that the next candidate is
of a different type, we return -1 telling the caller that "No object in
the window will ever be a good delta base for the current object, please
abort".  This relies on the fact that we sort by type first, so I think
one of the following is necessary:
 
 (1) you weaken this check (return 0, saying "This did not delta well but
     do not give up yet"),

 (2) you document this well so that --nosort user will know, or

 (3) you sort --nosort input by type.

>   I would obviously add the appropriate documentation for this flag if this
>   is accepted. I'll also try to send another documentation patch for
>   pack-objects with some information compiled from Linus's explanation to my
>   last message about pack-objects.

I need to rant here a bit.

Sometimes people say "Here is my patch.  If this is accepted, I'll add
documentation and tests".  My reaction is, "Don't you, as the person who
proposes that change, believe in your patch deeply enough yourself to be
willing to perfect it, to make it suitable for consumption by the
general public, whether it is included in my tree or not?  A change that
even you do not believe in deeply enough probably to perfect would not
benefit the general public, so thanks but no thanks, I'll pass."

Fortunately we haven't had this problem too many times on this list.

I would not have minded at all if you said:

	Obviously, appropriate documentation and tests are needed before
	inclusion, but I am sending this out primarily to seek opinions
	from the list to make sure this is going in the right direction,
	iow, this is an RFC.

What bugged me was the phrase "if this is accepted".

  parent reply	other threads:[~2007-12-07 21:26 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-12-07 21:10 [RFC/PATCH] Add a --nosort option to pack-objects Mike Hommey
2007-12-07 21:24 ` Nicolas Pitre
2007-12-07 21:44   ` Mike Hommey
2007-12-07 21:25 ` Junio C Hamano [this message]
2007-12-07 21:37   ` Junio C Hamano
2007-12-07 21:46   ` Mike Hommey
2007-12-07 22:20     ` Nicolas Pitre
2007-12-08  8:54       ` Mike Hommey

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=7v4peu19kr.fsf@gitster.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=mh@glandium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).