ronantakizawa/codereview-bench huggingface dataset:
Repository: containerd/containerd
File: internal/cri/server/container_image_mount_other.go (Go)

Code before:
	"github.com/containerd/containerd/v2/core/mount"
	"github.com/containerd/containerd/v2/core/snapshots"
	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
)

// addVolatileOptionOnImageVolumeMount is no-op on non-linux platforms.
func addVolatileOptionOnImageVolumeMount(mounts []mount.Mount) []mount.Mount {
	// no-op
	return mounts
}

// ensureImageVolumeMounted ensures target volume is mounted.
func ensureImageVolumeMounted(target string) (bool, error) {
	_, err := os.Stat(target)
	if err != nil {
		if os.IsNotExist(err) {
			return false, nil
		}
		return false, fmt.Errorf("failed to stat %s: %w", target, err)
	}
	return true, nil
}

// getImageVolumeSnapshotOpts is no-op on non-linux platforms.
func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
	// User namespaces are only supported on Linux
	return nil, nil
}

// clearImageVolumeMountMappings is no-op on non-linux platforms.
func clearImageVolumeMountMappings(mount *runtime.Mount) {
	// User namespaces are only supported on Linux
}


Review comment (refactor):
Today the opts are about userns, but they can be about other things in the future. Let's just remove this comment.
```suggestion
```

Code after:
	"github.com/containerd/containerd/v2/core/mount"
	"github.com/containerd/containerd/v2/core/snapshots"
	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
)

// addVolatileOptionOnImageVolumeMount is no-op on non-linux platforms.
func addVolatileOptionOnImageVolumeMount(mounts []mount.Mount) []mount.Mount {
	// no-op
	return mounts
}

// ensureImageVolumeMounted ensures target volume is mounted.
func ensureImageVolumeMounted(target string) (bool, error) {
	_, err := os.Stat(target)
	if err != nil {
		if os.IsNotExist(err) {
			return false, nil
		}
		return false, fmt.Errorf("failed to stat %s: %w", target, err)
	}
	return true, nil
}

func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
	return nil, nil
}
Repository: containerd/containerd
File: internal/cri/server/container_image_mount_linux_test.go (Go)

Code before:
package server

import (
	"context"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
)

