In git p4, git fast-import fails from die-now command and err (from Perforce) is not shown

[alrashedf]

When importing from Perforce using git p4 clone <depot location>, cloning works fine until Perforce command p4 raises an error. This error message is stored in err variable then git-fast-import is sent a die-now command to kill it. An exception is raised fatal: Unsupported command: die-now.

This patch forces python to call die() with the err message returned from Perforce.

This commit fixes the root cause of a bug that took me hours to find. I'm sure many faced the cryptic error and declared that git-p4 is not working for them.

cc: Karthik Nayak [email protected]

[gitgitgadget-git]

Welcome to GitGitGadget

Hi @alrashedf, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <[email protected]>, Ill Takalook <[email protected]>

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget-git]

There are issues in commit 4ae24ea:
Fixed a bug were the err message does not show because git-fast-import exits with an exception: fatal: Unsupported command: die-now
Commit checks stopped - the message is too short
First line of commit message is too long (> 76 columns)
Commit not signed off

[dscho]

/allow

[dscho]

@alrashedf please be inspired by https://github.blog/2022-06-30-write-better-commits-build-better-projects/

[gitgitgadget-git]

User alrashedf is now allowed to use GitGitGadget.

[alrashedf]

@alrashedf please be inspired by https://github.blog/2022-06-30-write-better-commits-build-better-projects/

I was inspired and rewrote the pull request message above.

[gitgitgadget-git]

There are issues in commit 1665860:
Fixed a bug were the err message does not show because git-fast-import exits with an exception: fatal: Unsupported command: die-now
Commit checks stopped - the message is too short
First line of commit message is too long (> 76 columns)
Commit not signed off

[dscho]

I was inspired and rewrote the pull request message above.

Excellent. Please amend the commit message now, that'll calm down GitGitGadget.

[gitgitgadget-git]

There are issues in commit 064615e:
Fixed a bug were the err message does not show because git-fast-import exits with an exception: fatal: Unsupported command: die-now
Commit checks stopped - the message is too short
First line of commit message is too long (> 76 columns)
Commit not signed off

[gitgitgadget-git]

There are issues in commit 23d3f8a:
Bug fix: During "git p4 clone" if p4 process returns
Commit not signed off
The first line must be separated from the rest by an empty line

[gitgitgadget-git]

There are issues in commit fe69a4d:
Bug fix: ensure P4 "err" is displayed when exception is raised.
Commit not signed off

[alrashedf]

@dscho It's been a few months since the pull request has passed all tests. When will it be merged?

[dscho]

@alrashedf as described in https://gitgitgadget.github.io/:

Unlike most open source projects, Git itself does not accept code contributions via Pull Requests. Instead, patches are submitted to the Git mailing list for review and will be applied manually by the Git maintainer.

As per #1668 (comment), you can use this here PR to make such a contribution by calling the /submit command (see e.g. this example and GitGitGadget's response after sending the patch out).

You will want to make sure that the initial comment, which is used as a "cover letter" between the commit message and the patch, does not repeat the commit message, but conveys any relevant context that the commit message cannot convey, e.g. motivating the inclusion of this patch by, say, stating that your work is blocked on the fixed bug.

[alrashedf]

/submit

[gitgitgadget-git]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1668/alrashedf/master-v1

To fetch this version to local tag pr-git-1668/alrashedf/master-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1668/alrashedf/master-v1

[gitgitgadget-git]

On the Git mailing list, Karthik Nayak wrote (reply to this):

Hello Fahad,

"Fahad Alrashed via GitGitGadget" <[email protected]> writes:

> From: Fahad Alrashed <[email protected]>
>
> Bug fix: During "git p4 clone" if p4 process returns

Nit: I haven't seen us use a prefix for the commit message. Also we use
a max line length of 72 characters (see .editorconfig). The commit
message seems to be wrapped at a much lower threshold.

> an error from the server, it will store it in variable

perhaps `in the 'err' variable` or `in a variable 'err'`.

> "err". The it will send a text command "die-now" to

s/The/Then

