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
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
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
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
On Fri, Nov 2, 2012 at 2:12 PM, Jeff King p...@peff.net 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 2, 2012 at 4:19 PM, Jeff King p...@peff.net 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
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 commit 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
[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 felipe.contre...@gmail.com
---
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
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
Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder jrnie...@gmail.com 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
On Wed, Oct 31, 2012 at 1:37 AM, Jonathan Nieder jrnie...@gmail.com 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