func TestGetImageVolumeSnapshotOpts(t *testing.T) {
	ctx := context.Background()

	mappings := []*runtime.IDMapping{
		{
			ContainerId: 0,
			HostId:      65536,
			Length:      65536,
		},
	}

	for _, test := range []struct {
		name        string
		mount       *runtime.Mount
		expectOpts  bool
		expectError bool
	}{
		{
			name: "with user namespace mappings",
			mount: &runtime.Mount{
				ContainerPath: "/test",
				UidMappings:   mappings,
				GidMappings:   mappings,
			},
			expectOpts:  true,
			expectError: false,
		},
		{
			name: "without mappings",
			mount: &runtime.Mount{
				ContainerPath: "/test",
			},
			expectOpts:  false,
			expectError: false,
		},
		{
			name: "with empty mappings",
			mount: &runtime.Mount{
				ContainerPath: "/test",
				UidMappings:   []*runtime.IDMapping{},

Review comment (suggestion):
A bool here is too generic. I'd check the opts are exactly what we expect, not just that _some_ opts are set. Like this is will age better if more options are set in the future.

Code after:
package server

import (
	"context"
	"testing"

	containerd "github.com/containerd/containerd/v2/client"
	"github.com/containerd/containerd/v2/core/snapshots"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
)

func TestGetImageVolumeSnapshotOpts(t *testing.T) {
	ctx := context.Background()

	mappings := []*runtime.IDMapping{
		{
			ContainerId: 0,
			HostId:      65536,
			Length:      65536,
		},
	}

	expectedOpts := optsToInfo(t, containerd.WithRemapperLabels(0, 65536, 0, 65536, 65536))

	for _, test := range []struct {
		name        string
		mount       *runtime.Mount
		expectInfo  *snapshots.Info
		expectError bool
	}{
		{
			name: "with user namespace mappings",
			mount: &runtime.Mount{
				ContainerPath: "/test",
				UidMappings:   mappings,
				GidMappings:   mappings,
			},
			expectInfo: expectedOpts,
		},
		{
			name: "without mappings",
			mount: &runtime.Mount{
				ContainerPath: "/test",
			},
		},
		{
			name: "with empty mappings",
			mount: &runtime.Mount{
				ContainerPath: "/test",
Repository: containerd/containerd
File: internal/cri/server/container_image_mount_linux.go (Go)

Code before:
		return false, fmt.Errorf("failed to check %s mountpoint: %w", target, err)
	}

	if mpInfo.Mountpoint != target {
		return false, nil
	}
	return true, nil
}

// getImageVolumeSnapshotOpts returns snapshot options with user namespace idmap labels
// from the mount's UID/GID mappings. This ensures that image volumes work correctly
// with user namespaces by applying idmap to the overlay lower layers.
func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
	uidMappings := mount.GetUidMappings()
	gidMappings := mount.GetGidMappings()

	if len(uidMappings) == 0 && len(gidMappings) == 0 {
		return nil, nil
	}

	uids, err := parseUsernsIDMap(uidMappings)
	if err != nil {
		return nil, fmt.Errorf("failed to parse UID mappings: %w", err)
	}

	gids, err := parseUsernsIDMap(gidMappings)
	if err != nil {
		return nil, fmt.Errorf("failed to parse GID mappings: %w", err)
	}

	// Both UID and GID mappings must be present for user namespace remapping
	if len(uids) == 0 || len(gids) == 0 {
		return nil, fmt.Errorf("user namespace remapping requires both UID and GID mappings, got %d UID mappings and %d GID mappings", len(uids), len(gids))
	}

	return []snapshots.Opt{
		containerd.WithRemapperLabels(0, uids[0].HostID, 0, gids[0].HostID, uids[0].Size),
	}, nil
}


Review comment (bug):
If the len of the mappings is 0, it is handled fine by the parseUsernsIDMap function. We can simplify this

```suggestion
	uids, err := parseUsernsIDMap(mount.GetUidMappings())
	if err != nil {
		return nil, fmt.Errorf("failed to parse UID mappings: %w", err)
	}

	gids, err := parseUsernsIDMap(mount.GetGidMappings())
```

Code after:
		return false, fmt.Errorf("failed to check %s mountpoint: %w", target, err)
	}

	if mpInfo.Mountpoint != target {
		return false, nil
	}
	return true, nil
}

// getImageVolumeSnapshotOpts returns snapshot options with user namespace idmap labels
// from the mount's UID/GID mappings. This ensures that image volumes work correctly
// with user namespaces by applying idmap to the overlay lower layers.
func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
	uids, err := parseUsernsIDMap(mount.GetUidMappings())
	if err != nil {
		return nil, fmt.Errorf("failed to parse UID mappings: %w", err)
	}

	gids, err := parseUsernsIDMap(mount.GetGidMappings())
	if err != nil {
		return nil, fmt.Errorf("failed to parse GID mappings: %w", err)
	}

	if len(uids) == 0 || len(gids) == 0 {
		return nil, nil
	}

	return []snapshots.Opt{
		containerd.WithRemapperLabels(0, uids[0].HostID, 0, gids[0].HostID, uids[0].Size),
	}, nil
}
Repository: containerd/containerd
File: internal/cri/server/container_image_mount_linux.go (Go)

Code before:
		return false, fmt.Errorf("failed to check %s mountpoint: %w", target, err)
	}

	if mpInfo.Mountpoint != target {
		return false, nil
	}
	return true, nil
}