> git-fast-import. However, git-fast-import raises an
> exception: "fatal: Unsupported command: die-now"
> and err is never displayed. This patch ensures that
> err is dispayed using "finally:"
>
> Signed-off-by: Fahad Alrashed <[email protected]>
> ---
>     In git p4, git fast-import fails from die-now command and err (from
>     Perforce) is not shown
>
>     When importing from Perforce using git p4 clone <depot location>,
>     cloning works fine until Perforce command p4 raises an error. This error
>     message is stored in err variable then git-fast-import is sent a die-now
>     command to kill it. An exception is raised fatal: Unsupported command:
>     die-now.
>
>     This patch forces python to call die() with the err message returned
>     from Perforce.
>
>     This commit fixes the root cause of a bug that took me hours to find.
>     I'm sure many faced the cryptic error and declared that git-p4 is not
>     working for them.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1668%2Falrashedf%2Fmaster-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1668/alrashedf/master-v1
> Pull-Request: https://github.com/git/git/pull/1668
>
>  git-p4.py | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 28ab12c72b6..f1ab31d5403 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):
>              if self.stream_have_file_info:
>                  if "depotFile" in self.stream_file:
>                      f = self.stream_file["depotFile"]
> -            # force a failure in fast-import, else an empty
> -            # commit will be made
> -            self.gitStream.write("\n")
> -            self.gitStream.write("die-now\n")
> -            self.gitStream.close()
> -            # ignore errors, but make sure it exits first
> -            self.importProcess.wait()
> -            if f:
> -                die("Error from p4 print for %s: %s" % (f, err))
> -            else:
> -                die("Error from p4 print: %s" % err)
> +            try:
> +                # force a failure in fast-import, else an empty
> +                # commit will be made
> +                self.gitStream.write("\n")
> +                self.gitStream.write("die-now\n")
> +                self.gitStream.close()
> +                # ignore errors, but make sure it exits first
> +                self.importProcess.wait()
> +            finally:
> +                if f:
> +                    die("Error from p4 print for %s: %s" % (f, err))
> +                else:
> +                    die("Error from p4 print: %s" % err)
>

This part looks good.

>          if 'depotFile' in marshalled and self.stream_have_file_info:
>              # start of a new file - output the old one first
>
> base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821
> --
> gitgitgadget

Thanks,
Karthik

[gitgitgadget-git]

User Karthik Nayak <[email protected]> has been added to the cc: list.

[alrashedf]

I've corrected the commit message as pointed out by Karthik.

/submit

[dscho]

You have to put the /submit command in a separate comment.

[alrashedf]

@dscho The check titled "CI / osx-reftable (macos-13)" is failing after timing out after 6 hours. It didn't fail previously.

What changed?

[dscho]

The check titled "CI / osx-reftable (macos-13)" is failing after timing out after 6 hours.

@alrashedf unfortunately, the p4 tests became flaky on macOS. Don't worry about it.

[alrashedf]

/submit

[gitgitgadget-git]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1668/alrashedf/master-v2

To fetch this version to local tag pr-git-1668/alrashedf/master-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1668/alrashedf/master-v2

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Fahad Alrashed via GitGitGadget" <[email protected]> writes:

> From: Fahad Alrashed <[email protected]>
> Subject: Re: [PATCH v2] Bug fix: ensure P4 "err" is displayed when exception is raised.

Our convention is to use the name of the area as the prefix, e.g.,

	git-p4: show Perforce error to the user

in the title.  Documentation/SubmittingPatches has more guidance on
the title, like omitting the full-stop at the end, and on the
proposed log message in general.

> During "git p4 clone" if p4 process returns an error from the server,
> it will store the message in the 'err' variable. Then it will send a
> text command "die-now" to git-fast-import. However, git-fast-import
> raises an exception: "fatal: Unsupported command: die-now" and err is
> never displayed.

Nicely explained.

> This patch ensures that err is dispayed using
> "finally:".

Instead, we write it more like

	Ensure that err is shown to the end user.

