{"thread":{"id":"44601","subject":"[PATCHv1 0/2] git-p4 patches","startedAt":"2016-12-02T22:43:34Z","lastAt":"2016-12-05T23:17:36Z","messageCount":9,"participants":["Luke Diamand","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"306824","messageId":"20161202224319.5385-1-luke@diamand.org","threadId":"44601","inReplyTo":null,"subject":"[PATCHv1 0/2] git-p4 patches","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2016-12-02T22:43:17Z","receivedAt":"2016-12-02T22:43:34Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This is a couple of small patches for git-p4.\n\nThe first just teaches git-p4 about git workspaces, so that you can\ndo \"git p4 submit\" from within a workspace (P.S. workspaces\nare completely *awesome*).\n\nThe second follows on from the work by Vinicius for shelving\nchangelists, by letting you update an existing shelved changelist.\n\nThis is a bit like using \"git commit --amend\" only far more painful....\n\nLuke Diamand (2):\n git-p4: support git-workspaces\n git-p4: support updating an existing shelved changelist\n\n Documentation/git-p4.txt | 4 ++++\n git-p4.py | 39 +++++++++++++++++++++++++++++++++++----\n t/t9807-git-p4-submit.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 77 insertions(+), 4 deletions(-)\n\n-- \n2.11.0.274.g0ea315c\n\n"},{"id":"306825","messageId":"20161202224319.5385-2-luke@diamand.org","threadId":"44601","inReplyTo":"20161202224319.5385-1-luke@diamand.org","subject":"[PATCHv1 1/2] git-p4: support git-workspaces","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2016-12-02T22:43:18Z","receivedAt":"2016-12-02T22:43:41Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Teach git-p4 about git-workspaces.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 0c4f2afd2..5e2db1919 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -566,6 +566,12 @@ def isValidGitDir(path):\n if (os.path.exists(path + \"/HEAD\")\n and os.path.exists(path + \"/refs\") and os.path.exists(path + \"/objects\")):\n return True;\n+\n+ # git workspace directory?\n+ if (os.path.exists(path + \"/HEAD\")\n+ and os.path.exists(path + \"/gitdir\")):\n+ return True\n+\n return False\n \n def parseRevision(ref):\n-- \n2.11.0.274.g0ea315c\n\n"},{"id":"306826","messageId":"20161202224319.5385-3-luke@diamand.org","threadId":"44601","inReplyTo":"20161202224319.5385-1-luke@diamand.org","subject":"[PATCHv1 2/2] git-p4: support updating an existing shelved changelist","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2016-12-02T22:43:19Z","receivedAt":"2016-12-02T22:43:44Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Adds new option \"--update-shelve CHANGELIST\" which updates\nan existing shelved changelist.\n\nThe original changelist must have been created by the current user.\n\nThis allows workflow something like:\n\n hack hack hack\n git commit\n git p4 submit --shelve\n $mail interested parties about shelved changelist\n make corrections\n git commit --amend\n git p4 submit --update-shelve $CHANGELIST\n $mail interested parties about shelved changelist\n etc\n\nSigned-off-by: Luke Diamand \n---\n Documentation/git-p4.txt | 4 ++++\n git-p4.py | 33 +++++++++++++++++++++++++++++----\n t/t9807-git-p4-submit.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 71 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex 1bbf43d15..ce40b9a54 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -308,6 +308,10 @@ These options can be used to modify 'git p4 submit' behavior.\n \tAfter creating each shelve, the relevant files are reverted/deleted.\n \tIf you have multiple commits pending multiple shelves will be created.\n \n+--update-shelve CHANGELIST::\n+\tUpdate an existing shelved changelist with this commit. Implies\n+\t--shelve.\n+\n --conflict=(ask|skip|quit)::\n \tConflicts can occur when applying a commit to p4. When this\n \thappens, the default behavior (\"ask\") is to prompt whether to\ndiff --git a/git-p4.py b/git-p4.py\nindex 5e2db1919..36242d384 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -262,6 +262,10 @@ def p4_revert(f):\n def p4_reopen(type, f):\n p4_system([\"reopen\", \"-t\", type, wildcard_encode(f)])\n \n+def p4_reopen_in_change(changelist, files):\n+ cmd = [\"reopen\", \"-c\", str(changelist)] + files\n+ p4_system(cmd)\n+\n def p4_move(src, dest):\n p4_system([\"move\", \"-k\", wildcard_encode(src), wildcard_encode(dest)])\n \n@@ -1298,6 +1302,9 @@ class P4Submit(Command, P4UserMap):\n optparse.make_option(\"--shelve\", dest=\"shelve\", action=\"store_true\",\n help=\"Shelve instead of submit. Shelved files are reverted, \"\n \"restoring the workspace to the state before the shelve\"),\n+ optparse.make_option(\"--update-shelve\", dest=\"update_shelve\", action=\"store\", type=\"int\",\n+ metavar=\"CHANGELIST\",\n+ help=\"update an existing shelved changelist, implies --shelve\")\n ]\n self.description = \"Submit changes from git to the perforce depot.\"\n self.usage += \" [name of git branch to submit into perforce depot]\"\n@@ -1306,6 +1313,7 @@ class P4Submit(Command, P4UserMap):\n self.preserveUser = gitConfigBool(\"git-p4.preserveUser\")\n self.dry_run = False\n self.shelve = False\n+ self.update_shelve = None\n self.prepare_p4_only = False\n self.conflict_behavior = None\n self.isWindows = (platform.system() == \"Windows\")\n@@ -1474,7 +1482,7 @@ class P4Submit(Command, P4UserMap):\n return 1\n return 0\n \n- def prepareSubmitTemplate(self):\n+ def prepareSubmitTemplate(self, changelist=None):\n \"\"\"Run \"p4 change -o\" to grab a change specification template.\n This does not use \"p4 -G\", as it is nice to keep the submission\n template in original order, since a human might edit it.\n@@ -1486,7 +1494,11 @@ class P4Submit(Command, P4UserMap):\n \n template = \"\"\n inFilesSection = False\n- for line in p4_read_pipe_lines(['change', '-o']):\n+ args = ['change', '-o']\n+ if changelist:\n+ args.append(str(changelist))\n+\n+ for line in p4_read_pipe_lines(args):\n if line.endswith(\"\\r\\n\"):\n line = line[:-2] + \"\\n\"\n if inFilesSection:\n@@ -1585,11 +1597,14 @@ class P4Submit(Command, P4UserMap):\n editedFiles = set()\n pureRenameCopy = set()\n filesToChangeExecBit = {}\n+ all_files = list()\n \n for line in diff:\n diff = parseDiffTreeEntry(line)\n modifier = diff['status']\n path = diff['src']\n+ all_files.append(path)\n+\n if modifier == \"M\":\n p4_edit(path)\n if isModeExecChanged(diff['src_mode'], diff['dst_mode']):\n@@ -1715,6 +1730,10 @@ class P4Submit(Command, P4UserMap):\n mode = filesToChangeExecBit[f]\n setP4ExecBit(f, mode)\n \n+ if self.update_shelve:\n+ print(\"all_files = %s\" % str(all_files))\n+ p4_reopen_in_change(self.update_shelve, all_files)\n+\n #\n # Build p4 change description, starting with the contents\n # of the git commit message.\n@@ -1723,7 +1742,7 @@ class P4Submit(Command, P4UserMap):\n logMessage = logMessage.strip()\n (logMessage, jobs) = self.separate_jobs_from_description(logMessage)\n \n- template = self.prepareSubmitTemplate()\n+ template = self.prepareSubmitTemplate(self.update_shelve)\n submitTemplate = self.prepareLogMessage(template, logMessage, jobs)\n \n if self.preserveUser:\n@@ -1795,7 +1814,10 @@ class P4Submit(Command, P4UserMap):\n if self.isWindows:\n message = message.replace(\"\\r\\n\", \"\\n\")\n submitTemplate = message[:message.index(separatorLine)]\n- if self.shelve:\n+\n+ if self.update_shelve:\n+ p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)\n+ elif self.shelve:\n p4_write_pipe(['shelve', '-i'], submitTemplate)\n else:\n p4_write_pipe(['submit', '-i'], submitTemplate)\n@@ -1921,6 +1943,9 @@ class P4Submit(Command, P4UserMap):\n if len(self.origin) == 0:\n self.origin = upstream\n \n+ if self.update_shelve:\n+ self.shelve = True\n+\n if self.preserveUser:\n if not self.canChangeChangelists():\n die(\"Cannot preserve user names without p4 super-user or admin permissions\")\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 42a5fada5..e37239e65 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -444,6 +444,44 @@ test_expect_success 'submit --shelve' '\n \t)\n '\n \n+# Update an existing shelved changelist\n+\n+test_expect_success 'submit --update-shelve' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 revert ... &&\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\ttest_commit \"test-update-shelved-change\" &&\n+\t\tgit p4 submit --origin=HEAD^ --shelve &&\n+\n+\t\tshelf_cl=$(p4 -G changes -s shelved -m 1 |\\\n+\t\t\tmarshal_dump change) &&\n+\t\ttest -n $shelf_cl &&\n+\t\techo \"updating shelved change list $shelf_cl\" &&\n+\n+\t\techo \"updated-line\" >>shelf.t &&\n+\t\techo added-file.t >added-file.t &&\n+\t\tgit add shelf.t added-file.t &&\n+\t\tgit rm -f test-update-shelved-change.t &&\n+\t\tgit commit --amend -C HEAD &&\n+\t\tgit show --stat HEAD &&\n+\t\tgit p4 submit -v --origin HEAD^ --update-shelve $shelf_cl &&\n+\t\techo \"done git p4 submit\"\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tchange=$(p4 -G changes -s shelved -m 1 //depot/... | \\\n+\t\t\t marshal_dump change) &&\n+\t\tp4 unshelve -c $change -s $change &&\n+\t\tgrep -q updated-line shelf.t &&\n+\t\tp4 describe -S $change | grep added-file.t &&\n+\t\ttest_path_is_missing test-update-shelved-change.t\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n2.11.0.274.g0ea315c\n\n"},{"id":"306962","messageId":"xmqq8tru3xom.fsf@gitster.mtv.corp.google.com","threadId":"44601","inReplyTo":"20161202224319.5385-2-luke@diamand.org","subject":"Re: [PATCHv1 1/2] git-p4: support git-workspaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T20:53:13Z","receivedAt":"2016-12-05T20:53:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> Teach git-p4 about git-workspaces.\n\nIs this what we call \"git worktree\", or something else?\n\n>\n> Signed-off-by: Luke Diamand \n> ---\n> git-p4.py | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 0c4f2afd2..5e2db1919 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -566,6 +566,12 @@ def isValidGitDir(path):\n> if (os.path.exists(path + \"/HEAD\")\n> and os.path.exists(path + \"/refs\") and os.path.exists(path + \"/objects\")):\n> return True;\n> +\n> + # git workspace directory?\n> + if (os.path.exists(path + \"/HEAD\")\n> + and os.path.exists(path + \"/gitdir\")):\n> + return True\n> +\n> return False\n> \n> def parseRevision(ref):\n"},{"id":"306963","messageId":"CAE5ih78Y_AbfgtW_6zMKLC8NzBxCKSagrgrjtfWZVOEwaAg6ZA@mail.gmail.com","threadId":"44601","inReplyTo":"xmqq8tru3xom.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv1 1/2] git-p4: support git-workspaces","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2016-12-05T21:13:22Z","receivedAt":"2016-12-05T21:14:15Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 5 December 2016 at 20:53, Junio C Hamano wrote:\n> Luke Diamand writes:\n>\n>> Teach git-p4 about git-workspaces.\n>\n> Is this what we call \"git worktree\", or something else?\n\nAh, I think you're right!\n\n>\n>>\n>> Signed-off-by: Luke Diamand \n>> ---\n>> git-p4.py | 6 ++++++\n>> 1 file changed, 6 insertions(+)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 0c4f2afd2..5e2db1919 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -566,6 +566,12 @@ def isValidGitDir(path):\n>> if (os.path.exists(path + \"/HEAD\")\n>> and os.path.exists(path + \"/refs\") and os.path.exists(path + \"/objects\")):\n>> return True;\n>> +\n>> + # git workspace directory?\n>> + if (os.path.exists(path + \"/HEAD\")\n>> + and os.path.exists(path + \"/gitdir\")):\n>> + return True\n>> +\n>> return False\n>>\n>> def parseRevision(ref):\n"},{"id":"306965","messageId":"xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com","threadId":"44601","inReplyTo":"CAE5ih78Y_AbfgtW_6zMKLC8NzBxCKSagrgrjtfWZVOEwaAg6ZA@mail.gmail.com","subject":"Re: [PATCHv1 1/2] git-p4: support git-workspaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T21:24:36Z","receivedAt":"2016-12-05T21:24:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> On 5 December 2016 at 20:53, Junio C Hamano wrote:\n>> Luke Diamand writes:\n>>\n>>> Teach git-p4 about git-workspaces.\n>>\n>> Is this what we call \"git worktree\", or something else?\n>\n> Ah, I think you're right!\n\nThen I'll queue it like the attached.\n\nHOWEVER.\n\nHow fast does isValidGitDir() function need to be? The primary one\nseems to check HEAD (but it does not notice a non-repository that\nhas a directory with that name), refs and objects (but it does not\nnotice a non-repository that has a non-directory with these names),\nand this new one uses a test that is even more sloppy.\n\nWhat I am trying to get at is if we want to use a single command\nthat can be given a path and answer \"Yes, that is a repository\"\nhere, and that single command should know how the repository should\nlook like. I offhand do not know we already have such a command we\ncan use, e.g. \"git rev-parse --is-git-dir $path\", but if there isn't\nperhaps we would want one, so that not just \"git p4\" but other\nscripted Porcelains can make use of it?\n\n-- >8 --\nFrom: Luke Diamand \nDate: Fri, 2 Dec 2016 22:43:18 +0000\nSubject: [PATCH] git-p4: support secondary working trees managed by \"git worktree\"\n\nTeach git-p4 about them.\n\nSigned-off-by: Luke Diamand \nSigned-off-by: Junio C Hamano \n---\n git-p4.py | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex fd5ca52462..b3c50ae7e5 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -566,6 +566,12 @@ def isValidGitDir(path):\n if (os.path.exists(path + \"/HEAD\")\n and os.path.exists(path + \"/refs\") and os.path.exists(path + \"/objects\")):\n return True;\n+\n+ # secondary working tree managed by \"git worktree\"?\n+ if (os.path.exists(path + \"/HEAD\")\n+ and os.path.exists(path + \"/gitdir\")):\n+ return True\n+\n return False\n \n def parseRevision(ref):\n-- \n2.11.0-222-g22b1346184\n\n"},{"id":"306966","messageId":"xmqqzika2ga5.fsf@gitster.mtv.corp.google.com","threadId":"44601","inReplyTo":"xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv1 1/2] git-p4: support git-workspaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T21:54:26Z","receivedAt":"2016-12-05T21:54:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Luke Diamand writes:\n> ...\n> if (os.path.exists(path + \"/HEAD\")\n> and os.path.exists(path + \"/refs\") and os.path.exists(path + \"/objects\")):\n> return True;\n> +\n> + # secondary working tree managed by \"git worktree\"?\n> + if (os.path.exists(path + \"/HEAD\")\n> + and os.path.exists(path + \"/gitdir\")):\n> + return True\n> +\n> return False\n\nWait a bit. How is this expected to work?\n\nI assume \"path\" is something like \"$somewhere/.git\" (either\nabsolute or relative), and by concatenating HEAD, refs and objects\nunder it the original checks form paths to expected directories,\nbecause \"$somewhere/.git\" is a directory.\n\nA worktree created with \"git worktree add\" gets \".git\" that is a\nfile, and the contents of the file records the path to a\nsubdirectory \"worktrees/$name\" of the primary repository the\nworktree is borrowing from, i.e. \"$somewhere/.git/worktrees/$name\".\n\nSo for this patch to work correctly, the caller, when in a \"git\nworktree\" managed secondary working tree, MUST have found the \".git\"\nfile at the root level of the working tree, MUST have read its\ncontents to learn that \"$somewhere/.git/worktrees/$name\" path, and\nthen feeding it to this function. But doesn't such a caller already\nknow that it is a valid repository? After all, it trusted the\ncontents of that \".git\" file at the root level.\n\n ... goes and looks at the callers ...\n\nBetween the two call sites of isValidGitDir() in main(), the first\none (i.e. if \".git\" in the current directory, made into abspath,\ndoes not look like a repository, ask \"rev-parse --git-dir\" for one)\nlooks correct and I think it would grab the correct $GIT_DIR just\nfine [*1*]. Isn't the real problem the second one, i.e. the one\nthat feeds a correct cmd.gitdir that we just obtained by asking\n\"rev-parse --git-dir\" to the sloppy isValidGitDir() again. The\nlatter second guesses \"rev-parse --git-dir\" and botches.\n\nI have a feeling that adding more to isValidGitDir() like this patch\ndoes is a step in the wrong direction. The first fix would be not\nto do the \"if isValidGitDir() says no, then do something else\" step\nif you already asked \"rev-parse --git-dir\" and got a valid $GIT_DIR,\nperhaps?\n\nStepping back even more. \n\nI wonder why the script needs to do os.environ.get(\"GIT_DIR\") in the\nfirst place to initialize cmd.gitdir field. If it instead asked\n\"rev-parse --gitdir\", the script would get the right answer that\nalready takes GIT_DIR environment into account.\n\n\n[Footnote]\n\n*1* This ends up asking \"rev-parse --git-dir\" only because\n isValidGitDir() is sloppy. If you are in a secondary working\n tree whose \".git\" is a file, the function would say \"no\", and\n that is the only thing that allows you to make a call to\n \"rev-parse --git-dir\" to obtain the correct result.\n\n"},{"id":"306967","messageId":"CAE5ih79efEu_2jgE9V-N7+UetyYu7RjH62whcfvMBtwM-Nb8Sg@mail.gmail.com","threadId":"44601","inReplyTo":"xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv1 1/2] git-p4: support git-workspaces","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2016-12-05T21:55:17Z","receivedAt":"2016-12-05T21:55:40Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 5 December 2016 at 21:24, Junio C Hamano wrote:\n> Luke Diamand writes:\n>\n>> On 5 December 2016 at 20:53, Junio C Hamano wrote:\n>>> Luke Diamand writes:\n>>>\n>>>> Teach git-p4 about git-workspaces.\n>>>\n>>> Is this what we call \"git worktree\", or something else?\n>>\n>> Ah, I think you're right!\n>\n> Then I'll queue it like the attached.\n>\n> HOWEVER.\n>\n> How fast does isValidGitDir() function need to be?\n\nIt doesn't need to be fast.\n\n> The primary one\n> seems to check HEAD (but it does not notice a non-repository that\n> has a directory with that name), refs and objects (but it does not\n> notice a non-repository that has a non-directory with these names),\n> and this new one uses a test that is even more sloppy.\n>\n> What I am trying to get at is if we want to use a single command\n> that can be given a path and answer \"Yes, that is a repository\"\n> here, and that single command should know how the repository should\n> look like. I offhand do not know we already have such a command we\n> can use, e.g. \"git rev-parse --is-git-dir $path\", but if there isn't\n> perhaps we would want one, so that not just \"git p4\" but other\n> scripted Porcelains can make use of it?\n\nThat would be nicer than random ad-hoc rules, certainly. I couldn't\nfind any obvious combination that would work.\n\nLuke\n"},{"id":"306978","messageId":"xmqqeg1m2cfz.fsf@gitster.mtv.corp.google.com","threadId":"44601","inReplyTo":"CAE5ih79efEu_2jgE9V-N7+UetyYu7RjH62whcfvMBtwM-Nb8Sg@mail.gmail.com","subject":"Re: [PATCHv1 1/2] git-p4: support git-workspaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T23:17:20Z","receivedAt":"2016-12-05T23:17:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n>> What I am trying to get at is if we want to use a single command\n>> that can be given a path and answer \"Yes, that is a repository\"\n>> here, and that single command should know how the repository should\n>> look like. I offhand do not know we already have such a command we\n>> can use, e.g. \"git rev-parse --is-git-dir $path\", but if there isn't\n>> perhaps we would want one, so that not just \"git p4\" but other\n>> scripted Porcelains can make use of it?\n>\n> That would be nicer than random ad-hoc rules, certainly. I couldn't\n> find any obvious combination that would work.\n\nI've already sent an update but my reading of this code is that\nthere is no need for the program to be able to, given a random\ndirectory path, ask isValidGitDir() \"is this a Git repository?\" at\nall. \n\nWhat the program wanted to know, IIUC, is \"Where is the 'Git\nrepository directory', if there is one?\". This is needed primarily\nbecause the program wants to error out before doing any operation\nthat requires a Git repository to work on, when the answer is \"there\nis none\".\n\nIt also wants to know it because (for whatever reason) it wants to\nexport it in GIT_DIR [*1*].\n\nAnd isValidGitDir() is a helper function used by the handcrafted\nlogic in main() to answer that question, but as far as I can tell,\nyou'd get a more correct answer by asking \"rev-parse --git-dir\"\nunconditionally (even when the user of the program exported GIT_DIR\nto you).\n\nI just was reading Lars's LFS changes, but that one has hardcoded\n\"Is there a '.git/lfs/objects/' directory directly inside the\ncurrent working directory?\" and similar, which I do not think would\nwork well in a secondary working tree where \".git\" is merely a file\n[*2*]. In a \"secondary working tree\"-aware world, I think you would\nneed to ask locations of --git-dir and --git-common-dir to rev-parse\nupfront and choose which ones are ought to be shared entities across\nthe family of worktrees and which ones are to be private per\nworktree. I suspect that LFS objects want to be shared across\nworking trees that share the object store, for example, which would\nmean that \"--git-dir\" is not what Lars would want to use.\n\n\n[Footnote]\n\n*1* I do not think this is necessary. Git tools know how to find\nthe repository by first checking GIT_DIR environment and if it is\nnot set then by walking up the directory hierarchy just fine without\nthe program exporting GIT_DIR for them.\n\n*2* The part that bases this path relative to getcwd() is OK, as the\nstart-up sequence in main() does a cdup to the top before everything\nelse.\n"}]}