Patchwork [1,of,6] largefiles: when setting/clearing x bit on largefiles, don't change other bits

login
register
mail settings
Submitter Mads Kiilerich
Date Oct. 7, 2016, 11:26 p.m.
Message ID <96315a5833ed015acb7b.1475882765@localhost.localdomain>
Download mbox | patch
Permalink /patch/16901/
State Accepted
Headers show

Comments

Mads Kiilerich - Oct. 7, 2016, 11:26 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1475881180 -7200
#      Sat Oct 08 00:59:40 2016 +0200
# Node ID 96315a5833ed015acb7bd8f6d7f1e38db6fa9c50
# Parent  1779dde4c9ef97cb242f8d501655f236f66e5439
largefiles: when setting/clearing x bit on largefiles, don't change other bits

It is only the X bit that it matters to copy from the standin to the largefile
in the working directory. While it generally doesn't do any harm to copy the
whole mode, it is also "wrong" to copy more than the X bit we care about. It
can make a difference if someone should try to handle largefiles differently,
such as marking them read-only.

Thus, do similar to what utils.setflags does and set the X bit where there are
R bits and obey umask.
Yuya Nishihara - Oct. 13, 2016, 1:39 p.m.
On Sat, 08 Oct 2016 01:26:05 +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1475881180 -7200
> #      Sat Oct 08 00:59:40 2016 +0200
> # Node ID 96315a5833ed015acb7bd8f6d7f1e38db6fa9c50
> # Parent  1779dde4c9ef97cb242f8d501655f236f66e5439
> largefiles: when setting/clearing x bit on largefiles, don't change other bits

The series looks good to me. Queued up to the patch 5, thanks.

> It is only the X bit that it matters to copy from the standin to the largefile
> in the working directory. While it generally doesn't do any harm to copy the
> whole mode, it is also "wrong" to copy more than the X bit we care about. It
> can make a difference if someone should try to handle largefiles differently,
> such as marking them read-only.
> 
> Thus, do similar to what utils.setflags does and set the X bit where there are
> R bits and obey umask.
> 
> diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
> --- a/hgext/largefiles/lfcommands.py
> +++ b/hgext/largefiles/lfcommands.py
> @@ -515,9 +515,13 @@ def updatelfiles(ui, repo, filelist=None
>              rellfile = lfile
>              relstandin = lfutil.standin(lfile)
>              if wvfs.exists(relstandin):
> -                mode = wvfs.stat(relstandin).st_mode
> -                if mode != wvfs.stat(rellfile).st_mode:
> -                    wvfs.chmod(rellfile, mode)
> +                standinexec = wvfs.stat(relstandin).st_mode & 0o100
> +                st = wvfs.stat(rellfile).st_mode
> +                if standinexec != st & 0o100:
> +                    st &= ~0o111
> +                    if standinexec:
> +                        st |= (st >> 2) & 0o111 & ~util.umask

This 2-bit shift seemed a bit obscure. Any comment would help future readers.
Mads Kiilerich - Oct. 13, 2016, 1:46 p.m.
On 10/13/2016 03:39 PM, Yuya Nishihara wrote:
> On Sat, 08 Oct 2016 01:26:05 +0200, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1475881180 -7200
>> #      Sat Oct 08 00:59:40 2016 +0200
>> # Node ID 96315a5833ed015acb7bd8f6d7f1e38db6fa9c50
>> # Parent  1779dde4c9ef97cb242f8d501655f236f66e5439
>> largefiles: when setting/clearing x bit on largefiles, don't change other bits
> The series looks good to me. Queued up to the patch 5, thanks.

I have also looked into reworking it to increasing the default chunksize 
to 128k everywhere - that seems a bit cleaner in hindsight. I will send 
that version if you un-queue it again ;-)

/Mads
Yuya Nishihara - Oct. 13, 2016, 1:58 p.m.
On Thu, 13 Oct 2016 15:46:25 +0200, Mads Kiilerich wrote:
> On 10/13/2016 03:39 PM, Yuya Nishihara wrote:
> > On Sat, 08 Oct 2016 01:26:05 +0200, Mads Kiilerich wrote:
> >> # HG changeset patch
> >> # User Mads Kiilerich <madski@unity3d.com>
> >> # Date 1475881180 -7200
> >> #      Sat Oct 08 00:59:40 2016 +0200
> >> # Node ID 96315a5833ed015acb7bd8f6d7f1e38db6fa9c50
> >> # Parent  1779dde4c9ef97cb242f8d501655f236f66e5439
> >> largefiles: when setting/clearing x bit on largefiles, don't change other bits
> > The series looks good to me. Queued up to the patch 5, thanks.
> 
> I have also looked into reworking it to increasing the default chunksize 
> to 128k everywhere - that seems a bit cleaner in hindsight. I will send 
> that version if you un-queue it again ;-)

