Skip to content

chore: add feature flag to the git reset optimisation #39988

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.

Sign up for GitHub

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

Merged
sondermanish merged 3 commits into from
Apr 1, 2025
Merged

chore: add feature flag to the git reset optimisation #39988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
056b693
chore: add feature flag to the git reset optimisation
Mar 31, 2025
5a603f9
Merge remote-tracking branch 'origin' into chore/add-feature-flag-res…
Apr 1, 2025
5f3ce61
PR comments
Apr 1, 2025
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
5 changes: 3 additions & 2 deletions app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,14 +246,15 @@ public Mono<Path> saveApplicationToGitRepo(
}

@Override
public Mono<Path> saveArtifactToGitRepo(Path baseRepoSuffix, GitResourceMap gitResourceMap, String branchName)
public Mono<Path> saveArtifactToGitRepo(
Path baseRepoSuffix, GitResourceMap gitResourceMap, String branchName, boolean keepWorkingDirChanges)
throws GitAPIException, IOException {

// Repo path will be:
// baseRepo : root/workspaceId/defaultAppId/repoName/{applicationData}
// Checkout to mentioned branch if not already checked-out
return fsGitHandler
.resetToLastCommit(baseRepoSuffix, branchName)
.resetToLastCommit(baseRepoSuffix, branchName, keepWorkingDirChanges)
.flatMap(isSwitched -> {
Path baseRepo = Paths.get(gitServiceConfig.getGitRootPath()).resolve(baseRepoSuffix);

Expand Down
153 changes: 110 additions & 43 deletions app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,12 @@ public Mono<Boolean> checkoutToBranch(Path repoSuffix, String branchName) {

@Override
public Mono<MergeStatusDTO> pullArtifactWithoutCheckout(
Path repoSuffix, String remoteUrl, String branchName, String privateKey, String publicKey)
Path repoSuffix,
String remoteUrl,
String branchName,
String privateKey,
String publicKey,
boolean keepWorkingDirChanges)
throws IOException {

TransportConfigCallback transportConfigCallback = new SshTransportConfigCallback(privateKey, publicKey);
Expand Down Expand Up @@ -629,7 +634,13 @@ public Mono<MergeStatusDTO> pullArtifactWithoutCheckout(
}
}
})
.onErrorResume(error -> Mono.error(error))
.onErrorResume(error -> {
if (keepWorkingDirChanges) {
return Mono.error(error);
}

return resetToLastCommit(git).flatMap(ignore -> Mono.error(error));
})
.timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS))
.name(GitSpan.FS_PULL)
.tap(Micrometer.observation(observationRegistry)),
Expand Down Expand Up @@ -848,7 +859,7 @@ public Mono<String> getRemoteDefaultBranch(Path repoSuffix, String remoteUrl, St
* @return Map of file names those are modified, conflicted etc.
*/
@Override
public Mono<GitStatusDTO> getStatus(Path repoPath, String branchName) {
public Mono<GitStatusDTO> getStatus(Path repoPath, String branchName, boolean keepWorkingDirChanges) {
Stopwatch processStopwatch =
StopwatchHelpers.startStopwatch(repoPath, AnalyticsEvents.GIT_STATUS.getEventName());
return Mono.using(
Expand Down Expand Up @@ -902,6 +913,16 @@ public Mono<GitStatusDTO> getStatus(Path repoPath, String branchName) {
response.setRemoteBranch("untracked");
}

// Remove modified changes from current branch so that checkout to other branches
// will be clean
if (!status.isClean() && !keepWorkingDirChanges) {
Copy link
Collaborator

@subrata71 subrata71 Mar 31, 2025

Choose a reason for hiding this comment

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

Isn't the status "not clean" enough to decide to do a git reset here? If not, can you please explain?

Copy link
Author

@dvj1988 dvj1988 Apr 1, 2025

Choose a reason for hiding this comment

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

This is revert of this PR

return resetToLastCommit(git).map(ref -> {
processStopwatch.stopAndLogTimeInMillis();
jgitStatusSpan.end();
return response;
});
}

processStopwatch.stopAndLogTimeInMillis();
jgitStatusSpan.end();
return Mono.just(response);
Expand Down Expand Up @@ -1087,7 +1108,8 @@ private String getPageName(String path) {
}

@Override
public Mono<String> mergeBranch(Path repoSuffix, String sourceBranch, String destinationBranch) {
public Mono<String> mergeBranch(
Path repoSuffix, String sourceBranch, String destinationBranch, boolean keepWorkingDirChanges) {
return Mono.using(
() -> Git.open(createRepoPath(repoSuffix).toFile()),
git -> Mono.fromCallable(() -> {
Expand Down Expand Up @@ -1119,7 +1141,17 @@ public Mono<String> mergeBranch(Path repoSuffix, String sourceBranch, String des
}
})
.onErrorResume(error -> {
return Mono.error(error);
if (keepWorkingDirChanges) {
return Mono.error(error);
}

try {
return resetToLastCommit(repoSuffix, destinationBranch, keepWorkingDirChanges)
.thenReturn(error.getMessage());
} catch (Exception e) {
log.error("Error while hard resetting to latest commit", e);
return Mono.error(e);
}
})
.timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS))
.name(GitSpan.FS_MERGE)
Expand Down Expand Up @@ -1252,7 +1284,8 @@ public Mono<String> fetchRemote(
}

@Override
public Mono<MergeStatusDTO> isMergeBranch(Path repoSuffix, String sourceBranch, String destinationBranch) {
public Mono<MergeStatusDTO> isMergeBranch(
Path repoSuffix, String sourceBranch, String destinationBranch, boolean keepWorkingDirChanges) {
Stopwatch processStopwatch =
StopwatchHelpers.startStopwatch(repoSuffix, AnalyticsEvents.GIT_MERGE_CHECK.getEventName());
return Mono.using(
Expand Down Expand Up @@ -1302,13 +1335,40 @@ public Mono<MergeStatusDTO> isMergeBranch(Path repoSuffix, String sourceBranch,
mergeResult.getMergeStatus().name());
return mergeStatus;
})
.flatMap(status -> {
if (keepWorkingDirChanges) {
return Mono.just(status);
}

try {
// Revert uncommitted changes if any
return resetToLastCommit(repoSuffix, destinationBranch, keepWorkingDirChanges)
.map(ignore -> {
processStopwatch.stopAndLogTimeInMillis();
return status;
});
} catch (Exception e) {
log.error("Error for hard resetting to latest commit", e);
return Mono.error(e);
}
})
.onErrorResume(error -> {
MergeStatusDTO mergeStatusDTO = new MergeStatusDTO();
mergeStatusDTO.setMergeAble(false);
mergeStatusDTO.setMessage(error.getMessage());
mergeStatusDTO.setReferenceDoc(ErrorReferenceDocUrl.GIT_MERGE_CONFLICT.getDocUrl());

return Mono.just(mergeStatusDTO);
if (keepWorkingDirChanges) {
return Mono.just(mergeStatusDTO);
}

try {
return resetToLastCommit(repoSuffix, destinationBranch, keepWorkingDirChanges)
.thenReturn(mergeStatusDTO);
} catch (Exception e) {
log.error("Error while hard resetting to latest commit", e);
return Mono.error(e);
}
})
.timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS)),
Git::close)
Expand Down Expand Up @@ -1426,11 +1486,17 @@ public Mono<Boolean> resetToLastCommit(Path repoSuffix) {
Git::close);
}

public Mono<Boolean> resetToLastCommit(Path repoSuffix, String branchName) {
public Mono<Boolean> resetToLastCommit(Path repoSuffix, String branchName, boolean keepWorkingDirChanges) {
return Mono.using(
() -> Git.open(createRepoPath(repoSuffix).toFile()),
git -> this.resetToLastCommit(git)
.flatMap(ref -> checkoutToBranch(repoSuffix, branchName).thenReturn(true)),
.flatMap(ref -> checkoutToBranch(repoSuffix, branchName).flatMap(aBoolean -> {
if (keepWorkingDirChanges) {
return Mono.just(true);
}

return resetToLastCommit(git).thenReturn(true);
})),
Git::close);
}

Expand Down Expand Up @@ -1459,40 +1525,41 @@ public Mono<Boolean> resetHard(Path repoSuffix, String branchName) {
.subscribeOn(scheduler);
}

public Mono<Boolean> rebaseBranch(Path repoSuffix, String branchName) {
return this.resetToLastCommit(repoSuffix, branchName).flatMap(isCheckedOut -> Mono.using(
() -> Git.open(createRepoPath(repoSuffix).toFile()),
git -> Mono.fromCallable(() -> {
Span jgitRebaseSpan = observationHelper.createSpan(GitSpan.JGIT_REBASE);
RebaseResult result = git.rebase()
.setUpstream("origin/" + branchName)
.call();
if (result.getStatus().isSuccessful()) {
jgitRebaseSpan.end();
return true;
} else {
log.error(
"Error while rebasing the branch, {}, {}",
result.getStatus().name(),
result.getConflicts());
git.rebase()
.setUpstream("origin/" + branchName)
.setOperation(RebaseCommand.Operation.ABORT)
.call();
jgitRebaseSpan.end();
throw new Exception("Error while rebasing the branch, "
+ result.getStatus().name());
}
})
.onErrorMap(e -> {
log.error("Error while rebasing the branch, {}", e.getMessage());
return e;
})
.timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS))
.name(GitSpan.FS_REBASE)
.tap(Micrometer.observation(observationRegistry)),
Git::close)
.subscribeOn(scheduler));
public Mono<Boolean> rebaseBranch(Path repoSuffix, String branchName, boolean keepWorkingDirChanges) {
return this.resetToLastCommit(repoSuffix, branchName, keepWorkingDirChanges)
.flatMap(isCheckedOut -> Mono.using(
() -> Git.open(createRepoPath(repoSuffix).toFile()),
git -> Mono.fromCallable(() -> {
Span jgitRebaseSpan = observationHelper.createSpan(GitSpan.JGIT_REBASE);
RebaseResult result = git.rebase()
.setUpstream("origin/" + branchName)
.call();
if (result.getStatus().isSuccessful()) {
jgitRebaseSpan.end();
return true;
} else {
log.error(
"Error while rebasing the branch, {}, {}",
result.getStatus().name(),
result.getConflicts());
git.rebase()
.setUpstream("origin/" + branchName)
.setOperation(RebaseCommand.Operation.ABORT)
.call();
jgitRebaseSpan.end();
throw new Exception("Error while rebasing the branch, "
+ result.getStatus().name());
}
})
.onErrorMap(e -> {
log.error("Error while rebasing the branch, {}", e.getMessage());
return e;
})
.timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS))
.name(GitSpan.FS_REBASE)
.tap(Micrometer.observation(observationRegistry)),
Git::close)
.subscribeOn(scheduler));
}

