git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 1/8] built-in add -i: allow filtering the modified files list
Date: Thu, 21 Nov 2019 17:06:25 +0900	[thread overview]
Message-ID: <xmqqeey18xha.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <1844c4d55e21c40cbdbfdd73c82b4a1a074ff184.1573821382.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Fri, 15 Nov 2019 12:36:15 +0000")

"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.

> +	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?

  reply	other threads:[~2019-11-21  8:06 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-15 12:36 [PATCH 0/8] build-in add -i: implement all commands in the main loop Johannes Schindelin via GitGitGadget
2019-11-15 12:36 ` [PATCH 1/8] built-in add -i: allow filtering the modified files list Johannes Schindelin via GitGitGadget
2019-11-21  8:06   ` Junio C Hamano [this message]
2019-11-25 15:20     ` Johannes Schindelin
2019-11-15 12:36 ` [PATCH 2/8] built-in add -i: prepare for multi-selection commands Johannes Schindelin via GitGitGadget
2019-11-21  8:12   ` Junio C Hamano
2019-11-25 15:21     ` Johannes Schindelin
2019-11-15 12:36 ` [PATCH 3/8] built-in add -i: implement the `update` command Johannes Schindelin via GitGitGadget
2019-11-15 12:36 ` [PATCH 4/8] built-in add -i: re-implement `revert` in C Johannes Schindelin via GitGitGadget
2019-11-15 12:36 ` [PATCH 5/8] built-in add -i: re-implement `add-untracked` " Johannes Schindelin via GitGitGadget
2019-11-15 12:36 ` [PATCH 6/8] built-in add -i: implement the `patch` command Johannes Schindelin via GitGitGadget
2019-11-15 12:36 ` [PATCH 7/8] built-in add -i: re-implement the `diff` command Johannes Schindelin via GitGitGadget
2019-11-15 12:36 ` [PATCH 8/8] built-in add -i: offer the `quit` command Johannes Schindelin via GitGitGadget
2019-11-18  2:17 ` [PATCH 0/8] build-in add -i: implement all commands in the main loop Junio C Hamano
2019-11-18  2:22   ` Junio C Hamano
2019-11-18 19:22     ` Johannes Schindelin
2019-11-18  2:27 ` Junio C Hamano
2019-11-18 18:53   ` Johannes Schindelin
2019-11-19  1:29     ` Junio C Hamano
2019-11-29 21:11 ` [PATCH v2 0/9] built-in " Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 1/9] add-interactive: make sure to release `rev.prune_data` Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 2/9] built-in add -i: allow filtering the modified files list Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 3/9] built-in add -i: prepare for multi-selection commands Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 4/9] built-in add -i: implement the `update` command Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 5/9] built-in add -i: re-implement `revert` in C Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 6/9] built-in add -i: re-implement `add-untracked` " Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 7/9] built-in add -i: implement the `patch` command Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 8/9] built-in add -i: re-implement the `diff` command Johannes Schindelin via GitGitGadget
2019-11-29 21:11   ` [PATCH v2 9/9] built-in add -i: offer the `quit` command Johannes Schindelin via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqeey18xha.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).