Git development
 help / color / mirror / Atom feed
* [PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again
From: René Scharfe @ 2017-02-11 19:51 UTC (permalink / raw)
  To: Git List, Stefan Beller; +Cc: Junio C Hamano

Don't throw the memory allocated for remove_dir_recursively() away after
a single call, use it for the other entries as well instead.

This change was done before in deb8e15a (rm: reuse strbuf for all
remove_dir_recursively() calls), but was reverted as a side-effect of
55856a35 (rm: absorb a submodules git dir before deletion). Reinstate
the optimization.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Was deb8e15a a rebase victim?

 builtin/rm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 452170a3ab..fb79dcab18 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -360,15 +360,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 */
 	if (!index_only) {
 		int removed = 0, gitmodules_modified = 0;
+		struct strbuf buf = STRBUF_INIT;
 		for (i = 0; i < list.nr; i++) {
 			const char *path = list.entry[i].name;
 			if (list.entry[i].is_submodule) {
-				struct strbuf buf = STRBUF_INIT;
-
+				strbuf_reset(&buf);
 				strbuf_addstr(&buf, path);
 				if (remove_dir_recursively(&buf, 0))
 					die(_("could not remove '%s'"), path);
-				strbuf_release(&buf);
 
 				removed = 1;
 				if (!remove_path_from_gitmodules(path))
@@ -382,6 +381,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			if (!removed)
 				die_errno("git rm: '%s'", path);
 		}
+		strbuf_release(&buf);
 		if (gitmodules_modified)
 			stage_updated_gitmodules();
 	}
-- 
2.11.1


^ permalink raw reply related

* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Junio C Hamano @ 2017-02-11 21:08 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <20170211075254.GA16053@ubuntu-512mb-blr1-01.localdomain>

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> On Fri, Feb 10, 2017 at 03:35:47PM -0800, Junio C Hamano wrote:
>
>> I am wondering if writing it like the following is easier to
>> understand.  I had a hard time figuring out what you are trying to
>> do, partly because "args" is quite a misnomer---implying "how many
>> arguments did we see" that is similar to opts that does mean "how
>> many options did handle_revision_opts() see?"
>
> Um, okay, I see that "args" is very confusing. Would it help if this variable
> was called "arg_not_rev"?

Not really.  If we absolutely need to have one variable that is
meant to escape the "if it begins with a dash" block and to affect
what comes next, I think the variable should mean "we know we saw a
revision and you do not have to call it again".  IOW the code that
needs to do "handle_rev_arg_called && arg_not_rev" is just being
silly.  At that point in the codeflow, I do not see why the code
needs to take two bits of information and combine them; the one that
sets these two variables should have done the work for it.

And that would make the if statement slightly easier to read
compared to the original.  I am however not suggesting to do that;
read on.

> Because the value that is returned from
> handle_revision_arg is 1 when it is not a revision, and 0 when it is a
> revision.

The function follows the convention to return 0 for success, -1 for
error/unexpected, by the way.

> Um, I am sorry, but I feel that decrementing left, and incrementing it again is
> also confusing.

Yes, but it is no more confusing than your original "left--".

If we want to make the flow of logic easier to follow, we need to
step back and view what the codepath _wants_ to do at the higher
level, which is:

 * If it is an option known to us, handle it and go to the next arg.

 * If it is an option that we do not understand, stuff it in
   argv[left++] and go to the next arg.

 * If it is a rev, handle it, and note that fact in got_rev_arg.

 * If it is not a rev and we haven't seen dashdash, verify that it
   and everything that follows it are pathnames (which is an inexact
   but a cheap way to avoid ambiguity), make all them the prune_data
   and conclude.

Because the second step currently is implemented by calling
handle_opt(), which not just tells if it is an option we understand
or not, but also mucks with argv[left++], you need to undo it once
you start making it possible for a valid "rev" to begin with a dash.
That is what your left-- was, and that is what "decrement and then
increment when it turns out it was an unknown option after all" is.

The first step to a saner flow _could_ be to stop passing the unkv
and unkc to handle_revision_opt() and instead make the caller
responsible for doing that.  That would express what your patch
wanted to do in the most natural way, i.e.

 * If it is an option known to us, handle it and go to the next arg.

 * If it is a rev, handle it, and note that fact in got_rev_arg
   (this change of order enables you to allow a rev that begins with
   a dash, which would have been misrecognised as a possible unknown
   option).

 * If it looks like an option (i.e. "begins with a dash"), then we
   already know that it is not something we understand, because the
   first step would have caught it already.  Stuff it in
   argv[left++] and go to the next arg.

 * If it is not a rev and we haven't seen dashdash, verify that it
   and everything that follows it are pathnames (which is an inexact
   but a cheap way to avoid ambiguity), make all them the prune_data
   and conclude.

Such a change to handle_revision_opt() unfortunately affects other
callers of the function, so it may not be worth it, but I think
"decrement and then increment, because this codepath wants to check
to see something that may ordinarily be clasified as an unknown
option if it is a rev" is an ugly workaround, just like your left--
was.  But I think the resulting code flow is much closer to the
above ideal.


^ permalink raw reply

* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Junio C Hamano @ 2017-02-11 23:40 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> Such a change to handle_revision_opt() unfortunately affects other
> callers of the function, so it may not be worth it, and I think
> "decrement and then increment, because this codepath wants to check
> to see something that may ordinarily be clasified as an unknown
> option if it is a rev" is an ugly workaround, just like your left--
> was.  But I think the resulting code flow is much closer to the
> above ideal.

Having re-analysed the codepath like so, I realize that the new
variable I introduced was misnamed.  Its purpose is to let the
"if arg begins with dash, do this" block communicate that what the
later part of the code is told to inspect in "arg" may be an option
that we do not recognise.  So I shouldn't have called it maybe_rev;
the message from the former to the latter is "this may be an unknown
option" and I should have called it "maybe_unknown_opt".


^ permalink raw reply

* [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Eric Wong @ 2017-02-12  0:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Vasco Almeida

When parsing an mbox, it is possible to get existing In-Reply-To
and References headers blindly appended into the headers of
message we generate.   This is probably the wrong thing to do
and we should prioritize what was given in the command-line,
cover letter, and previously-sent messages.

One example I've noticed in the wild was:

https://public-inbox.org/git/20161111124541.8216-17-vascomalmeida@sapo.pt/raw
---
 I'm not completely sure this is what Vasco was doing in that
 message, so it's an RFC for now...

 git-send-email.perl | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 068d60b3e6..5ab3d8585c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1543,7 +1543,13 @@ foreach my $t (@files) {
 			elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) {
 				push @xh, $_;
 			}
-
+			elsif (/^(?:References|In-Reply-To):/i) {
+				if (defined $initial_reply_to || $thread) {
+					warn
+"Ignoring $_ header in mbox body since it conflicts with\n
+--in-reply-to and --thread switches\n"
+				}
+			}
 		} else {
 			# In the traditional
 			# "send lots of email" format,
-- 
EW


^ permalink raw reply related

* Re: [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Junio C Hamano @ 2017-02-12  1:41 UTC (permalink / raw)
  To: Eric Wong; +Cc: git, Jeff King, Vasco Almeida
In-Reply-To: <20170212003432.GA19519@starla>

Eric Wong <e@80x24.org> writes:

> When parsing an mbox, it is possible to get existing In-Reply-To
> and References headers blindly appended into the headers of
> message we generate.   This is probably the wrong thing to do
> and we should prioritize what was given in the command-line,
> cover letter, and previously-sent messages.
>
> One example I've noticed in the wild was:
>
> https://public-inbox.org/git/20161111124541.8216-17-vascomalmeida@sapo.pt/raw
> ---
>  I'm not completely sure this is what Vasco was doing in that
>  message, so it's an RFC for now...

I think it is sensibleto give priority to the --in-reply-to option
given from the command line over the in-file one.  I am not sure if
we want to drop references, though.  Wouldn't it make more sense to
just add what we got from the command line to what we read from the
file?  I dunno.

>  git-send-email.perl | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 068d60b3e6..5ab3d8585c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1543,7 +1543,13 @@ foreach my $t (@files) {
>  			elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) {
>  				push @xh, $_;
>  			}
> -
> +			elsif (/^(?:References|In-Reply-To):/i) {
> +				if (defined $initial_reply_to || $thread) {
> +					warn
> +"Ignoring $_ header in mbox body since it conflicts with\n
> +--in-reply-to and --thread switches\n"
> +				}
> +			}
>  		} else {
>  			# In the traditional
>  			# "send lots of email" format,

^ permalink raw reply

* Re: [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Eric Wong @ 2017-02-12  2:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Vasco Almeida
In-Reply-To: <xmqq60kggp8r.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> > When parsing an mbox, it is possible to get existing In-Reply-To
> > and References headers blindly appended into the headers of
> > message we generate.   This is probably the wrong thing to do
> > and we should prioritize what was given in the command-line,
> > cover letter, and previously-sent messages.
> >
> > One example I've noticed in the wild was:
> >
> > https://public-inbox.org/git/20161111124541.8216-17-vascomalmeida@sapo.pt/raw
> > ---
> >  I'm not completely sure this is what Vasco was doing in that
> >  message, so it's an RFC for now...
> 
> I think it is sensibleto give priority to the --in-reply-to option
> given from the command line over the in-file one.  I am not sure if
> we want to drop references, though.  Wouldn't it make more sense to
> just add what we got from the command line to what we read from the
> file?  I dunno.

You're right, existing References in the bodies should probably
be prepended to current ones, as their order should be
oldest-to-newest.

I'll wait on comments a bit and work on a better version w/ tests
next week.

^ permalink raw reply

* Re: [RFC] send-email: avoid duplicate In-Reply-To and References headers
From: Junio C Hamano @ 2017-02-12  2:51 UTC (permalink / raw)
  To: Eric Wong; +Cc: git, Jeff King, Vasco Almeida
In-Reply-To: <20170212021208.GA16358@starla>

Eric Wong <e@80x24.org> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> I think it is sensibleto give priority to the --in-reply-to option
>> given from the command line over the in-file one.  I am not sure if
>> we want to drop references, though.  Wouldn't it make more sense to
>> just add what we got from the command line to what we read from the
>> file?  I dunno.
>
> You're right, existing References in the bodies should probably
> be prepended to current ones, as their order should be
> oldest-to-newest.

If you were to go that way, it may also make sense to demote the
original In-Reply-To you find in the file to an entry in the
References list, if it does not already appear there.

I didn't check the RFCs, but I think your original motivation is to
ensure that there is not more than one messages to which the current
message is replying to, iow, to ensure that there is only one message
that is In-Reply-To, so appending would not be a good solution there.

^ permalink raw reply

* Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Matthieu Moy @ 2017-02-12  9:48 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, gitster, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <1486752926-12020-3-git-send-email-kannan.siddharth12@gmail.com>

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

>  sha1_name.c              |  5 ++++
>  t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
>  create mode 100755 t/t4214-log-shorthand.sh
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 73a915f..d774e46 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
>  	if (!ret)
>  		return 0;
>  
> +	if (!strcmp(name, "-")) {
> +		name = "@{-1}";
> +		len = 5;
> +	}

One drawback of this approach is that further error messages will be
given from the "@{-1}" string that the user never typed.

After you do that, the existing "turn - into @{-1}" pieces of code
become useless and you should remove it (probably in a further patch).

There are at least:

$ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
builtin/checkout.c:975: if (!strcmp(arg, "-"))
builtin/checkout.c-976-         arg = "@{-1}";
--
builtin/merge.c:1231:   } else if (argc == 1 && !strcmp(argv[0], "-")) {
builtin/merge.c-1232-           argv[0] = "@{-1}";
--
builtin/revert.c:158:           if (!strcmp(argv[1], "-"))
builtin/revert.c-159-                   argv[1] = "@{-1}";
--
builtin/worktree.c:344: if (!strcmp(branch, "-"))
builtin/worktree.c-345-         branch = "@{-1}";

In the final version, obviously the same "refactoring" (specific
command -> git-wide) should be done for documentation (it should be in
this patch to avoid letting not-up-to-date documentation even for a
single commit).

> diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
> new file mode 100755
> index 0000000..dec966c
> --- /dev/null
> +++ b/t/t4214-log-shorthand.sh
> @@ -0,0 +1,73 @@
> +#!/bin/sh
> +
> +test_description='log can show previous branch using shorthand - for @{-1}'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	echo hello >world &&
> +	git add world &&
> +	git commit -m initial &&
> +	echo "hello second time" >>world &&
> +	git add world &&
> +	git commit -m second &&
> +	echo "hello other file" >>planet &&
> +	git add planet &&
> +	git commit -m third &&
> +	echo "hello yet another file" >>city &&
> +	git add city &&
> +	git commit -m fourth
> +'

You may use test_commit to save a few lines of code.

> +test_expect_success '"log -" should work' '
> +	git checkout -b testing-1 master^ &&
> +	git checkout -b testing-2 master~2 &&
> +	git checkout master &&
> +
> +	git log testing-2 >expect &&
> +	git log - >actual &&
> +	test_cmp expect actual
> +'

I'd have split this into a "setup branches" and a '"log -" should work'
test (to actually see where "setup branches" happen in the output, and
to allow running the setup step separately if needed). Not terribly
important.

> +test_expect_success 'symmetric revision range should work when one end is left empty' '
> +	git checkout testing-2 &&
> +	git checkout master &&
> +	git log ...@{-1} > expect.first_empty &&
> +	git log @{-1}... > expect.last_empty &&
> +	git log ...- > actual.first_empty &&
> +	git log -... > actual.last_empty &&

Nitpick: we stick the > and the filename (as you did in most places
already).

It may be worth adding tests for more cases like

* Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.

* -..- -> to make sure you handle the presence of two - properly.

* multiple separate arguments to make sure you handle them all, e.g.
  "git log - -", "git log HEAD -", "git log - HEAD".

The last two may be overkill, but the first one is probably important.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply

* Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Siddharth Kannan @ 2017-02-12 10:42 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <vpqbmu768on.fsf@anie.imag.fr>

Hey Matthieu,
On Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> 
> >  sha1_name.c              |  5 ++++
> >  t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 78 insertions(+)
> >  create mode 100755 t/t4214-log-shorthand.sh
> >
> > diff --git a/sha1_name.c b/sha1_name.c
> > index 73a915f..d774e46 100644
> > --- a/sha1_name.c
> > +++ b/sha1_name.c
> > @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
> >  	if (!ret)
> >  		return 0;
> >  
> > +	if (!strcmp(name, "-")) {
> > +		name = "@{-1}";
> > +		len = 5;
> > +	}
> 
> After you do that, the existing "turn - into @{-1}" pieces of code
> become useless and you should remove it (probably in a further patch).

Yeah, this is currently also implemented in checkout, apart from the
grepped list that you have supplied here. I will find all the
instances, and ensure that they work, and remove them. (This will
require some more digging into the codepath the commands, to ensure
that get_sha1_1 is called somewhere down the line)
> 
> > diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
> > ...
> > +test_expect_success 'setup' '
> > +	echo hello >world &&
> > +	git add world &&
> > +	git commit -m initial &&
> > +	echo "hello second time" >>world &&
> > ...
> 
> You may use test_commit to save a few lines of code.

Oh, yeah! I will use that. I need to work on improving the tests, as
well as adding the documentation.
> 
> > +test_expect_success 'symmetric revision range should work when one end is left empty' '
> > +	git checkout testing-2 &&
> > +	git checkout master &&
> > +	git log ...@{-1} > expect.first_empty &&
> > +	git log @{-1}... > expect.last_empty &&
> > +	git log ...- > actual.first_empty &&
> > +	git log -... > actual.last_empty &&
> 
> Nitpick: we stick the > and the filename (as you did in most places
> already).
Sorry, slipped my mind!
> 
> It may be worth adding tests for more cases like
> 
> * Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.

These do not work right now. The first and last cases here are handled
by peel_onion, if I remember correctly. I have to find out why exactly
these are not working. Thanks for mentioning this!

> 
> * -..- -> to make sure you handle the presence of two - properly.
> 
> * multiple separate arguments to make sure you handle them all, e.g.
>   "git log - -", "git log HEAD -", "git log - HEAD".

Yeah, will add these tests.

> 
> The last two may be overkill, but the first one is probably important.
> 
> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

--
Regards,

Siddharth Kannan.

^ permalink raw reply

* Bug in 'git describe' if I have two tags on the same commit.
From: Istvan Pato @ 2017-02-12 12:15 UTC (permalink / raw)
  To: git

I didn't get back the latest tag by 'git describe --tags --always' if
I have two tags on the same commit.

// repository ppa:git-core/ppa

(master)⚡ % cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.2 LTS"

(master)⚡ % git --version
git version 2.11.0

(master) [1] % git show-ref --tag
76c634390... refs/tags/1.0.0
b77c7cd17... refs/tags/1.1.0
b77c7cd17... refs/tags/1.2.0

(master) % git describe --tags --always
1.1.0-1-ge9e9ced

### Expected: 1.2.0

References:

https://www.kernel.org/pub/software/scm/git/docs/RelNotes-1.7.1.1.txt

* "git describe" did not tie-break tags that point at the same commit
  correctly; newer ones are preferred by paying attention to the
  tagger date now.

http://stackoverflow.com/questions/8089002/git-describe-with-two-tags-on-the-same-commit

Thanks,
Istvan Pato

^ permalink raw reply

* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Siddharth Kannan @ 2017-02-12 12:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com>

On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> 
> > Um, I am sorry, but I feel that decrementing left, and incrementing it again is
> > also confusing.
> 
> Yes, but it is no more confusing than your original "left--".
> ...
> 
>  * If it is an option known to us, handle it and go to the next arg.
> 
>  * If it is an option that we do not understand, stuff it in
>    argv[left++] and go to the next arg.
> 
> Because the second step currently is implemented by calling
> handle_opt(), which not just tells if it is an option we understand
> or not, but also mucks with argv[left++], you need to undo it once
> you start making it possible for a valid "rev" to begin with a dash.
> That is what your left-- was, and that is what "decrement and then
> increment when it turns out it was an unknown option after all" is.

So, our problem here is that the function handle_revision_opt is opaquely also
incrementing "left", which we need to decrement somehow.

Or: we could change the flow of the code so that this incrementing
will happen only when we have decided that the argument is not a
revision.
> 
>  * If it is an option known to us, handle it and go to the next arg.
> 
>  * If it is a rev, handle it, and note that fact in got_rev_arg
>    (this change of order enables you to allow a rev that begins with
>    a dash, which would have been misrecognised as a possible unknown
>    option).
> 
>  * If it looks like an option (i.e. "begins with a dash"), then we
>    already know that it is not something we understand, because the
>    first step would have caught it already.  Stuff it in
>    argv[left++] and go to the next arg.
> 
>  * If it is not a rev and we haven't seen dashdash, verify that it
>    and everything that follows it are pathnames (which is an inexact
>    but a cheap way to avoid ambiguity), make all them the prune_data
>    and conclude.

This "changing the order" gave me the idea to change the flow. I tried to
implement the above steps without touching the function handle_revision_opt. By
inserting the handle_revision_arg call just before calling handle_revision_opt.

The decrementing then incrementing or "left--" things have now been removed.
(But there is still one thing which doesn't look good)

diff --git a/revision.c b/revision.c
index b37dbec..8c0acea 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,11 +2203,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (seen_dashdash)
 		revarg_opt |= REVARG_CANNOT_BE_FILENAME;
 	read_from_stdin = 0;
+
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		int opts;
 		if (*arg == '-') {
-			int opts;
-
 			opts = handle_revision_pseudo_opt(submodule,
 						revs, argc - i, argv + i,
 						&flags);
@@ -2226,7 +2226,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				read_revisions_from_stdin(revs, &prune_data);
 				continue;
 			}
+		}
 
+		if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+			got_rev_arg = 1;
+		else if (*arg == '-') {
 			opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
 			if (opts > 0) {
 				i += opts - 1;
@@ -2234,11 +2238,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
-			continue;
-		}
-
-
-		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+		} else {
 			int j;
 			if (seen_dashdash || *arg == '^')
 				die("bad revision '%s'", arg);
@@ -2255,8 +2255,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			append_prune_data(&prune_data, argv + i);
 			break;
 		}
-		else
-			got_rev_arg = 1;
 	}
 
 	if (prune_data.nr) {


The "if (*arg =='-')" line is repeated. On analysing the resulting
revision.c:setup_revisions function, I feel that the codepath is still as
easily followable as it was earlier, and there is definitely no confusion
because of a mysterious decrement. Also, the repeated condition doesn't make it
any harder (it looks like a useful check because we already know that every
option would start with a "-"). But that's only my opinion, and you definitely
know better.

now the flow is very close to the ideal flow that we prefer:

1. If it is a pseudo_opt or --stdin, handle and go to the next arg
2. If it is a revision, note that in "got_rev_arg", and go to the next arg
3. If it starts with a "-" and is a known option, handle and go to the next arg
4. If it is none of {revision, known-option} and we haven't seen dashdash,
   verify that it and everything that follows it are pathnames (which is an
   inexact but a cheap way to avoid ambiguity), make all them the prune_data and
   conclude.

> But I think the resulting code flow is much closer to the
> above ideal.

(about Junio's version of the patch): Yes, I agree with you on this. It's like
the ideal, but the argv has already been populated, so the only remaining step
is "left++".

> 
> Such a change to handle_revision_opt() unfortunately affects other
> callers of the function, so it may not be worth it.

handle_revision_opt is called once apart from within setup_revisions,
from within revision.c:parse_revision_opt.

If this version is not acceptable, we should either revert back to your version
of the patch with the fixed variable name OR consider re-writing
handle_revision_opt, as per your suggested flow. Note that this will put the
code for "Stuff it in argv[left++]" in every caller.

Thank you for the time you have spent on analysing each version of the patch!

--
Best Regards,

Siddharth Kannan.

^ permalink raw reply related

* Git status performance on PS (command prompt)
From: Mark Gaiser @ 2017-02-12 15:53 UTC (permalink / raw)
  To: git

Hi,

I'm using ZSH with some fancy prompt. In that prompt is also a quick
git status overview (some symbols indicating if the branch is up to
date, if there are changes, etc..

The commands that are being executed to fetch the information:

For the file status:
git status --porcelain

For the repository status:
git status --porcelain -b

On small repositories (or even medium sized ones) this is fast, no
problems there.
But on larger repositories this is notably slow (i'm taking QtBase as
an example now, but the same is true for much of the Qt repositories,
or chromium or even the linux kernel itself). It's no problem if it's
slow when you do "git status" on the command line. That can be
expected to take a little while on large repositories. But in zsh
prompts the call to git isn't asynchronous [1] so any slowdown will be
noticed as the prompt simply doesn't completely show till after the
command.

I did a bit of profiling in git to figure out where this slowdown comes from.
Callgrind tells me that "read_directory_recursive" takes up ~62% of the time.
Within that call the function "last_exclude_matching_from_list" takes
up 49% of the time it takes to run "git status --porcelain -b".

I don't really know how this code is supposed to work (i'm a git user,
not a git developer), so i hope someone might be able to investigate
this further. I can however apply patches to git locally and help out
with testing.

Also, is there perhaps a command out there that might be better suited
for the git status i want to show in the prompt? What i have now is
merely from one of the oh-my-zsh themes.

Best regards,
Mark

p.s. please keep me in cc, i'm not subscribed to this list.

[1] Most prompts don't have async support, but there are prompts that
do provide this: https://github.com/sindresorhus/pure I guess that
would be a better solution for me in the short term. Still, having the
status being made faster would be beneficial.

^ permalink raw reply

* Confusing git messages when disk is full.
From: Jáchym Barvínek @ 2017-02-12 16:37 UTC (permalink / raw)
  To: git

Hello, I would like to report what I consider a bug in git, I hope I'm
doing it the right way.
I was trying to run `git pull` in  my repository and got the following
error: "git pull
Your configuration specifies to merge with the ref 'refs/heads/master'
from the remote, but no such ref was fetched."
Which was very confusing to me, I found some answers to what might be the cause
but none was the right one. The actual cause was that the filesystem
had no more free space.
When I cleaned the space, `git pull` then gave the expected answer
("Already up-to-date.").
I think the message is confusing and git should be able to report to
the user that the cause
is full disk.
Regards, Jáchym Barvínek

^ permalink raw reply

* Re: Git status performance on PS (command prompt)
From: brian m. carlson @ 2017-02-12 16:46 UTC (permalink / raw)
  To: Mark Gaiser; +Cc: git
In-Reply-To: <CAPd6JnGC3yeDo_sB+H+UKvh-PbiC2qySC=EbaRNLbkkwYkJM4A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3011 bytes --]

On Sun, Feb 12, 2017 at 04:53:47PM +0100, Mark Gaiser wrote:
> Hi,
> 
> I'm using ZSH with some fancy prompt. In that prompt is also a quick
> git status overview (some symbols indicating if the branch is up to
> date, if there are changes, etc..
> 
> The commands that are being executed to fetch the information:
> 
> For the file status:
> git status --porcelain
> 
> For the repository status:
> git status --porcelain -b

So your prompt is running git status twice?  Wouldn't it make more sense
to just run it once and do a head -n 1 to filter out the branch when you
need it?  git symbolic-ref HEAD might work as well.

> On small repositories (or even medium sized ones) this is fast, no
> problems there.
> But on larger repositories this is notably slow (i'm taking QtBase as
> an example now, but the same is true for much of the Qt repositories,
> or chromium or even the linux kernel itself). It's no problem if it's
> slow when you do "git status" on the command line. That can be
> expected to take a little while on large repositories. But in zsh
> prompts the call to git isn't asynchronous [1] so any slowdown will be
> noticed as the prompt simply doesn't completely show till after the
> command.
> 
> I did a bit of profiling in git to figure out where this slowdown comes from.
> Callgrind tells me that "read_directory_recursive" takes up ~62% of the time.
> Within that call the function "last_exclude_matching_from_list" takes
> up 49% of the time it takes to run "git status --porcelain -b".

This isn't entirely surprising.  When git status runs, it checks all the
files to see if the stat information is changed, and if so, updates the
status.  last_exclude_matching_from_list determines if the files are
ignored, which is also necessary to ensure that only the proper files
are listed.

That doesn't mean that there couldn't be some improvements there,
though.

> I don't really know how this code is supposed to work (i'm a git user,
> not a git developer), so i hope someone might be able to investigate
> this further. I can however apply patches to git locally and help out
> with testing.
> 
> Also, is there perhaps a command out there that might be better suited
> for the git status i want to show in the prompt? What i have now is
> merely from one of the oh-my-zsh themes.

I typically use the vcs_info support built into zsh[0], although that
can be very slow on my phone.  It performs adequately on most
repositories on my laptop and server machines, though, including on the
Linux kernel repo and our 2.4 and 9 GB repos at work.

I don't know that it has all the functionality that you need, as git
status provides finer-grained data, but it might.

[0] Example at https://github.com/bk2204/homedir/blob/master/.zsh/prompt_bmc_setup
-- 
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: What's cooking in git.git (Feb 2017, #03; Fri, 10)
From: Vegard Nossum @ 2017-02-12 18:32 UTC (permalink / raw)
  To: Junio C Hamano, René Scharfe; +Cc: git
In-Reply-To: <xmqq8tpdifoa.fsf@gitster.mtv.corp.google.com>

On 11/02/2017 04:12, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 10.02.2017 um 23:24 schrieb Junio C Hamano:
>>> * vn/xdiff-func-context (2017-01-15) 1 commit
>>>  - xdiff -W: relax end-of-file function detection
>>>
>>>  "git diff -W" has been taught to handle the case where a new
>>>  function is added at the end of the file better.
>>>
>>>  Will hold.
>>>  Discussion on an follow-up change to go back from the line that
>>>  matches the funcline to show comments before the function
>>>  definition has not resulted in an actionable conclusion.
>>
>> This one is a bug fix and can be merged already IMHO.
>
> Absolutely.  I was just waiting if the follow-up discussion would
> easily and quickly lead to another patch, forgot about what exactly
> I was waiting for (i.e. the gravity of not having the follow-up),
> and have left it in "Will hold" status forever.

I said I would resubmit the patches with more config options and more
command-line arguments (to avoid potentially breaking backwards
compatibility), but IIRC the response seemed to be "preceding blank line
heuristic is good enough" and "why bother", so I ended up not not
resubmitting anything.

>
> Let's merge it to 'next' and then decide if we want to also merge it
> to 'master' before the final.  The above step alone is a lot less
> contriversial and tricky bugfix.

Thanks,


Vegard

^ permalink raw reply

* [PATCH 0/3] prepare for a rev/range that begins with a dash
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
  To: git; +Cc: Siddharth Kannan
In-Reply-To: <xmqqa89sguu4.fsf@gitster.mtv.corp.google.com>

It turns out that telling handle_revision_opt() not to molest argv[left++]
does not have heavy fallout.

Junio C Hamano (3):
  handle_revision_opt(): do not update argv[left++] with an unknown arg
  setup_revisions(): swap if/else bodies to make the next step more readable
  setup_revisions(): allow a rev that begins with a dash

 revision.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
2.12.0-rc1-212-ga9adfb24fa


^ permalink raw reply

* [PATCH 1/3] handle_revision_opt(): do not update argv[left++] with an unknown arg
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
  To: git; +Cc: Siddharth Kannan
In-Reply-To: <20170212184132.12375-1-gitster@pobox.com>

In future steps, we will make it possible for a rev or a revision
range (i.e. what is understood by handle_revision_arg() helper) to
begin with a dash.  The setup_revisions() function however currently
considers anything that begins with a dash to be:

 - an option it itself understands and handles (some take effect by
   setting fields in the revision structure, some others are left
   in the argv[left++] to be handled in later steps);
 - an option handle_revision_opt() understands and tells us to skip;
 - an option handle_revision_opt() found to be incorrect; or
 - an option handle_revision_opt() did not understand, which is
   stuffed in argv[left++].

and does not give a chance to handle_revision_arg() to inspect it.
The handle_revision_opt() function returns a positive count, a
negative count or zero to allow the caller to tell the latter three
cases apart.  A rev that begins with a dash would be thrown into the
last category.

Teach handle_revision_opt() not to touch argv[left++] in the last
case.  Because the other one among the two callers of this function
immediately errors out with the usage string when it returns zero
(i.e. the last case above), there is no negative effect to that
caller.

In setup_revisions(), which is the other caller of this function,
we need to stuff the unknown arg to argv[left++] in this case, to
preserve the current behaviour.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec378..4f46b8ba81 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->ignore_missing = 1;
 	} else {
 		int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
-		if (!opts)
-			unkv[(*unkc)++] = arg;
 		return opts;
 	}
 	if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
+			/* arg looks like an opt but something we do not recognise. */
+			argv[left++] = arg;
 			continue;
 		}
 
-- 
2.12.0-rc1-212-ga9adfb24fa


^ permalink raw reply related

* [PATCH 2/3] setup_revisions(): swap if/else bodies to make the next step more readable
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
  To: git; +Cc: Siddharth Kannan
In-Reply-To: <20170212184132.12375-1-gitster@pobox.com>

Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do A", to make the change in
the the next step easier to read.  

No behaviour change is intended in this step.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 4f46b8ba81..eccf1ab695 100644
--- a/revision.c
+++ b/revision.c
@@ -2237,8 +2237,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			continue;
 		}
 
-
-		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+		if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
+			got_rev_arg = 1;
+		} else {
 			int j;
 			if (seen_dashdash || *arg == '^')
 				die("bad revision '%s'", arg);
@@ -2255,8 +2256,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			append_prune_data(&prune_data, argv + i);
 			break;
 		}
-		else
-			got_rev_arg = 1;
 	}
 
 	if (prune_data.nr) {
-- 
2.12.0-rc1-212-ga9adfb24fa


^ permalink raw reply related

* [PATCH 3/3] setup_revisions(): allow a rev that begins with a dash
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
  To: git; +Cc: Siddharth Kannan
In-Reply-To: <20170212184132.12375-1-gitster@pobox.com>

Now all the preparatory pieces are in place, it is a matter of
handling a truly unknown option _after_ handle_revision_arg()
decides that arg is not a rev.  

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 We _could_ do without a new variable maybe_opt and instead check if
 arg begins with a dash one more time, but it is cleaner to do it
 the way this patch does to avoid writing the same check twice.  We
 may be hit with a desire similar to but an opposite of the current
 topic (which wants to allow a rev that begins with a dash), to
 start allowing an option that does not begin with a dash someday.

 revision.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index eccf1ab695..0f772ba73d 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		int maybe_opt = 0;
+
 		if (*arg == '-') {
 			int opts;
 
@@ -2232,13 +2234,20 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
-			/* arg looks like an opt but something we do not recognise. */
-			argv[left++] = arg;
-			continue;
+			/*
+			 * arg looks like an opt but something we do not recognise.
+			 * It may be a rev that begins with a dash; fall through to
+			 * let handle_revision_arg() have a say in this.
+			 */
+			maybe_opt = 1;
 		}
 
 		if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
 			got_rev_arg = 1;
+		} else if (maybe_opt) {
+			/* it turns out that it is not a rev after all */
+			argv[left++] = arg;
+			continue;
 		} else {
 			int j;
 			if (seen_dashdash || *arg == '^')
-- 
2.12.0-rc1-212-ga9adfb24fa


^ permalink raw reply related

* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Junio C Hamano @ 2017-02-12 18:56 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <20170212123630.GA20872@ubuntu-512mb-blr1-01.localdomain>

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> This "changing the order" gave me the idea to change the flow. I tried to
> implement the above steps without touching the function handle_revision_opt. By
> inserting the handle_revision_arg call just before calling handle_revision_opt.

Changing the order is changing the order of the function calls,
i.e. changing the flow.  So at the idea level we are on the same
page.

I was shooting for not having to duplicate calls to
handle_revision_arg().  

>> But I think the resulting code flow is much closer to the
>> above ideal.
>
> (about Junio's version of the patch): Yes, I agree with you on this. It's like
> the ideal, but the argv has already been populated, so the only remaining step
> is "left++".
>> 
>> Such a change to handle_revision_opt() unfortunately affects other
>> callers of the function, so it may not be worth it.

See the 3-patch series I just sent out.  I didn't think it through
very carefully (especially the error message the other caller
produces), but the whole thing _smells_ correct to me.

^ permalink raw reply

* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-12 19:30 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Matthieu Moy
In-Reply-To: <20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net>

[+cc Matthieu Moy as author of a patch mentioned below]

On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:37PM +0000, Thomas Gummerer wrote:
> 
> > Thanks Junio for the review in the previous round.
> > 
> > Changes since v2:
> > 
> > - $IFS should now be supported by using "$@" everywhere instead of using
> >   a $files variable.
> > - Added a new patch showing the old behaviour of git stash create is
> >   preserved.
> > - Rephrased the documentation
> > - Simplified the option parsing in save_stash, by leaving the
> >   actual parsing to push_stash instead.
> 
> Overall, I like the direction this is heading. I raised a few issues,
> the most major of which is whether we want to allow the minor regression
> in "git stash create -m foo".
> 
> This also makes "git stash push -p <pathspec...>" work, which is good. I
> don't think "git stash -p <pathspec...>" does, though. I _think_ it
> would be trivial to do on top, since we already consider that case an
> error. That's somewhat outside the scope of your series, so I won't
> complain (too much :) ) if you don't dig into it, but it might just be
> trivial on top.

Hmm good idea, I think it would indeed be nice to add that.  Theres a
few things to consider though.  First, we need to switch git stash
without arguments over to use git stash push internally.  This does
introduce a slight regression as we currently allow git stash save --
-fmessage, (only messages starting with - are allowed).  I think that
regression would be acceptable however.

The other question is when we should allow the pathspec argument.
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options") made sure that all option arguments are treated the same.  I
think we could use the usual disambiguation of -- to allow pathspecs
after that, so stash -p wouldn't be treated specially, otherwise the
rules could get a bit confusing again.

The other question is how we would deal with the -m flag for
specifying a stash message.  I think we could special case it, but
that would allow users to do things such as git stash -m apply, which
would make the interface a bit more error prone.  So I'm leaning
towards disallowing that for now.

> A few other random bits I noticed while playing with the new code:
> 
>   $ git init
>   $ echo content >file && git add file && git commit -m file
>   $ echo change >file
> 
>   $ git stash push -p not-file
>   No changes.
>   No changes selected
> 
> Probably only one of those is necessary. :)
> 
> Let's keep trying a few things:
> 
>   $ git stash push not-file
>   Saved working directory and index state WIP on master: 5d5f951 file
>   Unstaged changes after reset:
>   M	file
>   M	file
> 
> The unstaged change is mentioned twice, which is weird. But weirder
> still is that we created a stash at all, as it's empty. Why didn't we
> hit the "no changes selected" path?
> 
> And one more:
> 
>   $ echo foo >untracked
>   $ git stash push untracked
>   Saved working directory and index state WIP on master: 5d5f951 file
>   Unstaged changes after reset:
>   M	file
>   M	file
>   Removing untracked
> 
> We removed the untracked file, even though it wasn't actually stashed! I
> thought at first this was because it was mentioned as a pathspec, but it
> seems to happen even with a different name:
> 
>   $ echo foo >untracked
>   $ git stash push does-not-exist
>   ...
>   Removing untracked
> 
> That seems dangerous.

Ouch, yeah this is clearly not good.  I'll fix this, and add tests for
these cases.

> -Peff

^ permalink raw reply

* [git-gui] Amending doesn't preserve timestamp
From: Juraj @ 2017-02-12 20:50 UTC (permalink / raw)
  To: git

I've just noticed that amending a commit from git-gui uses the time of
amending as the new timestamp of the commit, whereas git commit
--amend preserves the original timestamp. Maybe the two should work
the same, whatever it is decided to be the standard behavior.

Juraj

^ permalink raw reply

* Re: [PATCH] fetch: print an error when declining to request an unadvertised object
From: Matt McCutchen @ 2017-02-12 21:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwpcxlwpv.fsf@gitster.mtv.corp.google.com>

On Fri, 2017-02-10 at 10:36 -0800, Junio C Hamano wrote:
> > 
> There is this piece of code near the end of builtin/fetch-pack.c:
> 
> [...]
> 
> that happens before the command shows the list of fetched refs, and
> this code is prepared to inspect what happend to the requests it (in
> response to the user request) made to the underlying fetch
> machinery, and issue the error message.
> If you change your command line to "git fetch-pack REMOTE SHA1", you
> do see an error from the above.

Yes, "error: no such remote ref NNNN", which at least makes clear that
the operation didn't work, though it would be nice to give a more
specific error message.

