Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Ben Boeckel]

On Tue, Dec 05, 2017 at 14:13:37 -0800, Brandon Williams wrote:
> This patch should fix the regression.  Let me know if it doesn't solve the
> issue and I'll investigate some more.

Our test suite passes again. Thanks!

Acked-by: Ben Boeckel <ben.boec...@kitware.com>

--Ben

Re: gitattributes not read for diff-tree anymore in 2.15?

[Ben Boeckel]

On Tue, Dec 05, 2017 at 10:16:45 -0800, Brandon Williams wrote:
> Perfect, thanks!

OK, attached is a shell script which recreates the issue. I haven't been
able to get it to happen without the `GIT_WORK_TREE` and `GIT_INDEX_FILE`
setup involved, so that seems to be important.

I reran the bisect using this script and came up with this commit:

commit be4ca290570f9173db64ea1f925b5b3831c6efed
Author: David Turner 
Date:   Thu Apr 20 16:41:18 2017 -0400

Increase core.packedGitLimit



which seems even less relevant…

Thanks,

--Ben


diff-tree-break.sh
Description: Bourne shell script

Re: gitattributes not read for diff-tree anymore in 2.15?

[Ben Boeckel]

On Mon, Dec 04, 2017 at 15:03:55 -0800, Brandon Williams wrote:
> Reading the attributes files should be done regardless if the gitmodules
> file is read.  The gitmodules file should only come into play if you are
> dealing with submodules.

Yeah, it doesn't seem to make sense why this commit breaks us, but there
it is *shrug*.

> Do you mind providing a reproduction recipe with expected outcome vs
> actual outcome and I can take a closer look.

I'll work on one. It isn't as simple as I thought it was :) . The setup
we do before running the checks is apparently involved as running it
from the command line is not exhibiting the difference.

--Ben

gitattributes not read for diff-tree anymore in 2.15?

[Ben Boeckel]

Hi,

I've bisected a failure in our test suite to this commit:

commit 557a5998df19faf8641acfc5b6b1c3c2ba64dca9 (HEAD, refs/bisect/bad)
Author: Brandon Williams 
Date:   Thu Aug 3 11:20:00 2017 -0700

submodule: remove gitmodules_config

Now that the submodule-config subsystem can lazily read the gitmodules
file we no longer need to explicitly pre-read the gitmodules by calling
'gitmodules_config()' so let's remove it.

Signed-off-by: Brandon Williams 
Signed-off-by: Junio C Hamano 

Which is tags/v2.15.0-rc0~120^2.

Our test suite is in a Rust project here:

https://gitlab.kitware.com/utils/rust-git-checks

and the failing test(s) can be run using:

cargo test whitespace_all_ignored

The test checks that when `.gitattributes` says that whitespace errors
should be ignored, they aren't reported as errors. My guess is that not
reading the gitmodules configuration also skips reading attributes
files. Is this reasoning correct?

Thanks,

--Ben

[PATCH v3] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

When setting the `eol` attribute, paths can change their dirty status
without any change in the working directory. This can cause confusion
and should at least be mentioned with a remedy.

Reviewed-by: Torsten Bögershausen <tbo...@web.de>
Reviewed-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Ben Boeckel <maths...@gmail.com>
---
 Documentation/gitattributes.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c4f2be2..4c68bc1 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -151,7 +151,10 @@ unspecified.
 
 This attribute sets a specific line-ending style to be used in the
 working directory.  It enables end-of-line conversion without any
-content checks, effectively setting the `text` attribute.
+content checks, effectively setting the `text` attribute.  Note that
+setting this attribute on paths which are in the index with CRLF line
+endings may make the paths to be considered dirty.  Adding the path to
+the index again will normalize the line endings in the index.
 
 Set to string value "crlf"::
 
-- 
2.9.5

Re: [PATCH] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

On Wed, Aug 30, 2017 at 14:31:30 -0700, Junio C Hamano wrote:
> Is this "always makes them dirty" or "may make them dirty depending
> on the situation"?  
> 
> If the latter, I'd prefer to see s/makes/may make/ here.

Yes, a coworker reported that some files slipped through this. New patch
incoming.

--Ben

[PATCH v2] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

When setting the `eol` attribute, paths can change their dirty status
without any change in the working directory. This can cause confusion
and should at least be mentioned with a remedy.

Reviewed-by: Torsten Bögershausen <tbo...@web.de>
Signed-off-by: Ben Boeckel <maths...@gmail.com>
---
 Documentation/gitattributes.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c4f2be2..b940d37 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -151,7 +151,10 @@ unspecified.
 
 This attribute sets a specific line-ending style to be used in the
 working directory.  It enables end-of-line conversion without any
-content checks, effectively setting the `text` attribute.
+content checks, effectively setting the `text` attribute.  Note that
+setting this attribute on paths which are in the index with CRLF line
+endings makes the paths to be considered dirty.  Adding the path to the
+index again will normalize the line endings in the index.
 
 Set to string value "crlf"::
 
-- 
2.9.5

Re: [PATCH] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

