git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Git List" <git@vger.kernel.org>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Christian Couder" <christian.couder@gmail.com>
Subject: Re: [PATCH] revision: introduce prepare_revision_walk_extended()
Date: Sun, 24 Dec 2017 09:22:46 -0500	[thread overview]
Message-ID: <20171224142246.GA23648@sigill.intra.peff.net> (raw)
In-Reply-To: <c8980a2c-24e2-a877-fa66-31206123ce49@web.de>

On Thu, Dec 21, 2017 at 07:41:44PM +0100, René Scharfe wrote:

> Am 20.12.2017 um 14:08 schrieb Jeff King:
> > On Tue, Dec 19, 2017 at 10:33:55AM -0800, Junio C Hamano wrote:
> > 
> >> Should we take the patch posted as-is and move forward?
> > 
> > I suppose so.  I don't really have anything against the patch. My main
> > complaint was just that I don't think it's actually cleaning up the
> > gross parts of the interface. It's just substituting one gross thing (a
> > funny struct flag) for another (a special version of the prepare
> > function that takes a funny out-parameter).
> 
> If this is a fair description (and I have to admit that it is) then I
> don't understand why you aren't against the patch.  Let's drop it.

Heh, I almost wrote more on this. My thinking was two-fold:

  - while I think the fundamental gross-ness is still there after your
    patch, it does smooth some of the rough edges. So it's a slight
    improvement over the status quo.

  - I read an article a while back (which unfortunately I can't find
    now) about the idea of "default to yes" in open source. I.e., the
    idea that if somebody bothered to cook up a patch and there is no
    good reason to reject it, you should take it.

    Certainly there are cases where that can go too far: obvious ones
    like bad ideas where it is not really "default" anymore, but also
    subtle ones where the changes are code churn whose fallouts will
    be dealt with by others. But this patch is a good example. I'm not
    convinced it makes things better, but I don't think it makes things
    worse. If you, who looked a lot closer at the problem than I did,
    still think it's a good idea after our discussion, it's probably
    worth applying.

So my comments weren't really "this is a bad idea" as much as "is there
a better idea". We didn't come up with one after some discussion, and
I'm willing to let it go there for now.

But if you want to keep thinking on it, I'm game. ;)

> Can we do better?  How about something this?  It reverts bundle to
> duplicate the object_array, but creates just a commit_list in the other
> two cases.  As a side-effect this allows clearing flags for all entries
> with a single traversal.

I think this is basically the "copy it before-hand" thing from earlier
in the thread. Switching to just keeping commits seems like a nice
change. It's an easy (if minor) optimization, and it makes clear exactly
which part of the data we need to keep around.

The single-traversal thing I suspect doesn't matter much in practice. In
both cases if we would visit commit X twice, we'd immediately see on the
second visit that it has already been cleared and not do anymore work.

  Side note: Another question is whether it would simply be faster to
  clear the flags for _all_ objects that we've touched in the current
  process (we have clear_object_flags() for this already). Then we know
  that we touch each one once, and we as a bonus we don't even have to
  keep the previous tips. The downsides are:

    - if another traversal in the process looked at many objects, but
      our current traversal looked at few, then we would examine more
      objects than we need to (albeit with lower cost per object)

    - it's possible there's another traversal in the same process whose
      flags we would want to have saved. I suspect such a setup is
      broken already, though, unless there's a guarantee that the two
      traversals don't overlap.

So anyway, I think this is a reasonable approach, unless we're really
worried about the extra O(# of pending) allocation.

-Peff

  reply	other threads:[~2017-12-24 14:22 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-16 12:12 René Scharfe
2017-12-17 10:20 ` Martin Ågren
2017-12-18 15:10 ` Jeff King
2017-12-18 19:18   ` René Scharfe
2017-12-19 11:49     ` Jeff King
2017-12-19 18:33       ` Junio C Hamano
2017-12-20 13:08         ` Jeff King
2017-12-21 18:41           ` René Scharfe
2017-12-24 14:22             ` Jeff King [this message]
2017-12-25 17:36               ` René Scharfe
2017-12-25 17:41 ` [PATCH v2 0/9] revision: get rid of the flag leak_pending René Scharfe
2017-12-25 17:43   ` [PATCH v2 1/9] commit: avoid allocation in clear_commit_marks_many() René Scharfe
2018-01-10  7:54     ` Jeff King
2017-12-25 17:44   ` [PATCH v2 2/9] commit: use clear_commit_marks_many() in remove_redundant() René Scharfe
2017-12-25 17:44   ` [PATCH v2 3/9] ref-filter: use clear_commit_marks_many() in do_merge_filter() René Scharfe
2017-12-25 17:44   ` [PATCH v2 4/9] object: add clear_commit_marks_all() René Scharfe
2018-01-10  7:58     ` Jeff King
2018-01-11 18:57       ` René Scharfe
2018-01-12 15:20         ` Jeff King
2017-12-25 17:45   ` [PATCH v2 5/9] bisect: avoid using the rev_info flag leak_pending René Scharfe
2018-01-10  8:07     ` Jeff King
2018-01-11 18:57       ` René Scharfe
2018-01-12 15:23         ` Jeff King
2017-12-25 17:46   ` [PATCH v2 6/9] bundle: " René Scharfe
2017-12-28 21:13     ` Junio C Hamano
2018-01-10  8:18     ` Jeff King
2017-12-25 17:47   ` [PATCH v2 7/9] checkout: " René Scharfe
2017-12-28 21:24     ` Junio C Hamano
2017-12-25 17:47   ` [PATCH v2 8/9] revision: remove the unused " René Scharfe
2017-12-25 17:48   ` [PATCH v2 9/9] commit: remove unused function clear_commit_marks_for_object_array() René Scharfe
2017-12-28 20:32   ` [PATCH v2 0/9] revision: get rid of the flag leak_pending Junio C Hamano
2018-01-10  8:20   ` Jeff King

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171224142246.GA23648@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=martin.agren@gmail.com \
    --subject='Re: [PATCH] revision: introduce prepare_revision_walk_extended()' \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git