[PATCH] D15465: [git-clang-format]: New option to perform formatting against staged changes only

[Alexander Shukaev via Phabricator via cfe-commits]

Alexander-Shukaev updated this revision to Diff 112801.
Alexander-Shukaev added a comment.

Alright, so you got me excited about this task once again.  Now, I've just 
rebased to the latest `git-clang-format` and it has changed a bit.  
Nevertheless, I've updated the previous patch accordingly and applied those 
changes which @lodato was proposing (except for getting rid of 
`run_git_ls_files_and_save_to_tree` as I'm still not sure whether this would be 
the same as `git write-tree`).  Anyway, just tested the exact scenario posted 
by @lodato, and indeed with the current patch it works as he expects.  Namely, 
there is no diff from unstaged tree interfering anymore.  So far so good, but 
as I said in my previous post, there is another side of the medal.  If you 
didn't get it, then read through it again and try, for example, the following 
amended @lodato's scenario:

  $ mkdir tmp
  $ cd tmp
  $ git init
  $ cat > foo.cc < foo.cc  1:
 if not opts.diff:
   die('--diff is required when two commits are given')
+if opts.staged:
+  die('--staged is not allowed when two commits are given')
   else:
 if len(commits) > 2:
   die('at most two commits allowed; %d given' % len(commits))
-  changed_lines = compute_diff_and_extract_lines(commits, files)
+  changed_lines = compute_diff_and_extract_lines(commits, files, opts.staged)
   if opts.verbose >= 1:
 ignored_files = set(changed_lines)
   filter_by_extension(changed_lines, opts.extensions.lower().split(','))
@@ -159,10 +163,14 @@
  binary=opts.binary,
  style=opts.style)
   else:
-old_tree = create_tree_from_workdir(changed_lines)
+if opts.staged:
+  old_tree = run_git_ls_files_and_save_to_tree(changed_lines)
+else:
+  old_tree = create_tree_from_workdir(changed_lines)
 new_tree = run_clang_format_and_save_to_tree(changed_lines,
  binary=opts.binary,
- style=opts.style)
+ style=opts.style,
+ staged=opts.staged)
   if opts.verbose >= 1:
 print('old tree: %s' % old_tree)
 print('new tree: %s' % new_tree)
@@ -261,9 +269,10 @@
   return convert_string(stdout.strip())
 
 
