Re: [PATCH v2 04/11] sparse-checkout: 'set' subcommand

From: Elijah Newren <newren@gmail.com>
Date: Oct 6, 2019, 00:30 UTC
Message-ID: <CABPp-BGcZT2-m=D_X4d7zE9pzuWTKCnmURruahdQndpp=+-_WA@mail.gmail.com>
In-Reply-To: <CABPp-BHB0UM1G2tHQ7igRX7Sn1O9ujW6mq_e-UGanGMONHxiag@mail.gmail.com>

On Sat, Oct 5, 2019 at 3:44 PM Elijah Newren <newren@gmail.com> wrote:

Show 22 quoted lines

>
> On Thu, Sep 19, 2019 at 3:07 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > +static int write_patterns_and_update(struct pattern_list *pl)
> > +{
> > +       char *sparse_filename;
> > +       FILE *fp;
> > +
> > +       sparse_filename = get_sparse_checkout_filename();
> > +       fp = fopen(sparse_filename, "w");
> > +       write_patterns_to_file(fp, pl);
> > +       fclose(fp);
> > +       free(sparse_filename);
> > +
> > +       clear_pattern_list(pl);
>
> It seems slightly odd that pl is passed in but cleared in this
> function rather than in the caller that created pl.  Should this be
> moved to the caller, or, alternatively, a comment added to explain
> this side-effect for future callers of the function?
>
> The rest of the patch looked good to me.

Actually, thought of something else. What if the user calls 'git sparse-checkout set ...' without first calling 'git sparse-checkout init'? Should that report an error to the user, a suggestion to follow it up with 'sparse-checkout init', or should it just call sc_set_config() behind the scenes and allow bypassing the init subcommand?