Re: [PATCH v6 25/25] refs: break out ref conflict checks

[Junio C Hamano]

Michael Haggerty  writes:

> I hate to cause the maintainer extra work. I guess I was making two
> naive assumptions:
>
> * If we make the code-movement series simple and "obviously correct"
>   enough, then it could be merged pretty much straight through to
>   master.
>
> * If one or two topics conflict with the code movement, they could
>   be one-time rebased on top of the new master (I would be willing
>   to do this work).
>
> Maybe neither of these assumptions is valid.

It is sometimes necessary to declare "This is important, and it is
solid enough. Let's fast-track it.  Other people must work on top of
this." and make a project-wide decision to do so.  It was just that
was not doable within the time I had before I had to hand the tree
off.

You can still work with the other people involved (Lukas as the
ref-is-hidden topic contributor and Peff as the interim maintainer)
to make it happen, and I wouldn't be upset.

> I can see a few ways that we could make our series even more
> straightforward:
>
> 1. Leave refs.c in its original location (as suggested by Junio).
>Optionally, it could be moved at a later date when this area is
>quiescent.
>
> 2. Move content selectively from refs.c to refs/files-backend.c rather
>than moving the whole file there and then moving content selectively
>back to refs/refs.c.
>
> 3. Separate *all* of the non-obvious changes into a preparatory
>patch series, to be followed by a separate patch series that *only*
>moves code.

4. Move everything in the series as-is, *BUT* in each patch where
the series moved things to refs/refs.c from refs/files-backend.c,
move that back to refs.c instead.  No refs/refs.c anywhere in the
history or in the endgame.

I think having refs.h and refs.c at the toplevel for the public
interface, with refs/ hierarchy holding backend specifics, would be
a good organization for the endgame regardless of the conflicts with
in-flight topics, and 4. achieves that.  Perhaps you meant the same
thing as your 1.

As to the reordering of functions and structure defs, I am
reasonably sure that the order in the final refs/refs.c in your
series would be better thought-out than in the original refs.c, so
the "reorder stuff in refs.c" can be the first step before
everything is moved to refs/files-backend.c, perhaps?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[Michael Haggerty]

On 11/07/2015 12:24 AM, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
>> Michael Haggerty  writes:
>>
>>> [...] And then, if you are also OK with the
>>> new subdirectory introduced in this patch series, David and I seem to be
>>> in agreement that it is ready to go. It would be great if this patch
>>> series could be merged in a timely manner, as it will conflict with
>>> nearly any other changes that people might want to undertake in the refs
>>> code.
>>
>> Thanks for working well together.  Let me see what I can do today...
> 
> What I'll push out later today merges this to the tip of 'pu'.
> The resolution is the same for 'jch' or 'next' (I checked).
> 
> I have to say that the merge of this topioc is not pretty.  [...]

I hate to cause the maintainer extra work. I guess I was making two
naive assumptions:

* If we make the code-movement series simple and "obviously correct"
  enough, then it could be merged pretty much straight through to
  master.

* If one or two topics conflict with the code movement, they could
  be one-time rebased on top of the new master (I would be willing
  to do this work).

Maybe neither of these assumptions is valid. And maybe the correctness
of our series in its current form isn't quite obvious enough.

David and I will be the ones who benefit most from having this resolved
quickly, because there is lots more work that has to be done after the
code movement and it is kindof hard to continue that work while this
topic is up in the air.

I can see a few ways that we could make our series even more
straightforward:

1. Leave refs.c in its original location (as suggested by Junio).
   Optionally, it could be moved at a later date when this area is
   quiescent.

2. Move content selectively from refs.c to refs/files-backend.c rather
   than moving the whole file there and then moving content selectively
   back to refs/refs.c.

3. Separate *all* of the non-obvious changes into a preparatory
   patch series, to be followed by a separate patch series that *only*
   moves code.

The first idea would be a couple of hours of work (including adjusting
comments and commit messages). The second and third ideas would probably
best be done in combination, and might take a day or two of work because
they involve reordering patches.

Let us know what you think.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[Michael Haggerty]

On 11/08/2015 06:03 AM, Michael Haggerty wrote:
> [...]
> I can see a few ways that we could make our series even more
> straightforward:
> 
> 1. Leave refs.c in its original location (as suggested by Junio).
>Optionally, it could be moved at a later date when this area is
>quiescent.
> [...]

