From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qt1-f176.google.com (mail-qt1-f176.google.com [209.85.160.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C7709193402 for ; Tue, 27 May 2025 19:47:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.160.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748375250; cv=none; b=R8fQPwqRX1yUsv9WY7JvQZQjR8/DjeyfGYzFYbPaMdoscxctJo+A5jcbPTebOQORAl2K2cyI9s8ZIo3/TTyA1mmoowXgwAEZFSZhz6TFk3qicNHUW15GMhO/ZguAgcPKnRgGcyl8i0nhdpd1ZlhTBGE52e/xX9kVpl41f1kWyTU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748375250; c=relaxed/simple; bh=G2rafT66fDhRiGysU5vT2eVo7paLxasmbgHFkFl26PE=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=MDjuyJJ42TrXUM3IosRSPbmHP5OAJmBScXvwH+qxt7tToYrbr7Qocv5w5Kqs7a7YPgsXTuxvzGUbnm0IGM3K6DB8Z1PHVy5BHDmu01AmbztHFfjbjO2Omfco6JwKMU91SD973gzBDZ0e0jWLPEkHTyO7KHeyloUQrark1FwIIUc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=fail (p=none dis=none) header.from=sunshineco.com; spf=pass smtp.mailfrom=gmail.com; arc=none smtp.client-ip=209.85.160.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=fail (p=none dis=none) header.from=sunshineco.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-qt1-f176.google.com with SMTP id d75a77b69052e-49b3b0191f2so3812471cf.3 for ; Tue, 27 May 2025 12:47:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1748375247; x=1748980047; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=/P9x7jkR/cgmCMx1HCWf6bvEs42xkd1Mr516xw2pQas=; b=VGyaVuB6HzqlyfTOK1wUDQSlF1HMHpwRQxLzq0CLFWtX2VTnT2Xzk70OS/JuaZWpxP sqVkWJtcQ2xR2+iC8xePCqnmj6S6JGiWpvYpmZOCp+2a94jtvHcutqlpoUFSlly63XkC JTUdKeOIM6w1wK9t1pFJgpJ1JEaj0JG4vCP5e2nvWwVxghvM0w0mnIsY7zMKlrueROjG qM417asUuFESdGrX5zDOP2P5ZUUucj6YKnNTKjPVJoLpHe7OXs5eA2m7/ZJwU/OU41Xr 5FLt256un3upxaNHURN7yC7dgmX5HJvbKQjf+ACc3PR+atrA/MrAvay6zK6fVswk0BX1 B9Nw== X-Gm-Message-State: AOJu0YwL2kL62s9AmJuooQ4j0+57qVmwpepvDN79AoxRkKgDK8EDeZ66 S8+yCXdJu/S05Dw09uKVSctV/5bDFMKBlwlHuZd7pYl78n89ag5OK3oKp/9yYMOQC3w+uJTQqnz NodaunPJXMmGyp6VX1gqCVbe6SHuRwWM= X-Gm-Gg: ASbGncsys40Qgm72Exfes2HvAflgQBcUXjHwwqXTqWj/ct+nCNjnbOK1/dimMBUddcR FjPsJwxW3R2UtwoJ/UgrJQ5HOrMgKnMU7hXCnqnUmvPCmg2iqezPYtl/yT/Cr474gbF2GM3Z9jX BISeneDIrbf5Y8o6gG0ka2/hhLeov8ubM= X-Google-Smtp-Source: AGHT+IHyGiVJlGWO1jBOy2k3aWgWfCr0CYR9/0RnZ6PbeHEsjqHR2D387Mv742hLZTlSCkUtbH8NfhUxJNgm62iBspc= X-Received: by 2002:a05:622a:13c6:b0:471:fef5:ee85 with SMTP id d75a77b69052e-49f484b5eb0mr97688511cf.15.1748375247627; Tue, 27 May 2025 12:47:27 -0700 (PDT) Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20250527-pks-meson-tap-v2-0-ae360f77786e@pks.im> <20250527-pks-meson-tap-v2-1-ae360f77786e@pks.im> In-Reply-To: <20250527-pks-meson-tap-v2-1-ae360f77786e@pks.im> From: Eric Sunshine Date: Tue, 27 May 2025 15:47:16 -0400 X-Gm-Features: AX0GCFvc3MOzaOF8G2RY-S8kkgLzzoxfKpbZBKqH5QXzki9K0spCYkKWlbB-yjI Message-ID: Subject: Re: [PATCH v2 1/6] t: fix cases where output breaks TAP format To: Patrick Steinhardt Cc: git@vger.kernel.org, Phillip Wood , Junio C Hamano , Karthik Nayak , Ramsay Jones , Eli Schwartz , Todd Zullinger Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, May 27, 2025 at 10:03=E2=80=AFAM Patrick Steinhardt wro= te: > The TAP format does not allow arbitrary output outside of a specific > test case. If a test suite wants to print any such diagnostic output, > then this output has to be prefixed with "#" to mark it accordingly. > A bunch of our tests generate output outside of `test_expect_*` > testcases anyway without such a mark, which breaks strict TAP parsers. > > Upon further inspection, all of the output generated by such tests is > rather uninteresting. Refactor them so that we don't break the TAP > format. Nit: Can we avoid the word "refactor" for changes such as those made by this patch which clearly are not refactoring[*]. [*]: From Wikipedia: "... code refactoring is the process of restructuring existing source code=E2=80=94changing the factoring=E2=80=94w= ithout changing its external behavior." > Signed-off-by: Patrick Steinhardt > --- > diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh > @@ -30,7 +30,7 @@ setup_repo() { > test_repo=3Dtest > push_repo() { > - test_create_repo $test_repo > + test_create_repo $test_repo >/dev/null > cd $test_repo > setup_repo Yuck, but certainly the simplest "fix" in this particular case considering that, ultimately, this entire script ought to be reworked since it cd's around outside of tests with abandon. It would be nice to see this script get overhauled eventually but such an undertaking doesn't need to be part of this patch series. > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-op= tion.sh > @@ -48,7 +48,7 @@ commit_file () { > -test_create_repo sm1 && > +test_create_repo sm1 >/dev/null && > add_file . foo >/dev/null > > head1=3D$(add_file sm1 foo1 foo2) Unlike the case with t1007, in which the entire script needs an overhaul, it is much easier to fix the problems in this script without papering over them via ">/dev/null". In particular, it would be preferable to resolve the issue by wrapping test_expect_success around the code which currently resides outside of any test. So, for example, the above could become: test_expect_success 'setup submodule 1' ' test_create_repo sm1 && add_file . foo && head1=3D$(add_file sm1 foo1 foo2) && fullhead1=3D$(cd sm1; git rev-parse --verify HEAD) ' Note that I also dropped the ">/dev/null" redirect from the add_file() invocation. The same comment applies to similar changes made by this patch to other scripts, such as t4060, t7401. > diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encodi= ng.sh > @@ -7,12 +7,17 @@ test_description=3D'Clone repositories with non ASCII p= aths' > -ISO8859=3D"$(printf "$ISO8859_ESCAPED")" && > -echo content123 >"$ISO8859" && > -rm "$ISO8859" || { > +test_lazy_prereq FS_ACCEPTS_ISO_8859_1 ' > + ISO8859=3D"$(printf "$ISO8859_ESCAPED")" && > + echo content123 >"$ISO8859" 2>/dev/null && > + rm "$ISO8859" > +' Was the problem here that the `echo content123 > "$..."` was potentially spitting out an error message to stderr, thus you had to redirect it to /dev/null to silence it? If so, did the file get created in the error case? What I'm wondering is whether you also should use `rm -f` when removing the file.