[PATCH] t2017: redo physical reflog existance check

[PATCH] t2017: redo physical reflog existance check

[Erick Mattos]

Commit 6e6842e wiped out physical reflog checks of tests because of
redundancy and to hide low level detail of the implementation.

Although this is not a problem to all the other changes, it is laming to
the tenth test.  The implementation of the correspondent problem creates
a "touch" reflog that must be wiped out if not used by committing the
new branch.

Signed-off-by: Erick Mattos <erick.mattos@gmail.com>
---
 t/t2017-checkout-orphan.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
index 2d2f63f..0d5d0a0 100755
--- a/t/t2017-checkout-orphan.sh
+++ b/t/t2017-checkout-orphan.sh
@@ -93,8 +93,10 @@ test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates =
 test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '
 	git checkout master &&
 	git checkout -l --orphan eta &&
+	test -f .git/logs/refs/heads/eta &&
 	test_must_fail git rev-parse --verify eta@{0} &&
 	git checkout master &&
+	! test -f .git/logs/refs/heads/eta &&
 	test_must_fail git rev-parse --verify eta@{0}
 '
 
-- 
1.7.2.1.ga86e3

Re: [PATCH] t2017: redo physical reflog existance check

[Junio C Hamano]

Erick Mattos <erick.mattos@gmail.com> writes:

> Although this is not a problem to all the other changes, it is laming to
> the tenth test.  The implementation of the correspondent problem creates
> a "touch" reflog that must be wiped out if not used by committing the
> new branch.

I thought about it a bit when I sent out my patch, but I do not think that
is necessary.

The things you care about, after running "-l --orphan eta", are:

 - If you make a commit, you get eta@{...} reflog that records it; and

 - If you leave the still-to-be-born eta branch without making a commit,
   you do not leave eta@{...} reflog behind.

Your zeta@{...} test is about the former, and your eta@{...} test is about
the latter.  I think they already check what they want to see happen.

I also am afraid that the "test -f" check would expose the implementation
detail more than necessary.  We may want to come up with a different
implementation of this behaviour later that may not create an empty file
there.

Re: [PATCH] t2017: redo physical reflog existance check

[Erick Mattos]

Hi,

2010/7/22 Junio C Hamano <gitster@pobox.com>
> I thought about it a bit when I sent out my patch, but I do not think that
> is necessary.
>
> The things you care about, after running "-l --orphan eta", are:
>
>  - If you make a commit, you get eta@{...} reflog that records it; and
>
>  - If you leave the still-to-be-born eta branch without making a commit,
>   you do not leave eta@{...} reflog behind.
>
> Your zeta@{...} test is about the former, and your eta@{...} test is about
> the latter.  I think they already check what they want to see happen.

You have separate my concerns in the scripts very well but the result
you pointed out is not quite true.

For zeta, not testing physical existence of reflog file is not really
important because at the end you will have the reflog created anyway,
which is well tested by latter "git rev-parse --verify".  But that is
not the case of eta therefore the check is necessary.

The solution to the problem of creating reflogs when using -l and
--orphan while configured core.logAllRefUpdates=false was a simple
trick, completely based on actual implementation of reflog saving:
reflogs are saved when there is already a reflog file.

To make the new orphan branch ready to have a reflog on that config
was as simple as creating a "touch file" for reflog.  This is the goal
achieved by code.  Testing this goal by checking the touch reflog file
is important while in zeta case redundant, because of the final result
of having a reflog saved and consequently the touch file filled with
real reflog data.

But this clean solution should not leave a bogus file in case it is
not being used by canceling the creation of the new orphan branch and
doing a checkout to another branch.

So in eta case it is important to check if the reflog physical file is
really deleted.  No reflog will be created anyway in eta case so "git
ref-parse --verify" is not even relevant.  I have added formal reflog
check just in case.

Testing more is always better than testing less so I prefered to be
redundant and test thoroughly in all levels of detail.

> I also am afraid that the "test -f" check would expose the implementation
> detail more than necessary.  We may want to come up with a different
> implementation of this behaviour later that may not create an empty file
> there.

No exposure is being done by using "test -f" inside a script which its
sole purpose is to check a controlled event for developers.  Folder t
has "test -f" being employed in 92 scripts.

The only reason for the t folder is to let the developers be aware of
what they are possibly breaking, isn't it?!

I think this implementation is quite good.  I don't see a reason for
changing it.

The point is that you give a command (-l --orphan on
core.logAllRefUpdates=false config) that have to save the preference
of creating the reflog for later.  Data need to be saved once git is a
simple call-run-quit software.  And the way it is being saved is, at
minimum, very efficient.  No outside file or special config is being
created and no memory persistent variable/objects is being left
behind.  The implementation is working as expected and the subject is
only the testing script.