// getImageVolumeSnapshotOpts returns snapshot options with user namespace idmap labels
// from the mount's UID/GID mappings. This ensures that image volumes work correctly
// with user namespaces by applying idmap to the overlay lower layers.
func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
	uidMappings := mount.GetUidMappings()
	gidMappings := mount.GetGidMappings()

	if len(uidMappings) == 0 && len(gidMappings) == 0 {
		return nil, nil
	}

	uids, err := parseUsernsIDMap(uidMappings)
	if err != nil {
		return nil, fmt.Errorf("failed to parse UID mappings: %w", err)
	}

	gids, err := parseUsernsIDMap(gidMappings)
	if err != nil {
		return nil, fmt.Errorf("failed to parse GID mappings: %w", err)
	}

	// Both UID and GID mappings must be present for user namespace remapping
	if len(uids) == 0 || len(gids) == 0 {
		return nil, fmt.Errorf("user namespace remapping requires both UID and GID mappings, got %d UID mappings and %d GID mappings", len(uids), len(gids))
	}

	return []snapshots.Opt{
		containerd.WithRemapperLabels(0, uids[0].HostID, 0, gids[0].HostID, uids[0].Size),
	}, nil
}


Review comment (suggestion):
But after the suggested edit, here we need to return nil, nil. It is fine if a image doesn't have mappings.

Code after:
		return false, fmt.Errorf("failed to check %s mountpoint: %w", target, err)
	}

	if mpInfo.Mountpoint != target {
		return false, nil
	}
	return true, nil
}

// getImageVolumeSnapshotOpts returns snapshot options with user namespace idmap labels
// from the mount's UID/GID mappings. This ensures that image volumes work correctly
// with user namespaces by applying idmap to the overlay lower layers.
func (c *criService) getImageVolumeSnapshotOpts(ctx context.Context, mount *runtime.Mount) ([]snapshots.Opt, error) {
	uids, err := parseUsernsIDMap(mount.GetUidMappings())
	if err != nil {
		return nil, fmt.Errorf("failed to parse UID mappings: %w", err)
	}

	gids, err := parseUsernsIDMap(mount.GetGidMappings())
	if err != nil {
		return nil, fmt.Errorf("failed to parse GID mappings: %w", err)
	}

	if len(uids) == 0 || len(gids) == 0 {
		return nil, nil
	}

	return []snapshots.Opt{
		containerd.WithRemapperLabels(0, uids[0].HostID, 0, gids[0].HostID, uids[0].Size),
	}, nil
}
Repository: conwnet/github1s
File: src/vs/github1s/notification.ts (TypeScript)

Code before:
/**
 * @file github1s notification
 * @author netcon
 */

import 'vs/css!./notification';

const NOTIFICATION_STORAGE_KEY = 'GITHUB1S_NOTIFICATION';
// change it if a new notification should be shown
const NOTIFICATION_STORAGE_VALUE = '20210212';

const notification = {
	title: 'ATTENTION: This page is NOT officially provided by GitHub.',
	content: 'GitHub1s is open source project, not officially provided by GitHub.',
	link: 'https://github.com/conwnet/github1s',
};

const notificationHtml = `
<div class="notification-main">
	<div class="notification-title">${notification.title}</div>
	<div class="notification-content">
		${notification.content}
		<a class="notification-link" href="${notification.link}" target="_blank">See more</a>
	</div>
</div>
<div class="notification-footer">
	<button class="notification-confirm-button">I know</button>
	<div class="notification-show-me-again">
		<input type="checkbox" checked>Don't show me again</div>
	</div>
</div>
`;

