Branch name for git detached head #1098

richardschneider · 2017-01-01T05:14:48Z

Many build systems use a "detached head", which means that the normal git commands to get branch names do not work. Thankfully, they set an environment variable. The various environment variable names was obtained from https://github.com/GitTools/GitVersion

Bug #1097

Here's an example of it now working on AppVeyor https://ci.appveyor.com/project/richardschneider/docfx-seed/build/1.0.8

dnfclas · 2017-01-01T05:14:52Z

Hi @richardschneider, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, DNFBOT;

qinezh · 2017-01-01T08:59:11Z

Hi @richardschneider , thanks for your contribution. From my point, I don't think DocFX should handle the case. Consider a scenario, what if users just need generate theview source url with commit id instead of branch name? And, it seems a litter weird to add these specific logic in an utility class... Maybe you could add a post processor to do it if you really need it.

My suggestion is that you need resolve the issue in CI tools instead of in DocFX. Such as running command git checkout master after step that CI has cloned you repo.

What do you think about it?

richardschneider · 2017-01-01T19:43:04Z

git checkout master will not work because then I will be building the doc against the latest source code. AppVeyor uses the detached head so the doc is built against the specified commit or a Pull Request. An advantage is that the CI can be used to build from history.

AppVeyor is popular in open source dev because it uses a Windows VM and is free. My workflow is simple: Build code, Test Code, Publish to Nuget, Generate Doc, publish to gh-pages.

docfx does not work in this environment. It has the wrong branch name in the generated .yml files. How is this not a docfx bug?

The logic was placed in the Utility class because this is where the bug is. It uses git rev-parse --abbrev-ref HEAD to get the local branch name. This command does not work in a detached head and always returns 'HEAD`.

richardschneider · 2017-01-02T06:42:31Z

Did you read #1098 (comment) I'm of the opinion that this is docfx bug.

…

On 2 January 2017 at 19:22, Qinen Zhu ***@***.***> wrote: ??? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1098 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmhFeLvaCGn03E--DB2Wr3DaXIGoM4Wks5rOJe1gaJpZM4LYqjd> .

qinezh · 2017-01-02T08:16:55Z

What if users just need generate the view source url with commit id instead of branch name in AppVeyor?

richardschneider · 2017-01-02T09:15:03Z

@qinezh I don't understand your concern about view source with commit id..

I am simply fixing the branch name for an edge case (a git detached head).

Existing code will work as before. This fix has nothing to do with a commit id.

vicancy · 2017-01-05T01:15:07Z

The concern is that the code here highly dependents on CI's implementation.
How about a environment variable for docfx to fallback to, e.g. DOCFX_SOURCE_BRANCH_NAME? @qinezh @richardschneider

richardschneider · 2017-01-05T02:13:58Z

@vicancy good idea. I'll add it add the head of list, so it overrides all other environment variables.

I want to keep the other env names so that that docfx works with other build systems without having to make to make any changes.

vicancy · 2017-01-05T02:48:00Z

@richardschneider How about a section talking about setting up CI in docfx documentation? We can put that in Readme.md so that people can easily find that section. I would prefer removing those CI env names, considering that these names are prone to change, for instance, Jenkins already contains 2 env names.

richardschneider · 2017-01-05T02:50:51Z

@vicancy excellent. I was just looking at folder structure to find out where to this info. Could you tell me the folder/file to update.

vicancy · 2017-01-05T03:03:22Z

Mention it in Readme here: https://github.com/dotnet/docfx/blob/dev/README.md
And add a new section in https://github.com/dotnet/docfx/blob/dev/Documentation/tutorial/docfx_getting_started.md. Or add a new file talking about it in detail and reference the file in https://github.com/dotnet/docfx/blob/dev/Documentation/tutorial/toc.yml

Really appreciate your contributions @richardschneider 😄 ,

# Conflicts: # src/Microsoft.DocAsCode.Common/Git/GitUtility.cs

vicancy · 2017-01-05T05:00:23Z

Documentation/tutorial/docfx_getting_started.md

+
+Setting the environment variable `DOCFX_SOURCE_BRANCH_NAME` tells DocFX which branch name to use.
+
+Many build systems set an environment variable with the branch name.  DocFX uses the following:


Support DOCFX_SOURCE_BRANCH_NAME is enough? Use documentation to tell CI users how to configure environment variables with different build systems?

I prefer to have docfx run on common the build system without any special instructions.

vicancy · 2017-01-05T05:01:22Z

test/Microsoft.DocAsCode.Common.Tests/GitUtilityTest.cs

+    [Trait("Owner", "makaretu")]
+    public class GitUtilityTest
+    {
+


remove extra empty lines

vicancy · 2017-01-05T05:03:06Z

src/Microsoft.DocAsCode.Common/Git/GitUtility.cs

-                {
-                    remoteBranch = remoteBranch.Substring(index + 1);
-                }
+                remoteBranch = localBranch;


LogInfo to tell users that branch name is found in env

vicancy · 2017-01-05T05:04:49Z

src/Microsoft.DocAsCode.Common/Git/GitUtility.cs

+
+            // Many build systems use a "detach head", which means that the normal git commands
+            // to get branch names do not work.  Thankfully, they set an environment variable.
+            string localBranch = BuildSystemBranchName


what if the BuildSystemBranchName is found in env, however the repo is not in a detached mode?

The environment variable names are overrides. If any is specified, then the first specifies the branch name.

vicancy · 2017-01-06T03:11:54Z

Hi @richardschneider, the concern with your PR is that it introduces a new overwriting mechanism through Environment Variables for a specific case.

Actually docfx allows overwriting URL for documents through _gitContribute in http://dotnet.github.io/docfx/tutorial/docfx.exe_user_manual.html#322-reserved-metadata.

To support your case, we plan to support overwriting the git url for source files, aka, href for View Source button. So after the change, in CI machines, passing %BRANCH_NAME% through docfx --globalMatadata command line can work.

richardschneider · 2017-01-06T03:36:15Z

@vicancy I was just trying to get doxfx to work, as expected, with appyevor. If you want developers to jump through hoops and spend hours googling to get it to work then go for it and document it.

Also, it's my opinion that most documentation is generated on a build server, not on an individual devops machine. My issue #1078 is still open.

I thought I had a simple fix for this, but obviously I am wrong.

vicancy · 2017-01-06T06:19:20Z

Hi @richardschneider I agree with you that the simplicity of setting up docfx on CI machine is quite important. After further discussion, we plan to support both overwriting from globalMetadata and Environment Variables. We will take your PR to support Environment Variables.
For the PR, do you think it is more reasonable to change the logic to:

If DOCFX_SOURCE_BRANCH_NAME is set, set branch name to DOCFX_SOURCE_BRANCH_NAME
If the repo is in detached mode, set branch name to CI env variable if it exists
Otherwise, use current commit id

So that DOCFX_SOURCE_BRANCH_NAME has the highest priority, and CI env variables only take effect when the repo is in detached mode.

Again, really appreciate your patience and continuous contribution to docfx. 😄

richardschneider · 2017-01-06T07:25:08Z

Hi @vicancy and thanks for all comments.

If DOCFX_SOURCE_BRANCH_NAME is set, set branch name to DOCFX_SOURCE_BRANCH_NAME

PR already does this.

If the repo is in detached mode, set branch name to CI env variable if it exists

Okay, will make a change.

Otherwise, use current commit id

PR already does this.

…tached head The code is ugly but this logic has been been requested by the team.

vicancy · 2017-01-10T02:47:16Z

src/Microsoft.DocAsCode.Common/Git/GitUtility.cs

-                remoteBranch = RunGitCommandAndGetFirstLine(repoRootPath, GetRemoteBranchCommand);
-                var index = remoteBranch.IndexOf('/');
-                if (index > 0)
+                var isDetached = "HEAD" == RunGitCommandAndGetFirstLine(repoRootPath, GetLocalBranchCommand);


"HEAD" == Run [](start = 32, length = 14)

merge with GetLocalBranchName?

I think the addition of GetLocalBranchName is wrong. It changes the semantics of both RepoInfo.LocalBranch and RepoInfo.RemoteBranch. Previously they held a git branch name. Now they can also hold a commit id.

I want nothing to do with this. Its evil.

Not sure what you mean by "HEAD" == Run [](start = 32, length = 14)

However the code here is exactly GetLocalBranchName. And they have duplicate logic checking detached. Please make the code change cleaner rather than more complex.

vicancy · 2017-01-10T06:46:32Z

src/Microsoft.DocAsCode.Common/Git/GitUtility.cs

                remoteBranch = localBranch;
+                Logger.LogInfo($"Using branch '{localBranch}' from the environment variable.");


also log the environment variable name

superyyrrzz · 2017-01-11T02:02:24Z

richardschneider added 2 commits January 1, 2017 17:35

fix(GitUtility): get remote branch when git is a detached head

2634cc8

test(GitUtility): remove test because of caching

34cabfb

dnfclas added the cla-signed label Jan 1, 2017

richardschneider mentioned this pull request Jan 4, 2017

Getting branch name fails on AppVeyor #1097

Closed

richardschneider added 3 commits January 5, 2017 16:17

Merge remote-tracking branch 'upstream/dev' into detect-branch

ff92a6e

# Conflicts: # src/Microsoft.DocAsCode.Common/Git/GitUtility.cs

feat(GitUtility): Allow DOCFX_SOURCE_BRANCH_NAME environment variable

561be0c

docs(ci): using docfx with a build server

2375d60

vicancy reviewed Jan 5, 2017

View reviewed changes

richardschneider added 2 commits January 6, 2017 09:47

test(GitUtility): remove empty line

8406788

fix(GitUtility): LogInfo to tell users that branch name is found in env

9204df9

fix(GitUtility); only use build server env names when there is git de…

3d9a5b5

…tached head The code is ugly but this logic has been been requested by the team.

richardschneider mentioned this pull request Jan 7, 2017

docfx and appveyor appveyor/ci#1253

Closed

vicancy reviewed Jan 10, 2017

View reviewed changes

fix(GitUtilities): more logging and code cleanup

5f474cb

vicancy approved these changes Jan 11, 2017

View reviewed changes

vicancy merged commit 89fae94 into dotnet:dev Jan 11, 2017

richardschneider deleted the detect-branch branch January 11, 2017 05:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branch name for git detached head #1098

Branch name for git detached head #1098

richardschneider commented Jan 1, 2017

dnfclas commented Jan 1, 2017

qinezh commented Jan 1, 2017 •

edited

Loading

richardschneider commented Jan 1, 2017

richardschneider commented Jan 2, 2017 via email

qinezh commented Jan 2, 2017

richardschneider commented Jan 2, 2017

vicancy commented Jan 5, 2017 •

edited

Loading

richardschneider commented Jan 5, 2017

vicancy commented Jan 5, 2017

richardschneider commented Jan 5, 2017

vicancy commented Jan 5, 2017

vicancy Jan 5, 2017

richardschneider Jan 5, 2017

vicancy Jan 5, 2017

vicancy Jan 5, 2017

vicancy Jan 5, 2017

richardschneider Jan 5, 2017 •

edited

Loading

vicancy commented Jan 6, 2017

richardschneider commented Jan 6, 2017 •

edited

Loading

vicancy commented Jan 6, 2017

richardschneider commented Jan 6, 2017

vicancy Jan 10, 2017

richardschneider Jan 10, 2017 •

edited

Loading

vicancy Jan 10, 2017

vicancy Jan 10, 2017

superyyrrzz commented Jan 11, 2017


		Setting the environment variable `DOCFX_SOURCE_BRANCH_NAME` tells DocFX which branch name to use.

		Many build systems set an environment variable with the branch name. DocFX uses the following:

		remoteBranch = localBranch;
		Logger.LogInfo($"Using branch '{localBranch}' from the environment variable.");

Branch name for git detached head #1098

Branch name for git detached head #1098

Conversation

richardschneider commented Jan 1, 2017

dnfclas commented Jan 1, 2017

qinezh commented Jan 1, 2017 • edited Loading

richardschneider commented Jan 1, 2017

richardschneider commented Jan 2, 2017 via email

qinezh commented Jan 2, 2017

richardschneider commented Jan 2, 2017

vicancy commented Jan 5, 2017 • edited Loading

richardschneider commented Jan 5, 2017

vicancy commented Jan 5, 2017

richardschneider commented Jan 5, 2017

vicancy commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardschneider Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

vicancy commented Jan 6, 2017

richardschneider commented Jan 6, 2017 • edited Loading

vicancy commented Jan 6, 2017

richardschneider commented Jan 6, 2017

Choose a reason for hiding this comment

richardschneider Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superyyrrzz commented Jan 11, 2017

qinezh commented Jan 1, 2017 •

edited

Loading

vicancy commented Jan 5, 2017 •

edited

Loading

richardschneider Jan 5, 2017 •

edited

Loading

richardschneider commented Jan 6, 2017 •

edited

Loading

richardschneider Jan 10, 2017 •

edited

Loading