Re: [PATCH 5/5] mv: replace src_dir with a strvec

[Jeff King <peff@peff.net>]

On Thu, May 30, 2024 at 08:36:25AM -0700, Junio C Hamano wrote:

> Hmph, the rationale given by 9fcd9e4e (builtin/mv duplicate string
> list memory, 2024-05-27) essentially is "the number of elements are
> the same as the number of command line parameters", but I do not
> think that is quite correct.
> 
> When you do "mv srcA srcB ... dst", you'd inspect the command line
> arguments from left to right, notice that srcA is a directory, find
> the cache entries for paths that are inside srcA, append the paths
> in that directory to source[] and destination[] array, and extend
> argc.  "for (i = 0; i < argc; i++)" loop that appends one element to
> src_for_dst per iteration ends up running the number of paths being
> moved, which can be order of magnitude more than the command line
> parameters.
> 
> Of course, if we needed to make copies for correctness reasons (or
> to clarify memory ownership semantics), that alone may be a good
> justification and we do not need an excuse "it's just a handful of
> elements anyway" to begin with.
> 
> Anyway, that is about somebody else's patch, not this one ;-).

Heh, good digging. I actually wondered if I was making the same mistake
while writing mine, but double-checked that src_dir is not expanded in
that way. But I didn't think to check Patrick's original. ;)

IMHO it is probably still OK. We are bounded by the number of entries in
the index (and we already use proportional memory for other parts of the
operation).

-Peff