@Override
Expand Down
4 changes: 4 additions & 0 deletions ...server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ public enum FeatureFlagEnum {
rollout_datasource_test_rate_limit_enabled,
release_google_sheets_shared_drive_support_enabled,
release_gs_all_sheets_options_enabled,
/**
* Feature flag to detect if the git reset optimization is enabled
*/
release_git_reset_optimization_enabled,

// Deprecated CE flags over here
release_git_autocommit_feature_enabled,
Expand Down
3 changes: 2 additions & 1 deletion app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ Mono<Path> saveApplicationToGitRepo(
Path baseRepoSuffix, ArtifactGitReference artifactGitReference, String branchName)
throws IOException, GitAPIException;

Mono<Path> saveArtifactToGitRepo(Path baseRepoSuffix, GitResourceMap gitResourceMap, String branchName)
Mono<Path> saveArtifactToGitRepo(
Path baseRepoSuffix, GitResourceMap gitResourceMap, String branchName, boolean keepWorkingDirChanges)
throws GitAPIException, IOException;

/**
Expand Down
20 changes: 14 additions & 6 deletions ...ver/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,12 @@ Mono<String> pushArtifact(
* @return success message
*/
Mono<MergeStatusDTO> pullArtifactWithoutCheckout(
Path repoSuffix, String remoteUrl, String branchName, String privateKey, String publicKey)
Path repoSuffix,
String remoteUrl,
String branchName,
String privateKey,
String publicKey,
boolean keepWorkingDirChanges)
throws IOException;

/**
Expand Down Expand Up @@ -153,7 +158,7 @@ Mono<MergeStatusDTO> pullApplication(
* @param branchName branch name for which the status is required
* @return Map of file names those are added, removed, modified
*/
Mono<GitStatusDTO> getStatus(Path repoPath, String branchName);
Mono<GitStatusDTO> getStatus(Path repoPath, String branchName, boolean keepWorkingDirChanges);

/**
* This method merges source branch into destination branch for a git repository which is present on the partial
Expand All @@ -163,7 +168,8 @@ Mono<MergeStatusDTO> pullApplication(
* @param destinationBranch Merge operation is performed on this branch
* @return Merge status
*/
Mono<String> mergeBranch(Path repoSuffix, String sourceBranch, String destinationBranch);
Mono<String> mergeBranch(
Path repoSuffix, String sourceBranch, String destinationBranch, boolean keepWorkingDirChanges);

/**
* @param repoSuffix suffixedPath used to generate the base repo path this includes workspaceId, defaultAppId, repoName
Expand All @@ -190,7 +196,8 @@ Mono<String> fetchRemote(
* @param destinationBranch Merge operation is performed on this branch
* @return Whether the two branches can be merged or not with list of files where the conflicts are present
*/
Mono<MergeStatusDTO> isMergeBranch(Path repoSuffix, String sourceBranch, String destinationBranch);
Mono<MergeStatusDTO> isMergeBranch(
Path repoSuffix, String sourceBranch, String destinationBranch, boolean keepWorkingDirChanges);

