On Mon, Oct 7, 2019 at 11:26 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>
> On 10/5/2019 8:30 PM, Elijah Newren wrote:
> > On Sat, Oct 5, 2019 at 3:44 PM Elijah Newren <newren@xxxxxxxxx> wrote:
> >>
> >> On Thu, Sep 19, 2019 at 3:07 PM Derrick Stolee via GitGitGadget
> >> <gitgitgadget@xxxxxxxxx> 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?
>
> Maybe a warning would suffice. I still think the workflow of the
> following is most correct, and not difficult to recommend:
>
> * "git sparse-checkout init [--cone]" -OR- "git clone --sparse"
> * git sparse-checkout set [stuff]
> * git sparse-checkout disable
Recommending the right thing is easy, but users will call things out
of order despite documentation. If they call disable before init, I
see no problems that will lead to confusion. If they call set without
calling init, I can see them being surprised...so I commented on it
and asked if we want a warning or whatever.
