Hi Junio,
On Thu, 21 Nov 2019, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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
