Re: [RTC/PATCH] Add 'update-branch' hook

2014-04-26 Thread Junio C Hamano
Felipe Contreras felipe.contre...@gmail.com writes:

 So you grant that there is no reason anybody can think of why we would ever
 want a post-update-branch?

No, it only shows that you (and I) are not imaginative enough
(and/or we didn't bother spending enough brain cycles) to come up
with an example.  Your lack of imagination and foresight does not
give you any right to close the door to those who come after you who
have real needs, or make it awkward to add it later for them.

 Let's make a bet, we go for 'pre-update-branch' and five years from now, if
 there's no 'post-update-branch', you will publicly accept thta I was right.

 Deal?

Let me get this straight.  You spent a lot of effort to argue that
naming it update-branch is the right thing, but now you want me to
name it pre-update-branch, only so that you can prove you are right?

Playing a silly bet among friends may be fun from time to time.  But
I do not like using Git as a plaything, I am not your friend, and I
never felt it fun having to interact with you.  I am not interested
in proving you right or wrong.  You are not that interesting.

What you said however shows clearly the reason why it is not fun to
work with you, and I think that is a lot more important point.  Your
priorities are screwed up.  For the rest of us, making Git better is
the primary reason why we are here.  You seem to be saying that it
is more important to you, however, to win your little argument,
and you are willing to even sacrifice a better Git (in your mind,
with the hook named as update-branch) in order to win.

With a person with such screwed-up priorities, nobody whose first
objective is to make Git better can have a sane conversation.  Ask
those who said they do not want to work with you.  In the list
archive, there are plenty of examples to choose from, and I think
they will agree with me.

It is a pity that in all of these long flamefest, you may have meant
well to improve Git when you brought up something that needs to be
solved in your first few messages.  The rest of us may even have
agreed that it is good to address that issue on many of them.  But
the time something needs to be done and the way Felipe proposes
to solve it is good turns out to be different, i.e. when those who
agree with the former do not agree with the latter, the discussions
with you go downhill quick.  Each and every time.  See your index
is hard to learn for people---can we do something? topic, if you
want another example, where you try to twist words by Peff and
others and caught in doing so.

Now, I know you are going to say that is what *you* think, and even
if they agree, that is only what *they* think. it is not true! my
priorities are right and they are wrong!.

I'd freely give you that they are only *impressions* we have on you,
that we were forced to form by observing your past and present
behaviours.  It may not be true you.  You may be a loving an
wonderful person in reality, and you are not showing your true self
when you are on this list.  But you know something?  The project
advances by humans working together, and without telepathy, these
impression are the only thing we humans can go by.

I also know that you are going to say that is what *you* think.  I
have nothing more to say to you at that point.

It could be that your bet is a way for you to finally admitting
that naming the hook with pre- prefix will result in a better Git
than without, without you having to say Yes, you are right, let's
change it (which I rarely if ever saw you doing).  But still that
shows the same screwed-up priorities---winning your little argument
(or not losing it) matters more to you than working well with
others.  I do not think I want to work with such a person.



--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-26 Thread Felipe Contreras
Junio C Hamano wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  So you grant that there is no reason anybody can think of why we would ever
  want a post-update-branch?
 
 No, it only shows that you (and I) are not imaginative enough
 (and/or we didn't bother spending enough brain cycles) to come up
 with an example.

That is the same thing; nobody can think of a reason.

 Your lack of imagination and foresight does not give you any right to close
 the door to those who come after you who have real needs, or make it awkward
 to add it later for them.

No, but rationality and evidence are the only things we can use to make
decisions, and there is no reason to think something is going to happen, I
don't see why any rational person would think that it would.

  Let's make a bet, we go for 'pre-update-branch' and five years from now, if
  there's no 'post-update-branch', you will publicly accept thta I was right.
 
  Deal?
 
 Let me get this straight.  You spent a lot of effort to argue that
 naming it update-branch is the right thing, but now you want me to
 name it pre-update-branch, only so that you can prove you are right?

That is right.

It's impossible to convince you with logic and evidence, and even when you are
shown to be wrong, you don't accept it.

There is literally nothing anybody can do to convince you that imaginary fears
are just imaginary. At the end of the day your conclusion will be that the
improbable is still possible, well anything is possible, so saying I don't see
how A could happen, but it's possible is really not saying anything at all.

So yes, the only thing I can do is give up, however if you have any stakes in
progress of Git you would put your money where your mouth is.

If you had already done your job you should have some certainty on whether a
'post-update-branch' is going to happen or not. If you don't have such
certainty enough to make a bet, then why should anybody trust your conclusion?

 For the rest of us, making Git better is the primary reason why we are here.
 You seem to be saying that it is more important to you, however, to win
 your little argument, and you are willing to even sacrifice a better Git (in
 your mind, with the hook named as update-branch) in order to win.

I can't make Git better if you don't humble yourself. You need to accept when
you are wrong.

If I say A is not going to happen and I provide evidence, but you say it
would, and indeed it doesn't happen, maybe when I say B is not going to
happen, maybe you would actually listen, and if not, maybe when I say
C is not going to happen you would.

But if you are only willing to accept you were wrong when it's safe, then how
is anything going to change for the future.

 With a person with such screwed-up priorities, nobody whose first
 objective is to make Git better can have a sane conversation.

You are the one with the screwed priorities.

Time and time again the #1 issue people have raised about Git is the
user-interface. We even had Git user surveys to try to find out what people
wanted.

In these surveys the last thing people wanted was better performance, yet most
Git developers are still focusing on performance.

What people said needed improvement was the user-interface and documentation,
yet *nothing* has changed in these two areas. It's not a wonder no more surveys
are launched; because the results of such surveys are ignored anyway.

If a project has screwed-up priorities, it's when the areas of improvements
users say are needed get ignored.

 if you want another example, where you try to twist words by Peff and others
 and caught in doing so.

This is plainly intellectually dishonest. One year ago I made a summary of what
others said[1], I tried to keep it verbatim, CC'ed them, and invited them to
clarify if their position was misrepresented. Nobody, not even Jeff complained
about that.

Now, my mistake was thinking that A is better meant we should go for A,
however, that wasn't the case for Jeff. I didn't twist any words, I made a
wrong assumption.

However, I bet most people in that list agree that we should go for A, and if
you want, I can ask them all again (except Jeff, because we know his answer).
But I bet you are not interested in what they (or for that matter anyone) think
on the matter.

And you know it was an easy mistake to make, to accuse me of twisting words is
just dishonest.

 It could be that your bet is a way for you to finally admitting
 that naming the hook with pre- prefix will result in a better Git
 than without, without you having to say Yes, you are right, let's
 change it (which I rarely if ever saw you doing).  But still that
 shows the same screwed-up priorities---winning your little argument
 (or not losing it) matters more to you than working well with
 others.  I do not think I want to work with such a person.

That is not it at all.

How am I going to convince you of anything controversial in the future, if you
are never willing to admit 

Re: [RTC/PATCH] Add 'update-branch' hook

2014-04-24 Thread Stephen Leake
Felipe Contreras felipe.contre...@gmail.com writes:

  I have a branch which should always be recompiled on update;
  post-update-branch would be a good place for that.
 
  And why would pre-update-branch not serve that purpose?
 
 Because the code that needs to be compiled is not yet in the workspace

 And it won't be in 'post-update-branch' either.

Then you are using a very odd definition of post update

  % git checkout master
  % git branch feature-a stable
  - update-branch hook will be called here

 The hook will get 'feature-a' as the first argument, but the code in the
 workspace would correspond to 'master'; the checked out branch (pre or post).

Then the hooks should be called 'pre-branch', 'post-branch'; there is no
update involved.

The hook I need is actually post-merge, since merge is the command that
updates the workspace.

Sorry for the noise.
-- 
-- Stephe
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-24 Thread Felipe Contreras
Stephen Leake wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
   I have a branch which should always be recompiled on update;
   post-update-branch would be a good place for that.
  
   And why would pre-update-branch not serve that purpose?
  
  Because the code that needs to be compiled is not yet in the workspace
 
  And it won't be in 'post-update-branch' either.
 
 Then you are using a very odd definition of post update

It's not. The branch was updated, not the workspace.

   % git checkout master
   % git branch feature-a stable
   - update-branch hook will be called here
 
  The hook will get 'feature-a' as the first argument, but the code in the
  workspace would correspond to 'master'; the checked out branch (pre or 
  post).
 
 Then the hooks should be called 'pre-branch', 'post-branch'; there is no
 update involved.

Of course there is. A 'branch' hook would be triggered when you create a new
branch (e.g. `git branch`), however, it should not be triggered when you update
a branch (e.g. `git rebase`).

 The hook I need is actually post-merge, since merge is the command that
 updates the workspace.

I'd say it's probably 'post-checkout'.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-23 Thread Stephen Leake
Felipe Contreras felipe.contre...@gmail.com writes:

 Stephen Leake wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  Ilya Bobyr wrote:
  On 4/21/2014 2:17 PM, Felipe Contreras wrote:
   Ilya Bobyr wrote:
  
   Also, most have names that start with either pre- or post-.
   It seems reasonable for both pre-update-branch and
   post-update-branch to exist.
   I don't see what would be the point in that.
  
  Do you see the point in the other hooks doing that?
 
  Yes, there a reason for the existance of those hooks. Now tell me why would
  anybody use post-update-branch instead of pre-update-branch?
 
 I have a branch which should always be recompiled on update;
 post-update-branch would be a good place for that.

 And why would pre-update-branch not serve that purpose?

Because the code that needs to be compiled is not yet in the workspace

-- 
-- Stephe
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-23 Thread Felipe Contreras
Stephen Leake wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  Stephen Leake wrote:
  Felipe Contreras felipe.contre...@gmail.com writes:
  
   Ilya Bobyr wrote:
   On 4/21/2014 2:17 PM, Felipe Contreras wrote:
Ilya Bobyr wrote:
   
Also, most have names that start with either pre- or post-.
It seems reasonable for both pre-update-branch and
post-update-branch to exist.
I don't see what would be the point in that.
   
   Do you see the point in the other hooks doing that?
  
   Yes, there a reason for the existance of those hooks. Now tell me why 
   would
   anybody use post-update-branch instead of pre-update-branch?
  
  I have a branch which should always be recompiled on update;
  post-update-branch would be a good place for that.
 
  And why would pre-update-branch not serve that purpose?
 
 Because the code that needs to be compiled is not yet in the workspace

And it won't be in 'post-update-branch' either.

 % git checkout master
 % git branch feature-a stable
 - update-branch hook will be called here

The hook will get 'feature-a' as the first argument, but the code in the
workspace would correspond to 'master'; the checked out branch (pre or post).

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-23 Thread Felipe Contreras
Junio C Hamano wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  ... there are _already_ hooks without pre/post.
 
 Like commit-msg?  Yes, it would have been nicer if it were named
 verify-commit-message or something.

No it wouldn't. I can use the commit-msg hook to change the commit message and
to absolutely no verification, so verify-commit-message would be misleading.

Maybe you would like modify-and-or-verify-commit-message which would be
correct, but I wouldn't, I like short-and-sweet, and commit-msg is just that.

 Old mistakes are harder to change because of inertia.  It is not a
 good excuse to knowingly make a new mistake to add new exceptions
 that the users need to check documentations for, is it?

That's a nifty trick; label something a mistake, and then it suddenly becomes
one.

No, it's not a mistake, first it has to be proven to be mistake and I haven't
seen any arguments that try to do so.

Besides it's a red herring, you said such a name would be original and I've
just proved that it's not original, so the originality is not a concern.

  And it's not confusing,
 
 A simple fact that Ilya asked the question tells us otherwise ;-)

It's not any more confusing than these:

applypatch-msg:

When does this happen? Can I return an error?

pre-applypatch:

Again when does it happen? What does the input contains? The whole patch? 
Including the message?

post-applypatch:

Totally confused.

pre-commit:
prepare-commit-msg:
commit-msg:

What is the difference between these? Doesn't pre-commit contains the message 
already?

pre-receive:

Before receiving what?

update:

Updating what? When is it called? Can I cancel something?

The fact that somebody asked a question doesn't make a name confusing.

 I personally do not see an immediate need for post-update-branch,
 but if the new hook is about intervening an operation,

It's not about that, I can remove that feature if it would make you happier.

 Otherwise it would be impossible to later add post-update-branch

Which is never going to happen.

I'm still waiting for anybody to imagine any reason why we might want
post-udpate-branch.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-23 Thread Junio C Hamano
Felipe Contreras felipe.contre...@gmail.com writes:

 Junio C Hamano wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  ... there are _already_ hooks without pre/post.
 
 Like commit-msg?  Yes, it would have been nicer if it were named
 verify-commit-message or something.

 No it wouldn't. I can use the commit-msg hook to change the commit message and
 to absolutely no verification, so verify-commit-message would be misleading.

You are confused (and please do not spread the confusion).  If you
read the first paragraph of the documentation on the hook and think
for 5 seconds why --no-verify countermands it, you would realize
that the hook is primarily meant for verification.  We also allow
the hook to edit the message, but that is not even a useful feature
added as an afterthought; the documentation mentions it because the
implementation did not bother to make sure the hook did not touch
the message file.

It was a mistake not to call it with a clear name that tells
verification happens there.

 Old mistakes are harder to change because of inertia.  It is not a
 good excuse to knowingly make a new mistake to add new exceptions
 that the users need to check documentations for, is it?

I see no reason to waste more time on this point.
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-23 Thread Junio C Hamano
Felipe Contreras felipe.contre...@gmail.com writes:

  I have a branch which should always be recompiled on update;
  post-update-branch would be a good place for that.
 
  And why would pre-update-branch not serve that purpose?
 
 Because the code that needs to be compiled is not yet in the workspace

 And it won't be in 'post-update-branch' either.

  % git checkout master
  % git branch feature-a stable
  - update-branch hook will be called here

 The hook will get 'feature-a' as the first argument, but the code in the
 workspace would correspond to 'master'; the checked out branch (pre or post).

The whole point of a pre- hook is to run _before_ the externally
observable state changes due to the operation.

If Stephen has a separate build-tree that fetches from the branch
every time the tip of the branch changes in this repository to
produce build artifacts for the branch to be shared in his network,
perhaps via NFS or something.  git fetch that will be run from
that build-tree repository will *not* see the tip of the branch, and
running such a hook will not be possible from a pre-update-branch
hook.

We can certainly argue that such a hook could instead push to the
build-tree repository using the commit object name, but I tend to
think such an argument is merely sidestepping the real issue.  Some
hooks do want to observe the state _after_ the operation [*1*],
while some hooks can do without seeing exactly the state after the
operation.

So while I am generally not very supportive towards post-anything
hook, I would reject a claim that says pre-anything can be used
without inventing post-anything---do the same thing and allow the
operation and you are done.  That is not simply true.


[Footnote]

*1* A trivial example: send out an e-mail that contains the output
from git branch -l -v or git log --oneline --decorate --all
to a logger and expect to see the branch tip pointing at the
commit _after_ the operation.

--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-23 Thread Felipe Contreras
Junio C Hamano wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  Junio C Hamano wrote:
  Felipe Contreras felipe.contre...@gmail.com writes:
  
   ... there are _already_ hooks without pre/post.
  
  Like commit-msg?  Yes, it would have been nicer if it were named
  verify-commit-message or something.
 
  No it wouldn't. I can use the commit-msg hook to change the commit message 
  and
  to absolutely no verification, so verify-commit-message would be misleading.
 
 You are confused (and please do not spread the confusion).  If you
 read the first paragraph of the documentation on the hook and think
 for 5 seconds why --no-verify countermands it, you would realize
 that the hook is primarily meant for verification.

I do not care what the hook is primarily for, it's for more than just
verification.

 We also allow the hook to edit the message, but that is not even a useful
 feature added as an afterthought; the documentation mentions it because the
 implementation did not bother to make sure the hook did not touch the message
 file.

Indeed it's too late now, and now the hook does more than just verification,
therefore verify-commit-message wouldn't be an appropriate name.

 It was a mistake not to call it with a clear name that tells
 verification happens there.

No, the name is fine for what the hook does, if you would want the script to do
something different, *and* change the name of the script, that's a different
issue.

  Old mistakes are harder to change because of inertia.  It is not a
  good excuse to knowingly make a new mistake to add new exceptions
  that the users need to check documentations for, is it?
 
 I see no reason to waste more time on this point.

You haven't proved it's a mistake.

The only thing you have showed is that letting the 'commit-msg' modify the
message was a mistake, not that the name is wrong for what it currently does.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-23 Thread Felipe Contreras
Junio C Hamano wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
   I have a branch which should always be recompiled on update;
   post-update-branch would be a good place for that.
  
   And why would pre-update-branch not serve that purpose?
  
  Because the code that needs to be compiled is not yet in the workspace
 
  And it won't be in 'post-update-branch' either.
 
   % git checkout master
   % git branch feature-a stable
   - update-branch hook will be called here
 
  The hook will get 'feature-a' as the first argument, but the code in the
  workspace would correspond to 'master'; the checked out branch (pre or 
  post).
 
 The whole point of a pre- hook is to run _before_ the externally
 observable state changes due to the operation.
 
 If Stephen has a separate build-tree that fetches from the branch
 every time the tip of the branch changes in this repository to
 produce build artifacts for the branch to be shared in his network,
 perhaps via NFS or something.  git fetch that will be run from
 that build-tree repository will *not* see the tip of the branch, and
 running such a hook will not be possible from a pre-update-branch
 hook.
 
 We can certainly argue that such a hook could instead push to the
 build-tree repository using the commit object name,

Exactly, it could do that.

 but I tend to think such an argument is merely sidestepping the real issue.

So you grant that there is no reason anybody can think of why we would ever
want a post-update-branch?

 Some hooks do want to observe the state _after_ the operation [*1*], while
 some hooks can do without seeing exactly the state after the operation.

Yes, and when the operation is updating a branch, nobody can think of why we
would want the former.

 So while I am generally not very supportive towards post-anything
 hook, I would reject a claim that says pre-anything can be used
 without inventing post-anything---do the same thing and allow the
 operation and you are done.  That is not simply true.

Let's make a bet, we go for 'pre-update-branch' and five years from now, if
there's no 'post-update-branch', you will publicly accept thta I was right.

Deal?

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Ilya Bobyr
On 4/21/2014 3:24 PM, Felipe Contreras wrote:
 Ilya Bobyr wrote:
 On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras 
 felipe.contre...@gmail.com wrote:
 Ilya Bobyr wrote:
 test_expect_success 'setup' 
   mkdir -p .git/hooks 
   cat  .git/hooks/update-branch -\\EOF 
   #!/bin/sh
   echo \$@  .git/update-branch.args
   EOF
   chmod +x .git/hooks/update-branch 
   echo one  content 
   git add content 
   git commit -a -m one
 
 That is not maintainable at all.
 Maybe you could explain how is this less maintainable, compared to a separate
 function?
 Do I really have to explain that manually escaping a shell script is not
 maintainable?

This is rude.

Here is how you can do it without escaping:

test_expect_success 'setup' '
mkdir -p .git/hooks 
cat  .git/hooks/update-branch -\EOF 
#!/bin/sh
echo $@  .git/update-branch.args
EOF
chmod +x .git/hooks/update-branch 
echo one  content 
git add content 
git commit -a -m one
'

It is not different from most of the tests, I think.

 This is how it is suggested by t/README and how it is done in the other
 test suites.
 I can not see how your case is different, but I might be missing something.
 Let's take a cursoy look at `git grep -l 'EOF' t`.

 [...]

So the point is that some existing tests violate best practices?
I do not think this is a good justification to do the same for new tests.

 In fact my version is actually cleaner than these, because the code that is 
 run
 outside the cage is clearly delimited by a function.

It depends on the perspective.
If it fails, the failure would be missed regardless of if it is in a
function or not.
Most examples that you quoted only create files outside test_expect_success.
Even that is not necessary.

I am not telling you how you should write it.
I am just saying that you are breaking one of the recommendations on how
to write tests.
There are different options that adhere to the suggestions in t/README.
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Ilya Bobyr
On 4/20/2014 7:23 PM, Felipe Contreras wrote:
 [...]

 diff --git a/branch.c b/branch.c
 index 660097b..c2058d1 100644
 --- a/branch.c
 +++ b/branch.c
 @@ -4,6 +4,7 @@
  #include refs.h
  #include remote.h
  #include commit.h
 +#include run-command.h
  
  struct tracking {
   struct refspec spec;
 @@ -304,6 +305,11 @@ void create_branch(const char *head,
   if (real_ref  track)
   setup_tracking(ref.buf + 11, real_ref, track, quiet);
  
 + if (run_hook_le(NULL, update-branch, ref.buf + 11, sha1_to_hex(sha1), 
 NULL)) {
 + unlock_ref(lock);

lock is NULL if dont_change_ref is true.  unlock_ref() would crash in
that case.
You may want to add a test for that.

 + die(hook 'update-branch' returned error);
 + }
 +
   if (!dont_change_ref)
   if (write_ref_sha1(lock, sha1, msg)  0)
   die_errno(_(Failed to write ref));
 diff --git a/builtin/clone.c b/builtin/clone.c
 index 9b3c04d..6ec96e5 100644
 --- a/builtin/clone.c
 +++ b/builtin/clone.c
 @@ -581,9 +581,10 @@ static void update_remote_refs(const struct ref *refs,
   }
  }
  
 -static void update_head(const struct ref *our, const struct ref *remote,
 +static int update_head(const struct ref *our, const struct ref *remote,
   const char *msg)
  {
 + int err = 0;
   if (our  starts_with(our-name, refs/heads/)) {
   /* Local default branch link */
   create_symref(HEAD, our-name, NULL);
 @@ -591,6 +592,7 @@ static void update_head(const struct ref *our, const 
 struct ref *remote,
   const char *head = skip_prefix(our-name, 
 refs/heads/);
   update_ref(msg, HEAD, our-old_sha1, NULL, 0, 
 DIE_ON_ERR);
   install_branch_config(0, head, option_origin, 
 our-name);
 + err = run_hook_le(NULL, update-branch, head, 
 sha1_to_hex(our-old_sha1), NULL);

This is happening after the branch is updated and a config section for
it is created.

   }
   } else if (our) {
   struct commit *c = lookup_commit_reference(our-old_sha1);
 @@ -606,6 +608,7 @@ static void update_head(const struct ref *our, const 
 struct ref *remote,
   update_ref(msg, HEAD, remote-old_sha1,
  NULL, REF_NODEREF, DIE_ON_ERR);
   }
 + return err;
  }
  
  static int checkout(void)
 @@ -987,7 +990,7 @@ int cmd_clone(int argc, const char **argv, const char 
 *prefix)
   update_remote_refs(refs, mapped_refs, remote_head_points_at,
  branch_top.buf, reflog_msg.buf, transport, 
 !is_local);
  
 - update_head(our_head_points_at, remote_head, reflog_msg.buf);
 + err = update_head(our_head_points_at, remote_head, reflog_msg.buf);
  
   transport_unlock_pack(transport);
   transport_disconnect(transport);
 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
 index 1c41cbd..084dc36 100644
 --- a/git-rebase--interactive.sh
 +++ b/git-rebase--interactive.sh
 @@ -631,7 +631,11 @@ do_next () {
   git update-ref -m $message $head_name $newhead $orig_head 
   git symbolic-ref \
 -m $GIT_REFLOG_ACTION: returning to $head_name \
 -   HEAD $head_name
 +   HEAD $head_name 
 + if test -x $GIT_DIR/hooks/update-branch; then
 + $GIT_DIR/hooks/update-branch $branch_name \
 + $newhead $onto
 + fi

It looks like this is also after the branch was already updated.

   ;;
   esac  {
   test ! -f $state_dir/verbose ||
 diff --git a/git-rebase.sh b/git-rebase.sh
 index 2c75e9f..ededa32 100755
 --- a/git-rebase.sh
 +++ b/git-rebase.sh
 @@ -149,7 +149,11 @@ move_to_original_branch () {
   $head_name $(git rev-parse HEAD) $orig_head 
   git symbolic-ref \
   -m rebase finished: returning to $head_name \
 - HEAD $head_name ||
 + HEAD $head_name 
 + if test -x $GIT_DIR/hooks/update-branch; then
 + $GIT_DIR/hooks/update-branch $branch_name \
 + $(git rev-parse HEAD) $onto
 + fi ||

Same here.

   die $(gettext Could not move back to $head_name)
   ;;
   esac
 diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
 new file mode 100755
 index 000..d921c0e
 --- /dev/null
 +++ b/t/t5408-update-branch-hook.sh
 @@ -0,0 +1,39 @@
 +#!/bin/sh
 +
 +test_description='Test the update-branch hook'
 +
 +. ./test-lib.sh
 +
 +setup () {
 + mkdir -p .git/hooks 
 + cat  .git/hooks/update-branch -'EOF' 
 + #!/bin/sh
 + echo $@  .git/update-branch.args
 + EOF
 + chmod +x .git/hooks/update-branch 
 + echo one  content 
 + git add content 
 + git commit -a -m one
 +}
 +
 +setup
 +
 +test_expect_success 

Re: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Ilya Bobyr
On 4/21/2014 1:49 PM, Felipe Contreras wrote:
 Ilya Bobyr wrote:
 On 4/20/2014 7:23 PM, Felipe Contreras wrote:
 This hook is invoked whenever a branch is updated, either when a branch
 is created or updated with 'git branch', or when it's rebased with 'git
 rebase'. It receives two parameters; the name of the branch, and the
 SHA-1 of the latest commit, additionally, if there was a base commit the
 branch was rebased onto, a third parameter contains it.
 And the old branch SHA could be found from in the reflog, correct?
 Actually the old branch SHA-1 is actually the current one, since the branch
 hasn't been updated at that point. Personally I don't see much value in adding
 something the script can easily find out.

If the hook is about a branch update, I would expect it to provide both
old and new points for the branch, along with the name.

The fact that for rebases it also provides new base SHA is very
convenient.  As it is an optional argument it may make further extension
of the interface a bit awkward.
So, is seems reasonable to provide both from the very beginning.
I was looking for hooks like that, to maintain certain meta-data about
the branches.
Old SHA would be very useful in that case.

I am not sure if both SHAs are easily available at the point where the
hook is called.
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/21/2014 3:24 PM, Felipe Contreras wrote:
  Ilya Bobyr wrote:
  On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras 
  felipe.contre...@gmail.com wrote:
  Ilya Bobyr wrote:
  test_expect_success 'setup' 
mkdir -p .git/hooks 
cat  .git/hooks/update-branch -\\EOF 
#!/bin/sh
echo \$@  .git/update-branch.args
EOF
chmod +x .git/hooks/update-branch 
echo one  content 
git add content 
git commit -a -m one
  
  That is not maintainable at all.
  Maybe you could explain how is this less maintainable, compared to a 
  separate
  function?
  Do I really have to explain that manually escaping a shell script is not
  maintainable?
 
 This is rude.

So? I really don't see the need to explain that such a monstrosity would be
unmaintainable, that's a given.

 Here is how you can do it without escaping:
 
 test_expect_success 'setup' '
   mkdir -p .git/hooks 
   cat  .git/hooks/update-branch -\EOF 
   #!/bin/sh
   echo $@  .git/update-branch.args
   EOF
   chmod +x .git/hooks/update-branch 
   echo one  content 
   git add content 
   git commit -a -m one
 '
 
 It is not different from most of the tests, I think.

This is what I originally asked for.

  This is how it is suggested by t/README and how it is done in the other
  test suites.
  I can not see how your case is different, but I might be missing something.
  Let's take a cursoy look at `git grep -l 'EOF' t`.
 
  [...]
 
 So the point is that some existing tests violate best practices?

I don't know what you mean by best practices, but these are Git's best 
practices.

 I do not think this is a good justification to do the same for new tests.

It is not a justification to reject a patch either, specially if no better
alternative has been put forward.

Fortunately a better alternative has been put forward, so this is moot.
 
-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Ilya Bobyr
On 4/21/2014 11:45 PM, Felipe Contreras wrote:
 [...]
 This is how it is suggested by t/README and how it is done in the other
 test suites.
 I can not see how your case is different, but I might be missing something.
 Let's take a cursoy look at `git grep -l 'EOF' t`.

 [...]
 So the point is that some existing tests violate best practices?
 I don't know what you mean by best practices, but these are Git's best 
 practices.

I am talking about recommendations in t/README that I quoted.

--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/21/2014 11:45 PM, Felipe Contreras wrote:
  [...]
  This is how it is suggested by t/README and how it is done in the other
  test suites.
  I can not see how your case is different, but I might be missing 
  something.
  Let's take a cursoy look at `git grep -l 'EOF' t`.
 
  [...]
  So the point is that some existing tests violate best practices?
  I don't know what you mean by best practices, but these are Git's best 
  practices.
 
 I am talking about recommendations in t/README that I quoted.

Those are *guidelines*, best practices are defined as things you actually do,
as in actually practice.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Stephen Leake
Felipe Contreras felipe.contre...@gmail.com writes:

 Ilya Bobyr wrote:
 On 4/21/2014 2:17 PM, Felipe Contreras wrote:
  Ilya Bobyr wrote:
 
  Also, most have names that start with either pre- or post-.
  It seems reasonable for both pre-update-branch and
  post-update-branch to exist.
  I don't see what would be the point in that.
 
 Do you see the point in the other hooks doing that?

 Yes, there a reason for the existance of those hooks. Now tell me why would
 anybody use post-update-branch instead of pre-update-branch?

I have a branch which should always be recompiled on update;
post-update-branch would be a good place for that.

-- 
-- Stephe
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/20/2014 7:23 PM, Felipe Contreras wrote:
  [...]
 
  diff --git a/branch.c b/branch.c
  index 660097b..c2058d1 100644
  --- a/branch.c
  +++ b/branch.c
  @@ -4,6 +4,7 @@
   #include refs.h
   #include remote.h
   #include commit.h
  +#include run-command.h
   
   struct tracking {
  struct refspec spec;
  @@ -304,6 +305,11 @@ void create_branch(const char *head,
  if (real_ref  track)
  setup_tracking(ref.buf + 11, real_ref, track, quiet);
   
  +   if (run_hook_le(NULL, update-branch, ref.buf + 11, sha1_to_hex(sha1), 
  NULL)) {
  +   unlock_ref(lock);
 
 lock is NULL if dont_change_ref is true.  unlock_ref() would crash in
 that case.
 You may want to add a test for that.

That should be easy to fix.
 
  +   die(hook 'update-branch' returned error);
  +   }
  +
  if (!dont_change_ref)
  if (write_ref_sha1(lock, sha1, msg)  0)
  die_errno(_(Failed to write ref));
  diff --git a/builtin/clone.c b/builtin/clone.c
  index 9b3c04d..6ec96e5 100644
  --- a/builtin/clone.c
  +++ b/builtin/clone.c
  @@ -581,9 +581,10 @@ static void update_remote_refs(const struct ref *refs,
  }
   }
   
  -static void update_head(const struct ref *our, const struct ref *remote,
  +static int update_head(const struct ref *our, const struct ref *remote,
  const char *msg)
   {
  +   int err = 0;
  if (our  starts_with(our-name, refs/heads/)) {
  /* Local default branch link */
  create_symref(HEAD, our-name, NULL);
  @@ -591,6 +592,7 @@ static void update_head(const struct ref *our, const 
  struct ref *remote,
  const char *head = skip_prefix(our-name, 
  refs/heads/);
  update_ref(msg, HEAD, our-old_sha1, NULL, 0, 
  DIE_ON_ERR);
  install_branch_config(0, head, option_origin, 
  our-name);
  +   err = run_hook_le(NULL, update-branch, head, 
  sha1_to_hex(our-old_sha1), NULL);
 
 This is happening after the branch is updated and a config section for
 it is created.

I see that now, however, I cannot find where in builtin/clone.c is the branch
ref actually updated.

Worst, I don't see how I could possibly configure a hook to be triggered when
cloning, so I cannot test.
 
  }
  } else if (our) {
  struct commit *c = lookup_commit_reference(our-old_sha1);
  @@ -606,6 +608,7 @@ static void update_head(const struct ref *our, const 
  struct ref *remote,
  update_ref(msg, HEAD, remote-old_sha1,
 NULL, REF_NODEREF, DIE_ON_ERR);
  }
  +   return err;
   }
   
   static int checkout(void)
  @@ -987,7 +990,7 @@ int cmd_clone(int argc, const char **argv, const char 
  *prefix)
  update_remote_refs(refs, mapped_refs, remote_head_points_at,
 branch_top.buf, reflog_msg.buf, transport, 
  !is_local);
   
  -   update_head(our_head_points_at, remote_head, reflog_msg.buf);
  +   err = update_head(our_head_points_at, remote_head, reflog_msg.buf);
   
  transport_unlock_pack(transport);
  transport_disconnect(transport);
  diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
  index 1c41cbd..084dc36 100644
  --- a/git-rebase--interactive.sh
  +++ b/git-rebase--interactive.sh
  @@ -631,7 +631,11 @@ do_next () {
  git update-ref -m $message $head_name $newhead $orig_head 
  git symbolic-ref \
-m $GIT_REFLOG_ACTION: returning to $head_name \
  - HEAD $head_name
  + HEAD $head_name 
  +   if test -x $GIT_DIR/hooks/update-branch; then
  +   $GIT_DIR/hooks/update-branch $branch_name \
  +   $newhead $onto
  +   fi
 
 It looks like this is also after the branch was already updated.

This and the one below should be easy to fix.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/21/2014 1:49 PM, Felipe Contreras wrote:
  Ilya Bobyr wrote:
  On 4/20/2014 7:23 PM, Felipe Contreras wrote:
  This hook is invoked whenever a branch is updated, either when a branch
  is created or updated with 'git branch', or when it's rebased with 'git
  rebase'. It receives two parameters; the name of the branch, and the
  SHA-1 of the latest commit, additionally, if there was a base commit the
  branch was rebased onto, a third parameter contains it.
  And the old branch SHA could be found from in the reflog, correct?
  Actually the old branch SHA-1 is actually the current one, since the branch
  hasn't been updated at that point. Personally I don't see much value in 
  adding
  something the script can easily find out.
 
 If the hook is about a branch update, I would expect it to provide both
 old and new points for the branch, along with the name.

Again, I don't see the the point of passing something that is easy to find out:
`git rev-parse $branch` gives you that information.

 The fact that for rebases it also provides new base SHA is very
 convenient.  As it is an optional argument it may make further extension
 of the interface a bit awkward.
 So, is seems reasonable to provide both from the very beginning.

So basically `git branch` would send the same SHA-1 twice.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Felipe Contreras
Stephen Leake wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  Ilya Bobyr wrote:
  On 4/21/2014 2:17 PM, Felipe Contreras wrote:
   Ilya Bobyr wrote:
  
   Also, most have names that start with either pre- or post-.
   It seems reasonable for both pre-update-branch and
   post-update-branch to exist.
   I don't see what would be the point in that.
  
  Do you see the point in the other hooks doing that?
 
  Yes, there a reason for the existance of those hooks. Now tell me why would
  anybody use post-update-branch instead of pre-update-branch?
 
 I have a branch which should always be recompiled on update;
 post-update-branch would be a good place for that.

And why would pre-update-branch not serve that purpose?

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Ilya Bobyr
On 4/22/2014 9:31 AM, Felipe Contreras wrote:
 Stephen Leake wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 Ilya Bobyr wrote:
 On 4/21/2014 2:17 PM, Felipe Contreras wrote:
 Ilya Bobyr wrote:

 Also, most have names that start with either pre- or post-.
 It seems reasonable for both pre-update-branch and
 post-update-branch to exist.
 I don't see what would be the point in that.
 Do you see the point in the other hooks doing that?
 Yes, there a reason for the existance of those hooks. Now tell me why would
 anybody use post-update-branch instead of pre-update-branch?
 I have a branch which should always be recompiled on update;
 post-update-branch would be a good place for that.
 And why would pre-update-branch not serve that purpose?

pre- hook could be used, but if the hooks is not supposed to prevent
the operation, it seems reasonable to put it in the post- hook should
one be available.
For example, for clone and branch that would mean that that the branch
sections are already created in .git/config, but for pre- hooks,
should be find the right spot, configuration could probably be absent
just yet.

I do not think that someone is objecting adding just the pre- hook first.
But it seems unlikely that one can envision all the possible use cases
to say that post- hook would never be useful.
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-22 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/22/2014 9:31 AM, Felipe Contreras wrote:
  Stephen Leake wrote:
  Felipe Contreras felipe.contre...@gmail.com writes:
  Yes, there a reason for the existance of those hooks. Now tell me why 
  would
  anybody use post-update-branch instead of pre-update-branch?
  
  I have a branch which should always be recompiled on update;
  post-update-branch would be a good place for that.
  
  And why would pre-update-branch not serve that purpose?
 
 pre- hook could be used, but if the hooks is not supposed to prevent
 the operation, it seems reasonable to put it in the post- hook should
 one be available.

If 'pre-update-branch' can be used, then you are pretty much agreeing to the 
fact
that the 'post-udpate-branch' hook would be *useless*.

Such a script would work both as 'pre-update-branch' and 'post-update-branch',
therefore a single 'update-branch' would serve.

So I ask again:

Tell me why would anybody need 'post-update-branch' instead of
'pre-update-branch'?

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Eric Sunshine
On Sun, Apr 20, 2014 at 10:23 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 This hook is invoked whenever a branch is updated, either when a branch
 is created or updated with 'git branch', or when it's rebased with 'git
 rebase'. It receives two parameters; the name of the branch, and the
 SHA-1 of the latest commit, additionally, if there was a base commit the
 branch was rebased onto, a third parameter contains it.

 It can be used to verify the validity of branch names, and also to keep
 track of the origin point of a branch, which is otherwise not possible
 to find out [1].

 [1] http://thread.gmane.org/gmane.comp.version-control.git/198587

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---
 diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
 new file mode 100755
 index 000..d921c0e
 --- /dev/null
 +++ b/t/t5408-update-branch-hook.sh
 @@ -0,0 +1,39 @@
 +#!/bin/sh
 +
 +test_description='Test the update-branch hook'
 +
 +. ./test-lib.sh
 +
 +setup () {
 +   mkdir -p .git/hooks 
 +   cat  .git/hooks/update-branch -'EOF' 
 +   #!/bin/sh
 +   echo $@  .git/update-branch.args
 +   EOF
 +   chmod +x .git/hooks/update-branch 
 +   echo one  content 
 +   git add content 
 +   git commit -a -m one
 +}
 +
 +setup
 +
 +test_expect_success 'creating a branch' '
 +   git checkout -b test master 
 +   echo two  new 
 +   git add new 
 +   git commit -a -m two

Broken -chain.

 +   echo test $(git rev-parse master)  expected 
 +   test_cmp expected .git/update-branch.args
 +'
 +
 +test_expect_success 'doing a rebase' '
 +   git checkout -b next master 
 +   echo three  content 
 +   git commit -a -m three 
 +   git rebase --onto next master test 
 +   echo test $(git rev-parse HEAD) $(git rev-parse next)  expected 
 +   test_cmp expected .git/update-branch.args
 +'
 +
 +test_done
 --
 1.9.2+fc1.1.g5c924db
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Ilya Bobyr
On 4/20/2014 7:23 PM, Felipe Contreras wrote:
 This hook is invoked whenever a branch is updated, either when a branch
 is created or updated with 'git branch', or when it's rebased with 'git
 rebase'. It receives two parameters; the name of the branch, and the
 SHA-1 of the latest commit, additionally, if there was a base commit the
 branch was rebased onto, a third parameter contains it.

And the old branch SHA could be found from in the reflog, correct?
Maybe it is possible to add it as an extra argument?
Or if reflog could be used, a note in the description that would say so.
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/20/2014 7:23 PM, Felipe Contreras wrote:
  This hook is invoked whenever a branch is updated, either when a branch
  is created or updated with 'git branch', or when it's rebased with 'git
  rebase'. It receives two parameters; the name of the branch, and the
  SHA-1 of the latest commit, additionally, if there was a base commit the
  branch was rebased onto, a third parameter contains it.
 
 And the old branch SHA could be found from in the reflog, correct?

Actually the old branch SHA-1 is actually the current one, since the branch
hasn't been updated at that point. Personally I don't see much value in adding
something the script can easily find out.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Ilya Bobyr
On 4/21/2014 1:49 PM, Felipe Contreras wrote:
 Ilya Bobyr wrote:
 On 4/20/2014 7:23 PM, Felipe Contreras wrote:
 This hook is invoked whenever a branch is updated, either when a branch
 is created or updated with 'git branch', or when it's rebased with 'git
 rebase'. It receives two parameters; the name of the branch, and the
 SHA-1 of the latest commit, additionally, if there was a base commit the
 branch was rebased onto, a third parameter contains it.
 And the old branch SHA could be found from in the reflog, correct?
 Actually the old branch SHA-1 is actually the current one, since the branch
 hasn't been updated at that point. Personally I don't see much value in adding
 something the script can easily find out.

I did not understand that from the description.  That was my next
comment that I did not send just yet.
All the other hooks describe in detail if they are run before or after
the operation, and if it is possible to cancel the operation.
Also, most have names that start with either pre- or post-.
It seems reasonable for both pre-update-branch and
post-update-branch to exist.
This one would be pre-update-branch, I guess.

I was also wondering about git reset.  It could also change the branch
position, right?
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Ilya Bobyr
On 4/20/2014 7:23 PM, Felipe Contreras wrote:
 [...]

 diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
 new file mode 100755
 index 000..d921c0e
 --- /dev/null
 +++ b/t/t5408-update-branch-hook.sh
 @@ -0,0 +1,39 @@
 +#!/bin/sh
 +
 +test_description='Test the update-branch hook'
 +
 +. ./test-lib.sh
 +
 +setup () {
 + mkdir -p .git/hooks 
 + cat  .git/hooks/update-branch -'EOF' 
 + #!/bin/sh
 + echo $@  .git/update-branch.args
 + EOF
 + chmod +x .git/hooks/update-branch 
 + echo one  content 
 + git add content 
 + git commit -a -m one
 +}
 +
 +setup

According to t/README `setup` should be inside an assertion just as any
other test:

 Do's, don'ts  things to keep in mind
 -

 Here are a few examples of things you probably should and shouldn't do
 when writing tests.

 Do:

  - Put all code inside test_expect_success and other assertions.

Even code that isn't a test per se, but merely some setup code
should be inside a test assertion.

--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/20/2014 7:23 PM, Felipe Contreras wrote:
  [...]
 
  diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
  new file mode 100755
  index 000..d921c0e
  --- /dev/null
  +++ b/t/t5408-update-branch-hook.sh
  @@ -0,0 +1,39 @@
  +#!/bin/sh
  +
  +test_description='Test the update-branch hook'
  +
  +. ./test-lib.sh
  +
  +setup () {
  +   mkdir -p .git/hooks 
  +   cat  .git/hooks/update-branch -'EOF' 
  +   #!/bin/sh
  +   echo $@  .git/update-branch.args
  +   EOF
  +   chmod +x .git/hooks/update-branch 
  +   echo one  content 
  +   git add content 
  +   git commit -a -m one
  +}
  +
  +setup
 
 According to t/README `setup` should be inside an assertion just as any
 other test:

I have a bunch of 'setup' calls outside such assertions already in other test
scripts. If you know how to put single quotes inside of single quotes in a
shell script, please share that knowledge, otherwise the setup must be outside.

Of course we could do the extremely reduntant:

test_expect_success 'setup' '
  setup
'

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Felipe Contreras
Ilya Bobyr wrote:

 Also, most have names that start with either pre- or post-.
 It seems reasonable for both pre-update-branch and
 post-update-branch to exist.

I don't see what would be the point in that.

 This one would be pre-update-branch, I guess.
 
 I was also wondering about git reset.  It could also change the branch
 position, right?

That's right, maybe that command should call the hook 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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Ilya Bobyr
On 4/21/2014 2:15 PM, Felipe Contreras wrote:
 Ilya Bobyr wrote:
 On 4/20/2014 7:23 PM, Felipe Contreras wrote:
 [...]

 diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
 new file mode 100755
 index 000..d921c0e
 --- /dev/null
 +++ b/t/t5408-update-branch-hook.sh
 @@ -0,0 +1,39 @@
 +#!/bin/sh
 +
 +test_description='Test the update-branch hook'
 +
 +. ./test-lib.sh
 +
 +setup () {
 +   mkdir -p .git/hooks 
 +   cat  .git/hooks/update-branch -'EOF' 
 +   #!/bin/sh
 +   echo $@  .git/update-branch.args
 +   EOF
 +   chmod +x .git/hooks/update-branch 
 +   echo one  content 
 +   git add content 
 +   git commit -a -m one
 +}
 +
 +setup
 According to t/README `setup` should be inside an assertion just as any
 other test:
 I have a bunch of 'setup' calls outside such assertions already in other test
 scripts. If you know how to put single quotes inside of single quotes in a
 shell script, please share that knowledge, otherwise the setup must be 
 outside.

 Of course we could do the extremely reduntant:

 test_expect_success 'setup' '
   setup
 '

Setup does not look any different from the other tests.
If you need single quotes you could use double quotes outside.  Though,
you would have to quote other things as well.
t-basic.sh has a lot of tests that do that.
Like this, for example:

test_expect_success 'setup' 
mkdir -p .git/hooks 
cat  .git/hooks/update-branch -\\EOF 
#!/bin/sh
echo \$@  .git/update-branch.args
EOF
chmod +x .git/hooks/update-branch 
echo one  content 
git add content 
git commit -a -m one


--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Ilya Bobyr
On 4/21/2014 2:17 PM, Felipe Contreras wrote:
 Ilya Bobyr wrote:

 Also, most have names that start with either pre- or post-.
 It seems reasonable for both pre-update-branch and
 post-update-branch to exist.
 I don't see what would be the point in that.

Do you see the point in the other hooks doing that?
--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/21/2014 2:15 PM, Felipe Contreras wrote:
  Ilya Bobyr wrote:
  On 4/20/2014 7:23 PM, Felipe Contreras wrote:
  [...]
 
  diff --git a/t/t5408-update-branch-hook.sh b/t/t5408-update-branch-hook.sh
  new file mode 100755
  index 000..d921c0e
  --- /dev/null
  +++ b/t/t5408-update-branch-hook.sh
  @@ -0,0 +1,39 @@
  +#!/bin/sh
  +
  +test_description='Test the update-branch hook'
  +
  +. ./test-lib.sh
  +
  +setup () {
  + mkdir -p .git/hooks 
  + cat  .git/hooks/update-branch -'EOF' 
  + #!/bin/sh
  + echo $@  .git/update-branch.args
  + EOF
  + chmod +x .git/hooks/update-branch 
  + echo one  content 
  + git add content 
  + git commit -a -m one
  +}
  +
  +setup
  According to t/README `setup` should be inside an assertion just as any
  other test:
  I have a bunch of 'setup' calls outside such assertions already in other 
  test
  scripts. If you know how to put single quotes inside of single quotes in a
  shell script, please share that knowledge, otherwise the setup must be 
  outside.
 
  Of course we could do the extremely reduntant:
 
  test_expect_success 'setup' '
setup
  '
 
 Setup does not look any different from the other tests.
 If you need single quotes you could use double quotes outside.  Though,
 you would have to quote other things as well.
 t-basic.sh has a lot of tests that do that.
 Like this, for example:
 
 test_expect_success 'setup' 
   mkdir -p .git/hooks 
   cat  .git/hooks/update-branch -\\EOF 
   #!/bin/sh
   echo \$@  .git/update-branch.args
   EOF
   chmod +x .git/hooks/update-branch 
   echo one  content 
   git add content 
   git commit -a -m one
 

That is not maintainable at all.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Felipe Contreras
Ilya Bobyr wrote:
 On 4/21/2014 2:17 PM, Felipe Contreras wrote:
  Ilya Bobyr wrote:
 
  Also, most have names that start with either pre- or post-.
  It seems reasonable for both pre-update-branch and
  post-update-branch to exist.
  I don't see what would be the point in that.
 
 Do you see the point in the other hooks doing that?

Yes, there a reason for the existance of those hooks. Now tell me why would
anybody use post-update-branch instead of pre-update-branch?

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Junio C Hamano
Ilya Bobyr ilya.bo...@gmail.com writes:

 On 4/21/2014 2:17 PM, Felipe Contreras wrote:
 Ilya Bobyr wrote:

 Also, most have names that start with either pre- or post-.
 It seems reasonable for both pre-update-branch and
 post-update-branch to exist.
 I don't see what would be the point in that.

 Do you see the point in the other hooks doing that?

pre- and post- are primarily so that people can tell that pre-
happens before the operation and its primary motivation is to stop
an operation from happening as opposed to post- is called after
the fact and there is no way for it to intervene---it is too late;
it is primarily for things like logging easily.

As long as you can tell what you can use it for and when it is
called from the name of the hook, there is no fundamental reason why
you need to have pre- or post- prefix in your hook names, but unless
there is no other strong reason not to, it is probably a good idea
to follow suit.  There is not much value in trying to be original
in naming things, just to be different; it will only confuse the
users.


--
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Felipe Contreras
Ilya Bobyr wrote:
 On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras 
 felipe.contre...@gmail.com wrote:
  Ilya Bobyr wrote:
   test_expect_success 'setup' 
 mkdir -p .git/hooks 
 cat  .git/hooks/update-branch -\\EOF 
 #!/bin/sh
 echo \$@  .git/update-branch.args
 EOF
 chmod +x .git/hooks/update-branch 
 echo one  content 
 git add content 
 git commit -a -m one
   
 
  That is not maintainable at all.
 
 Maybe you could explain how is this less maintainable, compared to a separate
 function?

Do I really have to explain that manually escaping a shell script is not
maintainable?

 This is how it is suggested by t/README and how it is done in the other
 test suites.
 I can not see how your case is different, but I might be missing something.

Let's take a cursoy look at `git grep -l 'EOF' t`.

== t/t0009-prio-queue.sh ==

  cat expect 'EOF'
  1
  2
  3
  4
  5
  5
  6
  7
  8
  9
  10
  EOF
  test_expect_success 'basic ordering' '
  test-prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump actual 
  test_cmp expect actual
  '

Look at that, code outside the cage, not once, but in every test.

== t/t0040-parse-options.sh ==

  cat expect 'EOF'
  list: foo
  list: bar
  list: baz
  EOF
  test_expect_success '--list keeps list of strings' '
  test-parse-options --list foo --list=bar --list=baz output 
  test_cmp expect output
  '

Once again.

== t/t1411-reflog-show.sh ==
== t/t2020-checkout-detach.sh ==
== t/t3203-branch-output.sh ==
== t/t3412-rebase-root.sh ==
== t/t4014-format-patch.sh ==
== t/t4030-diff-textconv.sh ==

All these do something similar, not once, but many many times.

== t/t4031-diff-rewrite-binary.sh ==

  {
  echo #!$SHELL_PATH
  cat 'EOF'
  $PERL_PATH -e '$/ = undef; $_ = ; s/./ord($)/ge; print $_'  $1
  EOF
  } dump
  chmod +x dump

More code outside.

== t/t4042-diff-textconv-caching.sh ==

  cat helper 'EOF'
  #!/bin/sh
  sed 's/^/converted: /' $@ helper.out
  cat helper.out
  EOF
  chmod +x helper

== t/t5401-update-hooks.sh ==

  cat victim.git/hooks/pre-receive 'EOF'
  #!/bin/sh
  printf %s $@ $GIT_DIR/pre-receive.args
  cat - $GIT_DIR/pre-receive.stdin
  echo STDOUT pre-receive
  echo STDERR pre-receive 2
  EOF
  chmod u+x victim.git/hooks/pre-receive

Would you look at that? This is actually a hook test that is changing the hook
*outside* the cage.

== t/t5402-post-merge-hook.sh ==

  for clone in 1 2; do
  cat clone${clone}/.git/hooks/post-merge 'EOF'
  #!/bin/sh
  echo $@  $GIT_DIR/post-merge.args
  EOF
  chmod u+x clone${clone}/.git/hooks/post-merge
  done

Another hook test with code outside.

== t/t5403-post-checkout-hook.sh ==

Doing the same.

== t/t5516-fetch-push.sh ==

  mk_test_with_hooks() {
  repo_name=$1
  mk_test $@ 
  (
  cd $repo_name 
  mkdir .git/hooks 
  cd .git/hooks 

  cat pre-receive -'EOF' 
  #!/bin/sh
  cat - pre-receive.actual
  EOF

  cat update -'EOF' 
  #!/bin/sh
  printf %s %s %s\n $@ update.actual
  EOF

  cat post-receive -'EOF' 
  #!/bin/sh
  cat - post-receive.actual
  EOF

  cat post-update -'EOF' 
  #!/bin/sh
  for ref in $@
  do
  printf %s\n $ref post-update.actual
  done
  EOF

  chmod +x pre-receive update post-receive post-update
  )
  }

This one is using a function, just like I am. It's not run outside, but we can
do the same.

== t/t5571-pre-push-hook.sh ==

  write_script $HOOK 'EOF'
  echo $1 actual
  echo $2 actual
  cat actual
  EOF

Anhoter hook test with code outside.

== t/t7004-tag.sh ==

  cat fakeeditor 'EOF'
  #!/bin/sh
  test -n $1  exec $1
  echo A signed tag message
  echo from a fake editor.
  EOF
  chmod +x fakeeditor

== t/t7008-grep-binary.sh ==

  cat nul_to_q_textconv 'EOF'
  #!/bin/sh
  $PERL_PATH -pe 'y/\000/Q/'  $1
  EOF
  chmod +x nul_to_q_textconv

== t/t7504-commit-msg-hook.sh ==
== t/t8006-blame-textconv.sh ==
== t/t8007-cat-file-textconv.sh ==
== t/t9138-git-svn-authors-prog.sh ==

Very similar: scripts outside the cage.


In fact my version is actually cleaner than these, because the code that is run
outside the cage is clearly delimited by a function.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Felipe Contreras
Junio C Hamano wrote:
 Ilya Bobyr ilya.bo...@gmail.com writes:
 
  On 4/21/2014 2:17 PM, Felipe Contreras wrote:
  Ilya Bobyr wrote:
 
  Also, most have names that start with either pre- or post-.
  It seems reasonable for both pre-update-branch and
  post-update-branch to exist.
  I don't see what would be the point in that.
 
  Do you see the point in the other hooks doing that?
 
 pre- and post- are primarily so that people can tell that pre-
 happens before the operation and its primary motivation is to stop
 an operation from happening as opposed to post- is called after
 the fact and there is no way for it to intervene---it is too late;
 it is primarily for things like logging easily.
 
 As long as you can tell what you can use it for and when it is
 called from the name of the hook, there is no fundamental reason why
 you need to have pre- or post- prefix in your hook names, but unless
 there is no other strong reason not to, it is probably a good idea
 to follow suit.  There is not much value in trying to be original
 in naming things, just to be different; it will only confuse the
 users.

It's not original; there are _already_ hooks without pre/post. And it's not
confusing, update-branch doesn't tell much, not any hook name could, that's
what the documentation is for.

-- 
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: [RTC/PATCH] Add 'update-branch' hook

2014-04-21 Thread Junio C Hamano
Felipe Contreras felipe.contre...@gmail.com writes:

 ... there are _already_ hooks without pre/post.

Like commit-msg?  Yes, it would have been nicer if it were named
verify-commit-message or something.

Old mistakes are harder to change because of inertia.  It is not a
good excuse to knowingly make a new mistake to add new exceptions
that the users need to check documentations for, is it?

 And it's not confusing,

A simple fact that Ilya asked the question tells us otherwise ;-)

I personally do not see an immediate need for post-update-branch,
but if the new hook is about intervening an operation, it would be a
good idea to name the hook with pre- like other before doing
something, validate the operation and forbid hooks.  Otherwise it
would be impossible to later add post-update-branch for whatever
reason without inviting why does pre-update-branch hook is misnamed
as just update-branch, when other validation and post-action pair
are named pre-something and post-something?.
--
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