threads / rfc / 39521

[RFC] git-am: handling unborn branches

Subject: [RFC] git-am: handling unborn branches

## tl;dr

8 messages between Jun 4, 2015 and Jun 5, 2015.

replies: 7people: 3as markdown or json

Paul Tan· Jun 4, 2015, 10:34 UTC · lore
Hi,

git-am generally supports applying patches to unborn branches. However, there are 2 cases where git-am does not handle unborn branches which I would like to address before the git-am rewrite to C:

1. am --skip

For git am --skip, git-am.sh does a fast-forward checkout from HEAD to HEAD, discarding unmerged entries, and then resets the index to HEAD so that the index is not dirty.

        git read-tree --reset -u HEAD HEAD
        orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
        git reset HEAD
        git update-ref ORIG_HEAD $orig_head

This requires a valid HEAD. Since git-am requires an empty index for unborn branches in the patch application stage anyway, I think we should discard all entires in the index if we are on an unborn branch?

Or, the current behavior of git-am.sh will print some scary errors about the missing HEAD, but will then continue on to the next patch. If the index is not clean, it will then error out. Should we preserve this behavior? (without the errors about the missing HEAD)

2. am --abort

For git am --abort, git-am.sh does something similar. It does a fast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged entries, and then resets the index to ORIG_HEAD so that local changes will be unstaged.

        if safe_to_abort
        then
            git read-tree --reset -u HEAD ORIG_HEAD
            git reset ORIG_HEAD
        fi
        rm -fr "$dotest"

This, however, requires a valid HEAD and ORIG_HEAD. If we don't have a HEAD or ORIG_HEAD (because we were on an unborn branch when we started git am), what should we do? (Note: safe_to_abort returns true if we git am with no HEAD because $dotest/abort-safety will not exist) Should we discard all entires in the index as well? (Since we might think of the "original HEAD" as an empty tree?)

Or, the current behavior of git-am.sh will print some scary errors about the missing HEAD and ORIG_HEAD, but will not touch the index at all, and still delete the rebase-apply directory. Should we preserve this behavior (without the errors)?

Thanks, Paul

Junio C Hamano· Jun 4, 2015, 17:26 UTC · re: Paul Tan · lore

Re: [RFC] git-am: handling unborn branches

Paul Tan <pyokagan@gmail.com> writes:
> git-am generally supports applying patches to unborn branches.
> However, there are 2 cases where git-am does not handle unborn
> branches which I would like to address before the git-am rewrite to C:
Show 14 quoted lines
> 1. am --skip
>
> For git am --skip, git-am.sh does a fast-forward checkout from HEAD to
> HEAD, discarding unmerged entries, and then resets the index to HEAD
> so that the index is not dirty.
>
>         git read-tree --reset -u HEAD HEAD
>         orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
>         git reset HEAD
>         git update-ref ORIG_HEAD $orig_head
>
> This requires a valid HEAD. Since git-am requires an empty index for
> unborn branches in the patch application stage anyway, I think we
> should discard all entires in the index if we are on an unborn branch?

Yes, and it should also remove the new files the failed application brought in to the working tree, if any, to match the "--skip" done in the normal case (i.e. when we already have a history to apply patches to), I would think.

Show 6 quoted lines
> 2. am --abort
>
> For git am --abort, git-am.sh does something similar. It does a
> fast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged
> entries, and then resets the index to ORIG_HEAD so that local changes
> will be unstaged.

In general, the "apply to nothing" is more or less an afterthought and was not done as carefully as the rest of the program, so view whenever you see a strange behaviour as not a "strange spec" but likely to be a bug. You would do OK if you imagine what should happen if you were doing the same operation on top of a commit that records an empty tree and try to match the behaviour to that case.

Thanks.
Paul Tan· Jun 5, 2015, 06:37 UTC · re: Junio C Hamano · lore

Re: [RFC] git-am: handling unborn branches

On Fri, Jun 5, 2015 at 1:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Paul Tan <pyokagan@gmail.com> writes:
>
>> git-am generally supports applying patches to unborn branches.
>> However, there are 2 cases where git-am does not handle unborn
>> branches which I would like to address before the git-am rewrite to C:
>
>> 1. am --skip
>>
>> For git am --skip, git-am.sh does a fast-forward checkout from HEAD to
>> HEAD, discarding unmerged entries, and then resets the index to HEAD
>> so that the index is not dirty.
>>
>>         git read-tree --reset -u HEAD HEAD
>>         orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
>>         git reset HEAD
>>         git update-ref ORIG_HEAD $orig_head
>>
>> This requires a valid HEAD. Since git-am requires an empty index for
>> unborn branches in the patch application stage anyway, I think we
>> should discard all entires in the index if we are on an unborn branch?
>
> Yes, and it should also remove the new files the failed application
> brought in to the working tree, if any, to match the "--skip" done
> in the normal case (i.e. when we already have a history to apply
> patches to), I would think.

Hmm, actually git-am.sh doesn't seem to do that even when we have a history to apply patches to. This is okay in the non-3way case, as git-apply will check to see if the patch applies before it modifies the index, but if we fall back on 3-way merge, any new files the failed merge added will not be deleted in the "git read-tree --reset -u HEAD HEAD".

Should we do that? I dunno, but if we want to introduce this feature, I think a "git read-tree --reset HEAD HEAD && git read-tree -m -u $(git write-tree) HEAD" will do the trick? (We discard all unmerged entries, then fast-forward from the tree the failed merged left us with back to HEAD).

Clearing the index in the unborn branch case seems to be the most consistent thing to do for now (for the purpose of the git-am rewrite).

Thanks, Paul

Junio C Hamano· Jun 5, 2015, 15:36 UTC · re: Paul Tan · lore

Re: [RFC] git-am: handling unborn branches

Paul Tan <pyokagan@gmail.com> writes:
Show 8 quoted lines
> Hmm, actually git-am.sh doesn't seem to do that even when we have a
> history to apply patches to. This is okay in the non-3way case, as
> git-apply will check to see if the patch applies before it modifies
> the index, but if we fall back on 3-way merge, any new files the
> failed merge added will not be deleted in the "git read-tree --reset
> -u HEAD HEAD".
>
> Should we do that?

That sounds like the right thing to do; I agree that fixing it may or may not be outside the scope of the immediate series.

Paul Tan· Jun 5, 2015, 16:26 UTC · re: Junio C Hamano · lore

Re: [RFC] git-am: handling unborn branches

On Fri, Jun 5, 2015 at 11:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Paul Tan <pyokagan@gmail.com> writes:
>
>> Hmm, actually git-am.sh doesn't seem to do that even when we have a
>> history to apply patches to. This is okay in the non-3way case, as
>> git-apply will check to see if the patch applies before it modifies
>> the index, but if we fall back on 3-way merge, any new files the
>> failed merge added will not be deleted in the "git read-tree --reset
>> -u HEAD HEAD".
>>
>> Should we do that?
>
> That sounds like the right thing to do; I agree that fixing it may
> or may not be outside the scope of the immediate series.

Hmm, thinking about it, the equivalent C code would be greatly affected by whatever behavior we go with, so I think we should try fixing the behavior first.

This was done really quickly, but I think this may fix it:
diff --git a/git-am.sh b/git-am.sh
index 761befb..f7d54bf 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -502,7 +502,9 @@ then
         ;;
     t,)
         git rerere clear
-        git read-tree --reset -u HEAD HEAD
+        git read-tree --reset HEAD HEAD &&
+        our_tree=$(git write-tree) &&
+        git read-tree -m -u $our_tree HEAD
         orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
         git reset HEAD
         git update-ref ORIG_HEAD $orig_head
diff --git a/t/t4153-am-clean-index.sh b/t/t4153-am-clean-index.sh
new file mode 100755
index 0000000..6d696db
--- /dev/null
+++ b/t/t4153-am-clean-index.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='test clean index with am'
+. ./test-lib.sh
+
+test_expect_success setup '
+    test_commit initial file &&
+    test_commit master-commit file &&
+    git checkout -b conflict master^ &&
+    echo conflict-commit >file &&
+    echo newfile >newfile &&
+    git add newfile &&
+    test_tick &&
+    git commit -a -m conflict-commit &&
+    git format-patch --stdout initial >conflict.patch &&
+    git checkout master
+'
+
+test_expect_success 'am -3 pauses on conflict' '
+    test_must_fail git am -3 conflict.patch &&
+    echo newfile >expected &&
+    test_cmp newfile expected
+'
+
+test_expect_success 'am --skip removes newfile' '
+    git am --skip &&
+    test_path_is_missing newfile
+'
+
+test_done

However, it assumes that the contents of the index are from the failed
merge. If the user modified the index before running git-am --skip,
e.g. the user added a file, then that file would be deleted, which may
not be desired...

Thanks,
Paul
Junio C Hamano· Jun 5, 2015, 16:33 UTC · re: Paul Tan · lore

Re: [RFC] git-am: handling unborn branches

Paul Tan <pyokagan@gmail.com> writes:
> Hmm, thinking about it, the equivalent C code would be greatly
> affected by whatever behavior we go with, so I think we should try
> fixing the behavior first.

I am glad to see that sometimes people see the light when I say one of the greatest strength in scripted Porcelains is that they are far easier to modify than those writtein in C ;-)

Show 6 quoted lines
> This was done really quickly, but I think this may fix it:
> ...
> However, it assumes that the contents of the index are from the failed
> merge. If the user modified the index before running git-am --skip,
> e.g. the user added a file, then that file would be deleted, which may
> not be desired...

I do not think that is worth worrying about; if users made changes unrelated to what "git am" asked them to help it apply by modifying the working tree and updating the index, don't they "lose" their changes the same way anyway, whether it is a new file or an existing one?

Stefan Beller· Jun 4, 2015, 17:27 UTC · re: Paul Tan · lore

Re: [RFC] git-am: handling unborn branches

On Thu, Jun 4, 2015 at 3:34 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 20 quoted lines
> Hi,
>
> git-am generally supports applying patches to unborn branches.
> However, there are 2 cases where git-am does not handle unborn
> branches which I would like to address before the git-am rewrite to C:
>
> 1. am --skip
>
> For git am --skip, git-am.sh does a fast-forward checkout from HEAD to
> HEAD, discarding unmerged entries, and then resets the index to HEAD
> so that the index is not dirty.
>
>         git read-tree --reset -u HEAD HEAD
>         orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
>         git reset HEAD
>         git update-ref ORIG_HEAD $orig_head
>
> This requires a valid HEAD. Since git-am requires an empty index for
> unborn branches in the patch application stage anyway, I think we
> should discard all entires in the index if we are on an unborn branch?
That makes sense.
Show 31 quoted lines
>
> Or, the current behavior of git-am.sh will print some scary errors
> about the missing HEAD, but will then continue on to the next patch.
> If the index is not clean, it will then error out. Should we preserve
> this behavior? (without the errors about the missing HEAD)
>
> 2. am --abort
>
> For git am --abort, git-am.sh does something similar. It does a
> fast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged
> entries, and then resets the index to ORIG_HEAD so that local changes
> will be unstaged.
>
>         if safe_to_abort
>         then
>             git read-tree --reset -u HEAD ORIG_HEAD
>             git reset ORIG_HEAD
>         fi
>         rm -fr "$dotest"
>
> This, however, requires a valid HEAD and ORIG_HEAD. If we don't have a
> HEAD or ORIG_HEAD (because we were on an unborn branch when we started
> git am), what should we do? (Note: safe_to_abort returns true if we
> git am with no HEAD because $dotest/abort-safety will not exist)
> Should we discard all entires in the index as well? (Since we might
> think of the "original HEAD" as an empty tree?)
>
> Or, the current behavior of git-am.sh will print some scary errors
> about the missing HEAD and ORIG_HEAD, but will not touch the index at
> all, and still delete the rebase-apply directory. Should we preserve
> this behavior (without the errors)?
I guess so, looking at the documentation
       --abort
           Restore the original branch and abort the patching operation.

a user may want to not go to the unborn branch, but rather to the previous HEAD?

>
> Thanks,
> Paul
Paul Tan· Jun 5, 2015, 08:32 UTC · re: Stefan Beller · lore

Re: [RFC] git-am: handling unborn branches

On Fri, Jun 5, 2015 at 1:27 AM, Stefan Beller <sbeller@google.com> wrote:
Show 38 quoted lines
> On Thu, Jun 4, 2015 at 3:34 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> Or, the current behavior of git-am.sh will print some scary errors
>> about the missing HEAD, but will then continue on to the next patch.
>> If the index is not clean, it will then error out. Should we preserve
>> this behavior? (without the errors about the missing HEAD)
>>
>> 2. am --abort
>>
>> For git am --abort, git-am.sh does something similar. It does a
>> fast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged
>> entries, and then resets the index to ORIG_HEAD so that local changes
>> will be unstaged.
>>
>>         if safe_to_abort
>>         then
>>             git read-tree --reset -u HEAD ORIG_HEAD
>>             git reset ORIG_HEAD
>>         fi
>>         rm -fr "$dotest"
>>
>> This, however, requires a valid HEAD and ORIG_HEAD. If we don't have a
>> HEAD or ORIG_HEAD (because we were on an unborn branch when we started
>> git am), what should we do? (Note: safe_to_abort returns true if we
>> git am with no HEAD because $dotest/abort-safety will not exist)
>> Should we discard all entires in the index as well? (Since we might
>> think of the "original HEAD" as an empty tree?)
>>
>> Or, the current behavior of git-am.sh will print some scary errors
>> about the missing HEAD and ORIG_HEAD, but will not touch the index at
>> all, and still delete the rebase-apply directory. Should we preserve
>> this behavior (without the errors)?
>
> I guess so, looking at the documentation
>        --abort
>            Restore the original branch and abort the patching operation.
>
> a user may want to not go to the unborn branch, but rather to the previous
> HEAD?

I think we need to be consistent with the non-unborn-branch behavior of git-am. In normal use am --abort would reset the branch back to ORIG_HEAD, which is the HEAD when am was first run, thus losing all the applied commits. However, since f79d4c8 (git-am: teach git-am to apply a patch to an unborn branch, 2009-04-10) if git-am is run without a HEAD, no ORIG_HEAD would be created.

I guess if we are to follow this normal behavior, we need to destroy the current branch as well.

So the abort logic would be something like:
1. If we are still on an unborn branch, we clear the index.
2. If we are not on an unborn branch, but we have no ORIG_HEAD because
we started from an unborn branch, then we destroy the current branch.
3. If we are not on an unborn branch, and we have an ORIG_HEAD, then
we do the normal behavior of fast-forwarding and resetting the index
to ORIG_HEAD.

We also need to consider safe_to_abort() as well though, which was introduced in 7b3b7e3 (am --abort: keep unrelated commits since the last failure and warn, 2010-12-21). Specifically,

safe_to_abort () {
...
    if ! test -s "$dotest/abort-safety"
    then
        return 0
    fi
...
}

where we are allowed to reset HEAD if the abort-safety file is empty or does not exist. If patch application failed while we are on an unborn branch, we will have no abort-safety file. However, the user may have made commits since then, and we should not make the user lose those commits. As such, we should probably not reset HEAD if there is no abort-safety file.

Thanks, Paul

← back to recent threads