export const renderNotification = () => {
	// if user has confirm the notification and check `don't show me again`, ignore it
	if (!window.localStorage || window.localStorage.getItem(NOTIFICATION_STORAGE_KEY) === NOTIFICATION_STORAGE_VALUE) {
		return;
	}


Review comment (suggestion):
```suggestion
	content: 'GitHub1s is an open source project, which is not officially provided by GitHub.',
```

Code after:
/**
 * @file github1s notification
 * @author netcon
 */

import 'vs/css!./notification';

const NOTIFICATION_STORAGE_KEY = 'GITHUB1S_NOTIFICATION';
// change it if a new notification should be shown
const NOTIFICATION_STORAGE_VALUE = '20210212';

const notification = {
	title: 'ATTENTION: This page is NOT officially provided by GitHub.',
	content: 'GitHub1s is an open source project, which is not officially provided by GitHub.',
	link: 'https://github.com/conwnet/github1s',
};

const notificationHtml = `
<div class="notification-main">
	<div class="notification-title">${notification.title}</div>
	<div class="notification-content">
		${notification.content}
		<a class="notification-link" href="${notification.link}" target="_blank">See more</a>
	</div>
</div>
<div class="notification-footer">
	<button class="notification-confirm-button">I know</button>
	<div class="notification-show-me-again">
		<input type="checkbox" checked>Don't show me again</div>
	</div>
</div>
`;

export const renderNotification = () => {
	// if user has confirm the notification and check `don't show me again`, ignore it
	if (!window.localStorage || window.localStorage.getItem(NOTIFICATION_STORAGE_KEY) === NOTIFICATION_STORAGE_VALUE) {
		return;
	}
Repository: conwnet/github1s
File: extensions/github1s/src/listeners/router/explorer.ts (TypeScript)

Code before:
/**
 * @file url listener for file Explorer
 * @author netcon
 */

import * as vscode from 'vscode';
import {
	changedFileDecorationProvider,
	submoduleDecorationProvider,
	sourceControlDecorationProvider,
	fileSearchProvider,
} from '@/providers';
import { RouterState } from '@/router/types';

export const explorerRouterListener = (
	currentState: RouterState,
	previousState: RouterState
) => {
	if (
		currentState.owner !== previousState.owner ||
		currentState.repo !== previousState.repo ||
		currentState.ref !== previousState.ref
	) {
		// should update the explorer
		vscode.commands.executeCommand(
			'workbench.files.action.refreshFilesExplorer'
		);
		// TODO: maybe we should update the editors but not close it
		vscode.commands.executeCommand('workbench.action.closeAllGroups');

		changedFileDecorationProvider.updateDecorations();
		submoduleDecorationProvider.updateDecorations();
		sourceControlDecorationProvider.updateDecorations();
		fileSearchProvider.loadFilesForCurrentAuthority();
	}
};


Review comment (suggestion):
I suggest we use something like this:

```
if (
  ['owner', 'repo', 'ref', 'pageType']
  .find(key => currentState[key] !== previousState[key])
) {
  ...
}
```

So that it's easier for future changes.

Code after:
/**
 * @file url listener for file Explorer
 * @author netcon
 */

import * as vscode from 'vscode';
import {
	changedFileDecorationProvider,
	submoduleDecorationProvider,
	sourceControlDecorationProvider,
	fileSearchProvider,
} from '@/providers';
import { RouterState } from '@/router/types';

export const explorerRouterListener = (
	currentState: RouterState,
	previousState: RouterState
) => {
	if (
		currentState.owner !== previousState.owner ||
		currentState.repo !== previousState.repo ||
		currentState.ref !== previousState.ref ||
		currentState.pageType !== previousState.pageType
	) {
		// should update the explorer
		vscode.commands.executeCommand(
			'workbench.files.action.refreshFilesExplorer'
		);
		// TODO: maybe we should update the editors but not close it
		vscode.commands.executeCommand('workbench.action.closeAllGroups');

		changedFileDecorationProvider.updateDecorations();
		submoduleDecorationProvider.updateDecorations();
		sourceControlDecorationProvider.updateDecorations();
		fileSearchProvider.loadFilesForCurrentAuthority();
	}
};
Repository: conwnet/github1s
File: extensions/github1s/src/helpers/fetch.ts (TypeScript)

Code before:
			headers: { ...authHeaders, ...customHeaders },
		});
	} catch (e) {
		throttledReportNetworkError();
		throw new RequestError('Request Failed, Maybe an Network Error', token);
	}
	if (response.status < 400) {
		cache.set(url, await response.json());
		return cache.get(url);
	}
	if (response.status === 403) {
		// if there is no token saved and the rate limit exceeded,
		// open the authorizing overly for requesting a access token
		if (!token && isGitHubApi(url)) {
			vscode.commands.executeCommand(
				'github1s.authorizing-github-with-overlay'
			);
		}
		return response.json().then((data) => {
			throw new RequestRateLimitError(data.message, token);
		});
	}
	if (response.status === 401) {
		return response.json().then((data) => {
			// current token is invalid
			if (data.message === 'Bad credentials') {
				vscode.commands.executeCommand('github1s.views.settings.focus');
			}
			throw new RequestInvalidTokenError(data.message, token);
		});
	}
	if (response.status === 404) {
		throw new RequestNotFoundError('Not Found', token);
	}
	throw new RequestError(
		(await response.json().catch(noop))?.message ||
			`GitHub1s: Request got HTTP ${response.status} response`,
		token
	);
});


