"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.
> + 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.
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'.
- 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?
