From: Karthik Nayak <karthik.188@gmail.com>
To: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Cc: git@vger.kernel.org, christian.couder@gmail.com,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Date: Thu, 28 May 2015 12:38:34 +0530 [thread overview]
Message-ID: <5566BEF2.2000301@gmail.com> (raw)
In-Reply-To: <vpqpp5nfiea.fsf@anie.imag.fr>
On 05/26/2015 09:19 PM, Matthieu Moy wrote:
> Seconded. Some reasons/guide to split:
>
> * Split trivial and non-trivial stuff. I can quickly review a
> "rename-only" patch even if it's a bit long (essentially, I'll check
> that you did find-and-replace properly), but reviewing a mix of
> renames and actual code change is hard.
>
> * Split controversial and non-controversial stuff. For example, you
> changed the ordering of fields in a struct. Perhaps it was not a good
> idea. Perhaps it was a good idea, but then you want this reordering to
> be alone in its patch so that you can explain why it's a good idea in
> the commit message (you'll see me use the word "why" a lot when
> talking about commit messages; not a coincidence).u
Since one of the patches is to restructure and rename 'for-each-ref', I thought
It would be ideal to introduce the data structures within that patch, What do you
think?
>
> * Split code movement and other stuff. For example extraction of
> match_name_as_path() would be trivial if made in its own patch.
>
> I'd also make a separate patch "introduce the ref_list data-structure"
> to introduce struct ref_list and basic helper functions (constructors,
> mutators, destructors).
>
> It will take time and may appear to be counter-productive at first, but
> you'll get used to it, and you'll end up being actually more productive
> this way (well, maybe not "you" but the set "you + reviewers").
>
Thanks for this.
--
Regards,
Karthik
next prev parent reply other threads:[~2015-05-28 7:08 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-25 12:39 [WIP][Patch v2 0/2] Ref-filter: unification of 'tag -l', 'branch -l' and 'for-each-ref' Karthik Nayak
2015-05-25 12:45 ` [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter' Karthik Nayak
2015-05-25 17:15 ` Junio C Hamano
2015-05-25 17:59 ` Karthik Nayak
2015-05-25 19:39 ` Junio C Hamano
2015-05-26 6:58 ` Karthik Nayak
2015-05-26 15:49 ` Matthieu Moy
2015-05-28 7:08 ` Karthik Nayak [this message]
2015-05-28 11:26 ` Matthieu Moy
2015-05-25 12:45 ` [PATCH v2 2/2] ref-filter: move code from 'for-each-ref' Karthik Nayak
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=5566BEF2.2000301@gmail.com \
--to=karthik.188@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=matthieu.moy@grenoble-inp.fr \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.