On Thu, Aug 24, 2017 at 07:50:54 +0200, Torsten Bögershausen wrote:
>   This attribute sets a specific line-ending style to be used in the
>   working directory.  It enables end-of-line conversion without any
>  -content checks, effectively setting the `text` attribute.
>  +content checks, effectively setting the `text` attribute.  Note that
>  +setting this attribute on paths which are in the index with CRLF
>  +line endings makes the paths to be considered dirty.
>  +Adding the path to the index again will normalize the line
>  +endings in the index.

Yes, that sounds better. Will resubmit the patch.

--Ben

Re: [PATCH] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

On Wed, Aug 23, 2017 at 17:17:41 -0400, Ben Boeckel wrote:
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index c4f2be2..3044b71 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -151,7 +151,11 @@ unspecified.
>  
>  This attribute sets a specific line-ending style to be used in the
>  working directory.  It enables end-of-line conversion without any
> -content checks, effectively setting the `text` attribute.
> +content checks, effectively setting the `text` attribute.  Note that
> +setting this attribute on paths which are in the index with different
> +line endings than the attribute indicates the paths to be considered

Oops, missed a "causes" in here. ---^

I'll wait on uploading a new patch until after feedback on this one.

> +dirty.  Adding the path to the index again will normalize the line
> +endings in the index.

--Ben

[PATCH] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

When setting the `eol` attribute, paths can change their dirty status
without any change in the working directory. This can cause confusion
and should at least be mentioned with a remedy.

Signed-off-by: Ben Boeckel <maths...@gmail.com>
---
 Documentation/gitattributes.txt | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c4f2be2..3044b71 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -151,7 +151,11 @@ unspecified.
 
 This attribute sets a specific line-ending style to be used in the
 working directory.  It enables end-of-line conversion without any
-content checks, effectively setting the `text` attribute.
+content checks, effectively setting the `text` attribute.  Note that
+setting this attribute on paths which are in the index with different
+line endings than the attribute indicates the paths to be considered
+dirty.  Adding the path to the index again will normalize the line
+endings in the index.
 
 Set to string value "crlf"::
 
-- 
2.9.5

Re: Cannot checkout after setting the eol attribute

[Ben Boeckel]

On Wed, Aug 23, 2017 at 21:43:15 +0200, Torsten Bögershausen wrote:
> git reset does it's job - please see below.
> 
> The problem is that we need a "git commit" here.
> After applying .gitattributes, it may be neccessary to "normalize" the
> files. If there is something in the documentation, that can be
> improved, please let us know.

I'll have a patch up shortly.

> On Tue, Aug 22, 2017 at 03:44:41PM -0400, Ben Boeckel wrote:
> > The fact that plumbing is necessary to dig yourself out of a hole of the
> > `eol` attribute changes points to something needing to be changed, even
> > if it's only documentation. Could Git detect this and message about it
> > somehow when `git reset` cannot fix the working tree?
> 
> The thing is, that the working tree is "in a good state":
> We want "dos" with CRLF, and that is what we have.
> There is nothing that can be improved in the working tree.
> What needs to be fixed, is the index. And that needs to be done with
> "git add" "git commit."
> As Junio pointed out, the read-tree is not ideal
> (to fix a single file in a possible dirty working tree)
> 
> In your case it looks like this:
> 
> echo "dos eol=crlf" > .gitattributes
> git add .gitattributes &&
> git rm --cached dos && git add dos &&
> git commit

In this case, just adding the file should work: the file is on-disk as
intended and adding the file should normalize the line endings when
adding it into the index (basically, just `git add dos` should be
required to make the index look like it should).

> > Or maybe it could at least exit with failure instead of success?
> 
> I don't know.
> It -may- be possible to add a warning in "git reset".
> I can have a look at that...

Thanks.

--Ben

Re: Cannot checkout after setting the eol attribute

[Ben Boeckel]

On Tue, Aug 22, 2017 at 21:13:18 +0200, Torsten Bögershausen wrote:
> When you set the text attribute (in your case "eol=crlf" implies text)
> then the file(s) -must- be nomalized and commited so that they have LF
> in the repo (technically speaking the index)

This seems like a special case that Git could detect and message about
somehow.

> This is what is written about the "eol=crlf" attribute:
>   This setting forces Git to normalize line endings for this
>   file on checkin and convert them to CRLF when the file is
>   checked out.
> And this is what is implemented in Git.

Yeah, I read the docs, but the oddities of reset not doing its job
wasn't clear from this sentence :) .

> Long story short:
> 
> The following would solve your problem:
>git init
>echo $'dos\r' > dos
>git add dos
>git commit -m "dos newlines"
>echo "dos -crlf" > .gitattributes
>git add .gitattributes
>git commit -m "add attributes"
>echo "dos eol=crlf" > .gitattributes
>git read-tree --empty   # Clean index, force re-scan of working directory

The fact that plumbing is necessary to dig yourself out of a hole of the
`eol` attribute changes points to something needing to be changed, even
if it's only documentation. Could Git detect this and message about it
somehow when `git reset` cannot fix the working tree? Or maybe it could
at least exit with failure instead of success?

--Ben

Cannot checkout after setting the eol attribute

[Ben Boeckel]

Hi,

I specified the `eol` attribute on some files recently and the behavior
of Git is very strange.

Here is the set of commands to set up the repository used for the
discussion:

git init
echo $'dos\r' > dos
git add dos
git commit -m "dos newlines"
echo "dos -crlf" > .gitattributes
git add .gitattributes
git commit -m "add attributes"
echo "dos eol=crlf" > .gitattributes
git add .gitattributes
git commit -m "set eol attribute instead"

The following behaviors are observed:

  - `git reset --hard` does not make the working directory clean; and
  - `git rebase` gets *very* confused about the diffs in the working
tree because `git stash` can't reset the working tree;

There are probably other oddities lingering about as well. If I commit
what Git thinks is the difference, the diff (with invisibles made
visible) is:

% git diff | cat -A
diff --git a/dos b/dos$
index fde2310..4723a1b 100644$
--- a/dos$
+++ b/dos$
@@ -1 +1 @@$
-dos^M$
+dos$

Seen in 2.9.5 and 2.14.0.rc1.

--Ben

git bisect "commits left" miscount

[Ben Boeckel]

Hi,

When bisecting given a set of paths, git counts the number of remaining
commits improperly. Here's example output (based in the git.git
repository):

% git bisect start -- sha1_file.c
% git bisect good v2.10.0
% git bisect bad v2.10.3
Bisecting: 1 revision left to test after this (roughly 1 step)
[f7f0a87e0a27a1baaf782af7cec18fd23fdf35de] Merge branch 
'jc/verify-loose-object-header' into maint
% git bisect good
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[5827a03545663f6d6b491a35edb313900608568b] fetch: use "quick" has_sha1_file 
for tag following
% git bisect good
Bisecting: -1 revisions left to test after this (roughly 0 steps)
[39000e849970a554a257577dcb2fb844a523a1d1] Merge branch 
'jk/fetch-quick-tag-following' into maint
% git bisect good
No testable commit found.
Maybe you started with bad path parameters?

Note that it ends up with -1 revisions left and it doesn't end up with a
"this commit is the first bad commit" message.

Should git find the newest ancestors(s) of the given bad commit which
modifies the given paths and start counting from there?

I haven't bisected to find out when this was introduced yet (first seen
with 2.9.4; confirmed with 2.14.0-rc1).

Thanks,

--Ben

[PATCH v6] remote: add get-url subcommand

[Ben Boeckel]

Expanding `insteadOf` is a part of ls-remote --url and there is no way
to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
to query both as well as a way to get all configured urls.

Signed-off-by: Ben Boeckel <maths...@gmail.com>
---
 Documentation/git-remote.txt | 10 
 builtin/remote.c | 59 
 t/t5505-remote.sh| 37 +++
 3 files changed, 106 insertions(+)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4c6d6de..3c9bf45 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git remote remove' 
 'git remote set-head'  (-a | --auto | -d | --delete | )
 'git remote set-branches' [--add]  ...
+'git remote get-url' [--push] [--all] 
 'git remote set-url' [--push]   []
 'git remote set-url --add' [--push]  
 'git remote set-url --delete' [--push]  
@@ -131,6 +132,15 @@ The named branches will be interpreted as if specified 
with the
 With `--add`, instead of replacing the list of currently tracked
 branches, adds to that list.
 
+'get-url'::
+
+Retrieves the URLs for a remote. Configurations for `insteadOf` and
+`pushInsteadOf` are expanded here. By default, only the first URL is listed.
++
+With '--push', push URLs are queried rather than fetch URLs.
++
+With '--all', all URLs for the remote will be listed.
+
 'set-url'::
 
 Changes URLs for the remote. Sets first URL for remote  that matches
diff --git a/builtin/remote.c b/builtin/remote.c
index 181668d..e4c3ea1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,6 +18,7 @@ static const char * const builtin_remote_usage[] = {
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( | 
)...]"),
N_("git remote set-branches [--add]  ..."),
+   N_("git remote get-url [--push] [--all] "),
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
N_("git remote set-url --delete  "),
@@ -65,6 +66,11 @@ static const char * const builtin_remote_update_usage[] = {
NULL
 };
 