I just tried this (it was less work than expected). It doesn't make the
merge conflict with lf/ref-is-hidden-namespace go away because the
functions in the new refs.c are ordered differently than in the old
refs.c. But it does turn it into a simple rerere-able conflict.

I'll check how much work it would be to make the order of functions in
the new refs.c match that in the old one...

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[Junio C Hamano]

Junio C Hamano  writes:

> Michael Haggerty  writes:
>
>> Junio, if there are no more comments, would you mind
>>
>> s/verify_no_descendants/find_descendant_ref/
>>
>> in the log message of this commit? And then, if you are also OK with the
>> new subdirectory introduced in this patch series, David and I seem to be
>> in agreement that it is ready to go. It would be great if this patch
>> series could be merged in a timely manner, as it will conflict with
>> nearly any other changes that people might want to undertake in the refs
>> code.
>
> Thanks for working well together.  Let me see what I can do today...

What I'll push out later today merges this to the tip of 'pu'.
The resolution is the same for 'jch' or 'next' (I checked).

I have to say that the merge of this topioc is not pretty.  A topic
that is already in flight has changed ref_is_hidden() in refs.c; you
move this block of code first to refs/refs-backend.c and then to
refs/refs.c, and the recursive merge ends up saying "The trunk side
changed this block of code in refs/refs-backend.c while the side
branch removed that block".

The resolution has to become an evil merge that brings in a new file
refs/refs.c from the tip of your topic to the trunk while replaying
that change in the lost block to that new file.  Because an
in-flight topic like this one needs to be merged over and over until
it gets merged to 'master' I'd prepare an evil merge-fix to be
squashed into the result of an auto-merge to help this process for
the interim maintainer, but this topic is placing more burden than
it otherwise would to the entire process.

Incidentally, that was why I originally asked you if you want to be
an interim maintainer for this cycle.  Whoever is doing a large code
movement with a large patch series must be the one who would know
the best how its interaction with other topics is best managed.

I wonder if refs.c -> refs/refs.c rename is a good idea.  I do agree
that refs/ subdirectory that collects the backend implementation
details is a very sensible thing to do, but if the public and
generic API were left in refs.c, this particular conflict might have
been less severe and easier to handle.  Whatever.

For those who are listening in from sideline, in case when they need
to deal with a similar situation "the code our side changed gets
moved to elsewhere by a side branch", here is what I did:

 * let "git merge --conflict=diff3" attempt and fail.

 * a conflicted file will have something like this:

 ours
... the code with changes made by our side (trunk) ...
 base
... the code before our side (trunk) made the above changes ...

 theirs

 * create two new files, OURS and BASE.  Save the part in that
   conflicted file between  and  to OURS, and between 
   and  to BASE.

 * look at "diff -u BASE OURS", find in the (failed) automerge
   result where the original went (a sample of it is at the end of
   this message), and apply that change manually.

The above is only to resolve this conflict *once*.

Automating future merges of this branch into a slightly updated
codebase needs help from rerere and also merge-fix/ machinery (this
is not in core-git proper, but the tool is in the 'todo' branch and
its use is described in howto/maintain-git.txt).

Essentially the procedure were:

 * "git checkout pu^0"

 * let "git merge --conflict=diff3" attempt and fail.

 * accept removal of the conflicted block in refs/files-backend.c in
   the editor, do "git rerere" to record it.  commit the result.

 * apply the above diff between BASE and OURS, commit the result.

 * git update-ref refs/merge-fix/dt/refs-backend-pre-vtable HEAD

With this, the Reintegrate script (on 'todo', checked out in "Meta/"
subdirectory) will be able to reproduce the evil merge, e.g.

 $ git checkout pu
 $ echo dt/refs-backend-pre-vtable | Meta/Reintegrate

would first let "git rerere" replay the removal of conflicted block
in refs/files-backend.c and then amend the result by squashing the
change in merge-fix/dt/refs-backend-pre-vtable.


--- V_BASE  2015-11-06 14:51:10.150197900 -0800
+++ V_OURS  2015-11-06 14:51:05.638059250 -0800
@@ -117,7 +117,7 @@
return 0;
 }
 
