Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
On Fri, Nov 2, 2012 at 4:19 PM, Jeff King wrote: > On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote: >> May I just ask to include a summary of that rationale into the commit >> message rather than relying on people having internet access and knowing >> where to look? Adding the following to the commit message would be good >> enough for me: >> >> Note that >> >> $ git branch foo master~1 >> $ git fast-export foo master~1..master >> >> still does not update the "foo" ref, but a partial fix is better >> than no fix. > > Yes, I think that makes a lot of sense. > > Felipe, I notice that you sent out a big "fast-export improvements" > series. Does that supersede this? Yes. I noticed this patch fixes other tests. Cheers. -- Felipe Contreras -- 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 v3 4/4] fast-export: make sure refs are updated properly
On Fri, Nov 2, 2012 at 2:12 PM, Jeff King wrote: > On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote: > >> If the commit does not have the SHOWN or UNINTERESTING flag set but it >> is going to get the UNINTERESTING flag set during the walk because of >> a negative commit listed on the command line, this patch won't help. > > Right, so my understanding of the situation is that doing this: > > $ git branch foo master~1 > $ git fast-export foo master~1..master > > won't show "foo", which seems wrong to me. _But_ we currently get that > wrong already, so Felipe's patches are not making anything worse, but > are fixing some situations (namely when master~1 is not mentioned on the > command-line, but rather in a marks file). > > Is that correct? Yes, that's correct. But my patch ("make sure refs are updated properly") does _not_ change in any shape or form what happens with what you specify in the command line, only what happens with marks. Cheers. -- Felipe Contreras -- 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 v3 4/4] fast-export: make sure refs are updated properly
On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote: > > If so, then this series isn't regressing behavior; the only downside is > > that it's an incomplete fix. In theory this could get in the way of the > > full fix later on, but given the commit messages and the archive of this > > discussion, it would be simple enough to revert it later in favor of a > > more full fix. Is that accurate? > > From my understanding, yes. > > > Sorry if I am belaboring the discussion. I just want to make sure I > > understand the situation before deciding what to do with the topic. It > > sounds like the consensus at this point is "not perfect, but good enough > > to make forward progress". > > I appreciate that stance very much. The patch Sverre and I proposed was > also an incomplete fix (although I suspect it would fix the issue you > pointed out above), so I agree with the "perfect is the enemy of the good" > approach, obviously. Thanks for the response. > May I just ask to include a summary of that rationale into the commit > message rather than relying on people having internet access and knowing > where to look? Adding the following to the commit message would be good > enough for me: > > Note that > > $ git branch foo master~1 > $ git fast-export foo master~1..master > > still does not update the "foo" ref, but a partial fix is better > than no fix. Yes, I think that makes a lot of sense. Felipe, I notice that you sent out a big "fast-export improvements" series. Does that supersede this? -Peff -- 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 v3 4/4] fast-export: make sure refs are updated properly
Hi Peff, On Fri, 2 Nov 2012, Jeff King wrote: > On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote: > > > If the commit does not have the SHOWN or UNINTERESTING flag set but it > > is going to get the UNINTERESTING flag set during the walk because of > > a negative commit listed on the command line, this patch won't help. > > Right, so my understanding of the situation is that doing this: > > $ git branch foo master~1 > $ git fast-export foo master~1..master > > won't show "foo", which seems wrong to me. _But_ we currently get that > wrong already, so Felipe's patches are not making anything worse, but > are fixing some situations (namely when master~1 is not mentioned on the > command-line, but rather in a marks file). > > Is that correct? > > If so, then this series isn't regressing behavior; the only downside is > that it's an incomplete fix. In theory this could get in the way of the > full fix later on, but given the commit messages and the archive of this > discussion, it would be simple enough to revert it later in favor of a > more full fix. Is that accurate? >From my understanding, yes. > Sorry if I am belaboring the discussion. I just want to make sure I > understand the situation before deciding what to do with the topic. It > sounds like the consensus at this point is "not perfect, but good enough > to make forward progress". I appreciate that stance very much. The patch Sverre and I proposed was also an incomplete fix (although I suspect it would fix the issue you pointed out above), so I agree with the "perfect is the enemy of the good" approach, obviously. May I just ask to include a summary of that rationale into the commit message rather than relying on people having internet access and knowing where to look? Adding the following to the commit message would be good enough for me: Note that $ git branch foo master~1 $ git fast-export foo master~1..master still does not update the "foo" ref, but a partial fix is better than no fix. Thanks, Dscho -- 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 v3 4/4] fast-export: make sure refs are updated properly
Jeff King wrote: > If so, then this series isn't regressing behavior; the only downside is > that it's an incomplete fix. In theory this could get in the way of the > full fix later on, but given the commit messages and the archive of this > discussion, it would be simple enough to revert it later in favor of a > more full fix. Is that accurate? > > Sorry if I am belaboring the discussion. I just want to make sure I > understand the situation before deciding what to do with the topic. It > sounds like the consensus at this point is "not perfect, but good enough > to make forward progress". Patch 1, 2, and 4 are good modulo their descriptions. They should work fine without patch 3. Patch 3 is a regression in comprehensibility. I think we can do better. Maybe all it would take is a less confusing description, and tweaks to the code (to loop over revs->cmdline instead of revs->pending) could come on top. -- 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 v3 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote: > If the commit does not have the SHOWN or UNINTERESTING flag set but it > is going to get the UNINTERESTING flag set during the walk because of > a negative commit listed on the command line, this patch won't help. Right, so my understanding of the situation is that doing this: $ git branch foo master~1 $ git fast-export foo master~1..master won't show "foo", which seems wrong to me. _But_ we currently get that wrong already, so Felipe's patches are not making anything worse, but are fixing some situations (namely when master~1 is not mentioned on the command-line, but rather in a marks file). Is that correct? If so, then this series isn't regressing behavior; the only downside is that it's an incomplete fix. In theory this could get in the way of the full fix later on, but given the commit messages and the archive of this discussion, it would be simple enough to revert it later in favor of a more full fix. Is that accurate? Sorry if I am belaboring the discussion. I just want to make sure I understand the situation before deciding what to do with the topic. It sounds like the consensus at this point is "not perfect, but good enough to make forward progress". -Peff -- 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 v3 4/4] fast-export: make sure refs are updated properly
Sverre Rabbelier wrote: > Thanks for the thorough explanation. Perhaps some of that could make > it's way into the commit message? It's fine with me if it doesn't, since the original commit message covers the basics (current behavior and intent of the change) in its first two paragraphs and anyone wanting more detail can use GIT_NOTES_REF=refs/remotes/charon/notes/full \ git show --show-notes to find more details. Thanks, Jonathan -- 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 v3 4/4] fast-export: make sure refs are updated properly
On Wed, Oct 31, 2012 at 1:37 AM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> --- a/builtin/fast-export.c >> +++ b/builtin/fast-export.c >> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct >> object_array *pending, >> typename(e->item->type)); >> continue; >> } >> - if (commit->util) { >> - /* more than one name for the same object */ >> + >> + /* >> + * This ref will not be updated through a commit, lets make >> + * sure it gets properly upddated eventually. >> + */ >> + if (commit->util || commit->object.flags & SHOWN) { >> if (!(commit->object.flags & UNINTERESTING)) >> string_list_append(extra_refs, >> full_name)->util = commit; >> - } else >> + } >> + if (!commit->util) >> commit->util = full_name; > > Here's an explanation of why the above makes sense to me. > > get_tags_and_duplicates() gets called after the marks import and > before the revision walk. It walks through the revs from the > commandline and for each one: > > - peels it to a refname, and then to a commit > - stores the refname so fast-export knows what arg to pass to >the "commit" command during the revision walk > - if it already had a refname stored, instead adds the >(refname, commit) pair to the extra_refs list, so fast-export >knows to add a "reset" command later. > > If the commit already has the SHOWN flag set because it was pointed to > by a mark, it is not going to come up in the revision walk, so it will > not be mentioned in the output stream unless it is added to > extra_refs. That's what this patch does. That is correct. > Incidentally, the change from "else" to "if (!commit->util)" is > unnecessary because if a commit is already SHOWN then it will not be > encountered in the revision walk so commit->util does not need to be > set. Maybe, but that's yet another change, and with more changes come more possibilities of regressions. I haven't verified this is the case. If this makes sense, I would do it in another, separate patch. > If the commit does not have the SHOWN or UNINTERESTING flag set but it > is going to get the UNINTERESTING flag set during the walk because of > a negative commit listed on the command line, this patch won't help. I don't know what that means in practice. Cheers. -- Felipe Contreras -- 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 v3 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> --- a/builtin/fast-export.c >> +++ b/builtin/fast-export.c >> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct >> object_array *pending, >> typename(e->item->type)); >> continue; >> } >> - if (commit->util) { >> - /* more than one name for the same object */ >> + >> + /* >> + * This ref will not be updated through a commit, lets make >> + * sure it gets properly upddated eventually. >> + */ >> + if (commit->util || commit->object.flags & SHOWN) { >> if (!(commit->object.flags & UNINTERESTING)) >> string_list_append(extra_refs, >> full_name)->util = commit; >> - } else >> + } >> + if (!commit->util) >> commit->util = full_name; > > Here's an explanation of why the above makes sense to me. > > get_tags_and_duplicates() gets called after the marks import and > before the revision walk. It walks through the revs from the > commandline and for each one: > > - peels it to a refname, and then to a commit > - stores the refname so fast-export knows what arg to pass to >the "commit" command during the revision walk > - if it already had a refname stored, instead adds the >(refname, commit) pair to the extra_refs list, so fast-export >knows to add a "reset" command later. > > If the commit already has the SHOWN flag set because it was pointed to > by a mark, it is not going to come up in the revision walk, so it will > not be mentioned in the output stream unless it is added to > extra_refs. That's what this patch does. > > Incidentally, the change from "else" to "if (!commit->util)" is > unnecessary because if a commit is already SHOWN then it will not be > encountered in the revision walk so commit->util does not need to be > set. > > If the commit does not have the SHOWN or UNINTERESTING flag set but it > is going to get the UNINTERESTING flag set during the walk because of > a negative commit listed on the command line, this patch won't help. Thanks for the thorough explanation. Perhaps some of that could make it's way into the commit message? -- Cheers, Sverre Rabbelier -- 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 v3 4/4] fast-export: make sure refs are updated properly
Felipe Contreras wrote: > --- a/builtin/fast-export.c > +++ b/builtin/fast-export.c > @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array > *pending, > typename(e->item->type)); > continue; > } > - if (commit->util) { > - /* more than one name for the same object */ > + > + /* > + * This ref will not be updated through a commit, lets make > + * sure it gets properly upddated eventually. > + */ > + if (commit->util || commit->object.flags & SHOWN) { > if (!(commit->object.flags & UNINTERESTING)) > string_list_append(extra_refs, full_name)->util > = commit; > - } else > + } > + if (!commit->util) > commit->util = full_name; Here's an explanation of why the above makes sense to me. get_tags_and_duplicates() gets called after the marks import and before the revision walk. It walks through the revs from the commandline and for each one: - peels it to a refname, and then to a commit - stores the refname so fast-export knows what arg to pass to the "commit" command during the revision walk - if it already had a refname stored, instead adds the (refname, commit) pair to the extra_refs list, so fast-export knows to add a "reset" command later. If the commit already has the SHOWN flag set because it was pointed to by a mark, it is not going to come up in the revision walk, so it will not be mentioned in the output stream unless it is added to extra_refs. That's what this patch does. Incidentally, the change from "else" to "if (!commit->util)" is unnecessary because if a commit is already SHOWN then it will not be encountered in the revision walk so commit->util does not need to be set. If the commit does not have the SHOWN or UNINTERESTING flag set but it is going to get the UNINTERESTING flag set during the walk because of a negative commit listed on the command line, this patch won't help. Jonathan -- 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
[PATCH v3 4/4] fast-export: make sure refs are updated properly
When an object has already been exported (and thus is in the marks) it is flagged as SHOWN, so it will not be exported again, even if this time it's exported through a different ref. We don't need the object to be exported again, but we want the ref updated, which doesn't happen. Since we can't know if a ref was exported or not, let's just assume that if the commit was marked (flags & SHOWN), the user still wants the ref updated. So: % git branch test master % git fast-export $mark_flags master % git fast-export $mark_flags test Would export 'test' properly. Additionally, this fixes issues with remote helpers; now they can push refs wich objects have already been exported. Signed-off-by: Felipe Contreras --- builtin/fast-export.c | 11 --- t/t5800-remote-helpers.sh | 11 +++ t/t9350-fast-export.sh| 14 ++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 7fb6fe1..663a93d 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending, typename(e->item->type)); continue; } - if (commit->util) { - /* more than one name for the same object */ + + /* +* This ref will not be updated through a commit, lets make +* sure it gets properly upddated eventually. +*/ + if (commit->util || commit->object.flags & SHOWN) { if (!(commit->object.flags & UNINTERESTING)) string_list_append(extra_refs, full_name)->util = commit; - } else + } + if (!commit->util) commit->util = full_name; } } diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh index e7dc668..69a145a 100755 --- a/t/t5800-remote-helpers.sh +++ b/t/t5800-remote-helpers.sh @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' ' compare_refs clone HEAD server refs/heads/new-refspec ' +test_expect_success 'push ref with existing object' ' + (cd localclone && + git branch point-to-master master && + git push origin point-to-master + ) && + + (cd server && + git show-ref refs/heads/point-to-master + ) +' + test_done diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 6ea8f6f..a4178e3 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -446,4 +446,18 @@ test_expect_success 'proper extra refs handling' ' test_cmp expected actual ' +cat > expected << EOF +reset refs/heads/master +from :13 + +EOF + +test_expect_success 'refs are updated even if no commits need to be exported' ' + git fast-export --import-marks=tmp-marks \ + --export-marks=tmp-marks master > /dev/null && + git fast-export --import-marks=tmp-marks \ + --export-marks=tmp-marks master > actual && + test_cmp expected actual +' + test_done -- 1.8.0 -- 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