Unqueue everything?
Mads Kiilerich - Oct. 13, 2016, 2:03 p.m.
On 10/13/2016 03:58 PM, Yuya Nishihara wrote:
> On Thu, 13 Oct 2016 15:46:25 +0200, Mads Kiilerich wrote:
>> On 10/13/2016 03:39 PM, Yuya Nishihara wrote:
>>> The series looks good to me. Queued up to the patch 5, thanks. 
>> I have also looked into reworking it to increasing the default chunksize
>> to 128k everywhere - that seems a bit cleaner in hindsight. I will send
>> that version if you un-queue it again ;-)
> Unqueue everything?


I have no changes to the first two so it would be great to keep them ... 
but I could also easily resend them - whatever is least trouble ;-)

/Mads
Yuya Nishihara - Oct. 13, 2016, 2:13 p.m.
On Thu, 13 Oct 2016 16:03:03 +0200, Mads Kiilerich wrote:
> On 10/13/2016 03:58 PM, Yuya Nishihara wrote:
> > On Thu, 13 Oct 2016 15:46:25 +0200, Mads Kiilerich wrote:
> >> On 10/13/2016 03:39 PM, Yuya Nishihara wrote:
> >>> The series looks good to me. Queued up to the patch 5, thanks. 
> >> I have also looked into reworking it to increasing the default chunksize
> >> to 128k everywhere - that seems a bit cleaner in hindsight. I will send
> >> that version if you un-queue it again ;-)
> > Unqueue everything?
> 
> I have no changes to the first two so it would be great to keep them ... 
> but I could also easily resend them - whatever is least trouble ;-)

Okay, pruned the following changes from hg-committed:

6661746fd769 largefiles: use constant file chunk size instead of repeating 128 * 1024
5fc05f266ab0 largefiles: always use filechunkitersize when using filechunkiter
5f07c5a5c49a largefiles: always use filechunkiter when iterating files
Pierre-Yves David - Oct. 13, 2016, 3:57 p.m.
On 10/13/2016 04:13 PM, Yuya Nishihara wrote:
> On Thu, 13 Oct 2016 16:03:03 +0200, Mads Kiilerich wrote:
>> On 10/13/2016 03:58 PM, Yuya Nishihara wrote:
>>> On Thu, 13 Oct 2016 15:46:25 +0200, Mads Kiilerich wrote:
>>>> On 10/13/2016 03:39 PM, Yuya Nishihara wrote:
>>>>> The series looks good to me. Queued up to the patch 5, thanks.
>>>> I have also looked into reworking it to increasing the default chunksize
>>>> to 128k everywhere - that seems a bit cleaner in hindsight. I will send
>>>> that version if you un-queue it again ;-)
>>> Unqueue everything?
>>
>> I have no changes to the first two so it would be great to keep them ...
>> but I could also easily resend them - whatever is least trouble ;-)
>
> Okay, pruned the following changes from hg-committed:

Urg, direct pruning on hg-commmitted is problematic as it makes 
changesets disappear on the contributor see (see the related thread on 
the reviewers mailing list a while back and the drophack¹ extension (for 
reviewers only)).

Cheers,
Mads Kiilerich - Oct. 13, 2016, 4:03 p.m.
On 10/13/2016 05:57 PM, Pierre-Yves David wrote:
>
> On 10/13/2016 04:13 PM, Yuya Nishihara wrote:
>> Okay, pruned the following changes from hg-committed:
>
> Urg, direct pruning on hg-commmitted is problematic as it makes 
> changesets disappear on the contributor see (see the related thread on 
> the reviewers mailing list a while back and the drophack¹ extension 
> (for reviewers only)).

Don't worry; I don't use evolve - this is one of the reasons ;-)

