{"thread":{"id":"22659","subject":"[PATCH] require_work_tree broken with NONGIT_OK","startedAt":"2010-02-15T03:51:47Z","lastAt":"2010-02-15T15:14:30Z","messageCount":6,"participants":["Gabriel Filion","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"134596","messageId":"4B78C4D3.90407@gmail.com","threadId":"22659","inReplyTo":null,"subject":"[PATCH] require_work_tree broken with NONGIT_OK","fromName":"Gabriel Filion","fromEmail":"lelutin@gmail.com","sentAt":"2010-02-15T03:51:47Z","receivedAt":"2010-02-15T03:51:47Z","isPatch":true,"sender":{"key":"lelutin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/108728?v=4"},"body":"When sourcing git-sh-setup after having set NONGIT_OK, calling the\nfunction require_work_tree while outside of a git repository shows a\nsyntax error.\n\nThis is caused by the call to \"git rev-parse --is-inside-work-tree\"\nprinting a sentence when it is called outside of a git repository.\nRelying on the return code is better.\n---\n git-sh-setup.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex d56426d..8de2f03 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -128,7 +128,7 @@ cd_to_toplevel () {\n }\n  require_work_tree () {\n-\ttest $(git rev-parse --is-inside-work-tree) = true ||\n+\ttest git rev-parse --is-inside-work-tree >/dev/null 2>&1 ||\n \tdie \"fatal: $0 cannot be used without a working tree.\"\n }\n -- 1.6.6\n"},{"id":"134613","messageId":"20100215053404.GK3336@coredump.intra.peff.net","threadId":"22659","inReplyTo":"4B78C4D3.90407@gmail.com","subject":"Re: [PATCH] require_work_tree broken with NONGIT_OK","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T05:34:05Z","receivedAt":"2010-02-15T05:34:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 10:51:47PM -0500, Gabriel Filion wrote:\n\n> When sourcing git-sh-setup after having set NONGIT_OK, calling the\n> function require_work_tree while outside of a git repository shows a\n> syntax error.\n> \n> This is caused by the call to \"git rev-parse --is-inside-work-tree\"\n> printing a sentence when it is called outside of a git repository.\n> Relying on the return code is better.\n\nI think your fix is fine, but your analysis is slightly wrong. If we are\nnot in a work tree, the sentence goes to stderr, and nothing goes to\nstdout (which is what makes \"test\" unhappy).\n\nThis is not just a nitpick of your commit message, but I was worried\ngiven some other recent discussion that we were accidentally sending\nthat message to stdout, which would be a bug. But we're not.\n\n-Peff\n"},{"id":"134620","messageId":"7vzl3bj95l.fsf@alter.siamese.dyndns.org","threadId":"22659","inReplyTo":"4B78C4D3.90407@gmail.com","subject":"Re: [PATCH] require_work_tree broken with NONGIT_OK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-15T06:38:30Z","receivedAt":"2010-02-15T06:38:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gabriel Filion <lelutin@gmail.com> writes:\n\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index d56426d..8de2f03 100755\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -128,7 +128,7 @@ cd_to_toplevel () {\n>  }\n>   require_work_tree () {\n> -\ttest $(git rev-parse --is-inside-work-tree) = true ||\n\nThis needs to have dq around it, as \"Not a git repository\" case we fatal\nout without any output, like this:\n\n\ttest \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" = true ||\n\n\n> +\ttest git rev-parse --is-inside-work-tree >/dev/null 2>&1 ||\n\nI don't think this would ever work with \"test\" at the beginning.\n\n>  \tdie \"fatal: $0 cannot be used without a working tree.\"\n"},{"id":"134623","messageId":"4B78F6CB.2070304@gmail.com","threadId":"22659","inReplyTo":"7vzl3bj95l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] require_work_tree broken with NONGIT_OK","fromName":"Gabriel Filion","fromEmail":"lelutin@gmail.com","sentAt":"2010-02-15T07:24:59Z","receivedAt":"2010-02-15T07:24:59Z","isPatch":true,"sender":{"key":"lelutin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/108728?v=4"},"body":"On 2010-02-15 01:38, Junio C Hamano wrote:\n> Gabriel Filion <lelutin@gmail.com> writes:\n> \n>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n>> index d56426d..8de2f03 100755\n>> --- a/git-sh-setup.sh\n>> +++ b/git-sh-setup.sh\n>> @@ -128,7 +128,7 @@ cd_to_toplevel () {\n>>  }\n>>   require_work_tree () {\n>> -\ttest $(git rev-parse --is-inside-work-tree) = true ||\n> \n> This needs to have dq around it, as \"Not a git repository\" case we fatal\n> out without any output, like this:\n> \n> \ttest \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" = true ||\n> \n>> +\ttest git rev-parse --is-inside-work-tree >/dev/null 2>&1 ||\n> \n> I don't think this would ever work with \"test\" at the beginning.\n> \nWell, it would seem you are right! my bad for thinking that \"test\" was\nactually evaluating any command given in the expression :\\\n\nYour implementation seems to be working better and fixes the problem.\nThanks for reviewing the patch.\n\n-- \nGabriel Filion\n"},{"id":"134625","messageId":"20100215074922.GA5549@coredump.intra.peff.net","threadId":"22659","inReplyTo":"7vzl3bj95l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] require_work_tree broken with NONGIT_OK","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T07:49:22Z","receivedAt":"2010-02-15T07:49:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 10:38:30PM -0800, Junio C Hamano wrote:\n\n> > +\ttest git rev-parse --is-inside-work-tree >/dev/null 2>&1 ||\n> \n> I don't think this would ever work with \"test\" at the beginning.\n\nOops. I totally missed that when reviewing the patch. :-/\n\nThinking on this a bit more, I think Gabriel's script is a little\nbroken. It sets NONGIT_OK to not have a git repository, but then it\nrequires a working tree, which doesn't make any sense.\n\nThat being said, I think it is still a good change, as the correct error\nmessage is better than the shell barfing.\n\n-Peff\n"},{"id":"134644","messageId":"4B7964D6.1030403@gmail.com","threadId":"22659","inReplyTo":"20100215074922.GA5549@coredump.intra.peff.net","subject":"Re: [PATCH] require_work_tree broken with NONGIT_OK","fromName":"Gabriel Filion","fromEmail":"lelutin@gmail.com","sentAt":"2010-02-15T15:14:30Z","receivedAt":"2010-02-15T15:14:30Z","isPatch":true,"sender":{"key":"lelutin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/108728?v=4"},"body":"On 2010-02-15 02:49, Jeff King wrote:\n> Thinking on this a bit more, I think Gabriel's script is a little\n> broken. It sets NONGIT_OK to not have a git repository, but then it\n> requires a working tree, which doesn't make any sense.\n> \nI hit this bug while working on a script called git-bzr over at\nhttp://github.com/kfish/git-bzr when trying to use git-sh-setup to avoid\nreinventing the wheel.\n\nMost commands in there need to be run inside a git repository and some\ndon't. I was trying find out how to implemnt \"git bzr clone\", which for\nobvious reasons should not require a work tree..\n\nplus, I thought requiring presence in a work tree to display help\nmessages was not a very user-friendly concept (this was git-bzr's\nbehaviour some days ago).\n\nI'm thinking of changing things to use git-sh-setup by splitting the\nscript into non-work-tree-requiring commands in the main script and\ncommands requiring a work tree in another sub-script.\n\nWell, all this to simply illustrate possible use cases.\n\nI'll surely be opening another discussion about git-bzr pretty soon to\nsee if people would be interested in helping out.\n\nthanks again to both of you.\n\n-- \nGabriel Filion\n"}]}