On 5/20/2020 10:07 PM, brian m. carlson wrote: > Patch #1 reduces the number of options in the scenario which Stolee > mentioned above. There's now just a NULL and a non-NULL case, and the > NULL case is now relatively straightforward and uninteresting. I'm glad you and Junio were able to simplify this case! > Patch #2 adds a test for the particular set of options which will > trigger this case as an independent test. I didn't think it made sense > to put this in t0021, since ultimately that set of options isn't about > conversions and it would seem out of place there, so I put it in t2060. > > I'm ultimately on the fence for this case, because I think it's really a > corner case and testing this is probably not that interesting, so my > preference is for us to pick up patch 1 and drop patch 2. However, I > added patch 2 in case we do indeed want a test for this, and I'll let > Junio and others decide on what's best. I think the test is helpful, since it is not very complicated to set up. The test you created for t0021-conversion.sh earlier was quite complicated and required internal knowledge to get going. However, this test only uses porcelain commands to trigger the conditional. >From the discussion here, it is not obvious that info->commit is ever NULL, so having the test can prevent a future change from making that same assumption. This series is Reviewed-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> Thanks, -Stolee