-int ref_is_hidden(const char *refname)
+int ref_is_hidden(const char *refname, const char *refname_full)
 {
int i;
 
@@ -125,6 +125,7 @@
return 0;
for (i = hide_refs->nr - 1; i >= 0; i--) {
const char *match = hide_refs->items[i].string;
+   const char *subject;
int neg = 0;
int len;
 
@@ -133,10 +134,18 @@
match++;
}
 
-   if (!starts_with(refname, match))
+   if (*match == '^') {
+   subject = refname_full;
+   match++;
+   } else {
+   subject = refname;
+   }
+
+   

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[Junio C Hamano]

Michael Haggerty  writes:

> Junio, if there are no more comments, would you mind
>
> s/verify_no_descendants/find_descendant_ref/
>
> in the log message of this commit? And then, if you are also OK with the
> new subdirectory introduced in this patch series, David and I seem to be
> in agreement that it is ready to go. It would be great if this patch
> series could be merged in a timely manner, as it will conflict with
> nearly any other changes that people might want to undertake in the refs
> code.

Thanks for working well together.  Let me see what I can do today...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[Michael Haggerty]

On 11/05/2015 05:22 PM, David Turner wrote:
> [...] But while looking at it, I
> noticed that the commit message doesn't look quite right (my fault):
> 
> On Thu, 2015-11-05 at 05:00 +0100, Michael Haggerty wrote:
>> Create new function verify_no_descendants, to hold one of the ref
>> conflict checks used in verify_refname_available. Multiple backends
>> will need this function, so move it to the common code.
> 
> The function is find_descendant_ref not verify_no_descendants.

Thanks for noticing.

Junio, if there are no more comments, would you mind

s/verify_no_descendants/find_descendant_ref/

in the log message of this commit? And then, if you are also OK with the
new subdirectory introduced in this patch series, David and I seem to be
in agreement that it is ready to go. It would be great if this patch
series could be merged in a timely manner, as it will conflict with
nearly any other changes that people might want to undertake in the refs
code.

Thanks,
Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[David Turner]

On Thu, 2015-11-05 at 05:00 +0100, Michael Haggerty wrote:
> On 11/04/2015 10:01 PM, David Turner wrote:
> > On Tue, 2015-11-03 at 08:40 +0100, Michael Haggerty wrote:
> >> + * extras and skip must be sorted lists of reference names. Either one
> >> + * can be NULL, signifying the empty list.
> >> + */
> > 
> > My version had:
> > 
> > "skip can be NULL; extras cannot."
> > 
> > The first thing that function does is:
> > string_list_find_insert_index(extras, dirname, 0)
> > 
> > And that crashes when extras is null.  So I think my version is correct
> > here.
> 
> We're talking about the function find_descendant_ref(), which was added
> in this patch, right? Because the first thing that function does is
> 
> + if (!extras)
> + return NULL;
> 
> (This guard was in your version, too.) Also, the callsite doesn't
> protect against extras==NULL. So either we're talking about two
> different things here, or I disagree with you.

You're right.  I totally missed that.  But while looking at it, I
noticed that the commit message doesn't look quite right (my fault):

> Create new function verify_no_descendants, to hold one of the ref
> conflict checks used in verify_refname_available. Multiple backends
> will need this function, so move it to the common code.

The function is find_descendant_ref not verify_no_descendants.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[David Turner]

On Tue, 2015-11-03 at 08:40 +0100, Michael Haggerty wrote:
> + * extras and skip must be sorted lists of reference names. Either one
> + * can be NULL, signifying the empty list.
> + */

My version had:

"skip can be NULL; extras cannot."

The first thing that function does is:
string_list_find_insert_index(extras, dirname, 0)

And that crashes when extras is null.  So I think my version is correct
here.

Other than that, I've reviewed both the patches themselves and the
overall diff and everything looks good to me.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 25/25] refs: break out ref conflict checks

[Michael Haggerty]

On 11/04/2015 10:01 PM, David Turner wrote:
> On Tue, 2015-11-03 at 08:40 +0100, Michael Haggerty wrote:
>> + * extras and skip must be sorted lists of reference names. Either one
>> + * can be NULL, signifying the empty list.
>> + */
> 
> My version had:
> 
> "skip can be NULL; extras cannot."
> 
> The first thing that function does is:
> string_list_find_insert_index(extras, dirname, 0)
> 
> And that crashes when extras is null.  So I think my version is correct
> here.

We're talking about the function find_descendant_ref(), which was added
in this patch, right? Because the first thing that function does is

+   if (!extras)
+   return NULL;

(This guard was in your version, too.) Also, the callsite doesn't
protect against extras==NULL. So either we're talking about two
different things here, or I disagree with you.

> Other than that, I've reviewed both the patches themselves and the
> overall diff and everything looks good to me.

Thanks!

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html