> This all happens in transport.c::fetch_refs_via_pack().
> I think that function is a much better place to error or die than
> filter_refs().

I confirmed that checking the sought refs there works.  However, in
filter_refs, it's easy to give a more specific error message that the
server doesn't allow requests for unadvertised objects, and that code
works for "git fetch-pack" too.  To do the same in fetch_refs_via_pack,
we'd have to duplicate a few lines of code from filter_refs and expose
the allow_unadvertised_object_request variable, or just set a flag on
the "struct ref" in filter_refs and check it in fetch_refs_via_pack.

What do you think?  Do you not care about having a more specific error,
in which case I can copy the code from builtin/fetch-pack.c to
fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
the flag?  Or what?

Matt

^ permalink raw reply

* Re: [git-gui] Amending doesn't preserve timestamp
From: Igor Djordjevic BugA @ 2017-02-12 21:33 UTC (permalink / raw)
  To: Juraj, git
In-Reply-To: <CAEPqvoxM9_Ku-1YgwNiqearUBaaYbAJcehuSwZYNNfJLQNH1_g@mail.gmail.com>

On 12/02/2017 21:50, Juraj wrote:
> I've just noticed that amending a commit from git-gui uses the time of
> amending as the new timestamp of the commit, whereas git commit
> --amend preserves the original timestamp. Maybe the two should work
> the same, whatever it is decided to be the standard behavior.
> 
> Juraj
> 

Hi Juraj,

Just to report that it seems to work as expected on my end...? Amending
through both command line and git-gui preserves author date and updates
commiter date.

git version 2.11.1.windows.1
git-gui version 0.21.GITGUI
Tcl/Tk version 8.6.6

Regards, BugA

^ permalink raw reply

* Re: [git-gui] Amending doesn't preserve timestamp
From: Juraj @ 2017-02-12 21:40 UTC (permalink / raw)
  To: Igor Djordjevic BugA; +Cc: git@vger.kernel.org
In-Reply-To: <8a1179dfbb7743b6b8c23570306120b7@MAIL.fer.hr>

Hi Igor,

I forgot to write the version I'm using. It's on Ubuntu 16.04, git-gui
package version 1:2.7.4-0ubuntu1 (--version: git-gui version 0.20.0),
git version 2.7.4, tcl and tk 8.6.0+9. Perhaps it got fixed in a newer
version, in that case, my bad for not checking before posting.

Thanks,
Juraj

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox