[v2,0/2] Improve Fix code coverage for checkout

Message

brian m. carlson May 21, 2020, 2:07 a.m. UTC

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.

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.

brian m. carlson (2):
  builtin/checkout: simplify metadata initialization
  t2060: add a test for switch with --orphan and --discard-changes

 builtin/checkout.c | 4 +---
 t/t2060-switch.sh  | 8 ++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

Derrick Stolee May 21, 2020, 12:38 p.m. UTC | #1

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 <[e-mail]>

Thanks,
-Stolee