Re: [PATCH v2 1/2] dir: consider worktree config in path recursion

From: oss dev <gg.oss.dev@gmail.com>
Date: May 30, 2022, 18:48 UTC
Message-ID: <CAAA5oLnAwcH+Kr9UT1+V-_bjNNJS+8=U1+uMZUETSmP+nfM7kA@mail.gmail.com>
In-Reply-To: <900d91cb-1982-b892-17e3-ad678e711dc9@github.com>

> This could be strbuf_add(&sb, ".git", 4); to avoid a (minor)
> strlen() computation.

I checked gcc's disassembly--the strbuf_addstr is inlined and the strlen is evaluated at compile time. I believe gcc has evaluated strlen at compile time for string literals for quite some time. Also seems common in the rest of the code base to use strbuf_addstr with string literals.

Show 5 quoted lines

> With regards to Junio's comment about repeating real_pathdup()
> on the_repository->gitdir, we might be able to short-circuit
> this by making real_gitdir static and only calling
> real_pathdup() if real_gitdir is NULL. It would cut the cost
> of these checks by half for big traversals.

Yeah, I couldn't come up with a super clean way of caching real_pathdup (assuming we don't want to leak the memory), but based on subsequent comments it sounds like it was more a kicking-the-tires question than real concern.

> The test description should be a single line. The longer paragraph
> could be a comment before your "setup" test case.

I'm not sure what the current best practice is, but I expected best practice to be reflected here [1] and many other tests use multi-line descriptions (see e.g. t0000 [2] or t1512 [3] for a nice ascii graphic right in the description).

[1]: https://git-scm.com/docs/MyFirstContribution#write-new-test [2]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t0000-basic.sh [3]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t1512-rev-parse-disambiguation.sh

> Also, there is no need to number your tests in their names, as the
> testing infrastructure will apply numbers based on their order in
> the test file. These labels will grow stale if tests are removed
> or inserted into the order.

In v1 of the patch, I was asked to include a comment in the code that refers to a test for which the changes that are being made in this patch are meaningful. Grepping for other instances where this was done, similar comments generally refer to numbered testcases (see e.g. [1]). I figured if I don't number it then someone might suggest that similar comments generally refer to numbered test cases. Though I'll concede that referring to any specific tests will likely eventually result in stale references. For example, t6043 referenced by [1] (currently in master) appears to have been moved elsewhere. Personally, I'm indifferent.

[1]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/merge-recursive.c#L1620

Show 10 quoted lines

> This use of .gitignore to ignore the helper files being used
> during the test is interesting. I was thinking it would be better
> to create isolated directories where the only activity was that
> which you were testing:
>
>         mkdir -p worktree &&
>         test_create_repo worktree/contains-git &&
>
> ...or something like that. The names are more descriptive, and
> we don't need the .gitignore trick.

While trying to figure out what section this test should go into, I looked through several tests and was likely inspired by e.g. [1] or [2]. I also think there could be a benefit to ensuring that .gitignore works correctly with the new traversal behavior in the nested repo, i.e. the .gitignore becomes part of the test. In principle, I don't mind testing .gitignore more explicitly, but I also think the tests are quite lengthy already for a small patch and doing more with less seems attractive.

[1]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t1501-work-tree.sh#L198-L202 [2]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t2202-add-addremove.sh#L13-L17

Show 5 quoted lines

> I was surprised by this "local". It isn't needed at this
> point in the test script. We use it for helper methods to
> be sure that we aren't stomping variables from test scripts,
> but since the test is being executed inside an eval(), this
> local isn't important.

I tend to default to `local` or `declare` to limit unintended side effects after refactoring, etc., but I don't mind removing it if it's not desired. I'm ok either way, pls confirm.

> You can avoid the sub-shell using "git -C test1 ls-files ..."
> right?

Ok, I can update this and similar instances.

Show 5 quoted lines

> This also checks to see if _any_ of these "git add"
> commands fail, as opposed to failing immediately after
> the first one fails. I think your approach is simpler and
> should be relatively simple to identify which command did
> the wrong thing by looking at the test_cmp output.

Agreed, I'm combining several `git add`s into one test here -- I didn't want to get too long-winded since it seems like a minor patch.

Thanks for the comments; v3 of the patch had already been sent prior to these, so none is incorporated there. I'll include confirmed changes in the next version.