git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: oss dev <gg.oss.dev@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	christian w <usebees@gmail.com>, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v2 1/2] dir: consider worktree config in path recursion
Date: Mon, 30 May 2022 14:48:12 -0400	[thread overview]
Message-ID: <CAAA5oLnAwcH+Kr9UT1+V-_bjNNJS+8=U1+uMZUETSmP+nfM7kA@mail.gmail.com> (raw)
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.


> 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


> 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


> 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.


> 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.

  reply	other threads:[~2022-05-30 18:48 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-05 20:32 [RFC PATCH 0/1] " Goss Geppert
2022-05-05 20:32 ` [RFC PATCH 1/1] " Goss Geppert
2022-05-07  3:26   ` Elijah Newren
2022-05-07 17:59     ` oss dev
2022-05-06 17:02 ` [RFC PATCH 0/1] " Junio C Hamano
2022-05-06 20:00   ` oss dev
2022-05-10 17:15 ` [PATCH v2 0/2] " Goss Geppert
2022-05-10 17:15   ` [PATCH v2 1/2] " Goss Geppert
2022-05-11 16:37     ` Junio C Hamano
2022-05-20 19:45       ` oss dev
2022-05-24 14:29       ` Elijah Newren
2022-05-24 19:45         ` Junio C Hamano
2022-05-25  3:46           ` Elijah Newren
2022-05-11 23:07     ` Junio C Hamano
2022-05-20 20:01       ` oss dev
2022-05-23 19:23     ` Derrick Stolee
2022-05-30 18:48       ` oss dev [this message]
2022-05-10 17:15   ` [PATCH v2 2/2] dir: minor refactoring / clean-up Goss Geppert
2022-05-11 16:51     ` Junio C Hamano
2022-05-20 20:03       ` oss dev
2022-05-20 19:28 ` [PATCH v3 0/3] dir: traverse into repository Goss Geppert
2022-05-20 19:28   ` [PATCH v3 1/3] " Goss Geppert
2022-05-20 19:28   ` [PATCH v3 2/3] dir: cache git_dir's realpath Goss Geppert
2022-05-24 14:32     ` Elijah Newren
2022-05-20 19:28   ` [PATCH v3 3/3] dir: minor refactoring / clean-up Goss Geppert
2022-06-16 23:19 ` [PATCH v4 0/2] dir: traverse into repository Goss Geppert
2022-06-22  4:57   ` Elijah Newren
     [not found] ` <20220616231956.154-1-gg.oss@outlook.com>
2022-06-16 23:19   ` [PATCH v4 1/2] " Goss Geppert
2022-06-16 23:44 ` [PATCH v4 0/2] dir: traverse into repository (resending) Goss Geppert
     [not found] ` <20220616234433.225-1-gg.oss@outlook.com>
2022-06-16 23:44   ` [PATCH v4 1/2] dir: traverse into repository Goss Geppert
2022-06-16 23:44   ` [PATCH v4 2/2] dir: minor refactoring / clean-up Goss Geppert

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='CAAA5oLnAwcH+Kr9UT1+V-_bjNNJS+8=U1+uMZUETSmP+nfM7kA@mail.gmail.com' \
    --to=gg.oss.dev@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=usebees@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).