From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Nieder Subject: Re: [PATCH] refs.c: use a stringlist for repack_without_refs Date: Tue, 18 Nov 2014 15:45:00 -0800 Message-ID: <20141118234500.GO6527@google.com> References: <1416350636-12934-1-git-send-email-sbeller@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: git@vger.kernel.org, gitster@pobox.com, Ronnie Sahlberg To: Stefan Beller X-From: git-owner@vger.kernel.org Wed Nov 19 00:45:10 2014 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1XqsSM-0001XJ-2W for gcvg-git-2@plane.gmane.org; Wed, 19 Nov 2014 00:45:10 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933007AbaKRXpE (ORCPT ); Tue, 18 Nov 2014 18:45:04 -0500 Received: from mail-ig0-f182.google.com ([209.85.213.182]:48522 "EHLO mail-ig0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932303AbaKRXpC (ORCPT ); Tue, 18 Nov 2014 18:45:02 -0500 Received: by mail-ig0-f182.google.com with SMTP id hn15so33389igb.9 for ; Tue, 18 Nov 2014 15:45:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=WA4SWQUx0Aw1CR/9Sn5zu3Sf1jlun06VfjPRhdh3dp8=; b=bc/Vsld0cc0q7XTAqNg8vn++FitwExJUZ1ZJbWqiF4beVagpkTwvXyLLFGzWPqv8Uq ztBw3xgh2W8bO/riqf4TXe7z+v7iFmJZBrAE0hMp6QJVjP2B8ZfNYk22bn7m9pLrliib M2Ht/4Nf1syZw/WEe2wrX0IOu247Bwsuc94zJQb9/kJIrwcVYwO/gRk1CH2m58P4iD6o SPeudLRZQWekt0iP/QsUT5Ff030wQcT4q+8GivEHymVz6i/8YWuI1jiPyKXZWE7Axkug EAiTH4yzPUyP1HF4PmMzQg/XMvCp0mdT5MRAqEUiILFQZQ098EFcbEwNulfVXaRhUSrJ w8vQ== X-Received: by 10.50.153.8 with SMTP id vc8mr7038209igb.20.1416354301440; Tue, 18 Nov 2014 15:45:01 -0800 (PST) Received: from google.com ([2620:0:1000:5b00:1d6d:1067:602d:d9c7]) by mx.google.com with ESMTPSA id kd5sm8493532igb.20.2014.11.18.15.45.00 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 18 Nov 2014 15:45:00 -0800 (PST) Content-Disposition: inline In-Reply-To: <1416350636-12934-1-git-send-email-sbeller@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Stefan Beller wrote: > This patch was heavily inspired by a part of the ref-transactions-rename > series[1], but people tend to dislike large series and this part is > relatively easy to take out and unrelated, so I'll send it as a single > patch. > > [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html The above is a useful kind of comment to put below the three-dashes. It doesn't explain what the intent behind the patch is, why I should want this patch when considering whether to upgrade git, or what is going to break when I consider reverting it as part of fixing something else, so it doesn't belong in the commit message. > This patch doesn't intend any functional changes. It is just a refactoring, > which replaces a char** array by a stringlist in the function > repack_without_refs. Thanks. Why, though? Is it about having something simpler to pass from builtin/remote.c::remove_branches(), or something else? > Idea-by: Ronnie Sahlberg > Signed-off-by: Stefan Beller Isn't the patch by Ronnie? Sometimes I send a patch by someone else and make some change that I don't want them to be blamed for. Then I keep their sign-off and put a note in the commit message about the change I made. See output from git log origin/pu --grep='jc:' for more examples of that. Some nits below. > --- a/builtin/remote.c > +++ b/builtin/remote.c [...] > @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run) [...] > memset(&states, 0, sizeof(states)); > get_remote_ref_states(remote, &states, GET_REF_STATES); > > + for (i = 0; i < states.stale.nr; i++) > + string_list_insert(&delete_refs_list, > + states.stale.items[i].util); > + > + > if (states.stale.nr) { (style) The double blank line looks odd here. > printf_ln(_("Pruning %s"), remote); > printf_ln(_("URL: %s"), > @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run) > ? states.remote->url[0] > : _("(no URL)")); > > - delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs)); Now that there's no delete_refs array duplicating the string list, would it make sense to rename delete_refs_list to delete_refs? As a nice side-effect, that would make the definition of delete_refs_list and other places it is used appear in the patch. > for (i = 0; i < states.stale.nr; i++) { > const char *refname = states.stale.items[i].util; (optional) this could be for_each_string_list_item(ref, &delete_refs_list) { const char *refname = ref->string; ... which saves the reader from having to remember what states.stale.items means. [...] > +++ b/refs.c [...] > @@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err) [...] > - int i, ret, removed = 0; > + int count, ret, removed = 0; > > assert(err); > > - /* Look for a packed ref */ The old code has comments marking sections of the function: /* Look for a packed ref */ /* Avoid processing if we have nothing to do */ /* Remove refnames from the cache */ /* Remove any other accumulated cruft */ /* Write what remains */ Is dropping this comment intended? > - for (i = 0; i < n; i++) > - if (get_packed_ref(refnames[i])) > - break; > + count = 0; > + for_each_string_list_item(ref_to_delete, without) > + if (get_packed_ref(ref_to_delete->string)) > + count++; The old code breaks out early as soon as it finds a ref to delete. Can we do similar? E.g. for (i = 0; i < without->nr; i++) if (get_packed_ref(without->items[i].string)) break; (not about this patch) Is refs_to_delete leaked? [...] > @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, > int ref_transaction_commit(struct ref_transaction *transaction, > struct strbuf *err) > { > - int ret = 0, delnum = 0, i; > - const char **delnames; > + int ret = 0, i; > int n = transaction->nr; > struct ref_update **updates = transaction->updates; > + struct string_list refs_to_delete = STRING_LIST_INIT_DUP; The old code doesn't xstrdup the list items, so _NODUP should work fine (and be slightly more efficient). [...] > @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction, > } > > if (!(update->flags & REF_ISPRUNING)) > - delnames[delnum++] = update->lock->ref_name; > + string_list_insert(&refs_to_delete, > + update->lock->ref_name); string_list_append would be analagous to the old code. [....] > --- a/refs.h > +++ b/refs.h > @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void); > */ > int pack_refs(unsigned int flags); > > -extern int repack_without_refs(const char **refnames, int n, > - struct strbuf *err); > +extern int repack_without_refs(struct string_list *without, struct strbuf *err); A comment could mention whether the ref list needs to be sorted. (It doesn't, right?) Thanks and hope that helps, Jonathan