Review comment (security):
Should we use `data.message?.includes('Bad credentials')` instead of `===` since the message might change.

Code after:
			headers: { ...authHeaders, ...customHeaders },
		});
	} catch (e) {
		throttledReportNetworkError();
		throw new RequestError('Request Failed, Maybe an Network Error', token);
	}
	if (response.status < 400) {
		cache.set(url, await response.json());
		return cache.get(url);
	}
	if (response.status === 403) {
		// if there is no token saved and the rate limit exceeded,
		// open the authorizing overly for requesting a access token
		if (!token && isGitHubApi(url)) {
			vscode.commands.executeCommand(
				'github1s.authorizing-github-with-overlay'
			);
		}
		return response.json().then((data) => {
			throw new RequestRateLimitError(data.message, token);
		});
	}
	if (response.status === 401) {
		return response.json().then((data) => {
			// current token is invalid
			if (data.message?.includes('Bad credentials')) {
				vscode.commands.executeCommand('github1s.views.settings.focus');
			}
			throw new RequestInvalidTokenError(data.message, token);
		});
	}
	if (response.status === 404) {
		throw new RequestNotFoundError('Not Found', token);
	}
	throw new RequestError(
		(await response.json().catch(noop))?.message ||
			`GitHub1s: Request got HTTP ${response.status} response`,
		token
	);
});
Repository: conwnet/github1s
File: extensions/github1s/src/adapters/ossinsight/interfaces.ts (TypeScript)

Code before:
/**
 * @file trending interfaces
 * @author netcon
 */

import { memorize } from '@/helpers/func';
import { RankingPeriod } from './constants';

const OSSInsightEndpoint = `https://api.ossinsight.io`;

const resolveDataFromResponse = (response: Response) => {
	if (response.status < 200 || response.status >= 300) {
		return [];
	}
	return response.json().then((body) => body?.data || []);
};

export type RepoItem = {
	collection_names?: string;
	contributor_logins?: string;
	description?: string;
	forks?: number;
	language?: string;
	repo_name: string;
	stars?: number;
	total_score?: number;
};

export const getTrendingRepos = (period: RankingPeriod, language: string): Promise<RepoItem[]> => {
	const encodedLanguage = encodeURIComponent(language) || 'All';
	const periodValue =
		period === RankingPeriod.Today ? 'past_24_hours' : RankingPeriod.ThisWeek ? 'past_week' : 'past_month';
	const requestUrl = `${OSSInsightEndpoint}/q/trending-repos?language=${encodedLanguage}&period=${periodValue}`;
	return fetch(requestUrl).then(resolveDataFromResponse);
};