We have to remember that all we are talking here is about a very
uncommon situation when core.logAllRefUpdates is set to false.  I
personally don't even foresee a possible reason for not having reflogs
saved automatically anyway. :-|

Regards

Re: [PATCH] t2017: redo physical reflog existance check

[Junio C Hamano]

Erick Mattos <erick.mattos@gmail.com> writes:

> To make the new orphan branch ready to have a reflog on that config
> was as simple as creating a "touch file" for reflog.  This is the goal
> achieved by code.

I have to say that you are somewhat confused about the _goal_ then.  touch
is not a goal, it is means to a goal.

It does not matter how you implement the user visible effect, be it a
creation of an empty file, or some other means [*1*].  What matters is
that the user won't get a reflog for a branch that really didn't get
created and must-fail "rev-parse --verify" test checks that.

Another thing that could matter would be that future actions that want to
create a reflog for the same branch (perhaps after the user switches to
'master', another attempt is made to create eta with "checkout -b eta") or
another branch with a similar or related name (say "eta/real") are not get
broken by whatever you do to implement the "we want to create a reflog
when a ref is actually made but not right now" feature.  Perhaps the right
way to test that would be to actually try to run such operations and make
sure they do not fail.


[Footnote]

*1* For example, you could have implemented the feature by adding a config
item in ".git/config: [branch "eta"] need-to-create-reflog", and taught
refs.c::update_ref() to pay attention to it (I am not saying that it would
be a better implementation).

Re: [PATCH] t2017: redo physical reflog existance check

[Erick Mattos]

2010/7/22 Junio C Hamano <gitster@pobox.com>:
> Erick Mattos <erick.mattos@gmail.com> writes:
>
>> To make the new orphan branch ready to have a reflog on that config
>> was as simple as creating a "touch file" for reflog.  This is the goal
>> achieved by code.
>
> I have to say that you are somewhat confused about the _goal_ then.  touch
> is not a goal, it is means to a goal.

I believe you have understood the whole idea of my previous email even
though _words_ means a lot to you.  :-1

I meant that that was the code goal.  It was the implementation
objective which by itself was the solution chosen for the real
problem.

> It does not matter how you implement the user visible effect, be it a
> creation of an empty file, or some other means [*1*].  What matters is
> that the user won't get a reflog for a branch that really didn't get
> created and must-fail "rev-parse --verify" test checks that.
>
> Another thing that could matter would be that future actions that want to
> create a reflog for the same branch (perhaps after the user switches to
> 'master', another attempt is made to create eta with "checkout -b eta") or
> another branch with a similar or related name (say "eta/real") are not get
> broken by whatever you do to implement the "we want to create a reflog
> when a ref is actually made but not right now" feature.  Perhaps the right
> way to test that would be to actually try to run such operations and make
> sure they do not fail.

You were cutting off redundancy checks, weren't you?  If there is no
reflog file (tested by "test -f"), consequently no reflog (tested by
"rev-parse --verify") and no branch with the chosen name, certainly
someone can create one the "almost used" name, once there is no any
difference from a normal situation.

> [Footnote]
>
> *1* For example, you could have implemented the feature by adding a config
> item in ".git/config: [branch "eta"] need-to-create-reflog", and taught
> refs.c::update_ref() to pay attention to it (I am not saying that it would
> be a better implementation).

About the late parenthesis: thank God! ;-)

I don't see a need for so much reluctance: "test -f" is not a taboo
inside a script in t folder and the added tests don't change anything
about the design and implementation which IMHO is well fit.

With those two patch lines of mine "--orphan with -l and
core.logAllRefUpdates=false" testing script is finished.

Finally: you are the man in charge so I would really like to enforce
that if you need me to do anything more I will be _really_ glad to
help.  I love git and everything good done to it it is done to me too
as one of its daily user.

Best regards

Re: [PATCH] t2017: redo physical reflog existance check

[Jonathan Nieder]

Hi Erick,

First, thanks for checkout --orphan.  I was a skeptic but now that I
have seen it being used for things similar to "going open source" (and
the related "simplifying logs while working privately on a patch
series") it looks to be a pretty nice tool.

Erick Mattos wrote:

> I don't see a need for so much reluctance: "test -f" is not a taboo
> inside a script in t folder and the added tests don't change anything
> about the design and implementation which IMHO is well fit.

The principle (though we do not always adhere to it) is that test
scripts should pass or fail based only on advertised behavior, not
implementation details.  That way, _later_ any person who wants to
improve the implementation will not be impeded by tests.

The behavior that "test -f .git/logs/refs/heads/eta" checks for is not
part of the advertised behavior and though it does affect the
observable behavior, it is not immediately obvious how.  Wouldn't it
be best if the test described that advertised behavior while checking
for it?

e.g.:

	git config core.logallrefupdates false &&
	test_when_finished "git config core.logallrefupdates true" &&

  	git checkout master &&
  	git checkout -l --orphan eta &&
	test_must_fail git rev-parse --verify eta@{0} &&

	test_tick &&
	git commit -m "initial commit" &&
	git rev-parse --verify eta@{0}

Happily, I am not the man in charge, so feel free to take my words
at whatever value you choose. :)

