| Message ID | 20210728031437.14257-1-jerry@skydio.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] git-apply: silence errors for success cases | expand |
On Tue, Jul 27, 2021 at 8:14 PM Jerry Zhang <jerry@skydio.com> wrote: > > Certain invocations of "git apply --3way" > will print error messages even though git > is able to fall back on apply_fragments and > apply the patch successfully with a return > value of 0. To fix, return early from > try_threeway() in the following cases: > > When the patch is a rename and no lines have > changed. In this case, "git diff" doesn't > record the blob info, so 3way is neither > possible nor necessary. > > When the patch is an addition and there is > no add/add conflict, i.e. direct_to_threeway > is false. In this case, threeway will fail > since the preimage is not in cache, but isn't > necessary anyway since there is no conflict. > > Only error messaging is affected and other > behavior does not change. > > Signed-off-by: Jerry Zhang <jerry@skydio.com> > --- > V1->V2: rebase onto master and rerun tests > > I think I addressed previous comments. What > are the next steps for this patch? > > apply.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/apply.c b/apply.c > index 44bc31d6eb..fb321c707b 100644 > --- a/apply.c > +++ b/apply.c > @@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state, > > /* No point falling back to 3-way merge in these cases */ > if (patch->is_delete || > - S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode)) > + S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) || > + (patch->is_new && !patch->direct_to_threeway) || > + (patch->is_rename && !patch->lines_added && !patch->lines_deleted)) > return -1; > > /* Preimage the patch was prepared for */ > -- > 2.32.0.1314.g6ed4fcc4cc > Any updates on this patch? I had addressed the previous comment in this thread in a different patch "apply: adjust messages to account for --3way changes" that was already merged in a while ago. I originally intended this to mainly fix print message behavior, but while testing I found that it also fixed a separately reported issue from this thread: https://www.spinics.net/lists/git/msg421481.html. I probably owe an update / refresher on why I originally submitted these features in the first place. I've written a tool for branchless / patch stack based code review and development w/github. The core of this is a mechanism that can cherry-pick commits without touching the cache or working tree by using "git apply --cached --3way" with a custom index file. We've been using this internally with a custom built git for a while now and gotten pretty good feedback. We'd like to open source this tool soon, and it'd be nice if our open-source users (eventually) don't need to build git themselves :).
diff --git a/apply.c b/apply.c index 44bc31d6eb..fb321c707b 100644 --- a/apply.c +++ b/apply.c @@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state, /* No point falling back to 3-way merge in these cases */ if (patch->is_delete || - S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode)) + S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) || + (patch->is_new && !patch->direct_to_threeway) || + (patch->is_rename && !patch->lines_added && !patch->lines_deleted)) return -1; /* Preimage the patch was prepared for */
Certain invocations of "git apply --3way" will print error messages even though git is able to fall back on apply_fragments and apply the patch successfully with a return value of 0. To fix, return early from try_threeway() in the following cases: When the patch is a rename and no lines have changed. In this case, "git diff" doesn't record the blob info, so 3way is neither possible nor necessary. When the patch is an addition and there is no add/add conflict, i.e. direct_to_threeway is false. In this case, threeway will fail since the preimage is not in cache, but isn't necessary anyway since there is no conflict. Only error messaging is affected and other behavior does not change. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V1->V2: rebase onto master and rerun tests I think I addressed previous comments. What are the next steps for this patch? apply.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)