git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Jan Mazur via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <stolee@gmail.com>,
	Jan Mazur <mzr@meta.com>,  Jan Mazur <mzr@fb.com>
Subject: Re: [PATCH] bundle-uri: replace printf with fprintf in print_bundle_uri function
Date: Wed, 04 Jun 2025 09:09:44 -0700	[thread overview]
Message-ID: <xmqqy0u7bi9j.fsf@gitster.g> (raw)
In-Reply-To: <pull.1985.git.git.1749023960409.gitgitgadget@gmail.com> (Jan Mazur via GitGitGadget's message of "Wed, 04 Jun 2025 07:59:20 +0000")

"Jan Mazur via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: [PATCH] bundle-uri: replace printf with fprintf in print_bundle_uri function

That says _what_ the patch does.  We care more deeply about _why_ it
is a good idea for a patch to do so.  And ...

> From: Jan Mazur <mzr@fb.com>

... there is nothing described here.

> Signed-off-by: Jan Mazur <mzr@meta.com>

Something like this, perhaps?

    Subject: [PATCH] bundle-uri: send debug output to given FILE * stream

    d796cedb (bundle-uri: unit test "key=value" parsing, 2022-10-12)
    introduced the print_bundle_list() function, which takes a "FILE
    *fp" to write the output to.  Later with c93c3d2f (bundle-uri:
    parse bundle.heuristic=creationToken, 2023-01-31) the function
    started showing additional information, which is always written
    to the standard output stream.

    It does not look like a deliberate decision to do so, and it
    does not hurt, as all callers of the function passes stdout to
    it.
    
    We could change the function not to take fp and always write to
    the standard output to simplify, but let's use the FILE *fp
    provided by the caller consistently to write out output.

The change the patch makes looks obviously correct.

Thanks.

> ---
>     bundle-uri: replace printf with fprintf in print_bundle_uri function
>     
>     Small fix to bundle-uri convenience debug function.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1985%2Fmzr%2Fbundle_uri__print_bundle_list_fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1985/mzr/bundle_uri__print_bundle_list_fix-v1
> Pull-Request: https://github.com/git/git/pull/1985
>
>  bundle-uri.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bundle-uri.c b/bundle-uri.c
> index 9accf157b44..c9d65aa0ce8 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -122,7 +122,7 @@ void print_bundle_list(FILE *fp, struct bundle_list *list)
>  		int i;
>  		for (i = 0; i < BUNDLE_HEURISTIC__COUNT; i++) {
>  			if (heuristics[i].heuristic == list->heuristic) {
> -				printf("\theuristic = %s\n",
> +				fprintf(fp, "\theuristic = %s\n",
>  				       heuristics[list->heuristic].name);
>  				break;
>  			}
>
> base-commit: b07857f7dcffee4d3b428df8dce6c9b49a57c9c1

  reply	other threads:[~2025-06-04 16:09 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-04  7:59 [PATCH] bundle-uri: replace printf with fprintf in print_bundle_uri function Jan Mazur via GitGitGadget
2025-06-04 16:09 ` Junio C Hamano [this message]
2025-06-04 17:11 ` [PATCH v2] bundle-uri: send debug output to given FILE * stream Jan Mazur via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqy0u7bi9j.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=mzr@fb.com \
    --cc=mzr@meta.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).