The implementation (i.e., catching an error with try/finally) can be
seen from the patch text.  What is more important to capture in the
proposed log message is what _effect_ the commit wanted to make,
which is a good way to show _why_ we are making the change.

The intent of the original code being to always die at this point.
Even though it is necessary to enclose the earlier part up to
.wait() inside a try block to catch an exception, the two calls to
die() do not have to be inside finally block (in other words,
finally block could be just a no-op, and the die() calls can be done
after the whole try/finally block that kills the fast-import by
sending die-now and makes sure .wait() sees the process go), which
may have made the intent of the fix even more clear, which is "we
use the try/finally construct only to work around an exception
thrown by killing the fast-import process".

The patch posted as-is is not wrong per-se, though.

Thanks.

> diff --git a/git-p4.py b/git-p4.py
> index 28ab12c72b6..f1ab31d5403 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -3253,17 +3253,19 @@ def streamP4FilesCb(self, marshalled):
>              if self.stream_have_file_info:
>                  if "depotFile" in self.stream_file:
>                      f = self.stream_file["depotFile"]
> -            # force a failure in fast-import, else an empty
> -            # commit will be made
> -            self.gitStream.write("\n")
> -            self.gitStream.write("die-now\n")
> -            self.gitStream.close()
> -            # ignore errors, but make sure it exits first
> -            self.importProcess.wait()
> -            if f:
> -                die("Error from p4 print for %s: %s" % (f, err))
> -            else:
> -                die("Error from p4 print: %s" % err)
> +            try:
> +                # force a failure in fast-import, else an empty
> +                # commit will be made
> +                self.gitStream.write("\n")
> +                self.gitStream.write("die-now\n")
> +                self.gitStream.close()
> +                # ignore errors, but make sure it exits first
> +                self.importProcess.wait()
> +            finally:
> +                if f:
> +                    die("Error from p4 print for %s: %s" % (f, err))
> +                else:
> +                    die("Error from p4 print: %s" % err)
>  
>          if 'depotFile' in marshalled and self.stream_have_file_info:
>              # start of a new file - output the old one first
>
> base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821

[alrashedf]

/submit

[gitgitgadget-git]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1668/alrashedf/master-v3

To fetch this version to local tag pr-git-1668/alrashedf/master-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1668/alrashedf/master-v3

[gitgitgadget-git]

This branch is now known as fa/p4-error.

[gitgitgadget-git]

This patch series was integrated into seen via 3f7e03d.

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Fahad Alrashed via GitGitGadget" <[email protected]> writes:

> From: Fahad Alrashed <[email protected]>
>
> During "git p4 clone" if p4 process returns an error from the server,
> it will store the message in the 'err' variable. Then it will send a
> text command "die-now" to git-fast-import. However, git-fast-import
> raises an exception: "fatal: Unsupported command: die-now" and err is
> never displayed. This patch ensures that err is shown to the end user.
>
> Signed-off-by: Fahad Alrashed <[email protected]>
> ---

Looking good.  Queued.  Thanks.

[gitgitgadget-git]

This patch series was integrated into seen via 3367c07.

[gitgitgadget-git]

This patch series was integrated into seen via 18078f4.

[gitgitgadget-git]

There was a status update in the "New Topics" section about the branch fa/p4-error on the Git mailing list:

P4 update.

Will merge to 'next'.
source: <[email protected]>

[gitgitgadget-git]

This patch series was integrated into seen via 3367c07.

[gitgitgadget-git]

This patch series was integrated into seen via e187166.

[gitgitgadget-git]

This patch series was integrated into seen via 1c84a95.

[gitgitgadget-git]

This patch series was integrated into next via 58fd3fb.

[gitgitgadget-git]

There was a status update in the "Cooking" section about the branch fa/p4-error on the Git mailing list:

P4 update.

Will merge to 'master'.
source: <[email protected]>

[gitgitgadget-git]

This patch series was integrated into seen via bbffcd4.

[gitgitgadget-git]

This patch series was integrated into master via bbffcd4.

[gitgitgadget-git]

This patch series was integrated into next via bbffcd4.

[gitgitgadget-git]

Closed via bbffcd4.