Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4569 closed defect (fixed)

dependencies are not ignored when using git due to ensure_dependencies.py

Reported by: erikvold Assignee: trev
Priority: P2 Milestone:
Module: Automation Keywords:
Cc: trev, kzar Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29359969/

Description (last modified by trev)

How to reproduce

  1. git clone https://github.com/adblockplus/adblockplus newdir
  2. ./build.py build
  3. git status

Observed behaviour

Multiple dependencies which ensure_dependencies.py has downloaded are not ignored, and are listed as changes when using git status.

Expected behaviour

All of the dependencies which ensure_dependencies.py has downloaded are ignored, and are not listed as changes when using git status.

Summary

Basically I think that https://github.com/adblockplus/adblockplus/blob/master/ensure_dependencies.py#L129-L132 has the issue somewhere.

I wonder if we really need this code though, I think we will constantly run into bugs or git/hg updates that need addressing which will cause this code to break. Would it not just be easier to explicitly list the dependences as folders to ignore in ignore files? and remove this code.

Background

Function _ensure_line_exists() always removes all previous content from the file instead of merely adding a line. It doesn't seek before reading file's contents, so it attempts reading from the end of the file. As a result, it doesn't read anything.

Change History (12)

comment:1 Changed 3 years ago by trev

  • Cc trev kzar added

comment:2 Changed 3 years ago by kzar

I can't reproduce this as described (I'm assuming step 1a. is cd newdir), the dependencies are ignored by Git as expected. What version of Git and Python do you have? (I have Git 2.9.3 and Python 2.7.12+.) Also just a hunch but are you using a case insensitive filesystem?

comment:3 Changed 3 years ago by trev

I can reproduce that on Mac OS X with Git 2.9.2 and Python 2.7.12. git status command shows adblockpluscore/ and buildtools/ as untracked but not adblockplusui/. The file system is case insensitive but that's not the reason - .git/info/exclude file only lists /adblockplusui but not the other dependencies.

comment:4 Changed 3 years ago by trev

  • Description modified (diff)
  • Owner set to trev

I identified the bug in _ensure_line_exists() function, added the analysis to the description.

comment:5 Changed 3 years ago by trev

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:6 Changed 3 years ago by kzar

It's weird this bug didn't affect me, but your explanation and solution makes sense. (I even tested opening a file in "a+" mode and found the handle pointed to the start of the file. I guess my version of Python just behaves differently here.)

comment:7 Changed 3 years ago by trev

  • Ready set

@kzar: Given that you approved my patch, I guess that you consider this issue ready ;)

comment:9 Changed 3 years ago by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:10 Changed 3 years ago by kzar

  • Priority changed from Unknown to P2

comment:11 Changed 3 years ago by wspee

  • Blocking 4715 added

comment:12 Changed 3 years ago by wspee

  • Blocking 4715 removed
Note: See TracTickets for help on using tickets.