* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-01-30 21:03 UTC (permalink / raw)
To: Johannes Sixt; +Cc: René Scharfe, Git List, Junio C Hamano
In-Reply-To: <aa653d57-4a97-ac50-b20c-f94ed43a22fb@kdbg.org>
[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]
Hi Hannes,
On Mon, 30 Jan 2017, Johannes Sixt wrote:
> Am 30.01.2017 um 17:01 schrieb Johannes
> Schindelin:
> > On Sat, 28 Jan 2017, René Scharfe wrote:
> > > diff --git a/git-compat-util.h
> > > b/git-compat-util.h
> > > index 87237b092b..66cd466eea 100644
> > > --- a/git-compat-util.h
> > > +++ b/git-compat-util.h
> > > @@ -527,6 +527,16 @@ static inline int
> > > @@ ends_with(const char *str, const char
> > > @@ *suffix)
> > > return strip_suffix(str, suffix, &len);
> > > }
> > >
> > > +#define SWAP(a, b) do {
> > > \
> > > + void *_swap_a_ptr = &(a);
> > > \
> > > + void *_swap_b_ptr = &(b);
> > > \
> > > + unsigned char _swap_buffer[sizeof(a)];
> > > \
> > > + memcpy(_swap_buffer, _swap_a_ptr,
> > > sizeof(a)); \
> > > + memcpy(_swap_a_ptr, _swap_b_ptr,
> > > sizeof(a) + \
> > > + BUILD_ASSERT_OR_ZERO(sizeof(a)
> > > == sizeof(b))); \
> > > + memcpy(_swap_b_ptr, _swap_buffer,
> > > sizeof(a)); \
> > > +} while (0)
> > > +
> > > #if defined(NO_MMAP) ||
> > > defined(USE_WIN32_MMAP)
> >
> > It may seem as a matter of taste, or maybe
> > not: I prefer this without the
> > _swap_a_ptr
>
> The purpose of these pointers is certainly to
> avoid that the macro arguments are evaluated
> more than once.
I mistook "a" being used in sizeof(a) for breaking that assumption, but of
course a is *not* evaluated in that case. It is curious, though, that an
expression like "sizeof(a++)" would not be rejected.
Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2 3/4] introduce new format for git stash create
From: Junio C Hamano @ 2017-01-30 21:10 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-4-t.gummerer@gmail.com>
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
> + stash_msg="$1"
${1?"-m needs an argument"}
to error check "git stash create -m<Enter>"?
> + if test -z "$new_style"
> + then
> + stash_msg="$*"
> + fi
This breaks external users who do "git stash create" in the old
fashioned way, I think, but can be easily fixed with something like:
stash_msg=$1 untracked=$2
If the existing tests did not catch this, I guess there is a
coverage gap we may want to fill. Perhaps add a new test to 3903
that runs "git stash create message untracked" and makes sure it
still works?
^ permalink raw reply
* Re: [PATCH v2 4/4] stash: support filename argument
From: Junio C Hamano @ 2017-01-30 21:11 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-5-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Add an optional filename argument to git stash push, which allows for
> stashing a single (or multiple) files.
You can give pathspec with one or more elements, so "an optional
argument" sounds too limiting.
Allow 'git stash push' to take pathspec to specify which paths
to stash.
Also retitle
stash: teach 'push' (and 'create') to honor pathspec
or something.
> @@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
> only <message> does not trigger this action to prevent a misspelled
> subcommand from making an unwanted stash.
> +
> +If the paths argument is given in 'git stash push', only these files
> +are put in the new 'stash'. In addition only the indicated files are
> +changed in the working tree to match the index.
Actually the stash contains "all paths". You could say that you are
placing _modifications_ to these paths in stash, even though that is
not how Git's world model works (i.e. everything is a snapshot, and
modifications are merely difference between two successive
snapshots). A technically correct version may be:
When pathspec is given to 'git stash push', the new stash
records the modified states only for the files that match
the pathspec. The index entries and working tree files are
then rolled back to the state in HEAD only for these files,
too, leaving files that do not match the pathspec intact.
> diff --git a/git-stash.sh b/git-stash.sh
> index 5f08b43967..0072a38b4c 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -41,7 +41,7 @@ no_changes () {
> untracked_files () {
> excl_opt=--exclude-standard
> test "$untracked" = "all" && excl_opt=
> - git ls-files -o -z $excl_opt
> + git ls-files -o -z $excl_opt -- $1
Hmph, why "$1" is spelled without dq, implying that it is split at
$IFS boundary? This line alone makes me suspect that this is not
prepared to deal correctly with $IFS. Let's read on...
> @@ -59,6 +59,7 @@ create_stash () {
> stash_msg=
> untracked=
> new_style=
> + files=
> while test $# != 0
> do
> case "$1" in
> @@ -72,6 +73,12 @@ create_stash () {
> untracked="$1"
> new_style=t
> ;;
> + --)
> + shift
> + files="$@"
Isn't this the same as writing files="$*", i.e. concatenate the
multiple arguments with the first whitespace in $IFS in between?
> @@ -134,7 +141,7 @@ create_stash () {
> # Untracked files are stored by themselves in a parentless commit, for
> # ease of unpacking later.
> u_commit=$(
> - untracked_files | (
> + untracked_files $files | (
... and this lets it split at $IFS again when passing it down to the
helper. But the helper looks only at $1 so the second and subsequent
ones will be ignored altogether.
This cannot be correct, and any hunk in the remainder of the patch
that mentions $files will be incorrect for the same reason.
Is it possible to carry what the caller (and the end user) gave you
in "$@" without molesting it at all? That would mean you do not
need to introduce $files variable at all, and then the places that
do things like this:
> - create_stash -m "$stash_msg" -u "$untracked"
> + create_stash -m "$stash_msg" -u "$untracked" -- $files
can instead do
create_stash -m "$stash_msg" -u "$untracked" -- "$@"
That would allow you to work correctly with pathspec with $IFS
whitespaces in them.
^ permalink raw reply
* Re: [PATCH v2 2/4] stash: introduce push verb
From: Junio C Hamano @ 2017-01-30 21:12 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-3-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Introduce a new git stash push verb in addition to git stash save. The
> push verb is used to transition from the current command line arguments
> to a more conventional way, in which the message is specified after a -m
> parameter instead of being a positional argument.
I think the canonical way to express that is "... the message is
given as an argument to the -m option" (i.e. some options take an
argument, some others do not, and the "-m" takes one).
> This allows introducing a new filename argument to stash single files.
I do not want them to be "a filename argument", and I do not think
you meant them as such, either.
This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say
which subset of paths to stash (and leave others behind).
> +save_stash () {
> + push_options=
> + while test $# != 0
> + do
> + case "$1" in
> + -k|--keep-index)
> +...
> + esac
> + shift
> + done
It is a bit unfortunate that we need to duplicate the above
case/esac here. I do not know if doing it this way:
case "$1" in
--)
shift
break
;;
--help)
show_help
;;
-*)
# pass all options through to push_stash
push_options="$push_options $1"
;;
*)
break
;;
esac
and letting push_stash complain for an unknown option is easier to
maintain.
You are reversing the order of the options in the loop. Don't.
^ permalink raw reply
* Re: [PATCH] Completion: Add support for --submodule=diff
From: Peter Law @ 2017-01-30 21:13 UTC (permalink / raw)
To: git; +Cc: Jacob Keller, Junio C Hamano, szeder
In-Reply-To: <20161204144127.28452-1-peterjclaw@gmail.com>
Hi,
> Teach git-completion.bash about the 'diff' option to 'git diff
> --submodule=', which was added in Git 2.11.
I posted this patch back in December, but I've not heard anything. I'm
sure as maintainers you're all quite busy, but I was wondering how
long it usually takes to get a response to patches? (also whether I'd
gotten some part of the submission process wrong?)
Thanks,
Peter
^ permalink raw reply
* Re: [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard
From: Junio C Hamano @ 2017-01-30 21:13 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-2-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Don't mention git reset --hard in the documentation for git stash save.
> It's an implementation detail that doesn't matter to the end user and
> thus shouldn't be exposed to them.
Everybody understands what "reset --hard" does; it can be an
easier-to-read "short-hand" description to say "reset --hard"
instead of giving a lengthy description of what happens.
Because of that, I do not necessarily agree with the above
justification. I'll read the remainder of the series before
commenting further on the above.
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..0fc23c25ee 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -47,8 +47,9 @@ OPTIONS
>
> save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
>
> - Save your local modifications to a new 'stash', and run `git reset
> - --hard` to revert them. The <message> part is optional and gives
> + Save your local modifications to a new 'stash' and roll them
> + back both in the working tree and in the index.
"... roll them back both ..." is unclear where they are rolled back
to.
Perhaps "roll them back ... to HEAD" or something?
^ permalink raw reply
* Re: [PATCH] Completion: Add support for --submodule=diff
From: Brandon Williams @ 2017-01-30 21:43 UTC (permalink / raw)
To: Peter Law; +Cc: git, Jacob Keller, Junio C Hamano, szeder
In-Reply-To: <CAKoneT+Bn+MdbeNnPJsu23rbLCZ=jxADNVtpNefw9zNYMq26dA@mail.gmail.com>
On 01/30, Peter Law wrote:
> Hi,
>
> > Teach git-completion.bash about the 'diff' option to 'git diff
> > --submodule=', which was added in Git 2.11.
>
> I posted this patch back in December, but I've not heard anything. I'm
> sure as maintainers you're all quite busy, but I was wondering how
> long it usually takes to get a response to patches? (also whether I'd
> gotten some part of the submission process wrong?)
>
> Thanks,
> Peter
It looks like this must have just fallen through the cracks. Your patch
looks good to me and works when I test it locally.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-30 21:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301806591.3469@virtualbox>
Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:
> So I tried to verify that Visual C optimizes this well, and oh my deity,
> this was not easy. In Debug mode, it does not optimize, i.e. the memcpy()
> will be called, even for simple 32-bit integers. In Release mode, Visual
> Studio's defaults turn on "whole-program optimization" which means that
> the only swapping that is going on in the end is that the meaning of two
> registers is swapped, i.e. the SWAP() macro is expanded to... no assembler
> code at all.
>
> After trying this and that and something else, I finally ended up with the
> file-scope optimized SWAP() resulting in this assembly code:
>
> 00007FF791311000 mov eax,dword ptr [rcx]
> 00007FF791311002 mov r8d,dword ptr [rdx]
> 00007FF791311005 mov dword ptr [rcx],r8d
> 00007FF791311008 mov dword ptr [rdx],eax
This looks good.
> However, recent events (including some discussions on this mailing list
> which had unfortunate consequences) made me trust my instincts more. And
> my instincts tell me that it would be unwise to replace code that swaps
> primitive types in the straight-forward way by code that relies on
> advanced compiler optimization to generate efficient code, otherwise
> producing very suboptimal code.
I don't know how difficult it was to arrive at the result above, but I
wouldn't call inlining memcpy(3) an advanced optimization (anymore?),
since the major compilers seem to be doing that.
The SWAP in prio-queue.c seems to be the one with the biggest
performance impact. Or perhaps it's the one in lookup_object()? The
former is easier to measure, though.
Here's what I get with CFLAGS="-builtin -O2" (best of five):
$ time ./t/helper/test-prio-queue $(seq 100000 -1 1) dump >/dev/null
real 0m0.142s
user 0m0.120s
sys 0m0.020s
And this is with CFLAGS="-no-builtin -O2":
$ time ./t/helper/test-prio-queue $(seq 100000 -1 1) dump >/dev/null
real 0m0.170s
user 0m0.156s
sys 0m0.012s
Hmm. Not nice, but also not prohibitively slow.
> The commit you quoted embarrasses me, and I have no excuse for it. I would
> love to see that myswap() ugliness fixed by replacing it with a construct
> that is simpler, and generates good code even without any smart compiler.
I don't see a way to do that without adding a type parameter.
René
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Junio C Hamano @ 2017-01-30 21:58 UTC (permalink / raw)
To: cornelius.weig; +Cc: peff, git
In-Reply-To: <20170127100948.29408-2-cornelius.weig@tngtech.com>
cornelius.weig@tngtech.com writes:
> Notes:
> Changes wrt v2:
>
> - change wording in commit message s/do not typically/are not meant to/;
> - in update_refs_for_switch move refname to the enclosing block, so that
> should_autocreate_reflog has access. Thanks Junio for spotting this
> potential bug early :)
> - add test that asserts reflogs are created for tags if
> logAllRefUpdates=always. The case with logAllRefUpdates=true is IMHO already
> covered by the default case. To make that clearer, I explicitly added
> logAllRefUpdates=true.
These look all sensible. Especially thanks for reordering the code
to feed the real refname for the new branch in the "checkout"
codepath.
> When writing the test for git-tag, I realized that the option
> --no-create-reflog to git-tag does not take precedence over
> logAllRefUpdate=always. IOW the setting cannot be overridden on the command
> line. Do you think this is a defect or would it not be desirable to have this
> feature anyway?
"--no-create-reflog" should override the configuration set to "true"
or "always". Also "--create-reflog" should override the
configuration set to "false".
If the problem was inherited from the original code before your
change (e.g. you set logAllRefUpdates to true and then did
"update-ref --no-create-reflog refs/heads/foo". Does the code
before your change ignore the command lne option and create a reflog
for the branch?), then it would be ideal to fix the bug before this
series as a preparatory fix. If the problem was introduced by this
patch set, then we would need a fix not to introduce it ;-)
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfe685c..81ea2ed 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
> const char *old_desc, *reflog_msg;
> if (opts->new_branch) {
> if (opts->new_orphan_branch) {
> - if (opts->new_branch_log && !log_all_ref_updates) {
> + const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
> + if (opts->new_branch_log && should_autocreate_reflog(refname)) {
> int ret;
> - char *refname;
> struct strbuf err = STRBUF_INIT;
>
> - refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
> ret = safe_create_reflog(refname, 1, &err);
> - free(refname);
> if (ret) {
> fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
> opts->new_orphan_branch, err.buf);
Here you need to have another free(), as this block makes an early
return and you end up leaking refname.
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 1cfa8a2..1bf622d 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -71,6 +71,7 @@ test_expect_success 'creating a tag for an unknown revision should fail' '
>
> # commit used in the tests, test_tick is also called here to freeze the date:
> test_expect_success 'creating a tag using default HEAD should succeed' '
> + test_config core.logAllRefUpdates true &&
> test_tick &&
> echo foo >foo &&
> git add foo &&
This change is to make sure that 'true' does not affect tags (but
'always' does as seen in the later new test)? I am just double
checking, not objecting.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 0/5] urlmatch: allow wildcard-based matches
From: Junio C Hamano @ 2017-01-30 22:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt, Philip Oakley
In-Reply-To: <cover.1485512626.git.patrick.steinhardt@elego.de>
Patrick Steinhardt <patrick.steinhardt@elego.de> writes:
> - I realized that with my patches, "ranking" of URLs was broken.
> Previously, we've always taken the longest matching URL. As
> previously, only the user and path could actually differ, only
> these two components were used for the comparison. I've
> changed this now to also include the host part so that URLs
> with a longer host will take precedence. This resulted in a
> the patch 4.
Good thinking. I was wondering about this, too.
Thanks. Will read it through and replace.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-30 22:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701302158110.3469@virtualbox>
Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:
> It is curious, though, that an
> expression like "sizeof(a++)" would not be rejected.
Clang normally warns about something like this ("warning: expression
with side effects has no effect in an unevaluated context
[-Wunevaluated-expression]"), but not if the code is part of a macro. I
don't know if that's intended, but it sure is helpful in the case of SWAP.
> Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?
That might be a valid expectation, but GCC says "error: lvalue required
as unary '&' operand" and clang puts it "error: cannot take the address
of an rvalue of type".
René
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Brandon Williams @ 2017-01-30 22:21 UTC (permalink / raw)
To: René Scharfe
Cc: Johannes Schindelin, Johannes Sixt, Git List, Junio C Hamano
In-Reply-To: <77098ac8-1084-a5c4-a5e6-fb382e3dc3a0@web.de>
On 01/30, René Scharfe wrote:
> Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:
> >It is curious, though, that an
> >expression like "sizeof(a++)" would not be rejected.
>
> Clang normally warns about something like this ("warning: expression
> with side effects has no effect in an unevaluated context
> [-Wunevaluated-expression]"), but not if the code is part of a
> macro. I don't know if that's intended, but it sure is helpful in
> the case of SWAP.
>
> >Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?
>
> That might be a valid expectation, but GCC says "error: lvalue
> required as unary '&' operand" and clang puts it "error: cannot take
> the address of an rvalue of type".
>
> René
Perhaps we could disallow a side-effect operator in the macro. By
disallow I mean place a comment at the definition to the macro and
hopefully catch something like that in code-review. We have the same
issue with the `ALLOC_GROW()` macro.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 3/5] use SWAP macro
From: Junio C Hamano @ 2017-01-30 22:22 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Johannes Schindelin
In-Reply-To: <187c2b39-40cf-7e07-b489-d40cdf5f9145@web.de>
René Scharfe <l.s.r@web.de> writes:
> if (tree2->flags & UNINTERESTING) {
> - struct object *tmp = tree2;
> - tree2 = tree1;
> - tree1 = tmp;
> + SWAP(tree2, tree1);
A human would have written this SWAP(tree1, tree2).
Not that I think such a manual fix-up should be made in _this_
patch, which may end up mixing mechanical conversion (which we may
want to keep reproducible) and hand tweaks. But this swapped swap
reads somewhat silly.
> diff --git a/diff-no-index.c b/diff-no-index.c
> index f420786039..1ae09894d7 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -186,9 +186,8 @@ static int queue_diff(struct diff_options *o,
>
> if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
> unsigned tmp;
> - const char *tmp_c;
> tmp = mode1; mode1 = mode2; mode2 = tmp;
> - tmp_c = name1; name1 = name2; name2 = tmp_c;
> + SWAP(name1, name2);
Curious that mode swapping is left for a later iteration.
> diff --git a/diff.c b/diff.c
> index f08cd8e033..9de1ba264f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5118,13 +5118,11 @@ void diff_change(struct diff_options *options,
>
> if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
> unsigned tmp;
> - const unsigned char *tmp_c;
> - tmp = old_mode; old_mode = new_mode; new_mode = tmp;
> - tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
> + SWAP(old_mode, new_mode);
> + SWAP(old_sha1, new_sha1);
> tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
> new_sha1_valid = tmp;
So is this one.
> diff --git a/merge-recursive.c b/merge-recursive.c
> ...
> - tmp = ren2;
> - ren2 = ren1;
> - ren1 = tmp;
> + SWAP(ren2, ren1);
A human would have written this SWAP(ren1, ren2).
^ permalink raw reply
* Re: [PATCH 4/5] diff: use SWAP macro
From: Junio C Hamano @ 2017-01-30 22:22 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Johannes Schindelin
In-Reply-To: <84944ecd-d14e-b5e9-7566-9ab2b68c02fb@web.de>
René Scharfe <l.s.r@web.de> writes:
> Use the macro SWAP to exchange the value of pairs of variables instead
> of swapping them manually with the help of a temporary variable. The
> resulting code is shorter and easier to read.
>
> The two cases were not transformed by the semantic patch swap.cocci
> because it's extra careful and handles only cases where the types of all
> variables are the same
Ah, that explains my "huh?" in 3/5; thanks.
^ permalink raw reply
* Re: [PATCH] checkout: convert post_checkout_hook() to struct object_id
From: brian m. carlson @ 2017-01-30 22:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: René Scharfe, Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301359500.3469@virtualbox>
[-- Attachment #1: Type: text/plain, Size: 456 bytes --]
On Mon, Jan 30, 2017 at 02:01:12PM +0100, Johannes Schindelin wrote:
> Hi René,
>
> On Sat, 28 Jan 2017, René Scharfe wrote:
>
> > Signed-off-by: Rene Scharfe <l.s.r@web.de>
>
> These three SHA-1 -> OID patches all appear correct to me.
I concur. These look good.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/5] urlmatch: allow wildcard-based matches
From: Junio C Hamano @ 2017-01-30 22:52 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt, Philip Oakley
In-Reply-To: <xmqqy3xstdh9.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <patrick.steinhardt@elego.de> writes:
>
>> - I realized that with my patches, "ranking" of URLs was broken.
>> Previously, we've always taken the longest matching URL. As
>> previously, only the user and path could actually differ, only
>> these two components were used for the comparison. I've
>> changed this now to also include the host part so that URLs
>> with a longer host will take precedence. This resulted in a
>> the patch 4.
>
> Good thinking. I was wondering about this, too.
>
> Thanks. Will read it through and replace.
Ugh. When applied on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), Git fails its self-test in t5551 #31 (I do not run with
EXPENSIVE so some tests liks #30 are skipped, if it matters).
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Junio C Hamano @ 2017-01-30 22:57 UTC (permalink / raw)
To: cornelius.weig; +Cc: peff, git
In-Reply-To: <xmqq37g0us5p.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index bfe685c..81ea2ed 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>> const char *old_desc, *reflog_msg;
>> if (opts->new_branch) {
>> if (opts->new_orphan_branch) {
>> - if (opts->new_branch_log && !log_all_ref_updates) {
>> + const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
>> ...
>> if (ret) {
>> fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
>> opts->new_orphan_branch, err.buf);
>
> Here you need to have another free(), as this block makes an early
> return and you end up leaking refname.
I am building with the attached patch squashed on top.
The extra free(refname) is to plug the leak I pointed out, and the
type of refname is no longer const, because "const char *" cannot be
free()d without casting, and in this codepath I do not see a reason
to mark it as const.
When queued on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), however, this fails t2017#9 (orphan with -l makes
reflog when core.logAllRefUpdates = false).
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 81ea2eda99..e1a60fd8ea 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -612,7 +612,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
const char *old_desc, *reflog_msg;
if (opts->new_branch) {
if (opts->new_orphan_branch) {
- const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
+ char *refname;
+
+ refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
if (opts->new_branch_log && should_autocreate_reflog(refname)) {
int ret;
struct strbuf err = STRBUF_INIT;
@@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
opts->new_orphan_branch, err.buf);
strbuf_release(&err);
+ free(refname);
return;
}
strbuf_release(&err);
^ permalink raw reply related
* Re: [PATCH] Completion: Add support for --submodule=diff
From: Junio C Hamano @ 2017-01-30 23:03 UTC (permalink / raw)
To: Peter Law; +Cc: git, Jacob Keller, szeder
In-Reply-To: <CAKoneT+Bn+MdbeNnPJsu23rbLCZ=jxADNVtpNefw9zNYMq26dA@mail.gmail.com>
Peter Law <peterjclaw@gmail.com> writes:
>> Teach git-completion.bash about the 'diff' option to 'git diff
>> --submodule=', which was added in Git 2.11.
>
> I posted this patch back in December, but I've not heard anything. I'm
> sure as maintainers you're all quite busy, but I was wondering how
> long it usually takes to get a response to patches? (also whether I'd
> gotten some part of the submission process wrong?)
When there is clear "subsystem maintainer(s)" in the area, I try to
refrain from commenting until they speak up, and completion scripts
are one of these areas. I usually ping them after a few days but in
December with holidays and things people are usually slow, and so
was I X-<.
Will pick it up. Thanks for a reminder; you absolutely did the
right thing (i.e. sending it out with people who are likely to know
about the area CC'ed, waiting for a while and then sending a
reminder).
^ permalink raw reply
* Re: [PATCH 0/5] introduce SWAP macro
From: Junio C Hamano @ 2017-01-30 23:20 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Johannes Schindelin
In-Reply-To: <8ef4c833-45bd-6831-0683-6d01f30aa518@web.de>
René Scharfe <l.s.r@web.de> writes:
> Exchanging the value of two variables requires declaring a temporary
> variable and repeating their names. The swap macro in apply.c
> simplifies this for its callers without changing the compiled binary.
> Polish this macro and export it, then use it throughout the code to
> reduce repetition and hide the declaration of temporary variables.
All looked reasonable (modulo "swap tree2 and tree1???" ordering).
Also the object-id ones looked good.
Thanks.
^ permalink raw reply
* Re: [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths
From: Junio C Hamano @ 2017-01-30 23:21 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1485636764.2482.2.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> The current message printed by "git merge-recursive" for a rename/delete
> conflict is like this:
>
> CONFLICT (rename/delete): new-path deleted in HEAD and renamed in
> other-branch. Version other-branch of new-path left in tree.
>
> To be more helpful, the message should show both paths of the rename and
> state that the deletion occurred at the old path, not the new path. So
> change the message to the following format:
>
> CONFLICT (rename/delete): old-path deleted in HEAD and renamed to
> new-path in other-branch. Version other-branch of new-path left in tree.
Sounds like a sensible goal.
> Please check that my refactoring is indeed correct! All the existing tests pass
> for me, but the existing test coverage of these conflict messages looks poor.
This unfortunately is doing a bit too many things at once from that
point of view. I need to reserve a solid quiet 20-minutes without
distraction to check it, which I am hoping to do tonight.
Thanks.
>
> merge-recursive.c | 117 ++++++++++++++++++++++-------------------
> t/t6045-merge-rename-delete.sh | 23 ++++++++
> 2 files changed, 86 insertions(+), 54 deletions(-)
> create mode 100755 t/t6045-merge-rename-delete.sh
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d327209..e8fce10 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1061,16 +1061,20 @@ static int merge_file_one(struct merge_options *o,
> }
>
> static int handle_change_delete(struct merge_options *o,
> - const char *path,
> + const char *path, const char *old_path,
> const struct object_id *o_oid, int o_mode,
> - const struct object_id *a_oid, int a_mode,
> - const struct object_id *b_oid, int b_mode,
> + const struct object_id *changed_oid,
> + int changed_mode,
> + const char *change_branch,
> + const char *delete_branch,
> const char *change, const char *change_past)
> {
> - char *renamed = NULL;
> + char *alt_path = NULL;
> + const char *update_path = path;
> int ret = 0;
> +
> if (dir_in_way(path, !o->call_depth, 0)) {
> - renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
> + update_path = alt_path = unique_path(o, path, change_branch);
> }
>
> if (o->call_depth) {
> @@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,
> */
> ret = remove_file_from_cache(path);
> if (!ret)
> - ret = update_file(o, 0, o_oid, o_mode,
> - renamed ? renamed : path);
> - } else if (!a_oid) {
> - if (!renamed) {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree."),
> - change, path, o->branch1, change_past,
> - o->branch2, o->branch2, path);
> - ret = update_file(o, 0, b_oid, b_mode, path);
> - } else {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree at %s."),
> - change, path, o->branch1, change_past,
> - o->branch2, o->branch2, path, renamed);
> - ret = update_file(o, 0, b_oid, b_mode, renamed);
> - }
> + ret = update_file(o, 0, o_oid, o_mode, update_path);
> } else {
> - if (!renamed) {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree."),
> - change, path, o->branch2, change_past,
> - o->branch1, o->branch1, path);
> + if (!alt_path) {
> + if (!old_path) {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s in %s. Version %s of %s left in tree."),
> + change, path, delete_branch, change_past,
> + change_branch, change_branch, path);
> + } else {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s to %s in %s. Version %s of %s left in tree."),
> + change, old_path, delete_branch, change_past, path,
> + change_branch, change_branch, path);
> + }
> } else {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree at %s."),
> - change, path, o->branch2, change_past,
> - o->branch1, o->branch1, path, renamed);
> - ret = update_file(o, 0, a_oid, a_mode, renamed);
> + if (!old_path) {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s in %s. Version %s of %s left in tree at %s."),
> + change, path, delete_branch, change_past,
> + change_branch, change_branch, path, alt_path);
> + } else {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s to %s in %s. Version %s of %s left in tree at %s."),
> + change, old_path, delete_branch, change_past, path,
> + change_branch, change_branch, path, alt_path);
> + }
> }
> /*
> - * No need to call update_file() on path when !renamed, since
> - * that would needlessly touch path. We could call
> - * update_file_flags() with update_cache=0 and update_wd=0,
> - * but that's a no-op.
> + * No need to call update_file() on path when change_branch ==
> + * o->branch1 && !alt_path, since that would needlessly touch
> + * path. We could call update_file_flags() with update_cache=0
> + * and update_wd=0, but that's a no-op.
> */
> + if (change_branch != o->branch1 || alt_path)
> + ret = update_file(o, 0, changed_oid, changed_mode, update_path);
> }
> - free(renamed);
> + free(alt_path);
>
> return ret;
> }
> @@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,
> static int conflict_rename_delete(struct merge_options *o,
> struct diff_filepair *pair,
> const char *rename_branch,
> - const char *other_branch)
> + const char *delete_branch)
> {
> const struct diff_filespec *orig = pair->one;
> const struct diff_filespec *dest = pair->two;
> - const struct object_id *a_oid = NULL;
> - const struct object_id *b_oid = NULL;
> - int a_mode = 0;
> - int b_mode = 0;
> -
> - if (rename_branch == o->branch1) {
> - a_oid = &dest->oid;
> - a_mode = dest->mode;
> - } else {
> - b_oid = &dest->oid;
> - b_mode = dest->mode;
> - }
>
> if (handle_change_delete(o,
> o->call_depth ? orig->path : dest->path,
> + o->call_depth ? NULL : orig->path,
> &orig->oid, orig->mode,
> - a_oid, a_mode,
> - b_oid, b_mode,
> + &dest->oid, dest->mode,
> + rename_branch, delete_branch,
> _("rename"), _("renamed")))
> return -1;
>
> @@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,
> struct object_id *a_oid, int a_mode,
> struct object_id *b_oid, int b_mode)
> {
> + const char *modify_branch, *delete_branch;
> + struct object_id *changed_oid;
> + int changed_mode;
> +
> + if (a_oid) {
> + modify_branch = o->branch1;
> + delete_branch = o->branch2;
> + changed_oid = a_oid;
> + changed_mode = a_mode;
> + } else {
> + modify_branch = o->branch2;
> + delete_branch = o->branch1;
> + changed_oid = b_oid;
> + changed_mode = b_mode;
> + }
> +
> return handle_change_delete(o,
> - path,
> + path, NULL,
> o_oid, o_mode,
> - a_oid, a_mode,
> - b_oid, b_mode,
> + changed_oid, changed_mode,
> + modify_branch, delete_branch,
> _("modify"), _("modified"));
> }
>
> diff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh
> new file mode 100755
> index 0000000..8f33244
> --- /dev/null
> +++ b/t/t6045-merge-rename-delete.sh
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +test_description='Merge-recursive rename/delete conflict message'
> +. ./test-lib.sh
> +
> +test_expect_success 'rename/delete' '
> +echo foo >A &&
> +git add A &&
> +git commit -m "initial" &&
> +
> +git checkout -b rename &&
> +git mv A B &&
> +git commit -m "rename" &&
> +
> +git checkout master &&
> +git rm A &&
> +git commit -m "delete" &&
> +
> +test_must_fail git merge --strategy=recursive rename >output &&
> +test_i18ngrep "CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree." output
> +'
> +
> +test_done
^ permalink raw reply
* Re: difflame
From: Jeff King @ 2017-01-30 23:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Edmundo Carmona Antoranz, Git List
In-Reply-To: <xmqqd1f4uug6.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 30, 2017 at 01:08:41PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Tue, Jan 17, 2017 at 11:24:02PM -0600, Edmundo Carmona Antoranz wrote:
> >
> >> For a very long time I had wanted to get the output of diff to include
> >> blame information as well (to see when something was added/removed).
> >
> > This is something I've wanted, too. The trickiest part, though, is
> > blaming deletions, because git-blame only tracks the origin of content,
> > not the origin of a change.
>
> Hmph, this is a comment without looking at what difflame does
> internally, so you can ignore me if I am misunderstood what problem
> you are pointing out, but I am not sure how "tracks the origin of
> content" could be a problem.
>
> If output from "git show" says this:
>
> --- a/file
> +++ b/file
> @@ -1,5 +1,6 @@
> a
> b
> -c
> +C
> +D
> d
> e
>
> in order to annotate lines 'a', 'b', 'd', and 'e' for their origin,
> you would run 'blame' on the commit the above output was taken from
> (or its parent---they are in the context so either would be OK).
>
> You know where 'C' and 'D' came from already. It's the commit you
> are feeding "git show".
I think the point (or at least as I understand it) is that the diff is
not "git show" for a particular commit. It could be part of a much
larger diff that covers many commits.
As a non-hypothetical instance, I have a fork of git.git that has
various enhancements. I want to feed those enhancements upstream. I need
to know which commits should be cherry-picked to get those various
enhancements.
Looking at "log origin..fork" tells me too many commits, because it
includes ones which aren't useful anymore. Either because they already
went upstream, or because they were cherry-picked to the fork and their
upstream counterparts merged (or even equivalent commits made upstream
that obsoleted the features).
Looking at "git diff origin fork" tells me what the actual differences
are, but it doesn't show me which commits are responsible for them.
I can "git blame" each individual line of the diff (starting with "fork"
as the tip), but that doesn't work for lines that no longer exist (i.e.,
when the interesting change is a deletion).
> In order to run blame to find where 'c' came from, you need to start
> at the _parent_ of the commit the above output came from, and the
> hunk header shows which line range to find the final 'c'.
So perhaps that explains my comment more. "blame" is not good for
finding which commit took away a line. I've tried using "blame
--reverse", but it shows you the parent of the commit you are looking
for, which is slightly annoying. :)
"git log -S" is probably a better tool for finding that.
-Peff
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Jeff King @ 2017-01-30 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: cornelius.weig, git
In-Reply-To: <xmqq37g0us5p.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:
> > When writing the test for git-tag, I realized that the option
> > --no-create-reflog to git-tag does not take precedence over
> > logAllRefUpdate=always. IOW the setting cannot be overridden on the command
> > line. Do you think this is a defect or would it not be desirable to have this
> > feature anyway?
>
> "--no-create-reflog" should override the configuration set to "true"
> or "always". Also "--create-reflog" should override the
> configuration set to "false".
>
> If the problem was inherited from the original code before your
> change (e.g. you set logAllRefUpdates to true and then did
> "update-ref --no-create-reflog refs/heads/foo". Does the code
> before your change ignore the command lne option and create a reflog
> for the branch?), then it would be ideal to fix the bug before this
> series as a preparatory fix. If the problem was introduced by this
> patch set, then we would need a fix not to introduce it ;-)
I hadn't thought about that. I think "git branch --no-create-reflog" has
the same problem in the existing code.
I suspect nobody cares much in practice. Even if you say "don't create a
reflog now", if you have core.logAllRefUpdates turned on, then it's
likely that some _other_ operation will create the reflog later
accidentally (e.g., as soon as you "git checkout foo && git commit",
you'll get a reflog). I think you're fighting an uphill battle to turn
logAllRefUpdates on and then try to disable some reflogs selectively.
So I agree the current behavior is quietly broken, which is not good.
But I wonder if "--no-create-reflog" is really sane in the first place,
and whether we might be better off to simply disallow it.
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] git-prompt.sh: add submodule indicator
From: Junio C Hamano @ 2017-01-30 23:48 UTC (permalink / raw)
To: Benjamin Fuchs; +Cc: git, szeder.dev, sbeller, sandals, ville.skytta
In-Reply-To: <1485809065-11978-2-git-send-email-email@benjaminfuchs.de>
Benjamin Fuchs <email@benjaminfuchs.de> writes:
> I expirienced that working with submodules can be confusing. This indicator
> will make you notice very easy when you switch into a submodule.
I am not quite sure what the above wants to say. If you work on two
projects, A and B, then having something that quickly reminds you
which one you are in is beneficial and people often do so by having
the current directory name in the prompt.
The log message needs to explain why working on a submodule C of a
project A is any more special, i.e. why are the existing ways the
users are using to remind them between A and B cannot be used for C.
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 97eacd7..4c82e7f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -93,6 +93,10 @@
> # directory is set up to be ignored by git, then set
> # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
> # repository level by setting bash.hideIfPwdIgnored to "false".
> +#
> +# If you would like __git_ps1 to indicate that you are in a submodule,
> +# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
> +# the branch name.
>
> # check whether printf supports -v
> __git_printf_supports_v=
> @@ -284,6 +288,32 @@ __git_eread ()
> test -r "$f" && read "$@" <"$f"
> }
>
> +# __git_is_submodule
> +# Based on:
> +# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
> +__git_is_submodule ()
It seems like this function is checking if you are "IN" submodule,
not "IS" submodule. A misnamed function?
> +{
> + local git_dir parent_git module_name path
> + # Find the root of this git repo, then check if its parent dir is also a repo
> + git_dir="$(git rev-parse --show-toplevel)"
> + module_name="$(basename "$git_dir")"
This does not give "module_name" in the sense the word is used in
the submodule subsystem. If this variable is useful, call that
with "path" in its name (I do not think it alone is useful at all).
> + parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
> + if [[ -n $parent_git ]]; then
> + # List all the submodule paths for the parent repo
> + while read path
> + do
> + if [[ "$path" != "$module_name" ]]; then continue; fi
> + if [[ -d "$git_dir/../$path" ]]; then return 0; fi
> + done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
Instead of doing this "loop over submodules and just get true or
false", it may make a lot more sense to find the module name and
report that. That would allow you to have the actual submodule
name, not a generic "sub:" which does not help the users to tell
which one of 47 submodules they have in their top-level project
they are currently in.
If your projects are layed out like so
/home/bf/projects/A/
/home/bf/projects/A/lib/B/ -- submodule
/home/bf/projects/A/Doc/ -- another submodule
/home/bf/projects/A/Doc/technical -- a subdirectory of Doc submodule
and you are in /home/bf/projects/A/Doc/technical/ subdirectory, your
$git_dir variable (which is grossly misnamed, too; call it "top"
perhaps?) would be "/home/bf/projects/A/Doc" and then your
$parent_git variable (again, that is misnamed; call it
"parent_top"?) would be "/home/bf/projects/A". The submodule path
to the submodule you are currently in is "Doc" (you learn it as the
difference between these two).
You can ask the top-level project the name of the submodule that is
currently at "Doc" with "submodule--helper name". Unless the project
has moved it since it first added the submodule, the module name may
also be "Doc", but if it were moved, it may be different.
And that "module name" is a more useful thing than a hardcoded
string "sub:" to use in the prompt, I would think.
^ permalink raw reply
* Re: [PATCH 1/4] git-prompt.sh: add submodule indicator
From: Benjamin Fuchs @ 2017-01-31 0:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, szeder.dev, sbeller, sandals, ville.skytta
In-Reply-To: <xmqqr33krtww.fsf@gitster.mtv.corp.google.com>
Hi Junio,
thanks for your reply. Some of your suggestions are covered by my rework
in Patch [2/4].
In [2/4] I got rid of the loop by feedback of Gábor.
Sorry if my patch wasn't well formed.
On 31.01.2017 00:48, Junio C Hamano wrote:
> Benjamin Fuchs <email@benjaminfuchs.de> writes:
>
>> I expirienced that working with submodules can be confusing. This indicator
>> will make you notice very easy when you switch into a submodule.
> I am not quite sure what the above wants to say. If you work on two
> projects, A and B, then having something that quickly reminds you
> which one you are in is beneficial and people often do so by having
> the current directory name in the prompt.
>
> The log message needs to explain why working on a submodule C of a
> project A is any more special, i.e. why are the existing ways the
> users are using to remind them between A and B cannot be used for C.
A submodule can be anywhere in your parent git repository. And while
walking through the
parent repository it is sometime a good reminder to know when to just
entered a submodule.
Because now all git command will work on the submodule. I guess this
indicator is a good way
to show that.
And with patch [2/4] (and fixed it with [3/4]) I also introduced the
"dirty" indicator for the submodule, which can
show you, that your current checked out state in the submodule is not
the one committed
to the parent.
>
>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>> index 97eacd7..4c82e7f 100644
>> --- a/contrib/completion/git-prompt.sh
>> +++ b/contrib/completion/git-prompt.sh
>> @@ -93,6 +93,10 @@
>> # directory is set up to be ignored by git, then set
>> # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
>> # repository level by setting bash.hideIfPwdIgnored to "false".
>> +#
>> +# If you would like __git_ps1 to indicate that you are in a submodule,
>> +# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
>> +# the branch name.
>>
>> # check whether printf supports -v
>> __git_printf_supports_v=
>> @@ -284,6 +288,32 @@ __git_eread ()
>> test -r "$f" && read "$@" <"$f"
>> }
>>
>> +# __git_is_submodule
>> +# Based on:
>> +# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
>> +__git_is_submodule ()
> It seems like this function is checking if you are "IN" submodule,
> not "IS" submodule. A misnamed function?
Reworked in Patch [2/4]
>> +{
>> + local git_dir parent_git module_name path
>> + # Find the root of this git repo, then check if its parent dir is also a repo
>> + git_dir="$(git rev-parse --show-toplevel)"
>> + module_name="$(basename "$git_dir")"
> This does not give "module_name" in the sense the word is used in
> the submodule subsystem. If this variable is useful, call that
> with "path" in its name (I do not think it alone is useful at all).
Reworked in Patch [2/4]
>
>> + parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
>> + if [[ -n $parent_git ]]; then
>> + # List all the submodule paths for the parent repo
>> + while read path
>> + do
>> + if [[ "$path" != "$module_name" ]]; then continue; fi
>> + if [[ -d "$git_dir/../$path" ]]; then return 0; fi
>> + done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
> Instead of doing this "loop over submodules and just get true or
> false", it may make a lot more sense to find the module name and
> report that. That would allow you to have the actual submodule
> name, not a generic "sub:" which does not help the users to tell
> which one of 47 submodules they have in their top-level project
> they are currently in.
>
> If your projects are layed out like so
>
> /home/bf/projects/A/
> /home/bf/projects/A/lib/B/ -- submodule
> /home/bf/projects/A/Doc/ -- another submodule
> /home/bf/projects/A/Doc/technical -- a subdirectory of Doc submodule
>
> and you are in /home/bf/projects/A/Doc/technical/ subdirectory, your
> $git_dir variable (which is grossly misnamed, too; call it "top"
> perhaps?) would be "/home/bf/projects/A/Doc" and then your
> $parent_git variable (again, that is misnamed; call it
> "parent_top"?) would be "/home/bf/projects/A". The submodule path
> to the submodule you are currently in is "Doc" (you learn it as the
> difference between these two).
>
> You can ask the top-level project the name of the submodule that is
> currently at "Doc" with "submodule--helper name". Unless the project
> has moved it since it first added the submodule, the module name may
> also be "Doc", but if it were moved, it may be different.
>
> And that "module name" is a more useful thing than a hardcoded
> string "sub:" to use in the prompt, I would think.
Reworked in Patch [2/4].
^ permalink raw reply
* Re: [PATCH] Completion: Add support for --submodule=diff
From: Jacob Keller @ 2017-01-31 0:10 UTC (permalink / raw)
To: peterjclaw; +Cc: Git mailing list, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20161204144127.28452-1-peterjclaw@gmail.com>
On Sun, Dec 4, 2016 at 6:41 AM, <peterjclaw@gmail.com> wrote:
> From: Peter Law <PeterJCLaw@gmail.com>
>
> Teach git-completion.bash about the 'diff' option to 'git diff
> --submodule=', which was added in Git 2.11.
>
> Signed-off-by: Peter Law <PeterJCLaw@gmail.com>
> ---
> contrib/completion/git-completion.bash | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 21016bf8d..ab11e7371 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1206,7 +1206,7 @@ _git_describe ()
>
> __git_diff_algorithms="myers minimal patience histogram"
>
> -__git_diff_submodule_formats="log short"
> +__git_diff_submodule_formats="diff log short"
>
> __git_diff_common_options="--stat --numstat --shortstat --summary
> --patch-with-stat --name-only --name-status --color
> --
> 2.11.0
>
Yep, this looks good to me.
Thanks,
Jake
^ 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