* Issue when changing staged files in a pre-commit hook @ 2015-12-28 12:17 Niek van der Kooy 2016-01-19 11:53 ` Niek van der Kooy 2016-01-19 12:20 ` Duy Nguyen 0 siblings, 2 replies; 9+ messages in thread From: Niek van der Kooy @ 2015-12-28 12:17 UTC (permalink / raw) To: git I am having an issue where the default commit message is not changed when adding staged files in a pre-commit hook. Please see http://stackoverflow.com/questions/34492779/git-update-git-status-part-of-commit-message-after-pre-commit-hook for details. Is there a workaround / proper way to achieve what I am trying to do, or is this a bug? Regards, Niek van der Kooy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2015-12-28 12:17 Issue when changing staged files in a pre-commit hook Niek van der Kooy @ 2016-01-19 11:53 ` Niek van der Kooy 2016-01-19 12:20 ` Duy Nguyen 1 sibling, 0 replies; 9+ messages in thread From: Niek van der Kooy @ 2016-01-19 11:53 UTC (permalink / raw) To: git A few weeks ago I sent below email, and was wondering if anyone can either confirm this as a bug or tell me the correct way to approach the problem. Regards, Niek van der Kooy On 28 December 2015 at 13:17, Niek van der Kooy <niekvanderkooy@gmail.com> wrote: > I am having an issue where the default commit message is not changed > when adding staged files in a pre-commit hook. > Please see http://stackoverflow.com/questions/34492779/git-update-git-status-part-of-commit-message-after-pre-commit-hook > for details. > > Is there a workaround / proper way to achieve what I am trying to do, > or is this a bug? > > Regards, > Niek van der Kooy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2015-12-28 12:17 Issue when changing staged files in a pre-commit hook Niek van der Kooy 2016-01-19 11:53 ` Niek van der Kooy @ 2016-01-19 12:20 ` Duy Nguyen 2016-01-19 18:44 ` Junio C Hamano 1 sibling, 1 reply; 9+ messages in thread From: Duy Nguyen @ 2016-01-19 12:20 UTC (permalink / raw) To: Niek van der Kooy; +Cc: Git Mailing List On Mon, Dec 28, 2015 at 7:17 PM, Niek van der Kooy <niekvanderkooy@gmail.com> wrote: > I am having an issue where the default commit message is not changed > when adding staged files in a pre-commit hook. > Please see http://stackoverflow.com/questions/34492779/git-update-git-status-part-of-commit-message-after-pre-commit-hook > for details. you should probably copy the text here instead of putting a link. Sometimes I don't bother clicking another link. Make it easy for people to help you. Long story short, pre-commit hook adds a file, which ends up in the commit, but is not shown in git-status. > Is there a workaround / proper way to achieve what I am trying to do, > or is this a bug? When you commit something, git may prepare a temporary index that contains what's to be in the commit, unless you do "git commit" without -a/-A, or paths... The pre-commit is set up to see this index. So if you make changes in this index, they will end up in the new commit. But the index will then be discarded. The main index, $GIT_DIR/index, may or may not be untouched the whole time. This is the index that is used by "git status". If you do "git commit" without -a/-A/paths they "git status" should be consistent because your hook sees $GIT_DIR/index instead of a temporary one. I think it's the intended behavior. I believe $GIT_DIR is exported to your hook, so in theory you could do an extra "GIT_INDEX_FILE=$GIT_DIR/index git add public" (i.e. you update _two_ index files, one for committing, one for future git commands). But be careful. It may have unwanted effects. It's probably easier to make an alias that adds that 'public' file first then commits without hooks. -- Duy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2016-01-19 12:20 ` Duy Nguyen @ 2016-01-19 18:44 ` Junio C Hamano 2016-01-19 23:26 ` Duy Nguyen 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2016-01-19 18:44 UTC (permalink / raw) To: Duy Nguyen; +Cc: Niek van der Kooy, Git Mailing List Duy Nguyen <pclouds@gmail.com> writes: > I think it's the intended behavior. Yeah, pre-commit was designed for inspecting and rejecting, not for tweaking and munging. Perhaps "git commit" can be tightened to make sure that pre-commit that returns successfully did not muck with the working tree and the index? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2016-01-19 18:44 ` Junio C Hamano @ 2016-01-19 23:26 ` Duy Nguyen 2016-01-19 23:41 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Duy Nguyen @ 2016-01-19 23:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Niek van der Kooy, Git Mailing List On Wed, Jan 20, 2016 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote: > Duy Nguyen <pclouds@gmail.com> writes: > >> I think it's the intended behavior. > > Yeah, pre-commit was designed for inspecting and rejecting, not for > tweaking and munging. Perhaps "git commit" can be tightened to make > sure that pre-commit that returns successfully did not muck with the > working tree and the index? That was my impression from the docs, but then I saw this comment, /* * Re-read the index as pre-commit hook could have updated it, * and write it out as a tree. We must do this before we invoke * the editor and after we invoke run_status above. */ which comes from 2888605 (builtin-commit: fix partial-commit support - 2007-11-18) that admits "the hook can modify it (the index)". And I was about to update the docs, but the other way around, about updating index and side effects. -- Duy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2016-01-19 23:26 ` Duy Nguyen @ 2016-01-19 23:41 ` Junio C Hamano 2016-01-20 0:23 ` Duy Nguyen 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2016-01-19 23:41 UTC (permalink / raw) To: Duy Nguyen; +Cc: Niek van der Kooy, Git Mailing List Duy Nguyen <pclouds@gmail.com> writes: > On Wed, Jan 20, 2016 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote: >> Duy Nguyen <pclouds@gmail.com> writes: >> >>> I think it's the intended behavior. >> >> Yeah, pre-commit was designed for inspecting and rejecting, not for >> tweaking and munging. Perhaps "git commit" can be tightened to make >> sure that pre-commit that returns successfully did not muck with the >> working tree and the index? > > That was my impression from the docs, but then I saw this comment, > > /* > * Re-read the index as pre-commit hook could have updated it, > * and write it out as a tree. We must do this before we invoke > * the editor and after we invoke run_status above. > */ > > which comes from 2888605 (builtin-commit: fix partial-commit support - > 2007-11-18) that admits "the hook can modify it (the index)". And I > was about to update the docs, but the other way around, about updating > index and side effects. I do not think the comment is wrong per-se, but the code we have, either back then or even today, is insufficient to allow pre-commit hook that mucks with the fake index that is shown to it. Re-reading the in-core index at that point may help creating a commit whose tree matches what the hook did, but the extra change made by the hook is not ported forward in the real index that the user will use after a partial commit (and there is no easy way to do so cleanly--the change the hook makes may even overlap the change in the real index that are added but left uncommitted, and you would end up needing to run a threeway merge). The only sensible thing we can do at that point in the code after re-reading the index is to make sure that hasn't been changed by the pre-commit hook and complain loudly to die if we find it modified, I think. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2016-01-19 23:41 ` Junio C Hamano @ 2016-01-20 0:23 ` Duy Nguyen 2016-01-20 1:44 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Duy Nguyen @ 2016-01-20 0:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Niek van der Kooy, Git Mailing List On Wed, Jan 20, 2016 at 6:41 AM, Junio C Hamano <gitster@pobox.com> wrote: > Duy Nguyen <pclouds@gmail.com> writes: > >> On Wed, Jan 20, 2016 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote: >>> Duy Nguyen <pclouds@gmail.com> writes: >>> >>>> I think it's the intended behavior. >>> >>> Yeah, pre-commit was designed for inspecting and rejecting, not for >>> tweaking and munging. Perhaps "git commit" can be tightened to make >>> sure that pre-commit that returns successfully did not muck with the >>> working tree and the index? >> >> That was my impression from the docs, but then I saw this comment, >> >> /* >> * Re-read the index as pre-commit hook could have updated it, >> * and write it out as a tree. We must do this before we invoke >> * the editor and after we invoke run_status above. >> */ >> >> which comes from 2888605 (builtin-commit: fix partial-commit support - >> 2007-11-18) that admits "the hook can modify it (the index)". And I >> was about to update the docs, but the other way around, about updating >> index and side effects. > > I do not think the comment is wrong per-se, but the code we have, > either back then or even today, is insufficient to allow pre-commit > hook that mucks with the fake index that is shown to it. > > Re-reading the in-core index at that point may help creating a > commit whose tree matches what the hook did, but the extra change > made by the hook is not ported forward in the real index that the > user will use after a partial commit (and there is no easy way to do > so cleanly--the change the hook makes may even overlap the change in > the real index that are added but left uncommitted, and you would > end up needing to run a threeway merge). > > The only sensible thing we can do at that point in the code after > re-reading the index is to make sure that hasn't been changed by the > pre-commit hook and complain loudly to die if we find it modified, I > think. OK. Two more points - do we catch index changes for partial commit case only? Because when $GIT_DIR/index is used, we do not have this problem. - is Niek's use case still valid? If so, what do we do to support it (at least in partial commit case)? -- Duy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2016-01-20 0:23 ` Duy Nguyen @ 2016-01-20 1:44 ` Junio C Hamano 2016-01-22 8:58 ` Duy Nguyen 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2016-01-20 1:44 UTC (permalink / raw) To: Duy Nguyen; +Cc: Niek van der Kooy, Git Mailing List Duy Nguyen <pclouds@gmail.com> writes: > On Wed, Jan 20, 2016 at 6:41 AM, Junio C Hamano <gitster@pobox.com> wrote: > >> The only sensible thing we can do at that point in the code after >> re-reading the index is to make sure that hasn't been changed by the >> pre-commit hook and complain loudly to die if we find it modified, I >> think. > > OK. Two more points > > - do we catch index changes for partial commit case only? Because > when $GIT_DIR/index is used, we do not have this problem. I do not think you are correct. As-is commit with un-added changes in the working tree would have the same problem. If the hook munged the real index without touching the working tree, the next "commit -a" would revert that change unless you reflect the same munging the hook did to the working tree. So if you _really_ wanted to be lenient and allow hooks to muck with the index, you would have to do something along this line: - Take a snapshot of the working tree files (call it W0). - Prepare the false-index for partial commit if necessary. - Write-tree the index used to record the next commit (call the result T0) before calling the hook and showing that index to it. - Let the hook munge the index (and possibly the working tree. - Re-read the index and write-tree (call the result T1). - Take a snapshot of the working tree files (call it W1). - If T1 != T0: - If making a partial commit - apply diff(T0,T1) to the real index. - apply diff(T0,T1) - diff(W0,W1) to the working tree files. - Otherwise - apply diff(T0,T1) - diff(W0,W1) to the working tree files. And if any of the "apply the change" fails, fail the whole thing without leaving any damage. As the index and the HEAD after making a partial commit is allowed to be different, we cannot resolve the 3-way merge conflict using the index the usual way. And if all of the "apply the change" succeeds (including the easier cases where W0 == W1), allow such a change by the hook. That would give us a more lenient system, but I do not necessarily think that it would be a good thing. Think what the error message has to say when "if any of the apply fails" case. "Commit failed because your hook munged X in such and such way" (where X might be the index, the working tree, or both, and "such and such way" would describe the overlapping changes that cannot be automatically reconciled). What is the impression such a message give the user? A hook is allowed to muck with the index only under some complex condition but not always? Surely there is a solid technical reason why it cannot be allowed, but is that a rule that is easy to understand by end users? Wouldn't it be simpler if the rule the user has to remember is "pre-X hook are designed to inspect and reject, not to tweak and munge" (which has always been the case as far as I am concerned, and the fact that the code trusted hooks too much to follow the rule and did not verify was merely a bug) and rejected a hook that munged the index when it did so, regardless of the state of the index and the working tree when the commit command is run and the kind of commit (either as-is or partial) that we are creating? > - is Niek's use case still valid? If so, what do we do to support it > (at least in partial commit case)? Especially in partial commit case, but more in general in any case, I'd say the result is "undefined". I think the _only_ case we can safely port forward the munging done by the hook is during "git commit -a". In that case, we know that the working tree exactly matches the index being committed before the hook runs, and after the hook munged the index, regardless of what it did (or didn't) do to the working tree, the resulting HEAD, the index, and the working tree ought to match, so porting forward is just a matter of running "reset --hard" when we notice that the hook munged the contents to be committed with "commit -a". All other cases can create conflicts that we cannot express to the user to resolve, and will introuce "hook is allowed to muck with the index only under some complex condition but not always" confusion. The above does not have to be the final verdict, though. Somebody else may able to come up with a more lenient behaviour that is easy to understand by end users (I somehow doubt about the latter; coming up with "a more lenient" behaviour is easy, and anybody who hacks up such a behaviour may argue that the behaviour is easy to understand, but that is merely due to knowing the implementation--explaining it to the end users is a different matter). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Issue when changing staged files in a pre-commit hook 2016-01-20 1:44 ` Junio C Hamano @ 2016-01-22 8:58 ` Duy Nguyen 0 siblings, 0 replies; 9+ messages in thread From: Duy Nguyen @ 2016-01-22 8:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: Niek van der Kooy, Git Mailing List On Tue, Jan 19, 2016 at 05:44:22PM -0800, Junio C Hamano wrote: > Duy Nguyen <pclouds@gmail.com> writes: > > > On Wed, Jan 20, 2016 at 6:41 AM, Junio C Hamano <gitster@pobox.com> wrote: > > > >> The only sensible thing we can do at that point in the code after > >> re-reading the index is to make sure that hasn't been changed by the > >> pre-commit hook and complain loudly to die if we find it modified, I > >> think. > > > > OK. Two more points > > > > - do we catch index changes for partial commit case only? Because > > when $GIT_DIR/index is used, we do not have this problem. > > I do not think you are correct. As-is commit with un-added changes > in the working tree would have the same problem. ..... OK you convinced me that catching pre-commit modifying the index is the best option so far. We can do something like this. You'll have to put in the commit message though, you explain it much better than me. No document update is necessary because the user will be informed why git-commit fails. -- 8< -- diff --git a/builtin/commit.c b/builtin/commit.c index d054f84..f9f4957 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -656,6 +656,7 @@ static void adjust_comment_line_char(const struct strbuf *sb) } static int prepare_to_commit(const char *index_file, const char *prefix, + const unsigned char *tree_sha1, struct commit *current_head, struct wt_status *s, struct strbuf *author_ident) @@ -928,9 +929,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix, } /* - * Re-read the index as pre-commit hook could have updated it, - * and write it out as a tree. We must do this before we invoke - * the editor and after we invoke run_status above. + * Re-read the index and check if pre-commit hook has updated it, + * We must do this before we invoke the editor and after we + * invoke run_status above. */ discard_cache(); read_cache_from(index_file); @@ -938,6 +939,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix, error(_("Error building trees")); return 0; } + if (hashcmp(active_cache_tree->sha1, tree_sha1)) { + error(_("pre-commit hook has changed commit content")); + return 0; + } if (run_commit_hook(use_editor, index_file, "prepare-commit-msg", git_path(commit_editmsg), hook_arg1, hook_arg2, NULL)) @@ -1636,6 +1641,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) struct commit_extra_header *extra = NULL; struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; + unsigned char tree_sha1[GIT_SHA1_RAWSZ]; if (argc == 2 && !strcmp(argv[1], "-h")) usage_with_options(builtin_commit_usage, builtin_commit_options); @@ -1657,10 +1663,16 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (dry_run) return dry_run_commit(argc, argv, prefix, current_head, &s); index_file = prepare_index(argc, argv, prefix, current_head, 0); + if (update_main_cache_tree(0)) { + error(_("Error building trees")); + rollback_index_files(); + return 1; + } + hashcpy(tree_sha1, active_cache_tree->sha1); /* Set up everything for writing the commit object. This includes running hooks, writing the trees, and interacting with the user. */ - if (!prepare_to_commit(index_file, prefix, + if (!prepare_to_commit(index_file, prefix, tree_sha1, current_head, &s, &author_ident)) { rollback_index_files(); return 1; @@ -1749,7 +1761,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) append_merge_tag_headers(parents, &tail); } - if (commit_tree_extended(sb.buf, sb.len, active_cache_tree->sha1, + if (commit_tree_extended(sb.buf, sb.len, tree_sha1, parents, sha1, author_ident.buf, sign_commit, extra)) { rollback_index_files(); die(_("failed to write commit object")); diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh index 984889b..e6b5979 100755 --- a/t/t7503-pre-commit-hook.sh +++ b/t/t7503-pre-commit-hook.sh @@ -136,4 +136,12 @@ test_expect_success 'check the author in hook' ' git show -s ' +test_expect_success 'catch pre-commit hooks that modify index' ' + echo modified >>file && + write_script "$HOOK" <<-\EOF && + git rm -f file + EOF + test_must_fail git commit -m m file +' + test_done -- 8< -- ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-01-22 8:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-28 12:17 Issue when changing staged files in a pre-commit hook Niek van der Kooy 2016-01-19 11:53 ` Niek van der Kooy 2016-01-19 12:20 ` Duy Nguyen 2016-01-19 18:44 ` Junio C Hamano 2016-01-19 23:26 ` Duy Nguyen 2016-01-19 23:41 ` Junio C Hamano 2016-01-20 0:23 ` Duy Nguyen 2016-01-20 1:44 ` Junio C Hamano 2016-01-22 8:58 ` Duy Nguyen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.