Re: [PATCH v8] ls-remote: create '--sort' option

From: Jeff King <peff@peff.net>
Date: Apr 6, 2018, 18:58 UTC
Message-ID: <20180406185801.GA921@sigill.intra.peff.net>
In-Reply-To: <CAHwyqnU4jv=wW_no4ff_E2TgvGtga1GOhZoDKd93J5MMUtLFCA@mail.gmail.com>

On Thu, Apr 05, 2018 at 01:11:34AM +0200, Harald Nordgren wrote:

Show 6 quoted lines

> Without digging to much into the `ref-filter` code itself, it seems
> like there is an opportunity to generalize and unfify the logic
> between these two cases. As well as using `ALLOC_GROW`. But maybe that
> is best left as a follow-up task? Especially since this patch focuses
> on `ls-remote`. Seems possibly like too big of a change to start
> changing a different sub-command.

Yes and no. By refactoring we have the potential to regress another unrelated command. But by not refactoring, we set up a potential misuse of the other interface which may lead to more bugs down the road.

I think an important thing is to split out the refactoring steps into their own patches, so it's easy to see that they're not hurting the existing uses. I have some sample patches, which I'll send in a moment.

Show 5 quoted lines

> Wouldn't a `ref_array_push()` also require `ref->symref`, maybe then
> we could pass the whole ref? It needs to be very clear that it's a
> `ref` and not a `ref_array_item` that is being pushed. Much of my
> logic here deals specifically with trying to treat a ref as
> ref_array_item.

I think it would be OK to have the push operation return the actual item, which can then be filled in more. That's how the internal ref-filter code does it already.

Show 5 quoted lines

> From my viewpoint as implementer, I was very happy that I could
> implement the feature *without* invoking `filter_refs` since that
> `filter->kind` switching looks a pretty daunting. I'm not exactly sure
> what a `git ls-remote --contains HEAD` would do, maybe you could
> explain a bit more?

I agree it would be a bit larger of a retro-fit to adapt filter_refs() for our purpose here. And since we're not doing any filtering at this point, there's not much benefit.

Here are the refactoring patches I came up with. I think they're a benefit to ref-filter on their own, and then it should be pretty easy for you to build your patch on top.

  [1/3]: ref-filter: use "struct object_id" consistently
  [2/3]: ref-filter: make ref_array_item allocation more consistent
  [3/3]: ref-filter: factor ref_array pushing into its own function

 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c         | 36 +++++++++++++++++++++++++-----------
 ref-filter.h         | 10 +++++++++-
 4 files changed, 36 insertions(+), 14 deletions(-)

-Peff

PS I'm going to be offline for a few weeks starting tomorrow, so I won't
   be responsive to this thread. :) If you like the direction, please
   feel free to pick up those patches, rebase yours on top, and then
   send them all out as a series.

   In case it's easier than applying from the list, you can fetch them
   from the "jk/ref-array-push" branch of https://github.com/peff/git.