On Tue, Apr 14, 2020 at 01:40:53PM -0400, Derrick Stolee wrote: > On 4/14/2020 1:26 PM, Junio C Hamano wrote: > > Derrick Stolee <stolee@xxxxxxxxx> writes: > > > >> On 4/13/2020 6:21 PM, Junio C Hamano wrote: > >>> Taylor Blau <me@xxxxxxxxxxxx> writes: > >>> > >>>> Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*' > >>>> environment variables and potentially ignoring its arguments, but given > >>>> the discussion we had in v1, I don't feel strongly enough to recommend > >>>> that you change this. > >>>> > >>>> For what it's worth, I think that 'write_commit_graph' could behave more > >>>> purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set > >>>> 'flags' appropriately from the outside,... > >>> > >>> Yeah, I agree that it would be a lot cleaner if that is easy to > >>> arrange. I have a suspicion that Derrick already tried and the > >>> resulting code looked messier and was discarded? > >> > >> Perhaps I could fix both concerns by > >> > >> 1. Use a macro instead of a library call. > >> > >> 2. Check the _CHANGED_PATHS variable in the macro. > > > > I am not sure how use of a macro "fixes" purity, though. And what > > is the other concern? > > The concern was (1) checking the environment and (2) die()ing in the > library. I think that we might as well use a plain old function here instead of a macro, especially since the macro is just expanding out to what that function would be if we had written it that way. For what it's worth, my concern was more that I don't mind dying, but I'd rather do so in a calling function, and that by the time we're calling 'write_commit_graph', that we should be using return values instead of 'die()' or 'error()'. > > How widely would this "if we are testing, write out the graph file" > > call be sprinkled over the codebase? I am hoping that it won't be > > "everywhere", but only at strategic places (like "just once before > > we leave a subcommand that creates one or bunch of commits"). And > > how often would they be called? I am also hoping that it won't be > > "inside a tight loop". In short, I am wondering if we can promise > > our codebase that > > > > - git_test_write_commit_graph_or_die() calls won't be an eyesore > > (and/or distraction) for developers too much. > > > > - git_env_bool() call won't have performance impact. > > I could add a comment to the header file to say "this is only for > improving coverage of optional features in the test suite. Do not > call this method unless you know what you are doing." Works for me. > My intention right now is to only include this in 'git commit' > and 'git merge'. Earlier discussion included thoughts about > 'git rebase' and similar, but those are more rarely used when > constructing an "example repo" in the test scripts. Ditto. I think that there's benefit in having it in some places but not all (as Stolee pointed out earlier in the thread), and that trying to have it everywhere is a fool's errand. > -Stolee Thanks, Taylor
