Re: [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere

From: Jeff King <peff@peff.net>
Date: Jan 31, 2020, 10:30 UTC
Message-ID: <20200131103008.GD2916051@coredump.intra.peff.net>
In-Reply-To: <cover.1580424766.git.me@ttaylorr.com>

On Thu, Jan 30, 2020 at 03:00:40PM -0800, Taylor Blau wrote:

Show 16 quoted lines

> This series became a little bit longer than I was expecting it to be, so
> here is the high-level structure:
> 
>   - 1/6 fixes a bug in a test that would cause a subsequent failure if
>     unaddressed.
> 
>   - 2/6 does the first half of the removal by using 'struct
>     object_directory *'s within the 'commit_graph' structure.
> 
>   - 4/6 does the second half by removing 'char *object_dir' usage in the
>     'write_commit_graph_context' structure.
> 
>   - 5/6 ties 2/6 and 4/6 together by removing all path normalization
>     completely, fixing the uninitialized read bug.
> 
>   - And 6/6 cleans up.

With the exception of the patch-ordering discussion in the sub-thread with Martin, this looks good to me.

Patch 3 is a change in user-visible behavior, as it restricts how --object-dir can be used (it must be the main object-dir or an alternate within the repository). I don't _think_ anybody would care, as the semantics of those options seemed kind of ill-defined to me in the first place. But it's worth calling out as a potential risk. I suppose the alternative is to make a one-off fake "struct object_directory" within the process that isn't connected to the repository. But if nobody cares, I'd just as soon avoid that.

One other funny thing with this series: the Date headers of your patches seem out of order. They ordering in your cover letter here is fine and presumably reflects the commit topology:

Show 7 quoted lines

> Taylor Blau (6):
>   t5318: don't pass non-object directory to '--object-dir'
>   commit-graph.h: store object directory in 'struct commit_graph'
>   builtin/commit-graph.c: die() with unknown '--object-dir'
>   commit-graph.h: store an odb in 'struct write_commit_graph_context'
>   commit-graph.c: remove path normalization, comparison
>   commit-graph.h: use odb in 'load_commit_graph_one_fd_st'

but the Date headers in order of 1-6 are:

  Date:   Thu, 30 Jan 2020 15:00:43 -0800
  Date:   Thu, 30 Jan 2020 15:00:50 -0800
  Date:   Thu, 30 Jan 2020 15:00:54 -0800
  Date:   Thu, 30 Jan 2020 15:00:52 -0800
  Date:   Thu, 30 Jan 2020 15:00:45 -0800
  Date:   Thu, 30 Jan 2020 15:00:47 -0800

It's like your sending script rewrites the Date header and puts a 2-second bump between each one (which is good, and what git-send-email does), but got fed the patches in the wrong order (perhaps their _original_ date order, if there was clean-up rebasing).

-Peff