* Re: [RFC-PATCHv2] submodules: add a background story
From: Junio C Hamano @ 2017-02-14 21:56 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kb2jZ9fgct6gncDqmWFsbY4MRiboFXPvw7AMcU2KanyfQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> If we were to redesign the .gitmodules file, we might have it as
>
> [submodule "path"]
> url = git://example.org
> branch = .
> ...
>
> and the "path -> name/UID" mapping would be inside $GIT_DIR.
I am not sure how you are going to keep track of that mapping,
though. If .gitmodules file does not have a way to tell that what
used to be at "path" in its v1.0 is now at "htap" (instead the above
seems to assume there will just be an entry for [submodule "htap"]
in the newer version, without anything that links the old one with
the new one), how would the mapping inside $GIT_DIR know? Don't
forget that name was introduced as the identity because we cannot
assume that URL for a single project will never change.
I fully agree that our documentation and user education should
stress that names must be unique and immultable throughout the
history of a superproject, though.
^ permalink raw reply
* [PATCH 8/7] grep: treat revs the same for --untracked as for --no-index
From: Jeff King @ 2017-02-14 21:54 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <82212eaa-76d2-3357-8e06-5e4d56028c2e@google.com>
On Tue, Feb 14, 2017 at 10:19:56AM -0800, Jonathan Tan wrote:
> > I wasn't sure if we wanted to treat "untracked" in the same way.
> > Certainly we can catch the error here for the seen_dashdash case, but is
> > it also the case that:
> >
> > echo content >master
> > git grep --untracked pattern master
> >
> > should treat "master" as a path?
>
> It is already the case that it cannot be treated as a rev:
>
> $ git grep --untracked pattern master
> fatal: --no-index or --untracked cannot be used with revs.
>
> So I think it would be better if it was treated as a path, for consistency
> with --no-index. I'm OK either way, though.
Right, it's always been disallowed. But the early detection changes a
few user-visible behaviors like the exact error message, and how
disambiguation works (see below). I think the arguments for making that
change for --no-index are stronger than for --untracked. But it probably
makes sense for --untracked, too.
Here's a patch on top of my series that lumps the two together again. It
_could_ be squashed into the earlier patch, but I think I prefer keeping
it separate.
-- >8 --
Subject: [PATCH] grep: treat revs the same for --untracked as for --no-index
git-grep has always disallowed grepping in a tree (as
opposed to the working directory) with both --untracked
and --no-index. But we traditionally did so by first
collecting the revs, and then complaining when any were
provided.
The --no-index option recently learned to detect revs
much earlier. This has two user-visible effects:
- we don't bother to resolve revision names at all. So
when there's a rev/path ambiguity, we always choose to
treat it as a path.
- likewise, when you do specify a revision without "--",
the error you get is "no such path" and not "--untracked
cannot be used with revs".
The rationale for doing this with --no-index is that it is
meant to be used outside a repository, and so parsing revs
at all does not make sense.
This patch gives --untracked the same treatment. While it
_is_ meant to be used in a repository, it is explicitly
about grepping the non-repository contents. Telling the user
"we found a rev, but you are not allowed to use revs" is
not really helpful compared to "we treated your argument as
a path, and could not find it".
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 10 +++++-----
t/t7810-grep.sh | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 1454bef49..9304c33e7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -967,6 +967,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
int dummy;
int use_index = 1;
int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
+ int allow_revs;
struct option options[] = {
OPT_BOOL(0, "cached", &cached,
@@ -1165,6 +1166,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
* to it must resolve as a rev. If not, then we stop at the first
* non-rev and assume everything else is a path.
*/
+ allow_revs = use_index && !untracked;
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
unsigned char sha1[20];
@@ -1176,9 +1178,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
break;
}
- if (!use_index) {
+ if (!allow_revs) {
if (seen_dashdash)
- die(_("--no-index cannot be used with revs"));
+ die(_("--no-index or --untracked cannot be used with revs"));
break;
}
@@ -1201,7 +1203,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
- verify_filename(prefix, argv[j], j == i && use_index);
+ verify_filename(prefix, argv[j], j == i && allow_revs);
}
parse_pathspec(&pathspec, 0,
@@ -1273,8 +1275,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!use_index || untracked) {
int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
- if (list.nr)
- die(_("--no-index or --untracked cannot be used with revs."));
hit = grep_directory(&opt, &pathspec, use_exclude, use_index);
} else if (0 <= opt_exclude) {
die(_("--[no-]exclude-standard cannot be used for tracked contents."));
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 0ff9f6cae..cee42097b 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1032,7 +1032,7 @@ test_expect_success 'grep --no-index pattern -- path' '
test_expect_success 'grep --no-index complains of revs' '
test_must_fail git grep --no-index o master -- 2>err &&
- test_i18ngrep "no-index cannot be used with revs" err
+ test_i18ngrep "cannot be used with revs" err
'
test_expect_success 'grep --no-index prefers paths to revs' '
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* Re: [RFC-PATCHv2] submodules: add a background story
From: Stefan Beller @ 2017-02-14 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqo9yblz33.fsf@gitster.mtv.corp.google.com>
Sorry for dropping the ball here, I was stressed out a bit.
On Thu, Feb 9, 2017 at 3:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Do we need
>>
>> * an opinionated way to check for a specific state of a submodule
>> * (submodule helper to be plumbing?)
>> * expose the design mistake of having the (name->path) mapping inside the
>> working tree, i.e. never remove a name from the submodule config even when
>> the submodule doesn't exist any more.
>
> I am not sure about the last item.
>
> Are you talking about a case where submodule comes and goes (think:
> "git checkout v1.0" that would make submodules added since that
> version disappar)? .gitmodules that is checked out would not have
> any entry, but .git/config needs to record the end-user preference
> for the module, so that the user can do "git checkout -" to come
> back, no?
That is perfectly legit and I agree that is good design.
> IOW .git/config that mentions all the submodule the user
> ever showed interests in is not a design mistake, so you must be
> talking about something else, but I do not know what it is.
I mean that we
(1) have a gitmodules file tracked in git that includes the name.
The "tracking some information inside the version control to
help the very version control system" is also not bad. The bad part
is that the name *must not be changed* and
* we do not tell people about it in the docs
* we happily make commits that change the name of a submodule
(2) name the submodule by path be default
See
https://public-inbox.org/git/7e54658a-dcb2-64a7-3c67-0c4fa221b2fb@gmail.com/
> Oh, I see. You did not just rename the path, but also the name
> in the .gitmodules?
I wasn't even aware that the submodule name was something different from
the path because the name is by default set to be the path to it.
You could blame this specific instance on the user, but I rather blame it on Git
as such questions come up once in a while on the mailing list.
If we were to redesign the .gitmodules file, we might have it as
[submodule "path"]
url = git://example.org
branch = .
...
and the "path -> name/UID" mapping would be inside $GIT_DIR.
>
> Are they both in section (1)? I think the former (concepts) belongs
> to section 7 and the latter (file formats) belongs to section 5.
oops. Will fix.
>
>> diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
>> new file mode 100644
>> index 0000000000..3369d55ae9
>> --- /dev/null
>> +++ b/Documentation/gitsubmodules.txt
>> @@ -0,0 +1,194 @@
>> +gitsubmodules(7)
>> +================
>> +
>> +NAME
>> +----
>> +gitsubmodules - information about submodules
>> +
>> +SYNOPSIS
>> +--------
>> +$GIT_DIR/config, .gitmodules
>> +
>> +------------------
>> +git submodule
>> +------------------
>> +
>> +DESCRIPTION
>> +-----------
>> +
>> +A submodule allows you to keep another Git repository in a subdirectory
>> +...
>> +When cloning or pulling a repository containing submodules however,
>> +the submodules will not be checked out by default; You need to instruct
>> +'clone' to recurse into submodules. The 'init' and 'update' subcommands
>
> I think this is not "You need to", but rather "You can, if you want
> to have each and every submodules."
ok. In this man page for submodules I assumed an implicit
"[if you want these submodules to be there, then] you have to/need to ...
But I'll tone it down as it doesn't carry internal assumptions.
>> +
>> +** When you want to use a (third party) library tied to a specific version.
>> + Using submodules for a library allows you to have a clean history for
>> + your own project and only updating the library in the submodule when needed.
>
> I somehow do not see this as decoupling; it is keeping what is
> originally separate separate, isn't it?
ok I'll reword that to say keeping separate things separate.
>
>> +** In its current form Git scales up poorly for very large repositories that
>> + change a lot, as the history grows very large. For that you may want to look
>> + at shallow clone, sparse checkout or git-lfs.
>> + However you can also use submodules to e.g. hold large binary assets
>> + and these repositories are then shallowly cloned such that you do not
>> + have a large history locally.
>
> In other words, a better way to list these may be
>
> 1. using another project that stands on its own.
>
> 2. artificially split a (logically single) project into multiple
> repositories and tying them back together.
>
> The access control and performance reasons are subclasses of 2.
> IOW, if Git had per-path ACL and infinite scaling, you wouldn't be
> splitting your project into submodules for 2. You would still want
> to use somebody else's project by binding it as a subproject, instead
> of merging its history into yours.
Looking at the big picture with a logical view is better indeed.
>
>> +When working with submodules, you can think of them as in a state machine.
>> +So each submodule can be in a different state, the following indicators are used:
>> +
>> +* the existence of the setting of 'submodule.<name>.url' in the
>> + superprojects configuration
>> +* the existence of the submodules working tree within the
>> + working tree of the superproject
>> +* the existence of the submodules git directory within the superprojects
>> + git directory at $GIT_DIR/modules/<name> or within the submodules working
>> + tree
>> +
>> + State URL config working tree git dir
>> + -----------------------------------------------------
>> + uninitialized no no no
>> + initialized yes no no
>> + populated yes yes yes
>> + depopulated yes no yes
>> + deinitialized no no yes
>> + uninteresting no yes yes
>> +
>> + invalid no yes no
>> + invalid yes yes no
>
> I do not have strong opinions on these labels; are submodule folks
> happy with the above vocabulary?
Brandon suggested (in)active instead of (un)initialized, which is better as
it decouples the current process from the actual states. Once we reintroduce
[1], then the user would not need to run "init" (whether it is 'git
submodule init'
or implicit as e.g. 'git submodule update --init') any more, but the selection
of active submodules would be done via config.
[1] https://public-inbox.org/git/20161110203428.30512-35-sbeller@google.com/
>
> "uninteresting" is not explained in the below?
will fix.
>
>> ...
>> +SEE ALSO
>> +--------
>> +linkgit:git-submodule[1], linkgit:gitmodules[1].
>
> Ditto.
^ permalink raw reply
* Re: [PATCH v4 4/7] stash: introduce new format create
From: Thomas Gummerer @ 2017-02-14 21:40 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A . Holm, Jakub Narębski
In-Reply-To: <vpqefz0ohub.fsf@anie.imag.fr>
On 02/14, Matthieu Moy wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > create_stash () {
> > - stash_msg="$1"
> > - untracked="$2"
> > + stash_msg=
> > + untracked=
> > + new_style=
> > + while test $# != 0
> > + do
> > + case "$1" in
> > + -m|--message)
> > + shift
> > + test -z ${1+x} && usage
> > + stash_msg="$1"
> > + new_style=t
> > + ;;
> > + -u|--include-untracked)
> > + shift
> > + test -z ${1+x} && usage
> > + untracked="$1"
> > + new_style=t
> > + ;;
> > + *)
> > + if test -n "$new_style"
> > + then
> > + echo "invalid argument"
> > + option="$1"
> > + # TRANSLATORS: $option is an invalid option, like
> > + # `--blah-blah'. The 7 spaces at the beginning of the
> > + # second line correspond to "error: ". So you should line
> > + # up the second line with however many characters the
> > + # translation of "error: " takes in your language. E.g. in
> > + # English this is:
> > + #
> > + # $ git stash save --blah-blah 2>&1 | head -n 2
> > + # error: unknown option for 'stash save': --blah-blah
> > + # To provide a message, use git stash save -- '--blah-blah'
> > + eval_gettextln "error: unknown option for 'stash create': \$option"
>
> The TRANSLATORS: hint seems a typoed cut-and-paste from somewhere else.
> There are no 7 spaces in this message.
>
> Actually, if I read the code correctly, $option is not even necessarily
> an option as you're matching *. Perhaps you meant something like
>
> -*)
> option="$1"
> # TRANSLATORS: $option is an invalid option, like
> # `--blah-blah'. The 7 spaces at the beginning of the
> # second line correspond to "error: ". So you should line
> # up the second line with however many characters the
> # translation of "error: " takes in your language. E.g. in
> # English this is:
> #
> # $ git stash save --blah-blah 2>&1 | head -n 2
> # error: unknown option for 'stash save': --blah-blah
> # To provide a message, use git stash save -- '--blah-blah'
> eval_gettextln "error: unknown option for 'stash save': \$option
> To provide a message, use git stash save -- '\$option'"
> usage
> ;;
> *)
> if test -n "$new_style"
> then
> arg="$1"
> eval_gettextln "error: invalid argument for 'stash create': \$arg"
> usage
> fi
> break
> ;;
>
> (untested)
>
> Also, you may want to guard against
>
> git stash create "some message" -m "some other message"
>
> since you are already rejecting
>
> git stash create -m "some message" "some other message"
>
> ? Or perhaps apply "last one wins" for both "-m message" and
> "message"-without-dash-m.
Thanks, you're right I was missing some cases here. As I just
indicated in [1] however I think we can just make this an internal
interface, instead of user interface facing, so I think we'll need
less error checking.
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
[1]: http://public-inbox.org/git/20170214213038.GE652@hank/
^ permalink raw reply
* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-14 21:36 UTC (permalink / raw)
To: Matthieu Moy
Cc: Jeff King, git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <vpq60kdjl63.fsf@anie.imag.fr>
On 02/14, Matthieu Moy wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > I'm almost convinced of special casing "-p". (Maybe I'm easy to
> > convince as well, because it would be convenient ;) ) However it's a
> > bit weird that now "git stash -p file" would work, but "git stash -m
> > message" wouldn't. Maybe we should do it the other way around, and
> > only special case "-q", and see if there is an non option argument
> > after that? From a glance at the options that's the only one where
> > "git stash -<option> <verb>" could make sense to the user.
>
> Special-casing the allowed cases makes it easier to change the behavior
> in the future if needed: it's easy to add special cases and it doesn't
> break backward compatibility.
>
> So, if we don't have a good reason to do otherwise, I'd rather stay on
> the future-proof side.
Ok, after reading the rest of the messages in the thread I'm convinced
we should just special case "-p" for now.
It's by far my most used stash command as well, after using git stash
without any arguments.
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v2 2/2] completion: checkout: complete paths when ref given
From: Junio C Hamano @ 2017-02-14 21:31 UTC (permalink / raw)
To: cornelius.weig; +Cc: git, szeder.dev, bitte.keine.werbung.einwerfen, j6t
In-Reply-To: <20170214212404.31469-2-cornelius.weig@tngtech.com>
cornelius.weig@tngtech.com writes:
> From: Cornelius Weig <cornelius.weig@tngtech.com>
>
> Git-checkout completes words starting with '--' as options and other
> words as refs. Even after specifying a ref, further words not starting
> with '--' are completed as refs, which is invalid for git-checkout.
>
> This commit ensures that after specifying a ref, further non-option
> words are completed as paths. Four cases are considered:
>
> - If the word contains a ':', do not treat it as reference and use
> regular revlist completion.
> - If no ref is found on the command line, complete non-options as refs
> as before.
> - If the ref is HEAD or @, complete only with modified files because
> checking out unmodified files is a noop.
> This case also applies if no ref is given, but '--' is present.
Please at least do not do this one; a completion that is or pretends
to be more clever than the end users will confuse them at best and
annoy them. Maybe the user does not recall if she touched the path
or not, and just trying to be extra sure that it matches HEAD or
index by doing "git checkout [HEAD] path<TAB>". Leave the "make it
a noop" to Git, but just allow her do so.
I personally feel that "git checkout <anything>... foo<TAB>" should
just fall back to the normal "path on the filesystem" without any
cleverness, instead of opening a tree object or peek into the index.
^ permalink raw reply
* Re: [PATCH v3 4/5] stash: introduce new format create
From: Thomas Gummerer @ 2017-02-14 21:30 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170213230532.sr7lpl26mcfa4gfc@sigill.intra.peff.net>
On 02/13, Jeff King wrote:
> On Mon, Feb 13, 2017 at 04:57:34PM -0500, Jeff King wrote:
>
> > Yeah, I think your patch is actually fixing that case. But your search
> > is only part of the story. You found somebody using "-m" explicitly, but
> > what about somebody blindly calling:
> >
> > git stash create $*
> >
> > That's now surprising to somebody who puts "-m" in their message.
> >
> > > I *think* this regression is acceptable, but I'm happy to introduce
> > > another verb if people think otherwise.
> >
> > Despite what I wrote above, I'm still inclined to say that this isn't an
> > important regression. I'd be surprised if "stash create" is used
> > independently much at all.
>
> Just thinking on this more...do we really care about "fixing" the
> interface of "stash create"? This is really just about refactoring what
> underlies the new "push", right?
>
> So we could just do:
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 6d629fc43..ee37db135 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -711,7 +711,7 @@ clear)
> ;;
> create)
> shift
> - create_stash "$@" && echo "$w_commit"
> + create_stash -m "$*" && echo "$w_commit"
> ;;
> store)
> shift
>
> on top of your patch and keep the external interface the same.
>
> It might be nice to clean up the interface for "create" to match other
> ones, but from this discussion I think it is mostly a historical wart
> for scripting, and we are OK to just leave its slightly-broken interface
> in place forever.
Yeah tbh I don't personally care too much about modernizing the
interface to git stash create. What you have above makes a lot of
sense to me.
I also just noticed that git stash create -m message is not the worst
regression I introduced when trying to modernize this. In that case
only the -m would go missing, but that's probably not the end of the
world. The worse thing to do would be something like
git stash create -u untracked, where the intended message was "-u
untracked", but instead there is no message, but all untracked files
are now included in the stash as well.
In that light what you have above makes even more sense to me.
Thanks!
> I could go either way.
>
> -Peff
--
Thomas
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Christian Couder @ 2017-02-14 21:29 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git, Maxim Kuvyrkov
In-Reply-To: <20170214195513.7zae6x22advkrms6@sigill.intra.peff.net>
On Tue, Feb 14, 2017 at 8:55 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > This fixes the problem, but I think we can simplify it quite a bit by
>> > using resolve_refdup(). Here's the patch series I ended up with:
>> >
>> > [1/3]: show-branch: drop head_len variable
>> > [2/3]: show-branch: store resolved head in heap buffer
>> > [3/3]: show-branch: use skip_prefix to drop magic numbers
Yeah, I noticed there were a number of things that could be improved
in the area, but I didn't want to spend too much time on this, so
thanks for this series.
>> Yes, the whole thing is my fault ;-) and I agree with what these
>> patches do.
>>
>> The second one lacks free(head) but I think that is OK; it is
>> something we allocate in cmd_*() and use pretty much thruout the
>> rest of the program.
>
> Yes, I actually tested the whole thing under ASAN (which was necessary
> to notice the problem), which complained about the leak. I don't mind
> adding a free(head), but there are a bunch of similar "leaks" in that
> function, so I didn't bother.
Yeah, I didn't bother either.
> I notice Christian's patch added a few tests. I don't know if we'd want
> to squash them in (I didn't mean to override his patch at all; I was
> about to send mine out when I noticed his, and I wondered if we wanted
> to combine the two efforts).
I think it would be nice to have at least one test. Feel free to
squash mine if you want.
^ permalink raw reply
* [PATCH v2 1/2] completion: extract utility to complete paths from tree-ish
From: cornelius.weig @ 2017-02-14 21:24 UTC (permalink / raw)
To: git; +Cc: Cornelius Weig, szeder.dev, bitte.keine.werbung.einwerfen, j6t
In-Reply-To: <4f8a0aaa-4ce1-d4a6-d2e1-28aac7209c90@tngtech.com>
From: Cornelius Weig <cornelius.weig@tngtech.com>
The function __git_complete_revlist_file understands how to complete a
path such as 'topic:ref<TAB>'. In that case, the revision (topic) and
the path component (ref) are both part of the same word. However,
some cases require that the revision is specified elsewhere than the
current word for completion, such as 'git checkout topic ref<TAB>'.
In order to allow callers to specify the revision, extract a utility
function to complete paths from a tree-ish object. The utility will be
used later to implement path completion for git-checkout.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
contrib/completion/git-completion.bash | 73 +++++++++++++++++++---------------
1 file changed, 41 insertions(+), 32 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c7..4ab119d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -442,6 +442,46 @@ __git_compute_merge_strategies ()
__git_merge_strategies=$(__git_list_merge_strategies)
}
+# __git_complete_tree_file requires 2 argument:
+# 1: the the tree-like to look at for completion
+# 2: the path component to complete
+__git_complete_tree_file ()
+{
+ local pfx ls ref="$1" cur_="$2"
+ case "$cur_" in
+ ?*/*)
+ pfx="${cur_%/*}"
+ cur_="${cur_##*/}"
+ ls="$ref:$pfx"
+ pfx="$pfx/"
+ ;;
+ *)
+ ls="$ref"
+ ;;
+ esac
+
+ case "$COMP_WORDBREAKS" in
+ *:*) : great ;;
+ *) pfx="$ref:$pfx" ;;
+ esac
+
+ __gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
+ | sed '/^100... blob /{
+ s,^.* ,,
+ s,$, ,
+ }
+ /^120000 blob /{
+ s,^.* ,,
+ s,$, ,
+ }
+ /^040000 tree /{
+ s,^.* ,,
+ s,$,/,
+ }
+ s/^.* //')" \
+ "$pfx" "$cur_" ""
+}
+
__git_complete_revlist_file ()
{
local pfx ls ref cur_="$cur"
@@ -452,38 +492,7 @@ __git_complete_revlist_file ()
?*:*)
ref="${cur_%%:*}"
cur_="${cur_#*:}"
- case "$cur_" in
- ?*/*)
- pfx="${cur_%/*}"
- cur_="${cur_##*/}"
- ls="$ref:$pfx"
- pfx="$pfx/"
- ;;
- *)
- ls="$ref"
- ;;
- esac
-
- case "$COMP_WORDBREAKS" in
- *:*) : great ;;
- *) pfx="$ref:$pfx" ;;
- esac
-
- __gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
- | sed '/^100... blob /{
- s,^.* ,,
- s,$, ,
- }
- /^120000 blob /{
- s,^.* ,,
- s,$, ,
- }
- /^040000 tree /{
- s,^.* ,,
- s,$,/,
- }
- s/^.* //')" \
- "$pfx" "$cur_" ""
+ __git_complete_tree_file "$ref" "$cur_"
;;
*...*)
pfx="${cur_%...*}..."
--
2.10.2
^ permalink raw reply related
* [PATCH v2 2/2] completion: checkout: complete paths when ref given
From: cornelius.weig @ 2017-02-14 21:24 UTC (permalink / raw)
To: git; +Cc: Cornelius Weig, szeder.dev, bitte.keine.werbung.einwerfen, j6t
In-Reply-To: <20170214212404.31469-1-cornelius.weig@tngtech.com>
From: Cornelius Weig <cornelius.weig@tngtech.com>
Git-checkout completes words starting with '--' as options and other
words as refs. Even after specifying a ref, further words not starting
with '--' are completed as refs, which is invalid for git-checkout.
This commit ensures that after specifying a ref, further non-option
words are completed as paths. Four cases are considered:
- If the word contains a ':', do not treat it as reference and use
regular revlist completion.
- If no ref is found on the command line, complete non-options as refs
as before.
- If the ref is HEAD or @, complete only with modified files because
checking out unmodified files is a noop.
This case also applies if no ref is given, but '--' is present.
- If a ref other than HEAD or @ is found, offer only valid paths from
that revision.
Note that one corner-case is not covered by the current implementation:
if a refname contains a ':' and is followed by '--' the completion would
not recognize the valid refname.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
contrib/completion/git-completion.bash | 39 +++++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4ab119d..df46f62 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1068,7 +1068,7 @@ _git_bundle ()
_git_checkout ()
{
- __git_has_doubledash && return
+ local i c=2 ref="" seen_double_dash=""
case "$cur" in
--conflict=*)
@@ -1081,13 +1081,36 @@ _git_checkout ()
"
;;
*)
- # check if --track, --no-track, or --no-guess was specified
- # if so, disable DWIM mode
- local flags="--track --no-track --no-guess" track=1
- if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
- track=''
- fi
- __gitcomp_nl "$(__git_refs '' $track)"
+ while [ $c -lt $cword ]; do
+ i="${words[c]}"
+ case "$i" in
+ --) seen_double_dash=1 ;;
+ -*|?*:*) ;;
+ *) ref="$i"; break ;;
+ esac
+ ((c++))
+ done
+
+ case "$ref,$seen_double_dash,$cur" in
+ ,,*:*)
+ __git_complete_revlist_file
+ ;;
+ ,,*)
+ # check for --track, --no-track, or --no-guess
+ # if so, disable DWIM mode
+ local flags="--track --no-track --no-guess" track=1
+ if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
+ track=''
+ fi
+ __gitcomp_nl "$(__git_refs '' $track)"
+ ;;
+ ,1,*|@,*|HEAD,*)
+ __git_complete_index_file "--modified"
+ ;;
+ *)
+ __git_complete_tree_file "$ref" "$cur"
+ ;;
+ esac
;;
esac
}
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] completion: complete modified files for checkout with '--'
From: Cornelius Weig @ 2017-02-14 21:13 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git mailing list, j6t, Richard Wagner
In-Reply-To: <CAM0VKj=d+hAiF6_8TLuJfccNiPtHyg9F6zESA8SuTEeaLsrw4Q@mail.gmail.com>
On 02/14/2017 01:50 AM, SZEDER Gábor wrote:
> On Tue, Feb 14, 2017 at 12:33 AM, <cornelius.weig@tngtech.com> wrote:
>> From: Cornelius Weig <cornelius.weig@tngtech.com>
>>
>> The command line completion for git-checkout bails out when seeing '--'
>> as an isolated argument. For git-checkout this signifies the start of a
>> list of files which are to be checked out. Checkout of files makes only
>> sense for modified files,
>
> No, there is e.g. 'git checkout that-branch this-path', too.
Very true. Thanks for prodding me to this palpable oversight.
My error was to aim for a small improvement. I think the correct
approach is to improve the overall completion of git-checkout. IMHO it
is a completion bug that after giving a ref, completion will still offer
refs, e.g.
$ git checkout HEAD <TAB><TAB> --> list of refs
As far as I can see, giving two refs to checkout is always an error. The
correct behavior in the example above would be to offer paths instead.
I'll follow up with an improved version which considers these cases.
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Junio C Hamano @ 2017-02-14 21:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git-for-windows, git
In-Reply-To: <alpine.DEB.2.20.1702142150220.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Mon, 13 Feb 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > That is why I taught the Git for Windows CI job that tests the four
>> > upstream Git integration branches to *also* bisect test breakages and
>> > then upload comments to the identified commit on GitHub
>>
>> Good. I do not think it is useful to try 'pu' as an aggregate and
>> expect it to always build and work [*1*], but your "bisect and
>> pinpoint" approach makes it useful to identify individual topic that
>> brings in a breakage.
>
> Sadly the many different merge bases[*1*] between `next` and `pu` (which
> are the obvious good/bad points for bisecting automatically) bring my
> build agents to its knees. I may have to disable the bisecting feature as
> a consequence.
Probably a less resource intensive approach is to find the tips of
the topics not in 'next' but in 'pu' and test them. That would give
you which topic(s) are problematic, which is a better starting point
than "Oh, 'pu' does not build". After identifying which branch is
problematic, bisection of individual topic would be of more manageable
size.
$ git log --first-parent --oneline 'pu^{/^### match next}..pu'
will you the merges of topics left outside 'next'. I often reorder
to make the ones that look more OK than others closer to the bottom,
and if the breakages caused by them are caught earlier than they hit
'next', that would be ideal.
This is one of these times I wish "git bisect --first-parent" were
available. The above "log" gives me 27 merges right now, which
should be bisectable within 5 rounds to identify a single broken
topic (if there is only one breakage, that is).
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Johannes Schindelin @ 2017-02-14 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git-for-windows, git
In-Reply-To: <xmqq60kdbqmy.fsf@gitster.mtv.corp.google.com>
Hi,
On Mon, 13 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > That is why I taught the Git for Windows CI job that tests the four
> > upstream Git integration branches to *also* bisect test breakages and
> > then upload comments to the identified commit on GitHub
>
> Good. I do not think it is useful to try 'pu' as an aggregate and
> expect it to always build and work [*1*], but your "bisect and
> pinpoint" approach makes it useful to identify individual topic that
> brings in a breakage.
Sadly the many different merge bases[*1*] between `next` and `pu` (which
are the obvious good/bad points for bisecting automatically) bring my
build agents to its knees. I may have to disable the bisecting feature as
a consequence.
Ciao,
Johannes
Footnote *1*: There are currently 21, some of which stemming back from a
year ago. For bisecting, they all have to be tested individually, putting
a major dent into bisect's otherwise speedy process.
^ permalink raw reply
* Re: missing handling of "No newline at end of file" in git am
From: Olaf Hering @ 2017-02-14 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh93w8q0r.fsf@gitster.mtv.corp.google.com>
Am Tue, 14 Feb 2017 12:40:36 -0800
schrieb Junio C Hamano <gitster@pobox.com>:
> Olaf Hering <olaf@aepfle.de> writes:
>
> > How is git send-email and git am supposed to handle a text file
> > which lacks a newline at the very end? This is about git 2.11.0.
>
> I think this has always worked, though.
For me it complains in line 721, which is the problematic one.
I try to apply from mutt via (cd /some/dir && git am), but that
probably does not make a difference.
How would I debug it?
Olaf
^ permalink raw reply
* Re: missing handling of "No newline at end of file" in git am
From: Jeff King @ 2017-02-14 20:47 UTC (permalink / raw)
To: Olaf Hering; +Cc: git
In-Reply-To: <20170214201104.GA26407@aepfle.de>
On Tue, Feb 14, 2017 at 09:11:04PM +0100, Olaf Hering wrote:
> How is git send-email and git am supposed to handle a text file which
> lacks a newline at the very end? This is about git 2.11.0.
That workflow should handle this case, and the resulting applied patch
should not have a newline.
> Right now the patch in an email generated with 'git send-email' ends
> with '\ No newline at end of file', which 'git am' can not handle. To
> me it looks like whatever variant of "diff" is used does the right thing
> and indicates the lack of newline. Just the used variant of "patch" does
> not deal with it.
I can't reproduce here:
# new repo with nothing in it (the base commit is to have something to
# reset back to)
git init
git commit --allow-empty -m base
# our file with no trailing newline
printf foo >file
git add file
git commit -m no-newline
# now make a patch email; it should have the "\ No newline" bit at the
# end.
git format-patch -1
cat 0001-no-newline.patch
# and now reset back and try to apply it
git reset --hard HEAD^
git am 0001-no-newline.patch
# double check that it has no newline
xxd <file
I'm using format-patch instead of send-email, but that is the underlying
command that send-email is using. Is it possible that your patch is
getting munged during email transit in a way that destroy the "No
newline" message?
-Peff
^ permalink raw reply
* Re: missing handling of "No newline at end of file" in git am
From: Junio C Hamano @ 2017-02-14 20:40 UTC (permalink / raw)
To: Olaf Hering; +Cc: git
In-Reply-To: <20170214201104.GA26407@aepfle.de>
Olaf Hering <olaf@aepfle.de> writes:
> How is git send-email and git am supposed to handle a text file which
> lacks a newline at the very end? This is about git 2.11.0.
I think this has always worked, though.
$ cd /var/tmp/x
$ git init am-incomplete-line
$ cd am-incomplete-line/
$ echo one line >file
$ git add file
$ git commit -a -m initial
[master (root-commit) 27b4668] initial
1 file changed, 1 insertion(+)
create mode 100644 file
$ echo -n an incomplete line >>file
$ git diff file
diff --git a/file b/file
index e3c0674..f2ec9f0 100644
--- a/file
+++ b/file
@@ -1 +1,2 @@
one line
+an incomplete line
\ No newline at end of file
$ git commit -a -m 'incomplete second'
[master 57075ab] incomplete second
1 file changed, 1 insertion(+)
$ git format-patch -1
0001-incomplete-second.txt
$ cat 0001-incomplete-second.txt
From 57075ab402e2d3714ebc9e2e9d4efd8dbfd74d5a Mon Sep 17 00:00:00 2001
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 14 Feb 2017 12:35:50 -0800
Subject: [PATCH] incomplete second
---
file | 1 +
1 file changed, 1 insertion(+)
diff --git a/file b/file
index e3c0674..f2ec9f0 100644
--- a/file
+++ b/file
@@ -1 +1,2 @@
one line
+an incomplete line
\ No newline at end of file
--
2.12.0-rc1-235-g2fb706ef99
$ git checkout HEAD^
$ git am ./0001-incomplete-second.txt
Applying: incomplete second
$ git diff master
$ exit
^ permalink raw reply
* [PATCH 2/2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jeff King @ 2017-02-14 20:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Duy Nguyen, Stefan Beller
In-Reply-To: <20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net>
From: Jonathan Nieder <jrnieder@gmail.com>
To push from or fetch to the current repository, remote helpers need
to know what repository that is. Accordingly, Git sets the GIT_DIR
environment variable to the path to the current repository when
invoking remote helpers.
There is a special case it does not handle: "git ls-remote" and "git
archive --remote" can be run to inspect a remote repository without
being run from any local repository. GIT_DIR is not useful in this
scenario:
- if we are not in a repository, we don't need to set GIT_DIR to
override an existing GIT_DIR value from the environment. If GIT_DIR
is present then we would be in a repository if it were valid and
would have called die() if it weren't.
- not setting GIT_DIR may cause a helper to do the usual discovery
walk to find the repository. But we know we're not in one, or we
would have found it ourselves. So in the worst case it may expend
a little extra effort to try to find a repository and fail (for
example, remote-curl would do this to try to find repository-level
configuration).
So leave GIT_DIR unset in this case. This makes GIT_DIR easier to
understand for remote helper authors and makes transport code less of
a special case for repository discovery.
Noticed using b1ef400e (setup_git_env: avoid blind fall-back to
".git", 2016-10-20) from 'next':
$ cd /tmp
$ git ls-remote https://kernel.googlesource.com/pub/scm/git/git
fatal: BUG: setup_git_env called without repository
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I dropped this down to a single test instance, and used the nongit
helper to shorten it.
Possible patches on top:
- if we want to test this across more protocols, we can. I'm not sure
I see all that much value in it, given that we know the source of
the bug.
We probably _should_ have some kind of standard test-battery that
hits all protocols, or at the very least hits both dumb/smart http.
But waiting for that may be making the perfect the enemy of the
good. So I'm OK with doing it piece-wise for now if people really
feel we need to cover more protocols.
- Jonathan's original had some nice remote-ext tests, but they were
sufficiently complex that they should be spun into their own patch
with more explanation.
t/t5550-http-fetch-dumb.sh | 9 +++++++++
transport-helper.c | 5 +++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index aeb3a63f7..b69ece1d6 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -34,6 +34,15 @@ test_expect_success 'clone http repository' '
test_cmp file clone/file
'
+test_expect_success 'list refs from outside any repository' '
+ cat >expect <<-EOF &&
+ $(git rev-parse master) HEAD
+ $(git rev-parse master) refs/heads/master
+ EOF
+ nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'create password-protected repository' '
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
diff --git a/transport-helper.c b/transport-helper.c
index 91aed35eb..e4fd98238 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
helper->git_cmd = 0;
helper->silent_exec_failure = 1;
- argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
- get_git_dir());
+ if (have_git_dir())
+ argv_array_pushf(&helper->env_array, "%s=%s",
+ GIT_DIR_ENVIRONMENT, get_git_dir());
code = start_command(helper);
if (code < 0 && errno == ENOENT)
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* Re: [RFC PATCH] show decorations at the end of the line
From: Stephan Beyer @ 2017-02-14 20:36 UTC (permalink / raw)
To: Junio C Hamano, Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <xmqq1sv2fq6m.fsf@gitster.mtv.corp.google.com>
Hi,
On 02/13/2017 09:30 AM, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> I've signed off on this, because I think it's an "obvious" improvement,
>>> but I'm putting the "RFC" in the subject line because this is clearly a
>>> subjective thing.
>>
>> Side note: the one downside of showing the decorations at the end of
>> the line is that now they are obviously at the end of the line - and
>> thus likely to be more hidden by things like line truncation.
>
> Side note: I refrained from commenting on this patch because
> everybody knows that the what I would say anyway ;-) and I didn't
> want to speak first to discourage others from raising their opinion.
A further side note: the current behavior of
git log --oneline --decorate
is equivalent to
git log --pretty='format:%C(auto)%h%d %s'
and Linus' preferred version is equivalent to
git log --pretty='format:%C(auto)%h %s%d'
Most Git users I know have their own favorite version of git log
--pretty=format:... sometimes with --graph as an alias ("git lg" or "git
logk" (because its output reminds of gitk) or something).
I don't know what the main benefit of this patch would be, but if it
gets accepted, it should probably be mentioned somewhere that the old
behavior is easily accessible using the line mentioned above.
Cheers
Stephan
^ permalink raw reply
* [PATCH 1/2] remote: avoid reading $GIT_DIR config in non-repo
From: Jeff King @ 2017-02-14 20:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Duy Nguyen, Stefan Beller
In-Reply-To: <20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net>
The "git ls-remote" command can be run outside of a
repository, but needs to look up configured remotes. The
config code is smart enough to handle this case itself, but
we also check the historical "branches" and "remotes" paths
in $GIT_DIR. The git_path() function causes us to blindly
look at ".git/remotes", even if we know we aren't in a git
repository.
For now, this is just an unlikely bug (you probably don't
have such a file if you're not in a repository), but it will
become more obvious once we merge b1ef400ee (setup_git_env:
avoid blind fall-back to ".git", 2016-10-20):
[now]
$ git ls-remote
fatal: No remote configured to list refs from.
[with b1ef400ee]
$ git ls-remote
fatal: BUG: setup_git_env called without repository
We can fix this by skipping these sources entirely when
we're outside of a repository.
The test is a little more complex than the demonstration
above. Rather than detect the correct behavior by parsing
the error message, we can actually set up a case where the
remote name we give is a valid repository, but b1ef400ee
would cause us to die in the configuration step.
This test doesn't fail now, but it future-proofs us for the
b1ef400ee change.
Signed-off-by: Jeff King <peff@peff.net>
---
remote.c | 2 +-
t/t5512-ls-remote.sh | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/remote.c b/remote.c
index bf1bf2309..9f83fe2c4 100644
--- a/remote.c
+++ b/remote.c
@@ -693,7 +693,7 @@ static struct remote *remote_get_1(const char *name,
name = get_default(current_branch, &name_given);
ret = make_remote(name, 0);
- if (valid_remote_nick(name)) {
+ if (valid_remote_nick(name) && have_git_dir()) {
if (!valid_remote(ret))
read_remotes_file(ret);
if (!valid_remote(ret))
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 55fc83fc0..94fc9be9c 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -248,4 +248,13 @@ test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-complian
test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
'
+test_expect_success 'ls-remote works outside repository' '
+ # It is important for this repo to be inside the nongit
+ # area, as we want a repo name that does not include
+ # slashes (because those inhibit some of our configuration
+ # lookups).
+ nongit git init --bare dst.git &&
+ nongit git ls-remote dst.git
+'
+
test_done
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jeff King @ 2017-02-14 20:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Duy Nguyen, Stefan Beller
In-Reply-To: <xmqqtw7w8uay.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 14, 2017 at 11:08:05AM -0800, Junio C Hamano wrote:
> Thanks for prodding. I'm tempted to just rip out everything other
> than the 5-liner fix to transport.c and apply it, expecting that a
> follow-up patch with updated tests to come sometime not in too
> distant future.
I think we can at least include one basic test. Also, as it turns out
the problem I was seeing _wasn't_ the same one Jonathan fixed. There's
another bug. :)
So here's a patch series that fixes my bug, and then a cut-down version
of Jonathan's patch. I'd be happy to see more tests come on top. I don't
think there's a huge rush on getting any of this into master. There are
bugs in the existing code, but they're very hard to trigger in practice
(e.g., a non-repo which happens to have a bunch of repo-like files). It
only becomes an issue in 'next' when we die("BUG") to flush these cases
out.
[1/2]: remote: avoid reading $GIT_DIR config in non-repo
[2/2]: remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
remote.c | 2 +-
t/t5512-ls-remote.sh | 9 +++++++++
t/t5550-http-fetch-dumb.sh | 9 +++++++++
transport-helper.c | 5 +++--
4 files changed, 22 insertions(+), 3 deletions(-)
-Peff
^ permalink raw reply
* missing handling of "No newline at end of file" in git am
From: Olaf Hering @ 2017-02-14 20:11 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 441 bytes --]
How is git send-email and git am supposed to handle a text file which
lacks a newline at the very end? This is about git 2.11.0.
Right now the patch in an email generated with 'git send-email' ends
with '\ No newline at end of file', which 'git am' can not handle. To
me it looks like whatever variant of "diff" is used does the right thing
and indicates the lack of newline. Just the used variant of "patch" does
not deal with it.
Olaf
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Jeff King @ 2017-02-14 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Christian Couder, git, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <xmqqlgt88t0r.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This fixes the problem, but I think we can simplify it quite a bit by
> > using resolve_refdup(). Here's the patch series I ended up with:
> >
> > [1/3]: show-branch: drop head_len variable
> > [2/3]: show-branch: store resolved head in heap buffer
> > [3/3]: show-branch: use skip_prefix to drop magic numbers
> >
> > builtin/show-branch.c | 39 ++++++++++++---------------------------
> > 1 file changed, 12 insertions(+), 27 deletions(-)
>
> Yes, the whole thing is my fault ;-) and I agree with what these
> patches do.
>
> The second one lacks free(head) but I think that is OK; it is
> something we allocate in cmd_*() and use pretty much thruout the
> rest of the program.
Yes, I actually tested the whole thing under ASAN (which was necessary
to notice the problem), which complained about the leak. I don't mind
adding a free(head), but there are a bunch of similar "leaks" in that
function, so I didn't bother.
I notice Christian's patch added a few tests. I don't know if we'd want
to squash them in (I didn't mean to override his patch at all; I was
about to send mine out when I noticed his, and I wondered if we wanted
to combine the two efforts).
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers
From: Jeff King @ 2017-02-14 19:53 UTC (permalink / raw)
To: Pranit Bauva
Cc: Christian Couder, Git List, Junio C Hamano, Maxim Kuvyrkov,
Christian Couder
In-Reply-To: <CAFZEwPOYGRGd5PWSfLRd6vMs35TT1ZzUSfr79fwRA4VzVjAWXA@mail.gmail.com>
On Wed, Feb 15, 2017 at 12:23:52AM +0530, Pranit Bauva wrote:
> Did you purposely miss the one in line number 278 of
> builtin/show-branch.c because I think you only touched up the parts
> which were related to "refs/" but didn't explicitly mention it in the
> commit message?
>
> if (starts_with(pretty_str, "[PATCH] "))
> pretty_str += 8;
Not purposely. I was just fixing up the bits that I had noticed in the
earlier patches.
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index c03d3ec7c..19756595d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
> pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
> pretty_str = pretty.buf;
> }
> - if (starts_with(pretty_str, "[PATCH] "))
> - pretty_str += 8;
> + skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
Yeah, I agree this the same type of fix and is worth including. Thanks!
-Peff
^ permalink raw reply
* Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
From: Jeff King @ 2017-02-14 19:51 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Tan, git, gitster
In-Reply-To: <20170214185621.GC44208@google.com>
On Tue, Feb 14, 2017 at 10:56:21AM -0800, Brandon Williams wrote:
> On 02/14, Jeff King wrote:
> > - /* Check revs and then paths */
> > + /*
> > + * We have to find "--" in a separate pass, because its presence
> > + * influences how we will parse arguments that come before it.
> > + */
> > + for (i = 0; i < argc; i++) {
> > + if (!strcmp(argv[i], "--")) {
> > + seen_dashdash = 1;
> > + break;
> > + }
> > + }
>
> So this simply checks if "--" is an argument that was provided. This
> then allows grep to know ahead of time how to handle revs/paths
> preceding a "--" or in the absences of the "--". Seems sensible to me.
By the way, we have to check again later for "--" when parsing the revs
themselves. In theory you could set seen_dashdash to the offset of the
dashdash in the array, and do the iteration more like:
for (i = 0; i < dashdash_pos; i++)
handle_rev(argv[i]);
for (i = dashdash_pos + 1; i < argc; i++)
handle_path(argv[i]);
But our loops also handle the case where there is no "--" at all, and I
think that approach ends up convoluting the logic. I didn't go very far
in that direction before giving it up, though.
-Peff
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Junio C Hamano @ 2017-02-14 19:35 UTC (permalink / raw)
To: Jeff King; +Cc: Christian Couder, git, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This fixes the problem, but I think we can simplify it quite a bit by
> using resolve_refdup(). Here's the patch series I ended up with:
>
> [1/3]: show-branch: drop head_len variable
> [2/3]: show-branch: store resolved head in heap buffer
> [3/3]: show-branch: use skip_prefix to drop magic numbers
>
> builtin/show-branch.c | 39 ++++++++++++---------------------------
> 1 file changed, 12 insertions(+), 27 deletions(-)
Yes, the whole thing is my fault ;-) and I agree with what these
patches do.
The second one lacks free(head) but I think that is OK; it is
something we allocate in cmd_*() and use pretty much thruout the
rest of the program.
Thanks.
^ permalink raw reply
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