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

[Mark Lodato via Phabricator via cfe-commits]

lodato added a comment.

Oh, and I meant to start with: I'm so sorry for the extremely long delay. I was 
swamped with work before then I forgot about this. Please know that I 
appreciate your effort here and that I didn't mean to blow you off.

Best regards, Mark


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

[Mark Lodato via Phabricator via cfe-commits]

lodato added a comment.

I think the simplest solution to those problems is to require `--diff`. An 
alternative is to write the changes directly to the index without touching the 
working directory, but that would require some flag because the behavior is 
unintuitive, and the implementation would be complicated enough to warrant its 
own patch.

I reimplemented your patch in https://reviews.llvm.org/D41147 based on a 
significant refactoring, which I hope makes the code more clear.


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.

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

[Mark Lodato via Phabricator via cfe-commits]

lodato added a comment.

Sorry, I have been very busy with other work so I haven't had a chance to 
follow along. (I don't work on LLVM team - I just contributed this script.)

I'll try to carve out some time to review within the next week.


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

[Jason Newton via Phabricator via cfe-commits]

nevion added a comment.

I'm paying attention at least.  I updated your patch prior to your posting and 
temporarily made due with it.  I'm pretty nervous that I will lose work with my 
commit style and the lingering issue.  Based on what I've seen so far I can't 
use the git hooks and so I want to do this more manually which entices more 
accidents as that's when staged data is more frequent.


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.

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 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, 

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

[Jason Newton via Phabricator via cfe-commits]

nevion added a comment.

Can anyone inform me why these don't work on clang-format-diff?


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

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

[Jason Newton via Phabricator via cfe-commits]

nevion added a comment.

Hi - I'm coming here from Alexander's post on stackoverflow 

  and I'm interested to see both how this solution is progressing (no replies 
for 1.5 years is not a good sign), verify it still works, and to check if there 
are better options available hedging if this is stuck in limbo and out of 
usability date.


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

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

[Mark Lodato via cfe-commits]

lodato added inline comments.


Comment at: git-clang-format:124
@@ -121,3 +123,3 @@
   del opts.quiet
 
   commit, files = interpret_args(opts.args, dash_dash, opts.commit)

lodato wrote:
> This will work without `--diff` (otherwise it will try to apply changes in 
> the index to the working directory, which doesn't make sense), so could you 
> please add a check that `--staged` requires `--diff`?
Oops, will //not// work.


Repository:
  rL LLVM

http://reviews.llvm.org/D15465



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

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

[Mark Lodato via cfe-commits]

lodato added a subscriber: lodato.
lodato requested changes to this revision.
lodato added a reviewer: lodato.
lodato added a comment.
This revision now requires changes to proceed.

This does not work properly because it calls `clang-format` on the files in the 
working directory, even if `--staged` is given.  To fix, you need to somehow 
pass in the version of the files in the index into `clang-format`.  To do that, 
I think you'd want to pass in the blob via stdin and add `-assume-filename=...` 
to the command line.

Example:

  $ mkdir tmp
  $ cd tmp
  $ git init
  $ cat > foo.cc < foo.cc