Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrated Repository will show modifications when possible #17191

Merged
merged 2 commits into from
Dec 23, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions modules/git/repo_compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,14 @@
package git

import (
"bufio"
"bytes"
"errors"
"fmt"
"io"
"io/ioutil"
"os"
"path/filepath"
"regexp"
"strconv"
"strings"
Expand Down Expand Up @@ -188,6 +193,8 @@ func GetDiffShortStat(repoPath string, args ...string) (numFiles, totalAdditions
var shortStatFormat = regexp.MustCompile(
`\s*(\d+) files? changed(?:, (\d+) insertions?\(\+\))?(?:, (\d+) deletions?\(-\))?`)

var patchCommits = regexp.MustCompile(`^From\s(\w+)\s`)

func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int, err error) {
if len(stdout) == 0 || stdout == "\n" {
return 0, 0, 0, nil
Expand Down Expand Up @@ -267,3 +274,57 @@ func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) err
}
return err
}

// ReadPullHead will fetch a pull ref if possible or return an error
func (repo *Repository) ReadPullHead(prID int64) (commitSHA string, err error) {
headPath := fmt.Sprintf("refs/pull/%d/head", prID)
fullHeadPath := filepath.Join(repo.Path, headPath)
loadHead, err := os.Open(fullHeadPath)
if err != nil {
return "", err
}
defer loadHead.Close()
// Read only the first line of the patch - usually it contains the first commit made in patch
scanner := bufio.NewScanner(loadHead)
scanner.Scan()
commitHead := scanner.Text()
if len(commitHead) != 40 {
return "", errors.New("head file doesn't contain valid commit ID")
}
return commitHead, nil
}
Comment on lines +279 to +295
Copy link
Contributor

@zeripath zeripath Dec 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO!

This will break as soon as references are packed into files!

We have git.GetRefCommitID, we have (*git.Repository).GetRefCommitID

You should use the functionality that we have already written for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been:

	headRef := fmt.Sprintf(PullPrefix+"%d/head", prID)
	return repo.GetRefCommitID(headRef)


// ReadPatchCommit will check if a diff patch exists and return stats
func (repo *Repository) ReadPatchCommit(prID int64) (commitSHA string, err error) {
// Migrated repositories download patches to "pulls" location
patchFile := fmt.Sprintf("pulls/%d.patch", prID)
loadPatch, err := os.Open(filepath.Join(repo.Path, patchFile))
if err != nil {
return "", err
}
defer loadPatch.Close()
// Read only the first line of the patch - usually it contains the first commit made in patch
scanner := bufio.NewScanner(loadPatch)
scanner.Scan()
// Parse the Patch stats, sometimes Migration returns a 404 for the patch file
commitSHAGroups := patchCommits.FindStringSubmatch(scanner.Text())
if len(commitSHAGroups) != 0 {
commitSHA = commitSHAGroups[1]
} else {
return "", errors.New("patch file doesn't contain valid commit ID")
}
return commitSHA, nil
}

// WritePullHead will populate a PR head retrieved from patch file
func (repo *Repository) WritePullHead(prID int64, commitSHA string) error {
headPath := fmt.Sprintf("refs/pull/%d", prID)
fullHeadPath := filepath.Join(repo.Path, headPath)
// Create missing directory just in case
if err := os.MkdirAll(fullHeadPath, os.ModePerm); err != nil {
return err
}
commitBytes := []byte(commitSHA)
pullPath := filepath.Join(fullHeadPath, "head")
return ioutil.WriteFile(pullPath, commitBytes, os.ModePerm)
}
Comment on lines +319 to +330
Copy link
Contributor

@zeripath zeripath Dec 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same again! Don't write to git's references directly yourself!

You should have used git update-ref here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been simply:

	headRef := fmt.Sprintf(PullPrefix+"%d/head", prID)
	_, err := NewCommandContext(repo.Ctx, "update-ref", headRef, commitSHA).RunInDir(repo.Path)
	return err

40 changes: 39 additions & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,46 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) *git.C
setMergeTarget(ctx, pull)
ctx.Data["HasMerged"] = true

var baseCommit string
// Some migrated PR won't have any Base SHA and lose history, try to get one
if pull.MergeBase == "" {
var commitSHA, parentCommit string
// If there is a head or a patch file, and it is readable, grab info
commitSHA, err := ctx.Repo.GitRepo.ReadPullHead(pull.Index)
if err != nil {
// Head File does not exist, try the patch
commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index)
if err == nil {
// Recreate pull head in files for next time
if err := ctx.Repo.GitRepo.WritePullHead(pull.Index, commitSHA); err != nil {
log.Error("Could not write head file", err)
}
} else {
// There is no history available
log.Trace("No history file available for PR %d", pull.Index)
}
}
if commitSHA != "" {
// Get immediate parent of the first commit in the patch, grab history back
parentCommit, err = git.NewCommandContext(ctx, "rev-list", "-1", "--skip=1", commitSHA).RunInDir(ctx.Repo.GitRepo.Path)
if err == nil {
parentCommit = strings.TrimSpace(parentCommit)
}
// Special case on Git < 2.25 that doesn't fail on immediate empty history
if err != nil || parentCommit == "" {
log.Info("No known parent commit for PR %d, error: %v", pull.Index, err)
// bring at least partial history if it can work
parentCommit = commitSHA
}
}
baseCommit = parentCommit
} else {
// Keep an empty history or original commit
baseCommit = pull.MergeBase
}

compareInfo, err := ctx.Repo.GitRepo.GetCompareInfo(ctx.Repo.Repository.RepoPath(),
pull.MergeBase, pull.GetGitRefName(), true, false)
baseCommit, pull.GetGitRefName(), true, false)
if err != nil {
if strings.Contains(err.Error(), "fatal: Not a valid object name") || strings.Contains(err.Error(), "unknown revision or path not in the working tree") {
ctx.Data["IsPullRequestBroken"] = true
Expand Down