-def compute_diff_and_extract_lines(commits, files):
+def compute_diff_and_extract_lines(commits, files, staged=False):
   """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commits, files)
+  assert not staged or len(commits) < 2
+  diff_process = compute_diff(commits, files, staged)
   changed_lines = extract_lines(diff_process.stdout)
   diff_process.stdout.close()
   diff_process.wait()
@@ -273,17 +282,22 @@
   return changed_lines
 
 
-def compute_diff(commits, files):
+def compute_diff(commits, files, staged=False):
   """Return a subprocess object producing the diff from `commits`.
 
   The return value's `stdin` file object will produce a patch with the
   differences between the working directory and the first commit if a single
   one was specified, or the difference between both specified commits, filtered
   on `files` (if non-empty).  Zero context lines are used in the patch."""
-  git_tool = 'diff-index'
+  assert not staged or len(commits) < 2
+  cmd = ['git']
   if len(commits) > 1:
-git_tool = 'diff-tree'
-  cmd = ['git', git_tool, '-p', '-U0'] + commits + ['--']
+cmd.append('diff-tree')
+  else:
+cmd.append('diff-index')
+  if staged:
+cmd.append('--cached')
+  cmd.extend(['-p', '-U0'] + commits + ['--'])
   cmd.extend(files)
   p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
   p.stdin.close()
@@ -343,11 +357,43 @@
   return create_tree(filenames, '--stdin')
 
 
+def run_git_ls_files_and_save_to_tree(changed_lines):
+  """Run git ls-files and save the result to a git tree.
+
+  Returns the object ID (SHA-1) of the created tree."""
+  index_path = os.environ.get('GIT_INDEX_FILE')
+  def iteritems(container):
+  try:
+  return container.iteritems() # Python 2
+  except AttributeError:
+  return container.items() # Python 3
+  def index_info_generator():
+for filename, lin

[PATCH] D15465: [git-clang-format]: New option to perform formatting against staged changes only

[Alexander Shukaev via Phabricator via cfe-commits]

Alexander-Shukaev added a comment.

Did anybody have a chance to review it and/or try it out?


Repository:
  rL LLVM

https://reviews.llvm.org/D15465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D15465: [git-clang-format]: New option to perform formatting against staged changes only

[Alexander Shukaev via Phabricator via cfe-commits]

Alexander-Shukaev added a comment.

Hi @lodato, thanks mate.


Repository:
  rL LLVM

https://reviews.llvm.org/D15465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D15465: [git-clang-format]: New option to perform formatting against staged changes only

[Alexander Shukaev via Phabricator via cfe-commits]

Alexander-Shukaev added a comment.

Mark, just wanted to check if the review is still somewhere on your radar.


Repository:
  rL LLVM

https://reviews.llvm.org/D15465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D15465: [git-clang-format]: New option to perform formatting against staged changes only

[Alexander Shukaev via Phabricator via cfe-commits]

Alexander-Shukaev added a comment.

Man, I have to admit it's really a shame that I didn't find time to work on 
this further but I'm truly too busy these days.  However, I believe the primary 
point why I didn't have motivation to do this is because the flaw that was 
pointed out actually never bothered me in real life simply because I've never 
ever hit this case in production.  I confess that I use this solution, namely 
the one linked on Stack Overflow (to my personal Bitbucket repository) every 
single day and we even introduced it in my company.  Nobody ever complained so 
far.  It's true that sometimes one would want to stage only some changes and 
commit them, while having other changes unstaged, but I don't remember when I 
was in that situation last time.  If you really want to leave something out for 
another commit, then you can also stash those unstaged changes before you 
format/commit the staged ones.  Furthermore, let me clarify why the proposal by 
@lodato might not even fit into the picture (i.e. there is no universal 
solution this problem as I see it until now).  In particular, his example does 
not illustrate another side of the medal, namely let's say that the staged code 
was, in fact, badly formatted (not like in his example), and then you apply 
some code on top of it that is not yet staged (same like in his example).  By 
"on top" I mean really like he shows it, that those changes overlap, that is if 
you'd stage them, they'd overwrite the previously staged ones (which in our 
imaginary example are badly formatted now).  Now let's think what will happen 
if we follow his proposal.  We'd apply formatting purely to the "staged" 
version of the file by piping it from index as a blob directly to formatter, so 
far so good.  Or wait a minute, how would you actually format that file in 
place then?  That is you already have unstaged and potentially conflicting 
changes with the ones you'd get as a result of formatting the staged ones but 
how to reconcile these two versions now?  That is how do you put those 
formatted changes into unstaged state when you already have something 
completely different but also unstaged at the same line?  Turns out that the 
answer is, you can't, without introducing explicit conflicts in the unstaged 
tree, which is even more confusing to my taste.  Or would you just report the 
diff with an error message to the user leaving the act of applying those to 
them manually?  You could, but then you give up on that cool feature of 
automatic formatting.  To conclude, which approach you take, is all about pros 
and cons.  On the daily basis and from productivity standpoint, I care more 
about doing my changes for the target commit, trying to commit, if something is 
wrong getting it automatically formatted in the unstaged tree, reviewing this 
unstaged diff quickly, staging it, and finally committing my work.  That corner 
case with some irrelevant changes hanging in the unstaged tree and fooling 
formatter can rarely be a problem.  And even then, I treat it more like an 
answer from a formatter: "Hey bro, you have some unstaged crap out there, can 
you please already decide whether you need it now or not, otherwise I can't do 
my job for you?!", in which case I will

- either actually stage them if I really need them,
- or stash them if I want to deal with them later,
- or discard them altogether if they are garbage,

all of which will allow formatter to do it's job and me to finally produce the 
commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D15465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits