[RFC] git-am: handling unborn branches

[RFC] git-am: handling unborn branches

[Paul Tan]

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

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

[Junio C Hamano]

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.

> 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.

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

[Paul Tan]

On Fri, Jun 5, 2015 at 1:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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.

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

[Paul Tan]

On Fri, Jun 5, 2015 at 11:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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 ;-)

> 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?

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

[Stefan Beller]

On Thu, Jun 4, 2015 at 3:34 AM, Paul Tan <pyokagan@gmail.com> wrote:
> 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.

>
> 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

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

[Paul Tan]

On Fri, Jun 5, 2015 at 1:27 AM, Stefan Beller <sbeller@google.com> wrote:
> 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