git absorb fails with latest version sometimes #116

jrwrigh · 2024-07-20T23:38:14Z

I'm seeing some very strange (and wrong) behavior with git absorb.

Given changes with the following diff (among changes in other files):

@@ -281,7 +279,6 @@ PetscErrorCode DivDiffFluxProjectionSetup_Direct(Ceed ceed, User user, CeedData
       PetscCall(KSPSetFromOptions_WithMatCeed(projection->ksp, mat_mass));
       PetscCall(MatDestroy(&mat_mass));
     }
-
     PetscCallCeed(ceed, CeedQFunctionDestroy(&qf_mass));
     PetscCallCeed(ceed, CeedOperatorDestroy(&op_mass));
   }
@@ -393,7 +390,6 @@ PetscErrorCode DivDiffFluxProjectionSetup_Indirect(Ceed ceed, User user, CeedDat
       PetscCall(KSPSetFromOptions_WithMatCeed(projection->ksp, mat_mass));
       PetscCall(MatDestroy(&mat_mass));
     }
-
     PetscCallCeed(ceed, CeedQFunctionDestroy(&qf_mass));
     PetscCallCeed(ceed, CeedOperatorDestroy(&op_mass));
   }
@@ -487,10 +483,10 @@ PetscErrorCode DiffFluxProjectionApply(DivDiffFluxProjectionData diff_flux_proj,
       if (!diff_flux_proj->DivDiffFlux_loc) PetscCall(DMCreateLocalVector(projection->dm, &diff_flux_proj->DivDiffFlux_loc));
       else PetscCall(VecReadCeedToPetsc(diff_flux_proj->div_diff_flux_ceed, diff_flux_proj->DivDiffFlux_memtype, diff_flux_proj->DivDiffFlux_loc));
       PetscCall(ApplyCeedOperatorLocalToGlobal(Q_loc, DivDiffFlux, projection->l2_rhs_ctx));
-      PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_proj_rhs_view"));
+      PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_projection_rhs_view"));
 
       PetscCall(KSPSolve(projection->ksp, DivDiffFlux, DivDiffFlux));
-      PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_proj_view"));
+      PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_projection_view"));
 
       PetscCall(DMGlobalToLocal(projection->dm, DivDiffFlux, INSERT_VALUES, diff_flux_proj->DivDiffFlux_loc));
       PetscCall(VecReadPetscToCeed(diff_flux_proj->DivDiffFlux_loc, &diff_flux_proj->DivDiffFlux_memtype, diff_flux_proj->div_diff_flux_ceed));
@@ -503,10 +499,10 @@ PetscErrorCode DiffFluxProjectionApply(DivDiffFluxProjectionData diff_flux_proj,
 
       PetscCall(DMGetGlobalVector(projection->dm, &DiffFlux));
       PetscCall(ApplyCeedOperatorLocalToGlobal(Q_loc, DiffFlux, projection->l2_rhs_ctx));
-      PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_proj_rhs_view"));
+      PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_projection_rhs_view"));
 
       PetscCall(KSPSolve(projection->ksp, DiffFlux, DiffFlux));
-      PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_proj_view"));
+      PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_projection_view"));
 
       PetscCall(ApplyCeedOperatorGlobalToLocal(DiffFlux, NULL, diff_flux_proj->calc_div_diff_flux));
       PetscCall(DMRestoreGlobalVector(projection->dm, &DiffFlux));

If I run git absorb on these changes:

 ❯ git absorb --base HEAD~9 --verbose
Jul 20 23:22:44.351 INFO committed, header: +3,-2, commit: 1cbe61decf43fa39197e1a5d2802c7d0d494d6b9, line: 324, module: git_absorb
Jul 20 23:22:44.359 INFO committed, header: +4,-4, commit: c66020b9f7eec8b7bcc91e5abc88cae59d106607, line: 324, module: git_absorb
Jul 20 23:22:44.361 INFO committed, header: +2,-3, commit: 73c668aa6d18048ea06d564122824bfd8f86d741, line: 324, module: git_absorb
Jul 20 23:22:44.362 INFO committed, header: +1,-1, commit: e1f063aa7c0dec4ea8f4fb515b9c808b275c0cd9, line: 324, module: git_absorb
Jul 20 23:22:44.363 INFO committed, header: +7,-6, commit: 4544f753a8e49a31bfa3c16e7daa747a31de28a4, line: 324, module: git_absorb
Jul 20 23:22:44.365 INFO committed, header: +0,-2, commit: 058fb9c16197f077e55567eadbe9bc78135ff5d0, line: 324, module: git_absorb
Jul 20 23:22:44.366 INFO committed, header: +0,-1, commit: cae56aa9cbc9f4ab89235f29901a03e02f16698d, line: 324, module: git_absorb
Jul 20 23:22:44.367 INFO committed, header: +1,-1, commit: 619bb2abd7e3cbbc87271f8ec0de6e332cbaaada, line: 324, module: git_absorb
Jul 20 23:22:44.369 INFO committed, header: +1,-1, commit: 82c21df7fdac92b9c07b0c03f03a04b08d89d0e1, line: 324, module: git_absorb
Jul 20 23:22:44.370 INFO committed, header: +1,-1, commit: 8056b0a786e1742747aeb2bf8279b662aba0a0c2, line: 324, module: git_absorb
Jul 20 23:22:44.371 INFO committed, header: +1,-1, commit: 1bf148f9b1435f2108ef36ad05b5b7543e80b2ca, line: 324, module: git_absorb
Jul 20 23:22:44.373 INFO committed, header: +4,-4, commit: d071768bc88da7bd2d939ae9d8d1eea71e3bee7c, line: 324, module: git_absorb
Jul 20 23:22:44.374 INFO committed, header: +0,-1, commit: cb5fb086ab780cedc77a95b5aa06c19c6f37611f, line: 324, module: git_absorb

and examine the end result of the added fixup commits, I instead get a diff of:

@@ -392,7 +390,6 @@ PetscErrorCode DivDiffFluxProjectionSetup_Indirect(Ceed ceed, User user, CeedDat
       }
       PetscCall(KSPSetFromOptions_WithMatCeed(projection->ksp, mat_mass));
       PetscCall(MatDestroy(&mat_mass));
-    }
 
     PetscCallCeed(ceed, CeedQFunctionDestroy(&qf_mass));
     PetscCallCeed(ceed, CeedOperatorDestroy(&op_mass));
@@ -486,10 +483,10 @@ PetscErrorCode DiffFluxProjectionApply(DivDiffFluxProjectionData diff_flux_proj,
       PetscCall(DMGetGlobalVector(projection->dm, &DivDiffFlux));
       if (!diff_flux_proj->DivDiffFlux_loc) PetscCall(DMCreateLocalVector(projection->dm, &diff_flux_proj->DivDiffFlux_loc));
       else PetscCall(VecReadCeedToPetsc(diff_flux_proj->div_diff_flux_ceed, diff_flux_proj->DivDiffFlux_memtype, diff_flux_proj->DivDiffFlux_loc));
-      PetscCall(ApplyCeedOperatorLocalToGlobal(Q_loc, DivDiffFlux, projection->l2_rhs_ctx));
+      PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_projection_rhs_view"));
       PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_proj_rhs_view"));
 
-      PetscCall(KSPSolve(projection->ksp, DivDiffFlux, DivDiffFlux));
+      PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_projection_view"));
       PetscCall(VecViewFromOptions(DivDiffFlux, NULL, "-div_diff_flux_proj_view"));
 
       PetscCall(DMGlobalToLocal(projection->dm, DivDiffFlux, INSERT_VALUES, diff_flux_proj->DivDiffFlux_loc));
@@ -502,10 +499,10 @@ PetscErrorCode DiffFluxProjectionApply(DivDiffFluxProjectionData diff_flux_proj,
       Vec DiffFlux;
 
       PetscCall(DMGetGlobalVector(projection->dm, &DiffFlux));
-      PetscCall(ApplyCeedOperatorLocalToGlobal(Q_loc, DiffFlux, projection->l2_rhs_ctx));
+      PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_projection_rhs_view"));
       PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_proj_rhs_view"));
 
-      PetscCall(KSPSolve(projection->ksp, DiffFlux, DiffFlux));
+      PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_projection_view"));
       PetscCall(VecViewFromOptions(DiffFlux, NULL, "-div_diff_flux_proj_view"));
 
       PetscCall(ApplyCeedOperatorGlobalToLocal(DiffFlux, NULL, diff_flux_proj->calc_div_diff_flux));

For these changes, the removed line is completely wrong, but the added back line is correct. Another point of note is that the git index was not emptied, but git absorb did not report that fact like it normally would.

Running git absorb again does correct the problems in this circumstance, but I ran into an very similar issue before where repeatedly running git absorb did not fix things, but added further erroneous fixup commits.

Also, even after running git absorb multiple times, the fixup commits do not rebase in without conflicts.

This is with git absorb version 0.6.15 according to the arch package installation (the version flag doesn't work, see #115)

The text was updated successfully, but these errors were encountered:

jrwrigh · 2024-07-20T23:42:11Z

Rolling back to 0.6.13 and 0.6.12, I'm still seeing the exact same behavior as above. The rollback to 0.6.12 also includes rolling back to libgit2 1.7

tummychow · 2024-07-20T23:42:38Z

gonna need the commits in the stack as well, or at least the hunk line numbers (ie what lines were added/removed in the affected file). the diff in the index alone is insufficient to diagnose the issue

jrwrigh · 2024-07-20T23:51:10Z

You can replicate the offending situation from this commit.

To replicate:

git clone git@gitlab.com:phypid/honee.git
cd honee
git co jrwrigh/diff_flux_absorb_orig
git reset HEAD^
git add .
git absorb --base HEAD~9 --verbose
# There should be changes left in the index
# "Fix" that with another call to git absorb
git absorb --base HEAD~9 --verbose
# Index should now be cleared
git rebase --autosquash HEAD~31
# And now you run into a conflict when attempting to autosquash

tummychow · 2024-07-21T00:07:10Z

hm. well it looks like line 284 being removed is causing an off by one somewhere (ie if i discard that one hunk from the index then the rest seems to absorb correctly). this will be very unpleasant to diagnose. no promises

jrwrigh · 2024-07-21T00:57:35Z

Yeah, I kinda looked like an off-by-one error, but I'm not sure what exactly absorb is doing on the backend.

If it makes any difference, this is the 3rd time this kind of off-by-one issue has happened today on this branch. And I think all of them had issues with adding fixup changes to that commit specifically (feat: Add indirect div F_diff projection).

jrwrigh · 2024-07-21T03:58:11Z

Another thing I just noticed; if I stage all the changes except for everything below that line 284 removal (so don't stage the first diff I posted above), then the proceeding git rebase --autosquash fails. Not sure if that failure would have any relation to the off-by-one error.

Edit: To reproduce from the above repo

# Get to remote state of `jrwrigh/diff_flux_absorb_orig` branch
git reset HEAD^
# stage correct hunks. Not sure if there's a reproducable commandline way to do this
git absorb --base HEAD~9 --verbose
git stash # stash the remaining hunks
git rebase HEAD~16 --autosquash

Which results in:

Auto-merging src/diff_flux_projection.c
CONFLICT (content): Merge conflict in src/diff_flux_projection.c
error: could not apply 04a5ba1... fixup! feat: Add divergence of F_diff projection
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Recorded preimage for 'src/diff_flux_projection.c'
Could not apply 04a5ba1... fixup! feat: Add divergence of F_diff projection

jrwrigh · 2024-08-18T00:47:37Z

For what it's worth, I've run into these exact issues (weird off-by-one problem causing the entire process to fail) several times now.

Because each hunk needs to be displaced based on the previously applied hunk, sorting hunks will change the order in which they are applied, therefore undoing all our previous calculations. This fixes tummychow#116, but breaks the --one-fixup-per-commit feature in cases where there is an odd hunk in between some hunks that could've been merged together.

rbartlensky mentioned this issue Oct 15, 2024

Don't sort hunks based on dest_commit. #123

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git absorb fails with latest version sometimes #116

git absorb fails with latest version sometimes #116

jrwrigh commented Jul 20, 2024 •

edited

Loading

jrwrigh commented Jul 20, 2024

tummychow commented Jul 20, 2024

jrwrigh commented Jul 20, 2024

tummychow commented Jul 21, 2024 •

edited

Loading

jrwrigh commented Jul 21, 2024

jrwrigh commented Jul 21, 2024 •

edited

Loading

jrwrigh commented Aug 18, 2024

git absorb fails with latest version sometimes #116

git absorb fails with latest version sometimes #116

Comments

jrwrigh commented Jul 20, 2024 • edited Loading

jrwrigh commented Jul 20, 2024

tummychow commented Jul 20, 2024

jrwrigh commented Jul 20, 2024

tummychow commented Jul 21, 2024 • edited Loading

jrwrigh commented Jul 21, 2024

jrwrigh commented Jul 21, 2024 • edited Loading

jrwrigh commented Aug 18, 2024

jrwrigh commented Jul 20, 2024 •

edited

Loading

tummychow commented Jul 21, 2024 •

edited

Loading

jrwrigh commented Jul 21, 2024 •

edited

Loading