From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id C39A51F5AE for ; Wed, 21 Apr 2021 16:36:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235993AbhDUQhL convert rfc822-to-8bit (ORCPT ); Wed, 21 Apr 2021 12:37:11 -0400 Received: from mav.lukeshu.com ([104.207.138.63]:59508 "EHLO mav.lukeshu.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234887AbhDUQhL (ORCPT ); Wed, 21 Apr 2021 12:37:11 -0400 Received: from lukeshu-dw-thinkpad (c-73-229-136-185.hsd1.co.comcast.net [73.229.136.185]) by mav.lukeshu.com (Postfix) with ESMTPSA id C44AD80590; Wed, 21 Apr 2021 12:36:37 -0400 (EDT) Date: Wed, 21 Apr 2021 10:34:28 -0600 Message-ID: <87k0ov38bv.wl-lukeshu@lukeshu.com> From: Luke Shumaker To: =?ISO-8859-1?Q?=C6var_Arnfj=F6r=F0?= Bjarmason Cc: Luke Shumaker , git@vger.kernel.org, Luke Shumaker , Elijah Newren , Jeff King , Junio C Hamano Subject: Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name In-Reply-To: <8735vk3vyq.fsf@evledraar.gmail.com> References: <20210420190552.822138-1-lukeshu@lukeshu.com> <8735vk3vyq.fsf@evledraar.gmail.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, 21 Apr 2021 02:03:57 -0600, Ævar Arnfjörð Bjarmason wrote: > On Tue, Apr 20 2021, Luke Shumaker wrote: > > > -static void handle_tag(const char *name, struct tag *tag) > > +static void handle_tag(const char *refname, struct tag *tag) > > { > > unsigned long size; > > enum object_type type; > > char *buf; > > - const char *tagger, *tagger_end, *message; > > + const char *refbasename; > > + const char *tagname, *tagname_end, *tagger, *tagger_end, *message; > > Let's put the new "*tagname, *tagname_end" on its own line, the current > convention is to not conflate unrelated variable declarations on the > same line (as e.g. the existing "message" and "tagger" does. Ack. > > size_t message_size = 0; > > struct object *tagged; > > int tagged_mark; > > @@ -800,6 +801,11 @@ static void handle_tag(const char *name, struct tag *tag) > > message += 2; > > message_size = strlen(message); > > } > > + tagname = memmem(buf, message ? message - buf : size, "\ntag ", 5); > > + if (!tagname) > > + die("malformed tag %s", oid_to_hex(&tag->object.oid)); > > + tagname += 5; > > + tagname_end = strchrnul(tagname, '\n'); > > So it's no longer possible to export a reporitory with a missing "tag" > entry in a tag? Maybe OK, but we have an escape hatch for it with fsck, > we don't need one here? > > In any case a test for it would be good to have. I hadn't realized that it was possible for a tag object to be missing the "tag" entry, I will fix that. I don't think it's worth adding an option to fast-import to make it possible to create such an object, but fast-export should be able to handle it (and emit it in the stream such that fast-import would create it with the "tag" entry"). Yes, the whole patch needs tests. > > @@ -884,14 +890,19 @@ static void handle_tag(const char *name, struct tag *tag) > > > > if (tagged->type == OBJ_TAG) { > > printf("reset %s\nfrom %s\n\n", > > - name, oid_to_hex(&null_oid)); > > + refname, oid_to_hex(&null_oid)); > > } > > - skip_prefix(name, "refs/tags/", &name); > > - printf("tag %s\n", name); > > + refbasename = refname; > > + skip_prefix(refbasename, "refs/tags/", &refbasename); > > + printf("tag %s\n", refbasename); > > if (mark_tags) { > > mark_next_object(&tag->object); > > printf("mark :%"PRIu32"\n", last_idnum); > > } > > + if ((size_t)(tagname_end - tagname) != strlen(refbasename) || > > Would be more readable IMO to have a temporary variable for that > "tagname_end - tagname", then just cast that and use it here. > > > + strncmp(tagname, refbasename, (size_t)(tagname_end - tagname))) > > and here. Ack. > > @@ -2803,6 +2803,13 @@ static void parse_new_tag(const char *arg) > > read_next_command(); > > parse_mark(); > > > > + /* name ... */ > > + if (skip_prefix(command_buf.buf, "name ", &v)) { > > + name = strdupa(v); > > + read_next_command(); > > + } else > > + name = NULL; > > + > > Skip this whole (stylistically incorrect, should have {}) and just > initialize it to NULL when you declare the variable? In my defense, the guideline has always been to match the local style, and in fast-import.c this is done without {} 8 times and with {} 3 times :) -- Happy hacking, ~ Luke Shumaker