Re: [PATCH 1/8] built-in add -i: allow filtering the modified files list

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Thu, 21 Nov 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +enum modified_files_filter {
> > +	NO_FILTER = 0,
> > +	WORKTREE_ONLY = 1,
> > +	INDEX_ONLY = 2,
> > +};
> > +
> > +static int get_modified_files(struct repository *r,
> > +			      enum modified_files_filter filter,
> > +			      struct string_list *files,
> >  			      const struct pathspec *ps)
> >  {
> >  	struct object_id head_oid;
> >  	int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
> >  					     &head_oid, NULL);
> >  	struct collection_status s = { FROM_WORKTREE };
>
> Will have a comment on this later.

Yes, you're right, this initialization does not make sense. I changed it
to `{ 0 }` because I still want the struct to be zero'ed out.

> > +	int i;
> >
> >  	if (discard_index(r->index) < 0 ||
> >  	    repo_read_index_preload(r, ps, 0) < 0)
> > @@ -411,10 +424,16 @@ static int get_modified_files(struct repository *r, struct string_list *files,
> >  	s.files = files;
> >  	hashmap_init(&s.file_map, pathname_entry_cmp, NULL, 0);
> >
> > -	for (s.phase = FROM_WORKTREE; s.phase <= FROM_INDEX; s.phase++) {
> > +	for (i = 0; i < 2; i++) {
> >  		struct rev_info rev;
> >  		struct setup_revision_opt opt = { 0 };
> >
> > +		if (filter == INDEX_ONLY)
> > +			s.phase = i ? FROM_WORKTREE : FROM_INDEX;
> > +		else
> > +			s.phase = i ? FROM_INDEX : FROM_WORKTREE;
> > +		s.skip_unseen = filter && i;
>
> ;-)
>
> Looks somewhat crazy but it works---when the caller wants to do
> 'index-only' for example we are not interested in paths that did not
> appear in the INDEX side of the diff, so we run FROM_INDEX diff first
> and then the let next one (i.e. FROM_WORKTREE diff) be skipped for
> paths that are not in the result of the first one.  To me personally,
> I would find the tertially expession written like this
>
> 	s.phase = (i == 0) ? FROM_INDEX : FROM_WORKTREE;
>
> much easier to follow, as it matches the order which ones are done
> first.

Sure, that reverses the order, but it makes it more intuitive because `i
== 0` happens first. Changed.

> Also I notice two things.
>
>  - It used to make 100% sense to call this field .phase because we
>    always processed the first phase and then on to the second phase,
>    where the first one was called WORKTREE and the second one was
>    called INDEX.  In the new world order after this step, the name
>    .phase no longer makes any sense.  Sometimes we run diff-files in
>    phase 0 and diff-index in phase 1, but some other times we run
>    diff-index in phase 0 and diff-files in phase 0.  The variable
>    that has the closest meaning to the 'phase' is the newly
>    introduced 'i'.

I renamed it to `mode` in this commit. While I usually frown on renaming
fields in the same commit as introducing changes in behavior, I think in
this case it's kind of okay because it does not add many lines.

>  - The initialization of the local "struct collection_status s" at
>    the beginning of the function still uses .phase = FROM_WORKTREE
>    which might be somewhat misleading.  We cannot remove the whole
>    initialization, as the original used to nul initialize the other
>    fields in this structure and I suspect the code still relies on
>    it, but the initialization of .phase in particular no longer has
>    any effect; it always is assigned inside the loop before the
>    field gets used.
>
>
> >  		opt.def = is_initial ?
> >  			empty_tree_oid_hex() : oid_to_hex(&head_oid);
>
> By the way, this is not a new issue introduced by this patch, but I
> notice copy_pathspec() is used twice on the same rev.prune_data in
> this functino.  Who is responsible for releasing the resource held
> in this struct?

Good point. I assumed that the diff machinery would take care of this, but
it does not. So I introduced another patch to fix up what is already in
`next`.

Ciao,
Dscho