* (unknown),
@ 2016-04-11 19:04 miwilliams
2016-04-11 19:13 ` your mail Jeff King
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: miwilliams @ 2016-04-11 19:04 UTC (permalink / raw)
To: git
From 7201fe08ede76e502211a781250c9a0b702a78b2 Mon Sep 17 00:00:00 2001
From: Mike Williams <miwilliams@google•com>
Date: Mon, 11 Apr 2016 14:18:39 -0400
Subject: [PATCH 1/1] wt-status: Remove '!!' from
wt_status_collect_changed_cb
The wt_status_collect_changed_cb function uses an extraneous double
negation (!!)
when determining whether or not a submodule has new commits.
Signed-off-by: Mike Williams <miwilliams@google•com>
---
wt-status.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index ef74864..b955179 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -431,7 +431,7 @@ static void wt_status_collect_changed_cb(struct
diff_queue_struct *q,
d->worktree_status = p->status;
d->dirty_submodule = p->two->dirty_submodule;
if (S_ISGITLINK(p->two->mode))
- d->new_submodule_commits = !!hashcmp(p->one->sha1, p->two->sha1);
+ d->new_submodule_commits = hashcmp(p->one->sha1, p->two->sha1);
}
}
--
2.8.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: your mail
2016-04-11 19:04 (unknown), miwilliams
@ 2016-04-11 19:13 ` Jeff King
2016-04-11 19:18 ` none Matthieu Moy
2016-04-12 4:33 ` Stefan Beller
2 siblings, 0 replies; 9+ messages in thread
From: Jeff King @ 2016-04-11 19:13 UTC (permalink / raw)
To: miwilliams; +Cc: git
On Mon, Apr 11, 2016 at 07:04:23PM +0000, miwilliams@google•com wrote:
> From 7201fe08ede76e502211a781250c9a0b702a78b2 Mon Sep 17 00:00:00 2001
> From: Mike Williams <miwilliams@google•com>
> Date: Mon, 11 Apr 2016 14:18:39 -0400
> Subject: [PATCH 1/1] wt-status: Remove '!!' from
> wt_status_collect_changed_cb
These bits (minus the initial "From ..." line) should go into your
actual email headers. As it is, your email has no subject line.
> The wt_status_collect_changed_cb function uses an extraneous double
> negation (!!) when determining whether or not a submodule has new
> commits.
> [...]
> - d->new_submodule_commits = !!hashcmp(p->one->sha1, p->two->sha1);
> + d->new_submodule_commits = hashcmp(p->one->sha1, p->two->sha1);
It's not extraneous. hashcmp() returns 0 for equality, but an arbitrary
positive or negative value depending on how the two arguments differ.
The assigned "new_submodule_commits" is a bitfield of size 1. So the
"!!" is normalizing the value into "0" or "1".
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: none
2016-04-11 19:04 (unknown), miwilliams
2016-04-11 19:13 ` your mail Jeff King
@ 2016-04-11 19:18 ` Matthieu Moy
2016-04-12 4:33 ` Stefan Beller
2 siblings, 0 replies; 9+ messages in thread
From: Matthieu Moy @ 2016-04-11 19:18 UTC (permalink / raw)
To: miwilliams; +Cc: git
miwilliams@google•com writes:
> From 7201fe08ede76e502211a781250c9a0b702a78b2 Mon Sep 17 00:00:00 2001
> From: Mike Williams <miwilliams@google•com>
> Date: Mon, 11 Apr 2016 14:18:39 -0400
> Subject: [PATCH 1/1] wt-status: Remove '!!' from
> wt_status_collect_changed_cb
>
> The wt_status_collect_changed_cb function uses an extraneous double
> negation (!!)
> when determining whether or not a submodule has new commits.
It's not just a double negation, it's a way to ensure that the value is
0 or 1 (it's a relatively common idiom in C at least in Git's codebase).
new_submodule_commits is a 1-bit bitfield, and you don't want to assign
anything other than 1 or 0 (or you'll get modulo 2^n semantics, with
n==1).
So the old code is correct and your patch would introduce a bug.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re:
2016-04-11 19:04 (unknown), miwilliams
2016-04-11 19:13 ` your mail Jeff King
2016-04-11 19:18 ` none Matthieu Moy
@ 2016-04-12 4:33 ` Stefan Beller
2 siblings, 0 replies; 9+ messages in thread
From: Stefan Beller @ 2016-04-12 4:33 UTC (permalink / raw)
To: Mike Williams; +Cc: git@vger•kernel.org
On Mon, Apr 11, 2016 at 12:04 PM, <miwilliams@google•com> wrote:
> From 7201fe08ede76e502211a781250c9a0b702a78b2 Mon Sep 17 00:00:00 2001
> From: Mike Williams <miwilliams@google•com>
> Date: Mon, 11 Apr 2016 14:18:39 -0400
> Subject: [PATCH 1/1] wt-status: Remove '!!' from
> wt_status_collect_changed_cb
>
> The wt_status_collect_changed_cb function uses an extraneous double negation
> (!!)
How is an !! errornous?
It serves the purpose to map an integer value(-1,0,1,2,3,4)
to a boolean (0,1, or a real bit in a bit field).
> when determining whether or not a submodule has new commits.
>
> Signed-off-by: Mike Williams <miwilliams@google•com>
> ---
> wt-status.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ef74864..b955179 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -431,7 +431,7 @@ static void wt_status_collect_changed_cb(struct
> diff_queue_struct *q,
> d->worktree_status = p->status;
> d->dirty_submodule = p->two->dirty_submodule;
> if (S_ISGITLINK(p->two->mode))
> - d->new_submodule_commits = !!hashcmp(p->one->sha1,
> p->two->sha1);
> + d->new_submodule_commits = hashcmp(p->one->sha1,
> p->two->sha1);
> }
> }
>
> --
> 2.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger•kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: none
2018-10-10 15:20 ` Mihir Mehta
@ 2018-10-10 22:19 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2018-10-10 22:19 UTC (permalink / raw)
To: Mihir Mehta; +Cc: git
Mihir Mehta <mihir@cs•utexas.edu> writes:
> Thanks, Junio. Instead of removing that part of the patch, I opted to
> expand it to make it a little clearer (in my opinion) than it was
> before. Let me know if this works.
I am mildly negative on that change. "Omitting both would give an
empty diff" would be understandable to anybody who understands that
an omitted end of dot-dot is substituted with HEAD *and* thinks what
range HEAD..HEAD means, so it is just an additional noise to them,
and to those who do not want to waste time on thinking, it is a
statement that reads as if "it will be an error" without saying why
it is an error. So overall, it seems, at least to me, that the
additional text adds negative value.
So, I dunno.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: none
2019-03-03 13:29 ` Rohit Ashiwal
@ 2019-03-03 13:33 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2019-03-03 13:33 UTC (permalink / raw)
To: Rohit Ashiwal; +Cc: Johannes.Schindelin, christian.couder, git, t.gummerer
Rohit Ashiwal <rohit.ashiwal265@gmail•com> writes:
> Just to be clear of what caused the error:
> 1. Path not being file, or
> 2. File not being empty
> I am checking for both.
test -s <path> makes sure <path> is file; if it is not a file, then
it won't yield true.
So why do you need to say test_path_is_file yourself, if you are
asking "test -s"?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: none
2019-11-20 3:49 Han-Wen Nienhuys
@ 2019-11-20 4:52 ` Junio C Hamano
2019-11-20 5:00 ` none Han-Wen Nienhuys
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2019-11-20 4:52 UTC (permalink / raw)
To: Han-Wen Nienhuys; +Cc: git, Christian Couder, Johannes Schindelin
Han-Wen Nienhuys <hanwen@google•com> writes:
> I spent the last few weeks cobbling together an implementation of the
> reftable format in C and in Go. I thought this would be cool to add to
> git-core, but I doubt whether I will have enough time to see such an
> effort through. Maybe some of you would want to try integrating it
> into the Git-core code base? Example code is here:
>
> https://github.com/google/reftable/blob/master/c/api.h#L153
>
> cheers!
My initial impression was that the API overuses typedef. We tend to
avoid doing
struct _foo { ... };
typedef struct _foo foo;
and instead write "struct foo" explicitly to make us well aware of
what we are talking about. That lets us see that we are passing or
returning a structure by value (which we would like authors to think
thrice before doing in C) like so quite easily:
foo some_function(foo arg1, ...) { ... }
because it would be clear if it were written like so
struct foo some_function(struct foo arg1, ...) { ... }
without hiding the structure behind a typedef (it also lets us avoid
names with leading underscore, which is frowned upon by some people
for different reasons).
But the set of operations defined in the header file seemed at the
right granularity in order to interface with the refs.h & refs/* API
we have. It however was unclear to me how transactional ref updates
would work with it.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: none
2019-11-20 4:52 ` none Junio C Hamano
@ 2019-11-20 5:00 ` Han-Wen Nienhuys
0 siblings, 0 replies; 9+ messages in thread
From: Han-Wen Nienhuys @ 2019-11-20 5:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Johannes Schindelin
On Tue, Nov 19, 2019 at 8:53 PM Junio C Hamano <gitster@pobox•com> wrote:
> My initial impression was that the API overuses typedef. We tend to
> avoid doing
>
> struct _foo { ... };
> typedef struct _foo foo;
>
> and instead write "struct foo" explicitly to make us well aware of
> what we are talking about.
Thanks, i'll have a look at changing it. I use typedef mainly for
ergonomics, but now that the code is written, I can introduce more
verbosity.
> But the set of operations defined in the header file seemed at the
> right granularity in order to interface with the refs.h & refs/* API
> we have. It however was unclear to me how transactional ref updates
> would work with it.
Transactions have to interface with the file system. I imagine that
different libraries (libgit vs. cgit) would have different primitives
for dealing with the file system, hence I haven't implemented that
part. Do you have an idea of how I could implement it in a way that is
agnostic of libgit2 vs. cgit?
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: none
2023-10-16 18:43 Dorcas Litunya
@ 2023-10-17 20:21 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2023-10-17 20:21 UTC (permalink / raw)
To: Dorcas Litunya; +Cc: christian.couder, git
Dorcas Litunya <anonolitunya@gmail•com> writes:
> Bcc:
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> Reply-To:
> In-Reply-To: <xmqq1qdumrto.fsf@gitster•g>
What are these lines doing here?
> So should I replace this in the next version or leave this as is?
Perhaps I was not clear enough, but I found the commit title and
description need to be updated to clearly record the intent of the
change with a handful of points, so I will not be accepting the
patch as-is.
These two sections may be of help.
Documentation/MyFirstContribution.txt::now-what
Documentation/MyFirstContribution.txt::reviewing
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-17 20:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-11 19:04 (unknown), miwilliams
2016-04-11 19:13 ` your mail Jeff King
2016-04-11 19:18 ` none Matthieu Moy
2016-04-12 4:33 ` Stefan Beller
-- strict thread matches above, loose matches on Subject: below --
2018-10-05 6:20 [PATCH] doc: fix a typo and clarify a sentence Junio C Hamano
2018-10-10 15:20 ` Mihir Mehta
2018-10-10 22:19 ` none Junio C Hamano
2019-03-03 13:20 [PATCH 1/3] test functions: Add new function `test_file_not_empty` Junio C Hamano
2019-03-03 13:29 ` Rohit Ashiwal
2019-03-03 13:33 ` none Junio C Hamano
2019-11-20 3:49 Han-Wen Nienhuys
2019-11-20 4:52 ` none Junio C Hamano
2019-11-20 5:00 ` none Han-Wen Nienhuys
2023-10-16 18:43 Dorcas Litunya
2023-10-17 20:21 ` none Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox