Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Wed, Oct 31, 2012 at 2:51 AM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >>It's not my job to >> explain to you that 'git fast-export' doesn't work this way, you have >> a command line to type those commands and see for yourself if they do >> what you think they do with a vanilla version of git. That's exactly >> what I did, to make sure I'm not using assumptions as basis for >> arguing, it took me a few minutes. > > Well no, when I run "git blame" 10 years down the line and do not > understand what your code is doing, it is not at all reasonable to > expect me to checkout the parent commit, get it to compile with a > modern toolchain, and type those commands for myself. > > Instead, the commit message should be self-contained and explain what > the patch does. > > That has multiple parts: > > - first, what the current behavior is > > - second, what the intent behind the current behavior is. This is >crucial information because presumably we want the change not to >break that. > > - third, what change the patch makes > > - fourth, what the consequences of that are, in terms of new use >cases that become possible and old use cases that become less >convenient > > - fifth, optionally, how the need for this change was discovered >(real-life usage, code inspection, or something else) > > - sixth, optionally, implementation considerations and alternate >approaches that were discarded I don't see any "Explain in detail what different commands do, even if they are irrelevant to the patch in question because someone might think they would get broken by this patch when in fact they wouldn't", that might belong in the discussion, but not in the commit message, and certainly not in the form of any entitlement. Again, it's _your_ responsibility to make sure the commands you say might get broken do actually work with your current git, it's not mine to run them for you, even though that's exactly what I did, because I'm interested in getting things correctly on record. And FTR, since you removed it, here is what I proposed to add to the commit message: --- The reason this happens is that before traversing the commits, fast-export checks if any of the refs point to the same object, and any duplicated ref gets added to a list in order to issue 'reset' commands after the traversing. Unfortunately, it's not even checking if the commit is flagged as UNINTERESTING. The fix of course, is to do precisely that. --- With that, all the points above are tackled, except fourth, because there aren't any. 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 v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: >It's not my job to > explain to you that 'git fast-export' doesn't work this way, you have > a command line to type those commands and see for yourself if they do > what you think they do with a vanilla version of git. That's exactly > what I did, to make sure I'm not using assumptions as basis for > arguing, it took me a few minutes. Well no, when I run "git blame" 10 years down the line and do not understand what your code is doing, it is not at all reasonable to expect me to checkout the parent commit, get it to compile with a modern toolchain, and type those commands for myself. Instead, the commit message should be self-contained and explain what the patch does. That has multiple parts: - first, what the current behavior is - second, what the intent behind the current behavior is. This is crucial information because presumably we want the change not to break that. - third, what change the patch makes - fourth, what the consequences of that are, in terms of new use cases that become possible and old use cases that become less convenient - fifth, optionally, how the need for this change was discovered (real-life usage, code inspection, or something else) - sixth, optionally, implementation considerations and alternate approaches that were discarded If you run "git log", you'll see many good and bad examples to think over and compare to this goal. It's hard work to describe one's work well in terms that other people can understand, but I think it can be satisfying, and in any event, it's just as necessary as including comments near confusing code. Sincerely, 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 v2 3/4] fast-export: don't handle uninteresting refs
On Wed, Oct 31, 2012 at 2:08 AM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> I don't think it's my job to explain to you how 'git fast-export' >> works. > > Actually, if you are submitting a patch for inclusion, it is your job > to explain to future readers what the patch does. That's already explained. > Yes, the reader > might not be deeply familiar with the part of fast-export you are > modifying. This has nothing to do with what you said. I'm literally explaining to you how 'git fast-export' works in situations that are completely orthogonal to this patch, because you are using wrong examples as grounds to prevent this patch from being accepted. It's not my job to explain to you that 'git fast-export' doesn't work this way, you have a command line to type those commands and see for yourself if they do what you think they do with a vanilla version of git. That's exactly what I did, to make sure I'm not using assumptions as basis for arguing, it took me a few minutes. That being said, if your problem is that it's not clear to people not deeply familiar with that part of fast-export, this extra paragraph in addition to the current commit message should do the trick: --- The reason this happens is that before traversing the commits, fast-export checks if any of the refs point to the same object, and any duplicated ref gets added to a list in order to issue 'reset' commands after the traversing. Unfortunately, it's not even checking if the commit is flagged as UNINTERESTING. The fix of course, is to do precisely that. --- And to get that all had to do is ask: "Can you please add an explanation of what this part of the code does? For the ones of us not familiar with it". Not; "This patch looks unsafe", "This patch makes Sally mad", "This patch causes regressions", and so on. But hey, at least we are not arguing about what is wrong with this patch (or so I hope). 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 v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > This might help you, or other people involved in the problem, but not > anybody else. Ok, I give up. Bye. Sometimes the author of some code and the right person to interact with the development community by submitting and maintaining it are not the same person. Hopefully others more patient than we two can pick up where we left off. 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 v2 3/4] fast-export: don't handle uninteresting refs
Hi,
On Wed, Oct 31, 2012 at 1:57 AM, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> So, the above description conveyed zero information, as you mentioned.
I meant, this, of course:
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> This patch looks unsafe,
Which you know, because you received that message without the mistake.
> A clearer explanation would be the following:
>
> fast-export: don't emit "reset" command for negative refs
What is a negative ref?
> When "git fast-export" encounters two refs on the commandline
commandline?
Only two refs? How about four?
> referring to the same commit, it exports the first during the usual
> commit walk and the second using a "reset" command in a final pass
> over extra_refs:
That is not exactly true: (next^{commit}).
> $ git fast-export master next
> reset refs/heads/master
> commit refs/heads/master
> mark :1
> author Jonathan Nieder 1351644412 -0700
> committer Jonathan Nieder 1351644412
> -0700
> data 17
> My first commit!
>
> reset refs/heads/next
> from :1
I don't think this example is good. Where does it say that 'next'
points to master? Using 'points-to-master' or a 'git branch stable
master' and using 'master stable'.
Even simpler would be to use 'git fast-export master master'; it would
show the same behavior.
> Unfortunately the code to do this doesn't distinguish between positive
> and negative refs, producing confusing results:
>
> $ git fast-export ^master next
> reset refs/heads/next
> from :0
>
> $ git fast-export master ^next
> reset refs/heads/next
> from :0
>
> Use revs->cmdline instead of revs->pending to iterate over the
> rev-list
> arguments, checking the UNINTERESTING flag bit to distinguish between
> positive (master, --all, etc) and negative (next.., --not --all, etc)
> revs and avoid enqueueing negative revs in extra_revs.
Use what? You mean, "To solve the problem, lets use".
But this is not correct, cmdline is not being used. Have you even
looked at the patch?
> This does not affect revs that were excluded from the revision walk
> because pointed to by a mark, since those use the SHOWN bit on the
> commit object itself and not UNINTERESTING on the rev_cmdline_entry.
revs? You mean commits?
"excluded because point to by a mark"? Doesn't sound like proper
grammar. Maybe "excluded because they were pointed to by a mark".
And I don't see why this paragraph is needed at all. Why would the
reader think marks have anything to do with this? There's no mention
of marks before.
This might help you, or other people involved in the problem, but not
anybody else. Anything related to marks is completely orthogonal to
this patch, and there's no point in mentioning that.
--
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 v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > I don't think it's my job to explain to you how 'git fast-export' > works. Actually, if you are submitting a patch for inclusion, it is your job to explain to future readers what the patch does. Yes, the reader might not be deeply familiar with the part of fast-export you are modifying. It might have even been modified since then, by the time the reader is looking at the change! Sad but true. 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 v2 3/4] fast-export: don't handle uninteresting refs
On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote:
>
>>> Nope. I just don't want regressions, and found a patch description
>>> that did nothing to explain to the reader how it avoids regressions
>>> more than a little disturbing.
>>
>> I see, so you don't have any specific case where this could cause
>> regressions, you are just saying it _might_ (like all patches).
>
> Yes, exactly. The commit log needs a description of the current
> behavior, the intent behind the current code, the change the patch
> makes, and the motivation behind that change, like all patches.
> Despite the nice examples, it doesn't currently have that.
>
> The patch description just raises more questions for the reader. From
> the description, one might imagine that this patch causes
>
> git fast-export master
>
> not to emit anything when another branch that has already been
> exported is ahead of "master".
This is already the case.
I don't see what part of my patch description would give you the idea
that this would change in any way how the objects are flagged, or how
get_revision() decides how to traverse them.
I clearly stated that this doesn't affect *the first* ref, which is
handled properly already; this patch affects *the rest* of the refs,
of which you have none in that command above.
> If I understand correctly (though
> I haven't tested), this patch does cause
>
> git fast-export ^next master
>
> not to emit anything when next is ahead of "master". That doesn't
> seem like progress.
Again, this is already the case RIGHT NOW.
And nothing in my description should give you an idea that anything
would change for this case because the 2nd ref (*the first* doesn't
get affected), is not marked as UNINTERESTING.
Not only you are not reading what is in the description, but I don't
think you understand what the code actually does, and how it behaves.
Let me give you some examples:
% git fast-export ^next next
reset refs/heads/next
from :0
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea
The *only time* when this patch would have any effect is when you
specify more than *one ref*, and they both point to *exactly the same
object*.
Additionally, and this is something I just found out; when the are
pure refs (e.g. 'next'), and not refs to objects (e.g.
'next^{commit}').
In any other case; *there would be no change*.
After my patch:
% git fast-export ^next next
# nothing
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea
> But in the long term it is much easier to understand
> and maintain a patch series that does not introduce regressions in the
> first place
It does not introduce regressions.
I don't think it's my job to explain to you how 'git fast-export'
works. Above you made too many assumptions of what get broken, when in
fact that's the current behavior already... maybe, just maybe, you are
also making wrong assumptions about this patch as well.
--
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 v2 3/4] fast-export: don't handle uninteresting refs
Hi again, Felipe Contreras wrote: > They have been marked as UNINTERESTING for a reason, lets respect that. So, the above description conveyed zero information, as you mentioned. A clearer explanation would be the following: fast-export: don't emit "reset" command for negative refs When "git fast-export" encounters two refs on the commandline referring to the same commit, it exports the first during the usual commit walk and the second using a "reset" command in a final pass over extra_refs: $ git fast-export master next reset refs/heads/master commit refs/heads/master mark :1 author Jonathan Nieder 1351644412 -0700 committer Jonathan Nieder 1351644412 -0700 data 17 My first commit! reset refs/heads/next from :1 Unfortunately the code to do this doesn't distinguish between positive and negative refs, producing confusing results: $ git fast-export ^master next reset refs/heads/next from :0 $ git fast-export master ^next reset refs/heads/next from :0 Use revs->cmdline instead of revs->pending to iterate over the rev-list arguments, checking the UNINTERESTING flag bit to distinguish between positive (master, --all, etc) and negative (next.., --not --all, etc) revs and avoid enqueueing negative revs in extra_revs. This does not affect revs that were excluded from the revision walk because pointed to by a mark, since those use the SHOWN bit on the commit object itself and not UNINTERESTING on the rev_cmdline_entry. A patch meeting the above description would make perfect sense to me. Except for the somewhat strange testcase, the patch I am replying to would also be fine in the short term, as long as it had an analagous description (i.e., with an appropriate replacement for the second-to-last paragraph). Thanks for your patience, and hoping that helps, 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 v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote: >> Nope. I just don't want regressions, and found a patch description >> that did nothing to explain to the reader how it avoids regressions >> more than a little disturbing. > > I see, so you don't have any specific case where this could cause > regressions, you are just saying it _might_ (like all patches). Yes, exactly. The commit log needs a description of the current behavior, the intent behind the current code, the change the patch makes, and the motivation behind that change, like all patches. Despite the nice examples, it doesn't currently have that. The patch description just raises more questions for the reader. From the description, one might imagine that this patch causes git fast-export master not to emit anything when another branch that has already been exported is ahead of "master". If I understand correctly (though I haven't tested), this patch does cause git fast-export ^next master not to emit anything when next is ahead of "master". That doesn't seem like progress. I haven't reviewed the later patches in the series; maybe they fix these things. But in the long term it is much easier to understand and maintain a patch series that does not introduce regressions in the first place, and the context one might use to convincingly explain that a patch is not introducing a regression turns out to be essential for many other purposes as well. 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 v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> Well, that's what we have now, and you want to preserve this "feature" >> (aka bug), right? > > Nope. I just don't want regressions, and found a patch description > that did nothing to explain to the reader how it avoids regressions > more than a little disturbing. I see, so you don't have any specific case where this could cause regressions, you are just saying it _might_ (like all patches). > I also think the proposed behavior is wrong. That's a different matter, lets see what others think. -- 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 v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > Well, that's what we have now, and you want to preserve this "feature" > (aka bug), right? Nope. I just don't want regressions, and found a patch description that did nothing to explain to the reader how it avoids regressions more than a little disturbing. I also think the proposed behavior is wrong. I don't think there's much benefit to be gained from continuing to discuss this. Consider it a single data point: I would be deeply worried if this patch were applied without at least a clearer description of the change in behavior. Maybe I'm the only one! Hope that helps, 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 v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 10:45 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> So you think what we have now is the correct behavior: >> >> % git fast-export master ^master >> reset refs/heads/master >> from :0 > > No, I don't think that, either. Well, that's what we have now, and you want to preserve this "feature" (aka bug), right? And I still haven't why this is "unsafe", and what are those "examples not listed". -- 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 v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > So you think what we have now is the correct behavior: > > % git fast-export master ^master > reset refs/heads/master > from :0 No, I don't think that, either. Hope that helps, 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 v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: >> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder wrote: > >>> and in the examples listed in the patch >>> description the changed behavior does not look like an improvement. >> >> I disagree. >> >> % git log master ^master >> >> What do you expect? Nothing. > > Yep. > >> % git fast-export master ^master >> >> What do you expect? Nothing. So you think what we have now is the correct behavior: % git fast-export master ^master reset refs/heads/master from :0 That of course would crash fast-import. But hey, it's your opinion. Would be interesting to see if other people think the above is correct. 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 v2 3/4] fast-export: don't handle uninteresting refs
Hi Felipe, On Tue, 30 Oct 2012, Felipe Contreras wrote: > On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder wrote: > > Felipe Contreras wrote: > >> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder > >> wrote: > > > >>> and in the examples listed in the patch > >>> description the changed behavior does not look like an improvement. > >> > >> I disagree. > >> > >> % git log master ^master > >> > >> What do you expect? Nothing. > > > > Yep. > > > >> % git fast-export master ^master > >> > >> What do you expect? Nothing. > > > > Nope. > > That's _your_ opinion. I would like to see what others think. If you wanted to prove that you can work with others without offending them, I think that failed. Ciao, Johannes -- 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 v2 3/4] fast-export: don't handle uninteresting refs
(again to the mailing list) On Tue, Oct 30, 2012 at 7:59 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> They have been marked as UNINTERESTING for a reason, lets respect that. That doesn't say anything. > and in the examples listed in the patch > description the changed behavior does not look like an improvement. I disagree. % git log master ^master What do you expect? Nothing. % git fast-export master ^master What do you expect? Nothing. > Worse, the description lists a few examples but gives no convincing > explanation to reassure about the lack of bad behavior for examples > not listed. What examples not listed? -- 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 v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote:
> They have been marked as UNINTERESTING for a reason, lets respect that.
This patch looks unsafe, and in the examples listed in the patch
description the changed behavior does not look like an improvement.
Worse, the description lists a few examples but gives no convincing
explanation to reassure about the lack of bad behavior for examples
not listed.
Perhaps this patch has a prerequisite and has come out of order.
Hope that helps,
Jonathan
Patch left unsnipped so we can get a copy in the list archive.
> Currently the first ref is handled properly, but not the rest, so:
>
> % git fast-export master ^master
>
> Would currently throw a reset for master (2nd ref), which is not what we
> want.
>
> % git fast-export master ^foo ^bar ^roo
> % git fast-export master salsa..tacos
>
> Even if all these refs point to the same object; foo, bar, roo, salsa,
> and tacos would all get a reset.
>
> This is most certainly not what we want. After this patch, nothing gets
> exported, because nothing was selected (everything is UNINTERESTING).
>
> Signed-off-by: Felipe Contreras
> ---
> builtin/fast-export.c | 7 ---
> t/t9350-fast-export.sh | 6 ++
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 065f324..7fb6fe1 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -523,10 +523,11 @@ static void get_tags_and_duplicates(struct object_array
> *pending,
> typename(e->item->type));
> continue;
> }
> - if (commit->util)
> + if (commit->util) {
> /* more than one name for the same object */
> - string_list_append(extra_refs, full_name)->util =
> commit;
> - else
> + if (!(commit->object.flags & UNINTERESTING))
> + string_list_append(extra_refs, full_name)->util
> = commit;
> + } else
> commit->util = full_name;
> }
> }
> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index 49bdb44..6ea8f6f 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -440,4 +440,10 @@ test_expect_success 'fast-export quotes pathnames' '
> )
> '
>
> +test_expect_success 'proper extra refs handling' '
> + git fast-export master ^master master..master > actual &&
> + echo -n > expected &&
> + 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
