* Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently
From: Stefan Beller @ 2016-09-04 5:31 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git@vger.kernel.org
In-Reply-To: <eb88af2c-d7b1-295e-5f23-a85045bde753@gmail.com>
On Sat, Sep 3, 2016 at 5:25 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 03.09.2016 o 05:31, Stefan Beller pisze:
>
>> When moving code (e.g. a function is moved to another part of the file or
>> to a different file), the review process is different than reviewing new
>> code. When reviewing moved code we are only interested in the diff as
>> where there are differences in the moved code, e.g. namespace changes.
>>
>> However the inner part of these moved texts should not change.
>> To aid a developer reviewing such code, emit it with a different prefix
>> than the usual +,- to indicate it is overlapping code.
>
> What would be this different prefix?
I will discard the part of the different prefix as the design of 2/2
will change.
>
>
> Side note: I wonder if the cousin of unified diff, namely context diff[1],
> is something that we can and should support.
>
> [1]: https://www.gnu.org/software/diffutils/manual/html_node/Context-Format.html
> https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Context.html
>
> *** lao 2002-02-21 23:30:39.942229878 -0800
> --- tzu 2002-02-21 23:30:50.442260588 -0800
> ***************
> *** 1,7 ****
> - The Way that can be told of is not the eternal Way;
> - The name that can be named is not the eternal name.
> The Nameless is the origin of Heaven and Earth;
> ! The Named is the mother of all things.
> Therefore let there always be non-being,
> so we may see their subtlety,
> And let there always be being,
> --- 1,6 ----
> The Nameless is the origin of Heaven and Earth;
> ! The named is the mother of all things.
> !
So the line moved here?
Is it intentional that the line differs though?
(capitalisation of 'named")
Not sure I can read this diff correctly.
I think for this small side project I'd rather want
to 'just' support colors of moved code;)
> Therefore let there always be non-being,
> so we may see their subtlety,
> And let there always be being,
> ***************
> *** 9,11 ****
> --- 8,13 ----
> The two are the same,
> But after they are produced,
> they have different names.
> + They both may be called deep and profound.
> + Deeper and more profound,
> + The door of all subtleties!
^ permalink raw reply
* Re: [RFC/PATCH 0/2] Color moved code differently
From: Junio C Hamano @ 2016-09-04 6:41 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kYGnkcOxviukj9a8gyaERip5aunXcvsdH-UpBCb=vrVeQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> * On 2/2, doing it at xdiff.c level may be limiting this good idea
>> to flourish to its full potential, as the interface is fed only
>> one diff_filepair at a time.
>
> I realized that after I implemented it. I agree we would want to have
> it function cross file.
>
> So from my current understanding of the code,
> * diffcore_std would call a new function diffcore_detect_moved(void)
> just before diffcore_apply_filter is called.
> * The new function diffcore_detect_moved would then check if the
> diff is a valid textual diff (i.e. real files, not submodules, but
> deletion/creation of one file is allowed)
> If so we generate the diff internally and as in 2/2 would
> hash all added/removed lines with context and store it.
I do not think you should step outside diff_flush(). Only when
producing textual diff, you would have to run the textual diff
twice by going over the q twice:
* The first pass would run diff_flush_patch(), which would call
into xdiff the usual way, but the callback from xdiff would
capture the removed lines and the added lines without making any
output.
* The second pass would run diff_flush_patch(), but the callback
from xdiff would be called with additional information, namely,
the removed and the added lines captured in the first pass.
* I suspect that the fn_out_consume() function that is used for a
normal case (i.e. when we are not doing this more expensive
"moved to/moved from" coloring) can be used for the second pass
above (i.e. the "priv" aka "ecbdata" may need to be extended so
that it can tell which mode of operation it is asked to perform),
but if there is not enough similarity between the second pass of
this "moved from/moved to" mode and the normal mode of output, it
is also OK to have two different callback functions, i.e. the
original one to be used in the normal mode, the second one that
knows the "these are moved without modification" coloring. The
callback for the first pass is sufficiently different and I think
it is better to invent a new callback function to be used in the
first pass, instead of reusing fn_out_consume().
The fn_out_consume() function working in the "second pass of
moved from/moved to mode" would inspect line[] and see if it is
an added or a removed line, and then:
- if it is an added line, and it appears as a removed line
elsewhere in the patchset (you obtained the information in the
first pass), you show it as "this was moved from elsewhere".
- if it is a removed line, and it appears as an added line
elsewhere in the patchset (you obtained the information in the
first pass), you show it as "this was moved to elsewhere".
Or something like that.
^ permalink raw reply
* Re: [PATCH 9/9] rebase -i: rearrange fixup/squash lines using the rebase--helper
From: Johannes Schindelin @ 2016-09-04 6:47 UTC (permalink / raw)
To: Josh Triplett; +Cc: git, Junio C Hamano
In-Reply-To: <20160903180344.truur5ey6j6ah2wh@x>
Hi Josh,
On Sat, 3 Sep 2016, Josh Triplett wrote:
> On Fri, Sep 02, 2016 at 06:23:42PM +0200, Johannes Schindelin wrote:
> > Let's reimplement this with linear complexity (using a hash map to
> > match the commits' subject lines) for the common case; Sadly, the
> > fixup/squash feature's design neglected performance considerations,
> > allowing arbitrary prefixes (read: `fixup! hell` will match the
> > commit subject `hello world`), which means that we are stuck with
> > quadratic performance in the worst case.
>
> If the performance of that case matters enough, we can do better than
> quadratic complexity: maintain a trie of the subjects, allowing prefix
> lookups. (Or hash all the prefixes, which you can do in linear time on
> a string: hash next char, save hash, repeat.) However, that would
> pessimize the normal case of either a complete subject or a sha1, due to
> the extra time taken constructing the data structure. Probably not
> worth it, if you assume that most "fixup!" subjects come from `git
> commit --fixup` or similar automated means.
Right. My reaction to finding our that subject prefixes were allowed, too,
was "WTF?". And then: who uses that? And then: that's gonna hurt
performance! And then: but I can optimize for the common case!
The point is: only when people specify a strict prefix will the
performance be hurt. Meaning that the performance is linear in the most
common cases.
That is good enough for me, and probably good enough for the vast majority
of the users. If it ain't broke, don't fix it.
In the case that somebody needs strict prefixes to be handled more
efficiently, which I do not expect, the "hash all prefixes" approach may
work well, but it would slow down the common case, so I'd suggest doing
that only as a fallback (i.e. if a fixup! could not be matched up, fall
back to hashing the prefixes, re-hashing the commit subjects that were
already seen so far). If this needs to be implemented at all, I would also
suggest that the person in need of that improvement also needs to take
charge of this: I will not spend more time thinking about this.
Ciao,
Johannes
^ permalink raw reply
* Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently
From: Junio C Hamano @ 2016-09-04 6:48 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Stefan Beller, git
In-Reply-To: <eb88af2c-d7b1-295e-5f23-a85045bde753@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> Side note: I wonder if the cousin of unified diff, namely context diff[1],
> is something that we can and should support.
Yes, the lack of support for the copied context (instead of the
unified context) diff format has bugged me over the years. Reading
copied context diff format however is rapidly becoming a lost art,
unfortunately, partly due to our popularity, and the need for the
format has become a lessor issue.
I wonder if a pair of a pre-processor (on the input side, before
running 'git apply') and a post-processor (on the output side,
munging the output from 'git show/diff') would be sufficient.
In any case, I agree with you that it has nothing to do with what
Stefan is doing here.
^ permalink raw reply
* Re: [PATCH 1/6] git-gui: The term unified for remote in Japanese
From: Junio C Hamano @ 2016-09-04 6:54 UTC (permalink / raw)
To: Satoshi Yasushima; +Cc: git, Pat Thoyts
In-Reply-To: <1472913822-9088-1-git-send-email-s.yasushima@gmail.com>
Satoshi Yasushima <s.yasushima@gmail.com> writes:
> Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
> ---
I couldn't quite read/parse the title, but luckily I read Japanese ;-)
You saw different Japanese words used to translate the same original
word "remote" in different message strings, and you chose one of
them and use it everywhere. And you did the same for "blame" in
your patch 2/6.
I would have described them like so:
Subject: git-gui: consistently use the same word for "remote" in Japanese
Subject: git-gui: consistently use the same word for "blame" in Japanese
Thanks.
^ permalink raw reply
* Re: [PATCH] stash: allow ref of a stash by index
From: Junio C Hamano @ 2016-09-04 7:01 UTC (permalink / raw)
To: Jeff King
Cc: Aaron M Watson, git, Jon Seymour, David Caldwell,
Øystein Walle, Ævar Arnfjörð Bjarmason,
David Aguilar, Alex Henrie
In-Reply-To: <20160904015209.ba6arov46ntr2ouq@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
>> index 92df596..af11cff 100644
>> --- a/Documentation/git-stash.txt
>> +++ b/Documentation/git-stash.txt
>> @@ -35,11 +35,12 @@ A stash is by default listed as "WIP on
>> 'branchname' ...", but
>> you can give a more descriptive message on the command line when
>> you create one.
>>
>> -The latest stash you created is stored in `refs/stash`; older
>> -stashes are found in the reflog of this reference and can be named using
>> -the usual reflog syntax (e.g. `stash@{0}` is the most recently
>> -created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`
>> -is also possible).
>> +The latest stash you created is stored in `refs/stash`; older stashes
>> +are found in the reflog of this reference and can be named using the
>> +usual reflog syntax (e.g. `stash@{0}` is the most recently created
>> +stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also
>> +possible). Stashes may also be references by specifying just the stash
>> +index (e.g. the integer `n` is equivalent to `stash@{n}`).
>
> Yay, a documentation update. Should it be s/references/referenced/ in
> the second-to-last line?
This seems whitespace damaged, though. I see a few at the
beginning of lines.
Also, Aaron, next time please refrain from reflowing the paragraph
unnecessarily. I am guessing that you only added one sentence at
the end of an existing paragraph, and such a patch should clearly
show that the only change it did is to append at the end. Reflowing
will force reviewers to compare the preimage and postimage word by
word to spot what other things were changed.
> So I don't think this is technically a regression in any
> currently-functioning behavior, but it seems like a step in the wrong
> direction to add yet another layer of blind parsing.
Yes. I agree that the implementation of this patch goes in the
wrong direction, even though it means well.
>> diff --git a/t/t3907-stash-index.sh b/t/t3907-stash-index.sh
>> new file mode 100755
>> index 0000000..72a1838
>> --- /dev/null
>> +++ b/t/t3907-stash-index.sh
>
> Double yay, tests.
>
> Do we really need a whole new script for this, though? There are already
> "stash show" tests in t3903. We should be able to repeat one of them
> using "2" instead of "stash@{2}" (for example).
Yes, it seems a lot better direction to go. The existing script
t3903 may want to see a bit of modernization clean-up before that to
happen, though.
Thanks for a review.
^ permalink raw reply
* Re: [PATCH] stash: allow ref of a stash by index
From: Johannes Schindelin @ 2016-09-04 7:21 UTC (permalink / raw)
To: Jeff King
Cc: Aaron M Watson, git, Jon Seymour, David Caldwell,
Øystein Walle, Ævar Arnfjörð Bjarmason,
David Aguilar, Alex Henrie
In-Reply-To: <20160904015209.ba6arov46ntr2ouq@sigill.intra.peff.net>
Hi,
On Sat, 3 Sep 2016, Jeff King wrote:
> On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote:
>
> > Allows stashes to be referenced by index only. Instead of referencing
> > "stash@{n}" explicitly, it can simply be referenced as "n".
>
> This says "what" but not "why". I assume it is "because the former is
> more annoying to type".
>
> Are there any backwards-compatibility issues you can think of?
>
> I think that "123456" could be a sha1, but I do not see much point in
> referencing a sha1 as the argument of "stash show". And it looks like
> this code path is called only from is_stash_like(), so presumably the
> same logic would apply to other callers.
Maybe we could make it unambiguous, e.g. by using #<n> instead: #123456
cannot refer to a SHA-1.
But then, '#' are comment-starting in shells, so they would have to by
escaped. Maybe the best option would be to introduce a -n <n> option,
with the shortcut -<n> thanks to e0319ff (parseopt: add
OPT_NUMBER_CALLBACK, 2009-05-07).
Ciao,
Johannes
^ permalink raw reply
* Re: Fixup of a fixup not working right
From: Johannes Schindelin @ 2016-09-04 7:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Philip Oakley, Robert Dailey, Git
In-Reply-To: <xmqq60qdsoxj.fsf@gitster.mtv.corp.google.com>
Hi Junio & Philip,
On Fri, 2 Sep 2016, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
> > As I understand this it's implied by design. The issue is that the
> > rebase is looking for that named commit within its current rebase
> > range, and can't find it, so ignores it.
> >
> > There is a separate issue that all the fixup! fixup! messages are
> > essentially treated as being concatenations of the original fixup!, no
> > matter how many time the fiup is present.
>
> They can be handled separately, but they come from the same "design"
> that could be improved. When the "original" is not in the range to
> be rebased for whatever reason (including the most likely one, i.e.
> it has already graduated to become part of the public history), the
> best thing the user could do at that point may be, as you suggested
> to Robert in your message, to turn the "fixup! original" that did
> not make in time before "original" hit the public record into a
> standalone "fix original" follow-up change, and then to squash
> subsequent "fixup! fixup! original" (and other "fixup! original",
> too) into that commit. And a good direction forward may be to see
> if "rebase -i" can be taught to be more helpful for the user who
> wants to do that.
>
> Perhaps a change like this to "rebase -i":
>
> - The search for "original" when handling "pick fixup! original",
> when it does not find "original", could turn it into "reword
> fixup! original" without changing its position in the instruction
> sequence.
>
> - The search for "original" when handling "pick fixup! fixup!
> original", could be (probably unconditionally) changed to look
> for "fixup! original" to amend, instead of looking for "original"
> as the current code (this is your "separate issue"). The same
> "if the commit to be amended is not found, turn it into reword"
> rule from the above applies to this one, too.
>
> may be an improvement?
I would be *very* careful with such a change.
The point is that fixup! messages are really special, and are always
intended to be squashed into the referenced commit *before* the latter
hits `master`.
The entire design of the fixup! feature (using the commit subject as
identifier, which is only "unique enough" in a topic branch that is still
being developed) points to that.
I am fairly certain that we would run into tons of problems if we diluted
the concept of fixup! commits by changing the design so that fixup!
commits all of a sudden become their own, "real" commits that can be fixed
up themselves, as much of the current code simply does not expect that.
In short, I am opposed to this change.
And even if I am overruled, I would strongly suggest to implement this on
top of my rebase-i-extra branch (i.e. in the rebase--helper instead of the
shell script) to avoid double churn.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] compat: move strdup(3) replacement to its own file
From: Johannes Schindelin @ 2016-09-04 7:46 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <89725a44-8afa-1eb1-732a-23b1e264616c@web.de>
[-- Attachment #1: Type: text/plain, Size: 958 bytes --]
Hi René,
I imagine you Cc:ed me because the nedmalloc stuff came in via the Windows
port, contributed by Marius (who is no longer active on the Git project
because it works well enough for him)?
On Sat, 3 Sep 2016, René Scharfe wrote:
> Move our implementation of strdup(3) out of compat/nedmalloc/ and allow
> it to be used independently from USE_NED_ALLOCATOR. This reduces the
> difference of our copy of nedmalloc from the original, making it easier
> to update, and allows for easier testing and reusing of our version of
> strdup().
I would like to suggest an additional paragraph to explain why we do not
need to #include "git-compat-util.h" in nedmalloc from now on:
Please note that nedmalloc never actually uses strdup() itself,
therefore we need not enforce gitstrdup() usage in nedmalloc.c.
The patch looks quite straight-forward otherwise. (Junio, if you want an
ACK from me, you hereby got it).
Thanks!
Dscho
^ permalink raw reply
* Re: [PATCH] introduce hex2chr() for converting two hexadecimal digits to a character
From: Johannes Schindelin @ 2016-09-04 7:49 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <ac454d89-e1cc-083e-5cea-fc9751de9a0f@web.de>
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
Hi René,
On Sat, 3 Sep 2016, René Scharfe wrote:
> Add and use a helper function that decodes the char value of two
> hexadecimal digits. It returns a negative number on error, avoids
> running over the end of the given string and doesn't shift negative
> values.
I like it! Maybe stress a little bit why this is a good change? Like, DRY
up code, makes the code safer (bt avoiding shifting negative values)?
> 6 files changed, 21 insertions(+), 78 deletions(-)
Very, very nice!
Thanks,
Dscho
^ permalink raw reply
* Re: git add -p—splitting hunks, limit is too large
From: Johannes Schindelin @ 2016-09-04 8:01 UTC (permalink / raw)
To: Beau Martinez; +Cc: git
In-Reply-To: <CAEtDOuV+0CKRSu9mJa27+yQKJ-QRyNmwecrZKaaCh0St+VagZg@mail.gmail.com>
Hi Beau,
On Fri, 2 Sep 2016, Beau Martinez wrote:
> Hi git developers and community,
>
> I'd like to inquire as to why `git add -p` can only split hunks so
> much. The limit is too large; why can't you split until each hunk is
> only a line? I often have to run `edit` and split them manually
> myself.
Please note that git gui lets you stage lines individually.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
From: Johannes Schindelin @ 2016-09-04 7:55 UTC (permalink / raw)
To: Matthieu Moy
Cc: Ævar Arnfjörð Bjarmason, Jeff King, Git,
Junio C Hamano
In-Reply-To: <vpq8tva1cou.fsf@anie.imag.fr>
[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]
Hi,
On Fri, 2 Sep 2016, Matthieu Moy wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Hi Ævar,
> >
> > On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >>
> >> > The biggest problem with Strawberry Perl is that it is virtually
> >> > impossible to build the Subversion-Perl bindings using the Git for
> >> > Windows SDK when using Strawberry Perl.
> >> >
> >> > Which pretty much precludes it from being used in Git for Windows.
> >> >
> >> > And then there are the path issues... Git's Perl scripts are pretty
> >> > certain that they live in a POSIX-y environment. Which MSYS2 Perl
> >> > provides. Strawberry Perl not.
> >>
> >> This might be me missing the point, and I'm really just trying to be
> >> helpful here and make "prove" work for you because it's awesome, but
> >> as far as just you running this for development purposes does any of
> >> this SVN stuff matter? I.e. you can build Git itself not with
> >> Strawberry, but just use Strawberry to get a working copy of "prove".
> >
> > Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
> > take a substantial time to run). So if I run the test suite, I better do
> > it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
> > might just as well not bother with running the entire test suite...
>
> Maybe something like
>
> \path\to\strawberry-perl\perl.exe \path\to\prove ...
>
> without changing the PATH would work. I wouldn't call that convenient
> though.
Wouldn't Perl-specific environment variables set by Strawberry Perl (such
as PERL_PATH bleed through to the spawned child processes?
We're dancing around the issue, really. Rather than piling workaround on
workaround with no end in sight, I think it is time to admit that using
prove(1) on Windows is just not a good solution for the problem to re-run
failed tests.
Ciao,
Johannes
^ permalink raw reply
* Re: git add -p—splitting hunks, limit is too large
From: Johannes Schindelin @ 2016-09-04 8:10 UTC (permalink / raw)
To: Jeff King; +Cc: Beau Martinez, git
In-Reply-To: <20160902191425.ki7nfhlqgnihoqpw@sigill.intra.peff.net>
Hi Peff,
On Fri, 2 Sep 2016, Jeff King wrote:
> The good news (or maybe the bad) is that "add -p" is implemented
> entirely in Perl. :)
Yeah, you would definitely not call this "good news" if you were in my
shoes.
There is no question that it has grown way too unwieldy and that we (once
again, as with so many other scripts) missed the boat to convert it to C.
Scripting is nice for prototyping. But it comes at a high
portability/performance cost if taken too far. And we took it way too far.
Just look at all of those 1667 lines of git-add--interactive and weep. So
many things reimplemented in Perl instead of reusing functions in
libgit.a (or introducing them, making them usable from other parts of
Git). Wasted time is what I see there.
The worst part is that it completely violates our original "Unix
philosophy" of implementing the business logic in C and combinig it using
light-weight scripting.
And of course now the script is *so large* that nobody wants to undertake
the task of porting it to C.
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC/PATCH 0/2] Color moved code differently
From: Stefan Beller @ 2016-09-04 8:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqpookqi8k.fsf@gitster.mtv.corp.google.com>
On Sat, Sep 3, 2016 at 11:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> * On 2/2, doing it at xdiff.c level may be limiting this good idea
>>> to flourish to its full potential, as the interface is fed only
>>> one diff_filepair at a time.
>>
>> I realized that after I implemented it. I agree we would want to have
>> it function cross file.
>>
>> So from my current understanding of the code,
>> * diffcore_std would call a new function diffcore_detect_moved(void)
>> just before diffcore_apply_filter is called.
>> * The new function diffcore_detect_moved would then check if the
>> diff is a valid textual diff (i.e. real files, not submodules, but
>> deletion/creation of one file is allowed)
>> If so we generate the diff internally and as in 2/2 would
>> hash all added/removed lines with context and store it.
>
> I do not think you should step outside diff_flush().
After looking further into it, I think we need to step outside
or at the very least deep down in the xdl cellar from there,
because we need the context around the line both in the
pre and after image.
> Only when
> producing textual diff, you would have to run the textual diff
> twice by going over the q twice:
>
> * The first pass would run diff_flush_patch(), which would call
> into xdiff the usual way, but the callback from xdiff would
> capture the removed lines and the added lines without making any
> output.
The callback doesn't have the context around one line as easily
accessible as the xdl emit function, so rather we'd pass in a flag,
i.e. the hashmap where to store the added/removed lines or NULL.
Both passes (the first to find and the second enhanced pass that
outputs the lines) need to have access to the context of the respective
file.
>
> * The second pass would run diff_flush_patch(), but the callback
> from xdiff would be called with additional information, namely,
> the removed and the added lines captured in the first pass.
>
> * I suspect that the fn_out_consume() function that is used for a
> normal case (i.e. when we are not doing this more expensive
> "moved to/moved from" coloring) can be used for the second pass
> above (i.e. the "priv" aka "ecbdata" may need to be extended so
> that it can tell which mode of operation it is asked to perform),
> but if there is not enough similarity between the second pass of
> this "moved from/moved to" mode and the normal mode of output, it
> is also OK to have two different callback functions, i.e. the
> original one to be used in the normal mode, the second one that
> knows the "these are moved without modification" coloring. The
> callback for the first pass is sufficiently different and I think
> it is better to invent a new callback function to be used in the
> first pass, instead of reusing fn_out_consume().
The callback doesn't have easy accessible data IMHO,
e.g. we may get these lines in the callback:
(function context not required:)
---8<---
{
int i;
for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
- unsigned int val;
- /*
- * hex[1]=='\0' is caught when val is checked below,
- * but if hex[0] is NUL we have to avoid reading
- * past the end of the string:
- */
- if (!hex[0])
- return -1;
- val = (hexval(hex[0]) << 4) | hexval(hex[1]);
- if (val & ~0xff)
+ int val = hex2chr(hex);
+ if (val < 0)
return -1;
*sha1++ = val;
hex += 2;
---8<---
And for the first + line (int val = hex2chr(hex);) we need
to hash that line and 2 (made up threshold) before and after:
---8<---
int i;
for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
int val = hex2chr(hex);
if (val < 0)
return -1;
---8<---
and this is hard to reconstruct from the above diff fed into the
callback. So we have to do it in the xdl emit phase.
So I think the wrong part of the initial patch 2/2 is the too
late recording of the moved lines.
So rather whenever we call diff_flush_patch(), we have to first
call diff_prepare_moved_line_detection() first for all file pairs.
>
> The fn_out_consume() function working in the "second pass of
> moved from/moved to mode" would inspect line[] and see if it is
> an added or a removed line, and then:
>
> - if it is an added line, and it appears as a removed line
> elsewhere in the patchset (you obtained the information in the
> first pass), you show it as "this was moved from elsewhere".
>
> - if it is a removed line, and it appears as an added line
> elsewhere in the patchset (you obtained the information in the
> first pass), you show it as "this was moved to elsewhere".
>
> Or something like that.
^ permalink raw reply
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
From: Matthieu Moy @ 2016-09-04 9:19 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, Jeff King, Git,
Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609040952110.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi,
>
> On Fri, 2 Sep 2016, Matthieu Moy wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > Hi Ævar,
>> >
>> > On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> This might be me missing the point, and I'm really just trying to be
>> >> helpful here and make "prove" work for you because it's awesome, but
>> >> as far as just you running this for development purposes does any of
>> >> this SVN stuff matter? I.e. you can build Git itself not with
>> >> Strawberry, but just use Strawberry to get a working copy of "prove".
>> >
>> > Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
>> > take a substantial time to run). So if I run the test suite, I better do
>> > it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
>> > might just as well not bother with running the entire test suite...
>>
>> Maybe something like
>>
>> \path\to\strawberry-perl\perl.exe \path\to\prove ...
>>
>> without changing the PATH would work. I wouldn't call that convenient
>> though.
>
> Wouldn't Perl-specific environment variables set by Strawberry Perl (such
> as PERL_PATH bleed through to the spawned child processes?
>
> We're dancing around the issue, really. Rather than piling workaround on
> workaround with no end in sight, I think it is time to admit that using
> prove(1) on Windows is just not a good solution for the problem to re-run
> failed tests.
I didn't re-add Ævar's disclaimer, but my message was really not
intended to be an objection to your patch, just a (not necessarily good)
idea in case you or someone else on windows wanted to give one more
chance to prove.
I'm all for adding "make failed". Actually, we could even make the
feature more discoverable by echoing "You may run 'make failed' to
re-run failed tests" at the end of the tests when one of them failed.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [RFC/PATCH 0/2] Color moved code differently
From: Jacob Keller @ 2016-09-04 9:57 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git mailing list
In-Reply-To: <20160903033120.20511-1-sbeller@google.com>
On Fri, Sep 2, 2016 at 8:31 PM, Stefan Beller <sbeller@google.com> wrote:
> When moving code (e.g. a function is moved to another part of the file or
> to a different file), the review process is different than reviewing new
> code. When reviewing moved code we are only interested in the diff as
> where there are differences in the moved code, e.g. namespace changes.
>
> However the inner part of these moved texts should not change.
> To aid a developer reviewing such code, we'll color pure moved stuff
> differently.
>
> A line is colored differently if that line and the surroundign 2 lines
> appear as-is in the opposite part of the diff.
>
> Example:
> http://i.imgur.com/ay84q0q.png
>
In the example, the first and last lines of duplicate copies don't get
colored differently, and that threw me off. I feel like that was
maybe not intentional? If it was, can you explain why?
Thanks,
Jake
^ permalink raw reply
* Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently
From: Jakub Narębski @ 2016-09-04 10:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kYEieYGFAgORc8yaF3=8-L1E7K4afNGxDH5AgM5nHFgFw@mail.gmail.com>
W dniu 04.09.2016 o 07:31, Stefan Beller pisze:
> On Sat, Sep 3, 2016 at 5:25 AM, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 03.09.2016 o 05:31, Stefan Beller pisze:
>>
>>> When moving code (e.g. a function is moved to another part of the file or
>>> to a different file), the review process is different than reviewing new
>>> code. When reviewing moved code we are only interested in the diff as
>>> where there are differences in the moved code, e.g. namespace changes.
>>>
>>> However the inner part of these moved texts should not change.
>>> To aid a developer reviewing such code, emit it with a different prefix
>>> than the usual +,- to indicate it is overlapping code.
>>
>> What would be this different prefix?
>
> I will discard the part of the different prefix as the design of 2/2
> will change.
It would be nice to have at least an option of using different prefix
(or pair of prefixes), as not always it is possible to use color to
markup duplicates.
P.S. BTW. does this work with word-diff?
Best regards,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v13 10/14] apply: change error_routine when silent
From: Christian Couder @ 2016-09-04 10:54 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Karsten Blees,
Nguyen Thai Ngoc Duy, Eric Sunshine, Ramsay Jones, Johannes Sixt,
René Scharfe, Stefan Naewe, Christian Couder
In-Reply-To: <CAP8UFD2Lwd_1+cWT702deF8=iFmBRKCi9gLSOizPbyLmeKepsw@mail.gmail.com>
On Thu, Sep 1, 2016 at 10:19 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Sep 1, 2016 at 12:20 AM, Stefan Beller <sbeller@google.com> wrote:
>> On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> To avoid printing anything when applying with
>>> `state->apply_verbosity == verbosity_silent`, let's save the
>>> existing warn and error routines before applying, and let's
>>> replace them with a routine that does nothing.
>>>
>>> Then after applying, let's restore the saved routines.
>>>
>>> Helped-by: Stefan Beller <sbeller@google.com>
>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>>> ---
>>> apply.c | 21 ++++++++++++++++++++-
>>> apply.h | 8 ++++++++
>>> 2 files changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/apply.c b/apply.c
>>> index ddbb0a2..bf81b70 100644
>>> --- a/apply.c
>>> +++ b/apply.c
>>> @@ -112,6 +112,11 @@ void clear_apply_state(struct apply_state *state)
>>> /* &state->fn_table is cleared at the end of apply_patch() */
>>> }
>>>
>>> +static void mute_routine(const char *bla, va_list params)
>>
>> Instead of 'bla' you could go with 'format' as the man page for
>> [f]printf puts it.
>> Or you could leave it empty, i.e.
>>
>> static void mute_routine(const char *, va_list)
>> ...
>
> Ok to do that.
Actually I get the following error when doing that:
apply.c: In function ‘mute_routine’:
apply.c:115:1: error: parameter name omitted
static void mute_routine(const char *, va_list)
^
apply.c:115:1: error: parameter name omitted
make: *** [apply.o] Error 1
So I will leave it as is.
^ permalink raw reply
* Re: [PATCH] stash: allow ref of a stash by index
From: Philip Oakley @ 2016-09-04 10:57 UTC (permalink / raw)
To: Johannes Schindelin, Jeff King
Cc: Aaron M Watson, git, Jon Seymour, David Caldwell,
Øystein Walle, Ævar Arnfjörð Bjarmason,
David Aguilar, Alex Henrie
In-Reply-To: <alpine.DEB.2.20.1609040914200.129229@virtualbox>
From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Hi,
>
> On Sat, 3 Sep 2016, Jeff King wrote:
>
>> On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote:
>>
>> > Allows stashes to be referenced by index only. Instead of referencing
>> > "stash@{n}" explicitly, it can simply be referenced as "n".
>>
>> This says "what" but not "why". I assume it is "because the former is
>> more annoying to type".
>>
>> Are there any backwards-compatibility issues you can think of?
>>
>> I think that "123456" could be a sha1, but I do not see much point in
>> referencing a sha1 as the argument of "stash show". And it looks like
>> this code path is called only from is_stash_like(), so presumably the
>> same logic would apply to other callers.
>
> Maybe we could make it unambiguous, e.g. by using #<n> instead: #123456
> cannot refer to a SHA-1.
The alternative is to limit the length to less that the shortest ambiguous
sha1 length that has been used (by Git users - 5, 6, 7? )? Which is probably
allowing 1-4 characters, which is a reasonbly deep stash index...
If you need to refer to stash@{9362} you have bigger problems.
> But then, '#' are comment-starting in shells, so they would have to by
> escaped. Maybe the best option would be to introduce a -n <n> option,
> with the shortcut -<n> thanks to e0319ff (parseopt: add
> OPT_NUMBER_CALLBACK, 2009-05-07).
>
> Ciao,
> Johannes
>
^ permalink raw reply
* [PATCH 0/4] git add --chmod: always change the file
From: Thomas Gummerer @ 2016-09-04 11:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Jan Keromnes, git, Ingo Brückl, Edward Thomson,
Thomas Gummerer
In-Reply-To: <xmqq4m5zutyc.fsf@gitster.mtv.corp.google.com>
Thanks to Peff and Junio for your inputs on the best way to solve this
problem.
The patch series is made up as follows:
[1/4]: Documentation for the chmod option
[2,3/4]: Small refactoring to simplify the final step
[4/4]: The actual change that introduces the new behaviour.
Thomas Gummerer (4):
add: document the chmod option
update-index: use the same structure for chmod as add
read-cache: introduce chmod_index_entry
add: modify already added files when --chmod is given
Documentation/git-add.txt | 7 +++++-
builtin/add.c | 36 +++++++++++++++++++++----------
builtin/checkout.c | 2 +-
builtin/commit.c | 2 +-
builtin/update-index.c | 55 ++++++++++++++++++-----------------------------
cache.h | 12 ++++++-----
read-cache.c | 33 +++++++++++++++++++++-------
t/t3700-add.sh | 21 ++++++++++++++++++
8 files changed, 107 insertions(+), 61 deletions(-)
--
2.10.0.304.gf2ff484
^ permalink raw reply
* [PATCH 1/4] add: document the chmod option
From: Thomas Gummerer @ 2016-09-04 11:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Jan Keromnes, git, Ingo Brückl, Edward Thomson,
Thomas Gummerer
In-Reply-To: <20160904113954.21697-1-t.gummerer@gmail.com>
The git add --chmod option was introduced in 4e55ed3 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31), but was never
documented. Document the feature.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-add.txt | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 6a96a66..8caa691 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -11,7 +11,7 @@ SYNOPSIS
'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
[--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
- [--] [<pathspec>...]
+ [--chmod=(+|-)x] [--] [<pathspec>...]
DESCRIPTION
-----------
@@ -165,6 +165,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
be ignored, no matter if they are already present in the work
tree or not.
+--chmod=(+|-)::
+ Override the executable bit of the added files. The executable
+ bit is only changed in the index, the files on disk are left
+ unchanged.
+
\--::
This option can be used to separate command-line options from
the list of files, (useful when filenames might be mistaken
--
2.10.0.304.gf2ff484
^ permalink raw reply related
* [PATCH 3/4] read-cache: introduce chmod_index_entry
From: Thomas Gummerer @ 2016-09-04 11:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Jan Keromnes, git, Ingo Brückl, Edward Thomson,
Thomas Gummerer
In-Reply-To: <20160904113954.21697-1-t.gummerer@gmail.com>
As there are chmod options for both add and update-index, introduce a
new chmod_index_entry function to do the work. Use it in update-index,
while it will be used in add in the next patch.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/update-index.c | 8 +-------
cache.h | 2 ++
read-cache.c | 19 +++++++++++++++++++
3 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 85a57db..1569c81 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)
{
int pos;
struct cache_entry *ce;
- unsigned int mode;
char flip = force_mode == 0777 ? '+' : '-';
pos = cache_name_pos(path, strlen(path));
if (pos < 0)
goto fail;
ce = active_cache[pos];
- mode = ce->ce_mode;
- if (!S_ISREG(mode))
+ if (chmod_cache_entry(ce, force_mode) < 0)
goto fail;
- ce->ce_mode = create_ce_mode(force_mode);
- cache_tree_invalidate_path(&the_index, path);
- ce->ce_flags |= CE_UPDATE_IN_BASE;
- active_cache_changed |= CE_ENTRY_CHANGED;
report("chmod %cx '%s'", flip, path);
return;
diff --git a/cache.h b/cache.h
index b780a91..44a4f76 100644
--- a/cache.h
+++ b/cache.h
@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);
#define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))
#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)
#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
#define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
#define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
#define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);
extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);
extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
+extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);
extern int index_name_is_other(const struct index_state *, const char *, int);
diff --git a/read-cache.c b/read-cache.c
index 491e52d..367be57 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,
return ret;
}
+/*
+ * Change the mode of an index entry to force_mode, where force_mode can
+ * either be 0777 or 0666.
+ * Returns -1 if the chmod for the particular cache entry failed (if it's
+ * not a regular file), 0 otherwise.
+ */
+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
+ int force_mode)
+{
+ if (!S_ISREG(ce->ce_mode))
+ return -1;
+ ce->ce_mode = create_ce_mode(force_mode);
+ cache_tree_invalidate_path(istate, ce->name);
+ ce->ce_flags |= CE_UPDATE_IN_BASE;
+ istate->cache_changed |= CE_ENTRY_CHANGED;
+
+ return 0;
+}
+
int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
{
int len = ce_namelen(a);
--
2.10.0.304.gf2ff484
^ permalink raw reply related
* [PATCH 2/4] update-index: use the same structure for chmod as add
From: Thomas Gummerer @ 2016-09-04 11:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Jan Keromnes, git, Ingo Brückl, Edward Thomson,
Thomas Gummerer
In-Reply-To: <20160904113954.21697-1-t.gummerer@gmail.com>
While the chmod options for update-index and the add have the same
functionality, they are using different ways to parse and handle the
option internally. Unify these modes in order to make further
refactoring simpler.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/update-index.c | 49 +++++++++++++++++++++----------------------------
1 file changed, 21 insertions(+), 28 deletions(-)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index ba04b19..85a57db 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
return 0;
}
-static void chmod_path(int flip, const char *path)
+static void chmod_path(int force_mode, const char *path)
{
int pos;
struct cache_entry *ce;
unsigned int mode;
+ char flip = force_mode == 0777 ? '+' : '-';
pos = cache_name_pos(path, strlen(path));
if (pos < 0)
@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)
mode = ce->ce_mode;
if (!S_ISREG(mode))
goto fail;
- switch (flip) {
- case '+':
- ce->ce_mode |= 0111; break;
- case '-':
- ce->ce_mode &= ~0111; break;
- default:
- goto fail;
- }
+ ce->ce_mode = create_ce_mode(force_mode);
cache_tree_invalidate_path(&the_index, path);
ce->ce_flags |= CE_UPDATE_IN_BASE;
active_cache_changed |= CE_ENTRY_CHANGED;
+
report("chmod %cx '%s'", flip, path);
return;
fail:
@@ -788,16 +783,6 @@ static int really_refresh_callback(const struct option *opt,
return refresh(opt->value, REFRESH_REALLY);
}
-static int chmod_callback(const struct option *opt,
- const char *arg, int unset)
-{
- char *flip = opt->value;
- if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
- return error("option 'chmod' expects \"+x\" or \"-x\"");
- *flip = arg[0];
- return 0;
-}
-
static int resolve_undo_clear_callback(const struct option *opt,
const char *arg, int unset)
{
@@ -917,7 +902,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
- char set_executable_bit = 0;
+ char *chmod_arg = 0;
+ int force_mode = 0;
struct refresh_params refresh_args = {0, &has_errors};
int lock_error = 0;
int split_index = -1;
@@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
(parse_opt_cb *) cacheinfo_callback},
- {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
- N_("override the executable bit of the listed files"),
- PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
- chmod_callback},
+ OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
+ N_("override the executable bit of the listed files")),
{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
N_("mark files as \"not changing\""),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
@@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(update_index_usage, options);
+ if (!chmod_arg)
+ force_mode = 0;
+ else if (!strcmp(chmod_arg, "-x"))
+ force_mode = 0666;
+ else if (!strcmp(chmod_arg, "+x"))
+ force_mode = 0777;
+ else
+ die(_("option 'chmod' expects \"+x\" or \"-x\""));
+
git_config(git_default_config, NULL);
/* We can't free this memory, it becomes part of a linked list parsed atexit() */
@@ -1055,8 +1048,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
setup_work_tree();
p = prefix_path(prefix, prefix_length, path);
update_one(p);
- if (set_executable_bit)
- chmod_path(set_executable_bit, p);
+ if (force_mode)
+ chmod_path(force_mode, p);
free(p);
ctx.argc--;
ctx.argv++;
@@ -1100,8 +1093,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
}
p = prefix_path(prefix, prefix_length, buf.buf);
update_one(p);
- if (set_executable_bit)
- chmod_path(set_executable_bit, p);
+ if (force_mode)
+ chmod_path(force_mode, p);
free(p);
}
strbuf_release(&unquoted);
--
2.10.0.304.gf2ff484
^ permalink raw reply related
* [PATCH 4/4] add: modify already added files when --chmod is given
From: Thomas Gummerer @ 2016-09-04 11:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Jan Keromnes, git, Ingo Brückl, Edward Thomson,
Thomas Gummerer
In-Reply-To: <20160904113954.21697-1-t.gummerer@gmail.com>
When the chmod option was added to git add, it was hooked up to the diff
machinery, meaning that it only works when the version in the index
differs from the version on disk.
As the option was supposed to mirror the chmod option in update-index,
which always changes the mode in the index, regardless of the status of
the file, make sure the option behaves the same way in git add.
Reported-by: Jan Keromnes <janx@linux.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/add.c | 36 +++++++++++++++++++++++++-----------
builtin/checkout.c | 2 +-
builtin/commit.c | 2 +-
cache.h | 10 +++++-----
read-cache.c | 14 ++++++--------
t/t3700-add.sh | 21 +++++++++++++++++++++
6 files changed, 59 insertions(+), 26 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index b1dddb4..892198a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;
static int take_worktree_changes;
struct update_callback_data {
- int flags, force_mode;
+ int flags;
int add_errors;
};
+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+
+ if (pathspec && !ce_path_match(ce, pathspec, NULL))
+ continue;
+
+ if (chmod_cache_entry(ce, force_mode) < 0)
+ fprintf(stderr, "cannot chmod '%s'", ce->name);
+ }
+}
+
static int fix_unmerged_status(struct diff_filepair *p,
struct update_callback_data *data)
{
@@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q,
die(_("unexpected diff status %c"), p->status);
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
- if (add_file_to_index(&the_index, path,
- data->flags, data->force_mode)) {
+ if (add_file_to_index(&the_index, path, data->flags)) {
if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
die(_("updating files failed"));
data->add_errors++;
@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,
}
}
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
- int flags, int force_mode)
+int add_files_to_cache(const char *prefix,
+ const struct pathspec *pathspec, int flags)
{
struct update_callback_data data;
struct rev_info rev;
memset(&data, 0, sizeof(data));
data.flags = flags;
- data.force_mode = force_mode;
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
@@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb);
}
-static int add_files(struct dir_struct *dir, int flags, int force_mode)
+static int add_files(struct dir_struct *dir, int flags)
{
int i, exit_status = 0;
@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)
}
for (i = 0; i < dir->nr; i++)
- if (add_file_to_index(&the_index, dir->entries[i]->name,
- flags, force_mode)) {
+ if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {
if (!ignore_add_errors)
die(_("adding files failed"));
exit_status = 1;
@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
plug_bulk_checkin();
- exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
+ exit_status |= add_files_to_cache(prefix, &pathspec, flags);
if (add_new_files)
- exit_status |= add_files(&dir, flags, force_mode);
+ exit_status |= add_files(&dir, flags);
+ if (force_mode)
+ chmod_pathspec(&pathspec, force_mode);
unplug_bulk_checkin();
finish:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..a83c78f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
* entries in the index.
*/
- add_files_to_cache(NULL, NULL, 0, 0);
+ add_files_to_cache(NULL, NULL, 0);
/*
* NEEDSWORK: carrying over local changes
* when branches have different end-of-line
diff --git a/builtin/commit.c b/builtin/commit.c
index 77e3dc8..7a1ade0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -387,7 +387,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
*/
if (all || (also && pathspec.nr)) {
hold_locked_index(&index_lock, 1);
- add_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0);
+ add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
refresh_cache_or_die(refresh_flags);
update_main_cache_tree(WRITE_TREE_SILENT);
if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
diff --git a/cache.h b/cache.h
index 44a4f76..eb6d6b8 100644
--- a/cache.h
+++ b/cache.h
@@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate);
#define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))
#define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))
#define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))
-#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)
-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))
+#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))
#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
#define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
#define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
@@ -582,8 +582,8 @@ extern int remove_file_from_index(struct index_state *, const char *path);
#define ADD_CACHE_IGNORE_ERRORS 4
#define ADD_CACHE_IGNORE_REMOVAL 8
#define ADD_CACHE_INTENT 16
-extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);
-extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);
+extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
+extern int add_file_to_index(struct index_state *, const char *path, int flags);
extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
@@ -1820,7 +1820,7 @@ void packet_trace_identity(const char *prog);
* return 0 if success, 1 - if addition of a file failed and
* ADD_FILES_IGNORE_ERRORS was specified in flags
*/
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
/* diff.c */
extern int diff_auto_refresh_index;
diff --git a/read-cache.c b/read-cache.c
index 367be57..b92d72e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -627,7 +627,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)
hashcpy(ce->sha1, sha1);
}
-int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode)
+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
{
int size, namelen, was_same;
mode_t st_mode = st->st_mode;
@@ -656,11 +656,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
else
ce->ce_flags |= CE_INTENT_TO_ADD;
- if (S_ISREG(st_mode) && force_mode)
- ce->ce_mode = create_ce_mode(force_mode);
- else if (trust_executable_bit && has_symlinks)
+
+ if (trust_executable_bit && has_symlinks) {
ce->ce_mode = create_ce_mode(st_mode);
- else {
+ } else {
/* If there is an existing entry, pick the mode bits and type
* from it, otherwise assume unexecutable regular file.
*/
@@ -719,13 +718,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
return 0;
}
-int add_file_to_index(struct index_state *istate, const char *path,
- int flags, int force_mode)
+int add_file_to_index(struct index_state *istate, const char *path, int flags)
{
struct stat st;
if (lstat(path, &st))
die_errno("unable to stat '%s'", path);
- return add_to_index(istate, path, &st, flags, force_mode);
+ return add_to_index(istate, path, &st, flags);
}
struct cache_entry *make_cache_entry(unsigned int mode,
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 2978cb9..62c7f71 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -349,4 +349,25 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '
test_mode_in_index 100755 foo2
'
+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '
+ echo foo >foo3 &&
+ git add foo3 &&
+ git add --chmod=+x foo3 &&
+ test_mode_in_index 100755 foo3 &&
+ echo foo >xfoo3 &&
+ chmod 755 xfoo3 &&
+ git add xfoo3 &&
+ git add --chmod=-x xfoo3 &&
+ test_mode_in_index 100644 xfoo3
+'
+
+test_expect_success 'file status is changed after git add --chmod=+x' '
+ echo "AM foo4" >expected &&
+ echo foo >foo4 &&
+ git add foo4 &&
+ git add --chmod=+x foo4 &&
+ git status -s foo4 >actual &&
+ test_cmp expected actual
+'
+
test_done
--
2.10.0.304.gf2ff484
^ permalink raw reply related
* Re: Fixup of a fixup not working right
From: Philip Oakley @ 2016-09-04 11:47 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: Robert Dailey, Git
In-Reply-To: <alpine.DEB.2.20.1609040923390.129229@virtualbox>
From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Hi Junio & Philip,
>
> On Fri, 2 Sep 2016, Junio C Hamano wrote:
>
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>
>> > As I understand this it's implied by design. The issue is that the
>> > rebase is looking for that named commit within its current rebase
>> > range, and can't find it, so ignores it.
>> >
>> > There is a separate issue that all the fixup! fixup! messages are
>> > essentially treated as being concatenations of the original fixup!, no
>> > matter how many time the fiup is present.
>>
>> They can be handled separately, but they come from the same "design"
>> that could be improved. When the "original" is not in the range to
>> be rebased for whatever reason (including the most likely one, i.e.
>> it has already graduated to become part of the public history), the
>> best thing the user could do at that point may be, as you suggested
>> to Robert in your message, to turn the "fixup! original" that did
>> not make in time before "original" hit the public record into a
>> standalone "fix original" follow-up change, and then to squash
>> subsequent "fixup! fixup! original" (and other "fixup! original",
>> too) into that commit. And a good direction forward may be to see
>> if "rebase -i" can be taught to be more helpful for the user who
>> wants to do that.
>>
>> Perhaps a change like this to "rebase -i":
>>
>> - The search for "original" when handling "pick fixup! original",
>> when it does not find "original", could turn it into "reword
>> fixup! original" without changing its position in the instruction
>> sequence.
>>
>> - The search for "original" when handling "pick fixup! fixup!
>> original", could be (probably unconditionally) changed to look
>> for "fixup! original" to amend, instead of looking for "original"
>> as the current code (this is your "separate issue"). The same
>> "if the commit to be amended is not found, turn it into reword"
>> rule from the above applies to this one, too.
>>
>> may be an improvement?
>
> I would be *very* careful with such a change.
I agree about the need for care. The use case must be well understood.
> The point is that fixup! messages are really special, and are always
> intended to be squashed into the referenced commit *before* the latter
> hits `master`.
I think it's here that we have the hidden use case. I agree that all fixups
should be squashed before they hit the blessed golden repository.
I suspect that some use cases have intermediate repositories that contain a
'master' branch (it's just a name ;-) that isn't blessed and golden, e.g. at
the team review repo level. In such cases it is possible for a fixup! to be
passed up as part of the review, though it's not the current
norm/expectation.
>
> The entire design of the fixup! feature (using the commit subject as
> identifier, which is only "unique enough" in a topic branch that is still
> being developed) points to that.
>
> I am fairly certain that we would run into tons of problems if we diluted
> the concept of fixup! commits by changing the design so that fixup!
> commits all of a sudden become their own, "real" commits that can be fixed
> up themselves, as much of the current code simply does not expect that.
We already had that. the commit 22c5b13 (rebase -i: handle fixup! fixup!
in --autosquash, 2013-06-27) was an attempt to work around misunderstandings
about what fixed what.
In Robert's scenario (IIUC) that patch was too aggressive for the case where
the original commit is not part of the rebase. The patch lept in a little
too early in the processing so as to pretend that if it saw repeated "fixup!
" strings at the start of the messages it pretented there was only one, so
that they all applied in the original sequence.
I _think_ that the right approach would be to just bring such fixups
together in the to-do list ("as normal"), but still have the minimal common
commit message string still present, so that it would, in this case, still
have a resulting squashed commit that starts "!fixup ". It's important to be
moderately lenient to user choices - they may know something we don't, or at
least accept that their use case is 'unusual' [1]
>
> In short, I am opposed to this change.
It's not like G4W doesn't need fixup!s on the side branches e.g. 5eaffe9
("fixup! Handle new t1501 test case properly with MinGW", 2016-07-12)
> And even if I am overruled, I would strongly suggest to implement this on
> top of my rebase-i-extra branch (i.e. in the rebase--helper instead of the
> shell script) to avoid double churn.
I definitely agree there.
>
> Ciao,
> Johannes
>
--
Philip
[1] The removal of the "theirs" merge strategy is one I'd add to that list.
Calling it, for example, "reversed" would have kept it available while
reduced it's visibility. See http://marc.info/?l=git&m=121637513604413&w=2
^ 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