Inverted boolean logic in HistoryEntry.hasCorrespondingLayer()

[tnaroska]

Environment:

• Jib version: jib.core 0.20.0
• Build tool: N/A
• OS: N/A

Description of the issue:

Method hasCorrespondingLayer() of class com.google.cloud.tools.jib.image.json.HistoryEntry returns wrong values. It returns true for empty layers and vice versa.

Expected behavior:

Method should return false for empty layers and true for layers with content.

Steps to reproduce:

N/A. Sorry, just look at the one-line implementation of the method. The bug is obvious. https://github.com/GoogleContainerTools/jib/blob/master/jib-core/src/main/java/com/google/cloud/tools/jib/image/json/HistoryEntry.java#L138-L146

Additional Information:

There seems to be only one use of the method within jib here: https://github.com/GoogleContainerTools/jib/blob/0ed7dca36864b6b19ff61629fc578018041fa15f/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/BuildImageStep.java#L96-L98

The snippet is similarly wrong:

        if (!historyObject.hasCorrespondingLayer()) {
          nonEmptyLayerCount++;
        }

The if statement as written will trigger for empty layers, but the variable is named nonEmptyLayerCount. The logic is inverted which hides the underlying error in hasCorrespondingLayer().

hasCorrespondingLayer() needs to be fixed and this code adapted to:

        if (historyObject.hasCorrespondingLayer()) {
          nonEmptyLayerCount++;
        }

[suztomo]

@tnaroska I would need to look into more details, but would you share your observation that led you to investigate this nonEmptyLayerCount?

[tnaroska]

Hi @suztomo, I'm hacking a little tool to analyze our docker images based on jib-core. Part of that is loading the manifest and config of an image from the registry and then associating HistoryEntries with actual file layers. For that I need to filter HistoryEntries by their hasCorrespondingLayer() property to skip empty layers. That's when I found the issue in hasCorrespondingLayer(). It returns true for empty layers, but shouldn't.

Either the method implementation needs to be changed or it should be renamed to isEmptyLayer(). https://github.com/GoogleContainerTools/jib/blob/master/jib-core/src/main/java/com/google/cloud/tools/jib/image/json/HistoryEntry.java#L138-L146

[chanseokoh]

You are right. Originally the method name was isEmptyLayer(), but it got renamed during PR review while not inverting the condition. I think either inverting the condition or reverting the name back to isEmptyLayer() (in this case, the Javadocs of the method and the field empty_layer should be updated too) is fine.

[tnaroska]

Thanks for confirming @chanseokoh. Either way to fix it seems fine to me.

[suztomo]

Mmeo: jib-core is pre-1.0 version. This breaking change can go in next release without worrying about major version bump.

[ldetmer]

@tnaroska do you still require this fix?