Re: feature-request: git "cp" like there is git mv.

[Junio C Hamano]

Stefan Moch  writes:

> Are such redundant checks in general a pattern worth searching
> for and cleaning up globally? Or is this rather in the category
> of cleaning up only when noticed?

A clean-up patch that is otherwise a no-op is still welcome as it
will improve the health of the codebase, but they become hindrances
if there are too many of them to consume the review bandwidth that
would otherwise be better spent on other non no-op topics, and/or if
they add too many merge conflicts with other non no-op topics in
flight.

The amount of such negative impact a no-op clean-up patch can have
on the project does not depend on how the issue was discovered, so
we do not even have to know if the issue was discovered by actively
hunting or by noticing while working on a near-by area.

It is possible that by actively looking for, you may end up
producing more of the no-op clean-up patches and can more easily
interfere with other topics, which we may need to discourge or at
least ask you to slow down.  On the other hand, issues discovered
while working on a near-by area would typically not increase
conflicts with other topics in flight over the conflicts that would
be caused by that real work you were doing in a near-by area
already, so in that sense, "only when noticed" is a practical way to
avoid the clean-up fatigue.

Re: feature-request: git "cp" like there is git mv.

[Stefan Moch]

* Junio C Hamano  [2018-02-07T11:49:39-0800]:
> Stefan Moch  writes:
> 
> > * Jonathan Nieder  [2017-12-15T17:31:30-0800]:  
> >> This sounds like a reasonable thing to add.  See builtin/mv.c for
> >> how "git mv" works if you're looking for inspiration.
> >> 
> >> cmd_mv in that file looks rather long, so I'd also be happy if
> >> someone interested refactors to break it into multiple
> >> self-contained pieces for easier reading (git mostly follows
> >> https://www.kernel.org/doc/html/latest/process/coding-style.html#functions).
> >>   
> >
> > I looked at builtin/mv.c and have a rough idea how to split it
> > up to support both mv and cp commands.
> >
> > But first I noticed and removed a redundant check in cmd_mv,
> > also added a test case to check if mv --dry-run does not move
> > the file.  
> 
> I guess these two patches went unnoticed when posted at the end of
> last year.  Reading them again, I think they are good changes.

Thanks.

Are such redundant checks in general a pattern worth searching
for and cleaning up globally? Or is this rather in the category
of cleaning up only when noticed?


> As a no-op clean-up of a127331c ("mv: allow moving nested
> submodules", 2016-04-19), the attached would also make sense, I
> would think.
> 
> Thanks.
> 
>  builtin/mv.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 9662804d23..9cb07990fd 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -266,10 +266,11 @@ int cmd_mv(int argc, const char **argv, const
> char *prefix) const char *src = source[i], *dst = destination[i];
>   enum update_mode mode = modes[i];
>   int pos;
> - if (show_only || verbose)
> - printf(_("Renaming %s to %s\n"), src, dst);
> - if (show_only)
> + if (show_only) {
> + if (verbose)
> + printf(_("Renaming %s to %s\n"),
> src, dst); continue;
> + }
>   if (mode != INDEX && rename(src, dst) < 0) {
>   if (ignore_errors)
>   continue;
> 

As Stefan Beller already noted, this changes the printing
behavior:


See also the output of

git mv -n
git mv -n -v
git mv -v


without your patch:

$ git mv -n 1 2
Checking rename of '1' to '2'
Renaming 1 to 2
$ git mv -n -v 1 2
Checking rename of '1' to '2'
Renaming 1 to 2
$ git mv -v 1 2
Renaming 1 to 2


and with your patch:

$ git mv -n 1 2
Checking rename of '1' to '2'
$ git mv -n -v 1 2
Checking rename of '1' to '2'
Renaming 1 to 2
$ git mv -v 1 2


Having different outputs of “git mv -n” and “git mv -n -v” seems
odd, but not necessarily wrong. However, “git mv -v” with no
output at all, does not what the documentation says:

   -v, --verbose
   Report the names of files as they are moved.

Re: feature-request: git "cp" like there is git mv.

[Stefan Beller]

On Wed, Feb 7, 2018 at 11:49 AM, Junio C Hamano  wrote:
> Stefan Moch  writes:
>
>> * Jonathan Nieder  [2017-12-15T17:31:30-0800]:
>>> This sounds like a reasonable thing to add.  See builtin/mv.c for how
>>> "git mv" works if you're looking for inspiration.
>>>
>>> cmd_mv in that file looks rather long, so I'd also be happy if someone
>>> interested refactors to break it into multiple self-contained pieces
>>> for easier reading (git mostly follows
>>> https://www.kernel.org/doc/html/latest/process/coding-style.html#functions).
>>
>> I looked at builtin/mv.c and have a rough idea how to split it
>> up to support both mv and cp commands.
>>
>> But first I noticed and removed a redundant check in cmd_mv,
>> also added a test case to check if mv --dry-run does not move
>> the file.
>
> I guess these two patches went unnoticed when posted at the end of
> last year.  Reading them again, I think they are good changes.
>
> As a no-op clean-up of a127331c ("mv: allow moving nested
> submodules", 2016-04-19), the attached would also make sense, I
> would think.
>
> Thanks.
>
>  builtin/mv.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 9662804d23..9cb07990fd 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -266,10 +266,11 @@ int cmd_mv(int argc, const char **argv, const char 
> *prefix)
> const char *src = source[i], *dst = destination[i];
> enum update_mode mode = modes[i];
> int pos;
> -   if (show_only || verbose)
> -   printf(_("Renaming %s to %s\n"), src, dst);
> -   if (show_only)
> +   if (show_only) {
> +   if (verbose)
> +   printf(_("Renaming %s to %s\n"), src, dst);
> continue;
> +   }

This is actually changing behavior to

if (show_only && verbose)
print(...)

if show_only
continue

The second part is already there as is, only the printing behavior
actually changes.

So I might be missing the obvious here for the claim of no-op?

Looking up further we have (line 177):

if (show_only)
printf(_("Checking rename of '%s' to '%s'\n"), src, dst);

which prints regardless of verbosity.

Re: feature-request: git "cp" like there is git mv.

[Junio C Hamano]

Stefan Moch  writes:

> * Jonathan Nieder  [2017-12-15T17:31:30-0800]:
>> This sounds like a reasonable thing to add.  See builtin/mv.c for how
>> "git mv" works if you're looking for inspiration.
>> 
>> cmd_mv in that file looks rather long, so I'd also be happy if someone
>> interested refactors to break it into multiple self-contained pieces
>> for easier reading (git mostly follows
>> https://www.kernel.org/doc/html/latest/process/coding-style.html#functions).
>
> I looked at builtin/mv.c and have a rough idea how to split it
> up to support both mv and cp commands.
>
> But first I noticed and removed a redundant check in cmd_mv,
> also added a test case to check if mv --dry-run does not move
> the file.

I guess these two patches went unnoticed when posted at the end of
last year.  Reading them again, I think they are good changes.

As a no-op clean-up of a127331c ("mv: allow moving nested
submodules", 2016-04-19), the attached would also make sense, I
would think.

Thanks.

 builtin/mv.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 9662804d23..9cb07990fd 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -266,10 +266,11 @@ int cmd_mv(int argc, const char **argv, const char 
*prefix)
const char *src = source[i], *dst = destination[i];
enum update_mode mode = modes[i];
int pos;
-   if (show_only || verbose)
-   printf(_("Renaming %s to %s\n"), src, dst);
-   if (show_only)
+   if (show_only) {
+   if (verbose)
+   printf(_("Renaming %s to %s\n"), src, dst);
continue;
+   }
if (mode != INDEX && rename(src, dst) < 0) {
if (ignore_errors)
continue;

Re: feature-request: git "cp" like there is git mv.

[Stefan Moch]

* Jonathan Nieder  [2017-12-15T17:31:30-0800]:
> This sounds like a reasonable thing to add.  See builtin/mv.c for how
> "git mv" works if you're looking for inspiration.
> 
> cmd_mv in that file looks rather long, so I'd also be happy if someone
> interested refactors to break it into multiple self-contained pieces
> for easier reading (git mostly follows
> https://www.kernel.org/doc/html/latest/process/coding-style.html#functions).

I looked at builtin/mv.c and have a rough idea how to split it
up to support both mv and cp commands.

But first I noticed and removed a redundant check in cmd_mv,
also added a test case to check if mv --dry-run does not move
the file.


Stefan

Re: feature-request: git "cp" like there is git mv.

[Igor Djordjevic]

Hi Simon,

On 12/12/2017 11:53, Simon Doodkin wrote:
> 
> please develop a new feature, git "cp" like there is git mv 
> tomovefile1 tofile2 (to save space).
> 
> there is a solution in https://stackoverflow.com/a/44036771/466363
> however, it is not single easy command.

While having `git cp` alongside `git mv` would make sense, I`m afraid 
that is not what you are really after, nor it would help in your case.

Looking at referenced "Stack Overflow" post[1], it tries to address 
`git blame` not following "file copy", where it does "file rename".

Proposed steps seem to be "solution" from your perspective, and while 
that may be absolutely valid and acceptable for your specific case, I 
would argue it`s the wrong approach in general - because `git blame` 
already supports what you want (just not by default), and making 
three additional, unneeded and possibly confusing commits (one being 
a merge), just to "bend" `git blame` to fit your (out of line?) usage 
expectations doesn`t seem right.

I would say a better direction might be using `git blame` "-C[]" 
option[2], where desired effect is achieved without any artificial 
history fiddling.

Example being worth more than plain words, I`m providing a script[3] 
that demonstrates what I`m talking about :)

Regards, Buga

[1] https://stackoverflow.com/a/44036771/466363
[2] https://git-scm.com/docs/git-blame#git-blame--Cltnumgt
[3] Demo script showing how using (multiple) "-C" option(s), with 
specified numeric value, can make `git blame` provide desired 
information, recognizing "file copy" operation (line copy, actually, 
but that is what we are really interested in, using `git blame`):
--- >8 ---
git init

echo a >A
echo b >>A
echo c >>A

git add A
git commit -m "create file A"

git mv A B
git commit -m "rename file A -> B"

# ---
# (A) regular flow
cp B C
git add C
git commit -m "copy file B -> C"
# ---

# ---
# (B) proposed "solution", https://stackoverflow.com/a/44036771/466363
# git mv B C
# git commit -n -m "rename file B -> C"
# SAVED=`git rev-parse HEAD`
# git reset --hard HEAD^
# git mv B B-temp
# git commit -n -m "rename file B -> B-temp"
# git merge $SAVED # This will generate conflicts
# git commit -a -n --no-edit # Trivially resolved like this
# git mv B-temp B
# git commit -n -m "rename file B-temp -> B"
# ---

echo
echo '(1) blames B back to A, as expected:'
git blame -- B
# git blame shows that file B has a history (back to file A)...

echo
echo '(2) blames C only, missing B and A:'
git blame -- C
# ... while file C doesn't have a history

echo
echo '(3) blames C back to A, as expected:'
git blame -C -C3 -- C
# git blame shows that file C has a history (back to file A)

Re: feature-request: git "cp" like there is git mv.

[Jonathan Nieder]

Hi Simon,

Simon Doodkin wrote:

> please develop a new feature, git "cp" like there is git mv tomovefile1 
> tofile2
> (to save space).
>
> there is a solution in https://stackoverflow.com/a/44036771/466363
> however, it is not single easy command.

This sounds like a reasonable thing to add.  See builtin/mv.c for how
"git mv" works if you're looking for inspiration.

cmd_mv in that file looks rather long, so I'd also be happy if someone
interested refactors to break it into multiple self-contained pieces
for easier reading (git mostly follows
https://www.kernel.org/doc/html/latest/process/coding-style.html#functions).

Thanks,
Jonathan

RE: feature-request: git "cp" like there is git mv.

[Randall S. Becker]

-Original Message-
On December 13, 2017 11:40 AM Johannes Schindelin wrote:
>On Tue, 12 Dec 2017, Simon Doodkin wrote:
>> please develop a new feature, git "cp" like there is git mv 
>> tomovefile1 tofile2 (to save space).
>> there is a solution in https://stackoverflow.com/a/44036771/466363
>> however, it is not single easy command.
>This is not how this project works. The idea is that it is Open Source, so
that you can develop this feature yourself, and contribute a patch.

Agree with Johannes. Let's help though, to quantify the requirements so that
Simon can get this right. I'm putting my tyrannical repository manager hat
on here rather than developer so...

Are you looking to have git cp copy the entire history of tomovefile1 to
tofile2 or just copy the content of tomovefile1 to tofile2 and add and/or
commit the file?

In the latter, I see the convenience of this capability. Even so, a simple
cp would copy the content and then you can commit it fairly easily. In the
former, copying the entire history of a file inside the repository is going
to potentially cause tofile2 to appear in old commits where prior to the git
cp command the file was not present? In this situation, you are actually
rewriting history and potentially impacting signed commits (which would no
longer pass a signature check, I hope). Stitching repositories is sometimes
done when repairs or reorganization is required, but I'm concerned that this
is opening up a can of worms that breaks the atomicity of commits
(particularly signed ones). What I don't want, for my own teams, is for
members to think that git cp would be a harmless (unless it actually is)
command, rather than a repair/reorg mechanism used for splitting apart a
repository, or copying a file to a new project then splitting selectively.
So, I'm obviously a bit confused about the goal.

Simon: the stackoverflow post provides a few options on this command. Can
you clarify which particular direction you are interest it?

Cheers,
Randall

-- Brief whoami: NonStop developer since approximately
UNIX(421664400)/NonStop(2112884442) 
-- In my real life, I talk too much.

Re: feature-request: git "cp" like there is git mv.

[Johannes Schindelin]

Hi Simon,

On Tue, 12 Dec 2017, Simon Doodkin wrote:

> please develop a new feature, git "cp" like there is git mv tomovefile1
> tofile2 (to save space).
> 
> there is a solution in https://stackoverflow.com/a/44036771/466363
> however, it is not single easy command.

This is not how this project works. The idea is that it is Open Source, so
that you can develop this feature yourself, and contribute a patch.

Ciao,
Johannes