+static const char * const builtin_remote_geturl_usage[] = {
+   N_("git remote get-url [--push] [--all] "),
+   NULL
+};
+
 static const char * const builtin_remote_seturl_usage[] = {
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
@@ -1467,6 +1473,57 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+static int get_url(int argc, const char **argv)
+{
+   int i, push_mode = 0, all_mode = 0;
+   const char *remotename = NULL;
+   struct remote *remote;
+   const char **url;
+   int url_nr;
+   struct option options[] = {
+   OPT_BOOL('\0', "push", _mode,
+N_("query push URLs rather than fetch URLs")),
+   OPT_BOOL('\0', "all", _mode,
+N_("return all URLs")),
+   OPT_END()
+   };
+   argc = parse_options(argc, argv, NULL, options, 
builtin_remote_geturl_usage, 0);
+
+   if (argc != 1)
+   usage_with_options(builtin_remote_geturl_usage, options);
+
+   remotename = argv[0];
+
+   if (!remote_is_configured(remotename))
+   die(_("No such remote '%s'"), remotename);
+   remote = remote_get(remotename);
+
+   url_nr = 0;
+   if (push_mode) {
+   url = remote->pushurl;
+   url_nr = remote->pushurl_nr;
+   }
+   /* else fetch mode */
+
+   /* Use the fetch URL when no push URLs were found or requested. */
+   if (!url_nr) {
+   url = remote->url;
+   url_nr = remote->url_nr;
+   }
+
+   if (!url_nr)
+   die(_("no URLs configured for remote '%s'"), remotename);
+
+   if (all_mode) {
+   for (i = 0; i < url_nr; i++)
+   printf_ln("%s", url[i]);
+   } else {
+   printf_ln("%s", *url);
+   }
+
+   return 0;
+}
+
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
@@ -1576,6 +1633,8 @@ int cmd_remote(int argc, const char **argv, const char 
*prefix)
result = set_head(argc, argv);
else if (!strcmp(argv[0], "set-branches"))
result = set_branches(argc, argv);
+   else if (!strcmp(argv[0], "get-url"))
+   result = get_url(argc, argv);
else if (!strcmp(argv[0], "set-url"))
result = set_url(argc, argv);
else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-re

[PATCH v5] remote: add get-url subcommand

[Ben Boeckel]

Expanding `insteadOf` is a part of ls-remote --url and there is no way
to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
to query both as well as a way to get all configured urls.

Signed-off-by: Ben Boeckel <maths...@gmail.com>
---
 Documentation/git-remote.txt | 10 
 builtin/remote.c | 59 
 t/t5505-remote.sh| 39 +
 3 files changed, 108 insertions(+)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4c6d6de..3c9bf45 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git remote remove' 
 'git remote set-head'  (-a | --auto | -d | --delete | )
 'git remote set-branches' [--add]  ...
+'git remote get-url' [--push] [--all] 
 'git remote set-url' [--push]   []
 'git remote set-url --add' [--push]  
 'git remote set-url --delete' [--push]  
@@ -131,6 +132,15 @@ The named branches will be interpreted as if specified 
with the
 With `--add`, instead of replacing the list of currently tracked
 branches, adds to that list.
 
+'get-url'::
+
+Retrieves the URLs for a remote. Configurations for `insteadOf` and
+`pushInsteadOf` are expanded here. By default, only the first URL is listed.
++
+With '--push', push URLs are queried rather than fetch URLs.
++
+With '--all', all URLs for the remote will be listed.
+
 'set-url'::
 
 Changes URLs for the remote. Sets first URL for remote  that matches
diff --git a/builtin/remote.c b/builtin/remote.c
index 181668d..e4c3ea1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,6 +18,7 @@ static const char * const builtin_remote_usage[] = {
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( | 
)...]"),
N_("git remote set-branches [--add]  ..."),
+   N_("git remote get-url [--push] [--all] "),
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
N_("git remote set-url --delete  "),
@@ -65,6 +66,11 @@ static const char * const builtin_remote_update_usage[] = {
NULL
 };
 
+static const char * const builtin_remote_geturl_usage[] = {
+   N_("git remote get-url [--push] [--all] "),
+   NULL
+};
+
 static const char * const builtin_remote_seturl_usage[] = {
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
@@ -1467,6 +1473,57 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+static int get_url(int argc, const char **argv)
+{
+   int i, push_mode = 0, all_mode = 0;
+   const char *remotename = NULL;
+   struct remote *remote;
+   const char **url;
+   int url_nr;
+   struct option options[] = {
+   OPT_BOOL('\0', "push", _mode,
+N_("query push URLs rather than fetch URLs")),
+   OPT_BOOL('\0', "all", _mode,
+N_("return all URLs")),
+   OPT_END()
+   };
+   argc = parse_options(argc, argv, NULL, options, 
builtin_remote_geturl_usage, 0);
+
+   if (argc != 1)
+   usage_with_options(builtin_remote_geturl_usage, options);
+
+   remotename = argv[0];
+
+   if (!remote_is_configured(remotename))
+   die(_("No such remote '%s'"), remotename);
+   remote = remote_get(remotename);
+
+   url_nr = 0;
+   if (push_mode) {
+   url = remote->pushurl;
+   url_nr = remote->pushurl_nr;
+   }
+   /* else fetch mode */
+
+   /* Use the fetch URL when no push URLs were found or requested. */
+   if (!url_nr) {
+   url = remote->url;
+   url_nr = remote->url_nr;
+   }
+
+   if (!url_nr)
+   die(_("no URLs configured for remote '%s'"), remotename);
+
+   if (all_mode) {
+   for (i = 0; i < url_nr; i++)
+   printf_ln("%s", url[i]);
+   } else {
+   printf_ln("%s", *url);
+   }
+
+   return 0;
+}
+
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
@@ -1576,6 +1633,8 @@ int cmd_remote(int argc, const char **argv, const char 
*prefix)
result = set_head(argc, argv);
else if (!strcmp(argv[0], "set-branches"))
result = set_branches(argc, argv);
+   else if (!strcmp(argv[0], "get-url"))
+   result = get_url(argc, argv);
else if (!strcmp(argv[0], "set-url"))
result = set_url(argc, argv);
else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-rem

[PATCH v4] remote: add get-url subcommand

[Ben Boeckel]

Expanding `insteadOf` is a part of ls-remote --url and there is no way
to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
to query both as well as a way to get all configured urls.

Signed-off-by: Ben Boeckel <maths...@gmail.com>
---
 Documentation/git-remote.txt | 10 
 builtin/remote.c | 58 
 t/t5505-remote.sh| 53 
 3 files changed, 121 insertions(+)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4c6d6de..3c9bf45 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git remote remove' 
 'git remote set-head'  (-a | --auto | -d | --delete | )
 'git remote set-branches' [--add]  ...
+'git remote get-url' [--push] [--all] 
 'git remote set-url' [--push]   []
 'git remote set-url --add' [--push]  
 'git remote set-url --delete' [--push]  
@@ -131,6 +132,15 @@ The named branches will be interpreted as if specified 
with the
 With `--add`, instead of replacing the list of currently tracked
 branches, adds to that list.
 
+'get-url'::
+
+Retrieves the URLs for a remote. Configurations for `insteadOf` and
+`pushInsteadOf` are expanded here. By default, only the first URL is listed.
++
+With '--push', push URLs are queried rather than fetch URLs.
++
+With '--all', all URLs for the remote will be listed.
+
 'set-url'::
 
 Changes URLs for the remote. Sets first URL for remote  that matches
diff --git a/builtin/remote.c b/builtin/remote.c
index 181668d..192b9cb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,6 +18,7 @@ static const char * const builtin_remote_usage[] = {
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( | 
)...]"),
N_("git remote set-branches [--add]  ..."),
+   N_("git remote get-url [--push] [--all] "),
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
N_("git remote set-url --delete  "),
@@ -65,6 +66,11 @@ static const char * const builtin_remote_update_usage[] = {
NULL
 };
 
+static const char * const builtin_remote_geturl_usage[] = {
+   N_("git remote get-url [--push] [--all] "),
+   NULL
+};
+
 static const char * const builtin_remote_seturl_usage[] = {
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
@@ -1467,6 +1473,56 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+static int get_url(int argc, const char **argv)
+{
+   int i, push_mode = 0, all_mode = 0;
+   const char *remotename = NULL;
+   struct remote *remote;
+   const char **url;
+   int url_nr;
+   struct option options[] = {
+   OPT_BOOL('\0', "push", _mode,
+N_("query push URLs rather than fetch URLs")),
+   OPT_BOOL('\0', "all", _mode,
+N_("return all URLs")),
+   OPT_END()
+   };
+   argc = parse_options(argc, argv, NULL, options, 
builtin_remote_geturl_usage, 0);
+
+   if (argc != 1)
+   usage_with_options(builtin_remote_geturl_usage, options);
+
+   remotename = argv[0];
+
+   if (!remote_is_configured(remotename))
+   die(_("No such remote '%s'"), remotename);
+   remote = remote_get(remotename);
+
+   url_nr = 0;
+   if (push_mode) {
+   url = remote->pushurl;
+   url_nr = remote->pushurl_nr;
+   }
+
+   /* Fall back to the fetch URL if no push URLs are set. */
+   if (!url_nr) {
+   url = remote->url;
+   url_nr = remote->url_nr;
+   }
+
+   if (!url_nr)
+   die(_("no URLs configured for remote '%s'"), remotename);
+
+   if (all_mode) {
+   for (i = 0; i < url_nr; i++)
+   printf_ln("%s", url[i]);
+   } else {
+   printf_ln("%s", *url);
+   }
+
+   return 0;
+}
+
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
@@ -1576,6 +1632,8 @@ int cmd_remote(int argc, const char **argv, const char 
*prefix)
result = set_head(argc, argv);
else if (!strcmp(argv[0], "set-branches"))
result = set_branches(argc, argv);
+   else if (!strcmp(argv[0], "get-url"))
+   result = get_url(argc, argv);
else if (!strcmp(argv[0], "set-url"))
result = set_url(argc, argv);
else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 7a8499c..2cfd3cb 10

Re: [PATCH v3] remote: add get-url subcommand

[Ben Boeckel]

On Wed, Aug 05, 2015 at 13:34:18 -0700, Junio C Hamano wrote:
 Changes to these two files look reasonable.
 
 Don't you want to protect this feature from future breakage by
 others by adding a couple of tests, though, to t/t5505?

Thanks, I've done so locally. It actually brings up this case:

$ git remote add someremote foo
$ git remote get-url --push someremote
fatal: no URLs configured for remote 'someremote'

Is it better to use:

remote = remote_get(remotename);
remote-pushurl;

if (remote-pushurl_nr)
remote-pushurl;
else
remote-url;

or:

remote = pushremote_get(remotename);
remote-pushurl;

? What is the actual difference between the two?

Thanks,

--Ben
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] remote: add get-url subcommand

[Ben Boeckel]

Expanding `insteadOf` is a part of ls-remote --url and there is no way
to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
to query both as well as a way to get all configured urls.

Signed-off-by: Ben Boeckel maths...@gmail.com
---
 Documentation/git-remote.txt | 10 
 builtin/remote.c | 54 
 2 files changed, 64 insertions(+)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4c6d6de..3c9bf45 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git remote remove' name
 'git remote set-head' name (-a | --auto | -d | --delete | branch)
 'git remote set-branches' [--add] name branch...
+'git remote get-url' [--push] [--all] name
 'git remote set-url' [--push] name newurl [oldurl]
 'git remote set-url --add' [--push] name newurl
 'git remote set-url --delete' [--push] name url
@@ -131,6 +132,15 @@ The named branches will be interpreted as if specified 
with the
 With `--add`, instead of replacing the list of currently tracked
 branches, adds to that list.
 
+'get-url'::
+
+Retrieves the URLs for a remote. Configurations for `insteadOf` and
+`pushInsteadOf` are expanded here. By default, only the first URL is listed.
++
+With '--push', push URLs are queried rather than fetch URLs.
++
+With '--all', all URLs for the remote will be listed.
+
 'set-url'::
 
 Changes URLs for the remote. Sets first URL for remote name that matches
diff --git a/builtin/remote.c b/builtin/remote.c
index f4a6ec9..904869a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,6 +18,7 @@ static const char * const builtin_remote_usage[] = {
N_(git remote prune [-n | --dry-run] name),
N_(git remote [-v | --verbose] update [-p | --prune] [(group | 
remote)...]),
N_(git remote set-branches [--add] name branch...),
+   N_(git remote get-url [--push] [--all] name),
N_(git remote set-url [--push] name newurl [oldurl]),
N_(git remote set-url --add name newurl),
N_(git remote set-url --delete name url),
@@ -65,6 +66,11 @@ static const char * const builtin_remote_update_usage[] = {
NULL
 };
 
+static const char * const builtin_remote_geturl_usage[] = {
+   N_(git remote get-url [--push] [--all] name),
+   NULL
+};
+
 static const char * const builtin_remote_seturl_usage[] = {
N_(git remote set-url [--push] name newurl [oldurl]),
N_(git remote set-url --add name newurl),
@@ -1497,6 +1503,52 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+static int get_url(int argc, const char **argv)
+{
+   int i, push_mode = 0, all_mode = 0;
+   const char *remotename = NULL;
+   struct remote *remote;
+   const char **url;
+   int url_nr;
+   struct option options[] = {
+   OPT_BOOL('\0', push, push_mode,
+N_(query push URLs rather than fetch URLs)),
+   OPT_BOOL('\0', all, all_mode,
+N_(return all URLs)),
+   OPT_END()
+   };
+   argc = parse_options(argc, argv, NULL, options, 
builtin_remote_geturl_usage, 0);
+
+   if (argc != 1)
+   usage_with_options(builtin_remote_geturl_usage, options);
+
+   remotename = argv[0];
+
+   if (!remote_is_configured(remotename))
+   die(_(No such remote '%s'), remotename);
+   remote = remote_get(remotename);
+
+   if (push_mode) {
+   url = remote-pushurl;
+   url_nr = remote-pushurl_nr;
+   } else {
+   url = remote-url;
+   url_nr = remote-url_nr;
+   }
+
+   if (!url_nr)
+   die(_(no URLs configured for remote '%s'), remotename);
+
+   if (all_mode) {
+   for (i = 0; i  url_nr; i++)
+   printf_ln(%s, url[i]);
+   } else {
+   printf_ln(%s, *url);
+   }
+
+   return 0;
+}
+
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
@@ -1606,6 +1658,8 @@ int cmd_remote(int argc, const char **argv, const char 
*prefix)
result = set_head(argc, argv);
else if (!strcmp(argv[0], set-branches))
result = set_branches(argc, argv);
+   else if (!strcmp(argv[0], get-url))
+   result = get_url(argc, argv);
else if (!strcmp(argv[0], set-url))
result = set_url(argc, argv);
else if (!strcmp(argv[0], show))
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] remote: add get-url subcommand

[Ben Boeckel]

On Mon, Aug 03, 2015 at 19:38:15 -0400, Eric Sunshine wrote:
 On Mon, Aug 3, 2015 at 5:00 PM, Ben Boeckel maths...@gmail.com wrote:
  +   OPT_BOOL('\0', push, push_mode,
  +N_(query push URLs)),
 
 A bit more explanatory:
 
 query push URLs rather than fetch URLs

Fixed.

  +   OPT_BOOL('\0', all, all_mode,
  +N_(return all URLs)),
  +   OPT_END()
  +   };
  +   argc = parse_options(argc, argv, NULL, options, 
  builtin_remote_geturl_usage,
  +PARSE_OPT_KEEP_ARGV0);
 
 What is the reason for PARSE_OPT_KEEP_ARGV0 in this case?

Copied from get-url; I presume for more natural argv[] usage within the
function.

  +   if (argc  1 || argc  2)
  +   usage_with_options(builtin_remote_geturl_usage, options);
 
 So,  'argc' must be 1 or 2, which in 'argv' terms is argv[0] and argv[1]).

…

  +   remotename = argv[1];
 
 But here, argv[1] is accessed unconditionally, even though 'argc' may
 have been 1, thus out of bounds.

Yep, should be (argc  2 || argc  2) (or 1 if PARSE_OPT_KEEP_ARGV0 is
removed). Off-by-one when converting from get-url.

I'll reroll tomorrow morning in case there are more comments until then
(particularly about PARSE_OPT_KEEP_ARGV0).

Thanks,

--Ben
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] add git-url subcommand

