git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] fast-export: factor out print_oid()
Date: Thu, 13 Aug 2020 11:18:56 -0400	[thread overview]
Message-ID: <20200813151856.GE2244@syl.lan> (raw)
In-Reply-To: <1eb633f5-31ec-5d8d-69ac-35d0fb9772da@web.de>

On Thu, Aug 13, 2020 at 01:11:18PM +0200, René Scharfe wrote:
> Simplify the output code by splitting it up and reducing duplication.
> Reuse the logic for printing object IDs -- anonymized if needed -- by
> moving it to its own function, print_oid().
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/fast-export.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 9f37895d4cf..49bb50634ab 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -420,6 +420,14 @@ static const char *anonymize_oid(const char *oid_hex)
>  	return anonymize_str(&objs, generate_fake_oid, oid_hex, len, NULL);
>  }
>
> +static void print_oid(const struct object_id *oid, int anonymize)
> +{
> +	const char *oid_hex = oid_to_hex(oid);
> +	if (anonymize)
> +		oid_hex = anonymize_oid(oid_hex);
> +	fputs(oid_hex, stdout);
> +}
> +

The fact that this calls fputs makes this patch (in my own opinion)
noisier than it needs to be. This is because of all of the factoring out
of the other printfs. I'd expect that this looks something more like:

  -				       anonymize ?
  -				       anonymize_oid(oid_to_hex(&spec->oid)) :
  -				       oid_to_hex(&spec->oid));
  +				       anonymize_oid(anonymize, &spec->oid));

without moving around all of the other printf code.

>  static void show_filemodify(struct diff_queue_struct *q,
>  			    struct diff_options *options, void *data)
>  {
> @@ -470,21 +478,19 @@ static void show_filemodify(struct diff_queue_struct *q,
>  		case DIFF_STATUS_TYPE_CHANGED:
>  		case DIFF_STATUS_MODIFIED:
>  		case DIFF_STATUS_ADDED:
> +			printf("M %06o ", spec->mode);
>  			/*
>  			 * Links refer to objects in another repositories;
>  			 * output the SHA-1 verbatim.
>  			 */
>  			if (no_data || S_ISGITLINK(spec->mode))
> -				printf("M %06o %s ", spec->mode,
> -				       anonymize ?
> -				       anonymize_oid(oid_to_hex(&spec->oid)) :
> -				       oid_to_hex(&spec->oid));
> +				print_oid(&spec->oid, anonymize);
>  			else {
>  				struct object *object = lookup_object(the_repository,
>  								      &spec->oid);
> -				printf("M %06o :%d ", spec->mode,
> -				       get_object_mark(object));
> +				printf(":%d", get_object_mark(object));
>  			}
> +			putchar(' ');

... That said, this transformation looks correct from a quick glance.

>  			print_path(spec->path);
>  			string_list_insert(changed, spec->path);
>  			putchar('\n');
> @@ -724,12 +730,10 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
>  		else
>  			printf("merge ");
>  		if (mark)
> -			printf(":%d\n", mark);
> +			printf(":%d", mark);
>  		else
> -			printf("%s\n",
> -			       anonymize ?
> -			       anonymize_oid(oid_to_hex(&obj->oid)) :
> -			       oid_to_hex(&obj->oid));
> +			print_oid(&obj->oid, anonymize);
> +		putchar('\n');

As does this one.

>  		i++;
>  	}
>
> --
> 2.28.0

So, I guess what I'm trying to say is that this patch doesn't look
wrong, other than that it seems more invasive than I would have expected
it to be.

Thanks,
Taylor

  parent reply	other threads:[~2020-08-13 15:19 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-13 11:11 René Scharfe
2020-08-13 11:58 ` Jeff King
2020-08-13 17:17   ` René Scharfe
2020-08-13 15:18 ` Taylor Blau [this message]
2020-08-13 17:17   ` René Scharfe
2020-08-13 17:25     ` Jeff King
2020-08-15  7:14       ` René Scharfe
2020-08-17 22:08         ` Jeff King
2020-08-17 22:53           ` Junio C Hamano
2020-08-13 18:19   ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20200813151856.GE2244@syl.lan \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --subject='Re: [PATCH] fast-export: factor out print_oid()' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).