/Mads
Yuya Nishihara - Oct. 14, 2016, 3:29 p.m.
On Thu, 13 Oct 2016 17:57:57 +0200, Pierre-Yves David wrote:
> On 10/13/2016 04:13 PM, Yuya Nishihara wrote:
> > On Thu, 13 Oct 2016 16:03:03 +0200, Mads Kiilerich wrote:
> >> On 10/13/2016 03:58 PM, Yuya Nishihara wrote:
> >>> On Thu, 13 Oct 2016 15:46:25 +0200, Mads Kiilerich wrote:
> >>>> On 10/13/2016 03:39 PM, Yuya Nishihara wrote:
> >>>>> The series looks good to me. Queued up to the patch 5, thanks.
> >>>> I have also looked into reworking it to increasing the default chunksize
> >>>> to 128k everywhere - that seems a bit cleaner in hindsight. I will send
> >>>> that version if you un-queue it again ;-)
> >>> Unqueue everything?
> >>
> >> I have no changes to the first two so it would be great to keep them ...
> >> but I could also easily resend them - whatever is least trouble ;-)
> >
> > Okay, pruned the following changes from hg-committed:
> 
> Urg, direct pruning on hg-commmitted is problematic as it makes
> changesets disappear on the contributor see (see the related thread on
> the reviewers mailing list a while back and the drophack¹ extension (for
> reviewers only)).
> 
> [1] https://www.mercurial-scm.org/repo/evolve/file/tip/hgext/drophack.py

Oh, but my name is not Matt Mackall. :-)

I pruned them assuming Mads would already start working on new version so
he would have new commits in his local repository.

Do we need to ssh into m-s-o and run "hg drop" ?
Pierre-Yves David - Oct. 14, 2016, 7:33 p.m.
On 10/14/2016 05:29 PM, Yuya Nishihara wrote:
> On Thu, 13 Oct 2016 17:57:57 +0200, Pierre-Yves David wrote:
>> On 10/13/2016 04:13 PM, Yuya Nishihara wrote:
>>> On Thu, 13 Oct 2016 16:03:03 +0200, Mads Kiilerich wrote:
>>>> On 10/13/2016 03:58 PM, Yuya Nishihara wrote:
>>>>> On Thu, 13 Oct 2016 15:46:25 +0200, Mads Kiilerich wrote:
>>>>>> On 10/13/2016 03:39 PM, Yuya Nishihara wrote:
>>>>>>> The series looks good to me. Queued up to the patch 5, thanks.
>>>>>> I have also looked into reworking it to increasing the default chunksize
>>>>>> to 128k everywhere - that seems a bit cleaner in hindsight. I will send
>>>>>> that version if you un-queue it again ;-)
>>>>> Unqueue everything?
>>>>
>>>> I have no changes to the first two so it would be great to keep them ...
>>>> but I could also easily resend them - whatever is least trouble ;-)
>>>
>>> Okay, pruned the following changes from hg-committed:
>>
>> Urg, direct pruning on hg-commmitted is problematic as it makes
>> changesets disappear on the contributor see (see the related thread on
>> the reviewers mailing list a while back and the drophack¹ extension (for
>> reviewers only)).
>>
>> [1] https://www.mercurial-scm.org/repo/evolve/file/tip/hgext/drophack.py
>
> Oh, but my name is not Matt Mackall. :-)
>
> I pruned them assuming Mads would already start working on new version so
> he would have new commits in his local repository.

That asumption might be a bit too optimistic in general (does not really 
matters for Mads as he is not pulling markers anyway.

Cheers,

Patch

diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
--- a/hgext/largefiles/lfcommands.py
+++ b/hgext/largefiles/lfcommands.py
@@ -515,9 +515,13 @@  def updatelfiles(ui, repo, filelist=None
             rellfile = lfile
             relstandin = lfutil.standin(lfile)
             if wvfs.exists(relstandin):
-                mode = wvfs.stat(relstandin).st_mode
-                if mode != wvfs.stat(rellfile).st_mode:
-                    wvfs.chmod(rellfile, mode)
+                standinexec = wvfs.stat(relstandin).st_mode & 0o100
+                st = wvfs.stat(rellfile).st_mode
+                if standinexec != st & 0o100:
+                    st &= ~0o111
+                    if standinexec:
+                        st |= (st >> 2) & 0o111 & ~util.umask
+                    wvfs.chmod(rellfile, st)
                     update1 = 1
 
             updated += update1