git-mergetool reverse file ordering

git-mergetool reverse file ordering

[Luis Gutierrez]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

Hi,

Attached is a potential patch for reversing the order on which
git-mergetool presents the files to merge.

Currently, when running git-mergetool, it performs a sort of the files
to merge by alphabetical ordering. When working on C, this has the
annoying effect of presenting the merge for a .c* files before the
header files; which is always a bit harder to do. Reading the header
first to figure out what the other dude changed is usually preferred.

The attach patch reverse the order (-r flag to sort) so *.h* are
merged before  *.c* files

PS, given the simplicity of the patch, I have not tested it.

Regards

Luis

[-- Attachment #2: sort-for-c-files.txt --]
[-- Type: text/plain, Size: 501 bytes --]

diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..cce3b0d 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -453,10 +453,10 @@ then
 	then
 		files=$(git rerere remaining)
 	else
-		files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
+		files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u -r)
 	fi
 else
-	files=$(git ls-files -u -- "$@" | sed -e 's/^[^	]*	//' | sort -u)
+	files=$(git ls-files -u -- "$@" | sed -e 's/^[^	]*	//' | sort -u -r)
 fi
 
 if test -z "$files"

Re: git-mergetool reverse file ordering

[David Aguilar]

On Wed, Jul 27, 2016 at 11:14:28AM +0100, Luis Gutierrez wrote:
> Hi,
> 
> Attached is a potential patch for reversing the order on which
> git-mergetool presents the files to merge.
> 
> Currently, when running git-mergetool, it performs a sort of the files
> to merge by alphabetical ordering. When working on C, this has the
> annoying effect of presenting the merge for a .c* files before the
> header files; which is always a bit harder to do. Reading the header
> first to figure out what the other dude changed is usually preferred.
> 
> The attach patch reverse the order (-r flag to sort) so *.h* are
> merged before  *.c* files
> 
> PS, given the simplicity of the patch, I have not tested it.
> 
> Regards
> 
> Luis

Thanks for the sug, this is an interesting idea and I definitely
see why we would want something like this...

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index bf86270..cce3b0d 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -453,10 +453,10 @@ then
>  	then
>  		files=$(git rerere remaining)
>  	else
> -		files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
> +		files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u -r)
>  	fi
>  else
> -	files=$(git ls-files -u -- "$@" | sed -e 's/^[^	]*	//' | sort -u)
> +	files=$(git ls-files -u -- "$@" | sed -e 's/^[^	]*	//' | sort -u -r)
>  fi
>  
>  if test -z "$files"

While we won't take this patch as-is (please see
Documentation/SubmittingPatches for details about the patch
submission process), I am interested in the use case you've
described.

This use case makes me wonder whether the sorting we do here is
something that should be opened up a bit so that the it's not
quite so set in stone.

For example, an extension to the approach taken by this patch
would be to have `mergetool.reverseOrder` git config boolean
option that would tell us whether or not to use the "-r" flag
when calling sort.

But, IMO that is too rigid, and only addresses this narrow use
case.  What if users want a case-insensitive sort, or some other
preferred ordering?

We can address these concerns, and your use case, by opening it
up. Something like,

	sort=$(git config mergetool.sort || echo sort -u)

That preserves the existing behavior, and it opens it up so that
we can accomplish the same result as this patch by doing:

	git config mergetool.sort "sort -u -r"

Then, if someone later writes a nicer C/C++-specific sort that
sorts in the natural order but also keeps .h files before .c,
.cpp, etc. files with the same basename, then they could do:

	git config mergetool.sort crescent-fresh-sort

...and it'll be totally crescent.

Thoughts?  Would you be interested in helping work up a patch
for this idea?  At a minimum we should also write a test case in
t/t7610-mergetool.sh to verify that it works as advertised.

Let me know if you have any questions.

cheers,
-- 
David

Re: git-mergetool reverse file ordering

[John Keeping]

On Sat, Aug 13, 2016 at 08:42:21PM -0700, David Aguilar wrote:
> This use case makes me wonder whether the sorting we do here is
> something that should be opened up a bit so that the it's not
> quite so set in stone.
> 
> For example, an extension to the approach taken by this patch
> would be to have `mergetool.reverseOrder` git config boolean
> option that would tell us whether or not to use the "-r" flag
> when calling sort.
> 
> But, IMO that is too rigid, and only addresses this narrow use
> case.  What if users want a case-insensitive sort, or some other
> preferred ordering?
> 
> We can address these concerns, and your use case, by opening it
> up. Something like,
> 
> 	sort=$(git config mergetool.sort || echo sort -u)

Why not reuse the existing diff.orderFile config variable?  (Also
supported by the -O option to git-diff).

Re: git-mergetool reverse file ordering

[Luis Gutierrez]

> Thoughts?  Would you be interested in helping work up a patch
> for this idea?  At a minimum we should also write a test case in
> t/t7610-mergetool.sh to verify that it works as advertised.
....
> Why not reuse the existing diff.orderFile config variable?  (Also
> supported by the -O option to git-diff).


