Re: [RTC/PATCH] Add 'update-branch' hook
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
[RTC/PATCH] Add 'update-branch' hook
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
---
Documentation/githooks.txt| 9 +
branch.c | 6 ++
builtin/clone.c | 7 +--
git-rebase--interactive.sh| 6 +-
git-rebase.sh | 6 +-
t/t5408-update-branch-hook.sh | 39 +++
6 files changed, 69 insertions(+), 4 deletions(-)
create mode 100755 t/t5408-update-branch-hook.sh
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d954bf6..9e50697 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -381,6 +381,15 @@ rebase::
The commits are guaranteed to be listed in the order that they were
processed by rebase.
+update-branch
+~
+
+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.
+
GIT
---
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);
+ 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);
}
} 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
;;
esac