Regards,
Jonathan

Re: [PATCH] t2017: redo physical reflog existance check

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The principle (though we do not always adhere to it) is that test
> scripts should pass or fail based only on advertised behavior, not
> implementation details.  That way, _later_ any person who wants to
> improve the implementation will not be impeded by tests.

Well and 'nuff said ;-)  Thanks.

Re: [PATCH] t2017: redo physical reflog existance check

[Erick Mattos]

Hi,

2010/8/24 Jonathan Nieder <jrnieder@gmail.com>
> Hi Erick,
>
> First, thanks for checkout --orphan.  I was a skeptic but now that I
> have seen it being used for things similar to "going open source" (and
> the related "simplifying logs while working privately on a patch
> series") it looks to be a pretty nice tool.

You're welcome! It is my pleasure, really! The reason for me to start
participating is a concern to pay back some of the benefits I have
been having by using free software.

I wish I could help all the excellent programs I use but I intend to
help at least some. Nonetheless I would like to do it to all.

So I am really grateful that you let me know my work was useful to you.

I have been addressing problems I had when dealing with git and
--orphan is important for private branch working. Even that I use
--reset-author more often.

I would eventually add more stuff to git, while it is already quite a
wonderful tool right now though It can be enhanced. However I don't
plan to because I had been facing extreme opposition and I don't like
to disturb. I only want to spend my effort where it helps.

> Erick Mattos wrote:
>
> > I don't see a need for so much reluctance: "test -f" is not a taboo
> > inside a script in t folder and the added tests don't change anything
> > about the design and implementation which IMHO is well fit.
>
> The principle (though we do not always adhere to it) is that test
> scripts should pass or fail based only on advertised behavior, not
> implementation details.  That way, _later_ any person who wants to
> improve the implementation will not be impeded by tests.

Tests won't impede a developer but it will show him that he is
changing something in fact. And they will tell him he needs to adjust
the tests too.

But in general it is a good practice to adhere to that principle
unless implementation is what you are really testing. At this case,
all right behavior had their tests before it and this one is to check
for a bad implementation.

"The problem", test -f, is used 428 times inside 92 scripts and in a
similar way (test -f .git/...) 48 times inside 19 scripts. So I don't
think it is correct that I have to struggle so much to use it too.

And the major fact is that the actual test is testing NOTHING and that
the test lamed IS important!

> The behavior that "test -f .git/logs/refs/heads/eta" checks for is not
> part of the advertised behavior and though it does affect the
> observable behavior, it is not immediately obvious how.  Wouldn't it
> be best if the test described that advertised behavior while checking
> for it?

I really haven't got you point. Let's see:

'giving up --orphan not committed when -l and core.logAllRefUpdates =
false deletes reflog'

Don't you think reflog deletion is immediately obvious?

> e.g.:
>
>        git config core.logallrefupdates false &&
>        test_when_finished "git config core.logallrefupdates true" &&
>
>        git checkout master &&
>        git checkout -l --orphan eta &&
>        test_must_fail git rev-parse --verify eta@{0} &&
>
>        test_tick &&
>        git commit -m "initial commit" &&
>        git rev-parse --verify eta@{0}

Your test is not the same and that is already done by zeta: '--orphan
with -l makes reflog when core.logAllRefUpdates = false'.

> Happily, I am not the man in charge, so feel free to take my words
> at whatever value you choose. :)

Not quite happily though.

Unfortunately the problem which git does not address for now is that
git have cemented the distributed repository but it really haven't
created a distributed development workflow.

Someone still have to create a workflow where people merge code,
design, give the directions, do quality optimization and more managing
things TOGETHER.

While everybody have the right to access and work separately, there is
no existent tool to integrate the work without a need for a moderator
which is always a flow constriction.

You can see Git and Linux as example: while the widespread use of
distributed repositories, every management is closed to the
maintainers which can possibly ignore good stuff or hasty accept later
proved bad deals.

That dictatorial guidance leads to political problems, inherent of
human being existence, putting code subscribed to who is writing it
and not to how good it is.

I think that is the major reason for so much forks and developer
desertions nowadays and for not merging potential good stuff because
of people animosity.

I believe someone very clever will soon find a way for not depending
so much of one person alone and get a way of making code evolution
happen by itself in a real distributed work flow.

> Regards,
> Jonathan

Your words value much. So I would appreciate to know your following perceptions.

It is always a pleasure to talk to you and to have an opportunity to
get your always objective and balanced point-of-views.

Best regards