* Draft of Git Rev News edition 21
From: Christian Couder @ 2016-11-13 20:55 UTC (permalink / raw)
To: git
Cc: Thomas Ferris Nicolaisen, Jakub Narebski, Junio C Hamano,
Jeff King, Stefan Beller, Jacob Keller, Aaron Pelly,
Nguyen Thai Ngoc Duy, Johannes Schindelin, Alexei Lozovsky,
Martin Braun, Brendan Forster, Parker Moore, Shawn Pearce
Hi,
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-21.md
Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:
https://github.com/git/git.github.io/issues/199
You can also reply to this email.
I tried to cc everyone who appears in this edition but maybe I missed
some people, sorry about that.
Thomas, Jakub Narębski (who is now part of the team, welcome Jakub!)
and myself plan to publish this edition on Wednesday November 16.
Thanks,
Christian.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Stefano Zacchiroli @ 2016-11-13 17:50 UTC (permalink / raw)
To: Christian Couder
Cc: Josh Triplett, Junio C Hamano, git, Shawn O. Pierce, Jeff King
In-Reply-To: <CAP8UFD13sDOFuyZMWuoJeLFt_LAsfAHFBHpRwcdAGmA22xNEKQ@mail.gmail.com>
Hi everyone,
On Sat, Nov 05, 2016 at 12:04:08AM +0100, Christian Couder wrote:
> On Fri, Nov 4, 2016 at 10:19 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Fri, Nov 04, 2016 at 09:47:41PM +0100, Christian Couder wrote:
> >>
> >> Couldn't a RefTree be used to store refs that point to the base
> >> commit, the tip commit and the blob that contains the cover letter,
> >> and maybe also a ref pointing to the RefTree of the previous version
> >> of the series?
> >
> > That's really interesting! The Software Heritage project is working on
> > something similar, because they want to store all the refs as part of
> > their data model as well. I'll point them to the reftree work.
>
> Yeah, I know them :-) and I think I have already told Stefano
> Zacchiroli about this, but I am not sure anymore.
> Anyway I am CC'ing him.
Thanks Christian (and Josh, on swh-devel) for pointing me to this.
As a bit of background, the conceptual data model we have adopted for
Software Heritage [1] is indeed that of a global Merkle DAG, very much
inspired by Git, but where we deduplicate past the boundaries of
individual VCS repositories. This way we can store only once the same
software artifacts (blobs, trees, commits, etc.) even when they can be
found at different software origins [2] (be it due to GitHub-like forks,
projects moving around, or simply rogue copies of the same code
scattered around the Internet).
[1]: https://www.softwareheritage.org/
[2]: "software origin" is Software Heritage terminology, which just
stands for places on the Internet where we can find source code
In our original design the topmost entries in our Merkle hierarchy used
to be commits and tags, similar to what Git does. But then we realized
that doing so inhibited us from sharing entire repository states across
multiple software origins or multiple visits of the same software
origin. So we decided to add "repository snapshot objects" as our
topmost entries, which are essentially git-like objects that map refs to
the ID of the corresponding (typed-)objects. Rationale and a more
lengthy description of this is available on our wiki [3]. It is not
implemented yet, but we're pretty sold on the design at this point.
[3]: https://wiki.softwareheritage.org/index.php?title=Repository_snapshot_objects
Now, even if my only awareness of what's going on in Git upstream is
limited to sporadic chats with Josh and Christian :-), it seems to me
that various ideas in the Git ecosystem go in the same direction of our
snapshot objects (git-series, RefTree). Which is understandable, given a
number of use cases might be served by this.
I don't think we have much to contribute to discussion or implementation
here, and for our needs it doesn't really matter which one gets
implemented. That's because we need an implementation of the concept
which is *external* to Git anyhow. But even if it happens to exist
within actual VCS, it's not a big deal for us, as we do have ways to
distinguish "synthetic" objects in the DAG that we create for our own
needs from "real" objects coming from actual software origins.
(Another example of this concept we already have is when we inject
distribution source packages or tarballs in our archive. In that case we
create synthetic commits that points to the tree extracted from the
tarball/package, preserving the ability to distinguish them from real
commits coming from VCS out there.)
If you think we can help in any other way, other than sharing our
experiences and design considerations that is, please let me know! (I'm
not subscribed to the Git upstream mailing list, but feel free to Cc:-me
in conversations related to this topic.)
Cheers.
--
Stefano Zacchiroli . zack@upsilon.cc . upsilon.cc/zack . . o . . . o . o
Computer Science Professor . CTO Software Heritage . . . . . o . . . o o
Former Debian Project Leader . OSI Board Director . . . o o o . . . o .
« the first rule of tautology club is the first rule of tautology club »
^ permalink raw reply
* [PATCH] attr: mark a file-local symbol as static
From: Ramsay Jones @ 2016-11-13 16:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Stefan,
If you need to re-roll your 'sb/attr' branch, could you please
squash this into the relevant patch.
Alternatively, since there is only a single call site for git_attr()
(on line #1005), you could perhaps remove git_attr() and inline that
call. (However, that does make that line exceed 80 columns).
Thanks!
ATB,
Ramsay Jones
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 667ba85..84c4b08 100644
--- a/attr.c
+++ b/attr.c
@@ -169,7 +169,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)
return a;
}
-struct git_attr *git_attr(const char *name)
+static struct git_attr *git_attr(const char *name)
{
return git_attr_internal(name, strlen(name));
}
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-13 14:07 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xqHKVUfNm+jCsbMNazHpVhB46h60k75JzS35CrkR-d-UQ@mail.gmail.com>
Hello,
On Wed, Nov 9, 2016 at 5:28 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>> Add tests and documentation for the same.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 34 +++++++++++++++++++---------------
>> ref-filter.c | 29 +++++++++++++++++++++++++----
>> t/t6300-for-each-ref.sh | 24 ++++++++++++++++++++++++
>> 3 files changed, 68 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 600b703..f4ad297 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -96,7 +96,9 @@ refname::
>> slash-separated path components from the front of the refname
>> (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>> `<N>` must be a positive integer. If a displayed ref has fewer
>> - components than `<N>`, the command aborts with an error.
>> + components than `<N>`, the command aborts with an error. For the base
>> + directory of the ref (i.e. foo in refs/foo/bar/boz) append
>> + `:base`. For the entire directory path append `:dir`.
>>
>> objecttype::
>> The type of the object (`blob`, `tree`, `commit`, `tag`).
>> @@ -114,22 +116,23 @@ objectname::
>>
>> upstream::
>> The name of a local ref which can be considered ``upstream''
>> - from the displayed ref. Respects `:short` and `:strip` in the
>> - same way as `refname` above. Additionally respects `:track`
>> - to show "[ahead N, behind M]" and `:trackshort` to show the
>> - terse version: ">" (ahead), "<" (behind), "<>" (ahead and
>> - behind), or "=" (in sync). `:track` also prints "[gone]"
>> - whenever unknown upstream ref is encountered. Append
>> - `:track,nobracket` to show tracking information without
>> - brackets (i.e "ahead N, behind M"). Has no effect if the ref
>> - does not have tracking information associated with it.
>> + from the displayed ref. Respects `:short`, `:strip`, `:base`
>> + and `:dir` in the same way as `refname` above. Additionally
>> + respects `:track` to show "[ahead N, behind M]" and
>> + `:trackshort` to show the terse version: ">" (ahead), "<"
>> + (behind), "<>" (ahead and behind), or "=" (in sync). `:track`
>> + also prints "[gone]" whenever unknown upstream ref is
>> + encountered. Append `:track,nobracket` to show tracking
>> + information without brackets (i.e "ahead N, behind M"). Has
>> + no effect if the ref does not have tracking information
>> + associated with it.
>>
>> push::
>> The name of a local ref which represents the `@{push}`
>> location for the displayed ref. Respects `:short`, `:strip`,
>> - `:track`, and `:trackshort` options as `upstream`
>> - does. Produces an empty string if no `@{push}` ref is
>> - configured.
>> + `:track`, `:trackshort`, `:base` and `:dir` options as
>> + `upstream` does. Produces an empty string if no `@{push}` ref
>> + is configured.
>>
>
> At this point would it make more sense to document the extra values
> here in one block separately? For example, the upstream atom is
> getting pretty complex with all those options. Additionally, some of
> the options can be combined, like nobracket, but others cannot be
> comined so It may be worth documenting how and when those combinations
> work?
>
I do agree. The documentation seems cluttered. It'd be nice to clean
it up. Maybe
after this series? Or any suggestions on how to go about it?
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-11-13 2:44 UTC (permalink / raw)
To: Junio C Hamano, Jon Loeliger; +Cc: git
In-Reply-To: <xmqqpomiuu8q.fsf@gitster.mtv.corp.google.com>
On Sun, 2016-10-30 at 01:16 -0700, Junio C Hamano wrote:
> Jon Loeliger <jdl@jdl.com> writes:
>
> >
> > Is there an existing protocol provision, or an extension to
> > the protocol that would allow a distrustful client to say to
> > the server, "Really, you have Y2? Prove it."
>
> There is not, but I do not think it would be an effective solution.
>
> The issue is not the lack of protocol support, but how to determine
> that the other side needs such a proof for Y2 but not for other
> commits. How does your side know what makes Y2 special and why does
> yout side think they should not have Y2?
>
> Once you know how to determine Y2 is special, that knowledge can be
> used to abort the "push" before even starting. When you are pushing
> back the 'master' and that 'master' reaches Y2, which must be kept
> secret, you shouldn't be pushing that 'master' to them, whether they
> claim to have Y2 or not.
FWIW, I can imagine a protocol that would prove possession for all
objects, which would completely fix the problem. Each object would
have a "private" hash computed recursively over the object graph, just
like the ordinary object hash, but with a different seed. The object
database would be extended to cache the private hash of every object.
Then, during a fetch or push, when the two sides identify a matching
object, the side that would otherwise have had to send the object sends
the private hash. Support for storing multiple hashes per object might
also be useful in some way for the migration to a stronger hash
function than SHA-1.
The next best solution, which doesn't require a protocol change but
requires a little user intervention, is to have a configuration option
per remote for a set of refs whose reachable objects are known to be
safe to send to the server. This set presumably includes the remote's
own remote-tracking refs. During fetch and push, the client looks for
matches only among these "safe" objects, effectively emulating a
repository containing only the safe objects. A fetch may update the
remote-tracking refs to point to unsafe objects that were already in
the local repository, effectively making them safe, but only after the
server sends the content of these objects (and the client validates the
hashes!).
Unfortunately, I'm not signing up to implement either solution. :(
Matt
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-11-13 2:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwbuuuuy.fsf@gitster.mtv.corp.google.com>
On Sun, 2016-10-30 at 01:03 -0700, Junio C Hamano wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
>
> >
> > On Fri, 2016-10-28 at 22:31 -0700, Junio C Hamano wrote:
> > >
> > > Not sending to the list, where mails from Gmail/phone is known to
> > > get
> > > rejected.
> >
> > [I guess I can go ahead and quote this to the list.]
> >
> > >
> > > No. I'm saying that the scenario you gave is bad and people
> > > should be
> > > taught not to connect to untrustworthy sites.
> >
> > To clarify, are you saying:
> >
> > (1) don't connect to an untrusted server ever (e.g., we don't
> > promise
> > that the server can't execute arbitrary code on the client), or
> >
> > (2) don't connect to an untrusted server if the client repository
> > has
> > data that needs to be kept secret from the server?
>
> You sneaked "arbitrary code execution" into the discussion but I do
> not know where it came from. In any case, "don't pull from or push
> to untrustworthy place" would be a common sense advice that would
> make sense in any scenario ;-)
A blanket statement like that without explanation is not very helpful
to users who do find themselves needing to pull from or push to a
server they don't absolutely trust. The only "definitely safe" option
it leaves them is to run the entire thing in a sandbox. A statement of
the nature of the risk is much more helpful: users can determine that
they don't care about the risk, or if it does, what the easiest
workaround is.
The new risk we discovered in this thread is of leakage of private data
from the local repository. To avoid that risk, it's sufficient for
users to move private data to a separate repository, so that's the
advice I propose to give. Are you aware of issues with fetch/push with
potential impact beyond leakage of private data, which would make my
proposed text insufficient? I was giving "arbitrary code execution" as
an example of what the impact of such an issue could be.
> Just for future reference, when you have ideas/issues that might
> have possible security ramifications, I'd prefer to see it first
> discussed on a private list we created for that exact purpose, until
> we can assess the impact (if any). Right now MaintNotes says this:
>
> If you think you found a security-sensitive issue and want to
> disclose
> it to us without announcing it to wider public, please contact us
> at
> our security mailing list <git-security@googlegroups.com>. This
> is
> a closed list that is limited to people who need to know early
> about
> vulnerabilities, including:
>
> - people triaging and fixing reported vulnerabilities
> - people operating major git hosting sites with many users
> - people packaging and distributing git to large numbers of
> people
>
> where these issues are discussed without risk of the information
> leaking out before we're ready to make public announcements.
>
> We may want to tweak the description from "disclose it to us" to
> "have a discussion on it with us" (the former makes it sound as if
> the topic has to be a definite problem, the latter can include an
> idle speculation that may not be realistic attack vector).
OK. I'll admit that I didn't even look for a policy on reporting of
security issues because I believed the issue had low enough impact that
a report to a dedicated security contact point would be unwelcome.
Maybe that was reckless. The new text sounds good, if you put it in a
place where people like me would see it. :/
Matt
^ permalink raw reply
* Re: Feature request - show result of URL rewrites
From: Junio C Hamano @ 2016-11-13 1:46 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Git User, git
In-Reply-To: <1478974642.3227.4.camel@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>> Git lets you rewrite URLs using "url.<base>.insteadOf"
>> ...
>> Can you add a git-config option to show the result of this rewriting
>> whenever this occurs, as debugging more complicated rules can be
>> difficult/wasn't obvious without Wireshark.
>>
>> E.g. you could have the option 'url.printRewrites [True/False]' which
>> would print the line "Rewrote url 'git://github.com/git/git' to 'http
>> s://github.com/git/git'" to terminal/stdout when set to True.
>
> Such a configuration would be superfluous, the GIT_TRACE and
> GIT_CURL_VERBOSE environment variables already provide all the
> debugging information you need here.
While i tend to agree with you that this kind of thing should not be
a new "configuration" that you need to unset after you are done
debugging, and should instead be done with a single-shot request
mechanism like environment variables, I do not think your response
is a fair and useful one. Perhaps it is fair for the original
request that wants to have a knob to learn what the original was
rewritten to, but then the original request did not aim high enough
to get a useful feature, I would suspect.
GIT_TRACE or CURL_VERBOSE may show you the rewritten URL, and the
end user (presumably) knows what the original URL s/he handed Git to
go to, but I do not think these two are sufficient to find out what
triggered one URL to be rewritten to the other URL, which probably
is what the user wants to know the most.
Your response may become useful if remove.c::alias_url() is taught
to react to GIT_TRACE (or some other debugging mechanism we already
have), so that it reports what rule caused what URL to be rewritten
to what other URL.
This is just to illustrate where to patch and not meant to even
compile (e.g. "debug" is not even a variable defined anywhere), but
starting from something like this, perhaps?
remote.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/remote.c b/remote.c
index ad6c5424ed..09dca9468f 100644
--- a/remote.c
+++ b/remote.c
@@ -63,11 +63,9 @@ static int valid_remote(const struct remote *remote)
static const char *alias_url(const char *url, struct rewrites *r)
{
int i, j;
- struct counted_string *longest;
- int longest_i;
+ struct counted_string *longest = NULL;
+ int longest_i = -1, instead_of_j = -1;
- longest = NULL;
- longest_i = -1;
for (i = 0; i < r->rewrite_nr; i++) {
if (!r->rewrite[i])
continue;
@@ -77,12 +75,24 @@ static const char *alias_url(const char *url, struct rewrites *r)
longest->len < r->rewrite[i]->instead_of[j].len)) {
longest = &(r->rewrite[i]->instead_of[j]);
longest_i = i;
+ instead_of_j = j;
}
}
}
if (!longest)
return url;
+ if (debug) {
+ fprintf(stderr, "rewriting %s to %s%s\n",
+ url,
+ r->rewrite[longest_i]->base, url + longest->len);
+ fprintf(stderr, "due to '%.*s.insteadof' = '%.*s'\n",
+ r->rewrite[longest_i]->baselen,
+ r->rewrite[longest_i]->base,
+ r->rewrite[longest_i]->instead_of[j].len,
+ r->rewrite[longest_i]->instead_of[j].s);
+ }
+
return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
}
^ permalink raw reply related
* [PATCH] fetch/push: document that private data can be leaked
From: Matt McCutchen @ 2016-11-13 1:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <xmqqy416uvan.fsf@gitster.mtv.corp.google.com>
A malicious server may be able to use the fetch and push protocols to
steal data from a user's repository that the user did not intend to
share, via attacks similar to those described in the gitnamespaces(7)
man page. Mention this in the git-fetch(1), git-pull(1), and git-push(1)
man pages and recommend using separate repositories for private data and
interaction with untrusted servers.
Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
And here's a proposed patch. Based on the maint branch, ac84098.
Documentation/fetch-push-security.txt | 9 +++++++++
Documentation/git-fetch.txt | 2 ++
Documentation/git-pull.txt | 2 ++
Documentation/git-push.txt | 2 ++
4 files changed, 15 insertions(+)
create mode 100644 Documentation/fetch-push-security.txt
diff --git a/Documentation/fetch-push-security.txt b/Documentation/fetch-push-security.txt
new file mode 100644
index 0000000..00944ed
--- /dev/null
+++ b/Documentation/fetch-push-security.txt
@@ -0,0 +1,9 @@
+SECURITY
+--------
+The fetch and push protocols are not designed to prevent a malicious
+server from stealing data from your repository that you did not intend to
+share. The possible attacks are similar to the ones described in the
+"SECURITY" section of linkgit:gitnamespaces[7]. If you have private data
+that you need to protect from the server, keep it in a separate
+repository.
+
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index 9e42169..a461b4b 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -192,6 +192,8 @@ The first command fetches the `maint` branch from the repository at
objects will eventually be removed by git's built-in housekeeping (see
linkgit:git-gc[1]).
+include::fetch-push-security.txt[]
+
BUGS
----
Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index d033b25..0af2de9 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -237,6 +237,8 @@ If you tried a pull which resulted in complex conflicts and
would want to start over, you can recover with 'git reset'.
+include::fetch-push-security.txt[]
+
BUGS
----
Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 47b77e6..5ebef9e 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -559,6 +559,8 @@ Commits A and B would no longer belong to a branch with a symbolic name,
and so would be unreachable. As such, these commits would be removed by
a `git gc` command on the origin repository.
+include::fetch-push-security.txt[]
+
GIT
---
Part of the linkgit:git[1] suite
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] upload-pack: Optionally allow fetching any sha1
From: Junio C Hamano @ 2016-11-13 1:23 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce
In-Reply-To: <1478885028-30357-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 43665ab..8d3db40 100755
It seems that I haven't heard of 43665ab.
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -306,6 +306,28 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
> test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
> '
Specifically, the above seems to be missing in my tree.
Perhaps you noticed the lack of test for allowReachableSHA1InWant
and added one, but forgot to send it out, while building this patch
on top?
> +uploadpack.allowAnySHA1InWant::
> + Allow `upload-pack` to accept a fetch request that asks for any
> + object at all.
> + Defaults to `false`.
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Junio C Hamano @ 2016-11-13 1:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git, Jeff King
In-Reply-To: <alpine.DEB.2.20.1611121237230.3746@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Thanks. Dscho, does this fix both of these issues to you?
>
> Apparently it does because the CI jobs for `master` and for `next` pass.
OK, thanks for a quick confirmation.
> The one for `pu` still times out, of course.
Earlier you said 'pu' did even not build, but do we know where this
"still times out" comes from? As long as I don't merge anything
prematurely, which I need to be careful about, it shouldn't impact
the upcoming release, but we'd need to figure it out before moving
things forward post release.
Thanks.
^ permalink raw reply
* Re: [PATCH v7 11/17] ref-filter: introduce symref_atom_parser() and refname_atom_parser()
From: Karthik Nayak @ 2016-11-12 20:12 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xrHRMZemNfo3gjJryBRR5b4T8VT3t9_CBE5hJJfv+D3PQ@mail.gmail.com>
On Wed, Nov 9, 2016 at 5:22 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>
> What's the reasoning for using separate functions here if they are
> exactly identical except for name? Do we intend to add separate
> options for this? I don't really have a problem with separate
> functions here since it helps avoid confusion but they are identical
> otherwise...
>
I see no need to have separate functions. Maybe in the future when we
plan on adding
specific options we could split them. I'll drop the
symref_atom_parser() function and use
refname_atom_parser() both places.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 08/17] ref-filter: add support for %(upstream:track,nobracket)
From: Karthik Nayak @ 2016-11-12 20:01 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xrm-MMRa9RuaHUGNWJmH8UcYPj3RZCsy1uvVRMuLeP0ZQ@mail.gmail.com>
On Wed, Nov 9, 2016 at 5:15 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>> This is needed when we port branch.c to use ref-filter's printing APIs.
>>
>
> Makes sense. Seems a bit weird that we have the brackets normally
> rather than adding them as an option, but I think this is ok. We don't
> want to change all previous uses in this case.
>
> My only suggestion here would be to add code so that the options die()
> when we use nobracket along with trackshort or without track. This
> ensures that the nobracket option only applies to track mode?
>
Sure, seems reasonable. There's also the fact that even though nobracket
actually only supports the 'track' option. It might make sense to
leave it as it
is, i.e not die() when used with the other options cause, it kinda stays true to
it being used. e.g. %(upstream:nobracket,trackshort) does give 'trackshort'
without brackets, even though that's the default option and only way we print
'trackshort' information, it seems to make sense from the users point of view.
>> Add test and documentation for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 8 +++--
>> ref-filter.c | 67 +++++++++++++++++++++++++-------------
>> t/t6300-for-each-ref.sh | 2 ++
>> 3 files changed, 51 insertions(+), 26 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index fd365eb..3953431 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -118,9 +118,11 @@ upstream::
>> `refname` above. Additionally respects `:track` to show
>> "[ahead N, behind M]" and `:trackshort` to show the terse
>> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> - or "=" (in sync). Has no effect if the ref does not have
>> - tracking information associated with it. `:track` also prints
>> - "[gone]" whenever unknown upstream ref is encountered.
>> + or "=" (in sync). `:track` also prints "[gone]" whenever
>> + unknown upstream ref is encountered. Append `:track,nobracket`
>> + to show tracking information without brackets (i.e "ahead N,
>> + behind M"). Has no effect if the ref does not have tracking
>> + information associated with it.
>>
>
> Ok so my comment on the previous patch is fixed here, the new wording
> makes it much more clear that [gone] is not the same thing as no
> information. So I don't think we should bother changing the previous
> patch in the series. This might want to document that nobracket works
> even without track, even if it doesn't actually do anything? Or make
> the code more strict in that we die() if the values are put together
> that make no sense?
>
Speaking from what I said above, maybe it makes sense to leave it like it is?
>> push::
>> The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6d51b80..4d7e414 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -46,8 +46,10 @@ static struct used_atom {
>> union {
>> char color[COLOR_MAXLEN];
>> struct align align;
>> - enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>> - remote_ref;
>> + struct {
>> + enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
>> + unsigned int nobracket: 1;
>> + } remote_ref;
>> struct {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> unsigned int nlines;
>> @@ -75,16 +77,33 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>>
>> static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>> {
>> - if (!arg)
>> - atom->u.remote_ref = RR_NORMAL;
>> - else if (!strcmp(arg, "short"))
>> - atom->u.remote_ref = RR_SHORTEN;
>> - else if (!strcmp(arg, "track"))
>> - atom->u.remote_ref = RR_TRACK;
>> - else if (!strcmp(arg, "trackshort"))
>> - atom->u.remote_ref = RR_TRACKSHORT;
>> - else
>> - die(_("unrecognized format: %%(%s)"), atom->name);
>> + struct string_list params = STRING_LIST_INIT_DUP;
>> + int i;
>> +
>> + if (!arg) {
>> + atom->u.remote_ref.option = RR_NORMAL;
>> + return;
>> + }
>> +
>> + atom->u.remote_ref.nobracket = 0;
>> + string_list_split(¶ms, arg, ',', -1);
>> +
>> + for (i = 0; i < params.nr; i++) {
>> + const char *s = params.items[i].string;
>> +
>> + if (!strcmp(s, "short"))
>> + atom->u.remote_ref.option = RR_SHORTEN;
>> + else if (!strcmp(s, "track"))
>
> Should we add die()s here to disallow setting the remote_ref option
> multiple times? Otherwise, we should document that the last one wins?
> Not sure it's really a big deal here, but we could do it to ensure
> consistency for options which don't make sense together?
>
It makes sense to add a small note that the last option wins, I suppose
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 07/17] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
From: Karthik Nayak @ 2016-11-12 18:48 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xoKxGiNJO4g=Nm+KY=5=u8g2edoLOoe5K0ZAPrvoGj-Ug@mail.gmail.com>
On Wed, Nov 9, 2016 at 5:07 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> Make changes to t/t6300-for-each-ref.sh and
>> Documentation/git-for-each-ref.txt to reflect this change.
>>
>
> This will change behavior if people were expecting it to remain
> silent, but I think this could be considered a bug.
>
Didn't get you.
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Helped-by : Jacob Keller <jacob.keller@gmail.com>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 3 ++-
>> ref-filter.c | 4 +++-
>> t/t6300-for-each-ref.sh | 2 +-
>> 3 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 92184c4..fd365eb 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -119,7 +119,8 @@ upstream::
>> "[ahead N, behind M]" and `:trackshort` to show the terse
>> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> or "=" (in sync). Has no effect if the ref does not have
>> - tracking information associated with it.
>> + tracking information associated with it. `:track` also prints
>> + "[gone]" whenever unknown upstream ref is encountered.
>>
>
> I think this is poorly worded. If I understand, "has no effect if the
> ref does not have tracking information" so in that case we still print
> nothing, right? but otherwise we print [gone] only when the upstream
> ref no longer actually exists locally? I wonder if there is a better
> wording for this? I don't have one. Any suggestions to avoid confusing
> these two cases?
>
Dropping this, since its taken care of in the next patch.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: Feature request - show result of URL rewrites
From: Dennis Kaarsemaker @ 2016-11-12 18:17 UTC (permalink / raw)
To: Git User, git
In-Reply-To: <284668531.626.1478972994238.JavaMail.root@ichabod>
On Sat, 2016-11-12 at 18:49 +0100, Git User wrote:
> Hello
>
> Hopefully this is the right place to submit feature requests - let me
> know if there's somewhere else I should use!
>
> Git lets you rewrite URLs using "url.<base>.insteadOf"
>
> https://stackoverflow.com/a/11383587
> https://git-scm.com/docs/git-config
>
> Can you add a git-config option to show the result of this rewriting
> whenever this occurs, as debugging more complicated rules can be
> difficult/wasn't obvious without Wireshark.
>
> E.g. you could have the option 'url.printRewrites [True/False]' which
> would print the line "Rewrote url 'git://github.com/git/git' to 'http
> s://github.com/git/git'" to terminal/stdout when set to True.
Such a configuration would be superfluous, the GIT_TRACE and
GIT_CURL_VERBOSE environment variables already provide all the
debugging information you need here.
D.
^ permalink raw reply
* Feature request - show result of URL rewrites
From: Git User @ 2016-11-12 17:49 UTC (permalink / raw)
To: git
Hello
Hopefully this is the right place to submit feature requests - let me know if there's somewhere else I should use!
Git lets you rewrite URLs using "url.<base>.insteadOf"
https://stackoverflow.com/a/11383587
https://git-scm.com/docs/git-config
Can you add a git-config option to show the result of this rewriting whenever this occurs, as debugging more complicated rules can be difficult/wasn't obvious without Wireshark.
E.g. you could have the option 'url.printRewrites [True/False]' which would print the line "Rewrote url 'git://github.com/git/git' to 'https://github.com/git/git'" to terminal/stdout when set to True.
Thanks
^ permalink raw reply
* Re: [RFC] Add way to make Git credentials accessible from clean/smudge filter
From: Lars Schneider @ 2016-11-12 13:57 UTC (permalink / raw)
To: Jeff King; +Cc: Dennis Kaarsemaker, Matthieu Moy, git, me
In-Reply-To: <20161111202710.7zpalypsnorqeclq@sigill.intra.peff.net>
> On 11 Nov 2016, at 21:27, Jeff King <peff@peff.net> wrote:
>
> On Fri, Nov 11, 2016 at 09:02:52PM +0100, Dennis Kaarsemaker wrote:
>
>>>> Are you sure about that? If I do:
>>>>
>>>> echo url=https://example.com/repo.git |
>>>> git credential fill
>>>>
>>>> I get prompted for a username and password.
>>>
>>>
>>> Hm.. either I don't understand you or I expressed myself unclear.
>>>
>>> Let's say a user runs:
>>>
>>> $ git clone https://myrepo.git
>>>
>>> If no credential helper is setup, then Git asks the user for credentials.
>>> Afterwards Git starts downloading stuff. At some point Git will run my
>>> smudge filter on some files and in my case the smudge filter needs the
>>> Git credentials. AFAIK, the smudge filter has no way to get the credentials
>>> from Git at this point - not even by invoking "git credential".
>>> Is this correct?
>>
>> I think that's correct, but the same argument goes both ways: unless I
>> use a credential helper, or explicitely give a filter application my
>> credentials, I don't want a helper to be able to get to those
>> credentials. I'd consider that a security bug.
>
> Yeah, agreed. They are logically two separate operations, so I think it
> is a feature that they do not implicitly share credentials.
>
> I think the only place where we implicitly share credentials is when
> serving an HTTP fetch or push requires multiple HTTP requests. And there
> it seems pretty sane to do so.
Agreed. Thanks for your thoughts on this!
- Lars
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Lars Schneider @ 2016-11-12 13:33 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <a90fe336-5b62-4278-64e6-e73cb65a02a8@kdbg.org>
> On 11 Nov 2016, at 22:22, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 11.11.2016 um 22:07 schrieb Junio C Hamano:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> OK, then let's have
>>>
>>> filter_git () {
>>> rm -f rot13-filter.log &&
>>> git "$@"
>>> ...
>>>
>>> and call that -rc1.
>>
>> That is, to queue this on top of ls/filter-process and merge it
>> down.
>>
>> -- >8 --
>> Subject: t0021: remove debugging cruft
>>
>> The redirection of the standard error stream to a temporary file is
>> a leftover cruft during debugging. Remove it.
>>
>> Besides, it is reported by folks on the Windows that the test is
>> flaky with this redirection; somebody gets confused and this
>> merely-redirected-to file gets marked as delete-pending by git.exe
>> and makes it finish with a non-zero exit status when "git checkout"
>> finishes. Windows folks may want to figure that one out, but for
>> the purpose of this test, it shouldn't become a show-stopper.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Nicely summarized! Thank you very much!
>
> I've double-checked again that the redirection is really not used by the tests. The name does occur in a .gitignore, but I guess that is not important for the background filter process. It would be good if Lars could confirm this.
Yesterday, I wasn't sure if silencing stderr was required by the test.
Junio's assessment and solution is spot on.
Thanks a lot,
Lars
>
>> ---
>> t/t0021-conversion.sh | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index cb72fa49de..5ce2535017 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -27,8 +27,7 @@ file_size () {
>>
>> filter_git () {
>> rm -f rot13-filter.log &&
>> - git "$@" 2>git-stderr.log &&
>> - rm -f git-stderr.log
>> + git "$@"
>> }
>>
>> # Compare two files and ensure that `clean` and `smudge` respectively are
>>
>
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Johannes Schindelin @ 2016-11-12 11:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Jeff King
In-Reply-To: <xmqqa8d53ky9.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 11 Nov 2016, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > We have to use $PWD instead of $(pwd) because on Windows the latter
> > would add a C: style path to bash's Unix-style $PATH variable, which
> > becomes confused by the colon after the drive letter. ($PWD is a
> > Unix-style path.)
> >
> > In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
> > assembles a Unix-style path list with the colon as separators. It
> > converts the value to a Windows-style path list with the semicolon as
> > path separator when it forwards the variable to git.exe. The same
> > confusion happens when bash's original value is contaminated with
> > Windows style paths.
> >
> > Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> > ---
> > Am 11.11.2016 um 18:11 schrieb Johannes Sixt:
> >> Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
> >>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>> ...
> >
> > When the MSYS program such as bash invokes a non-MSYS program, it
> > translates the Unix-style paths in arguments and environment variables
> > to Windows stlye. We only have to ensure that we inject only Unix-style
> > paths in these places so as not to confuse the conversion algorithm.
> > Most of the time, we do not have to worry.
> >
> > On the other hand, when we write a path to a file that git.exe consumes
> > or receive a path from git.exe, i.e., when the path travels through
> > stdout and stdin, no automatic translation happens (which is quite
> > understandable), and we have do the translation explicitly. An example
> > for such a case is when we write a .git/info/alternates file via the
> > shell.
> >
> >> A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a
> >> moment.
> >
> > Here it is. I had proposed the t0021 part earlier, but it fell through
> > the cracks during the temporary maintainer change.
>
> Thanks. Dscho, does this fix both of these issues to you?
Apparently it does because the CI jobs for `master` and for `next` pass.
The one for `pu` still times out, of course.
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Johannes Schindelin @ 2016-11-12 10:08 UTC (permalink / raw)
To: Jacob Keller; +Cc: Junio C Hamano, Dennis Kaarsemaker, Git mailing list
In-Reply-To: <CA+P7+xrFYj7sepdz1xrEEaqmXQWfRkBz3CsWjYK6OZsZRMLbUA@mail.gmail.com>
Hi,
On Fri, 11 Nov 2016, Jacob Keller wrote:
> On Fri, Nov 11, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> >
> >> No tests or documentation updates yet, and I'm not sure whether
> >> --follow-symlinks in other modes than --no-index should be supported, ignored
> >> (as it is now) or cause an error, but I'm leaning towards the third option.
> >
> > My knee-jerk reaction is:
> >
> > * The --no-index mode should default to your --follow-symlinks
> > behaviour, without any option to turn it on or off.
> >
>
> I agree. We shouldn't have to specify this for no-index.
Ummm. *My* idea of --no-index was for it to behave as similar to the
--index version as possible. For example when comparing directories
containing symlinks. You seem intent on breaking this scenario.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length
From: Karthik Nayak @ 2016-11-12 9:56 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xqXY6XvUm8Kna_GwY4XOSSGPM=-ezZECgkGcbnX_Penmg@mail.gmail.com>
Hello,
On Fri, Nov 11, 2016 at 10:59 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Thu, Nov 10, 2016 at 9:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>
>> That does make sense, It would also not error out when we use
>> %(objectname:short=) and
>> not specify the length. Idk, if that's desirable or not. But it does
>> make the code a little more
>> confusing to read at the same time.
>>
>
> I am not sure that would be the case. If you see "objectname:short"
> you trreat this as if they had passed "objectname:short=<default
> abbrev>" but if you see "objectname:short=" you die, no?
>
Sorry, my bad.
On Fri, Nov 11, 2016 at 5:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> else if (!strcmp(arg, "short"))
>> - atom->u.objectname = O_SHORT;
>> - else
>> + atom->u.objectname.option = O_SHORT;
>> + else if (skip_prefix(arg, "short=", &arg)) {
>> + atom->u.objectname.option = O_LENGTH;
>> + if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
>> + atom->u.objectname.length == 0)
>> + die(_("positive value expected objectname:short=%s"), arg);
>> + if (atom->u.objectname.length < MINIMUM_ABBREV)
>> + atom->u.objectname.length = MINIMUM_ABBREV;
>> + } else
>> die(_("unrecognized %%(objectname) argument: %s"), arg);
>> }
>
> Users who want to use the default-abbrev, i.e. the autoscaling one
> introduced recently, must use "short", not "short=-1", with this
> code (especially with the "must be at least MINIMUM_ABBREV" logic),
> but I do not think it is a problem, so I think this is good.
>
I think I'll leave this as it is. If that's okay
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Karthik Nayak @ 2016-11-12 9:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqqoa1m5335.fsf@gitster.mtv.corp.google.com>
Hello,
On Fri, Nov 11, 2016 at 4:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>> @@ -49,6 +51,10 @@ static struct used_atom {
>>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>> unsigned int nlines;
>>> } contents;
>>> + struct {
>>> + const char *if_equals,
>>> + *not_equals;
>>
>>
>> Same here, why do we need both strings here stored separately? Could
>> we instead store which state to check and store the string once? I'm
>> not sure that really buys us any storage.
>
> I am not sure if storage is an issue, but I tend to agree that it
> would be semantically cleaner if this was done as a pair of <what
> operation uses this string constant?, the string constant>, and the
> former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.
>
> You could later enhance the comparison operator more easily with
> such an arrangement (e.g. if-equals-case-insensitively).
That's a rather good point you make there, I think using an enum with a string
constant would be ideal for future extensibility too. Thanks for the
explanation,
I shall change it.
--
Regards,
Karthik Nayak
^ permalink raw reply
* [PATCH] git-worktree.txt: Fix typo "to"/"two", add comma
From: Ben North @ 2016-11-12 8:55 UTC (permalink / raw)
To: git; +Cc: pclouds
Signed-off-by: Ben North <ben@redfrontdoor.org>
---
Documentation/git-worktree.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 0aeb020..e257c19 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -133,7 +133,7 @@ OPTIONS
+
If the last path components in the working tree's path is unique among
working trees, it can be used to identify worktrees. For example if
-you only have to working trees at "/abc/def/ghi" and "/abc/def/ggg",
+you only have two working trees, at "/abc/def/ghi" and "/abc/def/ggg",
then "ghi" or "def/ghi" is enough to point to the former working tree.
DETAILS
--
2.9.3
^ permalink raw reply related
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Jeff King @ 2016-11-12 7:06 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Junio C Hamano, git
In-Reply-To: <1478931094.12153.3.camel@kaarsemaker.net>
On Sat, Nov 12, 2016 at 07:11:34AM +0100, Dennis Kaarsemaker wrote:
> > * If normal "diff" that follows symlinks by default has an option
> > to disable it, then it is OK to also add --no-follow-symlinks
> > that is only valid in the --no-index mode, so that we can mimick
> > it better (I do not think this is the case, though).
>
> It does not, so no new option.
There is "--no-dereference", but it is not that helpful:
$ echo one >1
$ echo two >2
$ ln -s 1 sl1
$ ln -s 2 sl2
$ diff --no-dereference sl1 sl2
Symbolic links sl1 and sl2 differ
$ diff --no-dereference sl1 2
File sl1 is a symbolic link while file 2 is a regular file
So it does do the thing we are talking about here, but diff's handling
of non-file content is not nearly as useful as git's.
That said, I have no problem with proceeding without it and waiting
until somebody actually shows up who really _wants_ --no-dereference. I
won't be surprised if that never happens.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Dennis Kaarsemaker @ 2016-11-12 6:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com>
On Fri, 2016-11-11 at 13:27 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>
> > No tests or documentation updates yet, and I'm not sure whether
> > --follow-symlinks in other modes than --no-index should be supported, ignored
> > (as it is now) or cause an error, but I'm leaning towards the third option.
>
>
> My knee-jerk reaction is:
>
> * The --no-index mode should default to your --follow-symlinks
> behaviour, without any option to turn it on or off.
ok.
> * If normal "diff" that follows symlinks by default has an option
> to disable it, then it is OK to also add --no-follow-symlinks
> that is only valid in the --no-index mode, so that we can mimick
> it better (I do not think this is the case, though).
It does not, so no new option.
> * Other modes should not follow symbolic links ever, no need for a
> new option.
Makes sense.
> In any case, I'd advise you not to reroll this too quickly and
> frequently until the end of this cycle. During a feature freeze, I
> won't take new topics in 'pu' as that would add more things I need
> to worry about, and if you reroll in too quick succession, it will
> become harder to identify the latest set and queue after the
> release.
I'm in no hurry, so I'll sit on this until v2.11 is done.
D.
^ permalink raw reply
* [PATCH 11/11] worktree remove: new command
From: Nguyễn Thái Ngọc Duy @ 2016-11-12 2:23 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <20161112022337.13317-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-worktree.txt | 21 +++++----
builtin/worktree.c | 78 ++++++++++++++++++++++++++++++++++
contrib/completion/git-completion.bash | 5 ++-
t/t2028-worktree-move.sh | 26 ++++++++++++
4 files changed, 120 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 81f4fee..df0d551 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -14,6 +14,7 @@ SYNOPSIS
'git worktree lock' [--reason <string>] <worktree>
'git worktree move' <worktree> <new-path>
'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree remove' [--force] <worktree>
'git worktree unlock' <worktree>
DESCRIPTION
@@ -81,6 +82,13 @@ prune::
Prune working tree information in $GIT_DIR/worktrees.
+remove::
+
+Remove a working tree. Only clean working trees (no untracked files
+and no modification in tracked files) can be removed. Unclean working
+trees can be removed with `--force`. The main working tree cannot be
+removed.
+
unlock::
Unlock a working tree, allowing it to be pruned, moved or deleted.
@@ -90,9 +98,10 @@ OPTIONS
-f::
--force::
- By default, `add` refuses to create a new working tree when `<branch>`
- is already checked out by another working tree. This option overrides
- that safeguard.
+ By default, `add` refuses to create a new working tree when
+ `<branch>` is already checked out by another working tree and
+ `remove` refuses to remove an unclean working tree. This option
+ overrides that safeguard.
-b <new-branch>::
-B <new-branch>::
@@ -253,12 +262,6 @@ Multiple checkout in general is still experimental, and the support
for submodules is incomplete. It is NOT recommended to make multiple
checkouts of a superproject.
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
- warn if the working tree is dirty)
-
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 11be345..60a6199 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {
N_("git worktree lock [<options>] <path>"),
N_("git worktree move <worktree> <new-path>"),
N_("git worktree prune [<options>]"),
+ N_("git worktree remove [<options>] <worktree>"),
N_("git worktree unlock <path>"),
NULL
};
@@ -621,6 +622,81 @@ static int move_worktree(int ac, const char **av, const char *prefix)
return update_worktree_location(wt, dst.buf);
}
+static int remove_worktree(int ac, const char **av, const char *prefix)
+{
+ int force = 0;
+ struct option options[] = {
+ OPT_BOOL(0, "force", &force,
+ N_("force removing even if the worktree is dirty")),
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf sb = STRBUF_INIT;
+ const char *reason;
+ int ret = 0;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac != 1)
+ usage_with_options(worktree_usage, options);
+
+ worktrees = get_worktrees();
+ wt = find_worktree(worktrees, prefix, av[0]);
+ if (!wt)
+ die(_("'%s' is not a working directory"), av[0]);
+ if (is_main_worktree(wt))
+ die(_("'%s' is a main working directory"), av[0]);
+ if ((reason = is_worktree_locked(wt))) {
+ if (*reason)
+ die(_("already locked, reason: %s"), reason);
+ die(_("already locked, no reason"));
+ }
+ if (validate_worktree(wt, 0))
+ return -1;
+
+ if (!force) {
+ struct argv_array child_env = ARGV_ARRAY_INIT;
+ struct child_process cp;
+ char buf[1];
+
+ argv_array_pushf(&child_env, "%s=%s/.git",
+ GIT_DIR_ENVIRONMENT, wt->path);
+ argv_array_pushf(&child_env, "%s=%s",
+ GIT_WORK_TREE_ENVIRONMENT, wt->path);
+ memset(&cp, 0, sizeof(cp));
+ argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
+ cp.env = child_env.argv;
+ cp.git_cmd = 1;
+ cp.dir = wt->path;
+ cp.out = -1;
+ ret = start_command(&cp);
+ if (ret)
+ die_errno(_("failed to run git-status on '%s', code %d"),
+ av[0], ret);
+ ret = xread(cp.out, buf, sizeof(buf));
+ if (ret)
+ die(_("'%s' is dirty, use --force to delete it"), av[0]);
+ close(cp.out);
+ ret = finish_command(&cp);
+ if (ret)
+ die_errno(_("failed to run git-status on '%s', code %d"),
+ av[0], ret);
+ }
+ strbuf_addstr(&sb, wt->path);
+ if (remove_dir_recursively(&sb, 0)) {
+ error_errno(_("failed to delete '%s'"), sb.buf);
+ ret = -1;
+ }
+ strbuf_reset(&sb);
+ strbuf_addstr(&sb, git_common_path("worktrees/%s", wt->id));
+ if (remove_dir_recursively(&sb, 0)) {
+ error_errno(_("failed to delete '%s'"), sb.buf);
+ ret = -1;
+ }
+ strbuf_release(&sb);
+ free_worktrees(worktrees);
+ return ret;
+}
+
int cmd_worktree(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -645,5 +721,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
return unlock_worktree(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "move"))
return move_worktree(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "remove"))
+ return remove_worktree(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
}
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 613e03b..f6855af 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2715,7 +2715,7 @@ _git_whatchanged ()
_git_worktree ()
{
- local subcommands="add list lock move prune unlock"
+ local subcommands="add list lock move prune remove unlock"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
@@ -2733,6 +2733,9 @@ _git_worktree ()
prune,--*)
__gitcomp "--dry-run --expire --verbose"
;;
+ remove,--*)
+ __gitcomp "--force"
+ ;;
*)
;;
esac
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 74070bd..084acc6 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -89,4 +89,30 @@ test_expect_success 'move main worktree' '
test_must_fail git worktree move . def
'
+test_expect_success 'remove main worktree' '
+ test_must_fail git worktree remove .
+'
+
+test_expect_success 'remove locked worktree' '
+ git worktree lock destination &&
+ test_must_fail git worktree remove destination &&
+ git worktree unlock destination
+'
+
+test_expect_success 'remove worktree with dirty tracked file' '
+ echo dirty >>destination/init.t &&
+ test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with untracked file' '
+ git -C destination checkout init.t &&
+ : >destination/untracked &&
+ test_must_fail git worktree remove destination
+'
+
+test_expect_success 'force remove worktree with untracked file' '
+ git worktree remove --force destination &&
+ test_path_is_missing destination
+'
+
test_done
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox