Re: [PATCH v3] push: Provide situational hints for non-fast-forward errors
- From
Jeff King <peff@peff.net>
- Date
- Mar 23, 2012, 21:41 UTC
- Message-ID
- <20120323214114.GB18198@sigill.intra.peff.net>
- In-Reply-To
- <20120320043133.GA2755@gmail.com>
On Tue, Mar 20, 2012 at 12:31:33AM -0400, Christopher Tiwald wrote:
Show 7 quoted lines
> Changes since v2: > - Cleaned up commit message language, specifically in scenario > one. > - Created one config variable per piece of non-ff push advice. > Additionally, preserved 'pushNonFastForward' as a means of > disabling all non-ff push advice. Users who set this > config option should see no change to 'git push'.
This version looks pretty good to me, but I have one question:
Show 19 quoted lines
> @@ -1008,7 +1009,6 @@ struct ref {
> char *symref;
> unsigned int force:1,
> merge:1,
> - nonfastforward:1,
> deletion:1;
> enum {
> REF_STATUS_NONE = 0,
> @@ -1019,6 +1019,10 @@ struct ref {
> REF_STATUS_REMOTE_REJECT,
> REF_STATUS_EXPECTING_REPORT
> } status;
> + enum {
> + NON_FF_HEAD = 1,
> + NON_FF_OTHER
> + } nonfastforward;
> char *remote_status;
> struct ref *peer_ref; /* when renaming */
> char name[FLEX_ARRAY]; /* more */Why is this enum stored inside the ref? It doesn't actually know whether it is a HEAD or not, and we never actually store that value there. We always just store a boolean (remote.c, ll. 1294-1298) and access it as one (remote.c, l. 1300; transport.c, l. 1259).
The only time we use the enum values is via the "int nonfastforward" passed to transport_push. I think it would be a lot clearer to leave nonfastforward as a single bit in the ref, and then define the enum elsewhere (or even just use #define if we are not going to use the enum type). Like this on top of your patch:
diff --git a/cache.h b/cache.h index 427b600..35f3075 100644 --- a/cache.h +++ b/cache.h @@ -1009,6 +1009,7 @@ struct ref { char *symref; unsigned int force:1, merge:1, + nonfastforward:1, deletion:1; enum { REF_STATUS_NONE = 0, @@ -1019,10 +1020,6 @@ struct ref { REF_STATUS_REMOTE_REJECT, REF_STATUS_EXPECTING_REPORT } status; - enum { - NON_FF_HEAD = 1, - NON_FF_OTHER - } nonfastforward; char *remote_status; struct ref *peer_ref; /* when renaming */ char name[FLEX_ARRAY]; /* more */ diff --git a/transport.h b/transport.h index ce99ef8..1631a35 100644 --- a/transport.h +++ b/transport.h @@ -138,6 +138,8 @@ int transport_set_option(struct transport *transport, const char *name, void transport_set_verbosity(struct transport *transport, int verbosity, int force_progress); +#define NON_FF_HEAD 1 +#define NON_FF_OTHER 2 int transport_push(struct transport *connection, int refspec_nr, const char **refspec, int flags, int * nonfastforward); I don't think your patch is buggy, because the enum is perfectly capable of being used as a single bit. But it's confusing to read, because ref->nonfastforward will never actually be set to the NON_FF_OTHER enum value. -Peff