git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / 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
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 index

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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git