export type RecentHotCollectionItem = {
	id: number;
	last_2nd_month_rank: number;
	last_month_rank: number;
	name: string;
	rank: number;
	rank_changes: number;
	repo_id: number;
	repo_name: string;
	repos: number;
	visits: number;

Review comment (suggestion):
**Suggestion** _(non-blocking)_: Any reason to remove it from being an optional entity?

```suggestion
	forks?: string;
```

Code after:
/**
 * @file trending interfaces
 * @author netcon
 */

import { memorize } from '@/helpers/func';
import { RankingPeriod } from './constants';

const OSSInsightEndpoint = `https://api.ossinsight.io`;

const resolveDataFromResponse = (response: Response) => {
	if (response.status < 200 || response.status >= 300) {
		return [];
	}
	return response.json().then((body) => body?.data?.rows || []);
};

export type RepoItem = {
	collection_names?: string;
	contributor_logins?: string;
	description?: string;
	forks: string;
	language?: string;
	repo_name: string;
	stars: string;
	total_score: string;
};

export const getTrendingRepos = (period: RankingPeriod, language: string): Promise<RepoItem[]> => {
	const encodedLanguage = encodeURIComponent(language) || 'All';
	const periodValue =
		period === RankingPeriod.Today ? 'past_24_hours' : RankingPeriod.ThisWeek ? 'past_week' : 'past_month';
	const requestUrl = `${OSSInsightEndpoint}/v1/trends/repos/?language=${encodedLanguage}&period=${periodValue}`;
	return fetch(requestUrl).then(resolveDataFromResponse);
};

export type RecentHotCollectionItem = {
	id: string;
	name: string;
	repos: string;
	repo_id: string;
	repo_name: string;
	repo_rank_changes: string;
	repo_past_period_rank: string;
	repo_current_period_rank: string;
};
Repository: coqui-ai/TTS
File: tests/test_extract_tts_spectrograms.py (Python)

Code before:
import os
import unittest

import torch

from tests import get_tests_input_path

from TTS.tts.models.tacotron2 import Tacotron2
from TTS.tts.models.glow_tts import GlowTTS

from TTS.utils.audio import AudioProcessor
from TTS.utils.io import load_config

from TTS.bin.extract_tts_spectrograms import inference

torch.manual_seed(1)
use_cuda = torch.cuda.is_available()
device = torch.device("cuda:0" if torch.cuda.is_available() else "cpu")

c = load_config(os.path.join(get_tests_input_path(), "test_config.json"))
# set params from tacotron inference
c.bidirectional_decoder = False
c.double_decoder_consistency = False
ap = AudioProcessor(**c.audio)



Review comment (bug):
I guess I couldn't explain well enough. I was suggesting to test ` TTS/bin/extract_tts_spectrograms.py` directly not the model. So if something fails in ` TTS/bin/extract_tts_spectrograms.py` we should know about it. In this case, it is not possible. 

You have two options. 

One is to create a shell script like the those under  "tests/bash_tests/*"

Or something like `test_synthesize.py` to call the script in python using `run_cli`. 

Makes sense ? 

Code after:
import os
import unittest

import torch

from tests import get_tests_input_path

from tests import get_tests_output_path, run_cli

from TTS.tts.utils.generic_utils import setup_model

from TTS.utils.io import load_config
from TTS.tts.utils.text.symbols import phonemes, symbols

torch.manual_seed(1)

# pylint: disable=protected-access
class TestExtractTTSSpectrograms(unittest.TestCase):
    @staticmethod
    def test_GlowTTS():
        # set paths
        config_path = os.path.join(get_tests_input_path(), "test_glow_tts.json")
        checkpoint_path = os.path.join(get_tests_output_path(), 'checkpoint_test.pth.tar')
        output_path = os.path.join(get_tests_output_path(), 'output_extract_tts_spectrograms/')
        # load config
        c = load_config(config_path)