[Ben Boeckel]

Implements a get-url subcommand to git-remote which allows for querying the
URLs for a remote while expanding insteadOf and pushInsteadOf.

--Ben

Ben Boeckel (1):
  remote: add get-url subcommand

 Documentation/git-remote.txt | 10 
 builtin/remote.c | 55 
 2 files changed, 65 insertions(+)

-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] remote: add get-url subcommand

[Ben Boeckel]

Expanding `insteadOf` is a part of ls-remote --url and there is no way
to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
to query both as well as a way to get all configured urls.

Signed-off-by: Ben Boeckel maths...@gmail.com
---
 Documentation/git-remote.txt | 10 
 builtin/remote.c | 55 
 2 files changed, 65 insertions(+)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4c6d6de..c47b634 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git remote remove' name
 'git remote set-head' name (-a | --auto | -d | --delete | branch)
 'git remote set-branches' [--add] name branch...
+'git remote get-url' [--push] [--all] name
 'git remote set-url' [--push] name newurl [oldurl]
 'git remote set-url --add' [--push] name newurl
 'git remote set-url --delete' [--push] name url
@@ -131,6 +132,15 @@ The named branches will be interpreted as if specified 
with the
 With `--add`, instead of replacing the list of currently tracked
 branches, adds to that list.
 