/**
* This method will reset the repo to last commit for the specific branch
Expand All @@ -201,7 +208,8 @@ Mono<String> fetchRemote(
* @throws GitAPIException
* @throws IOException
*/
Mono<Boolean> resetToLastCommit(Path repoSuffix, String branchName) throws GitAPIException, IOException;
Mono<Boolean> resetToLastCommit(Path repoSuffix, String branchName, boolean keepWorkingDirChanges)
throws GitAPIException, IOException;

/**
* This method will reset the repo to last commit for the current branch on which it's present
Expand Down Expand Up @@ -237,7 +245,7 @@ Mono<String> fetchRemote(

Mono<Boolean> resetHard(Path repoSuffix, String branchName);

Mono<Boolean> rebaseBranch(Path repoSuffix, String branchName);
Mono<Boolean> rebaseBranch(Path repoSuffix, String branchName, boolean keepWorkingDirChanges);

Path createRepoPath(Path suffix);

Expand Down
7 changes: 5 additions & 2 deletions ...ppsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.appsmith.server.plugins.base.PluginService;
import com.appsmith.server.repositories.GitDeployKeysRepository;
import com.appsmith.server.services.AnalyticsService;
import com.appsmith.server.services.FeatureFlagService;
import com.appsmith.server.services.SessionUserService;
import com.appsmith.server.services.UserDataService;
import com.appsmith.server.services.UserService;
Expand Down Expand Up @@ -52,7 +53,8 @@ public GitFSServiceCECompatibleImpl(
GitAutoCommitHelper gitAutoCommitHelper,
GitProfileUtils gitProfileUtils,
GitAnalyticsUtils gitAnalyticsUtils,
GitArtifactHelperResolver gitArtifactHelperResolver) {
GitArtifactHelperResolver gitArtifactHelperResolver,
FeatureFlagService featureFlagService) {
super(
gitDeployKeysRepository,
gitPrivateRepoHelper,
Expand All @@ -75,6 +77,7 @@ public GitFSServiceCECompatibleImpl(
gitAutoCommitHelper,
gitProfileUtils,
gitAnalyticsUtils,
gitArtifactHelperResolver);
gitArtifactHelperResolver,
featureFlagService);
}
}
Loading