v3, 4 partsfast-export: general fixes

> 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

> --- 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
> +	)

> (cc-ing the git list)
> Felipe Contreras wrote:
>
>> 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
>
> Yes, makes perfect sense.
>
> For what it's worth,
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>

> [...]
>> --- 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
>> +     )
>
> Style: if you indent like this, the test becomes clearer:

>> +     git fast-export --import-marks=tmp-marks \
>> +             --export-marks=tmp-marks master > actual &&
>> +     test_cmp expected actual
>
> Redirections in git shell scripts are generally spelled as
> "do_something >actual", without a space between the operator and
> filename.

> --- 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;

> 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.

> 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.

> Dropping the Cc list, as this is off topic
>
> On Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:
>> 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.
>
> I seem to miss something here, but I don't get it how the notes ref
> becomes magically filled with the details of this discussion.
>
> Care to explain?

> > 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.
> 
> I seem to miss something here, but I don't get it how the notes ref
> becomes magically filled with the details of this discussion.

> On Wed, Oct 31, 2012 at 10:53:27AM +0100, Peter Baumann wrote:
> 
> > > 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.
> > 
> > I seem to miss something here, but I don't get it how the notes ref
> > becomes magically filled with the details of this discussion.
> 
> Thomas Rast (aka charon) keeps a mapping of commits to the email threads
> that led to them. You can fetch it from:
> 
>    git://repo.or.cz/git/trast.git
> 
> (try the notes/full and notes/terse refs).
> 

> 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.

> 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".

> 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?

> > 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.

>> 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?

> 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?