+'get-url'::
+
+Retrieves the URLs for a remote. Configurations for `insteadOf` and
+`pushInsteadOf` are expanded here. By default, only the first URL is listed.
++
+With '--push', push URLs are queried instead of fetch URLs.
++
+With '--all', all URLs for the remote will be listed.
+
 'set-url'::
 
 Changes URLs for the remote. Sets first URL for remote name that matches
diff --git a/builtin/remote.c b/builtin/remote.c
index f4a6ec9..9278a83 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,6 +18,7 @@ static const char * const builtin_remote_usage[] = {
N_(git remote prune [-n | --dry-run] name),
N_(git remote [-v | --verbose] update [-p | --prune] [(group | 
remote)...]),
N_(git remote set-branches [--add] name branch...),
+   N_(git remote get-url [--push] [--all] name),
N_(git remote set-url [--push] name newurl [oldurl]),
N_(git remote set-url --add name newurl),
N_(git remote set-url --delete name url),
@@ -65,6 +66,11 @@ static const char * const builtin_remote_update_usage[] = {
NULL
 };
 
+static const char * const builtin_remote_geturl_usage[] = {
+   N_(git remote get-url [--push] [--all] name),
+   NULL
+};
+
 static const char * const builtin_remote_seturl_usage[] = {
N_(git remote set-url [--push] name newurl [oldurl]),
N_(git remote set-url --add name newurl),
@@ -1497,6 +1503,53 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+static int get_url(int argc, const char **argv)
+{
+   int i, push_mode = 0, all_mode = 0;
+   const char *remotename = NULL;
+   struct remote *remote;
+   const char **url;
+   int url_nr;
+   struct option options[] = {
+   OPT_BOOL('\0', push, push_mode,
+N_(query push URLs)),
+   OPT_BOOL('\0', all, all_mode,
+N_(return all URLs)),
+   OPT_END()
+   };
+   argc = parse_options(argc, argv, NULL, options, 
builtin_remote_geturl_usage,
+PARSE_OPT_KEEP_ARGV0);
+
+   if (argc  1 || argc  2)
+   usage_with_options(builtin_remote_geturl_usage, options);
+
+   remotename = argv[1];
+
+   if (!remote_is_configured(remotename))
+   die(_(No such remote '%s'), remotename);
+   remote = remote_get(remotename);
+
+   if (push_mode) {
+   url = remote-pushurl;
+   url_nr = remote-pushurl_nr;
+   } else {
+   url = remote-url;
+   url_nr = remote-url_nr;
+   }
+
+   if (!url_nr)
+   die(_(no URLs configured for remote '%s'), remotename);
+
+   if (all_mode) {
+   for (i = 0; i  url_nr; i++)
+   printf_ln(%s, url[i]);
+   } else {
+   printf_ln(%s, *url);
+   }
+
+   return 0;
+}
+
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
@@ -1606,6 +1659,8 @@ int cmd_remote(int argc, const char **argv, const char 
*prefix)
result = set_head(argc, argv);
else if (!strcmp(argv[0], set-branches))
result = set_branches(argc, argv);
+   else if (!strcmp(argv[0], get-url))
+   result = get_url(argc, argv);
else if (!strcmp(argv[0], set-url))
result = set_url(argc, argv);
else if (!strcmp(argv[0], show))
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ls-remote: add --get-push-url option

[Ben Boeckel]

With pushInsteadOf and triangle workflows, a flag to have git fully
expand the push URL is also useful since it can be very different from
the --get-url output.

Signed-off-by: Ben Boeckel maths...@gmail.com
---
 Documentation/git-ls-remote.txt |  7 ++-
 builtin/ls-remote.c | 15 +--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 2e22915..66cda0e 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git ls-remote' [--heads] [--tags]  [-u exec | --upload-pack exec]
- [--exit-code] repository [refs...]
+ [--exit-code] [--get-url | --get-push-url] repository 
[refs...]
 
 DESCRIPTION
 ---
@@ -47,6 +47,11 @@ OPTIONS
url.base.insteadOf config setting (See linkgit:git-config[1]) and
exit without talking to the remote.
 
+--get-push-url::
+   Expand the URL of the given remote repository taking into account any
+   url.base.pushInsteadOf config setting (See linkgit:git-config[1])
+   and exit without talking to the remote.
+
 repository::
The remote repository to query.  This parameter can be
either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 4554dbc..25c6f79 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -5,7 +5,7 @@
 
 static const char ls_remote_usage[] =
 git ls-remote [--heads] [--tags]  [-u exec | --upload-pack exec]\n
- [-q | --quiet] [--exit-code] [--get-url] [repository 
[refs...]];
+ [-q | --quiet] [--exit-code] [--get-url | 
--get-push-url] [repository [refs...]];
 
 /*
  * Is there one among the list of patterns that match the tail part
@@ -40,6 +40,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
const char **pattern = NULL;
 
struct remote *remote;
+   struct remote *pushremote;
struct transport *transport;
const struct ref *ref;
 
@@ -78,6 +79,10 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
get_url = 1;
continue;
}
+   if (!strcmp(--get-push-url, arg)) {
+   get_url = 2;
+   continue;
+   }
if (!strcmp(--exit-code, arg)) {
/* return this code if no refs are reported */
status = 2;
@@ -109,9 +114,15 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
if (!remote-url_nr)
die(remote %s has no configured URL, dest);
 
-   if (get_url) {
+   if (get_url == 1) {
printf(%s\n, *remote-url);
return 0;
+   } else if (get_url == 2) {
+   pushremote = pushremote_get(dest);
+   if (!pushremote-url_nr)
+   die(remote %s has no configured push URL, dest);
+   printf(%s\n, *pushremote-url);
+   return 0;
}
 
transport = transport_get(remote, NULL);
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] add ls-remote --get-push-url option

[Ben Boeckel]

Not sure if it would be better to make a new variable or to reuse the existing
one. I'm reusing it currently because it makes it easier to ensure they
are mutually exclusive.

Please keep me CC'd to the list; I'm not subscribed.

Thanks,

--Ben

Ben Boeckel (1):
  ls-remote: add --get-push-url option

 Documentation/git-ls-remote.txt |  7 ++-
 builtin/ls-remote.c | 15 +--
 2 files changed, 19 insertions(+), 3 deletions(-)

-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add ls-remote --get-push-url option

[Ben Boeckel]

On Fri, Jul 31, 2015 at 12:02:14 -0700, Junio C Hamano wrote:
 Ben Boeckel maths...@gmail.com writes:
 
  With some sed, yes, but then so would `git remote show` just as useful
  too (and in that case, why does --get-url exist either? comes to
  mind).
 
 Either carelessness let it slip in, or it came before 'git remote show'.

Would adding `git remote show --url $remote` and `git remote show
--push-url $remote` be acceptable?

--Ben
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add ls-remote --get-push-url option

[Ben Boeckel]

On Fri, Jul 31, 2015 at 12:16:46 -0700, Junio C Hamano wrote:
 Or even git remote get url [$there], git remote get push-url [$there].

Looking at `git remote`'s existing subcommands, consistency there would
be something like:

git remote get-url $there
git remote get-url --push $there

The new call could also show *all* remotes which will be pushed to, not
just the first. Looking at the structure, it also has fields for
multiple fetch urls. I would assume git would fetch the union of refs;
not sure about conflicts and such.

 Or to mirror the existing ls-remote --get-url [$there], which directly asks
 where does this request go if I run it without '--get-url' option?:
 
 $ git push --get-url [$there [$refspec...]]
 $ git push --get-refspec [$there [$refspec...]]
 
 might be a better option.  The logic in push takes the current
 branch and configurations like branch.*.remote and push.default into
 account, so it is likely that you will get the exact information
 without too much code.

This is a little weird to me. `git push` is porcelain; `git ls-remote`
feels more like plumbing (command-list.txt seems to agree). Maybe it is
also useful since it takes these other configuration values into
account. But maybe pushing has enough extra factors? Might it make sense
in addition to a `git remote get-url`?

 I am not opposed to having a scriptable interface to obtain these
 pieces of information.  I was only objecting to teach ls-remote
 anything about push, which ls-remote does not have anything to do
 with.

OK. Thanks.

--Ben
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add ls-remote --get-push-url option

[Ben Boeckel]

On Fri, Jul 31, 2015 at 11:40:12 -0700, Junio C Hamano wrote:
 Probably get-url makes (some) sense because ls-remote is a fetch
 that does not actually fetch anything.  But get-push-url to
 ls-remote makes _no_ sense whatsoever.  ls-remote and fetch do not
 have to know or care about push-url; they do not even have to know
 there exists a thing called git push ;-)
 
 Wouldn't git push -v -n or something suit your needs already?

With some sed, yes, but then so would `git remote show` just as useful
too (and in that case, why does --get-url exist either? comes to
mind).

Our use case is that we have some scripts which setup the project with
the right remotes and such. To do this, we detect if your remotes are
set up properly already and not ask if things are OK already. This is
currently done with git config --get remote.$x.pushurl, but
`pushInsteadOf` is not expanded and causes false positives.

Where would a utility to have git expand its `pushInsteadOf` aliases
make more sense? Being right beside `insteadOf` expansion made sense to
me (certainly more than some locations for certain flags and actions,
but that boat sailed long ago :) ).

--Ben
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html