git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Pete Wyckoff <pw@padd.com>
To: Vitor Antunes <vitor.hda@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/2] git-p4: Improve rename detection support.
Date: Sat, 5 Feb 2011 19:21:13 -0500	[thread overview]
Message-ID: <20110206002113.GA31245@arf.padd.com> (raw)
In-Reply-To: <1296429563-18390-1-git-send-email-vitor.hda@gmail.com>

vitor.hda@gmail.com wrote on Sun, 30 Jan 2011 23:19 +0000:
> Only open files for edit after integrating if the SHA1 of source and destination
> differ from each other.
> Add git config option detectRenames to allow permanent rename detection. This
> options should be set to a true/false value.

I like the idea.

>      def applyCommit(self, id):
>          print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
> -        diffOpts = ("", "-M")[self.detectRename]
> +
> +        detectRenames = gitConfig("git-p4.detectRenames")
> +        if len(detectRenames) and detectRenames.lower() != "false" > 0:
> +            diffOpts = "-M"
> +        else:
> +            diffOpts = ("", "-M")[self.detectRename]

The comparisons confuse me.  detectRenames != "false" > 0  ?
How about just detectRenames == "true"?

You could rename the existing self.detectRename to add an "s" so
it all makes a bit more sense.

    if not self.detectRenames:
	# not explicitly set, check the config variable
	b = gitConfig("git-p4.detectRenames")
	if b == "true":
	    self.detectRenames = "-M"

    if self.detectRenames:
	diffOpts = "-M"
    else:
	diffOpts = ""

>          diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (diffOpts, id, id))
>          filesToAdd = set()
>          filesToDelete = set()
> @@ -640,7 +646,8 @@ class P4Submit(Command):
>              elif modifier == "R":
>                  src, dest = diff['src'], diff['dst']
>                  p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
> -                p4_system("edit \"%s\"" % (dest))
> +                if diff['src_sha1'] != diff['dst_sha1']:
> +                    p4_system("edit \"%s\"" % (dest))
>                  if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
>                      filesToChangeExecBit[dest] = diff['dst_mode']
>                  os.unlink(dest)

If you rename the file and also cause its perms to change (say
add +x), does it still work if dest is not open?

		-- Pete

  parent reply	other threads:[~2011-02-06  0:21 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-30 23:19 [PATCH 1/2] git-p4: Improve rename detection support Vitor Antunes
2011-01-30 23:19 ` [PATCH 2/2] git-p4: Add copy " Vitor Antunes
2011-02-06  0:25   ` Pete Wyckoff
2011-02-06 17:25     ` Vitor Antunes
2011-02-06 22:05       ` Pete Wyckoff
2011-02-07 11:11         ` Vitor Antunes
2011-02-12  0:29           ` Vitor Antunes
2011-02-12 13:32             ` Pete Wyckoff
2011-02-06  0:21 ` Pete Wyckoff [this message]
2011-02-06 12:39   ` [PATCH 1/2] git-p4: Improve rename " Vitor Antunes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110206002113.GA31245@arf.padd.com \
    --to=pw@padd.com \
    --cc=git@vger.kernel.org \
    --cc=vitor.hda@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).