Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git-backport-diff: Kill git-log after a match has been found #5

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

XanClic
Copy link
Contributor

@XanClic XanClic commented Oct 15, 2019

Hi Jeff,

Thomas has tried running git-backport-diff on a rather large patch series (several hundred patches) and reported it behaved like a fork bomb after my previous patch.

It looks like in contrary to what I thought, the git-log process is actually not terminated when the while loop stops consuming its output, and so when you have a large patch series (and a large git history), you get many lingering git-log processes that all cause heavy I/O.

I think this patch should fix the problem. (Sorry! :/)

Max

@huth
Copy link

huth commented Oct 16, 2019

I've searched a little bit and came accross this discussion:
https://stackoverflow.com/questions/81520/how-to-suppress-terminated-message-after-killing-in-bash#answer-5722874
And indeed, if I add a " wait %$job_id 2>/dev/null || true " after the kill line, the messages are gone for me, too. So please either add the "wait" line or the "-PIPE" here.

@XanClic
Copy link
Contributor Author

XanClic commented Oct 16, 2019

Ah, good, thanks. I had a wait in a previous version but decided to drop it because I didn’t see the need for it. So I’ll add a wait there (should even work without the %$job_id).

Max

@XanClic
Copy link
Contributor Author

XanClic commented Oct 17, 2019

I’ve force-pushed an updated commit (with a simple “wait &>/dev/null” added). Sorry, I don’t know how this is usually done on github. (I suppose I should have pushed the original patch not to master but to a sub-branch and then created a different pull request for a new branch now?)

So here’s the backport-diff (:wink:):

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/1:[0001] [FC] 'git-backport-diff: Kill git-log after match found'

And this is the relevant diff between the patches:

@@ -52,6 +52,7 @@
 +
 +        # Ignore errors
 +        kill %$job_id &>/dev/null || true
++        wait &> /dev/null
 +        rm -f "$git_fifo"
  
          if [[ -n "$uphash" ]]

d0c8cbd has indeed made finding a match faster, but the downside
is that the git-log process continues to run in the background even when
we no longer consume its output.  This is a problem particularly for
large patch series, where git-backport-diff may thus spawn hundreds of
subprocesses.

We don't need the git-log process after we found a match, so make it a
real job instead of an anonymous subprocess, which allows us to
terminate it after we have found a match.

Reported-by: Thomas Huth <[email protected]>
Fixes: d0c8cbd
Sometimes a commit title changes across different versions of a series.
This option allows the user to treat the new title as if it were the old
one so we avoid false “[down]”s.

(Not submitting to usptream because I feel like this is a misfeature, in
a way.)
I have no idea what this is, but it is here, so commit (to) it.

Signed-off-by: Hanna Reitz <[email protected]>
Allow setting options for `git diff` via `$DIFFOPTS`.

Signed-off-by: Hanna Reitz <[email protected]>
The command `egrep` is now considered obsolete (and prints that to
stderr), replace it by `grep -E`.

Signed-off-by: Hanna Reitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants