git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 1/8] built-in add -i: allow filtering the modified files list
Date: Mon, 25 Nov 2019 16:20:26 +0100 (CET)
Message-ID: <nycvar.QRO.7.76.6.1911251558200.31080@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqeey18xha.fsf@gitster-ct.c.googlers.com>

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

  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
2019-11-25 15:20     ` Johannes Schindelin [this message]
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 publically 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=nycvar.QRO.7.76.6.1911251558200.31080@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /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.org/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