I'll be happy to write a testcase, and to re-use the -O
(diff.orderFile config var) option to git-diff as sugguested by John
Keeping.

Is this the final spec?



I'll be happy to do that.

Luis

Re: git-mergetool reverse file ordering

[David Aguilar]

On Mon, Aug 15, 2016 at 09:19:35PM +0100, Luis Gutierrez wrote:
> > Thoughts?  Would you be interested in helping work up a patch
> > for this idea?  At a minimum we should also write a test case in
> > t/t7610-mergetool.sh to verify that it works as advertised.
> ....
> > Why not reuse the existing diff.orderFile config variable?  (Also
> > supported by the -O option to git-diff).
> 
> 
> I'll be happy to write a testcase, and to re-use the -O
> (diff.orderFile config var) option to git-diff as sugguested by John
> Keeping.
> 
> Is this the final spec?
> 
> 
> 
> I'll be happy to do that.
> 
> Luis

Hmm, I do like the idea of reusing the diff orderFile, but a
mechanism for sorting arbitrary inputs based on the orderFile
isn't currently exposed in a way that mergetool could use it.


Looking at the code in mergetool, we basically need something
that has the same spec as "sort" itself, namely that it can take
arbitrary arbitrary input on stdin and sort it.


Implementing the orderFile support would probably be best done
in C.  Would we want to expose it as an internal helper?

e.g.
	git diff--order-helper <order-file>

could be used to perform the sorting.

But, that sort is honestly kinda crude.  It can't implement the
interesting case where we want bar.h to sort before bar.c but
not foo.h.

If we did the sort option, we could have both.

Thoughts?

-- 
David

Re: git-mergetool reverse file ordering

[Johannes Sixt]

Am 17.08.2016 um 03:25 schrieb David Aguilar:
> Hmm, I do like the idea of reusing the diff orderFile, but a
> mechanism for sorting arbitrary inputs based on the orderFile
> isn't currently exposed in a way that mergetool could use it.

Instead of using 'git ls-files -u | sed ... | sort -u' you could use

   git diff-files --name-status -O... | sed -ne '/^U/s/^..//p'

> But, that sort is honestly kinda crude.  It can't implement the
> interesting case where we want bar.h to sort before bar.c but
> not foo.h.
>
> If we did the sort option, we could have both.

I don't think that we need a new filter when the diff machinery is 
capable of re-ordering files. We should enhance the diff machinery instead.

-- Hannes

Re: git-mergetool reverse file ordering

[Johannes Sixt]

Am 17.08.2016 um 08:05 schrieb Johannes Sixt:
> Am 17.08.2016 um 03:25 schrieb David Aguilar:
>> Hmm, I do like the idea of reusing the diff orderFile, but a
>> mechanism for sorting arbitrary inputs based on the orderFile
>> isn't currently exposed in a way that mergetool could use it.
>
> Instead of using 'git ls-files -u | sed ... | sort -u' you could use
>
>   git diff-files --name-status -O... | sed -ne '/^U/s/^..//p'

Or even better:

     git diff-files --name-only --diff-filter=U -O...

>
>> But, that sort is honestly kinda crude.  It can't implement the
>> interesting case where we want bar.h to sort before bar.c but
>> not foo.h.
>>
>> If we did the sort option, we could have both.
>
> I don't think that we need a new filter when the diff machinery is
> capable of re-ordering files. We should enhance the diff machinery instead.
>
> -- Hannes
>

Re: git-mergetool reverse file ordering

[David Aguilar]

On Wed, Aug 17, 2016 at 08:10:46AM +0200, Johannes Sixt wrote:
> Am 17.08.2016 um 08:05 schrieb Johannes Sixt:
> > Am 17.08.2016 um 03:25 schrieb David Aguilar:
> > > Hmm, I do like the idea of reusing the diff orderFile, but a
> > > mechanism for sorting arbitrary inputs based on the orderFile
> > > isn't currently exposed in a way that mergetool could use it.
> > 
> > Instead of using 'git ls-files -u | sed ... | sort -u' you could use
> > 
> >   git diff-files --name-status -O... | sed -ne '/^U/s/^..//p'
> 
> Or even better:
> 
>     git diff-files --name-only --diff-filter=U -O...

Nice!


> > > But, that sort is honestly kinda crude.  It can't implement the
> > > interesting case where we want bar.h to sort before bar.c but
> > > not foo.h.
> > > 

Note: I had a subtle typo above -- I meant to describe this desired
order:
	bar.h
	bar.c
	foo.h
	foo.c

which is not possible with an orderFile that has only:

	*.h
	*.c

But...

> > > If we did the sort option, we could have both.
> > 
> > I don't think that we need a new filter when the diff machinery is
> > capable of re-ordering files. We should enhance the diff machinery instead.
> > 

Reading the code more thoroughly, I fully agree.

Switching to a diff-files invocation lets us reuse the
diff.orderFile machinery and does not require exposing any
additional helpers.

While it would be *nice* if we had a way to sort foo.h before
foo.c and after bar.c, that's just a pie-in-the-sky idea.
Consistency is king.


The only thing that using diff-files doesn't address is the
rerere support in mergetool where it processes the files in
the order specified by "git rerere remaining".  This is why I
initially thought we needed a generic sort-like command.

That code path does not currently do any sorting (which may
explain why we didn't notice it if we were just looking for
"sort" invocations) but maybe that's an edge case that we don't
need to address initially (if at all).

Would it be okay for the rerere code path to not honor the
orderFile?  IMO if we documented the behavior then it would be
acceptable, and perhaps can be improved as a follow-up.

For the basic support the implementation becomes really
simple: switch to using diff-files instead of ls-files.

If we did want consistency in the "git rerere remaining" code
path, would it be acceptable to teach "rerere" about
"-O<orderfile>" so that we could drive it from mergetool?

I only suggest an option, and not config support, because I
would be hesitant to make "rerere remaining" unconditionally
honor the orderFile since that feature is diff-centric, while
"rerere remaining" is a different beast altogether.  Adding a
flag is a middle-ground that would allow mergetool to drive it
while not suddently changing rerere's behavior.


The patches could then be:

1. switch to diff-files, add tests, and document how
   diff.orderFile affects mergetool.

2. Teach mergetool about the "-O<orderFile>" flag so that it can
   override the configuration, and add tests.  It could be
   argued that this should be squashed into (1).

3. (optional) teach "rerere remaining" to honor the
   -O<orderfile> flag and teach mergetool to supply the option.

Sound good?


ciao,
-- 
David

Re: git-mergetool reverse file ordering

[Johannes Sixt]

Am 17.08.2016 um 08:46 schrieb David Aguilar:
> The only thing that using diff-files doesn't address is the
> rerere support in mergetool where it processes the files in
> the order specified by "git rerere remaining".  This is why I
> initially thought we needed a generic sort-like command.

I see. This is actually an important code path. How about this code 
structure:

if test $# -eq 0
then
	cd_to_toplevel

	if test -e "$GIT_DIR/MERGE_RR"
	then
		set -- $(git rerere remaining)
	fi
fi
files=$(git diff-files --name-only --diff-filter=U -- "$@")

This does not require an enhancement of rerere-remaining and still 
captures all three cases that currently go through separate branches. 
(Throw in some version of --ignore-submodules= if necessary, but I guess 
it is not.)

We do have a problem if there are file names with spaces, but it is not 
a new problem.

> The patches could then be:
>
> 1. switch to diff-files, add tests, and document how
>    diff.orderFile affects mergetool.
>
> 2. Teach mergetool about the "-O<orderFile>" flag so that it can
>    override the configuration, and add tests.  It could be
>    argued that this should be squashed into (1).
>
> 3. (optional) teach "rerere remaining" to honor the
>    -O<orderfile> flag and teach mergetool to supply the option.
>
> Sound good?

Sure, except that 3. won't be necessary.

-- Hannes

Re: git-mergetool reverse file ordering

[David Aguilar]

Hi Luis and Hannes,

On Wed, Aug 17, 2016 at 09:35:56AM +0200, Johannes Sixt wrote:
> Am 17.08.2016 um 08:46 schrieb David Aguilar:
> > The only thing that using diff-files doesn't address is the
> > rerere support in mergetool where it processes the files in
> > the order specified by "git rerere remaining".  This is why I
> > initially thought we needed a generic sort-like command.
> 
> I see. This is actually an important code path. How about this code
> structure:
> 
> if test $# -eq 0
> then
> 	cd_to_toplevel
> 
> 	if test -e "$GIT_DIR/MERGE_RR"
> 	then
> 		set -- $(git rerere remaining)
> 	fi
> fi
> files=$(git diff-files --name-only --diff-filter=U -- "$@")
> 

Beautiful.

> This does not require an enhancement of rerere-remaining and still captures
> all three cases that currently go through separate branches. (Throw in some
> version of --ignore-submodules= if necessary, but I guess it is not.)
> 
> We do have a problem if there are file names with spaces, but it is not a
> new problem.

Thanks for the heads-up about file names with spaces.  We set,

IFS='
'

in git-mergetool--lib.sh so file names with spaces should be ok.
Naturally, we won't be able to support paths with embedded
newlines, but that's not a new problem ;-)

We should probably also set core.quotePath=false when calling
diff-files so that git doesn't try to quote "unusual" paths,
e.g.

	git -c core.quotePath=false diff-files ...

Lastly, for anyone that's curious, I was wondering why we were
passing "-u" to "sort", and why we won't need to use "uniq" in
the new version.

The reason is that "ls-files -u" lists the different index
stages separately, and thus it reports duplicate paths.

"diff-files" with "--diff-filter=U" does not do that, so that's
another benefit to be gained from this change.

I think we've touched all the bases now.

Luis, I hope that makes sense.  Let us know if any of this is
unclear.


ciao,
-- 
David