Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Duy Nguyen]

On Tue, May 23, 2017 at 5:52 AM, Manish Goregaokar
 wrote:
> What work is remaining for prune-in-worktree? Link to the relevant 
> discussions?
>
> I might be able to take it over the finish line. (No guarantees)

The finish line should be pretty close. I've addressed Michael's other
comments except [1]. I pushed what I have done here [2]. The "wip"
commit was what I proposed to Michael and I believe he had a better
way of doing it  I think I had a look at the merge iterator once
before writing mine, but maybe I didn't look close enough to see it as
reusable in this use case.

[1] 
http://public-inbox.org/git/%3c00720e90-ed85-e8d8-a2e4-f42f93a33...@alum.mit.edu%3E/#r
[2] https://github.com/pclouds/git/commits/prune-in-worktrees-2
-- 
Duy

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Manish Goregaokar]

What work is remaining for prune-in-worktree? Link to the relevant discussions?

I might be able to take it over the finish line. (No guarantees)
-Manish Goregaokar


On Mon, May 22, 2017 at 4:17 AM, Duy Nguyen  wrote:
> On Sat, May 20, 2017 at 5:30 PM, Junio C Hamano  wrote:
>> By the way, doesn't nd/prune-in-worktree topic that has been cooking
>> in 'pu' supersede this change?  It not just protects the commit at
>> the tip of HEAD in each worktree, it also makes sure the ones in
>> HEAD's reflog are not prematurely pruned.
>
> You have probably noticed I have stayed silent for a long time (and
> probably will continue). But yes nd/prune-in-worktree should fix this.
> Unfortunately I will not be able to fix it up (I think Michael
> responded on my last question about the proper way to fix
> files_for_each_ref). So anyone feel free to pick that series up and
> fix it (or drop it). For now, pretend that I'm kidnapped by aliens.
> --
> Duy

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Duy Nguyen]

On Sat, May 20, 2017 at 5:30 PM, Junio C Hamano  wrote:
> By the way, doesn't nd/prune-in-worktree topic that has been cooking
> in 'pu' supersede this change?  It not just protects the commit at
> the tip of HEAD in each worktree, it also makes sure the ones in
> HEAD's reflog are not prematurely pruned.

You have probably noticed I have stayed silent for a long time (and
probably will continue). But yes nd/prune-in-worktree should fix this.
Unfortunately I will not be able to fix it up (I think Michael
responded on my last question about the proper way to fix
files_for_each_ref). So anyone feel free to pick that series up and
fix it (or drop it). For now, pretend that I'm kidnapped by aliens.
-- 
Duy

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Junio C Hamano]

Manish Goregaokar  writes:

> One thing which I think hasn't been covered yet is the rebase
> ORIG_HEAD. I'll see if that's still a problem on `pu` and make a patch
> for it if so.

IIRC, ORIG_HEAD, FETCH_HEAD, MERGE_HEAD and others are be transitory
and never served as the starting points of reachability traversal,
even in the primary worktree (also when you are not using any extra
worktrees).  The commits listed in the todo list of "rebase -i" and
"git cherry-pick A..B" are the same way.

Because ORIG_HEAD is expected to be in a reflog of some ref (at
least, the reflog of HEAD), I do not see much benefit (or "more
safety") in adding it to the starting points.

Among the ones that appear in .git/ and we never considered as
starting points, the commits in FETCH_HEAD might be the ones we may
want to give extra protection over what we currently have, simply
because the ones that do not have remote-tracking branches have NO
other refs pointing at them (compared to these transitory artifacts
resulting from a local operation, i.e. ORIG_HEAD and ones in the
todo list).  But they by definition are "new" objects, and the
reason why the user does *not* use remote-tracking branches for them
is because the user does not want to keep record of them unless the
user decides to merge them somewhere in the repository's history, so
even for them the benefit is dubious...

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Manish Goregaokar]

Yes, you are right (on both counts).

One thing which I think hasn't been covered yet is the rebase
ORIG_HEAD. I'll see if that's still a problem on `pu` and make a patch
for it if so.

(I recall `git prune` during a rebase messing up repo state, though
it's really my fault for trying that in the first place. Would be nice
if it worked, though)
-Manish Goregaokar


On Sat, May 20, 2017 at 3:30 AM, Junio C Hamano  wrote:
> manishea...@gmail.com writes:
>
>> +int for_each_worktree_ref(each_ref_fn fn, void *cb_data)
>> +{
>> + int i, flag, retval = 0;
>> + struct object_id oid;
>> + struct worktree **worktrees = get_worktrees(GWT_SORT_LINKED);
>> + struct commit* commit;
>> + for (i = 0; worktrees[i]; i++) {
>> + if ((commit = 
>> lookup_commit_reference(worktrees[i]->head_sha1))) {
>> + oid = commit->object.oid;
>> + if (!read_ref_full("HEAD", RESOLVE_REF_READING, 
>> oid.hash, )) {
>> + if ((retval = fn("HEAD", , flag, cb_data)))
>> + return retval;
>> + }
>> + }
>> + }
>> + return retval;
>> +}
>
> I would have expected for-each-worktree-ref to iterate over all the
> refs in a given worktree, but that is not what this does.  This
> instead iterates over worktrees and shows only their HEAD ref, no
> other refs.  This helper is somewhat misnamed.
>
> By the way, doesn't nd/prune-in-worktree topic that has been cooking
> in 'pu' supersede this change?  It not just protects the commit at
> the tip of HEAD in each worktree, it also makes sure the ones in
> HEAD's reflog are not prematurely pruned.
>
> Thanks.
>

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Junio C Hamano]

manishea...@gmail.com writes:

> +int for_each_worktree_ref(each_ref_fn fn, void *cb_data)
> +{
> + int i, flag, retval = 0;
> + struct object_id oid;
> + struct worktree **worktrees = get_worktrees(GWT_SORT_LINKED);
> + struct commit* commit;
> + for (i = 0; worktrees[i]; i++) {
> + if ((commit = 
> lookup_commit_reference(worktrees[i]->head_sha1))) {
> + oid = commit->object.oid;
> + if (!read_ref_full("HEAD", RESOLVE_REF_READING, 
> oid.hash, )) {
> + if ((retval = fn("HEAD", , flag, cb_data)))
> + return retval;
> + }
> + }
> + }
> + return retval;
> +}

I would have expected for-each-worktree-ref to iterate over all the
refs in a given worktree, but that is not what this does.  This
instead iterates over worktrees and shows only their HEAD ref, no
other refs.  This helper is somewhat misnamed.

By the way, doesn't nd/prune-in-worktree topic that has been cooking
in 'pu' supersede this change?  It not just protects the commit at
the tip of HEAD in each worktree, it also makes sure the ones in
HEAD's reflog are not prematurely pruned.

Thanks.

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Brandon Williams]

On 05/18, Samuel Lijin wrote:
> On Thu, May 18, 2017 at 5:40 AM, Simon Ruderich  wrote:
> > On Wed, May 17, 2017 at 06:45:31PM -0700, Manish Goregaokar wrote:
> >> Hm, my invocation of git-send-email keeps getting the threading wrong.
> >> Is there a recommended set of arguments to the command?
> >
> > The threading looks fine here (for both cases where you mentioned
> > it being wrong). Why do you think it's wrong? How does it look on
> > your end?
> 
> If you're on gmail (as Manish and I both are) patches in a subsequent

Gmail does not do threading, well not true threading.  It does grouping
of emails by subject line, that's it. If you want to view threading
(based on message-id) properly then you'll probably need to use a
different email client.

> version will be threaded (wrongly) against "earlier" versions of the
> patch. So if you have patch series A0, A1, A2, A3 and new version B0,
> B1, B2, if you thread them as
> 
> A0
> - A1
> - A2
> - A3
> - B0
>   - B1
>   - B2
> 
> gmail will show them in your inbox as
> 
> A0
> - B0
> 
> A1
> - B1
> 
> A2
> 
> A3
> - B2
> 
> Depending on whatever heuristics they use to match up "threads",
> presumably because most emails aren't threaded correctly.

-- 
Brandon Williams

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Samuel Lijin]

On Thu, May 18, 2017 at 5:40 AM, Simon Ruderich  wrote:
> On Wed, May 17, 2017 at 06:45:31PM -0700, Manish Goregaokar wrote:
>> Hm, my invocation of git-send-email keeps getting the threading wrong.
>> Is there a recommended set of arguments to the command?
>
> The threading looks fine here (for both cases where you mentioned
> it being wrong). Why do you think it's wrong? How does it look on
> your end?

If you're on gmail (as Manish and I both are) patches in a subsequent
version will be threaded (wrongly) against "earlier" versions of the
patch. So if you have patch series A0, A1, A2, A3 and new version B0,
B1, B2, if you thread them as

A0
- A1
- A2
- A3
- B0
  - B1
  - B2

gmail will show them in your inbox as

A0
- B0

A1
- B1

A2

A3
- B2

Depending on whatever heuristics they use to match up "threads",
presumably because most emails aren't threaded correctly.

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Simon Ruderich]

On Wed, May 17, 2017 at 06:45:31PM -0700, Manish Goregaokar wrote:
> Hm, my invocation of git-send-email keeps getting the threading wrong.
> Is there a recommended set of arguments to the command?

The threading looks fine here (for both cases where you mentioned
it being wrong). Why do you think it's wrong? How does it look on
your end?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Manish Goregaokar]

Hm, my invocation of git-send-email keeps getting the threading wrong.
Is there a recommended set of arguments to the command?

Thanks,
-Manish Goregaokar


On Wed, May 17, 2017 at 6:42 PM,   wrote:
> From: Manish Goregaokar 
>
> To ensure that `git prune` does not remove refs checked out
> in other worktrees, we need to include these HEADs in the
> set of roots. This adds the iteration function necessary
> to do this.
>
> Signed-off-by: Manish Goregaokar 
> ---
>  refs.c | 20 
>  refs.h |  1 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index 2d71774..7dc82ba 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3,10 +3,12 @@
>   */
>
>  #include "cache.h"
> +#include "commit.h"
>  #include "lockfile.h"
>  #include "refs.h"
>  #include "refs/refs-internal.h"
>  #include "object.h"
> +#include "worktree.h"
>  #include "tag.h"
>
>  /*
> @@ -1157,6 +1159,24 @@ int head_ref(each_ref_fn fn, void *cb_data)
> return head_ref_submodule(NULL, fn, cb_data);
>  }
>
> +int for_each_worktree_ref(each_ref_fn fn, void *cb_data)
> +{
> +   int i, flag, retval = 0;
> +   struct object_id oid;
> +   struct worktree **worktrees = get_worktrees(GWT_SORT_LINKED);
> +   struct commit* commit;
> +   for (i = 0; worktrees[i]; i++) {
> +   if ((commit = 
> lookup_commit_reference(worktrees[i]->head_sha1))) {
> +   oid = commit->object.oid;
> +   if (!read_ref_full("HEAD", RESOLVE_REF_READING, 
> oid.hash, )) {
> +   if ((retval = fn("HEAD", , flag, 
> cb_data)))
> +   return retval;
> +   }
> +   }
> +   }
> +   return retval;
> +}
> +
>  /*
>   * Call fn for each reference in the specified submodule for which the
>   * refname begins with prefix. If trim is non-zero, then trim that
> diff --git a/refs.h b/refs.h
> index 9fbff90..425a853 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -192,6 +192,7 @@ typedef int each_ref_fn(const char *refname,
>   * stop the iteration.
>   */
>  int head_ref(each_ref_fn fn, void *cb_data);
> +int for_each_worktree_ref(each_ref_fn fn, void *cb_data);
>  int for_each_ref(each_ref_fn fn, void *cb_data);
>  int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
>  int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
> --
> 2.10.1
>